diff --git a/app/models/post.rb b/app/models/post.rb index 3352856160d..cd2c6c7f459 100644 --- a/app/models/post.rb +++ b/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 # diff --git a/db/migrate/20220801044610_add_outbound_message_id_to_post.rb b/db/migrate/20220801044610_add_outbound_message_id_to_post.rb new file mode 100644 index 00000000000..1c1cd9dae36 --- /dev/null +++ b/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 diff --git a/db/post_migrate/20220825005115_backfill_outbound_message_id.rb b/db/post_migrate/20220825005115_backfill_outbound_message_id.rb new file mode 100644 index 00000000000..002cef79617 --- /dev/null +++ b/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 diff --git a/lib/email/message_id_service.rb b/lib/email/message_id_service.rb index 72d67912dd3..fe2af543433 100644 --- a/lib/email/message_id_service.rb +++ b/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 "" 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( - topic_id: message_ids.map { |message_id| message_id[message_id_topic_id_regexp, 1] }.compact, - post_number: 1 - ).pluck(:id) + + # 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. + 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 diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb index 249b6fe1192..7f4d7599691 100644 --- a/lib/email/receiver.rb +++ b/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) diff --git a/lib/email/sender.rb b/lib/email/sender.rb index ce5ab66fc3a..0bcf4a2cf7e 100644 --- a/lib/email/sender.rb +++ b/lib/email/sender.rb @@ -121,52 +121,7 @@ module Email return skip(SkippedEmailLog.reason_types[:sender_topic_deleted]) if topic.blank? add_attachments(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 . 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 + add_identification_field_headers(topic, post) # 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 diff --git a/spec/jobs/regular/group_smtp_email_spec.rb b/spec/jobs/regular/group_smtp_email_spec.rb index 01bc1ec1de8..898506f4f4c 100644 --- a/spec/jobs/regular/group_smtp_email_spec.rb +++ b/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: ") + expect(email_log.raw_headers).to include("In-Reply-To: ") 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") diff --git a/spec/lib/email/receiver_spec.rb b/spec/lib/email/receiver_spec.rb index 26de87b9d45..f286e2b334f 100644 --- a/spec/lib/email/receiver_spec.rb +++ b/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) diff --git a/spec/lib/email/sender_spec.rb b/spec/lib/email/sender_spec.rb index f448a93b61b..f0124acac16 100644 --- a/spec/lib/email/sender_spec.rb +++ b/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 - let(:random_message_id_suffix) { "5f1330cfd941f323d7f99b9e" } + describe "email threading" do 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("") + expect(message.header['Message-Id'].to_s).to eq("") + 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("") + 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("") + 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 eq("") + 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("") - 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("") + expect(message.header['References'].to_s).to eq("") 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("") - expect(message.header['In-Reply-To'].to_s).to eq("") + expect(message.header['Message-ID'].to_s).to eq("") + expect(message.header['References'].to_s).to eq(" ") end - it "sets the 'References' header to the topic canonical reference and all replied posts" do - message.header['X-Discourse-Post-Id'] = post_4.id + 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_6.id email_sender.send + expect(message.header['Message-ID'].to_s).to eq("") + expect(message.header['In-Reply-To'].to_s).to eq(" ") + 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("") + expect(message.header['In-Reply-To'].to_s).to eq(" ") + references = [ - "", - "", - "", + "", + "", + "" ] - 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 - 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") - - message.header['X-Discourse-Post-Id'] = post_4.id + 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 + message.header['X-Discourse-Post-Id'] = post_6.id + PostReply.create(post: post_2, reply: post_6) 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 eq(" ") references = [ - "<#{topic_incoming_email.message_id}>", - "", - "<#{post_2_incoming_email.message_id}>", + "", + "", + "", + "" ] - expect(message.header['References'].to_s).to eq(references.join(" ")) end - end describe "merges custom mandrill header" do diff --git a/spec/lib/message_id_service_spec.rb b/spec/lib/message_id_service_spec.rb index 281b87b9a42..d20c59fe207 100644 --- a/spec/lib/message_id_service_spec.rb +++ b/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([""]) + 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([""]) + 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([ + "", + "", + "", + "" + ]) + end + end + describe "find_post_from_message_ids" do let(:post_format_message_id) { "" } let(:topic_format_message_id) { "" } + let(:discourse_format_message_id) { "" } let(:default_format_message_id) { "<36ac1ddd-5083-461d-b72c-6372fb0e7f33@test.localhost>" } let(:gmail_format_message_id) { "" } @@ -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("")).to eq(true) expect(check_format("topic/1223@test.localhost")).to eq(true) expect(check_format("")).to eq(true) + expect(check_format("discourse/post/1223@test.localhost")).to eq(true) + expect(check_format("")).to eq(true) expect(check_format("topic/1223@blah")).to eq(false) expect(check_format("")).to eq(false)