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

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

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.

Post reassignments cannot reasonably occur in the network admin.

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

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?

I do now :-)

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

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.

Related: #15956

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.

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.

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

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.

Tested on rev 17134.

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

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

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.

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.

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?

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.

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.

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

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

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.

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

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.

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.

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.

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.

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.

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()?

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.

Related: #16361

Tried testing, but the patch fails to apply.

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.

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