From efd83d4aa8cf100975c6756ccabe2807dcb69671 Mon Sep 17 00:00:00 2001
From: natdeanlewissoftwire
<94526761+natdeanlewissoftwire@users.noreply.github.com>
Date: Mon, 17 Apr 2023 17:16:47 +0100
Subject: [PATCH] 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 | 6 +--
app/helpers/tasklist_helper.rb | 2 +-
app/models/form.rb | 49 +++++++++++++------
.../validations/financial_validations.rb | 2 +-
app/views/form/page.html.erb | 2 +-
app/views/locations/availability.erb | 1 -
app/views/locations/check_answers.html.erb | 1 -
app/views/locations/index.html.erb | 1 -
app/views/locations/local_authority.html.erb | 1 -
.../locations/mobility_standards.html.erb | 1 -
app/views/locations/name.html.erb | 1 -
app/views/locations/postcode.html.erb | 1 -
app/views/locations/show.html.erb | 1 -
app/views/locations/toggle_active.html.erb | 1 -
app/views/locations/type_of_unit.html.erb | 1 -
app/views/locations/units.html.erb | 1 -
app/views/schemes/confirm_secondary.html.erb | 1 -
app/views/schemes/details.html.erb | 5 +-
app/views/schemes/edit_name.html.erb | 5 +-
app/views/schemes/new.html.erb | 5 +-
.../schemes/primary_client_group.html.erb | 5 +-
.../schemes/secondary_client_group.html.erb | 1 -
app/views/schemes/show.html.erb | 1 -
app/views/schemes/support.html.erb | 1 -
app/views/schemes/toggle_active.html.erb | 1 -
spec/features/form/form_navigation_spec.rb | 2 +-
spec/models/form_spec.rb | 21 ++++----
27 files changed, 53 insertions(+), 67 deletions(-)
diff --git a/app/controllers/form_controller.rb b/app/controllers/form_controller.rb
index c584e40ee..e7bbf38ec 100644
--- a/app/controllers/form_controller.rb
+++ b/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)
- page_index = page_ids.index(@page.id)
- next_page_id = form.next_page(@page, @log, current_user)
+ next_page_id = form.next_page_id(@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" })
diff --git a/app/helpers/tasklist_helper.rb b/app/helpers/tasklist_helper.rb
index 9487d13a5..6a12cff29 100644
--- a/app/helpers/tasklist_helper.rb
+++ b/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
diff --git a/app/models/form.rb b/app/models/form.rb
index 8f9dc47f9..80fed4444 100644
--- a/app/models/form.rb
+++ b/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 nxt_page.id if nxt_page.routed_to?(log, current_user)
+ return :check_answers if next_page.nil?
+ 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)
- if nxt_page == :check_answers
+ next_page_id = next_page_id(page, log, current_user)
+ 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
diff --git a/app/models/validations/financial_validations.rb b/app/models/validations/financial_validations.rb
index d328054e9..857c4e193 100644
--- a/app/models/validations/financial_validations.rb
+++ b/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
diff --git a/app/views/form/page.html.erb b/app/views/form/page.html.erb
index 45711b0f1..9b4d14d9d 100644
--- a/app/views/form/page.html.erb
+++ b/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 %>
diff --git a/app/views/locations/availability.erb b/app/views/locations/availability.erb
index 279c5110c..8a00ddc89 100644
--- a/app/views/locations/availability.erb
+++ b/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 %>
diff --git a/app/views/locations/check_answers.html.erb b/app/views/locations/check_answers.html.erb
index 11272b5a9..b460ca464 100644
--- a/app/views/locations/check_answers.html.erb
+++ b/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)
diff --git a/app/views/locations/index.html.erb b/app/views/locations/index.html.erb
index 469768b11..7294b9729 100644
--- a/app/views/locations/index.html.erb
+++ b/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 %>
diff --git a/app/views/locations/local_authority.html.erb b/app/views/locations/local_authority.html.erb
index 69b36b5be..347f85866 100644
--- a/app/views/locations/local_authority.html.erb
+++ b/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])
diff --git a/app/views/locations/mobility_standards.html.erb b/app/views/locations/mobility_standards.html.erb
index 42bdb4bcf..da08f8bfa 100644
--- a/app/views/locations/mobility_standards.html.erb
+++ b/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 %>
diff --git a/app/views/locations/name.html.erb b/app/views/locations/name.html.erb
index 322a1da22..e5853a71f 100644
--- a/app/views/locations/name.html.erb
+++ b/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])
diff --git a/app/views/locations/postcode.html.erb b/app/views/locations/postcode.html.erb
index 5ede463ad..fe0505926 100644
--- a/app/views/locations/postcode.html.erb
+++ b/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
diff --git a/app/views/locations/show.html.erb b/app/views/locations/show.html.erb
index 79876e5b8..299a19357 100644
--- a/app/views/locations/show.html.erb
+++ b/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 %>
diff --git a/app/views/locations/toggle_active.html.erb b/app/views/locations/toggle_active.html.erb
index 7b995b2b5..c7c8270b5 100644
--- a/app/views/locations/toggle_active.html.erb
+++ b/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 %>
diff --git a/app/views/locations/type_of_unit.html.erb b/app/views/locations/type_of_unit.html.erb
index 5ff4ed3f1..45b76ef67 100644
--- a/app/views/locations/type_of_unit.html.erb
+++ b/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 %>
diff --git a/app/views/locations/units.html.erb b/app/views/locations/units.html.erb
index 30a9b3eff..12365adbb 100644
--- a/app/views/locations/units.html.erb
+++ b/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 %>
diff --git a/app/views/schemes/confirm_secondary.html.erb b/app/views/schemes/confirm_secondary.html.erb
index c145e573d..e267b3289 100644
--- a/app/views/schemes/confirm_secondary.html.erb
+++ b/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 %>
diff --git a/app/views/schemes/details.html.erb b/app/views/schemes/details.html.erb
index 4d45213f3..0e4f3a7d9 100644
--- a/app/views/schemes/details.html.erb
+++ b/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(
- text: "Back",
- href: :back,
- ) %>
+ <%= govuk_back_link(href: :back) %>
<% end %>
<%= form_for(@scheme, method: :patch) do |f| %>
diff --git a/app/views/schemes/edit_name.html.erb b/app/views/schemes/edit_name.html.erb
index 04e28c93a..1dc8bc147 100644
--- a/app/views/schemes/edit_name.html.erb
+++ b/app/views/schemes/edit_name.html.erb
@@ -1,10 +1,7 @@
<% content_for :title, "Scheme details" %>
<% content_for :before_content do %>
- <%= govuk_back_link(
- text: "Back",
- href: :back,
- ) %>
+ <%= govuk_back_link(href: :back) %>
<% end %>
<%= form_for(@scheme, method: :patch) do |f| %>
diff --git a/app/views/schemes/new.html.erb b/app/views/schemes/new.html.erb
index ff4f4d3a9..81c5febbf 100644
--- a/app/views/schemes/new.html.erb
+++ b/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(
- text: "Back",
- href: "javascript:history.go(-1);",
- ) %>
+ <%= govuk_back_link(href: "javascript:history.back()") %>
<% end %>
diff --git a/app/views/schemes/primary_client_group.html.erb b/app/views/schemes/primary_client_group.html.erb
index 610e916f4..8fe852c89 100644
--- a/app/views/schemes/primary_client_group.html.erb
+++ b/app/views/schemes/primary_client_group.html.erb
@@ -7,10 +7,7 @@
<% end %>
<% content_for :before_content do %>
- <%= govuk_back_link(
- text: "Back",
- href: back_button_path,
- ) %>
+ <%= govuk_back_link(href: back_button_path) %>
<% end %>
<%= form_for(@scheme, method: :patch) do |f| %>
diff --git a/app/views/schemes/secondary_client_group.html.erb b/app/views/schemes/secondary_client_group.html.erb
index 4f48dab00..fe292b1c2 100644
--- a/app/views/schemes/secondary_client_group.html.erb
+++ b/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 %>
diff --git a/app/views/schemes/show.html.erb b/app/views/schemes/show.html.erb
index 5ef55f9f4..beb4508e0 100644
--- a/app/views/schemes/show.html.erb
+++ b/app/views/schemes/show.html.erb
@@ -3,7 +3,6 @@
<% content_for :before_content do %>
<%= govuk_back_link(
- text: "Back",
href: "/schemes",
) %>
<% end %>
diff --git a/app/views/schemes/support.html.erb b/app/views/schemes/support.html.erb
index 0c06a059f..23948978b 100644
--- a/app/views/schemes/support.html.erb
+++ b/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 %>
diff --git a/app/views/schemes/toggle_active.html.erb b/app/views/schemes/toggle_active.html.erb
index f2ecc4a1a..e989be995 100644
--- a/app/views/schemes/toggle_active.html.erb
+++ b/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 %>
diff --git a/spec/features/form/form_navigation_spec.rb b/spec/features/form/form_navigation_spec.rb
index aa0223616..6fdec8366 100644
--- a/spec/features/form/form_navigation_spec.rb
+++ b/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
diff --git a/spec/models/form_spec.rb b/spec/models/form_spec.rb
index 327a79a2f..f2a80efe3 100644
--- a/spec/models/form_spec.rb
+++ b/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")
- expect(form.previous_page(page_ids, page_index, lettings_log, user)).to eq("conditional_question_no_page")
+ page = subsection.pages.find { |p| p.id == "conditional_question_no_second_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")
- expect(form.previous_page(page_ids, page_index, lettings_log, user)).to eq("conditional_question")
+ page = subsection.pages.find { |p| p.id == "conditional_question_no_page" }
+ 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