Merge branch 'main' into CLDC-1482-sales-log-buyer-company

# Conflicts:
#	db/schema.rb

app/models/legacy_user.rb

@@ -0,0 +1,5 @@
+class LegacyUser < ApplicationRecord
+  belongs_to :user
+
+  validates :old_user_id, uniqueness: true
+end

app/models/user.rb

@@ -11,6 +11,7 @@ class User < ApplicationRecord
   has_many :managed_lettings_logs, through: :organisation
   has_many :owned_sales_logs, through: :organisation
   has_many :managed_sales_logs, through: :organisation
+  has_many :legacy_users
 
   validates :name, presence: true
   validates :email, presence: true
@@ -113,7 +114,7 @@ class User < ApplicationRecord
   end
 
   def was_migrated_from_softwire?
-    old_user_id.present?
+    legacy_users.any? || old_user_id.present?
   end
 
   def send_confirmation_instructions

app/services/imports/lettings_logs_import_service.rb

@@ -226,7 +226,10 @@ module Imports
       # Sets the log creator
       owner_id = field_value(xml_doc, "meta", "owner-user-id").strip
       if owner_id.present?
-        attributes["created_by"] = User.find_by(old_user_id: owner_id)
+        user = LegacyUser.find_by(old_user_id: owner_id)&.user
+        @logger.warn "Missing user! We expected to find a legacy user with old_user_id #{owner_id}" unless user
+
+        attributes["created_by"] = user
       end
 
       apply_date_consistency!(attributes)

app/services/imports/user_import_service.rb

@@ -17,7 +17,7 @@ module Imports
       name = user_field_value(xml_document, "full-name") || email
       deleted = user_field_value(xml_document, "deleted")
 
-      if User.find_by(old_user_id:, organisation:)
+      if LegacyUser.find_by(old_user_id:)
         @logger.warn("User #{name} with old user id #{old_user_id} is already present, skipping.")
       elsif deleted == "true"
         @logger.warn("User #{name} with old user id #{old_user_id} is deleted, skipping.")
@@ -25,20 +25,22 @@ module Imports
         is_dpo = user.is_data_protection_officer? || is_dpo?(user_field_value(xml_document, "user-type"))
         role = highest_role(user.role, role(user_field_value(xml_document, "user-type")))
         user.update!(role:, is_dpo:)
+        user.legacy_users.create!(old_user_id:)
         @logger.info("Found duplicated email, updating user #{user.id} with role #{role} and is_dpo #{is_dpo}")
       else
-        User.create!(
+        user = User.create!(
           email:,
           name:,
           password: Devise.friendly_token,
           phone: user_field_value(xml_document, "telephone-no"),
-          old_user_id:,
           organisation:,
           role: role(user_field_value(xml_document, "user-type")),
           is_dpo: is_dpo?(user_field_value(xml_document, "user-type")),
           is_key_contact: is_key_contact?(user_field_value(xml_document, "contact-priority-id")),
           active: user_field_value(xml_document, "active"),
         )
+        user.legacy_users.create!(old_user_id:)
+        user
       end
     end
 

db/migrate/20220923093628_create_legacy_users.rb

@@ -0,0 +1,12 @@
+class CreateLegacyUsers < ActiveRecord::Migration[7.0]
+  def change
+    create_table :legacy_users do |t|
+      t.string :old_user_id
+      t.integer :user_id
+
+      t.timestamps
+    end
+
+    add_index :legacy_users, :old_user_id, unique: true
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.0].define(version: 2022_09_21_154025) do
+ActiveRecord::Schema[7.0].define(version: 2022_09_23_093628) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
 
@@ -42,6 +42,14 @@ ActiveRecord::Schema[7.0].define(version: 2022_09_21_154025) do
     t.index ["start_year", "lettype", "beds", "la"], name: "index_la_rent_ranges_on_start_year_and_lettype_and_beds_and_la", unique: true
   end
 
+  create_table "legacy_users", force: :cascade do |t|
+    t.string "old_user_id"
+    t.integer "user_id"
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
+    t.index ["old_user_id"], name: "index_legacy_users_on_old_user_id", unique: true
+  end
+
   create_table "lettings_logs", force: :cascade do |t|
     t.integer "status", default: 0
     t.datetime "created_at", null: false
@@ -324,9 +332,8 @@ ActiveRecord::Schema[7.0].define(version: 2022_09_21_154025) do
     t.string "purchid"
     t.integer "type"
     t.integer "ownershipsch"
-    t.integer "jointpur"
     t.string "othtype"
-    t.integer "companybuy"
+    t.integer "jointpur"
     t.index ["created_by_id"], name: "index_sales_logs_on_created_by_id"
     t.index ["managing_organisation_id"], name: "index_sales_logs_on_managing_organisation_id"
     t.index ["owning_organisation_id"], name: "index_sales_logs_on_owning_organisation_id"

lib/tasks/sync_legacy_users.rake

@@ -0,0 +1,11 @@
+namespace :core do
+  # TODO: Remove once ran on all environments.
+  desc "Creates a LegacyUser object for any existing Users"
+  task sync_legacy_users: :environment do
+    User.where.not(old_user_id: nil).includes(:legacy_users).find_each do |user|
+      next if user.legacy_users.where(old_user_id: user.old_user_id).any?
+
+      user.legacy_users.create!(old_user_id: user.old_user_id)
+    end
+  end
+end

spec/factories/legacy_user.rb

@@ -0,0 +1,6 @@
+FactoryBot.define do
+  factory :legacy_user do
+    old_user_id {}
+    user
+  end
+end

spec/factories/user.rb

@@ -5,7 +5,6 @@ FactoryBot.define do
     password { "pAssword1" }
     organisation
     role { "data_provider" }
-    old_user_id { 2 }
     trait :data_coordinator do
       role { "data_coordinator" }
     end
@@ -19,5 +18,15 @@ FactoryBot.define do
     confirmed_at { Time.zone.now }
     created_at { Time.zone.now }
     updated_at { Time.zone.now }
+
+    transient do
+      old_user_id { SecureRandom.uuid }
+    end
+
+    after(:create) do |user, evaluator|
+      FactoryBot.create(:legacy_user, old_user_id: evaluator.old_user_id, user:)
+
+      user.reload
+    end
   end
 end

spec/models/user_spec.rb

@@ -2,7 +2,7 @@ require "rails_helper"
 
 RSpec.describe User, type: :model do
   describe "#new" do
-    let(:user) { FactoryBot.create(:user) }
+    let(:user) { FactoryBot.create(:user, old_user_id: "3") }
     let(:other_organisation) { FactoryBot.create(:organisation) }
     let!(:owned_lettings_log) do
       FactoryBot.create(
@@ -74,6 +74,10 @@ RSpec.describe User, type: :model do
       expect(user.need_two_factor_authentication?(nil)).to be false
     end
 
+    it "can have one or more legacy users" do
+      expect(user.legacy_users.size).to eq(1)
+    end
+
     it "is confirmable" do
       allow(DeviseNotifyMailer).to receive(:confirmation_instructions).and_return(OpenStruct.new(deliver: true))
       expect(DeviseNotifyMailer).to receive(:confirmation_instructions).once

spec/requests/users_controller_spec.rb

@@ -1381,7 +1381,7 @@ RSpec.describe UsersController, type: :request do
           end
 
           before do
-            user.update(old_user_id: nil)
+            user.legacy_users.destroy_all
           end
 
           it "allows changing email and dpo" do

spec/services/imports/user_import_service_spec.rb

@@ -32,7 +32,7 @@ RSpec.describe Imports::UserImportService do
       FactoryBot.create(:organisation, old_org_id:)
       import_service.create_users("user_directory")
 
-      user = User.find_by(old_user_id:)
+      user = LegacyUser.find_by(old_user_id:)&.user
       expect(user.name).to eq("John Doe")
       expect(user.email).to eq("john.doe@gov.uk")
       expect(user.encrypted_password).not_to be_nil
@@ -54,7 +54,8 @@ RSpec.describe Imports::UserImportService do
       it "sets their role correctly" do
         FactoryBot.create(:organisation, old_org_id:)
         import_service.create_users("user_directory")
-        expect(User.find_by(old_user_id:)).to be_data_coordinator
+        user = LegacyUser.find_by(old_user_id:)&.user
+        expect(user).to be_data_coordinator
       end
     end
 
@@ -65,7 +66,7 @@ RSpec.describe Imports::UserImportService do
         FactoryBot.create(:organisation, old_org_id:)
         import_service.create_users("user_directory")
 
-        user = User.find_by(old_user_id:)
+        user = LegacyUser.find_by(old_user_id:)&.user
         expect(user.is_data_protection_officer?).to be true
       end
     end
@@ -77,7 +78,7 @@ RSpec.describe Imports::UserImportService do
         FactoryBot.create(:organisation, old_org_id:)
         import_service.create_users("user_directory")
 
-        user = User.find_by(old_user_id:)
+        user = LegacyUser.find_by(old_user_id:)&.user
         expect(user.is_key_contact?).to be true
       end
     end
@@ -89,7 +90,7 @@ RSpec.describe Imports::UserImportService do
         FactoryBot.create(:organisation, old_org_id:)
         import_service.create_users("user_directory")
 
-        user = User.find_by(old_user_id:)
+        user = LegacyUser.find_by(old_user_id:)&.user
         expect(user.is_key_contact?).to be true
       end
     end
@@ -118,10 +119,19 @@ RSpec.describe Imports::UserImportService do
           expect(user.reload).to be_data_coordinator
         end
 
-        it "does not create a new record" do
+        it "does not create a new user record" do
           expect { import_service.create_users("user_directory") }
             .not_to change(User, :count)
         end
+
+        it "creates a new legacy user record" do
+          expect { import_service.create_users("user_directory") }.to change(LegacyUser, :count)
+        end
+
+        it "associates the legacy user with the existing user" do
+          import_service.create_users("user_directory")
+          expect(LegacyUser.find_by(old_user_id:)&.user).to eq(user)
+        end
       end
 
       context "when the duplicate role is lower than the original role" do
@@ -159,7 +169,8 @@ RSpec.describe Imports::UserImportService do
 
         it "marks them as not active" do
           import_service.create_users("user_directory")
-          expect(User.find_by(old_user_id:).active).to be false
+          user = LegacyUser.find_by(old_user_id:)&.user
+          expect(user.active).to be false
         end
       end
     end