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

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

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,8 +187,12 @@ 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)
+      if str.blank?
+        nil
+      else
         BigDecimal(str, exception: false)
       end
+    end
 
     # Unsafe: A string that has more than just the integer value
     def unsafe_string_as_integer(xml_doc, attribute)
@@ -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

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
 

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
                 }
               ]
             },

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:

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

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

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

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
 

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