Merge pull request #2838 from govuk-forms/backward-route-validate

Backward route validate

Author: thomasiles

app/components/page_list_component/error_summary/view.rb

@@ -10,6 +10,10 @@ def self.error_id(number)
         "condition_#{number}"
       end
 
+      def self.page_error_id(page)
+        "page-#{page.id}"
+      end
+
       def self.generate_error_message(error_name, condition:, page:)
         # TODO: route_number is hardcoded as 1 here because we know there can be only two conditions. It will need to change in future
         # https://trello.com/c/BfkZEIgM/3446-set-route-count-dynamically-instead-of-hard-coding-it
@@ -24,6 +28,18 @@ def self.generate_error_message(error_name, condition:, page:)
         I18n.t(defaults.first, default: defaults.drop(1), scope:, **interpolation_variables)
       end
 
+      def self.multiple_branch_error_message(page)
+        page_errors = page.routing_conditions.filter(&:warning_goto_page_before_routing_page)
+
+        return if page_errors.blank?
+
+        if Forms::RoutesInput.route_with_selection_options?(page)
+          I18n.t("errors.routes.page_list.cannot_route_backwards_from_selection_page", question_number: page.position, count: page_errors.count)
+        else
+          I18n.t("errors.routes.page_list.cannot_route_backwards_from_generic_page", question_number: page.position)
+        end
+      end
+
       def error_object(error_name:, condition:, page:)
         OpenStruct.new(
           message: self.class.generate_error_message(error_name, condition:, page:),
@@ -32,15 +48,28 @@ def error_object(error_name:, condition:, page:)
       end
 
       def errors_for_summary
-        @form.conditions.map { |condition|
-          condition.validation_errors.map do |error|
-            error_object(
-              error_name: error.name,
-              page: condition.check_page,
-              condition: condition,
+        if FeatureService.new(group: @form.group).enabled?(:multiple_branches)
+          @form.pages.map { |page|
+            error_message = self.class.multiple_branch_error_message(page)
+
+            next if error_message.blank?
+
+            OpenStruct.new(
+              message: error_message,
+              link: "##{self.class.page_error_id(page)}",
             )
-          end
-        }.flatten
+          }.compact
+        else
+          @form.conditions.map { |condition|
+            condition.validation_errors.map do |error|
+              error_object(
+                error_name: error.name,
+                page: condition.check_page,
+                condition: condition,
+              )
+            end
+          }.flatten
+        end
       end
     end
   end

app/components/page_list_component/view.html.erb

@@ -33,11 +33,21 @@
           </div>
 
           <% if FeatureService.new(group: @form.group).enabled?(:multiple_branches) && page.routing_conditions.present? %>
-            <div class="govuk-summary-list__row">
+            <% error_message = PageListComponent::ErrorSummary::View.multiple_branch_error_message(page) %>
+            <div id="<%= PageListComponent::ErrorSummary::View.page_error_id(page) %>" class="govuk-summary-list__row app-page-list__row <%= class_names( "govuk-form-group--error": error_message.present?) %>">
               <dt class="govuk-summary-list__key app-page-list__key">
                 <%= t("page_conditions.condition_list_key", count: page.routing_conditions.count, question_number: page.position) %>
               </dt>
               <dd class="govuk-summary-list__value">
+
+              <% if error_message.present? %>
+                <ul class="govuk-list govuk-!-margin-0">
+                  <li class="app-page_list__route-text--error">
+                    <%= error_message %>
+                  </li>
+                </ul>
+              <% end %>
+
                 <% if page.routing_conditions.first.answer_value.present? %>
                   <% answer_value_groups(page).each do |_, group| %>
                     <p><%= condition_group_description(group) %></p>

app/controllers/routes_controller.rb

@@ -5,6 +5,7 @@ class RoutesController < FormsController
   def show
     authorize current_form, :can_view_form?
     @routes_input = Forms::RoutesInput.new(form: form_with_pages_and_conditions).assign_form_values
+    @routes_input.validate
   end
 
   def create

app/input_objects/forms/route_input.rb

@@ -9,7 +9,9 @@ class Forms::RouteInput < BaseInput
   attribute :goto
   attribute :answer_value
 
-  attr_accessor :page, :goto_options, :label
+  attr_accessor :page, :goto_page, :goto_options, :label
+
+  validate :route_is_not_backwards
 
   def goes_to_default_next_page?
     goto == DEFAULT_VALUE
@@ -28,4 +30,27 @@ def condition_attributes
       { goto_page_id: goto, skip_to_end: false, check_page_id: page.id }
     end
   end
+
+private
+
+  def route_is_not_backwards
+    return if skippable_for_backwards_validation?
+
+    return if goto_page.position > page.position
+
+    errors.add(:goto, error_message_for_backwards_route)
+  end
+
+  def skippable_for_backwards_validation?
+    goes_to_default_next_page? || goes_to_end_of_form? || goto_page.nil?
+  end
+
+  def error_message_for_backwards_route
+    if Forms::RoutesInput.route_with_selection_options?(page)
+      option_index = page.answer_settings.selection_options.find_index { |option| option["value"] == answer_value }
+      I18n.t("errors.routes.cannot_route_backwards_from_selection_page", question_number: page.position, option_number: option_index + 1)
+    else
+      I18n.t("errors.routes.cannot_route_backwards_from_generic_page", question_number: page.position)
+    end
+  end
 end

app/input_objects/forms/routes_input.rb

@@ -1,18 +1,24 @@
 class Forms::RoutesInput < BaseInput
+  include ActionView::Helpers::FormTagHelper
+
   attr_accessor :form, :routes
 
+  validate :routes_are_valid
+
   def self.too_many_selection_options?(page)
     page.answer_settings["selection_options"].length > 10
   end
 
+  def self.route_with_selection_options?(page)
+    page.answer_type == "selection" && page.answer_settings.only_one_option == "true" && !Forms::RoutesInput.too_many_selection_options?(page)
+  end
+
   def initialize(attributes = {})
     @form = attributes.delete(:form)
     super
   end
 
   def submit
-    # TODO: Add validations - this is here so we don't forget it but the model
-    # can't be invalid yet
     return false if invalid?
 
     Routes::SyncService.new(form:, routes:).sync_conditions_from_routes
@@ -36,12 +42,30 @@ def routes_attributes=(attributes)
       page = pages_by_id[route_attrs["page_id"].to_i]
       next unless page # Skip if page not found or doesn't belong to form
 
+      goto_page = pages_by_id[route_attrs["goto"].to_i]
+
       Forms::RouteInput.new(
         route_attrs.symbolize_keys.merge(
           page:,
+          goto_page:,
           goto_options: route_build_service.options_for_goto_page(page, route_attrs["goto"]),
         ),
       )
     }.compact
   end
+
+  def routes_are_valid
+    return if routes.nil?
+
+    routes.each.with_index do |route, index|
+      next if route.valid?
+
+      route.errors.each do |error|
+        attribute_key = "routes_attributes[#{index}][#{error.attribute}]"
+        error_field_id = field_id(:forms_routes_input_routes_attributes, error.attribute, index: index)
+
+        errors.add(attribute_key, error.message, url: "##{error_field_id}")
+      end
+    end
+  end
 end

app/services/routes/build_service.rb

@@ -17,9 +17,7 @@ def build_routes
     end
 
     form.pages.flat_map do |page|
-      if page.answer_type == "selection" &&
-          page.answer_settings.only_one_option == "true" &&
-          !Forms::RoutesInput.too_many_selection_options?(page)
+      if Forms::RoutesInput.route_with_selection_options?(page)
         build_routes_for_selection_page(page, conditions_by_key)
       else
         build_route_for_generic_page(page, conditions_by_key)
@@ -75,6 +73,7 @@ def build_routes_for_selection_page(page, conditions_by_key)
         page:,
         answer_value:,
         goto: goto_value_for(condition),
+        goto_page: condition&.goto_page,
         goto_options: options_for_goto_page(page, condition&.goto_page_id),
         label: { text: "If option #{index} (#{answer_value_label}), go to:" },
       )
@@ -91,6 +90,7 @@ def build_route_for_generic_page(page, conditions_by_key)
         page_id: page.id,
         page:,
         goto: goto_value_for(condition),
+        goto_page: condition&.goto_page,
         goto_options: options_for_goto_page(page, condition&.goto_page_id),
         label: { text: "After question #{page.position}, go to:" },
       ),

app/views/pages/index.html.erb

@@ -4,9 +4,7 @@
 
 <div class="govuk-grid-row">
   <div class="govuk-grid-column-two-thirds">
-    <% unless FeatureService.new(group: current_form.group).enabled?(:multiple_branches) %>
-      <%= render PageListComponent::ErrorSummary::View.new(current_form) %>
-    <% end %>
+    <%= render PageListComponent::ErrorSummary::View.new(current_form) %>
 
     <h1 class="govuk-heading-l govuk-!-margin-bottom-2">
       <span class="govuk-caption-l"><%= current_form.name %></span><span class="govuk-visually-hidden"> - </span>

app/views/routes/show.html.erb

@@ -33,7 +33,7 @@
                           route_form.hidden_field(:id) +
                             route_form.hidden_field(:page_id) +
                             route_form.hidden_field(:answer_value) +
-                            route_form.govuk_select(:goto, options_for_select(route_form.object.goto_options, route_form.object.goto), label: route_form.object.label)
+                            route_form.govuk_select(:goto, options_for_select(route_form.object.goto_options, route_form.object.goto), label: route_form.object.label, id: route_form.field_id(:goto))
                         end %>
                       </li>
                     <% end %>

config/locales/en.yml

@@ -365,6 +365,14 @@ en:
       goto_page_doesnt_exist: The question you’re taking the person to no longer exists - edit question %{question_number}’s route
       route_number_for_any_other_answer: for any other answer
     prefix: 'Error:'
+    routes:
+      cannot_route_backwards_from_generic_page: The route from question %{question_number} cannot go to a previous question - edit this route
+      cannot_route_backwards_from_selection_page: The route from question %{question_number}, option %{option_number} cannot go to a previous question - edit this route
+      page_list:
+        cannot_route_backwards_from_generic_page: The route from question %{question_number} cannot go to a previous question - edit this route
+        cannot_route_backwards_from_selection_page:
+          one: A route from question %{question_number} is going to a previous question - edit this route
+          other: Routes from question %{question_number} are going to a previous question - edit this question’s routes
   exit_page:
     no_content_added_html: "<p>No content added</p>"
   feedback_link:

spec/components/page_list_component/error_summary/error_summary_component_preview.rb

@@ -2,7 +2,7 @@ class PageListComponent::ErrorSummary::ErrorSummaryComponentPreview < ViewCompon
   include FactoryBot::Syntax::Methods
 
   def default
-    form = build(:form, id: 1, pages: [])
+    form = build(:form, :with_group, id: 1, pages: [])
 
     render(PageListComponent::ErrorSummary::View.new(form))
   end
@@ -13,7 +13,7 @@ def error_component_without_errors
     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:)
 
     # 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
@@ -35,7 +35,7 @@ def error_component_with_errors
     pages = [(build :page, id: 1, position: 1, question_text: "Enter your name", routing_conditions: routing_conditions_page_1),
              (build :page, id: 2, position: 2, question_text: "What is your pet's phone number?", routing_conditions: routing_conditions_page_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