From 06b7c44593fe8299d120a8956b57b4a9b484131e Mon Sep 17 00:00:00 2001 From: Krzysztof Kotlarek Date: Fri, 15 Jan 2021 09:43:26 +1100 Subject: [PATCH] FEATURE: reason to reject user signup (#11700) Feature for `Must Approve Users` setup. When a user is rejected, a staff member can optionally set a reason for audit purposes. In addition, feedback email can be sent to the user. Meta: https://meta.discourse.org/t/account-rejection-email/103112/8 --- .../app/components/reviewable-item.js | 13 +++++++++ .../controllers/reject-reason-reviewable.js | 22 +++++++++++++++ .../templates/components/reviewable-user.hbs | 4 +++ .../modal/reject-reason-reviewable.hbs | 19 +++++++++++++ .../discourse/tests/acceptance/review-test.js | 28 +++++++++++++++++++ .../tests/helpers/review-pretender.js | 25 ++++++++++++++++- app/controllers/reviewables_controller.rb | 2 ++ app/jobs/regular/user_email.rb | 2 ++ app/mailers/user_notifications.rb | 8 ++++++ app/models/reviewable.rb | 2 ++ app/models/reviewable_user.rb | 13 +++++++++ .../reviewable_action_serializer.rb | 5 +++- app/serializers/reviewable_user_serializer.rb | 2 +- config/locales/client.en.yml | 4 +++ config/locales/server.en.yml | 8 ++++++ ...025920_add_reject_reason_to_reviewables.rb | 7 +++++ lib/reviewable/actions.rb | 2 +- spec/models/reviewable_user_spec.rb | 18 ++++++++++-- 18 files changed, 178 insertions(+), 6 deletions(-) create mode 100644 app/assets/javascripts/discourse/app/controllers/reject-reason-reviewable.js create mode 100644 app/assets/javascripts/discourse/app/templates/modal/reject-reason-reviewable.hbs create mode 100644 db/migrate/20210111025920_add_reject_reason_to_reviewables.rb diff --git a/app/assets/javascripts/discourse/app/components/reviewable-item.js b/app/assets/javascripts/discourse/app/components/reviewable-item.js index 585a54f7ff7..bd77ff16f2f 100644 --- a/app/assets/javascripts/discourse/app/components/reviewable-item.js +++ b/app/assets/javascripts/discourse/app/components/reviewable-item.js @@ -110,6 +110,10 @@ export default Component.extend({ `/review/${reviewable.id}/perform/${action.id}?version=${version}`, { type: "PUT", + data: { + send_email: reviewable.sendEmail, + reject_reason: reviewable.rejectReason, + }, } ) .then((result) => { @@ -222,12 +226,21 @@ export default Component.extend({ } let msg = action.get("confirm_message"); + let requireRejectReason = action.get("require_reject_reason"); if (msg) { bootbox.confirm(msg, (answer) => { if (answer) { return this._performConfirmed(action); } }); + } else if (requireRejectReason) { + showModal("reject-reason-reviewable", { + title: "review.reject_reason.title", + model: this.reviewable, + }).setProperties({ + performConfirmed: this._performConfirmed.bind(this), + action, + }); } else { return this._performConfirmed(action); } diff --git a/app/assets/javascripts/discourse/app/controllers/reject-reason-reviewable.js b/app/assets/javascripts/discourse/app/controllers/reject-reason-reviewable.js new file mode 100644 index 00000000000..c1b4b7239e2 --- /dev/null +++ b/app/assets/javascripts/discourse/app/controllers/reject-reason-reviewable.js @@ -0,0 +1,22 @@ +import Controller from "@ember/controller"; +import ModalFunctionality from "discourse/mixins/modal-functionality"; +import { action } from "@ember/object"; + +export default Controller.extend(ModalFunctionality, { + rejectReason: null, + sendEmail: false, + + onShow() { + this.setProperties({ rejectReason: null, sendEmail: false }); + }, + + @action + perform() { + this.model.setProperties({ + rejectReason: this.rejectReason, + sendEmail: this.sendEmail, + }); + this.send("closeModal"); + this.performConfirmed(this.action); + }, +}); diff --git a/app/assets/javascripts/discourse/app/templates/components/reviewable-user.hbs b/app/assets/javascripts/discourse/app/templates/components/reviewable-user.hbs index f7476919c23..9e246ed2c68 100644 --- a/app/assets/javascripts/discourse/app/templates/components/reviewable-user.hbs +++ b/app/assets/javascripts/discourse/app/templates/components/reviewable-user.hbs @@ -34,6 +34,10 @@ {{/if}} + {{reviewable-field classes="reviewable-user-details reject-reason" + name=(i18n "review.user.reject_reason") + value=reviewable.reject_reason}} + {{#each userFields as |f|}} {{reviewable-field classes="reviewable-user-details user-field" name=f.name diff --git a/app/assets/javascripts/discourse/app/templates/modal/reject-reason-reviewable.hbs b/app/assets/javascripts/discourse/app/templates/modal/reject-reason-reviewable.hbs new file mode 100644 index 00000000000..c5f5c6334f5 --- /dev/null +++ b/app/assets/javascripts/discourse/app/templates/modal/reject-reason-reviewable.hbs @@ -0,0 +1,19 @@ +{{#d-modal-body class="explain-reviewable"}} + {{textarea value=rejectReason}} +
+ +
+{{/d-modal-body}} + +
+ {{d-button action=(route-action "closeModal") label="close"}} +
+ {{d-button icon="trash-alt" class="btn-danger" action=(action "perform")}} +
+
diff --git a/app/assets/javascripts/discourse/tests/acceptance/review-test.js b/app/assets/javascripts/discourse/tests/acceptance/review-test.js index a8dfcd18812..8a205de0b53 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/review-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/review-test.js @@ -1,5 +1,6 @@ import { acceptance, queryAll } from "discourse/tests/helpers/qunit-helpers"; import { click, fillIn, visit } from "@ember/test-helpers"; +import I18n from "I18n"; import selectKit from "discourse/tests/helpers/select-kit-helper"; import { test } from "qunit"; @@ -35,6 +36,33 @@ acceptance("Review", function (needs) { ); }); + test("Reject user", async function (assert) { + await visit("/review"); + await click( + `${user} .reviewable-actions button[data-name="Delete User..."]` + ); + await click(`${user} li[data-value="reject_user_delete"]`); + assert.ok( + queryAll(".reject-reason-reviewable-modal:visible .title") + .html() + .includes(I18n.t("review.reject_reason.title")), + "it opens reject reason modal when user is rejected" + ); + + await click(".modal-footer button[aria-label='Close']"); + + await click( + `${user} .reviewable-actions button[data-name="Delete User..."]` + ); + await click(`${user} li[data-value="reject_user_block"]`); + assert.ok( + queryAll(".reject-reason-reviewable-modal:visible .title") + .html() + .includes(I18n.t("review.reject_reason.title")), + "it opens reject reason modal when user is rejected and blocked" + ); + }); + test("Settings", async function (assert) { await visit("/review/settings"); diff --git a/app/assets/javascripts/discourse/tests/helpers/review-pretender.js b/app/assets/javascripts/discourse/tests/helpers/review-pretender.js index dd7f9259929..0af2e904a45 100644 --- a/app/assets/javascripts/discourse/tests/helpers/review-pretender.js +++ b/app/assets/javascripts/discourse/tests/helpers/review-pretender.js @@ -24,7 +24,7 @@ export default function (helpers) { created_at: "2019-01-14T19:49:53.571Z", username: "newbie", email: "newbie@example.com", - bundled_action_ids: ["approve", "reject"], + bundled_action_ids: ["approve", "reject", "reject_user"], }, { id: 4321, @@ -58,6 +58,12 @@ export default function (helpers) { id: "reject", action_ids: ["reject"], }, + { + id: "reject_user", + icon: "user-times", + label: "Delete User...", + action_ids: ["reject_user_delete", "reject_user_block"], + }, ], actions: [ { @@ -70,6 +76,23 @@ export default function (helpers) { label: "Reject", icon: "far-thumbs-down", }, + { + id: "reject_user_delete", + icon: "user-times", + button_class: null, + label: "Delete User", + description: "The user will be deleted from the forum.", + require_reject_reason: true, + }, + { + id: "reject_user_block", + icon: "ban", + button_class: null, + label: "Delete and Block User", + description: + "The user will be deleted, and we'll block their IP and email address.", + require_reject_reason: true, + }, ], reviewable_scores: [{ id: 1 }, { id: 2 }], users: [{ id: 1, username: "eviltrout" }], diff --git a/app/controllers/reviewables_controller.rb b/app/controllers/reviewables_controller.rb index 874a678c32a..35474d81525 100644 --- a/app/controllers/reviewables_controller.rb +++ b/app/controllers/reviewables_controller.rb @@ -189,6 +189,8 @@ class ReviewablesController < ApplicationController return render_json_error(error) end + args.merge!(reject_reason: params[:reject_reason], send_email: params[:send_email] == "true") if reviewable.type == 'ReviewableUser' + result = reviewable.perform(current_user, params[:action_id].to_sym, args) rescue Reviewable::InvalidAction => e # Consider InvalidAction an InvalidAccess diff --git a/app/jobs/regular/user_email.rb b/app/jobs/regular/user_email.rb index 4c1e3c3ccb3..5cb7dde6822 100644 --- a/app/jobs/regular/user_email.rb +++ b/app/jobs/regular/user_email.rb @@ -193,6 +193,8 @@ module Jobs email_args[:user_history] = UserHistory.where(id: args[:user_history_id]).first end + email_args[:reject_reason] = args[:reject_reason] + message = EmailLog.unique_email_per_post(post, user) do UserNotifications.public_send(type, user, email_args) end diff --git a/app/mailers/user_notifications.rb b/app/mailers/user_notifications.rb index 93ee5e3d880..bd7e6065ab4 100644 --- a/app/mailers/user_notifications.rb +++ b/app/mailers/user_notifications.rb @@ -37,6 +37,14 @@ class UserNotifications < ActionMailer::Base new_user_tips: tips) end + def signup_after_reject(user, opts = {}) + locale = user_locale(user) + build_email(user.email, + template: 'user_notifications.signup_after_reject', + locale: locale, + reject_reason: opts[:reject_reason]) + end + def suspicious_login(user, opts = {}) ipinfo = DiscourseIpInfo.get(opts[:client_ip]) location = ipinfo[:location] diff --git a/app/models/reviewable.rb b/app/models/reviewable.rb index 6adcb6ae422..7f3535816cc 100644 --- a/app/models/reviewable.rb +++ b/app/models/reviewable.rb @@ -704,6 +704,8 @@ end # latest_score :datetime # created_at :datetime not null # updated_at :datetime not null +# force_review :boolean default(FALSE), not null +# reject_reason :text # # Indexes # diff --git a/app/models/reviewable_user.rb b/app/models/reviewable_user.rb index d5bbbb077ad..4ede3c2ebee 100644 --- a/app/models/reviewable_user.rb +++ b/app/models/reviewable_user.rb @@ -27,11 +27,13 @@ class ReviewableUser < Reviewable actions.add(:reject_user_delete, bundle: reject) do |a| a.icon = 'user-times' a.label = "reviewables.actions.reject_user.delete.title" + a.require_reject_reason = true a.description = "reviewables.actions.reject_user.delete.description" end actions.add(:reject_user_block, bundle: reject) do |a| a.icon = 'ban' a.label = "reviewables.actions.reject_user.block.title" + a.require_reject_reason = true a.description = "reviewables.actions.reject_user.block.description" end end @@ -64,6 +66,17 @@ class ReviewableUser < Reviewable end begin + self.reject_reason = args[:reject_reason] + + if args[:send_email] != false && SiteSetting.must_approve_users? + # Execute job instead of enqueue because user has to exists to send email + Jobs::CriticalUserEmail.new.execute({ + type: :signup_after_reject, + user_id: target.id, + reject_reason: self.reject_reason + }) + end + delete_args = {} delete_args[:block_ip] = true if args[:block_ip] delete_args[:block_email] = true if args[:block_email] diff --git a/app/serializers/reviewable_action_serializer.rb b/app/serializers/reviewable_action_serializer.rb index 50c5310fcd7..cb062b1fb9f 100644 --- a/app/serializers/reviewable_action_serializer.rb +++ b/app/serializers/reviewable_action_serializer.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class ReviewableActionSerializer < ApplicationSerializer - attributes :id, :icon, :button_class, :label, :confirm_message, :description, :client_action + attributes :id, :icon, :button_class, :label, :confirm_message, :description, :client_action, :require_reject_reason def label I18n.t(object.label) @@ -27,4 +27,7 @@ class ReviewableActionSerializer < ApplicationSerializer object.client_action.present? end + def include_require_reject_reason? + object.require_reject_reason.present? + end end diff --git a/app/serializers/reviewable_user_serializer.rb b/app/serializers/reviewable_user_serializer.rb index abaf56b4c93..5841899cfd3 100644 --- a/app/serializers/reviewable_user_serializer.rb +++ b/app/serializers/reviewable_user_serializer.rb @@ -2,7 +2,7 @@ class ReviewableUserSerializer < ReviewableSerializer - attributes :link_admin, :user_fields + attributes :link_admin, :user_fields, :reject_reason payload_attributes( :username, diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 92cda30cef3..5468e5520e5 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -468,6 +468,7 @@ en: email: "Email" name: "Name" fields: "Fields" + reject_reason: "Reason" user_percentage: summary: @@ -566,6 +567,9 @@ en: other: "You have %{count} posts pending." ok: "OK" example_username: "username" + reject_reason: + title: "Why are you rejecting this user?" + send_email: "Send rejection email" user_action: user_posted_topic: "%{user} posted the topic" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index ec32420b17d..7dd6811e407 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -3842,6 +3842,14 @@ en: Enjoy your stay! + signup_after_reject: + title: "Signup After Reject" + subject_template: "You've been rejected on %{site_name}" + text_body_template: | + A staff member rejected your account on %{site_name}. + + %{reject_reason} + signup: title: "Signup" subject_template: "[%{email_prefix}] Confirm your new account" diff --git a/db/migrate/20210111025920_add_reject_reason_to_reviewables.rb b/db/migrate/20210111025920_add_reject_reason_to_reviewables.rb new file mode 100644 index 00000000000..43c2fdabb23 --- /dev/null +++ b/db/migrate/20210111025920_add_reject_reason_to_reviewables.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddRejectReasonToReviewables < ActiveRecord::Migration[6.0] + def change + add_column :reviewables, :reject_reason, :text + end +end diff --git a/lib/reviewable/actions.rb b/lib/reviewable/actions.rb index f433a8c8a89..bf265969e9a 100644 --- a/lib/reviewable/actions.rb +++ b/lib/reviewable/actions.rb @@ -33,7 +33,7 @@ class Reviewable < ActiveRecord::Base end class Action < Item - attr_accessor :icon, :button_class, :label, :description, :confirm_message, :client_action + attr_accessor :icon, :button_class, :label, :description, :confirm_message, :client_action, :require_reject_reason def initialize(id, icon = nil, button_class = nil, label = nil) super(id) diff --git a/spec/models/reviewable_user_spec.rb b/spec/models/reviewable_user_spec.rb index ba00b8641db..6ec3b4d88cf 100644 --- a/spec/models/reviewable_user_spec.rb +++ b/spec/models/reviewable_user_spec.rb @@ -67,7 +67,7 @@ RSpec.describe ReviewableUser, type: :model do end it "allows us to reject a user" do - result = reviewable.perform(moderator, :reject_user_delete) + result = reviewable.perform(moderator, :reject_user_delete, reject_reason: "reject reason") expect(result.success?).to eq(true) expect(reviewable.pending?).to eq(false) @@ -76,13 +76,14 @@ RSpec.describe ReviewableUser, type: :model do # Rejecting deletes the user record reviewable.reload expect(reviewable.target).to be_blank + expect(reviewable.reject_reason).to eq("reject reason") end it "allows us to reject and block a user" do email = reviewable.target.email ip = reviewable.target.ip_address - result = reviewable.perform(moderator, :reject_user_block) + result = reviewable.perform(moderator, :reject_user_block, reject_reason: "reject reason") expect(result.success?).to eq(true) expect(reviewable.pending?).to eq(false) @@ -91,11 +92,24 @@ RSpec.describe ReviewableUser, type: :model do # Rejecting deletes the user record reviewable.reload expect(reviewable.target).to be_blank + expect(reviewable.reject_reason).to eq("reject reason") expect(ScreenedEmail.should_block?(email)).to eq(true) expect(ScreenedIpAddress.should_block?(ip)).to eq(true) end + it "is not sending email to the user about rejection" do + SiteSetting.must_approve_users = true + Jobs::CriticalUserEmail.any_instance.expects(:execute).never + reviewable.perform(moderator, :reject_user_block, reject_reason: "reject reason", send_email: false) + end + + it "optionaly sends email with reject reason" do + SiteSetting.must_approve_users = true + Jobs::CriticalUserEmail.any_instance.expects(:execute).with(type: :signup_after_reject, user_id: reviewable.target_id, reject_reason: "reject reason").once + reviewable.perform(moderator, :reject_user_block, reject_reason: "reject reason", send_email: true) + end + it "allows us to reject a user who has posts" do Fabricate(:post, user: reviewable.target) result = reviewable.perform(moderator, :reject_user_delete)