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