Group - remove contacts action removes contacts who are not in group and reports incorrect numbers removed
Overview
removeContactsFromGroup doesn't check if a contact is in a group before removing or deleting them, resulting in some strange behaviour. The reported counts shown to the user of contacts removed are also incorrect. Subscription history is set when it doesn't need to be.
This function is also used by Api3 and for unsubscribes.
Current behaviour
If we're removing, no matter what the current status of the contact is in the group (Added, Pending, Removed or none at all), the status is set to Removed. If a contact had no relationship to the group at all, they will be set to Removed (and show on the Contact Groups tab as removed, which is confusing for the user as they were never in the group). In all cases, subscription history is added with status Removed.
Counts of contacts removed reported to user are incorrect.
If we're deleting, the row is simply deleted from the table. In all cases, subscription history is added with status Deleted.
Expected behaviour
If we're removing:
- If a contact is currently Added or Pending, it is set to Removed and subscription history is added.
- If a contact is Removed or has no group status, nothing happens.
This is somewhat confusing for smart groups. If a contact is not in a smart group because they don't meet the smart group criteria, do we want to set them to Removed, so they are effectively pre-removed if they later meet the smart group criteria? I think we do want this to happen for mailing groups because this function is used for unsubscribes. We don't want a contact to later become part of a smart group that was used for a mailing that they unsubscribed from, so I think we do want to set status to Removed no matter what.
Counts reported to user should also be correct based on what actually happened.
If we're deleting:
- If a contact is currently Added, Pending or Removed, it is deleted from the table and subscription history is added. For smart groups, this resets the contact to being a smart member of the group or not based on the smart group criteria.
- If a contact has no group status, nothing happens.
Fix
If this makes sense, I'll submit a PR.
Confusingly, removeContactsFromGroup is also used to add contacts to a group here. I will change that to use addContactsToGroup for Added or Pending. This will result in a change to the op for the hooks though (from CRM_Utils_Hook::pre('edit', 'GroupContact'...
to CRM_Utils_Hook::pre('create', 'GroupContact'...
. The docs say create is correct for a contact being added to a group, so it appears this is not correct as it stands. Definitely would value an expert opinion on this aspect.