Usability: Provide descriptions for permissions

Patch doesn't apply anymore.

Rerolled patch... I like this idea!

+1 from me
I think this is a real usability improvement

+1 on the functionality, but a few problems. In particular, don't use <br /><small> to indicate the description text. That's hard-coding the formatting into the code, which is badness.

I've attached a revised patch that adds 3 things:

1. Uses <div class="description"> instead of <small> in order to follow the same convention as elsewhere in Drupal.
2. Adds a #permissions .description class to drupal.css to account for it, using the same rules as for form items. (Themes can of course override it with whatever they want.)
3. Adds some example text to node.module and page.module so that we can see it in action. :-) Ideally I think the Docs team would be the best people to come up with the proper text to use for various core modules, but this is good for now.

Please review!

Hm. Well, then upload.module needs to be fixed, too, IMHO. :-) Still, does this patch work? Can we get this committed, with or without using it in other core modules?

forms API is in.

Gr. Yeah, I guess the patch should be revised to the new Form API, but I don't know the new API yet. :-) Anyone who does, would you have time to update this? I'm afraid I won't for a while now, since I'd have to learn the new API first.

Right then, my first attempt at working with the new forms API. Well, not really, as I only had to add one half line that had anything to do with forms, but I had to reimplement the array handling algorithm since the loop structure changed for the new forms API. Whatever.

I'm not crazy about putting a div in there explicitly, but I'm not sure how else to do it. If someone who knows the form API better than I do has a better suggestion, please share!

Anyway, here's the new patch, edited for the new forms API and rolled against today's HEAD.

i didn't apply the latest patch, but the code looks good and i like the idea.

Hm. I don't know if I should take silence as approval or as disapproval. :-) I'd really like to see this get into 4.7. Can someone else actually try the patch and give a thumbs up and "ready to commit" status? I don't want to set that on my own patch. :-)

I guess I should claim this issue... Any other reviews?

Rerolled against HEAD.

+1. This is good thing. Permission names currently dance between 'too short to understand' and 'too long and descriptive to fit in the window.'

Um, thanks Eaton, but did you install and review it? I don't think Dries et al are even going to look at it until it gets set "ready to be committed", but I don't feel comfortable setting my own submission. :-)

Can someone actually look at the code and set it ready to commit (if it is) instead of just saying they like the idea? I like the idea, too, but that doesn't help much. :-)

Crell, when so much people say they lie the patch you should update it once again. I'll promiss you to test the patch and set it ready to commit when everything seems to be fine..
Then it's up to Dries to decide what he wants, apply or reject..

Untill that I think it'll bring us more good than bad..

Steef

OK, update it to do what? :-) As far as I know it applies cleanly with latest head, still functions, and the only thing missing is text for all core modules which as I said is something I figure the Docs team would be better equipped to do than I am. If something is not fine, please tell me so that I can fix it. Otherwise I'm just waiting for someone to actually read the code.

Once again rerolling against HEAD. I'm not sure what's left to do with it. Someone please give me a hint or give me a ready-to-commit. :-)

Setting postponed, just to clean up the queue a bit. This isn't going into 4.7. I'll try again in 4.8.

Thanks, timcn, I'd nearly forgotten about this one! :-)

Applies cleanly and seems to work. Spiffy.

Some comments:

- The description for "change own username" is "The permission to change the own username." That just doesn't flow right in English. I'd suggest "Allows users to change their own username."

- There's still a <div> in the straight source in user_admin_perm(). It was there in my version, too, because I couldn't figure out a good way to get around it. I still think it shouldn't be there, but I'm not sure of a non-inefficient way around it. A theme function feels like overkill, but may be necessary. Thoughts from other experts?

The descriptions are a good usability improvement.

Should we be saving them to a table (like we do with, e.g., module descriptions) rather than loading them each time?

Is there a relation here to http://drupal.org/node/73874, Normalize permissions table? Should we have a table with an id, name, and description for each permission and then assign permissions on the id (a table with rid (role id) and pid (permission id)?

Nice! Although I suggest we kidnap someone from the docs team to review the descriptions.

There's still the issue of that div... :-(

@ nedjo: That issue would probably break this one again, but it's a separate matter. That's about changing the storage mechanism for permissions. (I'm personally not in favor of moving them all to the database, but I would support non-comma-string storage.) This issue is a usability improvement for the roles page. It shouldn't have any application outside of that page and that page isn't called often, so I see no reason to db-cache the description strings.

Well, if TPTB are OK with the div, I'm OK with it as well for now, I suppose.

Can we get some sort of feedback on the text of the descriptions? I think that's the only thing left here.

This is a good idea, but makes the permissions page unmanably large. I have a couple ideas:
- 'Help for each permission' link which replaces checkboxes with help.
- Put help in a separate row from the checkboxes so they are stacked.
or
- A combination of the two where they are stacked after clicking a link.

What about a tooltip?

Subscribing. Huge +1 to this functionality. Patch no longer even comes close to applying, but hey. :)

Ok, here's a re-roll. Still needs testing and an upgrade path (I renamed blog and forum permissions to be consistent with the other node types not only because it feels good, but also to save a bunch of typing ;)). I'd also like to try and find a better way to add the div in there, and include ?q=admin's "hide descriptions" link/feature. I'm also considering removing the safety check that allows the descriptions to be optional, as I believe they ought to be required.

Attached is the patch and a screen shot of how it looks.

I agree that descriptions should be required. It is seriously annoying to have some items listed with a description and then others without. I see this all the time in systems other than Drupal or a CMS. The built-in (i.e., core) items have their descriptions, then the third-party (i.e., contrib) entries are blank, leaving you wondering what they are for. Usually those are the items that are most in need of being documented.

Ok.

1. Upgrade path/Nicer handling of description div: Done. Permissions are renamed, and div was moved to theme_user_admin_perm() so it can be overridden.
2. Hide descriptions feature: I moved to a separate issue @ http://drupal.org/node/220263, since there are larger issues with the implementation of this feature that need to be addressed, independently of this patch.
3. Requiring descriptions: I tried ripping out the code that adds in the safety net, and without it, the permissions page gets really messed up. Bad. As in, it'll start displaying 'phantom' modules called "0" which cause floating checkboxes off in the middle of nowhere and such. :P So I don't think there's any way to avoid this safety net. Hopefully it'll police itself since it'll be super obvious that your module's description is missing compared to everyone else's. :\

Now all that's left is testing, since I sure haven't done any. ;)

Slightly more descriptive title.

Good call. Also, stealing this issue from you. Mwahahaha. ;)

+    'use PHP for block visibility' => t('Allow this user to enter PHP commands in the field for the block visibility settings. %warning',  

This user? Or users in this role?

+    'add content to books' => t('Add content of any content type to a book.'),

How about "Add posts of any content type to a book."

+    'post comments' => t('Write own comments on a post.'),
+    'post comments without approval' => t('Write own comments on a post without them being approved by a site administrator before publication.'),

Or just "Comment on a post (approval required).", "Comment on a post (no approval required)"? Or something.

+    'administer languages' => t('Configure languages for content and the user interface.'),
+    'translate interface' => t('Translate the built in interface and optionally other text.'),

Er. Huh?

+  $perms["edit any $type content"] = t('Edit any %type_name content, regardless of by whom it was created.', array('%type_name' => $info->name));

"...regardless of its author."?

+    'create url aliases' => t('Create, edit and delete the url alias for posts.'),
+    'administer url aliases' => t('Create, edit and delete any url alias.'),

Capitalize URL. Plus, "url alias" and "posts" do not agree.

+    'vote on polls' => t('Cast a vote on any poll.'),
+    'cancel own vote' => t('Remove own vote from poll results.'),
+    'inspect all votes' => t('See which user voted for which choice.'),
+    'translate content' => t('Create translations of site content.'),

This are good examples of where the descriptions are almost less clear than the permissions (the permissions here are very clear, unlike many on this list). Descriptions will almost be redundant on some of these, adding no value.

+    'view post access counter' => t('See how many people have visisted a post.'),

...visited...

+    'administer taxonomy' => t('Edit, create and delete vocabularies and terms.'),

I didn't really notice until I got here, but we need to introduce some consistency in wording, as a good example to contrib. Here, you have "Edit, create and delete". Reading back through, I also see:
- "Create, edit and delete..."
- "Delete, block or add..."
- "Add and remove..."
- For add, we have 'Post', 'Add', 'Write', 'Create', etc.
- Both 'see' and 'view' used for view (and 'Access', 'Use', and 'Perform', with slightly different meanings)
- 'Manage', 'configure', 'set', 'changes', and 'select' used for configure/manage

+     'change own username' => t('The permission to change the own username.')

This one is in a style altogether different from the others.

Now, all that said, I love this patch, and webchick, you rock for taking this on.

w00t! thanks for the review, Keith!

I missed your reply before I started my crusade to read over all of these thoroughly and attempt to fix some of them, since the patch before was for the most part a simple re-roll. I've incorporated your feedback where it still applied (THANK YOU for the much better wording on the node perms especially. Sheesh. :P), but if you could have another look through, I'd appreciate it.

+    'administer languages' => t('Configure languages for content and the user interface.'),
+    'translate interface' => t('Translate the built in interface and optionally other text.'),

Haha, uh. Yeah. Right. I cheated here and just stole the menu item descriptions, which I guess means we need to create sensible text in two places. ;)

Let's try this instead:

    'administer languages' => t('Manage the languages in which the website content and interface text may be displayed.'),
    'translate interface' => t('Manage the translations of the website interface text.'),

...sigh. ;P I'm definitely up for better ideas. ;)

+    'vote on polls' => t('Cast a vote on any poll.'),
+    'cancel own vote' => t('Remove own vote from poll results.'),
+    'translate content' => t('Create translations of site content.'),

Just made the descriptions match their permission names for now, unless we think of something better. Except:

    'inspect all votes' => t('View voting results.'),

I felt that was a bit more clear.

Good call on standardizing on permission descriptions. I decided to go with the following standards:
- Add: Make something that wasn't there before.
- View: Look at something.
- Delete: Get rid of something.
- Edit: Change something.
- Manage: Some or all of the above. No real need to call out "Add, View, Edit, Delete, Gaze Fondly, etc." as I don't think it adds much value to the descriptions and it's too hard to keep it consistent across 50+ names.
- Select: choose or pick something.
- Configure: Anything related to settings. For example, you manage comments, but you configure comment administration settings.
Speaking of which...
- Administration settings: Doo-hickeys that you can configure in the admin panel.

Might not be perfect, but hopefully it's a start. :)

And thanks for the props, but kkaefer did all the hard work; I'm just doing some polishing. :)

Marking back for review.

This is one big usability plus.

I like this. I think we should honor the setting on /admin to hide descriptions (i.e. reuse same variable). So the meaning of that variable is 'compact' mode' or somesuch. We could re-show that same link on perm page or not - i vote to re-show it. Either way, this is a nice enhancement.

This has been a personal itch for me for a long time.

Ok cool. This implements the hide descriptions link, per Moshe. This took a little bit of futzing, but not as bad as I thought. A nice side benefit of this futzing is that now the "hide descriptions" link isn't coupled to the ?q=admin screen, so we can re-use it wherever we want in the future. :)

So this patch is the same as the above, except it now has a "hide descriptions" link at the top which will revert to the "legacy" view.

Oops. As i was going through I spotted a stray newline in system.install. Silly vim.

Btw, for testing this patch we need the following:
- Do everything with blogs (create, delete, edit, etc.) as NON-uid 1 to make sure the permission renames still work.
- Do the same with forums.
- Setup a D5 (or even D6 site) with auth users having blog/forum permissions and ensure that you can do everything above after updating.
- Look over the permission descriptions and make sure they make sense. Let's not get overly bikeshed about this, as these can always be refined later, but if there's something that's clearly wrong or awkward, please suggest an alternative.

These are the steps I took to test this patch:

1. Installed a fresh Drupal HEAD
2. Applied patch
3. Added a user (user/3)
4. Enabled blog module
5. Tested all permissions with the blog module with user/3: Passed
6. Tested all permissions with the forum module with user/3: Passed (needed "administer site configuration" to access "administer forum" which is reasonable)
7. Enabled all modules
8. Visited admin/user/permissions and read through the descriptions and wrote up my small issues below...

Everything seems to be perfect. Some descriptions arn't that helpful though, my nitpicks are as follows:

cancel own vote: "Cancel own vote."

How about "Allow the user to cancel their own vote."?

translate interface: "Manage the translations of the website interface text."

"Translate interface" sounds like it allows the user to get a translated interface, not manage them. Am I wrong here?

vote on polls: "Vote on polls."

Hmmmmm.

Very nicely done, Angie. This is great! Other then the three nitpicks I listed above, everything works perfectly.

Re-rolled with Rob's suggestions

I think dmitrig01 meant to mark this back to needs review.

Looks like you changed:

vote on polls

Allow users to vote on polls.

translate interface

Translate the text of the website interface.

cancel own vote

Allows users to, after voting, cancel their vote.

This seems reasonable. If anyone has any other thoughts, please let them be known! The patch applies cleanly. Refer to my previous review for my thoughts on its functionality.

Oh, and the "Hide descriptions" link works very nicely, using the same setting as the "Hide descriptions" on the administration panel. This also means we can use the same Hide descriptions link on other pages.

One minor comment:

Allows users to, after voting, cancel their vote.

could be:
Allows users to to cancel their own votes.

Otherwise looks perfect, so leaving at RTBC since I'm not sure my change is any better.

Here we go. Based on consensus in #drupal, I think this is good to go.

The "Allow users to ...." is inconsistent with other permission descriptions, so we decided to go with:

    'vote on polls' => t('Cast votes on polls.'),
    'cancel own vote' => t('Change own votes.'),

Leaving RTBC, since that's the only change.

The patch in #51 used both "Can be performance heavy" and "Could have performance implications". The attached patch changes the instance of "Can be performance heavy." to "Could have performance implications." for consistency.

My only other very small nitpick with this patch is the wording "this permission has security implications". Technically, all permissions have security implications, 'tis only that some permissions have much more profound implications than others. I don't have a good suggestion for changing this, though, and I like the fact that it will probably make people stop and consider the..., well, implications, before granting those permissions.

I diffed the previous patch and this one to confirm that the modified "Can be performance heavy" was the only change, so leaving it at RTBC.

subscribing

Here are two screenshots - the full one, and a smaller, annotated one.

Note: This is not with the latest version of the patch installed

I like this patch, however I have a few nitpicks. #description this is somewhat deceiving as quite a number of form elements have this property and markup does not have it. Can we maybe find a better name to reflect that this is specific to this form? Also, !is_null should be !empty.

I'm not sure why we need !empty rather than !is_null, since the value's instantiated as NULL whenever it's not defined, but I've changed it per your request.

I realize #description isn't output in theme_markup() (though I'm not exactly sure why an exception is made here), but it seems to me that re-using the #description flag which means semantically the same thing as it does in any other form element is better approach than either embedding the div in the form markup itself (which removes its themability) or making up some bogus flag like #permission_description. Are you sure this is really necessary? If so, do you have suggestions on what I should call it instead?

However, to avoid this patch being caught in further debate, here's a re-roll with #description renamed to #permission_description. But I think we maybe ought to just fix theme_markup() in another patch so that it outputs a #description like other elements, so that placing the div in the theme function here is unnecessary. However, tweaky FAPI internals are /definitely/ out of scope for this patch. ;)

Tested and it's good

Yes this is good. Someone visits this screen and then adds a #description to her #type markup expecting something like on this screen and that won't be. That's why I asked for a separate key. Microoptimization of the day: empty is faster than is_null.

Ok, since #permission_description is nasty, I talked to chx about this some more, and it turns out what I actually want here is '#type' => 'item' instead. '#type' => 'markup' is quite literally "spit this out and nothing else."

The only hitch is that for some reason there's a no-wrap class on .form-item so I canceled that on the permission form only, so the table doesn't look ooglay.

Hm. Crap. That CSS hack is not working.

Oh there we go. The opposite of nowrap is not wrap, it's normal. :P

Nice job. The hidden gems of fapi like #type item. :)

Ok one last (really, last!) re-roll. kkaefer wasn't sold on the "cancel own vote" description, so it has now been changed to... *drumroll*

"Retract and optionally change own votes"

Ta-da!

Since this was a decision based on various people present in #drupal, it counts as reviewed, and therefore patch is still RTBC. ;)

Just want to add in a huge plus one here, I tested it out and it works great. One possible suggestions:

Block Module: For the Administer blocks permission, perhaps it should mention the fact that it allows you to create blocks, too.

Couple of comments:

* I spent 3 minutes searching for a setting that allows me to turn off the descriptions. Couldn't find it, gave up. Can we make the compact mode easier to spot? Am I blind?

* I like the descriptions on the permissions page but I found the original names to be mostly redundant. It's a bit cluttered now. I'm not sure we nailed the presentation bit. Giving it a bit more thought.

@Dries: There's a "Hide descriptions" link in the upper-left corner of the page, below the help text. It's not real obvious, but it is in the same place as the one on ?q=admin. Do you think if this was moved to the upper-right on both pages that it'd be easier to spot?

Some of the permissions are simple and do mostly one thing, and in those cases the descriptions are indeed a bit redundant and only there for consistency. But we also have an awful lot of permissions that aren't immediately obvious what they do (administer nodes, use PHP for block visibility, etc.). Bear in mind too that we already know what these permissions do, so the text is definitely redundant for us, but new users can use a bit of a hand figuring stuff out.

EDIT: I also thought of adding a title attribute to the permission names (or something similar) so the descriptions only show on mouseover, but the problem there is that a) it's inconsistent with the way we do descriptions everywhere else, and b) it's more work for a newbie to have to move their mouse around to figure out what everything does.

I like the descriptions, but I found myself not reading the _original_ permission names anymore. It's minor but I'm wondering if other people observed that as well.

Oddly enough, I don't get a 'Hide description' link in the upper-left corner of the permission page. Odd.

Yes, something is odd with this. With the patch applied, I have no "Hide descriptions" link on the /admin page or on the permissions page (I have one on the /admin page with the patch reverted). If, without the patch, I select "Hide descriptions", then apply the patch, permissions descriptions are hidden (the system is in compact mode) but I still don't get a "Show descriptions" link.

Ah, you probably need to clear your caches @ admin/settings/performance. I had to add a new theme function.

Try that, and if it still doesn't work, let me know and I'll futz some more.

Yes, that fixed it! Thanks webchick, I didn't think about that at all.

Back to RTBC.

D'oh. Works now. I've decided to commit this to CVS HEAD. Thanks all!

Gasp! Wow, THANK YOU! :D And thanks kkaefer for writing the docs! :)

OMG! This finally made it in! webchick, you rock!

Oh yay! Congrats webchick!

Hey, thanks. :) kkaefer deserves most of the credit though; he did most of the work, I just updated it for 6 and lobbied for it. :)

Bookmark.

This fix brought up the broken localization support for permission names even harder. I've submitted a patch to suggest a solution at http://drupal.org/node/313213 Please review!