diff --git a/app/controllers/admin/email_controller.rb b/app/controllers/admin/email_controller.rb index ac40a6e50a7..bfb4e75e188 100644 --- a/app/controllers/admin/email_controller.rb +++ b/app/controllers/admin/email_controller.rb @@ -10,14 +10,10 @@ class Admin::EmailController < Admin::AdminController def test params.require(:email_address) begin - Jobs::TestEmail.new.execute(to_address: params[:email_address]) - if SiteSetting.disable_emails == "yes" - render json: { sent_test_email_message: I18n.t("admin.email.sent_test_disabled") } - elsif SiteSetting.disable_emails == "non-staff" && !User.find_by_email(params[:email_address])&.staff? - render json: { sent_test_email_message: I18n.t("admin.email.sent_test_disabled_for_non_staff") } - else - render json: { sent_test_email_message: I18n.t("admin.email.sent_test") } - end + message = TestMailer.send_test(params[:email_address]) + Email::Sender.new(message, :test_message).send + + render json: { sent_test_email_message: I18n.t("admin.email.sent_test") } rescue => e render json: { errors: [e.message] }, status: 422 end diff --git a/app/jobs/regular/test_email.rb b/app/jobs/regular/test_email.rb deleted file mode 100644 index b0397260536..00000000000 --- a/app/jobs/regular/test_email.rb +++ /dev/null @@ -1,20 +0,0 @@ -require_dependency 'email/sender' - -module Jobs - - # Asynchronously send an email - class TestEmail < Jobs::Base - - sidekiq_options queue: 'critical' - - def execute(args) - - raise Discourse::InvalidParameters.new(:to_address) unless args[:to_address].present? - - message = TestMailer.send_test(args[:to_address]) - Email::Sender.new(message, :test_message).send - end - - end - -end diff --git a/app/jobs/regular/user_email.rb b/app/jobs/regular/user_email.rb index 7a3d089f8cb..5821123359a 100644 --- a/app/jobs/regular/user_email.rb +++ b/app/jobs/regular/user_email.rb @@ -54,9 +54,7 @@ module Jobs ) if message - Email::Sender.new(message, type, user).send( - is_critical: self.class == Jobs::CriticalUserEmail - ) + Email::Sender.new(message, type, user).send if (b = user.user_stat.bounce_score) > SiteSetting.bounce_score_erode_on_send # erode bounce score each time we send an email diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 3c149f60429..b363aba3e54 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -2174,8 +2174,6 @@ en: admin: email: sent_test: "sent!" - sent_test_disabled: "cannot send because emails are disabled" - sent_test_disabled_for_non_staff: "cannot send because emails are disabled for non-staff" user: deactivated: "Was deactivated due to too many bounced emails to '%{email}'." diff --git a/lib/email/sender.rb b/lib/email/sender.rb index fd0f6a3b470..7f23bddeeaf 100644 --- a/lib/email/sender.rb +++ b/lib/email/sender.rb @@ -11,6 +11,7 @@ require 'uri' require 'net/smtp' SMTP_CLIENT_ERRORS = [Net::SMTPFatalError, Net::SMTPSyntaxError] +BYPASS_DISABLE_TYPES = ["admin_login", "test_message"] module Email class Sender @@ -21,11 +22,10 @@ module Email @user = user end - def send(is_critical: false) - if SiteSetting.disable_emails == "yes" && - @email_type.to_s != "admin_login" && - !is_critical + def send + bypass_disable = BYPASS_DISABLE_TYPES.include?(@email_type.to_s) + if SiteSetting.disable_emails == "yes" && !bypass_disable return end @@ -35,7 +35,7 @@ module Email return skip(SkippedEmailLog.reason_types[:sender_message_blank]) if @message.blank? return skip(SkippedEmailLog.reason_types[:sender_message_to_blank]) if @message.to.blank? - if SiteSetting.disable_emails == "non-staff" + if SiteSetting.disable_emails == "non-staff" && !bypass_disable return unless User.find_by_email(to_address)&.staff? end diff --git a/spec/components/email/sender_spec.rb b/spec/components/email/sender_spec.rb index 5a9d24a7eca..e33010fc961 100644 --- a/spec/components/email/sender_spec.rb +++ b/spec/components/email/sender_spec.rb @@ -12,15 +12,24 @@ describe Email::Sender do before { SiteSetting.disable_emails = "yes" } it "doesn't deliver mail when mails are disabled" do - Mail::Message.any_instance.expects(:deliver_now).never - message = Mail::Message.new(to: moderator.email , body: "hello") - expect(Email::Sender.new(message, :hello).send).to eq(nil) + message = UserNotifications.email_login(moderator) + Email::Sender.new(message, :email_login).send + + expect(ActionMailer::Base.deliveries).to eq([]) end it "delivers mail when mails are disabled but the email_type is admin_login" do - Mail::Message.any_instance.expects(:deliver_now).once - message = Mail::Message.new(to: moderator.email , body: "hello") + message = UserNotifications.admin_login(moderator) Email::Sender.new(message, :admin_login).send + + expect(ActionMailer::Base.deliveries.first.to).to eq([moderator.email]) + end + + it "delivers mail when mails are disabled but the email_type is test_message" do + message = TestMailer.send_test(moderator.email) + Email::Sender.new(message, :test_message).send + + expect(ActionMailer::Base.deliveries.first.to).to eq([moderator.email]) end end diff --git a/spec/jobs/test_email_spec.rb b/spec/jobs/test_email_spec.rb deleted file mode 100644 index 7207abfff59..00000000000 --- a/spec/jobs/test_email_spec.rb +++ /dev/null @@ -1,25 +0,0 @@ -require 'rails_helper' -require_dependency 'jobs/base' - -describe Jobs::TestEmail do - - context '.execute' do - it 'raises an error when the address is missing' do - expect { Jobs::TestEmail.new.execute({}) }.to raise_error(Discourse::InvalidParameters) - end - - context 'with an address' do - - let (:mailer) { Mail::Message.new(to: 'eviltrout@test.domain') } - - it 'delegates to the test mailer' do - Email::Sender.any_instance.expects(:send) - TestMailer.expects(:send_test).with('eviltrout@test.domain').returns(mailer) - Jobs::TestEmail.new.execute(to_address: 'eviltrout@test.domain') - end - - end - - end - -end diff --git a/spec/jobs/user_email_spec.rb b/spec/jobs/user_email_spec.rb index f45d723e9fd..6218719ff93 100644 --- a/spec/jobs/user_email_spec.rb +++ b/spec/jobs/user_email_spec.rb @@ -80,15 +80,6 @@ describe Jobs::UserEmail do expect(ActionMailer::Base.deliveries).to eq([]) end - - it "sends when critical" do - SiteSetting.disable_emails = 'yes' - Jobs::CriticalUserEmail.new.execute(type: :confirm_new_email, user_id: user.id) - - expect(ActionMailer::Base.deliveries.first.to).to contain_exactly( - user.email - ) - end end context "recently seen" do diff --git a/spec/requests/admin/email_controller_spec.rb b/spec/requests/admin/email_controller_spec.rb index 55fc5d8b860..99c7fd8a1b1 100644 --- a/spec/requests/admin/email_controller_spec.rb +++ b/spec/requests/admin/email_controller_spec.rb @@ -137,32 +137,40 @@ describe Admin::EmailController do let(:eviltrout) { Fabricate(:evil_trout) } let(:admin) { Fabricate(:admin) } - it 'does not sends mail to anyone when setting is "yes"' do + it 'bypasses disable when setting is "yes"' do SiteSetting.disable_emails = 'yes' - post "/admin/email/test.json", params: { email_address: admin.email } - incoming = JSON.parse(response.body) - expect(incoming['sent_test_email_message']).to eq(I18n.t("admin.email.sent_test_disabled")) - end + expect(ActionMailer::Base.deliveries.first.to).to contain_exactly( + admin.email + ) - it 'sends mail to staff only when setting is "non-staff"' do - SiteSetting.disable_emails = 'non-staff' - - post "/admin/email/test.json", params: { email_address: admin.email } incoming = JSON.parse(response.body) expect(incoming['sent_test_email_message']).to eq(I18n.t("admin.email.sent_test")) - - post "/admin/email/test.json", params: { email_address: eviltrout.email } - incoming = JSON.parse(response.body) - expect(incoming['sent_test_email_message']).to eq(I18n.t("admin.email.sent_test_disabled_for_non_staff")) end - it 'sends mail to everyone when setting is "no"' do + it 'bypasses disable when setting is "non-staff"' do + SiteSetting.disable_emails = 'non-staff' + + post "/admin/email/test.json", params: { email_address: eviltrout.email } + + expect(ActionMailer::Base.deliveries.first.to).to contain_exactly( + eviltrout.email + ) + + incoming = JSON.parse(response.body) + expect(incoming['sent_test_email_message']).to eq(I18n.t("admin.email.sent_test")) + end + + it 'works when setting is "no"' do SiteSetting.disable_emails = 'no' post "/admin/email/test.json", params: { email_address: eviltrout.email } + expect(ActionMailer::Base.deliveries.first.to).to contain_exactly( + eviltrout.email + ) + incoming = JSON.parse(response.body) expect(incoming['sent_test_email_message']).to eq(I18n.t("admin.email.sent_test")) end