Fix groups_join_group() & groups_accept_invite() behavior

[dcavins]

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.

[dcavins]

Fix parameter order in uses of groups_delete_membership_request()

[dcavins]

Code formatting surrounding fixes in 6090.01

[r-a-y]

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.

[dcavins]

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 anyway. 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?

[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?

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]

Fix parameter order only in uses of groups_delete_membership_request()

[dcavins]

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

[r-a-y]

In 9309:

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

Props dcavins.

See #6090.

[dcavins]

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.