FIX: Handle edge cases for group SMTP email job (#13631)

Skip group SMTP email (and add log) if:

* topic is deleted
* post is deleted
* smtp has been disabled for the group

Skip without log if:

* enable_smtp site setting is false
* disable_emails site setting is yes

Co-authored-by: Alan Guo Xiang Tan <gxtan1990@gmail.com>

app/jobs/regular/group_smtp_email.rb

@@ -4,13 +4,32 @@ require_dependency 'email/sender'
 
 module Jobs
   class GroupSmtpEmail < ::Jobs::Base
+    include Skippable
+
     sidekiq_options queue: 'critical'
 
     def execute(args)
+      return if quit_email_early?
+
       group = Group.find_by(id: args[:group_id])
+      return if group.blank?
+
       post = Post.find_by(id: args[:post_id])
       email = args[:email]
       cc_addresses = args[:cc_emails]
+      recipient_user = User.find_by_email(email, primary: true)
+
+      if post.blank?
+        return skip(email, nil, recipient_user, :group_smtp_post_deleted)
+      end
+
+      if !Topic.exists?(id: post.topic_id)
+        return skip(email, post, recipient_user, :group_smtp_topic_deleted)
+      end
+
+      if !group.smtp_enabled
+        return skip(email, post, recipient_user, :group_smtp_disabled_for_group)
+      end
 
       # There is a rare race condition causing the Imap::Sync class to create
       # an incoming email and associated post/topic, which then kicks off
@@ -27,12 +46,10 @@ module Jobs
 
       ImapSyncLog.debug("Sending SMTP email for post #{post.id} in topic #{post.topic_id} to #{email}.", group)
 
-      recipient_user = ::UserEmail.find_by(email: email, primary: true)&.user
-      message = GroupSmtpMailer.send_mail(group, email, post, cc_addresses)
-
       # The EmailLog record created by the sender will have the raw email
       # stored, the group smtp ID, and any cc addresses recorded for later
       # cross referencing.
+      message = GroupSmtpMailer.send_mail(group, email, post, cc_addresses)
       Email::Sender.new(message, :group_smtp, recipient_user).send
 
       # Create an incoming email record to avoid importing again from IMAP
@@ -52,5 +69,19 @@ module Jobs
         created_via: IncomingEmail.created_via_types[:group_smtp]
       )
     end
+
+    def quit_email_early?
+      SiteSetting.disable_emails == 'yes' || !SiteSetting.enable_smtp
+    end
+
+    def skip(email, post, recipient_user, reason)
+      create_skipped_email_log(
+        email_type: :group_smtp,
+        to_address: email,
+        user_id: recipient_user&.id,
+        post_id: post&.id,
+        reason_type: SkippedEmailLog.reason_types[reason]
+      )
+    end
   end
 end

app/models/skipped_email_log.rb

@@ -39,6 +39,9 @@ class SkippedEmailLog < ActiveRecord::Base
       user_email_access_denied: 22,
       sender_topic_deleted: 23,
       user_email_no_email: 24,
+      group_smtp_post_deleted: 25,
+      group_smtp_topic_deleted: 26,
+      group_smtp_disabled_for_group: 27,
       # you need to add the reason in server.en.yml below the "skipped_email_log" key
       # when you add a new enum value
     )

app/models/user.rb

@@ -193,6 +193,10 @@ class User < ActiveRecord::Base
     joins(:user_emails).where("lower(user_emails.email) IN (?)", email)
   end
 
+  scope :with_primary_email, ->(email) do
+    joins(:user_emails).where("lower(user_emails.email) IN (?) AND user_emails.primary", email)
+  end
+
   scope :human_users, -> { where('users.id > 0') }
 
   # excluding fake users like the system user or anonymous users
@@ -371,9 +375,13 @@ class User < ActiveRecord::Base
     end
   end
 
-  def self.find_by_email(email)
+  def self.find_by_email(email, primary: false)
+    if primary
+      self.with_primary_email(Email.downcase(email)).first
+    else
       self.with_email(Email.downcase(email)).first
     end
+  end
 
   def self.find_by_username(username)
     find_by(username_lower: normalize_username(username))

config/locales/server.en.yml

@@ -4043,6 +4043,9 @@ en:
     sender_post_deleted: "post has been deleted"
     sender_message_to_invalid: "recipient has invalid email address"
     sender_topic_deleted: "topic has been deleted"
+    group_smtp_post_deleted: "post has been deleted"
+    group_smtp_topic_deleted: "topic has been deleted"
+    group_smtp_disabled_for_group: "smtp has been disabled for the group"
 
   color_schemes:
     base_theme_name: "Base"

spec/jobs/regular/group_smtp_email_spec.rb

@@ -85,6 +85,8 @@ RSpec.describe Jobs::GroupSmtpEmail do
 
   it "creates an IncomingEmail record with the correct details to avoid double processing IMAP" do
     subject.execute(args)
+    expect(ActionMailer::Base.deliveries.count).to eq(1)
+    expect(ActionMailer::Base.deliveries.last.subject).to eq("Re: Help I need support")
     incoming_email = IncomingEmail.find_by(post_id: post.id, topic_id: post.topic_id, user_id: post.user.id)
     expect(incoming_email).not_to eq(nil)
     expect(incoming_email.message_id).to eq("topic/#{post.topic_id}/#{post.id}@test.localhost")
@@ -96,6 +98,8 @@ RSpec.describe Jobs::GroupSmtpEmail do
 
   it "does not create a post reply key, it always replies to the group email_username" do
     subject.execute(args)
+    expect(ActionMailer::Base.deliveries.count).to eq(1)
+    expect(ActionMailer::Base.deliveries.last.subject).to eq("Re: Help I need support")
     email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id)
     post_reply_key = PostReplyKey.where(user_id: recipient_user, post_id: post.id).first
     expect(post_reply_key).to eq(nil)
@@ -105,6 +109,8 @@ RSpec.describe Jobs::GroupSmtpEmail do
 
   it "creates an EmailLog record with the correct details" do
     subject.execute(args)
+    expect(ActionMailer::Base.deliveries.count).to eq(1)
+    expect(ActionMailer::Base.deliveries.last.subject).to eq("Re: Help I need support")
     email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id)
     expect(email_log).not_to eq(nil)
     expect(email_log.message_id).to eq("topic/#{post.topic_id}/#{post.id}@test.localhost")
@@ -112,6 +118,8 @@ RSpec.describe Jobs::GroupSmtpEmail do
 
   it "creates an IncomingEmail record with the correct details to avoid double processing IMAP" do
     subject.execute(args)
+    expect(ActionMailer::Base.deliveries.count).to eq(1)
+    expect(ActionMailer::Base.deliveries.last.subject).to eq("Re: Help I need support")
     incoming_email = IncomingEmail.find_by(post_id: post.id, topic_id: post.topic_id, user_id: post.user.id)
     expect(incoming_email).not_to eq(nil)
     expect(incoming_email.message_id).to eq("topic/#{post.topic_id}/#{post.id}@test.localhost")
@@ -123,6 +131,8 @@ RSpec.describe Jobs::GroupSmtpEmail do
 
   it "does not create a post reply key, it always replies to the group email_username" do
     subject.execute(args)
+    expect(ActionMailer::Base.deliveries.count).to eq(1)
+    expect(ActionMailer::Base.deliveries.last.subject).to eq("Re: Help I need support")
     email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id)
     post_reply_key = PostReplyKey.where(user_id: recipient_user, post_id: post.id).first
     expect(post_reply_key).to eq(nil)
@@ -133,12 +143,16 @@ RSpec.describe Jobs::GroupSmtpEmail do
   it "falls back to the group name if full name is blank" do
     group.update(full_name: "")
     subject.execute(args)
+    expect(ActionMailer::Base.deliveries.count).to eq(1)
+    expect(ActionMailer::Base.deliveries.last.subject).to eq("Re: Help I need support")
     email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id)
     expect(email_log.raw_headers).to include("From: support-group via Discourse <#{group.email_username}")
   end
 
   it "has the group_smtp_id and the to_address filled in correctly" do
     subject.execute(args)
+    expect(ActionMailer::Base.deliveries.count).to eq(1)
+    expect(ActionMailer::Base.deliveries.last.subject).to eq("Re: Help I need support")
     email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id)
     expect(email_log.to_address).to eq("test@test.com")
     expect(email_log.smtp_group_id).to eq(group.id)
@@ -147,6 +161,8 @@ RSpec.describe Jobs::GroupSmtpEmail do
   context "when there are cc_addresses" do
     it "has the cc_addresses and cc_user_ids filled in correctly" do
       subject.execute(args)
+      expect(ActionMailer::Base.deliveries.count).to eq(1)
+      expect(ActionMailer::Base.deliveries.last.subject).to eq("Re: Help I need support")
       email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id)
       expect(email_log.cc_addresses).to eq("otherguy@test.com;cormac@lit.com")
       expect(email_log.cc_user_ids).to match_array([staged1.id, staged2.id])
@@ -157,6 +173,76 @@ RSpec.describe Jobs::GroupSmtpEmail do
     let(:post_id) { post.topic.posts.first.id }
     it "aborts and does not send a group SMTP email; the OP is the one that sent the email in the first place" do
       expect { subject.execute(args) }.not_to(change { EmailLog.count })
+      expect(ActionMailer::Base.deliveries.count).to eq(0)
+    end
+  end
+
+  context "when the post is deleted" do
+    it "aborts and adds a skipped email log" do
+      post.trash!
+      subject.execute(args)
+      expect(ActionMailer::Base.deliveries.count).to eq(0)
+      expect(SkippedEmailLog.exists?(
+        email_type: "group_smtp",
+        user: recipient_user,
+        post: nil,
+        to_address: recipient_user.email,
+        reason_type: SkippedEmailLog.reason_types[:group_smtp_post_deleted]
+      )).to eq(true)
+    end
+  end
+
+  context "when the topic is deleted" do
+    it "aborts and adds a skipped email log" do
+      post.topic.trash!
+      subject.execute(args)
+      expect(ActionMailer::Base.deliveries.count).to eq(0)
+      expect(SkippedEmailLog.exists?(
+        email_type: "group_smtp",
+        user: recipient_user,
+        post: post,
+        to_address: recipient_user.email,
+        reason_type: SkippedEmailLog.reason_types[:group_smtp_topic_deleted]
+      )).to eq(true)
+    end
+  end
+
+  context "when smtp is not enabled" do
+    it "returns without sending email" do
+      SiteSetting.enable_smtp = false
+      subject.execute(args)
+      expect(ActionMailer::Base.deliveries.count).to eq(0)
+    end
+  end
+
+  context "when disable_emails is yes" do
+    it "returns without sending email" do
+      SiteSetting.disable_emails = "yes"
+      subject.execute(args)
+      expect(ActionMailer::Base.deliveries.count).to eq(0)
+    end
+  end
+
+  context "group is deleted" do
+    it "returns without sending email" do
+      group.destroy
+      subject.execute(args)
+      expect(ActionMailer::Base.deliveries.count).to eq(0)
+    end
+  end
+
+  context "when smtp is not enabled for the group" do
+    it "returns without sending email" do
+      group.update!(smtp_enabled: false)
+      subject.execute(args)
+      expect(ActionMailer::Base.deliveries.count).to eq(0)
+      expect(SkippedEmailLog.exists?(
+        email_type: "group_smtp",
+        user: recipient_user,
+        post: post,
+        to_address: recipient_user.email,
+        reason_type: SkippedEmailLog.reason_types[:group_smtp_disabled_for_group]
+      )).to eq(true)
     end
   end
 end