Make WordPress Core

Opened 11 years ago

Closed 9 years ago

Last modified 9 years ago

#22959 closed enhancement (fixed)

Show all roles in user list table

Reported by: scribu's profile scribu Owned by: johnbillion's profile johnbillion
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: Role/Capability Keywords: has-patch
Focuses: administration Cc:

Description (last modified by scribu)

From [22686]:

we should show a comma-separated list of all roles, editable or otherwise, and this list should be filterable, either by user, or by the roles which can appear. Probably both.

Previously: #22361

Attachments (6)

22959.diff (3.0 KB) - added by scribu 11 years ago.
22959.1.diff (2.9 KB) - added by JustinSainton 11 years ago.
22959.2.diff (3.3 KB) - added by JustinSainton 11 years ago.
22959.3.diff (4.2 KB) - added by DrewAPicture 11 years ago.
22959.4.diff (3.6 KB) - added by DrewAPicture 11 years ago.
array return
22959.5.diff (4.4 KB) - added by johnbillion 9 years ago.

Download all attachments as: .zip

Change History (42)

@scribu
11 years ago

#1 @scribu
11 years ago

  • Description modified (diff)

#2 @scribu
11 years ago

  • Description modified (diff)

#3 @deltafactory
11 years ago

  • Cc jeff@… added

Where should filtering be added, and to what end? If we're expected to find existing elements in the array, perhaps it should be filtered pre-translation. If not, this at least gives an opportunity to add elements.

Before: return implode( ', ', $role_str );

After: return implode( ', ', apply_filters( 'wp_user_list_get_role_str', $role_str, $user_object ) );

If the intention was to modify the column's output after the implode(), another filter would be necessary.

#4 @phill_brown
11 years ago

  • Cc phill@… added

#5 @greenshady
11 years ago

  • Cc justin@… added

#6 @MikeHansenMe
11 years ago

  • Keywords needs-refresh added

#7 @JustinSainton
11 years ago

  • Keywords needs-refresh removed

#8 @mordauk
11 years ago

  • Cc pippin@… added

#9 @ryan
11 years ago

  • Milestone changed from 3.6 to Future Release

#10 @deltafactory
11 years ago

I know this ticket is now for a future release. I'm still interested in the inclusion of filtering of the roles, pre-translation. Am I the only one, or should I submit a patch to that effect?

#11 @JustinSainton
11 years ago

Refreshed against trunk, added inline docs, added filter for get_role_str method. Not über-sold on the filter name, but it definitely feels like a filter worth having.

#12 @DrewAPicture
11 years ago

  • Cc xoodrew@… added
  • Milestone changed from Future Release to 3.7

Replying to JustinSainton:

Refreshed against trunk, added inline docs, added filter for get_role_str method. Not über-sold on the filter name, but it definitely feels like a filter worth having.

Not really sold on the method name either. And since the filter is evaluated on the return, it should match the method name.

22959.3.diff:

  • Changes the function/filter name to get_role_list()|get_role_list
  • Accounts for back-compat on role-named classes on single row checkboxes
  • Fleshes out the doc block.

Side note: Is it kosher to just remove the $role param from the method in this type of context? Just curious.

Last edited 11 years ago by DrewAPicture (previous) (diff)

#13 @JustinSainton
11 years ago

I had a similar curiosity there. It's not likely that a ton of people were overriding or calling this method directly. Not entirely sure what the core standards are for using _deprecated_argument() (As in, I'm not sure if that is always done, or if it's only done in specific cases.). Left it that way because @scribu did it that way in the original patch and he's wicked smart.

Last edited 11 years ago by JustinSainton (previous) (diff)

#14 @nacin
11 years ago

Rather than removing the argument, let's continue to leave the role key there.

Otherwise, this looks good. Might need a slight cleanup. Can get_role_list() instead return an array of role key => role name? Then the classes don't need to be a faux-sanitized version of that, and we can do the implode on the fly.

@DrewAPicture
11 years ago

array return

#15 @DrewAPicture
11 years ago

  • Keywords dev-feedback added

22959.4.diff implements @nacin's suggestions in comment:14.

#16 @nacin
11 years ago

  • Keywords commit added; dev-feedback removed

Looks good. I feel it may be a bit late for this kind of an enhancement — marking for 3.7 commit but may end up later today being moved to 3.8.

#17 @nacin
11 years ago

  • Milestone changed from 3.7 to Future Release

It has occurred to me that there is a distinct possibility that a plugin that implements multiple roles might not want subsequent roles to actually be displayed. Maybe this should start with one role and be filterable to add additional roles. Or maybe roles should be marked as public or private. Or maybe who cares and 22959.4.diff is good. A role name could be omitted, I guess — add_role( 'some-slug', false ) (or ''). Might that be enough to consider a role to be "private"? Should we only list roles that are returned by get_editable_roles()? Just spitballing here. (As in, I would advise not bothering to patch any which way until a discussion comes to a consensus.)

Let's try to resolve this in 3.8, as I know it'll benefit plugins like bbPress.

(As is customary for a roles/caps ticket, see also #10201.)

#18 @deltafactory
11 years ago

Wouldn't the presence of this new filter give a plugin the opportunity to hide those private/unimportant roles at their discretion?

I think that seeing the full list by default is an important diagnostic when more complex role structures are in use.

#19 @tnash
9 years ago

Just wanted to re-ignite this ticket, recently came across a good example of where multiple roles and the way they were displayed could cause issues. If a user has both a subscriber role and an administrator role (for example the admin role added via wp-cli) then they are showing in the admin area user list with the role subscriber. While they do show up under administrator filter again, with role subscriber.

This can easily be confusing, and also allow someone to obfuscate their real role (I'm aware if someone could add a role, they can as easily add capabilities but still)

If nothing else showing the role with the most number of capabilities or if a role has edit options capability give it precedence over others.

#20 @johnbillion
9 years ago

  • Keywords commit removed
  • Milestone changed from Future Release to 4.3

#21 @MikeHansenMe
9 years ago

  • Keywords needs-refresh added

#22 @obenland
9 years ago

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

#23 @chriscct7
9 years ago

  • Focuses administration added

I'd be happy to take this over if @johnbillion wants a rescue on this one.

I think there's a couple key questions that need to be answered on this:

  • How many roles are shown by default? All of them is ideal unless you get 1 user with 50 roles and everyone else has 1. I think this would be so edge case core wouldn't need to worry about it, and could be avoided with a little overflow scroll thing if it gets nuts.
  • How do we let plugins limit their roles from showing on this page? I think two key ways. First a filter on what shows there, for that specific screen. But also I think instead of doing a "public" key on add_role, do "capability" so if current_user doesn't have that cap they can't see/add/edit/remove the role. Then if a plugin wants to hide a cap from everyone, they could use a capability that doesn't exist. This would let plugins build out dynamic capabilities.
  • Does the UI natively support multiple roles? I think it would make plugins lives incredibly easy if either WordPress core natively supported a Select2 multiselect like UI out of the box (yet another reason it'd be cool to land #31696 in 4.3) for adding to user's roles on the edit user page, or had a filter where plugins could flip it on. The absense of this would be something every plugin who wanted to use this (from EDD to Woo to bbPress to everyone using this) have to go out and implement this, and each would have to do it individually. And it would let WordPress show Super Administrator and Administrator on the multisite user listings page, which would be cool.
  • If WordPress starts supporting this, a user_has_role function could be added that aliases to user_has_cap so that down the road roles could be decoupled (in terms of not evaluating as a capability) if core wanted to.

Those are the ones I could remember at the moment.

#24 @netweb
9 years ago

Related: #17924

I think it is safe to say #17924 is related in the broader sense of how a plugin integrates roles.

I'd also suggest install and/or reference bbPress' implementation for whoever tackles this ;)

https://cldup.com/7nz6_lBsbw.png

#25 @chriscct7
9 years ago

  • Keywords ux-feedback added

The problem with bbPress's implementation it it scales badly when you have > 3 roles for a user. On the other hand, we can keep it simple in core and just list all roles in the existing role column and if a plugin like bbPress wants a specialized column it could continue to do so (perhaps using a filter to remove its' role from the overall Role(s) list.

#26 @obenland
9 years ago

With beta around the corner and this ticket not having seen action in 5 weeks, this is a strong candidate to not make it into 4.3.

#27 @chriscct7
9 years ago

Unless @johnbillion wants to remain owner and/or keep this on 4.3, I'd be willing to own this for 4.4

#28 @obenland
9 years ago

  • Milestone changed from 4.3 to Future Release
  • Owner changed from johnbillion to chriscct7

#29 @chriscct7
9 years ago

  • Keywords ui-feedback added

This could possibly be a UI/UX team focus area for 4.4 if someone wants to work on that. Will propose during today's meeting for that in Slack.

It's easy to show all the roles, but if we're going to show all of them, and I think it should, then some of the controls like change role should be reworked, to maybe something like GitHub's system for choosing tags for bulk issues where you select the tab and then hit add or remove. Also the role changer on the edit user screen should be turned into something like a Select2 Multiselect field or something like that to more easily support multiple roles.

This could easily tie into #19867 & #31696

#30 @chriscct7
9 years ago

  • Owner chriscct7 deleted

#31 @johnbillion
9 years ago

  • Keywords ux-feedback ui-feedback removed
  • Milestone changed from Future Release to 4.4

The benefits of showing all of a users' roles in this column (clarity and security) outweigh any negatives, such as the lack of a corresponding UI for controlling multiple roles for a user (#17924). Let's get this into 4.4.

@johnbillion
9 years ago

#32 @johnbillion
9 years ago

  • Keywords needs-refresh removed
  • Owner set to johnbillion
  • Status changed from assigned to accepted

#33 @mordauk
9 years ago

This makes me so happy.

#34 @johnbillion
9 years ago

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

In 34963:

On the Users list table, show all the roles of a user in a comma-separated list if they have more than one role. This prevents role obfuscation in situations where a user has had more than one role programmatically assigned to them.

Fixes #22959
Props scribu, JustinSainton, DrewAPicture, johnbillion

#35 @wonderboymusic
9 years ago

In 35011:

Users List Table: after [34963], remove unused code/add doc for global import.

See #22959.

#36 @wonderboymusic
9 years ago

In 35012:

Users List Table: after [35011], just use wp_roles(), no global import. #winning

See #22959.

Note: See TracTickets for help on using tickets.