Fix duplicate merge to not disregard zero values.

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

diff --git a/CRM/Dedupe/Merger.php b/CRM/Dedupe/Merger.php
index afb53df5f545cf88b5a0fc4ec3b46c6f7d9b71c0..a3431a9ec377ab9e388cf23ff12a5d696eefdef8 100644 (file)
--- a/CRM/Dedupe/Merger.php
+++ b/CRM/Dedupe/Merger.php
@@ -619,8 +619,11 @@ INNER JOIN  civicrm_membership membership2 ON membership1.membership_type_id = m
       }
       $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;
       }
     }

diff --git a/tests/phpunit/api/v3/JobTest.php b/tests/phpunit/api/v3/JobTest.php
index 9795dfb0f06d0586af21124871ab16c990fe72d8..b584cccd55430e48c1c311a8a142038a7360969b 100644 (file)
--- a/tests/phpunit/api/v3/JobTest.php
+++ b/tests/phpunit/api/v3/JobTest.php
@@ -77,6 +77,7 @@ class api_v3_JobTest extends CiviUnitTestCase {
     // 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', 'civicrm_phone'], TRUE);
   }
 
   /**
@@ -916,7 +917,57 @@ class api_v3_JobTest extends CiviUnitTestCase {
     $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' => 'tha_mouse@mouse.com'];
+    $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' => 'tha_mouse@mouse.com'];
+    $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']);
   }