FIX: mail threading wasn't working properly in Mac Mail

lib/email/sender.rb

@@ -67,8 +67,8 @@ module Email
 
       host = Email::Sender.host_for(Discourse.base_url)
 
-      topic_id = header_value('X-Discourse-Topic-Id')
+      post_id   = header_value('X-Discourse-Post-Id')
-      post_id = header_value('X-Discourse-Post-Id')
+      topic_id  = header_value('X-Discourse-Topic-Id')
       reply_key = header_value('X-Discourse-Reply-Key')
 
       # always set a default Message ID from the host
@@ -79,32 +79,42 @@ module Email
 
         post = Post.find_by(id: post_id)
         topic = Topic.find_by(id: topic_id)
+        first_post = topic.ordered_posts.first
 
-        topic_message_id = "<topic/#{topic_id}@#{host}>"
+        topic_message_id = first_post.incoming_email&.message_id.present? ? 
-        post_message_id = "<topic/#{topic_id}/#{post_id}@#{host}>"
+          "<#{first_post.incoming_email.message_id}>" :
+          "<topic/#{topic_id}@#{host}>"
 
-        incoming_email = IncomingEmail.find_by(post_id: post_id, topic_id: topic_id)
+        post_message_id = post.incoming_email&.message_id.present? ?
-        incoming_message_id = "<#{incoming_email.message_id}>" if incoming_email&.message_id.present?
+          "<#{post.incoming_email.message_id}>" :
+          "<topic/#{topic_id}/#{post_id}@#{host}>"
 
         referenced_posts = Post.includes(:incoming_email)
                                .where(id: PostReply.where(reply_id: post_id).select(:post_id))
                                .order(id: :desc)
 
         referenced_post_message_ids = referenced_posts.map do |post|
-          if post&.incoming_email&.message_id.present?
+          if post.incoming_email&.message_id.present?
             "<#{post.incoming_email.message_id}>"
           else
-            "<topic/#{topic_id}/#{post.id}@#{host}>"
+            if post.post_number == 1
+              "<topic/#{topic_id}@#{host}>"
+            else
+              "<topic/#{topic_id}/#{post.id}@#{host}>"
+            end
           end
         end
 
-        @message.header['Message-ID'] = incoming_message_id || post_message_id
+        # https://www.ietf.org/rfc/rfc2822.txt
-        if post && post.post_number > 1
+        if post.post_number == 1
-          @message.header['In-Reply-To'] = referenced_post_message_ids.first || topic_message_id
+          @message.header['Message-ID']  = topic_message_id
-          @message.header['References'] = [topic_message_id, referenced_post_message_ids].flatten.compact.uniq
+        else
+          @message.header['Message-ID']  = post_message_id
+          @message.header['In-Reply-To'] = referenced_post_message_ids[0] || topic_message_id
+          @message.header['References']  = [referenced_post_message_ids, topic_message_id].flatten.compact.uniq
         end
 
-        # http://www.ietf.org/rfc/rfc2919.txt
+        # https://www.ietf.org/rfc/rfc2919.txt
         if topic && topic.category && !topic.category.uncategorized?
           list_id = "<#{topic.category.name.downcase.tr(' ', '-')}.#{host}>"
 
@@ -117,7 +127,7 @@ module Email
           list_id = "<#{host}>"
         end
 
-        # http://www.ietf.org/rfc/rfc3834.txt
+        # https://www.ietf.org/rfc/rfc3834.txt
         @message.header['Precedence']   = 'list'
         @message.header['List-ID']      = list_id
         @message.header['List-Archive'] = topic.url if topic

spec/components/email/sender_spec.rb

@@ -96,9 +96,12 @@ describe Email::Sender do
     end
 
     context "adds a List-ID header to identify the forum" do
+      let(:category) { Fabricate(:category, name: 'Name With Space') }
+      let(:topic) { Fabricate(:topic, category: category) }
+      let(:post) { Fabricate(:post, topic: topic) }
+
       before do
-        category =  Fabricate(:category, name: 'Name With Space')
+        message.header['X-Discourse-Post-Id']  = post.id
-        topic = Fabricate(:topic, category_id: category.id)
         message.header['X-Discourse-Topic-Id'] = topic.id
       end
 
@@ -120,8 +123,12 @@ describe Email::Sender do
     end
 
     context "adds Precedence header" do
-      before do
+      let(:topic) { Fabricate(:topic) }
-        message.header['X-Discourse-Topic-Id'] = 5577
+      let(: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 add the right header' do
@@ -131,8 +138,12 @@ describe Email::Sender do
     end
 
     context "removes custom Discourse headers from topic notification mails" do
+      let(:topic) { Fabricate(:topic) }
+      let(:post) { Fabricate(:post, topic: topic) }
+
       before do
-        message.header['X-Discourse-Topic-Id'] = 5577
+        message.header['X-Discourse-Post-Id']  = post.id
+        message.header['X-Discourse-Topic-Id'] = topic.id
       end
 
       it 'should remove the right headers' do
@@ -160,21 +171,18 @@ describe Email::Sender do
       let(:post_3) { Fabricate(:post, topic: topic, post_number: 3) }
       let(:post_4) { Fabricate(:post, topic: topic, post_number: 4) }
 
-      let!(:incoming_email) { IncomingEmail.create(topic: topic, post: post_4, message_id: "foobar") }
+      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_1_3) { PostReply.create(post: post_1, reply: post_3) }
+      before { message.header['X-Discourse-Topic-Id'] = topic.id }
-      let!(:post_reply_2_3) { PostReply.create(post: post_2, reply: post_3) }
-
-      before do
-        message.header['X-Discourse-Topic-Id'] = topic.id
-      end
 
       it "doesn't set the 'In-Reply-To' and 'References' headers on the first post" do
         message.header['X-Discourse-Post-Id'] = post_1.id
 
         email_sender.send
 
-        expect(message.header['Message-Id'].to_s).to eq("<topic/#{topic.id}/#{post_1.id}@test.localhost>")
+        expect(message.header['Message-Id'].to_s).to eq("<topic/#{topic.id}@test.localhost>")
         expect(message.header['In-Reply-To'].to_s).to be_blank
         expect(message.header['References'].to_s).to be_blank
       end
@@ -189,34 +197,46 @@ describe Email::Sender do
       end
 
       it "sets the 'In-Reply-To' header to the newest replied post" do
-        message.header['X-Discourse-Post-Id'] = post_3.id
+        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_3.id}@test.localhost>")
+        expect(message.header['Message-Id'].to_s).to eq("<topic/#{topic.id}/#{post_4.id}@test.localhost>")
-        expect(message.header['In-Reply-To'].to_s).to eq("<topic/#{topic.id}/#{post_2.id}@test.localhost>")
+        expect(message.header['In-Reply-To'].to_s).to eq("<topic/#{topic.id}/#{post_3.id}@test.localhost>")
       end
 
       it "sets the 'References' header to the topic and all replied posts" do
-        message.header['X-Discourse-Post-Id'] = post_3.id
+        message.header['X-Discourse-Post-Id'] = post_4.id
 
         email_sender.send
 
         references = [
-          "<topic/#{topic.id}@test.localhost>",
+          "<topic/#{topic.id}/#{post_3.id}@test.localhost>",
           "<topic/#{topic.id}/#{post_2.id}@test.localhost>",
-          "<topic/#{topic.id}/#{post_1.id}@test.localhost>",
+          "<topic/#{topic.id}@test.localhost>",
         ]
 
         expect(message.header['References'].to_s).to eq(references.join(" "))
       end
 
       it "uses the incoming_email message_id when available" do
+        topic_incoming_email  = IncomingEmail.create(topic: topic, post: post_1, message_id: "foo@bar")
+        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("<#{incoming_email.message_id}>")
+        expect(message.header['Message-Id'].to_s).to eq("<#{post_4_incoming_email.message_id}>")
+
+        references = [
+          "<topic/#{topic.id}/#{post_3.id}@test.localhost>",
+          "<#{post_2_incoming_email.message_id}>",
+          "<#{topic_incoming_email.message_id}>",
+        ]
+
+        expect(message.header['References'].to_s).to eq(references.join(" "))
       end
 
     end
@@ -260,17 +280,20 @@ describe Email::Sender do
     end
 
     context "email log with a post id and topic id" do
+      let(:topic) { Fabricate(:topic) }
+      let(:post) { Fabricate(:post, topic: topic) }
+
       before do
-        message.header['X-Discourse-Post-Id'] = 3344
+        message.header['X-Discourse-Post-Id'] = post.id
-        message.header['X-Discourse-Topic-Id'] = 5577
+        message.header['X-Discourse-Topic-Id'] = topic.id
       end
 
       let(:email_log) { EmailLog.last }
 
       it 'should create the right log' do
         email_sender.send
-        expect(email_log.post_id).to eq(3344)
+        expect(email_log.post_id).to eq(post.id)
-        expect(email_log.topic_id).to eq(5577)
+        expect(email_log.topic_id).to eq(topic.id)
       end
     end