Admin cannot be demoted or removed from a group when he's the only one left #509
Conversation
|
This mostly works. However, there is an unintended consequence, at least I believe it is unintended, whereby when a Site Admin attempts to demote the sole Group Admin, the "Sorry, you are not allowed to perform this action.", after which, no other demotions can take place until a promotion takes place first. IOW:
This appears to only apply to the Site Admin, meaning, a Group Admin who happens not to have a User Role of Administrator, demotions work as expected. |
There was a problem hiding this comment.
Thanks a lot for you work on this. It's fine with me, but I would do the same kind of check when a site admin tries to remove the last group admin (directly here or in another PR).
I believe for this second case it's not behaving the right way into the Web version, so I'll give it a check.
includes/bp-groups/classes/class-bp-rest-group-membership-endpoint.php
Outdated
Show resolved
Hide resolved
|
@emaralive good point. Maybe we should improve the error message in this case: "Group needs 1 admin at least. Please promote a user before trying to demote/remove this group admin". |
| @@ -396,28 +370,35 @@ public function update_item( $request ) { | |||
| $role = $request->get_param( 'role' ); | |||
| $group_id = $group->id; | |||
|
|
|||
| $bp_loggedin_user_id = bp_loggedin_user_id(); | |||
|
|
|||
| // Check if the user is a member of the group. | |||
There was a problem hiding this comment.
Returns an early error for clarity when a user is not part of the group. For unban actions, we need to allow since banned users are not part of the group anymore.
| ); | ||
| } | ||
| } | ||
|
|
||
| // Get updated group member. | ||
| $group_member = new BP_Groups_Member( $user->ID, $group_id ); | ||
|
|
||
| // Setting context. |
There was a problem hiding this comment.
The API consumer should be setting this.
| ); | ||
|
|
||
| $admin_error = new WP_Error( |
There was a problem hiding this comment.
Improving the error message.
|
@emaralive and @imath I updated the pr with your suggestions. If you could take a look at the unit tests, that'd be great. If you see a scenario I haven't tested, let me know so that we can improve it. |
|
Superb improvement. 👍 BTW, someone might quibble over the placement (where it resides) of "at least" in the error message but, that won't be me. |
There was a problem hiding this comment.
Hi @renatonascalves sorry it took me so long to check it again. Thanks a lot for your improvements, let's have this in!
Ticket: https://buddypress.trac.wordpress.org/ticket/9163