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.

CommentFileSizeAuthor
#25 787684_role_weight_test_25.patch2.65 KBSivaji_Ganesh_Jojodae
#23 787684_role_weight_test_21.patch2.75 KBmarkabur
#20 787684_role_weight_test_20.patch2.43 KBmikey_p
#19 787684_role_weight_test_19.patch2.79 KBSivaji_Ganesh_Jojodae
#15 787684_role_weight_test_8.patch2.78 KBSivaji_Ganesh_Jojodae
#7 787684_role_weight_test_7.patch2.73 KBSivaji_Ganesh_Jojodae
#4 787684_role_weight_test_4.patch2.78 KBSivaji_Ganesh_Jojodae
#2 role_message_printout.jpg143.18 KBghills88
#1 787684_role_weight_test.patch2.33 KBSivaji_Ganesh_Jojodae
role_admin_form_fb_1.patch556 bytesSivaji_Ganesh_Jojodae
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sivaji_Ganesh_Jojodae’s picture

Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commented
FileSize
787684_role_weight_test.patch2.33 KB

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

ghills88’s picture

ghills88 CreditAttribution: ghills88 commented
FileSize
role_message_printout.jpg143.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

markabur CreditAttribution: markabur commented

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

Sivaji_Ganesh_Jojodae’s picture

Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commented
FileSize
787684_role_weight_test_4.patch2.78 KB

@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

codi CreditAttribution: codi commented
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

Dries CreditAttribution: Dries commented
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_Ganesh_Jojodae’s picture

Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commented
Status: Needs work » Needs review
Issue tags: -Quick fix
FileSize
787684_role_weight_test_7.patch2.73 KB

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

markabur CreditAttribution: markabur commented

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

mmccollow’s picture

mmccollow CreditAttribution: mmccollow commented
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

mmccollow CreditAttribution: mmccollow commented
Issue tags: -Quick fix

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

Cibes’s picture

Cibes CreditAttribution: Cibes commented

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

jergason CreditAttribution: jergason commented

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

jergason’s picture

jergason CreditAttribution: jergason commented
Status: Needs review » Reviewed & tested by the community

Tested against HEAD and it works fine.

Dries’s picture

Dries CreditAttribution: Dries commented
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_Ganesh_Jojodae’s picture

Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commented
FileSize
787684_role_weight_test_8.patch2.78 KB

Attached patch makes changes mentioned in #14.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
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

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Issue tags: -Quick fix
markabur’s picture

markabur CreditAttribution: markabur commented
Status: Needs review » Needs work

Updating status

Sivaji_Ganesh_Jojodae’s picture

Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commented
Status: Needs work » Needs review
FileSize
787684_role_weight_test_19.patch2.79 KB

Rerolled with changes mentioned in #16.

mikey_p’s picture

mikey_p CreditAttribution: mikey_p commented
FileSize
787684_role_weight_test_20.patch2.43 KB

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

markabur’s picture

markabur CreditAttribution: markabur commented

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

markabur CreditAttribution: markabur commented
Status: Needs review » Needs work
markabur’s picture

markabur CreditAttribution: markabur commented
Status: Needs work » Needs review
FileSize
787684_role_weight_test_21.patch2.75 KB

Reroll with those nits from #21.

Sivaji_Ganesh_Jojodae’s picture

Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commented
Issue tags: +String freeze

Adding string freeze tag

Sivaji_Ganesh_Jojodae’s picture

Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commented
FileSize
787684_role_weight_test_25.patch2.65 KB

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

mikey_p CreditAttribution: mikey_p commented
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

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
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.

Issue tags: +Novice, +String freeze

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.