We also should do a followup issue somewhere about why there is no confirmation form on "delete role"... that's a little dangerous. However, it's a preexisting problem.

http://drupal.org/node/619584#comment-2748900

Files: 
CommentFileSizeAuthor
#12 750290_confirm_role_delete_4.patch4.09 KBSivaji
PASSED: [[SimpleTest]]: [MySQL] 18,993 pass(es). View
#10 750290_confirm_role_delete_3.patch3.71 KBSivaji
PASSED: [[SimpleTest]]: [MySQL] 18,995 pass(es). View
#6 750290_confirm_role_delete_2.patch3.82 KBSivaji
FAILED: [[SimpleTest]]: [MySQL] 18,932 pass(es), 2 fail(s), and 4 exception(es). View
#2 750290_confirm_role_delete_1.patch2.91 KBSivaji
FAILED: [[SimpleTest]]: [MySQL] 18,816 pass(es), 2 fail(s), and 0 exception(es). View

Comments

catch’s picture

Title: CRSF for delete role » Add a confirm form for role deletion
Priority: Critical » Normal

Spoke to Mr Bailey's in irc, I hadn't actually reviewed the hunk of code and we determined it's a not a CRSF due to lack of link, just inconsistent, so retitling and downgrading.

Sivaji’s picture

Status: Active » Needs review
Issue tags: +Novice
FileSize
2.91 KB
FAILED: [[SimpleTest]]: [MySQL] 18,816 pass(es), 2 fail(s), and 0 exception(es). View

Attached patch will add confirm form.

Status: Needs review » Needs work

The last submitted patch, 750290_confirm_role_delete_1.patch, failed testing.

aspilicious’s picture

Probably you need to change some tests

sun’s picture

The tests need to be updated accordingly.

+++ modules/user/user.admin.inc	26 Mar 2010 10:29:37 -0000
@@ -961,15 +961,18 @@ function user_admin_role_validate($form,
+ * Form submit handler for the user_admin_role() form.
+ */ ¶

Trailing white-space here (and below).

+++ modules/user/user.admin.inc	26 Mar 2010 10:29:37 -0000
@@ -961,15 +961,18 @@ function user_admin_role_validate($form,
+  elseif ($form_state['values']['op'] == t('Delete role')) {    ¶
+    $form_state['redirect'] = 'admin/people/permissions/roles/delete/' . $form_state['values']['rid'];
+    return;
   }

This should really be a separate form button submit handler.

+++ modules/user/user.admin.inc	26 Mar 2010 10:29:37 -0000
@@ -980,6 +983,30 @@ function user_admin_role_submit($form, &
+  user_role_delete((int) $form_state['values']['rid']);

The cast to integer shouldn't be required.

142 critical left. Go review some!

Sivaji’s picture

Status: Needs review » Needs work
FileSize
3.82 KB
FAILED: [[SimpleTest]]: [MySQL] 18,932 pass(es), 2 fail(s), and 4 exception(es). View

1. Updated test.

// Test deleting a role.
-    $this->drupalPost("admin/people/permissions/roles/edit/{$role->rid}", NULL, t('Delete role'));
+    $this->drupalPost("admin/people/permissions/roles/delete/{$role->rid}", NULL, t('Delete'));

2. Removed type casting and $form['name'] from user_admin_role_delete_confirm().

Sivaji’s picture

Status: Needs work » Needs review

The last submitted patch, 750290_confirm_role_delete_2.patch, failed testing.

sun’s picture

+++ modules/user/user.module	26 Mar 2010 13:54:51 -0000
@@ -1533,7 +1533,13 @@ function user_menu() {
+  $items['admin/people/permissions/roles/delete/%user_role'] = array(
+    'title' => 'Delete role',
+    'page arguments' => array('user_admin_role_delete_confirm', 5),

We are missing a 'page callback' here.

+++ modules/user/user.admin.inc	26 Mar 2010 13:54:48 -0000
@@ -968,8 +971,8 @@ function user_admin_role_submit($form, &
   elseif ($form_state['values']['op'] == t('Delete role')) {
-    user_role_delete((int) $form_state['values']['rid']);
-    drupal_set_message(t('The role has been deleted.'));
+    $form_state['redirect'] = 'admin/people/permissions/roles/delete/' . $form_state['values']['rid'];
+    return;
   }

We still want to add a separate form button #submit handler user_admin_role_delete_submit() instead of this mixing here.

+++ modules/user/user.test	26 Mar 2010 13:54:53 -0000
@@ -1506,7 +1506,7 @@ class UserRoleAdminTestCase extends Drup
     // Test deleting a role.
-    $this->drupalPost("admin/people/permissions/roles/edit/{$role->rid}", NULL, t('Delete role'));
+    $this->drupalPost("admin/people/permissions/roles/delete/{$role->rid}", NULL, t('Delete'));

We should keep the original line here and just add a

$this->drupalPost(NULL, NULL, t('Delete'));

afterwards, so the proper redirection is tested as well.

142 critical left. Go review some!

Sivaji’s picture

FileSize
3.71 KB
PASSED: [[SimpleTest]]: [MySQL] 18,995 pass(es). View

@Sun Thanks for your comments. Type casting is required otherwise user_role_delete() does not works as expected.

We are missing a 'page callback' here.

Added page callback.

FYI: Role edit menu item is also not using 'page callback' see http://drupal.org/node/619584#comment-2771532

We still want to add a separate form button #submit handler user_admin_role_delete_submit() instead of this mixing here.

I am not sure how to add #submit there.

We should keep the original line here and just add a $this->drupalPost(NULL, NULL, t('Delete'));

Done.

andypost’s picture

@sivaji Take an example from http://api.drupal.org/api/function/node_form/7 for handler node_form_delete_submit()

Sivaji’s picture

Status: Needs work » Needs review
FileSize
4.09 KB
PASSED: [[SimpleTest]]: [MySQL] 18,993 pass(es). View

I guess point 2 in #9 mean this

@@ -934,6 +934,7 @@ function user_admin_role($form, $form_st
   $form['actions']['delete'] = array(
     '#type' => 'submit',
     '#value' => t('Delete role'),
+    '#submit' => array('user_admin_role_delete_submit'),
   );
 function user_admin_role_submit($form, &$form_state) {
   $role = (object)$form_state['values'];
   if ($form_state['values']['op'] == t('Save role')) {
     user_role_save($role);
     drupal_set_message(t('The role has been renamed.'));
   }
-  elseif ($form_state['values']['op'] == t('Delete role')) {
-    user_role_delete((int) $form_state['values']['rid']);
-    drupal_set_message(t('The role has been deleted.'));
-  }
   elseif ($form_state['values']['op'] == t('Add role')) {
     user_role_save($role);
     drupal_set_message(t('The role has been added.'));
 /**
+ * Form submit handler for the user_admin_role() form.
+ */
+function user_admin_role_delete_submit($form, &$form_state) {
+  $form_state['redirect'] = 'admin/people/permissions/roles/delete/' . $form_state['values']['rid'];
+}
+
Dries’s picture

Status: Needs review » Fixed

This looked good to me. Committed to CVS HEAD. Thanks!

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

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