Fix obscure dedupe scenario where 'bad' location data can overwrite good data

Our api permits the creation of addresses with no actual address-related data. I'm not sure this is a good
thing but one way or another some addresses got into our database with no useful information and I'm not convinced we
are alone.

When merging it's possible for a null address record to be treated as 'not a conflict' with an address record
with the same location but actual data, and for the null record to be retained. This fixes it such that in that
scenario the retained record is the one with data.

This is pretty obscure and an argument could be made for not fixing it. On the other hand this extends tests on the code
(in addition to the fairly extensive set in testBatchMergesAddresses) and I also did a few tangental cleanups

diff --git a/CRM/Dedupe/Merger.php b/CRM/Dedupe/Merger.php
index 088437f1a1a610237f7474020aea27a9b08e8e0a..12f206bb562aa7ff75453734dd5bda26e87b5149 100644 (file)
--- a/CRM/Dedupe/Merger.php
+++ b/CRM/Dedupe/Merger.php
@@ -1006,7 +1006,11 @@ INNER JOIN  civicrm_membership membership2 ON membership1.membership_type_id = m
   }
 
   /**
-   * Compare 2 addresses to see if they are the same.
+   * Compare 2 addresses to see if they are the effectively the same.
+   *
+   * Being the same would mean same location type and any populated fields that describe the locationn match.
+   *
+   * Metadata fields such as is_primary, on_hold, manual_geocode may differ.
    *
    * @param array $mainAddress
    * @param array $comparisonAddress
@@ -1026,6 +1030,39 @@ INNER JOIN  civicrm_membership membership2 ON membership1.membership_type_id = m
     return TRUE;
   }
 
+  /**
+   * Does the location array have valid data.
+   *
+   * While not UI-creatable some sites wind up with email or address rows with no actual email or address
+   * through non core-UI processes.
+   *
+   * @param array $location
+   *
+   * @return bool
+   */
+  public static function locationHasData($location) {
+    return !empty(self::getLocationDataFields($location));
+  }
+
+  /**
+   * Get the location data from a location array, filtering out metadata.
+   *
+   * This returns data like street_address but not metadata like is_primary, on_hold etc.
+   *
+   * @param array $location
+   *
+   * @return mixed
+   */
+  public static function getLocationDataFields($location) {
+    $keysToIgnore = array_merge(self::ignoredFields(), ['display', 'location_type_id']);
+    foreach ($location as $field => $value) {
+      if (in_array($field, $keysToIgnore, TRUE)) {
+        unset($location[$field]);
+      }
+    }
+    return $location;
+  }
+
   /**
    * A function to build an array of information about location blocks that is
    * required when merging location fields
@@ -2146,13 +2183,19 @@ INNER JOIN  civicrm_membership membership2 ON membership1.membership_type_id = m
           // If it exists on the 'main' contact already, skip it. Otherwise
           // if the location type exists already, log a conflict.
           foreach ($migrationInfo['main_details']['location_blocks'][$fieldName] as $mainAddressKey => $mainAddressRecord) {
+            if (!self::locationHasData($mainAddressRecord)) {
+              // Go ahead & overwrite the main address - it has no data in it.
+              // if it is the primary address then pass that honour to the address that actually has data.
+              $migrationInfo['location_blocks'][$fieldName][$mainAddressKey]['set_other_primary'] = $mainAddressRecord['is_primary'];
+              continue;
+            }
             if (self::locationIsSame($addressRecord, $mainAddressRecord)) {
               unset($migrationInfo[$key]);
-              break;
+              continue;
             }
-            elseif ($addressRecordLocTypeId == $mainAddressRecord['location_type_id']) {
+            if ($addressRecordLocTypeId == $mainAddressRecord['location_type_id']) {
               $conflicts[$key] = NULL;
-              break;
+              continue;
             }
           }
         }

diff --git a/tests/phpunit/api/v3/ContactTest.php b/tests/phpunit/api/v3/ContactTest.php
index bea3aed15ce4e050ed0705104cc2c52789c47f79..9dc8203194272fd66ad5065f1448232dec0d9005 100644 (file)
--- a/tests/phpunit/api/v3/ContactTest.php
+++ b/tests/phpunit/api/v3/ContactTest.php
@@ -3974,6 +3974,42 @@ class api_v3_ContactTest extends CiviUnitTestCase {
 
   }
 
+  /**
+   * Test that a blank location does not overwrite a location with data.
+   *
+   * This is a poor data edge case where a contact has an address record with no meaningful data.
+   * This record should be removed in favour of the one with data.
+   *
+   * @dataProvider  getBooleanDataProvider
+   *
+   * @param bool $isReverse
+   *
+   * @throws \CRM_Core_Exception
+   */
+  public function testMergeWithBlankLocationData($isReverse) {
+    $this->createLoggedInUser();
+    $this->ids['contact'][0] = $this->callAPISuccess('contact', 'create', $this->_params)['id'];
+    $this->ids['contact'][1] = $this->callAPISuccess('contact', 'create', $this->_params)['id'];
+    $contactIDWithBlankAddress = ($isReverse ? $this->ids['contact'][1] : $this->ids['contact'][0]);
+    $contactIDWithoutBlankAddress = ($isReverse ? $this->ids['contact'][0] : $this->ids['contact'][1]);
+    $this->callAPISuccess('Address', 'create', [
+      'contact_id' => $contactIDWithBlankAddress,
+      'location_type_id' => 1,
+    ]);
+    $this->callAPISuccess('Address', 'create', [
+      'country_id' => 'MX',
+      'contact_id' => $contactIDWithoutBlankAddress,
+      'street_address' => 'First on the left after you cross the border',
+      'postal_code' => 90210,
+      'location_type_id' => 1,
+    ]);
+
+    $contact = $this->doMerge($isReverse);
+    $this->assertEquals('Mexico', $contact['country']);
+    $this->assertEquals('90210', $contact['postal_code']);
+    $this->assertEquals('First on the left after you cross the border', $contact['street_address']);
+  }
+
   /**
    * Test merging 2 contacts with custom fields.
    *
@@ -4843,4 +4879,21 @@ class api_v3_ContactTest extends CiviUnitTestCase {
     $this->callAPISuccess('Contact', 'delete', ['id' => $contact2, 'skip_undelete' => 1]);
   }
 
+  /**
+   * Do the merge on the 2 contacts.
+   *
+   * @param bool $isReverse
+   *
+   * @return array|int
+   * @throws \CRM_Core_Exception
+   */
+  protected function doMerge($isReverse = FALSE) {
+    $this->callAPISuccess('Contact', 'merge', [
+      'to_keep_id' => $isReverse ? $this->ids['contact'][1] : $this->ids['contact'][0],
+      'to_remove_id' => $isReverse ? $this->ids['contact'][0] : $this->ids['contact'][1],
+      'auto_flip' => FALSE,
+    ]);
+    return $this->callAPISuccessGetSingle('Contact', ['id' => $isReverse ? $this->ids['contact'][1] : $this->ids['contact'][0]]);
+  }
+
 }