Merge pull request #1742 from alphagov/make-second-route-easier-fflag

Make it easier to find where and how to add second route

app/controllers/pages/conditions_controller.rb

@@ -2,17 +2,17 @@ class Pages::ConditionsController < PagesController
   before_action :can_add_page_routing, only: %i[new create]
 
   def routing_page
-    routing_page_input = Pages::RoutingPageInput.new(routing_page_id: params[:routing_page_id])
+    routing_page_input = Pages::RoutingPageInput.new({ routing_page_id: params[:routing_page_id] }, branch_routing_enabled:)
     render template: "pages/conditions/routing_page", locals: { form: current_form, routing_page_input: }
   end
 
   def set_routing_page
     routing_page_id = params[:pages_routing_page_input][:routing_page_id]
-    routing_page_input = Pages::RoutingPageInput.new(routing_page_id:)
+    routing_page_input = Pages::RoutingPageInput.new({ routing_page_id: }, branch_routing_enabled:)
 
     if routing_page_input.valid?
       routing_page = PageRepository.find(page_id: routing_page_id, form_id: current_form.id)
-      redirect_to new_condition_path(current_form.id, routing_page.id)
+      redirect_to new_condtion_or_skip_path(routing_page)
     else
       render template: "pages/conditions/routing_page", locals: { form: current_form, routing_page_input: }, status: :unprocessable_entity
     end
@@ -96,4 +96,16 @@ def condition_input_params
   def delete_condition_input_params
     params.require(:pages_delete_condition_input).permit(:answer_value, :goto_page_id, :confirm).merge(form: current_form, page:)
   end
+
+  def new_condtion_or_skip_path(page)
+    if FeatureService.new(group: current_form.group).enabled?(:branch_routing) && page.routing_conditions.present?
+      return new_secondary_skip_path(form_id: current_form.id, page_id: page.id)
+    end
+
+    new_condition_path(current_form.id, page.id)
+  end
+
+  def branch_routing_enabled
+    FeatureService.new(group: current_form.group).enabled?(:branch_routing)
+  end
 end

app/input_objects/pages/routing_page_input.rb

@@ -1,5 +1,24 @@
 class Pages::RoutingPageInput < BaseInput
   attr_accessor :routing_page_id
 
-  validates :routing_page_id, presence: true
+  validate :routing_page_id_present
+
+  def initialize(attributes = {}, branch_routing_enabled: false)
+    @branch_routing_enabled = branch_routing_enabled
+    super(attributes)
+  end
+
+private
+
+  def blank_error_symbol
+    if @branch_routing_enabled
+      :blank
+    else
+      :branch_routing_disabled_blank
+    end
+  end
+
+  def routing_page_id_present
+    errors.add(:routing_page_id, blank_error_symbol) if routing_page_id.blank?
+  end
 end

app/resources/api/v1/form_resource.rb

@@ -19,7 +19,12 @@ def group
   end
 
   def qualifying_route_pages
-    Api::V1::PageResource.qualifying_route_pages(pages)
+    max_routes_per_page = FeatureService.new(group:).enabled?(:branch_routing) ? 2 : 1
+
+    conditions = pages.flat_map(&:routing_conditions).compact_blank
+    condition_counts = conditions.group_by(&:check_page_id).transform_values(&:length)
+
+    Api::V1::PageResource.qualifying_route_pages(pages).filter { |page| condition_counts.fetch(page.id, 0) < max_routes_per_page }
   end
 
   def has_no_remaining_routes_available?

app/resources/api/v1/page_resource.rb

@@ -60,7 +60,7 @@ def show_optional_suffix?
   def self.qualifying_route_pages(pages)
     pages.filter do |page|
       page.answer_type == "selection" && page.answer_settings.only_one_option == "true" &&
-        page.position != pages.length && page.routing_conditions.empty?
+        page.position != pages.length
     end
   end
 end

app/views/pages/conditions/_routing_options.html.erb

@@ -1,36 +1,33 @@
+<% branch_routing_enabled = FeatureService.new(group: form.group).enabled?(:branch_routing) %>
+<% prefix = branch_routing_enabled ? 'branch_routing.' : '' %>
+
 <%= form_with(model: routing_page_input, url: set_routing_page_path(form.id), method: 'POST') do |f| %>
   <% if routing_page_input&.errors.any? %>
     <%= f.govuk_error_summary %>
   <% end %>
+
   <h1 class="govuk-heading-l">
     <span class="govuk-caption-l"><%= form.name %> </span>
     <%= t("page_titles.routing_page") %>
   </h1>
 
-  <p>
-    <%= t("routing_page.body_routing_text") %>
-  </p>
+  <%= t("routing_page.#{prefix}body_html") %>
 
-  <% if FeatureService.new(group: form.group).enabled?(:branch_routing) %>
-  <p>
-    <%= t("routing_page.body_branching_text") %>
-  </p>
-  <% end %>
+  <% hint_options = branch_routing_enabled ? {} : { hint: { text: t("routing_page.legend_hint_text") } } %>
 
   <% if form.qualifying_route_pages.length <= 10 %>
     <%= f.govuk_collection_radio_buttons :routing_page_id,
-                                         form.qualifying_route_pages,
-                                         :id, :question_with_number,
-                                         legend: { text: t("routing_page.legend_text"), size: 'm' },
-                                         hint:{ text: t("routing_page.legend_hint_text") }
+      form.qualifying_route_pages,
+      :id, :question_with_number,
+      **{ legend: { text: t("routing_page.legend_text"), size: 'm' } }.merge(hint_options)
     %>
   <% else %>
     <%= f.govuk_collection_select :routing_page_id,
-                                  form.qualifying_route_pages,
-                                  :id, :question_with_number,
-                                  label: { text: t("routing_page.legend_text"), size: 'm' },
-                                  hint: { text: t("routing_page.legend_hint_text") },
-                                  options: { include_blank: t("routing_page.dropdown_default_text") }
+      form.qualifying_route_pages,
+      :id, :question_with_number,
+      **{ label: { text: t("routing_page.legend_text"), size: 'm' },
+        options: { include_blank: t("routing_page.dropdown_default_text") }
+      }.merge(hint_options)
     %>
   <% end %>
 

app/views/pages/conditions/routing_page.html.erb

@@ -2,6 +2,8 @@
 <% content_for :back_link, govuk_back_link_to(form_pages_path(form.id)) %>
 <div class="govuk-grid-row">
   <div class="govuk-grid-column-two-thirds">
+    <% branch_routing_enabled = FeatureService.new(group: form.group).enabled?(:branch_routing) %>
+    <% prefix = branch_routing_enabled ? 'branch_routing.' : '' %>
 
     <% if policy(form).can_add_page_routing_conditions? %>
       <%= render partial: "pages/conditions/routing_options", locals: {form:, routing_page_input:} %>
@@ -11,24 +13,11 @@
         <%= t("page_titles.routing_page") %>
       </h1>
 
-      <p>
-        <%= t("routing_page.body_routing_text") %>
-      </p>
+      <%= t("routing_page.#{prefix}body_html") %>
 
-      <% if FeatureService.new(group: form.group).enabled?(:branch_routing) %>
-      <p>
-        <%= t("routing_page.body_branching_text") %>
-      </p>
-      <% end %>
-
-      <% if form.has_no_remaining_routes_available? %>
-        <h2 class="govuk-heading-m"><%= t("routing_page.no_remaining_routes_heading") %></h2>
-        <p><%= t("routing_page.no_remaining_routes") %></p>
-      <% else %>
-        <h2 class="govuk-heading-m"><%= t("routing_page.routing_requirements_not_met_heading") %></h2>
-        <%= t('routing_page.routing_requirements_not_met_html') %>
-      <% end %>
+      <%= t("routing_page.#{prefix}#{form.has_no_remaining_routes_available? ? 'no_remaining_routes' : 'routing_requirements_not_met'}_html") %>
     <% end %>
+
     <p>
       <%= govuk_link_to t('pages.go_to_your_questions'), form_pages_path(form.id) %>
     </p>

config/i18n-tasks.yml

@@ -132,6 +132,7 @@ ignore_unused:
 - 'errors.*'
 - 'date.formats.*'
 - 'helpers.*'
+- 'routing_page.branch_routing.*'
 
 ## Exclude these keys from the `i18n-tasks eq-base' report:
 # ignore_eq_base:

config/locales/en.yml

@@ -1379,15 +1379,35 @@ en:
         user_count: User count
       title: Number of users per organisation
   routing_page:
-    body_branching_text: People who select any other answer will continue to the next question. If you need to, you can make them skip one or more questions later in the form.
-    body_routing_text: You can set up a route so if someone selects a specific answer to a question they’ll skip forward to a later question, or the end of the form.
+    body_html: "<p>You can set up a route so if someone selects a specific answer to a question they’ll skip forward to a later question, or the end of the form.</p>\n"
+    branch_routing:
+      body_html: |
+        <p> You can add a route from a question where people can select only one answer from a list.</p>
+        <p> If someone selects the answer you specify, they’ll skip forward to a later question, or the end of the form.</p>
+        <p> People who select any other answer will continue to the next question. If you need to, you can add a second route so they skip one or more questions later in the form.</p>
+        <p> You can only add a route from a question that does not already have 2 routes. Go back to your questions if you need to edit an existing route.</p>
+      no_remaining_routes_html: |
+        <h2 class="govuk-heading-m">You have no more questions to add a route from</h2>
+        <p>You can only add a route from a question that does not already have 2 routes. If you need to edit an existing route, go back to your questions.</p>
+      routing_requirements_not_met_html: |
+        <h2 class="govuk-heading-m">You cannot add a route yet</h2>
+
+        <p>Before you can add a route, your form needs to have a question that:</p>
+
+        <ul class="govuk-list govuk-list--bullet">
+          <li>asks people to select only one option from a list</li>
+          <li>has at least one question after it</li>
+        </ul>
+
+        <p>It’s usually easiest to create all your form’s questions first, then add the routes you need.</p>
     dropdown_default_text: Select a question to start your route from
     legend_hint_text: A route can only start from a question where people select one item from a list. You can only add one route from each question.
     legend_text: Which question do you want your route to start from?
-    no_remaining_routes: A route can only start from a question where people select one item from a list. You can only add one route from each question.
-    no_remaining_routes_heading: You have no more questions to start a route from
-    routing_requirements_not_met_heading: What you need to create a route
+    no_remaining_routes_html: |
+      <h2 class="govuk-heading-m">You have no more questions to start a route from</h2>
+      <p> A route can only start from a question where people select one item from a list. You can only add one route from each question.</p>
     routing_requirements_not_met_html: |
+      <h2 class="govuk-heading-m">What you need to create a route</h2>
       <p>Before you can add a route you’ll need:</p>
       <ul class="govuk-list govuk-list--bullet">
         <li>2 or more questions</li>

config/locales/pages/conditions.yml

@@ -20,7 +20,8 @@ en:
         pages/routing_page_input:
           attributes:
             routing_page_id:
-              blank: Select the question you want your route to start from
+              blank: Select the question you want to add a route from
+              branch_routing_disabled_blank: Select the question you want your route to start from
   helpers:
     label:
       pages_conditions_input:

spec/input_objects/pages/routing_page_input_spec.rb

@@ -1,17 +1,29 @@
 require "rails_helper"
 
 RSpec.describe Pages::RoutingPageInput, type: :model do
-  let(:routing_page_input) { described_class.new(routing_page_id:) }
+  let(:routing_page_input) { described_class.new({ routing_page_id: }, branch_routing_enabled:) }
   let(:form) { build :form, :ready_for_routing, id: 1 }
   let(:pages) { form.pages }
   let(:routing_page_id) { pages.first.id }
+  let(:branch_routing_enabled) { false }
 
   describe "validations" do
     it "is invalid if routing_page_id is nil" do
-      error_message = I18n.t("activemodel.errors.models.pages/routing_page_input.attributes.routing_page_id.blank")
+      error_message = I18n.t("activemodel.errors.models.pages/routing_page_input.attributes.routing_page_id.branch_routing_disabled_blank")
       routing_page_input.routing_page_id = nil
       expect(routing_page_input).to be_invalid
       expect(routing_page_input.errors.full_messages_for(:routing_page_id)).to include("Routing page #{error_message}")
     end
+
+    context "when branch_routing_enabled is true" do
+      let(:branch_routing_enabled) { true }
+
+      it "is invalid if routing_page_id is nil" do
+        error_message = I18n.t("activemodel.errors.models.pages/routing_page_input.attributes.routing_page_id.blank")
+        routing_page_input.routing_page_id = nil
+        expect(routing_page_input).to be_invalid
+        expect(routing_page_input.errors.full_messages_for(:routing_page_id)).to include("Routing page #{error_message}")
+      end
+    end
   end
 end

spec/policies/form_policy_spec.rb

@@ -123,16 +123,20 @@
       end
 
       context "and the form only has a selection question with an existing route" do
-        let(:routing_conditions) { [(build :condition, id: 1, check_page_id: 1, answer_value: "Wales", goto_pageid: 2)] }
-        let(:pages) { [(build :page, :with_selection_settings, position: 1, id: 1, routing_conditions:), (build :page, position: 2, id: 2)] }
+        let(:pages) do
+          [
+            build(:page, :with_selection_settings, position: 1, id: 1, routing_conditions: build(:condition, id: 1, routing_page_id: 1, check_page_id: 1, answer_value: "Wales", goto_pageid: 3)),
+            build(:page, :with_simple_answer_type, position: 2, id: 2, routing_conditions: build(:condition, id: 2, routing_page_id: 2, check_page_id: 1, goto_pageid: nil, skip_to_end: true)),
+            build(:page, position: 3, id: 3),
+          ]
+        end
 
         it { is_expected.to forbid_actions(%i[can_add_page_routing_conditions]) }
       end
 
       context "and the form has a selection question without an existing route" do
         context "and the available selection question is the last page in the form" do
-          let(:routing_conditions) { [(build :condition, id: 1, check_page_id: 1, answer_value: "Wales", goto_pageid: 2)] }
-          let(:pages) { [(build :page, :with_selection_settings, position: 1, id: 1, routing_conditions:), (build :page, position: 2, id: 2), (build :page, :with_selection_settings, position: 3, id: 3)] }
+          let(:pages) { [(build :page, position: 1, id: 1), (build :page, position: 2, id: 2), (build :page, :with_selection_settings, position: 3, id: 3)] }
 
           it { is_expected.to forbid_actions(%i[can_add_page_routing_conditions]) }
         end

spec/requests/pages/conditions_controller_spec.rb

@@ -62,6 +62,21 @@
       expect(response).to redirect_to new_condition_path(form.id, selected_page.id)
     end
 
+    context "when the page already has a condition associated with it" do
+      let(:selected_page) do
+        page.routing_conditions = [(build :condition, id: 1, check_page_id: page.id, goto_page_id: 2)]
+        page
+      end
+
+      it "when branch_routing enabled, redirects the user to the new skip condition page", :feature_branch_routing do
+        expect(response).to redirect_to new_secondary_skip_path(form.id, selected_page.id)
+      end
+
+      it "when branch_routing disabled, redirects the user to the new conditions page" do
+        expect(response).to redirect_to new_condition_path(form.id, selected_page.id)
+      end
+    end
+
     context "when user should not be allowed to add routes to pages" do
       let(:user) { build :user }