Skip to:
Content

Opened 4 years ago

Closed 3 years ago

#6013 closed defect (bug) (fixed)

Allow Group Admins to Unban Members

Reported by: skyrie Owned by: DJPaul
Milestone: 2.2 Priority: normal
Severity: normal Version:
Component: Groups Keywords: has-patch commit
Cc:

Description

Right now, the only way to unban a member from a group is for the site admin to do it fron the backend. There is no way for a regular BuddyPress group admins to be able to unban members once they click the "Kick and Ban" option.

Group admins should be able to see banned members and unban them from the frontend group admin page.

See support thread here:
https://buddypress.org/support/topic/unban-from-group/

Attachments (3)

6013.1.patch (615 bytes) - added by thebigA 4 years ago.
Patch
6013.wp_parse_str.patch (1.6 KB) - added by r-a-y 4 years ago.
6013.groups_get_group_members.patch (699 bytes) - added by r-a-y 3 years ago.

Download all attachments as: .zip

Change History (15)

#1 @boonebgorges
4 years ago

  • Keywords needs-patch good-first-bug added
  • Milestone changed from Awaiting Review to Future Release

+1. This would be great.

#2 @DJPaul
4 years ago

  • Keywords good-first-bug removed
  • Owner set to DJPaul
  • Status changed from new to assigned

I am working through this ticket with a first-time contributor.

@thebigA
4 years ago

Patch

#3 @thebigA
4 years ago

When I looked at the source file for the template bp-templates/bp-legacy/buddypress/groups/single/admin.php I could see that there was already code to allow for a member being banned to be reinstated. This was never being processed so I looked to see what records the underlying dataset was working on.

The setting of the check for exclude_banned is in bp-groups-functions.php on line 544. This checks to see if the element passed through the parameters array is true. The original code had 'exclude_banned=false' which was being evaluated as 'false' being a string not a boolean so when it got to the test it was always evaluation as true since it was not an empty string. Changing the original code to 'exclude_banned=0" is evaluating as a boolean so now works as expected.

#4 @DJPaul
4 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 2.2

#5 @r-a-y
4 years ago

Good catch, thebigA.

The problem is with wp_parse_str(), which is used when we pass a querystring to convert into an array as in the case of bp_group_has_members() used in the group admin template.

When the querystring is converted over to an array, the 'true' or 'false' values are strings and need to be explicitly converted over to booleans.

I've attached a patch that fixes this by filtering 'wp_parse_str' and using PHP's filter_var() to convert the boolean over. This will fix any old string / boolean issues we've had with wp_parse_args() in the past.

I think this may be more applicable as an upstream patch to WP.

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

#6 @DJPaul
4 years ago

The filter fix would affect other code beyond BuddyPress; it could "fix" something that other people don't know that it's broken, causing a change in behaviour ("why does BuddyPress change this thing in X and Y plugins?"). I think I'd prefer to apply the original patch to BuddyPress, and, Ray, we take your patch upstream.

#7 @DJPaul
3 years ago

Any thoughts on this, r-a-y?

#8 @r-a-y
3 years ago

Regarding wp_parse_str(), I opened a ticket upstream:
https://core.trac.wordpress.org/ticket/30753

I kind of agree that we shouldn't add my patch, so let's get thebigA's patch committed perhaps with a unit test.

Will try and get to this later today if no one beats me to it.

#9 @r-a-y
3 years ago

Okay, so I've had a chance to think about thebigA's fix a bit more.

His patch requires a template change and a similar change will need to go in bp-default as well (if we want to fix bp-default).

groups_get_group_members.patch is an alternate fix that occurs where we do our comparison checks to fetch group members. It explicitly casts the string 'false' value for 'exclude_banned' to a proper false value and doesn't require any template changes. This is a little ugly, but we've done similar things in the past (see r7574).

I'm fine with either patch being committed.

#10 @boonebgorges
3 years ago

Notwithstanding [7574], I think we should just make the template change suggested in 6013.1.patch. The other options add layers of obfuscation or odd variable handling that I don't think will help us in the long run.

#11 @r-a-y
3 years ago

  • Keywords commit added

Let's commit thbigA's patch.

Unit test is already covered via source:/tags/2.1.1/tests/phpunit/testcases/groups/template.php#L367

#12 @r-a-y
3 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 9292:

bp-legacy: Show banned members when on a group's Manage Members page.

Previously, we passed exclude_banned=false in a querystring. This gets
interpreted by wp_parse_args() as string false. Our exclude_banned`
check in groups_get_group_members() does not interpret string 'false'
values, which led to banned members being excluded instead of included.

This commit switches the querystring to exclude_banned=0, which
groups_get_group_members() will interpret as a proper false value
allowing banned members to be shown.

Props thebigA.

Fixes #6013.

Note: See TracTickets for help on using tickets.