From ef17b299aaa3bf3ab70ca7cec02795bdcd2d72fc Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Wed, 11 Dec 2024 09:54:18 +0200 Subject: [PATCH] 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 1a45b4ad1..75a8c15c0 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 7ef8beec7..45d37bd98 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1135,31 +1135,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 ae373a67c..b483a7bee 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