FIX: Do not enqueue :group_smtp_email job if IMAP disabled for the group (#13307)

FIX: Do not enqueue :group_smtp_email job if IMAP disabled for the group (#13307)

When a group only has SMTP enabled and not IMAP, we do not want to enqueue the :group_smtp_email job because using the group’s SMTP credentials for sending user_private_message emails is handled by the UserNotifications class.

We do not want the :group_smtp_email job to be enqueued because that uses a reply key instead of the group.email_username for the reply-to address which is not what we want for SMTP only, and also creates an IncomingEmail record to prevent IMAP double syncing which we do not need either.

There is an open question about what happens when IMAP is enabled after SMTP has been enabled for a while, and also questions around whether we could do away with :group_smtp_email altogether and handle everything via EmailLog and UserNotifications, adding additional columns to the former and modifying the Imap::Sync class to take this into account…a lot more further testing for IMAP needs to be done to answer those questions.

For now, this fix should be sufficient to get the correct reply-to address for user_private_response messages sent in response to emails sent directly to the group’s email_username SMTP address.

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

diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb
index 86cf77f..5051070 100644
--- a/app/services/post_alerter.rb
+++ b/app/services/post_alerter.rb
@@ -577,11 +577,6 @@ class PostAlerter
     users
   end
 
-  def group_notifying_via_smtp(post)
-    return nil if !SiteSetting.enable_smtp || post.post_type != Post.types[:regular]
-    post.topic.allowed_groups.where(smtp_enabled: true).first
-  end
-
   def notify_pm_users(post, reply_to_user, notified)
     return unless post.topic
 
@@ -624,6 +619,11 @@ class PostAlerter
     end
   end
 
+  def group_notifying_via_smtp(post)
+    return nil if !SiteSetting.enable_smtp || !SiteSetting.enable_imap || post.post_type != Post.types[:regular]
+    post.topic.allowed_groups.where(smtp_enabled: true, imap_enabled: true).first
+  end
+
   def notify_group_direct_emailers(post)
     email_addresses = []
     emails_to_skip_send = []
diff --git a/spec/mailers/group_smtp_mailer_spec.rb b/spec/mailers/group_smtp_mailer_spec.rb
index 5858418..0eeddda 100644
--- a/spec/mailers/group_smtp_mailer_spec.rb
+++ b/spec/mailers/group_smtp_mailer_spec.rb
@@ -101,17 +101,13 @@ describe GroupSmtpMailer do
         group.update(imap_enabled: false)
       end
 
-      it "uses the reply key based reply to address" do
+      it "does not send the email" do
         post = PostCreator.create(user,
                                   topic_id: receiver.incoming_email.topic.id,
                                   raw: raw
                                  )
 
-        expect(ActionMailer::Base.deliveries.size).to eq(1)
-
-        sent_mail = ActionMailer::Base.deliveries[0]
-        post_reply_key = PostReplyKey.last
-        expect(sent_mail.reply_to).to contain_exactly("test+#{post_reply_key.reply_key}@test.com")
+        expect(ActionMailer::Base.deliveries.size).to eq(0)
       end
     end
   end
diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb
index 609100a..ea7cd26 100644
--- a/spec/services/post_alerter_spec.rb
+++ b/spec/services/post_alerter_spec.rb
@@ -1295,17 +1295,23 @@ describe PostAlerter do
   context "SMTP (group_smtp_email)" do
     before do
       SiteSetting.enable_smtp = true
+      SiteSetting.enable_imap = true
       Jobs.run_immediately!
     end
 
     fab!(:group) do
       Fabricate(
         :group,
-        smtp_server: "imap.gmail.com",
+        smtp_server: "smtp.gmail.com",
         smtp_port: 587,
+        smtp_ssl: true,
+        imap_server: "imap.gmail.com",
+        imap_port: 993,
+        imap_ssl: true,
         email_username: "discourse@example.com",
-        email_password: "discourse@example.com",
-        smtp_enabled: true
+        email_password: "password",
+        smtp_enabled: true,
+        imap_enabled: true
       )
     end
 
@@ -1349,6 +1355,27 @@ describe PostAlerter do
       expect(email.subject).to eq("Re: #{topic.title}")
     end
 
+    it "does not send a group smtp email if imap is not enabled for the group" do
+      group.update!(imap_enabled: false)
+      create_post_with_incoming
+      post = Fabricate(:post, topic: topic)
+      expect { PostAlerter.new.after_save_post(post, true) }.to change { ActionMailer::Base.deliveries.size }.by(0)
+    end
+
+    it "does not send a group smtp email if SiteSetting.enable_imap is false" do
+      SiteSetting.enable_imap = false
+      create_post_with_incoming
+      post = Fabricate(:post, topic: topic)
+      expect { PostAlerter.new.after_save_post(post, true) }.to change { ActionMailer::Base.deliveries.size }.by(0)
+    end
+
+    it "does not send a group smtp email if SiteSetting.enable_smtp is false" do
+      SiteSetting.enable_smtp = false
+      create_post_with_incoming
+      post = Fabricate(:post, topic: topic)
+      expect { PostAlerter.new.after_save_post(post, true) }.to change { ActionMailer::Base.deliveries.size }.by(0)
+    end
+
     it "does not send group smtp emails for a whisper" do
       create_post_with_incoming
       post = Fabricate(:post, topic: topic, post_type: Post.types[:whisper])

GitHub sha: b463a80c

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