From 1f59a8299d582d6a099280af5a7ab544873699ed Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 12 Jan 2023 13:54:15 +1000 Subject: [PATCH] DEV: Address TODOs for email Message-ID generation (#19842) Remove some old deprecated methods and update docs. Will leave the backwards-compatible Message-ID formats a little while longer just to be sure. --- lib/email/message_id_service.rb | 59 +++++++-------------------- spec/lib/message_id_service_spec.rb | 63 ----------------------------- 2 files changed, 15 insertions(+), 107 deletions(-) diff --git a/lib/email/message_id_service.rb b/lib/email/message_id_service.rb index 4ba6b0d7375..c2cdcbbf2b6 100644 --- a/lib/email/message_id_service.rb +++ b/lib/email/message_id_service.rb @@ -5,14 +5,21 @@ module Email # Email Message-IDs are used in both our outbound and inbound email # flow. For the outbound flow via Email::Sender, we assign a unique # Message-ID for any emails sent out from the application. - # If we are sending an email related to a topic, such as through the + # If we are sending an email related to a post, such as through the # PostAlerter class, then the Message-ID will contain references to - # the topic ID, and if it is for a specific post, the post ID, - # along with a random suffix to make the Message-ID truly unique. - # The host must also be included on the Message-IDs. + # the post ID. The host must also be included on the Message-IDs. + # The format looks like this: + # + # discourse/post/POST_ID@HOST + # + # We previously had the following formats, but support for these + # will be removed in 2023: + # + # topic/TOPIC_ID/POST_ID@HOST + # topic/TOPIC_ID@HOST # # For the inbound email flow via Email::Receiver, we use Message-IDs - # to discern which topic or post the inbound email reply should be + # to discern which topic and post the inbound email reply should be # in response to. In this case, the Message-ID is extracted from the # References and/or In-Reply-To headers, and compared with either # the IncomingEmail table, the Post table, or the IncomingEmail to @@ -29,38 +36,6 @@ 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}>" - end - - "" - 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 - - # 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 - - if canonical - "" - else - "" - end - end - ## # The outbound_message_id may be present because either: # @@ -96,7 +71,7 @@ module Email def find_post_from_message_ids(message_ids) message_ids = message_ids.map { |message_id| message_id_clean(message_id) } - # TODO (martin) 2023-01-01 We should remove these backwards-compatible + # TODO (martin) 2023-04-01 We should remove these backwards-compatible # formats for the Message-ID and solely use the discourse/post/999@host # format. topic_ids = @@ -131,11 +106,7 @@ module Email Post.where(id: post_ids).order(:created_at).last end - def random_suffix - SecureRandom.hex(12) - end - - # TODO (martin) 2023-01-01 We should remove these backwards-compatible + # TODO (martin) 2023-04-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) @@ -144,7 +115,7 @@ module Email !!(message_id =~ message_id_discourse_regexp) end - # TODO (martin) 2023-01-01 We should remove these backwards-compatible + # TODO (martin) 2023-04-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 diff --git a/spec/lib/message_id_service_spec.rb b/spec/lib/message_id_service_spec.rb index dc8fa436fcc..271bed77064 100644 --- a/spec/lib/message_id_service_spec.rb +++ b/spec/lib/message_id_service_spec.rb @@ -7,69 +7,6 @@ RSpec.describe Email::MessageIdService do subject { described_class } - describe "#generate_for_post" do - it "generates for the post using the message_id on the post's incoming_email" do - Fabricate(:incoming_email, message_id: "test@test.localhost", post: post) - post.reload - expect(subject.generate_for_post(post, use_incoming_email_if_present: true)).to eq( - "", - ) - end - - it "generates for the post without an incoming_email record" do - expect(subject.generate_for_post(post)).to match(subject.message_id_post_id_regexp) - expect(subject.generate_for_post(post, use_incoming_email_if_present: true)).to match( - subject.message_id_post_id_regexp, - ) - end - end - - 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, - created_via: IncomingEmail.created_via_types[:handle_mail], - ) - post.reload - expect(subject.generate_for_topic(topic, use_incoming_email_if_present: true)).to eq( - "", - ) - 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( - "", - ) - 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, - ) - end - - it "generates canonical for the topic" do - canonical_topic_id = subject.generate_for_topic(topic, canonical: true) - expect(canonical_topic_id).to match(subject.message_id_topic_id_regexp) - expect(canonical_topic_id).to eq("") - 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")