From edd1febb0f611b0345b177df19aa4ccdce08e3fe Mon Sep 17 00:00:00 2001 From: William Fish Date: Tue, 6 Dec 2022 11:25:20 +0000 Subject: [PATCH 1/4] HOTT-2237: Handle session integrity issues when fetching schemes --- app/controllers/steps/base_controller.rb | 21 +++++++++++++++++---- app/models/api/commodity.rb | 2 ++ config/application.rb | 1 + spec/models/api/commodity_spec.rb | 22 +++++++++++++++------- 4 files changed, 35 insertions(+), 11 deletions(-) diff --git a/app/controllers/steps/base_controller.rb b/app/controllers/steps/base_controller.rb index 6c538d79..6ec061d6 100644 --- a/app/controllers/steps/base_controller.rb +++ b/app/controllers/steps/base_controller.rb @@ -3,6 +3,7 @@ class BaseController < ::ApplicationController include CommodityHelper include ServiceHelper + rescue_from Errors::SessionIntegrityError, with: :handle_session_integrity_error rescue_from StandardError, with: :handle_exception default_form_builder GOVUKDesignSystemFormBuilder::FormBuilder @@ -53,14 +54,20 @@ def track_session end def handle_exception(exception) - Sentry.set_user(user_session.session.to_h.except('_csrf_token')) - track_session + with_session_tracking do + raise exception + end + end - raise exception + def handle_session_integrity_error(exception) + with_session_tracking do + Sentry.capture_exception(exception) + redirect_to sections_url, allow_other_host: true + end end def ensure_session_integrity - return redirect_to sections_url, allow_other_host: true if commodity_code.blank? + handle_session_integrity_error(Errors::SessionIntegrityError.new('commodity_code')) if commodity_code.blank? end def initialize_commodity_context_service @@ -70,5 +77,11 @@ def initialize_commodity_context_service def title t("page_titles.#{@step.class.try(:id)}", default: super) end + + def with_session_tracking + Sentry.set_user(user_session.session.to_h.except('_csrf_token')) + track_session + yield + end end end diff --git a/app/models/api/commodity.rb b/app/models/api/commodity.rb index 942da12c..da65e930 100644 --- a/app/models/api/commodity.rb +++ b/app/models/api/commodity.rb @@ -61,6 +61,8 @@ def applicable_excise_measures end def rules_of_origin_schemes + raise Errors::SessionIntegrityError, 'origin_country_code' if user_session.origin_country_code.blank? + RulesOfOriginScheme.build_collection( source, nil, diff --git a/config/application.rb b/config/application.rb index b08fd724..1ac337e5 100644 --- a/config/application.rb +++ b/config/application.rb @@ -14,6 +14,7 @@ require "action_view/railtie" # require "action_cable/engine" # require "rails/test_unit/railtie" +require "errors" # Require the gems listed in Gemfile, including any gems # you've limited to :test, :development, or :production. diff --git a/spec/models/api/commodity_spec.rb b/spec/models/api/commodity_spec.rb index fb5dc24b..e2587474 100644 --- a/spec/models/api/commodity_spec.rb +++ b/spec/models/api/commodity_spec.rb @@ -197,16 +197,24 @@ allow(Api::RulesOfOriginScheme).to receive(:build_collection).and_call_original end - let(:user_session) { build(:user_session, :with_row_to_gb_route) } - - it 'passes the correct arguments to the RulesOfOriginScheme collection builder' do - commodity.rules_of_origin_schemes + context 'when the user session has no origin country code in it' do + let(:user_session) { build(:user_session) } - expect(Api::RulesOfOriginScheme).to have_received(:build_collection).with('uk', nil, { country_code: 'AR', heading_code: '070200' }) + it { expect { commodity.rules_of_origin_schemes }.to raise_error(Errors::SessionIntegrityError, /origin_country_code/) } end - it 'returns a collection of RulesOfOriginScheme records' do - expect(commodity.rules_of_origin_schemes.first).to be_a(Api::RulesOfOriginScheme) + context 'when the user session has an origin country code in it' do + let(:user_session) { build(:user_session, :with_row_to_gb_route) } + + it 'passes the correct arguments to the RulesOfOriginScheme collection builder' do + commodity.rules_of_origin_schemes + + expect(Api::RulesOfOriginScheme).to have_received(:build_collection).with('uk', nil, { country_code: 'AR', heading_code: '070200' }) + end + + it 'returns a collection of RulesOfOriginScheme records' do + expect(commodity.rules_of_origin_schemes.first).to be_a(Api::RulesOfOriginScheme) + end end end From 9e5aefbfe36f4b9e0152be8a1c45e4fd930f39db Mon Sep 17 00:00:00 2001 From: William Fish Date: Tue, 6 Dec 2022 11:32:42 +0000 Subject: [PATCH 2/4] HOTT-2237: Fixes broken specs --- lib/errors.rb | 13 +++++++++++++ spec/controllers/steps/duty_controller_spec.rb | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-) create mode 100644 lib/errors.rb diff --git a/lib/errors.rb b/lib/errors.rb new file mode 100644 index 00000000..70a7c996 --- /dev/null +++ b/lib/errors.rb @@ -0,0 +1,13 @@ +module Errors + class SessionIntegrityError < StandardError + def initialize(expected_attribute) + super + + @expected_attribute = expected_attribute + end + + def message + "User session state error. You are missing #{@expected_attribute}. Do you have multiple tabs open?" + end + end +end diff --git a/spec/controllers/steps/duty_controller_spec.rb b/spec/controllers/steps/duty_controller_spec.rb index a9d8dbf0..d11b0322 100644 --- a/spec/controllers/steps/duty_controller_spec.rb +++ b/spec/controllers/steps/duty_controller_spec.rb @@ -4,7 +4,7 @@ allow(Api::MonetaryExchangeRate).to receive(:for).with('GBP').and_call_original end - let(:user_session) { build(:user_session, import_destination: 'XI', commodity_code: '0103921100') } + let(:user_session) { build(:user_session, import_destination: 'XI', commodity_code: '0103921100', country_of_origin: 'AF') } let(:duty_calculator) { instance_double('DutyCalculator', options: []) } describe 'GET #show' do From d6ed448f72a62e2f7e134a6343a5d48a85d2c5e7 Mon Sep 17 00:00:00 2001 From: William Fish Date: Tue, 6 Dec 2022 11:36:31 +0000 Subject: [PATCH 3/4] HOTT-2237: Lint --- config/application.rb | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/config/application.rb b/config/application.rb index 1ac337e5..be9465a8 100644 --- a/config/application.rb +++ b/config/application.rb @@ -1,20 +1,20 @@ -require_relative "boot" +require_relative 'boot' -require "rails" +require 'rails' # Pick the frameworks you want: -require "active_model/railtie" +require 'active_model/railtie' # require "active_job/railtie" require 'active_record/attribute_assignment' # require "active_record/railtie" # require "active_storage/engine" -require "action_controller/railtie" +require 'action_controller/railtie' # require "action_mailer/railtie" # require "action_mailbox/engine" # require "action_text/engine" -require "action_view/railtie" +require 'action_view/railtie' # require "action_cable/engine" # require "rails/test_unit/railtie" -require "errors" +require 'errors' # Require the gems listed in Gemfile, including any gems # you've limited to :test, :development, or :production. @@ -33,9 +33,9 @@ class Application < Rails::Application # config.time_zone = "Central Time (US & Canada)" # config.eager_load_paths << Rails.root.join("extras") config.middleware.use Rack::Deflater - config.time_zone = "London" + config.time_zone = 'London' config.exceptions_app = routes - config.trade_tariff_frontend_url = ENV["TRADE_TARIFF_FRONTEND_URL"] - config.duty_calculator_host = ENV.fetch("DUTY_CALCULATOR_HOST", "http://localhost") + config.trade_tariff_frontend_url = ENV['TRADE_TARIFF_FRONTEND_URL'] + config.duty_calculator_host = ENV.fetch('DUTY_CALCULATOR_HOST', 'http://localhost') end end From cff5e24ce906c1de0588f9dedb3c9b23dcffceaf Mon Sep 17 00:00:00 2001 From: William Fish Date: Tue, 6 Dec 2022 13:46:56 +0000 Subject: [PATCH 4/4] HOTT-2237: Use require_relative --- config/application.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/config/application.rb b/config/application.rb index be9465a8..3c0e0eb5 100644 --- a/config/application.rb +++ b/config/application.rb @@ -14,7 +14,8 @@ require 'action_view/railtie' # require "action_cable/engine" # require "rails/test_unit/railtie" -require 'errors' + +require_relative '../lib/errors' # Require the gems listed in Gemfile, including any gems # you've limited to :test, :development, or :production.