Refactor routing (#98)

* Refactor routing

* Tenant code is now in About This Log

* Invert form definition routing syntax

* Make GDPR acceptance explicit dependency

* Add ReadMe description

* Add ADR for form routing logic

* Update JSON schema

* Add frontend gem repo to readme

* Rubocop

README.md

@@ -135,10 +135,7 @@ The JSON should follow the structure:
                   }
                 }
               },
-              "conditional_route_to": {
-                "[page_name_to_route_to]": {"question_name": "expected_answer"},
-                "[page_name_to_route_to]": {"question_name": "expected_answer"}
-              }
+              "depends_on": { "question_key": "answer_value_required_for_this_page_to_be_shown" }
             }
           }
         }
@@ -160,6 +157,19 @@ Assumptions made by the format:
   - Radio question answer option selected matches one of conditional e.g. ["answer-options-1-string", "answer-option-3-string"]
   - Numeric question value matches condition e.g. [">2"], ["<7"] or ["== 6"]
 
+  Page routing:
+
+  - Form navigation works by stepping sequentially through every page defined in the JSON form definition for the given subsection. For every page it checks if it has "depends_on" conditions. If it does, it evaluates them to determine whether that page should be show or not.
+
+  - In this way we can build up whole branches by having:
+
+  ```jsonc
+  "page_1": { "questions": { "question_1: "answer_options": ["A", "B"] } },
+  "page_2": { "questions": { "question_2: "answer_options": ["C", "D"] }, "depends_on": { "question_1": "A" } },
+  "page_3": { "questions": { "question_3: "answer_options": ["E", "F"] }, "depends_on": { "question_1": "A" } },
+  "page_4": { "questions": { "question_4: "answer_options": ["G", "H"] }, "depends_on": { "question_1": "B" } },
+  ```
+
 ## JSON Form Validation against Schema
 
 To validate the form JSON against the schema you can run:
@@ -182,6 +192,10 @@ This will validate all forms in directories = ["config/forms", "spec/fixtures/fo
 - [Technical docs](https://www.rubydoc.info/gems/govuk_design_system_formbuilder/)
 - [GitHub repository](https://github.com/DFE-Digital/govuk-formbuilder)
 
+### GOV.UK Frontend for Rails
+
+- [Github repository](https://github.com/DFE-Digital/govuk-components)
+
 ### GOV.UK Frontend
 
 - [GitHub repository](https://github.com/alphagov/govuk-frontend)

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]
@@ -141,17 +141,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 = {}
-    subsection_keys = form.pages_for_subsection(subsection).keys
-    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
+    questions = form.questions_for_subsection(subsection)
+    form.filter_conditional_questions(questions, case_log)
   end
 
   def get_next_page_name(form, page_name, case_log)
@@ -37,10 +26,10 @@ module CheckAnswersHelper
     form.next_page(page_name)
   end
 
-  def create_update_answer_link(case_log, question_title, page, form)
-    question = form.questions_for_page(page)[question_title]
-    link_name = if question["type"] == "checkbox"
-                  question["answer_options"].keys.any? { |key| case_log[key] == "Yes" } ? "Change" : "Answer"
+  def create_update_answer_link(question_title, question_info, case_log, form)
+    page = form.page_for_question(question_title)
+    link_name = if question_info["type"] == "checkbox"
+                  question_info["answer_options"].keys.any? { |key| case_log[key] == "Yes" } ? "Change" : "Answer"
                 else
                   case_log[question_title].blank? ? "Answer" : "Change"
                 end

app/models/form.rb

@@ -60,37 +60,30 @@ class Form
     }.first
   end
 
-  def next_page(previous_page)
-    if all_pages[previous_page].key?("default_next_page")
-      next_page = all_pages[previous_page]["default_next_page"]
-      return :check_answers if next_page == "check_answers"
+  def page_for_question(question)
+    all_pages.find { |_page_key, page_value| page_value["questions"].key?(question) }.first
+  end
 
-      return next_page
-    end
+  def next_page(page, case_log)
+    subsection = subsection_for_page(page)
+    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)
 
-    subsection = subsection_for_page(previous_page)
-    previous_page_idx = pages_for_subsection(subsection).keys.index(previous_page)
-    pages_for_subsection(subsection).keys[previous_page_idx + 1] || :check_answers
+    next_page(nxt_page, case_log)
   end
 
-  def next_page_redirect_path(previous_page)
-    next_page = next_page(previous_page)
-    if next_page == :check_answers
-      subsection = subsection_for_page(previous_page)
+  def next_page_redirect_path(page, case_log)
+    nxt_page = next_page(page, case_log)
+    if nxt_page == :check_answers
+      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,18 @@ class Form
     applicable_questions
   end
 
+  def page_routed_to?(page, case_log)
+    return true unless (conditions = page_dependencies(page))
+
+    conditions.all? do |question, value|
+      case_log[question].present? && case_log[question] == value
+    end
+  end
+
+  def page_dependencies(page)
+    all_pages[page]["depends_on"]
+  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, page, form)%>
+      <%= create_update_answer_link(question_title, question_info, @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| %>
-        <% page_info["questions"].each do |question_title, question_info| %>
-          <% 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 %>
+      <% total_questions(subsection, @case_log, form).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 } %>
       <% end %>
       <%= form_with model: @case_log, method: "get", builder: GOVUKDesignSystemFormBuilder::FormBuilder do |f| %>
         <%= f.govuk_submit "Save and continue"  %>

config/forms/2021_2022.json

@@ -23,20 +23,15 @@
                     "1": "No"
                   }
                 }
-              },
-              "conditional_route_to": {
-                "organisation_details": { "gdpr_acceptance": "Yes" }
-              },
-              "default_next_page": "gdpr_declined"
+              }
             },
             "gdpr_declined": {
               "header": "You cannot use this service",
               "hint_text": "",
               "description": "We cannot accept data about a tenant or buyer unless they’ve seen the DLUHC privacy notice.",
               "questions": {
-
               },
-              "default_next_page" : "check_answers"
+              "depends_on": { "gdpr_acceptance": "No" }
             },
             "organisation_details": {
               "header": "About this log",
@@ -62,7 +57,8 @@
                     "1": "B"
                   }
                 }
-              }
+              },
+              "depends_on": { "gdpr_acceptance": "Yes" }
             },
             "sale_or_letting": {
               "header": "About this log",
@@ -79,9 +75,7 @@
                   }
                 }
               },
-              "conditional_route_to": {
-                "sale_completion_date": { "sale_or_letting": "Sale" }
-              }
+              "depends_on": { "gdpr_acceptance": "Yes" }
             },
             "tenant_same_property_renewal": {
               "header": "About this log",
@@ -97,7 +91,8 @@
                     "1": "No"
                   }
                 }
-              }
+              },
+              "depends_on": { "gdpr_acceptance": "Yes", "sale_or_letting": "Letting" }
             },
             "startdate": {
               "header": "About this log",
@@ -109,7 +104,8 @@
                   "hint_text": "For example, 27 3 2007",
                   "type": "date"
                 }
-              }
+              },
+              "depends_on": { "gdpr_acceptance": "Yes", "sale_or_letting": "Letting" }
             },
             "about_this_letting": {
               "header": "Tell us about this letting",
@@ -147,7 +143,8 @@
                     "1": "General Needs"
                   }
                 }
-              }
+              },
+              "depends_on": { "gdpr_acceptance": "Yes", "sale_or_letting": "Letting" }
             },
             "tenant_code": {
               "header": "",
@@ -160,7 +157,7 @@
                   "type": "text"
                 }
               },
-              "default_next_page": "check_answers"
+              "depends_on": { "gdpr_acceptance": "Yes", "sale_or_letting": "Letting" }
             },
             "sale_completion_date": {
               "header": "Sale Completion Date",
@@ -172,7 +169,8 @@
                   "hint_text": "For example, 27 3 2007",
                   "type": "date"
                 }
-              }
+              },
+              "depends_on": { "gdpr_acceptance": "Yes", "sale_or_letting": "Sale" }
             },
             "purchaser_code": {
               "header": "About this log",
@@ -185,7 +183,7 @@
                   "type": "text"
                 }
               },
-              "default_next_page": "check_answers"
+              "depends_on": { "gdpr_acceptance": "Yes", "sale_or_letting": "Sale" }
             }
           }
         }
@@ -197,18 +195,6 @@
         "household_characteristics": {
           "label": "Household characteristics",
           "pages": {
-            "tenant_code": {
-              "header": "",
-              "description": "",
-              "questions": {
-                "tenant_code": {
-                  "check_answer_label": "Tenant code",
-                  "header": "What is the tenant code?",
-                  "hint_text": "",
-                  "type": "text"
-                }
-              }
-            },
             "person_1_age": {
               "header": "",
               "description": "",

config/forms/schema/generic.json

@@ -44,7 +44,7 @@
                     "pages": {
                       "type": "object",
                       "patternProperties": {
-                        "^(?!(conditional_route_to))[a-z_]+$": {
+                        "^(?!(depends_on))[a-z_]+$": {
                           "description": "Page Name",
                           "type": "object",
                           "required": ["header", "questions"],
@@ -105,7 +105,7 @@
                             }
                           },
                           "additionalProperties": {
-                            "conditional_route_to": {
+                            "depends_on": {
                               "description": "",
                               "type": "object"
                             }

docs/adr/adr-009-form-routing-logic.md

@@ -0,0 +1,12 @@
+### ADR - 009: Form Routing Logic
+
+There are 2 ways you can think about form (page) routing logic:
+
+1. Based on the answer you give to a page you are navigated to some point in the form, i.e. a "Jump to"
+2. Each question is considered sequentially and independently and we evaluate whether it should be shown or not
+
+Our Form Definition DSL takes the second approach. This has a couple of advantages:
+
+- It makes the check answers pattern easier to code as you can ask each page directly: "Have the conditions for you to be shown been met?", with approach 1, you would effectively have to traverse the full route branch to see if a particular page was shown for each page/question which adds complexity.
+
+- It makes it easier to look at the JSON and see at a glance what conditions will show or hide a page, which is closer to how the business logic is discussed and is easier to reason about.

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

@@ -479,27 +479,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
 

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",
@@ -76,11 +74,7 @@
                     "1": "Letting"
                   }
                 }
-              },
-              "conditional_route_to": {
-                "tenant_same_property_renewal": { "sale_or_letting": "Letting" }
-              },
-              "default_next_page" : "check_answers"
+              }
             },
             "tenant_same_property_renewal": {
               "header": "About this log",
@@ -96,7 +90,8 @@
                     "1": "No"
                   }
                 }
-              }
+              },
+              "depends_on": { "sale_or_letting": "Letting" }
             },
             "tenancy_start_date": {
               "header": "About this log",
@@ -108,7 +103,8 @@
                   "hint_text": "For example, 27 3 2007",
                   "type": "date"
                 }
-              }
+              },
+              "depends_on": { "sale_or_letting": "Letting" }
             },
             "letting_type": {
               "header": "About this log",
@@ -146,7 +142,8 @@
                     "1": "General Needs"
                   }
                 }
-              }
+              },
+              "depends_on": { "sale_or_letting": "Letting" }
             },
             "sale_completion_date": {
               "header": "About this log",
@@ -158,7 +155,8 @@
                   "hint_text": "For example, 27 3 2007",
                   "type": "date"
                 }
-              }
+              },
+              "depends_on": { "sale_or_letting": "Sale" }
             },
             "purchaser_code": {
               "header": "About this log",
@@ -170,7 +168,8 @@
                   "hint_text": "",
                   "type": "text"
                 }
-              }
+              },
+              "depends_on": { "sale_or_letting": "Sale" }
             }
           }
         }

spec/fixtures/forms/test_form.json

@@ -244,13 +244,7 @@
                       "1": "No"
                     }
                   }
-                },
-                "conditional_route_to": {
-                  "rent": { "preg_occ": "Yes", "sex1": "Female" },
-                  "conditional_question_yes_page": { "preg_occ": "Yes" },
-                  "conditional_question_no_page": { "preg_occ": "No" }
-                },
-                "default_next_page": "check_answers"
+                }
               },
               "conditional_question_yes_page": {
                 "questions": {
@@ -264,7 +258,7 @@
                     }
                   }
                 },
-                "default_next_page": "check_answers"
+                "depends_on": { "preg_occ": "Yes" }
               },
               "conditional_question_no_page": {
                 "questions": {
@@ -278,7 +272,7 @@
                     }
                   }
                 },
-                "default_next_page": "conditional_question_no_second_page"
+                "depends_on": { "preg_occ": "No" }
               },
               "conditional_question_no_second_page": {
                 "questions": {
@@ -291,7 +285,8 @@
                       "1": "No"
                     }
                   }
-                }
+                },
+                "depends_on": { "preg_occ": "No", "sex1": "Male" }
               }
             }
           }

spec/fixtures/forms/test_validator.json

@@ -22,9 +22,8 @@
                   "type": "text"
                 }
               },
-              "conditional_route_to": {"test": "Yes"}
+              "depends_on": {"test": "Yes"}
             },
-            "conditional_route_to": {"test": "Yes"},
             "person_1_age": {
               "header": "",
               "description": "",

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