This patch modifies user.module to make the presentation of access permissions easier on the eyes and brain.

I have several custom node types on my site, and more on the way. As a result, it's tedious to wade through the access permissions for the node module on the http://example.com/admin/user/access page. Specifically, the permissions are sorted alphabetically, not by node type. Each new node type I add separates "create foo content", "edit own foo content", and "edit foo content" farther away from each other in the permissions list. It's very easy to make a mistake when setting different permissions for different content types.

The attached patch adds a fieldset and checkbox to the http://example.com/admin/user/settings page. If the checkbox is cleared, the asort($permissions) calls in user_admin_perm() and user_filters() are skipped. As a result, while /admin/user/access still has the modules sorted alphabetically, the permissions within each module are listed in the order in which they were defined. That does two good things:

  • The permissions for a node type all appear together in the node module list
  • The three permissions create, edit own, and edit appear in the same order for each content type. (They previously changed depending on whether the node type name's initial sorted before or after 'o'.

To avoid catching people by surprise, the default setting enables alpha sorting; out-of-box behavior is the same as before.

Also, the settings page was already a bit unwieldy, and this addition doesn't help. So, for a bit of chrome, the patch also marks that page's fieldsets collapsible, and collapses User e-mail settings by default.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

RobRoy’s picture

Status: Needs review » Needs work

Good idea, but you've got a bunch of other junk in that patch.

ewhipple’s picture

Status: Needs work » Needs review
FileSize
6.79 KB

You're right; sorry. I left in a tweak that I made to accomodate my Drupal5 port of me.module. The other stuff all belongs in, though. The changes are small, but there are several of them spread through the file. And, there's a bunch of extra context in there from diff -upNr. I'm new at patches; are those wrong options?

Here are each of the patch hunks explained. Line numbers are from the revised patch attached here.

  • 6-7: CVS tag line chopped down to $Id$
  • 15-17: function user_admin_perm() changed to block sorting on the access control page
  • 25-26, 32-33, 41-42: function user_admin_settings() tweaked in three places to make fieldsets collapsible
  • 50-53: new fieldset added to function user_admin_settings()
  • 61-64: function user_filters() changed to block sorting, for consistency with access control page
ewhipple’s picture

(side note) I'm confused. The filename on my first attachment got changed to the name of my second attachment, although the contents did not change. And the name on my second patch was changed with a '_0' duplicate-file marker. Is this expected behavior?

RobRoy’s picture

Status: Needs review » Needs work

Yeah diff -up are the options you want and you don't need to touch that $Id$ string at the top. You want to diff it against cvs not an older file too.

RobRoy’s picture

ewhipple’s picture

I can do that. Thanks for the link. I don't have CVS set up yet, so the revised patch will be delayed a bit.

ewhipple’s picture

FileSize
8.4 KB

Here it is. I diffed against CVS, from my Drupal root directory, with -up.

I added a named constant, USER_DEFAULT_PERMSORT, for the default value of the Drupal variable user_sort_access_perms. And, the new feature's collapsible fieldset on the settings page is expanded if a non-default setting is in place.

I tested three cases: out of the box settings (checkbox checked, variable not set), checkbox checked and saved, and checkbox cleared and saved. The sort behavior in each case was correct on example.com/admin/user/settings and in the "permission is" dropdown on /admin/user/user. The status settings checkbox display and fieldset collapse were also correct.

ewhipple’s picture

Status: Needs work » Needs review

Forgot to update status.

ChrisKennedy’s picture

Status: Needs review » Needs work

"t('Change lots o\' settings here.')" - why is this included?

The description translation needs to be merged into a single string.

The constant comment incorrectly refers to the value as a "return value", but this is a constant, not the return value of a function.

Also, please review the Drupal coding standards (http://drupal.org/node/318), especially string concatenations.

ewhipple’s picture

Version: 5.0-rc2 » 5.0
FileSize
8.03 KB

Well, dern! Sorry for the inconvenience. The "lots o'..." was left over from something else, and I forgot to remove it.

I've revised the comments for the named constant. It is used for the default return value of variable_get() in several places; I phrased the explanation poorly.

Thank you for the tip about the description text. I hadn't realized t() was so flexible.

ewhipple’s picture

Status: Needs work » Needs review
ewhipple’s picture

Status: Needs review » Needs work

Gee whiz, what a mess.

Disregard patch 2, it's not made from the Drupal root directory.

ewhipple’s picture

Status: Needs work » Needs review
FileSize
8.06 KB

I think I got it right this time.

drumm’s picture

Version: 5.0 » 6.x-dev
catch’s picture

Title: Turn off alpha sorting of permissions on Access control page » Group node create/edit/delete permissions by node type instead of alpha
Category: feature » bug
Status: Needs review » Needs work

Patch no longer applies, but I agree this'd make it much easier to deal with updates to node permissions.

Reclassifying as a bug - create/edit own/edit all/delete should appear in the same order for each node type, not dependent on whether it starts with letters before or after "o". With more and more node types on the average drupal site, this is only going to get harder as time goes on.

Also the proposed sort order should be made default behaviour rather than optional, which would also reduce it to a very small patch.

Pancho’s picture

FileSize
648 bytes

Okay, I rerolled the patch against HEAD.

Just as expected by catch, if we make the new ordering default and leave out all the other cruft, it's gonna be a very small patch. Actually it's just one line... :)

Think, we should make a step forward. Hope the patch works!

catch’s picture

Status: Needs work » Needs review
catch’s picture

Status: Needs review » Reviewed & tested by the community

Applied cleanly, made some new content types "Apple" and "Zebra", all permissions are now grouped sanely together. RTBC.

Pancho’s picture

Just to remember: As soon as this is committed, we need to inform module developers, that the permissions order inside their modules does matter now!

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

Ehm, I don't think this is any good idea. Why rely on contrib people doing something right or not, when we could sort as we need?

This issue is about grouping node type permission names, so let's focus on this. We know how node type permission names are built, because, well, they are built in node module programatically. So we can order on them, even if the ordering needs some regular expressions to do, to catch node type names in the middle of the permission name.

Pancho’s picture

Gábor is surely right in that this issue initially was about node module permissions.
But problems like this do arise in quite a number of contrib modules as well: If any module introduces more than one permission, it is simply not possible for the author to sort these permissions in a meaningful way.

The other point is: With contrib modules, we anyway rely on the diligence of contrib developers, not just concerning the names of permissions but also in several more delicate areas like performance, security or compatibility with other modules. So that should not be a reason to refuse contrib developers the tools they need.

For these two reasons I'm against a "lex node.module" and for a generic solution.

Jody Lynn’s picture

I like the patch as it is, applying to all modules.

I can't think of a time when having the permissions alphabetically is really an advantage (it is most likely that you don't know the correct wording of the permission you're looking for and you just read through your options for each module.)

Keeping the permissions in the order set by the modules seems preferable to me- it simply solves the node permissions issue and is a slight improvement for other modules as well. The worst thing that can happen this way is that the permissions are listed in an arbitrary order for most modules, but as it is they are currently being forced into a pretty useless order anyway.

catch’s picture

Title: Group node create/edit/delete permissions by node type instead of alpha » Usability: Group node create/edit/delete permissions by node type instead of alpha
Status: Needs work » Needs review

This still applies with offset, and it still makes a massive difference to the permissions page. Even more noticeable with the permissions standardisation on blog etc. which has increased the size of that list quite a bit.

contrib modules can do all kinds of crazy things anyway, like naming their permissions "zzzzz" "aaardvark" or whatever, so putting them in the wrong order ought to come under "don't do that then" imo.

Marking back to reivew.

Gábor Hojtsy’s picture

Well, the core modules themselves have their permissions in inconsistent orders, so I am not sure letting them be listed as-is is a good idea, it still requires you to take considerable attention. I don't have an exact list but you can check :)

catch’s picture

Status: Needs review » Needs work

Poll module, of course.

So how about standardising the order of poll.module's permissions? Would that be acceptable to get this in? regexp seems like overkill and the only situation I think this makes a big difference is with node permissions (since they're so similar).

Jody Lynn’s picture

Version: 6.x-dev » 7.x-dev
sun’s picture

Component: user.module » node system
Status: Needs work » Needs review
FileSize
7.05 KB

+1. New patch attached, re-ordered permissions to administer, create, edit, delete, access, etc.

sun’s picture

Sorry, wrong branch.

sun’s picture

Re-rolled patch against HEAD.

mikey_p’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
5.97 KB

I've tested this and it makes much more sense to see permissions grouped by node type, and further, to be able to specify their order.

With recent usability testing this patch seems like a great fit, as it does two things:

  1. no longer forces you to use a name that doesn't describe your permission accurately just to get a desired sort,
  2. allows you to re order the permissions within your module so that end users see things where the expect them.

This patch is just a reroll against head (was applying with offset).

drewish’s picture

subscribing... seems like a good change to me.

sun’s picture

*bump*

This can be backported to D6 and D5 as well.

Moonshine’s picture

+1 on this and application to D6 also.. Much easier on the brain. :)

yoroy’s picture

Can anybody post a screengrab of what this patch does please? Thank you.

catch’s picture

FileSize
20.64 KB
21.1 KB

Still applies with offset. Saves a lot of hunting around in the node permissions to find what you're after.

screenshots attached.

kika’s picture

Component: node system » usability

I'd even go as far as adding additional subheaders/grouping gfx/dividers to separate node types in this section, but let's start gradually.

gaele’s picture

Scanability could still be improved, either by moving the node type to the front

article: create content
article: delete any content

or by grouping as kika mentioned above:

article:
    create content
    delete any content
yoroy’s picture

I also think grouping would be a good idea. Then zebra-stripe the background colors on the groups instead of the single items.

lilou’s picture

Patch cannot apply (hunk @@ 7) in modules\user\user.admin.inc ... re-roll to CVS HEAD

mikey_p’s picture

Category: bug » feature

Patch still applies...

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Marking CNW, due to comments by the UX team.

Grouping is a worthy goal, but I don't think it's possible without a lot of underlying work, which would need to be done in a generic way and really thought about, so we should pick that up in a separate patch. But I definitely agree with gaele that:

article: create content
article: delete any content

Is much more scannable.

webchick’s picture

And to make it so we don't need to change existing permission names, we could also perhaps do:

Article: create article content
Article: delete own article content

Basically, same patch, just preface the permission names with the node type name.

(Note: This is based on a review of catch's patches @ #35.)

drewish’s picture

webchick, i'm not sure i understand what your concerns are about the patch.

how do you propose linking the the node type label to the permission? i don't see an easy way to do that and leave hook_perm generic enough to work with non-node bases permissions.

catch’s picture

Status: Needs work » Needs review

Like drewish, I don't see a clear way to prefix the the node types to the permissions - certainly not in a couple of lines, and apart from re-ordering permissions in some core modules, this is a one line patch. So IMO this is still an improvement over the current ordering, and while prefixes are a nice idea we could do that in another issue. Marking back to needs review.

If someone thinks the re-ordering would be worse than the current situation where revision permissions are mixed in with create/edit/delete and things are in completely unpredictable order I'd like to see them ;)

yoroy’s picture

Status: Needs review » Reviewed & tested by the community

Just from reading the discussion here, I'd also say that if prefixing is a big hassle we should leave it and commit this as is. Just the grouping by nodetype itself is a big improvement. (rtbc based on the patch still applying above, havent actually tested it)

webchick’s picture

Status: Reviewed & tested by the community » Needs review

So my goal behind setting this CNW was because I really want us to think about patches that affect the UI. When members of the UX team team chime in and say, "No, it really would be better like X" we, the development community, need to take that seriously. This is necessary in order to make D7 kick the ass of every Drupal release before it in terms of ease of use.

However, in talking to sun and in reading the patch more in detail today, you can make the argument that this is simply fixing a logic problem regarding the ordering of permissions; it doesn't touch UI per se. It's also a simple enough patch in terms of what it changes that it could be back-ported to D5 and D6, either officially or by sites who wanted the improvement and weren't scared to apply a patch.

So with this re-evaluation in mind, and with yoroy's follow-up, I'm game for committing this patch. I still would really like to see a follow-up issue for kika and gaele's grouping suggestion for this page. We probably need some plumbing patches in like #31535: Grouping of rows in theme_table() first. We should also think about how best to standardize the groupings so we don't end up with another huge CF like the modules page. :P

We should document this standard in the Update guide, and also in our UI guidelines that the UX team is working on. It looks like the general convention is:

- administer-related permissions
- access-related permissions
- create-related permissions
- delete-related permissions
- edit-related permissions
- other random crap

Two side-effects of this patch that I don't like:
1. Revision-related permissions now appear *above* the content-type permissions, which are much more common to set.
2. I don't think permissions make sense in the order of create, delete, edit. I think create, edit, delete (increasing order of scariness) makes a lot more sense.

Is it possible to fix those two things easily?

sun’s picture

FileSize
17.27 KB
sun’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
7.27 KB

Now, re-ordered: create, edit, delete.

And who of you saw that 'delete any' came in front of 'delete own'...? :-D

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome. Thanks!

webchick’s picture

Status: Fixed » Needs work

Actually, CNW until the docs are done.

We discussed on IRC the fact that the revision-related permissions had moved up, and decided this makes sense, since they are global permissions like access and administer.

Also note that edit/delete have been switched.

sun’s picture

Version: 7.x-dev » 6.x-dev
Status: Needs work » Needs review
FileSize
5.58 KB

Can we keep the pace? ;)

Gábor Hojtsy’s picture

@sun: well, I would think twice before committing this to Drupal 6. It requires update docs and different thinking to build arrays in _perm() hooks for Drupal 7, and we have no practical way to enforce it in Drupal 6. In other words, while we can "fix" Drupal core, contrib will be broken.

drewish’s picture

Version: 6.x-dev » 7.x-dev
Status: Needs review » Needs work

since it doesn't sound like this will be back ported to d6, i'm bumping this back to 7.x so the docs get updated as per angie's request.

webchick, what type of docs did you have in mind? upgrade instructions? changelog?

sun’s picture

I disagree with this decision. If you scroll back to top, you'll see that this issue existed for 5.x already. And, personally, I think that the order of permissions is a pain. We ensured that this patch performs minimum changes, so it can be back-ported easily. This patch does not break anything. Instead, it fixes a really annoying bug in content-type permissions in terms of usability, and allows module maintainers to have full control over the order of displayed permissions. IIRC, we had usability goals already for D6, and if I get this rejection right, we are preventing another usability improvement just because a contrib module may have its permissions listed in a confusing order - albeit contrib release cycles are much shorter than core's...

drewish’s picture

sun, my immediate problem in this issue is that you hijacked the thread from a "finish up the 7.x stuff" focus into a "lets try to push this into 6.x" focus. until 7.x is taken care of i'm going to keep reverting the status. while the 6.x behavior isn't good, it isn't a bug, so the change could be a feature change and typically those don't get committed to stable branches. you're welcome to apply the patch to any of your sites.

webchick’s picture

webchick, what type of docs did you have in mind? upgrade instructions? changelog?

Just update instructions I think. Probably not worth a changelog entry, since it's a minor change. Let people know that the permissions list is no longer sorted alphabetically, and it's now sorted in the order permissions are defined, and the recommended order for those permissions to be defined in is X.

mikey_p’s picture

catch’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Patch (to be ported)

Moving this back to d6 for review to see if we can persuade Gabor. I'd quite like to see it get in because I personally find it a hassle to spot the correct permissions in their current order, but changing them on people mid-cycle perhaps isn't such a great idea.

catch’s picture

I've also added a followup issue for better grouping of permissions in general per webchick's comments in #46 #310494: Allow grouping of permissions

Gábor Hojtsy’s picture

Don't get me wrong. I completely agree that it is better to have more logical grouping for permissions. It is just that I/we've got quite a wave of negative feedback from webchick, add1sun et al. on the installer change in the 6.x cycle, citing that books, video tutorials are out on the basics of Drupal 6 and we should not change behavior mid-version. That was a security problem, so there were not many choices to proceed. So while we might fix a usability issue, we also introduce a usability issue for those reading books, using video tutorials, etc. since they will not be able to match what they see on the site to what they see in the tutorial.

Here again the question is whether (1) users will be horrified that their permission page does not work anymore as it did ("omg, someone cracked my site?"), and (2) the available documentation (books, videos, etc) already cover, explain, show screenshots, etc. of how it works (I've just checked Packt's Drupal 6 book, and it has some nice screenshots on the access control page, which would not match anymore). We did get (unpleasant) user feedback on the installer issue again that the book they bought is useless (in this part) since we changed behaviors mid-version. So there is always a tradeoff in fixing usability issues while introducing others. It would be good to get a bit wider feedback on the issue.

webchick’s picture

As someone who freaked out about the installer issue, I do think this is different. The installer change:

  1. Required two extra steps in a set of instructions, thus invalidating all existing documentation, videos, books, etc.
  2. Had completely unhelpful help to explain to users what to do, increasing the support burden on our contributors.
  3. Greeted people with a BIG SCARY ERROR the second they attempted to install Drupal. We did a bunch of work during RC1 with the files directory to specifically *avoid* showing big scary errors to people first thing, since it was shown that this completely blocked them from installing Drupal without "VCR kid" to help them.

This, on the other hand, re-orders some stuff on the page.

But, it is a UI change, and those should always be considered very seriously for stable releases, regardless if it's the greatest usability improvement in the world or not.

However, since this page is already cluttered, it probably takes a freak like me that memorizes where things were to begin with and starts whining when they move around (#46 ;)), so we are probably safe to commit this to D6.

j_prentiss’s picture

Component: usability » other

Since I found this page searching for a way to change the confusing order of content_permissions, I gather this patch was never committed to D6.

I thought the original idea of a default sort order that matched the current mangled sort order, with an option to change the order on display, seems to get around the problem of installers and tutorials. In other words, if you do nothing, it works just like the existing documentation. If you change it, it's pretty obvious that you've gone off-script with a feature added since the documents were published.

Maybe the option to even see a button to change the sort order could be enabled in a separate admin settings page?

ps 'usability' not a valid component anymore? sorry to change any values

yoroy’s picture

Issue tags: +Usability

usability component deprecated indeed. We tag now! :-)

sun’s picture

Version: 6.x-dev » 7.x-dev
Status: Patch (to be ported) » Closed (fixed)

After having the pleasure to discuss many usability topics with various users and also the usability team in the past months, I gained a better understanding for questions like this now. Short form: Hell would break loose if we would commit this change to 6.x.

Reverting version and status.