Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#6597 closed defect (bug) (fixed)

Targeting the 'groups_leave_group' action can be difficult

Reported by: thebrandonallen Owned by: dcavins
Milestone: 2.4 Priority: normal
Severity: normal Version: 2.3.0
Component: Groups Keywords: dev-feedback has-patch commit
Cc: dcavins

Description

While working on #BB2488, I discovered that the groups_leave_group action can be difficult to target. Prior to 2.3, groups_leave_group() removed a member by "uninviting" them. Since 2.3, this is now done by groups_remove_member.

The groups_uninvite_user and groups_leave_group hooks are dissimilar enough that the potential for conflict is minimal. However, the groups_leave_group and groups_remove_member are very similar. The only difference being that one is initiated by the individual user, while the other is initiated by a group/site admin. This problem presents itself in two ways:

1) Functions hooked to both groups_leave_group and groups_remove_member are called twice, with groups_remove_member being the first action called. This occurs twice in BP core with bp_groups_leave_group_delete_recent_activity() and groups_clear_group_user_object_cache(). Technically, it should happen three times, but the bp_core_clear_cache action for groups_remove_member is missing. In each case, the function runs once on groups_remove_member, then again for groups_leave_group, but by the second time, the task is already complete.

2) If you're trying to target the "leave" and "remove" actions similarly, but not exactly the same, there's no way to do this, per point one above. In the case of #BB2488, an example use case would be to allow developers to treat the user initiated action of leaving a public group differently than the admin initiated action of removing the same user from a public group. Since the leave action is typically done via UI, we can do a bp_current_action check, but it's not 100% reliable, as groups_leave_group() may not always be called via UI event.

The easiest solution would be to replace the groups_remove_member() call with a call to BP_Groups_Member::remove(). The problem with that is that we then have two nearly identical functions.

Attachments (3)

6597.groups_remove_member_tweaks.patch (3.1 KB) - added by r-a-y 5 years ago.
6597.2.patch (2.0 KB) - added by thebrandonallen 5 years ago.
6597.03.patch (1.1 KB) - added by dcavins 5 years ago.
Use BP_Groups_Member::delete in groups_leave_group().

Download all attachments as: .zip

Change History (15)

#1 @thebrandonallen
5 years ago

  • Summary changed from Targeting the 'groups_leave_group' can be difficult to Targeting the 'groups_leave_group' action can be difficult

#2 @r-a-y
5 years ago

  • Keywords has-patch added

However, the groups_leave_group and groups_remove_member are very similar. The only difference being that one is initiated by the individual user, while the other is initiated by a group/site admin.

Good catch, thebrandonallen.

groups_remove_member_tweaks.patch is an attempt to consolidate all these different hooks under the groups_remove_member() function. It's not pretty by any means, but it should separate the use of the 'groups_remove_member' and 'groups_leave_group' hooks.

It does this by creating a third parameter for groups_remove_member() -- $context.

If passed, it adds a hook named after $context that is fired after a group member is removed successfully. This is to emulate how 'groups_leave_group' hook works at the moment. As I said, this isn't pretty ;)

Feedback appreciated.

#3 @thebrandonallen
5 years ago

This was, more or less, the idea I had as well. My brain took me one step further, and ended with groups_leave_group() being deprecated. Your "trimmed down" approach probably makes more sense ;)

After spending some time thinking about this, how about the below patch as an iteration? The dynamic action probably isn't needed, since it really only has one purpose. The result is functionally the same, but with less code, and we have the added benefit that the groups_leave_group action stays in it's original location.

#4 follow-up: @r-a-y
5 years ago

The real problem is wanting to stick to our wrapper functions where they sound correct to use, but end up creating problems.

The more I'm thinking about it, the more I like your initial solution, thebrandonallen:

The easiest solution would be to replace the groups_remove_member() call with a call to BP_Groups_Member::remove(). The problem with that is that we then have two nearly identical functions.

However, BP_Groups_Member::remove() is too lenient, which is probably why groups_uninvite_user() was used in the first place because it has more checks on is_confirmed and inviter_id before removing the user from a group.

Our patches in this ticket are both trying to work around the hook-firing issue, which to me sounds wrong.

Ugly as it may sound, reverting back to groups_uninvite_user() from r9811 might be the best course of action.

Or perhaps we change the groups_remove_member() call to BP_Groups_Member::remove_invite(), as the static method will bypass another hook.

@dcavins and others - Want to chime in?

Last edited 5 years ago by r-a-y (previous) (diff)

#5 in reply to: ↑ 4 @dcavins
5 years ago

  • Cc dcavins added

Replying to r-a-y:

The more I'm thinking about it, the more I like your initial solution, thebrandonallen:

The easiest solution would be to replace the groups_remove_member() call with a call to BP_Groups_Member::remove(). The problem with that is that we then have two nearly identical functions.

I'm working to extricate invitations and membership requests from the group membership table, so using uninvite_user to remove a member from a group doesn't look good to me.

Could we determine if the user was being removed or leaving a group by comparing the passed $user_id to the current user id? If they're the same, it was self-initiated; if they're different, we know that someone else initiated the action.

I think being able to target those two actions separately is useful. The question to me is whether it makes sense to continue having both functions or to roll them into one that's a bit more context aware.

#6 @DJPaul
5 years ago

  • Milestone changed from Awaiting Review to 2.4

#7 @DJPaul
5 years ago

  • Keywords needs-patch added; has-patch removed

It looks like this ticket needs more time and love to resolve properly.

As @r-a-y suggested inline, do we want to revert r9811 for 2.4?

#8 @dcavins
5 years ago

Some thoughts:

  • Reverting r9811 as the complete solution wouldn't fix @thebrandonallen's original problem: separating the hooks. Using groups_uninvite_user() within groups_leave_group() calls two hooks: groups_uninvite_user as well as groups_leave_group.
  • Using groups_remove_member() within groups_leave_group() was probably one of my many bad ideas.
  • I'm feeling like remove is applied to a user by an admin while leave is a user-initiated action.
  • In BP_Groups_Member there's a delete() that looks like an all-purpose, no-intent-implied function.

My preference is to basically revert r9811 (undoing the changes to groups_remove_member()) and use BP_Groups_Member::delete() within groups_leave_group(). See 6597.03.patch.

@r-a-y what do you think?

@dcavins
5 years ago

Use BP_Groups_Member::delete in groups_leave_group().

#9 @thebrandonallen
5 years ago

This was my original proposal, so I'm game. Just wasn't sure how important it was to stick to the wrapper structure.

Sidenote, I just noticed that @r-a-y isn't correctly parsed in Trac comments, which means there's no Slack notifications when he's at-mentioned. Should this get reported to Meta, or is it something on our end?

#10 @r-a-y
5 years ago

  • Keywords has-patch commit added; needs-patch removed

No need to report to Meta.

It looks like I get notified properly; I just missed these comments!

I think 03.patch works as well. Go ahead and commit, dcavins.

#11 @dcavins
5 years ago

  • Owner set to dcavins
  • Resolution set to fixed
  • Status changed from new to closed

In 10278:

Modify delete method in groups_leave_group().

Using groups_remove_member() in
groups_leave_group() led to confusion
in the action hooks that are called.
This change reverts the change in r9811
to avoid this problem.

Props thebrandonallen, r-a-y.

Fixes #6597.

#12 @dcavins
5 years ago

Thanks for the comments!

Note: See TracTickets for help on using tickets.