combine duplicate methods after rebase, ensure hard validations are triggered on all relevant fields

app/models/sales_log.rb

@@ -116,6 +116,10 @@ class SalesLog < Log
     la && LONDON_BOROUGHS.include?(la)
   end
 
+  def property_not_in_london?
+    !london_property?
+  end
+
   def income1_used_for_mortgage?
     inc1mort == 1
   end

app/models/validations/sales/financial_validations.rb

@@ -1,33 +1,39 @@
 module Validations::Sales::FinancialValidations
-  include Validations::Sales::SharedValidations
   # Validations methods need to be called 'validate_<page_name>' to run on model save
   # or 'validate_' to run on submit as well
 
   def validate_income1(record)
-    if record.ecstat1 && record.income1 && record.la && record.ownershipsch == 1
+    return unless record.ecstat1 && record.income1 && record.la && record.ownershipsch == 1
-      if record.london_property?
+
-        record.errors.add :income1, I18n.t("validations.financial.income1.over_hard_max", hard_max: 90_000) if record.income1 > 90_000
+    relevant_fields = %i[income1 ecstat1 ownershipsch la postcode_full]
-        record.errors.add :ecstat1, I18n.t("validations.financial.income1.over_hard_max", hard_max: 90_000) if record.income1 > 90_000
+    if record.london_property? && record.income1 > 90_000
-        record.errors.add :ownershipsch, I18n.t("validations.financial.income1.over_hard_max", hard_max: 90_000) if record.income1 > 90_000
+      relevant_fields.each { |field| record.errors.add field, I18n.t("validations.financial.income.over_hard_max", hard_max: 90_000) }
-        record.errors.add :la, I18n.t("validations.financial.income1.over_hard_max", hard_max: 90_000) if record.income1 > 90_000
+    elsif record.property_not_in_london? && record.income1 > 80_000
-        record.errors.add :postcode_full, I18n.t("validations.financial.income1.over_hard_max", hard_max: 90_000) if record.income1 > 90_000
+      relevant_fields.each { |field| record.errors.add field, I18n.t("validations.financial.income.over_hard_max", hard_max: 80_000) }
-      elsif record.income1 > 80_000
+    end
-        record.errors.add :income1, I18n.t("validations.financial.income1.over_hard_max", hard_max: 80_000)
+  end
-        record.errors.add :ecstat1, I18n.t("validations.financial.income1.over_hard_max", hard_max: 80_000)
+
-        record.errors.add :ownershipsch, I18n.t("validations.financial.income1.over_hard_max", hard_max: 80_000)
+  def validate_income2(record)
-        record.errors.add :la, I18n.t("validations.financial.income1.over_hard_max", hard_max: 80_000) if record.income1 > 80_000
+    return unless record.ecstat2 && record.income2 && record.la && record.ownershipsch == 1
-        record.errors.add :postcode_full, I18n.t("validations.financial.income1.over_hard_max", hard_max: 80_000) if record.income1 > 80_000
+
-      end
+    relevant_fields = %i[income2 ecstat2 ownershipsch la postcode_full]
+    if record.london_property? && record.income2 > 90_000
+      relevant_fields.each { |field| record.errors.add field, I18n.t("validations.financial.income.over_hard_max", hard_max: 90_000) }
+    elsif record.property_not_in_london? && record.income2 > 80_000
+      relevant_fields.each { |field| record.errors.add field, I18n.t("validations.financial.income.over_hard_max", hard_max: 80_000) }
     end
+  end
 
-    if record.income1 && record.income2
+  def validate_combined_income(record)
-      if record.london_property?
+    return unless record.ecstat1 && record.income1 && record.ecstat2 && record.income2 && record.la && record.ownershipsch == 1
-        record.errors.add :income1, I18n.t("validations.financial.income.combined_over_hard_max", hard_max: 90_000) if record.income1 + record.income2 > 90_000
+
-      elsif record.income1 + record.income2 > 80_000
+    combined_income = record.income1 + record.income2
-        record.errors.add :income1, I18n.t("validations.financial.income.combined_over_hard_max", hard_max: 80_000)
+    relevant_fields = %i[income1 ecstat1 income2 ecstat2 ownershipsch la postcode_full]
-      end
+    if record.london_property? && combined_income > 90_000
+      relevant_fields.each { |field| record.errors.add field, I18n.t("validations.financial.income.combined_over_hard_max", hard_max: 90_000) }
+    elsif record.property_not_in_london? && combined_income > 80_000
+      relevant_fields.each { |field| record.errors.add field, I18n.t("validations.financial.income.combined_over_hard_max", hard_max: 80_000) }
     end
-    validate_combined_income(record)
   end
 
   def validate_cash_discount(record)
@@ -53,31 +59,5 @@ module Validations::Sales::FinancialValidations
       record.errors.add :stairowned, I18n.t("validations.financial.staircasing.older_person_percentage_owned_maximum_75")
       record.errors.add :type, I18n.t("validations.financial.staircasing.older_person_percentage_owned_maximum_75")
     end
-  def validate_income2(record)
-    if record.ecstat2 && record.income2 && record.ownershipsch == 1
-      if record.london_property?
-        record.errors.add :income2, I18n.t("validations.financial.income.over_hard_max", hard_max: 90_000) if record.income2 > 90_000
-      elsif record.income2 > 80_000
-        record.errors.add :income2, I18n.t("validations.financial.income.over_hard_max", hard_max: 80_000)
-      end
-    end
-
-    validate_combined_income(record)
-
-    child_income_validation(record, :income2)
-  end
-
-  def validate_combined_income(record)
-    if record.income1 && record.income2
-      if record.london_property?
-        if record.income1 + record.income2 > 90_000
-          record.errors.add :income1, I18n.t("validations.financial.income.combined_over_hard_max", hard_max: 90_000)
-          record.errors.add :income2, I18n.t("validations.financial.income.combined_over_hard_max", hard_max: 90_000)
-        end
-      elsif record.income1 + record.income2 > 80_000
-        record.errors.add :income1, I18n.t("validations.financial.income.combined_over_hard_max", hard_max: 80_000)
-        record.errors.add :income2, I18n.t("validations.financial.income.combined_over_hard_max", hard_max: 80_000)
-      end
-    end
   end
 end

app/models/validations/sales/household_validations.rb

@@ -1,6 +1,5 @@
 module Validations::Sales::HouseholdValidations
   include Validations::SharedValidations
-  include Validations::Sales::SharedValidations
 
   def validate_household_number_of_other_members(record)
     (2..6).each do |n|
@@ -20,10 +19,6 @@ module Validations::Sales::HouseholdValidations
     end
   end
 
-  def validate_relat2(record)
-    child_income_validation(record, :relat2)
-  end
-
 private
 
   def validate_person_age_matches_relationship(record, person_num)

app/models/validations/sales/shared_validations.rb

@@ -1,7 +0,0 @@
-module Validations::Sales::SharedValidations
-  def child_income_validation(record, field)
-    if record.relat2 && record.income2 && (record.relat2 == "C" && record.income2.positive?)
-      record.errors.add field, I18n.t("validations.financial.income.child_has_income")
-    end
-  end
-end

app/models/validations/sales/soft_validations.rb

@@ -13,16 +13,16 @@ module Validations::Sales::SoftValidations
     income1 < ALLOWED_INCOME_RANGES[ecstat1][:soft_min]
   end
 
-  def staircase_bought_above_fifty?
-    stairbought && stairbought > 50
-  end
-
   def income2_under_soft_min?
     return false unless ecstat2 && income2 && ALLOWED_INCOME_RANGES[ecstat2]
 
     income2 < ALLOWED_INCOME_RANGES[ecstat2][:soft_min]
   end
 
+  def staircase_bought_above_fifty?
+    stairbought && stairbought > 50
+  end  
+
   def mortgage_over_soft_max?
     return false unless mortgage && inc1mort && (inc2mort || not_joint_purchase?)
     return false if income1_used_for_mortgage? && income1.blank? || income2_used_for_mortgage? && income2.blank?

app/models/validations/shared_validations.rb

@@ -108,10 +108,16 @@ module Validations::SharedValidations
     end
   end
 
-  def child_income_validation(record, field)
+  def validate_child_income(record)
-    if record.relat2 && record.income2
+    return unless record.income2 && (record.relat2 || record.ecstat2)
-      if record.relat2 == "C" && record.income2 > 0
+
-        record.errors.add field, I18n.t("validations.financial.income.child_has_income")
+    if record.income2.positive?
+      if is_relationship_child? record.relat2
+        record.errors.add :relat2, I18n.t("validations.financial.income.child_has_income")
+        record.errors.add :income2, I18n.t("validations.financial.income.child_has_income")
+      elsif is_economic_status_child? record.ecstat2
+        record.errors.add :ecstat2, I18n.t("validations.financial.income.child_has_income")
+        record.errors.add :income2, I18n.t("validations.financial.income.child_has_income")
       end
     end
   end
@@ -133,4 +139,12 @@ private
       record.errors.add question.id.to_sym, I18n.t("validations.numeric.above_min", field:, min:)
     end
   end
+
+  def is_relationship_child?(relationship)
+    relationship == "C"
+  end
+
+  def is_economic_status_child?(economic_status)
+    economic_status == 9
+  end
 end

db/schema.rb

@@ -520,12 +520,12 @@ ActiveRecord::Schema[7.0].define(version: 2023_02_03_174815) do
     t.integer "value_value_check"
     t.integer "old_persons_shared_ownership_value_check"
     t.integer "staircase_bought_value_check"
+    t.integer "income2_value_check"
     t.integer "monthly_charges_value_check"
     t.integer "saledate_check"
     t.integer "details_known_5"
     t.integer "details_known_6"
     t.index ["bulk_upload_id"], name: "index_sales_logs_on_bulk_upload_id"
-    t.integer "income2_value_check"
     t.index ["created_by_id"], name: "index_sales_logs_on_created_by_id"
     t.index ["owning_organisation_id"], name: "index_sales_logs_on_owning_organisation_id"
     t.index ["updated_by_id"], name: "index_sales_logs_on_updated_by_id"

spec/models/validations/sales/shared_validations_spec.rb

@@ -1,18 +0,0 @@
-require "rails_helper"
-
-RSpec.describe Validations::Sales::SharedValidations do
-  subject(:household_validator) { validator_class.new }
-
-  let(:validator_class) { Class.new { include Validations::Sales::HouseholdValidations } }
-  let(:record) { FactoryBot.create(:sales_log) }
-
-  describe "child income validation" do
-    it "adds an error when a child has an income greater than 0" do
-      record.relat2 = "C"
-      record.income2 = 100
-      household_validator.validate_relat2(record)
-      expect(record.errors["relat2"])
-        .to include(match I18n.t("validations.financial.income.child_has_income"))
-    end
-  end
-end