From ebdbb199a512128803bac973e1c41d664670c3af Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Wed, 19 Jun 2024 11:07:23 +1000 Subject: [PATCH] FIX: Rejection email sent even if reject reason too long (#27529) Followup 6b872c4c5382e5e58c14d55bc92b8da5ba158ce1 Even though we were showing a validation error for a reject reason that was too long, we were still sending an email and doing other operations on the user which we are rejecting. This commit fixes this by validating the reviewable model before attempting to do anything else after the reason is set. --- app/models/reviewable_user.rb | 3 +++ spec/requests/reviewables_controller_spec.rb | 15 +++++++++------ 2 files changed, 12 insertions(+), 6 deletions(-) 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)],