Skip to content

Commit

Permalink
Fix duplicate merge to not disregard zero values.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
eileenmcnaughton committed Aug 17, 2018
1 parent 0114e4a commit 493bf43
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 3 deletions.
7 changes: 5 additions & 2 deletions CRM/Dedupe/Merger.php
Original file line number Diff line number Diff line change
Expand Up @@ -619,8 +619,11 @@ public static function retrieveFields($main, $other) {
}
$key1 = CRM_Utils_Array::value($key, $mainEvs);
$key2 = CRM_Utils_Array::value($key, $otherEvs);
// CRM-17556 Get all non-empty fields, to make comparison easier
if (!empty($key1) || !empty($key2)) {
// We wish to retain '0' as it has a different meaning than NULL on a checkbox.
// 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, TRUE) || !in_array($key2, $valuesToIgnore, TRUE)) {
$result['custom'][] = $key;
}
}
Expand Down
53 changes: 52 additions & 1 deletion tests/phpunit/api/v3/JobTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ public function tearDown() {
// The membershipType create breaks transactions so this extra cleanup is needed.
$this->membershipTypeDelete(array('id' => $this->membershipTypeID));
$this->cleanUpSetUpIDs();
$this->quickCleanup(['civicrm_contact', 'civicrm_address', 'civicrm_email', 'civicrm_website'], TRUE);
}

/**
Expand Down Expand Up @@ -916,7 +917,57 @@ public function testBatchMergeCustomDataViewOnlyField() {
$mouse = $this->callAPISuccess('Contact', 'getsingle', $mouseParams);
$this->assertEquals('blah', $mouse['custom_' . $customField['id']]);

$this->customFieldDelete($customGroup['id']);
$this->customFieldDelete($customField['id']);
$this->customGroupDelete($customGroup['id']);
}

/**
* Test the batch merge retains 0 as a valid custom field value.
*
* Note that we set 0 on 2 fields with one on each contact to ensure that
* both merged & mergee fields are respected.
*/
public function testBatchMergeCustomDataZeroValueField() {
$customGroup = $this->CustomGroupCreate();
$customField = $this->customFieldCreate(array('custom_group_id' => $customGroup['id'], 'default_value' => NULL));
$customField2 = $this->customFieldCreate(array('custom_group_id' => $customGroup['id'], 'label' => 'field2', 'default_value' => NULL));

$mouseParams = ['first_name' => 'Mickey', 'last_name' => 'Mouse', 'email' => '[email protected]'];
$this->individualCreate(array_merge($mouseParams, ['custom_' . $customField['id'] => 0]));
$this->individualCreate(array_merge($mouseParams, ['custom_' . $customField2['id'] => 0]));

$result = $this->callAPISuccess('Job', 'process_batch_merge', array('check_permissions' => 0, 'mode' => 'safe'));
$this->assertEquals(1, count($result['values']['merged']));
$mouseParams['return'] = 'custom_' . $customField['id'];
$mouse = $this->callAPISuccess('Contact', 'getsingle', $mouseParams);
$this->assertEquals(0, $mouse['custom_' . $customField['id']]);

$this->customFieldDelete($customField['id']);
$this->customFieldDelete($customField2['id']);
$this->customGroupDelete($customGroup['id']);
}

/**
* Test the batch merge treats 0 vs 1 as a conflict.
*/
public function testBatchMergeCustomDataZeroValueFieldWithConflict() {
$customGroup = $this->CustomGroupCreate();
$customField = $this->customFieldCreate(array('custom_group_id' => $customGroup['id'], 'default_value' => NULL));

$mouseParams = ['first_name' => 'Mickey', 'last_name' => 'Mouse', 'email' => '[email protected]'];
$mouse1 = $this->individualCreate(array_merge($mouseParams, ['custom_' . $customField['id'] => 0]));
$mouse2 = $this->individualCreate(array_merge($mouseParams, ['custom_' . $customField['id'] => 1]));

$result = $this->callAPISuccess('Job', 'process_batch_merge', array('check_permissions' => 0, 'mode' => 'safe'));
$this->assertEquals(0, count($result['values']['merged']));

// Reverse which mouse has the zero to test we still get a conflict.
$this->individualCreate(array_merge($mouseParams, ['id' => $mouse1, 'custom_' . $customField['id'] => 1]));
$this->individualCreate(array_merge($mouseParams, ['id' => $mouse2, 'custom_' . $customField['id'] => 0]));
$result = $this->callAPISuccess('Job', 'process_batch_merge', array('check_permissions' => 0, 'mode' => 'safe'));
$this->assertEquals(0, count($result['values']['merged']));

$this->customFieldDelete($customField['id']);
$this->customGroupDelete($customGroup['id']);
}

Expand Down

0 comments on commit 493bf43

Please sign in to comment.