From 2f9e766fb732e0d61e13a7bd7b5a08d67c9b459c Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Fri, 31 Jan 2025 13:15:29 +0200 Subject: [PATCH 1/7] Refactor routes/show view specs --- spec/views/pages/routes/show.html.erb_spec.rb | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/spec/views/pages/routes/show.html.erb_spec.rb b/spec/views/pages/routes/show.html.erb_spec.rb index fe66a93ef..47a910130 100644 --- a/spec/views/pages/routes/show.html.erb_spec.rb +++ b/spec/views/pages/routes/show.html.erb_spec.rb @@ -2,8 +2,9 @@ describe "pages/routes/show.html.erb" do let(:form) { build :form, id: 1, pages: [page] } - let(:page) { build :page, id: 1, position: 1 } - let(:routes) { [build(:condition)] } + let(:pages) { [page] } + let(:page) { build :page, id: 1, position: 1, routing_conditions: [build(:condition)] } + let(:routes) { PageRoutesService.new(form:, pages:, page:).routes } let(:route_cards) do [ @@ -20,7 +21,7 @@ before do allow(RouteSummaryCardDataPresenter).to receive(:new).and_return(route_summary_card_data_service) - render template: "pages/routes/show", locals: { current_form: form, page:, pages: form.pages, routes:, back_link_url: "/back" } + render template: "pages/routes/show", locals: { current_form: form, page:, pages:, routes:, back_link_url: "/back" } end it "has the correct title" do @@ -50,8 +51,10 @@ expect(rendered).to have_link(I18n.t("pages.go_to_your_questions"), href: form_pages_path(form.id)) end - context "when the page has routing conditions" do - let(:page) { build :page, id: 1, position: 1, routing_conditions: [build(:condition, id: 101)] } + context "when the page has a skip and a secondary skip" do + include_context "with pages with routing" + + let(:page) { page_with_skip_and_secondary_skip } it "has a link to delete all routes" do expect(rendered).to have_link(I18n.t("page_route_card.delete_route"), href: delete_routes_path(form.id, page.id)) From 74d8f0faab09c2723083264c412a5120bbeb867c Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Fri, 31 Jan 2025 13:19:02 +0200 Subject: [PATCH 2/7] Refactor routes/show view - Replace `ul` with `p` - Use `routes` instead of `page.routing_conditions` --- app/views/pages/routes/show.html.erb | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/app/views/pages/routes/show.html.erb b/app/views/pages/routes/show.html.erb index 17da9e2c4..aec2bdccd 100644 --- a/app/views/pages/routes/show.html.erb +++ b/app/views/pages/routes/show.html.erb @@ -20,17 +20,11 @@ <%= govuk_summary_list(**card) %> <% end %> - <% if page.routing_conditions.present? %> - - <% else %> - <%= govuk_link_to t("pages.go_to_your_questions"), form_pages_path(current_form.id) %> + <% if routes.any? %> + <%= govuk_button_link_to t("page_route_card.delete_route"), delete_routes_path(current_form.id, page.id), warning: true %> <% end %> +

+ <%= govuk_link_to t("pages.go_to_your_questions"), form_pages_path(current_form.id) %> +

From e0f482b326940fae4ed5442234ad8458df9a53dc Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Fri, 31 Jan 2025 15:29:14 +0200 Subject: [PATCH 3/7] Refactor RouteSummaryCardDataPresenter specs --- .../route_summary_card_data_presenter_spec.rb | 78 +++++++------------ .../shared_context/pages_with_routing.rb | 26 +++++++ 2 files changed, 55 insertions(+), 49 deletions(-) diff --git a/spec/presenters/route_summary_card_data_presenter_spec.rb b/spec/presenters/route_summary_card_data_presenter_spec.rb index a5d1b7bc9..eaf431f04 100644 --- a/spec/presenters/route_summary_card_data_presenter_spec.rb +++ b/spec/presenters/route_summary_card_data_presenter_spec.rb @@ -3,32 +3,17 @@ describe RouteSummaryCardDataPresenter do include Capybara::RSpecMatchers - subject(:service) { described_class.new(form:, pages:, page: current_page, routes:) } + subject(:service) { described_class.new(form:, pages:, page:, routes:) } - let(:form) { build :form, id: 99, pages: } + include_context "with pages with routing" - let(:current_page) do - build(:page, id: 1, position: 1, question_text: "Current Question", next_page: next_page.id, routing_conditions:) - end + let(:form) { build :form, id: 99, pages: } + let(:page) { page_with_skip_route } let(:routes) do - PageRoutesService.new(form:, pages:, page: current_page).routes - end - - let(:next_page) do - build(:page, id: 2, position: 2, question_text: "Next Question", routing_conditions: next_page_routing_conditions) - end - - let(:pages) { [current_page, next_page] } - - let(:routing_condition) do - build(:condition, id: 1, routing_page_id: 1, check_page_id: 1, answer_value: "Yes", goto_page_id: 2, skip_to_end: false) + PageRoutesService.new(form:, pages:, page:).routes end - let(:routing_conditions) { [routing_condition] } - - let(:next_page_routing_conditions) { [] } - before do allow(form).to receive(:group).and_return(build(:group)) end @@ -41,26 +26,28 @@ # conditional route expect(result[0][:card][:title]).to eq("Route 1") - expect(result[0][:card][:actions].first).to have_link("Edit", href: "/forms/99/pages/1/conditions/1") - expect(result[0][:rows][0][:value][:text]).to eq("Yes") - expect(result[0][:rows][1][:value][:text]).to eq("2. Next Question") + expect(result[0][:card][:actions].first).to have_link("Edit", href: "/forms/99/pages/10/conditions/3") + expect(result[0][:rows][0][:value][:text]).to eq("Skip") + expect(result[0][:rows][1][:value][:text]).to eq("12. Question") # default route expect(result[1][:card][:title]).to eq("Route for any other answer") expect(result[1][:card][:actions][0]).to be_nil - expect(result[1][:rows][0][:value][:text]).to eq("2. Next Question") + expect(result[1][:rows][0][:value][:text]).to eq("11. Question to be skipped") end context "with branch_routing enabled", :feature_branch_routing do it "has the link to create a secondary skip" do result = service.summary_card_data - expect(result[1][:rows][1][:value][:text]).to have_link("Set one or more questions to skip later in the form (optional)", href: "/forms/99/pages/1/routes/any-other-answer/questions-to-skip/new") + expect(result[1][:rows][1][:value][:text]).to have_link("Set one or more questions to skip later in the form (optional)", href: "/forms/99/pages/10/routes/any-other-answer/questions-to-skip/new") end end context "when the goto_page does not exist" do - let(:routing_condition) do - build(:condition, id: 1, routing_page_id: 1, check_page_id: 1, answer_value: "Yes", goto_page_id: 999, skip_to_end: false) + before do + page.routing_conditions.first.tap do |condition| + condition.goto_page_id = 999 + end end it "uses placeholder text instead of question text" do @@ -71,8 +58,11 @@ end context "with skip to end condition" do - let(:routing_condition) do - build(:condition, id: 1, routing_page_id: 1, check_page_id: 1, answer_value: "No", goto_page_id: nil, skip_to_end: true) + before do + page.routing_conditions.first.tap do |condition| + condition.goto_page_id = nil + condition.skip_to_end = true + end end it 'shows "Check your answers" as destination' do @@ -81,22 +71,14 @@ end end - context "with a route with no answer_value" do - let(:routing_conditions) do - [build(:condition, id: 1, routing_page_id: 1, check_page_id: 1, answer_value: "Yes", goto_page_id: 2, skip_to_end: false)] - end - - let(:next_page_routing_conditions) do - [ - build(:condition, id: 2, routing_page_id: 2, check_page_id: 1, answer_value: nil, goto_page_id: nil, skip_to_end: true), - ] - end + context "with conditional routes and secondary skip routes" do + let(:page) { page_with_skip_and_secondary_skip } - it 'shows "Check your answers" as destination' do + it "shows the destination of the secondary skip route" do result = service.summary_card_data - expect(result[1][:rows][0][:value][:text]).to eq("2. Next Question") - expect(result[1][:rows][1][:value][:text]).to eq("2. Next Question") - expect(result[1][:rows][2][:value][:text]).to eq("Check your answers before submitting") + expect(result[1][:rows][0][:value][:text]).to eq("3. Question in branch 1") + expect(result[1][:rows][1][:value][:text]).to eq("4. Question at the end of branch 1 (start of a secondary skip)") + expect(result[1][:rows][2][:value][:text]).to eq("8. Question after a branch route (end of a secondary skip)") end context "when branch routing is not enabled", feature_branch_routing: false do @@ -109,18 +91,18 @@ context "with branch_routing enabled", :feature_branch_routing do it "shows the edit secondary skip link" do result = service.summary_card_data - expect(result[1][:card][:actions].first).to have_link("Edit", href: "/forms/99/pages/1/routes/any-other-answer/questions-to-skip") + expect(result[1][:card][:actions].first).to have_link("Edit", href: "/forms/99/pages/2/routes/any-other-answer/questions-to-skip") end it "shows the delete secondary skip link" do result = service.summary_card_data - expect(result[1][:card][:actions].second).to have_link("Delete", href: "/forms/99/pages/1/routes/any-other-answer/questions-to-skip/delete") + expect(result[1][:card][:actions].second).to have_link("Delete", href: "/forms/99/pages/2/routes/any-other-answer/questions-to-skip/delete") end end end context "with no conditional routes" do - let(:routing_conditions) { [] } + let(:page) { page_with_no_routes } it "returns only the default route card" do result = service.summary_card_data @@ -130,9 +112,7 @@ end context "when page is the last page" do - let(:current_page) do - build(:page, id: 1, position: 1, question_text: "Current Question", next_page: nil) - end + let(:page) { pages.last } it 'shows "Check your answers" as default destination' do result = service.summary_card_data diff --git a/spec/support/shared_context/pages_with_routing.rb b/spec/support/shared_context/pages_with_routing.rb index 17976d952..362130e85 100644 --- a/spec/support/shared_context/pages_with_routing.rb +++ b/spec/support/shared_context/pages_with_routing.rb @@ -4,17 +4,22 @@ build( :page, id: 1, + position: 1, + next_page: 2, question_text: "Question", ), build( :page, :with_selection_settings, id: 2, + position: 2, + next_page: 3, question_text: "Branch question (start of a route)", selection_options: [{ name: "First branch" }, { name: "Second branch" }], routing_conditions: [ build( :condition, + id: 1, answer_value: "Second branch", check_page_id: 2, goto_page_id: 5, @@ -25,15 +30,20 @@ build( :page, id: 3, + position: 3, + next_page: 4, question_text: "Question in branch 1", ), build( :page, id: 4, + position: 4, + next_page: 5, question_text: "Question at the end of branch 1 (start of a secondary skip)", routing_conditions: [ build( :condition, + id: 2, answer_value: nil, check_page_id: 2, goto_page_id: 8, @@ -44,37 +54,50 @@ build( :page, id: 5, + position: 5, + next_page: 6, question_text: "Question at the start of branch 2 (end of a route)", ), build( :page, id: 6, + position: 6, + next_page: 7, question_text: "Question in branch 2", ), build( :page, id: 7, + position: 7, + next_page: 8, question_text: "Question at the end of branch 2", ), build( :page, id: 8, + position: 8, + next_page: 9, question_text: "Question after a branch route (end of a secondary skip)", ), build( :page, id: 9, + position: 9, + next_page: 10, question_text: "Question", ), build( :page, :with_selection_settings, id: 10, + position: 10, + next_page: 11, question_text: "Skip question", selection_options: [{ name: "Skip" }, { name: "Don't skip" }], routing_conditions: [ build( :condition, + id: 3, answer_value: "Skip", check_page_id: 10, goto_page_id: 12, @@ -85,11 +108,14 @@ build( :page, id: 11, + position: 11, + next_page: 12, question_text: "Question to be skipped", ), build( :page, id: 12, + position: 12, question_text: "Question", ), ] From 92ce8fab75643f3a9b260e08b8eaa18bd9efdd86 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Tue, 4 Feb 2025 09:35:44 +0200 Subject: [PATCH 4/7] Change ready_for_routing trait to set next_page and position for pages Make sure that when setting up a form with lots of pages with the `:ready_for_routing` trait that those pages are correctly set up with all the proper metadata. --- spec/factories/models/forms.rb | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/spec/factories/models/forms.rb b/spec/factories/models/forms.rb index 0e05e47d9..a84811c1e 100644 --- a/spec/factories/models/forms.rb +++ b/spec/factories/models/forms.rb @@ -106,6 +106,19 @@ pages do Array.new(pages_count) { association(:page, :with_selection_settings) } end + + after(:build) do |form| + if form.pages.present? + # assign position and next_page to each page + form.pages.each.with_index(1).each_cons(2) do |page_with_index, next_page_with_index| + page, page_index = page_with_index + next_page, _next_page_index = next_page_with_index + + page.position = page_index + page.next_page = next_page&.id + end + end + end end trait :missing_pages do From 75e8846cd15b4f008e6c41b6b653a4fa676e9c6e Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Fri, 31 Jan 2025 13:21:31 +0200 Subject: [PATCH 5/7] Do not show delete all routes button if there is only one route --- app/views/pages/routes/show.html.erb | 2 +- spec/views/pages/routes/show.html.erb_spec.rb | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/app/views/pages/routes/show.html.erb b/app/views/pages/routes/show.html.erb index aec2bdccd..208affeb8 100644 --- a/app/views/pages/routes/show.html.erb +++ b/app/views/pages/routes/show.html.erb @@ -20,7 +20,7 @@ <%= govuk_summary_list(**card) %> <% end %> - <% if routes.any? %> + <% if routes.many? %> <%= govuk_button_link_to t("page_route_card.delete_route"), delete_routes_path(current_form.id, page.id), warning: true %> <% end %> diff --git a/spec/views/pages/routes/show.html.erb_spec.rb b/spec/views/pages/routes/show.html.erb_spec.rb index 47a910130..8a39cedcc 100644 --- a/spec/views/pages/routes/show.html.erb_spec.rb +++ b/spec/views/pages/routes/show.html.erb_spec.rb @@ -51,6 +51,16 @@ expect(rendered).to have_link(I18n.t("pages.go_to_your_questions"), href: form_pages_path(form.id)) end + context "when the page has a skip route" do + include_context "with pages with routing" + + let(:page) { page_with_skip_route } + + it "does not have a link to delete all routes" do + expect(rendered).not_to have_link(I18n.t("page_route_card.delete_route"), href: delete_routes_path(form.id, page.id)) + end + end + context "when the page has a skip and a secondary skip" do include_context "with pages with routing" From 7a6c6c1b0e4b428ae5459048ac78d7d86111cff1 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Mon, 3 Feb 2025 13:21:32 +0200 Subject: [PATCH 6/7] Don't show summary card for default route --- .../route_summary_card_data_presenter.rb | 7 +++-- .../route_summary_card_data_presenter_spec.rb | 30 +++---------------- 2 files changed, 8 insertions(+), 29 deletions(-) diff --git a/app/presenters/route_summary_card_data_presenter.rb b/app/presenters/route_summary_card_data_presenter.rb index c6529f36c..3f77d98ab 100644 --- a/app/presenters/route_summary_card_data_presenter.rb +++ b/app/presenters/route_summary_card_data_presenter.rb @@ -13,8 +13,9 @@ def initialize(form:, pages:, page:, routes:) end def summary_card_data - conditional_cards = conditional_route_cards - conditional_cards + [default_route_card] + cards = conditional_route_cards + cards << secondary_skip_card if secondary_skip + cards end private @@ -55,7 +56,7 @@ def conditional_route_card(routing_condition, index) } end - def default_route_card + def secondary_skip_card continue_to_name = page.has_next_page? ? page_name(page.next_page) : end_page_name actions = if FeatureService.new(group: form.group).enabled?(:branch_routing) && secondary_skip diff --git a/spec/presenters/route_summary_card_data_presenter_spec.rb b/spec/presenters/route_summary_card_data_presenter_spec.rb index eaf431f04..1c6c4018b 100644 --- a/spec/presenters/route_summary_card_data_presenter_spec.rb +++ b/spec/presenters/route_summary_card_data_presenter_spec.rb @@ -20,27 +20,15 @@ describe "#summary_card_data" do context "with conditional routes" do - it "returns an array of route cards including conditional and default routes" do + it "returns an array of route cards" do result = service.summary_card_data - expect(result.length).to eq(2) # 1 conditional + 1 default route + expect(result.length).to eq(1) # 1 conditional route # conditional route expect(result[0][:card][:title]).to eq("Route 1") expect(result[0][:card][:actions].first).to have_link("Edit", href: "/forms/99/pages/10/conditions/3") expect(result[0][:rows][0][:value][:text]).to eq("Skip") expect(result[0][:rows][1][:value][:text]).to eq("12. Question") - - # default route - expect(result[1][:card][:title]).to eq("Route for any other answer") - expect(result[1][:card][:actions][0]).to be_nil - expect(result[1][:rows][0][:value][:text]).to eq("11. Question to be skipped") - end - - context "with branch_routing enabled", :feature_branch_routing do - it "has the link to create a secondary skip" do - result = service.summary_card_data - expect(result[1][:rows][1][:value][:text]).to have_link("Set one or more questions to skip later in the form (optional)", href: "/forms/99/pages/10/routes/any-other-answer/questions-to-skip/new") - end end context "when the goto_page does not exist" do @@ -104,19 +92,9 @@ context "with no conditional routes" do let(:page) { page_with_no_routes } - it "returns only the default route card" do - result = service.summary_card_data - expect(result.length).to eq(1) - expect(result[0][:card][:title]).to eq("Route for any other answer") - end - end - - context "when page is the last page" do - let(:page) { pages.last } - - it 'shows "Check your answers" as default destination' do + it "returns an empty array" do result = service.summary_card_data - expect(result[0][:rows][0][:value][:text]).to eq("Check your answers before submitting") + expect(result).to eq [] end end end From 0a93b8d17051b29f22cea5d46b42d97d46b87953 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Tue, 4 Feb 2025 09:35:58 +0200 Subject: [PATCH 7/7] Show information about any other answer route on show routes page --- app/controllers/pages/routes_controller.rb | 6 ++- app/views/pages/routes/show.html.erb | 12 +++++ config/locales/en.yml | 6 +++ spec/views/pages/routes/show.html.erb_spec.rb | 48 +++++++++++++++++-- 4 files changed, 67 insertions(+), 5 deletions(-) diff --git a/app/controllers/pages/routes_controller.rb b/app/controllers/pages/routes_controller.rb index 8ec885754..07229ba21 100644 --- a/app/controllers/pages/routes_controller.rb +++ b/app/controllers/pages/routes_controller.rb @@ -3,7 +3,11 @@ def show back_link_url = form_pages_path(current_form.id) pages = FormRepository.pages(current_form) routes = PageRoutesService.new(form: current_form, pages:, page:).routes - render locals: { current_form:, page:, pages:, routes:, back_link_url: } + + # to be eligible for route question has to have one question after it, so should always have next_page + next_page = pages.find(proc { raise "Cannot find page with id #{page.next_page.inspect}" }) { _1.id == page.next_page } + + render locals: { current_form:, page:, pages:, next_page:, routes:, back_link_url: } end def delete diff --git a/app/views/pages/routes/show.html.erb b/app/views/pages/routes/show.html.erb index 208affeb8..cc05a9fac 100644 --- a/app/views/pages/routes/show.html.erb +++ b/app/views/pages/routes/show.html.erb @@ -20,6 +20,18 @@ <%= govuk_summary_list(**card) %> <% end %> + <% if routes.map(&:secondary_skip?).none? %> +

<%= t(".any_other_answer.heading") %>

+ +

<%= t(".any_other_answer.will_continue_to", next_page_position: next_page.position) %>

+ + <% if FeatureService.new(group: current_form.group).enabled? :branch_routing %> +

<%= t(".any_other_answer.skip_later") %>

+ + <%= govuk_button_link_to t(".any_other_answer.set_questions_to_skip"), new_secondary_skip_path(current_form.id, page.id), secondary: true %> + <% end %> + <% end %> + <% if routes.many? %> <%= govuk_button_link_to t("page_route_card.delete_route"), delete_routes_path(current_form.id, page.id), warning: true %> <% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 473d1fcad..f1f85210b 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1199,6 +1199,12 @@ en: back: Back to question %{page_number}’s routes caption: Question %{page_number}’s routes title: Are you sure you want to delete all question %{page_number}’s routes? + show: + any_other_answer: + heading: Any other answer + set_questions_to_skip: Set questions to skip + skip_later: If you need to, you can make these people skip one or more questions later in the form. For example, you might want them to skip route 1’s questions to create 2 different ‘branches’ of questions. + will_continue_to: People who select any other answer will continue to question %{next_page_position}. submit_save: Save question payment_link_input: body_html: | diff --git a/spec/views/pages/routes/show.html.erb_spec.rb b/spec/views/pages/routes/show.html.erb_spec.rb index 8a39cedcc..4eff0c6dd 100644 --- a/spec/views/pages/routes/show.html.erb_spec.rb +++ b/spec/views/pages/routes/show.html.erb_spec.rb @@ -2,8 +2,9 @@ describe "pages/routes/show.html.erb" do let(:form) { build :form, id: 1, pages: [page] } - let(:pages) { [page] } - let(:page) { build :page, id: 1, position: 1, routing_conditions: [build(:condition)] } + let(:pages) { [page, next_page] } + let(:page) { build :page, id: 1, position: 1, next_page: 2, routing_conditions: [build(:condition)] } + let(:next_page) { build :page, id: 2 } let(:routes) { PageRoutesService.new(form:, pages:, page:).routes } let(:route_cards) do @@ -21,7 +22,8 @@ before do allow(RouteSummaryCardDataPresenter).to receive(:new).and_return(route_summary_card_data_service) - render template: "pages/routes/show", locals: { current_form: form, page:, pages:, routes:, back_link_url: "/back" } + allow(form).to receive(:group).and_return(build(:group)) + render template: "pages/routes/show", locals: { current_form: form, page:, pages:, next_page:, routes:, back_link_url: "/back" } end it "has the correct title" do @@ -51,7 +53,7 @@ expect(rendered).to have_link(I18n.t("pages.go_to_your_questions"), href: form_pages_path(form.id)) end - context "when the page has a skip route" do + context "when the page has a single skip route" do include_context "with pages with routing" let(:page) { page_with_skip_route } @@ -61,6 +63,39 @@ end end + context "when the page does not have a secondary skip route" do + include_context "with pages with routing" + + let(:page) { page_with_skip_route } + let(:next_page) { pages.find { _1.id == page_with_skip_route.next_page } } + + it "has an any other answer section" do + expect(rendered).to have_css "h2.govuk-heading-m", text: "Any other answer" + end + + describe "any other answer section" do + it "shows the number of the next question in the form" do + expect(rendered).to have_text "People who select any other answer will continue to question 11." + end + + context "when the page is the last question" do + it "shows the check your answers page as the next question in the form" do + expect(rendered).to have_text "People who select any other answer will continue to question 11." + end + end + + context "when branch routing is enabled", :feature_branch_routing do + it "has a link to set questions to skip" do + expect(rendered).to have_link( + "Set questions to skip", + class: "govuk-button--secondary", + href: new_secondary_skip_path(form.id, page.id), + ) + end + end + end + end + context "when the page has a skip and a secondary skip" do include_context "with pages with routing" @@ -69,5 +104,10 @@ it "has a link to delete all routes" do expect(rendered).to have_link(I18n.t("page_route_card.delete_route"), href: delete_routes_path(form.id, page.id)) end + + it "does not have an any other answer section" do + expect(rendered).not_to have_css "h2", text: "Any other answer" + expect(rendered).not_to have_link "Set questions to skip" + end end end