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

[REF] Reduce interaction between dedupe code and createProfileContact #17920

Merged
merged 1 commit into from
Jul 27, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jul 23, 2020

Overview

Code cleanup in Dedupe function

Before

Problematic createProfileContact function called

After

A cut down version of the function called - with the goal to reduce further

Technical Details

Ideally we want to simply call apiv4 from the deduper. However, it seems to take a bit of analysis to determine
if anything is done in there that might have some impact. This takes one step in that direction. I copied the full function over
and then remove all the parts that are unreachable due to the passed in params or meaningless due to us not having Billing
fields. I think the Profile hooks are overkill - the merge hooks and contact hooks are being run and no profile is being submitted

Comments

@pfigel @JKingsnorth @demeritcowboy perhaps one of you can check this

Test cover should be pretty good

@civibot
Copy link

civibot bot commented Jul 23, 2020

(Standard links)

@civibot civibot bot added the master label Jul 23, 2020
Ideally we want to simply call apiv4 from the deduper. However, it seems to take a bit of analysis to determine
if anything is done in there that might have some impact. This takes one step in that direction.  I copied the full function over
and then remove all the parts that are unreachable due to the passed in params or meaningless due to us not having Billing
fields. I think the Profile hooks are overkill - the merge hooks and contact hooks are being run and no profile is being submitted
@demeritcowboy
Copy link
Contributor

I'm partly through this and this what I think you're saying:

  • Billing_XXX fields from $params aren't relevant when merging, since it's not a form/profile being submitted it's already-existing contact records and the fields will be in their appropriate locations already. So can skip addBillingNameFieldsIfOtherwiseNotSet.
    • sure
  • The hooks aren't relevant
    • This is maybe the one I'm least sure about, and where test coverage doesn't help since you're saying don't call these.
  • The manage opt-out part is not needed because we already know $fields is blank.
    • Agree.
  • Groups and tags are moved over some other way already.
    • ???
  • The specific addToGroupID isn't relevant since we know it's blank.
    • Agree.
  • Clear groupcontact cache doesn't need to be done here.
    • Not sure.

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy thanks!

So - groups & tags actually aren't touched in this function because $fields is NULL. In fact a recent change switches to checking $params but this would not have hit those lines

https://github.com/civicrm/civicrm-core/pull/17922/files#diff-c5e39924ad99ac88b48763a0d8fa246eR2024

GroupContactCache - is actually cleared in Contact.create - I'm working on a separate PR to remove it from that function

Re the change to not call the hooks - I can see a case for 'never change what has been done' - but on the other hand we are already calling contact pre & post hooks and dedupe hooks are there is no profile being submitted (as the hook implies). Perhaps @colemanw can weigh in (& potentially merge based on your feedback).

@colemanw
Copy link
Member

I agree with you that the profile hooks are inappropriate to this scenario. This is good cleanup.

@colemanw colemanw merged commit 27cd872 into civicrm:master Jul 27, 2020
@eileenmcnaughton eileenmcnaughton deleted the dupe branch July 27, 2020 21:34
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.

3 participants