From 5947d6a1911789bd18a653132667b09787009363 Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Tue, 19 Sep 2023 15:45:49 +0100 Subject: [PATCH] Add users to unassigned when the owner id is not given (#1929) * Add users to unassigned when the owner id is not given * test --- .../imports/lettings_logs_import_service.rb | 48 +++++++++---------- .../imports/sales_logs_import_service.rb | 47 +++++++++--------- .../lettings_logs_import_service_spec.rb | 24 ++++++++++ .../sales_logs_field_import_service_spec.rb | 2 +- .../imports/sales_logs_import_service_spec.rb | 26 ++++++++++ 5 files changed, 99 insertions(+), 48 deletions(-) diff --git a/app/services/imports/lettings_logs_import_service.rb b/app/services/imports/lettings_logs_import_service.rb index e8ed9a058..59d30172c 100644 --- a/app/services/imports/lettings_logs_import_service.rb +++ b/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) diff --git a/app/services/imports/sales_logs_import_service.rb b/app/services/imports/sales_logs_import_service.rb index 7848f8628..bc2a096bc 100644 --- a/app/services/imports/sales_logs_import_service.rb +++ b/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 diff --git a/spec/services/imports/lettings_logs_import_service_spec.rb b/spec/services/imports/lettings_logs_import_service_spec.rb index fa83f4d34..1dd5c5578 100644 --- a/spec/services/imports/lettings_logs_import_service_spec.rb +++ b/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") diff --git a/spec/services/imports/sales_logs_field_import_service_spec.rb b/spec/services/imports/sales_logs_field_import_service_spec.rb index 9bd7ce335..ef7762da8 100644 --- a/spec/services/imports/sales_logs_field_import_service_spec.rb +++ b/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 diff --git a/spec/services/imports/sales_logs_import_service_spec.rb b/spec/services/imports/sales_logs_import_service_spec.rb index 69e82ead8..a30b5bbf8 100644 --- a/spec/services/imports/sales_logs_import_service_spec.rb +++ b/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" }