diff --git a/app/models/reviewable_user.rb b/app/models/reviewable_user.rb index 85f8903357a..299fd648686 100644 --- a/app/models/reviewable_user.rb +++ b/app/models/reviewable_user.rb @@ -46,6 +46,9 @@ class ReviewableUser < Reviewable begin self.reject_reason = args[:reject_reason] + # Without this, we end up sending the email even if this reject_reason is too long. + self.validate! + if args[:send_email] && SiteSetting.must_approve_users? # Execute job instead of enqueue because user has to exists to send email Jobs::CriticalUserEmail.new.execute( diff --git a/spec/requests/reviewables_controller_spec.rb b/spec/requests/reviewables_controller_spec.rb index 4fcb0280a37..26b7e9f23f7 100644 --- a/spec/requests/reviewables_controller_spec.rb +++ b/spec/requests/reviewables_controller_spec.rb @@ -187,7 +187,7 @@ RSpec.describe ReviewablesController do expect(json["errors"][0]).to eq(I18n.t("reviewables.already_handled_and_user_not_exist")) end - it "returns a readable error message if reject_reason is too long" do + it "returns a readable error message if reject_reason is too long, does not send email, and does not delete the user" do sign_in(admin) Jobs.run_immediately! SiteSetting.must_approve_users = true @@ -195,11 +195,14 @@ RSpec.describe ReviewablesController do user.activate reviewable = ReviewableUser.find_by(target: user) - put "/review/#{reviewable.id}/perform/delete_user.json?version=0", - params: { - send_email: true, - reject_reason: "a" * 3000, - } + expect { + put "/review/#{reviewable.id}/perform/delete_user.json?version=0", + params: { + send_email: true, + reject_reason: "a" * 3000, + } + }.to not_change { ActionMailer::Base.deliveries.size }.and not_change { User.count } + expect(response.code).to eq("422") expect(response.parsed_body["errors"]).to eq( ["Reject reason " + I18n.t("errors.messages.too_long", count: 2000)],