From b764b42a2eba0bbf99679a63ae036b0591ce3878 Mon Sep 17 00:00:00 2001 From: Daniel Baark <5101747+baarkerlounger@users.noreply.github.com> Date: Tue, 16 Nov 2021 12:00:31 +0000 Subject: [PATCH 01/12] Bugfixes (#93) * Update footer email * Page validations need to match form page names * Update specs --- app/validations/household_validations.rb | 2 +- app/views/layouts/_footer.html.erb | 2 +- config/forms/2021_2022.json | 4 ++-- spec/controllers/case_logs_controller_spec.rb | 2 +- spec/features/case_log_spec.rb | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/validations/household_validations.rb b/app/validations/household_validations.rb index d3536ae8a..bb04599c7 100644 --- a/app/validations/household_validations.rb +++ b/app/validations/household_validations.rb @@ -45,7 +45,7 @@ module HouseholdValidations end end - def validate_household_pregnancy(record) + def validate_pregnancy(record) if (record.preg_occ == "Yes" || record.preg_occ == "Prefer not to say") && !women_of_child_bearing_age_in_household(record) record.errors.add :preg_occ, "You must answer no as there are no female tenants aged 16-50 in the property" end diff --git a/app/views/layouts/_footer.html.erb b/app/views/layouts/_footer.html.erb index ffba80da2..5e0da8f33 100644 --- a/app/views/layouts/_footer.html.erb +++ b/app/views/layouts/_footer.html.erb @@ -22,7 +22,7 @@

Email

diff --git a/config/forms/2021_2022.json b/config/forms/2021_2022.json index f7b8e8763..7ae709b1c 100644 --- a/config/forms/2021_2022.json +++ b/config/forms/2021_2022.json @@ -209,7 +209,7 @@ } } }, - "age1": { + "person_1_age": { "header": "", "description": "", "questions": { @@ -307,7 +307,7 @@ } } }, - "tenant_economic_status": { + "person_1_economic": { "header": "", "description": "", "questions": { diff --git a/spec/controllers/case_logs_controller_spec.rb b/spec/controllers/case_logs_controller_spec.rb index 8192173fc..1b4c1fcf6 100644 --- a/spec/controllers/case_logs_controller_spec.rb +++ b/spec/controllers/case_logs_controller_spec.rb @@ -125,7 +125,7 @@ RSpec.describe CaseLogsController, type: :controller do context "conditional routing" do before do - allow_any_instance_of(CaseLogValidator).to receive(:validate_household_pregnancy).and_return(true) + allow_any_instance_of(CaseLogValidator).to receive(:validate_pregnancy).and_return(true) end let(:case_log_form_conditional_question_yes_params) do diff --git a/spec/features/case_log_spec.rb b/spec/features/case_log_spec.rb index a616b7baf..6db2cbe6a 100644 --- a/spec/features/case_log_spec.rb +++ b/spec/features/case_log_spec.rb @@ -449,7 +449,7 @@ RSpec.describe "Test Features" do describe "conditional page routing", js: true do before do - allow_any_instance_of(CaseLogValidator).to receive(:validate_household_pregnancy).and_return(true) + allow_any_instance_of(CaseLogValidator).to receive(:validate_pregnancy).and_return(true) end it "can route the user to a different page based on their answer on the current page" do From f8f2e464f91238137137fa0627f535416a623d0d Mon Sep 17 00:00:00 2001 From: Daniel Baark <5101747+baarkerlounger@users.noreply.github.com> Date: Tue, 16 Nov 2021 17:19:40 +0000 Subject: [PATCH 02/12] Width layout class (#94) --- app/views/case_logs/edit.html.erb | 2 +- app/views/case_logs/index.html.erb | 2 +- app/views/form/check_answers.html.erb | 2 +- app/views/form/page.html.erb | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/views/case_logs/edit.html.erb b/app/views/case_logs/edit.html.erb index 25668b2ac..94fabb9a9 100644 --- a/app/views/case_logs/edit.html.erb +++ b/app/views/case_logs/edit.html.erb @@ -1,6 +1,6 @@ <%= turbo_frame_tag "case_log_form", target: "_top" do %>
-
+

Tasklist for log <%= @case_log.id %>

diff --git a/app/views/case_logs/index.html.erb b/app/views/case_logs/index.html.erb index 7352c304c..ea3d45436 100644 --- a/app/views/case_logs/index.html.erb +++ b/app/views/case_logs/index.html.erb @@ -2,7 +2,7 @@

Your logs

-
+
<%= link_to "Create new log", case_logs_path, method: :post, class: "govuk-button" %> diff --git a/app/views/form/check_answers.html.erb b/app/views/form/check_answers.html.erb index 26b153f8f..46cb5a4f9 100644 --- a/app/views/form/check_answers.html.erb +++ b/app/views/form/check_answers.html.erb @@ -1,6 +1,6 @@ <%= turbo_frame_tag "case_log_form", target: "_top" do %>
-
+

Check the answers you gave for <%= subsection.humanize(capitalize: false) %>

<%= display_answered_questions_summary(subsection, @case_log, form) %> <% form.pages_for_subsection(subsection).each do |page, page_info| %> diff --git a/app/views/form/page.html.erb b/app/views/form/page.html.erb index 238f11439..f29011b99 100644 --- a/app/views/form/page.html.erb +++ b/app/views/form/page.html.erb @@ -4,7 +4,7 @@ <%= turbo_frame_tag "case_log_form", target: "_top" do %>
-
+
<% if page_info["header"].present? %>

<%= page_info["header"] %> From 1f4775182a3ab1ebd8de9c0f9eecbbc65ce85e0f Mon Sep 17 00:00:00 2001 From: baarkerlounger Date: Tue, 16 Nov 2021 17:24:15 +0000 Subject: [PATCH 03/12] Fix conditional key --- config/forms/2021_2022.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/forms/2021_2022.json b/config/forms/2021_2022.json index 7ae709b1c..885d4dadf 100644 --- a/config/forms/2021_2022.json +++ b/config/forms/2021_2022.json @@ -1088,7 +1088,7 @@ "4": "Other" }, "conditional_for": { - "other_tenancy_type": ["Other"] + "tenancyother": ["Other"] } }, "tenancyother": { From 93efd969e51f28e6ae81e1b95fd322dbed3abd41 Mon Sep 17 00:00:00 2001 From: Daniel Baark <5101747+baarkerlounger@users.noreply.github.com> Date: Wed, 17 Nov 2021 08:56:47 +0000 Subject: [PATCH 04/12] Form changes and small fixes (#96) * Remove tenancy code question * Rubocop * Fix spec * Use Gov component gem header --- app/models/bulk_upload.rb | 10 ++++---- app/views/layouts/application.html.erb | 25 +++++--------------- config/forms/2021_2022.json | 12 ---------- db/migrate/20211116102527_change_datetime.rb | 11 ++++++++- db/schema.rb | 4 ++-- 5 files changed, 24 insertions(+), 38 deletions(-) diff --git a/app/models/bulk_upload.rb b/app/models/bulk_upload.rb index f546ea56d..dcab1753f 100644 --- a/app/models/bulk_upload.rb +++ b/app/models/bulk_upload.rb @@ -26,12 +26,14 @@ class BulkUpload else data_range = FIRST_DATA_ROW..last_row data_range.map do |row_num| - case_log = CaseLog.create + case_log = CaseLog.create! map_row(sheet.row(row_num)).each do |attr_key, attr_val| - begin - case_log.update_attribute(attr_key, attr_val) - rescue ArgumentError + update = case_log.update(attr_key => attr_val) + unless update + # TODO: determine what to do when a bulk upload contains field values that don't pass validations end + rescue ArgumentError + # TODO: determine what we want to do when bulk upload contains totally invalid data for a field. end end end diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 7ea45b6e4..ad52856b9 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -33,25 +33,12 @@ Skip to main content

diff --git a/spec/features/case_log_spec.rb b/spec/features/case_log_spec.rb index 079c3bc55..2088e483d 100644 --- a/spec/features/case_log_spec.rb +++ b/spec/features/case_log_spec.rb @@ -301,6 +301,19 @@ RSpec.describe "Test Features" do expect(page).to have_link("Change", href: "/case_logs/#{empty_case_log.id}/person_1_age") end + it "should have a change link for answered questions" do + visit("/case_logs/#{empty_case_log.id}/household_needs/check_answers") + assert_selector "a", text: /Answer\z/, count: 4 + assert_selector "a", text: "Change", count: 0 + visit("/case_logs/#{empty_case_log.id}/accessibility_requirements") + check("case-log-accessibility-requirements-housingneeds-c-field") + click_button("Save and continue") + visit("/case_logs/#{empty_case_log.id}/household_needs/check_answers") + assert_selector "a", text: /Answer\z/, count: 3 + assert_selector "a", text: "Change", count: 1 + expect(page).to have_link("Change", href: "/case_logs/#{empty_case_log.id}/accessibility_requirements") + end + it "should have a link pointing to the first question if no questions are answered" do visit("/case_logs/#{empty_case_log.id}/#{subsection}/check_answers") expect(page).to have_content("You answered 0 of 4 questions") From 548467561a490a1774d3c57f887ad6b939d6e583 Mon Sep 17 00:00:00 2001 From: Daniel Baark <5101747+baarkerlounger@users.noreply.github.com> Date: Thu, 18 Nov 2021 09:45:56 +0000 Subject: [PATCH 08/12] 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 | 24 +++++++-- app/controllers/case_logs_controller.rb | 15 +----- app/helpers/check_answers_helper.rb | 23 +++----- app/models/form.rb | 53 +++++++++++-------- app/views/form/_check_answers_table.html.erb | 2 +- app/views/form/check_answers.html.erb | 8 +-- config/forms/2021_2022.json | 44 ++++++--------- config/forms/schema/generic.json | 24 ++++----- docs/adr/adr-009-form-routing-logic.md | 12 +++++ spec/controllers/case_logs_controller_spec.rb | 23 -------- spec/features/case_log_spec.rb | 20 ++----- spec/fixtures/forms/test_aboutthislog.json | 27 +++++----- spec/fixtures/forms/test_form.json | 15 ++---- spec/fixtures/forms/test_validator.json | 3 +- spec/helpers/check_answers_helper_spec.rb | 6 +-- spec/models/form_spec.rb | 38 +++++++------ 16 files changed, 147 insertions(+), 190 deletions(-) create mode 100644 docs/adr/adr-009-form-routing-logic.md diff --git a/README.md b/README.md index c1afaf7f5..6e472e56c 100644 --- a/README.md +++ b/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,12 +157,25 @@ 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: `rake form_definition:validate["config/forms/2021_2022.json"]` -n.b. You may have to escape square brackets in zsh +n.b. You may have to escape square brackets in zsh `rake form_definition:validate\["config/forms/2021_2022.json"\]` This will validate the given form definition against the schema in `config/forms/schema/generic.json`. @@ -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) diff --git a/app/controllers/case_logs_controller.rb b/app/controllers/case_logs_controller.rb index 1672daf44..4d4ed6db3 100644 --- a/app/controllers/case_logs_controller.rb +++ b/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 diff --git a/app/helpers/check_answers_helper.rb b/app/helpers/check_answers_helper.rb index 04f6e23c5..fed58b71e 100644 --- a/app/helpers/check_answers_helper.rb +++ b/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 diff --git a/app/models/form.rb b/app/models/form.rb index a2479e20b..34385af99 100644 --- a/app/models/form.rb +++ b/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" diff --git a/app/views/form/_check_answers_table.html.erb b/app/views/form/_check_answers_table.html.erb index 0ff1681db..972a3dd58 100644 --- a/app/views/form/_check_answers_table.html.erb +++ b/app/views/form/_check_answers_table.html.erb @@ -7,7 +7,7 @@ <%= form.get_answer_label(@case_log, question_title) %>
- <%= create_update_answer_link(@case_log, question_title, page, form)%> + <%= create_update_answer_link(question_title, question_info, @case_log, form) %>
diff --git a/app/views/form/check_answers.html.erb b/app/views/form/check_answers.html.erb index 46cb5a4f9..0772d2a48 100644 --- a/app/views/form/check_answers.html.erb +++ b/app/views/form/check_answers.html.erb @@ -3,12 +3,8 @@

Check the answers you gave for <%= subsection.humanize(capitalize: false) %>

<%= 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" %> diff --git a/config/forms/2021_2022.json b/config/forms/2021_2022.json index b0ac1a524..a5fb65cce 100644 --- a/config/forms/2021_2022.json +++ b/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": "", diff --git a/config/forms/schema/generic.json b/config/forms/schema/generic.json index 1499878f0..66abda1ff 100644 --- a/config/forms/schema/generic.json +++ b/config/forms/schema/generic.json @@ -27,9 +27,9 @@ "properties": { "label": { "description": "", - "type": "string" + "type": "string" }, - "subsections": { + "subsections": { "type": "object", "patternProperties": { "[a-z_]+": { @@ -41,10 +41,10 @@ "description": "", "type": "string" }, - "pages": { + "pages": { "type": "object", "patternProperties": { - "^(?!(conditional_route_to))[a-z_]+$": { + "^(?!(depends_on))[a-z_]+$": { "description": "Page Name", "type": "object", "required": ["header", "questions"], @@ -64,7 +64,7 @@ "description": "Question Name", "type": "object", "required": ["header", "type"], - "properties": { + "properties": { "header": { "description": "", "type": "string" @@ -77,7 +77,7 @@ "description": "", "type": "string", "optional": "true" - } + } }, "additionalProperties": { "hint_text": { @@ -105,25 +105,25 @@ } }, "additionalProperties": { - "conditional_route_to": { + "depends_on": { "description": "", "type": "object" } - }, + }, "minProperties": 1 } - } + } } }, "minProperties": 1 } - } + } } - }, + }, "minProperties": 2 } }, "minProperties": 1 } } -} \ No newline at end of file +} diff --git a/docs/adr/adr-009-form-routing-logic.md b/docs/adr/adr-009-form-routing-logic.md new file mode 100644 index 000000000..72aa4d2f0 --- /dev/null +++ b/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. diff --git a/spec/controllers/case_logs_controller_spec.rb b/spec/controllers/case_logs_controller_spec.rb index 8a99caae0..c29bbf689 100644 --- a/spec/controllers/case_logs_controller_spec.rb +++ b/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 diff --git a/spec/features/case_log_spec.rb b/spec/features/case_log_spec.rb index 2088e483d..f8e877e36 100644 --- a/spec/features/case_log_spec.rb +++ b/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 diff --git a/spec/fixtures/forms/test_aboutthislog.json b/spec/fixtures/forms/test_aboutthislog.json index 2cfd8ff26..798469487 100644 --- a/spec/fixtures/forms/test_aboutthislog.json +++ b/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" } } } } diff --git a/spec/fixtures/forms/test_form.json b/spec/fixtures/forms/test_form.json index 4ac56645f..d60befb1c 100644 --- a/spec/fixtures/forms/test_form.json +++ b/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" } } } } diff --git a/spec/fixtures/forms/test_validator.json b/spec/fixtures/forms/test_validator.json index 2f8e770d3..da5e04a8e 100644 --- a/spec/fixtures/forms/test_validator.json +++ b/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": "", diff --git a/spec/helpers/check_answers_helper_spec.rb b/spec/helpers/check_answers_helper_spec.rb index ac3000f6b..5b0612d21 100644 --- a/spec/helpers/check_answers_helper_spec.rb +++ b/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 diff --git a/spec/models/form_spec.rb b/spec/models/form_spec.rb index 18e15c6b4..98a8114f4 100644 --- a/spec/models/form_spec.rb +++ b/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 From f947b0f74e0dd47b01d77f3dd9702437c7f6e413 Mon Sep 17 00:00:00 2001 From: Daniel Baark <5101747+baarkerlounger@users.noreply.github.com> Date: Thu, 18 Nov 2021 09:55:15 +0000 Subject: [PATCH 09/12] Check answers should be a single summary list (#100) --- app/views/form/_check_answers_table.html.erb | 24 +++++++++----------- app/views/form/check_answers.html.erb | 8 ++++--- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/app/views/form/_check_answers_table.html.erb b/app/views/form/_check_answers_table.html.erb index 972a3dd58..6e92c3272 100644 --- a/app/views/form/_check_answers_table.html.erb +++ b/app/views/form/_check_answers_table.html.erb @@ -1,13 +1,11 @@ -
-
-
- <%= question_info["check_answer_label"].to_s.present? ? question_info["check_answer_label"].to_s : question_info["header"].to_s%> -
-
- <%= form.get_answer_label(@case_log, question_title) %> -
-
- <%= create_update_answer_link(question_title, question_info, @case_log, form) %> -
-
-
+
+
+ <%= question_info["check_answer_label"].to_s.present? ? question_info["check_answer_label"].to_s : question_info["header"].to_s%> +
+
+ <%= form.get_answer_label(@case_log, question_title) %> +
+
+ <%= create_update_answer_link(question_title, question_info, @case_log, form) %> +
+
diff --git a/app/views/form/check_answers.html.erb b/app/views/form/check_answers.html.erb index 0772d2a48..74547c016 100644 --- a/app/views/form/check_answers.html.erb +++ b/app/views/form/check_answers.html.erb @@ -3,9 +3,11 @@

Check the answers you gave for <%= subsection.humanize(capitalize: false) %>

<%= display_answered_questions_summary(subsection, @case_log, form) %> - <% 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 %> +
+ <% 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" %> <% end %> From a0a7d7f136ea0911c1cade1cedbd107007e79d5b Mon Sep 17 00:00:00 2001 From: baarkerlounger Date: Thu, 18 Nov 2021 10:29:45 +0000 Subject: [PATCH 10/12] Bump gems --- Gemfile.lock | 47 +++++++++++++++++++++-------------------------- 1 file changed, 21 insertions(+), 26 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 04db4e6f3..2942e8f5a 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,6 +1,6 @@ GIT remote: https://github.com/rspec/rspec-core.git - revision: 42a9fe3a2dc9d5e68811ee646f4b9b4349c18b24 + revision: e36aa2a9ebe68acee3ce05190fc2124947b45925 branch: main specs: rspec-core (3.11.0.pre) @@ -26,7 +26,7 @@ GIT GIT remote: https://github.com/rspec/rspec-rails.git - revision: cfe4db707cc5a0c9437aa90e3059256f30368da4 + revision: d3e7b85877fcbcec63f8a76434d8750e7f3b7aef branch: main specs: rspec-rails (5.1.0.pre) @@ -136,7 +136,7 @@ GEM rack-test (>= 0.6.3) regexp_parser (>= 1.5, < 3.0) xpath (~> 3.2) - chartkick (4.1.0) + chartkick (4.1.2) childprocess (4.1.0) coderay (1.1.3) concurrent-ruby (1.1.9) @@ -170,7 +170,7 @@ GEM activemodel (>= 6.0) railties (>= 6.0) view_component (~> 2.39.0) - govuk_design_system_formbuilder (2.7.5) + govuk_design_system_formbuilder (2.7.6) actionview (>= 6.0) activemodel (>= 6.0) activesupport (>= 6.0) @@ -190,7 +190,7 @@ GEM railties (>= 5.2, < 6.2) responders (>= 2, < 4) iniparse (1.5.0) - jbuilder (2.11.2) + jbuilder (2.11.3) activesupport (>= 5.0.0) jquery-rails (4.4.0) rails-dom-testing (>= 1, < 3) @@ -225,8 +225,6 @@ GEM minitest (5.14.4) msgpack (1.4.2) nio4r (2.5.8) - nokogiri (1.12.5-x86_64-darwin) - racc (~> 1.4) nokogiri (1.12.5-x86_64-linux) racc (~> 1.4) overcommit (0.58.0) @@ -297,35 +295,34 @@ GEM roo (2.8.3) nokogiri (~> 1) rubyzip (>= 1.3.0, < 3.0.0) - rubocop (1.21.0) + rubocop (1.23.0) parallel (~> 1.10) parser (>= 3.0.0.0) rainbow (>= 2.2.2, < 4.0) regexp_parser (>= 1.8, < 3.0) rexml - rubocop-ast (>= 1.9.1, < 2.0) + rubocop-ast (>= 1.12.0, < 2.0) ruby-progressbar (~> 1.7) unicode-display_width (>= 1.4.0, < 3.0) - rubocop-ast (1.11.0) + rubocop-ast (1.13.0) parser (>= 3.0.1.1) - rubocop-govuk (4.1.0) - rubocop (= 1.21.0) - rubocop-ast (= 1.11.0) - rubocop-rails (= 2.12.2) + rubocop-govuk (4.2.0) + rubocop (= 1.23.0) + rubocop-ast (= 1.13.0) + rubocop-rails (= 2.12.4) rubocop-rake (= 0.6.0) - rubocop-rspec (= 2.4.0) + rubocop-rspec (= 2.6.0) rubocop-performance (1.12.0) rubocop (>= 1.7.0, < 2.0) rubocop-ast (>= 0.4.0) - rubocop-rails (2.12.2) + rubocop-rails (2.12.4) activesupport (>= 4.2.0) rack (>= 1.1) rubocop (>= 1.7.0, < 2.0) rubocop-rake (0.6.0) rubocop (~> 1.0) - rubocop-rspec (2.4.0) - rubocop (~> 1.0) - rubocop-ast (>= 1.1.0) + rubocop-rspec (2.6.0) + rubocop (~> 1.19) ruby-progressbar (1.11.0) ruby2_keywords (0.0.5) rubyzip (2.3.2) @@ -352,11 +349,11 @@ GEM sprockets (4.0.2) concurrent-ruby (~> 1.0) rack (> 1, < 3) - sprockets-rails (3.2.2) - actionpack (>= 4.0) - activesupport (>= 4.0) + sprockets-rails (3.4.0) + actionpack (>= 5.2) + activesupport (>= 5.2) sprockets (>= 3.0.0) - stimulus-rails (0.7.1) + stimulus-rails (0.7.2) rails (>= 6.0.0) thor (1.1.0) turbo-rails (7.1.1) @@ -368,7 +365,7 @@ GEM view_component (2.39.0) activesupport (>= 5.0.0, < 8.0) method_source (~> 1.0) - web-console (4.1.0) + web-console (4.2.0) actionview (>= 6.0.0) activemodel (>= 6.0.0) bindex (>= 0.4.0) @@ -386,8 +383,6 @@ GEM zeitwerk (2.5.1) PLATFORMS - x86_64-darwin-19 - x86_64-darwin-20 x86_64-linux DEPENDENCIES From b003497c70564a46dff65ba024460f220de9b135 Mon Sep 17 00:00:00 2001 From: baarkerlounger Date: Thu, 18 Nov 2021 10:32:41 +0000 Subject: [PATCH 11/12] Revert yanked turbo rails for now --- Gemfile | 1 + Gemfile.lock | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/Gemfile b/Gemfile index 3c8d77954..dcb8a18b3 100644 --- a/Gemfile +++ b/Gemfile @@ -34,6 +34,7 @@ gem "roo" # Json Schema gem "json-schema" gem "uk_postcode" +gem "turbo-rails", "~> 0.8" group :development, :test do # Call 'byebug' anywhere in the code to stop execution and get a debugger console diff --git a/Gemfile.lock b/Gemfile.lock index 2942e8f5a..04961facf 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -356,7 +356,7 @@ GEM stimulus-rails (0.7.2) rails (>= 6.0.0) thor (1.1.0) - turbo-rails (7.1.1) + turbo-rails (0.8.3) rails (>= 6.0.0) tzinfo (2.0.4) concurrent-ruby (~> 1.0) @@ -419,6 +419,7 @@ DEPENDENCIES scss_lint-govuk selenium-webdriver simplecov + turbo-rails (~> 0.8) tzinfo-data uk_postcode web-console (>= 4.1.0) From 04a6986827700e951a525acd01c9238527a1e693 Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Thu, 18 Nov 2021 11:26:49 +0000 Subject: [PATCH 12/12] Change armed forces question (#101) --- app/admin/case_logs.rb | 2 +- app/constants/db_enums.rb | 10 ++++++++ app/models/bulk_upload.rb | 2 +- app/models/case_log.rb | 1 + app/validations/household_validations.rb | 8 +++--- config/forms/2021_2022.json | 31 ++++++++--------------- db/schema.rb | 5 ++-- docs/api/DLUHC-CORE-Data.v1.json | 12 +++------ spec/features/case_log_spec.rb | 6 ++--- spec/fixtures/complete_case_log.json | 3 +-- spec/fixtures/forms/test_form.json | 20 +++++++++------ spec/helpers/check_answers_helper_spec.rb | 10 ++++---- spec/models/case_log_spec.rb | 10 ++++---- 13 files changed, 58 insertions(+), 62 deletions(-) diff --git a/app/admin/case_logs.rb b/app/admin/case_logs.rb index 2a15fbb2a..d34a22623 100644 --- a/app/admin/case_logs.rb +++ b/app/admin/case_logs.rb @@ -2,7 +2,7 @@ ActiveAdmin.register CaseLog do # See permitted parameters documentation: # https://github.com/activeadmin/activeadmin/blob/master/docs/2-resource-customization.md#setting-up-strong-parameters permit_params do - permitted = %i[status tenant_code age1 sex1 tenant_ethnic_group tenant_nationality previous_housing_situation armed_forces ecstat1 other_hhmemb relat2 age2 sex2 ecstat2 relat3 age3 sex3 ecstat3 relat4 age4 sex4 ecstat4 relat5 age5 sex5 ecstat5 relat6 age6 sex6 ecstat6 relat7 age7 person_7_gender ecstat7 relat8 age8 sex8 ecstat8 homelessness reason benefit_cap_spare_room_subsidy armed_forces_active armed_forces_injured armed_forces_partner medical_conditions pregnancy accessibility_requirements condition_effects tenancy_code tenancy_start_date starter_tenancy fixed_term_tenancy tenancy_type letting_type letting_provider la previous_postcode property_relet property_vacancy_reason property_reference property_unit_type property_building_type property_number_of_bedrooms property_void_date majorrepairs mrcdate property_number_of_times_relet property_wheelchair_accessible net_income net_income_frequency net_income_uc_proportion hb rent_frequency basic_rent service_charge personal_service_charge support_charge total_charge tshortfall time_lived_in_la time_on_la_waiting_list prevloc property_postcode reasonable_preference reasonable_preference_reason cbl_letting chr_letting cap_letting hbrentshortfall other_reason accessibility_requirements_fully_wheelchair_accessible_housing accessibility_requirements_wheelchair_access_to_essential_rooms accessibility_requirements_level_access_housing accessibility_requirements_other_disability_requirements accessibility_requirements_no_disability_requirements accessibility_requirements_do_not_know accessibility_requirements_prefer_not_to_say condition_effects_vision condition_effects_hearing condition_effects_mobility condition_effects_dexterity condition_effects_stamina condition_effects_learning condition_effects_memory condition_effects_mental_health condition_effects_social_or_behavioral condition_effects_other condition_effects_prefer_not_to_say reasonable_preference_reason_homeless reasonable_preference_reason_unsatisfactory_housing reasonable_preference_reason_medical_grounds reasonable_preference_reason_avoid_hardship reasonable_preference_reason_do_not_know other_tenancy_type override_net_income_validation net_income_known] + permitted = %i[status tenant_code age1 sex1 tenant_ethnic_group tenant_nationality previous_housing_situation armedforces ecstat1 other_hhmemb relat2 age2 sex2 ecstat2 relat3 age3 sex3 ecstat3 relat4 age4 sex4 ecstat4 relat5 age5 sex5 ecstat5 relat6 age6 sex6 ecstat6 relat7 age7 person_7_gender ecstat7 relat8 age8 sex8 ecstat8 homelessness reason benefit_cap_spare_room_subsidy armed_forces_active armed_forces_injured medical_conditions pregnancy accessibility_requirements condition_effects tenancy_code tenancy_start_date starter_tenancy fixed_term_tenancy tenancy_type letting_type letting_provider la previous_postcode property_relet property_vacancy_reason property_reference property_unit_type property_building_type property_number_of_bedrooms property_void_date majorrepairs mrcdate property_number_of_times_relet property_wheelchair_accessible net_income net_income_frequency net_income_uc_proportion hb rent_frequency basic_rent service_charge personal_service_charge support_charge total_charge tshortfall time_lived_in_la time_on_la_waiting_list prevloc property_postcode reasonable_preference reasonable_preference_reason cbl_letting chr_letting cap_letting hbrentshortfall other_reason accessibility_requirements_fully_wheelchair_accessible_housing accessibility_requirements_wheelchair_access_to_essential_rooms accessibility_requirements_level_access_housing accessibility_requirements_other_disability_requirements accessibility_requirements_no_disability_requirements accessibility_requirements_do_not_know accessibility_requirements_prefer_not_to_say condition_effects_vision condition_effects_hearing condition_effects_mobility condition_effects_dexterity condition_effects_stamina condition_effects_learning condition_effects_memory condition_effects_mental_health condition_effects_social_or_behavioral condition_effects_other condition_effects_prefer_not_to_say reasonable_preference_reason_homeless reasonable_preference_reason_unsatisfactory_housing reasonable_preference_reason_medical_grounds reasonable_preference_reason_avoid_hardship reasonable_preference_reason_do_not_know other_tenancy_type override_net_income_validation net_income_known] permitted end diff --git a/app/constants/db_enums.rb b/app/constants/db_enums.rb index d28393105..7a1c01b19 100644 --- a/app/constants/db_enums.rb +++ b/app/constants/db_enums.rb @@ -699,4 +699,14 @@ module DbEnums "East Renfrewshire" => "S12000011", } end + + def self.armed_forces + { + "A current or former regular in the UK Armed Forces (exc. National Service)" => 1, + "No" => 2, + "Tenant prefers not to say" => 3, + "A current or former reserve in the UK Armed Forces (exc. National Service)" => 4, + "A spouse / civil partner of a UK Armed Forces member who has separated or been bereaved within the last 2 years" => 5, + } + end end diff --git a/app/models/bulk_upload.rb b/app/models/bulk_upload.rb index dcab1753f..720cd5a36 100644 --- a/app/models/bulk_upload.rb +++ b/app/models/bulk_upload.rb @@ -96,7 +96,7 @@ class BulkUpload ecstat8: row[42], ethnic: row[43], national: row[44], - armed_forces: row[45], + armedforces: row[45], reservist: row[46], preg_occ: row[47], hb: row[48], diff --git a/app/models/case_log.rb b/app/models/case_log.rb index 2134a766a..32bc40739 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -109,6 +109,7 @@ class CaseLog < ApplicationRecord enum hb: DbEnums.housing_benefit, _suffix: true enum hbrentshortfall: DbEnums.polar_with_unknown, _suffix: true enum property_relet: DbEnums.polar, _suffix: true + enum armedforces: DbEnums.armed_forces, _suffix: true AUTOGENERATED_FIELDS = %w[id status created_at updated_at discarded_at].freeze diff --git a/app/validations/household_validations.rb b/app/validations/household_validations.rb index bb04599c7..5671b1b75 100644 --- a/app/validations/household_validations.rb +++ b/app/validations/household_validations.rb @@ -26,21 +26,21 @@ module HouseholdValidations end def validate_armed_forces_injured(record) - if (record.armed_forces == "Yes - a regular" || record.armed_forces == "Yes - a reserve") && record.reservist.blank? + if (record.armedforces == "A current or former regular in the UK Armed Forces (exc. National Service)" || record.armedforces == "A current or former reserve in the UK Armed Forces (exc. National Service)") && record.reservist.blank? record.errors.add :reservist, "You must answer the armed forces injury question if the tenant has served in the armed forces" end - if (record.armed_forces == "No" || record.armed_forces == "Prefer not to say") && record.reservist.present? + if (record.armedforces == "No" || record.armedforces == "Prefer not to say") && record.reservist.present? record.errors.add :reservist, "You must not answer the armed forces injury question if the tenant has not served in the armed forces or prefer not to say was chosen" end end def validate_armed_forces_active_response(record) - if record.armed_forces == "Yes - a regular" && record.leftreg.blank? + if record.armedforces == "A current or former regular in the UK Armed Forces (exc. National Service)" && record.leftreg.blank? record.errors.add :leftreg, "You must answer the armed forces active question if the tenant has served as a regular in the armed forces" end - if record.armed_forces != "Yes - a regular" && record.leftreg.present? + if record.armedforces != "A current or former regular in the UK Armed Forces (exc. National Service)" && record.leftreg.present? record.errors.add :leftreg, "You must not answer the armed forces active question if the tenant has not served as a regular in the armed forces" end end diff --git a/config/forms/2021_2022.json b/config/forms/2021_2022.json index a5fb65cce..45a31ddc1 100644 --- a/config/forms/2021_2022.json +++ b/config/forms/2021_2022.json @@ -870,20 +870,21 @@ "header": "Experience of the UK Armed Forces", "description": "", "questions": { - "armed_forces": { - "header": "Has the tenant ever served in the UK armed forces?", - "hint_text": "", + "armedforces": { + "header": "Is anyone in the household...", + "hint_text": "This excludes national service", "type": "radio", "check_answer_label": "Armed Forces", "answer_options": { - "0": "Yes - a regular", - "1": "Yes - a reserve", - "2": "No", - "3": "Prefer not to say" + "0":"A current or former regular in the UK Armed Forces (exc. National Service)", + "1":"A current or former reserve in the UK Armed Forces (exc. National Service)", + "2": "A spouse / civil partner of a UK Armed Forces member who has separated or been bereaved within the last 2 years", + "3": "No", + "4": "Tenant prefers not to say" }, "conditional_for": { - "leftreg": ["Yes - a regular", "Yes - a reserve"], - "reservist": ["Yes - a regular", "Yes - a reserve"] + "leftreg": ["A current or former regular in the UK Armed Forces (exc. National Service)"], + "reservist": ["A current or former regular in the UK Armed Forces (exc. National Service)"] } }, "leftreg": { @@ -908,18 +909,6 @@ "1": "No", "2": "Prefer not to say" } - }, - "armed_forces_partner": { - "header": "Was the tenant the spouse or civil partner of someone who served in the UK armed forces?", - "hint_text": "", - "type": "radio", - "check_answer_label": "Was the tenant the spouse or civil partner of someone who served in the UK armed forces?", - "answer_options": { - "0": "Yes - was the spouse or civil partner of a UK Armed Forces member and have separated within the last 2 years", - "1": "Yes - was the spouse or civil partner of a UK Armed Forces member who died within the last 2 years", - "2": "No", - "3": "Prefer not to say" - } } } }, diff --git a/db/schema.rb b/db/schema.rb index b011b0ce8..72f15b7b2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2021_11_16_102527) do +ActiveRecord::Schema.define(version: 2021_11_18_090831) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -25,7 +25,6 @@ ActiveRecord::Schema.define(version: 2021_11_16_102527) do t.integer "ethnic" t.integer "national" t.integer "prevten" - t.string "armed_forces" t.integer "ecstat1" t.integer "hhmemb" t.string "relat2" @@ -60,7 +59,6 @@ ActiveRecord::Schema.define(version: 2021_11_16_102527) do t.integer "underoccupation_benefitcap" t.integer "leftreg" t.integer "reservist" - t.string "armed_forces_partner" t.integer "illness" t.integer "preg_occ" t.string "accessibility_requirements" @@ -154,6 +152,7 @@ ActiveRecord::Schema.define(version: 2021_11_16_102527) do t.integer "incref" t.datetime "sale_completion_date" t.datetime "startdate" + t.integer "armedforces" t.index ["discarded_at"], name: "index_case_logs_on_discarded_at" end diff --git a/docs/api/DLUHC-CORE-Data.v1.json b/docs/api/DLUHC-CORE-Data.v1.json index 5723c9311..53837c5df 100644 --- a/docs/api/DLUHC-CORE-Data.v1.json +++ b/docs/api/DLUHC-CORE-Data.v1.json @@ -261,7 +261,7 @@ "ethnic": "White: English/Scottish/Welsh/Northern Irish/British", "national": "UK national resident in UK", "prevten": "Private sector tenancy", - "armed_forces": "Yes - a regular", + "armedforces": "A current or former regular in the UK Armed Forces (exc. National Service)", "ecstat1": "Full-time - 30 hours or more", "other_hhmemb": 7, "relat2": "Partner", @@ -297,7 +297,6 @@ "underoccupation_benefitcap": "No", "leftreg": "No", "reservist": "No", - "armed_forces_partner": "No", "illness": "Yes", "preg_occ": "No", "accessibility_requirements": "No", @@ -438,7 +437,7 @@ "type": "string", "minLength": 1 }, - "armed_forces": { + "armedforces": { "type": "string", "minLength": 1 }, @@ -787,10 +786,6 @@ "type": "string", "minLength": 1 }, - "armed_forces_partner": { - "type": "string", - "minLength": 1 - }, "illness": { "type": "string", "minLength": 1 @@ -1050,7 +1045,7 @@ "ethnic", "national", "prevten", - "armed_forces", + "armedforces", "ecstat1", "other_hhmemb", "relat2", @@ -1086,7 +1081,6 @@ "underoccupation_benefitcap", "leftreg", "reservist", - "armed_forces_partner", "illness", "preg_occ", "accessibility_requirements", diff --git a/spec/features/case_log_spec.rb b/spec/features/case_log_spec.rb index f8e877e36..9107b38c2 100644 --- a/spec/features/case_log_spec.rb +++ b/spec/features/case_log_spec.rb @@ -375,13 +375,13 @@ RSpec.describe "Test Features" do it "shows conditional questions if the required answer is selected and hides it again when a different answer option is selected", js: true do visit("/case_logs/#{id}/armed_forces") # Something about our styling makes the selenium webdriver think the actual radio buttons are not visible so we allow label click here - choose("case-log-armed-forces-yes-a-regular-field", allow_label_click: true) + choose("case-log-armedforces-a-current-or-former-regular-in-the-uk-armed-forces-exc-national-service-field", allow_label_click: true) expect(page).to have_selector("#reservist_div") choose("case-log-reservist-no-field", allow_label_click: true) expect(page).to have_checked_field("case-log-reservist-no-field", visible: false) - choose("case-log-armed-forces-no-field", allow_label_click: true) + choose("case-log-armedforces-no-field", allow_label_click: true) expect(page).not_to have_selector("#reservist_div") - choose("case-log-armed-forces-yes-a-regular-field", allow_label_click: true) + choose("case-log-armedforces-a-current-or-former-regular-in-the-uk-armed-forces-exc-national-service-field", allow_label_click: true) expect(page).to have_unchecked_field("case-log-reservist-no-field", visible: false) end end diff --git a/spec/fixtures/complete_case_log.json b/spec/fixtures/complete_case_log.json index bb16bc42e..fe8393178 100644 --- a/spec/fixtures/complete_case_log.json +++ b/spec/fixtures/complete_case_log.json @@ -6,7 +6,7 @@ "ethnic": "White: English/Scottish/Welsh/Northern Irish/British", "national": "UK national resident in UK", "prevten": "Private sector tenancy", - "armed_forces": "Yes - a regular", + "armedforces": "A current or former regular in the UK Armed Forces (exc. National Service)", "ecstat1": "Full-time - 30 hours or more", "other_hhmemb": 7, "hhmemb": 8, @@ -43,7 +43,6 @@ "underoccupation_benefitcap": "No", "leftreg": "No - they left up to 5 years ago", "reservist": "No", - "armed_forces_partner": "No", "illness": "Yes", "preg_occ": "No", "accessibility_requirements": "No", diff --git a/spec/fixtures/forms/test_form.json b/spec/fixtures/forms/test_form.json index d60befb1c..4b2fd6c65 100644 --- a/spec/fixtures/forms/test_form.json +++ b/spec/fixtures/forms/test_form.json @@ -107,23 +107,26 @@ "armed_forces": { "header": "Experience of the UK Armed Forces", "questions": { - "armed_forces": { - "header": "Has the tenant ever served in the UK armed forces?", + "armedforces": { + "header": "Is anyone in the household...", + "hint_text": "This excludes national service", "type": "radio", "check_answer_label": "Armed Forces", "answer_options": { - "0": "Yes - a regular", - "1": "Yes - a reserve", - "2": "No", - "3": "Prefer not to say" + "0":"A current or former regular in the UK Armed Forces (exc. National Service)", + "1":"A current or former reserve in the UK Armed Forces (exc. National Service)", + "2": "A spouse / civil partner of a UK Armed Forces member who has separated or been bereaved within the last 2 years", + "3": "No", + "4": "Tenant prefers not to say" }, "conditional_for": { - "leftreg": ["Yes - a regular", "Yes - a reserve"], - "reservist": ["Yes - a regular", "Yes - a reserve"] + "leftreg": ["A current or former regular in the UK Armed Forces (exc. National Service)"], + "reservist": ["A current or former regular in the UK Armed Forces (exc. National Service)"] } }, "leftreg": { "header": "Are they still serving?", + "hint_text": "", "type": "radio", "check_answer_label": "When did they leave the Armed Forces?", "answer_options": { @@ -135,6 +138,7 @@ }, "reservist": { "header": "Were they seriously injured or ill as a result of their service?", + "hint_text": "", "type": "radio", "check_answer_label": "Has anyone in the household been seriously injured or ill as a result of their service in the armed forces?", "answer_options": { diff --git a/spec/helpers/check_answers_helper_spec.rb b/spec/helpers/check_answers_helper_spec.rb index 5b0612d21..5b3ddf16f 100644 --- a/spec/helpers/check_answers_helper_spec.rb +++ b/spec/helpers/check_answers_helper_spec.rb @@ -11,7 +11,7 @@ RSpec.describe CheckAnswersHelper do ) end let(:case_log_with_met_radio_condition) do - FactoryBot.create(:case_log, armed_forces: "Yes - a regular", + FactoryBot.create(:case_log, armedforces: "A current or former regular in the UK Armed Forces (exc. National Service)", reservist: "No", leftreg: "Yes") end @@ -47,7 +47,7 @@ RSpec.describe CheckAnswersHelper do end it "ignores questions with unmet radio conditions" do - case_log["armed_forces"] = "No" + case_log["armedforces"] = "No" expect(total_answered_questions(subsection_with_radio_conditionals, case_log, form)).to equal(1) end @@ -134,14 +134,14 @@ RSpec.describe CheckAnswersHelper do context "conditional questions on the same page" do it "it filters out conditional questions that were not displayed" do result = total_questions(conditional_page_subsection, case_log, form) - expected_keys = %w[armed_forces illness accessibility_requirements condition_effects] + expected_keys = %w[armedforces illness accessibility_requirements condition_effects] expect(result.keys).to eq(expected_keys) end it "it includes conditional questions that were displayed" do - case_log["armed_forces"] = "Yes - a regular" + case_log["armedforces"] = "A current or former regular in the UK Armed Forces (exc. National Service)" result = total_questions(conditional_page_subsection, case_log, form) - expected_keys = %w[armed_forces leftreg reservist illness accessibility_requirements condition_effects] + expected_keys = %w[armedforces leftreg reservist illness accessibility_requirements condition_effects] expect(result.keys).to eq(expected_keys) end end diff --git a/spec/models/case_log_spec.rb b/spec/models/case_log_spec.rb index fad6ca4b9..19909ccc3 100644 --- a/spec/models/case_log_spec.rb +++ b/spec/models/case_log_spec.rb @@ -84,14 +84,14 @@ RSpec.describe Form, type: :model do context "armed forces injured validation" do it "must be answered if tenant was a regular or reserve in armed forces" do expect { - CaseLog.create!(armed_forces: "Yes - a regular", + CaseLog.create!(armedforces: "A current or former regular in the UK Armed Forces (exc. National Service)", reservist: nil) }.to raise_error(ActiveRecord::RecordInvalid) end it "must be answered if tenant was not a regular or reserve in armed forces" do expect { - CaseLog.create!(armed_forces: "No", + CaseLog.create!(armedforces: "No", reservist: "Yes") }.to raise_error(ActiveRecord::RecordInvalid) end @@ -223,14 +223,14 @@ RSpec.describe Form, type: :model do context "armed forces active validation" do it "must be answered if ever served in the forces as a regular" do expect { - CaseLog.create!(armed_forces: "Yes - a regular", + CaseLog.create!(armedforces: "A current or former regular in the UK Armed Forces (exc. National Service)", leftreg: nil) }.to raise_error(ActiveRecord::RecordInvalid) end it "must not be answered if not ever served as a regular" do expect { - CaseLog.create!(armed_forces: "No", + CaseLog.create!(armedforces: "No", leftreg: "Yes") }.to raise_error(ActiveRecord::RecordInvalid) end @@ -238,7 +238,7 @@ RSpec.describe Form, type: :model do # Crossover over tests here as injured must be answered as well for no error it "must be answered if ever served in the forces as a regular" do expect do - CaseLog.create!(armed_forces: "Yes - a regular", + CaseLog.create!(armedforces: "A current or former regular in the UK Armed Forces (exc. National Service)", leftreg: "Yes", reservist: "Yes") end