Change type of legacy old_visible_id fields to strings (#986)

These fields are set using IDs from the previous CORE service. There was an incorrect assumption that these fields were integers so we were casting them as such. We have discovered that these fields are strings (e.g., `027`) so this change adjusts our types.

app/services/imports/lettings_logs_import_service.rb

@@ -205,8 +205,8 @@ module Imports
 
       # Supported Housing fields
       if attributes["needstype"] == GN_SH[:supported_housing]
-        location_old_visible_id = safe_string_as_integer(xml_doc, "_1cschemecode")
+        location_old_visible_id = string_or_nil(xml_doc, "_1cschemecode")
-        scheme_old_visible_id = safe_string_as_integer(xml_doc, "_1cmangroupcode")
+        scheme_old_visible_id = string_or_nil(xml_doc, "_1cmangroupcode")
 
         schemes = Scheme.where(old_visible_id: scheme_old_visible_id, owning_organisation_id: attributes["owning_organisation_id"])
         location = Location.find_by(old_visible_id: location_old_visible_id, scheme: schemes)
@@ -399,7 +399,7 @@ module Imports
     end
 
     def find_organisation_id(xml_doc, id_field)
-      old_visible_id = unsafe_string_as_integer(xml_doc, id_field)
+      old_visible_id = string_or_nil(xml_doc, id_field)
       organisation = Organisation.find_by(old_visible_id:)
       raise "Organisation not found with legacy ID #{old_visible_id}" if organisation.nil?
 

app/services/imports/scheme_location_import_service.rb

@@ -91,7 +91,7 @@ module Imports
       attributes["units"] = safe_string_as_integer(xml_doc, "total-units")
       attributes["type_of_unit"] = safe_string_as_integer(xml_doc, "unit-type")
       attributes["location_old_id"] = string_or_nil(xml_doc, "id")
-      attributes["location_old_visible_id"] = safe_string_as_integer(xml_doc, "visible-id")
+      attributes["location_old_visible_id"] = string_or_nil(xml_doc, "visible-id")
       attributes["scheme_old_id"] = string_or_nil(xml_doc, "mgmtgroup")
       attributes
     end

db/migrate/20221111102656_change_old_visible_id_type.rb

@@ -0,0 +1,13 @@
+class ChangeOldVisibleIdType < ActiveRecord::Migration[7.0]
+  def up
+    change_column :organisations, :old_visible_id, :string
+    change_column :schemes, :old_visible_id, :string
+    change_column :locations, :old_visible_id, :string
+  end
+
+  def down
+    change_column :organisations, :old_visible_id, :integer
+    change_column :schemes, :old_visible_id, :integer
+    change_column :locations, :old_visible_id, :integer
+  end
+end

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_11_09_122033) do
+ActiveRecord::Schema[7.0].define(version: 2022_11_11_102656) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
 
@@ -255,7 +255,7 @@ ActiveRecord::Schema[7.0].define(version: 2022_11_09_122033) do
     t.integer "units"
     t.integer "type_of_unit"
     t.string "old_id"
-    t.integer "old_visible_id"
+    t.string "old_visible_id"
     t.string "mobility_type"
     t.datetime "startdate", precision: nil
     t.string "location_admin_district"
@@ -316,7 +316,7 @@ ActiveRecord::Schema[7.0].define(version: 2022_11_09_122033) do
     t.integer "supported_housing_units"
     t.integer "unspecified_units"
     t.string "old_org_id"
-    t.integer "old_visible_id"
+    t.string "old_visible_id"
     t.index ["old_visible_id"], name: "index_organisations_on_old_visible_id", unique: true
   end
 
@@ -395,7 +395,7 @@ ActiveRecord::Schema[7.0].define(version: 2022_11_09_122033) do
     t.bigint "managing_organisation_id"
     t.string "arrangement_type"
     t.string "old_id"
-    t.integer "old_visible_id"
+    t.string "old_visible_id"
     t.integer "total_units"
     t.boolean "confirmed"
     t.index ["managing_organisation_id"], name: "index_schemes_on_managing_organisation_id"

spec/factories/location.rb

@@ -17,7 +17,7 @@ FactoryBot.define do
       units { 20 }
       mobility_type { "A" }
       scheme { FactoryBot.create(:scheme, :export) }
-      old_visible_id { 111 }
+      old_visible_id { "111" }
     end
   end
 end

spec/fixtures/imports/logs/0b4a68df-30cc-474a-93c0-a56ce8fdad3b.xml

@@ -19,7 +19,7 @@
     <FORM>123456</FORM>
     <Landlord source-value="2">2 Local Authority</Landlord>
     <Group>
-      <_1cmangroupcode>123</_1cmangroupcode>
+      <_1cmangroupcode>0123</_1cmangroupcode>
       <_1cschemecode>10</_1cschemecode>
       <schPC/>
       <Q1e>3 No</Q1e>

spec/fixtures/imports/mgmtgroups/6d6d7618b58affe2a150a5ef2e9f4765fa6cd05d.xml

@@ -5,5 +5,5 @@
     <mgmtgroup:agent>456</mgmtgroup:agent>
     <mgmtgroup:institution>7c5bd5fb549c09z2c55d9cb90d7ba84927e64618</mgmtgroup:institution>
     <mgmtgroup:status>Approved</mgmtgroup:status>
-    <mgmtgroup:visible-id>123</mgmtgroup:visible-id>
+    <mgmtgroup:visible-id>0123</mgmtgroup:visible-id>
 </mgmtgroup:mgmtgroup>

spec/services/imports/lettings_logs_import_service_spec.rb

@@ -11,8 +11,8 @@ RSpec.describe Imports::LettingsLogsImportService do
   let(:fixture_directory) { "spec/fixtures/imports/logs" }
 
   let(:organisation) { FactoryBot.create(:organisation, old_visible_id: "1", provider_type: "PRP") }
-  let(:scheme1) { FactoryBot.create(:scheme, old_visible_id: 123, owning_organisation: organisation) }
+  let(:scheme1) { FactoryBot.create(:scheme, old_visible_id: "0123", owning_organisation: organisation) }
-  let(:scheme2) { FactoryBot.create(:scheme, old_visible_id: 456, owning_organisation: organisation) }
+  let(:scheme2) { FactoryBot.create(:scheme, old_visible_id: "456", owning_organisation: organisation) }
 
   def open_file(directory, filename)
     File.open("#{directory}/#{filename}.xml")
@@ -23,16 +23,16 @@ RSpec.describe Imports::LettingsLogsImportService do
            .to_return(status: 200, body: '{"status":200,"result":{"admin_district":"Westminster","codes":{"admin_district":"E08000035"}}}', headers: {})
 
     allow(Organisation).to receive(:find_by).and_return(nil)
-    allow(Organisation).to receive(:find_by).with(old_visible_id: organisation.old_visible_id.to_i).and_return(organisation)
+    allow(Organisation).to receive(:find_by).with(old_visible_id: organisation.old_visible_id).and_return(organisation)
 
     # Created by users
     FactoryBot.create(:user, old_user_id: "c3061a2e6ea0b702e6f6210d5c52d2a92612d2aa", organisation:)
     FactoryBot.create(:user, old_user_id: "e29c492473446dca4d50224f2bb7cf965a261d6f", organisation:)
 
     # Location setup
-    FactoryBot.create(:location, old_visible_id: 10, postcode: "LS166FT", scheme_id: scheme1.id, mobility_type: "W")
+    FactoryBot.create(:location, old_visible_id: "10", postcode: "LS166FT", scheme_id: scheme1.id, mobility_type: "W")
     FactoryBot.create(:location, scheme_id: scheme1.id)
-    FactoryBot.create(:location, old_visible_id: 10, postcode: "LS166FT", scheme_id: scheme2.id, mobility_type: "W")
+    FactoryBot.create(:location, old_visible_id: "10", postcode: "LS166FT", scheme_id: scheme2.id, mobility_type: "W")
 
     # Stub the form handler to use the real form
     allow(FormHandler.instance).to receive(:get_form).with("previous_lettings").and_return(real_2021_2022_form)

spec/services/imports/organisation_import_service_spec.rb

@@ -32,7 +32,7 @@ RSpec.describe Imports::OrganisationImportService do
     it "successfully create an organisation with the expected data" do
       import_service.create_organisations(folder_name)
 
-      organisation = Organisation.find_by(old_visible_id: 1)
+      organisation = Organisation.find_by(old_visible_id: "1")
       expect(organisation.name).to eq("HA Ltd")
       expect(organisation.provider_type).to eq("PRP")
       expect(organisation.phone).to eq("xxxxxxxx")
@@ -53,7 +53,7 @@ RSpec.describe Imports::OrganisationImportService do
       expect(organisation.unspecified_units).to eq(0)
       expect(organisation.unspecified_units).to eq(0)
       expect(organisation.old_org_id).to eq("7c5bd5fb549c09z2c55d9cb90d7ba84927e64618")
-      expect(organisation.old_visible_id).to eq(1)
+      expect(organisation.old_visible_id).to eq("1")
     end
 
     it "successfully create multiple organisations" do
@@ -62,8 +62,8 @@ RSpec.describe Imports::OrganisationImportService do
       expect(storage_service).to receive(:get_file_io).with(filenames[1]).ordered
 
       expect { import_service.create_organisations(folder_name) }.to change(Organisation, :count).by(2)
-      expect(Organisation).to exist(old_visible_id: 1)
+      expect(Organisation).to exist(old_visible_id: "1")
-      expect(Organisation).to exist(old_visible_id: 2)
+      expect(Organisation).to exist(old_visible_id: "2")
     end
   end
 
@@ -86,7 +86,7 @@ RSpec.describe Imports::OrganisationImportService do
       expect { import_service.create_organisations(folder_name) }.to change(Organisation, :count).by(1)
       expect { import_service.create_organisations(folder_name) }.to change(Organisation, :count).by(0)
 
-      expect(Organisation).to exist(old_visible_id: 1, name: "HA Ltd")
+      expect(Organisation).to exist(old_visible_id: "1", name: "HA Ltd")
     end
   end
 end

spec/services/imports/scheme_import_service_spec.rb

@@ -10,7 +10,7 @@ RSpec.describe Imports::SchemeImportService do
   let(:scheme_id) { "6d6d7618b58affe2a150a5ef2e9f4765fa6cd05d" }
 
   let!(:owning_org) { FactoryBot.create(:organisation, old_org_id: "7c5bd5fb549c09z2c55d9cb90d7ba84927e64618") }
-  let!(:managing_org) { FactoryBot.create(:organisation, old_visible_id: 456) }
+  let!(:managing_org) { FactoryBot.create(:organisation, old_visible_id: "456") }
 
   def open_file(directory, filename)
     File.open("#{directory}/#{filename}.xml")
@@ -46,7 +46,7 @@ RSpec.describe Imports::SchemeImportService do
       expect(scheme.owning_organisation).to eq(owning_org)
       expect(scheme.managing_organisation).to eq(managing_org)
       expect(scheme.old_id).to eq("6d6d7618b58affe2a150a5ef2e9f4765fa6cd05d")
-      expect(scheme.old_visible_id).to eq(123)
+      expect(scheme.old_visible_id).to eq("0123")
       expect(scheme.service_name).to eq("Management Group")
       expect(scheme.arrangement_type).to eq("Another organisation")
     end

spec/services/imports/scheme_location_import_service_spec.rb

@@ -142,7 +142,7 @@ RSpec.describe Imports::SchemeLocationImportService do
       expect(location.mobility_type).to eq("Fitted with equipment and adaptations")
       expect(location.type_of_unit).to eq("Bungalow")
       expect(location.old_id).to eq(first_location_id)
-      expect(location.old_visible_id).to eq(10)
+      expect(location.old_visible_id).to eq("10")
       expect(location.startdate).to eq("1900-01-01")
       expect(location.scheme).to eq(scheme)
     end