From 3437f9725e4908cb473f3aad46955e6c31f09c89 Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Fri, 1 Sep 2023 14:04:38 +0200 Subject: [PATCH] CLDC-2753 Add unassigned user for imported logs (#1883) * Add unassigned user if we can't find a legacy user for a log * Add generate_unassigned_logs_report * Log user email address if validation fails on user import --- app/services/imports/import_report_service.rb | 31 +++++++++++++++++++ .../imports/lettings_logs_import_service.rb | 17 ++++++++++ .../imports/sales_logs_import_service.rb | 17 ++++++++++ app/services/imports/user_import_service.rb | 12 +++++-- .../imports/import_report_service_spec.rb | 31 +++++++++++++++++++ .../lettings_logs_import_service_spec.rb | 24 ++++++++++++++ .../imports/sales_logs_import_service_spec.rb | 26 ++++++++++++++++ .../imports/user_import_service_spec.rb | 16 +++++++++- 8 files changed, 170 insertions(+), 4 deletions(-) diff --git a/app/services/imports/import_report_service.rb b/app/services/imports/import_report_service.rb index 61d43a9eb..4e387c4c4 100644 --- a/app/services/imports/import_report_service.rb +++ b/app/services/imports/import_report_service.rb @@ -11,6 +11,7 @@ module Imports def create_reports(report_suffix) generate_missing_data_coordinators_report(report_suffix) generate_logs_report(report_suffix) + generate_unassigned_logs_report(report_suffix) end def generate_missing_data_coordinators_report(report_suffix) @@ -53,5 +54,35 @@ module Imports @logger.info("Logs report available in s3 import bucket at #{report_name}") end + + def generate_unassigned_logs_report(report_suffix) + Rails.logger.info("Generating unassigned logs report") + + rep = CSV.generate do |report| + headers = ["Owning Organisation ID", "Old Owning Organisation ID", "Managing Organisation ID", "Old Managing Organisation ID", "Log ID", "Old Log ID", "Tenancy code", "Purchaser code"] + report << headers + + @institutions_csv.each do |row| + name = row[0] + organisation = Organisation.find_by(name:) + next unless organisation + + unassigned_user = organisation.users.find_by(name: "Unassigned") + next unless unassigned_user + + organisation.owned_lettings_logs.where(created_by: unassigned_user).each do |lettings_log| + report << [organisation.id, organisation.old_org_id, lettings_log.managing_organisation.id, lettings_log.managing_organisation.old_org_id, lettings_log.id, lettings_log.old_id, lettings_log.tenancycode, nil] + end + organisation.owned_sales_logs.where(created_by: unassigned_user).each do |sales_log| + report << [organisation.id, organisation.old_org_id, nil, nil, sales_log.id, sales_log.old_id, nil, sales_log.purchid] + end + end + end + + report_name = "UnassignedLogsReport_#{report_suffix}" + @storage_service.write_file(report_name, BYTE_ORDER_MARK + rep) + + @logger.info("Unassigned logs report available in s3 import bucket at #{report_name}") + end end end diff --git a/app/services/imports/lettings_logs_import_service.rb b/app/services/imports/lettings_logs_import_service.rb index 8bcaba27b..7b749524a 100644 --- a/app/services/imports/lettings_logs_import_service.rb +++ b/app/services/imports/lettings_logs_import_service.rb @@ -251,6 +251,23 @@ module Imports 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? + @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.") + if User.find_by(name: "Unassigned") + user = User.find_by(name: "Unassigned") + 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 + end attributes["created_by"] = user end diff --git a/app/services/imports/sales_logs_import_service.rb b/app/services/imports/sales_logs_import_service.rb index 0d780c87d..723159198 100644 --- a/app/services/imports/sales_logs_import_service.rb +++ b/app/services/imports/sales_logs_import_service.rb @@ -179,6 +179,23 @@ module Imports if owner_id.present? user = LegacyUser.find_by(old_user_id: owner_id)&.user + 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.") + if User.find_by(name: "Unassigned") + user = User.find_by(name: "Unassigned") + 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 + end attributes["created_by"] = user end diff --git a/app/services/imports/user_import_service.rb b/app/services/imports/user_import_service.rb index b0fe32bef..e280dcfe1 100644 --- a/app/services/imports/user_import_service.rb +++ b/app/services/imports/user_import_service.rb @@ -39,9 +39,15 @@ module Imports user.active = user_field_value(xml_document, "active") user.skip_confirmation_notification! - user.save! - user.legacy_users.create!(old_user_id:) - user + + begin + user.save! + user.legacy_users.create!(old_user_id:) + user + rescue ActiveRecord::RecordInvalid => e + @logger.error(e.message) + @logger.error("Could not save user with email: #{email}") + end end end diff --git a/spec/services/imports/import_report_service_spec.rb b/spec/services/imports/import_report_service_spec.rb index 1d241754d..abd318a8d 100644 --- a/spec/services/imports/import_report_service_spec.rb +++ b/spec/services/imports/import_report_service_spec.rb @@ -67,4 +67,35 @@ RSpec.describe Imports::ImportReportService do report_service.generate_logs_report("report_suffix.csv") end end + + describe "#generate_unassigned_logs_report" do + context "when there is no unassigned user (all the logs have ben assigned)" do + let(:institutions_csv) { CSV.parse("Institution name,Id,Old Completed lettings logs,Old In progress lettings logs,Old Completed sales logs,Old In progress sales logs\norg1,1,2,1,4,3", headers: true) } + + it "writes an empty unassigned logs report" do + expect(storage_service).to receive(:write_file).with("UnassignedLogsReport_report_suffix.csv", "\uFEFFOwning Organisation ID,Old Owning Organisation ID,Managing Organisation ID,Old Managing Organisation ID,Log ID,Old Log ID,Tenancy code,Purchaser code\n") + + report_service.generate_unassigned_logs_report("report_suffix.csv") + end + end + + context "when some logs have been added to Unassigned user" do + let(:organisation) { create(:organisation, old_org_id: "1", name: "org1") } + let(:organisation2) { create(:organisation, old_org_id: "2", name: "org2") } + let(:unassigned_user) { create(:user, name: "Unassigned", organisation:) } + let(:institutions_csv) { CSV.parse("Institution name,Id,Old Completed lettings logs,Old In progress lettings logs,Old Completed sales logs,Old In progress sales logs\norg1,1,2,1,4,3", headers: true) } + let!(:lettings_log) { create(:lettings_log, owning_organisation: organisation, managing_organisation: organisation2, created_by: unassigned_user, tenancycode: "tenancycode", old_id: "12") } + let!(:sales_log) { create(:sales_log, owning_organisation: organisation, created_by: unassigned_user, purchid: "purchid", old_id: "23") } + + before do + create(:organisation_relationship, parent_organisation: organisation, child_organisation: organisation2) + end + + it "writes a report with all unassigned logs" do + expect(storage_service).to receive(:write_file).with("UnassignedLogsReport_report_suffix.csv", "\uFEFFOwning Organisation ID,Old Owning Organisation ID,Managing Organisation ID,Old Managing Organisation ID,Log ID,Old Log ID,Tenancy code,Purchaser code\n#{organisation.id},1,#{organisation2.id},2,#{lettings_log.id},12,tenancycode,\n#{organisation.id},1,,,#{sales_log.id},23,,purchid\n") + + report_service.generate_unassigned_logs_report("report_suffix.csv") + end + end + end end diff --git a/spec/services/imports/lettings_logs_import_service_spec.rb b/spec/services/imports/lettings_logs_import_service_spec.rb index d055db112..a95cdac5b 100644 --- a/spec/services/imports/lettings_logs_import_service_spec.rb +++ b/spec/services/imports/lettings_logs_import_service_spec.rb @@ -135,6 +135,30 @@ RSpec.describe Imports::LettingsLogsImportService do let(:lettings_log_file) { open_file(fixture_directory, lettings_log_id) } let(:lettings_log_xml) { Nokogiri::XML(lettings_log_file) } + context "and the user does not exist" do + before { lettings_log_xml.at_xpath("//meta:owner-user-id").content = "fake_id" } + + it "creates a new unassigned user" do + expect(logger).to receive(:error).with("Lettings log '0ead17cb-1668-442d-898c-0d52879ff592' belongs to legacy user with owner-user-id: 'fake_id' which cannot be found. 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' belongs to legacy user with owner-user-id: 'fake_id' which cannot be found. Assigning log to 'Unassigned' user.") + expect(logger).to receive(:error).with("Lettings log 'fake_id' belongs to legacy user with owner-user-id: 'fake_id' which cannot be found. 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 void date is after the start date" do before { lettings_log_xml.at_xpath("//xmlns:VYEAR").content = 2023 } diff --git a/spec/services/imports/sales_logs_import_service_spec.rb b/spec/services/imports/sales_logs_import_service_spec.rb index 740614775..8f12be392 100644 --- a/spec/services/imports/sales_logs_import_service_spec.rb +++ b/spec/services/imports/sales_logs_import_service_spec.rb @@ -116,6 +116,32 @@ RSpec.describe Imports::SalesLogsImportService do end end + context "and the user does not exist" do + let(:sales_log_id) { "shared_ownership_sales_log" } + + before { sales_log_xml.at_xpath("//meta:owner-user-id").content = "fake_id" } + + it "creates a new unassigned user" do + expect(logger).to receive(:error).with("Sales log 'shared_ownership_sales_log' belongs to legacy user with owner-user-id: 'fake_id' which cannot be found. 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' belongs to legacy user with owner-user-id: 'fake_id' which cannot be found. Assigning log to 'Unassigned' user.") + expect(logger).to receive(:error).with("Sales log 'fake_id' belongs to legacy user with owner-user-id: 'fake_id' which cannot be found. 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 log startdate is before 22/23 collection period" do let(:sales_log_id) { "shared_ownership_sales_log" } diff --git a/spec/services/imports/user_import_service_spec.rb b/spec/services/imports/user_import_service_spec.rb index ea3388169..5e56b2782 100644 --- a/spec/services/imports/user_import_service_spec.rb +++ b/spec/services/imports/user_import_service_spec.rb @@ -44,10 +44,24 @@ RSpec.describe Imports::UserImportService do end it "refuses to create a user belonging to a non existing organisation" do - expect(logger).to receive(:error).with(/ActiveRecord::RecordInvalid/) + expect(logger).to receive(:error).with(/Could not save user with email: john.doe@gov.uk/) + expect(logger).to receive(:error).with(/Validation failed: Organisation Select the user’s organisation/) import_service.create_users("user_directory") end + context "when the user with the same email already exists" do + before do + create(:organisation, old_org_id:) + create(:user, email: "john.doe@gov.uk") + end + + it "logs an error and user email" do + expect(logger).to receive(:error).with(/Could not save user with email: john.doe@gov.uk/) + expect(logger).to receive(:error).with(/Validation failed: email Enter an email address that hasn’t already been used to sign up/) + import_service.create_users("user_directory") + end + end + context "when the user is a data coordinator" do let(:old_user_id) { "d4729b1a5dfb68bb1e01c08445830c0add40907c" }