Add users to unassigned when the owner id is not given (#1929)

* Add users to unassigned when the owner id is not given

* test

app/services/imports/lettings_logs_import_service.rb

@@ -249,32 +249,32 @@ module Imports
 
       # Sets the log creator
       owner_id = meta_field_value(xml_doc, "owner-user-id").strip
-      if owner_id.present?
-        user = LegacyUser.find_by(old_user_id: owner_id)&.user
-        if user.blank? || (user.organisation_id != attributes["managing_organisation_id"] && user.organisation_id != attributes["owning_organisation_id"])
-          if user.blank?
-            @logger.error("Lettings log '#{attributes['old_id']}' belongs to legacy user with owner-user-id: '#{owner_id}' which cannot be found. Assigning log to 'Unassigned' user.")
-          else
-            @logger.error("Lettings log '#{attributes['old_id']}' belongs to legacy user with owner-user-id: '#{owner_id}' which belongs to a different organisation. Assigning log to 'Unassigned' user.")
-          end
-          if User.find_by(name: "Unassigned", organisation_id: attributes["managing_organisation_id"])
-            user = User.find_by(name: "Unassigned", organisation_id: attributes["managing_organisation_id"])
-          else
-            user = User.new(
-              name: "Unassigned",
-              organisation_id: attributes["managing_organisation_id"],
-              is_dpo: false,
-              encrypted_password: SecureRandom.hex(10),
-              email: SecureRandom.uuid,
-              confirmed_at: Time.zone.now,
-              active: false,
-            )
-            user.save!(validate: false)
-          end
+      user = LegacyUser.find_by(old_user_id: owner_id)&.user
+      if owner_id.blank? || user.blank? || (user.organisation_id != attributes["managing_organisation_id"] && user.organisation_id != attributes["owning_organisation_id"])
+        if owner_id.blank?
+          @logger.error("Lettings log '#{attributes['old_id']}' does not have the owner id. Assigning log to 'Unassigned' user.")
+        elsif user.blank?
+          @logger.error("Lettings log '#{attributes['old_id']}' belongs to legacy user with owner-user-id: '#{owner_id}' which cannot be found. Assigning log to 'Unassigned' user.")
+        else
+          @logger.error("Lettings log '#{attributes['old_id']}' belongs to legacy user with owner-user-id: '#{owner_id}' which belongs to a different organisation. Assigning log to 'Unassigned' user.")
+        end
+        if User.find_by(name: "Unassigned", organisation_id: attributes["managing_organisation_id"])
+          user = User.find_by(name: "Unassigned", organisation_id: attributes["managing_organisation_id"])
+        else
+          user = User.new(
+            name: "Unassigned",
+            organisation_id: attributes["managing_organisation_id"],
+            is_dpo: false,
+            encrypted_password: SecureRandom.hex(10),
+            email: SecureRandom.uuid,
+            confirmed_at: Time.zone.now,
+            active: false,
+          )
+          user.save!(validate: false)
         end
-
-        attributes["created_by"] = user
       end
+
+      attributes["created_by"] = user
       attributes["values_updated_at"] = Time.zone.now
 
       apply_date_consistency!(attributes)

app/services/imports/sales_logs_import_service.rb

@@ -185,30 +185,31 @@ module Imports
 
       # Sets the log creator
       owner_id = meta_field_value(xml_doc, "owner-user-id").strip
-      if owner_id.present?
-        user = LegacyUser.find_by(old_user_id: owner_id)&.user
-
-        if user.blank? || user.organisation_id != attributes["owning_organisation_id"]
-          if user.blank?
-            @logger.error("Sales log '#{attributes['old_id']}' belongs to legacy user with owner-user-id: '#{owner_id}' which cannot be found. Assigning log to 'Unassigned' user.")
-          else
-            @logger.error("Sales log '#{attributes['old_id']}' belongs to legacy user with owner-user-id: '#{owner_id}' which belongs to a different organisation. Assigning log to 'Unassigned' user.")
-          end
-          if User.find_by(name: "Unassigned", organisation_id: attributes["owning_organisation_id"])
-            user = User.find_by(name: "Unassigned", organisation_id: attributes["owning_organisation_id"])
-          else
-            user = User.new(
-              name: "Unassigned",
-              organisation_id: attributes["owning_organisation_id"],
-              is_dpo: false,
-              encrypted_password: SecureRandom.hex(10),
-              email: SecureRandom.uuid,
-              confirmed_at: Time.zone.now,
-              active: false,
-            )
-            user.save!(validate: false)
-          end
+      user = LegacyUser.find_by(old_user_id: owner_id)&.user
+
+      if owner_id.blank? || user.blank? || user.organisation_id != attributes["owning_organisation_id"]
+        if owner_id.blank?
+          @logger.error("Sales log '#{attributes['old_id']}' does not have the owner id. Assigning log to 'Unassigned' user.")
+        elsif user.blank?
+          @logger.error("Sales log '#{attributes['old_id']}' belongs to legacy user with owner-user-id: '#{owner_id}' which cannot be found. Assigning log to 'Unassigned' user.")
+        else
+          @logger.error("Sales log '#{attributes['old_id']}' belongs to legacy user with owner-user-id: '#{owner_id}' which belongs to a different organisation. Assigning log to 'Unassigned' user.")
         end
+        if User.find_by(name: "Unassigned", organisation_id: attributes["owning_organisation_id"])
+          user = User.find_by(name: "Unassigned", organisation_id: attributes["owning_organisation_id"])
+        else
+          user = User.new(
+            name: "Unassigned",
+            organisation_id: attributes["owning_organisation_id"],
+            is_dpo: false,
+            encrypted_password: SecureRandom.hex(10),
+            email: SecureRandom.uuid,
+            confirmed_at: Time.zone.now,
+            active: false,
+          )
+          user.save!(validate: false)
+        end
+
         attributes["created_by"] = user
       end
       attributes["values_updated_at"] = Time.zone.now

spec/services/imports/lettings_logs_import_service_spec.rb

@@ -172,6 +172,30 @@ RSpec.describe Imports::LettingsLogsImportService do
         end
       end
 
+      context "and the user is not provided" do
+        before { lettings_log_xml.at_xpath("//meta:owner-user-id").content = nil }
+
+        it "creates a new unassigned user" do
+          expect(logger).to receive(:error).with("Lettings log '0ead17cb-1668-442d-898c-0d52879ff592' does not have the owner id. Assigning log to 'Unassigned' user.")
+          lettings_log_service.send(:create_log, lettings_log_xml)
+
+          lettings_log = LettingsLog.where(old_id: lettings_log_id).first
+          expect(lettings_log&.created_by&.name).to eq("Unassigned")
+        end
+
+        it "only creates one unassigned user" do
+          expect(logger).to receive(:error).with("Lettings log '0ead17cb-1668-442d-898c-0d52879ff592' does not have the owner id. Assigning log to 'Unassigned' user.")
+          expect(logger).to receive(:error).with("Lettings log 'fake_id' does not have the owner id. Assigning log to 'Unassigned' user.")
+          lettings_log_service.send(:create_log, lettings_log_xml)
+          lettings_log_xml.at_xpath("//meta:document-id").content = "fake_id"
+          lettings_log_service.send(:create_log, lettings_log_xml)
+
+          lettings_log = LettingsLog.where(old_id: lettings_log_id).first
+          second_lettings_log = LettingsLog.where(old_id: "fake_id").first
+          expect(lettings_log&.created_by).to eq(second_lettings_log&.created_by)
+        end
+      end
+
       context "and the user exists on a different organisation" do
         before do
           create(:legacy_user, old_user_id: "fake_id")

spec/services/imports/sales_logs_field_import_service_spec.rb

@@ -159,7 +159,7 @@ RSpec.describe Imports::SalesLogsFieldImportService do
       Imports::SalesLogsImportService.new(storage_service, logger).create_logs(fixture_directory)
       old_log_id
       sales_log_file.rewind
-      sales_log.update!(values_updated_at: nil)
+      sales_log.update!(created_by: sales_log.owning_organisation.users.first, values_updated_at: nil)
     end
 
     context "when the sales log has created_by value" do

spec/services/imports/sales_logs_import_service_spec.rb

@@ -163,6 +163,32 @@ RSpec.describe Imports::SalesLogsImportService do
       end
     end
 
+    context "and the user is not given" do
+      let(:sales_log_id) { "shared_ownership_sales_log" }
+
+      before { sales_log_xml.at_xpath("//meta:owner-user-id").content = nil }
+
+      it "creates a new unassigned user" do
+        expect(logger).to receive(:error).with("Sales log 'shared_ownership_sales_log' does not have the owner id. Assigning log to 'Unassigned' user.")
+        sales_log_service.send(:create_log, sales_log_xml)
+
+        sales_log = SalesLog.where(old_id: sales_log_id).first
+        expect(sales_log&.created_by&.name).to eq("Unassigned")
+      end
+
+      it "only creates one unassigned user" do
+        expect(logger).to receive(:error).with("Sales log 'shared_ownership_sales_log' does not have the owner id. Assigning log to 'Unassigned' user.")
+        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_service.send(:create_log, sales_log_xml)
+
+        sales_log = SalesLog.where(old_id: sales_log_id).first
+        second_sales_log = SalesLog.where(old_id: "fake_id").first
+        expect(sales_log&.created_by).to eq(second_sales_log&.created_by)
+      end
+    end
+
     context "and the user exists on a different organisation" do
       let(:sales_log_id) { "shared_ownership_sales_log" }