From 6e7a8cd467b7662ee003399295955a842a0e12f7 Mon Sep 17 00:00:00 2001 From: Richard Lynch Date: Tue, 15 Oct 2024 12:14:44 +0100 Subject: [PATCH] Move setting slug to after action Adjusts the completed slugs method to only update the list of visited slugs if the form is valid. There's been some rollbar errors where the user seems to have skipped ahead in the journey but hasn't be redirected back by the check_page_is_in_sequence callback. All the tests pass but this seems like it could be risky. I'm not 100% sure that no code is subtly relying on the visited slugs being updated on failed form verifications. We may want to consider a different approach to wizard state management, maybe passing the session answers to a state machine to determine which slugs are accessible. --- app/controllers/claims_controller.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/controllers/claims_controller.rb b/app/controllers/claims_controller.rb index d711406d4..b7ed3bf5a 100644 --- a/app/controllers/claims_controller.rb +++ b/app/controllers/claims_controller.rb @@ -4,13 +4,13 @@ class ClaimsController < BasePublicController skip_before_action :send_unstarted_claimants_to_the_start, only: [:new, :create] before_action :initialize_session_slug_history before_action :check_page_is_in_sequence, only: [:show, :update] - before_action :update_session_with_current_slug, only: [:update] before_action :set_backlink_path, only: [:show, :update] before_action :check_claim_not_in_progress, only: [:new] before_action :clear_claim_session, only: [:new], unless: -> { journey.start_with_magic_link? } before_action :prepend_view_path_for_journey before_action :persist_claim, only: [:new, :create] before_action :handle_magic_link, only: [:new], if: -> { journey.start_with_magic_link? } + after_action :update_session_with_current_slug, only: [:update] include AuthorisedSlugs include FormSubmittable @@ -90,7 +90,11 @@ def initialize_session_slug_history end def update_session_with_current_slug - session[:slugs] << params[:slug] unless Journeys::PageSequence::DEAD_END_SLUGS.include?(params[:slug]) + if @form.nil? || @form.valid? + session[:slugs] << params[:slug] unless Journeys::PageSequence::DEAD_END_SLUGS.include?(params[:slug]) + else + # Don't count form as visited if it's invalid + end end def check_claim_not_in_progress