Posted by ximo on February 14, 2007 at 3:39pm
11 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | user.module |
| Category: | task |
| Priority: | minor |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | needs backport to D7 |
Issue Summary
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.
Comments
#1
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
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...
#3
+1. It follows the KISS rule.
#4
A screenshot maybe? It helps to show this to the others ... :)
#5
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.)
#6
Your patch disables them all. Rest seems fine.
#7
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.
#8
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.
#9
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
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
Luckily it is not a string freeze violation to *remove* strings. To add is a violation and to modify is an evil violation.
#12
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
+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
We still need some feedback, please!
#15
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
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.
#17
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 :)
#18
Rerolled for HEAD, this one should work!
#19
Thanks, committed.
#20
Automatically closed -- issue fixed for two weeks with no activity.
#21
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
Moving all usability issues to Drupal - component usability.
#23
Not relevant anymore...
#24
Comment #21 was (and is) definitely still relevant - the ugly workaround is still there. However, it looks like that issue has been moved to #284917: Allow FAPI select, radios, and checkboxes to specify some options as disabled. I'll leave a comment there to make sure it fixes this code as part of that patch.
Moving this issue back to 6.x and leaving it closed.
#25
Nope, this belongs to the 8.x queue. The underlying FAPI-deficiency (#284917: Allow FAPI select, radios, and checkboxes to specify some options as disabled) is unfortunately still not solved. It might be backported to 7.x afterwards.
#26
There's really no reason to keep two issues open for the same thing, and the original bug here was fixed long ago.
Let's mark it postponed if you must leave it open, though. But I think it will be fixed just fine by the other issue.
#27
It turns out this always has been possible.
So here's a followup removing the cludgy workaround.
#28
The last submitted patch, 284917-show_auth_user_role-followup.patch, failed testing.
#29
Another try
#30
The last submitted patch, 284917-show_auth_user_role-followup-29.patch, failed testing.
#31
Oh my goodness.
If it's the "missing newline" bug that I keep running into, then this patch should work fine even for the picky testbot:
#32
currently a minor task