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

@@ -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
 

app/views/pages/delete.html.erb

@@ -5,29 +5,15 @@
   <div class="govuk-grid-column-two-thirds">
 
     <% 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 %>

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: |
             <p class="govuk-body">
-              <a class="govuk-notification-banner__link" href="%{routing_page_show_routes_href}">Question %{routing_page_position}’s route</a>
-              goes to this question. If you delete this question, question %{routing_page_position}’s routes will also be deleted.
+              <a class="govuk-notification-banner__link" href="%{show_routes_href}">Question %{route_owner_position}’s route</a>
+              goes to this question. If you delete this question, question %{route_owner_position}’s routes will also be deleted.
             </p>
         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: |
             <p class="govuk-body">
-              <a class="govuk-notification-banner__link" href="%{check_page_show_routes_href}">Question %{check_page_position}’s route</a>
+              <a class="govuk-notification-banner__link" href="%{show_routes_href}">Question %{route_owner_position}’s route</a>
               goes to this question. If you delete this question, the route to it will also be deleted.
             </p>
         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: |
             <p class="govuk-body">
               If you delete this question, its routes will also be deleted.
-              <a class="govuk-notification-banner__link" href="%{routing_page_show_routes_href}">View question %{routing_page_position}’s routes</a>.
+              <a class="govuk-notification-banner__link" href="%{show_routes_href}">View question %{route_owner_position}’s routes</a>.
             </p>
         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: |
             <p class="govuk-body">
-              <a class="govuk-notification-banner__link" href="%{check_page_show_routes_href}">Question %{check_page_position}’s route</a>
+              <a class="govuk-notification-banner__link" href="%{show_routes_href}">Question %{route_owner_position}’s route</a>
               starts at this question. If you delete this question, the route from it will also be deleted.
             </p>
       title: Are you sure you want to delete this question?

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