From ffa59d181fdfc9f5ebcbe0320bcf7ee03a08b208 Mon Sep 17 00:00:00 2001 From: eileen Date: Mon, 1 Jul 2019 14:56:08 +1200 Subject: [PATCH] Consolidate handling of conflicts between the batch job and get_conflicts api 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 | 23 +++++- CRM/Dedupe/Merger.php | 117 ++++++++++++++++++++------- api/v3/Contact.php | 84 ++----------------- tests/phpunit/api/v3/ContactTest.php | 35 ++++++-- tests/phpunit/api/v3/JobTest.php | 4 +- 5 files changed, 144 insertions(+), 119 deletions(-) diff --git a/CRM/Core/BAO/PrevNextCache.php b/CRM/Core/BAO/PrevNextCache.php index a886e6d184..416709e59a 100644 --- a/CRM/Core/BAO/PrevNextCache.php +++ b/CRM/Core/BAO/PrevNextCache.php @@ -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; diff --git a/CRM/Dedupe/Merger.php b/CRM/Dedupe/Merger.php index 91d5a6c882..a57d64e100 100644 --- a/CRM/Dedupe/Merger.php +++ b/CRM/Dedupe/Merger.php @@ -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; + } + } diff --git a/api/v3/Contact.php b/api/v3/Contact.php index d5f6fc3613..3c2fc5bed2 100644 --- a/api/v3/Contact.php +++ b/api/v3/Contact.php @@ -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', ]; } diff --git a/tests/phpunit/api/v3/ContactTest.php b/tests/phpunit/api/v3/ContactTest.php index 3378ae754e..146d5abad7 100644 --- a/tests/phpunit/api/v3/ContactTest.php +++ b/tests/phpunit/api/v3/ContactTest.php @@ -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() { diff --git a/tests/phpunit/api/v3/JobTest.php b/tests/phpunit/api/v3/JobTest.php index 802dd15b72..bc5adbd8a8 100644 --- a/tests/phpunit/api/v3/JobTest.php +++ b/tests/phpunit/api/v3/JobTest.php @@ -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']; -- 2.25.1