FIX: Link up reply to post correctly when emailing group (#13339)

FIX: Link up reply to post correctly when emailing group (#13339)

When replying to a user_private_message email originating from a group PM that does not have a reply key (e.g. when replying directly to the group’s SMTP address), we were mistakenly linking the new post created from the reply to the OP and the user who created the topic, based on the first IncomingEmail message ID in the topic, rather than using the correct reply to user and post number that the user actually replied to.

We now use the In-Reply-To header to look up the corresponding EmailLog record when the user who replied was sent a user_private_message email, and use the post from that as the reply_to_user/post.

This also removes superfluous filtering of incoming_email records. After already filtering by message_id and then addressed_to_user (which only returns incoming emails where the to, from, or cc address includes any of the user’s emails), we were filtering again but in the ruby code for the exact same conditions. After removing this all existing tests still pass.

diff --git a/app/models/email_log.rb b/app/models/email_log.rb
index 6cfbca4..33c8ea1 100644
--- a/app/models/email_log.rb
+++ b/app/models/email_log.rb
@@ -22,6 +22,17 @@ class EmailLog < ActiveRecord::Base
 
   scope :bounced, -> { where(bounced: true) }
 
+  scope :addressed_to_user, ->(user) do
+    where(<<~SQL, user_id: user.id)
+      EXISTS(
+        SELECT 1
+        FROM user_emails
+        WHERE user_emails.user_id = :user_id AND
+        email_logs.to_address = user_emails.email
+      )
+    SQL
+  end
+
   after_create do
     # Update last_emailed_at if the user_id is present and email was sent
     User.where(id: user_id).update_all("last_emailed_at = CURRENT_TIMESTAMP") if user_id.present?
diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb
index 5ec587e..da699c0 100644
--- a/lib/email/receiver.rb
+++ b/lib/email/receiver.rb
@@ -109,7 +109,7 @@ module Email
       # server (e.g. a message_id generated by Gmail) and does not need to
       # be updated, because message_ids from the IMAP server are not guaranteed
       # to be unique.
-      return unless discourse_generated_message_id?
+      return unless discourse_generated_message_id?(@message_id)
 
       incoming_email.update(
         imap_uid_validity: @opts[:imap_uid_validity],
@@ -745,23 +745,27 @@ module Email
 
     def create_group_post(group, user, body, elided)
       message_ids = Email::Receiver.extract_reply_message_ids(@mail, max_message_id_count: 5)
-      post_ids = []
 
+      # incoming emails with matching message ids, and then cross references
+      # these with any email addresses for the user vs to/from/cc of the
+      # incoming emails. in effect, any incoming email record for these
+      # message ids where the user is involved in any way will be returned
       incoming_emails = IncomingEmail.where(message_id: message_ids)
       if !group.allow_unknown_sender_topic_replies
         incoming_emails = incoming_emails.addressed_to_user(user)
       end
-
-      incoming_emails = incoming_emails.pluck(:post_id, :from_address, :to_addresses, :cc_addresses)
-
-      incoming_emails.each do |post_id, from_address, to_addresses, cc_addresses|
-        if group.allow_unknown_sender_topic_replies
-          post_ids << post_id
-        else
-          post_ids << post_id if contains_email_address_of_user?(from_address, user) ||
-            contains_email_address_of_user?(to_addresses, user) ||
-            contains_email_address_of_user?(cc_addresses, user)
-        end
+      post_ids = incoming_emails.pluck(:post_id) || []
+
+      # if the user is directly replying to an email send to them from discourse,
+      # there will be a corresponding EmailLog record, so we can use that as the
+      # reply post if it exists
+      if discourse_generated_message_id?(mail.in_reply_to)
+        post_id_from_email_log = EmailLog.where(message_id: mail.in_reply_to)
+          .addressed_to_user(user)
+          .order(created_at: :desc)
+          .limit(1)
+          .pluck(:post_id).last
+        post_ids << post_id_from_email_log
       end
 
       if post_ids.any? && post = Post.where(id: post_ids).order(:created_at).last
@@ -989,9 +993,9 @@ module Email
       @host ||= Email::Sender.host_for(Discourse.base_url)
     end
 
-    def discourse_generated_message_id?
-      !!(@message_id =~ message_id_post_id_regexp) ||
-        !!(@message_id =~ message_id_topic_id_regexp)
+    def discourse_generated_message_id?(message_id)
+      !!(message_id =~ message_id_post_id_regexp) ||
+        !!(message_id =~ message_id_topic_id_regexp)
     end
 
     def message_id_post_id_regexp
diff --git a/spec/components/email/receiver_spec.rb b/spec/components/email/receiver_spec.rb
index 636cc73..11a87b4 100644
--- a/spec/components/email/receiver_spec.rb
+++ b/spec/components/email/receiver_spec.rb
@@ -985,6 +985,95 @@ describe Email::Receiver do
       end
     end
 
+    context "emailing a group by email_username and following reply flow" do
+      let!(:topic) do
+        group.update!(email_username: "team@somesmtpaddress.com")
+        process(:email_to_group_email_username_1)
+        Topic.last
+      end
+      fab!(:user_in_group) do
+        u = Fabricate(:user)
+        Fabricate(:group_user, user: u, group: group)
+        u
+      end
+
+      before do
+        NotificationEmailer.enable
+        Jobs.run_immediately!
+      end
+
+      def reply_as_group_user
+        group_post = PostCreator.create(
+          user_in_group,
+          raw: "Thanks for your request. Please try to restart.",
+          topic_id: topic.id
+        )
+        email_log = EmailLog.last
+        [email_log, group_post]
+      end
+
+      it "the inbound processed email creates an incoming email and topic record correctly, and adds the group to the topic" do
+        incoming = IncomingEmail.find_by(topic: topic)
+        user = User.find_by_email("two@foo.com")
+        expect(topic.topic_allowed_users.first.user_id).to eq(user.id)
+        expect(topic.topic_allowed_groups.first.group_id).to eq(group.id)
+        expect(incoming.to_addresses).to eq("team@somesmtpaddress.com")
+        expect(incoming.from_address).to eq("two@foo.com")
+        expect(incoming.message_id).to eq("u4w8c9r4y984yh98r3h69873@foo.bar.mail")
+      end
+
+      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/#{topic.id}/#{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)
+        expect(IncomingEmail.exists?(post_id: group_post.id)).to eq(false)
+      end
+
+      it "processes a reply from the OP user to the group SMTP username, linking the reply_to_post_number correctly by
+      matching in_reply_to to the email log" do
+        email_log, group_post = reply_as_group_user
+
+        reply_email = email(:email_to_group_email_username_2)
+        reply_email.gsub!("MESSAGE_ID_REPLY_TO", email_log.message_id)
+        expect do
+          Email::Receiver.new(reply_email).process!
+        end.to change { Topic.count }.by(0).and change { Post.count }.by(1)
+
+        reply_post = Post.last
+        expect(reply_post.reply_to_user).to eq(user_in_group)
+        expect(reply_post.reply_to_post_number).to eq(group_post.post_number)
+      end
+
+      it "processes the reply from the user as a brand new topic if they have replied from a different address (e.g. auto forward) and allow_unknown_sender_topic_replies is disabled" do
+        email_log, group_post = reply_as_group_user
+
+        reply_email = email(:email_to_group_email_username_2_as_unknown_sender)
+        reply_email.gsub!("MESSAGE_ID_REPLY_TO", email_log.message_id)
+        expect do
+          Email::Receiver.new(reply_email).process!
+        end.to change { Topic.count }.by(1).and change { Post.count }.by(1)
+
+        reply_post = Post.last
+        expect(reply_post.topic_id).not_to eq(topic.id)
+      end
+
+      it "processes the reply from the user as a reply if they have replied from a different address (e.g. auto forward) and allow_unknown_sender_topic_replies is enabled" do
+        group.update!(allow_unknown_sender_topic_replies: true)
+        email_log, group_post = reply_as_group_user
+
+        reply_email = email(:email_to_group_email_username_2_as_unknown_sender)
+        reply_email.gsub!("MESSAGE_ID_REPLY_TO", email_log.message_id)
+        expect do
+          Email::Receiver.new(reply_email).process!
+        end.to change { Topic.count }.by(0).and change { Post.count }.by(1)
+
+        reply_post = Post.last
+        expect(reply_post.topic_id).to eq(topic.id)
+      end
+    end
+

[... diff too long, it was truncated ...]

GitHub sha: e9dc88a7b672dc9938b8f5f59058f501302870c2

This commit appears in #13339 which was approved by tgxworld. It was merged by martin.