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>

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 = []

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)
+        expect(ActionMailer::Base.deliveries.size).to eq(0)
-
-        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")
       end
     end
   end

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",
+        email_password: "password",
-        smtp_enabled: true
+        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])