From c35bceede42121787de6daf33163f7beec6db724 Mon Sep 17 00:00:00 2001 From: Kat Date: Wed, 29 Mar 2023 09:06:12 +0100 Subject: [PATCH] Refactor validation error information infor a hash --- app/models/validations/tenancy_validations.rb | 2 +- .../imports/lettings_logs_import_service.rb | 188 ++++++------------ .../lettings_logs_import_service_spec.rb | 1 + 3 files changed, 62 insertions(+), 129 deletions(-) diff --git a/app/models/validations/tenancy_validations.rb b/app/models/validations/tenancy_validations.rb index 14727d3b0..2d4c15640 100644 --- a/app/models/validations/tenancy_validations.rb +++ b/app/models/validations/tenancy_validations.rb @@ -30,7 +30,7 @@ module Validations::TenancyValidations conditions.each do |condition| next unless condition[:condition] - record.errors.add :tenancylength, condition[:error] + record.errors.add :tenancylength, :tenancylength_invalid, message: condition[:error] record.errors.add :tenancy, condition[:error] end end diff --git a/app/services/imports/lettings_logs_import_service.rb b/app/services/imports/lettings_logs_import_service.rb index 3cb2f74cc..e7e4a7345 100644 --- a/app/services/imports/lettings_logs_import_service.rb +++ b/app/services/imports/lettings_logs_import_service.rb @@ -271,6 +271,19 @@ module Imports end end + ERRORS = { + %i[chcharge out_of_range] => { to_delete: %w[chcharge], message: "Removing chcharge, because it is outside the expected range" }, + %i[referral internal_transfer_non_social_housing] => { to_delete: %w[referral], message: "Removing internal transfer referral since previous tenancy is a non social housing" }, + %i[referral internal_transfer_fixed_or_lifetime] => { to_delete: %w[referral], message: "Removing internal transfer referral since previous tenancy is fixed terms or lifetime" }, + %i[earnings under_hard_min] => { + to_delete: %w[earnings incfreq], + to_assign: { "incref" => 1, "net_income_known" => 2 }, + message: "Where the income is 0, set earnings and income to blank and set incref to refused", + }, + %i[tenancylength tenancy] => { to_delete: %w[tenancylength tenancy], message: "Removing tenancylength as invalid" }, + %i[prevten over_20_foster_care] => { to_delete: %w[prevten age1], message: "Removing age1 and prevten as incompatible" }, + }.freeze + def rescue_validation_or_raise(lettings_log, attributes, previous_status, exception) # Blank out all invalid fields for in-progress logs if %w[saved submitted-invalid].include?(previous_status) @@ -279,135 +292,54 @@ module Imports attributes.delete(error.attribute.to_s) end @logs_overridden << lettings_log.old_id - save_lettings_log(attributes, previous_status) - elsif lettings_log.errors.of_kind?(:referral, :internal_transfer_non_social_housing) - @logger.warn("Log #{lettings_log.old_id}: Removing internal transfer referral since previous tenancy is a non social housing") - @logs_overridden << lettings_log.old_id - attributes.delete("referral") - save_lettings_log(attributes, previous_status) - elsif lettings_log.errors.of_kind?(:referral, :internal_transfer_fixed_or_lifetime) - @logger.warn("Log #{lettings_log.old_id}: Removing internal transfer referral since previous tenancy is fixed terms or lifetime") - @logs_overridden << lettings_log.old_id - attributes.delete("referral") - save_lettings_log(attributes, previous_status) - elsif lettings_log.errors.of_kind?(:earnings, :under_hard_min) - @logger.warn("Log #{lettings_log.old_id}: Where the income is 0, set earnings and income to blank and set incref to refused") - @logs_overridden << lettings_log.old_id - - attributes.delete("earnings") - attributes.delete("incfreq") - attributes["incref"] = 1 - attributes["net_income_known"] = 2 - save_lettings_log(attributes, previous_status) - elsif lettings_log.errors.include?(:tenancylength) && lettings_log.errors.include?(:tenancy) - @logger.warn("Log #{lettings_log.old_id}: Removing tenancylength as invalid") - @logs_overridden << lettings_log.old_id - attributes.delete("tenancylength") - attributes.delete("tenancy") - save_lettings_log(attributes, previous_status) - elsif lettings_log.errors.of_kind?(:prevten, :over_20_foster_care) - @logger.warn("Log #{lettings_log.old_id}: Removing age1 and prevten as incompatible") - @logs_overridden << lettings_log.old_id - attributes.delete("prevten") - attributes.delete("age1") - save_lettings_log(attributes, previous_status) - elsif lettings_log.errors.of_kind?(:prevten, :non_temp_accommodation) - @logger.warn("Log #{lettings_log.old_id}: Removing vacancy reason and previous tenancy since this accommodation is not temporary") - @logs_overridden << lettings_log.old_id - attributes.delete("prevten") - attributes.delete("rsnvac") - save_lettings_log(attributes, previous_status) - elsif lettings_log.errors.of_kind?(:joint, :not_joint_tenancy) - @logger.warn("Log #{lettings_log.old_id}: Removing joint tenancy as there is only 1 person in the household") - @logs_overridden << lettings_log.old_id - attributes.delete("joint") - save_lettings_log(attributes, previous_status) - elsif lettings_log.errors.of_kind?(:offered, :over_20) - @logger.warn("Log #{lettings_log.old_id}: Removing offered as the value is above the maximum of 20") - @logs_overridden << lettings_log.old_id - attributes.delete("offered") - save_lettings_log(attributes, previous_status) - elsif lettings_log.errors.of_kind?(:earnings, :over_hard_max) - @logger.warn("Log #{lettings_log.old_id}: Removing working situation because income is too high for it") - @logs_overridden << lettings_log.old_id - attributes.delete("ecstat1") - save_lettings_log(attributes, previous_status) - elsif lettings_log.errors.of_kind?(:tshortfall, :no_outstanding_charges) - @logger.warn("Log #{lettings_log.old_id}: Removing tshortfall as there are no outstanding charges") - @logs_overridden << lettings_log.old_id - attributes.delete("tshortfall") - attributes.delete("hbrentshortfall") - save_lettings_log(attributes, previous_status) - elsif lettings_log.errors.of_kind?(:age2, :outside_the_range) - @logger.warn("Log #{lettings_log.old_id}: Removing age2 because it is outside the allowed range") - @logs_overridden << lettings_log.old_id - attributes.delete("age2") - attributes.delete("age2_known") - save_lettings_log(attributes, previous_status) - elsif lettings_log.errors.of_kind?(:beds, :over_max) - @logger.warn("Log #{lettings_log.old_id}: Removing number of bedrooms because it is over the max/") + return save_lettings_log(attributes, previous_status) + end + + errors = { + %i[chcharge out_of_range] => { to_delete: %w[chcharge], message: "Removing chcharge, because it is outside the expected range" }, + %i[referral internal_transfer_non_social_housing] => { to_delete: %w[referral], message: "Removing internal transfer referral since previous tenancy is a non social housing" }, + %i[referral internal_transfer_fixed_or_lifetime] => { to_delete: %w[referral], message: "Removing internal transfer referral since previous tenancy is fixed terms or lifetime" }, + %i[earnings under_hard_min] => { to_delete: %w[earnings incfreq], to_set: { incref: 1, net_income_known: 2 }, message: "Where the income is 0, set earnings and income to blank and set incref to refused" }, + %i[tenancylength tenancylength_invalid] => { to_delete: %w[tenancylength tenancy], message: "Removing tenancylength as invalid" }, + %i[prevten over_20_foster_care] => { to_delete: %w[prevten age1], message: "Removing age1 and prevten as incompatible" }, + %i[prevten non_temp_accommodation] => { to_delete: %w[prevten rsnvac], message: "Removing vacancy reason and previous tenancy since this accommodation is not temporary" }, + %i[joint not_joint_tenancy] => { to_delete: %w[joint], message: "Removing joint tenancy as there is only 1 person in the household" }, + %i[offered over_20] => { to_delete: %w[offered], message: "Removing offered as the value is above the maximum of 20" }, + %i[earnings over_hard_max] => { to_delete: %w[ecstat1], message: "Removing working situation because income is too high for it" }, + %i[tshortfall no_outstanding_charges] => { to_delete: %w[tshortfall hbrentshortfall], message: "Removing tshortfall as there are no outstanding charges" }, + %i[age2 outside_the_range] => { to_delete: %w[age2 age2_known], message: "Removing age2 because it is outside the allowed range" }, + %i[beds over_max] => { to_delete: %w[beds], message: "Removing number of bedrooms because it is over the max" }, + %i[tcharge complete_1_of_3] => { to_delete: %w[brent scharge pscharge supcharg tcharge], message: "Removing charges, because multiple household charges are selected" }, + %i[scharge under_min] => { to_delete: %w[brent scharge pscharge supcharg tcharge], message: "Removing charges, because service charge is under 0" }, + %i[tshortfall must_be_positive] => { to_delete: %w[tshortfall tshortfall_known], message: "Removing tshortfall, because it is not positive" }, + %i[referral referral_invalid] => { to_delete: %w[referral], message: "Removing referral, because it is not a temporary accommodation" }, + %i[pscharge outside_the_range] => { to_delete: %w[brent scharge pscharge supcharg tcharge], message: "Removing charges, because pscharge is outside of the range" }, + %i[supcharg outside_the_range] => { to_delete: %w[brent scharge pscharge supcharg tcharge], message: "Removing charges, because supcharg is outside of the range" }, + %i[scharge outside_the_range] => { to_delete: %w[brent scharge pscharge supcharg tcharge], message: "Removing charges, because scharge is outside of the range" }, + %i[location_id not_active] => { to_delete: %w[location_id scheme_id], message: "Removing scheme and location because it was not active during the tenancy start date" }, + } + + errors.each do |(error, fields)| + next unless lettings_log.errors.of_kind?(*error) + + @logger.warn("Log #{lettings_log.old_id}: #{fields[:message]}") @logs_overridden << lettings_log.old_id - attributes.delete("beds") - save_lettings_log(attributes, previous_status) - elsif lettings_log.errors.of_kind?(:tcharge, :complete_1_of_3) - @logger.warn("Log #{lettings_log.old_id}: Removing charges, because multiple household charges are selected/") - @logs_overridden << lettings_log.old_id - %w[brent scharge pscharge supcharg tcharge].each { |name| attributes.delete(name) } - save_lettings_log(attributes, previous_status) - elsif lettings_log.errors.of_kind?(:scharge, :under_min) - @logger.warn("Log #{lettings_log.old_id}: Removing charges, because service charge is under 0/") - @logs_overridden << lettings_log.old_id - %w[brent scharge pscharge supcharg tcharge].each { |name| attributes.delete(name) } - save_lettings_log(attributes, previous_status) - elsif lettings_log.errors.of_kind?(:tshortfall, :must_be_positive) - @logger.warn("Log #{lettings_log.old_id}: Removing tshortfall, because it is not positive/") - @logs_overridden << lettings_log.old_id - attributes.delete("tshortfall") - attributes.delete("tshortfall_known") - save_lettings_log(attributes, previous_status) - elsif lettings_log.errors.of_kind?(:referral, :referral_invalid) - @logger.warn("Log #{lettings_log.old_id}: Removing referral, because it is not a temporary accommodation/") - @logs_overridden << lettings_log.old_id - attributes.delete("referral") - save_lettings_log(attributes, previous_status) - elsif lettings_log.errors.of_kind?(:pscharge, :outside_the_range) - @logger.warn("Log #{lettings_log.old_id}: Removing charges, because pscharge is outside of the range/") - @logs_overridden << lettings_log.old_id - %w[brent scharge pscharge supcharg tcharge].each { |name| attributes.delete(name) } - save_lettings_log(attributes, previous_status) - elsif lettings_log.errors.of_kind?(:supcharg, :outside_the_range) - @logger.warn("Log #{lettings_log.old_id}: Removing charges, because supcharg is outside of the range/") - @logs_overridden << lettings_log.old_id - %w[brent scharge pscharge supcharg tcharge].each { |name| attributes.delete(name) } - save_lettings_log(attributes, previous_status) - elsif lettings_log.errors.of_kind?(:scharge, :outside_the_range) - @logger.warn("Log #{lettings_log.old_id}: Removing charges, because scharge is outside of the range/") - @logs_overridden << lettings_log.old_id - %w[brent scharge pscharge supcharg tcharge].each { |name| attributes.delete(name) } - save_lettings_log(attributes, previous_status) - elsif lettings_log.errors.of_kind?(:location_id, :not_active) - @logger.warn("Log #{lettings_log.old_id}: Removing scheme and location because it was not active during the tenancy start date/") - @logs_overridden << lettings_log.old_id - %w[location_id scheme_id].each { |name| attributes.delete(name) } - save_lettings_log(attributes, previous_status) - elsif lettings_log.errors.of_kind?(:chcharge, :out_of_range) - @logger.warn("Log #{lettings_log.old_id}: Removing chcharge, because it is outside the expected range/") - @logs_overridden << lettings_log.old_id - attributes.delete("chcharge") - save_lettings_log(attributes, previous_status) - else - @logger.error("Log #{lettings_log.old_id}: Failed to import") - lettings_log.errors.each do |error| - @logger.error("Validation error: Field #{error.attribute}:") - @logger.error("\tOwning Organisation: #{lettings_log.owning_organisation&.name}") - @logger.error("\tManaging Organisation: #{lettings_log.managing_organisation&.name}") - @logger.error("\tOld CORE ID: #{lettings_log.old_id}") - @logger.error("\tOld CORE: #{attributes[error.attribute.to_s]&.inspect}") - @logger.error("\tNew CORE: #{lettings_log.read_attribute(error.attribute)&.inspect}") - @logger.error("\tError message: #{error.type}") - end - raise exception - end + fields[:to_delete].each { |field| attributes.delete(field) } + fields[:to_set].each { |field, value| attributes[field] = value } if fields[:to_set] + return save_lettings_log(attributes, previous_status) + end + + @logger.error("Log #{lettings_log.old_id}: Failed to import") + lettings_log.errors.each do |error| + @logger.error("Validation error: Field #{error.attribute}:") + @logger.error("\tOwning Organisation: #{lettings_log.owning_organisation&.name}") + @logger.error("\tManaging Organisation: #{lettings_log.managing_organisation&.name}") + @logger.error("\tOld CORE ID: #{lettings_log.old_id}") + @logger.error("\tOld CORE: #{attributes[error.attribute.to_s]&.inspect}") + @logger.error("\tNew CORE: #{lettings_log.read_attribute(error.attribute)&.inspect}") + @logger.error("\tError message: #{error.type}") + end + raise exception end def compute_differences(lettings_log, attributes) diff --git a/spec/services/imports/lettings_logs_import_service_spec.rb b/spec/services/imports/lettings_logs_import_service_spec.rb index 80c96932a..24aa568f3 100644 --- a/spec/services/imports/lettings_logs_import_service_spec.rb +++ b/spec/services/imports/lettings_logs_import_service_spec.rb @@ -224,6 +224,7 @@ RSpec.describe Imports::LettingsLogsImportService do it "intercepts the relevant validation error" do expect(logger).to receive(:warn).with(/Where the income is 0, set earnings and income to blank and set incref to refused/) + expect(logger).to receive(:warn).with(/Differences found when saving log/) expect { lettings_log_service.send(:create_log, lettings_log_xml) } .not_to raise_error end