WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 7 weeks ago

#15855 new defect (bug)

Dropdown isn't shown when doing a user 'removal'

Reported by: scribu Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch needs-testing ux-feedback
Focuses: multisite Cc:

Description (last modified by scribu)

Steps to reproduce:

  1. Have a MultiSite install
  2. Go to wp-admin/users.php and delete a user
  3. A confirmation screen appears, with only a "Confirm" button.

Expected behaviour:

The user dropdown is shown, asking to assing the user's posts to a different user.

Currently, you end up with authorless posts.

Attachments (6)

15855.patch (1.8 KB) - added by SergeyBiryukov 3 years ago.
15855-site-users-confirmation.patch (4.0 KB) - added by SergeyBiryukov 3 years ago.
15855.2.patch (9.4 KB) - added by SergeyBiryukov 3 years ago.
15855.3.patch (12.6 KB) - added by SergeyBiryukov 3 years ago.
15855.4.patch (13.7 KB) - added by SergeyBiryukov 3 years ago.
15855.5.patch (14.8 KB) - added by SergeyBiryukov 3 years ago.

Download all attachments as: .zip

Change History (54)

comment:1 nacin3 years ago

  • Milestone changed from Awaiting Review to 3.1

comment:2 nacin3 years ago

We need to figure out what is a regression here and fix that.

My thoughts:

  • A user can only be removed from a blog in the site admin. Actual user deletion must occur from the network admin.
  • On removing a user from a blog in the site admin, we need to be able to reassign their posts.
  • Post reassignments cannot reasonably occur in the network admin. Perhaps a warning that this user may have data in blogs they are attached to (if get_blogs_of_user() > 0) and that's it.

comment:3 scribu3 years ago

Post reassignments cannot reasonably occur in the network admin.

Funily enough, that's already what happens now: you get a dropdown for each site.

comment:4 scribu3 years ago

  • Description modified (diff)

comment:5 ocean903 years ago

It comes with r14176.

'Post reassignments cannot reasonably occur in the network admin. Perhaps a warning that this user may have data in blogs they are >attached to (if get_blogs_of_user() > 0) and that's it.

It's possible at the moment, you know that?

comment:6 nacin3 years ago

I do now :-)

SergeyBiryukov3 years ago

comment:7 SergeyBiryukov3 years ago

  • Keywords has-patch added; needs-patch removed

I've also discovered that after r17010 there should be $_REQUEST['user'] instead of $_REQUEST['reassign_user']. The patch fixes it too.

comment:8 scribu3 years ago

Patch works well.

Just found out that while on network/site-users.php, clicking on Remove gives you no confirmation at all.

Also, shouldn't the 'Remove' link be red? It's blue on all screens.

comment:9 scribu3 years ago

Related: #15956

comment:10 SergeyBiryukov3 years ago

Made an attempt to add the confirmation when removing users from network/site-users.php in the second patch. Would love to get feedback for learning purposes.

comment:11 SergeyBiryukov3 years ago

Per IRC discussion, we need to:

  1. Create different dropdowns for each user, like in network/users.php.
  2. Try to make the confirmation in wp-admin/users.php into a function and reuse it in site-users.php.

comment:12 SergeyBiryukov3 years ago

Revised the first patch to use the same confirmation function for deletion and removal. Will try to reuse it in network/site-users.php.

comment:13 SergeyBiryukov3 years ago

I've dropped a couple of current_user_can('delete_user', $id) checks in case 'dodelete' because there's already a check for that several lines earlier.

SergeyBiryukov3 years ago

comment:14 kcristiano3 years ago

Tested on rev 17134.

Before patch, could not reassign user. After works fine.

comment:15 scribu3 years ago

confirm_remove_users() should be moved to wp-admin/includes/user.php and renamed to _confirm_remove_users().

comment:16 follow-up: westi3 years ago

Not a big fan of a shared function like this when 80% of the code through is different for the two paths.

A shared function for the shared code maybe but this doesn't sit well in it's current state for me.

SergeyBiryukov3 years ago

comment:17 SergeyBiryukov3 years ago

Wrapped up into a single patch.

So we have confirmation screens with different dropdowns for each user when performing deletion or removal in users.php, deletion in network/users.php and removal in network/site-users.php.

comment:18 in reply to: ↑ 16 ; follow-up: SergeyBiryukov3 years ago

Replying to westi:

A shared function for the shared code maybe but this doesn't sit well in it's current state for me.

How can it be improved?

comment:19 in reply to: ↑ 18 ; follow-up: westi3 years ago

Replying to SergeyBiryukov:

Replying to westi:

A shared function for the shared code maybe but this doesn't sit well in it's current state for me.

How can it be improved?

Doing this switch ( $action ) { three times in the same function makes it really hard to ensure that any changes made in the future are done correctly.

I would make a shared function for the dropdown generation code and move the rest of the code back into wp-admin/users.php in the two different cases where your new function is called with the different actions.

comment:20 in reply to: ↑ 19 SergeyBiryukov3 years ago

Replying to westi:

I would make a shared function for the dropdown generation code and move the rest of the code back into wp-admin/users.php in the two different cases where your new function is called with the different actions.

It's also used in network/site-users.php, so a part of the code will be duplicated there too. Working on it.

SergeyBiryukov3 years ago

comment:21 SergeyBiryukov3 years ago

Revised the patch. I agree that it makes more sense this way.

comment:22 SergeyBiryukov3 years ago

  • Keywords i18n-change added

I've added a new string to indicate the number of users removed.

comment:23 nacin3 years ago

I've just gone through these processes in 3.0 and again in 3.1. To be clear:

  • Single site, wp-admin/users.php. Delete a user, you get the confirmation and you are allowed to reassign posts. Same for 3.0 and 3.1.
  • Multisite, wp-admin/users.php. Remove a user, you get a confirmation but are not allowed to reassign posts. Same for 3.0 and 3.1. Not ideal, and would be a good fix. needs-patch
  • Multisite, wp-admin/network/users.php (3.1) and wp-admin/ms-users.php (3.0). Delete a user, you get a confirmation and are allowed to reassign posts for each user. This is good.
  • Multisite, wp-admin/network/site-users.php (3.1). Remove a user, and you get no confirmation at all. This is bad. (Not a regression from ms-sites.php in 3.0, though, which had one ugly UI.) needs-patch

I am going to now evaluate the underlying code and the proposed patches and implement what I can, in the least intrusive way possible. The smaller amount of code churn, the better. I am less concerned about DRY code for 3.1 if that means that we risk breaking less things.

To be clear, there is no regression here, at all. I would be fine punting all together, but it's inconsistent enough (and damaging enough, as it causes authorless posts) to deserve a good look.

Last edited 3 years ago by nacin (previous) (diff)

comment:24 westi3 years ago

"authorless posts"

By this do you mean that the author gets set to 0 or that the author no longer has a role on the blog?

If the second I wouldn't be averse to moving this out of 3.1 as it is not a regression.

comment:25 follow-ups: nacin3 years ago

  • Keywords i18n-change removed

The author no longer has a role on the blog. The author ID remains the same in the DB, so restoring the user should restore their posts.

The next person who edits the post will end up inheriting the post, I imagine.

The one issue with punting is that it affects non-super admins on wp-admin/users.php, which is annoying. I am going to see if I can rework the code branching just a bit as we already have the code there for user deletion.

No problems with punting if you want to do so.

comment:26 in reply to: ↑ 25 ; follow-up: SergeyBiryukov3 years ago

Replying to nacin:

The one issue with punting is that it affects non-super admins on wp-admin/users.php, which is annoying. I am going to see if I can rework the code branching just a bit as we already have the code there for user deletion.

I guess the first patch should do it.

comment:27 in reply to: ↑ 26 nacin3 years ago

Replying to SergeyBiryukov:

Replying to nacin:

The one issue with punting is that it affects non-super admins on wp-admin/users.php, which is annoying. I am going to see if I can rework the code branching just a bit as we already have the code there for user deletion.

I guess the first patch should do it.

Perfect.

comment:28 follow-up: nacin3 years ago

  • Keywords 3.2-early added
  • Milestone changed from 3.1 to Future Release

Problem with the patch: It doesn't delete posts or links.

The problem is actually in remove_user_from_blog -- it doesn't do anything if $reassign isn't set. No posts are deleted. (This makes sense, as the user is still in the network.)

As westi pointed out, posts do not become authorless. The user is no longer associated with the blog in terms of permissions, but the post still retains the user association. It's basically like saying that the user has no role on the blog. It's actually not that big of a deal.

Thus, I'm punting this. I don't think forcing reassignment is necessary, and the other option -- Leave the user's posts alone -- just sounds weird. The current UX kinda sucks, but it isn't enough to deal with in a non-regressed state.

comment:29 in reply to: ↑ 25 westi3 years ago

Replying to nacin:

The next person who edits the post will end up inheriting the post, I imagine.

This is an annoying bug IMHO too - which is would be nice to resolve.

If you are using WP for a multi-author site where contributors come and go and then you go back and fix a typo in someone who has lefts post it becomes your which is a little annoying.

IMHO re-assignment should be optional on removal/deletion and we should try and fix the edit issue too.

comment:30 in reply to: ↑ 28 ; follow-up: SergeyBiryukov3 years ago

Replying to nacin:

The problem is actually in remove_user_from_blog -- it doesn't do anything if $reassign isn't set. No posts are deleted. (This makes sense, as the user is still in the network.)

Perhaps remove_user_from_blog() should then be modified to delete the posts for consistency with wp_delete_user()?

comment:31 in reply to: ↑ 30 nacin3 years ago

Replying to SergeyBiryukov:

Perhaps remove_user_from_blog() should then be modified to delete the posts for consistency with wp_delete_user()?

No, because that is expected behavior. They no longer have a role, but the user still exists in the network.

comment:32 SergeyBiryukov3 years ago

Related: #16359, #16360, #16361

Last edited 3 years ago by SergeyBiryukov (previous) (diff)

comment:33 designsimply3 years ago

  • Keywords needs-refresh added

Tried testing, but the patch fails to apply.

SergeyBiryukov3 years ago

comment:34 SergeyBiryukov3 years ago

  • Keywords needs-testing added; 3.2-early needs-refresh removed
  • Milestone changed from Future Release to 3.3

15855.5.patch is the refreshed patch.

Replying to scribu:

Also, shouldn't the 'Remove' link be red? It's blue on all screens.

Fixed.

Replying to nacin:

Problem with the patch: It doesn't delete posts or links.

The problem is actually in remove_user_from_blog -- it doesn't do anything if $reassign isn't set. No posts are deleted. (This makes sense, as the user is still in the network.)

In the current version I've just changed "Delete all posts and links" label to "Leave as is" for Multisite. Perhaps we should go further and introduce "Leave as is" as a new option, and actually delete posts when requested to do so.

comment:35 SergeyBiryukov3 years ago

  • Keywords ux-feedback added

comment:36 SergeyBiryukov3 years ago

Related: #17905 (deleting user's posts/links upon removal)

comment:37 SergeyBiryukov2 years ago

  • Milestone changed from 3.3 to Future Release

comment:38 SergeyBiryukov2 years ago

  • Keywords 3.4-early added

comment:39 scribu2 years ago

  • Keywords 3.4-early removed
  • Milestone changed from Future Release to 3.4

comment:40 ryan2 years ago

  • Milestone changed from 3.4 to Future Release

comment:41 jeremyfelt3 months ago

  • Component changed from Multisite to Users
  • Focuses multisite added

comment:42 SergeyBiryukov8 weeks ago

#27170 was marked as a duplicate.

comment:43 Ipstenu8 weeks ago

I couldn;'t find this one earlier, thanks SergeyBot.

From #27170 - Removing a user from a site should behave like deletion, with regards to post attribution. I should be asked if I want to delete posts, KEEP posts, or reallocate before I can remove a user from a site. (of course, what we do with keeping and how to give posts an author without an author is ... beyond my brain)

comment:44 knutsp8 weeks ago

-1

Removing a user that still exist (in the network) should not do anything with posts. This is a demotion of role, nothing else. Reassigning of posts, if needed, can be done by multi edit.

comment:45 Ipstenu8 weeks ago

Conversely, in SINGLE site if you remove a user, you are asked to delete their posts or reallocate. You can't leave the posts.

Removing a user from a site in the network removes them. They're not demoted, they have NO role.

Lack of parity here :)

comment:46 knutsp8 weeks ago

On a single site there is no "remove user", only delete. By "demote" I include setting the role to "no role", as the least (empty) form of access (login) possible.

comment:47 Ipstenu8 weeks ago

And that's the issue, knutsp :) If I want to delete a user and all his posts from my site on the network, that should be as easy as when I do it on Single Site. And if I want to remove a user but keep his posts on my single site, that too should be possible.

My 'workaround' has been to change the user on the network to have 'no role' on that site, so they cannot do anything save login (which is a network 'problem' in and of itself - not really a problem, but a thing).

comment:48 Cleanshooter7 weeks ago

The current inability to remove users from the network with out either deleting their posts or reassigning them to another user is a problem. When you have hundreds of sites and thousands of users network admins have no idea "who" to assign the post to. We just know that we need to delete a user (for security reasons) and we need to keep all the sites post data intact. Right now we can't do this.

I have yet to look at the code in the site-users.php on this to find out how this might be possible. Maybe adding the option to set the author field to a string instead of a user id? Or... It might be better if there were a field in the user table admins could set to change a users "active" status. This would be easy to add the the bulk actions. Or even a role for "inactive" users that would prevent them from logging in at all. There are a lot of ways to do it but we just need something.

http://wordpress.org/support/topic/delete-user-keep-posts-with-that-user?replies=2
http://en.forums.wordpress.com/topic/can-i-delete-a-user-without-also-deleting-their-content
http://wordpress.org/support/topic/delete-user-but-keep-who-wrote-the-post?replies=5
http://wordpress.org/support/topic/delete-user-but-keep-author-on-posts?replies=3
http://wordpress.org/support/topic/how-to-delete-user-and-keep-his-name-at-article?replies=2

There a lot of people that have recognized a need for this already. I'm kinda surprised its not on the todo list already.

I hope that you'll consider adding this to a future Milestone because as of right now this posses a security issue.

Note: See TracTickets for help on using tickets.