From 312d0ec048025ab82cd2617bf3fc7a0e3fcad783 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Tue, 10 Dec 2024 12:57:29 +0200 Subject: [PATCH 1/8] Refactor specs to use factories --- spec/requests/pages_controller_spec.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/spec/requests/pages_controller_spec.rb b/spec/requests/pages_controller_spec.rb index 8335caaa3..c83e3126f 100644 --- a/spec/requests/pages_controller_spec.rb +++ b/spec/requests/pages_controller_spec.rb @@ -82,7 +82,8 @@ describe "#delete" do describe "given a valid page" do let(:page) do - Page.new({ + build( + :page, id: 1, form_id: 2, question_text: "What is your work address?", @@ -90,7 +91,7 @@ answer_type: "address", next_page: nil, is_optional: false, - }) + ) end before do From 59b2fad5d88dbd38604fd35ce6ac9bcbaa478063 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Wed, 15 Jan 2025 13:32:33 +0000 Subject: [PATCH 2/8] Change mock form response to be a persisted ActiveResource object If the form resource object is not persisted then ActiveResource won't make an API request if `form.pages` is accessed. --- spec/requests/pages_controller_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/requests/pages_controller_spec.rb b/spec/requests/pages_controller_spec.rb index c83e3126f..fbd3eb7f4 100644 --- a/spec/requests/pages_controller_spec.rb +++ b/spec/requests/pages_controller_spec.rb @@ -1,7 +1,7 @@ require "rails_helper" RSpec.describe PagesController, type: :request do - let(:form_response) { build :form, id: 2 } + let(:form_response) { Form.new(attributes_for(:form, id: 2), true) } let(:group) { create(:group, organisation: standard_user.organisation) } let(:membership) { create :membership, group:, user: standard_user } From 7357c19f0dffd9ce1fd1a3f4b6329b593ca92163 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Tue, 10 Dec 2024 12:58:25 +0200 Subject: [PATCH 3/8] Add warning for deleting page at start of one or more routes Co-authored-by: hannahkc --- app/controllers/pages_controller.rb | 4 ++- app/views/pages/delete.html.erb | 9 ++++++ config/locales/en.yml | 8 ++++++ spec/requests/pages_controller_spec.rb | 35 +++++++++++++++++++++++- spec/views/pages/delete.html.erb_spec.rb | 34 +++++++++++++++++++++-- 5 files changed, 85 insertions(+), 5 deletions(-) diff --git a/app/controllers/pages_controller.rb b/app/controllers/pages_controller.rb index b222efd56..4799999b5 100644 --- a/app/controllers/pages_controller.rb +++ b/app/controllers/pages_controller.rb @@ -17,6 +17,8 @@ def delete @back_url = edit_question_path(current_form.id, @page.id) @delete_confirmation_input = Forms::DeleteConfirmationInput.new + + render locals: { current_form: } end def destroy @@ -35,7 +37,7 @@ def destroy redirect_to @back_url end else - render :delete + render :delete, locals: { current_form: } end rescue StandardError flash[:message] = "Deletion unsuccessful" diff --git a/app/views/pages/delete.html.erb b/app/views/pages/delete.html.erb index dd7f38a04..f320b5d2a 100644 --- a/app/views/pages/delete.html.erb +++ b/app/views/pages/delete.html.erb @@ -3,6 +3,15 @@
+ + <% if @page.routing_conditions.present? %> + <%= govuk_notification_banner(title_text: "Important") do |banner| + banner.with_heading(text: t(".notification_banner.start_of_route.heading_text", routing_page_position: @page.position), tag: :h3) + + t(".notification_banner.start_of_route.html", routing_page_position: @page.position, routing_page_show_routes_href: show_routes_path(current_form.id, @page.id)) + end %> + <% end %> + <%= render( @delete_confirmation_input, url: @url, diff --git a/config/locales/en.yml b/config/locales/en.yml index 6196a59f5..c94072fad 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1149,6 +1149,14 @@ en: pages: answer_settings: Answer settings delete: + notification_banner: + start_of_route: + heading_text: Question %{routing_page_position} is the start of a route + html: | +

+ If you delete this question, its routes will also be deleted. + View question %{routing_page_position}’s routes. +

title: Are you sure you want to delete this question? delete_question: Delete question go_to_your_questions: Back to your questions diff --git a/spec/requests/pages_controller_spec.rb b/spec/requests/pages_controller_spec.rb index fbd3eb7f4..736a0a6f9 100644 --- a/spec/requests/pages_controller_spec.rb +++ b/spec/requests/pages_controller_spec.rb @@ -100,7 +100,7 @@ GroupForm.create!(form_id: 2, group_id: group.id) - get delete_page_path(form_id: 2, page_id: 1) + get delete_page_path(form_id: 2, page_id: page.id) end it "renders the delete page template" do @@ -118,6 +118,39 @@ expect(response).to have_http_status :forbidden end end + + context "when page to delete has no routes" do + it "does not render a warning" do + expect(response.body).not_to include "Important" + end + end + + context "when page to delete is start of one or more routes" do + let(:page) do + build( + :page, + :with_selection_settings, + id: 1, + form_id: 2, + question_text: "What is your favourite colour?", + selection_options: [{ name: "Red" }, { name: "Green" }, { name: "Blue" }], + only_one_option: true, + routing_conditions: [ + build(:condition, routing_page_id: 1, check_page_id: 1, value: "red", skip_to_end: true), + build(:condition, routing_page_id: 1, check_page_id: 1, value: "green", goto_page_id: 3), + ], + ) + end + + it "renders a warning about deleting this page" do + assert_select(".govuk-notification-banner", count: 1) do + assert_select "*", "Important" + assert_select "h3", "Question #{page.position} is the start of a route" + assert_select "p.govuk-body", /If you delete this question, its routes will also be deleted/ + assert_select "p.govuk-body a", "View question #{page.position}’s routes" + end + end + end end end diff --git a/spec/views/pages/delete.html.erb_spec.rb b/spec/views/pages/delete.html.erb_spec.rb index e05b32e61..824cbd6f3 100644 --- a/spec/views/pages/delete.html.erb_spec.rb +++ b/spec/views/pages/delete.html.erb_spec.rb @@ -1,13 +1,18 @@ require "rails_helper" RSpec.describe "pages/delete" do + let(:page) { build :page, id: 1 } + before do - assign(:back_url, edit_question_path(form_id: 1, page_id: 1)) + assign(:back_url, edit_question_path(form_id: 1, page_id: page.id)) assign(:delete_confirmation_input, Forms::DeleteConfirmationInput.new) assign(:item_name, "What’s your name?") - assign(:url, destroy_page_path(form_id: 1, page_id: 1)) + assign(:page, page) + assign(:url, destroy_page_path(form_id: 1, page_id: page.id)) + + current_form = build :form, id: 1 - render + render locals: { current_form: } end it "has a page title" do @@ -39,4 +44,27 @@ expect(rendered).not_to have_css ".govuk-hint" end end + + describe "when page to delete is not associated with any routes" do + it "does not render a notification banner" do + expect(rendered).not_to have_css ".govuk-notification-banner" + end + end + + describe "when page to delete is the start of one or more routes" do + let(:page) { build :page, id: 1, form_id: 1, position: 2, routing_conditions: true } + + it "renders a notification banner" do + expect(rendered).to have_css ".govuk-notification-banner" + end + + describe "notification banner" do + subject(:banner) { rendered.html.at_css(".govuk-notification-banner") } + + it { is_expected.to have_text "Important" } + it { is_expected.to have_css "h3.govuk-notification-banner__heading", text: "Question 2 is the start of a route", count: 1 } + it { is_expected.to have_css "p.govuk-body", text: "If you delete this question, its routes will also be deleted." } + it { is_expected.to have_link "View question 2’s routes", class: "govuk-notification-banner__link", href: show_routes_path(1, 1) } + end + end end From 62357d9a61951c334d84762c66a740599b136f4b Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Tue, 10 Dec 2024 13:08:12 +0200 Subject: [PATCH 4/8] Stop notification banner from showing when there is an error summary Design System Guidance says don't show both notification banner and error summary [[1]]. [1]: https://design-system.service.gov.uk/components/notification-banner#when-not-to-use-this-component --- app/views/pages/delete.html.erb | 12 +++++++----- spec/views/pages/delete.html.erb_spec.rb | 15 ++++++++++++++- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/app/views/pages/delete.html.erb b/app/views/pages/delete.html.erb index f320b5d2a..7bc88e4fe 100644 --- a/app/views/pages/delete.html.erb +++ b/app/views/pages/delete.html.erb @@ -4,12 +4,14 @@
- <% if @page.routing_conditions.present? %> - <%= govuk_notification_banner(title_text: "Important") do |banner| - banner.with_heading(text: t(".notification_banner.start_of_route.heading_text", routing_page_position: @page.position), tag: :h3) + <% unless @delete_confirmation_input.errors.any? %> + <% if @page.routing_conditions.present? %> + <%= govuk_notification_banner(title_text: "Important") do |banner| + banner.with_heading(text: t(".notification_banner.start_of_route.heading_text", routing_page_position: @page.position), tag: :h3) - t(".notification_banner.start_of_route.html", routing_page_position: @page.position, routing_page_show_routes_href: show_routes_path(current_form.id, @page.id)) - end %> + t(".notification_banner.start_of_route.html", routing_page_position: @page.position, routing_page_show_routes_href: show_routes_path(current_form.id, @page.id)) + end %> + <% end %> <% end %> <%= render( diff --git a/spec/views/pages/delete.html.erb_spec.rb b/spec/views/pages/delete.html.erb_spec.rb index 824cbd6f3..d68c323bf 100644 --- a/spec/views/pages/delete.html.erb_spec.rb +++ b/spec/views/pages/delete.html.erb_spec.rb @@ -1,11 +1,12 @@ require "rails_helper" RSpec.describe "pages/delete" do + let(:delete_confirmation_input) { Forms::DeleteConfirmationInput.new } let(:page) { build :page, id: 1 } before do assign(:back_url, edit_question_path(form_id: 1, page_id: page.id)) - assign(:delete_confirmation_input, Forms::DeleteConfirmationInput.new) + assign(:delete_confirmation_input, delete_confirmation_input) assign(:item_name, "What’s your name?") assign(:page, page) assign(:url, destroy_page_path(form_id: 1, page_id: page.id)) @@ -66,5 +67,17 @@ it { is_expected.to have_css "p.govuk-body", text: "If you delete this question, its routes will also be deleted." } it { is_expected.to have_link "View question 2’s routes", class: "govuk-notification-banner__link", href: show_routes_path(1, 1) } end + + context "but there was an error in the user's input" do + let(:delete_confirmation_input) do + delete_confirmation_input = Forms::DeleteConfirmationInput.new(confirm: "") + delete_confirmation_input.validate + delete_confirmation_input + end + + it "does not render the notification banner" do + expect(rendered).not_to have_css ".govuk-notification-banner" + end + end end end From fc5efa5edf9db18ec387912a234a98b5bd59b12d Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Tue, 10 Dec 2024 17:23:30 +0200 Subject: [PATCH 5/8] Add warning for deleting page at start of secondary skip route Co-authored-by: hannahkc --- app/controllers/pages_controller.rb | 1 + app/views/pages/delete.html.erb | 8 ++- config/locales/en.yml | 7 +++ spec/requests/pages_controller_spec.rb | 48 ++++++++++++++++- spec/views/pages/delete.html.erb_spec.rb | 67 +++++++++++++++++++++++- 5 files changed, 128 insertions(+), 3 deletions(-) diff --git a/app/controllers/pages_controller.rb b/app/controllers/pages_controller.rb index 4799999b5..8a6e319de 100644 --- a/app/controllers/pages_controller.rb +++ b/app/controllers/pages_controller.rb @@ -11,6 +11,7 @@ def index def delete @page = PageRepository.find(page_id: params[:page_id], form_id: current_form.id) + @check_page = PageRepository.find(page_id: @page.routing_conditions.first.check_page_id, form_id: current_form.id) if @page.routing_conditions.first&.secondary_skip? @url = destroy_page_path(current_form.id, @page.id) @confirm_deletion_legend = t("forms_delete_confirmation_input.confirm_deletion_page") @item_name = @page.question_text diff --git a/app/views/pages/delete.html.erb b/app/views/pages/delete.html.erb index 7bc88e4fe..953cb01b6 100644 --- a/app/views/pages/delete.html.erb +++ b/app/views/pages/delete.html.erb @@ -5,7 +5,13 @@
<% unless @delete_confirmation_input.errors.any? %> - <% if @page.routing_conditions.present? %> + <% if @page.routing_conditions.present? && @page.routing_conditions.first.secondary_skip? %> + <%= govuk_notification_banner(title_text: "Important") do |banner| + banner.with_heading(text: t(".notification_banner.start_of_secondary_skip_route.heading_text", routing_page_position: @page.position), tag: :h3) + + t(".notification_banner.start_of_secondary_skip_route.html", check_page_position: @check_page.position, check_page_show_routes_href: show_routes_path(current_form.id, @check_page.id)) + end %> + <% elsif @page.routing_conditions.present? %> <%= govuk_notification_banner(title_text: "Important") do |banner| banner.with_heading(text: t(".notification_banner.start_of_route.heading_text", routing_page_position: @page.position), tag: :h3) diff --git a/config/locales/en.yml b/config/locales/en.yml index c94072fad..741bc528f 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1157,6 +1157,13 @@ en: If you delete this question, its routes will also be deleted. View question %{routing_page_position}’s routes.

+ start_of_secondary_skip_route: + heading_text: Question %{routing_page_position} is the start of a route + html: | +

+ Question %{check_page_position}’s route + starts at this question. If you delete this question, the route from it will also be deleted. +

title: Are you sure you want to delete this question? delete_question: Delete question go_to_your_questions: Back to your questions diff --git a/spec/requests/pages_controller_spec.rb b/spec/requests/pages_controller_spec.rb index 736a0a6f9..c6db3574e 100644 --- a/spec/requests/pages_controller_spec.rb +++ b/spec/requests/pages_controller_spec.rb @@ -96,7 +96,8 @@ before do allow(FormRepository).to receive(:find).and_return(form_response) - allow(PageRepository).to receive_messages(find: page, destroy: true) + allow(PageRepository).to receive(:find).with(page_id: page.id.to_s, form_id: 2).and_return(page) + allow(PageRepository).to receive_messages(destroy: true) GroupForm.create!(form_id: 2, group_id: group.id) @@ -151,6 +152,51 @@ end end end + + context "when page to delete is start of a secondary skip route" do + let(:check_page) do + check_page = build( + :page, + :with_selection_settings, + id: 1, + form_id: 2, + position: 1, + question_text: "What is your favourite colour?", + selection_options: [{ name: "Red" }, { name: "Green" }, { name: "Blue" }], + only_one_option: true, + routing_conditions: [ + build(:condition, routing_page_id: 1, check_page_id: 1, value: "green", goto_page_id: 3), + ], + ) + + allow(PageRepository).to receive(:find).with(page_id: 1, form_id: 2).and_return(check_page) + + check_page + end + + let(:secondary_skip_page) do + build( + :page, + id: 5, + form_id: 2, + position: 5, + routing_conditions: [ + build(:condition, routing_page_id: 5, check_page_id: check_page.id, value: nil, goto_page_id: 8), + ], + ) + end + + let(:page) { secondary_skip_page } + + it "renders a warning about deleting this page" do + assert_select(".govuk-notification-banner", count: 1) do + assert_select "*", "Important" + assert_select "h3", "Question 5 is the start of a route" + assert_select "p.govuk-body a", "Question 1’s route" + assert_select "p.govuk-body", /Question 1’s route\s*starts at this question. If you delete this question, the route from it will also be deleted./ + end + end + end end end diff --git a/spec/views/pages/delete.html.erb_spec.rb b/spec/views/pages/delete.html.erb_spec.rb index d68c323bf..b05e8205f 100644 --- a/spec/views/pages/delete.html.erb_spec.rb +++ b/spec/views/pages/delete.html.erb_spec.rb @@ -53,7 +53,17 @@ end describe "when page to delete is the start of one or more routes" do - let(:page) { build :page, id: 1, form_id: 1, position: 2, routing_conditions: true } + let(:page) do + build( + :page, + id: 1, + form_id: 1, + position: 2, + routing_conditions: [ + build(:condition, routing_page_id: 1, check_page_id: 1), + ], + ) + end it "renders a notification banner" do expect(rendered).to have_css ".govuk-notification-banner" @@ -80,4 +90,59 @@ end end end + + describe "when page to delete is start of a secondary skip route" do + let(:check_page) do + check_page = build( + :page, + id: 2, + form_id: 1, + position: 1, + routing_conditions: [ + build(:condition, routing_page_id: 1, check_page_id: 1, goto_page_id: 1), + ], + ) + + assign(:check_page, check_page) + + check_page + end + + let(:page) do + build( + :page, + id: 1, + form_id: 1, + position: 2, + routing_conditions: [ + build(:condition, routing_page_id: 1, check_page_id: check_page.id, value: nil), + ], + ) + end + + it "renders a notification banner" do + expect(rendered).to have_css ".govuk-notification-banner" + end + + describe "notification banner" do + subject(:banner) { rendered.html.at_css(".govuk-notification-banner") } + + it { is_expected.to have_text "Important" } + it { is_expected.to have_css "h3.govuk-notification-banner__heading", text: "Question 2 is the start of a route", count: 1 } + it { is_expected.to have_link "Question 1’s route", class: "govuk-notification-banner__link", href: show_routes_path(1, 2) } + it { is_expected.to have_css "p.govuk-body", text: "If you delete this question, the route from it will also be deleted." } + end + + context "but there was an error in the user's input" do + let(:delete_confirmation_input) do + delete_confirmation_input = Forms::DeleteConfirmationInput.new(confirm: "") + delete_confirmation_input.validate + delete_confirmation_input + end + + it "does not render the notification banner" do + expect(rendered).not_to have_css ".govuk-notification-banner" + end + end + end end From ee931e94ddabbf2b2fdf3ee0a0eb6226245113a1 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Tue, 10 Dec 2024 17:54:33 +0200 Subject: [PATCH 6/8] Add warning for deleting page at end of route Co-authored-by: hannahkc --- app/controllers/pages_controller.rb | 7 +- app/views/pages/delete.html.erb | 6 ++ config/locales/en.yml | 7 ++ spec/requests/pages_controller_spec.rb | 101 ++++++++++++++++------- spec/views/pages/delete.html.erb_spec.rb | 59 ++++++++++++- 5 files changed, 148 insertions(+), 32 deletions(-) diff --git a/app/controllers/pages_controller.rb b/app/controllers/pages_controller.rb index 8a6e319de..21119c0d9 100644 --- a/app/controllers/pages_controller.rb +++ b/app/controllers/pages_controller.rb @@ -11,12 +11,17 @@ def index def delete @page = PageRepository.find(page_id: params[:page_id], form_id: current_form.id) - @check_page = PageRepository.find(page_id: @page.routing_conditions.first.check_page_id, form_id: current_form.id) if @page.routing_conditions.first&.secondary_skip? @url = destroy_page_path(current_form.id, @page.id) @confirm_deletion_legend = t("forms_delete_confirmation_input.confirm_deletion_page") @item_name = @page.question_text @back_url = edit_question_path(current_form.id, @page.id) + all_form_conditions = current_form.pages.flat_map(&:routing_conditions).compact_blank + @page_goto_conditions = all_form_conditions.select { |condition| condition.goto_page_id == @page.id } + + @check_page = PageRepository.find(page_id: @page.routing_conditions.first.check_page_id, form_id: current_form.id) if @page.routing_conditions.first&.secondary_skip? + @routing_page = PageRepository.find(page_id: @page_goto_conditions.first.routing_page_id, form_id: current_form.id) if @page_goto_conditions.any? + @delete_confirmation_input = Forms::DeleteConfirmationInput.new render locals: { current_form: } diff --git a/app/views/pages/delete.html.erb b/app/views/pages/delete.html.erb index 953cb01b6..745a8ae83 100644 --- a/app/views/pages/delete.html.erb +++ b/app/views/pages/delete.html.erb @@ -17,6 +17,12 @@ t(".notification_banner.start_of_route.html", routing_page_position: @page.position, routing_page_show_routes_href: show_routes_path(current_form.id, @page.id)) end %> + <% elsif @page_goto_conditions.present? %> + <%= govuk_notification_banner(title_text: "Important") do |banner| + banner.with_heading(text: t(".notification_banner.end_of_route.heading_text", goto_page_position: @page.position), tag: :h3) + + t(".notification_banner.end_of_route.html", routing_page_position: @routing_page.position, routing_page_show_routes_href: show_routes_path(current_form.id, @routing_page.id)) + end %> <% end %> <% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 741bc528f..a26ae889a 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1150,6 +1150,13 @@ en: answer_settings: Answer settings delete: notification_banner: + end_of_route: + heading_text: Question %{goto_page_position} is at the end of a route + html: | +

+ Question %{routing_page_position}’s route + goes to this question. If you delete this question, question %{routing_page_position}’s routes will also be deleted. +

start_of_route: heading_text: Question %{routing_page_position} is the start of a route html: | diff --git a/spec/requests/pages_controller_spec.rb b/spec/requests/pages_controller_spec.rb index c6db3574e..012c2eb3e 100644 --- a/spec/requests/pages_controller_spec.rb +++ b/spec/requests/pages_controller_spec.rb @@ -94,9 +94,20 @@ ) end + let(:pages) { [page] } + before do + ActiveResource::HttpMock.respond_to do |mock| + mock.get "/api/v1/forms/2/pages", headers, pages.to_json, 200 + end + allow(FormRepository).to receive(:find).and_return(form_response) - allow(PageRepository).to receive(:find).with(page_id: page.id.to_s, form_id: 2).and_return(page) + + pages.each do |page| + allow(PageRepository).to receive(:find).with(page_id: page.id.to_s, form_id: 2).and_return(page) + allow(PageRepository).to receive(:find).with(page_id: page.id, form_id: 2).and_return(page) + end + allow(PageRepository).to receive_messages(destroy: true) GroupForm.create!(form_id: 2, group_id: group.id) @@ -153,40 +164,72 @@ end end - context "when page to delete is start of a secondary skip route" do - let(:check_page) do - check_page = build( - :page, - :with_selection_settings, - id: 1, - form_id: 2, - position: 1, - question_text: "What is your favourite colour?", - selection_options: [{ name: "Red" }, { name: "Green" }, { name: "Blue" }], - only_one_option: true, - routing_conditions: [ - build(:condition, routing_page_id: 1, check_page_id: 1, value: "green", goto_page_id: 3), - ], - ) + context "when page to delete is at the end of a route" do + let(:pages) do + [ + build( + :page, + :with_selection_settings, + id: 1, + form_id: 2, + position: 1, + question_text: "What is your favourite colour?", + selection_options: [{ name: "Red" }, { name: "Green" }, { name: "Blue" }], + only_one_option: true, + routing_conditions: [ + build(:condition, routing_page_id: 1, check_page_id: 1, value: "green", goto_page_id: 3), + ], + ), + build( + :page, + id: 3, + form_id: 2, + position: 3, + ), + ] + end - allow(PageRepository).to receive(:find).with(page_id: 1, form_id: 2).and_return(check_page) + let(:page) { pages.last } - check_page + it "renders a warning about deleting this page" do + assert_select(".govuk-notification-banner", count: 1) do + assert_select "*", "Important" + assert_select "h3", "Question 3 is at the end of a route" + assert_select "p.govuk-body a", "Question 1’s route" + assert_select "p.govuk-body", /Question 1’s route\s*goes to this question. If you delete this question, question 1’s routes will also be deleted./ + end end + end - let(:secondary_skip_page) do - build( - :page, - id: 5, - form_id: 2, - position: 5, - routing_conditions: [ - build(:condition, routing_page_id: 5, check_page_id: check_page.id, value: nil, goto_page_id: 8), - ], - ) + context "when page to delete is start of a secondary skip route" do + let(:pages) do + [ + build( + :page, + :with_selection_settings, + id: 1, + form_id: 2, + position: 1, + question_text: "What is your favourite colour?", + selection_options: [{ name: "Red" }, { name: "Green" }, { name: "Blue" }], + only_one_option: true, + routing_conditions: [ + build(:condition, routing_page_id: 1, check_page_id: 1, value: "green", goto_page_id: 3), + ], + ), + build( + :page, + id: 5, + form_id: 2, + position: 5, + routing_conditions: [ + build(:condition, routing_page_id: 5, check_page_id: 1, value: nil, goto_page_id: 8), + ], + ), + ] end - let(:page) { secondary_skip_page } + let(:page) { pages.last } it "renders a warning about deleting this page" do assert_select(".govuk-notification-banner", count: 1) do diff --git a/spec/views/pages/delete.html.erb_spec.rb b/spec/views/pages/delete.html.erb_spec.rb index b05e8205f..fd723e858 100644 --- a/spec/views/pages/delete.html.erb_spec.rb +++ b/spec/views/pages/delete.html.erb_spec.rb @@ -4,6 +4,8 @@ let(:delete_confirmation_input) { Forms::DeleteConfirmationInput.new } let(:page) { build :page, id: 1 } + let(:current_form) { build :form, id: 1 } + before do assign(:back_url, edit_question_path(form_id: 1, page_id: page.id)) assign(:delete_confirmation_input, delete_confirmation_input) @@ -11,8 +13,6 @@ assign(:page, page) assign(:url, destroy_page_path(form_id: 1, page_id: page.id)) - current_form = build :form, id: 1 - render locals: { current_form: } end @@ -91,6 +91,61 @@ end end + describe "when page to delete is at the end of one or more routes" do + let(:routing_page) do + build( + :page, + id: 1, + form_id: 1, + position: 2, + routing_conditions: [ + build(:condition, routing_page_id: 1, check_page_id: 1, goto_page_id: 3), + ], + ) + end + + let(:page) do + build( + :page, + id: 3, + form_id: 1, + position: 7, + ) + end + + before do + assign(:routing_page, routing_page) + assign(:page_goto_conditions, routing_page.routing_conditions) + + render locals: { current_form: } + end + + it "renders a notification banner" do + expect(rendered).to have_css ".govuk-notification-banner" + end + + describe "notification banner" do + subject(:banner) { rendered.html.at_css(".govuk-notification-banner") } + + it { is_expected.to have_text "Important" } + it { is_expected.to have_css "h3.govuk-notification-banner__heading", text: "Question 7 is at the end of a route", count: 1 } + it { is_expected.to have_link "Question 2’s route", class: "govuk-notification-banner__link", href: show_routes_path(1, 1) } + it { is_expected.to have_css "p.govuk-body", text: "Question 2’s route goes to this question. If you delete this question, question 2’s routes will also be deleted.", normalize_ws: true } + end + + context "but there was an error in the user's input" do + let(:delete_confirmation_input) do + delete_confirmation_input = Forms::DeleteConfirmationInput.new(confirm: "") + delete_confirmation_input.validate + delete_confirmation_input + end + + it "does not render the notification banner" do + expect(rendered).not_to have_css ".govuk-notification-banner" + end + end + end + describe "when page to delete is start of a secondary skip route" do let(:check_page) do check_page = build( From a6d9a7dfa29ffa6e897312222ba5c3c5db2df0b3 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Tue, 10 Dec 2024 18:36:47 +0200 Subject: [PATCH 7/8] Add warning for deleting page at end of secondary skip route Co-authored-by: hannahkc --- app/controllers/pages_controller.rb | 1 + app/views/pages/delete.html.erb | 6 +++ config/locales/en.yml | 7 +++ spec/requests/pages_controller_spec.rb | 46 ++++++++++++++++ spec/views/pages/delete.html.erb_spec.rb | 67 ++++++++++++++++++++++++ 5 files changed, 127 insertions(+) diff --git a/app/controllers/pages_controller.rb b/app/controllers/pages_controller.rb index 21119c0d9..e037a0682 100644 --- a/app/controllers/pages_controller.rb +++ b/app/controllers/pages_controller.rb @@ -21,6 +21,7 @@ def delete @check_page = PageRepository.find(page_id: @page.routing_conditions.first.check_page_id, form_id: current_form.id) if @page.routing_conditions.first&.secondary_skip? @routing_page = PageRepository.find(page_id: @page_goto_conditions.first.routing_page_id, form_id: current_form.id) if @page_goto_conditions.any? + @check_page = PageRepository.find(page_id: @page_goto_conditions.first.check_page_id, form_id: current_form.id) if @page_goto_conditions.any? @delete_confirmation_input = Forms::DeleteConfirmationInput.new diff --git a/app/views/pages/delete.html.erb b/app/views/pages/delete.html.erb index 745a8ae83..1a91b5819 100644 --- a/app/views/pages/delete.html.erb +++ b/app/views/pages/delete.html.erb @@ -17,6 +17,12 @@ t(".notification_banner.start_of_route.html", routing_page_position: @page.position, routing_page_show_routes_href: show_routes_path(current_form.id, @page.id)) end %> + <% elsif @page_goto_conditions.present? && @page_goto_conditions.first&.secondary_skip? %> + <%= govuk_notification_banner(title_text: "Important") do |banner| + banner.with_heading(text: t(".notification_banner.end_of_secondary_skip_route.heading_text", goto_page_position: @page.position), tag: :h3) + + t(".notification_banner.end_of_secondary_skip_route.html", check_page_position: @check_page.position, check_page_show_routes_href: show_routes_path(current_form.id, @check_page.id)) + end %> <% elsif @page_goto_conditions.present? %> <%= govuk_notification_banner(title_text: "Important") do |banner| banner.with_heading(text: t(".notification_banner.end_of_route.heading_text", goto_page_position: @page.position), tag: :h3) diff --git a/config/locales/en.yml b/config/locales/en.yml index a26ae889a..c651668c5 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1157,6 +1157,13 @@ en: Question %{routing_page_position}’s route goes to this question. If you delete this question, question %{routing_page_position}’s routes will also be deleted.

+ end_of_secondary_skip_route: + heading_text: Question %{goto_page_position} is at the end of a route + html: | +

+ Question %{check_page_position}’s route + goes to this question. If you delete this question, the route to it will also be deleted. +

start_of_route: heading_text: Question %{routing_page_position} is the start of a route html: | diff --git a/spec/requests/pages_controller_spec.rb b/spec/requests/pages_controller_spec.rb index 012c2eb3e..39da009c9 100644 --- a/spec/requests/pages_controller_spec.rb +++ b/spec/requests/pages_controller_spec.rb @@ -240,6 +240,52 @@ end end end + + context "when page to delete is at the end of a secondary skip route" do + let(:pages) do + [ + build( + :page, + :with_selection_settings, + id: 1, + form_id: 2, + position: 1, + question_text: "What is your favourite colour?", + selection_options: [{ name: "Red" }, { name: "Green" }, { name: "Blue" }], + only_one_option: true, + routing_conditions: [ + build(:condition, routing_page_id: 1, check_page_id: 1, value: "green", goto_page_id: 3), + ], + ), + build( + :page, + id: 5, + form_id: 2, + position: 5, + routing_conditions: [ + build(:condition, routing_page_id: 5, check_page_id: 1, value: nil, goto_page_id: 8), + ], + ), + build( + :page, + id: 8, + form_id: 2, + position: 8, + ), + ] + end + + let(:page) { pages.last } + + it "renders a warning about deleting this page" do + assert_select(".govuk-notification-banner", count: 1) do + assert_select "*", "Important" + assert_select "h3", "Question 8 is at the end of a route" + assert_select "p.govuk-body a", "Question 1’s route" + assert_select "p.govuk-body", /Question 1’s route\s*goes to this question. If you delete this question, the route to it will also be deleted./ + end + end + end end end diff --git a/spec/views/pages/delete.html.erb_spec.rb b/spec/views/pages/delete.html.erb_spec.rb index fd723e858..5e8e2adc0 100644 --- a/spec/views/pages/delete.html.erb_spec.rb +++ b/spec/views/pages/delete.html.erb_spec.rb @@ -200,4 +200,71 @@ end end end + + describe "when page to delete is at the end of a secondary skip route" do + let(:check_page) do + build( + :page, + id: 1, + form_id: 1, + position: 3, + routing_conditions: [ + build(:condition, routing_page_id: 1, check_page_id: 1, goto_page_id: 3), + ], + ) + end + + let(:routing_page) do + build( + :page, + id: 12, + form_id: 1, + position: 7, + routing_conditions: [ + build(:condition, routing_page_id: 12, check_page_id: 1, goto_page_id: 9), + ], + ) + end + + let(:page) do + build( + :page, + id: 9, + form_id: 1, + position: 12, + ) + end + + before do + assign(:check_page, check_page) + assign(:page_goto_conditions, routing_page.routing_conditions) + + render locals: { current_form: } + end + + it "renders a notification banner" do + expect(rendered).to have_css ".govuk-notification-banner" + end + + describe "notification banner" do + subject(:banner) { rendered.html.at_css(".govuk-notification-banner") } + + it { is_expected.to have_text "Important" } + it { is_expected.to have_css "h3.govuk-notification-banner__heading", text: "Question 12 is at the end of a route", count: 1 } + it { is_expected.to have_link "Question 3’s route", class: "govuk-notification-banner__link", href: show_routes_path(1, 1) } + it { is_expected.to have_css "p.govuk-body", text: "Question 3’s route goes to this question. If you delete this question, the route to it will also be deleted.", normalize_ws: true } + end + + context "but there was an error in the user's input" do + let(:delete_confirmation_input) do + delete_confirmation_input = Forms::DeleteConfirmationInput.new(confirm: "") + delete_confirmation_input.validate + delete_confirmation_input + end + + it "does not render the notification banner" do + expect(rendered).not_to have_css ".govuk-notification-banner" + end + end + end end From 75e8a4aac4d0fa87456741db3c064664a72a6e92 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Wed, 11 Dec 2024 09:54:18 +0200 Subject: [PATCH 8/8] Refactor page delete confirmation view Move logic around routing from the view to the controller for the page#delete action. The main aim is to reduce the repetition of the notification banner call in the view; to achieve this we rely on the consistency of the content, where the warning refers to up to two different questions; the question about to be deleted, and the question that "owns" the route (which depends on the kind of route involved). A future refactor might appropriately move this logic out of the controller to a service, or maybe something more generally useful like a model that expresses the relationships between conditions and routes, but before that we need to make this view work with cases where the page to be deleted is associated with more than two routes, so for now I think this is a good place to stop. --- app/controllers/pages_controller.rb | 24 +++++++++++++++++--- app/views/pages/delete.html.erb | 28 ++++++------------------ config/locales/en.yml | 18 +++++++-------- spec/views/pages/delete.html.erb_spec.rb | 27 +++++++++++++++++------ 4 files changed, 57 insertions(+), 40 deletions(-) diff --git a/app/controllers/pages_controller.rb b/app/controllers/pages_controller.rb index e037a0682..0fa0df672 100644 --- a/app/controllers/pages_controller.rb +++ b/app/controllers/pages_controller.rb @@ -19,9 +19,27 @@ def delete all_form_conditions = current_form.pages.flat_map(&:routing_conditions).compact_blank @page_goto_conditions = all_form_conditions.select { |condition| condition.goto_page_id == @page.id } - @check_page = PageRepository.find(page_id: @page.routing_conditions.first.check_page_id, form_id: current_form.id) if @page.routing_conditions.first&.secondary_skip? - @routing_page = PageRepository.find(page_id: @page_goto_conditions.first.routing_page_id, form_id: current_form.id) if @page_goto_conditions.any? - @check_page = PageRepository.find(page_id: @page_goto_conditions.first.check_page_id, form_id: current_form.id) if @page_goto_conditions.any? + if @page.routing_conditions.any? && @page.routing_conditions.first.secondary_skip? + @routing = :start_of_secondary_skip_route + + # route owner is condition check page + @route_owner = PageRepository.find(page_id: @page.routing_conditions.first.check_page_id, form_id: current_form.id) + elsif @page.routing_conditions.any? + @routing = :start_of_route + + # route owner is us + @route_owner = @page + elsif @page_goto_conditions.any? && @page_goto_conditions.first.secondary_skip? + @routing = :end_of_secondary_skip_route + + # route owner is condition check page + @route_owner = PageRepository.find(page_id: @page_goto_conditions.first.check_page_id, form_id: current_form.id) + elsif @page_goto_conditions.any? + @routing = :end_of_route + + # route owner is condition routing page + @route_owner = PageRepository.find(page_id: @page_goto_conditions.first.routing_page_id, form_id: current_form.id) + end @delete_confirmation_input = Forms::DeleteConfirmationInput.new diff --git a/app/views/pages/delete.html.erb b/app/views/pages/delete.html.erb index 1a91b5819..8de33d382 100644 --- a/app/views/pages/delete.html.erb +++ b/app/views/pages/delete.html.erb @@ -5,29 +5,15 @@
<% unless @delete_confirmation_input.errors.any? %> - <% if @page.routing_conditions.present? && @page.routing_conditions.first.secondary_skip? %> + <% if @routing.present? %> <%= govuk_notification_banner(title_text: "Important") do |banner| - banner.with_heading(text: t(".notification_banner.start_of_secondary_skip_route.heading_text", routing_page_position: @page.position), tag: :h3) + banner.with_heading(text: t(".notification_banner.#{@routing}.heading_text", page_position: @page.position), tag: :h3) - t(".notification_banner.start_of_secondary_skip_route.html", check_page_position: @check_page.position, check_page_show_routes_href: show_routes_path(current_form.id, @check_page.id)) - end %> - <% elsif @page.routing_conditions.present? %> - <%= govuk_notification_banner(title_text: "Important") do |banner| - banner.with_heading(text: t(".notification_banner.start_of_route.heading_text", routing_page_position: @page.position), tag: :h3) - - t(".notification_banner.start_of_route.html", routing_page_position: @page.position, routing_page_show_routes_href: show_routes_path(current_form.id, @page.id)) - end %> - <% elsif @page_goto_conditions.present? && @page_goto_conditions.first&.secondary_skip? %> - <%= govuk_notification_banner(title_text: "Important") do |banner| - banner.with_heading(text: t(".notification_banner.end_of_secondary_skip_route.heading_text", goto_page_position: @page.position), tag: :h3) - - t(".notification_banner.end_of_secondary_skip_route.html", check_page_position: @check_page.position, check_page_show_routes_href: show_routes_path(current_form.id, @check_page.id)) - end %> - <% elsif @page_goto_conditions.present? %> - <%= govuk_notification_banner(title_text: "Important") do |banner| - banner.with_heading(text: t(".notification_banner.end_of_route.heading_text", goto_page_position: @page.position), tag: :h3) - - t(".notification_banner.end_of_route.html", routing_page_position: @routing_page.position, routing_page_show_routes_href: show_routes_path(current_form.id, @routing_page.id)) + t( + ".notification_banner.#{@routing}.html", + route_owner_position: @route_owner.position, + show_routes_href: show_routes_path(current_form.id, @route_owner.id), + ) end %> <% end %> <% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index c651668c5..fa0057e79 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1151,31 +1151,31 @@ en: delete: notification_banner: end_of_route: - heading_text: Question %{goto_page_position} is at the end of a route + heading_text: Question %{page_position} is at the end of a route html: |

- Question %{routing_page_position}’s route - goes to this question. If you delete this question, question %{routing_page_position}’s routes will also be deleted. + Question %{route_owner_position}’s route + goes to this question. If you delete this question, question %{route_owner_position}’s routes will also be deleted.

end_of_secondary_skip_route: - heading_text: Question %{goto_page_position} is at the end of a route + heading_text: Question %{page_position} is at the end of a route html: |

- Question %{check_page_position}’s route + Question %{route_owner_position}’s route goes to this question. If you delete this question, the route to it will also be deleted.

start_of_route: - heading_text: Question %{routing_page_position} is the start of a route + heading_text: Question %{page_position} is the start of a route html: |

If you delete this question, its routes will also be deleted. - View question %{routing_page_position}’s routes. + View question %{route_owner_position}’s routes.

start_of_secondary_skip_route: - heading_text: Question %{routing_page_position} is the start of a route + heading_text: Question %{page_position} is the start of a route html: |

- Question %{check_page_position}’s route + Question %{route_owner_position}’s route starts at this question. If you delete this question, the route from it will also be deleted.

title: Are you sure you want to delete this question? diff --git a/spec/views/pages/delete.html.erb_spec.rb b/spec/views/pages/delete.html.erb_spec.rb index 5e8e2adc0..30bbc9eff 100644 --- a/spec/views/pages/delete.html.erb_spec.rb +++ b/spec/views/pages/delete.html.erb_spec.rb @@ -65,6 +65,13 @@ ) end + before do + assign(:routing, :start_of_route) + assign(:route_owner, page) + + render locals: { current_form: } + end + it "renders a notification banner" do expect(rendered).to have_css ".govuk-notification-banner" end @@ -114,7 +121,9 @@ end before do - assign(:routing_page, routing_page) + assign(:routing, :end_of_route) + assign(:route_owner, routing_page) + assign(:page_goto_conditions, routing_page.routing_conditions) render locals: { current_form: } @@ -148,7 +157,7 @@ describe "when page to delete is start of a secondary skip route" do let(:check_page) do - check_page = build( + build( :page, id: 2, form_id: 1, @@ -157,10 +166,6 @@ build(:condition, routing_page_id: 1, check_page_id: 1, goto_page_id: 1), ], ) - - assign(:check_page, check_page) - - check_page end let(:page) do @@ -175,6 +180,13 @@ ) end + before do + assign(:routing, :start_of_secondary_skip_route) + assign(:route_owner, check_page) + + render locals: { current_form: } + end + it "renders a notification banner" do expect(rendered).to have_css ".govuk-notification-banner" end @@ -236,8 +248,9 @@ end before do - assign(:check_page, check_page) assign(:page_goto_conditions, routing_page.routing_conditions) + assign(:routing, :end_of_secondary_skip_route) + assign(:route_owner, check_page) render locals: { current_form: } end