From 169f266b53d87cf90dec26e1ac6f7c03a2c7fdb2 Mon Sep 17 00:00:00 2001 From: baarkerlounger Date: Mon, 14 Feb 2022 12:11:36 +0000 Subject: [PATCH] Refactor other field validation --- Gemfile | 5 +++-- app/models/case_log.rb | 14 -------------- app/models/validations/household_validations.rb | 6 ++---- app/models/validations/shared_validations.rb | 15 +++++++++++++++ app/models/validations/tenancy_validations.rb | 4 +++- config/locales/en.yml | 2 ++ spec/models/case_log_spec.rb | 9 --------- .../validations/household_validations_spec.rb | 16 ++++++++++++++++ 8 files changed, 41 insertions(+), 30 deletions(-) create mode 100644 app/models/validations/shared_validations.rb diff --git a/Gemfile b/Gemfile index 7aa7295b7..9eb64a26d 100644 --- a/Gemfile +++ b/Gemfile @@ -54,17 +54,18 @@ gem "sentry-rails" gem "sentry-ruby" group :development, :test do - # Call 'byebug' anywhere in the code to stop execution and get a debugger console + # Check gems for known vulnerabilities gem "bundler-audit" + # Call 'byebug' anywhere in the code to stop execution and get a debugger console gem "byebug", platforms: %i[mri mingw x64_mingw] gem "dotenv-rails" gem "pry-byebug" end group :development do - # Access an interactive console on exception pages or by calling 'console' anywhere in the code. gem "listen", "~> 3.3" gem "overcommit", ">= 0.37.0" + # Access an interactive console on exception pages or by calling 'console' anywhere in the code. gem "web-console", ">= 4.1.0" # Display performance information such as SQL time and flame graphs for each request in your browser. # Can be configured to work on production as well see: https://github.com/MiniProfiler/rack-mini-profiler/blob/master/README.md diff --git a/app/models/case_log.rb b/app/models/case_log.rb index 197e3b389..920bbd5e7 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -13,20 +13,6 @@ class CaseLogValidator < ActiveModel::Validator validation_methods = public_methods.select { |method| method.starts_with?("validate_") } validation_methods.each { |meth| public_send(meth, record) } end - -private - - def validate_other_field(record, main_field, other_field) - main_field_label = main_field.humanize(capitalize: false) - other_field_label = other_field.humanize(capitalize: false) - if record[main_field] == "Other" && record[other_field].blank? - record.errors.add other_field.to_sym, "If #{main_field_label} is other then #{other_field_label} must be provided" - end - - if record[main_field] != "Other" && record[other_field].present? - record.errors.add other_field.to_sym, "#{other_field_label} must not be provided if #{main_field_label} was not other" - end - end end class CaseLog < ApplicationRecord diff --git a/app/models/validations/household_validations.rb b/app/models/validations/household_validations.rb index 128a40823..e253ee34b 100644 --- a/app/models/validations/household_validations.rb +++ b/app/models/validations/household_validations.rb @@ -1,4 +1,5 @@ module Validations::HouseholdValidations + include Validations::SharedValidations include Constants::CaseLog # Validations methods need to be called 'validate_' to run on model save @@ -14,14 +15,11 @@ module Validations::HouseholdValidations end end - def validate_other_reason_for_leaving_last_settled_home(record) - validate_other_field(record, "reason", "other_reason_for_leaving_last_settled_home") - end - def validate_reason_for_leaving_last_settled_home(record) if record.reason == "Don’t know" && record.underoccupation_benefitcap != "Don’t know" record.errors.add :underoccupation_benefitcap, I18n.t("validations.household.underoccupation_benefitcap.dont_know_required") end + validate_other_field(record, :reason, :other_reason_for_leaving_last_settled_home) end def validate_armed_forces_injured(record) diff --git a/app/models/validations/shared_validations.rb b/app/models/validations/shared_validations.rb new file mode 100644 index 000000000..f9ea78788 --- /dev/null +++ b/app/models/validations/shared_validations.rb @@ -0,0 +1,15 @@ +module Validations::SharedValidations + def validate_other_field(record, main_field = nil, other_field = nil) + return unless main_field || other_field + + main_field_label = main_field.to_s.humanize(capitalize: false) + other_field_label = other_field.to_s.humanize(capitalize: false) + if record[main_field] == "Other" && record[other_field].blank? + record.errors.add other_field.to_sym, I18n.t("validations.other_field_missing", main_field_label:, other_field_label:) + end + + if record[main_field] != "Other" && record[other_field].present? + record.errors.add other_field.to_sym, I18n.t("validations.other_field_not_required", main_field_label:, other_field_label:) + end + end +end diff --git a/app/models/validations/tenancy_validations.rb b/app/models/validations/tenancy_validations.rb index 456eff451..f45bd0cd9 100644 --- a/app/models/validations/tenancy_validations.rb +++ b/app/models/validations/tenancy_validations.rb @@ -1,6 +1,8 @@ module Validations::TenancyValidations # Validations methods need to be called 'validate_' to run on model save # or 'validate_' to run on submit as well + include Validations::SharedValidations + def validate_fixed_term_tenancy(record) is_present = record.tenancylength.present? is_in_range = record.tenancylength.to_i.between?(2, 99) @@ -21,6 +23,6 @@ module Validations::TenancyValidations end def validate_other_tenancy_type(record) - validate_other_field(record, "tenancy", "tenancyother") + validate_other_field(record, :tenancy, :tenancyother) end end diff --git a/config/locales/en.yml b/config/locales/en.yml index 96f4f6458..05d7963b5 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -35,6 +35,8 @@ en: updated: "Organisation details updated" 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" date: invalid_date: "Please enter a valid date" outside_collection_window: "Date must be within the current collection windows" diff --git a/spec/models/case_log_spec.rb b/spec/models/case_log_spec.rb index 229904279..571d8ecc1 100644 --- a/spec/models/case_log_spec.rb +++ b/spec/models/case_log_spec.rb @@ -56,15 +56,6 @@ RSpec.describe CaseLog do end context "with reason for leaving last settled home validation set to other" do - it "must be provided if main reason for leaving last settled home was given as other" do - expect { - described_class.create!(reason: "Other", - other_reason_for_leaving_last_settled_home: nil, - owning_organisation:, - managing_organisation:) - }.to raise_error(ActiveRecord::RecordInvalid) - end - it "must not be provided if the main reason for leaving settled home is not other" do expect { described_class.create!(reason: "Repossession", diff --git a/spec/models/validations/household_validations_spec.rb b/spec/models/validations/household_validations_spec.rb index b4e582b5e..12dbb7e80 100644 --- a/spec/models/validations/household_validations_spec.rb +++ b/spec/models/validations/household_validations_spec.rb @@ -165,4 +165,20 @@ RSpec.describe Validations::HouseholdValidations do end end end + + describe "reason for leaving last settled home validations" do + context "when reason is other" do + it "validates that a reason is provided" do + record.reason = "Other" + record.other_reason_for_leaving_last_settled_home = nil + household_validator.validate_reason_for_leaving_last_settled_home(record) + expect(record.errors["other_reason_for_leaving_last_settled_home"]) + .to include(match I18n.t( + "validations.other_field_missing", + main_field_label: "reason", + other_field_label: "other reason for leaving last settled home", + )) + end + end + end end