From 99c4e78f886308056ea9123470936ee80a42f6d7 Mon Sep 17 00:00:00 2001 From: Samuel Young Date: Tue, 28 Oct 2025 12:37:19 +0000 Subject: [PATCH] CLDC-4094: Fix associating log locations after merge (#3112) * CLDC-4094: Use IDs to reassociate logs after a merge using names was unsafe as these are not guaranteed unique in an organisation or scheme build a list of old to new log IDs as merging schemes and locations, then use this when merging lettings logs * CLDC-4094: Add a verifying test --- .../merge/merge_organisations_service.rb | 8 ++++-- .../merge/merge_organisations_service_spec.rb | 25 +++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/app/services/merge/merge_organisations_service.rb b/app/services/merge/merge_organisations_service.rb index f87d811a4..ef7418d85 100644 --- a/app/services/merge/merge_organisations_service.rb +++ b/app/services/merge/merge_organisations_service.rb @@ -4,6 +4,8 @@ class Merge::MergeOrganisationsService @merging_organisations = Organisation.find(merging_organisation_ids) @merge_date = merge_date || Time.zone.today @absorbing_organisation_active_from_merge_date = absorbing_organisation_active_from_merge_date + @pre_to_post_merge_scheme_ids = {} + @pre_to_post_merge_location_ids = {} end def call @@ -70,12 +72,14 @@ private merging_organisation.owned_schemes.each do |scheme| new_scheme = Scheme.new(scheme.attributes.except("id", "owning_organisation_id", "old_id", "old_visible_id").merge(owning_organisation: @absorbing_organisation, startdate: [scheme&.startdate, @merge_date].compact.max)) new_scheme.save!(validate: false) + @pre_to_post_merge_scheme_ids[scheme.id] = new_scheme.id scheme.scheme_deactivation_periods.each do |deactivation_period| split_scheme_deactivation_period_between_organisations(deactivation_period, new_scheme) end scheme.locations.each do |location| new_location = Location.new(location.attributes.except("id", "scheme_id", "old_id", "old_visible_id").merge(scheme: new_scheme, startdate: [location&.startdate, @merge_date].compact.max)) new_location.save!(validate: false) + @pre_to_post_merge_location_ids[location.id] = new_location.id location.location_deactivation_periods.each do |deactivation_period| split_location_deactivation_period_between_organisations(deactivation_period, new_location) end @@ -98,8 +102,8 @@ private lettings_log.managing_organisation = @absorbing_organisation end if lettings_log.scheme.present? - scheme_to_set = @absorbing_organisation.owned_schemes.find_by(service_name: lettings_log.scheme.service_name) - location_to_set = scheme_to_set.locations.find_by(name: lettings_log.location&.name, postcode: lettings_log.location&.postcode) + scheme_to_set = @absorbing_organisation.owned_schemes.find_by(id: @pre_to_post_merge_scheme_ids[lettings_log.scheme.id]) + location_to_set = scheme_to_set.locations.find_by(id: @pre_to_post_merge_location_ids[lettings_log.location&.id]) lettings_log.scheme = scheme_to_set if scheme_to_set.present? # in some cases the lettings_log location is nil even if scheme is present (they're two different questions). diff --git a/spec/services/merge/merge_organisations_service_spec.rb b/spec/services/merge/merge_organisations_service_spec.rb index 2be375c17..84e0ebbe1 100644 --- a/spec/services/merge/merge_organisations_service_spec.rb +++ b/spec/services/merge/merge_organisations_service_spec.rb @@ -728,6 +728,31 @@ RSpec.describe Merge::MergeOrganisationsService do expect(incomplete_lettings_log.location).to be_nil end + it "associates correct lettings logs with correct locations which share a name and postcode" do + scheme = create(:scheme, owning_organisation: merging_organisation) + location_0 = create(:location, scheme:, name: "duplicate name", postcode: "EE1 1EE", location_admin_district: "dist1", startdate: Time.zone.today) + location_1 = create(:location, scheme:, name: "duplicate name", postcode: "EE1 1EE", location_admin_district: "dist2", startdate: Time.zone.today) + location_2 = create(:location, scheme:, name: "duplicate name", postcode: "EE1 1EE", location_admin_district: "dist3", startdate: Time.zone.today) + + lettings_log_0 = build(:lettings_log, scheme:, owning_organisation: merging_organisation, startdate: Time.zone.today, location: location_0) + lettings_log_0.save!(validate: false) + lettings_log_1 = build(:lettings_log, scheme:, owning_organisation: merging_organisation, startdate: Time.zone.today, location: location_1) + lettings_log_1.save!(validate: false) + lettings_log_2 = build(:lettings_log, scheme:, owning_organisation: merging_organisation, startdate: Time.zone.today, location: location_2) + lettings_log_2.save!(validate: false) + + expect(Rails.logger).not_to receive(:error) + merge_organisations_service.call + + lettings_log_0.reload + lettings_log_1.reload + lettings_log_2.reload + + expect(lettings_log_0.location.location_admin_district).to eq("dist1") + expect(lettings_log_1.location.location_admin_district).to eq("dist2") + expect(lettings_log_2.location.location_admin_district).to eq("dist3") + end + context "with merge date in closed collection year" do subject(:merge_organisations_service) { described_class.new(absorbing_organisation_id: absorbing_organisation.id, merging_organisation_ids:, merge_date: Time.zone.local(2021, 3, 3)) }