From 04b823ff77e6617ff2d58d56e857656c3885d325 Mon Sep 17 00:00:00 2001 From: Kat Date: Wed, 17 Nov 2021 11:40:24 +0000 Subject: [PATCH 1/4] Fix validation name --- app/validations/date_validations.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/validations/date_validations.rb b/app/validations/date_validations.rb index b44cb8d53..77f737344 100644 --- a/app/validations/date_validations.rb +++ b/app/validations/date_validations.rb @@ -3,7 +3,7 @@ module DateValidations date_valid?("mrcdate", record) end - def validate_tenancy_start_date(record) + def validate_startdate(record) date_valid?("startdate", record) end From 79def05518f3d311eefc4f45ff0f62649480cb3f Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Wed, 17 Nov 2021 16:19:51 +0000 Subject: [PATCH 2/4] Display correct answer/change link for the checkboxes in the checck answers page (#99) --- app/helpers/check_answers_helper.rb | 11 ++++++++--- app/views/form/_check_answers_table.html.erb | 2 +- spec/features/case_log_spec.rb | 13 +++++++++++++ 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/app/helpers/check_answers_helper.rb b/app/helpers/check_answers_helper.rb index ba6373b72..04f6e23c5 100644 --- a/app/helpers/check_answers_helper.rb +++ b/app/helpers/check_answers_helper.rb @@ -37,9 +37,14 @@ module CheckAnswersHelper form.next_page(page_name) end - def create_update_answer_link(case_log_answer, case_log_id, page) - link_name = case_log_answer.blank? ? "Answer" : "Change" - link_to(link_name, "/case_logs/#{case_log_id}/#{page}", class: "govuk-link").html_safe + 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" + else + case_log[question_title].blank? ? "Answer" : "Change" + end + 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) diff --git a/app/views/form/_check_answers_table.html.erb b/app/views/form/_check_answers_table.html.erb index eaa54075a..0ff1681db 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], @case_log.id, page)%> + <%= create_update_answer_link(@case_log, question_title, page, form)%>
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 3/4] 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 4/4] 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 %>