Consolidate handling of conflicts between the batch job and get_conflicts api
authoreileen <emcnaughton@wikimedia.org>
Mon, 1 Jul 2019 02:56:08 +0000 (14:56 +1200)
committereileen <emcnaughton@wikimedia.org>
Mon, 1 Jul 2019 22:09:04 +0000 (10:09 +1200)
This ensures that conflicts are stored during batch_merge to the prev_next cache with the same format as when the api
calls get_conflicts. The code doing this wrangling is moved from the api to the BAO layer.

We add a test to ensure the output is the same & use the previously added test to check the string is the same too.

Test cover here is pretty good

CRM/Core/BAO/PrevNextCache.php
CRM/Dedupe/Merger.php
api/v3/Contact.php
tests/phpunit/api/v3/ContactTest.php
tests/phpunit/api/v3/JobTest.php

index a886e6d1849f3cf48bacb8168f5d18865fdf8189..416709e59a4fe73774ebe335a9588a293cc00dd7 100644 (file)
@@ -170,11 +170,12 @@ WHERE  cachekey     = %3 AND
    * @param int $id2
    * @param string $cacheKey
    * @param array $conflicts
+   * @param string $mode
    *
    * @return bool
    * @throws CRM_Core_Exception
    */
-  public static function markConflict($id1, $id2, $cacheKey, $conflicts) {
+  public static function markConflict($id1, $id2, $cacheKey, $conflicts, $mode) {
     if (empty($cacheKey) || empty($conflicts)) {
       return FALSE;
     }
@@ -193,15 +194,31 @@ WHERE  cachekey     = %3 AND
     $pncFind = CRM_Core_DAO::executeQuery($sql, $params);
 
     $conflictTexts = [];
-    foreach ($conflicts as $conflict) {
-      $conflictTexts[] = "{$conflict['title']}: '{$conflict[$id1]}' vs. '{$conflict[$id2]}'";
+
+    foreach ($conflicts as $entity => $entityConflicts) {
+      if ($entity === 'contact') {
+        foreach ($entityConflicts as $conflict) {
+          $conflictTexts[] = "{$conflict['title']}: '{$conflict[$id1]}' vs. '{$conflict[$id2]}'";
+        }
+      }
+      else {
+        foreach ($entityConflicts as $locationConflict) {
+          if (!is_array($locationConflict)) {
+            continue;
+          }
+          $displayField = CRM_Dedupe_Merger::getLocationBlockInfo()[$entity]['displayField'];
+          $conflictTexts[] = "{$locationConflict['title']}: '{$locationConflict[$displayField][$id1]}' vs. '{$locationConflict[$displayField][$id2]}'";
+        }
+      }
     }
     $conflictString = implode(', ', $conflictTexts);
+
     while ($pncFind->fetch()) {
       $data = $pncFind->data;
       if (!empty($data)) {
         $data = CRM_Core_DAO::unSerializeField($data, CRM_Core_DAO::SERIALIZE_PHP);
         $data['conflicts'] = $conflictString;
+        $data[$mode]['conflicts'] = $conflicts;
 
         $pncUp = new CRM_Core_DAO_PrevNextCache();
         $pncUp->id = $pncFind->id;
index 91d5a6c8828b60c2de733ace5787e179bd44853f..a57d64e1001de5254a66b4e6d665f8f2d5457e09 100644 (file)
@@ -919,22 +919,16 @@ INNER JOIN  civicrm_membership membership2 ON membership1.membership_type_id = m
    *  An empty array to be filed with conflict information.
    *
    * @return bool
+   *
+   * @throws \CRM_Core_Exception
+   * @throws \CiviCRM_API3_Exception
+   * @throws \API_Exception
    */
   public static function skipMerge($mainId, $otherId, &$migrationInfo, $mode = 'safe', &$conflicts = []) {
 
     $conflicts = self::getConflicts($migrationInfo, $mainId, $otherId, $mode);
 
     if (!empty($conflicts)) {
-      foreach ($conflicts as $key => $val) {
-        if ($val === NULL and $mode == 'safe') {
-          // un-resolved conflicts still present. Lets skip this merge after saving the conflict / reason.
-          return TRUE;
-        }
-        else {
-          // copy over the resolved values
-          $migrationInfo[$key] = $val;
-        }
-      }
       // if there are conflicts and mode is aggressive, allow hooks to decide if to skip merges
       return (bool) $migrationInfo['skip_merge'];
     }
@@ -950,15 +944,7 @@ INNER JOIN  civicrm_membership membership2 ON membership1.membership_type_id = m
    * @return bool
    */
   public static function locationIsSame($mainAddress, $comparisonAddress) {
-    $keysToIgnore = [
-      'id',
-      'is_primary',
-      'is_billing',
-      'manual_geo_code',
-      'contact_id',
-      'reset_date',
-      'hold_date',
-    ];
+    $keysToIgnore = self::ignoredFields();
     foreach ($comparisonAddress as $field => $value) {
       if (in_array($field, $keysToIgnore)) {
         continue;
@@ -2106,6 +2092,7 @@ INNER JOIN  civicrm_membership membership2 ON membership1.membership_type_id = m
    *
    * @throws \CRM_Core_Exception
    * @throws \CiviCRM_API3_Exception
+   * @throws \API_Exception
    */
   protected static function dedupePair(&$resultStats, &$deletedContacts, $mode, $checkPermissions, $mainId, $otherId, $cacheKeyString) {
 
@@ -2128,16 +2115,7 @@ INNER JOIN  civicrm_membership membership2 ON membership1.membership_type_id = m
 
     // store any conflicts
     if (!empty($conflicts)) {
-      foreach ($conflicts as $key => $dnc) {
-        unset($conflicts[$key]);
-        $conflicts[substr($key, 5)] = [
-          'title' => $migrationInfo['rows'][$key]['title'],
-          $mainId => $migrationInfo['rows'][$key]['main'],
-          $otherId => $migrationInfo['rows'][$key]['other'],
-          'key' => $key,
-        ];
-      }
-      CRM_Core_BAO_PrevNextCache::markConflict($mainId, $otherId, $cacheKeyString, $conflicts);
+      CRM_Core_BAO_PrevNextCache::markConflict($mainId, $otherId, $cacheKeyString, $conflicts, $mode);
     }
     else {
       CRM_Core_BAO_PrevNextCache::deletePair($mainId, $otherId, $cacheKeyString);
@@ -2426,8 +2404,69 @@ INNER JOIN  civicrm_membership membership2 ON membership1.membership_type_id = m
     $conflicts = $migrationData['fields_in_conflict'];
     // allow hook to override / manipulate migrationInfo as well
     $migrationInfo = $migrationData['migration_info'];
-    $migrationInfo['skip_merge'] = CRM_Utils_Array::value('skip_merge', $migrationData);
-    return $conflicts;
+    foreach ($conflicts as $key => $val) {
+      if ($val !== NULL || $mode !== 'safe') {
+        // copy over the resolved values
+        $migrationInfo[$key] = $val;
+        unset($conflicts[$key]);
+      }
+    }
+    $migrationInfo['skip_merge'] = $migrationData['skip_merge'] ?? !empty($conflicts);
+    return self::formatConflictArray($conflicts, $migrationInfo['rows'], $migrationInfo['main_details']['location_blocks'], $migrationInfo['other_details']['location_blocks'], $mainId, $otherId);
+  }
+
+  /**
+   * @param array $conflicts
+   * @param array $migrationInfo
+   * @param $toKeepContactLocationBlocks
+   * @param $toRemoveContactLocationBlocks
+   * @param $toKeepID
+   * @param $toRemoveID
+   *
+   * @return mixed
+   * @throws \CRM_Core_Exception
+   */
+  protected static function formatConflictArray($conflicts, $migrationInfo, $toKeepContactLocationBlocks, $toRemoveContactLocationBlocks, $toKeepID, $toRemoveID) {
+    $return = [];
+    foreach (array_keys($conflicts) as $index) {
+      if (substr($index, 0, 14) === 'move_location_') {
+        $parts = explode('_', $index);
+        $entity = $parts[2];
+        $blockIndex = $parts[3];
+        $locationTypeID = $toKeepContactLocationBlocks[$entity][$blockIndex]['location_type_id'];
+        $entityConflicts = [
+          'location_type_id' => $locationTypeID,
+          'title' => $migrationInfo[$index]['title'],
+        ];
+        foreach ($toKeepContactLocationBlocks[$entity][$blockIndex] as $fieldName => $fieldValue) {
+          if (in_array($fieldName, self::ignoredFields())) {
+            continue;
+          }
+          $toRemoveValue = CRM_Utils_Array::value($fieldName, $toRemoveContactLocationBlocks[$entity][$blockIndex]);
+          if ($fieldValue !== $toRemoveValue) {
+            $entityConflicts[$fieldName] = [
+              $toKeepID => $fieldValue,
+              $toRemoveID => $toRemoveValue,
+            ];
+          }
+        }
+        $return[$entity][] = $entityConflicts;
+      }
+      elseif (substr($index, 0, 5) === 'move_') {
+        $contactFieldsToCompare[] = str_replace('move_', '', $index);
+        $return['contact'][str_replace('move_', '', $index)] = [
+          'title' => $migrationInfo[$index]['title'],
+          $toKeepID => $migrationInfo[$index]['main'],
+          $toRemoveID => $migrationInfo[$index]['other'],
+        ];
+      }
+      else {
+        // Can't think of why this would be the case but perhaps it's ensuring it isn't as we
+        // refactor this.
+        throw new CRM_Core_Exception(ts('Unknown parameter') . $index);
+      }
+    }
+    return $return;
   }
 
   /**
@@ -2454,4 +2493,20 @@ INNER JOIN  civicrm_membership membership2 ON membership1.membership_type_id = m
     );
   }
 
+  /**
+   * @return array
+   */
+  protected static function ignoredFields(): array {
+    $keysToIgnore = [
+      'id',
+      'is_primary',
+      'is_billing',
+      'manual_geo_code',
+      'contact_id',
+      'reset_date',
+      'hold_date',
+    ];
+    return $keysToIgnore;
+  }
+
 }
index d5f6fc3613763f20ea1a63c75c506dfa6db926ad..3c2fc5bed28f07efda93bdbad960bf5de36a36e8 100644 (file)
@@ -1211,89 +1211,15 @@ function _civicrm_api3_contact_merge_spec(&$params) {
  */
 function civicrm_api3_contact_get_merge_conflicts($params) {
   $migrationInfo = [];
-  $contactFieldsToCompare = [];
-  $entitiesToCompare = [];
-  $return = [];
+  $result = [];
   foreach ((array) $params['mode'] as $mode) {
-    $result[$mode] = CRM_Dedupe_Merger::getConflicts(
+    $result[$mode]['conflicts'] = CRM_Dedupe_Merger::getConflicts(
       $migrationInfo,
       $params['to_remove_id'], $params['to_keep_id'],
-      $params['mode']
+      $mode
     );
-    $return = [];
-    foreach (array_keys($result[$mode]) as $index) {
-      if (substr($index, 0, 14) === 'move_location_') {
-        $parts = explode('_', $index);
-        $entity = $parts[2];
-        $locationTypeID = $migrationInfo['location_blocks'][$entity][$parts[3]]['locTypeId'];
-        $return[$mode]['conflicts'][$entity][] = ['location_type_id' => $locationTypeID];
-        $entitiesToCompare[$entity][] = $locationTypeID;
-      }
-      elseif (substr($index, 0, 5) === 'move_') {
-        $contactFieldsToCompare[] = str_replace('move_', '', $index);
-        $return[$mode]['conflicts']['contact'][0][str_replace('move_', '', $index)] = [];
-      }
-      else {
-        // Can't think of why this would be the case but perhaps it's ensuring it isn't as we
-        // refactor this.
-        throw new API_Exception(ts('Unknown parameter') . $index);
-      }
-    }
-  }
-  // We get the contact & location details once, now we know what we need for both modes (if both being fetched).
-  $contacts = civicrm_api3('Contact', 'get', [
-    'id' => [
-      'IN' => [
-        $params['to_keep_id'],
-        $params['to_remove_id'],
-      ],
-    ],
-    'return' => $contactFieldsToCompare,
-  ])['values'];
-  foreach ($contactFieldsToCompare as $fieldName) {
-    foreach ((array) $params['mode'] as $mode) {
-      if (isset($return[$mode]['conflicts']['contact'][0][$fieldName])) {
-        $return[$mode]['conflicts']['contact'][0][$fieldName][$params['to_keep_id']] = CRM_Utils_Array::value($fieldName, $contacts[$params['to_keep_id']]);
-        $return[$mode]['conflicts']['contact'][0][$fieldName][$params['to_remove_id']] = CRM_Utils_Array::value($fieldName, $contacts[$params['to_remove_id']]);
-      }
-    }
-  }
-  foreach ($entitiesToCompare as $entity => $locations) {
-    $contactLocationDetails = civicrm_api3($entity, 'get', [
-      'contact_id' => ['IN' => [$params['to_keep_id'], $params['to_remove_id']]],
-      'location_type_id' => ['IN' => $locations],
-    ])['values'];
-    $detailsByLocation = [];
-    foreach ($contactLocationDetails as $locationDetail) {
-      if ((int) $locationDetail['contact_id'] === $params['to_keep_id']) {
-        $detailsByLocation[$locationDetail['location_type_id']]['to_keep'] = $locationDetail;
-      }
-      elseif ((int) $locationDetail['contact_id'] === $params['to_remove_id']) {
-        $detailsByLocation[$locationDetail['location_type_id']]['to_remove'] = $locationDetail;
-      }
-      else {
-        // Can't think of why this would be the case but perhaps it's ensuring it isn't as we
-        // refactor this.
-        throw new API_Exception(ts('Unknown parameter') . $index);
-      }
-    }
-    foreach ((array) $params['mode'] as $mode) {
-      foreach ($return[$mode]['conflicts'][$entity] as $index => $entityData) {
-        $locationTypeID = $entityData['location_type_id'];
-        foreach ($detailsByLocation[$locationTypeID]['to_keep'] as $fieldName => $keepContactValue) {
-          $fieldsToIgnore = ['id', 'contact_id', 'is_primary', 'is_billing', 'manual_geo_code', 'contact_id', 'reset_date', 'hold_date'];
-          if (in_array($fieldName, $fieldsToIgnore)) {
-            continue;
-          }
-          $otherContactValue = $detailsByLocation[$locationTypeID]['to_remove'][$fieldName];
-          if (!empty($keepContactValue) && !empty($otherContactValue) && $keepContactValue !== $otherContactValue) {
-            $return[$mode]['conflicts'][$entity][$index][$fieldName] = [$params['to_keep_id'] => $keepContactValue, $params['to_remove_id'] => $otherContactValue];
-          }
-        }
-      }
-    }
   }
-  return civicrm_api3_create_success($return, $params);
+  return civicrm_api3_create_success($result, $params);
 }
 
 /**
@@ -1315,7 +1241,7 @@ function _civicrm_api3_contact_get_merge_conflicts_spec(&$params) {
   $params['mode'] = [
     'title' => ts('Dedupe mode'),
     'description' => ts("'safe' or 'aggressive'  - these modes map to the merge actions & may affect resolution done by hooks "),
-    'api.default' => ['safe'],
+    'api.default' => 'safe',
   ];
 }
 
index 3378ae754efcc332d6113be6f36b0bb929eb71da..146d5abad7e0840ab4533a36b58189d13ce48b69 100644 (file)
@@ -1728,6 +1728,7 @@ class api_v3_ContactTest extends CiviUnitTestCase {
       'api.address.create.1' => ['location_type_id' => 'home', 'street_address' => 'medium house', 'city' => 'small city'],
       'api.address.create.2' => ['location_type_id' => 'work', 'street_address' => 'medium office', 'city' => 'small city'],
       'external_identifier' => 'uniquer and specialler',
+      'api.email.create' => ['location_type_id' => 'Other', 'email' => 'bob@example.com'],
       $this->getCustomFieldName('text') => 'mummy loves me more',
     ]);
     $conflicts = $this->callAPISuccess('Contact', 'get_merge_conflicts', ['to_keep_id' => $contact1, 'to_remove_id' => $contact2])['values'];
@@ -1735,19 +1736,22 @@ class api_v3_ContactTest extends CiviUnitTestCase {
       'safe' => [
         'conflicts' => [
           'contact' => [
-            [
-              'first_name' => [$contact1 => 'Anthony', $contact2 => 'different'],
-              'external_identifier' => [$contact1 => 'unique and special', $contact2 => 'uniquer and specialler'],
-              $this->getCustomFieldName('text') => [$contact1 => 'mummy loves me', $contact2 => 'mummy loves me more'],
-            ],
+            'first_name' => [$contact1 => 'Anthony', $contact2 => 'different', 'title' => 'First Name'],
+            'external_identifier' => [$contact1 => 'unique and special', $contact2 => 'uniquer and specialler', 'title' => 'External Identifier'],
+            $this->getCustomFieldName('text') => [$contact1 => 'mummy loves me', $contact2 => 'mummy loves me more', 'title' => 'Enter text here'],
           ],
           'address' => [
             [
               'location_type_id' => '1',
+              'title' => 'Address 1 (Home)',
               'street_address' => [
                 $contact1 => 'big house',
                 $contact2 => 'medium house',
               ],
+              'display' => [
+                $contact1 => "big house\nsmall city, \n",
+                $contact2 => "medium house\nsmall city, \n",
+              ],
             ],
             [
               'location_type_id' => '2',
@@ -1755,6 +1759,11 @@ class api_v3_ContactTest extends CiviUnitTestCase {
                 $contact1 => 'big office',
                 $contact2 => 'medium office',
               ],
+              'title' => 'Address 2 (Work)',
+              'display' => [
+                $contact1 => "big office\nsmall city, \n",
+                $contact2 => "medium office\nsmall city, \n",
+              ],
             ],
           ],
           'email' => [
@@ -1764,11 +1773,27 @@ class api_v3_ContactTest extends CiviUnitTestCase {
                 $contact1 => 'bob@example.com',
                 $contact2 => 'anthony_anderson@civicrm.org',
               ],
+              'title' => 'Email 1 (Home)',
+              'display' => [
+                $contact1 => 'bob@example.com',
+                $contact2 => 'anthony_anderson@civicrm.org',
+              ],
             ],
           ],
         ],
       ],
     ], $conflicts);
+
+    $result = $this->callAPISuccess('Job', 'process_batch_merge');
+    $defaultRuleGroupID = $this->callAPISuccessGetValue('RuleGroup', [
+      'contact_type' => 'Individual',
+      'used' => 'Unsupervised',
+      'return' => 'id',
+      'options' => ['limit' => 1],
+    ]);
+
+    $duplicates = $this->callAPISuccess('Dedupe', 'getduplicates', ['rule_group_id' => $defaultRuleGroupID]);
+    $this->assertEquals($conflicts['safe'], $duplicates['values'][0]['safe']);
   }
 
   private function createEmployerOfMembership() {
index 802dd15b723ff49ce4b759411a58cc250153a429..bc5adbd8a8890ed2ec14e3cfe523b72000a5d693 100644 (file)
@@ -1050,9 +1050,11 @@ class api_v3_JobTest extends CiviUnitTestCase {
    *
    * Note that we set 0 on 2 fields with one on each contact to ensure that
    * both merged & mergee fields are respected.
+   *
+   * @throws \CRM_Core_Exception
    */
   public function testBatchMergeCustomDataZeroValueField() {
-    $customGroup = $this->CustomGroupCreate();
+    $customGroup = $this->customGroupCreate();
     $customField = $this->customFieldCreate(['custom_group_id' => $customGroup['id'], 'default_value' => NULL]);
 
     $mouseParams = ['first_name' => 'Mickey', 'last_name' => 'Mouse', 'email' => 'tha_mouse@mouse.com'];