Skip to content

Commit

Permalink
Move setting slug to after action
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
rjlynch committed Oct 17, 2024
1 parent b47a166 commit 6e7a8cd
Showing 1 changed file with 6 additions and 2 deletions.
8 changes: 6 additions & 2 deletions app/controllers/claims_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 6e7a8cd

Please sign in to comment.