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

@@ -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

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

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_<page_name>' 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)
+  def validate_armed_forces(record)
-    if (record.armedforces == "No" || record.armedforces == "Prefer not to say") && record.reservist.present?
+    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

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

app/models/validations/tenancy_validations.rb

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

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"

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

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

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