From 3d674cf8ff1fbabf4ef193091ce12f1c0c989fd4 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Wed, 5 Feb 2025 15:24:44 +0200 Subject: [PATCH] Add warning when user is about to delete condition with secondary skip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently (when branching is enabled) deleting a skip route for a question that has a secondary skip route puts the form in a state where there is a route with an error that can’t be deleted. We do want users to be able to delete individual routes for a question. So we’ve agreed that we will delete the secondary skip for a question when deleting any other routes for that question. And we’ll let the user know before the delete a route, with a warning. This commit adds the warning; note that the deletion will take place in forms-api, see alphagov/forms-api#686. To enable this we add a method `#has_secondary_skip?` to the DeleteConfirmationInput object... I'm not sure if this is the best place for this, but it'll do for now. --- .../pages/delete_condition_input.rb | 14 ++++- app/views/pages/conditions/delete.html.erb | 4 ++ config/locales/en.yml | 3 ++ .../pages/delete_condition_input_spec.rb | 30 +++++++++++ .../pages/conditions_controller_spec.rb | 2 +- .../pages/conditions/delete.html.erb_spec.rb | 52 ++++++++++++++++++- 6 files changed, 102 insertions(+), 3 deletions(-) diff --git a/app/input_objects/pages/delete_condition_input.rb b/app/input_objects/pages/delete_condition_input.rb index a534c5b3a..11afbcf77 100644 --- a/app/input_objects/pages/delete_condition_input.rb +++ b/app/input_objects/pages/delete_condition_input.rb @@ -18,6 +18,18 @@ def submit def goto_page_question_text return I18n.t("page_conditions.check_your_answers") if goto_page_id.nil? && record.skip_to_end - FormRepository.pages(form).filter { |p| p.id == goto_page_id }.first&.question_text + pages.filter { |p| p.id == goto_page_id }.first&.question_text + end + + def has_secondary_skip? + check_page = pages.find(proc { raise "Cannot find page with id #{check_page_id}" }) { it.id == check_page_id } + page_conditions_service = PageConditionsService.new(form:, pages:, page: check_page) + page_conditions_service.check_conditions.any? { it != record && it.routing_page_id != it.check_page_id } + end + +private + + def pages + @pages ||= FormRepository.pages(form) end end diff --git a/app/views/pages/conditions/delete.html.erb b/app/views/pages/conditions/delete.html.erb index 0fd2f7120..1a6f4f6d6 100644 --- a/app/views/pages/conditions/delete.html.erb +++ b/app/views/pages/conditions/delete.html.erb @@ -6,6 +6,10 @@ <%= form_with(model: delete_condition_input, url: delete_condition_path(delete_condition_input.form.id, delete_condition_input.page.id, delete_condition_input.record.id), method: :delete) do |f| %> <% if delete_condition_input.errors.any? %> <%= f.govuk_error_summary %> + <% elsif delete_condition_input.has_secondary_skip? %> + <%= govuk_notification_banner(title_text: t("banner.default.title")) do |banner| %> + <% banner.with_heading(text: t(".any_other_answer_warning")) %> + <% end %> <% end %>

diff --git a/config/locales/en.yml b/config/locales/en.yml index c25d0d86b..f8bab7de5 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1148,6 +1148,9 @@ en: your_form_is_live: Your form is live pages: answer_settings: Answer settings + conditions: + delete: + any_other_answer_warning: If you delete this route, the route for any other answer will also be deleted delete: notification_banner: end_of_route: diff --git a/spec/input_objects/pages/delete_condition_input_spec.rb b/spec/input_objects/pages/delete_condition_input_spec.rb index 2cabc05a6..0cec8ddd4 100644 --- a/spec/input_objects/pages/delete_condition_input_spec.rb +++ b/spec/input_objects/pages/delete_condition_input_spec.rb @@ -68,4 +68,34 @@ end end end + + describe "#has_secondary_skip?" do + context "when the condition does not have a secondary skip condition" do + subject(:has_secondary_skip?) { delete_condition_input.has_secondary_skip? } + + it { is_expected.to be false } + end + + context "when the condition has a secondary skip condition" do + subject(:has_secondary_skip?) { delete_condition_input.has_secondary_skip? } + + let(:condition) do + condition = build :condition, id: 1, routing_page_id: start_of_branches.id, check_page_id: start_of_branches.id, answer_value: "Wales", goto_page_id: start_of_second_branch.id + start_of_branches.routing_conditions << condition + condition + end + + let(:start_of_branches) { pages.first } + let(:end_of_first_branch) { pages.second } + let(:start_of_second_branch) { pages.third } + let(:end_of_branches) { pages.last } + + before do + secondary_skip_condition = build :condition, id: 2, routing_page_id: end_of_first_branch.id, check_page_id: start_of_branches.id, answer_value: nil, goto_page_id: end_of_branches.id + end_of_first_branch.routing_conditions << secondary_skip_condition + end + + it { is_expected.to be true } + end + end end diff --git a/spec/requests/pages/conditions_controller_spec.rb b/spec/requests/pages/conditions_controller_spec.rb index 36b38946d..be2bcaea4 100644 --- a/spec/requests/pages/conditions_controller_spec.rb +++ b/spec/requests/pages/conditions_controller_spec.rb @@ -295,7 +295,7 @@ end describe "#delete" do - let(:condition) { build :condition, id: 1, routing_page_id: 1, check_page_id: 1, answer_value: "Wales", goto_page_id: 3 } + let(:condition) { build :condition, id: 1, routing_page_id: selected_page.id, check_page_id: selected_page.id, answer_value: "Wales", goto_page_id: 3 } before do selected_page.routing_conditions = [condition] diff --git a/spec/views/pages/conditions/delete.html.erb_spec.rb b/spec/views/pages/conditions/delete.html.erb_spec.rb index 02beeaff1..22f9f6525 100644 --- a/spec/views/pages/conditions/delete.html.erb_spec.rb +++ b/spec/views/pages/conditions/delete.html.erb_spec.rb @@ -3,12 +3,14 @@ describe "pages/conditions/delete.html.erb" do let(:delete_condition_input) { Pages::DeleteConditionInput.new(form:, page:, record: condition) } let(:form) { build :form, :ready_for_routing, id: 1 } - let(:condition) { build :condition, id: 1, routing_page_id: 1, check_page_id: 1, answer_value: "Wales", goto_page_id: pages.last.id } + let(:condition) { build :condition, id: 1, routing_page_id: pages.first.id, check_page_id: pages.first.id, answer_value: "Wales", goto_page_id: pages.last.id } + let(:secondary_skip_condition) { nil } let(:pages) { form.pages } let(:page) { pages.first } before do page.position = 1 + secondary_skip_condition render template: "pages/conditions/delete", locals: { delete_condition_input: } end @@ -43,4 +45,52 @@ expect(rendered).to have_css ".govuk-error-summary" end end + + context "when the condition does not have a secondary skip condition" do + let(:secondary_skip_condition) { nil } + + it "does not render a warning" do + expect(rendered).not_to have_css ".govuk-notification-banner" + end + end + + context "when the condition has a secondary skip condition" do + let(:condition) do + condition = build :condition, id: 1, routing_page_id: start_of_branches.id, check_page_id: start_of_branches.id, answer_value: "Wales", goto_page_id: start_of_second_branch.id + start_of_branches.routing_conditions << condition + condition + end + + let(:secondary_skip_condition) do + condition = build :condition, id: 2, routing_page_id: end_of_first_branch.id, check_page_id: start_of_branches.id, answer_value: nil, goto_page_id: end_of_branches.id + end_of_first_branch.routing_conditions << condition + condition + end + + let(:start_of_branches) { pages.first } + let(:end_of_first_branch) { pages.second } + let(:start_of_second_branch) { pages.third } + let(:end_of_branches) { pages.last } + + it "renders a warning" do + expect(rendered).to have_css ".govuk-notification-banner", text: "If you delete this route, the route for any other answer will also be deleted" + end + + context "when there is a validation error" do + let(:delete_condition_input) do + delete_condition_input = Pages::DeleteConditionInput.new(form:, page:, record: condition) + delete_condition_input.confirm = nil + delete_condition_input.validate + delete_condition_input + end + + it "renders an error summary" do + expect(rendered).to have_css ".govuk-error-summary" + end + + it "does not render a warning" do + expect(rendered).not_to have_css ".govuk-notification-banner", text: "If you delete this route, the route for any other answer will also be deleted" + end + end + end end