Skip to:
Content

BuddyPress.org

Opened 9 years ago

Closed 9 years ago

#6090 closed defect (bug) (fixed)

Fix groups_join_group() & groups_accept_invite() behavior

Reported by: dcavins's profile dcavins Owned by:
Milestone: 2.3 Priority: normal
Severity: normal Version:
Component: Groups Keywords: needs-patch
Cc:

Description

I uncovered a problem with the group membership functions groups_join_group() and groups_accept_invite() while writing some unit tests. The problem is that the function groups_delete_membership_request() was being used with the parameters in the wrong order. It expects, in this order:
$membership_id, $user_id, $group_id

But in groups_join_group() and groups_accept_invite() we were using it with these parameters:
groups_delete_membership_request( $user_id, $group_id )

So it was treating the passed user id as a membership id, and, since groups_delete_membership_request() was using BP_Groups_Member::delete, the group membership record with the table id matching the new member's user id was being deleted. (E.g. groups_delete_membership_request( 14, 2 ) deletes the 14th row of the table, no matter what it represents. ) Oops.

I've added a "null" in the function parameter list so that the user_id and group_id are passed as expected to groups_delete_membership_request(). I've also changed groups_delete_membership_request() to use BP_Groups_Member::delete_request() which is bit more restrained than BP_Groups_Member::delete.

Attachments (3)

6090.01.patch (1.3 KB) - added by dcavins 9 years ago.
Fix parameter order in uses of groups_delete_membership_request()
6090.02-formatting-only.patch (3.5 KB) - added by dcavins 9 years ago.
Code formatting surrounding fixes in 6090.01
6090.01b.patch (950 bytes) - added by dcavins 9 years ago.
Fix parameter order only in uses of groups_delete_membership_request()

Download all attachments as: .zip

Change History (11)

@dcavins
9 years ago

Fix parameter order in uses of groups_delete_membership_request()

@dcavins
9 years ago

Code formatting surrounding fixes in 6090.01

#1 follow-up: @r-a-y
9 years ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 2.2

For 01.patch:

The problem is that the function groups_delete_membership_request() was being used with the parameters in the wrong order. It expects, in this order:
$membership_id, $user_id, $group_id

But in groups_join_group() and groups_accept_invite() we were using it with these >parameters:
groups_delete_membership_request( $user_id, $group_id )

Yikes! Good catch! Go ahead and commit these changes.

I've also changed groups_delete_membership_request() to use BP_Groups_Member::delete_request() which is bit more restrained than BP_Groups_Member::delete.

However, I would hold off on making the delete_request() method change in groups_delete_membership_request(). Some devs might be using this function in this open manner.

Also go ahead with 02.patch.

#2 in reply to: ↑ 1 @dcavins
9 years ago

Replying to r-a-y:
Thanks for looking over the patch r-a-y.

I'm a bit worried about the use of BP_Groups_Member::delete in groups_delete_membership_request() because that will delete any record that involves the user and group, which could include any requests, invitations or memberships, right? In these functions, it follows the deletion of invitations while the group membership is being created, so I guess those other possible records are also being deleted. It's also used in groups_reject_membership_request() so it could potentially delete invitations as well as requests there.

Short of using delete_request(), I'm not sure how to prevent the function from being overzealous.

Is this worth deprecating the current function and switching over to a new function?

Version 0, edited 9 years ago by dcavins (next)

#3 @r-a-y
9 years ago

I'm a bit worried about the use of BP_Groups_Member::delete in groups_delete_membership_request() because that will delete any record that involves the user and group, which could include any requests, invitations or memberships, right?

After thinking about this some more, I think you are right. Internally, groups_delete_membership_request() is always called with groups_check_for_membership_request() right before it. So I would be okay with the change with the caveat (and in the spirit of boonebgorges) that we add in a unit test exhibiting the conflict when using groups_delete_membership_request() against requests and invites.

@dcavins
9 years ago

Fix parameter order only in uses of groups_delete_membership_request()

#4 @dcavins
9 years ago

6090.01b.patch is ready to go (and is fairly important). The other patches can wait, I believe, pending some other work.

#5 @r-a-y
9 years ago

In 9309:

Groups: Fix wrong parameter usage for `groups_delete_membership_request().

Props dcavins.

See #6090.

#6 @DJPaul
9 years ago

  • Keywords needs-patch added; has-patch commit removed
  • Milestone changed from 2.2 to 2.3

#7 @dcavins
9 years ago

I've added tests for groups_delete_membership_request(), groups_reject_membership_request(), groups_reject_invite(), groups_uninvite_user() and groups_delete_invite() in ticket #6209.

Please take a look and offer feedback.

I believe this ticket can be resolved as "fixed," but that doesn't seem to be an option.

#8 @dcavins
9 years ago

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.