From 363113900acf926efc1760f2880ae9b179be36dd Mon Sep 17 00:00:00 2001 From: Samuel Young Date: Thu, 19 Jun 2025 13:57:42 +0100 Subject: [PATCH] CLDC-4039: Fix merge functionality not working (#3080) * ensure nil locations are never overwritten * add a verifying test * fixup! add a verifying test remove unneeded context * fixup! add a verifying test clarify test structure --- .../merge/merge_organisations_service.rb | 6 +++++- .../merge/merge_organisations_service_spec.rb | 21 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/app/services/merge/merge_organisations_service.rb b/app/services/merge/merge_organisations_service.rb index c831c875f..f87d811a4 100644 --- a/app/services/merge/merge_organisations_service.rb +++ b/app/services/merge/merge_organisations_service.rb @@ -102,7 +102,11 @@ private location_to_set = scheme_to_set.locations.find_by(name: lettings_log.location&.name, postcode: lettings_log.location&.postcode) lettings_log.scheme = scheme_to_set if scheme_to_set.present? - lettings_log.location = location_to_set if location_to_set.present? + # in some cases the lettings_log location is nil even if scheme is present (they're two different questions). + # in certain cases the location_to_set query can find a location in the scheme with nil values for name and postcode, + # so we can end up setting the location to non nil. + # hence the extra check here + lettings_log.location = location_to_set if location_to_set.present? && lettings_log.location.present? end lettings_log.owning_organisation = @absorbing_organisation lettings_log.managing_organisation = @absorbing_organisation if lettings_log.managing_organisation == merging_organisation diff --git a/spec/services/merge/merge_organisations_service_spec.rb b/spec/services/merge/merge_organisations_service_spec.rb index 84016a9b4..2be375c17 100644 --- a/spec/services/merge/merge_organisations_service_spec.rb +++ b/spec/services/merge/merge_organisations_service_spec.rb @@ -707,6 +707,27 @@ RSpec.describe Merge::MergeOrganisationsService do expect(owned_lettings_log.managing_organisation).to eq(absorbing_organisation) end + it "does not change the lettings log location" do + scheme = create(:scheme, owning_organisation: merging_organisation) + create(:location, scheme:, name: nil, postcode: nil) + # necessary to have a couple valid locations else the scheme will be invalid + create(:location, scheme:) + create(:location, scheme:) + incomplete_lettings_log = build(:lettings_log, scheme:, owning_organisation: merging_organisation, startdate: Time.zone.today) + incomplete_lettings_log.save!(validate: false) + + # if the location is overwritten with the nil one above, it will fail validation + # since a rollback will occur incomplete_lettings_log will not change so there's nothing to verify later + # so instead we verify that no rollback occurs + expect(Rails.logger).not_to receive(:error) + merge_organisations_service.call + + incomplete_lettings_log.reload + + # also ensure it wasn't overwritten with a valid location + expect(incomplete_lettings_log.location).to be_nil + 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)) }