From 63547498070db18475d6dcadc99cca9826904993 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Wed, 26 Feb 2025 10:16:01 +0200 Subject: [PATCH 1/3] Update error messages for routing errors - Add route_number to available interpolations for conditions error messages translations - Update error message for route when end question is before the start question - Update error message for route answer value has been deleted or changed - Update error message for route where start and end are consecutive --- .../page_list_component/error_summary/view.rb | 4 +++- app/components/page_list_component/view.html.erb | 4 +++- config/locales/en.yml | 7 ++++--- .../page_list_component/error_summary/view_spec.rb | 14 +++++++------- spec/components/page_list_component/view_spec.rb | 8 ++++---- 5 files changed, 21 insertions(+), 16 deletions(-) diff --git a/app/components/page_list_component/error_summary/view.rb b/app/components/page_list_component/error_summary/view.rb index 450b6d011..d7f745884 100644 --- a/app/components/page_list_component/error_summary/view.rb +++ b/app/components/page_list_component/error_summary/view.rb @@ -11,7 +11,9 @@ def self.error_id(number) end def error_object(error_name:, condition:, page:) - OpenStruct.new(message: I18n.t("page_conditions.errors.#{error_name}", question_number: page.position), link: "##{self.class.error_id(condition.id)}") + # TODO: route_number is hardcoded as 1 here because we know there can be only two conditions. It will need to change in future + route_number = condition.secondary_skip? ? I18n.t("page_conditions.errors.route_number_for_any_other_answer") : 1 + OpenStruct.new(message: I18n.t("page_conditions.errors.#{error_name}", question_number: page.position, route_number:), link: "##{self.class.error_id(condition.id)}") end def conditions_with_check_pages diff --git a/app/components/page_list_component/view.html.erb b/app/components/page_list_component/view.html.erb index 1e0013c00..04af4b17a 100644 --- a/app/components/page_list_component/view.html.erb +++ b/app/components/page_list_component/view.html.erb @@ -43,7 +43,9 @@ diff --git a/config/locales/en.yml b/config/locales/en.yml index 2e44c0556..4fd85ac8d 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1023,10 +1023,11 @@ en: condition_goto_page_text_with_errors: "[Question not selected]" condition_name: Question %{question_number}’s routes errors: - answer_value_doesnt_exist: The answer that question %{question_number}’s route is based on no longer exists - edit question %{question_number}’s route - cannot_have_goto_page_before_routing_page: The question you’re taking the person to is now above the route - edit question %{question_number}’s route - cannot_route_to_next_page: Question %{question_number}’s route is now next to the route’s end question - this makes the route unnecessary. Edit question %{question_number}’s route. + answer_value_doesnt_exist: The answer that question %{question_number}’s route %{route_number} is based on no longer exists - edit or delete this route + cannot_have_goto_page_before_routing_page: Question %{question_number}’s route %{route_number} is going to a question that comes before the start of the route - edit or delete this route + cannot_route_to_next_page: Question %{question_number}’s route %{route_number} is not skipping any questions - edit or delete this route goto_page_doesnt_exist: The question you’re taking the person to no longer exists - edit question %{question_number}’s route + route_number_for_any_other_answer: for any other answer none_of_the_above: None of the above route: Route secondary_skip_description: After %{check_page_question_text} go to %{goto_page_question_text} diff --git a/spec/components/page_list_component/error_summary/view_spec.rb b/spec/components/page_list_component/error_summary/view_spec.rb index 3e7d4f325..de5678cc2 100644 --- a/spec/components/page_list_component/error_summary/view_spec.rb +++ b/spec/components/page_list_component/error_summary/view_spec.rb @@ -48,7 +48,7 @@ end it "renders the error link" do - condition_answer_value_error = I18n.t("page_conditions.errors.answer_value_doesnt_exist", question_number: 1) + condition_answer_value_error = I18n.t("page_conditions.errors.answer_value_doesnt_exist", question_number: 1, route_number: 1) expect(page).to have_link(condition_answer_value_error, href: "##{described_class.error_id(routing_conditions[0].id)}") end end @@ -71,8 +71,8 @@ end it "renders both error links" do - condition_answer_value_error = I18n.t("page_conditions.errors.answer_value_doesnt_exist", question_number: 1) - condition_goto_page_error = I18n.t("page_conditions.errors.goto_page_doesnt_exist", question_number: 2) + condition_answer_value_error = I18n.t("page_conditions.errors.answer_value_doesnt_exist", question_number: 1, route_number: 1) + condition_goto_page_error = I18n.t("page_conditions.errors.goto_page_doesnt_exist", question_number: 2, route_number: 1) expect(page).to have_link(condition_answer_value_error, href: "##{described_class.error_id(routing_conditions_page_with_answer_value_missing[0].id)}") expect(page).to have_link(condition_goto_page_error, href: "##{described_class.error_id(routing_conditions_page_with_goto_page_missing[0].id)}") end @@ -90,7 +90,7 @@ end it "renders the error summary" do - error_message = I18n.t("page_conditions.errors.cannot_route_to_next_page", question_number: 2) + error_message = I18n.t("page_conditions.errors.cannot_route_to_next_page", question_number: 2, route_number: "for any other answer") expect(page).to have_css ".govuk-error-summary", text: error_message expect(page).to have_link error_message, href: "#condition_#{branch_any_other_answer_route.id}" end @@ -121,7 +121,7 @@ it "returns an error object in the correct format" do condition = build :condition, id: 1 page = build :page, position: 1 - expect(error_summary_component.error_object(error_name: "answer_value_doesnt_exist", condition:, page:)).to eq OpenStruct.new(message: I18n.t("page_conditions.errors.answer_value_doesnt_exist", question_number: 1), link: "##{described_class.error_id(1)}") + expect(error_summary_component.error_object(error_name: "answer_value_doesnt_exist", condition:, page:)).to eq OpenStruct.new(message: I18n.t("page_conditions.errors.answer_value_doesnt_exist", question_number: 1, route_number: 1), link: "##{described_class.error_id(1)}") end end @@ -149,8 +149,8 @@ describe "#errors_for_summary" do it "returns all of the routing errors for a form with their respective positions and links" do expect(error_summary_component.errors_for_summary).to eq [ - OpenStruct.new(message: I18n.t("page_conditions.errors.answer_value_doesnt_exist", question_number: 1), link: "##{described_class.error_id(1)}"), - OpenStruct.new(message: I18n.t("page_conditions.errors.goto_page_doesnt_exist", question_number: 2), link: "##{described_class.error_id(2)}"), + OpenStruct.new(message: I18n.t("page_conditions.errors.answer_value_doesnt_exist", question_number: 1, route_number: 1), link: "##{described_class.error_id(1)}"), + OpenStruct.new(message: I18n.t("page_conditions.errors.goto_page_doesnt_exist", question_number: 2, route_number: 1), link: "##{described_class.error_id(2)}"), ] end end diff --git a/spec/components/page_list_component/view_spec.rb b/spec/components/page_list_component/view_spec.rb index b23f37f7b..9819667a5 100644 --- a/spec/components/page_list_component/view_spec.rb +++ b/spec/components/page_list_component/view_spec.rb @@ -106,7 +106,7 @@ let(:routing_conditions) { [(build :condition, :with_answer_value_missing, id: 1, routing_page_id: 1, check_page_id: 1, goto_page_id: 3)] } it "renders the errors in an unordered list" do - condition_answer_value_error = I18n.t("page_conditions.errors.answer_value_doesnt_exist", question_number: 1) + condition_answer_value_error = I18n.t("page_conditions.errors.answer_value_doesnt_exist", question_number: 1, route_number: 1) expect(page).to have_css("ul > li", text: condition_answer_value_error) end @@ -144,8 +144,8 @@ let(:routing_conditions) { [(build :condition, :with_answer_value_and_goto_page_missing, id: 1, routing_page_id: 1, check_page_id: 1)] } it "renders the errors in an unordered list" do - condition_answer_value_error = I18n.t("page_conditions.errors.answer_value_doesnt_exist", question_number: 1) - condition_goto_page_error = I18n.t("page_conditions.errors.goto_page_doesnt_exist", question_number: 1) + condition_answer_value_error = I18n.t("page_conditions.errors.answer_value_doesnt_exist", question_number: 1, route_number: 1) + condition_goto_page_error = I18n.t("page_conditions.errors.goto_page_doesnt_exist", question_number: 1, route_number: 1) expect(page).to have_css("ul > li", text: condition_answer_value_error) expect(page).to have_css("ul > li", text: condition_goto_page_error) end @@ -176,7 +176,7 @@ end it "renders an error message" do - error_message = I18n.t("page_conditions.errors.cannot_route_to_next_page", question_number: 2) + error_message = I18n.t("page_conditions.errors.cannot_route_to_next_page", question_number: 2, route_number: "for any other answer") expect(page).to have_css ".app-page_list__route-text--error", text: error_message end end From a651309b6c4b1a6ade0a68b3af52322a631bbd62 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Thu, 27 Feb 2025 16:01:12 +0200 Subject: [PATCH 2/3] Refactor generating routing error messages for page list component --- .../page_list_component/error_summary/view.rb | 11 +++++++++-- app/components/page_list_component/view.html.erb | 5 ++--- app/components/page_list_component/view.rb | 6 +++++- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/app/components/page_list_component/error_summary/view.rb b/app/components/page_list_component/error_summary/view.rb index d7f745884..b2d5c20f1 100644 --- a/app/components/page_list_component/error_summary/view.rb +++ b/app/components/page_list_component/error_summary/view.rb @@ -10,10 +10,17 @@ def self.error_id(number) "condition_#{number}" end - def error_object(error_name:, condition:, page:) + def self.generate_error_message(error_name, condition:, page:) # TODO: route_number is hardcoded as 1 here because we know there can be only two conditions. It will need to change in future route_number = condition.secondary_skip? ? I18n.t("page_conditions.errors.route_number_for_any_other_answer") : 1 - OpenStruct.new(message: I18n.t("page_conditions.errors.#{error_name}", question_number: page.position, route_number:), link: "##{self.class.error_id(condition.id)}") + I18n.t("page_conditions.errors.#{error_name}", question_number: page.position, route_number:) + end + + def error_object(error_name:, condition:, page:) + OpenStruct.new( + message: self.class.generate_error_message(error_name, condition:, page:), + link: "##{self.class.error_id(condition.id)}", + ) end def conditions_with_check_pages diff --git a/app/components/page_list_component/view.html.erb b/app/components/page_list_component/view.html.erb index 04af4b17a..9cdccdd8d 100644 --- a/app/components/page_list_component/view.html.erb +++ b/app/components/page_list_component/view.html.erb @@ -33,6 +33,7 @@ <% page.routing_conditions.each do |condition| %> + <% condition_page = self.condition_page(condition) %> <% condition_page_position = self.condition_page_position(condition) %>
">
@@ -43,9 +44,7 @@
    <% condition.validation_errors.each do |error| %>
  • - <%# TODO: route_number is hardcoded as 1 here because we know there can be only two conditions. It will need to change in future %> - <% route_number = condition.secondary_skip? ? t("page_conditions.errors.route_number_for_any_other_answer") : 1 %> - <%= t("page_conditions.errors.#{error.name}", question_number: condition_page_position, route_number:) %> + <%= PageListComponent::ErrorSummary::View.generate_error_message(error.name, condition:, page: condition_page) %>
    • <% end %>
diff --git a/app/components/page_list_component/view.rb b/app/components/page_list_component/view.rb index b6f08ad17..8a1dbbca0 100644 --- a/app/components/page_list_component/view.rb +++ b/app/components/page_list_component/view.rb @@ -57,8 +57,12 @@ def page_position(page) page.position end + def condition_page(condition) + condition.attributes["check_page"] ||= @pages.find { |page| page.id == condition.check_page_id } + end + def condition_page_position(condition) - check_page = @pages.find { |page| page.id == condition.check_page_id } + check_page = condition_page(condition) page_position(check_page) end From 4d6d527e6b5b171415d3f62986a43185da412419 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Thu, 27 Feb 2025 18:00:00 +0200 Subject: [PATCH 3/3] Change the cannot route to next page error message for any other answer route specifically We want to use different content for the error when a route has its goto page before the page it is routing from if the route is an any other answer route. This commit adds the new content and tweaks the logic of the code to generate the error messages for routing warnings to first look for a specific any other answer error message before using the more generic error message. This lets make other error message more specific in future more easily, while keeping duplication to a minimum. We also move the errors messages to the `errors.*` scope, so that i18n-tasks doesn't complain about unused keys. --- .../page_list_component/error_summary/view.rb | 11 +++++-- config/locales/en.yml | 14 +++++---- .../error_summary/view_spec.rb | 29 ++++++++++++++----- .../page_list_component/view_spec.rb | 24 +++++++++++---- 4 files changed, 58 insertions(+), 20 deletions(-) diff --git a/app/components/page_list_component/error_summary/view.rb b/app/components/page_list_component/error_summary/view.rb index b2d5c20f1..290187f7f 100644 --- a/app/components/page_list_component/error_summary/view.rb +++ b/app/components/page_list_component/error_summary/view.rb @@ -12,8 +12,15 @@ def self.error_id(number) def self.generate_error_message(error_name, condition:, page:) # TODO: route_number is hardcoded as 1 here because we know there can be only two conditions. It will need to change in future - route_number = condition.secondary_skip? ? I18n.t("page_conditions.errors.route_number_for_any_other_answer") : 1 - I18n.t("page_conditions.errors.#{error_name}", question_number: page.position, route_number:) + route_number = condition.secondary_skip? ? I18n.t("errors.page_conditions.route_number_for_any_other_answer") : 1 + + interpolation_variables = { question_number: page.position, route_number: } + + scope = "errors.page_conditions" + defaults = [:"#{error_name}"] + defaults.prepend(:"any_other_answer_route.#{error_name}") if condition.secondary_skip? + + I18n.t(defaults.first, default: defaults.drop(1), scope:, **interpolation_variables) end def error_object(error_name:, condition:, page:) diff --git a/config/locales/en.yml b/config/locales/en.yml index 4fd85ac8d..3c4daa701 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -292,6 +292,14 @@ en: errors: messages: non_government_email: Enter a government email address + page_conditions: + answer_value_doesnt_exist: The answer that question %{question_number}’s route %{route_number} is based on no longer exists - edit or delete this route + any_other_answer_route: + cannot_have_goto_page_before_routing_page: Question %{question_number}’s route for any other answer is going to a question that comes before the question it skips from - edit or delete this route + cannot_have_goto_page_before_routing_page: Question %{question_number}’s route %{route_number} is going to a question that comes before the start of the route - edit or delete this route + cannot_route_to_next_page: Question %{question_number}’s route %{route_number} is not skipping any questions - edit or delete this route + goto_page_doesnt_exist: The question you’re taking the person to no longer exists - edit question %{question_number}’s route + route_number_for_any_other_answer: for any other answer prefix: 'Error:' footer: accessibility_statement: Accessibility statement @@ -1022,12 +1030,6 @@ en: condition_goto_page_text: "%{goto_page_question_number}, “%{goto_page_question_text}”" condition_goto_page_text_with_errors: "[Question not selected]" condition_name: Question %{question_number}’s routes - errors: - answer_value_doesnt_exist: The answer that question %{question_number}’s route %{route_number} is based on no longer exists - edit or delete this route - cannot_have_goto_page_before_routing_page: Question %{question_number}’s route %{route_number} is going to a question that comes before the start of the route - edit or delete this route - cannot_route_to_next_page: Question %{question_number}’s route %{route_number} is not skipping any questions - edit or delete this route - goto_page_doesnt_exist: The question you’re taking the person to no longer exists - edit question %{question_number}’s route - route_number_for_any_other_answer: for any other answer none_of_the_above: None of the above route: Route secondary_skip_description: After %{check_page_question_text} go to %{goto_page_question_text} diff --git a/spec/components/page_list_component/error_summary/view_spec.rb b/spec/components/page_list_component/error_summary/view_spec.rb index de5678cc2..0775c9b8c 100644 --- a/spec/components/page_list_component/error_summary/view_spec.rb +++ b/spec/components/page_list_component/error_summary/view_spec.rb @@ -48,7 +48,7 @@ end it "renders the error link" do - condition_answer_value_error = I18n.t("page_conditions.errors.answer_value_doesnt_exist", question_number: 1, route_number: 1) + condition_answer_value_error = I18n.t("errors.page_conditions.answer_value_doesnt_exist", question_number: 1, route_number: 1) expect(page).to have_link(condition_answer_value_error, href: "##{described_class.error_id(routing_conditions[0].id)}") end end @@ -71,8 +71,8 @@ end it "renders both error links" do - condition_answer_value_error = I18n.t("page_conditions.errors.answer_value_doesnt_exist", question_number: 1, route_number: 1) - condition_goto_page_error = I18n.t("page_conditions.errors.goto_page_doesnt_exist", question_number: 2, route_number: 1) + condition_answer_value_error = I18n.t("errors.page_conditions.answer_value_doesnt_exist", question_number: 1, route_number: 1) + condition_goto_page_error = I18n.t("errors.page_conditions.goto_page_doesnt_exist", question_number: 2, route_number: 1) expect(page).to have_link(condition_answer_value_error, href: "##{described_class.error_id(routing_conditions_page_with_answer_value_missing[0].id)}") expect(page).to have_link(condition_goto_page_error, href: "##{described_class.error_id(routing_conditions_page_with_goto_page_missing[0].id)}") end @@ -90,7 +90,22 @@ end it "renders the error summary" do - error_message = I18n.t("page_conditions.errors.cannot_route_to_next_page", question_number: 2, route_number: "for any other answer") + error_message = I18n.t("errors.page_conditions.cannot_route_to_next_page", question_number: 2, route_number: "for any other answer") + expect(page).to have_css ".govuk-error-summary", text: error_message + expect(page).to have_link error_message, href: "#condition_#{branch_any_other_answer_route.id}" + end + end + + context "and the any other answer route skip to question has been moved to before the skip from question" do + before do + branch_any_other_answer_route.has_routing_errors = true + branch_any_other_answer_route.validation_errors = [OpenStruct.new(name: "cannot_have_goto_page_before_routing_page")] + + render_inline(error_summary_component) + end + + it "renders an error message" do + error_message = I18n.t("errors.page_conditions.any_other_answer_route.cannot_have_goto_page_before_routing_page", question_number: 2, route_number: "for any other answer") expect(page).to have_css ".govuk-error-summary", text: error_message expect(page).to have_link error_message, href: "#condition_#{branch_any_other_answer_route.id}" end @@ -121,7 +136,7 @@ it "returns an error object in the correct format" do condition = build :condition, id: 1 page = build :page, position: 1 - expect(error_summary_component.error_object(error_name: "answer_value_doesnt_exist", condition:, page:)).to eq OpenStruct.new(message: I18n.t("page_conditions.errors.answer_value_doesnt_exist", question_number: 1, route_number: 1), link: "##{described_class.error_id(1)}") + expect(error_summary_component.error_object(error_name: "answer_value_doesnt_exist", condition:, page:)).to eq OpenStruct.new(message: I18n.t("errors.page_conditions.answer_value_doesnt_exist", question_number: 1, route_number: 1), link: "##{described_class.error_id(1)}") end end @@ -149,8 +164,8 @@ describe "#errors_for_summary" do it "returns all of the routing errors for a form with their respective positions and links" do expect(error_summary_component.errors_for_summary).to eq [ - OpenStruct.new(message: I18n.t("page_conditions.errors.answer_value_doesnt_exist", question_number: 1, route_number: 1), link: "##{described_class.error_id(1)}"), - OpenStruct.new(message: I18n.t("page_conditions.errors.goto_page_doesnt_exist", question_number: 2, route_number: 1), link: "##{described_class.error_id(2)}"), + OpenStruct.new(message: I18n.t("errors.page_conditions.answer_value_doesnt_exist", question_number: 1, route_number: 1), link: "##{described_class.error_id(1)}"), + OpenStruct.new(message: I18n.t("errors.page_conditions.goto_page_doesnt_exist", question_number: 2, route_number: 1), link: "##{described_class.error_id(2)}"), ] end end diff --git a/spec/components/page_list_component/view_spec.rb b/spec/components/page_list_component/view_spec.rb index 9819667a5..b8e35d149 100644 --- a/spec/components/page_list_component/view_spec.rb +++ b/spec/components/page_list_component/view_spec.rb @@ -106,7 +106,7 @@ let(:routing_conditions) { [(build :condition, :with_answer_value_missing, id: 1, routing_page_id: 1, check_page_id: 1, goto_page_id: 3)] } it "renders the errors in an unordered list" do - condition_answer_value_error = I18n.t("page_conditions.errors.answer_value_doesnt_exist", question_number: 1, route_number: 1) + condition_answer_value_error = I18n.t("errors.page_conditions.answer_value_doesnt_exist", question_number: 1, route_number: 1) expect(page).to have_css("ul > li", text: condition_answer_value_error) end @@ -125,7 +125,7 @@ let(:routing_conditions) { [(build :condition, :with_goto_page_missing, id: 1, routing_page_id: 1, check_page_id: 1, answer_value: "Wales")] } it "renders the errors in an unordered list" do - condition_goto_page_error = I18n.t("page_conditions.errors.goto_page_doesnt_exist", question_number: 1) + condition_goto_page_error = I18n.t("errors.page_conditions.goto_page_doesnt_exist", question_number: 1) expect(page).to have_css("ul > li", text: condition_goto_page_error) end @@ -144,8 +144,8 @@ let(:routing_conditions) { [(build :condition, :with_answer_value_and_goto_page_missing, id: 1, routing_page_id: 1, check_page_id: 1)] } it "renders the errors in an unordered list" do - condition_answer_value_error = I18n.t("page_conditions.errors.answer_value_doesnt_exist", question_number: 1, route_number: 1) - condition_goto_page_error = I18n.t("page_conditions.errors.goto_page_doesnt_exist", question_number: 1, route_number: 1) + condition_answer_value_error = I18n.t("errors.page_conditions.answer_value_doesnt_exist", question_number: 1, route_number: 1) + condition_goto_page_error = I18n.t("errors.page_conditions.goto_page_doesnt_exist", question_number: 1, route_number: 1) expect(page).to have_css("ul > li", text: condition_answer_value_error) expect(page).to have_css("ul > li", text: condition_goto_page_error) end @@ -176,7 +176,21 @@ end it "renders an error message" do - error_message = I18n.t("page_conditions.errors.cannot_route_to_next_page", question_number: 2, route_number: "for any other answer") + error_message = I18n.t("errors.page_conditions.cannot_route_to_next_page", question_number: 2, route_number: "for any other answer") + expect(page).to have_css ".app-page_list__route-text--error", text: error_message + end + end + + context "and the any other answer route skip to question has been moved to before the skip from question" do + before do + branch_any_other_answer_route.has_routing_errors = true + branch_any_other_answer_route.validation_errors = [OpenStruct.new(name: "cannot_have_goto_page_before_routing_page")] + + render_inline(page_list_component) + end + + it "renders an error message" do + error_message = I18n.t("errors.page_conditions.any_other_answer_route.cannot_have_goto_page_before_routing_page", question_number: 2, route_number: "for any other answer") expect(page).to have_css ".app-page_list__route-text--error", text: error_message end end