From 7547878cde9cbdf55ed292be2e795137990587d8 Mon Sep 17 00:00:00 2001 From: Penar Musaraj Date: Mon, 5 Sep 2022 09:31:17 +0200 Subject: [PATCH] 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. --- .../addon/controllers/admin-user-index.js | 5 +- .../discourse/app/controllers/user.js | 58 +++++++++--------- .../discourse/app/templates/user.hbs | 2 +- .../tests/acceptance/admin-user-index-test.js | 53 +++++++++++++++++ .../acceptance/user-profile-summary-test.js | 59 ++++++++++++++++++- config/locales/client.en.yml | 3 +- 6 files changed, 145 insertions(+), 35 deletions(-) diff --git a/app/assets/javascripts/admin/addon/controllers/admin-user-index.js b/app/assets/javascripts/admin/addon/controllers/admin-user-index.js index 5dbdaaa3233..b6859b9ce0e 100644 --- a/app/assets/javascripts/admin/addon/controllers/admin-user-index.js +++ b/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); }, }, { diff --git a/app/assets/javascripts/discourse/app/controllers/user.js b/app/assets/javascripts/discourse/app/controllers/user.js index 221a7e7048d..d1e053ef347 100644 --- a/app/assets/javascripts/discourse/app/controllers/user.js +++ b/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 = [ - { - label: I18n.t("composer.cancel"), - class: "btn", - link: true, - }, - { - label: - `${iconHTML("exclamation-triangle")} ` + - I18n.t("admin.user.delete_and_block"), - class: "btn btn-danger", - callback() { - performDestroy(true); + this.dialog.alert({ + title: I18n.t("admin.user.delete_confirm_title"), + message: I18n.t("admin.user.delete_confirm"), + class: "delete-user-modal", + buttons: [ + { + label: I18n.t("admin.user.delete_dont_block"), + class: "btn-primary", + action: () => { + return performDestroy(false); + }, }, - }, - { - label: I18n.t("admin.user.delete_dont_block"), - class: "btn btn-primary", - callback() { - performDestroy(false); + { + icon: "exclamation-triangle", + label: I18n.t("admin.user.delete_and_block"), + class: "btn-danger", + action: () => { + return performDestroy(true); + }, }, - }, - ]; - - bootbox.dialog(message, buttons, { classes: "delete-user-modal" }); + { + label: I18n.t("composer.cancel"), + }, + ], + }); }, updateNotificationLevel(params) { diff --git a/app/assets/javascripts/discourse/app/templates/user.hbs b/app/assets/javascripts/discourse/app/templates/user.hbs index c990875aaf6..b6706f30041 100644 --- a/app/assets/javascripts/discourse/app/templates/user.hbs +++ b/app/assets/javascripts/discourse/app/templates/user.hbs @@ -216,7 +216,7 @@ {{/if}} {{#if this.canDeleteUser}} -
+
{{/if}} diff --git a/app/assets/javascripts/discourse/tests/acceptance/admin-user-index-test.js b/app/assets/javascripts/discourse/tests/acceptance/admin-user-index-test.js index d0442a55b40..8e14be163d4 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/admin-user-index-test.js +++ b/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"); + }); }); diff --git a/app/assets/javascripts/discourse/tests/acceptance/user-profile-summary-test.js b/app/assets/javascripts/discourse/tests/acceptance/user-profile-summary-test.js index 7734a0b5048..169cb95748f 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/user-profile-summary-test.js +++ b/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"); + }); +}); diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 502dff6fe46..ea5fb2f12e8 100644 --- a/config/locales/client.en.yml +++ b/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.

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 block this email and IP address" delete_dont_block: "Delete only" deleting_user: "Deleting user..."