Merge pull request #1798 from alphagov/ldeb-refactor-page-list-summar…

…y-component

Refactor PageListComponent

app/components/page_list_component/view.html.erb

@@ -1,7 +1,7 @@
 
 <%if @pages.present?  %>
   <div class="app-page-list">
-    <%= form_with url: move_page_url(@form_id), method: :post, builder: GOVUKDesignSystemFormBuilder::FormBuilder do |f| %>
+    <%= form_with url: move_page_url(@form.id), method: :post, builder: GOVUKDesignSystemFormBuilder::FormBuilder do |f| %>
       <dl class="govuk-summary-list">
         <% @pages.each_with_index do |page, index| %>
           <div id="<%= page_row_id(page) %>" class="govuk-summary-list__row">
@@ -25,14 +25,14 @@
                 <% end %>
               </div>
 
-              <%= govuk_link_to edit_question_path(@form_id, page.id) do %>
+              <%= govuk_link_to edit_question_path(@form.id, page.id) do %>
                 <%= t("forms.form_overview.edit") %> <span class="govuk-visually-hidden"><%= page.position %></span>
               <% end %>
 
             </dd>
           </div>
 
-          <%  conditions_for_page_with_index(page.id).each do |condition, _route_index| %>
+          <%  page.routing_conditions.each do |condition| %>
             <div id="<%= PageListComponent::ErrorSummary::View.error_id(condition.id) %>" class="govuk-summary-list__row app-page-list__row <%= class_names( "govuk-form-group--error": condition.has_routing_errors?) %>">
               <dt class="govuk-summary-list__key govuk-summary-list__key app-page-list__key">
                 <%= t("page_conditions.condition_name", question_number: condition_page_position(condition)) %>
@@ -52,7 +52,7 @@
                 </p>
 
               <dd class="govuk-summary-list__actions govuk-!-padding-bottom-6">
-                <%= govuk_link_to show_routes_path(form_id: @form_id, page_id: condition.check_page_id) do %>
+                <%= govuk_link_to show_routes_path(form_id: @form.id, page_id: condition.check_page_id) do %>
                   <%= t("forms.form_overview.edit_with_visually_hidden_text_html", visually_hidden_text: t("page_conditions.condition_name", question_number: page.position)) %>
                 <% end %>
               </dd>

app/components/page_list_component/view.rb

@@ -2,10 +2,10 @@ module PageListComponent
   class View < ViewComponent::Base
     delegate :question_text_with_optional_suffix, to: :helpers
 
-    def initialize(form_id:, pages: [])
+    def initialize(form:, pages: [])
       super
+      @form = form
       @pages = pages
-      @form_id = form_id
     end
 
     def show_up_button(index)
@@ -62,35 +62,9 @@ def condition_page_position(condition)
       page_position(check_page)
     end
 
-    def conditions_for_page_with_index(page_id)
-      routing_conditions_with_index.fetch(page_id, [])
-    end
-
-    def routing_conditions_with_index
-      @routing_conditions_with_index ||= process_routing_conditions
-    end
-
     def skip_condition_route_page_text(condition)
       routing_page = @pages.find { |page| page.id == condition.routing_page_id }
       I18n.t("page_conditions.skip_condition_route_page_text", route_page_question_text: routing_page.question_text, route_page_question_number: routing_page.position)
     end
-
-    # Create hash of page_id => [condition, index]
-    # where index is the index of the condition in the array of conditions for
-    # the page referenced by check_page_id
-    def process_routing_conditions
-      all_form_conditions = @pages.flat_map(&:routing_conditions).compact_blank
-
-      all_form_conditions
-        .group_by(&:check_page_id)
-        .values
-        .flat_map { |conditions|
-          conditions.map.with_index(1) do |condition, index|
-            [condition.routing_page_id, [condition, index]] # inclde routing_page_id, so we can group by it
-          end
-        }
-        .group_by(&:first)
-        .transform_values { |pairs| pairs.map(&:last) } # drop routing_page_id from the value of the hash - it is now the key
-    end
   end
 end

app/views/pages/index.html.erb

@@ -27,7 +27,7 @@
 
     <% if @pages.any? %>
       <h2 class="govuk-heading-m govuk-!-margin-top-7 govuk-!-margin-bottom-0"><%= t("forms.form_overview.your_questions") %></h2>
-      <%= render PageListComponent::View.new(pages: @pages, form_id: current_form.id) %>
+      <%= render PageListComponent::View.new(form: current_form, pages: @pages) %>
       <%= render MarkCompleteComponent::View.new(form_model: @mark_complete_input, path: form_pages_path(current_form.id), legend: t("pages.index.mark_complete.legend")) %>
     <% end %>
 

spec/components/page_list_component/page_list_component_preview.rb

@@ -2,36 +2,41 @@ class PageListComponent::PageListComponentPreview < ViewComponent::Preview
   include FactoryBot::Syntax::Methods
 
   def default
-    render(PageListComponent::View.new(pages: [], form_id: 0))
+    form = build(:form, id: 0)
+    render(PageListComponent::View.new(pages: [], form:))
   end
 
   def with_pages_and_no_conditions
+    form = build(:form, id: 0)
     pages = [build(:page, id: 1, position: 1, question_text: "Enter your name", routing_conditions: []),
              build(:page, id: 2, position: 2, question_text: "What is your pet's phone number?", routing_conditions: []),
              build(:page, id: 3, position: 3, question_text: "How many pets do you own?", routing_conditions: [])]
-    render(PageListComponent::View.new(pages:, form_id: 0))
+    render(PageListComponent::View.new(pages:, form:))
   end
 
   def with_pages_and_one_condition
+    form = build(:form, id: 0)
     routing_conditions = [(build :condition, id: 1, routing_page_id: 1, check_page_id: 1, answer_value: "Wales", goto_page_id: 3)]
     pages = [build(:page, id: 1, position: 1, question_text: "Enter your name", routing_conditions:),
              build(:page, id: 2, position: 2, question_text: "What is your pet's phone number?", routing_conditions: []),
              build(:page, id: 3, position: 3, question_text: "How many pets do you own?", routing_conditions: [])]
-    render(PageListComponent::View.new(pages:, form_id: 0))
+    render(PageListComponent::View.new(pages:, form:))
   end
 
   def with_pages_and_multiple_conditions
+    form = build(:form, id: 0)
     routing_conditions_1 = [(build :condition, id: 1, routing_page_id: 1, check_page_id: 1, answer_value: "Wales", goto_page_id: 3),
                             (build :condition, id: 2, routing_page_id: 1, check_page_id: 1, answer_value: "England", goto_page_id: 2)]
     routing_conditions_2 = [(build :condition, id: 3, routing_page_id: 2, check_page_id: 2, answer_value: "Wales", goto_page_id: 3),
                             (build :condition, id: 4, routing_page_id: 2, check_page_id: 2, answer_value: "England", goto_page_id: 2)]
     pages = [(build :page, id: 1, position: 1, question_text: "Enter your name", routing_conditions: routing_conditions_1),
              (build :page, id: 2, position: 2, question_text: "What is your pet's phone number?", routing_conditions: routing_conditions_2),
              (build :page, id: 3, position: 3, question_text: "How many pets do you own?", routing_conditions: [])]
-    render(PageListComponent::View.new(pages:, form_id: 0))
+    render(PageListComponent::View.new(pages:, form:))
   end
 
   def with_pages_and_conditions_with_errors
+    form = build(:form, id: 0)
     routing_conditions_1 = [(build :condition, :with_answer_value_missing, id: 1, routing_page_id: 1, check_page_id: 1, goto_page_id: 3),
                             (build :condition, :with_goto_page_missing, id: 2, routing_page_id: 1, check_page_id: 1, answer_value: "England"),
                             (build :condition, :with_answer_value_and_goto_page_missing, id: 3, routing_page_id: 1, check_page_id: 1),
@@ -40,6 +45,6 @@ def with_pages_and_conditions_with_errors
     pages = [(build :page, id: 1, position: 1, question_text: "Enter your name", routing_conditions: routing_conditions_1),
              (build :page, id: 2, position: 2, question_text: "What is your pet's phone number?", routing_conditions: routing_conditions_2),
              (build :page, id: 3, position: 3, question_text: "How many pets do you own?", routing_conditions: [])]
-    render(PageListComponent::View.new(pages:, form_id: 0))
+    render(PageListComponent::View.new(pages:, form:))
   end
 end

spec/components/page_list_component/view_spec.rb

@@ -1,9 +1,10 @@
 require "rails_helper"
 
 RSpec.describe PageListComponent::View, type: :component do
+  let(:form) { build :form, id: 1 }
   let(:pages) { [] }
   let(:routing_conditions) { [] }
-  let(:page_list_component) { described_class.new(pages:, form_id: 0) }
+  let(:page_list_component) { described_class.new(pages:, form:) }
 
   describe "rendering component" do
     before do
@@ -304,66 +305,5 @@
         end
       end
     end
-
-    describe "#conditions_for_page_with_index" do
-      context "when there are no conditions" do
-        let(:pages) do
-          [(build :page, id: 1, position: 1, question_text: "What country do you live in?", routing_conditions:)]
-        end
-        let(:routing_conditions) { [] }
-
-        it "returns an array of conditions for the page" do
-          page_id = 1
-          expect(page_list_component.conditions_for_page_with_index(page_id)).to eq([])
-        end
-      end
-
-      context "when there is one page with one condition" do
-        let(:pages) do
-          [(build :page, id: 1, position: 1, question_text: "What country do you live in?", routing_conditions:)]
-        end
-        let(:routing_conditions) { [build(:condition, id: 1, routing_page_id: 1, check_page_id: 1, answer_value: "Wales", goto_page_id: 3)] }
-
-        it "returns an array of conditions for the page" do
-          page_id = 1
-          expect(page_list_component.conditions_for_page_with_index(page_id)).to eq([[routing_conditions.first, 1]])
-        end
-      end
-
-      context "when there is one page with multiple conditions" do
-        let(:pages) do
-          [(build :page, id: 1, position: 1, question_text: "What country do you live in?", routing_conditions:)]
-        end
-
-        let(:routing_conditions) do
-          [
-            build(:condition, id: 1, routing_page_id: 1, check_page_id: 1, answer_value: "Wales", goto_page_id: 3),
-            build(:condition, id: 2, routing_page_id: 2, check_page_id: 1, answer_value: nil, goto_page_id: 4),
-          ]
-        end
-
-        it "returns the correct condition with index" do
-          page_id = 2
-          expect(page_list_component.conditions_for_page_with_index(page_id)).to eq([[routing_conditions.second, 2]])
-        end
-      end
-
-      context "when there is one page with one condition and a condition for another pages" do
-        let(:pages) do
-          [(build :page, id: 1, position: 1, question_text: "What country do you live in?", routing_conditions:)]
-        end
-        let(:routing_conditions) do
-          [
-            build(:condition, id: 1, routing_page_id: 1, check_page_id: 1, answer_value: "Wales", goto_page_id: 3),
-            build(:condition, id: 1, routing_page_id: 2, check_page_id: 2, answer_value: "England", goto_page_id: 3),
-          ]
-        end
-
-        it "returns an array of conditions for the page" do
-          page_id = 1
-          expect(page_list_component.conditions_for_page_with_index(page_id)).to eq([[routing_conditions.first, 1]])
-        end
-      end
-    end
   end
 end