Usability UMN: Allow roles to be weighted

Yes please.

Note also that we discussed changing "authenticated user" to "registered user" during the testing since no-one seemed to pay much attention to "authenticated". If that happens, then we've got less chance of the default ordering staying as it is when extra roles are added.

Well, the weird thing about roles in Drupal, is that roles are really split into two trees, though this concept isn't fully exposed to the user. There are really two super-classes: anonymous and authenticated, and then the other roles all fall as subclasses of the authenticated role. I would think it would make more sense to split the columns into something like this.

|  anonymous |                      authenticated                   |
|------------|------------------------------------------------------|
| default   | default | administrator | Extra Role 1 | Extra Role 2 |

Then the roles after default on the authenticated side would be sorted by their permissions. Maybe this is the wrong issue to post this suggestion in, but I figured I'd just through it out there, as I think the ordering of roles should really only be including the authenticated-type roles.

Yeah that's a good point. Anonymous should always go on the left whatever happens, and probably Authenticated too.

The importance of a role, and the place that role would most logically be put in a list of roles is nearly impossible to define simply from a count of permissions.

in your example, the goal is to get the roles to show in the order of:
anonymous, authenticated, moderator, admin

In every site I've worked on that order could not be defined by the number of permissions.

An admin user is one that can administer content and other things, the admin role has very few permissions. Admin Users are users that have admin and authenticated roles assigned to them

If you make use of the fact that a user in drupal is the sum of the permissions of all the roles they are in, you will see that there is no easy way to order roles on the page in a way that matches the logic of most sites (unless a weight is added to the role).

This came up in the latest UB usability testing too: http://www.drupalusability.org/node/24

I'll take a stab at this to complement #745418: Order the "pre-auth" role on admin/user/permissions. in the LoginToboggan queue.

Wow, as usual, the pages that have been working for a long time contain the cruftiest code. We were currently displaying two COMPLETELY different forms in the same form function, with a big IF statement switching between the normal listing of roles and the form to rename a single role. Messy messy.

So this patch ends up looking a lot bigger than what it actually changes, since I broke out these out into two separate forms, user_admin_roles() and user_admin_role(). Of course we need to add a column to the database for "weight", but since this only affects the order of roles, the user_roles() function is unaffected and we don't have any API change there.

I did make a small change to user_role_load(), which previously would only work with role IDs if the value passed in was a true integer. Since the menu system passes strings, I changed this to use is_numeric() instead of is_int(), this makes it so we can use user_role_load() as an auto-loader in hook_menu().

Oh another change that I'm sure someone will ask about: This patch makes it so that "anonymous" and "authenticated" do not necessary need to be the first two roles. As I stated, in #9, this is because I want this to work for the "pre-authenticated" role added by LoginToboggan. Logically, this means that "pre-authenticated" would be after "anonymous" but before "authenticated".

This would be really nice - I hope it isn't too late for D7. And the cleanup is great. Some feedback:

1. +      'weight' => array(
   +        'type' => 'int',
   +        'size' => 'tiny',
   +        'not null' => TRUE,
   +        'default' => 0,
   +        'description' => 'The order of this role in listings and the user interface.',
   +      ),
   

   Other weight fields in core are no longer using 'tiny', so this shouldn't either.

   (Also, for the description, should it be "The weight of this role..."?)

2. +  db_add_field('role', 'weight', array('type' => 'int', 'size' => 'tiny', 'not null' => TRUE, 'default' => 0));
   +  db_add_index('role', 'name_weight', array('name', 'weight'));
   

   Unless I'm missing something, is this adding an index in the update function that does not get added on installation?

   Also, same thing as above for 'tiny', of course...

3. /**
    * Form to re-order roles or add a new one.
    *
    * @ingroup forms
    * @see theme_user_admin_roles()
    */
   function user_admin_roles() {
   

   I believe all form callback functions are now supposed to take $form as the first parameter - see http://drupal.org/node/224333#hook_forms_signature

4. +      $row[] = l(t('edit'), 'admin/people/permissions/roles/edit/' . $rid);
   

   Is the change from "edit role" to "edit" deliberate? Probably not part of this issue...

5.   $order = 0;
     foreach ($roles as $rid => $name) {
       $form['roles'][$rid]['#role'] = (object) array(
         'rid' => $rid,
         'name' => $name,
         'weight' => $order,
       );
   

   Is there a reason to use this auto-incrementing $order as opposed to using the stored weight for each role that is returned from user_roles()?

6. Overall, I found the default weighting confusing with this patch - when I install using the standard profile, "administrator" winds up first, and when I add a new role, it (sometimes) winds up first as well:
   • I'd expect that anonymous and authenticated would appear first on a fresh installation, so maybe user_install() should take care of giving them low initial weights? (They could still be adjusted, of course - and something like LoginToboggan could still insert in between them on install - but by default it seems like they belong first.)
   • I'd also expect that adding a new role via the UI always put it at the end of the list, not the beginning, so perhaps we should have code to guarantee that?
7. When rearranging things so that anonymous and authenticated aren't first, this looks kind of strange to me when combined with the auto-check-and-disable-all-checkboxes feature that was committed in #78941: Usability: Auto-check permissions if "authenticated" has them:

   

   I'm kind of hoping that patch gets rolled back at some point anyway, though :) This is just another place where it causes weirdness...

8. +      $output .= '<p>' . t('You may reorder permissions by using the drag handles. It is generally recommended to sort your roles from most restricted (anonymous) to most privileged (administrator roles).') . '</p>';
   

   I think in general the standards now are not to specifically refer to the drag handles in help text... so maybe that text should be rewritten a bit?

9. -  $field = is_int($role) ? 'rid' : 'name';
   +  $field = is_numeric($role) ? 'rid' : 'name';
   

   This isn't correct, unfortunately - see #619584: Deleting user role throws PHP notices and prevents delete operation for further discussion, and where the main fix for that bug is taking place. It might be OK as an interim step, though, since it seems like it's needed for the rest of this patch.

Before I respond to most of these comments (thanks David for the great review), I should not that "weight" is not returned by user_roles() at all. user_roles() returns an array of $rid => $name pairs, just like it always has, the "weight" column is just used to order the roles, and is not actually accessible at all once the list of roles has been retrieved. Otherwise we'd end up with A) An API change to user_roles() and B) making user_roles generally less useful (IMO), when do you really need to know the weight of a role? So far it's been completely unnecessary, and the "order" is really more important than the "weight", which you can determine from user_roles() easily.

As there is no API change involved with this, I don't see why it shouldn't be possible for D7.

Ah, good point. I was only looking at the patch file and saw "SELECT *" in user_roles() so figured it all was being returned, but you are correct. In that case, after actually looking at user_roles(), I'll replace my point #5 above with the following two (small) things:

• We can probably just select 'rid' and 'name' in the user_roles() database queries, rather than SELECT *.
• The bottom of that function currently contains this:
  

    // Filter to remove unmatched system roles.
    return array_filter($roles);
  

  I'm pretty sure the array_filter() is made unnecessary by your patch, and can be removed.

Quality stuff. Just chiming in here to give this a big fat UX +1. It would even increase scannability of the sea of checkboxes a bit (less ticks on the left, more on the right, easier to spot the gaps)

Talked this over a bit with quicksketch in IRC. Naming the biggest role 'Zuperadmin' simply doesn't cut it :)

Here's a rerolled patch. I could go through and note the fixes for each item, but since David is right on all counts, let's just say that I've implemented all of his suggestions.

The #7 and #9 items are a bit out of scope for what can be fixed here. Re-iterating for people just jumping in now:

#7: The user permissions page gets confusing if "authenticated user" is moved to the middle: True, but I specifically want to be able to move authenticated user because of LoginToboggan's "pre-authenticated user" role. Which is what prompted this entire patch in the first place. If the user wants to move "authenticated user" they should be able to.

#9: The change to user_role_load() is not correct: This change is definitely not perfect, #619584: Deleting user role throws PHP notices and prevents delete operation is the correct approach, but much more comprehensive. We could put in either patch first though, since overlap is minimal and the approaches both set out to accomplish some of the same things.

Tried this patch out tonight, and it added all of the expected user interface features. I can adjust the permissions.

My issue with the patch is that I don't understand what the role order means. Aren't permissions merely additive? If so, it seems that the order the permissions are listed in is irrelevant.

As the (new) help text states, you should order your permissions from least permissive (anonymous) to most permissive (administrators). This makes it so when you visit the permissions page, users further to the right have more permissions. It's the original goal of this issue (see the main issue description up top).

#619584: Deleting user role throws PHP notices and prevents delete operation was committed so chances are this patch needs to get rerolled. Rock on quicksketch.

Here we are. This could use one more eye to make sure I didn't accidentally introduce any regressions after #619584: Deleting user role throws PHP notices and prevents delete operation.

Weird, I didn't update hook_menu() in that last patch. I must have reverted it somewhere along the way. One more time...

Tested this patch and it works. Reviewed this patch and it looks good. Given that it fixes an important UX problem, I decided to commit this to CVS HEAD. Thanks.

Follow-up:

1) When clicking "edit role", there's no way to get back without saving or deleting. Stuck. Thus, added a "Cancel" link there.

2) The help text on the Roles page is too elaborative, and duplicates a lot of information that is explained in detail on User module's help page. The module help page has been vastly improved, so the help text on the Roles page should neither duplicate all of that, nor be so lengthy.

Improved screen:

This all makes sense to me. The re-addition of the drupal_goto() in user_admin_role() was a regression on my part, since access is already denied to those pages after #619584: Deleting user role throws PHP notices and prevents delete operation.

I wanted to simplify the help text, but oddly, I didn't want to introduce additional changes that might not be necessary. ;-)

New help text is great, cancel link works and doesn't have any problems.

Minor qualm: we should use the new user_role_load(DRUPAL_ANONYMOUS_RID) instead of user_roles() in hook_help(). Either that or user_roles() should be static cached in some way. I was a little surprised that it's not cached at all.

Thanks for reviewing! Just realized that this entire user_roles() loading is ill, as we have a static string that is not supposed to change.

Works for me! Clever swapping in of t('anonymous user'), which may be changed via string overrides or other means.

+  $form['actions']['cancel'] = array(
+    '#type' => 'link',
+    '#title' => t('Cancel'),
+    '#href' => 'admin/people/permissions/roles',
+  );

Note sure I understand the rationale for this - isn't this inconsistent with what is done in similar situations elsewhere in Drupal? It's true that there's no back link on this page right now, but I believe that's just due to the missing breadcrumbs (which is a separate bug occurring all over Drupal 7). When the breadcrumbs issue gets fixed, I think there will be reasonable navigation from here back to the main roles page.

Also, I don't think any of this was caused by the patch here - the same situation exists even in Drupal 6 as far as I know.

+        '%anonymous-user' => t('anonymous user'),
+        '%edit-role' => t('edit role'),

I was about to say that this is bad for translatability to split out strings from the main sentence like that, but then I realized that @quicksketch's point about why that makes sense for 'anonymous user' also applies to the case of 'edit role' (since it's an exact match to the link text and therefore will be automatically matched to it). Should there be a code comment explaining why we do this here? Because the general rule is that this is bad, and we don't want to promote bad habits :)

***

Finally, I am a little late getting back to the main patch that was committed :) But looking at it now it looks great - however, I'd recommend one change. The patch used these as the default role weights during installation:

anonymous user: 0
authenticated user: 1
administrator: 2

Instead I'd recommend switching to -2, -1, 0 (with the latter not necessary to specify since it's the default). The reason is that would make it so any role added by any installation profile automatically gets (by default) an appropriate weight that puts it in the "correct" place in the list. With the way it is now, installation profiles that want to add roles would have to remember to specify '2' (unless they are in the extreme minority that wants to be before authenticated in the list), which is kind of odd.

Shame on me for +1'ing a patch I didn't apply and look at, but that 'Save order' label on the submit confused me. For a split second I was paying for something at some webshop. It's not even necessarily re-ordering that I did here.

'Save' should be sufficient. Can we sneak that in here?

The reason is that would make it so any role added by any installation profile automatically gets (by default) an appropriate weight that puts it in the "correct" place in the list.

I should note that you do not in fact need to specify a weight at all. I added this code to user_role_save():

+  if (!isset($role->weight)) {
+    // Set a role weight to make this new role last.
+    $query = db_select('role');
+    $query->addExpression('MAX(weight)');
+    $role->weight = $query->execute()->fetchField() + 1;
+  }

So I could have omitted the weight entirely. Setting anonymous and authenticated to -2 and -1 would not last for long, since the first time the roles page is saved, they will be set to 0 and 1 anyway.

Looks like that was a crosspost. "Needs work" for #29 and #30.

I'm guessing 'Save order' was chosen so as to more strongly distinguish it from the 'Add role' button that is right near it...? (Since if you add a role and click 'Save', the role will not actually be added.) But on the other hand, the ecommerce double-meaning of 'Save order' is indeed a bit unfortunate :)

I should note that you do not in fact need to specify a weight at all. I added this code to user_role_save():

Ah - you are right! I saw that in the patch but assumed it was in the form submit handler. That makes sense.

So maybe as you suggested we shouldn't even bother setting the admin role weight to 2 at all, then. Seems a little cleaner.

Another followup at #755870: PHP notice for undefined $roles variable in user_roles() - quick and simple enough for its own issue.

This would be a great and very useful feature to work into core.

right now permission page looks like this:

4. anonymous user 3. authenticated user 1. administrator 2. member

I want it to look like this:

4. anonymous user 3. authenticated user 2. member 1. administrator

While its not the end of the world, it is annoying, and is illogical.

Anyone succesfull With any modules/patch for 6.x?

While I view Drupal as the swiss army knife of cms systems, Its funny how a lot of simple things are not thought of...

This would be a great and very useful feature to work into core.

This feature is already in core, right now this is just minor back and forth. This cleanup probably should be moved to new issue, we're done with the "feature request" at this point and addressing minor bugs.

Personally, I especially stumbled over the current help text a couple of times already. The latest patch in here cleans that up and shortens it to something that can be quickly parsed and understood by humans.

So while I agree that the follow-up(s) could have been done in separate follow-up issues, I also agree with quicksketch that we have to do the remaining minor tweaks.

Yes, I see this issue is for D7, nice change.
I did a messy work around in D6 by adding number prefixes to the roles to force the order.

anonymous user
authenticated user
2 advertiser
3 blogger
4 forum moderator
5 story and book editor
6 chief editor

Hey webchick and dbeall, I've built a custom module to back-port this feature to Drupal 6. Looking for any kind of community interest in a forum post here: http://drupal.org/node/1024736

and in my CVS application here: http://drupal.org/node/1020838