From 3bed7d29fa1d9e5a49766bf5c557af7b500cfbab Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Tue, 18 Feb 2025 10:26:27 +0200 Subject: [PATCH 1/3] Refactor arguments to conditions_for_page_with_index --- app/components/page_list_component/view.html.erb | 2 +- app/components/page_list_component/view.rb | 4 ++-- spec/components/page_list_component/view_spec.rb | 16 ++++++++-------- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/app/components/page_list_component/view.html.erb b/app/components/page_list_component/view.html.erb index a6c8d2e01..02f249eb3 100644 --- a/app/components/page_list_component/view.html.erb +++ b/app/components/page_list_component/view.html.erb @@ -32,7 +32,7 @@ - <% conditions_for_page_with_index(page.id).each do |condition, _route_index| %> + <% conditions_for_page_with_index(page).each do |condition, _route_index| %>
">
<%= t("page_conditions.condition_name", question_number: condition_page_position(condition)) %> diff --git a/app/components/page_list_component/view.rb b/app/components/page_list_component/view.rb index 2c05f4667..1842fbc6c 100644 --- a/app/components/page_list_component/view.rb +++ b/app/components/page_list_component/view.rb @@ -62,8 +62,8 @@ 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, []) + def conditions_for_page_with_index(page) + routing_conditions_with_index.fetch(page.id, []) end def routing_conditions_with_index diff --git a/spec/components/page_list_component/view_spec.rb b/spec/components/page_list_component/view_spec.rb index 8347c3d0c..5e7442285 100644 --- a/spec/components/page_list_component/view_spec.rb +++ b/spec/components/page_list_component/view_spec.rb @@ -313,8 +313,8 @@ 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([]) + page = pages.first + expect(page_list_component.conditions_for_page_with_index(page)).to eq([]) end end @@ -325,8 +325,8 @@ 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]]) + page = pages.first + expect(page_list_component.conditions_for_page_with_index(page)).to eq([[routing_conditions.first, 1]]) end end @@ -343,8 +343,8 @@ 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]]) + page = build(:page, id: 2) + expect(page_list_component.conditions_for_page_with_index(page)).to eq([[routing_conditions.second, 2]]) end end @@ -360,8 +360,8 @@ 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]]) + page = pages.first + expect(page_list_component.conditions_for_page_with_index(page)).to eq([[routing_conditions.first, 1]]) end end end From 3eb64197642ceb0a02af9a43b73eb898e06bba36 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Tue, 18 Feb 2025 10:33:47 +0200 Subject: [PATCH 2/3] Refactor arguments to PageListComponent::View --- app/components/page_list_component/view.html.erb | 6 +++--- app/components/page_list_component/view.rb | 4 ++-- app/views/pages/index.html.erb | 2 +- .../page_list_component_preview.rb | 15 ++++++++++----- spec/components/page_list_component/view_spec.rb | 3 ++- 5 files changed, 18 insertions(+), 12 deletions(-) diff --git a/app/components/page_list_component/view.html.erb b/app/components/page_list_component/view.html.erb index 02f249eb3..122942575 100644 --- a/app/components/page_list_component/view.html.erb +++ b/app/components/page_list_component/view.html.erb @@ -1,7 +1,7 @@ <%if @pages.present? %>
- <%= 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| %>
<% @pages.each_with_index do |page, index| %>
@@ -25,7 +25,7 @@ <% end %>
- <%= 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") %> <%= page.position %> <% end %> @@ -52,7 +52,7 @@

- <%= 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 %>
diff --git a/app/components/page_list_component/view.rb b/app/components/page_list_component/view.rb index 1842fbc6c..76915298c 100644 --- a/app/components/page_list_component/view.rb +++ b/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) diff --git a/app/views/pages/index.html.erb b/app/views/pages/index.html.erb index 27af6f3fa..ad259b176 100644 --- a/app/views/pages/index.html.erb +++ b/app/views/pages/index.html.erb @@ -27,7 +27,7 @@ <% if @pages.any? %>

<%= t("forms.form_overview.your_questions") %>

- <%= 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 %> diff --git a/spec/components/page_list_component/page_list_component_preview.rb b/spec/components/page_list_component/page_list_component_preview.rb index d9b192bbe..99fc7e1c5 100644 --- a/spec/components/page_list_component/page_list_component_preview.rb +++ b/spec/components/page_list_component/page_list_component_preview.rb @@ -2,25 +2,29 @@ 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), @@ -28,10 +32,11 @@ def with_pages_and_multiple_conditions 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 diff --git a/spec/components/page_list_component/view_spec.rb b/spec/components/page_list_component/view_spec.rb index 5e7442285..7505f575b 100644 --- a/spec/components/page_list_component/view_spec.rb +++ b/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 From 0c752b68ea422ec6b4250156a062d43d78cce60b Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Mon, 24 Feb 2025 11:36:10 +0200 Subject: [PATCH 3/3] Remove unneeded #conditions_for_page_with_index method --- .../page_list_component/view.html.erb | 2 +- app/components/page_list_component/view.rb | 26 -------- .../page_list_component/view_spec.rb | 61 ------------------- 3 files changed, 1 insertion(+), 88 deletions(-) diff --git a/app/components/page_list_component/view.html.erb b/app/components/page_list_component/view.html.erb index 122942575..76c3f160d 100644 --- a/app/components/page_list_component/view.html.erb +++ b/app/components/page_list_component/view.html.erb @@ -32,7 +32,7 @@
- <% conditions_for_page_with_index(page).each do |condition, _route_index| %> + <% page.routing_conditions.each do |condition| %>
">
<%= t("page_conditions.condition_name", question_number: condition_page_position(condition)) %> diff --git a/app/components/page_list_component/view.rb b/app/components/page_list_component/view.rb index 76915298c..b6f08ad17 100644 --- a/app/components/page_list_component/view.rb +++ b/app/components/page_list_component/view.rb @@ -62,35 +62,9 @@ def condition_page_position(condition) page_position(check_page) end - def conditions_for_page_with_index(page) - 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 diff --git a/spec/components/page_list_component/view_spec.rb b/spec/components/page_list_component/view_spec.rb index 7505f575b..8e49e5b8f 100644 --- a/spec/components/page_list_component/view_spec.rb +++ b/spec/components/page_list_component/view_spec.rb @@ -305,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 = pages.first - expect(page_list_component.conditions_for_page_with_index(page)).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 = pages.first - expect(page_list_component.conditions_for_page_with_index(page)).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 = build(:page, id: 2) - expect(page_list_component.conditions_for_page_with_index(page)).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 = pages.first - expect(page_list_component.conditions_for_page_with_index(page)).to eq([[routing_conditions.first, 1]]) - end - end - end end end