This is similar to #609108: Menu admin page should say something when you save and #620592: Taxonomy admin page ... need feedback when saving. Role admin page admin/people/permissions/roles doesn't reports anything when you save. Attached patch adds statement to print necessary status message.

Files: 
CommentFileSizeAuthor
#25 787684_role_weight_test_25.patch2.65 KBSivaji
PASSED: [[SimpleTest]]: [MySQL] 26,788 pass(es). View
#23 787684_role_weight_test_21.patch2.75 KBmarkabur
PASSED: [[SimpleTest]]: [MySQL] 25,924 pass(es). View
#20 787684_role_weight_test_20.patch2.43 KBmikey_p
PASSED: [[SimpleTest]]: [MySQL] 25,183 pass(es). View
#19 787684_role_weight_test_19.patch2.79 KBSivaji
PASSED: [[SimpleTest]]: [MySQL] 25,197 pass(es). View
#15 787684_role_weight_test_8.patch2.78 KBSivaji
PASSED: [[SimpleTest]]: [MySQL] 24,655 pass(es). View
#7 787684_role_weight_test_7.patch2.73 KBSivaji
PASSED: [[SimpleTest]]: [MySQL] 20,955 pass(es). View
#4 787684_role_weight_test_4.patch2.78 KBSivaji
PASSED: [[SimpleTest]]: [MySQL] 20,185 pass(es). View
#2 role_message_printout.jpg143.18 KBghills88
#1 787684_role_weight_test.patch2.33 KBSivaji
PASSED: [[SimpleTest]]: [MySQL] 20,170 pass(es). View
role_admin_form_fb_1.patch556 bytesSivaji
PASSED: [[SimpleTest]]: [MySQL] 20,140 pass(es). View

Comments

Sivaji’s picture

FileSize
2.33 KB
PASSED: [[SimpleTest]]: [MySQL] 20,170 pass(es). View

Attached is a patch with test for role weight change operation.

ghills88’s picture

FileSize
143.18 KB

I tested this patch off the latest HEAD from CVS and ran into unexpected behaviors on the Permissions tab.

Although the patch works as desired when saving changes on the Roles tab, anytime the permissions page is viewed the message is displayed dozens of time in a long list. See the screenshot attached.

markabur’s picture

Would it make sense to check whether user_role_save() was actually successful, and make the message reflect that?

Sivaji’s picture

FileSize
2.78 KB
PASSED: [[SimpleTest]]: [MySQL] 20,185 pass(es). View

@ghills88, Thank you for testing, i could not see any unexpected behaviors on the Permissions tab.

@markabur, I guess it makes sense to check user_role_save() response, attached patch will do it.

codi’s picture

Status: Needs review » Reviewed & tested by the community

Tested patch against alpha 4. Works as expected and doesn't display what ghills88 mentioned. Not sure if I'm allowed to switch the issue to RTBC without the automated tests running, but I'm doing it anyways.

Edit: Looks like the automated test ran and all is good.

Dries’s picture

Status: Reviewed & tested by the community » Needs work
+++ modules/user/user.admin.inc	4 May 2010 03:35:51 -0000
@@ -858,10 +858,17 @@ function user_admin_roles($form, $form_s
+    drupal_set_message(t('There is a problem in saving the form. Try later or contact site administrator'));

I'm not sure I agree with the 'try later or contact site administrator' comment. Not only does it have a typo, it is also something we shouldn't say. It is not very helpful, especially because chances are really high that the person performing the operation is the site administrator.

Sivaji’s picture

Status: Needs work » Needs review
Issue tags: -Quick fix
FileSize
2.73 KB
PASSED: [[SimpleTest]]: [MySQL] 20,955 pass(es). View

Attached patch uses drupal_set_message(t('Unable to save role setting.'), 'error'); instead of drupal_set_message(t('There is a problem in saving the form. Try later or contact site administrator'));. Let me know if there are any other changes to be made.

markabur’s picture

I think the new message is an improvement. Thanks for fixing the spelling error too. :-)

mmccollow’s picture

Issue tags: +Quick fix

I tested this against CVS HEAD and it works as expected. I think the message is clear, and having some feedback when you click save is a great improvement.

mmccollow’s picture

Issue tags: -Quick fix

Didn't mean to add the quick fix tag back, apologies. It's my first patch review :)

Cibes’s picture

Works fine and resolves the issue. (How would I get the operation to fail in order to test the error message?)

I don't know much about the weight system but just to be sure as it might happen in the random check: Assigning same weight to two roles is okay? And is there a limit that could be reached by just adding 1 no matter what the value is?

jergason’s picture

#7: 787684_role_weight_test_7.patch queued for re-testing.

jergason’s picture

Status: Needs review » Reviewed & tested by the community

Tested against HEAD and it works fine.

Dries’s picture

Status: Reviewed & tested by the community » Needs review
+++ modules/user/user.admin.inc	5 May 2010 07:35:25 -0000
@@ -858,10 +858,17 @@ function user_admin_roles($form, $form_s
+    drupal_set_message(t('Unable to save role setting.'), 'error');

Shouldn't that be 'settings' (plural)?

+++ modules/user/user.admin.inc	5 May 2010 07:35:25 -0000
@@ -858,10 +858,17 @@ function user_admin_roles($form, $form_s
+    drupal_set_message(t('The role setting has been saved.'));

Ditto.

+++ modules/user/user.test	5 May 2010 07:35:26 -0000
@@ -1466,7 +1466,7 @@ class UserRoleAdminTestCase extends Drup
+      'description' => 'Test adding, editing and deleting user roles and role weight change operation.',

Shouldn't that be 'operations' (plural)?

Sivaji’s picture

FileSize
2.78 KB
PASSED: [[SimpleTest]]: [MySQL] 24,655 pass(es). View

Attached patch makes changes mentioned in #14.

tim.plunkett’s picture

Issue tags: +Quick fix
+++ modules/user/user.admin.inc	8 Sep 2010 06:07:31 -0000
@@ -860,10 +860,17 @@ function user_admin_roles($form, $form_s
+    drupal_set_message(t('There is a problem in saving the form. Try later or contact site administrator'));

Needs full stop.

+++ modules/user/user.test	8 Sep 2010 06:07:35 -0000
@@ -1641,6 +1641,28 @@ class UserRoleAdminTestCase extends Drup
+    // Retrive the saved role and compare its weight.

Retrive/Retrieve

Powered by Dreditor.

tim.plunkett’s picture

Issue tags: -Quick fix
markabur’s picture

Status: Needs review » Needs work

Updating status

Sivaji’s picture

Status: Needs work » Needs review
FileSize
2.79 KB
PASSED: [[SimpleTest]]: [MySQL] 25,197 pass(es). View

Rerolled with changes mentioned in #16.

mikey_p’s picture

FileSize
2.43 KB
PASSED: [[SimpleTest]]: [MySQL] 25,183 pass(es). View

The changes from #6 and #7 didn't make it into the latest patch.

markabur’s picture

Looks good, the changes are all there and the message shows when re-ordered roles are saved. This test description has some weird grammar:

-      'description' => 'Test adding, editing and deleting user roles.',
+      'description' => 'Test adding, editing and deleting user roles and role weight change operations.',

How about:

+      'description' => 'Test adding, editing and deleting user roles and changing role weights.',

Also, while we're here, might as well fix this typo up in line 1349 (creat => create):

      'description' => 'Test the creat user administration page.',

Otherwise it's RTBC as far as I can tell.

markabur’s picture

Status: Needs review » Needs work
markabur’s picture

Status: Needs work » Needs review
FileSize
2.75 KB
PASSED: [[SimpleTest]]: [MySQL] 25,924 pass(es). View

Reroll with those nits from #21.

Sivaji’s picture

Issue tags: +string freeze

Adding string freeze tag

Sivaji’s picture

FileSize
2.65 KB
PASSED: [[SimpleTest]]: [MySQL] 26,788 pass(es). View

Attached is the patch rolled against the recent code base with following changes

1. Removed user_role_save() status check.

+    $status[] = user_role_save($role);
+  }
+  if (in_array(FALSE, $status)) {
+    drupal_set_message(t('Unable to save role settings.'), 'error');
+  }
+  else {
+    drupal_set_message(t('The role settings have been updated.'));
   }

2. Replaced 'rand' to 'random' in comment line.

+    // Pick up a rand role and get its weight.
+    // Pick up a random role and get its weight.

Hope this is ready to go.

mikey_p’s picture

Status: Needs review » Reviewed & tested by the community

Still applies and looks good to me. The error message isn't very helpful anyway.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Looks like all previous feedback has been addressed.

Committed to HEAD. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Novice, -string freeze

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