Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor PageListComponent #1798

Merged
merged 3 commits into from
Feb 26, 2025
Merged

Refactor PageListComponent #1798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3bed7d2
Refactor arguments to conditions_for_page_with_index
lfdebrux Feb 18, 2025
3eb6419
Refactor arguments to PageListComponent::View
lfdebrux Feb 18, 2025
0c752b6
Remove unneeded #conditions_for_page_with_index method
lfdebrux Feb 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions app/components/page_list_component/view.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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">
Expand All @@ -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)) %>
Expand All @@ -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>
Expand Down
30 changes: 2 additions & 28 deletions app/components/page_list_component/view.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
2 changes: 1 addition & 1 deletion app/views/pages/index.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 %>

Expand Down
15 changes: 10 additions & 5 deletions spec/components/page_list_component/page_list_component_preview.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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
64 changes: 2 additions & 62 deletions spec/components/page_list_component/view_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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