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

@@ -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)

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}"
+        if allow_updates
-        record.update!(attributes)
+          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)

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)

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

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