From 2ae94b01d703d723e553b8f555097e69e40204d0 Mon Sep 17 00:00:00 2001
From: baarkerlounger <5101747+baarkerlounger@users.noreply.github.com>
Date: Wed, 9 Mar 2022 09:39:16 +0000
Subject: [PATCH] Generically validate all numeric question min and maxes
 (#362)

* Generically validate all numeric question min and maxes

* Update error message
---
 Gemfile.lock                                  | 16 ++---
 app/models/form.rb                            |  4 ++
 .../validations/household_validations.rb      | 20 -------
 app/models/validations/shared_validations.rb  | 21 +++++++
 config/locales/en.yml                         |  3 +-
 spec/fixtures/forms/2021_2022.json            | 10 ++--
 spec/models/case_log_spec.rb                  |  7 ++-
 .../validations/household_validations_spec.rb | 60 ++++++++++++-------
 spec/requests/case_logs_controller_spec.rb    |  4 +-
 9 files changed, 87 insertions(+), 58 deletions(-)

diff --git a/Gemfile.lock b/Gemfile.lock
index 1436927e9..fb5db6311 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -111,7 +111,7 @@ GEM
     ast (2.4.2)
     aws-eventstream (1.2.0)
     aws-partitions (1.563.0)
-    aws-sdk-core (3.128.0)
+    aws-sdk-core (3.128.1)
       aws-eventstream (~> 1, >= 1.0.2)
       aws-partitions (~> 1, >= 1.525.0)
       aws-sigv4 (~> 1.1)
@@ -127,7 +127,7 @@ GEM
       aws-eventstream (~> 1, >= 1.0.2)
     bcrypt (3.1.16)
     bindex (0.8.1)
-    bootsnap (1.10.3)
+    bootsnap (1.11.1)
       msgpack (~> 1.2)
     builder (3.2.4)
     bundler-audit (0.9.0.1)
@@ -203,7 +203,7 @@ GEM
       responders (>= 2, < 4)
     iniparse (1.5.0)
     io-wait (0.2.1)
-    jmespath (1.6.0)
+    jmespath (1.6.1)
     jquery-rails (4.4.0)
       rails-dom-testing (>= 1, < 3)
       railties (>= 4.2.0)
@@ -407,13 +407,13 @@ GEM
       rexml (~> 3.2, >= 3.2.5)
       rubyzip (>= 1.2.2)
     semantic_range (3.0.0)
-    sentry-rails (5.1.1)
+    sentry-rails (5.2.0)
       railties (>= 5.0)
-      sentry-ruby-core (~> 5.1.1)
-    sentry-ruby (5.1.1)
+      sentry-ruby-core (~> 5.2.0)
+    sentry-ruby (5.2.0)
       concurrent-ruby (~> 1.0, >= 1.0.2)
-      sentry-ruby-core (= 5.1.1)
-    sentry-ruby-core (5.1.1)
+      sentry-ruby-core (= 5.2.0)
+    sentry-ruby-core (5.2.0)
       concurrent-ruby
     simplecov (0.21.2)
       docile (~> 1.1)
diff --git a/app/models/form.rb b/app/models/form.rb
index b1b00082b..37e3627a3 100644
--- a/app/models/form.rb
+++ b/app/models/form.rb
@@ -132,6 +132,10 @@ class Form
     questions.select(&:read_only?)
   end
 
+  def numeric_questions
+    questions.select { |q| q.type == "numeric" }
+  end
+
   def is_last_question?(page, subsection, case_log)
     subsection_ids = subsections.map(&:id)
     subsection.id == subsection_ids[subsection_ids.length - 1] && next_page(page, case_log) == :check_answers
diff --git a/app/models/validations/household_validations.rb b/app/models/validations/household_validations.rb
index de9a41929..f1c8a3db9 100644
--- a/app/models/validations/household_validations.rb
+++ b/app/models/validations/household_validations.rb
@@ -38,7 +38,6 @@ module Validations::HouseholdValidations
 
   def validate_household_number_of_other_members(record)
     (2..8).each do |n|
-      validate_person_age(record, n)
       validate_person_age_matches_economic_status(record, n)
       validate_person_age_matches_relationship(record, n)
       validate_person_age_and_gender_match_economic_status(record, n)
@@ -47,10 +46,6 @@ module Validations::HouseholdValidations
     validate_partner_count(record)
   end
 
-  def validate_person_1_age(record)
-    validate_person_age(record, 1, 16)
-  end
-
   def validate_person_1_economic(record)
     validate_person_age_matches_economic_status(record, 1)
   end
@@ -104,21 +99,6 @@ private
     end
   end
 
-  def validate_person_age(record, person_num, lower_bound = 1)
-    age = record.public_send("age#{person_num}")
-    return unless age
-
-    begin
-      Integer(record.public_send("age#{person_num}_before_type_cast"))
-    rescue ArgumentError
-      record.errors.add "age#{person_num}".to_sym, I18n.t("validations.household.age.must_be_valid", lower_bound:)
-    end
-
-    if age < lower_bound || age > 120
-      record.errors.add "age#{person_num}".to_sym, I18n.t("validations.household.age.must_be_valid", lower_bound:)
-    end
-  end
-
   def validate_person_age_matches_economic_status(record, person_num)
     age = record.public_send("age#{person_num}")
     economic_status = record.public_send("ecstat#{person_num}")
diff --git a/app/models/validations/shared_validations.rb b/app/models/validations/shared_validations.rb
index 98085bfcf..29c588a06 100644
--- a/app/models/validations/shared_validations.rb
+++ b/app/models/validations/shared_validations.rb
@@ -12,4 +12,25 @@ module Validations::SharedValidations
       record.errors.add other_field.to_sym, I18n.t("validations.other_field_not_required", main_field_label:, other_field_label:)
     end
   end
+
+  def validate_numeric_min_max(record)
+    record.form.numeric_questions.each do |question|
+      next unless question.min || question.max
+      next unless record[question.id]
+
+      field = question.check_answer_label || question.id
+
+      begin
+        answer = Integer(record.public_send("#{question.id}_before_type_cast"))
+      rescue ArgumentError
+        record.errors.add question.id.to_sym, I18n.t("validations.numeric.valid", field:, min: question.min, max: question.max)
+      end
+
+      next unless answer
+
+      if (question.min && question.min > answer) || (question.max && question.max < answer)
+        record.errors.add question.id.to_sym, I18n.t("validations.numeric.valid", field:, min: question.min, max: question.max)
+      end
+    end
+  end
 end
diff --git a/config/locales/en.yml b/config/locales/en.yml
index f0b3b8020..d12912794 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -37,6 +37,8 @@ en:
   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"
+    numeric:
+      valid: "%{field} must be a number between %{min} and %{max}"
     date:
       invalid_date: "Please enter a valid date"
       outside_collection_window: "Date must be within the current collection windows"
@@ -101,7 +103,6 @@ en:
       preg_occ:
         no_female: "You must answer no as there are no female tenants aged 16-50 in the property"
       age:
-        must_be_valid: "Tenant age must be an integer between %{lower_bound} and 120"
         retired_male: "Male tenant who is retired must be 65 or over"
         retired_female: "Female tenant who is retired must be 60 or over"
       ecstat:
diff --git a/spec/fixtures/forms/2021_2022.json b/spec/fixtures/forms/2021_2022.json
index 5b3bbb7f9..f7a232724 100644
--- a/spec/fixtures/forms/2021_2022.json
+++ b/spec/fixtures/forms/2021_2022.json
@@ -34,8 +34,8 @@
                     "check_answer_label": "Lead tenant’s age",
                     "header": "What is the tenant’s age?",
                     "type": "numeric",
-                    "min": 0,
-                    "max": 150,
+                    "min": 16,
+                    "max": 120,
                     "step": 1,
                     "width": 2
                   }
@@ -72,7 +72,7 @@
                     "hint_text": "The maximum number of others is 1",
                     "type": "numeric",
                     "min": 0,
-                    "max": 1,
+                    "max": 7,
                     "step": 1,
                     "width": 2,
                     "conditional_for": {
@@ -99,8 +99,8 @@
                     "check_answer_label": "Person 2’s age",
                     "header": "Do you know person 2’s age?",
                     "type": "numeric",
-                    "min": 0,
-                    "max": 150,
+                    "min": 1,
+                    "max": 120,
                     "step": 1,
                     "width": 2
                   },
diff --git a/spec/models/case_log_spec.rb b/spec/models/case_log_spec.rb
index fbd6975c7..9e4038346 100644
--- a/spec/models/case_log_spec.rb
+++ b/spec/models/case_log_spec.rb
@@ -88,8 +88,7 @@ RSpec.describe CaseLog do
       expect(validator).to receive(:validate_startdate)
     end
 
-    it "validates ages" do
-      expect(validator).to receive(:validate_person_1_age)
+    it "validates other household member details" do
       expect(validator).to receive(:validate_household_number_of_other_members)
     end
 
@@ -127,6 +126,10 @@ RSpec.describe CaseLog do
       expect(validator).to receive(:validate_previous_housing_situation)
     end
 
+    it "validates the min and max of numeric questions" do
+      expect(validator).to receive(:validate_numeric_min_max)
+    end
+
     it "validates armed forces" do
       expect(validator).to receive(:validate_armed_forces)
     end
diff --git a/spec/models/validations/household_validations_spec.rb b/spec/models/validations/household_validations_spec.rb
index 19c524c8d..aab5a1dc5 100644
--- a/spec/models/validations/household_validations_spec.rb
+++ b/spec/models/validations/household_validations_spec.rb
@@ -9,55 +9,55 @@ RSpec.describe Validations::HouseholdValidations do
   describe "age validations" do
     it "validates that person 1's age is a number" do
       record.age1 = "random"
-      household_validator.validate_person_1_age(record)
+      household_validator.validate_numeric_min_max(record)
       expect(record.errors["age1"])
-        .to include(match I18n.t("validations.household.age.must_be_valid", lower_bound: 16))
+        .to include(match I18n.t("validations.numeric.valid", field: "Lead tenant’s age", min: 16, max: 120))
     end
 
     it "validates that other household member ages are a number" do
-      record.age3 = "random"
-      household_validator.validate_household_number_of_other_members(record)
-      expect(record.errors["age3"])
-        .to include(match I18n.t("validations.household.age.must_be_valid", lower_bound: 1))
+      record.age2 = "random"
+      household_validator.validate_numeric_min_max(record)
+      expect(record.errors["age2"])
+        .to include(match I18n.t("validations.numeric.valid", field: "Person 2’s age", min: 1, max: 120))
     end
 
     it "validates that person 1's age is greater than 16" do
       record.age1 = 15
-      household_validator.validate_person_1_age(record)
+      household_validator.validate_numeric_min_max(record)
       expect(record.errors["age1"])
-        .to include(match I18n.t("validations.household.age.must_be_valid", lower_bound: 16))
+        .to include(match I18n.t("validations.numeric.valid", field: "Lead tenant’s age", min: 16, max: 120))
     end
 
     it "validates that other household member ages are greater than 1" do
-      record.age4 = 0
-      household_validator.validate_household_number_of_other_members(record)
-      expect(record.errors["age4"])
-        .to include(match I18n.t("validations.household.age.must_be_valid", lower_bound: 1))
+      record.age2 = 0
+      household_validator.validate_numeric_min_max(record)
+      expect(record.errors["age2"])
+        .to include(match I18n.t("validations.numeric.valid", field: "Person 2’s age", min: 1, max: 120))
     end
 
     it "validates that person 1's age is less than 121" do
       record.age1 = 121
-      household_validator.validate_person_1_age(record)
+      household_validator.validate_numeric_min_max(record)
       expect(record.errors["age1"])
-        .to include(match I18n.t("validations.household.age.must_be_valid", lower_bound: 16))
+        .to include(match I18n.t("validations.numeric.valid", field: "Lead tenant’s age", min: 16, max: 120))
     end
 
     it "validates that other household member ages are greater than 121" do
-      record.age4 = 123
-      household_validator.validate_household_number_of_other_members(record)
-      expect(record.errors["age4"])
-        .to include(match I18n.t("validations.household.age.must_be_valid", lower_bound: 1))
+      record.age2 = 123
+      household_validator.validate_numeric_min_max(record)
+      expect(record.errors["age2"])
+        .to include(match I18n.t("validations.numeric.valid", field: "Person 2’s age", min: 1, max: 120))
     end
 
     it "validates that person 1's age is between 16 and 120" do
       record.age1 = 63
-      household_validator.validate_person_1_age(record)
+      household_validator.validate_numeric_min_max(record)
       expect(record.errors["age1"]).to be_empty
     end
 
     it "validates that other household member ages are between 1 and 120" do
       record.age6 = 45
-      household_validator.validate_household_number_of_other_members(record)
+      household_validator.validate_numeric_min_max(record)
       expect(record.errors["age6"]).to be_empty
     end
   end
@@ -415,6 +415,26 @@ RSpec.describe Validations::HouseholdValidations do
         household_validator.validate_household_number_of_other_members(record)
         expect(record.errors["ecstat2"]).to be_empty
       end
+
+      it "validates that the number of other household members cannot be less than 0" do
+        record.other_hhmemb = -1
+        household_validator.validate_numeric_min_max(record)
+        expect(record.errors["other_hhmemb"])
+          .to include(match I18n.t("validations.numeric.valid", field: "Number of Other Household Members", min: 0, max: 7))
+      end
+
+      it "validates that the number of other household members cannot be more than 7" do
+        record.other_hhmemb = 8
+        household_validator.validate_numeric_min_max(record)
+        expect(record.errors["other_hhmemb"])
+          .to include(match I18n.t("validations.numeric.valid", field: "Number of Other Household Members", min: 0, max: 7))
+      end
+
+      it "expects that the number of other household members is between the min and max" do
+        record.other_hhmemb = 5
+        household_validator.validate_numeric_min_max(record)
+        expect(record.errors["other_hhmemb"]).to be_empty
+      end
     end
 
     context "when the household contains a retired female" do
diff --git a/spec/requests/case_logs_controller_spec.rb b/spec/requests/case_logs_controller_spec.rb
index 147dbda81..4c396aaa7 100644
--- a/spec/requests/case_logs_controller_spec.rb
+++ b/spec/requests/case_logs_controller_spec.rb
@@ -71,7 +71,7 @@ RSpec.describe CaseLogsController, type: :request do
         it "validates case log parameters" do
           json_response = JSON.parse(response.body)
           expect(response).to have_http_status(:unprocessable_entity)
-          expect(json_response["errors"]).to match_array([["offered", [I18n.t("validations.property.offered.relet_number")]], ["age1", [I18n.t("validations.household.age.must_be_valid", lower_bound: 16)]]])
+          expect(json_response["errors"]).to match_array([["offered", [I18n.t("validations.property.offered.relet_number")]], ["age1", [I18n.t("validations.numeric.valid", field: "Lead tenant’s age", min: 16, max: 120)]]])
         end
       end
 
@@ -345,7 +345,7 @@ RSpec.describe CaseLogsController, type: :request do
 
       it "returns an error message" do
         json_response = JSON.parse(response.body)
-        expect(json_response["errors"]).to eq({ "age1" => ["Tenant age must be an integer between 16 and 120"] })
+        expect(json_response["errors"]).to eq({ "age1" => ["Lead tenant’s age must be a number between 16 and 120"] })
       end
     end