User cancel confirmation does not work with user cancel action

I think I fixed the issue, submitting my first patch ever..

Thanks for the patch! However, I assume this issue exists in Drupal 8 too, in which case it needs to be fixed there and backported.

Also looks like a possible duplicate of #935232: "Require e-mail confirmation to cancel account" when cancelling multiple accounts., although since this issue already has a patch I'll mark that one duplicate instead.

Whoa, I definitely added the wrong tag there :)

Here's Drupal 8 version of the patch.
Not sure how to backport... But now this thread has both patches in it :)

Can anyone review this properly for Drupal 8?

I don't have a mail server set up and I think this is a pretty important fix actually.

#6: drupal8-cancel_user_email_fix-1599286-6.patch queued for re-testing.

Nice work @donutdan4114... generally we don't rtbc our own patches!

Also as this is a bug we need to write a test to make sure we don't break this in the future... You need to write a simpletest (https://drupal.org/simpletest) and this will help you test emails... http://api.drupal.org/api/drupal/modules%21simpletest%21drupal_web_test_...

Thanks for the info @alexpott, not sure when I'll have enough time to do those tests. If someone else wants to write the tests, that'd be helpful...

Restoring tag

Wow d.o it says I've added the tag but it didn't really add! lets try again...

Drupal7 patch is in comment #1.

Drupal 7 patch with tests (cannot currently get Drupal 8 setup on my local).

Attempting to roll D8 patch even though I can't install D8 on my local yet.

@donutdan4114 thanks for the work!

+++ b/core/modules/user/lib/Drupal/user/Tests/UserCancelTest.phpundefined
@@ -438,4 +438,57 @@ function testMassUserCancelByAdmin() {
+   * Create multiple users and sends e-mail notifications to confirm account deletion.

Should be less than 80 chars and start with Creates (see http://drupal.org/coding-standards)... but as this is a test it should be something like: Tests e-mail notification during multiple user cancellation.

+++ b/core/modules/user/lib/Drupal/user/Tests/UserCancelTest.phpundefined
@@ -438,4 +438,57 @@ function testMassUserCancelByAdmin() {
+    config('user.settings')->set('cancel_method', 'user_cancel_reassign')->save();
+    // Enable account cancellation notification.
+    config('user.settings')->set('notify.status_canceled', TRUE)->save();

This should be written like this:

    Drupal::config('user.settings')
      ->set('cancel_method', 'user_cancel_reassign')
      ->set('notify.status_canceled', TRUE)
      ->save();

+++ b/core/modules/user/lib/Drupal/user/Tests/UserCancelTest.phpundefined
@@ -438,4 +438,57 @@ function testMassUserCancelByAdmin() {
+    $this->assertText(t('Are you sure you want to cancel these user accounts?'), 'Confirmation form to cancel accounts displayed.');
+    $this->assertText(t('When cancelling these accounts'), 'Allows to select account cancellation method.');
+    $this->assertText(t('Require e-mail confirmation to cancel account.'), 'Allows to send confirmation mail.');
+    $this->assertText(t('Notify user when account is canceled.'), 'Allows to send notification mail.');

It's not really necessary to test the text here - this patch has not changed the text.

+++ b/core/modules/user/lib/Drupal/user/Tests/UserCancelTest.phpundefined
@@ -438,4 +438,57 @@ function testMassUserCancelByAdmin() {
+    $status = TRUE;

With the changes suggested before we can remove this.

+++ b/core/modules/user/lib/Drupal/user/Tests/UserCancelTest.phpundefined
@@ -438,4 +438,57 @@ function testMassUserCancelByAdmin() {
+      $status = $status && (strpos($this->content, t('A confirmation request to cancel %name account has been sent to %mail address.', array('%name' => $account->name, '%mail' => $account->mail))) !== FALSE);

We should just assert that each message appears in the content here...

+++ b/core/modules/user/lib/Drupal/user/Tests/UserCancelTest.phpundefined
@@ -438,4 +438,57 @@ function testMassUserCancelByAdmin() {
+      $user = user_load($account->uid, TRUE);
+      // Account exists.
+      $status = $status && $user;
+      // Make sure account is still enabled.
+      $status = $status && $user->status;
+    }
+    $this->assertTrue($status, 'Users are found in the database.');

Rather than combining all the assertions in to one just $this->assertTrue($user->status, "User still enabled after cancel confirmation sent"); or something like that.

+++ b/core/modules/user/user.moduleundefined
@@ -1642,13 +1642,20 @@ function _user_cancel($edit, $account, $method) {
+        drupal_set_message(t('A confirmation request to cancel %name account has been sent to %email address.',array('%name'=>$account->name,'%email'=>$account->mail)));

Missing a spaces and I think the message can be improved (which means the test has to change).. something along the lines of drupal_set_message(t('Cancellation request sent to %email.', array('%email' => $account->mail)));

*edit Alexpott got to it first.

Updated patch based on @alexpott recommendations.

Just a couple more tweaks:

+++ b/core/modules/user/lib/Drupal/user/Tests/UserCancelTest.phpundefined
@@ -438,4 +438,52 @@ function testMassUserCancelByAdmin() {
     $this->assertEqual($user1->status, 1, 'User #1 still exists and is not blocked.');
   }
+
+
+  /**

Only need on blank line

+++ b/core/modules/user/lib/Drupal/user/Tests/UserCancelTest.phpundefined
@@ -438,4 +438,52 @@ function testMassUserCancelByAdmin() {
+  function testMassUserCancelByAdminConfirm() {

public function testMassUserCancelByAdminConfirm() {

+++ b/core/modules/user/lib/Drupal/user/Tests/UserCancelTest.phpundefined
@@ -438,4 +438,52 @@ function testMassUserCancelByAdmin() {
+    Drupal::config('user.settings')

This needs to be \Drupal::config('user.settings'), sorry about the confusion

+++ b/core/modules/user/lib/Drupal/user/Tests/UserCancelTest.phpundefined
@@ -438,4 +438,52 @@ function testMassUserCancelByAdmin() {
+    $admin_user = $this->drupalCreateUser(array('administer users'));
+    $this->drupalLogin($admin_user);

You can combine this, no need for a variable.
$this->drupalLogin($this->drupalCreateUser(array('administer users')));

+++ b/core/modules/user/lib/Drupal/user/Tests/UserCancelTest.phpundefined
@@ -438,4 +438,52 @@ function testMassUserCancelByAdmin() {
+      $account = $this->drupalCreateUser(array());

You don't need the array(), that's the default value

+++ b/core/modules/user/lib/Drupal/user/Tests/UserCancelTest.phpundefined
@@ -438,4 +438,52 @@ function testMassUserCancelByAdmin() {
+    // Send confirmation e-emails to users.
...
+      $this->assertText(t('Cancellation request sent to %email.', array('%mail' => $account->mail)));

+++ b/core/modules/user/user.moduleundefined
@@ -1642,13 +1642,20 @@ function _user_cancel($edit, $account, $method) {
+        drupal_set_message(t('Cancellation request sent to %email.', array('%email' => $account->mail)));

e-emails

+++ b/core/modules/user/lib/Drupal/user/Tests/UserCancelTest.phpundefined
@@ -438,4 +438,52 @@ function testMassUserCancelByAdmin() {
+      $this->assertTrue($user->uid, "User account exists.");
...
+      $this->assertTrue($user->status, "User account is enabled.");

Use single quotes unless double quotes are needed.

Thanks @tim.plunkett for more good feedback

bad patch..

Uploading a non-empty patch this time.

Trying assertRaw().

Trying to apply D7 patch to D8, doesn't work...

Tiny update to the D8 patch.

D7 patch update.

Bumping this, as it's been fixed for awhile and should be fixed in both Drupal 7 and 8.

No one will review it if its marked for the wrong version.

#30: drupal8-confirm_user_cancel_with_tests-1599286-29.patch queued for re-testing.

Created new Drupal 8 patch, the whole user cancellation process changed since the previous patch.

Try #2

Try #3

Changed $account->uid to $account->id()..

I think I figured out all the new D8 changes...

#43: drupal8-confirm_user_cancel_with_tests-1599286-43.patch queued for re-testing.

#43: drupal8-confirm_user_cancel_with_tests-1599286-43.patch queued for re-testing.

31: drupal7-confirm_user_cancel_with_tests-1599286-31.patch queued for re-testing.

31: drupal7-confirm_user_cancel_with_tests-1599286-31.patch queued for re-testing.

Generally the patch for D7 looks good to me.

The code is just weird for the user_mail() call to stop the flow of the function ...

Can we return or use else {} instead?

Fabianx, not sure what you're referring to.

Patch in #31 still good for Drupal 7.

This still needs to be fixed in Drupal 8 first (https://www.drupal.org/core/backport-policy) - as far as I can see it wasn't fixed there yet.

Added logic in D8 when deleting multiple user accounts to listen to the user_cancel_confirm checkbox.

Added test.

Still needs review/RTBC.... 3 years in the making.

Verified as an ongoing concern.

It seems the only way you can get to this page with core is through the 'user_cancel_user_action' Action plugin ( \Drupal\user\Plugin\Action\CancelUser)

Rewritten summary reflecting the current state of Drupal / appropriate references and terminology.

Since #62 does not apply on >= 8.9, I tried to do a reroll. Also did a few minor changes.

Fix Custom Commands Failed.

Retry.

And again.

Hi

Adding patch for 9.3.x for review.

Regarding #80: We ideally do the issue fixing in the latest fix version(9.3.x in this case) and generally do not back tag the core version unless we have a pressing reason to do so. The issue persists in the latest 9.3.x.

There were few deprecated functions used which I replaced respectively.

Applied patch #81 in drupal -9.3.x-dev
Applied successfully and looks good for me

For ref sharing screenshots ....

Note that without #2829964 this feature only works for users that already have an active session.

Composer cannot apply this patch with drupal 9.4.8

Rerolled #81 patch for 10.1-dev and also improved how the issue is covered with tests. (For some reasons I could not generate a proper interdiff between #81 and my changes, sorry.)

#85 Thx for the heads up but if I am right that is not a blocker for solving this task.

Update: MR diff also applies on 9.4.8.

1) Drupal\Tests\user\Functional\UserCancelTest::testMassUserCancelConfirmByAdmin
All cancelled user accounts got a confirmation e-mail.
Failed asserting that actual size 2 matches expected size 3.

FTW, it passes on my local perfectly. Transient error? Let's retry...

We have had a previous patch in production for months without issues. This latest MR adds welcome test coverage.
Moving to RTBC.

I'm doing a light review of RTBC issues. It is really exciting to see an older issue get to RTBC. Thanks everyone!

This really needs an Issue Summary update. Explaining the resolution is valuable to reviewers and committers. Adding tag for an update. I tested the patch, not rigorously, and it does appear to work. I have yet to do a code review.

I reviewed, looking at the comments mostly. I can't comment on the quality of the fix itself.

I did not when testing the change that the admin does not get a status message when the cancel action is taken. The other actions I tried to show a message.

Attaching patch from current state of MR!2958.
Applies fine on latest Drupal 10.6.x.

The only drawback I have is that when the admin requires confirmation, the browser is redirected to the /admin/people page without any visible notification that the users were notified.