From 92a77772eb763966a120d26e4beb8a6523889cda Mon Sep 17 00:00:00 2001 From: eileen Date: Mon, 18 Jul 2016 16:32:23 +1200 Subject: [PATCH] CRM-19076 add test for hooks. In resolving how to deal with addresses for this ticket we decided to err on the side of 'never lose data' ie. doing things like keeping 2 identical addresses if they have different location types. THe justification was that less conservative logic could be implemented using tests. This test adds the logic for use case where address data is more aggressively updated. In doing so I felt the existing hook was almost unworkable & opted to add a different hook, specific to addesses. --- CRM/Dedupe/Merger.php | 232 ++++++++++++++++++------------- CRM/Utils/Hook.php | 18 +++ tests/phpunit/api/v3/JobTest.php | 93 +++++++++++-- 3 files changed, 236 insertions(+), 107 deletions(-) diff --git a/CRM/Dedupe/Merger.php b/CRM/Dedupe/Merger.php index fc5269a0d8..b734541eb0 100644 --- a/CRM/Dedupe/Merger.php +++ b/CRM/Dedupe/Merger.php @@ -959,7 +959,7 @@ INNER JOIN civicrm_membership membership2 ON membership1.membership_type_id = m * * @return bool */ - static protected function addressIsSame($mainAddress, $comparisonAddress) { + static public function addressIsSame($mainAddress, $comparisonAddress) { $keysToIgnore = array('id', 'is_primary', 'is_billing', 'manual_geo_code', 'contact_id'); foreach ($comparisonAddress as $field => $value) { if (in_array($field, $keysToIgnore)) { @@ -1491,7 +1491,7 @@ INNER JOIN civicrm_membership membership2 ON membership1.membership_type_id = m $qfZeroBug = 'e8cddb72-a257-11dc-b9cc-0016d3330ee9'; $relTables = CRM_Dedupe_Merger::relTables(); - $moveTables = $locBlocks = $tableOperations = array(); + $moveTables = $locationMigrationInfo = $tableOperations = array(); foreach ($migrationInfo as $key => $value) { if ($value == $qfZeroBug) { $value = '0'; @@ -1505,23 +1505,7 @@ INNER JOIN civicrm_membership membership2 ON membership1.membership_type_id = m // Set up initial information for handling migration of location blocks elseif (substr($key, 0, 14) == 'move_location_' and $value != NULL) { - $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 = CRM_Utils_Array::value('operation', $migrationInfo['location_blocks'][$fieldName][$fieldCount]); - } - // default operation is overwrite. - if (!$operation) { - $operation = 2; - } - $locBlocks[$fieldName][$fieldCount]['operation'] = $operation; - + $locationMigrationInfo[$key] = $value; } elseif (substr($key, 0, 15) == 'move_rel_table_' and $value == '1') { @@ -1535,83 +1519,7 @@ INNER JOIN civicrm_membership membership2 ON membership1.membership_type_id = m } } } - - // **** Do location related migration. - // @todo Handle OpenID (not currently in API). - if (!empty($locBlocks)) { - - $locationBlocks = self::getLocationBlockInfo(); - - $primaryBlockIds = CRM_Contact_BAO_Contact::getLocBlockIds($mainId, array('is_primary' => 1)); - $billingBlockIds = CRM_Contact_BAO_Contact::getLocBlockIds($mainId, array('is_billing' => 1)); - - foreach ($locBlocks as $name => $block) { - if (!is_array($block) || CRM_Utils_System::isNull($block)) { - continue; - } - $daoName = 'CRM_Core_DAO_' . $locationBlocks[$name]['label']; - $primaryDAOId = (array_key_exists($name, $primaryBlockIds)) ? array_pop($primaryBlockIds[$name]) : NULL; - $billingDAOId = (array_key_exists($name, $billingBlockIds)) ? array_pop($billingBlockIds[$name]) : NULL; - - foreach ($block as $blkCount => $values) { - - // For the block which belongs to other-contact, link the location block to main-contact - $otherBlockDAO = new $daoName(); - $otherBlockDAO->contact_id = $mainId; - - // Get the ID of this block on the 'other' contact, otherwise skip - $otherBlockId = CRM_Utils_Array::value('id', $migrationInfo['other_details']['location_blocks'][$name][$blkCount]); - if (!$otherBlockId) { - continue; - } - $otherBlockDAO->id = $otherBlockId; - - // Add/update location and type information from the form, if applicable - if ($locationBlocks[$name]['hasLocation']) { - $locTypeId = CRM_Utils_Array::value('locTypeId', $migrationInfo['location_blocks'][$name][$blkCount]); - $otherBlockDAO->location_type_id = $locTypeId; - } - if ($locationBlocks[$name]['hasType']) { - $typeTypeId = CRM_Utils_Array::value('typeTypeId', $migrationInfo['location_blocks'][$name][$blkCount]); - $otherBlockDAO->{$locationBlocks[$name]['hasType']} = $typeTypeId; - } - - // Get main block ID - $mainBlockId = CRM_Utils_Array::value('mainContactBlockId', $migrationInfo['location_blocks'][$name][$blkCount], 0); - - // if main contact already has primary & billing, set the flags to 0. - if ($primaryDAOId) { - $otherBlockDAO->is_primary = 0; - } - if ($billingDAOId) { - $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)) { - $deleteDAO = new $daoName(); - $deleteDAO->id = $mainBlockId; - $deleteDAO->find(TRUE); - - // if we about to delete a primary / billing block, set the flags for new block - // that we going to assign to main-contact - if ($primaryDAOId && ($primaryDAOId == $deleteDAO->id)) { - $otherBlockDAO->is_primary = 1; - } - if ($billingDAOId && ($billingDAOId == $deleteDAO->id)) { - $otherBlockDAO->is_billing = 1; - } - - $deleteDAO->delete(); - $deleteDAO->free(); - } - - $otherBlockDAO->update(); - $otherBlockDAO->free(); - } - } - } + self::mergeLocations($mainId, $otherId, $locationMigrationInfo, $migrationInfo); // **** Do tables related migrations if (!empty($moveTables)) { @@ -2133,4 +2041,136 @@ INNER JOIN civicrm_membership membership2 ON membership1.membership_type_id = m return $result['values'][$contact_id]; } + /** + * Merge location. + * + * Based on the data in the $locationMigrationInfo merge the locations for 2 contacts. + * + * The data is in the format received from the merge form (which is a fairly confusing format). + * + * It is converted into an array of DAOs which is passed to the alterLocationMergeData hook + * before saving or deleting the DAOs. A new hook is added to allow these to be altered after they have + * been calculated and before saving because + * - the existing format & hook combo is so confusing it is hard for developers to change & inherently fragile + * - passing to a hook right before save means calculations only have to be done once + * - the existing pattern of passing dissimilar data to the same (merge) hook with a different 'type' is just + * ugly. + * + * The use of the new hook is tested, including the fact it is called before contributions are merged, as this + * is likely to be siginificant data in merge hooks. + * + * @param int $mainId + * @param int $otherId + * @param array $locationMigrationInfo + * Portion of the migration_info that holds location migration information. + * + * @param array $migrationInfo + * Migration info for the merge. This is passed to the hook as informational only. + */ + public static function mergeLocations($mainId, $otherId, $locationMigrationInfo, $migrationInfo) { + foreach ($locationMigrationInfo as $key => $value) { + $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 = CRM_Utils_Array::value('operation', $migrationInfo['location_blocks'][$fieldName][$fieldCount]); + } + // default operation is overwrite. + if (!$operation) { + $operation = 2; + } + $locBlocks[$fieldName][$fieldCount]['operation'] = $operation; + } + $blocksDAO = array(); + + // @todo Handle OpenID (not currently in API). + if (!empty($locBlocks)) { + $locationBlocks = self::getLocationBlockInfo(); + + $primaryBlockIds = CRM_Contact_BAO_Contact::getLocBlockIds($mainId, array('is_primary' => 1)); + $billingBlockIds = CRM_Contact_BAO_Contact::getLocBlockIds($mainId, array('is_billing' => 1)); + + foreach ($locBlocks as $name => $block) { + $blocksDAO[$name] = array('delete' => array(), 'update' => array()); + if (!is_array($block) || CRM_Utils_System::isNull($block)) { + continue; + } + $daoName = 'CRM_Core_DAO_' . $locationBlocks[$name]['label']; + $primaryDAOId = (array_key_exists($name, $primaryBlockIds)) ? array_pop($primaryBlockIds[$name]) : NULL; + $billingDAOId = (array_key_exists($name, $billingBlockIds)) ? array_pop($billingBlockIds[$name]) : NULL; + + foreach ($block as $blkCount => $values) { + $otherBlockId = CRM_Utils_Array::value('id', $migrationInfo['other_details']['location_blocks'][$name][$blkCount]); + $mainBlockId = CRM_Utils_Array::value('mainContactBlockId', $migrationInfo['location_blocks'][$name][$blkCount], 0); + if (!$otherBlockId) { + continue; + } + + // For the block which belongs to other-contact, link the location block to main-contact + $otherBlockDAO = new $daoName(); + $otherBlockDAO->contact_id = $mainId; + + // Get the ID of this block on the 'other' contact, otherwise skip + $otherBlockDAO->id = $otherBlockId; + + // Add/update location and type information from the form, if applicable + if ($locationBlocks[$name]['hasLocation']) { + $locTypeId = CRM_Utils_Array::value('locTypeId', $migrationInfo['location_blocks'][$name][$blkCount]); + $otherBlockDAO->location_type_id = $locTypeId; + } + if ($locationBlocks[$name]['hasType']) { + $typeTypeId = CRM_Utils_Array::value('typeTypeId', $migrationInfo['location_blocks'][$name][$blkCount]); + $otherBlockDAO->{$locationBlocks[$name]['hasType']} = $typeTypeId; + } + + // if main contact already has primary & billing, set the flags to 0. + if ($primaryDAOId) { + $otherBlockDAO->is_primary = 0; + } + if ($billingDAOId) { + $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)) { + $deleteDAO = new $daoName(); + $deleteDAO->id = $mainBlockId; + $deleteDAO->find(TRUE); + + // if we about to delete a primary / billing block, set the flags for new block + // that we going to assign to main-contact + if ($primaryDAOId && ($primaryDAOId == $deleteDAO->id)) { + $otherBlockDAO->is_primary = 1; + } + if ($billingDAOId && ($billingDAOId == $deleteDAO->id)) { + $otherBlockDAO->is_billing = 1; + } + $blocksDAO[$name]['delete'][$deleteDAO->id] = $deleteDAO; + } + $blocksDAO[$name]['update'][$otherBlockDAO->id] = $otherBlockDAO; + } + } + } + + CRM_Utils_Hook::alterLocationMergeData($blocksDAO, $mainId, $otherId, $migrationInfo); + foreach ($blocksDAO as $blockDAOs) { + if (!empty($blockDAOs['update'])) { + foreach ($blockDAOs['update'] as $blockDAO) { + $blockDAO->save(); + } + } + if (!empty($blockDAOs['delete'])) { + foreach ($blockDAOs['delete'] as $blockDAO) { + $blockDAO->delete(); + } + } + } + } + } diff --git a/CRM/Utils/Hook.php b/CRM/Utils/Hook.php index ddee93bfde..a60b911bcb 100644 --- a/CRM/Utils/Hook.php +++ b/CRM/Utils/Hook.php @@ -1143,6 +1143,24 @@ abstract class CRM_Utils_Hook { return self::singleton()->invoke(5, $type, $data, $mainId, $otherId, $tables, self::$_nullObject, 'civicrm_merge'); } + /** + * This hook allows modification of the data calculated for merging locations. + * + * @param array $blocksDAO + * Array of location DAO to be saved. These are arrays in 2 keys 'update' & 'delete'. + * @param int $mainId + * Contact_id of the contact that survives the merge. + * @param int $otherId + * Contact_id of the contact that will be absorbed and deleted. + * @param array $migrationInfo + * Calculated migration info, informational only. + * + * @return mixed + */ + public static function alterLocationMergeData(&$blocksDAO, $mainId, $otherId, $migrationInfo) { + return self::singleton()->invoke(4, $blocksDAO, $mainId, $otherId, $migrationInfo, self::$_nullObject, self::$_nullObject, 'civicrm_alterLocationMergeData'); + } + /** * This hook provides a way to override the default privacy behavior for notes. * diff --git a/tests/phpunit/api/v3/JobTest.php b/tests/phpunit/api/v3/JobTest.php index 833aa7ac8e..8eb7529396 100644 --- a/tests/phpunit/api/v3/JobTest.php +++ b/tests/phpunit/api/v3/JobTest.php @@ -334,10 +334,18 @@ class api_v3_JobTest extends CiviUnitTestCase { $result = $this->callAPISuccess('Job', 'process_batch_merge', array('mode' => $dataSet['mode'])); $this->assertEquals($dataSet['skipped'], count($result['values']['skipped']), 'Failed to skip the right number:' . $dataSet['skipped']); $this->assertEquals($dataSet['merged'], count($result['values']['merged'])); - $result = $this->callAPISuccess('Contact', 'get', array('contact_sub_type' => 'Student', 'sequential' => 1, 'is_deceased' => array('IN' => array(0, 1)))); + $result = $this->callAPISuccess('Contact', 'get', array( + 'contact_sub_type' => 'Student', + 'sequential' => 1, + 'is_deceased' => array('IN' => array(0, 1)), + 'options' => array('sort' => 'id ASC'), + )); $this->assertEquals(count($dataSet['expected']), $result['count']); foreach ($dataSet['expected'] as $index => $contact) { foreach ($contact as $key => $value) { + if ($key == 'gender_id') { + $key = 'gender'; + } $this->assertEquals($value, $result['values'][$index][$key]); } } @@ -590,7 +598,7 @@ class api_v3_JobTest extends CiviUnitTestCase { foreach ($dataSet['contact_2'] as $address) { $this->callAPISuccess('Address', 'create', array_merge(array('contact_id' => $contactID2), $address)); } - $this->hookClass->setHook('civicrm_merge', array($this, 'hookMostRecentDonor')); + $this->hookClass->setHook('civicrm_alterLocationMergeData', array($this, 'hookMostRecentDonor')); $result = $this->callAPISuccess('Job', 'process_batch_merge', array('mode' => 'safe')); $this->assertEquals(1, count($result['values']['merged'])); @@ -603,7 +611,7 @@ class api_v3_JobTest extends CiviUnitTestCase { $this->assertEquals($locationTypes['values'][$addresses['values'][$index][$key]], $value); } else { - $this->assertEquals($addresses['values'][$index][$key], $value); + $this->assertEquals($addresses['values'][$index][$key], $value, 'Unexpected value for ' . $key); } } } @@ -632,19 +640,78 @@ class api_v3_JobTest extends CiviUnitTestCase { } /** - * Implement merge hook, prioritising address details of most recent donor. + * Test hook allowing modification of the data calculated for merging locations. + * + * We are testing a nuanced real life situation where the address data of the + * most recent donor gets priority - resulting in the primary address being set + * to the primary address of the most recent donor and address data on a per + * location type basis also being set to the most recent donor. Hook also excludes + * a fully matching address with a different location. + * + * This has been added to the test suite to ensure the code supports more this + * type of intervention. * - * @param string $type - * @param array $data + * @param array $blocksDAO + * Array of location DAO to be saved. These are arrays in 2 keys 'update' & 'delete'. * @param int $mainId + * Contact_id of the contact that survives the merge. * @param int $otherId - * @param array $tables + * Contact_id of the contact that will be absorbed and deleted. + * @param array $migrationInfo + * Calculated migration info, informational only. + * + * @return mixed */ - public function hookMostRecentDonor($type, &$data, $mainId = NULL, $otherId = NULL, $tables = NULL) { - if ($type != 'batch') { - return; + public function hookMostRecentDonor(&$blocksDAO, $mainId, $otherId, $migrationInfo) { + + $lastDonorID = $this->callAPISuccessGetValue('Contribution', array( + 'return' => 'contact_id', + 'contact_id' => array('IN' => array($mainId, $otherId)), + 'options' => array('sort' => 'receive_date DESC', 'limit' => 1), + )); + // Since the last donor is not the main ID we are prioritising info from the last donor. + // In the test this should always be true - but keep the check in case + // something changes that we need to detect. + if ($lastDonorID != $mainId) { + foreach ($migrationInfo['other_details']['location_blocks'] as $blockType => $blocks) { + foreach ($blocks as $block) { + if ($block['is_primary']) { + $primaryAddressID = $block['id']; + if (!empty($migrationInfo['main_details']['location_blocks'][$blockType])) { + foreach ($migrationInfo['main_details']['location_blocks'][$blockType] as $mainBlock) { + if (empty($blocksDAO[$blockType]['update'][$block['id']]) && $mainBlock['location_type_id'] == $block['location_type_id']) { + // This was an address match - we just need to check the is_primary + // is true on the matching kept address. + $primaryAddressID = $mainBlock['id']; + $blocksDAO[$blockType]['update'][$primaryAddressID] = _civicrm_api3_load_DAO($blockType); + $blocksDAO[$blockType]['update'][$primaryAddressID]->id = $primaryAddressID; + } + $mainLocationTypeID = $mainBlock['location_type_id']; + // We also want to be more ruthless about removing matching addresses. + unset($mainBlock['location_type_id']); + if (CRM_Dedupe_Merger::addressIsSame($block, $mainBlock) + && (!isset($blocksDAO[$blockType]['update']) || !isset($blocksDAO[$blockType]['update'][$mainBlock['id']])) + && (!isset($blocksDAO[$blockType]['delete']) || !isset($blocksDAO[$blockType]['delete'][$mainBlock['id']])) + ) { + $blocksDAO[$blockType]['delete'][$mainBlock['id']] = _civicrm_api3_load_DAO($blockType); + $blocksDAO[$blockType]['delete'][$mainBlock['id']]->id = $mainBlock['id']; + } + // Arguably the right way to handle this is just to set is_primary for the primary + // and for the merge fn to call something like BAO::add & hooks to work etc. + // if that happens though this should keep working... + elseif ($mainBlock['is_primary'] && $mainLocationTypeID != $block['location_type_id']) { + $blocksDAO['address']['update'][$mainBlock['id']] = _civicrm_api3_load_DAO($blockType); + $blocksDAO['address']['update'][$mainBlock['id']]->is_primary = 0; + $blocksDAO['address']['update'][$mainBlock['id']]->id = $mainBlock['id']; + } + + } + $blocksDAO[$blockType]['update'][$primaryAddressID]->is_primary = 1; + } + } + } + } } - $data = $data; } /** @@ -758,6 +825,10 @@ class api_v3_JobTest extends CiviUnitTestCase { array_merge(array('location_type_id' => 'Work', 'is_primary' => 1), $address2), array_merge(array('location_type_id' => 'Home', 'is_primary' => 0), $address1), ), + 'expected_hook' => array( + array_merge(array('location_type_id' => 'Work', 'is_primary' => 0), $address2), + array_merge(array('location_type_id' => 'Home', 'is_primary' => 1), $address1), + ), ), ), array( -- 2.25.1