From 403400d940e94dcb5cec6031e35ec1d2d5e83e95 Mon Sep 17 00:00:00 2001 From: eileen Date: Tue, 12 Nov 2019 14:23:28 +1300 Subject: [PATCH] Add resolved to return array for get_conflicts The goal here is to be able to display what the script will do if a force mode merge is done. The expection of this is the reason why the 'conflicts' has a key - to allow 'resolved' to schmooze in next to it in a non-breaking way. I haven't added in locations yet as I want to move code around before I do that or the spaghetti will be breeding --- CRM/Dedupe/Merger.php | 35 ++++++--- api/v3/Contact.php | 2 +- tests/phpunit/api/v3/ContactTest.php | 77 ++++++++++++++----- .../phpunit/api/v3/JobTestCustomDataTest.php | 10 ++- 4 files changed, 91 insertions(+), 33 deletions(-) diff --git a/CRM/Dedupe/Merger.php b/CRM/Dedupe/Merger.php index 57e6156155..3cd8eed281 100644 --- a/CRM/Dedupe/Merger.php +++ b/CRM/Dedupe/Merger.php @@ -933,7 +933,7 @@ INNER JOIN civicrm_membership membership2 ON membership1.membership_type_id = m */ public static function skipMerge($mainId, $otherId, &$migrationInfo, $mode = 'safe', &$conflicts = []) { - $conflicts = self::getConflicts($migrationInfo, $mainId, $otherId, $mode); + $conflicts = self::getConflicts($migrationInfo, $mainId, $otherId, $mode)['conflicts']; // A hook could have set skip_merge in order to alter merge behaviour. // This is a something we might ideally deprecate since they really 'should' // mess with the conflicts array instead. @@ -2346,13 +2346,13 @@ INNER JOIN civicrm_membership membership2 ON membership1.membership_type_id = m // allow hook to override / manipulate migrationInfo as well $migrationInfo = $migrationData['migration_info']; foreach ($conflicts as $key => $val) { - if ($val !== NULL || $mode !== 'safe') { - // copy over the resolved values - $migrationInfo[$key] = $val; - unset($conflicts[$key]); - } + // Copy over the resolved values. If we are in aggressive mode we update to null + // so as not to copy over. Why it's different to safe mode is a bit murky. + // Working theory is it doesn't matter what we do in safe mode here if $val is NULL. + // as the merge is not gonna happen if $val == NULL + $migrationInfo[$key] = $val ?? ($mode === 'safe' ? $migrationInfo[$key] : NULL); } - return self::formatConflictArray($conflicts, $migrationInfo['rows'], $migrationInfo['main_details']['location_blocks'], $migrationInfo['other_details']['location_blocks'], $mainId, $otherId); + return self::formatConflictArray($conflicts, $migrationInfo['rows'], $migrationInfo['main_details']['location_blocks'], $migrationInfo['other_details']['location_blocks'], $mainId, $otherId, $mode); } /** @@ -2362,12 +2362,29 @@ INNER JOIN civicrm_membership membership2 ON membership1.membership_type_id = m * @param $toRemoveContactLocationBlocks * @param $toKeepID * @param $toRemoveID + * @param string $mode * * @return mixed * @throws \CRM_Core_Exception */ - protected static function formatConflictArray($conflicts, $migrationInfo, $toKeepContactLocationBlocks, $toRemoveContactLocationBlocks, $toKeepID, $toRemoveID) { + protected static function formatConflictArray($conflicts, $migrationInfo, $toKeepContactLocationBlocks, $toRemoveContactLocationBlocks, $toKeepID, $toRemoveID, $mode) { $return = []; + $resolved = []; + foreach ($conflicts as $key => $val) { + if ($val !== NULL) { + // copy over the resolved values + $resolved[$key] = $val; + unset($conflicts[$key]); + } + elseif ($mode === 'aggressive') { + unset($conflicts[$key]); + if (strpos($key, 'move_location_') !== 0) { + // @todo - just handling plain contact fields for now because I think I need a bigger refactor + // of the below to handle locations & will do as a follow up. + $resolved['contact'][substr($key, 5)] = $migrationInfo[$key]['main']; + } + } + } foreach (array_keys($conflicts) as $index) { if (substr($index, 0, 14) === 'move_location_') { $parts = explode('_', $index); @@ -2406,7 +2423,7 @@ INNER JOIN civicrm_membership membership2 ON membership1.membership_type_id = m throw new CRM_Core_Exception(ts('Unknown parameter') . $index); } } - return $return; + return ['conflicts' => $return, 'resolved' => $resolved]; } /** diff --git a/api/v3/Contact.php b/api/v3/Contact.php index cc98dcaf98..a0f8ed68d2 100644 --- a/api/v3/Contact.php +++ b/api/v3/Contact.php @@ -1204,7 +1204,7 @@ function civicrm_api3_contact_get_merge_conflicts($params) { $migrationInfo = []; $result = []; foreach ((array) $params['mode'] as $mode) { - $result[$mode]['conflicts'] = CRM_Dedupe_Merger::getConflicts( + $result[$mode] = CRM_Dedupe_Merger::getConflicts( $migrationInfo, $params['to_remove_id'], $params['to_keep_id'], $mode diff --git a/tests/phpunit/api/v3/ContactTest.php b/tests/phpunit/api/v3/ContactTest.php index b48a3bb32e..395d6660b6 100644 --- a/tests/phpunit/api/v3/ContactTest.php +++ b/tests/phpunit/api/v3/ContactTest.php @@ -1744,25 +1744,10 @@ class api_v3_ContactTest extends CiviUnitTestCase { /** * Test the function that determines if 2 contacts have conflicts. * - * @throws \Exception + * @throws \CRM_Core_Exception */ public function testMergeGetConflicts() { - $this->createCustomGroupWithFieldOfType(); - $contact1 = $this->individualCreate([ - 'email' => 'bob@example.com', - 'api.address.create' => ['location_type_id' => 'work', 'street_address' => 'big office', 'city' => 'small city'], - 'api.address.create.2' => ['location_type_id' => 'home', 'street_address' => 'big house', 'city' => 'small city'], - 'external_identifier' => 'unique and special', - $this->getCustomFieldName('text') => 'mummy loves me', - ]); - $contact2 = $this->individualCreate([ - 'first_name' => 'different', - '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', - ]); + list($contact1, $contact2) = $this->createDeeplyConflictedContacts(); $conflicts = $this->callAPISuccess('Contact', 'get_merge_conflicts', ['to_keep_id' => $contact1, 'to_remove_id' => $contact2])['values']; $this->assertEquals([ 'safe' => [ @@ -1813,10 +1798,11 @@ class api_v3_ContactTest extends CiviUnitTestCase { ], ], ], + 'resolved' => [], ], ], $conflicts); - $result = $this->callAPISuccess('Job', 'process_batch_merge'); + $this->callAPISuccess('Job', 'process_batch_merge'); $defaultRuleGroupID = $this->callAPISuccessGetValue('RuleGroup', [ 'contact_type' => 'Individual', 'used' => 'Unsupervised', @@ -1825,14 +1811,36 @@ class api_v3_ContactTest extends CiviUnitTestCase { ]); $duplicates = $this->callAPISuccess('Dedupe', 'getduplicates', ['rule_group_id' => $defaultRuleGroupID]); - $this->assertEquals($conflicts['safe'], $duplicates['values'][0]['safe']); + $this->assertEquals($conflicts['safe']['conflicts'], $duplicates['values'][0]['safe']['conflicts']); } + /** + * + * @throws \CRM_Core_Exception + */ + public function testGetConflictsAggressiveMode() { + list($contact1, $contact2) = $this->createDeeplyConflictedContacts(); + $conflicts = $this->callAPISuccess('Contact', 'get_merge_conflicts', ['to_keep_id' => $contact1, 'to_remove_id' => $contact2, 'mode' => ['safe', 'aggressive']])['values']; + $this->assertEquals([ + 'contact' => [ + 'external_identifier' => 'uniquer and specialler', + 'first_name' => 'different', + 'custom_1' => 'mummy loves me more', + ], + ], $conflicts['aggressive']['resolved']); + } + + /** + * Create inherited membership type for employer relationship. + * + * @return int + * + * @throws \CRM_Core_Exception + */ private function createEmployerOfMembership() { $params = [ 'domain_id' => CRM_Core_Config::domainID(), 'name' => 'Organization Membership', - 'description' => NULL, 'member_of_contact_id' => 1, 'financial_type_id' => 1, 'minimum_fee' => 10, @@ -1845,7 +1853,7 @@ class api_v3_ContactTest extends CiviUnitTestCase { 'is_active' => 1, ]; $membershipType = $this->callAPISuccess('membership_type', 'create', $params); - return $membershipType["values"][$membershipType["id"]]; + return $membershipType['values'][$membershipType['id']]; } /** @@ -4443,4 +4451,31 @@ class api_v3_ContactTest extends CiviUnitTestCase { ]); } + /** + * Create pair of contacts with multiple conflicts. + * + * @return array + * + * @throws \CRM_Core_Exception + */ + protected function createDeeplyConflictedContacts(): array { + $this->createCustomGroupWithFieldOfType(); + $contact1 = $this->individualCreate([ + 'email' => 'bob@example.com', + 'api.address.create' => ['location_type_id' => 'work', 'street_address' => 'big office', 'city' => 'small city'], + 'api.address.create.2' => ['location_type_id' => 'home', 'street_address' => 'big house', 'city' => 'small city'], + 'external_identifier' => 'unique and special', + $this->getCustomFieldName('text') => 'mummy loves me', + ]); + $contact2 = $this->individualCreate([ + 'first_name' => 'different', + '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', + ]); + return [$contact1, $contact2]; + } + } diff --git a/tests/phpunit/api/v3/JobTestCustomDataTest.php b/tests/phpunit/api/v3/JobTestCustomDataTest.php index 2ab1dc37dd..07e783507e 100644 --- a/tests/phpunit/api/v3/JobTestCustomDataTest.php +++ b/tests/phpunit/api/v3/JobTestCustomDataTest.php @@ -108,6 +108,8 @@ class api_v3_JobTestCustomDataTest extends CiviUnitTestCase { /** * Cleanup after tests. + * + * @throws \CRM_Core_Exception */ public function tearDown() { $this->quickCleanup(['civicrm_contact'], TRUE); @@ -129,6 +131,10 @@ class api_v3_JobTestCustomDataTest extends CiviUnitTestCase { * 4) NULL (ie not set) * - in safe mode NULL is not a conflict with any option but the other * combos are a conflict. + * + * @param array $dataSet + * + * @throws \CRM_Core_Exception */ public function testBatchMergeCheckboxCustomFieldHandling($dataSet) { $customFieldLabel = 'custom_' . $this->customStringCheckboxID; @@ -137,8 +143,8 @@ class api_v3_JobTestCustomDataTest extends CiviUnitTestCase { $contactID = $this->individualCreate($contact1Params); $this->individualCreate($contact2Params); $result = $this->callAPISuccess('Job', 'process_batch_merge', ['mode' => $dataSet['mode']]); - $this->assertEquals($dataSet['merged'], count($result['values']['merged'])); - $this->assertEquals($dataSet['skipped'], count($result['values']['skipped'])); + $this->assertCount($dataSet['merged'], $result['values']['merged']); + $this->assertCount($dataSet['skipped'], $result['values']['skipped']); $contact = $this->callAPISuccess('Contact', 'getsingle', ['id' => $contactID, 'return' => $customFieldLabel]); $this->assertEquals($dataSet['expected'], $contact[$customFieldLabel]); } -- 2.25.1