Fix duplicate merge to not disregard zero values. #12669
Conversation
|
(Standard links)
|
eileenmcnaughton
force-pushed
the
dedupe_zero
branch
3 times, most recently
from
c1f3e07 to
6fa6791
Compare
CRM/Dedupe/Merger.php
Outdated
| // However I can't think of a case where an empty string is more meaningful than null | ||
| // or where it would be FALSE or something else nullish. | ||
| $valuesToIgnore = [NULL, '']; | ||
| if (!in_array($key1, $valuesToIgnore) || !in_array($key2, $valuesToIgnore)) { |
There was a problem hiding this comment.
let's use the 'strict' argument to in_array, and add the empty array to $valuesToIgnore
There was a problem hiding this comment.
@ejegg good thinking - done. Also port in gerrit
|
@eileenmcnaughton the test failures i think might be related https://test.civicrm.org/job/CiviCRM-Core-PR/22120/ |
eileenmcnaughton
force-pushed
the
dedupe_zero
branch
from
6fa6791 to
493bf43
Compare
Currently a field with 0 in it will not be carried across in a merge. In fact in a checkbox situation the differenct between 0 and null is significant (I chose not to opt in vs I was never presented with a choice). This patch preserves the 0. I would note that further digging is required to see how a confict is handled - ie. if one contact has 0 & the other has 1 is this blocked as a confligct Additional test on conflict handling
eileenmcnaughton
force-pushed
the
dedupe_zero
branch
from
493bf43 to
26b7ff2
Compare
|
@JKingsnorth @deepak-srivastava any takers for review? |
|
@eileenmcnaughton I'll review this tomorrow [= |
|
thanks @JKingsnorth |
|
Sorry, something came up, first on my list tomorrow. |
|
I'm currently unable to recreate the original issue, by doing:
So I've opened a PR with just the tests to investigate further: #12750 |
|
@eileenmcnaughton - weird, the tests you wrote passed without the patch! So presumably we can keep the code as-is (unless you can dig out the steps to recreate the issue you were experiencing) and just add the tests? |
|
@eileenmcnaughton What's the status with this one? |
|
@eileenmcnaughton @JKingsnorth @mattwire I have moved and modified the UTs in a separate PR to capture the two use-cases. This is the #13323 which is expected to fail without this patch. Please have a look |
|
I am merging this PR as it fixes the issue. As per the UT concern, I made the desired change in UT (PR #13323) to capture this use-case. After merge I will rebase my PR for merge-on-pass, to show this fix worked for |
|
Thanks @monishdeb |
Overview
Fix merge process to treat 0 as different to NULL
Before
A contact who has selected 0 for a checkbox field is merged to a contact who has a NULL value. The 0 value is not transferred over.
After
A contact who has selected 0 for a checkbox field is merged to a contact who has a NULL value. The 0 value is transferred over.
Technical Details
In a checkbox situation the difference between
0 and null is significant (I chose not to opt in vs
I was never presented with a choice).
I also added a test to check that a conflict between 0 & 1 is treated as a conflict (it turns out it is)
ping @JKingsnorth