From 65070890bc548d1db2d560b15765267170272e05 Mon Sep 17 00:00:00 2001 From: eileenmcnaughton Date: Fri, 11 Dec 2015 01:10:55 +0000 Subject: [PATCH] CRM-17275 fix import class to do a fallback external identifier check --- CRM/Contact/Import/Parser/Contact.php | 92 ++++++++++++------- .../CRM/Contact/Import/Parser/ContactTest.php | 43 ++++++++- 2 files changed, 100 insertions(+), 35 deletions(-) diff --git a/CRM/Contact/Import/Parser/Contact.php b/CRM/Contact/Import/Parser/Contact.php index 53e0fb38f3..d79abbc697 100644 --- a/CRM/Contact/Import/Parser/Contact.php +++ b/CRM/Contact/Import/Parser/Contact.php @@ -536,49 +536,26 @@ class CRM_Contact_Import_Parser_Contact extends CRM_Contact_Import_Parser { } } - //get contact id to format common data in update/fill mode, - //if external identifier is present, CRM-4423 - if ($this->_updateWithId && empty($params['id']) && !empty($params['external_identifier'])) { - if ($cid = CRM_Core_DAO::getFieldValue('CRM_Contact_DAO_Contact', $params['external_identifier'], 'id', 'external_identifier')) { - $formatted['id'] = $cid; - } - else { - // CRM-17275 - External identifier is treated as a special field/ - // Having it set will inhibit various efforts to retrieve a duplicate - // However, it is valid to update a contact with no external identifier to having - // an external identifier if they match according to the dedupe rules so - // we check for that possibility here. - // There is probably a better approach but this fix is the FIRST (!#!) time - /// unit tests have been added to this & we need to build those up a bit before - // doing much else in here. Remember when you have build a house of card the - // golden rule ... walk away ... carefully. - // (did I mention unit tests...) - $checkParams = array('check_permissions' => FALSE, 'match' => $params); - unset($checkParams['match']['external_identifier']); - $checkParams['match']['contact_type'] = $this->_contactType; - $possibleMatches = civicrm_api3('Contact', 'duplicatecheck', $checkParams); - foreach (array_keys($possibleMatches['values']) as $possibleID) { - if (!CRM_Core_DAO::getFieldValue('CRM_Contact_DAO_Contact', $possibleID, 'external_identifier', 'id')) { - $formatted['id'] = $cid = $possibleID; - } - } + // Get contact id to format common data in update/fill mode, + // prioritising a dedupe rule check over an external_identifier check, but falling back on ext id. + if ($this->_updateWithId && empty($params['id'])) { + $possibleMatches = $this->getPossibleContactMatches($params); + foreach ($possibleMatches as $possibleID) { + $params['id'] = $formatted['id'] = $possibleID; } } - //format common data, CRM-4062 $this->formatCommonData($params, $formatted, $contactFields); $relationship = FALSE; $createNewContact = TRUE; // Support Match and Update Via Contact ID - if ($this->_updateWithId) { + if ($this->_updateWithId && isset($params['id'])) { $createNewContact = FALSE; - if (empty($params['id']) && !empty($params['external_identifier'])) { - if ($cid) { - $params['id'] = $cid; - } - } - + // @todo - it feels like all the rows from here to the end of the IF + // could be removed in favour of a simple check for whether the contact_type & id match + // the call to the deprecated function seems to add no value other that to do an additional + // check for the contact_id & type. $error = _civicrm_api3_deprecated_duplicate_formatted_contact($formatted); if (CRM_Core_Error::isAPIError($error, CRM_Core_ERROR::DUPLICATE_CONTACT)) { $matchedIDs = explode(',', $error['error_message']['params'][0]); @@ -2162,4 +2139,51 @@ class CRM_Contact_Import_Parser_Contact extends CRM_Contact_Import_Parser { return $allowToCreate; } + /** + * Get the possible contact matches. + * + * 1) the chosen dedupe rule falling back to + * 2) a check for the external ID. + * + * CRM-17275 + * + * @param array $params + * + * @return array + * IDs of possible matches. + * + * @throws \CRM_Core_Exception + * @throws \CiviCRM_API3_Exception + */ + protected function getPossibleContactMatches($params) { + $extIDMatch = NULL; + + if (!empty($params['external_identifier'])) { + $extIDContact = civicrm_api3('Contact', 'get', array( + 'external_identifier' => $params['external_identifier'], + 'return' => 'id', + )); + if (isset($extIDContact['id'])) { + $extIDMatch = $extIDContact['id']; + } + } + $checkParams = array('check_permissions' => FALSE, 'match' => $params); + $checkParams['match']['contact_type'] = $this->_contactType; + + $possibleMatches = civicrm_api3('Contact', 'duplicatecheck', $checkParams); + if (!$extIDMatch) { + return array_keys($possibleMatches['values']); + } + if ($possibleMatches['count']) { + if (in_array($extIDMatch, array_keys($possibleMatches['values']))) { + return array($extIDMatch); + } + else { + throw new CRM_Core_Exception(ts( + 'Matching this contact based on the de-dupe rule would cause an external ID conflict')); + } + } + return array($extIDMatch); + } + } diff --git a/tests/phpunit/CRM/Contact/Import/Parser/ContactTest.php b/tests/phpunit/CRM/Contact/Import/Parser/ContactTest.php index 0a63081955..eb5c1a417e 100644 --- a/tests/phpunit/CRM/Contact/Import/Parser/ContactTest.php +++ b/tests/phpunit/CRM/Contact/Import/Parser/ContactTest.php @@ -84,6 +84,31 @@ class CRM_Contact_Imports_Parser_ContactTest extends CiviUnitTestCase { $this->callAPISuccessGetSingle('Contact', $originalValues); } + /** + * Test import parser will fallback to external identifier. + * + * In this case no primary match exists (e.g the details are not supplied) so it falls back on external identifier. + * + * CRM-17275 + * + * @throws \Exception + */ + public function testImportParserWithUpdateWithExternalIdentifierButNoPrimaryMatch() { + list($originalValues, $result) = $this->setUpBaseContact(array( + 'external_identifier' => 'windows', + 'email' => NULL, + )); + + $this->assertEquals('windows', $result['external_identifier']); + + $originalValues['nick_name'] = 'Old Bill'; + $this->runImport($originalValues, CRM_Import_Parser::DUPLICATE_UPDATE, CRM_Import_Parser::VALID); + $originalValues['id'] = $result['id']; + + $this->assertEquals('Old Bill', $this->callAPISuccessGetValue('Contact', array('id' => $result['id'], 'return' => 'nick_name'))); + $this->callAPISuccessGetSingle('Contact', $originalValues); + } + /** * Test that the import parser adds the external identifier where none is set. * @@ -99,6 +124,22 @@ class CRM_Contact_Imports_Parser_ContactTest extends CiviUnitTestCase { $this->callAPISuccessGetSingle('Contact', $originalValues); } + /** + * Test that the import parser changes the external identifier when there is a dedupe match. + * + * @throws \Exception + */ + public function testImportParserWithUpdateWithChangedExternalIdentifier() { + list($contactValues, $result) = $this->setUpBaseContact(array('external_identifier' => 'windows')); + $contact_id = $result['id']; + $contactValues['nick_name'] = 'Old Bill'; + $contactValues['external_identifier'] = 'android'; + $this->runImport($contactValues, CRM_Import_Parser::DUPLICATE_UPDATE, CRM_Import_Parser::VALID); + $contactValues['id'] = $contact_id; + $this->assertEquals('Old Bill', $this->callAPISuccessGetValue('Contact', array('id' => $contact_id, 'return' => 'nick_name'))); + $this->callAPISuccessGetSingle('Contact', $contactValues); + } + /** * Run the import parser. * @@ -114,7 +155,7 @@ class CRM_Contact_Imports_Parser_ContactTest extends CiviUnitTestCase { $parser->_contactType = 'Individual'; $parser->_onDuplicate = $onDuplicateAction; $parser->init(); - $this->assertEquals($expectedResult, $parser->import($onDuplicateAction, $values)); + $this->assertEquals($expectedResult, $parser->import($onDuplicateAction, $values), 'Return code from parser import was not as expected'); } /** -- 2.25.1