CLDC-1985 Fix error message persistence bug (#1515)

* feat: assign simultaneously

* feat: assign simultaneously

* feat: revert

* feat: fix back persistence behaviour and update similar issues elsewhere, also remove redundant back text in govuk back links

* feat: add previous_page_redirect_paths to avoid infinite loops

* feat: leave here to avoid possible loops

* feat: leave here to avoid possible loops

* feat: add check answers page behaviour

* refactor: tweak

* feat: make tests pass, simplify code and incorporate tasklist/check answers back routing

* feat: update tests

* refactor: lint

app/controllers/form_controller.rb

@@ -122,11 +122,9 @@ private
 
   def successful_redirect_path
     if is_referrer_check_answers?
-      page_ids = form.subsection_for_page(@page).pages.map(&:id)
+      next_page_id = form.next_page_id(@page, @log, current_user)
-      page_index = page_ids.index(@page.id)
-      next_page_id = form.next_page(@page, @log, current_user)
       next_page = form.get_page(next_page_id)
-      previous_page = form.previous_page(page_ids, page_index, @log, current_user)
+      previous_page = form.previous_page_id(@page, @log, current_user)
 
       if next_page&.interruption_screen? || next_page_id == previous_page
         return send("#{@log.class.name.underscore}_#{next_page_id}_path", @log, { referrer: "check_answers" })

app/helpers/tasklist_helper.rb

@@ -15,7 +15,7 @@ module TasklistHelper
     if subsection.pages.first.routed_to?(log, current_user)
       subsection.pages.first.id
     else
-      log.form.next_page(subsection.pages.first, log, current_user)
+      log.form.next_page_id(subsection.pages.first, log, current_user)
     end
   end
 

app/models/form.rb

@@ -61,30 +61,54 @@ class Form
     subsections.find { |s| s.pages.find { |p| p.id == page.id } }
   end
 
-  def next_page(page, log, current_user)
+  def next_page_id(page, log, current_user)
     return page.next_unresolved_page_id || :check_answers if log.unresolved
 
     page_ids = subsection_for_page(page).pages.map(&:id)
     page_index = page_ids.index(page.id)
     page_id = if page.interruption_screen? && log[page.questions[0].id] == 1 && page.routed_to?(log, current_user)
-                previous_page(page_ids, page_index, log, current_user)
+                previous_page_id(page, log, current_user)
               else
                 page_ids[page_index + 1]
               end
-    nxt_page = get_page(page_id)
+    next_page = get_page(page_id)
 
-    return :check_answers if nxt_page.nil?
+    return :check_answers if next_page.nil?
-    return nxt_page.id if nxt_page.routed_to?(log, current_user)
+    return next_page.id if next_page.routed_to?(log, current_user)
 
-    next_page(nxt_page, log, current_user)
+    next_page_id(next_page, log, current_user)
   end
 
   def next_page_redirect_path(page, log, current_user)
-    nxt_page = next_page(page, log, current_user)
+    next_page_id = next_page_id(page, log, current_user)
-    if nxt_page == :check_answers
+    if next_page_id == :check_answers
       "#{type}_log_#{subsection_for_page(page).id}_check_answers_path"
     else
-      "#{type}_log_#{nxt_page}_path"
+      "#{type}_log_#{next_page_id}_path"
+    end
+  end
+
+  def previous_page_id(page, log, current_user)
+    page_ids = subsection_for_page(page).pages.map(&:id)
+    page_index = page_ids.index(page.id)
+    return :tasklist if page_index.zero?
+
+    page_id = page_ids[page_index - 1]
+    previous_page = get_page(page_id)
+
+    return previous_page.id if previous_page.routed_to?(log, current_user)
+
+    previous_page_id(previous_page, log, current_user)
+  end
+
+  def previous_page_redirect_path(page, log, current_user, referrer)
+    previous_page_id = previous_page_id(page, log, current_user)
+    if referrer == "check_answers"
+      "#{type}_log_#{subsection_for_page(page).id}_check_answers_path"
+    elsif previous_page_id == :tasklist
+      "#{type}_log_path"
+    else
+      "#{type}_log_#{previous_page_id}_path"
     end
   end
 
@@ -206,13 +230,6 @@ class Form
     questions.select { |q| q.type == "numeric" }
   end
 
-  def previous_page(page_ids, page_index, log, current_user)
-    prev_page = get_page(page_ids[page_index - 1])
-    return prev_page.id if prev_page.routed_to?(log, current_user)
-
-    previous_page(page_ids, page_index - 1, log, current_user)
-  end
-
   def send_chain(arr, log)
     Array(arr).inject(log) { |o, a| o.public_send(*a) }
   end

app/models/validations/financial_validations.rb

@@ -138,7 +138,7 @@ module Validations::FinancialValidations
         message = I18n.t("validations.financial.carehome.out_of_range", period:, min_chcharge:, max_chcharge:)
 
         record.errors.add :period, message
-        record.errors.add :chcharge, :out_of_range, message: message
+        record.errors.add :chcharge, :out_of_range, message:
       end
     end
   end

app/views/form/page.html.erb

@@ -1,7 +1,7 @@
 <% content_for :title, @page.header.presence || @page.questions.first.header.html_safe %>
 
 <% content_for :before_content do %>
-  <%= govuk_back_link(href: "javascript:history.back()") %>
+  <%= govuk_back_link(href: send(@log.form.previous_page_redirect_path(@page, @log, current_user, params[:referrer]), @log)) %>
 <% end %>
 
 <div data-controller="govukfrontend"></div>

app/views/locations/availability.erb

@@ -2,7 +2,6 @@
 
 <% content_for :before_content do %>
   <%= govuk_back_link(
-        text: "Back",
         href: params[:referrer] == "check_answers" ? scheme_location_check_answers_path(@scheme, @location, route: params[:route]) : scheme_location_mobility_standards_path(@scheme, @location, route: params[:route]),
         ) %>
 <% end %>

app/views/locations/check_answers.html.erb

@@ -3,7 +3,6 @@
 
 <% content_for :before_content do %>
   <%= govuk_back_link(
-    text: "Back",
     href: case params[:route]
           when "locations"
             scheme_locations_path(@scheme)

app/views/locations/index.html.erb

@@ -4,7 +4,6 @@
 
 <% content_for :before_content do %>
   <%= govuk_back_link(
-    text: "Back",
     href: "/schemes/#{@scheme.id}",
   ) %>
 <% end %>

app/views/locations/local_authority.html.erb

@@ -2,7 +2,6 @@
 
 <% content_for :before_content do %>
   <%= govuk_back_link(
-        text: "Back",
         href: case params[:referrer]
               when "check_local_authority"
                 scheme_location_check_answers_path(@scheme, @location, route: params[:route])

app/views/locations/mobility_standards.html.erb

@@ -2,7 +2,6 @@
 
 <% content_for :before_content do %>
   <%= govuk_back_link(
-        text: "Back",
         href: params[:referrer] == "check_answers" ? scheme_location_check_answers_path(@scheme, @location, route: params[:route]) : scheme_location_type_of_unit_path(@scheme, @location, route: params[:route]),
       ) %>
 <% end %>

app/views/locations/name.html.erb

@@ -2,7 +2,6 @@
 
 <% content_for :before_content do %>
   <%= govuk_back_link(
-        text: "Back",
         href: case params[:referrer]
               when "check_answers"
                 scheme_location_check_answers_path(@scheme, @location, route: params[:route])

app/views/locations/postcode.html.erb

@@ -2,7 +2,6 @@
 
 <% content_for :before_content do %>
   <%= govuk_back_link(
-        text: "Back",
         href: if params[:referrer] == "check_answers"
                 scheme_location_check_answers_path(@scheme, @location, route: params[:route])
               else

app/views/locations/show.html.erb

@@ -3,7 +3,6 @@
 
 <% content_for :before_content do %>
   <%= govuk_back_link(
-    text: "Back",
     href: scheme_locations_path(@scheme),
   ) %>
 <% end %>

app/views/locations/toggle_active.html.erb

@@ -3,7 +3,6 @@
 
 <% content_for :before_content do %>
   <%= govuk_back_link(
-    text: "Back",
     href: scheme_location_path(@location.scheme, @location),
   ) %>
 <% end %>

app/views/locations/type_of_unit.html.erb

@@ -2,7 +2,6 @@
 
 <% content_for :before_content do %>
   <%= govuk_back_link(
-        text: "Back",
         href: params[:referrer] == "check_answers" ? scheme_location_check_answers_path(@scheme, @location, route: params[:route]) : scheme_location_units_path(@scheme, @location, route: params[:route]),
       ) %>
 <% end %>

app/views/locations/units.html.erb

@@ -2,7 +2,6 @@
 
 <% content_for :before_content do %>
   <%= govuk_back_link(
-        text: "Back",
         href: params[:referrer] == "check_answers" ? scheme_location_check_answers_path(@scheme, @location, route: params[:route]) : scheme_location_name_path(@scheme, @location, route: params[:route]),
       ) %>
 <% end %>

app/views/schemes/confirm_secondary.html.erb

@@ -2,7 +2,6 @@
 
 <% content_for :before_content do %>
   <%= govuk_back_link(
-    text: "Back",
     href: request.query_parameters["check_answers"] ? "/schemes/#{@scheme.id}/check-answers" : "/schemes/#{@scheme.id}/primary-client-group",
   ) %>
 <% end %>

app/views/schemes/details.html.erb

@@ -1,10 +1,7 @@
  <% content_for :title, "Create a new supported housing scheme" %>
 
 <% content_for :before_content do %>
-  <%= govuk_back_link(
+   <%= govuk_back_link(href: :back) %>
-        text: "Back",
-        href: :back,
-      ) %>
 <% end %>
 
 <%= form_for(@scheme, method: :patch) do |f| %>

app/views/schemes/edit_name.html.erb

@@ -1,10 +1,7 @@
 <% content_for :title, "Scheme details" %>
 
 <% content_for :before_content do %>
-  <%= govuk_back_link(
+  <%= govuk_back_link(href: :back) %>
-    text: "Back",
-    href: :back,
-  ) %>
 <% end %>
 
 <%= form_for(@scheme, method: :patch) do |f| %>

app/views/schemes/new.html.erb

@@ -6,10 +6,7 @@
       <% content_for :title, "Create a new supported housing scheme" %>
 
       <% content_for :before_content do %>
-        <%= govuk_back_link(
+        <%= govuk_back_link(href: "javascript:history.back()") %>
-              text: "Back",
-              href: "javascript:history.go(-1);",
-            ) %>
       <% end %>
 
       <h1 class="govuk-heading-l">

app/views/schemes/primary_client_group.html.erb

@@ -7,10 +7,7 @@
 <% end %>
 
 <% content_for :before_content do %>
-  <%= govuk_back_link(
+  <%= govuk_back_link(href: back_button_path) %>
-    text: "Back",
-    href: back_button_path,
-  ) %>
 <% end %>
 
 <%= form_for(@scheme, method: :patch) do |f| %>

app/views/schemes/secondary_client_group.html.erb

@@ -2,7 +2,6 @@
 
 <% content_for :before_content do %>
   <%= govuk_back_link(
-    text: "Back",
     href: request.query_parameters["check_answers"] ? "/schemes/#{@scheme.id}/check-answers" : "/schemes/#{@scheme.id}/confirm-secondary-client-group",
   ) %>
 <% end %>

app/views/schemes/show.html.erb

@@ -3,7 +3,6 @@
 
 <% content_for :before_content do %>
   <%= govuk_back_link(
-    text: "Back",
     href: "/schemes",
   ) %>
 <% end %>

app/views/schemes/support.html.erb

@@ -2,7 +2,6 @@
 
 <% content_for :before_content do %>
   <%= govuk_back_link(
-    text: "Back",
     href: request.query_parameters["check_answers"] ? "/schemes/#{@scheme.id}/check-answers" : "/schemes/#{@scheme.id}/secondary-client-group",
   ) %>
 <% end %>

app/views/schemes/toggle_active.html.erb

@@ -3,7 +3,6 @@
 
 <% content_for :before_content do %>
   <%= govuk_back_link(
-        text: "Back",
         href: scheme_details_path(@scheme),
       ) %>
 <% end %>

spec/features/form/form_navigation_spec.rb

@@ -110,7 +110,7 @@ RSpec.describe "Form Navigation" do
         click_button("Save and continue")
         click_link(text: "Back")
         click_link(text: "Back")
-        expect(page).to have_current_path("/lettings-logs")
+        expect(page).to have_current_path("/lettings-logs/#{id}")
       end
 
       context "when changing an answer from the check answers page", js: true do

spec/models/form_spec.rb

@@ -17,11 +17,11 @@ RSpec.describe Form, type: :model do
   let(:conditional_section_complete_lettings_log) { FactoryBot.build(:lettings_log, :conditional_section_complete) }
 
   describe ".next_page" do
-    let(:previous_page) { form.get_page("person_1_age") }
+    let(:previous_page_id) { form.get_page("person_1_age") }
     let(:value_check_previous_page) { form.get_page("net_income_value_check") }
 
     it "returns the next page given the previous" do
-      expect(form.next_page(previous_page, lettings_log, user)).to eq("person_1_gender")
+      expect(form.next_page_id(previous_page_id, lettings_log, user)).to eq("person_1_gender")
     end
 
     context "when the current page is a value check page" do
@@ -33,12 +33,12 @@ RSpec.describe Form, type: :model do
 
       it "returns the previous page if answer is `No` and the page is routed to" do
         lettings_log.net_income_value_check = 1
-        expect(form.next_page(value_check_previous_page, lettings_log, user)).to eq("net_income")
+        expect(form.next_page_id(value_check_previous_page, lettings_log, user)).to eq("net_income")
       end
 
       it "returns the next page if answer is `Yes` answer and the page is routed to" do
         lettings_log.net_income_value_check = 0
-        expect(form.next_page(value_check_previous_page, lettings_log, user)).to eq("net_income_uc_proportion")
+        expect(form.next_page_id(value_check_previous_page, lettings_log, user)).to eq("net_income_uc_proportion")
       end
     end
   end
@@ -46,31 +46,30 @@ RSpec.describe Form, type: :model do
   describe ".previous_page" do
     context "when the current page is not a value check page" do
       let!(:subsection) { form.get_subsection("conditional_question") }
-      let!(:page_ids) { subsection.pages.map(&:id) }
 
       before do
         lettings_log.preg_occ = 2
       end
 
       it "returns the previous page if the page is routed to" do
-        page_index = page_ids.index("conditional_question_no_second_page")
+        page = subsection.pages.find { |p| p.id == "conditional_question_no_second_page" }
-        expect(form.previous_page(page_ids, page_index, lettings_log, user)).to eq("conditional_question_no_page")
+        expect(form.previous_page_id(page, lettings_log, user)).to eq("conditional_question_no_page")
       end
 
       it "returns the page before the previous one if the previous page is not routed to" do
-        page_index = page_ids.index("conditional_question_no_page")
+        page = subsection.pages.find { |p| p.id == "conditional_question_no_page" }
-        expect(form.previous_page(page_ids, page_index, lettings_log, user)).to eq("conditional_question")
+        expect(form.previous_page_id(page, lettings_log, user)).to eq("conditional_question")
       end
     end
   end
 
   describe "next_page_redirect_path" do
-    let(:previous_page) { form.get_page("net_income") }
+    let(:previous_page_id) { form.get_page("net_income") }
     let(:last_previous_page) { form.get_page("housing_benefit") }
     let(:previous_conditional_page) { form.get_page("conditional_question") }
 
     it "returns a correct page path if there is no conditional routing" do
-      expect(form.next_page_redirect_path(previous_page, lettings_log, user)).to eq("lettings_log_net_income_uc_proportion_path")
+      expect(form.next_page_redirect_path(previous_page_id, lettings_log, user)).to eq("lettings_log_net_income_uc_proportion_path")
     end
 
     it "returns a check answers page if previous page is the last page" do