FEATURE: Overhaul email threading (#17996)

See https://meta.discourse.org/t/discourse-email-messages-are-incorrectly-threaded/233499
for thorough reasoning.

This commit changes how we generate Message-IDs and do email
threading for emails sent from Discourse. The main changes are
as follows:

* Introduce an outbound_message_id column on Post that
  is either a) filled with a Discourse-generated Message-ID
  the first time that post is used for an outbound email
  or b) filled with an original Message-ID from an external
  mail client or service if the post was created from an
  incoming email.
* Change Discourse-generated Message-IDs to be more consistent
  and static, in the format `discourse/post/:post_id@:host`
* Do not send References or In-Reply-To headers for emails sent
  for the OP of topics.
* Make sure that In-Reply-To is filled with either a) the OP's
  Message-ID if the post is not a direct reply or b) the parent
  post's Message-ID
* Make sure that In-Reply-To has all referenced post's Message-IDs
* Make sure that References is filled with a chain of Message-IDs
  from the OP down to the parent post of the new post.

We also are keeping X-Discourse-Post-Id and X-Discourse-Topic-Id,
headers that we previously removed, for easier visual debugging
of outbound emails.

Finally, we backfill the `outbound_message_id` for posts that have
a linked `IncomingEmail` record, using the `message_id` of that record.
We do not need to do that for posts that don't have an incoming email
since they are backfilled at runtime if `outbound_message_id` is missing.

app/models/post.rb

@@ -1209,6 +1209,7 @@ end
 #  action_code             :string
 #  locked_by_id            :integer
 #  image_upload_id         :bigint
+#  outbound_message_id     :string
 #
 # Indexes
 #

db/migrate/20220801044610_add_outbound_message_id_to_post.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class AddOutboundMessageIdToPost < ActiveRecord::Migration[7.0]
+  def change
+    add_column :posts, :outbound_message_id, :string
+  end
+end

db/post_migrate/20220825005115_backfill_outbound_message_id.rb

@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+class BackfillOutboundMessageId < ActiveRecord::Migration[7.0]
+  def up
+    # best effort backfill, we don't care about years worth of message_id
+    # preservation
+    #
+    # we also don't need to backfill outbound_message_id for posts that
+    # do _not_ have an incoming email linked, since that will be backfilled
+    # at runtime if it is missing
+    sql_query = <<~SQL
+      UPDATE posts
+      SET outbound_message_id = ie.message_id
+      FROM incoming_emails AS ie
+      WHERE ie.post_id = posts.id
+        AND posts.created_at >= :one_year_ago
+        AND posts.outbound_message_id IS NULL
+    SQL
+    DB.exec(sql_query, one_year_ago: 1.year.ago)
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

lib/email/message_id_service.rb

@@ -29,6 +29,8 @@ module Email
         "<#{SecureRandom.uuid}@#{host}>"
       end
 
+      # TODO (martin) 2023-01-01 Deprecated, remove this once the new threading
+      # systems have been in place for a while.
       def generate_for_post(post, use_incoming_email_if_present: false)
         if use_incoming_email_if_present && post.incoming_email&.message_id.present?
           return "<#{post.incoming_email.message_id}>"
@@ -37,6 +39,8 @@ module Email
         "<topic/#{post.topic_id}/#{post.id}.#{random_suffix}@#{host}>"
       end
 
+      # TODO (martin) 2023-01-01 Deprecated, remove this once the new threading
+      # systems have been in place for a while.
       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
@@ -58,13 +62,50 @@ module Email
         end
       end
 
+      ##
+      # The outbound_message_id may be present because either:
+      #
+      # * The post was created via incoming email and Email::Receiver, and
+      #   references a Message-ID generated by an external email client or service.
+      # * At least one email has been sent because of the post being created
+      #   to inform interested parties via email.
+      #
+      # If it is blank then we should assume Discourse was the originator
+      # of the post, and generate a Message-ID to be used from now on using
+      # our discourse/post/POST_ID@HOST format.
+      def generate_or_use_existing(post_ids)
+        post_ids = Array.wrap(post_ids)
+        return [] if post_ids.empty?
+
+        DB.exec(<<~SQL, host: host)
+          UPDATE posts
+          SET outbound_message_id = 'discourse/post/' || posts.id || '@' || :host
+          WHERE outbound_message_id IS NULL AND posts.id IN (#{post_ids.join(",")});
+        SQL
+
+        DB.query_single(<<~SQL)
+          SELECT '<' || posts.outbound_message_id || '>'
+          FROM posts
+          WHERE posts.id IN (#{post_ids.join(",")})
+          ORDER BY posts.created_at ASC;
+        SQL
+      end
+
+      ##
+      # Uses extracted Message-IDs from both the In-Reply-To and References
+      # headers from an incoming email.
       def find_post_from_message_ids(message_ids)
         message_ids = message_ids.map { |message_id| message_id_clean(message_id) }
-        post_ids =  message_ids.map { |message_id| message_id[message_id_post_id_regexp, 1] }.compact.map(&:to_i)
+
-        post_ids << Post.where(
+        # TODO (martin) 2023-01-01 We should remove these backwards-compatible
-          topic_id: message_ids.map { |message_id| message_id[message_id_topic_id_regexp, 1] }.compact,
+        # formats for the Message-ID and solely use the discourse/post/999@host
-          post_number: 1
+        # format.
-        ).pluck(:id)
+        topic_ids = message_ids.map { |message_id| message_id[message_id_topic_id_regexp, 1] }.compact.map(&:to_i)
+        post_ids = message_ids.map { |message_id| message_id[message_id_post_id_regexp, 1] }.compact.map(&:to_i)
+
+        post_ids << message_ids.map { |message_id| message_id[message_id_discourse_regexp, 1] }.compact.map(&:to_i)
+
+        post_ids << Post.where(outbound_message_id: message_ids).or(Post.where(topic_id: topic_ids, post_number: 1)).pluck(:id)
         post_ids << EmailLog.where(message_id: message_ids).pluck(:post_id)
         post_ids << IncomingEmail.where(message_id: message_ids).pluck(:post_id)
 
@@ -81,11 +122,18 @@ module Email
         SecureRandom.hex(12)
       end
 
+      # TODO (martin) 2023-01-01 We should remove these backwards-compatible
+      # formats for the Message-ID and solely use the discourse/post/999@host
+      # format.
       def discourse_generated_message_id?(message_id)
         !!(message_id =~ message_id_post_id_regexp) ||
-          !!(message_id =~ message_id_topic_id_regexp)
+          !!(message_id =~ message_id_topic_id_regexp) ||
+          !!(message_id =~ message_id_discourse_regexp)
       end
 
+      # TODO (martin) 2023-01-01 We should remove these backwards-compatible
+      # formats for the Message-ID and solely use the discourse/post/999@host
+      # format.
       def message_id_post_id_regexp
         Regexp.new "topic/\\d+/(\\d+|\\d+\.\\w+)@#{Regexp.escape(host)}"
       end
@@ -94,6 +142,10 @@ module Email
         Regexp.new "topic/(\\d+|\\d+\.\\w+)@#{Regexp.escape(host)}"
       end
 
+      def message_id_discourse_regexp
+        Regexp.new "discourse/post/(\\d+)@#{Regexp.escape(host)}"
+      end
+
       def message_id_rfc_format(message_id)
         message_id.present? && !is_message_id_rfc?(message_id) ? "<#{message_id}>" : message_id
       end

lib/email/receiver.rb

@@ -1328,7 +1328,11 @@ module Email
       end
 
       if result.post
-        @incoming_email.update_columns(topic_id: result.post.topic_id, post_id: result.post.id)
+        IncomingEmail.transaction do
+          @incoming_email.update_columns(topic_id: result.post.topic_id, post_id: result.post.id)
+          result.post.update(outbound_message_id: @incoming_email.message_id)
+        end
+
         if result.post.topic&.private_message? && !is_bounce?
           add_other_addresses(result.post, user, @mail)
 

lib/email/sender.rb

@@ -121,52 +121,7 @@ module Email
         return skip(SkippedEmailLog.reason_types[:sender_topic_deleted]) if topic.blank?
 
         add_attachments(post)
-
+        add_identification_field_headers(topic, post)
-        # If the topic was created from an incoming email, then the Message-ID from
-        # that email will be the canonical reference, otherwise the canonical reference
-        # will be <topic/TOPIC_ID@host>. The canonical reference is used in the
-        # References header.
-        #
-        # This is so the sender of the original email still gets their nice threading
-        # maintained (because their mail client will initiate threading based on
-        # the Message-ID it generated) in the case where there is an incoming email.
-        #
-        # In the latter case, everyone will start their thread with the canonical reference,
-        # because we send it in the References header for all emails.
-        topic_canonical_reference_id = Email::MessageIdService.generate_for_topic(
-          topic, canonical: true, use_incoming_email_if_present: true
-        )
-
-        referenced_posts = Post.includes(:incoming_email)
-          .joins("INNER JOIN post_replies ON post_replies.post_id = posts.id ")
-          .where("post_replies.reply_post_id = ?", post_id)
-          .order(id: :desc)
-
-        referenced_post_message_ids = referenced_posts.map do |referenced_post|
-          if referenced_post.incoming_email&.message_id.present?
-            "<#{referenced_post.incoming_email.message_id}>"
-          else
-            if referenced_post.post_number == 1
-              topic_canonical_reference_id
-            else
-              Email::MessageIdService.generate_for_post(referenced_post)
-            end
-          end
-        end
-
-        # See https://www.ietf.org/rfc/rfc2822.txt for the message format
-        # specification, more useful information can be found in Email::MessageIdService
-        #
-        # The References header is how mail clients handle threading. The Message-ID
-        # must always be unique.
-        if post.post_number == 1
-          @message.header['Message-ID']  = Email::MessageIdService.generate_for_topic(topic)
-          @message.header['References']  = [topic_canonical_reference_id]
-        else
-          @message.header['Message-ID']  = Email::MessageIdService.generate_for_post(post)
-          @message.header['In-Reply-To'] = referenced_post_message_ids[0] || topic_canonical_reference_id
-          @message.header['References']  = [topic_canonical_reference_id, referenced_post_message_ids].flatten.compact.uniq
-        end
 
         # See https://www.ietf.org/rfc/rfc2919.txt for the List-ID
         # specification.
@@ -216,10 +171,6 @@ module Email
       email_log.post_id = post_id if post_id.present?
       email_log.topic_id = topic_id if topic_id.present?
 
-      # Remove headers we don't need anymore
-      @message.header['X-Discourse-Topic-Id'] = nil if topic_id.present?
-      @message.header['X-Discourse-Post-Id']  = nil if post_id.present?
-
       if reply_key.present?
         @message.header['Reply-To'] = header_value('Reply-To').gsub!("%{reply_key}", reply_key)
         @message.header[Email::MessageBuilder::ALLOW_REPLY_BY_EMAIL_HEADER] = nil
@@ -514,5 +465,118 @@ module Email
     def self.bounce_address(bounce_key)
       SiteSetting.reply_by_email_address.sub("%{reply_key}", "verp-#{bounce_key}")
     end
+
+    ##
+    # When sending an email for the first post (OP) of the topic, we do not
+    # set References or In-Reply-To headers, since there is nothing yet
+    # to reference. This counts as the first email in the thread.
+    #
+    # Once set, the post's `outbound_message_id` should _always_ be used
+    # when sending emails relating to a particular post to maintain threading.
+    # This will either be:
+    #
+    # a) A Message-ID generated in an external main client or service which
+    #    is recorded when creating a post from an IncomingEmail via Email::Receiver
+    # b) A Message-ID generated by Discourse and recorded when sending an email
+    #    for a newly created post, which is created and saved here to the
+    #    outbound_message_id column on the Post.
+    #
+    # The RFC that covers using "Identification Fields", which are References,
+    # In-Reply-To, Message-ID, et. al. can be in the RFC link below. It's a good idea to read
+    # this beginning in the area immediately after these quotes, at least to understand
+    # the 3 main headers:
+    #
+    # > The "Message-ID:" field provides a unique message identifier that
+    # > refers to a particular version of a particular message.  The
+    # > uniqueness of the message identifier is guaranteed by the host that
+    # > generates it.
+    #
+    # > ...
+    #
+    # > The "In-Reply-To:" field may be used to identify the message (or
+    # > messages) to which the new message is a reply, while the "References:"
+    # > field may be used to identify a "thread" of conversation.
+    #
+    # https://www.rfc-editor.org/rfc/rfc5322.html#section-3.6.4
+    #
+    # It is a long read, but to understand the decision making process for this
+    # threading logic you can take a look at:
+    #
+    # https://meta.discourse.org/t/discourse-email-messages-are-incorrectly-threaded/233499
+    def add_identification_field_headers(topic, post)
+      @message.header["Message-ID"] = Email::MessageIdService.generate_or_use_existing(post.id).first
+
+      if post.post_number > 1
+        op_message_id = Email::MessageIdService.generate_or_use_existing(topic.first_post.id).first
+
+        ##
+        # Whenever we reply to a post directly _or_ quote a post, a PostReply
+        # record is made, with the reply_post_id referencing the newly created
+        # post, and the post_id referencing the post that was quoted or replied to.
+        referenced_posts = Post
+          .joins("INNER JOIN post_replies ON post_replies.post_id = posts.id ")
+          .where("post_replies.reply_post_id = ?", post.id)
+          .order(id: :desc)
+          .to_a
+
+        ##
+        # No referenced posts means that we are just creating a new post not
+        # referring to anything, and as such we should just fall back to using
+        # the OP.
+        if referenced_posts.empty?
+          @message.header["In-Reply-To"] = op_message_id
+          @message.header["References"] = op_message_id
+        else
+          ##
+          # When referencing _multiple_ posts then we just choose the most recent one
+          # to use for References so we have a single parent to work with, but
+          # every directly replied to post can go into In-Reply-To.
+          #
+          # We want to make sure all of the outbound_message_ids are already filled here.
+          in_reply_to_message_ids = MessageIdService.generate_or_use_existing(referenced_posts.map(&:id))
+          @message.header["In-Reply-To"] = in_reply_to_message_ids
+          most_recent_post_message_id = in_reply_to_message_ids.last
+
+          ##
+          # The RFC specifically states that the content of the parent's References
+          # field (in our case a tree of replies based on the PostReply table in
+          # addition to the OP post's Message-ID) first, _then_ the parent's
+          # Message-ID (in our case the outbound_message_id of the post we are replying to).
+          #
+          # This creates a thread from the OP all the way down to the most recent post we
+          # are replying to.
+          reply_tree = referenced_post_reply_tree(referenced_posts.first)
+          parent_message_ids = MessageIdService.generate_or_use_existing(reply_tree.values.flatten)
+
+          @message.header["References"] = [
+            op_message_id, parent_message_ids, most_recent_post_message_id
+          ].flatten.uniq
+        end
+      end
+    end
+
+    def referenced_post_reply_tree(post)
+      results = DB.query(<<~SQL, start_post_id: post.id)
+        WITH RECURSIVE cte AS (
+          SELECT reply_post_id, post_id FROM post_replies
+          WHERE reply_post_id = :start_post_id
+          UNION
+          SELECT pr.reply_post_id, pr.post_id
+          FROM post_replies pr
+          INNER JOIN cte
+          ON cte.post_id = pr.reply_post_id
+        )
+        SELECT DISTINCT cte.*, posts.created_at, posts.outbound_message_id
+        FROM cte
+        INNER JOIN posts ON posts.id = cte.reply_post_id
+        ORDER BY posts.created_at DESC, post_id DESC;
+      SQL
+      results.inject({}) do |hash, value|
+        # We only want to get a single replied-to post, which is the most recently
+        # created post, since we cannot deal with multiple parents for References
+        hash[value.reply_post_id] ||= [value.post_id]
+        hash
+      end
+    end
   end
 end

spec/jobs/regular/group_smtp_email_spec.rb

@@ -21,7 +21,6 @@ RSpec.describe Jobs::GroupSmtpEmail do
   let(:staged1) { Fabricate(:staged, email: "otherguy@test.com") }
   let(:staged2) { Fabricate(:staged, email: "cormac@lit.com") }
   let(:normaluser) { Fabricate(:user, email: "justanormalguy@test.com", username: "normaluser") }
-  let(:random_message_id_suffix) { "5f1330cfd941f323d7f99b9e" }
 
   before do
     SiteSetting.enable_smtp = true
@@ -33,7 +32,6 @@ RSpec.describe Jobs::GroupSmtpEmail do
     TopicAllowedUser.create(user: staged1, topic: topic)
     TopicAllowedUser.create(user: staged2, topic: topic)
     TopicAllowedUser.create(user: normaluser, topic: topic)
-    Email::MessageIdService.stubs(:random_suffix).returns(random_message_id_suffix)
   end
 
   it "sends an email using the GroupSmtpMailer and Email::Sender" do
@@ -61,7 +59,7 @@ RSpec.describe Jobs::GroupSmtpEmail do
     PostReply.create(post: second_post, reply: post)
     subject.execute(args)
     email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id)
-    expect(email_log.raw_headers).to include("In-Reply-To: <topic/#{post.topic_id}/#{second_post.id}.#{random_message_id_suffix}@#{Email::Sender.host_for(Discourse.base_url)}>")
+    expect(email_log.raw_headers).to include("In-Reply-To: <discourse/post/#{second_post.id}@#{Email::Sender.host_for(Discourse.base_url)}>")
     expect(email_log.as_mail_message.html_part.to_s).not_to include(I18n.t("user_notifications.in_reply_to"))
   end
 
@@ -82,7 +80,7 @@ RSpec.describe Jobs::GroupSmtpEmail do
     subject.execute(args)
     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}.#{random_message_id_suffix}@test.localhost")
+    expect(email_log.message_id).to eq("discourse/post/#{post.id}@test.localhost")
   end
 
   it "creates an IncomingEmail record with the correct details to avoid double processing IMAP" do
@@ -91,7 +89,7 @@ RSpec.describe Jobs::GroupSmtpEmail do
     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}.#{random_message_id_suffix}@test.localhost")
+    expect(incoming_email.message_id).to eq("discourse/post/#{post.id}@test.localhost")
     expect(incoming_email.created_via).to eq(IncomingEmail.created_via_types[:group_smtp])
     expect(incoming_email.to_addresses).to eq("test@test.com")
     expect(incoming_email.cc_addresses).to eq("otherguy@test.com;cormac@lit.com")
@@ -115,7 +113,7 @@ RSpec.describe Jobs::GroupSmtpEmail do
     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}.#{random_message_id_suffix}@test.localhost")
+    expect(email_log.message_id).to eq("discourse/post/#{post.id}@test.localhost")
   end
 
   it "creates an IncomingEmail record with the correct details to avoid double processing IMAP" do
@@ -124,7 +122,7 @@ RSpec.describe Jobs::GroupSmtpEmail do
     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}.#{random_message_id_suffix}@test.localhost")
+    expect(incoming_email.message_id).to eq("discourse/post/#{post.id}@test.localhost")
     expect(incoming_email.created_via).to eq(IncomingEmail.created_via_types[:group_smtp])
     expect(incoming_email.to_addresses).to eq("test@test.com")
     expect(incoming_email.cc_addresses).to eq("otherguy@test.com;cormac@lit.com")

spec/lib/email/receiver_spec.rb

@@ -373,6 +373,12 @@ RSpec.describe Email::Receiver do
       expect(IncomingEmail.last.created_via).to eq(IncomingEmail.created_via_types[:imap])
     end
 
+    it "stores the message_id of the incoming email against the post as outbound_message_id" do
+      expect { process(:text_reply, source: :handle_mail) }.to change(Post, :count)
+      message_id = IncomingEmail.last.message_id
+      expect(Post.last.outbound_message_id).to eq(message_id)
+    end
+
     it "automatically elides gmail quotes" do
       SiteSetting.always_show_trimmed_content = true
       expect { process(:gmail_html_reply) }.to change { topic.posts.count }
@@ -898,6 +904,12 @@ RSpec.describe Email::Receiver do
       expect { process(:cc) }.to raise_error(Email::Receiver::TooManyRecipientsError)
     end
 
+    it "uses the incoming_email message-id as the new post's outbound_message_id" do
+      expect { process(:cc) }.to change(Topic, :count)
+      message_id = IncomingEmail.last.message_id
+      expect(Topic.last.first_post.outbound_message_id).to eq(message_id)
+    end
+
     describe "reply-to header" do
       before do
         SiteSetting.block_auto_generated_emails = false
@@ -977,7 +989,7 @@ RSpec.describe Email::Receiver do
         expect { process(:email_reply_4) }.to change { topic.posts.count }.by(1)
       end
 
-      describe "replying with various message-id formats" do
+      describe "replying with various message-id formats using In-Reply-To header" do
         let!(:topic) do
           process(:email_reply_1)
           Topic.last
@@ -1021,6 +1033,11 @@ RSpec.describe Email::Receiver do
           expect { process_mail_with_message_id("topic/#{topic.id}/#{post.id}.x3487nxy877843x@test.localhost") }.to change { Post.count }.by(1)
           expect(topic.reload.posts.last.raw).to include("This is email reply testing with Message-ID formats")
         end
+
+        it "posts a reply using a message-id in the format discourse/post/POST_ID@HOST" do
+          expect { process_mail_with_message_id("discourse/post/#{post.id}@test.localhost") }.to change { Post.count }.by(1)
+          expect(topic.reload.posts.last.raw).to include("This is email reply testing with Message-ID formats")
+        end
       end
     end
 
@@ -1236,7 +1253,6 @@ RSpec.describe Email::Receiver do
         NotificationEmailer.enable
         SiteSetting.disallow_reply_by_email_after_days = 10000
         Jobs.run_immediately!
-        Email::MessageIdService.stubs(:random_suffix).returns("blah123")
       end
 
       def reply_as_group_user
@@ -1261,7 +1277,7 @@ RSpec.describe Email::Receiver do
 
       it "creates an EmailLog when someone from the group replies, and does not create an IncomingEmail record for the reply" do
         email_log, group_post = reply_as_group_user
-        expect(email_log.message_id).to eq("topic/#{original_inbound_email_topic.id}/#{group_post.id}.blah123@test.localhost")
+        expect(email_log.message_id).to eq("discourse/post/#{group_post.id}@test.localhost")
         expect(email_log.to_address).to eq("two@foo.com")
         expect(email_log.email_type).to eq("user_private_message")
         expect(email_log.post_id).to eq(group_post.id)

spec/lib/email/sender_spec.rb

@@ -240,23 +240,6 @@ RSpec.describe Email::Sender do
       end
     end
 
-    describe "removes custom Discourse headers from topic notification mails" do
-      fab!(:topic) { Fabricate(:topic) }
-      fab!(:post) { Fabricate(:post, topic: topic) }
-
-      before do
-        message.header['X-Discourse-Post-Id']  = post.id
-        message.header['X-Discourse-Topic-Id'] = topic.id
-      end
-
-      it 'should remove the right headers' do
-        email_sender.send
-        expect(message.header['X-Discourse-Topic-Id']).not_to be_present
-        expect(message.header['X-Discourse-Post-Id']).not_to be_present
-        expect(message.header['X-Discourse-Reply-Key']).not_to be_present
-      end
-    end
-
     describe "removes custom Discourse headers from digest/registration/other mails" do
       it 'should remove the right headers' do
         email_sender.send
@@ -266,35 +249,40 @@ RSpec.describe Email::Sender do
       end
     end
 
-    context "with email threading" do
+    describe "email threading" do
-      let(:random_message_id_suffix) { "5f1330cfd941f323d7f99b9e" }
       fab!(:topic) { Fabricate(:topic) }
 
       fab!(:post_1) { Fabricate(:post, topic: topic, post_number: 1) }
       fab!(:post_2) { Fabricate(:post, topic: topic, post_number: 2) }
       fab!(:post_3) { Fabricate(:post, topic: topic, post_number: 3) }
       fab!(:post_4) { Fabricate(:post, topic: topic, post_number: 4) }
+      fab!(:post_5) { Fabricate(:post, topic: topic, post_number: 5) }
+      fab!(:post_6) { Fabricate(:post, topic: topic, post_number: 6) }
 
       let!(:post_reply_1_4) { PostReply.create(post: post_1, reply: post_4) }
       let!(:post_reply_2_4) { PostReply.create(post: post_2, reply: post_4) }
       let!(:post_reply_3_4) { PostReply.create(post: post_3, reply: post_4) }
+      let!(:post_reply_4_5) { PostReply.create(post: post_4, reply: post_5) }
+      let!(:post_reply_4_6) { PostReply.create(post: post_4, reply: post_6) }
+      let!(:post_reply_5_6) { PostReply.create(post: post_5, reply: post_6) }
 
       before do
         message.header['X-Discourse-Topic-Id'] = topic.id
-        Email::MessageIdService.stubs(:random_suffix).returns(random_message_id_suffix)
       end
 
-      it "doesn't set the 'In-Reply-To' header but does set the 'References' header on the first post" do
+      it "doesn't set References or In-Reply-To headers on the first post, only generates a Message-ID and saves it against the post" do
         message.header['X-Discourse-Post-Id'] = post_1.id
 
         email_sender.send
+        post_1.reload
 
-        expect(message.header['Message-Id'].to_s).to eq("<topic/#{topic.id}.#{random_message_id_suffix}@test.localhost>")
+        expect(message.header['Message-Id'].to_s).to eq("<discourse/post/#{post_1.id}@test.localhost>")
+        expect(post_1.outbound_message_id).to eq("discourse/post/#{post_1.id}@test.localhost")
         expect(message.header['In-Reply-To'].to_s).to be_blank
-        expect(message.header['References'].to_s).to eq("<topic/#{topic.id}@test.localhost>")
+        expect(message.header['References'].to_s).to be_blank
       end
 
-      it "sets the 'References' header with the incoming email Message-ID if it exists on the first post" do
+      it "uses the existing Message-ID header from the incoming email when sending the first post email" do
         incoming = Fabricate(
           :incoming_email,
           topic: topic,
@@ -302,69 +290,78 @@ RSpec.describe Email::Sender do
           message_id: "blah1234@someemailprovider.com",
           created_via: IncomingEmail.created_via_types[:handle_mail]
         )
+        post_1.update!(outbound_message_id: incoming.message_id)
         message.header['X-Discourse-Post-Id'] = post_1.id
 
         email_sender.send
 
-        expect(message.header['Message-Id'].to_s).to eq("<topic/#{topic.id}.#{random_message_id_suffix}@test.localhost>")
+        expect(message.header['Message-Id'].to_s).to eq("<blah1234@someemailprovider.com>")
         expect(message.header['In-Reply-To'].to_s).to be_blank
-        expect(message.header['References'].to_s).to eq("<blah1234@someemailprovider.com>")
+        expect(message.header['References'].to_s).to be_blank
       end
 
-      it "sets the 'In-Reply-To' header to the topic canonical reference by default" do
+      it "if no post is directly replied to then the Message-ID of post 1 via outbound_message_id should be used" do
         message.header['X-Discourse-Post-Id'] = post_2.id
 
         email_sender.send
 
-        expect(message.header['Message-Id'].to_s).to eq("<topic/#{topic.id}/#{post_2.id}.#{random_message_id_suffix}@test.localhost>")
+        expect(message.header['Message-Id'].to_s).to eq("<discourse/post/#{post_2.id}@test.localhost>")
-        expect(message.header['In-Reply-To'].to_s).to eq("<topic/#{topic.id}@test.localhost>")
+        expect(message.header['In-Reply-To'].to_s).to eq("<discourse/post/#{post_1.id}@test.localhost>")
+        expect(message.header['References'].to_s).to eq("<discourse/post/#{post_1.id}@test.localhost>")
       end
 
-      it "sets the 'In-Reply-To' header to the newest replied post" do
+      it "sets the References header to the most recently created replied post, as well as the OP, if there are no other replies in the chain" do
         message.header['X-Discourse-Post-Id'] = post_4.id
 
         email_sender.send
 
-        expect(message.header['Message-Id'].to_s).to eq("<topic/#{topic.id}/#{post_4.id}.#{random_message_id_suffix}@test.localhost>")
+        expect(message.header['Message-ID'].to_s).to eq("<discourse/post/#{post_4.id}@test.localhost>")
-        expect(message.header['In-Reply-To'].to_s).to eq("<topic/#{topic.id}/#{post_3.id}.#{random_message_id_suffix}@test.localhost>")
+        expect(message.header['References'].to_s).to eq("<discourse/post/#{post_1.id}@test.localhost> <discourse/post/#{post_3.id}@test.localhost>")
       end
 
-      it "sets the 'References' header to the topic canonical reference and all replied posts" do
+      it "sets the In-Reply-To header to all the posts that the post is connected to via PostReply" do
-        message.header['X-Discourse-Post-Id'] = post_4.id
+        message.header['X-Discourse-Post-Id'] = post_6.id
 
         email_sender.send
 
+        expect(message.header['Message-ID'].to_s).to eq("<discourse/post/#{post_6.id}@test.localhost>")
+        expect(message.header['In-Reply-To'].to_s).to eq("<discourse/post/#{post_4.id}@test.localhost> <discourse/post/#{post_5.id}@test.localhost>")
+      end
+
+      it "sets the In-Reply-To and References header to the most recently created replied post and includes the parents of that post in References, as well as the OP" do
+        message.header['X-Discourse-Post-Id'] = post_4.id
+        PostReply.create(post: post_2, reply: post_3)
+
+        email_sender.send
+
+        expect(message.header['Message-ID'].to_s).to eq("<discourse/post/#{post_4.id}@test.localhost>")
+        expect(message.header['In-Reply-To'].to_s).to eq("<discourse/post/#{post_1.id}@test.localhost> <discourse/post/#{post_2.id}@test.localhost> <discourse/post/#{post_3.id}@test.localhost>")
+
         references = [
-          "<topic/#{topic.id}@test.localhost>",
+          "<discourse/post/#{post_1.id}@test.localhost>",
-          "<topic/#{topic.id}/#{post_3.id}.#{random_message_id_suffix}@test.localhost>",
+          "<discourse/post/#{post_2.id}@test.localhost>",
-          "<topic/#{topic.id}/#{post_2.id}.#{random_message_id_suffix}@test.localhost>",
+          "<discourse/post/#{post_3.id}@test.localhost>"
         ]
-
         expect(message.header['References'].to_s).to eq(references.join(" "))
       end
 
-      it "uses the incoming_email message_id when available, but always uses a random message-id" do
+      it "handles a complex reply tree to the OP for References, only using one Message-ID if there are multiple parents for a post" do
-        topic_incoming_email  = IncomingEmail.create(
+        message.header['X-Discourse-Post-Id'] = post_6.id
-          topic: topic, post: post_1, message_id: "foo@bar", created_via: IncomingEmail.created_via_types[:handle_mail]
+        PostReply.create(post: post_2, reply: post_6)
-        )
-        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")
-
-        message.header['X-Discourse-Post-Id'] = post_4.id
 
         email_sender.send
 
-        expect(message.header['Message-Id'].to_s).to eq("<topic/#{topic.id}/#{post_4.id}.5f1330cfd941f323d7f99b9e@test.localhost>")
+        expect(message.header['Message-ID'].to_s).to eq("<discourse/post/#{post_6.id}@test.localhost>")
+        expect(message.header['In-Reply-To'].to_s).to eq("<discourse/post/#{post_2.id}@test.localhost> <discourse/post/#{post_4.id}@test.localhost> <discourse/post/#{post_5.id}@test.localhost>")
 
         references = [
-          "<#{topic_incoming_email.message_id}>",
+          "<discourse/post/#{post_1.id}@test.localhost>",
-          "<topic/#{topic.id}/#{post_3.id}.#{random_message_id_suffix}@test.localhost>",
+          "<discourse/post/#{post_3.id}@test.localhost>",
-          "<#{post_2_incoming_email.message_id}>",
+          "<discourse/post/#{post_4.id}@test.localhost>",
+          "<discourse/post/#{post_5.id}@test.localhost>"
         ]
-
         expect(message.header['References'].to_s).to eq(references.join(" "))
       end
-
     end
 
     describe "merges custom mandrill header" do

spec/lib/message_id_service_spec.rb

@@ -52,9 +52,39 @@ RSpec.describe Email::MessageIdService do
     end
   end
 
+  describe "#generate_or_use_existing" do
+    it "does not override a post's existing outbound_message_id" do
+      post.update!(outbound_message_id: "blah@host.test")
+      result = subject.generate_or_use_existing(post.id)
+      expect(result).to eq(["<blah@host.test>"])
+    end
+
+    it "generates an outbound_message_id in the correct format if it's blank for the post" do
+      post.update!(outbound_message_id: nil)
+      result = subject.generate_or_use_existing(post.id)
+      expect(result).to eq(["<discourse/post/#{post.id}@#{Email::MessageIdService.host}>"])
+    end
+
+    it "handles bulk posts with a mixture of existing and new outbound_message_ids, returning in created_at order" do
+      topic = Fabricate(:topic)
+      post_bulk1 = Fabricate(:post, topic: topic, created_at: 10.days.ago, outbound_message_id: "blah@test.host")
+      post_bulk2 = Fabricate(:post, topic: topic, created_at: 12.days.ago, outbound_message_id: nil)
+      post_bulk3 = Fabricate(:post, topic: topic, created_at: 11.days.ago, outbound_message_id: "sf92c349438509=3453@test.host")
+      post_bulk4 = Fabricate(:post, topic: topic, created_at: 3.days.ago, outbound_message_id: nil)
+      result = subject.generate_or_use_existing([post_bulk1.id, post_bulk2.id, post_bulk3.id, post_bulk4.id])
+      expect(result).to eq([
+        "<discourse/post/#{post_bulk2.id}@#{Email::MessageIdService.host}>",
+        "<sf92c349438509=3453@test.host>",
+        "<blah@test.host>",
+        "<discourse/post/#{post_bulk4.id}@#{Email::MessageIdService.host}>"
+      ])
+    end
+  end
+
   describe "find_post_from_message_ids" do
     let(:post_format_message_id) { "<topic/#{topic.id}/#{post.id}.test123@test.localhost>" }
     let(:topic_format_message_id) { "<topic/#{topic.id}.test123@test.localhost>" }
+    let(:discourse_format_message_id) { "<discourse/post/#{post.id}@test.localhost>" }
     let(:default_format_message_id) { "<36ac1ddd-5083-461d-b72c-6372fb0e7f33@test.localhost>" }
     let(:gmail_format_message_id) { "<CAPGrNgZ7QEFuPcsxJBRZLhBhAYPO_ruYpCANSdqiQEbc9Otpiw@mail.gmail.com>" }
 
@@ -66,6 +96,15 @@ RSpec.describe Email::MessageIdService do
       expect(subject.find_post_from_message_ids([topic_format_message_id])).to eq(post)
     end
 
+    it "finds a post based only on a discourse-format message id" do
+      expect(subject.find_post_from_message_ids([discourse_format_message_id])).to eq(post)
+    end
+
+    it "finds a post from the post's outbound_message_id" do
+      post.update!(outbound_message_id: subject.message_id_clean(discourse_format_message_id))
+      expect(subject.find_post_from_message_ids([discourse_format_message_id])).to eq(post)
+    end
+
     it "finds a post from the email log" do
       email_log = Fabricate(:email_log, message_id: subject.message_id_clean(default_format_message_id))
       expect(subject.find_post_from_message_ids([default_format_message_id])).to eq(email_log.post)
@@ -104,6 +143,8 @@ RSpec.describe Email::MessageIdService do
       expect(check_format("<topic/1223/4525@test.localhost>")).to eq(true)
       expect(check_format("topic/1223@test.localhost")).to eq(true)
       expect(check_format("<topic/1223@test.localhost>")).to eq(true)
+      expect(check_format("discourse/post/1223@test.localhost")).to eq(true)
+      expect(check_format("<discourse/post/1223@test.localhost>")).to eq(true)
 
       expect(check_format("topic/1223@blah")).to eq(false)
       expect(check_format("<CAPGrNgZ7QEFuPcsxJBRZLhBhAYPO_ruYpCANSdqiQEbc9Otpiw@mail.gmail.com>")).to eq(false)