og_user_roles_page_form_submit deleting admin roles
mfer - November 13, 2009 - 20:31
| Project: | OG User Roles |
| Version: | 6.x-4.0 |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
Description
The Roles form is deleting roles not listed on the form. This happens in og_user_roles_page_form_submit.
How to reproduce:
- Add two roles.
- In the config at admin/og/og_user_roles add one role to the form and auto assign the other role to new group admins.
- When you use the roles form on the group og_user_roles_page_form_submit will remove the admin role as well.
og_user_roles_page_form_submit should only process the roles on the form itself.
#1
I'm seeing this too, though it's frustrating that the automatic roles don't appear in the form at all at ?q=og/users/$gid/roles
#2
Here's a patch that fixes the bug and the usability issue of the automatic roles not being visible. Just the changes to function og_user_roles_page_form_submit() are enough to fix the bug of the roles being removed.
re-used a t() string, so no alteration to .po files needed.
#3
Here's a screen shot showing the altered table.
#4
Thanks!
Unfortunately, it seems like the whole default roles functionality is not tested yet. :-/ Would it be too much to ask you to also enhance the tests?
+++ og_user_roles.pages.inc 13 Nov 2009 21:13:26 -0000@@ -109,12 +110,24 @@ function og_user_roles_page_form($form_s
+ $auto_role_names = array();
...
+ $form['user_roles'][$account->uid]['automatic_roles'] = array(
This variable + form structure key should be named $default_roles.
+++ og_user_roles.pages.inc 13 Nov 2009 21:13:26 -0000@@ -109,12 +110,24 @@ function og_user_roles_page_form($form_s
+ '#type' => 'markup',
We can skip the #type here.
+++ og_user_roles.pages.inc 13 Nov 2009 21:13:26 -0000@@ -143,12 +158,13 @@ function og_user_roles_page_form_submit(
+ $header = array_merge(array('', '<em>'. t('Default role assignments') .'</em>'), $form['#roles']);
Please remove the <em> and shorten to "Default roles".
This review is powered by Dreditor.
#5
I tested the defaults by hand - and mfer is using them. I can re-roll the patch, but don't really have the time to code up tests right now.
hmm, I guess there are no translations yet - I was trying to re-use the t() string t('Default role assignments')
I used the EM tags to highlight the fact that this heading was not a role name - some other approach?
#6
Here's a re-roll. Moves the check_plain() of role names to an array_map() call so as to keep doing it for the same strings for many users.
#7
I've tested the patch and it works.
#8
ok, I took the time to parse the original post into a test that does exactly the same.
And, indeed, the last assertion is failing.
I committed attached patch to HEAD. Let's see whether this patch makes it work. :)
#9
Thanks for reporting, reviewing, and testing! Committed attached patch to HEAD.
A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.
#10
Automatically closed -- issue fixed for 2 weeks with no activity.
#11
Looking at the resulting screen by coincidence in http://localize.drupal.org/node/616...
It looks like we should skip the "Default roles" column entirely when no default roles have been assigned?