FIX: Canonical Message-ID was incorrect for some cases (#15701)

When creating a direct message to a group with group SMTP
set up, and adding another person to that message in the OP,
we send an email to the second person in the OP via the group_smtp
job. This in turn creates an IncomingEmail record to guard against
IMAP double sync.

The issue with this was that this IncomingEmail (which is essentialy
a placeholder/dummy one) was having its Message-ID used as the canonical
References Message-ID for subsequent emails sent out to user_private_message
recipients (such as members of the group), causing threading issues in
the mail client. The canonical <topic/ID@HOST> format should be used
instead for these cases.

This commit fixes the issue by only using the IncomingEmail for the
OP's Message-ID if the OP was created via our handle_mail email receiver
pipeline. It does not make sense to use it in other cases.

lib/email/message_id_service.rb

@@ -39,8 +39,15 @@ module Email
 
       def generate_for_topic(topic, use_incoming_email_if_present: false, canonical: false)
         first_post = topic.ordered_posts.first
+        incoming_email = first_post.incoming_email
 
-        if use_incoming_email_if_present && first_post.incoming_email&.message_id.present?
+        # If the incoming email was created by handle_mail, then it was an
+        # inbound email sent to Discourse and handled by Email::Receiver,
+        # this is the only case where we want to use the original Message-ID
+        # because we want to maintain threading in the original mail client.
+        if use_incoming_email_if_present &&
+            incoming_email&.message_id.present? &&
+            incoming_email&.created_via == IncomingEmail.created_via_types[:handle_mail]
           return "<#{first_post.incoming_email.message_id}>"
         end
 

spec/components/email/sender_spec.rb

@@ -123,8 +123,10 @@ describe Email::Sender do
     let(:reply_key) { "abcd" * 8 }
 
     let(:message) do
-      message = Mail::Message.new to: 'eviltrout@test.domain',
-                                  body: '**hello**'
+      message = Mail::Message.new(
+        to: 'eviltrout@test.domain',
+        body: '**hello**'
+      )
       message.stubs(:deliver_now)
       message
     end
@@ -288,7 +290,13 @@ describe Email::Sender do
       end
 
       it "sets the 'References' header with the incoming email Message-ID if it exists on the first post" do
-        incoming = Fabricate(:incoming_email, topic: topic, post: post_1, message_id: "blah1234@someemailprovider.com")
+        incoming = Fabricate(
+          :incoming_email,
+          topic: topic,
+          post: post_1,
+          message_id: "blah1234@someemailprovider.com",
+          created_via: IncomingEmail.created_via_types[:handle_mail]
+        )
         message.header['X-Discourse-Post-Id'] = post_1.id
 
         email_sender.send
@@ -331,7 +339,9 @@ describe Email::Sender do
       end
 
       it "uses the incoming_email message_id when available, but always uses a random message-id" do
-        topic_incoming_email  = IncomingEmail.create(topic: topic, post: post_1, message_id: "foo@bar")
+        topic_incoming_email  = IncomingEmail.create(
+          topic: topic, post: post_1, message_id: "foo@bar", created_via: IncomingEmail.created_via_types[:handle_mail]
+        )
         post_2_incoming_email = IncomingEmail.create(topic: topic, post: post_2, message_id: "bar@foo")
         post_4_incoming_email = IncomingEmail.create(topic: topic, post: post_4, message_id: "wat@wat")
 

spec/lib/message_id_service_spec.rb

@@ -24,11 +24,24 @@ describe Email::MessageIdService do
 
   describe "#generate_for_topic" do
     it "generates for the topic using the message_id on the first post's incoming_email" do
-      Fabricate(:incoming_email, message_id: "test213428@somemailservice.com", post: post)
+      Fabricate(:incoming_email, message_id: "test213428@somemailservice.com", post: post, created_via: IncomingEmail.created_via_types[:handle_mail])
       post.reload
       expect(subject.generate_for_topic(topic, use_incoming_email_if_present: true)).to eq("<test213428@somemailservice.com>")
     end
 
+    it "does not use the first post's incoming email if it was created via group_smtp, only handle_mail" do
+      incoming = Fabricate(
+        :incoming_email,
+        message_id: "test213428@somemailservice.com",
+        post: post,
+        created_via: IncomingEmail.created_via_types[:group_smtp]
+      )
+      post.reload
+      expect(subject.generate_for_topic(topic, use_incoming_email_if_present: true)).to match(subject.message_id_topic_id_regexp)
+      incoming.update(created_via: IncomingEmail.created_via_types[:handle_mail])
+      expect(subject.generate_for_topic(topic, use_incoming_email_if_present: true)).to eq("<test213428@somemailservice.com>")
+    end
+
     it "generates for the topic without an incoming_email record" do
       expect(subject.generate_for_topic(topic)).to match(subject.message_id_topic_id_regexp)
       expect(subject.generate_for_topic(topic, use_incoming_email_if_present: true)).to match(subject.message_id_topic_id_regexp)