From 4c54e0bd41f31c5bf40855ff23e55e77d3d35115 Mon Sep 17 00:00:00 2001 From: eileen Date: Wed, 29 Jul 2020 18:53:51 +1200 Subject: [PATCH] [REF] Extract function to get locations to merge, rename 'operation' to is_replace' This is a preliminary refactor. I have a bug to fix in this code but cannot yet make sense of it. The mergeHandler class is simply an object to refactor the functions onto. Much of this code is hard to work with as the use of static functions necessitates a lot of compilation and compiling so the process of cleaning it up involves extracting functionns to this new class (which may one day replace the Merger class). As it is a refactoring process the functions reflect the old code more than the ideal code. However, it provides the change to give some documentation regarding the locBlock array and also to rename the confusing field 'operation' to 'is_replace' --- CRM/Dedupe/MergeHandler.php | 61 ++++++++++++++++++++++++++++ CRM/Dedupe/Merger.php | 49 ++++++---------------- tests/phpunit/api/v3/ContactTest.php | 14 ++----- 3 files changed, 78 insertions(+), 46 deletions(-) diff --git a/CRM/Dedupe/MergeHandler.php b/CRM/Dedupe/MergeHandler.php index 891bc96c90..6b86b2f9b9 100644 --- a/CRM/Dedupe/MergeHandler.php +++ b/CRM/Dedupe/MergeHandler.php @@ -33,6 +33,30 @@ class CRM_Dedupe_MergeHandler { */ protected $toRemoveID; + /** + * Migration info array. + * + * This is a nasty bunch of data used in mysterious ways. We want to work to make it more + * sensible but for now we store it. + * + * @var array + */ + protected $migrationInfo = []; + + /** + * @return array + */ + public function getMigrationInfo(): array { + return $this->migrationInfo; + } + + /** + * @param array $migrationInfo + */ + public function setMigrationInfo(array $migrationInfo) { + $this->migrationInfo = $migrationInfo; + } + /** * @return mixed */ @@ -129,4 +153,41 @@ class CRM_Dedupe_MergeHandler { return \Civi::$statics[__CLASS__]['dynamic']; } + /** + * Get the location blocks designated to be moved during the merge. + * + * Note this is a refactoring step and future refactors should develop a more coherent array + * + * @return array + * The format is ['address' => [0 => ['is_replace' => TRUE]], 'email' => [0...],[1....] + * where the entity is address, the internal indexing for the addresses on both contact is 1 + * and the intent to replace the existing address is TRUE. + */ + public function getLocationBlocksToMerge(): array { + $locBlocks = []; + foreach ($this->getMigrationInfo() as $key => $value) { + $isLocationField = (substr($key, 0, 14) === 'move_location_' and $value != NULL); + if (!$isLocationField) { + continue; + } + $locField = explode('_', $key); + $fieldName = $locField[2]; + $fieldCount = $locField[3]; + + // Set up the operation type (add/overwrite) + // Ignore operation for websites + // @todo Tidy this up + $operation = 0; + if ($fieldName !== 'website') { + $operation = $this->getMigrationInfo()['location_blocks'][$fieldName][$fieldCount]['operation'] ?? NULL; + } + // default operation is overwrite. + if (!$operation) { + $operation = 2; + } + $locBlocks[$fieldName][$fieldCount]['is_replace'] = $operation === 2; + } + return $locBlocks; + } + } diff --git a/CRM/Dedupe/Merger.php b/CRM/Dedupe/Merger.php index 2300680614..d6b1c7bb7c 100644 --- a/CRM/Dedupe/Merger.php +++ b/CRM/Dedupe/Merger.php @@ -1388,7 +1388,9 @@ INNER JOIN civicrm_membership membership2 ON membership1.membership_type_id = m $removeTables = array_merge($moveTables, $relTables[substr($key, 5)]['tables']); } } - self::mergeLocations($mainId, $otherId, $migrationInfo); + $mergeHandler = new CRM_Dedupe_MergeHandler((int) $mainId, (int) $otherId); + $mergeHandler->setMigrationInfo($migrationInfo); + self::mergeLocations($mergeHandler); // **** Do contact related migrations // @todo - move all custom field processing to the move class & eventually have an @@ -1396,7 +1398,7 @@ INNER JOIN civicrm_membership membership2 ON membership1.membership_type_id = m $customFieldBAO = new CRM_Core_BAO_CustomField(); $customFieldBAO->move($otherId, $mainId, $submittedCustomFields); // add the related tables and unset the ones that don't sport any of the duplicate contact's info - $mergeHandler = new CRM_Dedupe_MergeHandler((int) $mainId, (int) $otherId); + CRM_Dedupe_Merger::moveContactBelongings($mergeHandler, $moveTables, $tableOperations); unset($moveTables, $tableOperations); @@ -1775,43 +1777,19 @@ INNER JOIN civicrm_membership membership2 ON membership1.membership_type_id = m * The use of the new hook is tested, including the fact it is called before contributions are merged, as this * is likely to be significant data in merge hooks. * - * @param int $mainId - * @param int $otherId - * - * @param array $migrationInfo - * Migration info for the merge. This is passed to the hook as informational only. + * @param \CRM_Dedupe_MergeHandler $mergeHandler */ - public static function mergeLocations($mainId, $otherId, $migrationInfo) { - foreach ($migrationInfo as $key => $value) { - $isLocationField = (substr($key, 0, 14) === 'move_location_' and $value != NULL); - if (!$isLocationField) { - continue; - } - $locField = explode('_', $key); - $fieldName = $locField[2]; - $fieldCount = $locField[3]; - - // Set up the operation type (add/overwrite) - // Ignore operation for websites - // @todo Tidy this up - $operation = 0; - if ($fieldName !== 'website') { - $operation = $migrationInfo['location_blocks'][$fieldName][$fieldCount]['operation'] ?? NULL; - } - // default operation is overwrite. - if (!$operation) { - $operation = 2; - } - $locBlocks[$fieldName][$fieldCount]['operation'] = $operation; - } + public static function mergeLocations($mergeHandler) { + $locBlocks = $mergeHandler->getLocationBlocksToMerge(); $blocksDAO = []; + $migrationInfo = $mergeHandler->getMigrationInfo(); // @todo Handle OpenID (not currently in API). if (!empty($locBlocks)) { $locationBlocks = self::getLocationBlockInfo(); - $primaryBlockIds = CRM_Contact_BAO_Contact::getLocBlockIds($mainId, ['is_primary' => 1]); - $billingBlockIds = CRM_Contact_BAO_Contact::getLocBlockIds($mainId, ['is_billing' => 1]); + $primaryBlockIds = CRM_Contact_BAO_Contact::getLocBlockIds($mergeHandler->getToKeepID(), ['is_primary' => 1]); + $billingBlockIds = CRM_Contact_BAO_Contact::getLocBlockIds($mergeHandler->getToKeepID(), ['is_billing' => 1]); foreach ($locBlocks as $name => $block) { $blocksDAO[$name] = ['delete' => [], 'update' => []]; @@ -1832,7 +1810,7 @@ INNER JOIN civicrm_membership membership2 ON membership1.membership_type_id = m // For the block which belongs to other-contact, link the location block to main-contact $otherBlockDAO = new $daoName(); - $otherBlockDAO->contact_id = $mainId; + $otherBlockDAO->contact_id = $mergeHandler->getToKeepID(); // Get the ID of this block on the 'other' contact, otherwise skip $otherBlockDAO->id = $otherBlockId; @@ -1871,9 +1849,8 @@ INNER JOIN civicrm_membership membership2 ON membership1.membership_type_id = m $otherBlockDAO->is_billing = 0; } - $operation = CRM_Utils_Array::value('operation', $values, 2); // overwrite - need to delete block which belongs to main-contact. - if (!empty($mainBlockId) && ($operation == 2)) { + if (!empty($mainBlockId) && $values['is_replace']) { $deleteDAO = new $daoName(); $deleteDAO->id = $mainBlockId; $deleteDAO->find(TRUE); @@ -1893,7 +1870,7 @@ INNER JOIN civicrm_membership membership2 ON membership1.membership_type_id = m } } - CRM_Utils_Hook::alterLocationMergeData($blocksDAO, $mainId, $otherId, $migrationInfo); + CRM_Utils_Hook::alterLocationMergeData($blocksDAO, $mergeHandler->getToKeepID(), $mergeHandler->getToRemoveID(), $migrationInfo); foreach ($blocksDAO as $blockDAOs) { if (!empty($blockDAOs['update'])) { foreach ($blockDAOs['update'] as $blockDAO) { diff --git a/tests/phpunit/api/v3/ContactTest.php b/tests/phpunit/api/v3/ContactTest.php index 62c1ff50e5..4d99ff2562 100644 --- a/tests/phpunit/api/v3/ContactTest.php +++ b/tests/phpunit/api/v3/ContactTest.php @@ -3968,23 +3968,17 @@ class api_v3_ContactTest extends CiviUnitTestCase { */ public function testMerge() { $this->createLoggedInUser(); - $otherContact = $this->callAPISuccess('contact', 'create', $this->_params); - $retainedContact = $this->callAPISuccess('contact', 'create', $this->_params); - $this->callAPISuccess('contact', 'merge', [ - 'to_keep_id' => $retainedContact['id'], - 'to_remove_id' => $otherContact['id'], - 'auto_flip' => FALSE, - ]); + $this->ids['contact'][0] = $this->callAPISuccess('Contact', 'create', $this->_params)['id']; + $this->ids['contact'][1] = $this->callAPISuccess('Contact', 'create', $this->_params)['id']; + $retainedContact = $this->doMerge(); - $contacts = $this->callAPISuccess('contact', 'get', $this->_params); - $this->assertEquals($retainedContact['id'], $contacts['id']); $activity = $this->callAPISuccess('Activity', 'getsingle', [ 'target_contact_id' => $retainedContact['id'], 'activity_type_id' => 'Contact Merged', ]); $this->assertEquals(date('Y-m-d'), date('Y-m-d', strtotime($activity['activity_date_time']))); $activity2 = $this->callAPISuccess('Activity', 'getsingle', [ - 'target_contact_id' => $otherContact['id'], + 'target_contact_id' => $this->ids['contact'][1], 'activity_type_id' => 'Contact Deleted by Merge', ]); $this->assertEquals($activity['id'], $activity2['parent_id']); -- 2.25.1