diff --git a/.githooks/pre-commit b/.githooks/pre-commit new file mode 100755 index 00000000..2763da5d --- /dev/null +++ b/.githooks/pre-commit @@ -0,0 +1,56 @@ +#!/bin/sh +# caveat: this script assumes all modifications to a file were staged in the commit +# beware if you are in the habit of committing only partial modifications to a file: +# THIS HOOK WILL ADD ALL MODIFICATIONS TO A FILE TO THE COMMIT IF ANY FILE WAS CHANGED BY LINTING + +list="issue spike task" + +listRE="^($(printf '%s\n' "$list" | tr ' ' '|'))/" + +BRANCH_NAME=$(git branch --show-current | grep -E "$listRE" | sed 's/* //') + +printf '\n\033[0;105mChecking "%s"... \033[0m\n' "$BRANCH_NAME" + +if echo "$BRANCH_NAME" | grep -q '^(rebase)|(production)*$'; then + printf '\n\033[0;32mNo checks necessary on "%s", pushing now... ๐ŸŽ‰\033[0m\n' "$BRANCH_NAME" + exit 0 +fi + +RUBY_FILES="$(git diff --diff-filter=d --name-only --cached | grep -E '(Gemfile|Rakefile|\.(rb|rake|ru))$')" +PRE_STATUS="$(git status | wc -l)" + +WORK_DONE=0 + +if [ -z "$RUBY_FILES" ]; then + printf '\n\033[0;31mThere are currently no updated files in "%s". ๐Ÿคจ\033[0m\n' "$BRANCH_NAME" +fi + +if [ -n "$RUBY_FILES" ]; then + printf '\n\033[0;33mRunning Rubocop...\033[0m\n' + for file in $RUBY_FILES; do + git show :"$file" | bundle exec rubocop -A --stdin "$file" + done + RUBOCOP_EXIT_CODE=$? + WORK_DONE=1 +else + printf '\n\033[0;32mContinuing as there are no changes to check... ๐ŸŽ‰\033[0m\n' + RUBOCOP_EXIT_CODE=0 +fi + +POST_STATUS="$(git status | wc -l)" + +if [ ! $RUBOCOP_EXIT_CODE -eq 0 ]; then + git reset HEAD + printf '\n\033[0;31mLinting has uncorrectable errors; please fix and restage your commit. ๐Ÿ˜–\033[0m\n' + exit 1 +fi + +if [ "$PRE_STATUS" != "$POST_STATUS" ]; then + git add "$RUBY_FILES" +fi + +if [ $WORK_DONE -eq 1 ]; then + printf '\n\033[0;32mLinting completed successfully! ๐ŸŽ‰\033[0m\n' +fi + +exit 0 diff --git a/.githooks/pre-push b/.githooks/pre-push new file mode 100755 index 00000000..18deee74 --- /dev/null +++ b/.githooks/pre-push @@ -0,0 +1,57 @@ +#!/bin/bash + +list="issue spike task" + +listRE="^($(printf '%s\n' "$list" | tr ' ' '|'))/" + +BRANCH_NAME=$(git branch --show-current | grep -E "$listRE" | sed 's/* //') + +printf '\n\033[0;105mChecking "%s"... \033[0m\n' "$BRANCH_NAME" + +if echo "$BRANCH_NAME" | grep -q '^(rebase)|(production)*$'; then + printf '\n\033[0;32mNo checks necessary on "%s", pushing now... ๐ŸŽ‰\033[0m\n' "$BRANCH_NAME" + exit 0 +fi + +# Check for existence of "new or modified" test files +TEST_FILES="$(git diff --diff-filter=ACDM --name-only --cached | grep -E '(./test/*)$')" +RUBY_FILES="$(git diff --diff-filter=ACDM --name-only --cached | grep -E '(_test\.rb)$')" +PRE_STATUS="$(git status | wc -l)" + +WORK_DONE=0 + +if [ -z "$TEST_FILES" ]; then + printf '\n\033[0;31mThere are currently no new tests found in "%s". ๐Ÿคจ\033[0m\n' "$BRANCH_NAME" + printf '\n\033[0;31mContinuing without new tests... ๐Ÿ˜–\033[0m\n' +fi + + +if [ -n "$RUBY_FILES" ]; then + printf '\nRunning Rails Tests...' + bundle exec rails test + RUBY_TEST_EXIT_CODE=$? + WORK_DONE=1 +else + RUBY_TEST_EXIT_CODE=0 +fi + +if [ -n "$RUBY_FILES" ]; then + printf '\nRunning System Tests...' + bundle exec rails test:system + RUBY_SYSTEM_EXIT_CODE=$? + WORK_DONE=1 +else + RUBY_SYSTEM_EXIT_CODE=0 +fi + + +if [ ! $RUBY_TEST_EXIT_CODE -eq 0 ] || [ ! $RUBY_SYSTEM_EXIT_CODE -eq 0 ]; then + printf '\n\033[0;31mCannot push, tests are failing. Use --no-verify to force push. ๐Ÿ˜–\033[0m\n' + exit 1 +fi + +if [ $WORK_DONE = 1 ]; then + printf '\n\033[0;32mAll tests are green, pushing... ๐ŸŽ‰\033[0m\n' +fi + +exit 0 diff --git a/CHANGELOG.md b/CHANGELOG.md index e3a5c143..caf640b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,8 +2,24 @@ ## Unreleased -## 2.0.1 - 2025-01 - +## 2.0.1 - 2025-02 + +- (Jon) Updated the logging to utilise a call-flow diagram to display the + sequence of messages that are sent between the app and the api. + [GH-478](https://github.com/epimorphics/ukhpi/issues/478) +- (Jon) Updated visibility and search input types + - Added conditional class for hiding elements in non-dev environments. + - Changed text fields to search fields for better UX. + - Cleaned up HTML structure for improved readability. +- (Jon) Added pre-commit and pre-push hooks + - Introduced a pre-commit hook to run Rubocop on staged Ruby files. + - Implemented a pre-push hook to run Rails tests before pushing. +- (Jon) Changed time measurement from microseconds to milliseconds in the main + query method +- (Jon) Removed duplicated gem entry +- (Jon) Added dynamic `LOG_LEVEL` env variable + - Defaults to `debug` in development and `info` in production/test. +- (Jon) Enhanced Sentry logging on both Rails and VUE/JS implementations - (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 @@ -36,6 +52,16 @@ - (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.6 - 2024-10 + +- (Jon) Split the error logging into it's own method as well as adjusted the + logged message to be either the response message or the response status +- (Jon) Renamed `render_error` method to `handle_error` +- (Jon) Set the Internal Error Instrumentation to an `unless` statement to + ensure the application does not report internal errors to the Prometheus + metrics when the error is a 404 thereby reducing the noise in the Slack alerts + channel + ## 1.7.5 - 2024-09 - (Jon) Created a `local` makefile target to allow for local development without @@ -137,7 +163,7 @@ - (Bogdan) Added alt text to application logo [GH-404](https://github.com/epimorphics/ukhpi/issues/404) -## 1.7.4 - 2024-04-19 +## 1.7.4 - 2024-05-01 - (Jon) Updated print presenter to use [`.try(:first)`](https://apidock.com/rails/Object/try) which resolves by diff --git a/Gemfile b/Gemfile index f678e3ba..d62d84ee 100644 --- a/Gemfile +++ b/Gemfile @@ -49,10 +49,11 @@ group :development, :test do end group :development do + # Devtools panel for Rails development + # https://chromewebstore.google.com/detail/rails-panel/gjpfobpafnhjhbajcjgccbbdofdckggg?pli=1 + gem 'meta_request' # 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/Makefile b/Makefile index f4cbec73..8b64b65b 100644 --- a/Makefile +++ b/Makefile @@ -37,8 +37,11 @@ ${GITHUB_TOKEN}: @echo ${PAT} > ${GITHUB_TOKEN} assets: + @echo "Installing bundler packages ..." @./bin/bundle install + @echo "Installing yarn packages ..." @yarn install + @echo "Cleaning and precompiling static assets ..." @./bin/rails assets:clean assets:precompile auth: ${GITHUB_TOKEN} ${BUNDLE_CFG} @@ -68,14 +71,6 @@ image: auth lint: assets @./bin/bundle exec rubocop -local: - @echo "Installing bundler packages ..." - @./bin/bundle install - @echo "Installing yarn packages ..." - @yarn install - @echo "Starting local server ..." - @./bin/rails server -p ${PORT} - publish: image @echo Publishing image: ${REPO}:${TAG} ... @docker tag ${NAME}:${TAG} ${REPO}:${TAG} 2>&1 @@ -89,8 +84,12 @@ 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: +secret: + @echo "Creating secret ..." @export SECRET_KEY_BASE=$(./bin/rails secret) + +server: + @echo "Starting local server ..." @API_SERVICE_URL=${API_SERVICE_URL} ./bin/rails server -p ${PORT} start: diff --git a/app/assets/stylesheets/_lr-common.scss b/app/assets/stylesheets/_lr-common.scss index c814c092..30625417 100644 --- a/app/assets/stylesheets/_lr-common.scss +++ b/app/assets/stylesheets/_lr-common.scss @@ -25,6 +25,12 @@ $lr-green-nav-darkened: darken( $lr-green-nav, 20% ); /* settings */ +ol, ul, nav ol, nav ul { + list-style: unset; + list-style-type: none; + list-style-position: inside; +} + /* elements */ a { diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 022c71af..b9945fff 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -36,7 +36,7 @@ def log_request_result start = Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) yield duration = Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) - start - detailed_request_log(duration) + detailed_log_result(duration) end # Handle specific types of exceptions and render the appropriate error page @@ -85,7 +85,7 @@ def render_500(_exception = nil) # rubocop:disable Naming/VariableNumber render_error(500) end - def render_error(status, sentry_code = nil) # rubocop:disable Metrics/AbcSize + def render_error(status, sentry_code = nil) reset_response status = Rack::Utils::SYMBOL_TO_STATUS_CODE[status] if status.is_a?(Symbol) @@ -98,7 +98,7 @@ def render_error(status, sentry_code = nil) # rubocop:disable Metrics/AbcSize end end - def render_html_error_page(status, sentry_code) # rubocop:disable Metrics/MethodLength + def render_html_error_page(status, sentry_code) render 'exceptions/error_page', locals: { status: status, sentry_code: sentry_code }, layout: true, @@ -111,42 +111,55 @@ def reset_response private - # rubocop:disable Metrics/AbcSize, Metrics/MethodLength - def detailed_request_log(duration) - env = request.env + # rubocop:disable Metrics/AbcSize, Metrics/MethodLength, Layout/LineLength, Metrics/PerceivedComplexity, Metrics/CyclomaticComplexity + def detailed_log_result(duration) + # prevent OK responses from being logged as they're logged elsewhere + return unless response&.status != 200 + env = request.env + query = env['QUERY_STRING'] || URI.parse(env['REQUEST_URI']).query log_fields = { - duration: duration, + message: response.message || Rack::Utils::HTTP_STATUS_CODES[response.status], + path: env['REQUEST_PATH'] || URI.parse(env['REQUEST_URI']).path, request_id: env['X_REQUEST_ID'], - forwarded_for: env['X_FORWARDED_FOR'], - path: env['REQUEST_PATH'], - query_string: env['QUERY_STRING'], - user_agent: env['HTTP_USER_AGENT'], - accept: env['HTTP_ACCEPT'], - body: request.body.gets&.gsub("\n", '\n'), + request_time: (duration / 1000) || env['REQUEST_TIME'], # in milliseconds method: request.method, - status: response.status, - message: Rack::Utils::HTTP_STATUS_CODES[response.status] + status: response.status } - case response.status - when 500..599 + log_fields[:query_string] = query if query.present? + + if env['HTTP_USER_AGENT'] && Rails.env.production? + log_fields[:user_agent] = env['HTTP_USER_AGENT'] + end + + if (500..599).include?(Rack::Utils::SYMBOL_TO_STATUS_CODE[response.status]) log_fields[:message] = env['action_dispatch.exception'].to_s - Rails.logger.error(JSON.generate(log_fields)) + log_fields[:backtrace] = env['action_dispatch.backtrace'].join("\n") unless Rails.env.production? + end + + log_response(response.status, log_fields.sort.to_h) + end + # rubocop:enable Metrics/AbcSize, Metrics/MethodLength, Layout/LineLength + + # Log the error with the appropriate log level based on the status code + def log_response(status, error_log) + case status + when 500..599 + Rails.logger.error(JSON.generate(error_log)) when 400..499 - Rails.logger.warn(JSON.generate(log_fields)) + Rails.logger.warn(JSON.generate(error_log)) else - Rails.logger.info(JSON.generate(log_fields)) + Rails.logger.info(JSON.generate(error_log)) end end - # rubocop:enable Metrics/AbcSize, Metrics/MethodLength # Notify subscriber(s) of an internal error event with the payload of the # exception once done # @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 + # rubocop:disable Metrics/AbcSize def instrument_internal_error(exc, status = nil) err = { message: exc&.message || exc, @@ -167,5 +180,5 @@ def instrument_internal_error(exc, status = nil) # Return the event id for the internal error event sevent&.event_id end - # rubocop:enable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity, Metrics/MethodLength + # rubocop:enable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity end diff --git a/app/controllers/browse_controller.rb b/app/controllers/browse_controller.rb index f3ff76a2..6ea8c5ed 100644 --- a/app/controllers/browse_controller.rb +++ b/app/controllers/browse_controller.rb @@ -14,6 +14,7 @@ def show elsif explain_non_json?(user_selections) redirect_to_html_view(user_selections) else + LoggingHelper.log_request({ params: params }) render_view_state(setup_view_state(user_selections)) end end diff --git a/app/controllers/compare_controller.rb b/app/controllers/compare_controller.rb index a66da0ca..a169ec51 100644 --- a/app/controllers/compare_controller.rb +++ b/app/controllers/compare_controller.rb @@ -41,6 +41,11 @@ def perform_query(user_compare_selections) # rubocop:disable Metrics/MethodLengt ) user_compare_selections.selected_locations.each do |location| + msg = 'Received Data Services API request from UKHPI service' + msg += " for #{location.label}" if location + log_fields = { params: base_selection.with('location', location.uri) } + log_fields[:message] = msg + LoggingHelper.log_request(log_fields) query_results[location.label] = query_with(base_selection, location) end diff --git a/app/javascript/packs/ukhpi_vue.js b/app/javascript/packs/ukhpi_vue.js index 2b6297fe..d71487bd 100644 --- a/app/javascript/packs/ukhpi_vue.js +++ b/app/javascript/packs/ukhpi_vue.js @@ -49,13 +49,19 @@ if (i18n.locale === 'cy') { } document.addEventListener('DOMContentLoaded', () => { + // Sentry.io logging + Sentry.init({ + dsn: 'https://1150348b449a444bb3ac47ddd82b37c4@sentry.io/251669', + debug: process.env.NODE_ENV === 'development', + environment: process.env.NODE_ENV, + integrations: [ + new Integrations.Vue({ Vue, attachProps: true }) + ], + release: window.ukhpi.version || '1.0.0', + ignoreErrors: ['Non-Error promise rejection captured'] + }) + if (process.env.NODE_ENV === 'production') { - // Sentry.io logging - Sentry.init({ - dsn: 'https://1150348b449a444bb3ac47ddd82b37c4@sentry.io/251669', - integrations: [new Integrations.Vue({ Vue, attachProps: true })], - release: window.ukhpi.version || 'unknown-version' - }) Sentry.configureScope(scope => { scope.setTag('app', 'ukhpi-js') }) diff --git a/app/lib/logging_helper.rb b/app/lib/logging_helper.rb new file mode 100644 index 00000000..460c08ea --- /dev/null +++ b/app/lib/logging_helper.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +# :nodoc: +class LoggingHelper + def self.log_request(fields, type = 'info') # rubocop:disable Metrics/AbcSize, Metrics/MethodLength, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity + puts "\n" if Rails.env.development? && Rails.logger.debug? # rubocop:disable Rails/Output + + # Extract service and params from fields hash + service = fields.delete(:service) if fields.key(:service) + params = fields.delete(:params) if fields.key(:params) + + fields[:message] ||= 'Received Data Services API request from the UKHPI service' + fields[:method] ||= 'GET' + fields[:path] ||= URI.parse(service.data_api).path if service + # fields[:request_path] ||= URI.parse(service.data_api).path if service + fields[:query_string] ||= JSON.generate(params) if params + fields[:request_status] ||= 'received' + fields[:status] ||= Rack::Utils::SYMBOL_TO_STATUS_CODE[:ok] + + Rails.logger.send(type) { JSON.generate(fields.sort.to_h) } unless fields.empty? + Rails.logger.flush if Rails.logger.respond_to?(:flush) + end +end diff --git a/app/lib/version.rb b/app/lib/version.rb index 4d205e68..e0a4c35c 100644 --- a/app/lib/version.rb +++ b/app/lib/version.rb @@ -5,5 +5,5 @@ module Version MINOR = 0 PATCH = 1 SUFFIX = nil - VERSION = "#{MAJOR}.#{MINOR}.#{PATCH}#{SUFFIX && ".#{SUFFIX}"}" + VERSION = "#{MAJOR}.#{MINOR}.#{PATCH}#{SUFFIX && ".#{SUFFIX}"}".freeze end diff --git a/app/lib/welsh_grammar.rb b/app/lib/welsh_grammar.rb index 72389598..52da38c9 100644 --- a/app/lib/welsh_grammar.rb +++ b/app/lib/welsh_grammar.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true -MUTATIONS = { +# Prevent warnings about constant alrady being initialized +MUTATIONS ||= { 'yn' => [ Mutation.new(/b(.*)/i, 'm\1', 'ym'), diff --git a/app/models/concerns/user_choices.rb b/app/models/concerns/user_choices.rb index 4bbdc3cf..196f6bd1 100644 --- a/app/models/concerns/user_choices.rb +++ b/app/models/concerns/user_choices.rb @@ -3,6 +3,7 @@ # Shared functionality for user models that provide access to the users' # choices articulated via the params in the incoming request module UserChoices + # Define the user parameters that can be set by the user Struct.new('UserParam', :default_value, :array?, :alias) attr_reader :params @@ -54,7 +55,8 @@ def alternative_key(key) end # Recognise a date. Accepts ISO-8601 strings or Date objects. - # @param date Either an ISO0-8601 date string, or a date object + # Dates that match YYYY-MM will be transformed to YYYY-MM-01. + # @param date Either an ISO-8601 date string, or a date object def parse_date(date) if date.is_a?(Date) date diff --git a/app/models/latest_values_command.rb b/app/models/latest_values_command.rb index aaa43dad..30835174 100644 --- a/app/models/latest_values_command.rb +++ b/app/models/latest_values_command.rb @@ -1,57 +1,101 @@ # frozen_string_literal: true # Command object to query the API for the latest available values -class LatestValuesCommand +class LatestValuesCommand # rubocop:disable Metrics/ClassLength include DataService attr_reader :results def perform_query(service = nil) hpi = service_api(service) - (hpi && run_query(hpi)) || no_service end private - def service_api(service) # rubocop:disable Metrics/AbcSize - service || dataset(:ukhpi) - rescue Faraday::ConnectionFailed => e - Rails.logger.error { 'Failed to connect to UK HPI ' } - Rails.logger.error { "Status: #{e.status}, body: '#{e.message}'" } - Rails.logger.error { e } - nil - rescue DataServicesApi::ServiceException => e - Rails.logger.error { 'Failed to get response from UK HPI service' } - Rails.logger.error { "Status: #{e.status}, body: '#{e.service_message}'" } - nil - rescue RuntimeError => e - Rails.logger.error { "Runtime error #{e.inspect}" } - Rails.logger.error { e.class } - Rails.logger.error { e.backtrace.join("\n") } - Rails.logger.error { "Caused by: #{e.cause}" } if e.cause - nil + def service_api(service) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity + log_fields = {} + log_type = 'error' + + service = nil if service.blank? + begin + # Set service to ukhpi dataset if not already set + service ||= dataset(:ukhpi) + + rescue Faraday::ConnectionFailed => e + log_fields[:message] = 'Failed to connect to UKHPI service' + log_fields[:body] = e.message + log_fields[:backtrace] = e&.backtrace&.join("\n") if Rails.env.development? + log_fields[:request_status] = log_type + log_fields[:status] = e.status + + service = nil + rescue DataServicesApi::ServiceException => e + log_fields[:message] = 'Failed to get response from UKHPI service' + log_fields[:body] = e.service_message + log_fields[:request_status] = log_type + log_fields[:status] = e.status + + service = nil + rescue RuntimeError => e + log_fields[:message] = "Runtime error #{e.inspect}" + log_fields[:body] = "Caused by: #{e.cause} in " if e.cause + log_fields[:body] += "\r\n(#{e.class})" if Rails.application.config.log_level == :debug + log_fields[:backtrace] = e&.backtrace&.join("\n") if Rails.env.development? + log_fields[:request_status] = log_type + log_fields[:status] = e.status + + service = nil + end + + LoggingHelper.log_request(log_fields, log_type) unless log_fields.empty? + + service end - def run_query(hpi) # rubocop:disable Metrics/AbcSize + def run_query(hpi) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity + log_fields = {} + log_type = 'error' + start = Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) + success = true query = add_date_range_constraint(base_query) query = add_location_constraint(query) query = add_sort_constraint(query) query = add_limit_constraint(query) - start = Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) + # Log the initial request received, passing in the service and query params + LoggingHelper.log_request({ service: hpi, params: query }) + begin @results = hpi.query(query) + + msg = 'completed Data Services API request from the UKHPI service' + log_type = 'info' rescue NoMethodError => e - Rails.logger.debug { "Application failed with: NoMethodError #{e.inspect}" } + msg = "application failed with: NoMethodError: #{e.inspect}" + log_fields[:status] = 405 + success = false + rescue ArgumentError => e + msg = "Data Services API request failed with: ArgumentError: #{e.inspect}" + log_fields[:status] = 422 success = false rescue RuntimeError => e - Rails.logger.error { "API query failed with: #{e.inspect}" } + msg = "Data Services API request failed with: #{e.inspect}" + log_fields[:status] = 400 success = false end - time_taken = (Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) - start) - Rails.logger.info { format("API query '#{query.to_json}' completed in %.0f ฮผs\n", time_taken) } + # Calculate the time taken to execute the query and pass in the details to be logged + time_taken = (Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) - start) / 1000 + msg += format(' in %.0f ms', time_taken) if time_taken.positive? + log_fields[:message] = msg.upcase_first + log_fields[:request_status] = success ? 'completed' : 'error' + log_fields[:request_time] = time_taken + log_fields[:status] = Rack::Utils::SYMBOL_TO_STATUS_CODE[:ok] if success + + # Log the final request status and response + LoggingHelper.log_request(log_fields, log_type) unless log_fields.empty? + puts "\n" if Rails.env.development? && Rails.logger.debug? success end diff --git a/app/models/locations.rb b/app/models/locations.rb index ba2f90e4..bd62d2af 100644 --- a/app/models/locations.rb +++ b/app/models/locations.rb @@ -6,24 +6,26 @@ class Locations extend LocationsTable - Struct.new('LocationSearchType', :label, :rdf_types) + # Prevent a warning about LocationSearchType being redefined + LocationSearchType = Struct.new('LocationSearchType', :label, :rdf_types) + + # Formatting update to reduce line length warnings from Rubocop + EUROPEAN_REGION = ['http://data.ordnancesurvey.co.uk/ontology/admingeo/EuropeanRegion'].freeze + LOCAL_AUTHORITIES = [ + 'http://data.ordnancesurvey.co.uk/ontology/admingeo/Borough', + 'http://data.ordnancesurvey.co.uk/ontology/admingeo/District', + 'http://data.ordnancesurvey.co.uk/ontology/admingeo/LondonBorough', + 'http://data.ordnancesurvey.co.uk/ontology/admingeo/MetropolitanDistrict', + 'http://data.ordnancesurvey.co.uk/ontology/admingeo/UnitaryAuthority' + ].freeze + ENGLAND_REGION = ['http://landregistry.data.gov.uk/def/ukhpi/Region'].freeze + ENGLAND_COUNTY = ['http://data.ordnancesurvey.co.uk/ontology/admingeo/County'].freeze + LOCATION_SEARCH_TYPES = { - 'country' => Struct::LocationSearchType.new('Country', [ - 'http://data.ordnancesurvey.co.uk/ontology/admingeo/EuropeanRegion' - ]), - 'local_authority' => Struct::LocationSearchType.new('Local authority', [ - 'http://data.ordnancesurvey.co.uk/ontology/admingeo/Borough', - 'http://data.ordnancesurvey.co.uk/ontology/admingeo/District', - 'http://data.ordnancesurvey.co.uk/ontology/admingeo/LondonBorough', - 'http://data.ordnancesurvey.co.uk/ontology/admingeo/MetropolitanDistrict', - 'http://data.ordnancesurvey.co.uk/ontology/admingeo/UnitaryAuthority' - ]), - 'england_region' => Struct::LocationSearchType.new('Region (England only)', [ - 'http://landregistry.data.gov.uk/def/ukhpi/Region' - ]), - 'england_county' => Struct::LocationSearchType.new('County (England only)', [ - 'http://data.ordnancesurvey.co.uk/ontology/admingeo/County' - ]) + 'country' => LocationSearchType.new('Country', EUROPEAN_REGION), + 'local_authority' => LocationSearchType.new('Local authority', LOCAL_AUTHORITIES), + 'england_region' => LocationSearchType.new('Region (England only)', ENGLAND_REGION), + 'england_county' => LocationSearchType.new('County (England only)', ENGLAND_COUNTY) }.freeze # @return The Location with the given URI, if it exists diff --git a/app/models/user_selections.rb b/app/models/user_selections.rb index e8fc2af1..53300b5f 100644 --- a/app/models/user_selections.rb +++ b/app/models/user_selections.rb @@ -8,7 +8,7 @@ # standard set of statistics is presented if there is no information in the # user parameters yet. This functionality combines the previous # `models/UserPreferences` and `presenters/Aspects` -class UserSelections +class UserSelections # rubocop:disable Metrics/ClassLength include UserChoices include UserSelectionValidations include UserLanguage diff --git a/app/services/query_command.rb b/app/services/query_command.rb index ee79337d..3dca2cc1 100644 --- a/app/services/query_command.rb +++ b/app/services/query_command.rb @@ -6,17 +6,26 @@ class QueryCommand attr_reader :user_selections, :results, :query + # Create a new UKHPI query command object + # @param [UserSelections] user_selections the user's selections + # @return [QueryCommand] the new command object def initialize(user_selections) @user_selections = user_selections @query = build_query end # Perform the UKHPI query encapsulated by this command object - # @param service Optional API service end-point to use. Defaults to the UKHPI - # API service endpoint - def perform_query(service = nil) - time_taken = execute_query(service, query) - Rails.logger.info(format("API roundtrip took %.0f ฮผs\n", time_taken)) + # @param [DataServicesApi::Service] service the API service to use + # Defaults to the UKHPI API service endpoint + def perform_query(service = nil) # rubocop:disable Metrics/AbcSize + log_fields = { message: 'Completed Data Services API request from the UKHPI service' } + time_taken = execute_query(service, query) / 1000 + log_fields[:message] += format(' in %.0f ms', time_taken) if time_taken.positive? + log_fields[:request_status] = 'completed' + log_fields[:request_time] = time_taken + log_fields[:status] = Rack::Utils::SYMBOL_TO_STATUS_CODE[:ok] + LoggingHelper.log_request(log_fields) unless log_fields.empty? + puts "\n" if Rails.env.development? && Rails.logger.debug? end # @return True if this a query execution command @@ -38,21 +47,29 @@ def build_query add_sort(query1) end + # @return [DataServicesApi::Service] the API service to use def api_service(service) @api_service ||= service || default_service end + # @return [DataServicesApi::Service] the default API service to use def default_service dataset(:ukhpi) end - # Run the given query, stash the results, and return time taken in microseconds. + # Execute the given query using the given service + # @param [DataServicesApi::Service] service the API service to use + # @param [DataServicesApi::Query] query the query to execute + # @return [Integer] the time taken to execute the query in microseconds def execute_query(service, query) start = Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) @results = api_service(service).query(query) (Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) - start) end + # Add a date range constraint to the given query + # @param [DataServicesApi::Query] query the query to add the constraint to + # @return [DataServicesApi::Query] the modified query def add_date_range_constraint(query) from = month_year_value(user_selections.from_date) to = month_year_value(user_selections.to_date) @@ -61,19 +78,29 @@ def add_date_range_constraint(query) .le('ukhpi:refMonth', to) end + # Add a location constraint to the given query + # @param [DataServicesApi::Query] query the query to add the constraint to + # @return [DataServicesApi::Query] the modified query def add_location_constraint(query) value = DataServicesApi::Value.uri(location_uri) query.eq('ukhpi:refRegion', value) end + # Add a sort constraint to the given query + # @param [DataServicesApi::Query] query the query to add the constraint to + # @return [DataServicesApi::Query] the modified query def add_sort(query) query.sort(:up, 'ukhpi:refMonth') end + # Convert provided date to a year-month value + # @param [Date] date the date to convert + # @return [DataServicesApi::Value] the year-month value def month_year_value(date) DataServicesApi::Value.year_month(date.year, date.month) end + # @return [String] the URI of the selected location def location_uri user_selections.selected_location end diff --git a/app/views/browse/_data_view.html.haml b/app/views/browse/_data_view.html.haml index 3450a198..b4c979e0 100644 --- a/app/views/browse/_data_view.html.haml +++ b/app/views/browse/_data_view.html.haml @@ -12,7 +12,7 @@ = visibility_action - if visible - .o-data-view__options.u-js-hidden + .o-data-view__options{:class => ("u-js-hidden" unless Rails.env.development?)} = t('preposition.for').titlecase = succeed ',' do = data_view.selected_location_label @@ -33,7 +33,7 @@ %a{ href: data_view.add_remove_statistic(!checked, stat) } = stat.label - .o-data-view__data-table.u-js-hidden + .o-data-view__data-table{:class => ("u-js-hidden" unless Rails.env.development?)} - table_data = data_view.as_table_data %table %thead diff --git a/app/views/browse/_user_selections_controls.html.haml b/app/views/browse/_user_selections_controls.html.haml index 27e054ba..851457bc 100644 --- a/app/views/browse/_user_selections_controls.html.haml +++ b/app/views/browse/_user_selections_controls.html.haml @@ -4,7 +4,7 @@ %p = label_tag(:'location-term', t('browse.edit.form.search_prompt')) - = text_field_tag(:'location-term') + = search_field_tag(:'location-term') %button{ type: 'submit', value: 'search', name: 'form-action' } = t('action.search') diff --git a/app/views/common/_puma_stats.html.haml b/app/views/common/_puma_stats.html.haml index 2efbd0be..5324ae78 100644 --- a/app/views/common/_puma_stats.html.haml +++ b/app/views/common/_puma_stats.html.haml @@ -1,5 +1,8 @@ -%div - %pre - %code - = "[#{JSON.pretty_generate(puma_stats: JSON.parse(Puma.stats), time: Time.now)}]" +%div{style: 'padding: 1rem;'} + %details + %summary + Puma Stats + %pre + %code + = "[#{JSON.pretty_generate(puma_stats: JSON.parse(Puma.stats), time: Time.now)}]" diff --git a/app/views/compare/show.html.haml b/app/views/compare/show.html.haml index f4bcaeba..fefd9320 100644 --- a/app/views/compare/show.html.haml +++ b/app/views/compare/show.html.haml @@ -8,7 +8,7 @@ %p.u-muted = t('compare.show.prompt') - .c-location-compare--non-js.u-js-hidden + .c-location-compare--non-js{:class => ("u-js-hidden" unless Rails.env.development?)} %form.c-compare__form{ action: compare_path, method: 'get' } %p %label{ for: 'select-in' } @@ -49,7 +49,7 @@ %li.c-compare__add-location %label{ for: 'location-term', id: 'location-term-label'} = t('compare.show.add_location') - %input{ id: 'location-term', type: 'text', name: 'location-term', "aria-labelledby": "location-term-label", value: @view_state.search_term } + %input{ id: 'location-term', type: 'search', name: 'location-term', "aria-labelledby": "location-term-label", value: @view_state.search_term } %button{ type: 'submit', value: 'search', name: 'form-action' } = t('action.search') @@ -70,7 +70,7 @@ %p = msg - else - %table.c-compare__results.u-js-hidden + %table.c-compare__results{:class => ("u-js-hidden" unless Rails.env.development?)} %thead %tr %th{ scope: 'col' } diff --git a/app/views/layouts/webpack_application.html.haml b/app/views/layouts/webpack_application.html.haml index 3e5ef16c..3bb8f236 100644 --- a/app/views/layouts/webpack_application.html.haml +++ b/app/views/layouts/webpack_application.html.haml @@ -6,6 +6,7 @@ %meta{ name: 'viewport', content: 'width=device-width, initial-scale=1, shrink-to-fit=no' } %title = (yield(:title) + " - " unless yield(:title).blank?).to_s + I18n.t('common.header.app_title') + :javascript document.querySelector('html').classList.add('js'); window.ukhpi = window.ukhpi || {}; @@ -16,7 +17,8 @@ - if Rails.env.production? = render partial: 'common/google-analytics' = javascript_include_tag 'cookie', defer: true - = csrf_meta_tags + + = csrf_meta_tags = stylesheet_link_tag 'application', media: 'all' = render partial: 'common/favicons' @@ -46,3 +48,4 @@ = yield = render partial: 'common/footer' + = render partial: 'common/puma_stats' if Rails.env.development? diff --git a/config/environments/development.rb b/config/environments/development.rb index fcaae2d8..ee827384 100644 --- a/config/environments/development.rb +++ b/config/environments/development.rb @@ -53,4 +53,7 @@ # API location can be specified in the environment but defaults to the dev service config.api_service_url = ENV.fetch('API_SERVICE_URL', 'http://localhost:8888') + + # Set the log level to the value of the LOG_LEVEL environment variable, or 'debug' if not set + config.log_level = ENV.fetch('LOG_LEVEL', 'debug').to_sym end diff --git a/config/environments/production.rb b/config/environments/production.rb index 38d2a375..faf7bc79 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -92,4 +92,7 @@ # API location is specified in the environment variable API_SERVICE_URL config.api_service_url = ENV.fetch('API_SERVICE_URL', nil) + + # Set the log level to the value of the LOG_LEVEL environment variable, or 'info' if not set + config.log_level = ENV.fetch('LOG_LEVEL', 'info').to_sym end diff --git a/config/environments/test.rb b/config/environments/test.rb index cfe3a61f..b487fd77 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -47,4 +47,7 @@ # API location can be specified in the environment # But defaults to the dev service config.api_service_url = ENV.fetch('API_SERVICE_URL', 'http://localhost:8080') + + # Set the log level to the value of the LOG_LEVEL environment variable, or 'info' if not set + config.log_level = ENV.fetch('LOG_LEVEL', 'info').to_sym end diff --git a/config/initializers/sentry.rb b/config/initializers/sentry.rb index 3147c0e2..a3826e74 100644 --- a/config/initializers/sentry.rb +++ b/config/initializers/sentry.rb @@ -1,14 +1,34 @@ # frozen_string_literal: true +require 'version' + Rails.application.reloader.to_prepare do if ENV['SENTRY_API_KEY'] Sentry.init do |config| + # https://docs.sentry.io/platforms/ruby/configuration/options/#breadcrumbs-logger + config.breadcrumbs_logger = %i[sentry_logger monotonic_active_support_logger http_logger] + # The DSN tells the SDK where to send events. config.dsn = ENV['SENTRY_API_KEY'] - config.environment = ENV.fetch('DEPLOYMENT_ENVIRONMENT') { Rails.env } - config.enabled_environments = %w[production] + # Only report errors in these environments: + config.enabled_environments = %w[production prod preprod dev] + # Ignore exceptions that are not useful to us + config.excluded_exceptions += [ + 'ActionController::BadRequest', + 'ActionController::RoutingError', + 'ActiveRecord::RecordNotFound' + ] + # Set the environment name from the DEPLOYMENT_ENVIRONMENT environment variable + config.environment = ENV.fetch('DEPLOYMENT_ENVIRONMENT', Rails.env) + ## Default to only reporting info, warnings and errors to Sentry + config.logger.level = Rails.application.config.log_level || :info + # Set the release version to the current version config.release = Version::VERSION - config.breadcrumbs_logger = %i[active_support_logger http_logger] - config.excluded_exceptions += ['ActionController::BadRequest'] + # Set traces_sample_rate to 1.0 to capture 100% of transactions for tracing. + # Sentry recommends adjusting this value in production hence the ternary operator. + config.traces_sample_rate = Rails.env.development? ? 1.0 : 0.1 + # Set profiles_sample_rate to profile 100% of sampled transactions. + # Sentry recommends adjusting this value in production hence the ternary operator. + config.profiles_sample_rate = Rails.env.production? ? 1.0 : 0.1 end end end diff --git a/config/puma.rb b/config/puma.rb index d8d43964..766ebdf9 100644 --- a/config/puma.rb +++ b/config/puma.rb @@ -14,10 +14,6 @@ # default is 3000. port ENV.fetch('PORT', 3000) -# Specifies the `metrics_port` that Puma will listen on to export metrics; -# default is 9393. -# metrics_port = ENV.fetch('METRICS_PORT', 9393) - # Specifies the `environment` that Puma will run in. # environment ENV.fetch('RAILS_ENV', 'development') @@ -53,12 +49,15 @@ # Allow puma to be restarted by `rails restart` command. plugin :tmp_restart -# Uncomment the following line once ruby is updated to 2.7 or greater to allow -# the use of the puma-metrics plugin as we're using puma 6.0.0 or greater # Additional metrics from the Puma server to be exposed in the /metrics endpoint -# plugin :metrics +plugin :metrics + +# Specifies the `metrics_port` that Puma will listen on to export metrics; +# default is 9393. +metrics_port = ENV.fetch('METRICS_PORT', 9393) + # Bind the metric server to "url". "tcp://" is the only accepted protocol. -# metrics_url "tcp://0.0.0.0:#{metrics_port}" if Rails.env.development? +metrics_url "tcp://0.0.0.0:#{metrics_port}" if Rails.env.development? # Use a custom log formatter to emit Puma log messages in a JSON format log_formatter do |str| diff --git a/test/models/user_selections_test.rb b/test/models/user_selections_test.rb index d305c425..0477cced 100644 --- a/test/models/user_selections_test.rb +++ b/test/models/user_selections_test.rb @@ -77,7 +77,7 @@ class UserSelectionsTest < ActiveSupport::TestCase end end - describe '#with' do + describe '#with_the_new_singular_value' do it 'should create a new user preferences with the new singular value' do selections0 = user_selections('location' => 'test-region-0') _(selections0.selected_location).must_equal 'test-region-0' @@ -93,7 +93,9 @@ class UserSelectionsTest < ActiveSupport::TestCase _(selections2.selected_location).must_equal 'test-region-2' _(selections2.from_date).must_equal Date.new(2017, 3, 25) end + end + describe '#with_the_additional_array_value' do it 'should create a new user preferences with an additional array value' do selections0 = user_selections('in' => ['averagePrice']) _(selections0.selected_indicators).must_equal ['averagePrice'] @@ -112,7 +114,9 @@ class UserSelectionsTest < ActiveSupport::TestCase _(selections2.selected_indicators).must_include 'percentageMonthlyChange' _(selections2.selected_indicators).must_include 'percentageAnnualChange' end + end + describe '#with_array_valued_params' do it 'should not create duplicate values in array-valued params' do selections0 = user_selections('in' => ['averagePrice']) selections1 = selections0.with('in', 'averagePrice') @@ -121,7 +125,7 @@ class UserSelectionsTest < ActiveSupport::TestCase end end - describe '#without' do + describe '#without_the_given_key_for_singlular_values' do it 'should create a new user preferences value without the given key for singlular values' do selections0 = user_selections('location' => 'test-region-0') _(selections0.selected_location).must_equal 'test-region-0' @@ -131,7 +135,9 @@ class UserSelectionsTest < ActiveSupport::TestCase assert selections0.params.key?('location') assert_not selections1.params.key?('location') end + end + describe '#without_the_given_value_for_array_values' do it 'should create a new user preferences value without the given value for array values' do selections0 = user_selections('in' => %w[averagePrice percentageMonthlyChange]) _(selections0.selected_indicators.length).must_equal 2 @@ -158,19 +164,34 @@ class UserSelectionsTest < ActiveSupport::TestCase end end - describe '#valid?' do + describe '#empty_model_is_valid?' do it 'should report the empty model is valid' do selections = user_selections({}) _(selections.valid?).must_equal(true) end + end + + describe '#formatted_date_is_valid?' do + it 'should allow a YYYY-MM date as valid' do + selections = user_selections( + 'from' => '2017-01' + ) + _(selections.valid?).must_equal(true) + _(selections.errors).must_be_empty + end + end + + describe '#from_date_is_valid?' do it 'should report that a correctly formatted from date is valid' do selections = user_selections( 'from' => '2017-01-01' ) _(selections.valid?).must_equal(true) end + end + describe '#from_date_is_invalid?' do it 'should report that an incorrectly formatted from date is invalid' do selections = user_selections( 'from' => '2017-0' @@ -178,15 +199,10 @@ class UserSelectionsTest < ActiveSupport::TestCase _(selections.valid?).must_equal(false) _(selections.errors).must_include('incorrect or missing "from" date') end + end - it 'should allow a YYYY-MM date as valid' do - selections = user_selections( - 'from' => '2017-01' - ) - _(selections.valid?).must_equal(true) - _(selections.errors).must_be_empty - end + describe '#to_date_is_invalid?' do it 'should report that an incorrectly formatted to date is invalid' do selections = user_selections( 'to' => '2017-0' @@ -194,14 +210,18 @@ class UserSelectionsTest < ActiveSupport::TestCase _(selections.valid?).must_equal(false) _(selections.errors).must_include('incorrect or missing "to" date') end + end + describe '#location_is_valid?' do it 'should report that a correct location URI is valid' do selections = user_selections( location: 'http://landregistry.data.gov.uk/id/region/united-kingdom' ) _(selections.valid?).must_equal(true) end + end + describe '#location_is_invalid?' do it 'should report that an incorrect location URI is invalid' do selections = user_selections( location: 'http://landregistry.data.gov.uk/id/region/' @@ -209,35 +229,53 @@ class UserSelectionsTest < ActiveSupport::TestCase _(selections.valid?).must_equal(false) _(selections.errors).must_include('unrecognised location') end + end + describe '#indicator_is_valid?' do it 'should report that a correctly stated indicator is valid' do selections = user_selections('in' => %w[pac pmc]) _(selections.valid?).must_equal(true) end + end + describe '#indicator_is_invalid?' do it 'should report that an incorrectly stated indicator is not valid' do selections = user_selections('in' => %w[pmc percentageMonthlyWombles]) _(selections.valid?).must_equal(false) end + end + describe '#statistic_is_valid?' do it 'should report that a correctly stated statistic is valid' do selections = user_selections('st' => %w[det sem]) _(selections.valid?).must_equal(true) end + end + describe '#statistic_is_invalid?' do it 'should report that an incorrectly stated statistic is not valid' do selections = user_selections('st' => %w[det percentageMonthlyWombles]) _(selections.valid?).must_equal(false) end end - describe 'language handling' do + describe '#default_language_selected' do it 'should return English as the default' do selections = user_selections({}) assert selections.english? assert_not selections.welsh? end + end + describe '#default_language_params' do + it 'should return English when that language is selected' do + selections = user_selections('lang' => 'en') + assert selections.english? + assert_not selections.welsh? + end + end + + describe '#alternative_language_params' do it 'should return Welsh when that language is selected' do selections = user_selections('lang' => 'cy') current_locale = I18n.locale @@ -248,19 +286,17 @@ class UserSelectionsTest < ActiveSupport::TestCase ensure I18n.locale = current_locale end + end - it 'should return English when that language is selected' do - selections = user_selections('lang' => 'en') - assert selections.english? - assert_not selections.welsh? - end - + describe '#ignore_other_languages' do it 'should ignore other languages' do selections = user_selections('lang' => 'fr') assert selections.english? assert_not selections.welsh? end + end + describe '#alternative_language_params_switch' do it 'should generate the correct options to switch to Welsh language' do selections = user_selections( 'from' => '2017-01' @@ -270,7 +306,9 @@ class UserSelectionsTest < ActiveSupport::TestCase _(alt_params.params['from']).must_equal('2017-01') _(alt_params.params['lang']).must_equal('cy') end + end + describe '#default_language_params_switch' do it 'should generate the correct options to switch to English language' do selections = user_selections( 'from' => '2017-01', diff --git a/test/services/query_command_test.rb b/test/services/query_command_test.rb index b3c53a8c..9f83465a 100644 --- a/test/services/query_command_test.rb +++ b/test/services/query_command_test.rb @@ -2,7 +2,7 @@ require 'test_helper' -class MockService +class MockService # :nodoc: attr_reader :captured def query(query) @@ -10,7 +10,7 @@ def query(query) end end -def validate_json(json) +def validate_json(json) # rubocop:disable Metrics/MethodLength _(json).must_match_json_expression( '@and': [