Refactor routing

app/controllers/case_logs_controller.rb

@@ -59,7 +59,7 @@ class CaseLogsController < ApplicationController
     @case_log.page = params[:case_log][:page]
     responses_for_page = responses_for_page(@case_log.page)
     if @case_log.update(responses_for_page) && @case_log.has_no_unresolved_soft_errors?
-      redirect_path = get_next_page_path(form, @case_log.page, @case_log)
+      redirect_path = form.next_page_redirect_path(@case_log.page, @case_log)
       redirect_to(send(redirect_path, @case_log))
     else
       page_info = form.all_pages[@case_log.page]
@@ -137,17 +137,4 @@ private
 
     params.require(:case_log).permit(CaseLog.editable_fields)
   end
-
-  def get_next_page_path(form, page, case_log = {})
-    content = form.all_pages[page]
-
-    if content.key?("conditional_route_to")
-      content["conditional_route_to"].each do |route, conditions|
-        if conditions.keys.all? { |x| case_log[x].present? } && conditions.all? { |k, v| v.include?(case_log[k]) }
-          return "case_log_#{route}_path"
-        end
-      end
-    end
-    form.next_page_redirect_path(page)
-  end
 end

app/helpers/check_answers_helper.rb

@@ -10,19 +10,8 @@ module CheckAnswersHelper
   end
 
   def total_questions(subsection, case_log, form)
-    total_questions = {}
+    questions = form.questions_for_subsection(subsection)
-    subsection_keys = form.pages_for_subsection(subsection).keys
+    form.filter_conditional_questions(questions, case_log)
-    page_name = subsection_keys.first
-
-    while page_name.to_s != "check_answers" && subsection_keys.include?(page_name)
-      questions = form.questions_for_page(page_name)
-      applicable_questions = form.filter_conditional_questions(questions, case_log)
-      total_questions = total_questions.merge(applicable_questions)
-
-      page_name = get_next_page_name(form, page_name, case_log)
-    end
-
-    total_questions
   end
 
   def get_next_page_name(form, page_name, case_log)
@@ -37,9 +26,10 @@ module CheckAnswersHelper
     form.next_page(page_name)
   end
 
-  def create_update_answer_link(case_log_answer, case_log_id, page)
+  def create_update_answer_link(question_title, case_log, form)
-    link_name = case_log_answer.blank? ? "Answer" : "Change"
+    page = form.page_for_question(question_title)
-    link_to(link_name, "/case_logs/#{case_log_id}/#{page}", class: "govuk-link").html_safe
+    link_name = case_log[question_title].blank? ? "Answer" : "Change"
+    link_to(link_name, "/case_logs/#{case_log.id}/#{page}", class: "govuk-link").html_safe
   end
 
   def create_next_missing_question_link(case_log_id, subsection, case_log, form)

app/models/form.rb

@@ -60,37 +60,30 @@ class Form
     }.first
   end
 
-  def next_page(previous_page)
+  def page_for_question(question)
-    if all_pages[previous_page].key?("default_next_page")
+    all_pages.find { |_page_key, page_value| page_value["questions"].key?(question) }.first
-      next_page = all_pages[previous_page]["default_next_page"]
-      return :check_answers if next_page == "check_answers"
-
-      return next_page
   end
 
-    subsection = subsection_for_page(previous_page)
+  def next_page(page, case_log)
-    previous_page_idx = pages_for_subsection(subsection).keys.index(previous_page)
+    subsection = subsection_for_page(page)
-    pages_for_subsection(subsection).keys[previous_page_idx + 1] || :check_answers
+    page_idx = pages_for_subsection(subsection).keys.index(page)
+    nxt_page = pages_for_subsection(subsection).keys[page_idx + 1]
+    return :check_answers if nxt_page.nil?
+    return nxt_page if page_routed_to?(nxt_page, case_log)
+
+    next_page(nxt_page, case_log)
   end
 
-  def next_page_redirect_path(previous_page)
+  def next_page_redirect_path(page, case_log)
-    next_page = next_page(previous_page)
+    nxt_page = next_page(page, case_log)
-    if next_page == :check_answers
+    if nxt_page == :check_answers
-      subsection = subsection_for_page(previous_page)
+      subsection = subsection_for_page(page)
       "case_log_#{subsection}_check_answers_path"
     else
-      "case_log_#{next_page}_path"
+      "case_log_#{nxt_page}_path"
     end
   end
 
-  def previous_page(current_page)
-    subsection = subsection_for_page(current_page)
-    current_page_idx = pages_for_subsection(subsection).keys.index(current_page)
-    return unless current_page_idx.positive?
-
-    pages_for_subsection(subsection).keys[current_page_idx - 1]
-  end
-
   def all_questions
     @all_questions ||= all_pages.map { |_page_key, page_value|
       page_value["questions"]
@@ -101,6 +94,10 @@ class Form
     applicable_questions = questions
 
     questions.each do |k, question|
+      unless page_routed_to?(page_for_question(k), case_log)
+        applicable_questions = applicable_questions.reject { |z| z == k }
+      end
+
       question.fetch("conditional_for", []).each do |conditional_question_key, condition|
         if condition_not_met(case_log, k, question, condition)
           applicable_questions = applicable_questions.reject { |z| z == conditional_question_key }
@@ -110,6 +107,25 @@ class Form
     applicable_questions
   end
 
+  def page_routed_to?(page, case_log)
+    return true unless pages_with_routing_conditions.include?(page)
+
+    applicable_conditions = all_routing_conditions.flat_map { |conditions|
+      conditions.select { |k, _condition| k == page }
+    }.flat_map(&:values)
+    applicable_conditions.all? do |condition|
+      condition.all? { |k, v| case_log[k].present? && case_log[k] == v }
+    end
+  end
+
+  def pages_with_routing_conditions
+    @pages_with_routing_conditions ||= all_routing_conditions.flat_map(&:keys)
+  end
+
+  def all_routing_conditions
+    @all_routing_conditions ||= all_pages.map { |_page_key, page| page["conditional_route_to"] }.compact
+  end
+
   def condition_not_met(case_log, question_key, question, condition)
     case question["type"]
     when "numeric"

app/views/form/_check_answers_table.html.erb

@@ -7,7 +7,7 @@
       <%= form.get_answer_label(@case_log, question_title) %>
     </dd>
     <dd class="govuk-summary-list__actions">
-      <%= create_update_answer_link(@case_log[question_title], @case_log.id, page)%>
+      <%= create_update_answer_link(question_title, @case_log, form) %>
     </dd>
   </div>
 </dl>

app/views/form/check_answers.html.erb

@@ -3,12 +3,8 @@
     <div class="govuk-grid-column-three-quarters-from-desktop">
       <h1 class="govuk-heading-l">Check the answers you gave for <%= subsection.humanize(capitalize: false) %></h1>
       <%= display_answered_questions_summary(subsection, @case_log, form) %>
-      <% form.pages_for_subsection(subsection).each do |page, page_info| %>
+      <% total_questions(subsection, @case_log, form).each do |question_title, question_info| %>
-        <% page_info["questions"].each do |question_title, question_info| %>
+        <%= render partial: 'form/check_answers_table', locals: { question_title: question_title, question_info: question_info, case_log: @case_log, form: form } %>
-          <% if total_questions(subsection, @case_log, form).include?(question_title) %>
-            <%= render partial: 'form/check_answers_table', locals: { question_title: question_title, question_info: question_info, case_log: @case_log, page: page, form: form } %>
-          <%end %>
-        <%end %>
       <% end %>
       <%= form_with model: @case_log, method: "get", builder: GOVUKDesignSystemFormBuilder::FormBuilder do |f| %>
         <%= f.govuk_submit "Save and continue"  %>

config/forms/2021_2022.json

@@ -26,8 +26,7 @@
               },
               "conditional_route_to": {
                 "organisation_details": { "gdpr_acceptance": "Yes" }
-              },
+              }
-              "default_next_page": "gdpr_declined"
             },
             "gdpr_declined": {
               "header": "You cannot use this service",
@@ -35,8 +34,7 @@
               "description": "We cannot accept data about a tenant or buyer unless they’ve seen the DLUHC privacy notice.",
               "questions": {
 
-              },
+              }
-              "default_next_page" : "check_answers"
             },
             "organisation_details": {
               "header": "About this log",
@@ -159,8 +157,7 @@
                   "hint_text": "",
                   "type": "text"
                 }
-              },
+              }
-              "default_next_page": "check_answers"
             },
             "sale_completion_date": {
               "header": "Sale Completion Date",
@@ -184,8 +181,7 @@
                   "hint_text": "",
                   "type": "text"
                 }
-              },
+              }
-              "default_next_page": "check_answers"
             }
           }
         }

spec/controllers/case_logs_controller_spec.rb

@@ -200,27 +200,4 @@ RSpec.describe CaseLogsController, type: :controller do
       end
     end
   end
-
-  describe "get_next_page_path" do
-    let(:previous_page) { "net_income" }
-    let(:last_previous_page) { "housing_benefit" }
-    let(:previous_conditional_page) { "conditional_question" }
-    let(:form_handler) { FormHandler.instance }
-    let(:form) { form_handler.get_form("test_form") }
-    let(:case_log_controller) { CaseLogsController.new }
-
-    it "returns a correct page path if there is no conditional routing" do
-      expect(case_log_controller.send(:get_next_page_path, form, previous_page)).to eq("case_log_net_income_uc_proportion_path")
-    end
-
-    it "returns a check answers page if previous page is the last page" do
-      expect(case_log_controller.send(:get_next_page_path, form, last_previous_page)).to eq("case_log_income_and_benefits_check_answers_path")
-    end
-
-    it "returns a correct page path if there is conditional routing" do
-      responses_for_page = {}
-      responses_for_page["preg_occ"] = "No"
-      expect(case_log_controller.send(:get_next_page_path, form, previous_conditional_page, responses_for_page)).to eq("case_log_conditional_question_no_page_path")
-    end
-  end
 end

spec/features/case_log_spec.rb

@@ -466,27 +466,17 @@ RSpec.describe "Test Features" do
       expect(page).to have_current_path("/case_logs/#{id}/conditional_question_no_page")
     end
 
-    it "can route based on page inclusion rules" do
-      visit("/case_logs/#{id}/conditional_question_yes_page")
-      choose("case-log-cbl-yes-field", allow_label_click: true)
-      click_button("Save and continue")
-      expect(page).to have_current_path("/case_logs/#{id}/conditional_question/check_answers")
-    end
-
-    it "can route to the default next page" do
-      visit("/case_logs/#{id}/conditional_question")
-      click_button("Save and continue")
-      expect(page).to have_current_path("/case_logs/#{id}/conditional_question/check_answers")
-    end
-
     it "can route based on multiple conditions" do
       visit("/case_logs/#{id}/person_1_gender")
       choose("case-log-sex1-female-field", allow_label_click: true)
       click_button("Save and continue")
+      expect(page).to have_current_path("/case_logs/#{id}/household_number_of_other_members")
       visit("/case_logs/#{id}/conditional_question")
-      choose("case-log-preg-occ-yes-field", allow_label_click: true)
+      choose("case-log-preg-occ-no-field", allow_label_click: true)
       click_button("Save and continue")
-      expect(page).to have_current_path("/case_logs/#{id}/rent")
+      expect(page).to have_current_path("/case_logs/#{id}/conditional_question_no_page")
+      click_button("Save and continue")
+      expect(page).to have_current_path("/case_logs/#{id}/conditional_question/check_answers")
     end
   end
 end

spec/fixtures/forms/test_aboutthislog.json

@@ -24,8 +24,7 @@
               },
               "conditional_route_to": {
                 "organisation_details": { "gdpr_acceptance": "Yes" }
-              },
+              }
-              "default_next_page": "gdpr_declined"
             },
             "gdpr_declined": {
               "header": "You cannot use this service",
@@ -33,8 +32,7 @@
               "description": "We cannot accept data about a tenant or buyer unless they’ve seen the DLUHC privacy notice.",
               "questions": {
 
-              },
+              }
-              "default_next_page" : "check_answers"
             },
             "organisation_details": {
               "header": "About this log",
@@ -79,8 +77,7 @@
               },
               "conditional_route_to": {
                 "tenant_same_property_renewal": { "sale_or_letting": "Letting" }
-              },
+              }
-              "default_next_page" : "check_answers"
             },
             "tenant_same_property_renewal": {
               "header": "About this log",

spec/fixtures/forms/test_form.json

@@ -246,11 +246,10 @@
                   }
                 },
                 "conditional_route_to": {
-                  "rent": { "preg_occ": "Yes", "sex1": "Female" },
                   "conditional_question_yes_page": { "preg_occ": "Yes" },
-                  "conditional_question_no_page": { "preg_occ": "No" }
+                  "conditional_question_no_page": { "preg_occ": "No" },
-                },
+                  "conditional_question_no_second_page": { "preg_occ": "No", "sex1": "Male" }
-                "default_next_page": "check_answers"
+                }
               },
               "conditional_question_yes_page": {
                 "questions": {
@@ -263,8 +262,7 @@
                       "1": "No"
                     }
                   }
-                },
+                }
-                "default_next_page": "check_answers"
               },
               "conditional_question_no_page": {
                 "questions": {
@@ -277,8 +275,7 @@
                       "1": "No"
                     }
                   }
-                },
+                }
-                "default_next_page": "conditional_question_no_second_page"
               },
               "conditional_question_no_second_page": {
                 "questions": {

spec/helpers/check_answers_helper_spec.rb

@@ -119,6 +119,7 @@ RSpec.describe CheckAnswersHelper do
 
       it "counts correct questions when the conditional question is answered" do
         case_log["preg_occ"] = "No"
+        case_log["sex1"] = "Male"
         expect(total_number_of_questions(conditional_routing_subsection, case_log, form)).to eq(3)
       end
     end
@@ -126,7 +127,6 @@ RSpec.describe CheckAnswersHelper do
     context "total questions" do
       it "returns total questions" do
         result = total_questions(subsection, case_log, form)
-        expect(result.class).to eq(Hash)
         expected_keys = %w[earnings incfreq benefits hb]
         expect(result.keys).to eq(expected_keys)
       end
@@ -157,14 +157,14 @@ RSpec.describe CheckAnswersHelper do
           case_log["preg_occ"] = "Yes"
           case_log["sex1"] = "Female"
           result = total_questions(conditional_routing_subsection, case_log, form)
-          expected_keys = %w[preg_occ]
+          expected_keys = %w[preg_occ cbl]
           expect(result.keys).to match_array(expected_keys)
         end
 
         it "it includes conditional pages and questions that were displayed" do
           case_log["preg_occ"] = "No"
           result = total_questions(conditional_routing_subsection, case_log, form)
-          expected_keys = %w[preg_occ conditional_question_no_question conditional_question_no_second_question]
+          expected_keys = %w[preg_occ conditional_question_no_question]
           expect(result.keys).to match_array(expected_keys)
         end
       end

spec/models/form_spec.rb

@@ -3,11 +3,12 @@ require "rails_helper"
 RSpec.describe Form, type: :model do
   form_handler = FormHandler.instance
   let(:form) { form_handler.get_form("test_form") }
+  let(:case_log) { FactoryBot.build(:case_log, :in_progress) }
 
   describe ".next_page" do
     let(:previous_page) { "person_1_age" }
     it "returns the next page given the previous" do
-      expect(form.next_page(previous_page)).to eq("person_1_gender")
+      expect(form.next_page(previous_page, case_log)).to eq("person_1_gender")
     end
   end
 
@@ -18,22 +19,6 @@ RSpec.describe Form, type: :model do
     end
   end
 
-  describe ".previous_page" do
-    context "given a page in the middle of a subsection" do
-      let(:current_page) { "person_1_age" }
-      it "returns the previous page given the current" do
-        expect(form.previous_page(current_page)).to eq("tenant_code")
-      end
-    end
-
-    context "given the first page in a subsection" do
-      let(:current_page) { "tenant_code" }
-      it "returns empty string" do
-        expect(form.previous_page(current_page)).to be_nil
-      end
-    end
-  end
-
   describe ".questions_for_subsection" do
     let(:subsection) { "income_and_benefits" }
     it "returns all questions for subsection" do
@@ -42,4 +27,23 @@ RSpec.describe Form, type: :model do
       expect(result.keys).to eq(%w[earnings incfreq benefits hb])
     end
   end
+
+  describe "next_page_redirect_path" do
+    let(:previous_page) { "net_income" }
+    let(:last_previous_page) { "housing_benefit" }
+    let(:previous_conditional_page) { "conditional_question" }
+
+    it "returns a correct page path if there is no conditional routing" do
+      expect(form.next_page_redirect_path(previous_page, case_log)).to eq("case_log_net_income_uc_proportion_path")
+    end
+
+    it "returns a check answers page if previous page is the last page" do
+      expect(form.next_page_redirect_path(last_previous_page, case_log)).to eq("case_log_income_and_benefits_check_answers_path")
+    end
+
+    it "returns a correct page path if there is conditional routing" do
+      case_log["preg_occ"] = "No"
+      expect(form.next_page_redirect_path(previous_conditional_page, case_log)).to eq("case_log_conditional_question_no_page_path")
+    end
+  end
 end