FIX: Group SMTP email improvements (#11633)

Fixes a rare race condition causing the `Imap::Sync` class to create an incoming email and associated post/topic, which then kicks off the PostAlerter to notify others in the PM about a reply in the topic, but for the OP which is not necessary (because the person emailing the IMAP inbox already knows about the OP). Basically, we should never be sending the group SMTP email for the first post in a topic.

Also in this PR:

* Custom attribute accessors for the to/from/cc addresses on `IncomingEmail`, to parse them from an array to a joined string so the logic for this is only in one place.
* Store extra detail against the `IncomingEmail` created in `GroupSmtpMailer`
* regex test Mail header Reply-To as string instead of Field, which fixes `warning: deprecated Object#=~ is called on Mail::Field; it always returns nil`
* Add DEBUG_IMAP to log all IMAP logs as warnings for easier debugging
* Changed the Rails logging to `ImapSyncLog` in the `GroupSmtpMailer`

app/jobs/regular/group_smtp_email.rb

@@ -10,9 +10,23 @@ module Jobs
       group = Group.find_by(id: args[:group_id])
       post = Post.find_by(id: args[:post_id])
       email = args[:email]
-      recipient_user = ::UserEmail.find_by(email: email, primary: true)&.user
 
-      Rails.logger.debug("[IMAP] Sending email for group #{group.name} and post #{post.id}")
+      # There is a rare race condition causing the Imap::Sync class to create
+      # an incoming email and associated post/topic, which then kicks off
+      # the PostAlerter to notify others in the PM about a reply in the topic,
+      # but for the OP which is not necessary (because the person emailing the
+      # IMAP inbox already knows about the OP)
+      #
+      # Basically, we should never be sending this notification for the first
+      # post in a topic.
+      if post.is_first_post?
+        ImapSyncLog.warn("Aborting SMTP email for post #{post.id} in topic #{post.topic_id} to #{email}, the post is the OP and should not send an email.", group)
+        return
+      end
+
+      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)
       Email::Sender.new(message, :group_smtp, recipient_user).send
 
@@ -23,7 +37,11 @@ module Jobs
         topic_id: post.topic_id,
         post_id: post.id,
         raw: message.to_s,
-        message_id: message.message_id
+        subject: message.subject,
+        message_id: message.message_id,
+        to_addresses: message.to,
+        cc_addresses: message.cc,
+        from_address: message.from
       )
     end
   end

app/models/imap_sync_log.rb

@@ -12,7 +12,11 @@ class ImapSyncLog < ActiveRecord::Base
   def self.log(message, level, group_id = nil)
     now = Time.now.strftime("%Y-%m-%d %H:%M:%S.%L")
     new_log = create(message: message, level: ImapSyncLog.levels[level], group_id: group_id)
-    Rails.logger.send(level, "#{level[0].upcase}, [#{now}] [IMAP] (group_id #{group_id}) #{message}")
+    if ENV["DEBUG_IMAP"]
+      Rails.logger.send(:warn, "#{level[0].upcase}, [#{now}] [IMAP] (group_id #{group_id}) #{message}")
+    else
+      Rails.logger.send(level, "#{level[0].upcase}, [#{now}] [IMAP] (group_id #{group_id}) #{message}")
+    end
     new_log
   end
 

app/models/incoming_email.rb

@@ -28,6 +28,27 @@ class IncomingEmail < ActiveRecord::Base
       )
     SQL
   end
+
+  def to_addresses=(to)
+    if to&.is_a?(Array)
+      to = to.map(&:downcase).join(";")
+    end
+    super(to)
+  end
+
+  def cc_addresses=(cc)
+    if cc&.is_a?(Array)
+      cc = cc.map(&:downcase).join(";")
+    end
+    super(cc)
+  end
+
+  def from_address=(from)
+    if from&.is_a?(Array)
+      from = from.first
+    end
+    super(from)
+  end
 end
 
 # == Schema Information

lib/email/receiver.rb

@@ -147,8 +147,8 @@ module Email
         raw: Email::Cleaner.new(@raw_email).execute,
         subject: subject,
         from_address: @from_email,
-        to_addresses: @mail.to&.map(&:downcase)&.join(";"),
+        to_addresses: @mail.to,
-        cc_addresses: @mail.cc&.map(&:downcase)&.join(";"),
+        cc_addresses: @mail.cc,
         imap_uid_validity: @opts[:imap_uid_validity],
         imap_uid: @opts[:imap_uid],
         imap_group_id: @opts[:imap_group_id],

lib/email/sender.rb

@@ -173,7 +173,7 @@ module Email
         end
       end
 
-      if reply_key.present? && @message.header['Reply-To'] =~ /\<([^\>]+)\>/
+      if reply_key.present? && @message.header['Reply-To'].to_s =~ /\<([^\>]+)\>/
         email = Regexp.last_match[1]
         @message.header['List-Post'] = "<mailto:#{email}>"
       end

spec/jobs/regular/group_smtp_email_spec.rb

@@ -3,13 +3,18 @@
 require 'rails_helper'
 
 RSpec.describe Jobs::GroupSmtpEmail do
-  let(:post) { Fabricate(:post) }
+  fab!(:post) do
-  let(:group) { Fabricate(:imap_group) }
+    topic = Fabricate(:topic)
-  let!(:recipient_user) { Fabricate(:user, email: "test@test.com") }
+    Fabricate(:post, topic: topic)
+    Fabricate(:post, topic: topic)
+  end
+  fab!(:group) { Fabricate(:imap_group) }
+  fab!(:recipient_user) { Fabricate(:user, email: "test@test.com") }
+  let(:post_id) { post.id }
   let(:args) do
     {
       group_id: group.id,
-      post_id: post.id,
+      post_id: post_id,
       email: "test@test.com"
     }
   end
@@ -32,7 +37,7 @@ RSpec.describe Jobs::GroupSmtpEmail do
     subject.execute(args)
     incoming = IncomingEmail.find_by(post_id: post.id, user_id: post.user_id, topic_id: post.topic_id)
     expect(incoming).not_to eq(nil)
-    expect(incoming.message_id).to eq("topic/#{post.topic_id}@test.localhost")
+    expect(incoming.message_id).to eq("topic/#{post.topic_id}/#{post.id}@test.localhost")
   end
 
   it "creates a PostReplyKey and correctly uses it for the email reply_key substitution" do
@@ -42,4 +47,19 @@ RSpec.describe Jobs::GroupSmtpEmail do
     expect(post_reply_key).not_to eq(nil)
     expect(incoming.raw).to include("Reply-To: Discourse <test+#{post_reply_key.reply_key}@incoming.com>")
   end
+
+  it "has the from_address and the to_addresses and subject filled in correctly" do
+    subject.execute(args)
+    incoming = IncomingEmail.find_by(post_id: post.id, user_id: post.user_id, topic_id: post.topic_id)
+    expect(incoming.to_addresses).to eq("test@test.com")
+    expect(incoming.subject).to include("Re: This is a test topic")
+    expect(incoming.from_address).to eq("discourseteam@ponyexpress.com")
+  end
+
+  context "when the post in the argument is the OP" 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 { IncomingEmail.count })
+    end
+  end
 end