Prevent duplicate group membership request acceptance.

[dcavins]

groups_accept_membership_request() should check to see if the member
already belongs to the group before processing the acceptance of the
request. Otherwise, repeat acceptances can create duplicate activity
items.

Patch attached, unit tests to follow.

[dcavins]

I'm attaching a test, but it's not 100% repeatable, so it's a bad test. If anyone can see what's wrong with the test, please let me know.

I do know that running groups_accept_membership_request() multiple times creates multiple activity items (especially if you add sleep() time between calls to the function). But doing the same thing in the testing environment doesn't seem to fail reliably.

Thanks for your feedback!

[dcavins]

Unit test.

[boonebgorges]

I'm guessing it's not reproducible in the case of unit tests because the tests run too quickly to allow the race condition to kick in. I wouldn't bother worrying about writing a repeatable test, I guess.

The approach in 6798.01.patch​ looks good to me, except that I'd think you should return false rather than true. In these cases, the membership is not, in fact, being accepted.

[dcavins]

Thanks for taking a look, @boonebgorges.

I'm still dissatisfied with the unreliable unit test. If I can think of a way to make it fail, short of introducing sleep(), I'll do so.

I also agree that the return value should be false and will update.

[dcavins]

It looks like BP_Groups_Member->save() should be making this check, so this ticket is redundant.
​https://buddypress.trac.wordpress.org/browser/trunk/src/bp-groups/classes/class-bp-groups-member.php#L229

The behavior I'm seeing--multiple "joined group" activity items for a request accepted multiple times--must be caused by something else. Closing this ticket as invalid.