From ae5c25d55bf96a0b7de78d2141f33e2ab0f6bfac Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Mon, 16 Dec 2024 17:10:22 +0000 Subject: [PATCH 01/27] style: Renamed the `handle_error` method to `handle_exceptions` --- app/controllers/exceptions_controller.rb | 2 +- config/initializers/exceptions.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/exceptions_controller.rb b/app/controllers/exceptions_controller.rb index cba7da59..e0b9fc75 100644 --- a/app/controllers/exceptions_controller.rb +++ b/app/controllers/exceptions_controller.rb @@ -4,7 +4,7 @@ class ExceptionsController < ApplicationController layout 'application' - def render_error + def handle_exceptions env = request.env exception = env['action_dispatch.exception'] status_code = ActionDispatch::ExceptionWrapper.new(env, exception).status_code diff --git a/config/initializers/exceptions.rb b/config/initializers/exceptions.rb index b0de5388..48551584 100644 --- a/config/initializers/exceptions.rb +++ b/config/initializers/exceptions.rb @@ -3,5 +3,5 @@ # Custom error handling via a Rack middleware action Rails.application.config.exceptions_app = lambda do |env| - ExceptionsController.action(:render_error).call(env) + ExceptionsController.action(:handle_exceptions).call(env) end From 2e6b96cb8722beee65a02cb800fc18b9bb6c82ca Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Mon, 16 Dec 2024 17:11:03 +0000 Subject: [PATCH 02/27] fix: set any unmatched route to be redirected to handle_exception method --- config/routes.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/config/routes.rb b/config/routes.rb index adf861e7..8a39efa3 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -15,4 +15,6 @@ get 'doc/ukhpi', to: 'doc#ukhpi', as: 'ukhpi_doc' get 'doc/ukhpi-dsd', to: 'doc#ukhpi_dsd' get 'doc/ukhpi-user-guide', to: 'doc#ukhpi_user_guide' + + get '*unmatched_route', to: 'exceptions#handle_exceptions' end From e71919ebb4fe757586dc29143a03550f278aebd2 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 17 Dec 2024 12:27:55 +0000 Subject: [PATCH 03/27] fix: undefined method `user_selections' for an instance of Hash checks for user_selections or passes in new, empty, user_selections to ensure the key is present --- app/controllers/browse_controller.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/controllers/browse_controller.rb b/app/controllers/browse_controller.rb index 8b7f2a3e..d40408b7 100644 --- a/app/controllers/browse_controller.rb +++ b/app/controllers/browse_controller.rb @@ -60,7 +60,8 @@ def render_view_state(view_state) @view_state = view_state if view_state.respond_to?(:[]) && view_state[:error] Rails.logger.debug { "Application experienced an issue with this request: #{view_state}" } if Rails.env.development? # rubocop:disable Metrics/LineLength - render_request_error(@view_state.user_selections, :internal_server_error) + user_selections = view_state.respond_to?(:user_selections) ? view_state.user_selections : UserSelections.new + render_request_error(user_selections, :internal_server_error) else respond_to do |format| format.html From 5d8b26f23b05e178467760b2d797ae0e4dfbc967 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 17 Dec 2024 12:29:08 +0000 Subject: [PATCH 04/27] style: updated comments primarily disabling rubocop linting warnings but also adjusted a debugging phrase --- app/controllers/browse_controller.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/app/controllers/browse_controller.rb b/app/controllers/browse_controller.rb index d40408b7..eab20250 100644 --- a/app/controllers/browse_controller.rb +++ b/app/controllers/browse_controller.rb @@ -3,7 +3,7 @@ # Controller for the main user experience of browsing the UKHPI statistics. # Usually the primary interaction will be via JavaScript and XHR, but we also # support non-JS access by setting browse preferences in the `edit` action. -class BrowseController < ApplicationController +class BrowseController < ApplicationController # rubocop:disable Metrics/ClassLength layout 'webpack_application' def show @@ -56,10 +56,11 @@ def setup_view_state(user_selections) { user_selections: user_selections, error: e.message } end + # rubocop:disable Metrics/MethodLength, Metrics/LineLength def render_view_state(view_state) @view_state = view_state if view_state.respond_to?(:[]) && view_state[:error] - Rails.logger.debug { "Application experienced an issue with this request: #{view_state}" } if Rails.env.development? # rubocop:disable Metrics/LineLength + Rails.logger.debug { "UKHPI experienced an issue with this request: #{view_state}" } if Rails.env.development? user_selections = view_state.respond_to?(:user_selections) ? view_state.user_selections : UserSelections.new render_request_error(user_selections, :internal_server_error) else @@ -69,6 +70,7 @@ def render_view_state(view_state) end end end + # rubocop:enable Metrics/MethodLength, Metrics/LineLength # Look at the `action` parameter, which may be set by various action buttons # on the form, to determine whether we need to do any processing before @@ -108,7 +110,7 @@ def match_location(view_state) end def match_no_locations - flash[:search] = 'Sorry, no locations match that search term' # rubocop:disable Rails/I18nLocaleTexts + flash.now[:search] = 'Sorry, no locations match that search term' # rubocop:disable Rails/I18nLocaleTexts end def match_single_location(view_state, locations) @@ -129,7 +131,7 @@ def view_result(view_state) }.merge(new_params)) end - def render_request_error(user_selections, status) + def render_request_error(user_selections, status) # rubocop:disable Metrics/MethodLength respond_to do |format| @view_state = { user_selections: user_selections } request_status = status == 400 ? :bad_request : :internal_server_error From b58b04275b8db9d3edd2bf96fb307e0935498c37 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 17 Dec 2024 12:29:37 +0000 Subject: [PATCH 05/27] style: disabled rubocop linting warning --- app/models/data_view.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/data_view.rb b/app/models/data_view.rb index f2844e46..52e24037 100644 --- a/app/models/data_view.rb +++ b/app/models/data_view.rb @@ -7,7 +7,7 @@ # a view of the `averagePrice` indicator, together with the relevant dates, # location and other options, and access to the underlying data, to enable # the renderer to draw the display -class DataView +class DataView # rubocop:disable Metrics/ClassLength include Rails.application.routes.url_helpers attr_reader :user_selections, :query_result, :indicator, :theme From 9e7afac5660838d4f23137d4c9517101c8a083e7 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 17 Dec 2024 12:31:21 +0000 Subject: [PATCH 06/27] fix: resolved duplicate element warnings on statistic view controls by assigning unique names and id's to the checkboxes for each row --- app/views/browse/_data_view.html.haml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/views/browse/_data_view.html.haml b/app/views/browse/_data_view.html.haml index 8b8ffda4..3450a198 100644 --- a/app/views/browse/_data_view.html.haml +++ b/app/views/browse/_data_view.html.haml @@ -26,9 +26,10 @@ %br %span.o-data-view__options-statistics - data_view.each_statistic do |stat| + - x = x ? x + 1 : 0 %label - checked = data_view.statistic_selected?(stat) - %input{ type: 'checkbox', value: stat.slug, checked: checked } + %input{ id: "checkbox-#{node_id}-#{stat.slug}-#{x}", name: "checkbox-#{node_id}-#{stat.slug}", type: 'checkbox', value: stat.slug, checked: checked } %a{ href: data_view.add_remove_statistic(!checked, stat) } = stat.label From cf8499803f87d1feca0c9277d5a74883d4934c10 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Wed, 18 Dec 2024 14:51:10 +0000 Subject: [PATCH 07/27] fix: check for slug before comparing to seeing if it's value is `vol` --- app/models/ukhpi_indicator.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/ukhpi_indicator.rb b/app/models/ukhpi_indicator.rb index 72560cbf..02864d86 100644 --- a/app/models/ukhpi_indicator.rb +++ b/app/models/ukhpi_indicator.rb @@ -17,7 +17,7 @@ def initialize(slug, root_name, label_key) # @return True if this indicator denotes sales volume def volume? - slug == 'vol' + slug && slug == 'vol' end # @return The label for this indicator From ba6a7ac6d230bdef80c42314a00e06b44d242f55 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Wed, 18 Dec 2024 15:41:38 +0000 Subject: [PATCH 08/27] Fix: casting long strings to `Date`, `Time` or `DateTime` `ActiveModel::Type::Date` raises `ArgumentError` for strings longer than 128 characters; this rescues and logs if long string is attempted --- app/models/concerns/user_choices.rb | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/app/models/concerns/user_choices.rb b/app/models/concerns/user_choices.rb index 7130e764..b9fcf301 100644 --- a/app/models/concerns/user_choices.rb +++ b/app/models/concerns/user_choices.rb @@ -54,15 +54,23 @@ def alternative_key(key) end # Recognise a date. Accepts ISO-8601 strings or Date objects. - # Dates that match YYYY-MM will be transformed to YYYY-MM-01. # @param date Either an ISO0-8601 date string, or a date object def parse_date(date) if date.is_a?(Date) date else - date_str = date.to_s.match?(/\d\d\d\d-(1[012]|0[1-9])/) ? "#{date}-01" : date.to_s + # We need to truncate the date to at most the final 10 characters expected + # to avoid potential errors from maliciously long strings. + potential_date = date.to_s.first(10) + # strings that match YYYY-MM will be transformed to YYYY-MM-01 (i.e. 10 chars) + date_str = potential_date.match?(/\d\d\d\d-(1[012]|0[1-9])/) ? "#{potential_date}-01" : potential_date + # We can now parse the date string and fail if it is not a valid date. Date.parse(date_str) end + rescue ArgumentError => e + # We use truncate here to show the original date value in the logs with an + # ellipsis but with a maximum length of 128 characters to clarify the error. + Rails.logger.error("Failed to parse date '#{date.to_s.truncate(128)}': #{e.message || e}") end def array_valued?(param) From a26438281e517ac29e4e66fa45f3fa9fcf30b1e3 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Fri, 20 Dec 2024 13:00:01 +0000 Subject: [PATCH 09/27] style: disable line length warning from rubocop --- app/models/concerns/user_choices.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/concerns/user_choices.rb b/app/models/concerns/user_choices.rb index b9fcf301..4bbdc3cf 100644 --- a/app/models/concerns/user_choices.rb +++ b/app/models/concerns/user_choices.rb @@ -63,7 +63,7 @@ def parse_date(date) # to avoid potential errors from maliciously long strings. potential_date = date.to_s.first(10) # strings that match YYYY-MM will be transformed to YYYY-MM-01 (i.e. 10 chars) - date_str = potential_date.match?(/\d\d\d\d-(1[012]|0[1-9])/) ? "#{potential_date}-01" : potential_date + date_str = potential_date.match?(/\d\d\d\d-(1[012]|0[1-9])/) ? "#{potential_date}-01" : potential_date # rubocop:disable Layout/LineLength # We can now parse the date string and fail if it is not a valid date. Date.parse(date_str) end From 5280f14ca019589c85dad78532f74bc5c2de5111 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Fri, 20 Dec 2024 13:01:20 +0000 Subject: [PATCH 10/27] build: adds `meta_request` gem Allows debugging via Chrome DevTools while in development --- Gemfile | 3 ++- Gemfile.lock | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/Gemfile b/Gemfile index 267fdf37..f678e3ba 100644 --- a/Gemfile +++ b/Gemfile @@ -51,7 +51,8 @@ end group :development do # Access an IRB console on exception pages or by using <%= console %> in views gem 'web-console' - + # Rails Panel is a Chrome extension for Rails development that will end your tailing of development.log. + gem 'meta_request' # Spring speeds up development by keeping your application running in the background. Read more: https://github.com/rails/spring gem 'spring' end diff --git a/Gemfile.lock b/Gemfile.lock index 2ea53fd2..554d0e3b 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -210,6 +210,9 @@ GEM net-smtp marcel (1.0.4) matrix (0.4.2) + meta_request (0.8.5) + rack-contrib (>= 1.1, < 3) + railties (>= 3.0.0, < 9) method_source (1.1.0) mini_mime (1.1.5) minitest (5.25.1) @@ -266,6 +269,8 @@ GEM puma (>= 6.0) racc (1.8.1) rack (3.1.8) + rack-contrib (2.5.0) + rack (< 4) rack-proxy (0.7.7) rack rack-session (2.0.0) @@ -475,6 +480,7 @@ DEPENDENCIES json_expressions json_rails_logger! m + meta_request minitest-rails minitest-reporters mocha From 007b1444cec9a92053d6466622c461e75c7f34ed Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Fri, 20 Dec 2024 13:02:13 +0000 Subject: [PATCH 11/27] build: removed assets and start targets from server target in makefile with intention being to manually include those targets when required --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 3934ec9a..f4cbec73 100644 --- a/Makefile +++ b/Makefile @@ -89,7 +89,7 @@ run: start @if docker network inspect dnet > /dev/null 2>&1; then echo "Using docker network dnet"; else echo "Create docker network dnet"; docker network create dnet; sleep 2; fi @docker run -p ${PORT}:3000 -e API_SERVICE_URL=${API_SERVICE_URL} --network dnet --rm --name ${SHORTNAME} ${NAME}:${TAG} -server: assets start +server: @export SECRET_KEY_BASE=$(./bin/rails secret) @API_SERVICE_URL=${API_SERVICE_URL} ./bin/rails server -p ${PORT} From 7d8290a440e98a0409dcb00905ead4051a58aa28 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Fri, 20 Dec 2024 13:03:01 +0000 Subject: [PATCH 12/27] docs: updated CHANGELOG with unreleased changes also includes a minor layout adjustment --- CHANGELOG.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b5bc2e8..749a16e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changes to the UKHPI app by version and date +## Unreleased + +- (Jon) Fix for casting long strings to `Date`, `Time` or `DateTime` + ## 2.0.1 - 2024-12 - (Jon) Improves error metrics reporting to ensure that logging always happens @@ -23,7 +27,8 @@ ## 1.8.0 - 2024-10 -- (Dan) Updates ruby version to 2.7.8 and alpine version to 3.16 [GH-455](https://github.com/epimorphics/ukhpi/issues/455) +- (Dan) Updates ruby version to 2.7.8 and alpine version to 3.16 + [GH-455](https://github.com/epimorphics/ukhpi/issues/455) ## 1.7.5 - 2024-09 From b6f895173ac8534c0b20770c2023dae2478548ea Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Fri, 20 Dec 2024 13:03:37 +0000 Subject: [PATCH 13/27] style: disabled rubocop warnings on test fixtures --- test/models/ukhpi_indicator_test.rb | 2 +- test/models/ukhpi_statistic_test.rb | 2 +- test/models/ukhpi_theme_test.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test/models/ukhpi_indicator_test.rb b/test/models/ukhpi_indicator_test.rb index 5f26a7da..27bb2cec 100644 --- a/test/models/ukhpi_indicator_test.rb +++ b/test/models/ukhpi_indicator_test.rb @@ -4,7 +4,7 @@ # Unit tests on the UkhpiIndicator class class UkhpiIndicatorTest < ActiveSupport::TestCase - describe 'UkhpiIndicator' do + describe 'UkhpiIndicator' do # rubocop:disable Metrics/BlockLength describe '#initialize' do it 'should provide access to the initialised values' do ind = UkhpiIndicator.new('foo', 'foo_root', 'average_price') diff --git a/test/models/ukhpi_statistic_test.rb b/test/models/ukhpi_statistic_test.rb index 1d8c3201..6b7593b7 100644 --- a/test/models/ukhpi_statistic_test.rb +++ b/test/models/ukhpi_statistic_test.rb @@ -4,7 +4,7 @@ # Unit tests on the UkhpiStatistic class class UkhpiStatisticTest < ActiveSupport::TestCase - describe 'UkhpiStatistic' do + describe 'UkhpiStatistic' do # rubocop:disable Metrics/BlockLength describe '#initialize' do it 'should provide accessors to initialization state' do stat = UkhpiStatistic.new('foo', 'foo_r', 'all_property_types', true) diff --git a/test/models/ukhpi_theme_test.rb b/test/models/ukhpi_theme_test.rb index 7ee869a3..b4d791ba 100644 --- a/test/models/ukhpi_theme_test.rb +++ b/test/models/ukhpi_theme_test.rb @@ -4,7 +4,7 @@ # Unit tests on the UkhpiTheme class class UkhpiThemeTest < ActiveSupport::TestCase - describe 'UkhpiTheme' do + describe 'UkhpiTheme' do # rubocop:disable Metrics/BlockLength describe '#initialize' do it 'should provide accessors to initialization state' do stat = stub From d4999fa638d99c78b18bc0f0c32e17be871cbdd9 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Mon, 6 Jan 2025 10:22:56 +0000 Subject: [PATCH 14/27] refactor: updated CHANGELOG to remove rolled back release header --- CHANGELOG.md | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 749a16e6..f8eca75e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,10 +2,8 @@ ## Unreleased -- (Jon) Fix for casting long strings to `Date`, `Time` or `DateTime` - -## 2.0.1 - 2024-12 - +- (Jon) Fix for casting long strings to `Date`, `Time` or `DateTime` in Ruby + 3.1.0 - (Jon) Improves error metrics reporting to ensure that logging always happens with the appropriate severity depending on the exception status while reducing the types of errors that can trigger a an error metric and therefore a From b6f638a57d4e010f13c6fb3ce3593431167154d4 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Mon, 6 Jan 2025 12:15:48 +0000 Subject: [PATCH 15/27] fix: remove exceptions controller and intializer to ensure the appropriate error message is handled correctly by the application --- app/controllers/exceptions_controller.rb | 31 ------------------------ config/initializers/exceptions.rb | 7 ------ 2 files changed, 38 deletions(-) delete mode 100644 app/controllers/exceptions_controller.rb delete mode 100644 config/initializers/exceptions.rb diff --git a/app/controllers/exceptions_controller.rb b/app/controllers/exceptions_controller.rb deleted file mode 100644 index 11c20945..00000000 --- a/app/controllers/exceptions_controller.rb +++ /dev/null @@ -1,31 +0,0 @@ -# frozen_string_literal: true - -# Controller to handle dynamically displaying error messages -class ExceptionsController < ApplicationController - layout 'application' - - def render_error - env = request.env - exception = env['action_dispatch.exception'] - status_code = ActionDispatch::ExceptionWrapper.new(env, exception).status_code - - sentry_code = maybe_report_to_sentry(exception, status_code) - # add the exception to the prometheus metrics - instrument_internal_error(exception) - - render :error_page, - locals: { status: status_code, sentry_code: sentry_code }, - layout: true, - status: status_code - end - - private - - def maybe_report_to_sentry(exception, status_code) - return nil if Rails.env.development? # Why are we reporting to Senty in dev? - return nil unless status_code >= 500 - - sevent = Sentry.capture_exception(exception) - sevent&.event_id - end -end diff --git a/config/initializers/exceptions.rb b/config/initializers/exceptions.rb deleted file mode 100644 index b0de5388..00000000 --- a/config/initializers/exceptions.rb +++ /dev/null @@ -1,7 +0,0 @@ -# frozen_string_literal: true - -# Custom error handling via a Rack middleware action -Rails.application.config.exceptions_app = - lambda do |env| - ExceptionsController.action(:render_error).call(env) - end From e9c62ed37b46b5d7a1d6b11928f26e0a1b3bf3bd Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Mon, 6 Jan 2025 12:17:41 +0000 Subject: [PATCH 16/27] refactor: implement updated `instrument_internal_error` method Notify subscriber(s) of an internal error event with the payload of the exception once done but only if error status is over 500 --- app/controllers/application_controller.rb | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 34285f77..c45ab2cd 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -74,7 +74,26 @@ def detailed_request_log(duration) # @param [Exception] exp the exception that caused the error # @return [ActiveSupport::Notifications::Event] provides an object-oriented # interface to the event - def instrument_internal_error(exception) - ActiveSupport::Notifications.instrument('internal_error.application', exception: exception) + # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity, Metrics/MethodLength + def instrument_internal_error(exc, status = nil) + err = { + message: exc&.message || exc, + status: Rack::Utils::SYMBOL_TO_STATUS_CODE[exc] + } + err[:status] = status if status + err[:type] = exc.class&.name if exc&.class + err[:cause] = exc&.cause if exc&.cause + err[:backtrace] = exc&.backtrace if exc&.backtrace && Rails.env.development? + # Log the exception to the Rails logger with the appropriate severity + Rails.logger.send(err[:status] < 500 ? :warn : :error, JSON.generate(err)) + # Return unless the status code is 500 or greater to ensure subscribers are NOT notified + return nil unless err[:status] >= 500 + + sevent = Sentry.capture_exception(exc) unless Rails.env.development? + # Instrument the internal error event to notify subscribers of the error + ActiveSupport::Notifications.instrument('internal_error.application', exception: err) + # Return the event id for the internal error event + sevent&.event_id end + # rubocop:enable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity, Metrics/MethodLength end From dffe863fc0fd3741f50e26cec9eb411c3b2ea2e0 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Mon, 6 Jan 2025 12:22:53 +0000 Subject: [PATCH 17/27] refactor: ensure correct error response is presented also lists any errors based on current query as well as improves the contact information presentation --- app/views/exceptions/error_page.html.haml | 35 ++++++++++++----------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/app/views/exceptions/error_page.html.haml b/app/views/exceptions/error_page.html.haml index e27f6e64..6933d242 100644 --- a/app/views/exceptions/error_page.html.haml +++ b/app/views/exceptions/error_page.html.haml @@ -1,3 +1,6 @@ +- puts "Rendering error page with status: #{status.inspect}" if Rails.env.development? +- puts "View state: #{@view_state.inspect}" if Rails.env.development? + %article.u-clear - if status == 400 %h1.o-heading--1 Request not understood @@ -7,16 +10,16 @@ message as a result of using the HPI or PPD applications, please let us know so that we can correct the problem. - - if @view_state.user_selections&.errors + - if @view_state && @view_state[:user_selections].errors.any? %p The following issues were identified in your request: %ul.list.list-bullet - - @view_state.user_selections.errors.each do |err_msg| + - @view_state[:user_selections].errors.each do |err_msg| %li = err_msg - - elsif status == 404 + - if status == 404 %h1.o-heading--1 Page not found %p @@ -24,7 +27,7 @@ Please check the spelling of the page address (URL). If you require further assistance, please see the contact details below. - - else + - if status == 500 %h1.o-heading--1 Application error %p @@ -42,16 +45,14 @@ = sentry_code %h2.o-heading--2 Who to contact - %p - If you are unable to access the data, please - %a{ href: 'http://site.landregistry.gov.uk/contact-us/form' } - fill in our contact form. - - %p - For general transaction data enquiries, email - %a{ href: 'mailto:DRO@landregistry.gov.uk' } - DRO@landregistry.gov.uk - - %p - For general price paid data enquiries, contact - = mail_to(Rails.application.config.contact_email_address) + %ul.list.list-bullet + %li + If you are unable to access the data, please + %a{ href: '//site.landregistry.gov.uk/contact-us/form' } + fill in our contact form. + %li + For general transaction data enquiries, email + = mail_to('DRO@landregistry.gov.uk') + %li + For general price paid data enquiries, contact + = mail_to(Rails.application.config.contact_email_address) From 97451b9135c023eea673f029725e3f190fa97adc Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Mon, 6 Jan 2025 12:23:42 +0000 Subject: [PATCH 18/27] fix: better error control Handle specific types of exceptions and render the appropriate error page or attempt to render a generic error page if no specific error page exists --- app/controllers/application_controller.rb | 72 +++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index c45ab2cd..e721666b 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -2,6 +2,8 @@ # :nodoc: class ApplicationController < ActionController::Base + include Rails.application.routes.url_helpers + include ActionView::Helpers::TranslationHelper # Prevent CSRF attacks by raising an exception. # For APIs, you may want to use :null_session instead. protect_from_forgery with: :exception @@ -37,6 +39,76 @@ def log_request_result detailed_request_log(duration) end + # Handle specific types of exceptions and render the appropriate error page + # or attempt to render a generic error page if no specific error page exists + unless Rails.application.config.consider_all_requests_local + rescue_from StandardError do |e| + # Trigger the appropriate error handling method based on the exception + case e.class + when ActionController::RoutingError, ActionView::MissingTemplate + :render404 + when ActionController::InvalidCrossOriginRequest + :render403 + when ActionController::BadRequest, ActionController::ParameterMissing + :render400 + else + :handle_internal_error + end + end + end + + # Render the appropriate error page based on the exception + def handle_internal_error(exception) + if exception.instance_of? ArgumentError + render_error(400) + else + Rails.logger.warn "No explicit error page for exception #{exception} - #{exception.class}" + # Instrument ActiveSupport::Notifications for internal server errors only: + sentry_code = instrument_internal_error(exception) + render_error(500, sentry_code) + end + end + + def render_400(_exception = nil) # rubocop:disable Naming/VariableNumber + render_error(400) + end + + def render_403(_exception = nil) # rubocop:disable Naming/VariableNumber + render_error(403) + end + + def render_404(_exception = nil) # rubocop:disable Naming/VariableNumber + render_error(404) + end + + def render_500(_exception = nil) # rubocop:disable Naming/VariableNumber + render_error(500) + end + + def render_error(status, sentry_code = nil) # rubocop:disable Metrics/AbcSize + reset_response + + status = Rack::Utils::SYMBOL_TO_STATUS_CODE[status] if status.is_a?(Symbol) + @view_state ||= LandingState.new(UserLanguageSelection.new(params)) + + respond_to do |format| + format.html { render_html_error_page(status, sentry_code) } + # Anything else returns the status as human readable plain string + format.all { render plain: Rack::Utils::HTTP_STATUS_CODES[status].to_s, status: status } + end + end + + def render_html_error_page(status, sentry_code) # rubocop:disable Metrics/MethodLength + render 'exceptions/error_page', + locals: { status: status, sentry_code: sentry_code }, + layout: true, + status: status + end + + def reset_response + self.response_body = nil + end + private # rubocop:disable Metrics/AbcSize From 70f98c68538faeb688a975cb6d73e417703584fb Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Mon, 6 Jan 2025 12:29:31 +0000 Subject: [PATCH 19/27] refactor: updated `render_request_error` Converts status code to integer if it is a symbol alongside passing the status_code value to ensure the proper error message is displayed --- app/controllers/browse_controller.rb | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/app/controllers/browse_controller.rb b/app/controllers/browse_controller.rb index eab20250..f3ff76a2 100644 --- a/app/controllers/browse_controller.rb +++ b/app/controllers/browse_controller.rb @@ -10,7 +10,7 @@ def show user_selections = UserSelections.new(params) if !user_selections.valid? - render_request_error(user_selections, 400) + render_request_error(user_selections, :bad_request) elsif explain_non_json?(user_selections) redirect_to_html_view(user_selections) else @@ -56,13 +56,11 @@ def setup_view_state(user_selections) { user_selections: user_selections, error: e.message } end - # rubocop:disable Metrics/MethodLength, Metrics/LineLength def render_view_state(view_state) @view_state = view_state if view_state.respond_to?(:[]) && view_state[:error] - Rails.logger.debug { "UKHPI experienced an issue with this request: #{view_state}" } if Rails.env.development? - user_selections = view_state.respond_to?(:user_selections) ? view_state.user_selections : UserSelections.new - render_request_error(user_selections, :internal_server_error) + Rails.logger.debug { "Application experienced an issue with this request: #{view_state}" } if Rails.env.development? # rubocop:disable Metrics/LineLength + render_request_error(@view_state.user_selections, :internal_server_error) else respond_to do |format| format.html @@ -70,7 +68,6 @@ def render_view_state(view_state) end end end - # rubocop:enable Metrics/MethodLength, Metrics/LineLength # Look at the `action` parameter, which may be set by various action buttons # on the form, to determine whether we need to do any processing before @@ -110,7 +107,7 @@ def match_location(view_state) end def match_no_locations - flash.now[:search] = 'Sorry, no locations match that search term' # rubocop:disable Rails/I18nLocaleTexts + flash[:search] = 'Sorry, no locations match that search term' # rubocop:disable Rails/I18nLocaleTexts end def match_single_location(view_state, locations) @@ -131,19 +128,20 @@ def view_result(view_state) }.merge(new_params)) end - def render_request_error(user_selections, status) # rubocop:disable Metrics/MethodLength + def render_request_error(user_selections, status_code) # rubocop:disable Metrics/MethodLength + # Convert status code to integer if it is a symbol + status_code = Rack::Utils::SYMBOL_TO_STATUS_CODE[status_code] if status_code.is_a?(Symbol) respond_to do |format| - @view_state = { user_selections: user_selections } - request_status = status == 400 ? :bad_request : :internal_server_error + @view_state ||= { user_selections: user_selections } format.html do render 'exceptions/error_page', - locals: { status: status, sentry_code: nil }, + locals: { status: status_code, sentry_code: nil }, layout: true, - status: request_status + status: status_code end format.json do - render(json: { status: 'request error' }, status: request_status) + render(json: { status: 'request error' }, status: status_code) end end end From 2cbfa93d79d0385c4af4995ddcf8f625cf792375 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Mon, 6 Jan 2025 12:36:50 +0000 Subject: [PATCH 20/27] refactor: handle differences in @view_state.user_selections --- app/views/common/_header.html.haml | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/app/views/common/_header.html.haml b/app/views/common/_header.html.haml index a5256a23..0a14e1a9 100644 --- a/app/views/common/_header.html.haml +++ b/app/views/common/_header.html.haml @@ -32,12 +32,13 @@ .o-secondary-banner - if Rails.application.config.welsh_language_enabled %nav.o-secondary-banner__lang-select{ 'aria-label' => 'language selector' } - - if @view_state.user_selections.english? + - user_selections = @view_state.respond_to?(:[]) ? @view_state[:user_selections] : @view_state.user_selections + - if user_selections.english? English - else - = link_to('English', @view_state.user_selections.alternative_language_params.params) + = link_to('English', user_selections.alternative_language_params.params) | - - if @view_state.user_selections.welsh? + - if user_selections.welsh? Cymraeg - else - = link_to('Cymraeg', @view_state.user_selections.alternative_language_params.params) + = link_to('Cymraeg', user_selections.alternative_language_params.params) From 9bec514816f92b3ebac34869eabbf53821a129d6 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Mon, 6 Jan 2025 12:37:34 +0000 Subject: [PATCH 21/27] build: set specific path for `unmatched_route` to `render_404` helper --- config/routes.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/routes.rb b/config/routes.rb index 8a39efa3..d679ca1f 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -16,5 +16,5 @@ get 'doc/ukhpi-dsd', to: 'doc#ukhpi_dsd' get 'doc/ukhpi-user-guide', to: 'doc#ukhpi_user_guide' - get '*unmatched_route', to: 'exceptions#handle_exceptions' + get '*unmatched_route', to: 'application#render_404' end From 8b5ba75abb0893fe46e6dc6273f25ed2e60c9c66 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Mon, 6 Jan 2025 12:38:02 +0000 Subject: [PATCH 22/27] style: disabled rubocop warnings --- app/controllers/application_controller.rb | 8 ++++---- app/controllers/compare_controller.rb | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index e721666b..022c71af 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true # :nodoc: -class ApplicationController < ActionController::Base +class ApplicationController < ActionController::Base # rubocop:disable Metrics/ClassLength include Rails.application.routes.url_helpers include ActionView::Helpers::TranslationHelper # Prevent CSRF attacks by raising an exception. @@ -111,7 +111,7 @@ def reset_response private - # rubocop:disable Metrics/AbcSize + # rubocop:disable Metrics/AbcSize, Metrics/MethodLength def detailed_request_log(duration) env = request.env @@ -139,11 +139,11 @@ def detailed_request_log(duration) Rails.logger.info(JSON.generate(log_fields)) end end - # rubocop:enable Metrics/AbcSize + # rubocop:enable Metrics/AbcSize, Metrics/MethodLength # Notify subscriber(s) of an internal error event with the payload of the # exception once done - # @param [Exception] exp the exception that caused the error + # @param [exc] exp the exception that caused the error # @return [ActiveSupport::Notifications::Event] provides an object-oriented # interface to the event # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity, Metrics/MethodLength diff --git a/app/controllers/compare_controller.rb b/app/controllers/compare_controller.rb index 4998baf9..a66da0ca 100644 --- a/app/controllers/compare_controller.rb +++ b/app/controllers/compare_controller.rb @@ -31,7 +31,7 @@ def render_print render 'compare/print', layout: 'print' end - def perform_query(user_compare_selections) + def perform_query(user_compare_selections) # rubocop:disable Metrics/MethodLength query_results = {} base_selection = UserSelections.new( __safe_params: { From 6d963911cf8f3d813a9b4a96f1b77f5795fb5334 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Mon, 6 Jan 2025 13:15:44 +0000 Subject: [PATCH 23/27] docs: updated CHANGELOG --- CHANGELOG.md | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f8eca75e..69022118 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,14 @@ ## Unreleased +- (Jon) Updated the `ApplicationController` to handle each error status + appropriately as well as ensure that the `instrument_internal_error` method is + called when an internal error is raised +- (Jon) Updated the `instrument_internal_error` method to replace the + `maybe_report_to_sentry` method while reporting internal errors to the + Prometheus metrics only when necessary +- (Jon) Removed the custom error handling by deleting the `ExceptionsController` + and the `exceptions` initialiser - (Jon) Fix for casting long strings to `Date`, `Time` or `DateTime` in Ruby 3.1.0 - (Jon) Improves error metrics reporting to ensure that logging always happens @@ -9,11 +17,6 @@ the types of errors that can trigger a an error metric and therefore a notification in slack [GH-149](https://github.com/epimorphics/hmlr-linked-data/issues/149) -- (Jon) Updated the `maybe_report_to_sentry` method to call the - `instrument_internal_error` method for reporting internal errors to the - Prometheus metrics when necessary -- (Jon) Renamed the `handle_error` method to `handle_exceptions` to better - reflect the method's purpose ## 2.0.0 - 2024-11 From 873ca46a5b109044352d7a1be90d31681c57e67f Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 7 Jan 2025 13:47:02 +0000 Subject: [PATCH 24/27] fix: use `if/elsif/else` in error page responses to ensure a message is displayed at all times. --- app/views/exceptions/error_page.html.haml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/exceptions/error_page.html.haml b/app/views/exceptions/error_page.html.haml index 6933d242..cfca09b2 100644 --- a/app/views/exceptions/error_page.html.haml +++ b/app/views/exceptions/error_page.html.haml @@ -19,7 +19,7 @@ %li = err_msg - - if status == 404 + - elsif status == 404 %h1.o-heading--1 Page not found %p @@ -27,7 +27,7 @@ Please check the spelling of the page address (URL). If you require further assistance, please see the contact details below. - - if status == 500 + - else %h1.o-heading--1 Application error %p From 243b4ab7267d3214df0b56fba1a58d0fa033f04e Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 7 Jan 2025 13:49:53 +0000 Subject: [PATCH 25/27] docs: updated CHANGELOG --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 69022118..3bca022a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ ## Unreleased +- (Jon) Refactored UI code to use if/elsif/else in error page responses to + ensure a message is displayed at all times no matter what status is passed in + as per best practices - (Jon) Updated the `ApplicationController` to handle each error status appropriately as well as ensure that the `instrument_internal_error` method is called when an internal error is raised From b93e32ebb79d5880dacdad6c88683a2d6df5c49b Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 7 Jan 2025 14:18:13 +0000 Subject: [PATCH 26/27] build: updated version patch cadence --- app/lib/version.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/lib/version.rb b/app/lib/version.rb index da348ade..4d205e68 100644 --- a/app/lib/version.rb +++ b/app/lib/version.rb @@ -3,7 +3,7 @@ module Version MAJOR = 2 MINOR = 0 - PATCH = 0 + PATCH = 1 SUFFIX = nil VERSION = "#{MAJOR}.#{MINOR}.#{PATCH}#{SUFFIX && ".#{SUFFIX}"}" end From 2c22473df12e1c097db3dd1fbfb6d1504ba2bc66 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 7 Jan 2025 14:19:43 +0000 Subject: [PATCH 27/27] docs: updated CHANGELOG Added update release version patch cadence with date stamp --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3bca022a..e3a5c143 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +## 2.0.1 - 2025-01 + - (Jon) Refactored UI code to use if/elsif/else in error page responses to ensure a message is displayed at all times no matter what status is passed in as per best practices