Dupe improve custom data handling

The  current  custom data handling code does the  following

1) For normal single rows it first inserts a row. This has the impact of rendering the
update that follows meaningless (this was an intentional change). It then deletes the row.
Hence the upshot is simply that it deletes the row. A separate process transfers the custom
data for the row. In other words we are engaging in 3 queries with a fairly high chance of
causing deadlocks in order to just delete the row.

2) For single rows where the entity reference refers to the merged contact the row is
updated to refer to the merged contact (without the insert this works) and a further unnecessary delete follows

3) For custom groups supporting multiple rows the rows are updated to have the new entity id. An unnecessary delete follows.

This change only affects the first of these. I would like to, in a future PR, change UPDATE IGNORE to just UPDATE &
remove the unnecessary delete - with more testing.

Note that this does include a slight change of behaviour. Currently if ANY fields in a custom group
are transferred from one contact to another during merge the row is deleted (with all the custom fields in it).
However, if no fields in a set are deleted then the row is not deleted.

This felt like it was a bit short on consistency. If has a potential advantage from a DB size point of view (any
deleting is better than none) but it also increases the number of locking queries in a process that is fairly
prone to cause DB locks. Based on these considerations I didn't think it worth re-adding code complexity to
retain inconsistent deletion.

A note on tests - I pre-added a bunch of tests into _api3_ContactTest to cover the 3 scenarios above.

diff --git a/CRM/Core/DAO.php b/CRM/Core/DAO.php
index e6813869de29c2b848400e29c30fea6380efa531..45ce562df11eef3188074e819fa935528850bbae 100644 (file)
--- a/CRM/Core/DAO.php
+++ b/CRM/Core/DAO.php
@@ -2471,6 +2471,7 @@ SELECT contact_id
     $fields = civicrm_api3('CustomField', 'get', [
       'return'    => ['column_name', 'custom_group_id.table_name'],
       'data_type' => 'ContactReference',
+      'options' => ['limit' => 0],
     ])['values'];
     foreach ($fields as $field) {
       $cidRefs[$field['custom_group_id.table_name']][] = $field['column_name'];

diff --git a/CRM/Dedupe/Merger.php b/CRM/Dedupe/Merger.php
index 6a03efb90fddd39af3ddb7fd0f124fb28c28eacb..ff244b39d61875d9a419a8b9a6c81bb76b528b43 100644 (file)
--- a/CRM/Dedupe/Merger.php
+++ b/CRM/Dedupe/Merger.php
@@ -9,6 +9,8 @@
  +--------------------------------------------------------------------+
  */
 
+use Civi\Api4\CustomGroup;
+
 /**
  *
  * @package CRM
@@ -479,24 +481,19 @@ INNER JOIN  civicrm_membership membership2 ON membership1.membership_type_id = m
    *
    * @param int $mainId
    * @param int $otherId
-   * @param bool $tables
+   * @param array $tables
    * @param array $tableOperations
-   * @param array $customTableToCopyFrom
    *
+   * @throws \API_Exception
    * @throws \CiviCRM_API3_Exception
+   * @throws \Civi\API\Exception\UnauthorizedException
    */
-  public static function moveContactBelongings($mainId, $otherId, $tables, $tableOperations, array $customTableToCopyFrom) {
+  public static function moveContactBelongings($mainId, $otherId, $tables, $tableOperations) {
     $cidRefs = self::cidRefs();
     $eidRefs = self::eidRefs();
     $cpTables = self::cpTables();
     $paymentTables = self::paymentTables();
-
-    // getting all custom tables
-    $customTables = [];
-    // @todo this duplicates cidRefs?
-    CRM_Core_DAO::appendCustomTablesExtendingContacts($customTables);
-    CRM_Core_DAO::appendCustomContactReferenceFields($customTables);
-    $customTables = array_keys($customTables);
+    self::filterRowBasedCustomDataFromCustomTables($cidRefs);
 
     $affected = array_merge(array_keys($cidRefs), array_keys($eidRefs));
 
@@ -513,20 +510,9 @@ INNER JOIN  civicrm_membership membership2 ON membership1.membership_type_id = m
 
     $mainId = (int) $mainId;
     $otherId = (int) $otherId;
-    $multi_value_tables = array_keys(CRM_Dedupe_Merger::getMultiValueCustomSets('cidRefs'));
 
     $sqls = [];
     foreach ($affected as $table) {
-      // skipping non selected single-value custom table's value migration
-      if (!in_array($table, $multi_value_tables)) {
-        if (in_array($table, $customTables) && !in_array($table, $customTableToCopyFrom)) {
-          if (isset($cidRefs[$table]) && ($delCol = array_search('entity_id', $cidRefs[$table])) !== FALSE) {
-            // remove entity_id from the field list
-            unset($cidRefs[$table][$delCol]);
-          }
-        }
-      }
-
       // Call custom processing function for objects that require it
       if (isset($cpTables[$table])) {
         foreach ($cpTables[$table] as $className => $fnName) {
@@ -553,16 +539,6 @@ INNER JOIN  civicrm_membership membership2 ON membership1.membership_type_id = m
 
           $preOperationSqls = self::operationSql($mainId, $otherId, $table, $tableOperations);
           $sqls = array_merge($sqls, $preOperationSqls);
-
-          if (in_array($table, $customTableToCopyFrom) && !self::customRecordExists($mainId, $table, $field) && $field == 'entity_id') {
-            // this is the entity_id column of a custom field group where:
-            // - the custom table should be copied as indicated by $customTableToCopyFrom
-            //   e.g. because a field in the group was selected in a form
-            // - AND no record exists yet for the $mainId contact
-            // we only do this for column "entity_id" as we wouldn't want to
-            // run this INSERT for ContactReference fields
-            $sqls[] = "INSERT INTO $table ($field) VALUES ($mainId)";
-          }
           $sqls[] = "UPDATE IGNORE $table SET $field = $mainId WHERE $field = $otherId";
           $sqls[] = "DELETE FROM $table WHERE $field = $otherId";
         }
@@ -585,6 +561,42 @@ INNER JOIN  civicrm_membership membership2 ON membership1.membership_type_id = m
     CRM_Dedupe_Merger::addMembershipToRealtedContacts($mainId);
   }
 
+  /**
+   * Filter out custom tables from cidRefs unless they are there due to a contact reference or are a multiple set.
+   *
+   * The only fields where we want to move the data by sql is where entity reference fields
+   * on another contact refer to the contact being merged, or it is a multiple record set.
+   * The transference of custom data from one contact to another is done in 2 other places in the dedupe process but should
+   * not be done in moveAllContactData.
+   *
+   * Note it's a bit silly the way we build & then cull cidRefs - however, poor hook placement means that
+   * until we fully deprecate calling the hook from cidRefs we are stuck.
+   *
+   * It was deprecated in code (via deprecation notices if people altered it) in Mar 2019 but in docs only in Apri 2020.
+   *
+   * @param array $cidRefs
+   *
+   * @throws \API_Exception
+   * @throws \Civi\API\Exception\UnauthorizedException
+   */
+  protected static function filterRowBasedCustomDataFromCustomTables(array &$cidRefs) {
+    $customTables = (array) CustomGroup::get()
+      ->setCheckPermissions(FALSE)
+      ->setSelect(['table_name'])
+      ->addWhere('is_multiple', '=', 0)
+      ->addWhere('extends', 'IN', CRM_Contact_BAO_ContactType::contactTypes())
+      ->execute()
+      ->indexBy('table_name');
+    foreach (array_intersect_key($cidRefs, $customTables) as $tableName => $cidSpec) {
+      if (in_array('entity_id', $cidSpec, TRUE)) {
+        unset($cidRefs[$tableName][array_search('entity_id', $cidSpec, TRUE)]);
+      }
+      if (empty($cidRefs[$tableName])) {
+        unset($cidRefs[$tableName]);
+      }
+    }
+  }
+
   /**
    * Given a contact ID, will check if a record exists in given table.
    *
@@ -1246,7 +1258,11 @@ INNER JOIN  civicrm_membership membership2 ON membership1.membership_type_id = m
    *   Respect logged in user permissions.
    *
    * @return bool
+   *
+   * @throws \API_Exception
+   * @throws \CRM_Core_Exception
    * @throws \CiviCRM_API3_Exception
+   * @throws \Civi\API\Exception\UnauthorizedException
    */
   public static function moveAllBelongings($mainId, $otherId, $migrationInfo, $checkPermissions = TRUE) {
     if (empty($migrationInfo)) {
@@ -1286,12 +1302,11 @@ INNER JOIN  civicrm_membership membership2 ON membership1.membership_type_id = m
     self::mergeLocations($mainId, $otherId, $migrationInfo);
 
     // **** Do contact related migrations
-    $customTablesToCopyValues = self::getAffectedCustomTables($submittedCustomFields);
     // @todo - move all custom field processing to the move class & eventually have an
     // overridable DAO class for it.
     $customFieldBAO = new CRM_Core_BAO_CustomField();
     $customFieldBAO->move($otherId, $mainId, $submittedCustomFields);
-    CRM_Dedupe_Merger::moveContactBelongings($mainId, $otherId, $moveTables, $tableOperations, $customTablesToCopyValues);
+    CRM_Dedupe_Merger::moveContactBelongings($mainId, $otherId, $moveTables, $tableOperations);
     unset($moveTables, $tableOperations);
 
     // **** Do table related removals
@@ -1422,39 +1437,6 @@ INNER JOIN  civicrm_membership membership2 ON membership1.membership_type_id = m
     return TRUE;
   }
 
-  /**
-   * Builds an Array of Custom tables for given custom field ID's.
-   *
-   * @param $customFieldIDs
-   *
-   * @return array
-   *   Array of custom table names
-   */
-  private static function getAffectedCustomTables($customFieldIDs) {
-    $customTableToCopyValues = [];
-
-    foreach ($customFieldIDs as $fieldID) {
-      if (!empty($fieldID)) {
-        $customField = civicrm_api3('custom_field', 'getsingle', [
-          'id' => $fieldID,
-          'is_active' => TRUE,
-        ]);
-        if (!civicrm_error($customField) && !empty($customField['custom_group_id'])) {
-          $customGroup = civicrm_api3('custom_group', 'getsingle', [
-            'id' => $customField['custom_group_id'],
-            'is_active' => TRUE,
-          ]);
-
-          if (!civicrm_error($customGroup) && !empty($customGroup['table_name'])) {
-            $customTableToCopyValues[] = $customGroup['table_name'];
-          }
-        }
-      }
-    }
-
-    return $customTableToCopyValues;
-  }
-
   /**
    * Get fields in the contact table suitable for merging.
    *

diff --git a/tests/phpunit/CRM/Dedupe/MergerTest.php b/tests/phpunit/CRM/Dedupe/MergerTest.php
index 37e4f8ba21b69abfa3a544293d4029e16d69b260..c1ce045ad9c18e7920eba0941e693e19493c28df 100644 (file)
--- a/tests/phpunit/CRM/Dedupe/MergerTest.php
+++ b/tests/phpunit/CRM/Dedupe/MergerTest.php
@@ -35,6 +35,9 @@ class CRM_Dedupe_MergerTest extends CiviUnitTestCase {
     parent::tearDown();
   }
 
+  /**
+   * @throws \CRM_Core_Exception
+   */
   public function createDupeContacts() {
     // create a group to hold contacts, so that dupe checks don't consider any other contacts in the DB
     $params = [
@@ -834,6 +837,8 @@ class CRM_Dedupe_MergerTest extends CiviUnitTestCase {
    * for a merge, only those values are merged, while all other fields of the
    * custom group retain their original value, specifically for a contact with
    * no records on the custom group table.
+   *
+   * @throws \CRM_Core_Exception
    */
   public function testMigrationOfSomeCustomDataOnEmptyCustomRecord() {
     // Create Custom Fields
@@ -886,6 +891,11 @@ class CRM_Dedupe_MergerTest extends CiviUnitTestCase {
   /**
    * Test that ContactReference fields are updated to point to the main contact
    * after a merge is performed and the duplicate contact is deleted.
+   *
+   * @throws \API_Exception
+   * @throws \CRM_Core_Exception
+   * @throws \CiviCRM_API3_Exception
+   * @throws \Civi\API\Exception\UnauthorizedException
    */
   public function testMigrationOfContactReferenceCustomField() {
     // Create Custom Fields
@@ -916,7 +926,7 @@ class CRM_Dedupe_MergerTest extends CiviUnitTestCase {
     // pointing to the duplicate (to be deleted) contact
     $unrelatedContact = $this->individualCreate([
       'first_name'               => 'Unrelated',
-      'first_name'               => 'Contact',
+      'last_name'               => 'Contact',
       'email'                    => 'unrelated@example.com',
       "custom_{$refFieldContact['id']}" => $duplicateContactID,
     ]);
@@ -955,8 +965,10 @@ class CRM_Dedupe_MergerTest extends CiviUnitTestCase {
    *   Array of fields to be merged from source into target contact, of the form
    *   ['move_<fieldName>' => <fieldValue>]
    *
+   * @throws \API_Exception
    * @throws \CRM_Core_Exception
    * @throws \CiviCRM_API3_Exception
+   * @throws \Civi\API\Exception\UnauthorizedException
    */
   private function mergeContacts($originalContactID, $duplicateContactID, $params) {
     $rowsElementsAndInfo = CRM_Dedupe_Merger::getRowsElementsAndInfo($originalContactID, $duplicateContactID);
@@ -977,6 +989,8 @@ class CRM_Dedupe_MergerTest extends CiviUnitTestCase {
    * @param $contactID
    * @param $expectedValue
    * @param $customFieldName
+   *
+   * @throws \CRM_Core_Exception
    */
   private function assertCustomFieldValue($contactID, $expectedValue, $customFieldName) {
     $this->assertEntityCustomFieldValue('Contact', $contactID, $expectedValue, $customFieldName);
@@ -990,6 +1004,8 @@ class CRM_Dedupe_MergerTest extends CiviUnitTestCase {
    * @param $id
    * @param $expectedValue
    * @param $customFieldName
+   *
+   * @throws \CRM_Core_Exception
    */
   private function assertEntityCustomFieldValue($entity, $id, $expectedValue, $customFieldName) {
     $data = $this->callAPISuccess($entity, 'getsingle', [
@@ -1005,6 +1021,7 @@ class CRM_Dedupe_MergerTest extends CiviUnitTestCase {
    *
    * @return array
    *   Data for the created custom group record
+   * @throws \CRM_Core_Exception
    */
   private function setupCustomGroupForIndividual() {
     $customGroup = $this->callAPISuccess('custom_group', 'get', [
@@ -1031,11 +1048,12 @@ class CRM_Dedupe_MergerTest extends CiviUnitTestCase {
    * Creates a custom field on the provided custom group with the given field
    * label.
    *
-   * @param $fieldLabel
-   * @param $createGroup
+   * @param string $fieldLabel
+   * @param array $createGroup
    *
    * @return array
    *   Data for the created custom field record
+   * @throws \CRM_Core_Exception
    */
   private function setupCustomField($fieldLabel, $createGroup) {
     return $this->callAPISuccess('custom_field', 'create', [
@@ -1048,6 +1066,8 @@ class CRM_Dedupe_MergerTest extends CiviUnitTestCase {
 
   /**
    * Set up some contacts for our matching.
+   *
+   * @throws \CRM_Core_Exception
    */
   public function setupMatchData() {
     $fixtures = [