Merge branch 'main' into CLDC-1465-bedrooms-in-property

app/models/legacy_user.rb

@@ -0,0 +1,5 @@
+class LegacyUser < ApplicationRecord
+  belongs_to :user
+
+  validates :old_user_id, uniqueness: true
+end

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

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

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)

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
 

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

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"

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

spec/factories/legacy_user.rb

@@ -0,0 +1,6 @@
+FactoryBot.define do
+  factory :legacy_user do
+    old_user_id {}
+    user
+  end
+end

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

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,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,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},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}

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

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

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