"Edit" links on roles admin page are confusing to users: "Edit permissions" should be the primary link

Looks good. Tagging.

Removed help section as it is very obvious what the button does now To be honest though id be more in favor of adding a column on this table and providing both "Rename" and "Delete" as links instead on this page. Thoughts?

Ok so this is a very rough implementation of it. i need to standardize te rename and edit forms and maybe combine them like they were before info one form handler. BUt ui just wanted to show what i meant.

This patch:
* removes the help text because it is obvious now
* breaks out "edit role" into 2 columns: "rename role" and "delete role"
* adds a confirmation page when you click delete role to prevent catastrophic data loss and return you to the roles page.

I dont like the way im doing this right now but i wanted to post ity before i took off for the day. Ill probbaly recombine the rename and delete forms back into the "user_admin_role() form and cleanup the paths if everyone likes the idea in the first place.

Attached is pacth and picture.

Ok. Rerolled and moved everything back into the user_admin_role() and added a switch in each of the form, validate and submit callbacks to test for which form was submitting. Its quite a bit cleaner. I'm thinking this is done unless someone has suggestions or review notes. -mf

All patches since #3 have had superfluous and trailing whitespace. The one from #5 additionally adds a bunch of tabs to the mix, as well as uneven indentations. Also, the change to switch-case loops makes #5 a monster to review, compared to earlier patches. I'd consider dropping the switches for now, and if you really think they're needed, make a new issue for them (esp. considering that some of the switches made are completely unrelated to this patch, e.g. the user_admin_role_validate() one).

Anyway, here's a re-roll with a slightly altered logic (ie., containing more of the pre-#5-patch logic) and whitespace fixes.

Looks good in general...

As for the code, I think #4 is actually more along the lines of the right approach. These are totally different forms, so I'm not sure I understand the rationale for putting their submit/validate callbacks all together in one function?

As for the functionality, I think separating out the delete link is definitely good -- it's consistent with the way other places in Drupal do it. I'm not 100% sure about calling the other link "rename role" though. (Contrib modules might easily tack other functionality onto the role edit form, and there are other patches in the queue for D7 core that might do so as well -- e.g., #182023: Add a third default role to core for handling Administrative duties and #256287: Give roles a description value.)

Here is an idea, though. The usability testers clicked on "edit role" expecting to be able to edit the permissions there. So why not give them what they want?? In other words, get rid of the "edit permissions" link altogether -- just have two links, 'edit' and 'delete', and that's it. Clicking on 'edit' would take you to a page where you could rename the role and edit its permissions, on the same page. Would that work?

@ Freso: I don't know how the tabs got in there. I use "replace tabs with spaces in eclipse but maybe the PDT auto formatter added them in (i wish it worked better). I'll make sure they aren't there for the next round.

I have a rule that once i get to 2 elseif's i break it out into a switch statement. It just makes life easier. Thats why i changed it.

@David: I agree about breaking these forms apart. It makes it easier to follow the trail for new developers. (i was confused for a bit when i started on these forms so why not). i just put them back together because thats how they were organized originally.

I need to rethink on how to integrate the role descriptions and leave it open for additions by contrib modules. I *really* like the simplicity of calling these things what they are until they need to be overridden and changed by the contrib module that wants to add functionality (for usability sake) but i understand the concerns for sure.

So if there is agreement for breaking out the forms (add, rename, delete) ill do that as a first step and maybe try to roll in webchicks role description so we arent stepping on eachothers toes moving functions around.

@ David_Rothstein:
We don't want users to edit the permissions of the role under "edit role", as that would mean there were two places to change the same settings. It has been suggested (and shot down) before. Try to search around for it on the dev-list or here on drupal.org.

Also, I believe it makes sense to rename the links to "rename" and "delete" roles respectively. If another patch/module comes around, adding more stuff to the "Rename role" form, it should (IMHO) be the responsibility of that patch/module to change the link and other text as well.

Oh. And I also agree on splitting out the forms. My primary goal with #6 was to clean up the whitespace, and I just came across some logic flaws in the patch, due to the removal/restructuring of code which I fixed as well... I didn't think about doing a bigger rewrite of it. But good idea. Michael, are you on it? :)

@ michaelfavia:
Oh, don't get me wrong. I love switch()es. :) The usage of them here just means there's 10 times as much code to review.

We don't want users to edit the permissions of the role under "edit role", as that would mean there were two places to change the same settings. It has been suggested (and shot down) before. Try to search around for it on the dev-list or here on drupal.org.

@Freso: There already are two places to change the same settings. When you click on the "edit permissions" link that is on the role page now, it takes you to a page that allows you to edit the permissions for that role only. That's not the same as the main permissions page at admin/user/permissions.

I searched around a bit and really couldn't find the previous discussion... any pointers?

Anyway, this could be proposed as a followup since it's a bigger change. Splitting up the links as in the current patch is certainly better than the way things are now, so agreed, let's start with that.

Also just discovered #247084: Usability: Make "edit permissions" more prominent on roles page which is related to this.

Let's omit some needless words here and make "rename role" and "delete role" simply "rename" and "delete".

I normally agree with wasteful repetition. Disagree here. Because we have "permissions" in the same table we need to be clear. Especially since it was a point of confusion previously.

rerolled to move the forms into their own functions and used arguments to pass the rids, etc. probably needs a little cleanup but anyone have feedback for improving?

@ yoroy:
Apart from what Michael said in #13, this would also be bad with regards to being internationalised. Some languages may have a different form of "rename" and "delete" depending on what is being renamed or deleten (e.g. on the gender of "role"), which in turn might lead to the string "delete" needing 2, 3, 4, n different translations depending on context, but only one translation being possible as the generic string doesn't carry any information on what is being deleted.

The new functions do not have any Doxygen. And I need to test what happens if I should go to ".../rename/foo" or just ".../rename/". With the old logic, this would cause the "add" form to be called instead (ie., if ($rid) { ... } else { /* add role form */ }). However, I am currently unable to install Drupal 7 on my testing server, as my PostgreSQL version is too old... so I'm currently trying to update this.

Also, this will need some tests to go in. Just so you know. :)

There's an issue for adding context to t() - we should deal with context for translators there - not add extra words to the English interface strings if they aren't needed. For text, I'd go for the screenshot in #4 - but with "permissions", "rename", "delete" (i.e. no 'edit', or 'role' anywhere).

+1 good stuff ksenzee

I reviewed and tested this patch. It basically works great and is a major code cleanup too. I think it's pretty close to being ready. However, I noticed the following things not yet mentioned above:

1. Breadcrumbs do not appear correctly on the rename role page. I can't remember off the top of my head which magical menu constant you need to do what you want here (hopefully there is one), but MENU_CALLBACK is the wrong one.
2. Following up on @Freso's comment, going to "admin/user/roles/rename" works fine, but "admin/user/roles/rename/foo" does not - it gives you a rename form anyway along with a PHP notice. To solve this problem, we should change the menu paths to use '%role' rather than '%' and then create a role_load() function to validate that the role actually exists.

3. -  $items['admin/user/roles/edit'] = array(
   +  $items['admin/user/roles/rename/%'] = array(
   

   Should we really change from "edit" to "rename" in the URL? Doing it in the user interface text is fine because sites have ways to override that (if they are adding more functionality to the form), but the URL is not something that they can really override. So it seems like this will just lead to the possibility of broken links during the D6->D7 upgrade as well as less flexibility for contrib modules. I'd vote for keeping the URL more general.

4. In the function user_admin_role(), the PHP Docblock now lists the wrong validate and submit handlers.

5. +    t('Are you sure you want to delete the role: %title?', array('%title' => $role->name)),
   

   I would change this to: "Are you sure you want to delete the %title role?"

6. +      $rows[] = array($name, $edit_permissions, l(t('rename role'), 'admin/user/roles/rename/' . $rid), l(t('delete role'), 'admin/user/roles/delete/'.$rid));
   

   Very minor, there should be a space before [edit: and after] the last dot.

Also:

• Regarding tests, it seems that there aren't any existing ones for the role administration functionality, but I don't think it's fair to ask this patch to add them - this is basically just a user interface change. It looks like those types of tests are being handled over at #300993: User roles and permissions API. Also, trying to write these tests before a good API is in place means that the tests are necessarily going to suck :)
• Finally, regarding @catch's suggestion in #17, changing "edit permissions" to "permissions" would mean that the "Operations" column would need to be renamed (since they are no longer all verbs), so not sure if that is a good idea or not.

1. This should be fixed in the attached patch.
2. I didn't have the time to deal with that right now... so still open.
3. As #2.
4. As #2 and #3.
5. Changed.
6. Fixed. And although minor, still important. Well caught. :)
7. Even though this might not call for functionality tests, there should still be unit tests to all functions. And I'm not sure if the policy is still as strict (it should be, IMHO), but it used to be that no new function would be allowed without a unit test. And this patch introduces several new functions. (Esp. with #2.)

And I'm off to have dinner. =)

Great review guys thank you.

@freso: Ill add doxygen as soon as people actually like the rest of the patch. no need to rewrite it 5 times :).

@david thx for great feedback. Im going to make a number of your suggested changes and also try to incorporate webchicks desire for a role description #256287: Give roles a description value which just wouldnt work well with the current UI.

I think im going to modify this slightly to adopt the look of the Content Types page (admin/build/types). With links for edit delete and permissions after the title and the description. This should add a little uniformity and make it a little easier to add more role based functionality without sacrificing usability out of the box. If anyone has an alternative suggestion im all ears.

Please supply screenshots of changes.

@michaelfavia Please supply screenshots, of your proposed approach.

Im not exactly sure why it is so en vogue to run around asking for screen shots on every issue but its getting a little tiresome imo. I attach screen shots whenever applicable and you're always welcome to apply the patch yourself and test/review it and attach them yourself. Short of that it just amounts to comment spam.

That aside.

I altered this patch to:

* include webchicks description fields for roles (since they are so related)
* move "add role" to a local menu task like "content types" does for uniformity
* Added delete link and reordered the operations also allowed description editing on all content types (even protected ones so they can be customized as a site might need to).
* removes description of the default roles since the roles have that now themselves

Because i broke out the add and edit forms from the main page i rolled them together and just used '#default_value' and the presence of $rid (again like the node save operation) to determine if we are adding or editing a role. Also simplified some of the validate logic to reflect this. I still need to add doxygen and ill probably cleanup the existing menu calls but i tink this is now the correct approach. Thoughts?

One remaining bug is that when the "required roles" are edited the disabling of the name field for some reason results in a "required field" error. I havent had a moment to chase it down but i will.

We ask for screenshots because we don't have our own copy of head running and don't know how to apply patches. Each their own expertise. Ours is to analyze how things present themselves in the user interface and if proposed changes enhance the user experience.

Screenshot communicates ui changes things much quicker and clearer then describing them in words. It also widens the audience for review. So, thanks for adding the screenshots. We will keep asking for them though! :-)

- the local task for adding a role adds another click to the process of creating a new role. Consistency is not a reason to make things harder to do. Our local tasks/tabs are notorious for not being seen by users. This is not an improvement.
- Not sure if it's helpful to include webchick's request for a role description in this patch as there's enough to discuss already. I'll let others chime in on that too though.

Per webchick's own request, I think we should keep #256287: Give roles a description value out of this issue. Let's keep this one issue. I'm fine with not renaming "edit" to "rename" though, since there is already work in progress on adding more stuff to that page/link/menu entry/form.

Also, either "Operations" is renamed, or "permissions" should stay "edit permissions". And if we're keeping it as that, it should probably be "edit role" instead of just "edit", to make it easier to distinguish between the two "edit"'s.

Finally, this issue was actually originally about moving "edit permissions" before the "edit [role]". The latest patch moves this back again.

Retitled as requested by catch in IRC. Reworks the UI while adding descriptions for the roles as well. This is not a "kitten" issue.

After looking at some screenshots and talking w/ michaelfavia in IRC I like the direction this patch is going. Initially I was against adding a description field, as my 'grand vision' for the role editing pages was to merge it with the role-specific permissions page. That being said, with fields having landed I'd hate to handcuff what people are going to be able to do with this stuff, and having both of these pages merged could make for a cluttered interface quite quickly.

I do think a merging the role name and description into the same column similar to how we handle it on the permissions page would be a good improvement though.

I like the new direction too, although as others have said, it might not be great to do it all at once. The previous patch seemed very close to being RTBC, and the new code could easily be a followup patch after that one.

@yoroy in #24: Screenshots are definitely useful, no argument about that :) But I think the issue being raised was that screenshots should not be viewed as a "service" that people writing patches are expected to provide along with their patch. A screenshot is something that anyone who is interested in the patch can provide in order to help out everyone else who is interested. Also, while doing a patch review, at some point I think it is very useful to have a live installation, rather than just a screenshot. Sometimes issues come up that are only apparent while navigating around the site. Also, screenshots tend to merge together "theme issues" with "page structure issues" and those are not the same thing. For example, it's certainly true from the above screenshot that the "add role" local task/tab is practically invisible, as you said. But how much of that is due to a problem with the Garland theme itself, vs. an actual problem with the navigation model introduced by this patch?

It is a shame that this barrier of applying patches exists, but I don't know if there's an easy way to get rid of it.

yeah understood. Still, if a patch adds something to the ui, it needs a screenshot. Just as writing test is a requirement for code, screenshots are required for ui additions, it just gets the point so much better across, especially in the early stages of a patch, so ideally by the one who writes it because then we still have room to provide feedback and direction.

And for the visibility of the tab, indeed, Garland is the main issue. But introducing an extra click is theme independant and that is my main objection to this change.

Thanks for responding, it's good to learn about our mutual perspectives :)

Both catch and webchick are amenable to rolling these into one issue from IRC because they are related.

While i generally agree about the dissapointingly hidden nature of local menu tasks in Garland i dont think that it is a good excuse not to handle the process this way. That is a problem with the theme in specific not the implementation. Adding a role isnt a frequent operation even when auser finds themselves on this page and i dont think the clutter of a full webform on this page helps the cause. Especially as people start adding other fields to roles via Fields API etc in D7. This is how its done for content types and users know and understand that process. Lets unify the UIs for similar task types.

Again, the Garland issue is not my main point. You are hiding an important task behind a button where we currently have a direct way to add a new role in the context. Also, the consistency argument is not that strong at the moment. Context trumps consistency everytime. And if you look at core's poll module, you'll find something similar as what we have here: a text field to add a new item, placed below the table of existing items. It's nice to unify, but let's try and do that using the ui pattern that gets the job done easier/faster.

My point was that adding the description field and any additional fields that other contributed modules might want to append will make the current implementation ungainly. My patch reuses the same form for role addition and editing and makes the validation and submission logic on the back end MUCH easier to maintain. You would rather see the role name and description and other added fields rolled into the bottom of the "list of roles" page? You're only saving a single click and youre adding a bunch of confusion for new users in my opinion. Especially if contrib modules add functionality and fields.

Yes, I realize that and that's why it's a pity that these patches were rolled into one. We're quite far removed from the issue that's pointed out in the first post now. Users got confused by the order things are in.

Instead, we're now discussing how to add ui for adding a description. I'd argue that fixing an issue that was found during usability testing is more important than a new feature request that's "nice to have". The "nice to have" one is now making a more important task (add new role) harder to accomplish.

Anyway, let's get some more eyes on this.

I agree with yoroy - description isn't required, so we could easily bring the old textarea back on that page.

A major issue in testing was that when we present tables of stuff, without a link underneath to add a new role, users read all the way down before getting anywhere near a tab. I don't think this is a problem exclusive to Garland, I think it's because people read down, then want to do something, and then they're left hanging at the bottom of the page. We can have an 'add role' tab as well, but please bring back the textfield in the table.

Yes, I realize that and that's why it's a pity that these patches were rolled into one. Yeah. And even if Michael apparently didn't follow the conversation with webchick, I can say that she wasn't convinced with the issue merging (I'd be happy to dig in my logs). I still say we split them up again and focus on the UI at hand, even if we stay clear of renaming to "rename" so both issues can get in more easily (ie., less code to review => better and faster review and response => quicker RTBC => quicker commit). But then, that's just IMHO. :)

Ok all done from my end. Fixed as requested in #drupal-usability by catch and others. There are a few more improvements id like to make for actually using the role descriptions but ill open issues for those.This is my final patch unless additional changes are required.

Patch notes:
* Moved descriptions under role names to mimic "content type" table display
* The user_admin_role form can be cleaned up a little more if #227966: Use default values to #disabled form fields is fixed but it is only 3 lines.
* added doxygen code comments
* only remaining bug is lack of breadcrumb on edit and delete role but content type doesnt have them either so I'm not considering this a problem

@Freso: Please check your logs next time before you remember something for me. I'm hard working and opinionated, not deceitful.

Mar 09 10:34:29 michaelfavia catch, webchick freso was suggesting that i break this patch apart for the different issues that are VERY interrelated. i dont mind doing so if required but the patch seemed simple enough to me and the solution doesnt break up into good independent chunks
Mar 09 10:36:16 catch michaelfavia: I don't think this is a kittens issue at all - doing it at once seems fine.
Mar 09 10:36:36 catch michaelfavia: just retitle the issue to "make roles admin look like content types".
Mar 09 10:36:55 webchick michaelfavia, I would say keep them separate.
Mar 09 10:37:05 webchick Well. Hrm.
Mar 09 10:37:10 catch hehe.
Mar 09 10:37:13 webchick I see how they kinda cancel each other out.
Mar 09 10:37:25 webchick But is there support for adding descriptions to roles?
Mar 09 10:37:32 webchick That issue has been there for a *very* long time.
...
Mar 09 11:20:41 webchick Ok off the phone now.
Mar 09 11:20:49 webchick What were we talking about again? :D
Mar 09 11:21:02 webchick Oh right. Roles and descriptions and interfaces (oh my!)
Mar 09 11:21:13 webchick michaelfavia, do whatveer catch says. :D

I think moving it to the left is fine - re-roll with that change, a couple of small comment fixes and a bit of dbtng conversion. There's a couple of insert/update queries which also probably ought to be converted - although as far as I can see they're just moved around rather than added. Otherwise looks great to me

Patch above when applied creates this screenshot. I assume this was unintended?

I can put hte permissions next to the role name in parenthesis (kind of like content types does for machine name) if we are trying to decrease their exposure here. Im also personally happy to remove them because they are not terribly useful in my opinion now that the "role name" headers jog down with the scrolling page on the permissions page. If we want to keep single role editing functionality we could make the links on the top of the general permissions page goto it.

Screenshot of what this would look like sorry for comment spam.

Michael, yes, not intended, that'll teach me.

Here's what I meant, with attached screenshot to prove I actually looked at the page before posting the patch this time.

To clarify - everyone who got here wanted to change permissions - but they clicked edit role instead - thinking this would get them to the permissions page (hence this issue being posted originally). I also think it makes sense to keep 'edit' and 'delete' on the right - since that keeps them in the same position as they are on the content types screen. Adding permissions inline makes that one column look a little crowded IMO.

Still needs dbtng update on those queries (if that's the rule at the moment, I'm never sure).

Agreed i was just trying to understand what was being suggested. So lacking any further advice and seeing as how we have doxygen, upgrade path and UI reviews and that DBTNG will be handled in that issue, id like to ask for someone to perform a code review if they have a moment. Thx.

Rerolled and lots of edit no longer needed since dbtng update removed.

For ease of understanding this patch:
* Adds a role description field to the roles and associated add/edit form
* Reorganizes the roles list to resemble content type admin list
* Adds a delete form and confirmation page similar to taxonomy
* Lets users edit the descriptions of the protected roles but not change their name at this time (for no other reason than historical precedence, this is possible if desired)

Still unhappy that this removes the textfield to directly add a new role on this overview page.

Lets work on this in Drupal 8, doesn't seem viable in this stage of the lifecycle neither very important in terms of proning users with usability issues

This issue has morphed many times, but for Drupal 7, let's not lose track of the very simple patch that began it.

The attached is a simple reroll of the patch from #3 by @ksenzee and @michaelfavia. I didn't actually do any work here, so I'm almost tempted to just mark it RTBC myself :)

If this goes in, we can then move it back to Drupal 8 for the rest.

Can anyone point me out to the rationale why this is good? We seem to change more, then the initial confusion by users.

I'm not a fan of the 'rename or delete' link -- it is inconsistent with the rest of Drupal, and looks a bit weird. We should split it up in two separate links so let's proceed with #43 instead. Thanks!

current content types screen:

current roles screen:

So right now, unpatched, roles admin presents its operations similar to content types screen. And looking around in core, most table listings with operations show the 'edit' link first.

So like in #53, what exactly is being fixed here and why is this better?

Maybe it would be more clear if "locked" will be removed from column Operations (since it is no operation but a state).

Lets push this somehow.

In addition to those changes, wouldn't it make sense to put the 'Add role' button next to the textfield? Right now, the margin is somewhat unnatural.

Default behavior was to put the button in the "Operations" column.
Here is a patch with the button next to the textfield.

#59 looks nicer to me. Thoughts?

Agreed, #59 is much more easily understood. Also, subscribe.

I agree with #59

D7 is so much better then D5 wich i have been using on my website until now...

Tested patch, still works...
Please add this to head before we have to rewrite the patch.

Committed to CVS HEAD. Thanks.

+++ modules/user/user.admin.inc	27 Dec 2009 22:48:47 -0000
@@ -860,14 +860,14 @@ function theme_user_admin_new_role($vari
+      $rows[] = array(t('!name %locked', array('!name' => $name, '%locked' => t('(locked)'))), '', $edit_permissions);

This should be t('!name <em>(locked)</em>')

t() replacement should only be used for variables and it's perfectly valid to use this kind of HTML inside t(). Doing it incorrectly makes it harder for translators.

Powered by Dreditor.

Damnit...preview showed I was going to be adding tags, then it removed them. #$@T%@

Here's a patch for Dave's requested fix.

Makes sense.

Only, how about we fix the XSS in this line while we're here. user_roles() does not sanitize the role name. :P

Also make sense :)

Although, then we will be filtering the hardcoded role names, but not the user-provided ones two lines below that... So it might be better to not worry about that here, and just leave the XSS fixes to #316136: New role name not filtered into admin/user/permissions (which is the main issue dealing with that).

But basically either #68 or #70 looks RTBC.

I agree, there are a bunch of problems here. Let's solve the minor problem with #68 and worry about the filtering in #316136: New role name not filtered into admin/user/permissions.

#70: drupal_role_locked_fix.patch queued for re-testing.

Since the patch needed in #68 affects translation anyway, can we change the word? Saying locked really implies that the hard-coded roles can be unlocked (and that the user-created (or install profile created) roles could be locked– which, of course, they cannot be).

How about:

built-in

Sounds even worse, unless we are talking about a car. Not sure if we want to change this so late in the game though

Not sure built-in is the right word at all, but "locked" and "default" for these unchangeable roles are the wrong words. But with no ideas much less consensus for another word, the patch from 68 should be committed.

#68: drupal_role_locked_fix.patch queued for re-testing.

bump to 8

The fix from #68 is already in core.

Setting this back to "active", though, because I'm pretty sure that was just a followup and there was something else bigger that this issue was supposed to still be open for. I don't remember exactly what, though :)

Setting this back to "active", though, because I'm pretty sure that was just a followup and there was something else bigger that this issue was supposed to still be open for. I don't remember exactly what, though :)

Well, duh, it's for the original issue described in the issue summary above. That was never fixed.

It's actually even worse in Drupal 8 than Drupal 7, since the secondary links are hidden and hard to find:

So we need to make "Edit permissions" the primary link.

And the "Edit" link should also be renamed back to "Edit role" (like it is in Drupal 7). As discussed earlier in the issue, "Edit" makes no sense here when there are two things that can be edited.

I think anything else discussed above is somewhat out of scope and either already happened elsewhere or could happen in another issue:

1. I will reopen #256287: Give roles a description value for adding a role description.
2. I think this (from #7) is worth a new issue if one doesn't exist already:
   

   Here is an idea, though. The usability testers clicked on "edit role" expecting to be able to edit the permissions there. So why not give them what they want?? In other words, get rid of the "edit permissions" link altogether -- just have two links, 'edit' and 'delete', and that's it. Clicking on 'edit' would take you to a page where you could rename the role and edit its permissions, on the same page. Would that work?

   That would be consistent, for example, with how editing menus / menu links works in Drupal 8.

I'm editing the issue summary to change it from Full HTML to Filtered HTML. This will allow people to edit it (and I don't think there was a good reason for it to be Full HTML anymore). It could probably use an update.