Usability: 'authenticated users' role is concealed

ximo - February 14, 2007 - 15:39
Project:Drupal
Version:7.x-dev
Component:usability
Category:bug report
Priority:normal
Assigned:Unassigned
Status:needs work
Description

This is not a big issue, but I thought I'd mention it.

Although the 'Add user' form states "The user receives the combined permissions of the authenticated user role, and all roles selected here.", I believe one could avoid that sentence and tell the user in a more logical way. My suggestion is: Add a disabled, checked checkbox with the 'authenticated users' role name and remove the text below. Cleaner and easier to understand.

#1

ximo - February 14, 2007 - 17:04

I see now that one can't set an individual checkbox in a group of checkboxes as disabled with the current Form API, a patch is needed for this to be possible: http://drupal.org/node/104715

I support that patch however, and think it should be part of the Forms API. Would be useful in many situations for module developers, and in this case in user.module..

#2

ximo - February 14, 2007 - 21:47
Status:active» needs review

Combining the attached patch with the patch found here: http://drupal.org/node/104715, I made it work. The attached patch doesn't remove the 'authenticated users' checkbox from the Roles list (as the original code does), instead it disables it (using the patch linked to earlier) and adds its default value of TRUE. I also changed the description from saying "and all roles" to "and other roles", I found it better to keep the description. Easy!

Not sure this is something people will find useful, but it was a good practice for me, being my first patch and first hack into Drupal core. Needed to start somewhere :)

Along the way I also came up with another idea; it would be nice with a setting that hides all descriptions from all forms, as with the help texts. Hm...

AttachmentSize
user.module.patch_12.txt 1.35 KB

#3

gaele - April 20, 2007 - 09:14

+1. It follows the KISS rule.

#4

Dries - April 20, 2007 - 13:37

A screenshot maybe? It helps to show this to the others ... :)

#5

ximo - April 20, 2007 - 16:14

I realize now that this really is nitpicking, but maybe somebody else will find it worth doing? Generally, I think explaining something in a visual way is much better than explaining it in text. An example of this is this very issue, I should've included the screenshot right away.

Nuff talk, see the before/after shots in the attached file.

(Don't mind the muted sound OSD that got into the screenshot, I closed Photoshop before noticing it, and didn't feel like opening it again just to remove it.)

AttachmentSize
authenticated users disabled checkbox.png 47.41 KB

#6

Gurpartap Singh - May 6, 2007 - 20:21
Status:needs review» needs work

Your patch disables them all. Rest seems fine.

#7

Pancho - January 9, 2008 - 16:13
Title:Better way to say that new users will be assigned the 'authenticated users' role automatically» Usability: 'authenticated users' role is concealed
Category:feature request» bug report
Priority:minor» normal
Status:needs work» needs review

I like this very much, and this is a must-have, not just a nice little feature. In fact, I think this is a usability bug, as the 'authenticated users' has been (automatically) assigned to the user, but is concealed from the admin.

Still, Gurpartap is right: Your patch in #2 disables all checkboxes.
This is because you assume the FormAPI extension in #104715 to be committed. Unfortunately this has been definitely postponed to D7.

After some fiddling I came up with a solution for D6 that doesn't rely on the FormAPI extension. It doesn't exactly result in beautiful code, but it's the only way to do it right now. This demonstrates how much that small FormAPI extension is needed to produce lean code.

Okay. But the good news is: it finally works, tested well and makes the behavior of roles more intuitive. No string changes involved.

Remember that this should (and can) be changed to nicer code in D7, as soon as #104715 has been committed.

AttachmentSize
show_auth_user_role.patch 1.79 KB

#8

ximo - January 10, 2008 - 00:07

Thanks for picking this up again, I had totally forgotten about this issue :)

There is a string change in your patch though, my guess is that it snuck in from my earlier patch. The attached patch (alt #1) sets the string back to its "frozen" state. The other patch (alt #2) removes the description string alltogether, as the checkboxes are pretty self-explanatory.

Other than that, the patch looks great. The way it's done is somewhat hackish, but obviously the best we can do at the moment. And it works.

AttachmentSize
show_auth_user_role.patch (alt #1: string freeze friendly) 1.78 KB
show_auth_user_role.patch (alt #2: no description string) 1.62 KB

#9

Pancho - January 10, 2008 - 11:43

Oops, I overlooked that... I'd let the string in, we can change it for D7, when we have to touch that code again, anyway...

#10

ximo - January 11, 2008 - 01:05

Anybody care to comment on this? It would be nice to have this fixed for Drupal 6, as it is pretty bad usability..

I'd go for Alt #2, without the string. The checkboxes tell it all anyway.

#11

Gábor Hojtsy - January 11, 2008 - 01:45

Luckily it is not a string freeze violation to *remove* strings. To add is a violation and to modify is an evil violation.

#12

ximo - January 16, 2008 - 16:22

That's good, let's go for alternative 2 then. To recap, alternative 2 in comment #8 does this to admin/user/user/create:
- Adds a disabled "authenticated user" checkbox to the list of roles
- Removes the description "The user receives the combined permissions of the authenticated user role, and all roles selected here."

I believe it uses standard Forms API structure, and the HTML produced is all good.

This is a simple change, and by removing a long sentance and introducing a visual indicator instead, it's easier to understand and much clearner. Small fix, noticable difference. See this illustration for before/after shots.

I think this needs more feedback before it can be committed, so please comment :)

#13

Pancho - January 16, 2008 - 16:52

+1 This is great! What else should I say? It works. And it improves usability a lot. RTBC.

After committing please keep open! After #104715 has been committed to D7, we can simplify the usage of FormAPI.

#14

Pancho - January 21, 2008 - 14:40

We still need some feedback, please!

#15

Gábor Hojtsy - January 21, 2008 - 14:54

Well, make sure to add a code comment on the unusual (re)use of the 'checkboxes' form API construct. If I understand it right, you are adding a subelement to the form item, which will be part of the list of generated elements for that checkbox list. If this gets documented right, this looks like a good UI improvement.

#16

Pancho - January 21, 2008 - 22:58

Didn't apply anymore. Rerolled with a much more detailed code comment on this rather unusual FormAPI usage.
If the comment is not too long now, I guess it should be RTBC.

AttachmentSize
show_auth_user_role.patch 2.18 KB

#17

ximo - January 22, 2008 - 01:49
Status:needs review» reviewed & tested by the community

I added a TODO comment instead of the last line, so that IDEs will pick this up. I also referenced this issue instead, as we can't be sure the other issue actually will be relevant. This issue will provide background information and links to other relevant issues.

Other than that - RTBC if Gábor approves, and thanks for the help with this Pancho :)

AttachmentSize
show_auth_user_role_2_with_todo.patch 2.15 KB

#18

ximo - January 22, 2008 - 02:10

Rerolled for HEAD, this one should work!

AttachmentSize
show_auth_user_role.patch 2.14 KB

#19

Gábor Hojtsy - January 22, 2008 - 07:52
Status:reviewed & tested by the community» fixed

Thanks, committed.

#20

Anonymous (not verified) - February 5, 2008 - 08:12
Status:fixed» closed

Automatically closed -- issue fixed for two weeks with no activity.

#21

Pancho - February 8, 2008 - 01:39
Version:6.x-dev» 7.x-dev
Status:closed» needs work

Reopening this for D7. The rather ugly workaround should be replaced by more straight-forward code, after #104715 has been committed to HEAD (D7). I will post a patch after that has happened.

#22

Sutharsan - August 31, 2008 - 20:49
Component:user system» usability

Moving all usability issues to Drupal - component usability.

 
 

Drupal is a registered trademark of Dries Buytaert.