From 81ff68bb1162a51a4d1c3b53186a50f707fae93f Mon Sep 17 00:00:00 2001
From: Daniel Baark <5101747+baarkerlounger@users.noreply.github.com>
Date: Tue, 2 Nov 2021 09:38:06 +0000
Subject: [PATCH] CLDC-510: Income range soft validations (#72)
* Init
* Add some tests
* Add failing spec
* Move soft validations into a module
* Update test
* Rubocop
* Scope are auto created by enums
* Rename folder to validations
* No instance variable
* Commit both lines
* Add error indication
* Make partial slightly more generic
* Fix back link
* Write failing test
* All specs currently passing. Can this be real?
* Check page should have an override question
* Fix back button for check answers pages
* Don't really need a wrapper method for the validations
* We're really validating a page here not a question
* Dup variable
* Bit of a nasty hack but maybe better than deriving back link?
* Set a no cache header instead of reloading
* Move a teeny bit of logic out of the controller
* Rubocop
* Extract method
---
app/controllers/case_logs_controller.rb | 27 +++----
app/models/case_log.rb | 22 +++---
app/models/form.rb | 9 +++
app/validations/soft_validations.rb | 45 ++++++++++++
.../_validation_override_question.html.erb | 11 +++
app/views/form/page.html.erb | 10 ++-
config/forms/2021_2022.json | 9 +++
.../20211028090049_add_net_income_override.rb | 5 ++
db/schema.rb | 1 +
spec/controllers/case_logs_controller_spec.rb | 14 ++--
spec/features/case_log_spec.rb | 72 +++++++++++++++++--
spec/fixtures/forms/test_form.json | 9 +++
12 files changed, 196 insertions(+), 38 deletions(-)
create mode 100644 app/validations/soft_validations.rb
create mode 100644 app/views/form/_validation_override_question.html.erb
create mode 100644 db/migrate/20211028090049_add_net_income_override.rb
diff --git a/app/controllers/case_logs_controller.rb b/app/controllers/case_logs_controller.rb
index 48fe5726d..f8bef385a 100644
--- a/app/controllers/case_logs_controller.rb
+++ b/app/controllers/case_logs_controller.rb
@@ -56,16 +56,14 @@ class CaseLogsController < ApplicationController
def submit_form
form = FormHandler.instance.get_form("2021_2022")
@case_log = CaseLog.find(params[:id])
- previous_page = params[:case_log][:previous_page]
- questions_for_page = form.questions_for_page(previous_page)
- responses_for_page = question_responses(questions_for_page)
- @case_log.previous_page = previous_page
- if @case_log.update(responses_for_page)
- redirect_path = get_next_page_path(form, previous_page, @case_log)
+ @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_to(send(redirect_path, @case_log))
else
- page_info = form.all_pages[previous_page]
- render "form/page", locals: { form: form, page_key: previous_page, page_info: page_info }, status: :unprocessable_entity
+ page_info = form.all_pages[@case_log.page]
+ render "form/page", locals: { form: form, page_key: @case_log.page, page_info: page_info }, status: :unprocessable_entity
end
end
@@ -101,9 +99,12 @@ private
API_ACTIONS = %w[create show update destroy].freeze
- def question_responses(questions_for_page)
- questions_for_page.each_with_object({}) do |(question_key, question_info), result|
+ def responses_for_page(page)
+ form = FormHandler.instance.get_form("2021_2022")
+ form.expected_responses_for_page(page).each_with_object({}) do |(question_key, question_info), result|
question_params = params["case_log"][question_key]
+ next unless question_params
+
if question_info["type"] == "checkbox"
question_info["answer_options"].keys.reject { |x| x.match(/divider/) }.each do |option|
result[option] = question_params.include?(option)
@@ -129,8 +130,8 @@ private
params.require(:case_log).permit(CaseLog.editable_fields)
end
- def get_next_page_path(form, previous_page, case_log = {})
- content = form.all_pages[previous_page]
+ 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|
@@ -139,6 +140,6 @@ private
end
end
end
- form.next_page_redirect_path(previous_page)
+ form.next_page_redirect_path(page)
end
end
diff --git a/app/models/case_log.rb b/app/models/case_log.rb
index e9afa7807..9c6660052 100644
--- a/app/models/case_log.rb
+++ b/app/models/case_log.rb
@@ -10,9 +10,9 @@ class CaseLogValidator < ActiveModel::Validator
# If we've come from the form UI we only want to validate the specific fields
# that have just been submitted. If we're submitting a log via API or Bulk Upload
# we want to validate all data fields.
- question_to_validate = options[:previous_page]
- if question_to_validate
- public_send("validate_#{question_to_validate}", record) if respond_to?("validate_#{question_to_validate}")
+ page_to_validate = options[:page]
+ if page_to_validate
+ public_send("validate_#{page_to_validate}", record) if respond_to?("validate_#{page_to_validate}")
else
validation_methods = public_methods.select { |method| method.starts_with?("validate_") }
validation_methods.each { |meth| public_send(meth, record) }
@@ -36,25 +36,19 @@ end
class CaseLog < ApplicationRecord
include Discard::Model
+ include SoftValidations
default_scope -> { kept }
- scope :not_started, -> { where(status: "not_started") }
- scope :in_progress, -> { where(status: "in_progress") }
scope :not_completed, -> { where.not(status: "completed") }
- scope :completed, -> { where(status: "completed") }
- validate :instance_validations
+ validates_with CaseLogValidator, ({ page: @page } || {})
before_save :update_status!
- attr_writer :previous_page
+ attr_accessor :page
enum status: { "not_started" => 0, "in_progress" => 1, "completed" => 2 }
AUTOGENERATED_FIELDS = %w[id status created_at updated_at discarded_at].freeze
- def instance_validations
- validates_with CaseLogValidator, ({ previous_page: @previous_page } || {})
- end
-
def self.editable_fields
attribute_names - AUTOGENERATED_FIELDS
end
@@ -125,6 +119,10 @@ private
dynamically_not_required << "fixed_term_tenancy"
end
+ unless net_income_in_soft_max_range? || net_income_in_soft_min_range?
+ dynamically_not_required << "override_net_income_validation"
+ end
+
unless tenancy_type == "Other"
dynamically_not_required << "other_tenancy_type"
end
diff --git a/app/models/form.rb b/app/models/form.rb
index fc11d2b36..ea803e5b6 100644
--- a/app/models/form.rb
+++ b/app/models/form.rb
@@ -41,6 +41,15 @@ class Form
pages_for_subsection(subsection).map { |title, _value| questions_for_page(title) }.reduce(:merge)
end
+ # Returns a hash with soft validation questions as keys
+ def soft_validations_for_page(page)
+ all_pages[page]["soft_validations"]
+ end
+
+ def expected_responses_for_page(page)
+ questions_for_page(page).merge(soft_validations_for_page(page) || {})
+ end
+
def first_page_for_subsection(subsection)
pages_for_subsection(subsection).keys.first
end
diff --git a/app/validations/soft_validations.rb b/app/validations/soft_validations.rb
new file mode 100644
index 000000000..b62490130
--- /dev/null
+++ b/app/validations/soft_validations.rb
@@ -0,0 +1,45 @@
+module SoftValidations
+ def has_no_unresolved_soft_errors?
+ soft_errors.empty? || soft_errors_overridden?
+ end
+
+ def soft_errors
+ {}.merge(net_income_validations)
+ end
+
+ def soft_errors_overridden?
+ public_send(soft_errors.keys.first) if soft_errors.present?
+ end
+
+private
+
+ def net_income_validations
+ net_income_errors = {}
+ if net_income_in_soft_min_range?
+ net_income_errors["override_net_income_validation"] = OpenStruct.new(
+ message: "Net income is lower than expected based on the main tenant's working situation. Are you sure this is correct?",
+ hint_text: "This is based on the tenant's work situation: #{person_1_economic_status}",
+ )
+ elsif net_income_in_soft_max_range?
+ net_income_errors["override_net_income_validation"] = OpenStruct.new(
+ message: "Net income is higher than expected based on the main tenant's working situation. Are you sure this is correct?",
+ hint_text: "This is based on the tenant's work situation: #{person_1_economic_status}",
+ )
+ else
+ update_column(:override_net_income_validation, nil)
+ end
+ net_income_errors
+ end
+
+ def net_income_in_soft_max_range?
+ return unless weekly_net_income && person_1_economic_status
+
+ weekly_net_income.between?(applicable_income_range.soft_max, applicable_income_range.hard_max)
+ end
+
+ def net_income_in_soft_min_range?
+ return unless weekly_net_income && person_1_economic_status
+
+ weekly_net_income.between?(applicable_income_range.soft_min, applicable_income_range.hard_min)
+ end
+end
diff --git a/app/views/form/_validation_override_question.html.erb b/app/views/form/_validation_override_question.html.erb
new file mode 100644
index 000000000..947791e75
--- /dev/null
+++ b/app/views/form/_validation_override_question.html.erb
@@ -0,0 +1,11 @@
+
+ <%= f.govuk_check_boxes_fieldset @case_log.soft_errors.keys.first,
+ legend: { text: @case_log.soft_errors.values.first.message, size: "l" },
+ hint: { text: @case_log.soft_errors.values.first.hint_text } do %>
+
+ <%= f.govuk_check_box @case_log.soft_errors.keys.first, @case_log.soft_errors.keys.first,
+ label: { text: "Yes" },
+ checked: f.object.send(@case_log.soft_errors.keys.first)
+ %>
+ <% end %>
+
diff --git a/app/views/form/page.html.erb b/app/views/form/page.html.erb
index 878fefd66..dca0b7672 100644
--- a/app/views/form/page.html.erb
+++ b/app/views/form/page.html.erb
@@ -1,5 +1,7 @@
+
+
<% content_for :before_content do %>
- <%= link_to 'Back', 'javascript:history.back()', class: "govuk-back-link" %>
+ <%= link_to 'Back', :back, class: "govuk-back-link" %>
<% end %>
<%= turbo_frame_tag "case_log_form", target: "_top" do %>
@@ -17,7 +19,11 @@
<% end %>
- <%= f.hidden_field :previous_page, value: page_key %>
+ <% if @case_log.soft_errors.present? && @case_log.soft_errors.keys.first == page_info["soft_validations"]&.keys&.first %>
+ <%= render partial: "form/validation_override_question", locals: { f: f } %>
+ <% end %>
+
+ <%= f.hidden_field :page, value: page_key %>
<%= f.govuk_submit "Save and continue" %>
<% end %>
<% end %>
diff --git a/config/forms/2021_2022.json b/config/forms/2021_2022.json
index 9f185b120..cb74d6edc 100644
--- a/config/forms/2021_2022.json
+++ b/config/forms/2021_2022.json
@@ -1492,6 +1492,15 @@
"2": "Yearly"
}
}
+ },
+ "soft_validations": {
+ "override_net_income_validation": {
+ "check_answer_label": "Net income confirmed?",
+ "type": "validation_override",
+ "answer_options": {
+ "override_net_income_validation": "Yes"
+ }
+ }
}
},
"net_income_uc_proportion": {
diff --git a/db/migrate/20211028090049_add_net_income_override.rb b/db/migrate/20211028090049_add_net_income_override.rb
new file mode 100644
index 000000000..f776ada87
--- /dev/null
+++ b/db/migrate/20211028090049_add_net_income_override.rb
@@ -0,0 +1,5 @@
+class AddNetIncomeOverride < ActiveRecord::Migration[6.1]
+ def change
+ add_column :case_logs, :override_net_income_validation, :boolean
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 5c4ffd1ea..7f75e5e87 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -133,6 +133,7 @@ ActiveRecord::Schema.define(version: 2021_10_28_095000) do
t.boolean "reasonable_preference_reason_do_not_know"
t.datetime "discarded_at"
t.string "other_tenancy_type"
+ t.boolean "override_net_income_validation"
t.string "net_income_known"
t.index ["discarded_at"], name: "index_case_logs_on_discarded_at"
end
diff --git a/spec/controllers/case_logs_controller_spec.rb b/spec/controllers/case_logs_controller_spec.rb
index 29b129e5e..3adcf8335 100644
--- a/spec/controllers/case_logs_controller_spec.rb
+++ b/spec/controllers/case_logs_controller_spec.rb
@@ -52,13 +52,13 @@ RSpec.describe CaseLogsController, type: :controller do
%w[ accessibility_requirements_fully_wheelchair_accessible_housing
accessibility_requirements_wheelchair_access_to_essential_rooms
accessibility_requirements_level_access_housing],
- previous_page: "accessibility_requirements" }
+ page: "accessibility_requirements" }
end
let(:new_case_log_form_params) do
{
accessibility_requirements: %w[accessibility_requirements_level_access_housing],
- previous_page: "accessibility_requirements",
+ page: "accessibility_requirements",
}
end
@@ -88,7 +88,7 @@ RSpec.describe CaseLogsController, type: :controller do
accessibility_requirements_wheelchair_access_to_essential_rooms
accessibility_requirements_level_access_housing],
tenant_code: tenant_code,
- previous_page: "accessibility_requirements" }
+ page: "accessibility_requirements" }
end
let(:questions_for_page) do
{ "accessibility_requirements" =>
@@ -124,17 +124,21 @@ RSpec.describe CaseLogsController, type: :controller do
end
context "conditional routing" do
+ before do
+ allow_any_instance_of(CaseLogValidator).to receive(:validate_household_pregnancy).and_return(true)
+ end
+
let(:case_log_form_conditional_question_yes_params) do
{
pregnancy: "Yes",
- previous_page: "conditional_question",
+ page: "conditional_question",
}
end
let(:case_log_form_conditional_question_no_params) do
{
pregnancy: "No",
- previous_page: "conditional_question",
+ page: "conditional_question",
}
end
diff --git a/spec/features/case_log_spec.rb b/spec/features/case_log_spec.rb
index 6d26628f2..f46a64a3e 100644
--- a/spec/features/case_log_spec.rb
+++ b/spec/features/case_log_spec.rb
@@ -32,10 +32,6 @@ RSpec.describe "Test Features" do
describe "Create new log" do
it "redirects to the task list for the new log" do
visit("/case_logs")
- # Ensure that we've finished creating both case logs before running the
- # Capybara click part to ensure we don't get creation race conditions
- expect(page).to have_link(nil, href: "/case_logs/#{case_log.id}")
- expect(page).to have_link(nil, href: "/case_logs/#{empty_case_log.id}")
click_link("Create new log")
id = CaseLog.order(created_at: :desc).first.id
expect(page).to have_content("Tasklist for log #{id}")
@@ -233,6 +229,15 @@ RSpec.describe "Test Features" do
expect(page).to have_current_path("/case_logs/#{id}/#{pages[index + 1]}")
end
end
+
+ context "when changing an answer from the check answers page" do
+ it "the back button routes correctly" do
+ visit("/case_logs/#{id}/household_characteristics/check_answers")
+ first("a", text: /Answer/).click
+ click_link("Back")
+ expect(page).to have_current_path("/case_logs/#{id}/household_characteristics/check_answers")
+ end
+ end
end
end
@@ -377,11 +382,66 @@ RSpec.describe "Test Features" do
end
end
+ describe "Soft Validation" do
+ context "given a weekly net income that is above the expected amount for the given economic status but below the hard max" do
+ let!(:case_log) { FactoryBot.create(:case_log, :in_progress, person_1_economic_status: "Full-time - 30 hours or more") }
+ let(:income_over_soft_limit) { 750 }
+ let(:income_under_soft_limit) { 700 }
+
+ it "prompts the user to confirm the value is correct" do
+ visit("/case_logs/#{case_log.id}/net_income")
+ fill_in("case-log-net-income-field", with: income_over_soft_limit)
+ choose("case-log-net-income-frequency-weekly-field", allow_label_click: true)
+ click_button("Save and continue")
+ expect(page).to have_content("Are you sure this is correct?")
+ check("case-log-override-net-income-validation-override-net-income-validation-field", allow_label_click: true)
+ click_button("Save and continue")
+ expect(page).to have_current_path("/case_logs/#{case_log.id}/net_income_uc_proportion")
+ end
+
+ it "does not require confirming the value if the value is amended" do
+ visit("/case_logs/#{case_log.id}/net_income")
+ fill_in("case-log-net-income-field", with: income_over_soft_limit)
+ choose("case-log-net-income-frequency-weekly-field", allow_label_click: true)
+ click_button("Save and continue")
+ fill_in("case-log-net-income-field", with: income_under_soft_limit)
+ click_button("Save and continue")
+ expect(page).to have_current_path("/case_logs/#{case_log.id}/net_income_uc_proportion")
+ case_log.reload
+ expect(case_log.override_net_income_validation).to be_nil
+ end
+
+ it "clears the confirmation question if the amount was amended and the page is returned to using the back button", js: true do
+ visit("/case_logs/#{case_log.id}/net_income")
+ fill_in("case-log-net-income-field", with: income_over_soft_limit)
+ choose("case-log-net-income-frequency-weekly-field", allow_label_click: true)
+ click_button("Save and continue")
+ fill_in("case-log-net-income-field", with: income_under_soft_limit)
+ click_button("Save and continue")
+ click_link(text: "Back")
+ expect(page).not_to have_content("Are you sure this is correct?")
+ end
+
+ it "does not clear the confirmation question if the page is returned to using the back button and the amount is still over the soft limit", js: true do
+ visit("/case_logs/#{case_log.id}/net_income")
+ fill_in("case-log-net-income-field", with: income_over_soft_limit)
+ choose("case-log-net-income-frequency-weekly-field", allow_label_click: true)
+ click_button("Save and continue")
+ check("case-log-override-net-income-validation-override-net-income-validation-field", allow_label_click: true)
+ click_button("Save and continue")
+ click_link(text: "Back")
+ expect(page).to have_content("Are you sure this is correct?")
+ end
+ end
+ end
+
describe "conditional page routing", js: true do
+ before do
+ allow_any_instance_of(CaseLogValidator).to receive(:validate_household_pregnancy).and_return(true)
+ end
+
it "can route the user to a different page based on their answer on the current page" do
visit("case_logs/#{id}/conditional_question")
- # using a question name that is already in the db to avoid
- # having to add a new column to the db for this test
choose("case-log-pregnancy-yes-field", allow_label_click: true)
click_button("Save and continue")
expect(page).to have_current_path("/case_logs/#{id}/conditional_question_yes_page")
diff --git a/spec/fixtures/forms/test_form.json b/spec/fixtures/forms/test_form.json
index 35590cb3e..2bcd34aaa 100644
--- a/spec/fixtures/forms/test_form.json
+++ b/spec/fixtures/forms/test_form.json
@@ -328,6 +328,15 @@
"2": "Yearly"
}
}
+ },
+ "soft_validations": {
+ "override_net_income_validation": {
+ "check_answer_label": "Net income confirmed?",
+ "type": "validation_override",
+ "answer_options": {
+ "override_net_income_validation": "Yes"
+ }
+ }
}
},
"net_income_uc_proportion": {