From 8f9886cbc8538a99ed334cbe22244c96d6cb7cf4 Mon Sep 17 00:00:00 2001 From: James Rose Date: Fri, 11 Nov 2022 11:30:15 +0000 Subject: [PATCH] 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. --- .../imports/lettings_logs_import_service.rb | 6 +++--- .../imports/scheme_location_import_service.rb | 2 +- .../20221111102656_change_old_visible_id_type.rb | 13 +++++++++++++ db/schema.rb | 8 ++++---- spec/factories/location.rb | 2 +- .../logs/0b4a68df-30cc-474a-93c0-a56ce8fdad3b.xml | 2 +- .../6d6d7618b58affe2a150a5ef2e9f4765fa6cd05d.xml | 2 +- .../imports/lettings_logs_import_service_spec.rb | 10 +++++----- .../imports/organisation_import_service_spec.rb | 10 +++++----- spec/services/imports/scheme_import_service_spec.rb | 4 ++-- .../imports/scheme_location_import_service_spec.rb | 2 +- 11 files changed, 37 insertions(+), 24 deletions(-) create mode 100644 db/migrate/20221111102656_change_old_visible_id_type.rb diff --git a/app/services/imports/lettings_logs_import_service.rb b/app/services/imports/lettings_logs_import_service.rb index 1aecc4f81..ab15e7f3d 100644 --- a/app/services/imports/lettings_logs_import_service.rb +++ b/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") - scheme_old_visible_id = safe_string_as_integer(xml_doc, "_1cmangroupcode") + location_old_visible_id = string_or_nil(xml_doc, "_1cschemecode") + 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? diff --git a/app/services/imports/scheme_location_import_service.rb b/app/services/imports/scheme_location_import_service.rb index 4214555cc..837be0b49 100644 --- a/app/services/imports/scheme_location_import_service.rb +++ b/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 diff --git a/db/migrate/20221111102656_change_old_visible_id_type.rb b/db/migrate/20221111102656_change_old_visible_id_type.rb new file mode 100644 index 000000000..d4c568bb7 --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index ea4b59670..5faff28ba 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_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" diff --git a/spec/factories/location.rb b/spec/factories/location.rb index a8418f8ba..3359f64cd 100644 --- a/spec/factories/location.rb +++ b/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 diff --git a/spec/fixtures/imports/logs/0b4a68df-30cc-474a-93c0-a56ce8fdad3b.xml b/spec/fixtures/imports/logs/0b4a68df-30cc-474a-93c0-a56ce8fdad3b.xml index 59c1eada7..3faacf28b 100644 --- a/spec/fixtures/imports/logs/0b4a68df-30cc-474a-93c0-a56ce8fdad3b.xml +++ b/spec/fixtures/imports/logs/0b4a68df-30cc-474a-93c0-a56ce8fdad3b.xml @@ -19,7 +19,7 @@
123456
2 Local Authority - <_1cmangroupcode>123 + <_1cmangroupcode>0123 <_1cschemecode>10 3 No diff --git a/spec/fixtures/imports/mgmtgroups/6d6d7618b58affe2a150a5ef2e9f4765fa6cd05d.xml b/spec/fixtures/imports/mgmtgroups/6d6d7618b58affe2a150a5ef2e9f4765fa6cd05d.xml index 5a7b23b5b..712d3f6e5 100644 --- a/spec/fixtures/imports/mgmtgroups/6d6d7618b58affe2a150a5ef2e9f4765fa6cd05d.xml +++ b/spec/fixtures/imports/mgmtgroups/6d6d7618b58affe2a150a5ef2e9f4765fa6cd05d.xml @@ -5,5 +5,5 @@ 456 7c5bd5fb549c09z2c55d9cb90d7ba84927e64618 Approved - 123 + 0123 diff --git a/spec/services/imports/lettings_logs_import_service_spec.rb b/spec/services/imports/lettings_logs_import_service_spec.rb index 576249971..453ee7e30 100644 --- a/spec/services/imports/lettings_logs_import_service_spec.rb +++ b/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(:scheme2) { FactoryBot.create(:scheme, old_visible_id: 456, 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) } 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) diff --git a/spec/services/imports/organisation_import_service_spec.rb b/spec/services/imports/organisation_import_service_spec.rb index 48a0d72bb..c8fcd4829 100644 --- a/spec/services/imports/organisation_import_service_spec.rb +++ b/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: 2) + expect(Organisation).to exist(old_visible_id: "1") + 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 diff --git a/spec/services/imports/scheme_import_service_spec.rb b/spec/services/imports/scheme_import_service_spec.rb index efba81a1c..97e9359b5 100644 --- a/spec/services/imports/scheme_import_service_spec.rb +++ b/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 diff --git a/spec/services/imports/scheme_location_import_service_spec.rb b/spec/services/imports/scheme_location_import_service_spec.rb index 8b93d4969..c11c2ca2d 100644 --- a/spec/services/imports/scheme_location_import_service_spec.rb +++ b/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