From eca284630c860869b3ae7c59b6b925b00af5ce57 Mon Sep 17 00:00:00 2001 From: eileen Date: Tue, 21 Apr 2020 17:34:09 +1200 Subject: [PATCH] Fix Dedupe entity_tag mangling bug --- CRM/Core/DAO.php | 25 +++++++++-- CRM/Dedupe/MergeHandler.php | 27 ++++++++++++ CRM/Dedupe/Merger.php | 22 ++++++---- CRM/Logging/Schema.php | 4 ++ Civi/Api4/EntityTag.php | 2 - tests/phpunit/CRM/Dedupe/MergerTest.php | 47 +++++++++++++-------- tests/phpunit/CiviTest/CiviUnitTestCase.php | 4 ++ tests/phpunit/api/v3/JobTest.php | 26 +++++++++++- 8 files changed, 124 insertions(+), 33 deletions(-) diff --git a/CRM/Core/DAO.php b/CRM/Core/DAO.php index d5d4a72048..9b7b024e5c 100644 --- a/CRM/Core/DAO.php +++ b/CRM/Core/DAO.php @@ -2427,6 +2427,8 @@ SELECT contact_id * * Refer to CRM-17454 for information on the danger of querying the information * schema to derive this. + * + * @throws \CiviCRM_API3_Exception */ public static function getReferencesToContactTable() { if (isset(\Civi::$statics[__CLASS__]) && isset(\Civi::$statics[__CLASS__]['contact_references'])) { @@ -2441,13 +2443,30 @@ SELECT contact_id } self::appendCustomTablesExtendingContacts($contactReferences); self::appendCustomContactReferenceFields($contactReferences); - - // FixME for time being adding below line statically as no Foreign key constraint defined for table 'civicrm_entity_tag' - $contactReferences['civicrm_entity_tag'][] = 'entity_id'; \Civi::$statics[__CLASS__]['contact_references'] = $contactReferences; return \Civi::$statics[__CLASS__]['contact_references']; } + /** + * Get all dynamic references to the given table. + * + * @param string $tableName + * + * @return array + */ + public static function getDynamicReferencesToTable($tableName) { + if (!isset(\Civi::$statics[__CLASS__]['contact_references_dynamic'][$tableName])) { + \Civi::$statics[__CLASS__]['contact_references_dynamic'][$tableName] = []; + $coreReferences = CRM_Core_DAO::getReferencesToTable($tableName); + foreach ($coreReferences as $coreReference) { + if ($coreReference instanceof \CRM_Core_Reference_Dynamic) { + \Civi::$statics[__CLASS__]['contact_references_dynamic'][$tableName][$coreReference->getReferenceTable()][] = $coreReference->getReferenceKey(); + } + } + } + return \Civi::$statics[__CLASS__]['contact_references_dynamic'][$tableName]; + } + /** * Add custom tables that extend contacts to the list of contact references. * diff --git a/CRM/Dedupe/MergeHandler.php b/CRM/Dedupe/MergeHandler.php index b28c006300..0bef4e3cc7 100644 --- a/CRM/Dedupe/MergeHandler.php +++ b/CRM/Dedupe/MergeHandler.php @@ -110,4 +110,31 @@ class CRM_Dedupe_MergeHandler { return $relTables; } + /** + * Get an array of tables that have a dynamic reference to the contact table. + * + * This would be the case when the table uses entity_table + entity_id rather than an FK. + * + * There are a number of tables that 'could' but don't have contact related data so we + * do a cached check for this, ensuring the query is only done once per batch run. + * + * @return array + */ + public function getTablesDynamicallyRelatedToContactTable() { + if (!isset(\Civi::$statics[__CLASS__]['dynamic'])) { + \Civi::$statics[__CLASS__]['dynamic'] = []; + foreach (CRM_Core_DAO::getDynamicReferencesToTable('civicrm_contact') as $tableName => $field) { + $sql[] = "(SELECT '$tableName' as civicrm_table, '{$field[0]}' as field_name FROM $tableName WHERE entity_table = 'civicrm_contact' LIMIT 1)"; + } + $sqlString = implode(' UNION ', $sql); + if ($sqlString) { + $result = CRM_Core_DAO::executeQuery($sqlString); + while ($result->fetch()) { + \Civi::$statics[__CLASS__]['dynamic'][$result->civicrm_table] = $result->field_name; + } + } + } + return \Civi::$statics[__CLASS__]['dynamic']; + } + } diff --git a/CRM/Dedupe/Merger.php b/CRM/Dedupe/Merger.php index d3fcacca62..c19ed52f2a 100644 --- a/CRM/Dedupe/Merger.php +++ b/CRM/Dedupe/Merger.php @@ -479,8 +479,7 @@ INNER JOIN civicrm_membership membership2 ON membership1.membership_type_id = m * Based on the provided two contact_ids and a set of tables, move the * belongings of the other contact to the main one. * - * @param int $mainId - * @param int $otherId + * @param CRM_Dedupe_MergeHandler $mergeHandler * @param array $tables * @param array $tableOperations * @@ -488,9 +487,11 @@ INNER JOIN civicrm_membership membership2 ON membership1.membership_type_id = m * @throws \CiviCRM_API3_Exception * @throws \Civi\API\Exception\UnauthorizedException */ - public static function moveContactBelongings($mainId, $otherId, $tables, $tableOperations) { + public static function moveContactBelongings($mergeHandler, $tables, $tableOperations) { + $mainId = $mergeHandler->getToKeepID(); + $otherId = $mergeHandler->getToRemoveID(); $cidRefs = self::cidRefs(); - $eidRefs = self::eidRefs(); + $eidRefs = $mergeHandler->getTablesDynamicallyRelatedToContactTable(); $cpTables = self::cpTables(); $paymentTables = self::paymentTables(); self::filterRowBasedCustomDataFromCustomTables($cidRefs); @@ -500,6 +501,9 @@ INNER JOIN civicrm_membership membership2 ON membership1.membership_type_id = m // if there aren't any specific tables, don't affect the ones handled by relTables() // also don't affect tables in locTables() CRM-15658 $relTables = self::relTables(); + // These arrays don't make a lot of sense. For now ensure the tested handling of tags works... + // it is moved over further down.... + unset($relTables['rel_table_tags']); $handled = self::locTables(); foreach ($relTables as $params) { @@ -545,10 +549,8 @@ INNER JOIN civicrm_membership membership2 ON membership1.membership_type_id = m } if (isset($eidRefs[$table])) { - foreach ($eidRefs[$table] as $entityTable => $entityId) { - $sqls[] = "UPDATE IGNORE $table SET $entityId = $mainId WHERE $entityId = $otherId AND $entityTable = 'civicrm_contact'"; - $sqls[] = "DELETE FROM $table WHERE $entityId = $otherId AND $entityTable = 'civicrm_contact'"; - } + $sqls[] = "UPDATE IGNORE $table SET {$eidRefs[$table]}= $mainId WHERE {$eidRefs[$table]} = $otherId AND entity_table = 'civicrm_contact'"; + $sqls[] = "DELETE FROM $table WHERE {$eidRefs[$table]} = $otherId AND entity_table = 'civicrm_contact'"; } } @@ -1307,7 +1309,9 @@ INNER JOIN civicrm_membership membership2 ON membership1.membership_type_id = m // overridable DAO class for it. $customFieldBAO = new CRM_Core_BAO_CustomField(); $customFieldBAO->move($otherId, $mainId, $submittedCustomFields); - CRM_Dedupe_Merger::moveContactBelongings($mainId, $otherId, $moveTables, $tableOperations); + // 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); // **** Do table related removals diff --git a/CRM/Logging/Schema.php b/CRM/Logging/Schema.php index cd8ee48acb..5d4ae83f83 100644 --- a/CRM/Logging/Schema.php +++ b/CRM/Logging/Schema.php @@ -1041,6 +1041,10 @@ COLS; */ public function getLogTablesForContact() { $tables = array_keys(CRM_Core_DAO::getReferencesToContactTable()); + // This additional hardcoding has been moved from getReferencesToContactTable + // to here as it is not needed in the other place where the function is called. + // It may not be needed here either... + $tables[] = 'civicrm_entity_tag'; return array_intersect($tables, $this->tables); } diff --git a/Civi/Api4/EntityTag.php b/Civi/Api4/EntityTag.php index 06ae229c19..e2a48276a0 100644 --- a/Civi/Api4/EntityTag.php +++ b/Civi/Api4/EntityTag.php @@ -14,8 +14,6 @@ * * @package CRM * @copyright CiviCRM LLC https://civicrm.org/licensing - * $Id$ - * */ namespace Civi\Api4; diff --git a/tests/phpunit/CRM/Dedupe/MergerTest.php b/tests/phpunit/CRM/Dedupe/MergerTest.php index c1ce045ad9..e796173fa4 100644 --- a/tests/phpunit/CRM/Dedupe/MergerTest.php +++ b/tests/phpunit/CRM/Dedupe/MergerTest.php @@ -149,6 +149,8 @@ class CRM_Dedupe_MergerTest extends CiviUnitTestCase { /** * Test the batch merge. + * + * @throws \CRM_Core_Exception */ public function testBatchMergeSelectedDuplicates() { $this->createDupeContacts(); @@ -283,23 +285,8 @@ class CRM_Dedupe_MergerTest extends CiviUnitTestCase { */ public function testGetCidRefs() { $this->entityCustomGroupWithSingleFieldCreate(__FUNCTION__, 'Contacts'); - $this->assertEquals(array_merge($this->getStaticCIDRefs(), $this->getHackedInCIDRef()), CRM_Dedupe_Merger::cidRefs()); - $this->assertEquals(array_merge($this->getCalculatedCIDRefs(), $this->getHackedInCIDRef()), CRM_Dedupe_Merger::cidRefs()); - } - - /** - * Get the list of not-really-cid-refs that are currently hacked in. - * - * This is hacked into getCIDs function. - * - * @return array - */ - public function getHackedInCIDRef() { - return [ - 'civicrm_entity_tag' => [ - 0 => 'entity_id', - ], - ]; + $this->assertEquals($this->getStaticCIDRefs(), CRM_Dedupe_Merger::cidRefs()); + $this->assertEquals($this->getCalculatedCIDRefs(), CRM_Dedupe_Merger::cidRefs()); } /** @@ -307,6 +294,8 @@ class CRM_Dedupe_MergerTest extends CiviUnitTestCase { * * It turns out there are 2 code paths retrieving this data so my initial * focus is on ensuring they match. + * + * @throws \CRM_Core_Exception */ public function testGetMatches() { $this->setupMatchData(); @@ -336,6 +325,8 @@ class CRM_Dedupe_MergerTest extends CiviUnitTestCase { /** * Test results are returned when criteria are passed in. + * + * @throws \CRM_Core_Exception */ public function testGetMatchesCriteriaMatched() { $this->setupMatchData(); @@ -348,6 +339,8 @@ class CRM_Dedupe_MergerTest extends CiviUnitTestCase { /** * Test results are returned when criteria are passed in & limit is respected. + * + * @throws \CRM_Core_Exception */ public function testGetMatchesCriteriaMatchedWithLimit() { $this->setupMatchData(); @@ -361,6 +354,8 @@ class CRM_Dedupe_MergerTest extends CiviUnitTestCase { /** * Test results are returned when criteria are passed in & limit is respected. + * + * @throws \CRM_Core_Exception */ public function testGetMatchesCriteriaMatchedWithSearchLimit() { $this->setupMatchData(); @@ -374,6 +369,8 @@ class CRM_Dedupe_MergerTest extends CiviUnitTestCase { /** * Test getting matches where there are no criteria. + * + * @throws \CRM_Core_Exception */ public function testGetMatchesNoCriteria() { $this->setupMatchData(); @@ -385,6 +382,8 @@ class CRM_Dedupe_MergerTest extends CiviUnitTestCase { /** * Test getting matches with a limit in play. + * + * @throws \CRM_Core_Exception */ public function testGetMatchesNoCriteriaButLimit() { $this->setupMatchData(); @@ -614,6 +613,9 @@ class CRM_Dedupe_MergerTest extends CiviUnitTestCase { * * Note the handling is silly - we are testing to lock in over short term * changes not to imply any contract on the function. + * + * @throws \CRM_Core_Exception + * @throws \CiviCRM_API3_Exception */ public function testGetRowsElementsAndInfoSpecialInfo() { $contact1 = $this->individualCreate([ @@ -702,6 +704,8 @@ class CRM_Dedupe_MergerTest extends CiviUnitTestCase { /** * CRM-19653 : Test that custom field data should/shouldn't be overriden on * selecting/not selecting option to migrate data respectively + * + * @throws \CRM_Core_Exception */ public function testCustomDataOverwrite() { // Create Custom Field @@ -771,7 +775,16 @@ class CRM_Dedupe_MergerTest extends CiviUnitTestCase { /** * dev/core#996 Ensure that the oldest created date is retained even if duplicates have been flipped + * * @dataProvider createdDateMergeCases + * + * @param $keepContactKey + * @param $duplicateContactKey + * + * @throws \API_Exception + * @throws \CRM_Core_Exception + * @throws \CiviCRM_API3_Exception + * @throws \Civi\API\Exception\UnauthorizedException */ public function testCreatedDatePostMerge($keepContactKey, $duplicateContactKey) { $this->setupMatchData(); diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php index 9c5498db65..a20b4735f5 100644 --- a/tests/phpunit/CiviTest/CiviUnitTestCase.php +++ b/tests/phpunit/CiviTest/CiviUnitTestCase.php @@ -929,6 +929,8 @@ class CiviUnitTestCase extends PHPUnit\Framework\TestCase { * * @return int * id of created pledge + * + * @throws \CRM_Core_Exception */ public function pledgeCreate($params) { $params = array_merge([ @@ -954,6 +956,8 @@ class CiviUnitTestCase extends PHPUnit\Framework\TestCase { * Delete contribution. * * @param int $pledgeId + * + * @throws \CRM_Core_Exception */ public function pledgeDelete($pledgeId) { $params = [ diff --git a/tests/phpunit/api/v3/JobTest.php b/tests/phpunit/api/v3/JobTest.php index 073f0fc664..444714204a 100644 --- a/tests/phpunit/api/v3/JobTest.php +++ b/tests/phpunit/api/v3/JobTest.php @@ -67,6 +67,7 @@ class api_v3_JobTest extends CiviUnitTestCase { // The membershipType create breaks transactions so this extra cleanup is needed. $this->membershipTypeDelete(['id' => $this->membershipTypeID]); $this->cleanUpSetUpIDs(); + $this->quickCleanUpFinancialEntities(); $this->quickCleanup(['civicrm_contact', 'civicrm_address', 'civicrm_email', 'civicrm_website', 'civicrm_phone'], TRUE); parent::tearDown(); } @@ -435,8 +436,8 @@ class api_v3_JobTest extends CiviUnitTestCase { $this->entityTagAdd(['contact_id' => $contact2ID, 'tag_id' => 'Short']); $this->entityTagAdd(['contact_id' => $contact2ID, 'tag_id' => 'Tall']); $result = $this->callAPISuccess('Job', 'process_batch_merge', ['mode' => 'safe']); - $this->assertEquals(0, count($result['values']['skipped'])); - $this->assertEquals(1, count($result['values']['merged'])); + $this->assertCount(0, $result['values']['skipped']); + $this->assertCount(1, $result['values']['merged']); $this->callAPISuccessGetCount('Contribution', ['contact_id' => $contactID], 2); $this->callAPISuccessGetCount('Contribution', ['contact_id' => $contact2ID], 0); $this->callAPISuccessGetCount('FinancialItem', ['contact_id' => $contactID], 2); @@ -453,6 +454,27 @@ class api_v3_JobTest extends CiviUnitTestCase { $this->callAPISuccessGetCount('ActivityContact', ['contact_id' => $contact2ID], 2); } + /** + * Test that non-contact entity tags are untouched in merge. + * + * @throws \CRM_Core_Exception + */ + public function testContributionEntityTag() { + $this->callAPISuccess('OptionValue', 'create', ['option_group_id' => 'tag_used_for', 'value' => 'civicrm_contribution', 'label' => 'Contribution']); + $tagID = $this->tagCreate(['name' => 'Big', 'used_for' => 'civicrm_contribution'])['id']; + $contact1 = (int) $this->individualCreate(); + $contact2 = (int) $this->individualCreate(); + $contributionID = NULL; + while ($contributionID !== $contact2) { + $contributionID = (int) $this->callAPISuccess('Contribution', 'create', ['contact_id' => $contact1, 'total_amount' => 5, 'financial_type_id' => 'Donation'])['id']; + } + $entityTagParams = ['entity_id' => $contributionID, 'entity_table' => 'civicrm_contribution', 'tag_id' => $tagID]; + $this->callAPISuccess('EntityTag', 'create', $entityTagParams); + $this->callAPISuccessGetSingle('EntityTag', $entityTagParams); + $this->callAPISuccess('Job', 'process_batch_merge', ['mode' => 'safe']); + $this->callAPISuccessGetSingle('EntityTag', $entityTagParams); + } + /** * Check that the merge carries across various related entities. * -- 2.25.1