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.

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

Conversation

@eileenmcnaughton
Copy link
Contributor Author

@colemanw @totten - this is failing due to a missing env check class - I think ? Coleman? might have removed that but not removed the test?

@colemanw
Copy link
Member

jenkins retest this please

@davecivicrm
Copy link
Contributor

Jenkins, retest this please.

@davecivicrm
Copy link
Contributor

Tested ext id match and conflict and it's looking good.

davecivicrm added a commit that referenced this pull request Dec 22, 2015
CRM-17275 fix import class to do a fallback external identifier check
@davecivicrm davecivicrm merged commit 4a6dc56 into civicrm:master Dec 22, 2015
@eileenmcnaughton eileenmcnaughton deleted the CRM-17275-import branch January 11, 2016 03:43
/**
* 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants