From 22e566e7aeae4e61eccaae30c7ac50e80789b61e Mon Sep 17 00:00:00 2001 From: baarkerlounger <5101747+baarkerlounger@users.noreply.github.com> Date: Mon, 14 Feb 2022 13:52:12 +0000 Subject: [PATCH] Refactor Reason for leaving last settled home validations (#303) * Refactor other field validation * Refactor reason for leaving last settled home validations * Additional tests * Remove dupe code * Remove dupe validation * Armed forces injured validations * Armed forces * Major repairs date validations * Void date validations --- Gemfile | 5 +- app/models/case_log.rb | 14 -- .../validations/household_validations.rb | 14 +- app/models/validations/shared_validations.rb | 15 ++ app/models/validations/tenancy_validations.rb | 4 +- config/locales/en.yml | 2 + spec/models/case_log_spec.rb | 209 ++---------------- .../validations/date_validations_spec.rb | 122 ++++++++++ .../validations/household_validations_spec.rb | 140 ++++++++++++ 9 files changed, 306 insertions(+), 219 deletions(-) create mode 100644 app/models/validations/shared_validations.rb diff --git a/Gemfile b/Gemfile index 7aa7295b7..9eb64a26d 100644 --- a/Gemfile +++ b/Gemfile @@ -54,17 +54,18 @@ gem "sentry-rails" gem "sentry-ruby" group :development, :test do - # Call 'byebug' anywhere in the code to stop execution and get a debugger console + # Check gems for known vulnerabilities gem "bundler-audit" + # Call 'byebug' anywhere in the code to stop execution and get a debugger console gem "byebug", platforms: %i[mri mingw x64_mingw] gem "dotenv-rails" gem "pry-byebug" end group :development do - # Access an interactive console on exception pages or by calling 'console' anywhere in the code. gem "listen", "~> 3.3" gem "overcommit", ">= 0.37.0" + # Access an interactive console on exception pages or by calling 'console' anywhere in the code. gem "web-console", ">= 4.1.0" # Display performance information such as SQL time and flame graphs for each request in your browser. # Can be configured to work on production as well see: https://github.com/MiniProfiler/rack-mini-profiler/blob/master/README.md diff --git a/app/models/case_log.rb b/app/models/case_log.rb index 197e3b389..920bbd5e7 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -13,20 +13,6 @@ class CaseLogValidator < ActiveModel::Validator validation_methods = public_methods.select { |method| method.starts_with?("validate_") } validation_methods.each { |meth| public_send(meth, record) } end - -private - - def validate_other_field(record, main_field, other_field) - main_field_label = main_field.humanize(capitalize: false) - other_field_label = other_field.humanize(capitalize: false) - if record[main_field] == "Other" && record[other_field].blank? - record.errors.add other_field.to_sym, "If #{main_field_label} is other then #{other_field_label} must be provided" - end - - if record[main_field] != "Other" && record[other_field].present? - record.errors.add other_field.to_sym, "#{other_field_label} must not be provided if #{main_field_label} was not other" - end - end end class CaseLog < ApplicationRecord diff --git a/app/models/validations/household_validations.rb b/app/models/validations/household_validations.rb index 128a40823..6116cd6b2 100644 --- a/app/models/validations/household_validations.rb +++ b/app/models/validations/household_validations.rb @@ -1,4 +1,5 @@ module Validations::HouseholdValidations + include Validations::SharedValidations include Constants::CaseLog # Validations methods need to be called 'validate_' to run on model save @@ -14,23 +15,18 @@ module Validations::HouseholdValidations end end - def validate_other_reason_for_leaving_last_settled_home(record) - validate_other_field(record, "reason", "other_reason_for_leaving_last_settled_home") - end - def validate_reason_for_leaving_last_settled_home(record) if record.reason == "Don’t know" && record.underoccupation_benefitcap != "Don’t know" record.errors.add :underoccupation_benefitcap, I18n.t("validations.household.underoccupation_benefitcap.dont_know_required") + record.errors.add :reason, I18n.t("validations.household.underoccupation_benefitcap.dont_know_required") end + validate_other_field(record, :reason, :other_reason_for_leaving_last_settled_home) end - def validate_armed_forces_injured(record) - if (record.armedforces == "No" || record.armedforces == "Prefer not to say") && record.reservist.present? + def validate_armed_forces(record) + if (record.armedforces == "No" || record.armedforces == "Tenant prefers not to say") && record.reservist.present? record.errors.add :reservist, I18n.t("validations.household.reservist.injury_not_required") end - end - - def validate_armed_forces_active_response(record) if record.armedforces != "A current or former regular in the UK Armed Forces (excluding National Service)" && record.leftreg.present? record.errors.add :leftreg, I18n.t("validations.household.leftreg.question_not_required") end diff --git a/app/models/validations/shared_validations.rb b/app/models/validations/shared_validations.rb new file mode 100644 index 000000000..f9ea78788 --- /dev/null +++ b/app/models/validations/shared_validations.rb @@ -0,0 +1,15 @@ +module Validations::SharedValidations + def validate_other_field(record, main_field = nil, other_field = nil) + return unless main_field || other_field + + main_field_label = main_field.to_s.humanize(capitalize: false) + other_field_label = other_field.to_s.humanize(capitalize: false) + if record[main_field] == "Other" && record[other_field].blank? + record.errors.add other_field.to_sym, I18n.t("validations.other_field_missing", main_field_label:, other_field_label:) + end + + if record[main_field] != "Other" && record[other_field].present? + record.errors.add other_field.to_sym, I18n.t("validations.other_field_not_required", main_field_label:, other_field_label:) + end + end +end diff --git a/app/models/validations/tenancy_validations.rb b/app/models/validations/tenancy_validations.rb index 456eff451..f45bd0cd9 100644 --- a/app/models/validations/tenancy_validations.rb +++ b/app/models/validations/tenancy_validations.rb @@ -1,6 +1,8 @@ module Validations::TenancyValidations # Validations methods need to be called 'validate_' to run on model save # or 'validate_' to run on submit as well + include Validations::SharedValidations + def validate_fixed_term_tenancy(record) is_present = record.tenancylength.present? is_in_range = record.tenancylength.to_i.between?(2, 99) @@ -21,6 +23,6 @@ module Validations::TenancyValidations end def validate_other_tenancy_type(record) - validate_other_field(record, "tenancy", "tenancyother") + validate_other_field(record, :tenancy, :tenancyother) end end diff --git a/config/locales/en.yml b/config/locales/en.yml index 96f4f6458..05d7963b5 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -35,6 +35,8 @@ en: updated: "Organisation details updated" validations: + 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" date: invalid_date: "Please enter a valid date" outside_collection_window: "Date must be within the current collection windows" diff --git a/spec/models/case_log_spec.rb b/spec/models/case_log_spec.rb index 229904279..65adaf965 100644 --- a/spec/models/case_log_spec.rb +++ b/spec/models/case_log_spec.rb @@ -44,61 +44,6 @@ RSpec.describe CaseLog do end # TODO: replace these with validator specs and checks for method call here - context "with a reason for leaving last settled home validation" do - it "checks the reason for leaving must be don’t know if reason for leaving settled home (Q9a) is don’t know." do - expect { - described_class.create!(reason: "Don’t know", - underoccupation_benefitcap: "Yes - benefit cap", - owning_organisation:, - managing_organisation:) - }.to raise_error(ActiveRecord::RecordInvalid) - end - end - - context "with reason for leaving last settled home validation set to other" do - it "must be provided if main reason for leaving last settled home was given as other" do - expect { - described_class.create!(reason: "Other", - other_reason_for_leaving_last_settled_home: nil, - owning_organisation:, - managing_organisation:) - }.to raise_error(ActiveRecord::RecordInvalid) - end - - it "must not be provided if the main reason for leaving settled home is not other" do - expect { - described_class.create!(reason: "Repossession", - other_reason_for_leaving_last_settled_home: "the other reason provided", - owning_organisation:, - managing_organisation:) - }.to raise_error(ActiveRecord::RecordInvalid) - end - end - - context "with armed forces injured validation" do - it "must not be answered if tenant was not a regular or reserve in armed forces" do - expect { - described_class.create!(armedforces: "No", - reservist: "Yes", - owning_organisation:, - managing_organisation:) - }.to raise_error(ActiveRecord::RecordInvalid) - end - end - - context "when validating pregnancy questions" do - it "Can answer yes if valid second tenant" do - expect { - described_class.create!(preg_occ: "Yes", - sex1: "Male", age1: 99, - sex2: "Female", - age2: 20, - owning_organisation:, - managing_organisation:) - }.not_to raise_error - end - end - context "when validating property vacancy and let as" do it "cannot have a previously let as type, if it hasn't been let before" do expect { @@ -226,28 +171,6 @@ RSpec.describe CaseLog do end end - context "when validating armed forces is active" do - it "must not be answered if not ever served as a regular" do - expect { - described_class.create!(armedforces: "No", - leftreg: "Yes", - owning_organisation:, - managing_organisation:) - }.to raise_error(ActiveRecord::RecordInvalid) - end - - # Crossover over tests here as injured must be answered as well for no error - it "must be answered if ever served in the forces as a regular" do - expect { - described_class.create!(armedforces: "A current or former regular in the UK Armed Forces (excluding National Service)", - leftreg: "Yes", - reservist: "Yes", - owning_organisation:, - managing_organisation:) - }.not_to raise_error - end - end - context "when validating household members" do it "validate that persons aged under 16 must have relationship Child" do expect { @@ -431,122 +354,6 @@ RSpec.describe CaseLog do end end - context "when validating major repairs date" do - it "cannot be later than the tenancy start date" do - expect { - described_class.create!( - mrcdate: Date.new(2021, 10, 10), - startdate: Date.new(2021, 10, 9), - owning_organisation:, - managing_organisation:, - ) - }.to raise_error(ActiveRecord::RecordInvalid) - - expect { - described_class.create!( - mrcdate: Date.new(2021, 10, 9), - startdate: Date.new(2021, 10, 10), - owning_organisation:, - managing_organisation:, - ) - }.not_to raise_error - end - - it "must not be completed if reason for vacancy is first let" do - expect { - described_class.create!( - mrcdate: Date.new(2020, 10, 10), - rsnvac: "First let of new-build property", - owning_organisation:, - managing_organisation:, - ) - }.to raise_error(ActiveRecord::RecordInvalid) - - expect { - described_class.create!( - mrcdate: Date.new(2020, 10, 10), - rsnvac: "First let of conversion, rehabilitation or acquired property", - owning_organisation:, - managing_organisation:, - ) - }.to raise_error(ActiveRecord::RecordInvalid) - - expect { - described_class.create!( - mrcdate: Date.new(2020, 10, 10), - rsnvac: "First let of leased property", - owning_organisation:, - managing_organisation:, - ) - }.to raise_error(ActiveRecord::RecordInvalid) - end - - it "must have less than two years between the tenancy start date and major repairs date" do - expect { - described_class.create!( - startdate: Date.new(2021, 10, 10), - mrcdate: Date.new(2017, 10, 10), - owning_organisation:, - managing_organisation:, - ) - }.to raise_error(ActiveRecord::RecordInvalid) - end - end - - context "when saving void date" do - it "must have less than 10 years between the tenancy start date and void" do - expect { - described_class.create!( - startdate: Date.new(2021, 10, 10), - property_void_date: Date.new(2009, 10, 10), - owning_organisation:, - managing_organisation:, - ) - }.to raise_error(ActiveRecord::RecordInvalid) - - expect { - described_class.create!( - startdate: Date.new(2021, 10, 10), - property_void_date: Date.new(2015, 10, 10), - owning_organisation:, - managing_organisation:, - ) - }.not_to raise_error - end - - it "must be before the tenancy start date" do - expect { - described_class.create!( - startdate: Date.new(2021, 10, 10), - property_void_date: Date.new(2021, 10, 11), - owning_organisation:, - managing_organisation:, - ) - }.to raise_error(ActiveRecord::RecordInvalid) - - expect { - described_class.create!( - startdate: Date.new(2021, 10, 10), - property_void_date: Date.new(2019, 10, 10), - owning_organisation:, - managing_organisation:, - ) - }.not_to raise_error - end - - it "must be before major repairs date if major repairs date provided" do - expect { - described_class.create!( - startdate: Date.new(2021, 10, 10), - mrcdate: Date.new(2019, 10, 10), - property_void_date: Date.new(2019, 11, 11), - owning_organisation:, - managing_organisation:, - ) - }.to raise_error(ActiveRecord::RecordInvalid) - end - end - context "when validating local authority" do it "Has to be london if rent type london affordable rent" do expect { @@ -699,6 +506,22 @@ RSpec.describe CaseLog do it "validates reasonable preference" do expect(validator).to receive(:validate_reasonable_preference) end + + it "validates reason for leaving last settled home" do + expect(validator).to receive(:validate_reason_for_leaving_last_settled_home) + end + + it "validates armed forces" do + expect(validator).to receive(:validate_armed_forces) + end + + it "validates property major repairs date" do + expect(validator).to receive(:validate_property_major_repairs) + end + + it "validates property void date" do + expect(validator).to receive(:validate_property_void_date) + end end describe "status" do diff --git a/spec/models/validations/date_validations_spec.rb b/spec/models/validations/date_validations_spec.rb index 6be9e6cf9..7c9477a40 100644 --- a/spec/models/validations/date_validations_spec.rb +++ b/spec/models/validations/date_validations_spec.rb @@ -24,5 +24,127 @@ RSpec.describe Validations::DateValidations do date_validator.validate_startdate(record) expect(record.errors["startdate"]).to include(match I18n.t("validations.date.invalid_date")) end + + it "does not raise an error when valid" do + record.startdate = Time.zone.local(2022, 1, 1) + date_validator.validate_startdate(record) + expect(record.errors["startdate"]).to be_empty + end + end + + describe "major repairs date" do + it "cannot be after the tenancy start date" do + record.startdate = Time.zone.local(2022, 1, 1) + record.mrcdate = Time.zone.local(2022, 2, 1) + date_validator.validate_property_major_repairs(record) + expect(record.errors["mrcdate"]) + .to include(match I18n.t("validations.property.mrcdate.before_tenancy_start")) + end + + it "must be before the tenancy start date" do + record.startdate = Time.zone.local(2022, 2, 1) + record.mrcdate = Time.zone.local(2022, 1, 1) + date_validator.validate_property_major_repairs(record) + expect(record.errors["mrcdate"]).to be_empty + end + + it "cannot be more than 2 years before the tenancy start date" do + record.startdate = Time.zone.local(2022, 2, 1) + record.mrcdate = Time.zone.local(2020, 1, 1) + date_validator.validate_property_major_repairs(record) + expect(record.errors["mrcdate"]) + .to include(match I18n.t("validations.property.mrcdate.730_days_before_tenancy_start")) + end + + it "must be within 2 years of the tenancy start date" do + record.startdate = Time.zone.local(2022, 2, 1) + record.mrcdate = Time.zone.local(2020, 3, 1) + date_validator.validate_property_major_repairs(record) + expect(record.errors["mrcdate"]).to be_empty + end + + context "when reason for vacancy is first let of property" do + it "validates that no major repair date is provided for a new build" do + record.rsnvac = "First let of new-build property" + record.mrcdate = Time.zone.local(2022, 1, 1) + date_validator.validate_property_major_repairs(record) + expect(record.errors["mrcdate"]) + .to include(match I18n.t("validations.property.mrcdate.not_first_let")) + end + + it "validates that no major repair date is provided for a conversion" do + record.rsnvac = "First let of conversion, rehabilitation or acquired property" + record.mrcdate = Time.zone.local(2022, 1, 1) + date_validator.validate_property_major_repairs(record) + expect(record.errors["mrcdate"]) + .to include(match I18n.t("validations.property.mrcdate.not_first_let")) + end + + it "validates that no major repair date is provided for a leased property" do + record.rsnvac = "First let of leased property" + record.mrcdate = Time.zone.local(2022, 1, 1) + date_validator.validate_property_major_repairs(record) + expect(record.errors["mrcdate"]) + .to include(match I18n.t("validations.property.mrcdate.not_first_let")) + end + end + + context "when the reason for vacancy is not the first let of property" do + it "expects that major repairs can have been done" do + record.rsnvac = "Tenant moved to care home" + record.mrcdate = Time.zone.local(2022, 1, 1) + date_validator.validate_property_major_repairs(record) + expect(record.errors["mrcdate"]).to be_empty + end + end + end + + describe "property void date" do + it "cannot be after the tenancy start date" do + record.startdate = Time.zone.local(2022, 1, 1) + record.property_void_date = Time.zone.local(2022, 2, 1) + date_validator.validate_property_void_date(record) + expect(record.errors["property_void_date"]) + .to include(match I18n.t("validations.property.void_date.before_tenancy_start")) + end + + it "must be before the tenancy start date" do + record.startdate = Time.zone.local(2022, 2, 1) + record.property_void_date = Time.zone.local(2022, 1, 1) + date_validator.validate_property_void_date(record) + expect(record.errors["property_void_date"]).to be_empty + end + + it "cannot be more than 10 years before the tenancy start date" do + record.startdate = Time.zone.local(2022, 2, 1) + record.property_void_date = Time.zone.local(2012, 1, 1) + date_validator.validate_property_void_date(record) + expect(record.errors["property_void_date"]) + .to include(match I18n.t("validations.property.void_date.ten_years_before_tenancy_start")) + end + + it "must be within 10 years of the tenancy start date" do + record.startdate = Time.zone.local(2022, 2, 1) + record.property_void_date = Time.zone.local(2012, 3, 1) + date_validator.validate_property_void_date(record) + expect(record.errors["property_void_date"]).to be_empty + end + + context "when major repairs have been carried out" do + it "cannot be after major repairs date" do + record.mrcdate = Time.zone.local(2022, 1, 1) + record.property_void_date = Time.zone.local(2022, 2, 1) + date_validator.validate_property_void_date(record) + expect(record.errors["property_void_date"]) + .to include(match I18n.t("validations.property.void_date.after_mrcdate")) + end + + it "must be before major repairs date" do + record.mrcdate = Time.zone.local(2022, 2, 1) + record.property_void_date = Time.zone.local(2022, 1, 1) + date_validator.validate_property_void_date(record) + expect(record.errors["property_void_date"]).to be_empty + end + end end end diff --git a/spec/models/validations/household_validations_spec.rb b/spec/models/validations/household_validations_spec.rb index b4e582b5e..2b620b35e 100644 --- a/spec/models/validations/household_validations_spec.rb +++ b/spec/models/validations/household_validations_spec.rb @@ -165,4 +165,144 @@ RSpec.describe Validations::HouseholdValidations do end end end + + describe "reason for leaving last settled home validations" do + let(:field) { "validations.other_field_not_required" } + let(:main_field_label) { "reason" } + let(:other_field_label) { "other reason for leaving last settled home" } + let(:expected_error) { I18n.t(field, main_field_label:, other_field_label:) } + + context "when reason is other" do + let(:field) { "validations.other_field_missing" } + + it "validates that a reason is provided" do + record.reason = "Other" + record.other_reason_for_leaving_last_settled_home = nil + household_validator.validate_reason_for_leaving_last_settled_home(record) + expect(record.errors["other_reason_for_leaving_last_settled_home"]) + .to include(match(expected_error)) + end + + it "expects that a reason is provided" do + record.reason = "Other" + record.other_reason_for_leaving_last_settled_home = "Some unusual reason" + household_validator.validate_reason_for_leaving_last_settled_home(record) + expect(record.errors["other_reason_for_leaving_last_settled_home"]).to be_empty + end + end + + context "when reason is not other" do + it "validates that other reason is not provided" do + record.reason = "Repossession" + record.other_reason_for_leaving_last_settled_home = "Some other reason" + household_validator.validate_reason_for_leaving_last_settled_home(record) + expect(record.errors["other_reason_for_leaving_last_settled_home"]) + .to include(match(expected_error)) + end + + it "expects that other reason is not provided" do + record.reason = "Repossession" + record.other_reason_for_leaving_last_settled_home = nil + household_validator.validate_reason_for_leaving_last_settled_home(record) + expect(record.errors["other_reason_for_leaving_last_settled_home"]).to be_empty + end + end + + context "when reason is don't know" do + let(:expected_error) { I18n.t("validations.household.underoccupation_benefitcap.dont_know_required") } + + it "validates that under occupation benefit cap is also not known" do + record.reason = "Don’t know" + record.underoccupation_benefitcap = "Yes - benefit cap" + household_validator.validate_reason_for_leaving_last_settled_home(record) + expect(record.errors["underoccupation_benefitcap"]) + .to include(match(expected_error)) + expect(record.errors["reason"]) + .to include(match(expected_error)) + end + + it "expects that under occupation benefit cap is also not known" do + record.reason = "Don’t know" + record.underoccupation_benefitcap = "Don’t know" + household_validator.validate_reason_for_leaving_last_settled_home(record) + expect(record.errors["underoccupation_benefitcap"]).to be_empty + expect(record.errors["reason"]).to be_empty + end + end + end + + describe "armed forces validations" do + context "when the tenant or partner was and is not a member of the armed forces" do + it "validates that injured in the armed forces is not yes" do + record.armedforces = "No" + record.reservist = "Yes" + household_validator.validate_armed_forces(record) + expect(record.errors["reservist"]) + .to include(match I18n.t("validations.household.reservist.injury_not_required")) + end + end + + context "when the tenant prefers not to say if they were or are in the armed forces" do + it "validates that injured in the armed forces is not yes" do + record.armedforces = "Tenant prefers not to say" + record.reservist = "Yes" + household_validator.validate_armed_forces(record) + expect(record.errors["reservist"]) + .to include(match I18n.t("validations.household.reservist.injury_not_required")) + end + end + + context "when the tenant was or is a regular member of the armed forces" do + it "expects that injured in the armed forces can be yes" do + record.armedforces = "A current or former regular in the UK Armed Forces (excluding National Service)" + record.reservist = "Yes" + household_validator.validate_armed_forces(record) + expect(record.errors["reservist"]).to be_empty + end + end + + context "when the tenant was or is a reserve member of the armed forces" do + it "expects that injured in the armed forces can be yes" do + record.armedforces = "A current or former reserve in the UK Armed Forces (excluding National Service)" + record.reservist = "Yes" + household_validator.validate_armed_forces(record) + expect(record.errors["reservist"]).to be_empty + end + end + + context "when the tenant's partner was or is a member of the armed forces" do + it "expects that injured in the armed forces can be yes" do + record.armedforces = "A spouse / civil partner of a UK Armed Forces member who has separated or been bereaved within the last 2 years" + record.reservist = "Yes" + household_validator.validate_armed_forces(record) + expect(record.errors["reservist"]).to be_empty + end + end + + context "when the tenant or partner has left the armed forces" do + it "validates that they served in the armed forces" do + record.armedforces = "No" + record.leftreg = "Yes" + household_validator.validate_armed_forces(record) + expect(record.errors["leftreg"]) + .to include(match I18n.t("validations.household.leftreg.question_not_required")) + end + + it "expects that they served in the armed forces" do + record.armedforces = "A current or former regular in the UK Armed Forces (excluding National Service)" + record.leftreg = "Yes" + household_validator.validate_armed_forces(record) + expect(record.errors["leftreg"]).to be_empty + end + + it "expects that they served in the armed forces and may have been injured" do + record.armedforces = "A current or former regular in the UK Armed Forces (excluding National Service)" + record.leftreg = "Yes" + record.reservist = "Yes" + household_validator.validate_armed_forces(record) + expect(record.errors["leftreg"]).to be_empty + expect(record.errors["reservist"]).to be_empty + end + end + end end