From: eileen <emcnaughton@wikimedia.org>
Date: Tue, 21 Apr 2020 05:34:09 +0000 (+1200)
Subject: Fix Dedupe entity_tag mangling bug
X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=eca284630c860869b3ae7c59b6b925b00af5ce57;p=civicrm-core.git

Fix Dedupe entity_tag mangling bug
---

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.
    *