From fbb67f5fabac25241e5a76517e127427fbac0474 Mon Sep 17 00:00:00 2001 From: Arthur Campbell <51094020+arfacamble@users.noreply.github.com> Date: Thu, 2 Feb 2023 17:05:42 +0000 Subject: [PATCH] CLDC-1923 amend validation message for single ended ranges (#1253) * replace range validation with multiple to account for single-ended ranges * add test for question with min but no max * add min and max to household count question and remove bespoke validation. Remove code allowing max and no min given this situation does not seem to exist int eh form * amend minor typo, add back a soft validation that had mysteriously gone missing --- .../shared_ownership_deposit_value_check.rb | 2 +- .../questions/number_of_others_in_property.rb | 2 + .../sales/household_validations.rb | 8 ---- app/models/validations/shared_validations.rb | 20 ++++++--- config/locales/en.yml | 6 ++- ...ared_ownership_deposit_value_check_spec.rb | 2 +- .../validations/household_validations_spec.rb | 4 +- .../sales/household_validations_spec.rb | 42 ------------------- .../validations/shared_validations_spec.rb | 24 +++++++---- .../requests/lettings_logs_controller_spec.rb | 2 +- 10 files changed, 40 insertions(+), 72 deletions(-) diff --git a/app/models/form/sales/pages/shared_ownership_deposit_value_check.rb b/app/models/form/sales/pages/shared_ownership_deposit_value_check.rb index 907968028..be2126482 100644 --- a/app/models/form/sales/pages/shared_ownership_deposit_value_check.rb +++ b/app/models/form/sales/pages/shared_ownership_deposit_value_check.rb @@ -8,7 +8,7 @@ class Form::Sales::Pages::SharedOwnershipDepositValueCheck < ::Form::Page ] @informative_text = {} @title_text = { - "translation" => "soft_validations.shared_owhership_deposit.title_text", + "translation" => "soft_validations.shared_ownership_deposit.title_text", "arguments" => [ { "key" => "expected_shared_ownership_deposit_value", diff --git a/app/models/form/sales/questions/number_of_others_in_property.rb b/app/models/form/sales/questions/number_of_others_in_property.rb index 3eb1f34e6..cf590291b 100644 --- a/app/models/form/sales/questions/number_of_others_in_property.rb +++ b/app/models/form/sales/questions/number_of_others_in_property.rb @@ -7,5 +7,7 @@ class Form::Sales::Questions::NumberOfOthersInProperty < ::Form::Question @type = "numeric" @hint_text = "You can provide details for a maximum of 4 other people." @width = 2 + @min = 0 + @max = 4 end end diff --git a/app/models/validations/sales/household_validations.rb b/app/models/validations/sales/household_validations.rb index 4f2290268..f9318ab0f 100644 --- a/app/models/validations/sales/household_validations.rb +++ b/app/models/validations/sales/household_validations.rb @@ -1,14 +1,6 @@ module Validations::Sales::HouseholdValidations include Validations::SharedValidations - def validate_number_of_other_people_living_in_the_property(record) - return if record.hholdcount.blank? - - unless record.hholdcount >= 0 && record.hholdcount <= 4 - record.errors.add :hholdcount, I18n.t("validations.numeric.valid", field: "Number of other people living in the property", min: 0, max: 4) - end - end - def validate_household_number_of_other_members(record) (2..6).each do |n| validate_person_age_matches_relationship(record, n) diff --git a/app/models/validations/shared_validations.rb b/app/models/validations/shared_validations.rb index 2d7ac0c9e..8452b9a52 100644 --- a/app/models/validations/shared_validations.rb +++ b/app/models/validations/shared_validations.rb @@ -20,20 +20,16 @@ module Validations::SharedValidations next unless question.min || question.max next unless record[question.id] - field = question.check_answer_label || question.id - min = [question.prefix, number_with_delimiter(question.min, delimiter: ","), question.suffix].join("") - max = [question.prefix, number_with_delimiter(question.max, delimiter: ","), question.suffix].join("") - begin answer = Float(record.public_send("#{question.id}_before_type_cast")) rescue ArgumentError - record.errors.add question.id.to_sym, I18n.t("validations.numeric.valid", field:, min:, max:) + add_range_error(record, question) 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:, max:) + add_range_error(record, question) end end end @@ -108,4 +104,16 @@ private def person_is_partner?(relationship) relationship == "P" end + + def add_range_error(record, question) + field = question.check_answer_label || question.id + min = [question.prefix, number_with_delimiter(question.min, delimiter: ","), question.suffix].join("") if question.min + max = [question.prefix, number_with_delimiter(question.max, delimiter: ","), question.suffix].join("") if question.max + + if min && max + record.errors.add question.id.to_sym, I18n.t("validations.numeric.within_range", field:, min:, max:) + elsif min + record.errors.add question.id.to_sym, I18n.t("validations.numeric.above_min", field:, min:) + end + end end diff --git a/config/locales/en.yml b/config/locales/en.yml index a469aa737..22aa05214 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -131,7 +131,8 @@ en: 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 between %{min} and %{max}" + within_range: "%{field} must be between %{min} and %{max}" + above_min: "%{field} must be at least %{min}" date: invalid_date: "Enter a date in the correct format, for example 31 1 2022" outside_collection_window: "Enter a date within the current collection windows" @@ -470,9 +471,10 @@ en: title_text: "You told us the time between the start of the tenancy and the major repairs completion date is more than 2 years" void_date: title_text: "You told us the time between the start of the tenancy and the void date is more than 2 years" - shared_owhership_deposit: + shared_ownership_deposit: title_text: "Mortgage, deposit and cash discount total should equal £%{expected_shared_ownership_deposit_value}" old_persons_shared_ownership: "At least one buyer should be aged over 64 for Older persons’ shared ownership scheme" + staircase_bought_seems_high: "You said %{percentage}% was bought in this staircasing transaction, which seems high. Are you sure?" monthly_charges_over_soft_max: title_text: "The amount of monthly charges is high for this type of property and sale type" diff --git a/spec/models/form/sales/pages/shared_ownership_deposit_value_check_spec.rb b/spec/models/form/sales/pages/shared_ownership_deposit_value_check_spec.rb index afe25f4d3..893a5cb5b 100644 --- a/spec/models/form/sales/pages/shared_ownership_deposit_value_check_spec.rb +++ b/spec/models/form/sales/pages/shared_ownership_deposit_value_check_spec.rb @@ -33,7 +33,7 @@ RSpec.describe Form::Sales::Pages::SharedOwnershipDepositValueCheck, type: :mode it "has the correct title_text" do expect(page.title_text).to eq({ - "translation" => "soft_validations.shared_owhership_deposit.title_text", + "translation" => "soft_validations.shared_ownership_deposit.title_text", "arguments" => [ { "key" => "expected_shared_ownership_deposit_value", diff --git a/spec/models/validations/household_validations_spec.rb b/spec/models/validations/household_validations_spec.rb index 0d1be05ae..daa3feef1 100644 --- a/spec/models/validations/household_validations_spec.rb +++ b/spec/models/validations/household_validations_spec.rb @@ -463,14 +463,14 @@ RSpec.describe Validations::HouseholdValidations do record.hhmemb = -1 household_validator.validate_numeric_min_max(record) expect(record.errors["hhmemb"]) - .to include(match I18n.t("validations.numeric.valid", field: "Number of Household Members", min: 0, max: 8)) + .to include(match I18n.t("validations.numeric.within_range", field: "Number of Household Members", min: 0, max: 8)) end it "validates that the number of household members cannot be more than 8" do record.hhmemb = 9 household_validator.validate_numeric_min_max(record) expect(record.errors["hhmemb"]) - .to include(match I18n.t("validations.numeric.valid", field: "Number of Household Members", min: 0, max: 8)) + .to include(match I18n.t("validations.numeric.within_range", field: "Number of Household Members", min: 0, max: 8)) end it "expects that the number of other household members is between the min and max" do diff --git a/spec/models/validations/sales/household_validations_spec.rb b/spec/models/validations/sales/household_validations_spec.rb index 26d369a35..10367390e 100644 --- a/spec/models/validations/sales/household_validations_spec.rb +++ b/spec/models/validations/sales/household_validations_spec.rb @@ -5,48 +5,6 @@ RSpec.describe Validations::Sales::HouseholdValidations do let(:validator_class) { Class.new { include Validations::Sales::HouseholdValidations } } - describe "#validate_number_of_other_people_living_in_the_property" do - context "when within permitted bounds" do - let(:record) { build(:sales_log, hholdcount: 2) } - - it "does not add an error" do - household_validator.validate_number_of_other_people_living_in_the_property(record) - - expect(record.errors[:hholdcount]).not_to be_present - end - end - - context "when blank" do - let(:record) { build(:sales_log, hholdcount: nil) } - - it "does not add an error" do - household_validator.validate_number_of_other_people_living_in_the_property(record) - - expect(record.errors[:hholdcount]).not_to be_present - end - end - - context "when below lower bound" do - let(:record) { build(:sales_log, hholdcount: -1) } - - it "adds an error" do - household_validator.validate_number_of_other_people_living_in_the_property(record) - - expect(record.errors[:hholdcount]).to be_present - end - end - - context "when higher than upper bound" do - let(:record) { build(:sales_log, hholdcount: 5) } - - it "adds an error" do - household_validator.validate_number_of_other_people_living_in_the_property(record) - - expect(record.errors[:hholdcount]).to be_present - end - end - end - describe "household member validations" do let(:record) { build(:sales_log) } diff --git a/spec/models/validations/shared_validations_spec.rb b/spec/models/validations/shared_validations_spec.rb index c1d8cb3af..151050d8b 100644 --- a/spec/models/validations/shared_validations_spec.rb +++ b/spec/models/validations/shared_validations_spec.rb @@ -18,42 +18,42 @@ RSpec.describe Validations::SharedValidations do record.age1 = "random" shared_validator.validate_numeric_min_max(record) expect(record.errors["age1"]) - .to include(match I18n.t("validations.numeric.valid", field: "Lead tenant’s age", min: 16, max: 120)) + .to include(match I18n.t("validations.numeric.within_range", field: "Lead tenant’s age", min: 16, max: 120)) end it "validates that other household member ages are a number" do record.age2 = "random" shared_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)) + .to include(match I18n.t("validations.numeric.within_range", 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 shared_validator.validate_numeric_min_max(record) expect(record.errors["age1"]) - .to include(match I18n.t("validations.numeric.valid", field: "Lead tenant’s age", min: 16, max: 120)) + .to include(match I18n.t("validations.numeric.within_range", field: "Lead tenant’s age", min: 16, max: 120)) end it "validates that other household member ages are greater than 1" do record.age2 = 0 shared_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)) + .to include(match I18n.t("validations.numeric.within_range", 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 shared_validator.validate_numeric_min_max(record) expect(record.errors["age1"]) - .to include(match I18n.t("validations.numeric.valid", field: "Lead tenant’s age", min: 16, max: 120)) + .to include(match I18n.t("validations.numeric.within_range", field: "Lead tenant’s age", min: 16, max: 120)) end it "validates that other household member ages are greater than 121" do record.age2 = 123 shared_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)) + .to include(match I18n.t("validations.numeric.within_range", field: "Person 2’s age", min: 1, max: 120)) end it "validates that person 1's age is between 16 and 120" do @@ -69,21 +69,27 @@ RSpec.describe Validations::SharedValidations do end end + it "adds the correct validation text when a question has a min but not a max" do + sales_record.savings = -10 + shared_validator.validate_numeric_min_max(sales_record) + expect(sales_record.errors["savings"]).to include(match I18n.t("validations.numeric.above_min", field: "Buyer’s total savings (to nearest £10) before any deposit paid", min: "£0")) + end + context "when validating percent" do it "validates that suffixes are added in the error message" do sales_record.stairbought = 150 shared_validator.validate_numeric_min_max(sales_record) expect(sales_record.errors["stairbought"]) - .to include(match I18n.t("validations.numeric.valid", field: "Percentage bought in this staircasing transaction", min: "0%", max: "100%")) + .to include(match I18n.t("validations.numeric.within_range", field: "Percentage bought in this staircasing transaction", min: "0%", max: "100%")) end end context "when validating price" do it "validates that £ prefix and , is added in the error message" do - sales_record.income1 = "random" + sales_record.income1 = -5 shared_validator.validate_numeric_min_max(sales_record) expect(sales_record.errors["income1"]) - .to include(match I18n.t("validations.numeric.valid", field: "Buyer 1’s gross annual income", min: "£0", max: "£999,999")) + .to include(match I18n.t("validations.numeric.within_range", field: "Buyer 1’s gross annual income", min: "£0", max: "£999,999")) end end end diff --git a/spec/requests/lettings_logs_controller_spec.rb b/spec/requests/lettings_logs_controller_spec.rb index 1bb0afbf9..57311b70d 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.valid", field: "Lead tenant’s age", min: 16, max: 120)]]]) + 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)]]]) end end