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 2d42a1d77..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_21_154025) 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_21_154025) 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,9 +332,8 @@ ActiveRecord::Schema[7.0].define(version: 2022_09_21_154025) do t.string "purchid" t.integer "type" t.integer "ownershipsch" - t.integer "jointpur" t.string "othtype" - t.integer "companybuy" + 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