Skip to:
Content

BuddyPress.org

Opened 7 years ago

Closed 5 years ago

Last modified 5 years ago

#7476 closed defect (bug) (fixed)

Deletion of user also deletes any groups that they created - without any Warning!

Reported by: shanebp's profile shanebp Owned by: boonebgorges's profile boonebgorges
Milestone: 4.0 Priority: normal
Severity: normal Version: 1.0
Component: Administration Keywords: has-patch
Cc: shane@…

Description

If you delete a user via wp-admin, then all groups created by that user are also deleted. Without any Warning! There is no indication that those groups will be deleted. This seems extremely severe.

Is this known and expected behaviour?

Can a Warning be added?
Something similar to the WP option to assign posts to another user?

Attachments (4)

7476.01.patch (3.4 KB) - added by r-a-y 7 years ago.
7476.b.01.patch (1.3 KB) - added by DJPaul 6 years ago.
7476.b.02.patch (1.5 KB) - added by DJPaul 6 years ago.
7476.b.03.patch (1.9 KB) - added by DJPaul 5 years ago.

Download all attachments as: .zip

Change History (27)

#1 @boonebgorges
7 years ago

Will follow up on this later, but briefly: this dates to https://buddypress.trac.wordpress.org/ticket/2614 and [3229].

#2 @r-a-y
7 years ago

  • Component changed from Core to Administration
  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 2.9
  • Owner set to slaFFik
  • Version changed from 2.8.1 to 1.0

We can add some warning text to the "Delete Users" admin page with the 'delete_user_form' hook.

The problem is we don't have a parameter to query groups by creator_id. 01.patch does this and adds a warning prompt to the "Delete Users" page.


It would be nice to reassign groups to the next group admin or mod if one exists. Actually, it looks like we only delete groups if the user is the only group admin:
https://buddypress.trac.wordpress.org/browser/tags/2.8.2/src/bp-groups/classes/class-bp-groups-member.php?marks=1353,1354#L1335

The current check in the patch doesn't do a group admin count check, so that will need to be adjusted.

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

@r-a-y
7 years ago

#3 @hnla
7 years ago

  • Keywords early added
  • Milestone changed from 2.9 to 3.0

Lets deal with in 3.0 'early'

#4 @DJPaul
7 years ago

  • Keywords needs-patch added; has-patch early removed

#5 @boonebgorges
6 years ago

The message in 7476.01.patch seems better than nothing, but it kinda backs you into a corner: if you want to delete the user for legitimate reasons, but don't want to delete the groups, you have no recourse.

In theory, a better option seems like a checkbox:

[x] Delete groups created by the user

which I suppose could be checked by default. But in order to make this work, we probably have to reassign someone else as the "creator" of those groups, because it's likely that various parts of BP will break if they have 0 members, or if creator_id is empty. So maybe, when the checkbox is unchecked, you'd see:

[ ] Delete groups created by the user
    <em>You will be reassigned as the administrator of the deleted user's groups</em>

If we go with this, we'd probably want to link to the groups in the confirmation screen, in case the admin then wants to go modify the auto-reassigned settings.

I know this is a lot more complex, but it feels like a more generous strategy than a warning without recourse. Does anyone have thoughts, or better ideas?

#6 @DJPaul
6 years ago

The group creator doesn't show in the UI, right? It's just that first member becomes the group's first administrator.

I like what @boonebgorges suggested, but I'm tempted to suggest we just quietly re-assign the group creator field -- if we definitely don't show this piece of data anywhere in our templates.

#7 @DJPaul
6 years ago

  • Keywords 2nd-opinion added

Thoughts on above?

#8 @slaFFik
6 years ago

  • Owner slaFFik deleted
  • Status changed from new to assigned

#9 @DJPaul
6 years ago

  • Milestone changed from 3.0 to Under Consideration

#10 follow-up: @DJPaul
6 years ago

Please can I have your thoughts on re-assigning the group creator property to the site owner/admin when a user account is deleted?

This let us fix this issue for 3.0, as there hasn't been any action or consensus on template changes in the last 3 months otherwise.

#11 in reply to: ↑ 10 @boonebgorges
6 years ago

Replying to DJPaul:

Please can I have your thoughts on re-assigning the group creator property to the site owner/admin when a user account is deleted?

This let us fix this issue for 3.0, as there hasn't been any action or consensus on template changes in the last 3 months otherwise.

This seems fine to me. If it's too much work to do a checkbox toggle, the message suggested in 7476.01.patch is better than nothing.

@DJPaul
6 years ago

@DJPaul
6 years ago

#12 @DJPaul
6 years ago

7476.b.02.patch makes it so if the last group admin leaves a group, we try to fetch a site admin, and make them the group admin instead -- rather than delete the group. We can work on the UI for the future, but this should be miles better than silently deleting the group.

There's some juggling to attempt to fetch an admin on the right site (for multisite), could use a sanity check on that.

#13 @r-a-y
6 years ago

I think if we're going to add in the b patch, we should also add a notice on the Delete User prompt that groups where the deleted user is the sole group admin will be reassigned to the site admin, instead of deleted.

Might also want to add some group meta with a marker like reassigned so these groups can be queried and a decision to delete them can be made at a later date.

#14 @DJPaul
6 years ago

There is not a thing such as “the” site admin, only an admin. So if there was a UI change, we’d need a drop down to allow a choice. A UI change alone won’t supoort the use cases of deleting a user via CLI or REST API. I don’t think there is time to make a good change here for 3.0.

Given the current behaviour is silently and absolutely destructive, not destroying the data is a improvement.
I don’t mind adding a bit of meta to track these auto-assignments, but I somewhat doubtful about how useful it will be (happy to be proven wrong).

#15 @DJPaul
6 years ago

  • Milestone changed from Under Consideration to 4.0

@DJPaul
5 years ago

#16 @DJPaul
5 years ago

  • Keywords has-patch added; needs-patch 2nd-opinion removed

7476.b.03.patch carries on with the next iteration of my 'b' series patch.

I've removed the fiddling with the group meta member count, which seemed unnecessary. I've switched the method to use groups_leave_group() to leave each group, and this also seems to make more sense, because we get the groups_leave_group action triggered as well now, etc.

#17 @boonebgorges
5 years ago

The approach in 7476.b.03.patch seems good to me.

One thought: Perhaps we leave the existing method in place, and instead put this new logic in a new, more appropriately named method. Something like leave_all_for_user(). Then, we call that from groups_remove_data_for_user() instead. We could also improve the documentation on groups_remove_data_for_user() that explains more clearly what happens at user deletion, and then explains how to remove_filter() to modify it.

#18 @DJPaul
5 years ago

Given the method is destructive - bugged, really - I think we are best to replace it.

#19 @boonebgorges
5 years ago

Thanks, @DJPaul. This seems fine to me.

I'm adding some tests, which is leading me to make a few changes. Notably, groups_promote_member() and groups_leave_group() don't work properly when called programatically - they're meant to be screen controllers. So I'm swapping them out for the related business functions. The test is also surfacing an existing bug, which is that the admin/mod cache is not busted when a group membership is deleted. I'll fix that too.

#20 @boonebgorges
5 years ago

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

In 12273:

Groups: Improve treatment of single-member groups on user deletion.

Instead of deleting single-member groups on user deletion, we simply
remove the member from the group. Because BuddyPress expects groups to
have at least one admin member, in these cases we assign a site admin
to be the sole group admin.

This changeset also fixes a related bug whereby the group admin/mod
cache was not properly invalidated on membership deletion.

Props djpaul.
Fixes #7476.

#21 @boonebgorges
5 years ago

In 12274:

Correct array syntax from [12273].

Square-bracket syntax for defining arrays was introduced in PHP 5.4.

See #7476.

#22 @boonebgorges
5 years ago

In 12275:

Fix another PHP 5.3 syntax error after [12273].

See #7476.

#23 @boonebgorges
5 years ago

In 12276:

Fix another PHP 5.3 syntax error after [12273].

See #7476.

Note: See TracTickets for help on using tickets.