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

@@ -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,
+  # the post ID. The host must also be included on the Message-IDs.
-  # along with a random suffix to make the Message-ID truly unique.
+  # The format looks like this:
-  # The host must also be included on the Message-IDs.
+  #
+  # 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
-
-        "<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
-
-        # 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
-          "<topic/#{topic.id}@#{host}>"
-        else
-          "<topic/#{topic.id}.#{random_suffix}@#{host}>"
-        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
+      # TODO (martin) 2023-04-01 We should remove these backwards-compatible
-        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)
@@ -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

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(
-        "<test@test.localhost>",
-      )
-    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(
-        "<test213428@somemailservice.com>",
-      )
-    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(
-        "<test213428@somemailservice.com>",
-      )
-    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("<topic/#{topic.id}@test.localhost>")
-    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")