From cee7c8873e1d4749dbc130e0f95e456ef17db3b5 Mon Sep 17 00:00:00 2001 From: Ted-U <92022120+Ted-U@users.noreply.github.com> Date: Thu, 14 Jul 2022 14:02:02 +0100 Subject: [PATCH] Added "skip for now" button (#701) --- app/controllers/form_controller.rb | 37 ++++++++- app/models/case_log.rb | 1 - app/models/form/question.rb | 10 +++ .../validations/submission_validations.rb | 10 --- app/views/form/page.html.erb | 15 +++- config/forms/2021_2022.json | 66 +++++----------- config/forms/2022_2023.json | 70 +++++------------ config/locales/en.yml | 1 + spec/features/form/check_answers_page_spec.rb | 2 +- spec/features/form/form_navigation_spec.rb | 76 ++++++++++++++++++- spec/features/form/page_routing_spec.rb | 1 + .../form/progressive_total_field_spec.rb | 4 + spec/features/form/saving_data_spec.rb | 1 - spec/rails_helper.rb | 1 + 14 files changed, 180 insertions(+), 115 deletions(-) delete mode 100644 app/models/validations/submission_validations.rb diff --git a/app/controllers/form_controller.rb b/app/controllers/form_controller.rb index df924665a..aed35e7a1 100644 --- a/app/controllers/form_controller.rb +++ b/app/controllers/form_controller.rb @@ -7,11 +7,16 @@ class FormController < ApplicationController if @case_log @page = @case_log.form.get_page(params[:case_log][:page]) responses_for_page = responses_for_page(@page) - if @case_log.update(responses_for_page) - session[:errors] = nil + mandatory_questions_with_no_response = mandatory_questions_with_no_response(responses_for_page) + + if mandatory_questions_with_no_response.empty? && @case_log.update(responses_for_page) + session[:errors] = session[:fields] = nil redirect_to(successful_redirect_path) else redirect_path = "case_log_#{@page.id}_path" + mandatory_questions_with_no_response.map do |question| + @case_log.errors.add question.id.to_sym, question.unanswered_error_message + end session[:errors] = @case_log.errors.to_json Rails.logger.info "User triggered validation(s) on: #{@case_log.errors.map(&:attribute).join(', ')}" redirect_to(send(redirect_path, @case_log)) @@ -48,6 +53,7 @@ class FormController < ApplicationController messages.each { |message| @case_log.errors.add field.to_sym, message } end end + session["fields"].each { |field, value| @case_log[field] = value } if session["fields"] @subsection = @case_log.form.subsection_for_page(page) @page = @case_log.form.get_page(page.id) if @page.routed_to?(@case_log, current_user) @@ -120,4 +126,31 @@ private redirect_path = @case_log.form.next_page_redirect_path(@page, @case_log, current_user) send(redirect_path, @case_log) end + + def mandatory_questions_with_no_response(responses_for_page) + session["fields"] = {} + calc_questions = @page.questions.map(&:result_field) + @page.questions.select do |question| + next if calc_questions.include?(question.id) + + question_is_required?(question) && question_missing_response?(responses_for_page, question) + end + end + + def question_is_required?(question) + CaseLog::OPTIONAL_FIELDS.exclude?(question.id) && + @page.subsection.applicable_questions(@case_log).map(&:id).include?(question.id) + end + + def question_missing_response?(responses_for_page, question) + if %w[checkbox validation_override].include?(question.type) + question.answer_options.keys.reject { |x| x.match(/divider/) }.all? do |option| + session["fields"][option] = @case_log[option] = params["case_log"][question.id].include?(option) ? 1 : 0 + params["case_log"][question.id].exclude?(option) + end + else + session["fields"][question.id] = @case_log[question.id] = responses_for_page[question.id] + responses_for_page[question.id].nil? || responses_for_page[question.id].blank? + end + end end diff --git a/app/models/case_log.rb b/app/models/case_log.rb index c2465105c..ddf728329 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -8,7 +8,6 @@ class CaseLogValidator < ActiveModel::Validator include Validations::TenancyValidations include Validations::DateValidations include Validations::LocalAuthorityValidations - include Validations::SubmissionValidations def validate(record) validation_methods = public_methods.select { |method| method.starts_with?("validate_") } diff --git a/app/models/form/question.rb b/app/models/form/question.rb index 0a2bc9998..17d047cdb 100644 --- a/app/models/form/question.rb +++ b/app/models/form/question.rb @@ -177,6 +177,16 @@ class Form::Question type == "radio" && RADIO_REFUSED_VALUE[id.to_sym]&.include?(value) end + def display_label + check_answer_label || header || id.humanize + end + + def unanswered_error_message + return I18n.t("validations.declaration.missing") if id == "declaration" + + I18n.t("validations.not_answered", question: display_label.downcase) + end + def suffix_label(case_log) return "" unless suffix return suffix if suffix.is_a?(String) diff --git a/app/models/validations/submission_validations.rb b/app/models/validations/submission_validations.rb deleted file mode 100644 index c44799892..000000000 --- a/app/models/validations/submission_validations.rb +++ /dev/null @@ -1,10 +0,0 @@ -module Validations::SubmissionValidations - # Validations methods need to be called 'validate_' to run on model save - # or 'validate_' to run on submit as well - - def validate_declaration(record) - if record.declaration&.zero? - record.errors.add :declaration, I18n.t("validations.declaration.missing") - end - end -end diff --git a/app/views/form/page.html.erb b/app/views/form/page.html.erb index f4f5fe9ae..f0edd1afb 100644 --- a/app/views/form/page.html.erb +++ b/app/views/form/page.html.erb @@ -5,7 +5,6 @@ <% end %>
- <%= form_with model: @case_log, url: form_case_log_path(@case_log), method: "post", local: true do |f| %>
"> @@ -39,9 +38,17 @@ <% end %> <%= f.hidden_field :page, value: @page.id %> - <% if !@page.id.include?("value_check") %> - <%= f.govuk_submit "Save and continue" %> - <% end %> + +
+ <% if !@page.id.include?("value_check") && if request.query_parameters["referrer"] != "check_answers" %> + <%= f.govuk_submit "Save and continue" %> + <%= govuk_link_to "Skip for now", send(@case_log.form.next_page_redirect_path(@page, @case_log, current_user), @case_log) %> + <% else %> + <%= f.govuk_submit "Save changes" %> + <%= govuk_link_to "Cancel", "/logs/#{@case_log.id}/setup/check-answers" %> + <% end %> + <% end %> +
<% end %> diff --git a/config/forms/2021_2022.json b/config/forms/2021_2022.json index d93f04e22..fac033857 100644 --- a/config/forms/2021_2022.json +++ b/config/forms/2021_2022.json @@ -1078,7 +1078,8 @@ "step": 1, "width": 2 } - } + }, + "depends_on": [{ "declaration": 1 }] }, "no_females_pregnant_household_lead_hhmemb_value_check": { "depends_on": [{ "no_females_in_a_pregnant_household?": true }], @@ -1120,11 +1121,7 @@ } }, "females_in_soft_age_range_in_pregnant_household_lead_hhmemb_value_check": { - "depends_on": [ - { - "female_in_pregnant_household_in_soft_validation_range?": true - } - ], + "depends_on": [{ "female_in_pregnant_household_in_soft_validation_range?": true }], "title_text": { "translation": "soft_validations.pregnancy.title", "arguments": [ @@ -1207,7 +1204,8 @@ "value": "Not known" } } - } + }, + "depends_on": [{ "declaration": 1 }] }, "no_females_pregnant_household_lead_age_value_check": { "depends_on": [{ "no_females_in_a_pregnant_household?": true }], @@ -1249,11 +1247,7 @@ } }, "females_in_soft_age_range_in_pregnant_household_lead_age_value_check": { - "depends_on": [ - { - "female_in_pregnant_household_in_soft_validation_range?": true - } - ], + "depends_on": [{ "female_in_pregnant_household_in_soft_validation_range?": true }], "title_text": { "translation": "soft_validations.pregnancy.title", "arguments": [ @@ -1318,7 +1312,8 @@ } } } - } + }, + "depends_on": [{ "declaration": 1 }] }, "no_females_pregnant_household_lead_value_check": { "depends_on": [{ "no_females_in_a_pregnant_household?": true }], @@ -1360,11 +1355,7 @@ } }, "females_in_soft_age_range_in_pregnant_household_lead_value_check": { - "depends_on": [ - { - "female_in_pregnant_household_in_soft_validation_range?": true - } - ], + "depends_on": [{ "female_in_pregnant_household_in_soft_validation_range?": true }], "title_text": { "translation": "soft_validations.pregnancy.title", "arguments": [ @@ -1435,7 +1426,8 @@ } } } - } + }, + "depends_on": [{ "declaration": 1 }] }, "lead_tenant_ethnic_background_arab": { "header": "", @@ -1456,11 +1448,7 @@ } } }, - "depends_on": [ - { - "ethnic_group": 4 - } - ] + "depends_on": [{ "ethnic_group": 4 }] }, "lead_tenant_ethnic_background_asian": { "header": "", @@ -1490,11 +1478,7 @@ } } }, - "depends_on": [ - { - "ethnic_group": 2 - } - ] + "depends_on": [{ "ethnic_group": 2 }] }, "lead_tenant_ethnic_background_black": { "header": "", @@ -1518,11 +1502,7 @@ } } }, - "depends_on": [ - { - "ethnic_group": 3 - } - ] + "depends_on": [{ "ethnic_group": 3 }] }, "lead_tenant_ethnic_background_mixed": { "header": "", @@ -1549,11 +1529,7 @@ } } }, - "depends_on": [ - { - "ethnic_group": 1 - } - ] + "depends_on": [{ "ethnic_group": 1 }] }, "lead_tenant_ethnic_background_white": { "header": "", @@ -1580,11 +1556,7 @@ } } }, - "depends_on": [ - { - "ethnic_group": 0 - } - ] + "depends_on": [{ "ethnic_group": 0 }] }, "lead_tenant_nationality": { "header": "", @@ -1650,7 +1622,8 @@ } } } - } + }, + "depends_on": [{ "declaration": 1 }] }, "lead_tenant_working_situation": { "header": "", @@ -1697,7 +1670,8 @@ } } } - } + }, + "depends_on": [{ "declaration": 1 }] }, "lead_tenant_under_retirement_value_check": { "depends_on": [{ "person_1_retired_under_soft_min_age?": true }], diff --git a/config/forms/2022_2023.json b/config/forms/2022_2023.json index 66cc072f3..db26c7d0f 100644 --- a/config/forms/2022_2023.json +++ b/config/forms/2022_2023.json @@ -1113,7 +1113,8 @@ "step": 1, "width": 2 } - } + }, + "depends_on": [{ "declaration": 1 }] }, "no_females_pregnant_household_lead_hhmemb_value_check": { "depends_on": [{ "no_females_in_a_pregnant_household?": true }], @@ -1155,11 +1156,7 @@ } }, "females_in_soft_age_range_in_pregnant_household_lead_hhmemb_value_check": { - "depends_on": [ - { - "female_in_pregnant_household_in_soft_validation_range?": true - } - ], + "depends_on": [{ "female_in_pregnant_household_in_soft_validation_range?": true }], "title_text": { "translation": "soft_validations.pregnancy.title", "arguments": [ @@ -1242,7 +1239,8 @@ "value": "Not known" } } - } + }, + "depends_on": [{ "declaration": 1 }] }, "no_females_pregnant_household_lead_age_value_check": { "depends_on": [{ "no_females_in_a_pregnant_household?": true }], @@ -1284,11 +1282,7 @@ } }, "females_in_soft_age_range_in_pregnant_household_lead_age_value_check": { - "depends_on": [ - { - "female_in_pregnant_household_in_soft_validation_range?": true - } - ], + "depends_on": [{ "female_in_pregnant_household_in_soft_validation_range?": true }], "title_text": { "translation": "soft_validations.pregnancy.title", "arguments": [ @@ -1353,7 +1347,8 @@ } } } - } + }, + "depends_on": [{ "declaration": 1 }] }, "no_females_pregnant_household_lead_value_check": { "depends_on": [{ "no_females_in_a_pregnant_household?": true }], @@ -1395,11 +1390,7 @@ } }, "females_in_soft_age_range_in_pregnant_household_lead_value_check": { - "depends_on": [ - { - "female_in_pregnant_household_in_soft_validation_range?": true - } - ], + "depends_on": [{ "female_in_pregnant_household_in_soft_validation_range?": true }], "title_text": { "translation": "soft_validations.pregnancy.title", "arguments": [ @@ -1470,7 +1461,8 @@ } } } - } + }, + "depends_on": [{ "declaration": 1 }] }, "lead_tenant_ethnic_background_arab": { "header": "", @@ -1491,11 +1483,7 @@ } } }, - "depends_on": [ - { - "ethnic_group": 4 - } - ] + "depends_on": [{ "ethnic_group": 4 }] }, "lead_tenant_ethnic_background_asian": { "header": "", @@ -1525,11 +1513,7 @@ } } }, - "depends_on": [ - { - "ethnic_group": 2 - } - ] + "depends_on": [{ "ethnic_group": 2 }] }, "lead_tenant_ethnic_background_black": { "header": "", @@ -1553,11 +1537,7 @@ } } }, - "depends_on": [ - { - "ethnic_group": 3 - } - ] + "depends_on": [{ "ethnic_group": 3 }] }, "lead_tenant_ethnic_background_mixed": { "header": "", @@ -1584,11 +1564,7 @@ } } }, - "depends_on": [ - { - "ethnic_group": 1 - } - ] + "depends_on": [{ "ethnic_group": 1 }] }, "lead_tenant_ethnic_background_white": { "header": "", @@ -1615,11 +1591,7 @@ } } }, - "depends_on": [ - { - "ethnic_group": 0 - } - ] + "depends_on": [{ "ethnic_group": 0 }] }, "lead_tenant_nationality": { "header": "", @@ -1649,7 +1621,8 @@ } } } - } + }, + "depends_on": [{ "declaration": 1 }] }, "lead_tenant_working_situation": { "header": "", @@ -1696,7 +1669,8 @@ } } } - } + }, + "depends_on": [{ "declaration": 1 }] }, "lead_tenant_under_retirement_value_check": { "depends_on": [{ "person_1_retired_under_soft_min_age?": true }], @@ -1743,9 +1717,7 @@ } }, "lead_tenant_over_retirement_value_check": { - "depends_on": [ - { "person_1_not_retired_over_soft_max_age?": true } - ], + "depends_on": [{ "person_1_not_retired_over_soft_max_age?": true }], "title_text": { "translation": "soft_validations.retirement.max.title", "arguments": [ diff --git a/config/locales/en.yml b/config/locales/en.yml index 4c3083b10..d27eb0c39 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -51,6 +51,7 @@ en: organisation: name_missing: "Enter the organisation’s name" provider_type_missing: "Select the organisation’s type" + not_answered: "You must answer %{question}" other_field_missing: "If %{main_field_label} is other then %{other_field_label} must be provided" other_field_not_required: "%{other_field_label} must not be provided if %{main_field_label} was not other" diff --git a/spec/features/form/check_answers_page_spec.rb b/spec/features/form/check_answers_page_spec.rb index aab201137..f3b9418a2 100644 --- a/spec/features/form/check_answers_page_spec.rb +++ b/spec/features/form/check_answers_page_spec.rb @@ -136,7 +136,7 @@ RSpec.describe "Form Check Answers Page" do first("a", text: /Change/).click uncheck("case-log-accessibility-requirements-housingneeds-c-field") check("case-log-accessibility-requirements-housingneeds-b-field") - click_button("Save and continue") + click_button("Save changes") expect(page).to have_current_path("/logs/#{empty_case_log.id}/household-needs/check-answers") end end diff --git a/spec/features/form/form_navigation_spec.rb b/spec/features/form/form_navigation_spec.rb index 1dbdaef25..d0ca551b4 100644 --- a/spec/features/form/form_navigation_spec.rb +++ b/spec/features/form/form_navigation_spec.rb @@ -13,6 +13,15 @@ RSpec.describe "Form Navigation" do created_by: user, ) end + let(:empty_case_log) do + FactoryBot.create( + :case_log, + owning_organisation: user.organisation, + managing_organisation: user.organisation, + created_by: user, + ) + end + let(:id) { case_log.id } let(:question_answers) do { @@ -47,11 +56,23 @@ RSpec.describe "Form Navigation" do pages = question_answers.map { |_key, val| val[:path] } pages[0..-2].each_with_index do |val, index| visit("/logs/#{id}/#{val}") - click_button("Save and continue") + click_link("Skip for now") expect(page).to have_current_path("/logs/#{id}/#{pages[index + 1]}") end end + it "a question page has a Skip for now link that lets you move on to the next question without inputting anything" do + visit("logs/#{empty_case_log.id}/tenant-code-test") + click_link(text: "Skip for now") + expect(page).to have_current_path("/logs/#{empty_case_log.id}/person-1-age") + end + + it "routes to check answers when skipping on the last page in the form" do + visit("logs/#{empty_case_log.id}/propcode") + click_link(text: "Skip for now") + expect(page).to have_current_path("/logs/#{empty_case_log.id}/household-characteristics/check-answers") + end + describe "Back link directs correctly", js: true do it "go back to tasklist page from tenant code" do visit("/logs/#{id}") @@ -89,4 +110,57 @@ RSpec.describe "Form Navigation" do end end end + + describe "Editing a log" do + it "a question page has a link allowing you to cancel your input and return to the check answers page" do + visit("logs/#{id}/tenant-code-test?referrer=check_answers") + click_link(text: "Cancel") + expect(page).to have_current_path("/logs/#{id}/setup/check-answers") + end + + context "when clicking save and continue on a mandatory question with no input" do + let(:id) { empty_case_log.id } + + it "shows a validation error on radio questions" do + visit("/logs/#{id}/renewal") + click_button("Save and continue") + expect(page).to have_selector("#error-summary-title") + expect(page).to have_selector("#case-log-renewal-error") + expect(page).to have_title("Error") + end + + it "shows a validation error on date questions" do + visit("/logs/#{id}/tenancy-start-date") + click_button("Save and continue") + expect(page).to have_selector("#error-summary-title") + expect(page).to have_selector("#case-log-startdate-error") + expect(page).to have_title("Error") + end + + context "when the page has a main and conditional question" do + context "when the conditional question is required but not answered" do + it "shows a validation error for the conditional question" do + visit("/logs/#{id}/armed-forces") + choose("case-log-armedforces-1-field", allow_label_click: true) + click_button("Save and continue") + expect(page).to have_selector("#error-summary-title") + expect(page).to have_selector("#case-log-leftreg-error") + expect(page).to have_title("Error") + end + end + end + end + + context "when clicking save and continue on an optional question with no input" do + let(:id) { empty_case_log.id } + + it "does not show a validation error" do + visit("/logs/#{id}/tenant-code") + click_button("Save and continue") + expect(page).not_to have_selector("#error-summary-title") + expect(page).not_to have_title("Error") + expect(page).to have_current_path("/logs/#{id}/property-reference") + end + end + end end diff --git a/spec/features/form/page_routing_spec.rb b/spec/features/form/page_routing_spec.rb index 83a1078ff..644bbb608 100644 --- a/spec/features/form/page_routing_spec.rb +++ b/spec/features/form/page_routing_spec.rb @@ -43,6 +43,7 @@ RSpec.describe "Form Page Routing" do choose("case-log-preg-occ-2-field", allow_label_click: true) click_button("Save and continue") expect(page).to have_current_path("/logs/#{id}/conditional-question-no-page") + choose("case-log-cbl-0-field", allow_label_click: true) click_button("Save and continue") expect(page).to have_current_path("/logs/#{id}/conditional-question/check-answers") end diff --git a/spec/features/form/progressive_total_field_spec.rb b/spec/features/form/progressive_total_field_spec.rb index 2e9b0cdf9..2ddd7dffd 100644 --- a/spec/features/form/progressive_total_field_spec.rb +++ b/spec/features/form/progressive_total_field_spec.rb @@ -33,6 +33,7 @@ RSpec.describe "Accessible Automcomplete" do it "total displays despite error message", js: true do visit("/logs/#{case_log.id}/rent") + choose("case-log-period-1-field", allow_label_click: true) fill_in("case-log-brent-field", with: 500) fill_in("case-log-scharge-field", with: 50) fill_in("case-log-pscharge-field", with: 50) @@ -40,6 +41,9 @@ RSpec.describe "Accessible Automcomplete" do expect(find("#case-log-tcharge-field").value).to eq("5600.00") click_button("Save and continue") expect(page).to have_selector(".govuk-error-summary") + fill_in("case-log-scharge-field", with: nil) + fill_in("case-log-pscharge-field", with: nil) + fill_in("case-log-supcharg-field-error", with: nil) fill_in("case-log-brent-field", with: 500) expect(find("#case-log-tcharge-field").value).to eq("500.00") fill_in("case-log-supcharg-field-error", with: 50) diff --git a/spec/features/form/saving_data_spec.rb b/spec/features/form/saving_data_spec.rb index 97aeb7521..425c0dca3 100644 --- a/spec/features/form/saving_data_spec.rb +++ b/spec/features/form/saving_data_spec.rb @@ -26,7 +26,6 @@ RSpec.describe "Form Saving Data" do tenancycode: { type: "text", answer: "BZ737", path: "tenant-code-test" }, age1: { type: "numeric", answer: 25, path: "person_1_age" }, sex1: { type: "radio", answer: { "F" => "Female" }, path: "person_1_gender" }, - hhmemb: { type: "numeric", answer: 3, path: "household_number_of_members" }, } end diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 5787ffe66..78867bad9 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -80,6 +80,7 @@ RSpec.configure do |config| Capybara.server = :puma, { Silent: true } config.include Devise::Test::ControllerHelpers, type: :controller + config.include Devise::Test::ControllerHelpers, type: :view config.include Devise::Test::IntegrationHelpers, type: :request config.include ViewComponent::TestHelpers, type: :component config.include Capybara::RSpecMatchers, type: :component