WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 5 months ago

#40113 closed defect (bug) (fixed)

Change User Roles on Multisite

Reported by: eangulus Owned by: jeremyfelt
Milestone: 4.8 Priority: normal
Severity: normal Version: 3.1
Component: Users Keywords: has-patch
Focuses: multisite Cc:

Description

Found bug that can be replicated as follows.

Clean install of WordPress 4.7.3, setup as multisite using subfolders

Add a sub site then add a user to subsite.

Go to Network Dashboard, Sites then edit subsite.

Go to users. Tick a user and select a new role using the droplist at the bottom of the users list. Click change.

This gives an error of Sorry, you are not allowed to give users that role.

Do the same as above but use the droplist a button at top of users list, and it works as expected.

Attachments (2)

40113.patch (1010 bytes) - added by desrosj 7 months ago.
40113.diff (941 bytes) - added by jeremyfelt 5 months ago.

Download all attachments as: .zip

Change History (20)

#1 @swissspidy
7 months ago

  • Component changed from General to Users

#2 follow-up: @desrosj
7 months ago

  • Keywords needs-patch added

Hi @eangulus! Thanks for the detailed bug report.

I am able to reproduce the issue you described. Weird that it is only happening when using the role dropdown at the bottom of the table.

#3 in reply to: ↑ 2 @eangulus
7 months ago

Replying to desrosj:

Hi @eangulus! Thanks for the detailed bug report.

I am able to reproduce the issue you described. Weird that it is only happening when using the role dropdown at the bottom of the table.

You're welcome. Feels good after so many years of using WordPress I am finally able to do my small contribution of reporting my first bug.

Very strange indeed, spent many hours trying to work out what the problem was. In the end the developer of User Role Editor and myself worked it out.

@desrosj
7 months ago

#4 @desrosj
7 months ago

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

The attached patch copies the logic in wp-admin/users.php for consistency.

The current logic will always respect the second dropdown field, though. I can see some scenarios where this would create undesired changes.

Say a person is at the bottom of the page and selects a role. Then they scroll up selecting users, and then select a different role in the dropdown at the top. The bottom dropdown value would be applied to all selected users. This could be very bad, and a huge pain if a large number of users had been selected.

To reproduce, select a role in the dropdown at the top of the table, and then select a different role in the bottom dropdown. Click change in either row, and the second dropdown is the value respected.

Some potential solutions to solve this.

  • Use JavaScript to always have the values of these fields match (change one, change the other).
  • Always respect the first dropdown. It seems that Bulk Edit currently uses this logic.

This ticket was mentioned in Slack in #core-multisite by desrosj. View the logs.


7 months ago

#6 follow-up: @desrosj
7 months ago

Looked through the commit history https://core.trac.wordpress.org/log/trunk/src/wp-admin/network/site-users.php, this has not been working for at least 4 years, since the file was moved into src from trunk.

This should get patched, but I will open a separate ticket for a broader discussion of about the top and bottom dropdown fields.

Last edited 7 months ago by desrosj (previous) (diff)

#8 in reply to: ↑ 6 @netweb
7 months ago

  • Version changed from 4.7.3 to 3.1

Replying to desrosj:

Looked through the commit history https://core.trac.wordpress.org/log/trunk/src/wp-admin/network/site-users.php, this has not been working for at least 4 years, since the file was moved into src from trunk.

I dug a little deeper, the code you're looking for was introduced in r16560 ~6 years ago as part of WP 3.1

This should get patched, but I will open a separate ticket for a broader discussion of about the top and bottom dropdown fields.

Related from 4.6 #35307

#9 @johnbillion
6 months ago

  • Milestone changed from Awaiting Review to 4.8
  • Owner set to johnbillion
  • Status changed from new to reviewing

#10 @flixos90
6 months ago

While discussing this a couple weeks ago during the weekly multisite chat, we also thought about the option to add some lines of JavaScript to keep the two dropdowns in sync. In general, especially if we don't do it, we need to think which value should take precedence (new_role or new_role2).

#11 @theMikeD
6 months ago

Ran into this bug this morning. Posting here to track it.

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


6 months ago

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


5 months ago

#14 @johnjamesjacoby
5 months ago

Patch here seems OK to me at a cursory.

On the whole, I don't like that we have role assignments on the bottom in the first place. Like media/posts/pages, it seems more like a top-only thing to me.

Or worse, role assignments is unique in that it's not a view controller like list-table filters are, so they may deserve their own unique UI to separate them from Filters entirely.

#15 @johnjamesjacoby
5 months ago

FWIW, I remove the bottom ones in my WP Pretty Filters plugin.

https://files.slack.com/files-tmb/T024MFP4J-F5E93MPNZ-25292d6f9e/screen_shot_2017-05-16_at_11.16.46_am_1024.png

#16 @jeremyfelt
5 months ago

  • Keywords needs-testing removed
  • Owner changed from johnbillion to jeremyfelt

The approach in 40113.patch makes sense as an immediate fix as it aligns wp-admin/network/site-users.php with wp-admin/users.php.

I do think it's worth exploring new behavior in another ticket. Like @johnjamesjacoby mentions, removing roles entirely at the bottom of the list table may be a good answer.

@jeremyfelt
5 months ago

#17 @jeremyfelt
5 months ago

40113.diff aligns closer with wp-admin/users.php.

#18 @jeremyfelt
5 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 40780:

Multisite: Handle both role change selections in site-users.php.

Previously, a role selected below the list table would not apply on save. This aligns the behavior with wp-admin/users.php and allows role changes from both selections, deferring to the bottom selection when both are populated.

Props desrosj.
Fixes #40113.

Note: See TracTickets for help on using tickets.