Import fixes and add update capabilities (#500)

app/services/imports/case_logs_import_service.rb

@@ -96,8 +96,8 @@ module Imports
 
       attributes["prevten"] = unsafe_string_as_integer(xml_doc, "Q11")
       attributes["prevloc"] = string_or_nil(xml_doc, "Q12aONS")
-      attributes["previous_postcode_known"] = previous_postcode_known(xml_doc)
       attributes["ppostcode_full"] = compose_postcode(xml_doc, "PPOSTC1", "PPOSTC2")
+      attributes["previous_postcode_known"] = previous_postcode_known(xml_doc, attributes["ppostcode_full"])
       attributes["layear"] = unsafe_string_as_integer(xml_doc, "Q12c")
       attributes["waityear"] = unsafe_string_as_integer(xml_doc, "Q12d")
       attributes["homeless"] = unsafe_string_as_integer(xml_doc, "Q13")
@@ -142,15 +142,17 @@ module Imports
       attributes["postcode_known"] = attributes["postcode_full"].nil? ? 0 : 1
 
       # Not specific to our form but required for consistency (present in import)
-      attributes["old_form_id"] = Integer(field_value(xml_doc, "xmlns", "FORM"))
+      attributes["old_form_id"] = safe_string_as_integer(xml_doc, "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"))
+      attributes["old_id"] = field_value(xml_doc, "meta", "document-id")
 
       # 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["is_la_inferred"] = false # Always keep the given LA
       attributes["first_time_property_let_as_social_housing"] = first_time_let(attributes["rsnvac"])
+      attributes["declaration"] = declaration(xml_doc)
 
       unless attributes["brent"].nil? &&
           attributes["scharge"].nil? &&
@@ -163,8 +165,19 @@ module Imports
       end
 
       case_log = CaseLog.new(attributes)
-      case_log.save!
+      save_case_log(case_log, attributes)
       compute_differences(case_log, attributes)
+      check_status_completed(case_log)
+    end
+
+    def save_case_log(case_log, attributes)
+      case_log.save!
+    rescue ActiveRecord::RecordNotUnique
+      unless attributes["old_id"].nil?
+        record = CaseLog.find_by(old_id: attributes["old_id"])
+        @logger.info "Updating case log #{case_log.id} with legacy ID #{attributes['old_id']}"
+        record.update!(attributes)
+      end
     end
 
     def compute_differences(case_log, attributes)
@@ -172,10 +185,16 @@ module Imports
       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}")
+          differences.push("#{key} #{value.inspect} #{case_log_value.inspect}")
+        end
       end
+      @logger.warn "Differences found when saving log #{case_log.id}: #{differences}" unless differences.empty?
+    end
+
+    def check_status_completed(case_log)
+      unless case_log.status == "completed"
+        @logger.warn "Case log #{case_log.id} is not completed"
       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)
@@ -186,8 +205,8 @@ 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?
+      str = string_or_nil(xml_doc, attribute)
+      if str.nil?
         nil
       else
         BigDecimal(str, exception: false)
@@ -196,8 +215,8 @@ module Imports
 
     # Unsafe: A string that has more than just the integer value
     def unsafe_string_as_integer(xml_doc, attribute)
-      str = field_value(xml_doc, "xmlns", attribute)
-      if str.blank?
+      str = string_or_nil(xml_doc, attribute)
+      if str.nil?
         nil
       else
         str.to_i
@@ -281,7 +300,7 @@ module Imports
     end
 
     def sex(xml_doc, index)
-      sex = field_value(xml_doc, "xmlns", "P#{index}Sex")
+      sex = string_or_nil(xml_doc, "P#{index}Sex")
       case sex
       when "Male"
         "M"
@@ -295,7 +314,7 @@ module Imports
     end
 
     def relat(xml_doc, index)
-      relat = field_value(xml_doc, "xmlns", "P#{index}Rel")
+      relat = string_or_nil(xml_doc, "P#{index}Rel")
       case relat
       when "Child"
         "C"
@@ -311,7 +330,7 @@ module Imports
     def age_known(xml_doc, index, hhmemb)
       return nil if index > hhmemb
 
-      age_refused = field_value(xml_doc, "xmlns", "P#{index}AR")
+      age_refused = string_or_nil(xml_doc, "P#{index}AR")
       if age_refused == "AGE_REFUSED"
         1 # No
       else
@@ -332,19 +351,21 @@ module Imports
       end
     end
 
-    def previous_postcode_known(xml_doc)
-      previous_postcode_known = field_value(xml_doc, "xmlns", "Q12bnot")
+    def previous_postcode_known(xml_doc, previous_postcode)
+      previous_postcode_known = string_or_nil(xml_doc, "Q12bnot")
       if previous_postcode_known == "Temporary or Unknown"
         0
+      elsif previous_postcode.nil?
+        nil
       else
         1
       end
     end
 
     def compose_postcode(xml_doc, outcode, incode)
-      outcode_value = field_value(xml_doc, "xmlns", outcode)
-      incode_value = field_value(xml_doc, "xmlns", incode)
-      if outcode_value.blank? || incode_value.blank?
+      outcode_value = string_or_nil(xml_doc, outcode)
+      incode_value = string_or_nil(xml_doc, incode)
+      if outcode_value.nil? || incode_value.nil?
         nil
       else
         "#{outcode_value}#{incode_value}"
@@ -400,7 +421,7 @@ module Imports
 
     # Letters should be lowercase to match case
     def housing_needs(xml_doc, letter)
-      housing_need = field_value(xml_doc, "xmlns", "Q10-#{letter}")
+      housing_need = string_or_nil(xml_doc, "Q10-#{letter}")
       if housing_need == "Yes"
         1
       else
@@ -409,7 +430,7 @@ module Imports
     end
 
     def net_income_known(xml_doc, earnings)
-      incref = field_value(xml_doc, "xmlns", "Q8Refused")
+      incref = string_or_nil(xml_doc, "Q8Refused")
       if incref == "Refused"
         # Tenant prefers not to say
         2
@@ -438,5 +459,12 @@ module Imports
         0
       end
     end
+
+    def declaration(xml_doc)
+      declaration = string_or_nil(xml_doc, "Qdp")
+      if declaration == "Yes"
+        1
+      end
+    end
   end
 end

app/services/imports/import_service.rb

@@ -14,7 +14,7 @@ module Imports
         xml_document = Nokogiri::XML(file_io)
         send(create_method, xml_document)
       rescue StandardError => e
-        @logger.error "#{e.class} in #{filename}: #{e.message}"
+        @logger.error "#{e.class} in #{filename}: #{e.message}. Caller: #{e.backtrace.first}"
       end
     end
 

db/migrate/20220425143130_add_old_id_and_index_to_case_logs.rb

@@ -0,0 +1,8 @@
+class AddOldIdAndIndexToCaseLogs < ActiveRecord::Migration[7.0]
+  def change
+    change_table :case_logs, bulk: true do |t|
+      t.column :old_id, :string
+    end
+    add_index :case_logs, :old_id, unique: true
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.0].define(version: 2022_04_21_140410) do
+ActiveRecord::Schema[7.0].define(version: 2022_04_25_143130) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
 
@@ -219,7 +219,9 @@ ActiveRecord::Schema[7.0].define(version: 2022_04_21_140410) do
     t.integer "old_form_id"
     t.integer "lar"
     t.integer "irproduct"
+    t.string "old_id"
     t.index ["managing_organisation_id"], name: "index_case_logs_on_managing_organisation_id"
+    t.index ["old_id"], name: "index_case_logs_on_old_id", unique: true
     t.index ["owning_organisation_id"], name: "index_case_logs_on_owning_organisation_id"
   end
 

spec/fixtures/exports/case_logs.xml

@@ -161,6 +161,7 @@
     <old_form_id/>
     <lar/>
     <irproduct/>
+    <old_id/>
     <providertype>1</providertype>
   </form>
 </forms>

spec/services/imports/case_logs_import_service_spec.rb

@@ -5,8 +5,6 @@ RSpec.describe Imports::CaseLogsImportService do
   let(:fixture_directory) { "spec/fixtures/softwire_imports/case_logs" }
   let(:case_log_id) { "0ead17cb-1668-442d-898c-0d52879ff592" }
   let(:case_log_id2) { "166fc004-392e-47a8-acb8-1c018734882b" }
-  let(:case_log_file) { File.open("#{fixture_directory}/#{case_log_id}.xml") }
-  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) }
@@ -14,24 +12,38 @@ RSpec.describe Imports::CaseLogsImportService do
   context "when importing users" do
     subject(:case_log_service) { described_class.new(storage_service, logger) }
 
+    def open_file(directory, filename)
+      File.open("#{directory}/#{filename}.xml")
+    end
+
     before do
       # Stub the S3 file listing and download
       allow(storage_service).to receive(:list_files)
                                   .and_return(%W[#{remote_folder}/#{case_log_id}.xml #{remote_folder}/#{case_log_id2}.xml])
       allow(storage_service).to receive(:get_file_io)
                                   .with("#{remote_folder}/#{case_log_id}.xml")
-                                  .and_return(case_log_file)
+                                  .and_return(open_file(fixture_directory, case_log_id), open_file(fixture_directory, case_log_id))
       allow(storage_service).to receive(:get_file_io)
                                   .with("#{remote_folder}/#{case_log_id2}.xml")
-                                  .and_return(case_log_file2)
+                                  .and_return(open_file(fixture_directory, case_log_id2), open_file(fixture_directory, case_log_id2))
       # Stub the form handler to use the real form
       allow(FormHandler.instance).to receive(:get_form).with(anything).and_return(real_2021_2022_form)
     end
 
     it "successfully create all case logs" do
       expect(logger).not_to receive(:error)
+      expect(logger).not_to receive(:warn)
+      expect(logger).not_to receive(:info)
       expect { case_log_service.create_logs(remote_folder) }
         .to change(CaseLog, :count).by(2)
     end
+
+    it "only updates existing case logs" do
+      expect(logger).not_to receive(:error)
+      expect(logger).not_to receive(:warn)
+      expect(logger).to receive(:info).with(/Updating case log/).twice
+      expect { 2.times { case_log_service.create_logs(remote_folder) } }
+        .to change(CaseLog, :count).by(2)
+    end
   end
 end