From 3f9e7eb3267655323a8c50e4059388be0a9d71e6 Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
Date: Thu, 21 Mar 2019 20:46:14 +0000
Subject: [PATCH] FIX: Respect the disable_emails=non-staff site setting
 correctly

This reverts commit c279792130e24dffbbdee8d6cbb0bad46cc13b72.

This commit inadvertently removed all of the non-staff email logic, rather than just for the 'test email' button.

https://meta.discourse.org/t/112231/5
---
 app/controllers/admin/email_controller.rb    | 2 ++
 config/locales/server.en.yml                 | 1 +
 lib/email/sender.rb                          | 4 ++++
 spec/components/email/sender_spec.rb         | 6 +++---
 spec/requests/admin/email_controller_spec.rb | 4 ++--
 5 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/app/controllers/admin/email_controller.rb b/app/controllers/admin/email_controller.rb
index 148b364dc00..ac40a6e50a7 100644
--- a/app/controllers/admin/email_controller.rb
+++ b/app/controllers/admin/email_controller.rb
@@ -13,6 +13,8 @@ class Admin::EmailController < Admin::AdminController
       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
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index f0270644f2b..3c149f60429 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -2175,6 +2175,7 @@ en:
     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 b8c62eb6ed9..fd0f6a3b470 100644
--- a/lib/email/sender.rb
+++ b/lib/email/sender.rb
@@ -35,6 +35,10 @@ 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"
+        return unless User.find_by_email(to_address)&.staff?
+      end
+
       return skip(SkippedEmailLog.reason_types[:sender_message_to_invalid]) if to_address.end_with?(".invalid")
 
       if @message.text_part
diff --git a/spec/components/email/sender_spec.rb b/spec/components/email/sender_spec.rb
index 5cd476aa0d2..5a9d24a7eca 100644
--- a/spec/components/email/sender_spec.rb
+++ b/spec/components/email/sender_spec.rb
@@ -27,10 +27,10 @@ describe Email::Sender do
     context "disable_emails is enabled for non-staff users" do
       before { SiteSetting.disable_emails = "non-staff" }
 
-      it "delivers mail to normal user" do
-        Mail::Message.any_instance.expects(:deliver_now).once
+      it "doesn't deliver mail to normal user" do
+        Mail::Message.any_instance.expects(:deliver_now).never
         message = Mail::Message.new(to: user.email, body: "hello")
-        Email::Sender.new(message, :hello).send
+        expect(Email::Sender.new(message, :hello).send).to eq(nil)
       end
 
       it "delivers mail to staff user" do
diff --git a/spec/requests/admin/email_controller_spec.rb b/spec/requests/admin/email_controller_spec.rb
index 3ff6bedca4d..55fc5d8b860 100644
--- a/spec/requests/admin/email_controller_spec.rb
+++ b/spec/requests/admin/email_controller_spec.rb
@@ -146,7 +146,7 @@ describe Admin::EmailController do
         expect(incoming['sent_test_email_message']).to eq(I18n.t("admin.email.sent_test_disabled"))
       end
 
-      it 'sends mail to everyone when setting is "non-staff"' do
+      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 }
@@ -155,7 +155,7 @@ describe Admin::EmailController do
 
         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"))
+        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