Posted by sivaji on May 2, 2010 at 11:25am
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | user.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Novice, string freeze |
Issue Summary
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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| role_admin_form_fb_1.patch | 556 bytes | Idle | PASSED: [[SimpleTest]]: [MySQL] 20,140 pass(es). | View details |
Comments
#1
Attached is a patch with test for role weight change operation.
#2
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.
#3
Would it make sense to check whether user_role_save() was actually successful, and make the message reflect that?
#4
@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.#5
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.
#6
+++ 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.
#7
Attached patch uses
drupal_set_message(t('Unable to save role setting.'), 'error');instead ofdrupal_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.#8
I think the new message is an improvement. Thanks for fixing the spelling error too. :-)
#9
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.
#10
Didn't mean to add the quick fix tag back, apologies. It's my first patch review :)
#11
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?
#12
#7: 787684_role_weight_test_7.patch queued for re-testing.
#13
Tested against HEAD and it works fine.
#14
+++ 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)?
#15
Attached patch makes changes mentioned in #14.
#16
+++ 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.
#17
#18
Updating status
#19
Rerolled with changes mentioned in #16.
#20
The changes from #6 and #7 didn't make it into the latest patch.
#21
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.
#22
#23
Reroll with those nits from #21.
#24
Adding
string freezetag#25
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.
#26
Still applies and looks good to me. The error message isn't very helpful anyway.
#27
Looks like all previous feedback has been addressed.
Committed to HEAD. Thanks!
#28
Automatically closed -- issue fixed for 2 weeks with no activity.