og_user_roles_page_form_submit deleting admin roles

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

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.

Here's a screen shot showing the altered table.

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.

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?

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.

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. :)

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.

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?

well, let it be a different issue.