From 82555ca7612e497df31f9f4fc6969049a9887988 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Wed, 1 Feb 2017 23:02:41 +0100 Subject: [PATCH] FIX: mail threading wasn't working properly in Mac Mail --- lib/email/sender.rb | 38 +++++++++------ spec/components/email/sender_spec.rb | 71 ++++++++++++++++++---------- 2 files changed, 71 insertions(+), 38 deletions(-) diff --git a/lib/email/sender.rb b/lib/email/sender.rb index e5bee8c03ac..444ae7d1cb6 100644 --- a/lib/email/sender.rb +++ b/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 = "" - post_message_id = "" + topic_message_id = first_post.incoming_email&.message_id.present? ? + "<#{first_post.incoming_email.message_id}>" : + "" - incoming_email = IncomingEmail.find_by(post_id: post_id, topic_id: topic_id) - incoming_message_id = "<#{incoming_email.message_id}>" if incoming_email&.message_id.present? + post_message_id = post.incoming_email&.message_id.present? ? + "<#{post.incoming_email.message_id}>" : + "" 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 - "" + if post.post_number == 1 + "" + else + "" + end end end - @message.header['Message-ID'] = incoming_message_id || post_message_id - if post && post.post_number > 1 - @message.header['In-Reply-To'] = referenced_post_message_ids.first || topic_message_id - @message.header['References'] = [topic_message_id, referenced_post_message_ids].flatten.compact.uniq + # https://www.ietf.org/rfc/rfc2822.txt + if post.post_number == 1 + @message.header['Message-ID'] = topic_message_id + 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 diff --git a/spec/components/email/sender_spec.rb b/spec/components/email/sender_spec.rb index 09076a56277..bde8d4cf41e 100644 --- a/spec/components/email/sender_spec.rb +++ b/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') - topic = Fabricate(:topic, category_id: category.id) + message.header['X-Discourse-Post-Id'] = post.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 - message.header['X-Discourse-Topic-Id'] = 5577 + let(:topic) { Fabricate(:topic) } + 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) } - let!(:post_reply_2_3) { PostReply.create(post: post_2, reply: post_3) } - - before do - message.header['X-Discourse-Topic-Id'] = topic.id - end + before { message.header['X-Discourse-Topic-Id'] = topic.id } 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("") + expect(message.header['Message-Id'].to_s).to eq("") 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("") - expect(message.header['In-Reply-To'].to_s).to eq("") + expect(message.header['Message-Id'].to_s).to eq("") + expect(message.header['In-Reply-To'].to_s).to eq("") 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 = [ - "", + "", "", - "", + "", ] 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 = [ + "", + "<#{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-Topic-Id'] = 5577 + message.header['X-Discourse-Post-Id'] = post.id + 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.topic_id).to eq(5577) + expect(email_log.post_id).to eq(post.id) + expect(email_log.topic_id).to eq(topic.id) end end