Added "skip for now" button (#701)

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)
+      mandatory_questions_with_no_response = mandatory_questions_with_no_response(responses_for_page)
-        session[:errors] = nil
+
+      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

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_") }

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)

app/models/validations/submission_validations.rb

@@ -1,10 +0,0 @@
-module Validations::SubmissionValidations
-  # Validations methods need to be called 'validate_<page_name>' 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

app/views/form/page.html.erb

@@ -5,7 +5,6 @@
 <% end %>
 
 <div data-controller="govukfrontend"></div>
-
 <%= form_with model: @case_log, url: form_case_log_path(@case_log), method: "post", local: true do |f| %>
   <div class="govuk-grid-row">
     <div class="govuk-grid-column-<%= @page.questions[0].type == "interruption_screen" ? "full-from-desktop" : "two-thirds-from-desktop" %>">
@@ -39,9 +38,17 @@
       <% end %>
 
       <%= f.hidden_field :page, value: @page.id %>
-      <% if !@page.id.include?("value_check") %>
+
-        <%= f.govuk_submit "Save and continue" %>
+      <div class="govuk-button-group">
-      <% 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 %>
+      </div>
     </div>
   </div>
 <% end %>

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": [
+              "depends_on": [{ "female_in_pregnant_household_in_soft_validation_range?": true }],
-                {
-                  "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": [
+              "depends_on": [{ "female_in_pregnant_household_in_soft_validation_range?": true }],
-                {
-                  "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": [
+              "depends_on": [{ "female_in_pregnant_household_in_soft_validation_range?": true }],
-                {
-                  "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": [
+              "depends_on": [{ "ethnic_group": 4 }]
-                {
-                  "ethnic_group": 4
-                }
-              ]
             },
             "lead_tenant_ethnic_background_asian": {
               "header": "",
@@ -1490,11 +1478,7 @@
                   }
                 }
               },
-              "depends_on": [
+              "depends_on": [{ "ethnic_group": 2 }]
-                {
-                  "ethnic_group": 2
-                }
-              ]
             },
             "lead_tenant_ethnic_background_black": {
               "header": "",
@@ -1518,11 +1502,7 @@
                   }
                 }
               },
-              "depends_on": [
+              "depends_on": [{ "ethnic_group": 3 }]
-                {
-                  "ethnic_group": 3
-                }
-              ]
             },
             "lead_tenant_ethnic_background_mixed": {
               "header": "",
@@ -1549,11 +1529,7 @@
                   }
                 }
               },
-              "depends_on": [
+              "depends_on": [{ "ethnic_group": 1 }]
-                {
-                  "ethnic_group": 1
-                }
-              ]
             },
             "lead_tenant_ethnic_background_white": {
               "header": "",
@@ -1580,11 +1556,7 @@
                   }
                 }
               },
-              "depends_on": [
+              "depends_on": [{ "ethnic_group": 0 }]
-                {
-                  "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 }],

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": [
+              "depends_on": [{ "female_in_pregnant_household_in_soft_validation_range?": true }],
-                {
-                  "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": [
+              "depends_on": [{ "female_in_pregnant_household_in_soft_validation_range?": true }],
-                {
-                  "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": [
+              "depends_on": [{ "female_in_pregnant_household_in_soft_validation_range?": true }],
-                {
-                  "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": [
+              "depends_on": [{ "ethnic_group": 4 }]
-                {
-                  "ethnic_group": 4
-                }
-              ]
             },
             "lead_tenant_ethnic_background_asian": {
               "header": "",
@@ -1525,11 +1513,7 @@
                   }
                 }
               },
-              "depends_on": [
+              "depends_on": [{ "ethnic_group": 2 }]
-                {
-                  "ethnic_group": 2
-                }
-              ]
             },
             "lead_tenant_ethnic_background_black": {
               "header": "",
@@ -1553,11 +1537,7 @@
                   }
                 }
               },
-              "depends_on": [
+              "depends_on": [{ "ethnic_group": 3 }]
-                {
-                  "ethnic_group": 3
-                }
-              ]
             },
             "lead_tenant_ethnic_background_mixed": {
               "header": "",
@@ -1584,11 +1564,7 @@
                   }
                 }
               },
-              "depends_on": [
+              "depends_on": [{ "ethnic_group": 1 }]
-                {
-                  "ethnic_group": 1
-                }
-              ]
             },
             "lead_tenant_ethnic_background_white": {
               "header": "",
@@ -1615,11 +1591,7 @@
                   }
                 }
               },
-              "depends_on": [
+              "depends_on": [{ "ethnic_group": 0 }]
-                {
-                  "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": [
+              "depends_on": [{ "person_1_not_retired_over_soft_max_age?": true }],
-                { "person_1_not_retired_over_soft_max_age?": true }
-              ],
               "title_text": {
                 "translation": "soft_validations.retirement.max.title",
                 "arguments": [

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"

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

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

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

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)

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
 

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