dev/core#2047 Fix merge code so that deleted contacts are not left without a primary address #18555
Conversation
|
(Standard links)
|
e5fd7bf to
98facda
Compare
1e9382e to
9654a75
Compare
|
@JKingsnorth @pfigel any chance one of you could review this? |
|
@eileenmcnaughton a bit low on bandwidth for the next 2 weeks or so, can review after if no one's gotten to it |
|
thanks @pfigel |
|
@jitendrapurohit (cc @petednz) could I get you to review this & the other dedupe PR? |
9654a75 to
346243c
Compare
|
@pfigel no one else jumped in :-( Do you have a chance to look ? |
|
can look at this today. |
|
@eileenmcnaughton Have tested the following usecase Contact A: (Duplicate) Contact B: Used the email only rule to merge the above contacts A => B and selected the home address to move to contact B. Before the patch: After merge, Contact A is deleted (correct), Contact A is only left with no primary address. Billing has is_primary = 0. After the patch Same behaviour. Billing address is on contact A but is_primary is not updated on this address. Contact A is left with no primary address. Also get the below e-notice after the merge.
|
CRM/Dedupe/MergeHandler.php
Outdated
| public function getBlocksToUpdateForDeletedContact($entity) { | ||
| $movedBlocks = $this->getLocationBlocksToMerge()[$entity]; | ||
| $deletedContactsBlocks = $this->getLocationBlocksForContactToRemove()[$entity]; | ||
| $unMovedBlocks = array_diff_key($deletedContactsBlocks, $movedBlocks); |
There was a problem hiding this comment.
If this line is updated to
$unMovedBlocks = array_values(array_diff_key($deletedContactsBlocks, $movedBlocks));
it passes the above testing.
There was a problem hiding this comment.
Thanks @jitendrapurohit - I've just made that change - if it still passes tests then the flow I tested will still be fine
…thout a primary address This was picked up in tests to ensure that removing a line of code creating excessing queries would not cause regressions. https://lab.civicrm.org/dev/core/-/issues/2039 I considered altering the test to exclude deleted contacts but we run the risk that if a contact is undeleted they will have no primary address so I figured the integrity makes sense. Note there are a couple of queries in this code that can go (retrieving stuff we already have) - depending how I go on getting review on this & related tidy up I'll remove them in a later PR
346243c to
2203317
Compare
There was a problem hiding this comment.
think it is good to merge now. Confirmed that the above issue is fixed. Thanks @eileenmcnaughton
|
thanks @jitendrapurohit ! |
|
I have tested the use-case again mentioned by @jitendrapurohit and on multiple duplicate contacts. Looks good to me. Merging now. |
Overview
Fixes dedupe code so the deleted contact does not wind up with 1 or more address/ emails etc where none are marked primary.
This was picked up through adding test cover for https://lab.civicrm.org/dev/core/-/issues/2039
I considered altering the test to exclude deleted contacts but we run the risk that if a contact
is undeleted they will have no primary address so I figured the integrity makes sense.
Before
When a contact with 2 addresses is merged and only the primary is moved over the deleted contact has an address still, but it is not marked is_primary. If it is subsequently undeleted then there is an integrity issue
After
Ensure the deleted contact's remaining location entities have at least one primary
Technical Details
Note there are a couple of queries in this code that can go (retrieving stuff
we already have) - depending how I go on getting review on this & related tidy up I'll remove them
in a later PR
Comments
If #18552 is merged first I'll need to rebase. #18500 is also a related cleanup