From d7538df04b19290cb4f55588f368990082aecfca Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Thu, 28 Sep 2023 14:25:30 +0100 Subject: [PATCH] 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 | 1 + .../imports/lettings_logs_import_service.rb | 21 ++++++++----- .../imports/sales_logs_import_service.rb | 22 +++++++++----- .../shared_ownership_sales_log2.xml | 2 +- .../lettings_logs_import_service_spec.rb | 29 ++++++++++++++++++ .../imports/sales_logs_import_service_spec.rb | 30 +++++++++++++++++++ 6 files changed, 89 insertions(+), 16 deletions(-) diff --git a/app/models/log.rb b/app/models/log.rb index 5354b3ba5..c1778b299 100644 --- a/app/models/log.rb +++ b/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 diff --git a/app/services/imports/lettings_logs_import_service.rb b/app/services/imports/lettings_logs_import_service.rb index 59d30172c..0083bda35 100644 --- a/app/services/imports/lettings_logs_import_service.rb +++ b/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 - 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 = 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 diff --git a/app/services/imports/sales_logs_import_service.rb b/app/services/imports/sales_logs_import_service.rb index bc2a096bc..d55ace377 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.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 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/lettings_logs_import_service_spec.rb b/spec/services/imports/lettings_logs_import_service_spec.rb index 1dd5c5578..c9ba509de 100644 --- a/spec/services/imports/lettings_logs_import_service_spec.rb +++ b/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) } diff --git a/spec/services/imports/sales_logs_import_service_spec.rb b/spec/services/imports/sales_logs_import_service_spec.rb index a30b5bbf8..447a33365 100644 --- a/spec/services/imports/sales_logs_import_service_spec.rb +++ b/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) }