From bf972143a116a9c6dbf6e2882d40cf472a81cf8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Meny?= Date: Tue, 26 Apr 2022 10:33:36 +0100 Subject: [PATCH] Import fixes and add update capabilities (#500) --- .../imports/case_logs_import_service.rb | 66 +++++++++++++------ app/services/imports/import_service.rb | 2 +- ...43130_add_old_id_and_index_to_case_logs.rb | 8 +++ db/schema.rb | 4 +- spec/fixtures/exports/case_logs.xml | 1 + .../imports/case_logs_import_service_spec.rb | 20 ++++-- 6 files changed, 76 insertions(+), 25 deletions(-) create mode 100644 db/migrate/20220425143130_add_old_id_and_index_to_case_logs.rb diff --git a/app/services/imports/case_logs_import_service.rb b/app/services/imports/case_logs_import_service.rb index adec47526..7a9799078 100644 --- a/app/services/imports/case_logs_import_service.rb +++ b/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 - raise "Differences found when saving log #{case_log.id}: #{differences}" unless differences.empty? + @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 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 diff --git a/app/services/imports/import_service.rb b/app/services/imports/import_service.rb index bd48dc8a3..3f319b358 100644 --- a/app/services/imports/import_service.rb +++ b/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 diff --git a/db/migrate/20220425143130_add_old_id_and_index_to_case_logs.rb b/db/migrate/20220425143130_add_old_id_and_index_to_case_logs.rb new file mode 100644 index 000000000..882a77f43 --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index dcd9e007a..40cd9cd83 100644 --- a/db/schema.rb +++ b/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 diff --git a/spec/fixtures/exports/case_logs.xml b/spec/fixtures/exports/case_logs.xml index a5dd626af..e837ae266 100644 --- a/spec/fixtures/exports/case_logs.xml +++ b/spec/fixtures/exports/case_logs.xml @@ -161,6 +161,7 @@ + 1 diff --git a/spec/services/imports/case_logs_import_service_spec.rb b/spec/services/imports/case_logs_import_service_spec.rb index 111106f31..801d1a065 100644 --- a/spec/services/imports/case_logs_import_service_spec.rb +++ b/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