Allow a user to have a link to many legacy users (#894)

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.
- Updates the user import service so that we create this association for new users
- Creates a Rake script to backfill the association for existing users

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_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
@@ -324,8 +332,8 @@ ActiveRecord::Schema[7.0].define(version: 2022_09_20_132907) do
     t.string "purchid"
     t.integer "type"
     t.integer "ownershipsch"
-    t.integer "jointpur"
     t.string "othtype"
+    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