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

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

diff --git a/app/jobs/regular/group_smtp_email.rb b/app/jobs/regular/group_smtp_email.rb
index 8092dc3..66eadb9 100644
--- a/app/jobs/regular/group_smtp_email.rb
+++ b/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
diff --git a/app/models/skipped_email_log.rb b/app/models/skipped_email_log.rb
index 1a4ec41..2af1b2d 100644
--- a/app/models/skipped_email_log.rb
+++ b/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
     )
diff --git a/app/models/user.rb b/app/models/user.rb
index 6dcd9dd..f9a2379 100644
--- a/app/models/user.rb
+++ b/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,8 +375,12 @@ class User < ActiveRecord::Base
     end
   end
 
-  def self.find_by_email(email)
-    self.with_email(Email.downcase(email)).first
+  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)
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index 61a2eef..f8d75b4 100644
--- a/config/locales/server.en.yml
+++ b/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"
diff --git a/spec/jobs/regular/group_smtp_email_spec.rb b/spec/jobs/regular/group_smtp_email_spec.rb
index 8e24ac7..9b792a8 100644
--- a/spec/jobs/regular/group_smtp_email_spec.rb
+++ b/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")

[... diff too long, it was truncated ...]

GitHub sha: 0f688f45bddaac17b2840ab9eed6b2dec2a7f9cf

This commit appears in #13631 which was approved by tgxworld. It was merged by martin.