Skip to:
Content

BuddyPress.org

Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#6798 closed defect (bug) (no action required)

Prevent duplicate group membership request acceptance.

Reported by: dcavins Owned by: dcavins
Milestone: Priority: normal
Severity: normal Version: 2.4.0
Component: Groups Keywords: has-patch needs-unit-tests
Cc: dcavins

Description

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.

Attachments (2)

6798.01.patch (602 bytes) - added by dcavins 6 years ago.
6798.02.patch (1.5 KB) - added by dcavins 6 years ago.
Unit test.

Download all attachments as: .zip

Change History (7)

@dcavins
6 years ago

#1 @dcavins
6 years ago

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
6 years ago

Unit test.

#2 @boonebgorges
6 years ago

  • Milestone changed from Awaiting Review to 2.5
  • Owner set to dcavins
  • Status changed from new to assigned

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.

#3 @dcavins
6 years ago

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.

#4 @dcavins
6 years ago

  • Milestone 2.5 deleted
  • Resolution set to invalid
  • Status changed from assigned to closed

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.

This ticket was mentioned in Slack in #buddypress by offereins. View the logs.


5 years ago

Note: See TracTickets for help on using tickets.