From ffcc831f04c0a3e5c4bd83a1024c57e3f58b0b83 Mon Sep 17 00:00:00 2001 From: eileen Date: Thu, 30 Jul 2020 00:20:43 +1200 Subject: [PATCH] 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 --- CRM/Dedupe/Merger.php | 51 +++++++++++++++++++++++--- tests/phpunit/api/v3/ContactTest.php | 53 ++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 4 deletions(-) diff --git a/CRM/Dedupe/Merger.php b/CRM/Dedupe/Merger.php index 088437f1a1..12f206bb56 100644 --- 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 bea3aed15c..9dc8203194 100644 --- 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]]); + } + } -- 2.25.1