From 217ce62a82253f9c131e0f572876d35d817eadbc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Meny?= Date: Fri, 22 Apr 2022 12:25:40 +0100 Subject: [PATCH] Reduce differences between imported values and saved values (#486) * Compare imported values and saved values * Remove London validations * 2021-2022 form fixes * Code review changes --- .../local_authority_validations.rb | 41 ------------ .../imports/case_logs_import_service.rb | 64 +++++++++++++++++-- app/services/imports/import_service.rb | 2 + config/forms/2021_2022.json | 42 +++++++++++- config/locales/en.yml | 2 - .../local_authority_validations_spec.rb | 23 ------- .../imports/case_logs_import_service_spec.rb | 9 ++- ...ection_confirmation_import_service_spec.rb | 9 +-- .../organisation_la_import_service_spec.rb | 7 +- .../imports/user_import_service_spec.rb | 9 +-- 10 files changed, 120 insertions(+), 88 deletions(-) diff --git a/app/models/validations/local_authority_validations.rb b/app/models/validations/local_authority_validations.rb index 23ac8c590..0409572b9 100644 --- a/app/models/validations/local_authority_validations.rb +++ b/app/models/validations/local_authority_validations.rb @@ -10,13 +10,6 @@ module Validations::LocalAuthorityValidations end def validate_la(record) - if record.la.present? && !LONDON_BOROUGHS.include?(record.la) && record.is_london_rent? - record.errors.add :la, I18n.t("validations.property.la.london_rent") - if record.postcode_known? && record.postcode_full.present? - record.errors.add :postcode_full, I18n.t("validations.property.la.london_rent_postcode") - end - end - if record.la_known? && record.la.blank? record.errors.add :la, I18n.t("validations.property.la.la_known") end @@ -28,38 +21,4 @@ module Validations::LocalAuthorityValidations record.errors.add :la, I18n.t("validations.property.la.la_invalid_for_org", org_name:, la_name:) end end - - LONDON_BOROUGHS = %w[E09000001 - E09000002 - E09000003 - E09000004 - E09000005 - E09000006 - E09000007 - E09000008 - E09000009 - E09000010 - E09000011 - E09000012 - E09000013 - E09000014 - E09000015 - E09000016 - E09000017 - E09000018 - E09000019 - E09000020 - E09000021 - E09000022 - E09000023 - E09000024 - E09000025 - E09000026 - E09000027 - E09000028 - E09000029 - E09000030 - E09000031 - E09000032 - E09000033].freeze end diff --git a/app/services/imports/case_logs_import_service.rb b/app/services/imports/case_logs_import_service.rb index 806f55856..adec47526 100644 --- a/app/services/imports/case_logs_import_service.rb +++ b/app/services/imports/case_logs_import_service.rb @@ -65,6 +65,7 @@ module Imports end (2..8).each do |index| attributes["relat#{index}"] = relat(xml_doc, index) + attributes["details_known_#{index}"] = details_known(index, attributes) end attributes["ethnic"] = unsafe_string_as_integer(xml_doc, "P1Eth") attributes["ethnic_group"] = ethnic_group(attributes["ethnic"]) @@ -140,17 +141,41 @@ module Imports attributes["postcode_full"] = compose_postcode(xml_doc, "POSTCODE", "POSTCOD2") attributes["postcode_known"] = attributes["postcode_full"].nil? ? 0 : 1 - # Not specific to our form but required for CDS and can't be inferred + # Not specific to our form but required for consistency (present in import) attributes["old_form_id"] = Integer(field_value(xml_doc, "xmlns", "FORM")) + attributes["created_at"] = Time.zone.parse(field_value(xml_doc, "meta", "created-date")) + attributes["updated_at"] = Time.zone.parse(field_value(xml_doc, "meta", "modified-date")) - # Specific to us + # Required for our form invalidated questions (not present in import) attributes["previous_la_known"] = 1 # Defaulting to Yes (Required) attributes["la_known"] = 1 # Defaulting to Yes (Required) - attributes["created_at"] = Date.parse(field_value(xml_doc, "meta", "created-date")) - attributes["updated_at"] = Date.parse(field_value(xml_doc, "meta", "modified-date")) + attributes["is_la_inferred"] = false # Always keep the given LA + attributes["first_time_property_let_as_social_housing"] = first_time_let(attributes["rsnvac"]) + + unless attributes["brent"].nil? && + attributes["scharge"].nil? && + attributes["pscharge"].nil? && + attributes["supcharg"].nil? + attributes["brent"] ||= BigDecimal("0.0") + attributes["scharge"] ||= BigDecimal("0.0") + attributes["pscharge"] ||= BigDecimal("0.0") + attributes["supcharg"] ||= BigDecimal("0.0") + end case_log = CaseLog.new(attributes) case_log.save! + compute_differences(case_log, attributes) + end + + def compute_differences(case_log, attributes) + differences = [] + attributes.each do |key, value| + case_log_value = case_log.send(key.to_sym) + if value != case_log_value + differences.push("#{key} #{value} #{case_log_value}") + end + end + raise "Differences found when saving log #{case_log.id}: #{differences}" unless differences.empty? end # Safe: A string that represents only an integer (or empty/nil) @@ -162,7 +187,11 @@ module Imports # Safe: A string that represents only a decimal (or empty/nil) def safe_string_as_decimal(xml_doc, attribute) str = field_value(xml_doc, "xmlns", attribute) - BigDecimal(str, exception: false) + if str.blank? + nil + else + BigDecimal(str, exception: false) + end end # Unsafe: A string that has more than just the integer value @@ -182,7 +211,7 @@ module Imports if day.nil? || month.nil? || year.nil? nil else - Date.new(year, month, day) + Time.zone.local(year, month, day) end end @@ -290,6 +319,19 @@ module Imports end end + def details_known(index, attributes) + if index > attributes["hhmemb"] + nil + elsif attributes["age#{index}_known"] == 1 && + attributes["sex#{index}"] == "R" && + attributes["relat#{index}"] == "R" && + attributes["ecstat#{index}"] == 10 + 1 # No + else + 0 # Yes + end + end + def previous_postcode_known(xml_doc) previous_postcode_known = field_value(xml_doc, "xmlns", "Q12bnot") if previous_postcode_known == "Temporary or Unknown" @@ -305,7 +347,7 @@ module Imports if outcode_value.blank? || incode_value.blank? nil else - "#{outcode_value} #{incode_value}" + "#{outcode_value}#{incode_value}" end end @@ -388,5 +430,13 @@ module Imports 0 end end + + def first_time_let(rsnvac) + if [15, 16, 17].include?(rsnvac) + 1 + else + 0 + end + end end end diff --git a/app/services/imports/import_service.rb b/app/services/imports/import_service.rb index 8ac8a035d..bd48dc8a3 100644 --- a/app/services/imports/import_service.rb +++ b/app/services/imports/import_service.rb @@ -13,6 +13,8 @@ module Imports file_io = @storage_service.get_file_io(filename) xml_document = Nokogiri::XML(file_io) send(create_method, xml_document) + rescue StandardError => e + @logger.error "#{e.class} in #{filename}: #{e.message}" end end diff --git a/config/forms/2021_2022.json b/config/forms/2021_2022.json index 828f3e2ec..05e1f9de1 100644 --- a/config/forms/2021_2022.json +++ b/config/forms/2021_2022.json @@ -849,6 +849,38 @@ } }, "depends_on": [ + { + "renewal": 0, + "rsnvac": 5 + }, + { + "renewal": 0, + "rsnvac": 6 + }, + { + "renewal": 0, + "rsnvac": 8 + }, + { + "renewal": 0, + "rsnvac": 9 + }, + { + "renewal": 0, + "rsnvac": 10 + }, + { + "renewal": 0, + "rsnvac": 11 + }, + { + "renewal": 0, + "rsnvac": 12 + }, + { + "renewal": 0, + "rsnvac": 13 + }, { "renewal": 0, "rsnvac": 16 @@ -856,6 +888,14 @@ { "renewal": 0, "rsnvac": 17 + }, + { + "renewal": 0, + "rsnvac": 18 + }, + { + "renewal": 0, + "rsnvac": 19 } ] }, @@ -1032,7 +1072,7 @@ }, "depends_on": [ { - "startertenancy": 0 + "startertenancy": 2 } ] }, diff --git a/config/locales/en.yml b/config/locales/en.yml index 2d66ac916..5db7495d1 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -67,8 +67,6 @@ en: offered: relet_number: "Number of times the property has been re-let must be between 0 and 20" la: - london_rent: "Local authority must be in London" - london_rent_postcode: "The postcode must be a London postcode because you told us the rent type is London Affordable Rent or London Living Rent" la_known: "Enter name of local authority" la_invalid_for_org: "%{org_name} does not operate in %{la_name}" rsnvac: diff --git a/spec/models/validations/local_authority_validations_spec.rb b/spec/models/validations/local_authority_validations_spec.rb index ddd3afe8f..1ad97a72f 100644 --- a/spec/models/validations/local_authority_validations_spec.rb +++ b/spec/models/validations/local_authority_validations_spec.rb @@ -38,35 +38,12 @@ RSpec.describe Validations::LocalAuthorityValidations do describe "#validate_la" do context "when the rent type is London affordable" do - let(:expected_error) { I18n.t("validations.property.la.london_rent") } - - it "validates that the local authority is in London" do - record.la = "E07000105" - record.rent_type = 2 - local_auth_validator.validate_la(record) - expect(record.errors["la"]).to include(match(expected_error)) - expect(record.errors["postcode_full"]).to be_empty - end - it "expects that the local authority is in London" do record.la = "E09000033" record.rent_type = 2 local_auth_validator.validate_la(record) expect(record.errors["la"]).to be_empty end - - context "when the la has been derived from a known postcode" do - let(:expected_error) { I18n.t("validations.property.la.london_rent_postcode") } - - it "also adds an error to the postcode field" do - record.la = "E07000105" - record.rent_type = 2 - record.postcode_known = 1 - record.postcode_full = "BN18 7TR" - local_auth_validator.validate_la(record) - expect(record.errors["postcode_full"]).to include(match(expected_error)) - end - end end context "when previous la is known" do diff --git a/spec/services/imports/case_logs_import_service_spec.rb b/spec/services/imports/case_logs_import_service_spec.rb index 55b6cadca..111106f31 100644 --- a/spec/services/imports/case_logs_import_service_spec.rb +++ b/spec/services/imports/case_logs_import_service_spec.rb @@ -9,9 +9,10 @@ RSpec.describe Imports::CaseLogsImportService do let(:case_log_file2) { File.open("#{fixture_directory}/#{case_log_id2}.xml") } let(:storage_service) { instance_double(StorageService) } let(:real_2021_2022_form) { Form.new("config/forms/2021_2022.json", "2021_2022") } + let(:logger) { instance_double(ActiveSupport::Logger) } context "when importing users" do - subject(:case_log_service) { described_class.new(storage_service) } + subject(:case_log_service) { described_class.new(storage_service, logger) } before do # Stub the S3 file listing and download @@ -27,8 +28,10 @@ RSpec.describe Imports::CaseLogsImportService do allow(FormHandler.instance).to receive(:get_form).with(anything).and_return(real_2021_2022_form) end - it "successfully create a case log with the expected data" do - case_log_service.create_logs(remote_folder) + it "successfully create all case logs" do + expect(logger).not_to receive(:error) + expect { case_log_service.create_logs(remote_folder) } + .to change(CaseLog, :count).by(2) end end end diff --git a/spec/services/imports/data_protection_confirmation_import_service_spec.rb b/spec/services/imports/data_protection_confirmation_import_service_spec.rb index eddfca6a2..12f260618 100644 --- a/spec/services/imports/data_protection_confirmation_import_service_spec.rb +++ b/spec/services/imports/data_protection_confirmation_import_service_spec.rb @@ -6,9 +6,10 @@ RSpec.describe Imports::DataProtectionConfirmationImportService do let(:old_id) { old_org_id } let(:import_file) { File.open("#{fixture_directory}/#{old_id}.xml") } let(:storage_service) { instance_double(StorageService) } + let(:logger) { instance_double(ActiveSupport::Logger) } context "when importing data protection confirmations" do - subject(:import_service) { described_class.new(storage_service) } + subject(:import_service) { described_class.new(storage_service, logger) } before do allow(storage_service) @@ -22,8 +23,8 @@ RSpec.describe Imports::DataProtectionConfirmationImportService do context "when the organisation in the import file doesn't exist in the system" do it "does not create a data protection confirmation" do - expect { import_service.create_data_protection_confirmations("data_protection_directory") } - .to raise_error(ActiveRecord::RecordInvalid, /Organisation must exist/) + expect(logger).to receive(:error).with(/Organisation must exist/) + import_service.create_data_protection_confirmations("data_protection_directory") end end @@ -73,7 +74,7 @@ RSpec.describe Imports::DataProtectionConfirmationImportService do end it "logs that the record already exists" do - expect(Rails.logger).to receive(:warn) + expect(logger).to receive(:warn) import_service.create_data_protection_confirmations("data_protection_directory") end end diff --git a/spec/services/imports/organisation_la_import_service_spec.rb b/spec/services/imports/organisation_la_import_service_spec.rb index c0cf0cc4c..8247c2b36 100644 --- a/spec/services/imports/organisation_la_import_service_spec.rb +++ b/spec/services/imports/organisation_la_import_service_spec.rb @@ -6,9 +6,10 @@ RSpec.describe Imports::OrganisationLaImportService do let(:old_id) { "00013f30e159d7f72a3abe9ea93fb5b685d311e4" } let(:import_file) { File.open("#{fixture_directory}/#{old_id}.xml") } let(:storage_service) { instance_double(StorageService) } + let(:logger) { instance_double(ActiveSupport::Logger) } context "when importing data protection confirmations" do - subject(:import_service) { described_class.new(storage_service) } + subject(:import_service) { described_class.new(storage_service, logger) } before do allow(storage_service) @@ -22,8 +23,8 @@ RSpec.describe Imports::OrganisationLaImportService do context "when the organisation in the import file doesn't exist in the system" do it "does not create an organisation la record" do - expect { import_service.create_organisation_las("organisation_la_directory") } - .to raise_error(ActiveRecord::RecordInvalid, /Organisation must exist/) + expect(logger).to receive(:error).with(/Organisation must exist/) + import_service.create_organisation_las("organisation_la_directory") end end diff --git a/spec/services/imports/user_import_service_spec.rb b/spec/services/imports/user_import_service_spec.rb index 70552f41a..91487b593 100644 --- a/spec/services/imports/user_import_service_spec.rb +++ b/spec/services/imports/user_import_service_spec.rb @@ -6,9 +6,10 @@ RSpec.describe Imports::UserImportService do let(:old_org_id) { "7c5bd5fb549c09a2c55d7cb90d7ba84927e64618" } let(:user_file) { File.open("#{fixture_directory}/#{old_user_id}.xml") } let(:storage_service) { instance_double(StorageService) } + let(:logger) { instance_double(ActiveSupport::Logger) } context "when importing users" do - subject(:import_service) { described_class.new(storage_service) } + subject(:import_service) { described_class.new(storage_service, logger) } before do allow(storage_service).to receive(:list_files) @@ -33,8 +34,8 @@ RSpec.describe Imports::UserImportService do end it "refuses to create a user belonging to a non existing organisation" do - expect { import_service.create_users("user_directory") } - .to raise_error(ActiveRecord::RecordInvalid, /Organisation must exist/) + expect(logger).to receive(:error).with(/Organisation must exist/) + import_service.create_users("user_directory") end context "when the user is a data coordinator" do @@ -100,7 +101,7 @@ RSpec.describe Imports::UserImportService do end it "logs that the user already exists" do - expect(Rails.logger).to receive(:warn) + expect(logger).to receive(:warn) import_service.create_users("user_directory") end end