Let users have many legacy user IDs

While migrating users from the previous service to the new one we discovered that email addresses are not unique in the previous service. This means that one user in the new service might relate to multiple users in the previous service.

This change adds a new `LegacyUser` model that can belong to a `User`. Each `LegacyUser` model has one `old_user_id` that corresponds to the user ID in the legacy service.

When importing users we now create this association.

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,11 @@ class User < ApplicationRecord
   end
 
   def was_migrated_from_softwire?
-    old_user_id.present?
+    legacy_users.any?
+  end
+
+  def old_user_id
+    legacy_users&.first&.old_user_id || read_attribute(:old_user_id)
   end
 
   def send_confirmation_instructions

app/services/imports/lettings_logs_import_service.rb

@@ -226,7 +226,7 @@ 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)
+        attributes["created_by"] = LegacyUser.find_by(old_user_id: owner_id)&.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_20_132907) 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_20_132907) 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
@@ -232,12 +240,12 @@ ActiveRecord::Schema[7.0].define(version: 2022_09_20_132907) do
     t.integer "void_date_value_check"
     t.integer "housingneeds_type"
     t.integer "housingneeds_other"
-    t.index ["created_by_id"], name: "index_lettings_logs_on_created_by_id"
-    t.index ["location_id"], name: "index_lettings_logs_on_location_id"
-    t.index ["managing_organisation_id"], name: "index_lettings_logs_on_managing_organisation_id"
-    t.index ["old_id"], name: "index_lettings_logs_on_old_id", unique: true
-    t.index ["owning_organisation_id"], name: "index_lettings_logs_on_owning_organisation_id"
-    t.index ["scheme_id"], name: "index_lettings_logs_on_scheme_id"
+    t.index ["created_by_id"], name: "index_case_logs_on_created_by_id"
+    t.index ["location_id"], name: "index_case_logs_on_location_id"
+    t.index ["managing_organisation_id"], name: "index_case_logs_on_managing_organisation_id"
+    t.index ["old_id"], name: "index_case_logs_on_old_id", unique: true
+    t.index ["owning_organisation_id"], name: "index_case_logs_on_owning_organisation_id"
+    t.index ["scheme_id"], name: "index_case_logs_on_scheme_id"
   end
 
   create_table "locations", force: :cascade do |t|

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, evaulator|
+      FactoryBot.create(:legacy_user, old_user_id: evaulator.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,14 @@ 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.old_user_id).to eq("3")
+
+      user.legacy_users.destroy_all
+      user.update!(old_user_id: "2")
+      expect(user.old_user_id).to eq("2")
+    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