From 7fc74ca6d130e4299cb206a763da63ae59cbc2fc Mon Sep 17 00:00:00 2001 From: James Rose Date: Fri, 23 Sep 2022 12:49:19 +0100 Subject: [PATCH] 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 | 5 ++++ app/models/user.rb | 3 ++- .../imports/lettings_logs_import_service.rb | 5 +++- app/services/imports/user_import_service.rb | 8 +++--- .../20220923093628_create_legacy_users.rb | 12 +++++++++ db/schema.rb | 12 +++++++-- lib/tasks/sync_legacy_users.rake | 11 ++++++++ spec/factories/legacy_user.rb | 6 +++++ spec/factories/user.rb | 11 +++++++- spec/models/user_spec.rb | 6 ++++- spec/requests/users_controller_spec.rb | 2 +- .../imports/user_import_service_spec.rb | 25 +++++++++++++------ 12 files changed, 89 insertions(+), 17 deletions(-) create mode 100644 app/models/legacy_user.rb create mode 100644 db/migrate/20220923093628_create_legacy_users.rb create mode 100644 lib/tasks/sync_legacy_users.rake create mode 100644 spec/factories/legacy_user.rb diff --git a/app/models/legacy_user.rb b/app/models/legacy_user.rb new file mode 100644 index 000000000..dcab274a6 --- /dev/null +++ b/app/models/legacy_user.rb @@ -0,0 +1,5 @@ +class LegacyUser < ApplicationRecord + belongs_to :user + + validates :old_user_id, uniqueness: true +end diff --git a/app/models/user.rb b/app/models/user.rb index 56dd2821f..40a430489 100644 --- a/app/models/user.rb +++ b/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 diff --git a/app/services/imports/lettings_logs_import_service.rb b/app/services/imports/lettings_logs_import_service.rb index 97be11dc2..911da4ac3 100644 --- a/app/services/imports/lettings_logs_import_service.rb +++ b/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) diff --git a/app/services/imports/user_import_service.rb b/app/services/imports/user_import_service.rb index 1d1fbc84e..f5a730479 100644 --- a/app/services/imports/user_import_service.rb +++ b/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 diff --git a/db/migrate/20220923093628_create_legacy_users.rb b/db/migrate/20220923093628_create_legacy_users.rb new file mode 100644 index 000000000..a8f92545d --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 87714f00f..c2b54d354 100644 --- a/db/schema.rb +++ b/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" diff --git a/lib/tasks/sync_legacy_users.rake b/lib/tasks/sync_legacy_users.rake new file mode 100644 index 000000000..cff3dd34d --- /dev/null +++ b/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 diff --git a/spec/factories/legacy_user.rb b/spec/factories/legacy_user.rb new file mode 100644 index 000000000..a0396a30e --- /dev/null +++ b/spec/factories/legacy_user.rb @@ -0,0 +1,6 @@ +FactoryBot.define do + factory :legacy_user do + old_user_id {} + user + end +end diff --git a/spec/factories/user.rb b/spec/factories/user.rb index e021e0727..7f9b45932 100644 --- a/spec/factories/user.rb +++ b/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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index ed77046fd..450e334a7 100644 --- a/spec/models/user_spec.rb +++ b/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 diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 3168defa1..a404bbd97 100644 --- a/spec/requests/users_controller_spec.rb +++ b/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 diff --git a/spec/services/imports/user_import_service_spec.rb b/spec/services/imports/user_import_service_spec.rb index c4aea00bc..b8a831dbc 100644 --- a/spec/services/imports/user_import_service_spec.rb +++ b/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