From 64744dcf2614f772aef86b19a61eab39913c7647 Mon Sep 17 00:00:00 2001 From: Kat Date: Tue, 26 Sep 2023 15:39:27 +0100 Subject: [PATCH] deduplicate sales logs on import --- .../imports/lettings_logs_import_service.rb | 2 +- .../imports/sales_logs_import_service.rb | 22 ++++++++++++------- .../shared_ownership_sales_log2.xml | 2 +- .../imports/sales_logs_import_service_spec.rb | 20 +++++++++++++++++ 4 files changed, 36 insertions(+), 10 deletions(-) diff --git a/app/services/imports/lettings_logs_import_service.rb b/app/services/imports/lettings_logs_import_service.rb index 299326691..4d837ad77 100644 --- a/app/services/imports/lettings_logs_import_service.rb +++ b/app/services/imports/lettings_logs_import_service.rb @@ -298,7 +298,7 @@ module Imports end record else - duplicate_logs = lettings_log.created_by.lettings_logs.duplicate_logs(lettings_log) + duplicate_logs = lettings_log.owning_organisation.owned_lettings_logs.duplicate_logs(lettings_log) if duplicate_logs.count.positive? @logger.info("Duplicate log with id #{duplicate_logs.map(&:id).join(', ')} found for log #{lettings_log.old_id}, skipping log") else diff --git a/app/services/imports/sales_logs_import_service.rb b/app/services/imports/sales_logs_import_service.rb index bc2a096bc..f2d9d8966 100644 --- a/app/services/imports/sales_logs_import_service.rb +++ b/app/services/imports/sales_logs_import_service.rb @@ -223,17 +223,23 @@ module Imports def save_sales_log(attributes, previous_status) sales_log = SalesLog.new(attributes) begin - sales_log.save! - sales_log - rescue ActiveRecord::RecordNotUnique legacy_id = attributes["old_id"] record = SalesLog.find_by(old_id: legacy_id) - - if allow_updates - attributes["updated_at"] = Time.zone.now - record.update!(attributes) + if record.present? + if allow_updates + attributes["updated_at"] = Time.zone.now + record.update!(attributes) + end + record + else + duplicate_logs = sales_log.owning_organisation.owned_sales_logs.duplicate_logs(sales_log) + if duplicate_logs.count.positive? + @logger.info("Duplicate log with id #{duplicate_logs.map(&:id).join(', ')} found for log #{sales_log.old_id}, skipping log") + else + sales_log.save! + end + sales_log end - record rescue ActiveRecord::RecordInvalid => e rescue_validation_or_raise(sales_log, attributes, previous_status, e) end diff --git a/spec/fixtures/imports/sales_logs/shared_ownership_sales_log2.xml b/spec/fixtures/imports/sales_logs/shared_ownership_sales_log2.xml index 50c31ec6f..efdbe3eae 100644 --- a/spec/fixtures/imports/sales_logs/shared_ownership_sales_log2.xml +++ b/spec/fixtures/imports/sales_logs/shared_ownership_sales_log2.xml @@ -17,7 +17,7 @@ Yes 2023-01-17 - Shared ownership example + Shared ownership example 2 1 Yes - a shared ownership scheme 2 Shared Ownership diff --git a/spec/services/imports/sales_logs_import_service_spec.rb b/spec/services/imports/sales_logs_import_service_spec.rb index a30b5bbf8..a3ceffe80 100644 --- a/spec/services/imports/sales_logs_import_service_spec.rb +++ b/spec/services/imports/sales_logs_import_service_spec.rb @@ -81,6 +81,23 @@ RSpec.describe Imports::SalesLogsImportService do expect(updated_logs).to eq(0) end + it "does not import the log if a duplicate log exists on the system" do + expect(logger).not_to receive(:error) + expect(logger).not_to receive(:warn) + expect(logger).not_to receive(:info).with(/Updating sales log/) + expect(logger).to receive(:info).with(/Duplicate log with id \d+ found for log shared_ownership_sales_log, skipping log/) + expect(logger).to receive(:info).with(/Duplicate log with id \d+ found for log shared_ownership_sales_log2, skipping log/) + expect(logger).to receive(:info).with(/Duplicate log with id \d+ found for log outright_sale_sales_log, skipping log/) + expect(logger).to receive(:info).with(/Duplicate log with id \d+ found for log discounted_ownership_sales_log, skipping log/) + + sales_log_service.create_logs(remote_folder) + expect(SalesLog.count).to eq(4) + SalesLog.update_all(old_id: nil) + + sales_log_service.create_logs(remote_folder) + expect(SalesLog.count).to eq(4) + end + context "with updates allowed" do subject(:sales_log_service) { described_class.new(storage_service, logger, allow_updates: true) } @@ -142,6 +159,7 @@ RSpec.describe Imports::SalesLogsImportService do expect(logger).to receive(:error).with("Sales log 'fake_id' belongs to legacy user with owner-user-id: 'fake_id' which cannot be found. Assigning log to 'Unassigned' user.") sales_log_service.send(:create_log, sales_log_xml) sales_log_xml.at_xpath("//meta:document-id").content = "fake_id" + sales_log_xml.at_xpath("//xmlns:PurchaserCode").content = "fake_code" sales_log_service.send(:create_log, sales_log_xml) sales_log = SalesLog.where(old_id: sales_log_id).first @@ -181,6 +199,7 @@ RSpec.describe Imports::SalesLogsImportService do expect(logger).to receive(:error).with("Sales log 'fake_id' does not have the owner id. Assigning log to 'Unassigned' user.") sales_log_service.send(:create_log, sales_log_xml) sales_log_xml.at_xpath("//meta:document-id").content = "fake_id" + sales_log_xml.at_xpath("//xmlns:PurchaserCode").content = "fake_code" sales_log_service.send(:create_log, sales_log_xml) sales_log = SalesLog.where(old_id: sales_log_id).first @@ -210,6 +229,7 @@ RSpec.describe Imports::SalesLogsImportService do expect(logger).to receive(:error).with("Sales log 'fake_id' belongs to legacy user with owner-user-id: 'fake_id' which belongs to a different organisation. Assigning log to 'Unassigned' user.") sales_log_service.send(:create_log, sales_log_xml) sales_log_xml.at_xpath("//meta:document-id").content = "fake_id" + sales_log_xml.at_xpath("//xmlns:PurchaserCode").content = "fake_code" sales_log_service.send(:create_log, sales_log_xml) sales_log = SalesLog.where(old_id: sales_log_id).first