Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
66 changes: 45 additions & 21 deletions app/components/page_list_component/view.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -32,33 +32,57 @@
</dd>
</div>

<% page.routing_conditions.each do |condition| %>
<% condition_page = self.condition_page(condition) %>
<% condition_page_position = self.condition_page_position(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?) %>">
<% if @form.group.multiple_branches_enabled? && page.routing_conditions.present? %>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest using the feature service to check whether the feature is enabled, rather than looking at the form record directly.

Suggested change
<% if @form.group.multiple_branches_enabled? && page.routing_conditions.present? %>
<% if FeatureService.new(@form.group).enabled?(:multiple_branches) && page.routing_conditions.present? %>

Then in the tests whether the flag is enabled or not can be changed by mocking the feature service with the helpers in spec/support/features.rb, removing the need to add a group to the form in each test.

<div class="govuk-summary-list__row">
<dt class="govuk-summary-list__key govuk-summary-list__key app-page-list__key">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<dt class="govuk-summary-list__key govuk-summary-list__key app-page-list__key">
<dt class="govuk-summary-list__key app-page-list__key">

<%= t("page_conditions.condition_name", question_number: condition_page_position) %>
<%= t("page_conditions.condition_name", question_number: page.position) %>
</dt>

<dd class="govuk-summary-list__value">
<ul class="govuk-list govuk-!-margin-0">
<% condition.validation_errors.each do |error| %>
<li class="app-page_list__route-text--error">
<%= PageListComponent::ErrorSummary::View.generate_error_message(error.name, condition:, page: condition_page) %>
</li>
<% end %>
</ul>

<p>
<%= condition_description(condition) %>
</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 %>
<%= t("forms.form_overview.edit_with_visually_hidden_text_html", visually_hidden_text: t("page_conditions.condition_name", question_number: condition_page_position)) %>
<% if page.routing_conditions.first.answer_value.present? %>
<p><%= I18n.t("page_conditions.if_answer_is") %></p>
<ul class="govuk-list govuk-list--bullet">
<% page.routing_conditions.each do |condition| %>
<li>
<%= branch_condition_description(condition) %>
</li>
<% end %>
</ul>
<% else %>
<p>
<%= branch_condition_description(page.routing_conditions.first) %>
</p>
<% end %>
</dd>
</div>
<% else %>
<% page.routing_conditions.each do |condition| %>
<% condition_page = self.condition_page(condition) %>
<% condition_page_position = self.condition_page_position(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) %>
</dt>

<dd class="govuk-summary-list__value">
<ul class="govuk-list govuk-!-margin-0">
<% condition.validation_errors.each do |error| %>
<li class="app-page_list__route-text--error">
<%= PageListComponent::ErrorSummary::View.generate_error_message(error.name, condition:, page: condition_page) %>
</li>
<% end %>
</ul>

<p>
<%= condition_description(condition) %>
</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 %>
<%= t("forms.form_overview.edit_with_visually_hidden_text_html", visually_hidden_text: t("page_conditions.condition_name", question_number: condition_page_position)) %>
<% end %>
</dd>
</div>
<% end %>
<% end %>
<% end %>
</dl>
Expand Down
8 changes: 8 additions & 0 deletions app/components/page_list_component/view.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,14 @@ def condition_description(condition)
end
end

def branch_condition_description(condition)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about calling this condition_description2 or something like that? branch_condition_description sounds to me like it's only for branch conditions.

if condition.answer_value.present?
I18n.t("page_conditions.branch_condition_description", goto_page_question_text: goto_page_text_for_condition(condition), answer_value: answer_value_text_for_condition(condition))
else
I18n.t("page_conditions.unconditional_description", goto_page_question_text: goto_page_text_for_condition(condition))
end
end

def condition_check_page_text(condition)
check_page = @pages.find { |page| page.id == condition.check_page_id }
I18n.t("page_conditions.condition_check_page_text", check_page_question_text: check_page.question_text)
Expand Down
4 changes: 3 additions & 1 deletion app/views/pages/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@

<div class="govuk-grid-row">
<div class="govuk-grid-column-two-thirds">
<%= render PageListComponent::ErrorSummary::View.new(current_form) %>
<% unless current_form.group.multiple_branches_enabled? %>
<%= render PageListComponent::ErrorSummary::View.new(current_form) %>
<% end %>

<h1 class="govuk-heading-l govuk-!-margin-bottom-2">
<span class="govuk-caption-l"><%= current_form.name %></span><span class="govuk-visually-hidden"> - </span>
Expand Down
3 changes: 3 additions & 0 deletions config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1358,6 +1358,7 @@ en:
If you pasted the web address, check you copied the entire address.
title: Page not found
page_conditions:
branch_condition_description: "%{answer_value} go to %{goto_page_question_text}"
condition_answer_value_text: "“%{answer_value}”"
condition_answer_value_text_with_errors: "[Answer not selected]"
condition_check_page_text: "“%{check_page_question_text}”"
Expand All @@ -1376,10 +1377,12 @@ en:
end_of_form: End of form
exit_page: Add an exit page
exit_page_label: An ‘exit page’ to leave the form
if_answer_is: 'If the answer is:'
none_of_the_above: None of the above
route: Route
secondary_skip_description: After %{check_page_question_text} go to %{goto_page_question_text}
skip_condition_route_page_text: "%{route_page_question_number}, “%{route_page_question_text}”"
unconditional_description: Go to %{goto_page_question_text}
page_route_card:
any_other_answer: Route for any other answer
conditional_answer_value: "%{answer_value}"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,15 @@ class PageListComponent::PageListComponentPreview < ViewComponent::Preview

def default
pages = []
form = build(:form, id: 0, pages:)
form = build(:form, :with_group, id: 0, pages:)
render(PageListComponent::View.new(pages:, form:))
end

def with_pages_and_no_conditions
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: [])]
form = build(:form, id: 0, pages:)
form = build(:form, :with_group, id: 0, pages:)
render(PageListComponent::View.new(pages:, form:))
end

Expand All @@ -20,7 +20,7 @@ def with_pages_and_one_condition
pages = [build(:page, id: 1, position: 1, question_text: "Enter your name", routing_conditions: [condition]),
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: [])]
form = build(:form, id: 0, pages:)
form = build(:form, :with_group, id: 0, pages:)

# We need to build the records rather than create them so that we don't save them to the database when we view the
# preview. However, this means that the associations aren't available so we need to manually set the associations
Expand All @@ -41,7 +41,7 @@ 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: [])]
form = build(:form, id: 0, pages:)
form = build(:form, :with_group, id: 0, pages:)

# We need to build the records rather than create them so that we don't save them to the database when we view the
# preview. However, this means that the associations aren't available so we need to manually set the associations
Expand All @@ -65,7 +65,7 @@ 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: [])]
form = build(:form, id: 1, pages:)
form = build(:form, :with_group, id: 1, pages:)

# We need to build the records rather than create them so that we don't save them to the database when we view the
# preview. However, this means that the associations aren't available so we need to manually set the associations
Expand Down
83 changes: 77 additions & 6 deletions spec/components/page_list_component/view_spec.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
require "rails_helper"

RSpec.describe PageListComponent::View, type: :component do
let(:form) { create :form }
let(:form) { create :form, :with_group }
let(:pages) { form.reload.pages }
let(:page_list_component) { described_class.new(pages:, form:) }

Expand Down Expand Up @@ -46,7 +46,7 @@
end

context "when the form has multiple pages" do
let(:form) { create :form, :with_pages }
let(:form) { create :form, :with_pages, :with_group }
let(:first_page) { form.pages.first }
let(:second_page) { form.pages.second }

Expand Down Expand Up @@ -79,7 +79,7 @@
end

context "when the form has conditions" do
let(:form) { create :form, :ready_for_routing }
let(:form) { create :form, :ready_for_routing, :with_group }
let(:edit_condition_path) { "/forms/0/pages/1/conditions/1" }

context "when the page has a single condition" do
Expand Down Expand Up @@ -144,7 +144,7 @@
end

context "when the form has a valid branch route" do
let(:form) { create :form, :ready_for_routing }
let(:form) { create :form, :ready_for_routing, :with_group }
let!(:secondary_skip_condition) { create :condition, routing_page_id: pages.second.id, check_page_id: pages.first.id, answer_value: nil, goto_page_id: pages.fourth.id }

before do
Expand All @@ -164,7 +164,7 @@
end

context "when the form has a branch route with an error" do
let(:form) { create :form, :ready_for_routing }
let(:form) { create :form, :ready_for_routing, :with_group }

before do
create :condition, routing_page_id: pages.first.id, check_page_id: pages.first.id, answer_value: "Option 1", goto_page_id: pages.third.id
Expand All @@ -183,7 +183,7 @@
end

describe "class methods" do
let(:form) { create :form, :with_pages }
let(:form) { create :form, :with_pages, :with_group }

describe "show_up_button" do
it "returns false when index is 0" do
Expand Down Expand Up @@ -322,5 +322,76 @@
end
end
end

describe "#branch_condition_description" do
context "when condition has all values set" do
let(:condition) do
create(
:condition,
routing_page_id: pages.first.id,
check_page_id: pages.first.id,
answer_value: "Option 1",
goto_page_id: pages.third.id,
)
end

it "returns complete condition description" do
expected_text = "“#{condition.answer_value}” go to #{pages.third.position}, “#{pages.third.question_text}”"
expect(page_list_component.branch_condition_description(condition)).to eq(expected_text)
end
end

context "when answer value is 'none_of_the_above'" do
let(:condition) do
create(
:condition,
routing_page_id: pages.first.id,
check_page_id: pages.first.id,
answer_value: "none_of_the_above",
goto_page_id: pages.third.id,
)
end

it "returns description with 'None of the above' text" do
expected_text = "“None of the above” go to #{pages.third.position}, “#{pages.third.question_text}”"
expect(page_list_component.branch_condition_description(condition)).to eq(expected_text)
end
end

context "when skip_to_end is true" do
let(:condition) do
create(
:condition,
routing_page_id: pages.first.id,
check_page_id: pages.first.id,
answer_value: "Option 1",
goto_page_id: nil,
skip_to_end: true,
)
end

it "returns description with 'Check your answers' text" do
expected_text = "“#{condition.answer_value}” go to end of form."
expect(page_list_component.branch_condition_description(condition)).to eq(expected_text)
end
end

context "when showing an unconditional route" do
let(:condition) do
create(
:condition,
routing_page_id: pages.second.id,
check_page_id: pages.first.id,
answer_value: nil,
goto_page_id: pages.fourth.id,
)
end

it "returns correct description" do
expected_text = "Go to #{pages.fourth.position}, “#{pages.fourth.question_text}”"
expect(page_list_component.branch_condition_description(condition)).to eq(expected_text)
end
end
end
end
end
2 changes: 1 addition & 1 deletion spec/views/pages/index.html.erb_spec.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
require "rails_helper"

describe "pages/index.html.erb" do
let(:form) { create :form, pages: }
let(:form) { create :form, :with_group, pages: }
let(:pages) { [] }
let(:mark_complete_input) { Forms::MarkPagesSectionCompleteInput.new(form:).assign_form_values }

Expand Down