CLDC-2772 Deduplicate logs on import (#1945)

* Do not import logs if a duplicate log exists on the system

* deduplicate sales logs on import

* Only deduplicate logs created on new core

* Update test names

app/models/log.rb

@@ -45,6 +45,7 @@ class Log < ApplicationRecord
   }
   scope :created_by, ->(user) { where(created_by: user) }
   scope :imported, -> { where.not(old_id: nil) }
+  scope :not_imported, -> { where(old_id: nil) }
 
   attr_accessor :skip_update_status, :skip_update_uprn_confirmed
 

app/services/imports/lettings_logs_import_service.rb

@@ -289,16 +289,23 @@ module Imports
     def save_lettings_log(attributes, previous_status)
       lettings_log = LettingsLog.new(attributes)
       begin
-        lettings_log.save!
-        lettings_log
-      rescue ActiveRecord::RecordNotUnique
         legacy_id = attributes["old_id"]
         record = LettingsLog.find_by(old_id: legacy_id)
-        if allow_updates
+        if record.present?
-          attributes["updated_at"] = Time.zone.now
+          if allow_updates
-          record.update!(attributes)
+            attributes["updated_at"] = Time.zone.now
+            record.update!(attributes)
+          end
+          record
+        else
+          duplicate_logs = lettings_log.owning_organisation.owned_lettings_logs.not_imported.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
+            lettings_log.save!
+          end
+          lettings_log
         end
-        record
       rescue ActiveRecord::RecordInvalid => e
         rescue_validation_or_raise(lettings_log, attributes, previous_status, e)
       end

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 record.present?
-        if allow_updates
+          if allow_updates
-          attributes["updated_at"] = Time.zone.now
+            attributes["updated_at"] = Time.zone.now
-          record.update!(attributes)
+            record.update!(attributes)
+          end
+          record
+        else
+          duplicate_logs = sales_log.owning_organisation.owned_sales_logs.not_imported.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

spec/fixtures/imports/sales_logs/shared_ownership_sales_log2.xml

@@ -17,7 +17,7 @@
   <Group>
     <Qdp>Yes</Qdp>
     <CompletionDate>2023-01-17</CompletionDate>
-    <PurchaserCode>Shared ownership example</PurchaserCode>
+    <PurchaserCode>Shared ownership example 2</PurchaserCode>
     <Ownership>1 Yes - a shared ownership scheme</Ownership>
     <Q16SaleType>2 Shared Ownership</Q16SaleType>
     <Q30SaleType/>

spec/services/imports/lettings_logs_import_service_spec.rb

@@ -103,6 +103,35 @@ RSpec.describe Imports::LettingsLogsImportService do
         expect(updated_logs).to eq(0)
       end
 
+      it "does not import the log if a duplicate log exists on the system (that was not migrated from old CORE)" do
+        expect(logger).not_to receive(:error)
+        expect(logger).not_to receive(:warn)
+        expect(logger).not_to receive(:info).with(/Updating lettings log/)
+        expect(logger).to receive(:info).with(/Duplicate log with id \d+ found for log #{lettings_log_id}, skipping log/)
+        expect(logger).to receive(:info).with(/Duplicate log with id \d+ found for log #{lettings_log_id2}, skipping log/)
+        expect(logger).to receive(:info).with(/Duplicate log with id \d+ found for log #{lettings_log_id3}, skipping log/)
+
+        lettings_log_service.create_logs(remote_folder)
+        expect(LettingsLog.count).to eq(3)
+        LettingsLog.update_all(old_id: nil)
+
+        lettings_log_service.create_logs(remote_folder)
+        expect(LettingsLog.count).to eq(3)
+      end
+
+      it "imports the log if a duplicate imported log exists on the system (with different legacy ID)" do
+        expect(logger).not_to receive(:error)
+        expect(logger).not_to receive(:warn)
+        expect(logger).not_to receive(:info).with(/Updating lettings log/)
+
+        lettings_log_service.create_logs(remote_folder)
+        expect(LettingsLog.count).to eq(3)
+        LettingsLog.all.each { |log| log.update(old_id: "old_id_#{rand(1..999_999)}") }
+
+        lettings_log_service.create_logs(remote_folder)
+        expect(LettingsLog.count).to eq(6)
+      end
+
       context "with updates allowed" do
         subject(:lettings_log_service) { described_class.new(storage_service, logger, allow_updates: true) }
 

spec/services/imports/sales_logs_import_service_spec.rb

@@ -81,6 +81,36 @@ 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 (that was not migrated from old CORE)" 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
+
+    it "imports the log if a duplicate imported log exists on the system (with different legacy ID)" do
+      expect(logger).not_to receive(:error)
+      expect(logger).not_to receive(:warn)
+      expect(logger).not_to receive(:info).with(/Updating sales log/)
+
+      sales_log_service.create_logs(remote_folder)
+      expect(SalesLog.count).to eq(4)
+      SalesLog.all.each { |log| log.update(old_id: "old_id_#{rand(1..999_999)}") }
+
+      sales_log_service.create_logs(remote_folder)
+      expect(SalesLog.count).to eq(8)
+    end
+
     context "with updates allowed" do
       subject(:sales_log_service) { described_class.new(storage_service, logger, allow_updates: true) }