Usability: Group node create/edit/delete permissions by node type instead of alpha

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

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.

http://drupal.org/patch

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

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.

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!

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

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!

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.

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.

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.

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.

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 :)

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

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

Sorry, wrong branch.

Re-rolled patch against HEAD.

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

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

*bump*

This can be backported to D6 and D5 as well.

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

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

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

screenshots attached.

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.

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

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

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

Patch still applies...

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.

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

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.

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 ;)

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)

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: Allow table row groups in table.html.twig and template_preprocess_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?

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

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

Awesome. Thanks!

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.

Can we keep the pace? ;)

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

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?

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

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

http://drupal.org/node/224333#sorting-permissions

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.

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

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.

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.

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

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

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.