Merge pull request #2842 from govuk-forms/fix-none-of-the-above-route-backwards

Fix none of the above route backwards

Author: thomasiles

app/input_objects/forms/route_input.rb

@@ -9,7 +9,7 @@ class Forms::RouteInput < BaseInput
   attribute :goto
   attribute :answer_value
 
-  attr_accessor :page, :goto_page, :goto_options, :label
+  attr_accessor :page, :goto_page, :goto_options
 
   validate :route_is_not_backwards
 
@@ -31,14 +31,23 @@ def condition_attributes
     end
   end
 
+  def label
+    if Forms::RoutesInput.route_with_selection_options?(page)
+      answer_value_label = answer_value == Condition::NONE_OF_THE_ABOVE ? I18n.t("page_conditions.none_of_the_above") : answer_value
+      return { text: "If option #{option_index} (#{answer_value_label}), go to:" }
+    end
+
+    { text: "After question #{page.position}, go to:" }
+  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)
+    errors.add(:goto, :backwards_route, message: error_message_for_backwards_route)
   end
 
   def skippable_for_backwards_validation?
@@ -47,10 +56,17 @@ def skippable_for_backwards_validation?
 
   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)
+      I18n.t("errors.routes.cannot_route_backwards_from_selection_page", question_number: page.position, option_number: option_index)
     else
       I18n.t("errors.routes.cannot_route_backwards_from_generic_page", question_number: page.position)
     end
   end
+
+  def option_index
+    if answer_value == Condition::NONE_OF_THE_ABOVE
+      page.answer_settings.selection_options.length + 1
+    else
+      page.answer_settings.selection_options.find_index { |option| option["value"] == answer_value } + 1
+    end
+  end
 end

app/services/routes/build_service.rb

@@ -57,13 +57,12 @@ def options_for_goto_page(page, selected = nil)
 private
 
   def build_routes_for_selection_page(page, conditions_by_key)
-    options = page.answer_settings&.selection_options || []
+    options = page.answer_settings&.selection_options&.dup || []
 
     options << NONE_OF_THE_ABOVE_OPTION if page.is_optional
 
-    options.map.with_index(1) do |option, index|
+    options.map do |option|
       answer_value = option["value"]
-      answer_value_label = answer_value == NONE_OF_THE_ABOVE_OPTION[:value] ? I18n.t("page_conditions.none_of_the_above") : option["name"]
       key = [page.id, answer_value]
       condition = conditions_by_key[key]
 
@@ -75,7 +74,6 @@ def build_routes_for_selection_page(page, conditions_by_key)
         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:" },
       )
     end
   end
@@ -92,7 +90,6 @@ def build_route_for_generic_page(page, conditions_by_key)
         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:" },
       ),
     ]
   end

spec/input_objects/forms/route_input_spec.rb

@@ -12,7 +12,7 @@
   let(:attributes) do
     {
       id: 1,
-      page_id: 2,
+      page_id: page.id,
       goto: goto_page.id,
       answer_value: "Yes",
       page: page,
@@ -27,16 +27,11 @@
   describe "attributes" do
     it "can be initialized with a hash of attributes" do
       expect(route_input.id).to eq(1)
-      expect(route_input.page_id).to eq(2)
+      expect(route_input.page_id).to eq(page.id)
       expect(route_input.goto).to eq(goto_page.id)
       expect(route_input.answer_value).to eq("Yes")
       expect(route_input.page).to eq(page)
     end
-
-    it "allows access to other accessors like label" do
-      route_input.label = "Go to next page"
-      expect(route_input.label).to eq("Go to next page")
-    end
   end
 
   describe "#goes_to_default_next_page?" do
@@ -104,6 +99,43 @@
     end
   end
 
+  describe "#label" do
+    context "when the route is to the next page" do
+      it "returns the correct label" do
+        expect(route_input.label).to eq({ text: "After question 1, go to:" })
+      end
+    end
+
+    context "when the route is for a generic page" do
+      let(:middle_page) { build_stubbed(:page, position: 2) }
+      let(:goto_page) { build_stubbed(:page, position: 3) }
+
+      it "sets the label correctly for a generic page" do
+        expect(route_input.label).to eq({ text: "After question 1, go to:" })
+      end
+    end
+
+    context "when the route is for a selection page" do
+      let(:page) { build_stubbed(:page, :with_selection_settings, position: 1) }
+      let(:goto_page) { build_stubbed(:page, position: 2) }
+      let(:attributes) { super().merge(answer_value: "Option 1") }
+
+      it "sets the label correctly for a selection page" do
+        expect(route_input.label).to eq({ text: "If option 1 (Option 1), go to:" })
+      end
+    end
+
+    context "when the route is for a selection page with a none of the above option" do
+      let(:page) { build_stubbed(:page, :with_selection_settings, position: 1) }
+      let(:goto_page) { build_stubbed(:page, position: 2) }
+      let(:attributes) { super().merge(answer_value: Condition::NONE_OF_THE_ABOVE) }
+
+      it "sets the label correctly for a selection page with a none of the above option" do
+        expect(route_input.label).to eq({ text: "If option 3 (None of the above), go to:" })
+      end
+    end
+  end
+
   describe "#route_is_not_backwards" do
     context "when the route is not backwards" do
       let(:page) { build_stubbed(:page, position: 1) }

spec/services/routes/build_service_spec.rb

@@ -36,11 +36,6 @@
           expect(route_for_page1.goto).to eq(Forms::RouteInput::DEFAULT_VALUE)
         end
 
-        it "sets the label correctly for a generic page" do
-          route_for_page1 = service.build_routes.first
-          expect(route_for_page1.label).to eq({ text: "After question #{pages.first.position}, go to:" })
-        end
-
         context "when a condition exists for the generic page" do
           let!(:conditions) do
             [
@@ -111,18 +106,6 @@
           expect(route_for_no).not_to be_nil
         end
 
-        it "sets the correct goto value and label for each option route" do
-          routes_for_page1 = service.build_routes.select { |r| r.page_id == pages.first.id }
-          route_for_yes = routes_for_page1.find { |r| r.answer_value == "Yes" }
-          route_for_no = routes_for_page1.find { |r| r.answer_value == "No" }
-
-          expect(route_for_yes.goto).to eq(Forms::RouteInput::DEFAULT_VALUE)
-          expect(route_for_yes.label).to eq({ text: "If option 1 (Yes), go to:" })
-
-          expect(route_for_no.goto).to eq(Forms::RouteInput::DEFAULT_VALUE)
-          expect(route_for_no.label).to eq({ text: "If option 2 (No), go to:" })
-        end
-
         context "when the selection page has more than 10 options" do
           let(:selection_options) do
             (1..11).map do |i|