From 10ffc620bf68a28c638d9b3565eb602a62959a86 Mon Sep 17 00:00:00 2001
From: baarkerlounger <5101747+baarkerlounger@users.noreply.github.com>
Date: Thu, 13 Jan 2022 15:17:42 +0000
Subject: [PATCH] CLDC-896: Integer Validation (#214)

* Failing spec

* Check pre-type cast value

* Update spec error message

* Ensure age doesn't silently drop strings either
---
 .../validations/household_validations.rb      | 18 +++---
 .../validations/property_validations.rb       | 14 ++++-
 config/locales/en.yml                         |  3 +-
 spec/models/case_log_spec.rb                  | 30 ---------
 .../validations/property_validations_spec.rb  | 61 +++++++++++++++++++
 spec/requests/case_log_controller_spec.rb     |  2 +-
 6 files changed, 86 insertions(+), 42 deletions(-)
 create mode 100644 spec/models/validations/property_validations_spec.rb

diff --git a/app/models/validations/household_validations.rb b/app/models/validations/household_validations.rb
index 56982f65b..d361e15a1 100644
--- a/app/models/validations/household_validations.rb
+++ b/app/models/validations/household_validations.rb
@@ -63,11 +63,7 @@ module Validations::HouseholdValidations
   end
 
   def validate_person_1_age(record)
-    return unless record.age1
-
-    if !record.age1.is_a?(Integer) || record.age1 < 16 || record.age1 > 120
-      record.errors.add :age1, I18n.t("validations.household.age.over_16")
-    end
+    validate_person_age(record, 1, 16)
   end
 
   def validate_person_1_economic(record)
@@ -110,12 +106,18 @@ private
     end
   end
 
-  def validate_person_age(record, person_num)
+  def validate_person_age(record, person_num, lower_bound = 1)
     age = record.public_send("age#{person_num}")
     return unless age
 
-    if !age.is_a?(Integer) || age < 1 || age > 120
-      record.errors.add "age#{person_num}".to_sym, I18n.t("validations.household.age.must_be_valid")
+    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: 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: lower_bound)
     end
   end
 
diff --git a/app/models/validations/property_validations.rb b/app/models/validations/property_validations.rb
index 0668c066c..d91ea931f 100644
--- a/app/models/validations/property_validations.rb
+++ b/app/models/validations/property_validations.rb
@@ -6,7 +6,19 @@ module Validations::PropertyValidations
   POSTCODE_REGEXP = /^(([A-Z]{1,2}[0-9][A-Z0-9]?|ASCN|STHL|TDCU|BBND|[BFS]IQQ|PCRN|TKCA) ?[0-9][A-Z]{2}|BFPO ?[0-9]{1,4}|(KY[0-9]|MSR|VG|AI)[ -]?[0-9]{4}|[A-Z]{2} ?[0-9]{2}|GE ?CX|GIR ?0A{2}|SAN ?TA1)$/i
 
   def validate_property_number_of_times_relet(record)
-    if record.offered && !/^[1-9]$|^0[1-9]$|^1[0-9]$|^20$/.match?(record.offered.to_s)
+    return unless record.offered
+
+    # Since offered is an integer type ActiveRecord will automatically cast that for us
+    # but it's type casting is a little lax so "random" becomes 0. To make sure that doesn't pass
+    # validation and then get silently dropped we attempt strict type casting on the original value
+    # as part of our validation.
+    begin
+      Integer(record.offered_before_type_cast)
+    rescue ArgumentError
+      record.errors.add :offered, I18n.t("validations.property.offered.relet_number")
+    end
+
+    if record.offered.negative? || record.offered > 20
       record.errors.add :offered, I18n.t("validations.property.offered.relet_number")
     end
   end
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 9163aaefd..6047440be 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -86,8 +86,7 @@ en:
       preg_occ:
         no_female: "You must answer no as there are no female tenants aged 16-50 in the property"
       age:
-        over_16: "Tenant age must be an integer between 16 and 120"
-        must_be_valid: "Tenant age must be an integer between 0 and 120"
+        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/models/case_log_spec.rb b/spec/models/case_log_spec.rb
index 0afdd3a6e..1c77b47d6 100644
--- a/spec/models/case_log_spec.rb
+++ b/spec/models/case_log_spec.rb
@@ -72,36 +72,6 @@ RSpec.describe Form, type: :model do
       }.to raise_error(ActiveRecord::RecordInvalid)
     end
 
-    it "validates number of relets is a number" do
-      expect {
-        CaseLog.create!(
-          offered: "random",
-          owning_organisation: owning_organisation,
-          managing_organisation: managing_organisation,
-        )
-      }.to raise_error(ActiveRecord::RecordInvalid)
-    end
-
-    it "validates number of relets is under 20" do
-      expect {
-        CaseLog.create!(
-          offered: 21,
-          owning_organisation: owning_organisation,
-          managing_organisation: managing_organisation,
-        )
-      }.to raise_error(ActiveRecord::RecordInvalid)
-    end
-
-    it "validates number of relets is over 0" do
-      expect {
-        CaseLog.create!(
-          offered: 0,
-          owning_organisation: owning_organisation,
-          managing_organisation: managing_organisation,
-        )
-      }.to raise_error(ActiveRecord::RecordInvalid)
-    end
-
     context "reasonable preference is yes" do
       it "validates a reason must be selected" do
         expect {
diff --git a/spec/models/validations/property_validations_spec.rb b/spec/models/validations/property_validations_spec.rb
new file mode 100644
index 000000000..17cea86ac
--- /dev/null
+++ b/spec/models/validations/property_validations_spec.rb
@@ -0,0 +1,61 @@
+require "rails_helper"
+require_relative "../../request_helper"
+
+RSpec.describe CaseLog do
+  let(:owning_organisation) { FactoryBot.create(:organisation) }
+  let(:managing_organisation) { owning_organisation }
+
+  before do
+    RequestHelper.stub_http_requests
+  end
+
+  describe "#new" do
+    it "raises an error when offered is present and invalid" do
+      expect {
+        CaseLog.create!(
+          offered: "random",
+          owning_organisation: owning_organisation,
+          managing_organisation: managing_organisation,
+        )
+      }.to raise_error(ActiveRecord::RecordInvalid)
+    end
+  end
+end
+
+RSpec.describe Validations::PropertyValidations do
+  let(:subject) { subject_class.new }
+  let(:subject_class) { Class.new { include Validations::PropertyValidations } }
+  let(:record) { FactoryBot.create(:case_log) }
+  let(:expected_error) { "Number of times property has been offered for relet must be a number between 0 and 20" }
+
+  describe "#validate_property_number_of_times_relet" do
+    it "does not add an error if the record offered is missing" do
+      record.offered = nil
+      subject.validate_property_number_of_times_relet(record)
+      expect(record.errors).to be_empty
+    end
+
+    it "does not add an error if offered is valid (number between 0 and 20)" do
+      record.offered = 0
+      subject.validate_property_number_of_times_relet(record)
+      expect(record.errors).to be_empty
+      record.offered = 10
+      subject.validate_property_number_of_times_relet(record)
+      expect(record.errors).to be_empty
+      record.offered = 20
+      subject.validate_property_number_of_times_relet(record)
+      expect(record.errors).to be_empty
+    end
+
+    it "does add an error when offered is invalid" do
+      record.offered = "invalid"
+      subject.validate_property_number_of_times_relet(record)
+      expect(record.errors).to_not be_empty
+      expect(record.errors["offered"]).to include(match(expected_error))
+      record.offered = 21
+      subject.validate_property_number_of_times_relet(record)
+      expect(record.errors).to_not be_empty
+      expect(record.errors["offered"]).to include(match(expected_error))
+    end
+  end
+end
diff --git a/spec/requests/case_log_controller_spec.rb b/spec/requests/case_log_controller_spec.rb
index e27a57f41..67d7d647e 100644
--- a/spec/requests/case_log_controller_spec.rb
+++ b/spec/requests/case_log_controller_spec.rb
@@ -72,7 +72,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", ["Property number of times relet must be between 0 and 20"]], ["age1", ["Tenant age must be an integer between 16 and 120"]]])
+        expect(json_response["errors"]).to match_array([["offered", ["Number of times property has been offered for relet must be a number between 0 and 20"]], ["age1", ["Tenant age must be an integer between 16 and 120"]]])
       end
     end