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/csv/lettings_log_csv_service.rb b/app/services/csv/lettings_log_csv_service.rb index 101cd7e6c..79e333770 100644 --- a/app/services/csv/lettings_log_csv_service.rb +++ b/app/services/csv/lettings_log_csv_service.rb @@ -1,6 +1,6 @@ module Csv class LettingsLogCsvService - CSV_FIELDS_TO_OMIT = %w[hhmemb net_income_value_check sale_or_letting first_time_property_let_as_social_housing renttype needstype postcode_known is_la_inferred totchild totelder totadult net_income_known is_carehome previous_la_known is_previous_la_inferred age1_known age2_known age3_known age4_known age5_known age6_known age7_known age8_known letting_allocation_unknown details_known_2 details_known_3 details_known_4 details_known_5 details_known_6 details_known_7 details_known_8 rent_type wrent wscharge wpschrge wsupchrg wtcharge wtshortfall rent_value_check old_form_id old_id retirement_value_check tshortfall_known pregnancy_value_check hhtype new_old vacdays].freeze + CSV_FIELDS_TO_OMIT = %w[hhmemb net_income_value_check sale_or_letting first_time_property_let_as_social_housing renttype needstype postcode_known is_la_inferred totchild totelder totadult net_income_known is_carehome previous_la_known is_previous_la_inferred age1_known age2_known age3_known age4_known age5_known age6_known age7_known age8_known letting_allocation_unknown details_known_2 details_known_3 details_known_4 details_known_5 details_known_6 details_known_7 details_known_8 rent_type wrent wscharge wpschrge wsupchrg wtcharge wtshortfall rent_value_check old_form_id old_id retirement_value_check tshortfall_known pregnancy_value_check hhtype new_old vacdays la prevloc].freeze def initialize(user) @user = user 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 5eb6c7e27..f3c4778eb 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,6 +11,7 @@ # It's strongly recommended that you check this file into your version control system. ActiveRecord::Schema[7.0].define(version: 2022_09_21_125813) do + # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -42,6 +43,14 @@ ActiveRecord::Schema[7.0].define(version: 2022_09_21_125813) 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 +333,9 @@ ActiveRecord::Schema[7.0].define(version: 2022_09_21_125813) do t.string "purchid" t.integer "type" t.integer "ownershipsch" - t.integer "jointpur" t.string "othtype" t.integer "beds" + 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/fixtures/files/lettings_logs_download_non_support.csv b/spec/fixtures/files/lettings_logs_download_non_support.csv index 3aeb62131..a4815fb53 100644 --- a/spec/fixtures/files/lettings_logs_download_non_support.csv +++ b/spec/fixtures/files/lettings_logs_download_non_support.csv @@ -1,2 +1,2 @@ -id,status,created_at,updated_at,created_by_name,is_dpo,owning_organisation_name,managing_organisation_name,renewal,startdate,irproduct_other,tenancycode,propcode,age1,sex1,ecstat1,relat2,age2,sex2,ecstat2,armedforces,leftreg,illness,housingneeds_a,housingneeds_b,housingneeds_c,housingneeds_h,prevloc_label,prevloc,illness_type_1,illness_type_2,la_label,la,postcode_full,wchair,preg_occ,cbl,earnings,incfreq,benefits,hb,period,brent,scharge,pscharge,supcharg,tcharge,offered,layear,ppostcode_full,mrcdate,declaration,ethnic,national,prevten,age3,sex3,ecstat3,age4,sex4,ecstat4,age5,sex5,ecstat5,age6,sex6,ecstat6,age7,sex7,ecstat7,age8,sex8,ecstat8,homeless,underoccupation_benefitcap,reservist,startertenancy,tenancylength,tenancy,rsnvac,unittype_gn,beds,waityear,reasonpref,chr,cap,reasonother,housingneeds_f,housingneeds_g,illness_type_3,illness_type_4,illness_type_8,illness_type_5,illness_type_6,illness_type_7,illness_type_9,illness_type_10,rp_homeless,rp_insan_unsat,rp_medwel,rp_hardship,rp_dontknow,tenancyother,property_owner_organisation,property_manager_organisation,purchaser_code,reason,majorrepairs,hbrentshortfall,property_relet,incref,sale_completion_date,unitletas,builtype,voiddate,lettype,nocharge,household_charge,referral,tshortfall,chcharge,ppcodenk,ethnic_group,ethnic_other,has_benefits,refused,housingneeds,wchchrg,newprop,relat3,relat4,relat5,relat6,relat7,relat8,lar,irproduct,joint,illness_type_0,sheltered,major_repairs_date_value_check,void_date_value_check,housingneeds_type,housingneeds_other,unittype_sh,scheme_code,scheme_service_name,scheme_sensitive,scheme_type,scheme_registered_under_care_act,scheme_owning_organisation_name,scheme_managing_organisation_name,scheme_primary_client_group,scheme_has_other_client_group,scheme_secondary_client_group,scheme_support_type,scheme_intended_stay,scheme_created_at,location_code,location_postcode,location_name,location_units,location_type_of_unit,location_mobility_type,location_admin_district,location_startdate -{id},in_progress,2022-02-08 16:52:15 +0000,2022-02-08 16:52:15 +0000,Danny Rojas,No,DLUHC,DLUHC,,,,,,,,,,,,,,,,,,,,,,,,Westminster,E09000033,SE1 1TE,2,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,0,,,,,,,,0,0,,,,,,,,,,,,,,,,,,,6,{scheme_code},{scheme_service_name},{scheme_sensitive},Missing,No,DLUHC,DLUHC,{scheme_primary_client_group},,{scheme_secondary_client_group},{scheme_support_type},{scheme_intended_stay},2022-06-05 01:00:00 +0100,{location_code},SE1 1TE,Downing Street,20,Bungalow,Fitted with equipment and adaptations,Westminster,{location_startdate} +id,status,created_at,updated_at,created_by_name,is_dpo,owning_organisation_name,managing_organisation_name,renewal,startdate,irproduct_other,tenancycode,propcode,age1,sex1,ecstat1,relat2,age2,sex2,ecstat2,armedforces,leftreg,illness,housingneeds_a,housingneeds_b,housingneeds_c,housingneeds_h,prevloc_label,illness_type_1,illness_type_2,la_label,postcode_full,wchair,preg_occ,cbl,earnings,incfreq,benefits,hb,period,brent,scharge,pscharge,supcharg,tcharge,offered,layear,ppostcode_full,mrcdate,declaration,ethnic,national,prevten,age3,sex3,ecstat3,age4,sex4,ecstat4,age5,sex5,ecstat5,age6,sex6,ecstat6,age7,sex7,ecstat7,age8,sex8,ecstat8,homeless,underoccupation_benefitcap,reservist,startertenancy,tenancylength,tenancy,rsnvac,unittype_gn,beds,waityear,reasonpref,chr,cap,reasonother,housingneeds_f,housingneeds_g,illness_type_3,illness_type_4,illness_type_8,illness_type_5,illness_type_6,illness_type_7,illness_type_9,illness_type_10,rp_homeless,rp_insan_unsat,rp_medwel,rp_hardship,rp_dontknow,tenancyother,property_owner_organisation,property_manager_organisation,purchaser_code,reason,majorrepairs,hbrentshortfall,property_relet,incref,sale_completion_date,unitletas,builtype,voiddate,lettype,nocharge,household_charge,referral,tshortfall,chcharge,ppcodenk,ethnic_group,ethnic_other,has_benefits,refused,housingneeds,wchchrg,newprop,relat3,relat4,relat5,relat6,relat7,relat8,lar,irproduct,joint,illness_type_0,sheltered,major_repairs_date_value_check,void_date_value_check,housingneeds_type,housingneeds_other,unittype_sh,scheme_code,scheme_service_name,scheme_sensitive,scheme_type,scheme_registered_under_care_act,scheme_owning_organisation_name,scheme_managing_organisation_name,scheme_primary_client_group,scheme_has_other_client_group,scheme_secondary_client_group,scheme_support_type,scheme_intended_stay,scheme_created_at,location_code,location_postcode,location_name,location_units,location_type_of_unit,location_mobility_type,location_admin_district,location_startdate +{id},in_progress,2022-02-08 16:52:15 +0000,2022-02-08 16:52:15 +0000,Danny Rojas,No,DLUHC,DLUHC,,,,,,,,,,,,,,,,,,,,,,,Westminster,SE1 1TE,2,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,0,,,,,,,,0,0,,,,,,,,,,,,,,,,,,,6,{scheme_code},{scheme_service_name},{scheme_sensitive},Missing,No,DLUHC,DLUHC,{scheme_primary_client_group},,{scheme_secondary_client_group},{scheme_support_type},{scheme_intended_stay},2022-06-05 01:00:00 +0100,{location_code},SE1 1TE,Downing Street,20,Bungalow,Fitted with equipment and adaptations,Westminster,{location_startdate} 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