Prevent erroneous GroupContact hooks for Contact already in group

[larssandergreen]

Overview

Because CRM_Contact_BAO_GroupContact::addContactsToGroup sets a GroupContact create hook including each Contact ID passed to it, regardless of if that Contact was already in the Group, it generates erroneous hooks when adding Contacts to Groups they were already Added to. This can be observed in CiviRules, for example, with a rule that is triggered by Contact is added to Group.

Before

If you add a Contact to a Group that they are already Added to using CRM_Contact_BAO_GroupContact::addContactsToGroup (for example, from Search Actions), you'll get a GroupContact created hook including that Contact.

After

Hook no longer includes Contacts already Added to the Group.

Technical Details

There are two extensions that use bulkAddContactsToGroup, so will now trigger hooks when they didn't previously. Doesn't look like a problem to me, but paging @artfulrobot here just in case (for Chassé (1, 2) and RefineMailing).

The other change here is we now call the post hook before the cache clearing that it used to come after. That doesn't seem like it should be a problem, but I suppose if an extension was checking the GroupContact cache, there could be a change. I only see one use in a test for MailChimp grepping universe and it doesn't look like it should matter there. Could move the GroupContact cache clear to before the post_hook, but then it would run once per 200 Contacts (which is not a big deal, but less clean). I think this is probably not worth worrying about.

[civibot]

(Standard links)

• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[artfulrobot]

@larssandergreen Thanks so much for the helpful pings!

Generally, I wonder whether the current behaviour is useful? e.g. if adding to a group triggers a 'welcome' email or such, which you might want sent even if they're in that group already?

I can definitely see your point that a more common case is to want to know when something has happened, not when it didn't because it was already so.

What do you think to the pre hook including all the contacts, and the post hook including only those who were added? That way authors could presumably choose a hook based on whether they want to know when "add to group" was the intention (pre hook) or the historical fact (post hook)? Perhaps this is too nuanced?

In Refine Mailing, it is dead code - not been used for years, so no problem there (note to self: remove that one day). I do a rude SQL insert into group-contact instead of using the API for raw speed and because I can't see hooks being valuable there.

The Chassé ones are probably OK for people, unless they have some CiviRule set up for add-to-group (by what you report). The first adds them to a mailing group - this is the one that will cause problems in certain set-ups, if any. The second one is highly unlikely to affect anyone unless they have an expensive process listening for that hook. I think it's probably ok.

Thanks again for the ping.

[larssandergreen]

Thanks @artfulrobot.

This general question is also one I have thought about. Sometimes, adding to a group that a contact is already added to is meaningful and sometimes it isn't. Sometimes, I think we'd want to add Subscription History and sometimes we wouldn't (and sometimes we want the hook and sometimes we don't). We don't really have any way to distinguish the two cases.

This gets even worse when we turn to removing (I think removal is actually probably meaningful in all cases except when they are already removed). I don't really have a good solution for all this except to try to make the behaviour consistent, which I think means no hook if we're not actually doing anything.

I think having a pre hook but not a post hook would be potentially confusing if someone wanted to use both and didn't expect the contact ids to change between the two. Tempting, but doesn't that kind of break the general expectation about how the hooks work?

[artfulrobot]

Yeah, I'm not sure what consitency or not there is in when hooks fire. But I agree the split pre/post could be confusing. The best solution would pass all the info to the hook, so the implementer could decided what they wanted to do. i.e. pass the intent (add contacts 1,2,3 to group 4) and the reality (2,3 are already in the group).

I'm fine with this change in behaviour of the hook, so don't let me hold up this MR.

[eileenmcnaughton]

Looking through the patch & discussion I think I'm generally ok merging this although any last thoughts from @colemanw @seamuslee001 @demeritcowboy appreciated.

My issue with the hook is unchanged by this - ie a single-contact-hook is being used in bulk mode & is an anomaly. I understand why but still feel ick about it.

[artfulrobot]

Ooh good spot, @eileenmcnaughton yeah, that's not great - for performance as anything else.