Make WordPress Core

Opened 13 years ago

Last modified 12 months ago

#15855 assigned defect (bug)

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

Reported by: scribu's profile scribu Owned by: jeremyfelt's profile jeremyfelt
Milestone: Priority: normal
Severity: normal Version:
Component: Users Keywords: needs-patch
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 13 years ago.
15855-site-users-confirmation.patch (4.0 KB) - added by SergeyBiryukov 13 years ago.
15855.2.patch (9.4 KB) - added by SergeyBiryukov 13 years ago.
15855.3.patch (12.6 KB) - added by SergeyBiryukov 13 years ago.
15855.4.patch (13.7 KB) - added by SergeyBiryukov 13 years ago.
15855.5.patch (14.8 KB) - added by SergeyBiryukov 13 years ago.

Download all attachments as: .zip

Change History (63)

#1 @nacin
13 years ago

  • Milestone changed from Awaiting Review to 3.1

#2 @nacin
13 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.

#3 @scribu
13 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.

#4 @scribu
13 years ago

  • Description modified (diff)

#5 @ocean90
13 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?

#6 @nacin
13 years ago

I do now :-)

#7 @SergeyBiryukov
13 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.

#8 @scribu
13 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.

#9 @scribu
13 years ago

Related: #15956

#10 @SergeyBiryukov
13 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.

#11 @SergeyBiryukov
13 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.

#12 @SergeyBiryukov
13 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.

#13 @SergeyBiryukov
13 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.

#14 @kcristiano
13 years ago

Tested on rev 17134.

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

#15 @scribu
13 years ago

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

#16 follow-up: @westi
13 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.

#17 @SergeyBiryukov
13 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.

#18 in reply to: ↑ 16 ; follow-up: @SergeyBiryukov
13 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?

#19 in reply to: ↑ 18 ; follow-up: @westi
13 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.

#20 in reply to: ↑ 19 @SergeyBiryukov
13 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.

#21 @SergeyBiryukov
13 years ago

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

#22 @SergeyBiryukov
13 years ago

  • Keywords i18n-change added

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

#23 @nacin
13 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 13 years ago by nacin (previous) (diff)

#24 @westi
13 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.

#25 follow-ups: @nacin
13 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.

#26 in reply to: ↑ 25 ; follow-up: @SergeyBiryukov
13 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.

#27 in reply to: ↑ 26 @nacin
13 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.

#28 follow-up: @nacin
13 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.

#29 in reply to: ↑ 25 @westi
13 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.

#30 in reply to: ↑ 28 ; follow-up: @SergeyBiryukov
13 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()?

#31 in reply to: ↑ 30 @nacin
13 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.

#32 @SergeyBiryukov
13 years ago

Related: #16359, #16360, #16361

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

#33 @designsimply
13 years ago

  • Keywords needs-refresh added

Tried testing, but the patch fails to apply.

#34 @SergeyBiryukov
13 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.

#35 @SergeyBiryukov
13 years ago

  • Keywords ux-feedback added

#36 @SergeyBiryukov
13 years ago

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

#37 @SergeyBiryukov
12 years ago

  • Milestone changed from 3.3 to Future Release

#38 @SergeyBiryukov
12 years ago

  • Keywords 3.4-early added

#39 @scribu
12 years ago

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

#40 @ryan
12 years ago

  • Milestone changed from 3.4 to Future Release

#41 @jeremyfelt
10 years ago

  • Component changed from Multisite to Users
  • Focuses multisite added

#42 @SergeyBiryukov
10 years ago

#27170 was marked as a duplicate.

#43 @Ipstenu
10 years 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)

#44 @knutsp
10 years 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.

#45 @Ipstenu
10 years 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 :)

#46 @knutsp
10 years 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.

#47 @Ipstenu
10 years 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).

#48 follow-up: @Cleanshooter
10 years 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.

#49 @wonderboymusic
9 years ago

  • Milestone changed from Future Release to 4.4

#50 @wonderboymusic
9 years ago

  • Keywords needs-patch added; has-patch needs-testing ux-feedback removed

This ticket was mentioned in Slack in #core by sergey. View the logs.


8 years ago

#52 @obenland
8 years ago

  • Owner set to jeremyfelt
  • Status changed from new to assigned

#53 @DrewAPicture
8 years ago

  • Milestone changed from 4.4 to Future Release

Based on the lack of a patch and somebody to own it, punting to Future Release.

#54 in reply to: ↑ 48 @thomaswm
8 years ago

Replying to Cleanshooter:

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.

Related: #34615.

#55 follow-up: @MadtownLems
4 years ago

I'm very much interested in the ability to, on MultiSite, remove a User from a subsite and re-assign their posts to a different User.

Before I spend way too much time on a patch for this, is it possible to confirm this functionality would likely get accepted for Core? I'm not sure if there have been any other discussions on this in the past 4 years...

#56 in reply to: ↑ 55 @SergeyBiryukov
4 years ago

Replying to MadtownLems:

I'm very much interested in the ability to, on MultiSite, remove a User from a subsite and re-assign their posts to a different User.

Before I spend way too much time on a patch for this, is it possible to confirm this functionality would likely get accepted for Core? I'm not sure if there have been any other discussions on this in the past 4 years...

Thanks for your interest!

I think refreshing 15855.5.patch would be a good first step. It allowed for reassigning posts on user removal, but not for deleting them, which can be tackled separately.

Note: See TracTickets for help on using tickets.