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

[shanebp]

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?

[boonebgorges]

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

[r-a-y]

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.

[hnla]

Lets deal with in 3.0 'early'

[boonebgorges]

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?

[DJPaul]

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.

[DJPaul]

Thoughts on above?

[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.

[boonebgorges]

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]

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.

[r-a-y]

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.

[DJPaul]

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).

[DJPaul]

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.

[boonebgorges]

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.

[DJPaul]

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

[boonebgorges]

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.

[boonebgorges]

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.

[boonebgorges]

In 12274:

Correct array syntax from [12273].

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

See #7476.

[boonebgorges]

In 12275:

Fix another PHP 5.3 syntax error after [12273].

See #7476.

[boonebgorges]

In 12276:

Fix another PHP 5.3 syntax error after [12273].

See #7476.