From 2e881d855ece0afd5c2d2121a9b566c95abc6fdc Mon Sep 17 00:00:00 2001
From: Arthur Campbell <arfa.camble@gmail.com>
Date: Thu, 6 Apr 2023 17:18:21 +0100
Subject: [PATCH] changes after PO review two step values had been missed in
 previous work in various places there were custom validations applied that
 duplicated the functionality of the generic min max validation that is
 applied to all numeric questions, in these situations sometimes the min and
 max on the question class was inconsistent with the limit that triggered
 validations these have been corrected and made consistent various tests were
 affected by this and have been amended accordingly

---
 app/models/form/lettings/questions/beds.rb    |  4 +-
 app/models/form/lettings/questions/offered.rb |  2 +-
 .../lettings/questions/offered_social_let.rb  |  2 +-
 .../form/sales/questions/mortgage_length.rb   |  1 +
 app/models/form/sales/questions/person_age.rb |  1 +
 .../validations/property_validations.rb       | 26 ----------
 .../sales/sale_information_validations.rb     |  2 +-
 .../imports/lettings_logs_import_service.rb   |  4 +-
 config/forms/2021_2022.json                   |  8 +--
 config/forms/2022_2023.json                   |  8 +--
 config/locales/en.yml                         |  4 --
 spec/fixtures/forms/2021_2022.json            | 14 +++---
 .../questions/offered_social_let_spec.rb      |  8 +--
 spec/models/lettings_log_spec.rb              |  8 +--
 .../validations/property_validations_spec.rb  | 49 -------------------
 .../requests/lettings_logs_controller_spec.rb |  2 +-
 .../lettings_logs_import_service_spec.rb      |  4 +-
 17 files changed, 34 insertions(+), 113 deletions(-)

diff --git a/app/models/form/lettings/questions/beds.rb b/app/models/form/lettings/questions/beds.rb
index 75d2a835d..6fa6c7c2b 100644
--- a/app/models/form/lettings/questions/beds.rb
+++ b/app/models/form/lettings/questions/beds.rb
@@ -7,8 +7,8 @@ class Form::Lettings::Questions::Beds < ::Form::Question
     @type = "numeric"
     @width = 2
     @check_answers_card_number = 0
-    @max = 150
-    @min = 0
+    @max = 12
+    @min = 1
     @hint_text = "If shared accommodation, enter the number of bedrooms occupied by this household. A bedsit has 1 bedroom."
     @step = 1
     @question_number = 22
diff --git a/app/models/form/lettings/questions/offered.rb b/app/models/form/lettings/questions/offered.rb
index e41def301..2c9828d31 100644
--- a/app/models/form/lettings/questions/offered.rb
+++ b/app/models/form/lettings/questions/offered.rb
@@ -7,7 +7,7 @@ class Form::Lettings::Questions::Offered < ::Form::Question
     @type = "numeric"
     @width = 2
     @check_answers_card_number = 0
-    @max = 150
+    @max = 20
     @min = 0
     @hint_text = I18n.t("hints.offered")
     @step = 1
diff --git a/app/models/form/lettings/questions/offered_social_let.rb b/app/models/form/lettings/questions/offered_social_let.rb
index d3ac29516..dd57acbfe 100644
--- a/app/models/form/lettings/questions/offered_social_let.rb
+++ b/app/models/form/lettings/questions/offered_social_let.rb
@@ -7,7 +7,7 @@ class Form::Lettings::Questions::OfferedSocialLet < ::Form::Question
     @type = "numeric"
     @width = 2
     @check_answers_card_number = 0
-    @max = 150
+    @max = 20
     @min = 0
     @hint_text = I18n.t("hints.offered")
     @step = 1
diff --git a/app/models/form/sales/questions/mortgage_length.rb b/app/models/form/sales/questions/mortgage_length.rb
index adaa94d01..218ea03a1 100644
--- a/app/models/form/sales/questions/mortgage_length.rb
+++ b/app/models/form/sales/questions/mortgage_length.rb
@@ -7,6 +7,7 @@ class Form::Sales::Questions::MortgageLength < ::Form::Question
     @type = "numeric"
     @min = 0
     @max = 60
+    @step = 1
     @width = 5
     @suffix = " years"
     @hint_text = "You should round up to the nearest year. Value should not exceed 60 years."
diff --git a/app/models/form/sales/questions/person_age.rb b/app/models/form/sales/questions/person_age.rb
index 779e09669..cdbc9f80b 100644
--- a/app/models/form/sales/questions/person_age.rb
+++ b/app/models/form/sales/questions/person_age.rb
@@ -12,6 +12,7 @@ class Form::Sales::Questions::PersonAge < ::Form::Question
     @check_answers_card_number = person_index
     @min = 0
     @max = 110
+    @step = 1
     @question_number = 29 + (4 * person_index)
   end
 end
diff --git a/app/models/validations/property_validations.rb b/app/models/validations/property_validations.rb
index d2b6d2a0b..e9680facf 100644
--- a/app/models/validations/property_validations.rb
+++ b/app/models/validations/property_validations.rb
@@ -2,24 +2,6 @@ module Validations::PropertyValidations
   # Validations methods need to be called 'validate_<page_name>' to run on model save
   # or 'validate_' to run on submit as well
 
-  def validate_property_number_of_times_relet(record)
-    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, :over_20, message: I18n.t("validations.property.offered.relet_number")
-    end
-  end
-
   REFERRAL_INVALID_TMP = [8, 10, 12, 13, 14, 15].freeze
   def validate_rsnvac(record)
     if !record.first_time_property_let_as_social_housing? && record.has_first_let_vacancy_reason?
@@ -51,10 +33,6 @@ module Validations::PropertyValidations
   end
 
   def validate_shared_housing_rooms(record)
-    if record.beds.present? && record.beds <= 0
-      record.errors.add :beds, I18n.t("validations.property.beds.non_positive")
-    end
-
     unless record.unittype_gn.nil?
       if record.is_bedsit? && record.beds != 1 && record.beds.present?
         record.errors.add :unittype_gn, I18n.t("validations.property.unittype_gn.one_bedroom_bedsit")
@@ -70,10 +48,6 @@ module Validations::PropertyValidations
         record.errors.add :beds, I18n.t("validations.property.unittype_gn.one_seven_bedroom_shared")
       end
     end
-
-    if record.beds.present? && record.beds > 12
-      record.errors.add :beds, :over_max, message: I18n.t("validations.property.beds.over_max")
-    end
   end
 
   def validate_uprn(record)
diff --git a/app/models/validations/sales/sale_information_validations.rb b/app/models/validations/sales/sale_information_validations.rb
index e4714e81a..f99d79cf6 100644
--- a/app/models/validations/sales/sale_information_validations.rb
+++ b/app/models/validations/sales/sale_information_validations.rb
@@ -12,7 +12,7 @@ module Validations::Sales::SaleInformationValidations
   end
 
   def validate_years_living_in_property_before_purchase(record)
-    return unless record.proplen && record.proplen.nonzero?
+    return unless record.proplen&.nonzero?
 
     case record.type
     when 18
diff --git a/app/services/imports/lettings_logs_import_service.rb b/app/services/imports/lettings_logs_import_service.rb
index 94fb2ec3e..97ed3fe06 100644
--- a/app/services/imports/lettings_logs_import_service.rb
+++ b/app/services/imports/lettings_logs_import_service.rb
@@ -290,10 +290,10 @@ module Imports
         %i[prevten over_20_foster_care] => %w[prevten age1],
         %i[prevten non_temp_accommodation] => %w[prevten rsnvac],
         %i[joint not_joint_tenancy] => %w[joint],
-        %i[offered over_20] => %w[offered],
+        %i[offered outside_the_range] => %w[offered],
         %i[earnings over_hard_max] => %w[ecstat1],
         %i[tshortfall no_outstanding_charges] => %w[tshortfall hbrentshortfall],
-        %i[beds over_max] => %w[beds],
+        %i[beds outside_the_range] => %w[beds],
         %i[tcharge complete_1_of_3] => %w[brent scharge pscharge supcharg tcharge],
         %i[scharge under_min] => %w[brent scharge pscharge supcharg tcharge],
         %i[tshortfall must_be_positive] => %w[tshortfall tshortfall_known],
diff --git a/config/forms/2021_2022.json b/config/forms/2021_2022.json
index 741195f01..cf8ac4cd9 100644
--- a/config/forms/2021_2022.json
+++ b/config/forms/2021_2022.json
@@ -566,7 +566,7 @@
                   "hint_text": "This is after the last tenancy ended. If the property is being offered for let for the first time, enter 0.",
                   "type": "numeric",
                   "min": 0,
-                  "max": 150,
+                  "max": 20,
                   "step": 1,
                   "width": 2
                 }
@@ -588,7 +588,7 @@
                   "hint_text": "If the property is being offered for let for the first time, enter 0.",
                   "type": "numeric",
                   "min": 0,
-                  "max": 150,
+                  "max": 20,
                   "step": 1,
                   "width": 2
                 }
@@ -702,8 +702,8 @@
                   "header": "How many bedrooms does the property have?",
                   "hint_text": "If shared accommodation, enter the number of bedrooms occupied by this household. A bedsit has 1 bedroom.",
                   "type": "numeric",
-                  "min": 0,
-                  "max": 150,
+                  "min": 1,
+                  "max": 12,
                   "step": 1,
                   "width": 2
                 }
diff --git a/config/forms/2022_2023.json b/config/forms/2022_2023.json
index e13ce5516..7c189849d 100644
--- a/config/forms/2022_2023.json
+++ b/config/forms/2022_2023.json
@@ -561,7 +561,7 @@
                   "hint_text": "This is after the last tenancy ended. If the property is being offered for let for the first time, enter 0.",
                   "type": "numeric",
                   "min": 0,
-                  "max": 150,
+                  "max": 20,
                   "step": 1,
                   "width": 2
                 }
@@ -583,7 +583,7 @@
                   "hint_text": "If the property is being offered for let for the first time, enter 0.",
                   "type": "numeric",
                   "min": 0,
-                  "max": 150,
+                  "max": 20,
                   "step": 1,
                   "width": 2
                 }
@@ -697,8 +697,8 @@
                   "header": "How many bedrooms does the property have?",
                   "hint_text": "If shared accommodation, enter the number of bedrooms occupied by this household. A bedsit has 1 bedroom.",
                   "type": "numeric",
-                  "min": 0,
-                  "max": 150,
+                  "min": 1,
+                  "max": 12,
                   "step": 1,
                   "width": 2
                 }
diff --git a/config/locales/en.yml b/config/locales/en.yml
index f7968b336..bd8ad5244 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -218,8 +218,6 @@ en:
         ten_years_before_tenancy_start: "Enter a void date no more than 10 years before the tenancy start date"
         before_tenancy_start: "Enter a void date that is before the tenancy start date"
         after_mrcdate: "Void date must be before the major repairs date if provided"
-      offered:
-        relet_number: "Enter a number between 0 and 20 for the amount of times the property has been re-let"
       la:
         la_invalid_for_org: "%{org_name} does not operate in %{la_name}"
         postcode_invalid_for_org: "Enter a postcode in an area covered by %{org_name}"
@@ -235,8 +233,6 @@ en:
         one_seven_bedroom_shared: "A shared house must have 1 to 7 bedrooms"
         one_three_bedroom_single_tenant_shared: "A shared house with fewer than two tenants must have 1 to 3 bedrooms"
       beds:
-        non_positive: "Number of bedrooms has to be greater than 0"
-        over_max: "Number of bedrooms cannot be more than 12"
         bedsits_have_max_one_bedroom: "Number of bedrooms must be 1 if the property is a bedsit"
       proptype:
         bedsits_have_max_one_bedroom: "Answer cannot be 'Bedsit' if the property has 2 or more bedrooms"
diff --git a/spec/fixtures/forms/2021_2022.json b/spec/fixtures/forms/2021_2022.json
index 381f5a7b5..9ea11f671 100644
--- a/spec/fixtures/forms/2021_2022.json
+++ b/spec/fixtures/forms/2021_2022.json
@@ -997,11 +997,12 @@
             "care_home_charge": {
               "questions": {
                 "offered": {
-                  "check_answer_label": "Basic Rent",
-                  "header": "What is the basic rent?",
-                  "hint_text": "Eligible for housing benefit or Universal Credit",
+                  "check_answer_label": "Times previously offered since becoming available",
+                  "header": "Since becoming available for re-let, how many times has the property been previously offered?",
+                  "hint_text": "This is after the last tenancy ended. If the property is being offered for let for the first time, enter 0.",
                   "type": "numeric",
                   "min": 0,
+                  "max": 20,
                   "step": 1,
                   "width": 4
                 }
@@ -1015,11 +1016,12 @@
             "care_home_charge_bi_weekly": {
               "questions": {
                 "offered": {
-                  "check_answer_label": "Basic Rent",
-                  "header": "What is the basic rent?",
-                  "hint_text": "Eligible for housing benefit or Universal Credit",
+                  "check_answer_label": "Times previously offered since becoming available",
+                  "header": "Since becoming available for re-let, how many times has the property been previously offered?",
+                  "hint_text": "This is after the last tenancy ended. If the property is being offered for let for the first time, enter 0.",
                   "type": "numeric",
                   "min": 0,
+                  "max": 20,
                   "step": 1,
                   "width": 4
                 }
diff --git a/spec/models/form/lettings/questions/offered_social_let_spec.rb b/spec/models/form/lettings/questions/offered_social_let_spec.rb
index 6516c661b..ac1930495 100644
--- a/spec/models/form/lettings/questions/offered_social_let_spec.rb
+++ b/spec/models/form/lettings/questions/offered_social_let_spec.rb
@@ -6,7 +6,7 @@ RSpec.describe Form::Lettings::Questions::OfferedSocialLet, type: :model do
   let(:page) { instance_double(Form::Page) }
 
   it "has correct page" do
-    expect(question.page).to eq page
+    expect(question.page).to be page
   end
 
   it "has the correct id" do
@@ -26,12 +26,12 @@ RSpec.describe Form::Lettings::Questions::OfferedSocialLet, type: :model do
   end
 
   it "has the correct minimum and maximum values" do
-    expect(question.min).to eq 0
-    expect(question.max).to eq 150
+    expect(question.min).to be 0
+    expect(question.max).to be 20
   end
 
   it "has the correct step" do
-    expect(question.step).to eq 1
+    expect(question.step).to be 1
   end
 
   it "is not marked as derived" do
diff --git a/spec/models/lettings_log_spec.rb b/spec/models/lettings_log_spec.rb
index 1e6fd3244..eba5ea1d0 100644
--- a/spec/models/lettings_log_spec.rb
+++ b/spec/models/lettings_log_spec.rb
@@ -112,10 +112,6 @@ RSpec.describe LettingsLog do
       expect(validator).to receive(:validate_shared_housing_rooms)
     end
 
-    it "validates number of times the property has been relet" do
-      expect(validator).to receive(:validate_property_number_of_times_relet)
-    end
-
     it "validates tenancy type" do
       expect(validator).to receive(:validate_fixed_term_tenancy)
       expect(validator).to receive(:validate_other_tenancy_type)
@@ -3023,11 +3019,11 @@ RSpec.describe LettingsLog do
     end
 
     context "when a non setup field is invalid" do
-      subject(:model) { described_class.new(beds: 404) }
+      subject(:model) { build(:lettings_log, :completed, offered: 234) }
 
       it "blanks it" do
         model.valid?
-        expect { model.blank_invalid_non_setup_fields! }.to change(model, :beds)
+        expect { model.blank_invalid_non_setup_fields! }.to change(model, :offered)
       end
     end
   end
diff --git a/spec/models/validations/property_validations_spec.rb b/spec/models/validations/property_validations_spec.rb
index 3b3e42e9e..e918e3405 100644
--- a/spec/models/validations/property_validations_spec.rb
+++ b/spec/models/validations/property_validations_spec.rb
@@ -6,39 +6,6 @@ RSpec.describe Validations::PropertyValidations do
   let(:property_validator_class) { Class.new { include Validations::PropertyValidations } }
   let(:record) { FactoryBot.create(:lettings_log) }
 
-  describe "#validate_property_number_of_times_relet" do
-    let(:expected_error) { I18n.t("validations.property.offered.relet_number") }
-
-    it "does not add an error if the record offered is missing" do
-      record.offered = nil
-      property_validator.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
-      property_validator.validate_property_number_of_times_relet(record)
-      expect(record.errors).to be_empty
-      record.offered = 10
-      property_validator.validate_property_number_of_times_relet(record)
-      expect(record.errors).to be_empty
-      record.offered = 20
-      property_validator.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"
-      property_validator.validate_property_number_of_times_relet(record)
-      expect(record.errors).not_to be_empty
-      expect(record.errors["offered"]).to include(match(expected_error))
-      record.offered = 21
-      property_validator.validate_property_number_of_times_relet(record)
-      expect(record.errors).not_to be_empty
-      expect(record.errors["offered"]).to include(match(expected_error))
-    end
-  end
-
   describe "#validate_shared_housing_rooms" do
     context "when number of bedrooms has not been answered" do
       it "does not add an error" do
@@ -129,22 +96,6 @@ RSpec.describe Validations::PropertyValidations do
         expect(record.errors["beds"]).to include(I18n.t("validations.property.unittype_gn.one_three_bedroom_single_tenant_shared"))
       end
     end
-
-    context "when a negative number of bedrooms is entered" do
-      it "adds an error" do
-        record.beds = -4
-        property_validator.validate_shared_housing_rooms(record)
-        expect(record.errors["beds"]).to include(I18n.t("validations.property.beds.non_positive"))
-      end
-    end
-
-    context "when a room number higher than 12 has been entered" do
-      it "adds an error" do
-        record.beds = 13
-        property_validator.validate_shared_housing_rooms(record)
-        expect(record.errors["beds"]).to include(I18n.t("validations.property.beds.over_max"))
-      end
-    end
   end
 
   describe "#validate_unitletas" do
diff --git a/spec/requests/lettings_logs_controller_spec.rb b/spec/requests/lettings_logs_controller_spec.rb
index d95e0a3fd..6de5c4db2 100644
--- a/spec/requests/lettings_logs_controller_spec.rb
+++ b/spec/requests/lettings_logs_controller_spec.rb
@@ -82,7 +82,7 @@ RSpec.describe LettingsLogsController, type: :request do
         it "validates lettings 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.numeric.within_range", field: "Lead tenant’s age", min: 16, max: 120)]]])
+          expect(json_response["errors"]).to match_array([["offered", [I18n.t("validations.numeric.within_range", field: "Times previously offered since becoming available", min: 0, max: 20)]], ["age1", [I18n.t("validations.numeric.within_range", field: "Lead tenant’s age", min: 16, max: 120)]]])
         end
       end
 
diff --git a/spec/services/imports/lettings_logs_import_service_spec.rb b/spec/services/imports/lettings_logs_import_service_spec.rb
index deccde0c0..6c691ab32 100644
--- a/spec/services/imports/lettings_logs_import_service_spec.rb
+++ b/spec/services/imports/lettings_logs_import_service_spec.rb
@@ -406,7 +406,7 @@ RSpec.describe Imports::LettingsLogsImportService do
         end
 
         it "intercepts the relevant validation error" do
-          expect(logger).to receive(:warn).with(/Removing offered with error: Enter a number between 0 and 20 for the amount of times the property has been re-let/)
+          expect(logger).to receive(:warn).with(/Removing offered with error: Times previously offered since becoming available must be between 0 and 20/)
           expect { lettings_log_service.send(:create_log, lettings_log_xml) }
             .not_to raise_error
         end
@@ -502,7 +502,7 @@ RSpec.describe Imports::LettingsLogsImportService do
         end
 
         it "intercepts the relevant validation error" do
-          expect(logger).to receive(:warn).with(/Removing beds with error: Number of bedrooms cannot be more than 12/)
+          expect(logger).to receive(:warn).with(/Removing beds with error: Number of bedrooms must be between 1 and 12/)
           expect { lettings_log_service.send(:create_log, lettings_log_xml) }
             .not_to raise_error
         end