Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CRM-17275 fix import class to do a fallback external identifier check #7427

Merged
merged 1 commit into from
Dec 22, 2015
Merged

CRM-17275 fix import class to do a fallback external identifier check #7427

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 58 additions & 34 deletions CRM/Contact/Import/Parser/Contact.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,49 +536,26 @@ public function import($onDuplicate, &$values, $doGeocodeAddress = FALSE) {
}
}

//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]);
Expand Down Expand Up @@ -2162,4 +2139,51 @@ public function checkRelatedContactFields($relKey, $params) {
return $allowToCreate;
}

/**
* Get the possible contact matches.
*
* 1) the chosen dedupe rule falling back to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is falling back to the default "Unsupervised" rule not the Rule chosen in the Import UI. I added: $checkParams['dedupe_rule_id'] = $this->_dedupeRuleGroupID ?? NULL; to get things to work on my end.

Is that the way this is supposed to work? The import code checks for dupes a number of times. But I've been struggling with inconsistent results. As my importing is based off of my own custom rules.

I can do a PR if indeed the selected Rule should run at this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@darrick I've been doing some clean up on this code over this release & last - I'm happy to look into this & put up a PR if you are OK to test it - it's a bit of a moving target - you can see that this was just merged #23455 allowing code removal in #23461

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. I'll check that out. It's the weekend and I should stop working. I made two tests here: darrick@9b1470a
the second test fails when external_identifier doesn't match but Dedupe does match. The first test passes when not external_identifier is passed in. My current use case is trying to maintain sync with external data even after contacts are merged on the Civi side.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh nice - I guess the thing that will change with the way things are going is that we would use getSubmittedValue('dedupe_rule_id') to do the rule ID - but I'd also quite like to combine the 2 separate places where we do do dedupe look ups -

Yes - get some weekend in!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@darrick your test + some additional clean up is in this PR #23473 - I also did a version with even more cleanup - #23476 - if you have the capacity to take a look & confirm we can get them merged

* 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);
}

}
43 changes: 42 additions & 1 deletion tests/phpunit/CRM/Contact/Import/Parser/ContactTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,31 @@ public function testImportParserWithUpdateWithExternalIdentifier() {
$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.
*
Expand All @@ -99,6 +124,22 @@ public function testImportParserWithUpdateWithNoExternalIdentifier() {
$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.
*
Expand All @@ -114,7 +155,7 @@ protected function runImport($originalValues, $onDuplicateAction, $expectedResul
$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');
}

/**
Expand Down