From b938e394be8f130021b8dee2c645413be71eaa46 Mon Sep 17 00:00:00 2001 From: James Rose Date: Fri, 23 Sep 2022 11:54:11 +0100 Subject: [PATCH] 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 | 5 ++++ app/models/user.rb | 7 +++++- .../imports/lettings_logs_import_service.rb | 2 +- app/services/imports/user_import_service.rb | 8 +++--- .../20220923093628_create_legacy_users.rb | 12 +++++++++ db/schema.rb | 22 ++++++++++------ spec/factories/legacy_user.rb | 6 +++++ spec/factories/user.rb | 11 +++++++- spec/models/user_spec.rb | 10 +++++++- spec/requests/users_controller_spec.rb | 2 +- .../imports/user_import_service_spec.rb | 25 +++++++++++++------ 11 files changed, 88 insertions(+), 22 deletions(-) create mode 100644 app/models/legacy_user.rb create mode 100644 db/migrate/20220923093628_create_legacy_users.rb 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..707a8e41c 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,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 diff --git a/app/services/imports/lettings_logs_import_service.rb b/app/services/imports/lettings_logs_import_service.rb index 97be11dc2..e9d44a87c 100644 --- a/app/services/imports/lettings_logs_import_service.rb +++ b/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) 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..0af743586 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 @@ -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| diff --git a/spec/factories/legacy_user.rb b/spec/factories/legacy_user.rb new file mode 100644 index 000000000..b1f5578d9 --- /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..ce286f8e7 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, evaulator| + FactoryBot.create(:legacy_user, old_user_id: evaulator.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..bad70e31e 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,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 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..1257679ff 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