FIX: Regression with admin user delete dialog buttons (#18179)

This also adds a test to prevent regressions and refactors the very similar delete dialog in the user summary screen.

app/assets/javascripts/admin/addon/controllers/admin-user-index.js

@@ -378,6 +378,7 @@ export default Controller.extend(CanCheckEmails, {
       };
 
       this.dialog.alert({
+        title: I18n.t("admin.user.delete_confirm_title"),
         message: I18n.t("admin.user.delete_confirm"),
         class: "delete-user-modal",
         buttons: [
@@ -385,7 +386,7 @@ export default Controller.extend(CanCheckEmails, {
             label: I18n.t("admin.user.delete_dont_block"),
             class: "btn-primary",
             action: () => {
-              return performDestroy(true);
+              return performDestroy(false);
             },
           },
           {
@@ -393,7 +394,7 @@ export default Controller.extend(CanCheckEmails, {
             label: I18n.t("admin.user.delete_and_block"),
             class: "btn-danger",
             action: () => {
-              return performDestroy(false);
+              return performDestroy(true);
             },
           },
           {

app/assets/javascripts/discourse/app/controllers/user.js

@@ -4,10 +4,8 @@ import { and, equal, gt, not, or } from "@ember/object/computed";
 import CanCheckEmails from "discourse/mixins/can-check-emails";
 import User from "discourse/models/user";
 import I18n from "I18n";
-import bootbox from "bootbox";
 import discourseComputed from "discourse-common/utils/decorators";
 import getURL from "discourse-common/lib/get-url";
-import { iconHTML } from "discourse-common/lib/icon-library";
 import { isEmpty } from "@ember/utils";
 import optionalService from "discourse/lib/optional-service";
 import { prioritizeNameInUx } from "discourse/lib/settings";
@@ -16,6 +14,7 @@ import { dasherize } from "@ember/string";
 
 export default Controller.extend(CanCheckEmails, {
   router: service(),
+  dialog: service(),
   userNotifications: controller("user-notifications"),
   adminTools: optionalService(),
 
@@ -203,11 +202,10 @@ export default Controller.extend(CanCheckEmails, {
 
     adminDelete() {
       const userId = this.get("model.id");
-      const message = I18n.t("admin.user.delete_confirm");
       const location = document.location.pathname;
 
       const performDestroy = (block) => {
-        bootbox.dialog(I18n.t("admin.user.deleting_user"));
+        this.dialog.notice(I18n.t("admin.user.deleting_user"));
         let formData = { context: location };
         if (block) {
           formData["block_email"] = true;
@@ -216,43 +214,43 @@ export default Controller.extend(CanCheckEmails, {
         }
         formData["delete_posts"] = true;
 
-        this.adminTools
+        return this.adminTools
           .deleteUser(userId, formData)
           .then((data) => {
             if (data.deleted) {
               document.location = getURL("/admin/users/list/active");
             } else {
-              bootbox.alert(I18n.t("admin.user.delete_failed"));
+              this.dialog.alert(I18n.t("admin.user.delete_failed"));
             }
           })
-          .catch(() => bootbox.alert(I18n.t("admin.user.delete_failed")));
+          .catch(() => this.dialog.alert(I18n.t("admin.user.delete_failed")));
       };
 
-      const buttons = [
+      this.dialog.alert({
-        {
+        title: I18n.t("admin.user.delete_confirm_title"),
-          label: I18n.t("composer.cancel"),
+        message: I18n.t("admin.user.delete_confirm"),
-          class: "btn",
+        class: "delete-user-modal",
-          link: true,
+        buttons: [
-        },
-        {
-          label:
-            `${iconHTML("exclamation-triangle")} ` +
-            I18n.t("admin.user.delete_and_block"),
-          class: "btn btn-danger",
-          callback() {
-            performDestroy(true);
-          },
-        },
           {
             label: I18n.t("admin.user.delete_dont_block"),
-          class: "btn btn-primary",
+            class: "btn-primary",
-          callback() {
+            action: () => {
-            performDestroy(false);
+              return performDestroy(false);
             },
           },
-      ];
+          {
-
+            icon: "exclamation-triangle",
-      bootbox.dialog(message, buttons, { classes: "delete-user-modal" });
+            label: I18n.t("admin.user.delete_and_block"),
+            class: "btn-danger",
+            action: () => {
+              return performDestroy(true);
+            },
+          },
+          {
+            label: I18n.t("composer.cancel"),
+          },
+        ],
+      });
     },
 
     updateNotificationLevel(params) {

app/assets/javascripts/discourse/app/templates/user.hbs

@@ -216,7 +216,7 @@
               {{/if}}
 
               {{#if this.canDeleteUser}}
-                <div class='pull-right'><DButton @action={{action "adminDelete"}} @icon="exclamation-triangle" @label="user.admin_delete" @class="btn-danger" /></div>
+                <div class='pull-right'><DButton @action={{action "adminDelete"}} @icon="exclamation-triangle" @label="user.admin_delete" @class="btn-danger btn-delete-user" /></div>
               {{/if}}
             </dl>
             <PluginOutlet @name="user-profile-secondary" @tagName="span" @connectorTagName="div" @args={{hash model=this.model}} />

app/assets/javascripts/discourse/tests/acceptance/admin-user-index-test.js

@@ -10,6 +10,8 @@ import I18n from "I18n";
 import { SECOND_FACTOR_METHODS } from "discourse/models/user";
 
 const { TOTP, BACKUP_CODE, SECURITY_KEY } = SECOND_FACTOR_METHODS;
+let deleteAndBlock = null;
+
 acceptance("Admin - User Index", function (needs) {
   needs.user();
   needs.pretender((server, helper) => {
@@ -97,6 +99,34 @@ acceptance("Admin - User Index", function (needs) {
         allowed_methods: [TOTP, BACKUP_CODE, SECURITY_KEY],
       });
     });
+
+    server.get("/admin/users/5.json", () => {
+      return helper.response({
+        id: 5,
+        username: "user5",
+        name: null,
+        avatar_template: "/letter_avatar_proxy/v4/letter/b/f0a364/{size}.png",
+        active: true,
+        can_be_deleted: true,
+        post_count: 0,
+      });
+    });
+
+    server.delete("/admin/users/5.json", (request) => {
+      const data = helper.parsePostData(request.requestBody);
+
+      if (data.block_email || data.block_ip || data.block_urls) {
+        deleteAndBlock = true;
+      } else {
+        deleteAndBlock = false;
+      }
+
+      return helper.response({});
+    });
+  });
+
+  needs.hooks.beforeEach(() => {
+    deleteAndBlock = null;
   });
 
   test("can edit username", async function (assert) {
@@ -213,4 +243,27 @@ acceptance("Admin - User Index", function (needs) {
       "user is redirected to the 2FA page"
     );
   });
+
+  test("delete user - delete without blocking works as expected", async function (assert) {
+    await visit("/admin/users/5/user5");
+    await click(".btn-user-delete");
+
+    assert.equal(
+      query("#dialog-title").textContent,
+      I18n.t("admin.user.delete_confirm_title"),
+      "dialog has a title"
+    );
+
+    await click(".dialog-footer .btn-primary");
+
+    assert.notOk(deleteAndBlock, "user does not get blocked");
+  });
+
+  test("delete user - delete and block works as expected", async function (assert) {
+    await visit("/admin/users/5/user5");
+    await click(".btn-user-delete");
+    await click(".dialog-footer .btn-danger");
+
+    assert.ok(deleteAndBlock, "user does not get blocked");
+  });
 });

app/assets/javascripts/discourse/tests/acceptance/user-profile-summary-test.js

@@ -3,12 +3,14 @@ import {
   exists,
   query,
 } from "discourse/tests/helpers/qunit-helpers";
-import { visit } from "@ember/test-helpers";
+import { click, visit } from "@ember/test-helpers";
 import { test } from "qunit";
 import I18n from "I18n";
 import userFixtures from "discourse/tests/fixtures/user-fixtures";
 import { cloneJSON } from "discourse-common/lib/object";
 
+let deleteAndBlock = null;
+
 acceptance("User Profile - Summary", function (needs) {
   needs.user();
   needs.pretender((server, helper) => {
@@ -113,3 +115,58 @@ acceptance("User Profile - Summary - Stats", function (needs) {
     );
   });
 });
+
+acceptance("User Profile - Summary - Admin", function (needs) {
+  needs.user({
+    username: "eviltrout",
+  });
+
+  needs.pretender((server, helper) => {
+    server.get("/admin/users/5.json", () => {
+      return helper.response({
+        id: 5,
+        username: "charlie",
+        name: null,
+        avatar_template: "/letter_avatar_proxy/v4/letter/b/f0a364/{size}.png",
+        active: true,
+      });
+    });
+    server.delete("/admin/users/5.json", (request) => {
+      const data = helper.parsePostData(request.requestBody);
+
+      if (data.block_email || data.block_ip || data.block_urls) {
+        deleteAndBlock = true;
+      } else {
+        deleteAndBlock = false;
+      }
+
+      return helper.response({});
+    });
+  });
+
+  needs.hooks.beforeEach(() => {
+    deleteAndBlock = null;
+  });
+
+  test("Delete only action", async function (assert) {
+    await visit("/u/charlie/summary");
+    await click(".btn-delete-user");
+    await click(".dialog-footer .btn-primary");
+
+    assert.notOk(deleteAndBlock, "first button does not block user");
+  });
+
+  test("Delete and block", async function (assert) {
+    await visit("/u/charlie/summary");
+    await click(".btn-delete-user");
+
+    assert.equal(
+      query("#dialog-title").textContent,
+      I18n.t("admin.user.delete_confirm_title"),
+      "dialog has a title"
+    );
+
+    await click(".dialog-footer .btn-danger");
+    assert.ok(deleteAndBlock, "second button also block user");
+  });
+});

config/locales/client.en.yml

@@ -5390,7 +5390,8 @@ en:
         cant_delete_all_too_many_posts:
           one: "Can't delete all posts because the user has more than %{count} post. (delete_all_posts_max)"
           other: "Can't delete all posts because the user has more than %{count} posts. (delete_all_posts_max)"
-        delete_confirm: "It is generally preferable to anonymize users rather than deleting them, to avoid removing content from existing discussions.<br><br>Are you SURE you want to delete this user? This is permanent!"
+        delete_confirm_title: "Are you SURE you want to delete this user? This is permanent!"
+        delete_confirm: "It is generally preferable to anonymize users rather than deleting them, to avoid removing content from existing discussions."
         delete_and_block: "Delete and <b>block</b> this email and IP address"
         delete_dont_block: "Delete only"
         deleting_user: "Deleting user..."