From 2c47e62248865a31575da1f20f4b6c5464a04ac1 Mon Sep 17 00:00:00 2001 From: James Rose Date: Thu, 18 May 2023 12:21:05 +0100 Subject: [PATCH] Disable log updates by default for importers (#1635) - Previously, when attempting to import a log that already exists in the service, the importers would update its attributes. - In most situations we don't want this as it could override changes that the user has made to the logs. - This change adds an option to the log import services to allow this functionality to be toggled. --- app/services/imports/import_service.rb | 5 +- .../imports/lettings_logs_import_service.rb | 11 ++- .../imports/sales_logs_import_service.rb | 12 +++- .../lettings_logs_import_service_spec.rb | 68 +++++++++++++++++-- .../imports/sales_logs_import_service_spec.rb | 34 +++++++++- 5 files changed, 114 insertions(+), 16 deletions(-) diff --git a/app/services/imports/import_service.rb b/app/services/imports/import_service.rb index 1db1f4d6a..2a2e0f3a6 100644 --- a/app/services/imports/import_service.rb +++ b/app/services/imports/import_service.rb @@ -1,11 +1,14 @@ module Imports class ImportService + attr_accessor :allow_updates + private - def initialize(storage_service, logger = Rails.logger) + def initialize(storage_service, logger = Rails.logger, allow_updates: false) @storage_service = storage_service @logger = logger @logs_with_discrepancies = [] + @allow_updates = allow_updates end def import_from(folder, create_method) diff --git a/app/services/imports/lettings_logs_import_service.rb b/app/services/imports/lettings_logs_import_service.rb index 75d7f1737..1d63d5d78 100644 --- a/app/services/imports/lettings_logs_import_service.rb +++ b/app/services/imports/lettings_logs_import_service.rb @@ -1,6 +1,6 @@ module Imports class LettingsLogsImportService < LogsImportService - def initialize(storage_service, logger = Rails.logger) + def initialize(storage_service, logger = Rails.logger, allow_updates: false) @logs_with_discrepancies = Set.new @logs_overridden = Set.new super @@ -275,8 +275,13 @@ module Imports rescue ActiveRecord::RecordNotUnique legacy_id = attributes["old_id"] record = LettingsLog.find_by(old_id: legacy_id) - @logger.info "Updating lettings log #{record.id} with legacy ID #{legacy_id}" - record.update!(attributes) + if allow_updates + attributes["updated_at"] = Time.zone.now + @logger.info "Updating lettings log #{record.id} with legacy ID #{legacy_id}" + record.update!(attributes) + else + @logger.info "Lettings log #{record.id} with legacy ID #{legacy_id} already present, skipping." + end record rescue ActiveRecord::RecordInvalid => e rescue_validation_or_raise(lettings_log, attributes, previous_status, e) diff --git a/app/services/imports/sales_logs_import_service.rb b/app/services/imports/sales_logs_import_service.rb index 6e2c11bf3..59ce65d60 100644 --- a/app/services/imports/sales_logs_import_service.rb +++ b/app/services/imports/sales_logs_import_service.rb @@ -1,6 +1,6 @@ module Imports class SalesLogsImportService < LogsImportService - def initialize(storage_service, logger = Rails.logger) + def initialize(storage_service, logger = Rails.logger, allow_updates: false) @logs_with_discrepancies = Set.new @logs_overridden = Set.new super @@ -198,8 +198,14 @@ module Imports rescue ActiveRecord::RecordNotUnique legacy_id = attributes["old_id"] record = SalesLog.find_by(old_id: legacy_id) - @logger.info "Updating sales log #{record.id} with legacy ID #{legacy_id}" - record.update!(attributes) + + if allow_updates + @logger.info "Updating sales log #{record.id} with legacy ID #{legacy_id}" + attributes["updated_at"] = Time.zone.now + record.update!(attributes) + else + @logger.info "Lettings log #{record.id} with legacy ID #{legacy_id} already present, skipping." + end record rescue ActiveRecord::RecordInvalid => e rescue_validation_or_raise(sales_log, attributes, previous_status, e) diff --git a/spec/services/imports/lettings_logs_import_service_spec.rb b/spec/services/imports/lettings_logs_import_service_spec.rb index 6132890a1..07e863982 100644 --- a/spec/services/imports/lettings_logs_import_service_spec.rb +++ b/spec/services/imports/lettings_logs_import_service_spec.rb @@ -85,12 +85,40 @@ RSpec.describe Imports::LettingsLogsImportService do .to change(LettingsLog, :count).by(3) end - it "only updates existing lettings logs" do + it "does not by default update existing lettings logs" do expect(logger).not_to receive(:error) expect(logger).not_to receive(:warn) - expect(logger).to receive(:info).with(/Updating lettings log/).exactly(3).times + expect(logger).not_to receive(:info).with(/Updating lettings log/) + + start_time = Time.current + expect { 2.times { lettings_log_service.create_logs(remote_folder) } } - .to change(LettingsLog, :count).by(3) + .to change(LettingsLog, :count).by(3) + + end_time = Time.current + + updated_logs = LettingsLog.where(updated_at: start_time..end_time).count + expect(updated_logs).to eq(0) + end + + context "with updates allowed" do + subject(:lettings_log_service) { described_class.new(storage_service, logger, allow_updates: true) } + + it "only updates existing lettings logs" do + expect(logger).not_to receive(:error) + expect(logger).not_to receive(:warn) + expect(logger).to receive(:info).with(/Updating lettings log/).exactly(3).times + + start_time = Time.current + + expect { 2.times { lettings_log_service.create_logs(remote_folder) } } + .to change(LettingsLog, :count).by(3) + + end_time = Time.current + + updated_logs = LettingsLog.where(updated_at: start_time..end_time).count + expect(updated_logs).to eq(3) + end end it "creates organisation relationship once" do @@ -1078,12 +1106,40 @@ RSpec.describe Imports::LettingsLogsImportService do .to change(LettingsLog, :count).by(1) end - it "only updates existing lettings logs" do + it "does not by default update existing lettings logs" do expect(logger).not_to receive(:error) expect(logger).not_to receive(:warn) - expect(logger).to receive(:info).with(/Updating lettings log/).once + expect(logger).not_to receive(:info).with(/Updating lettings log/) + + start_time = Time.current + expect { 2.times { lettings_log_service.create_logs(remote_folder) } } - .to change(LettingsLog, :count).by(1) + .to change(LettingsLog, :count).by(1) + + end_time = Time.current + + updated_logs = LettingsLog.where(updated_at: start_time..end_time).count + expect(updated_logs).to eq(0) + end + + context "with updates allowed" do + subject(:lettings_log_service) { described_class.new(storage_service, logger, allow_updates: true) } + + it "only updates existing lettings logs" do + expect(logger).not_to receive(:error) + expect(logger).not_to receive(:warn) + expect(logger).to receive(:info).with(/Updating lettings log/).once + + start_time = Time.current + + expect { 2.times { lettings_log_service.create_logs(remote_folder) } } + .to change(LettingsLog, :count).by(1) + + end_time = Time.current + + updated_logs = LettingsLog.where(updated_at: start_time..end_time).count + expect(updated_logs).to eq(1) + end end it "creates organisation relationship once" do diff --git a/spec/services/imports/sales_logs_import_service_spec.rb b/spec/services/imports/sales_logs_import_service_spec.rb index c40819223..52ea5f59b 100644 --- a/spec/services/imports/sales_logs_import_service_spec.rb +++ b/spec/services/imports/sales_logs_import_service_spec.rb @@ -56,12 +56,40 @@ RSpec.describe Imports::SalesLogsImportService do .to change(SalesLog, :count).by(4) end - it "only updates existing sales logs" do + it "does not by default update existing sales logs" do expect(logger).not_to receive(:error) expect(logger).not_to receive(:warn) - expect(logger).to receive(:info).with(/Updating sales log/).exactly(4).times + expect(logger).not_to receive(:info).with(/Updating sales log/) + + start_time = Time.current + expect { 2.times { sales_log_service.create_logs(remote_folder) } } - .to change(SalesLog, :count).by(4) + .to change(SalesLog, :count).by(4) + + end_time = Time.current + + updated_logs = SalesLog.where(updated_at: start_time..end_time).count + expect(updated_logs).to eq(0) + end + + context "with updates allowed" do + subject(:sales_log_service) { described_class.new(storage_service, logger, allow_updates: true) } + + it "only updates existing sales logs" do + expect(logger).not_to receive(:error) + allow(logger).to receive(:warn) + expect(logger).to receive(:info).with(/Updating sales log/).exactly(4).times + + start_time = Time.current + + expect { 2.times { sales_log_service.create_logs(remote_folder) } } + .to change(SalesLog, :count).by(4) + + end_time = Time.current + + updated_logs = SalesLog.where(updated_at: start_time..end_time).count + expect(updated_logs).to eq(4) + end end context "when there are status discrepancies" do