FIX: Link up reply to post correctly when emailing group (PR #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.


I removed this completely because, unless I am gravely mistaken, it is doing the exact same thing as this:

This is the relevant test to check the reply_to_user and reply_to_post_number is correct. I added a whole flow test to make sure it was all working correctly, in my head and in the code, rather than just testing for this one specific case.

Isn’t email_logs.to_address an email here? Why do we need to use an ILIKE matcher here?

I think we should play it safe here and specify the ordering here since there is no guarantee that the .last will return the EmailLog we want.

Minor perf concern here. Is it possible to just pluck the post_id so we don’t have to load the entire ActiveRecord object?

Email logs are cleaned up after 90 days by default, was wondering if that will affect the logic here.

        group.update!(email_username: "")

Are we able to use fab! here?

I think the naming is confusing here because group_user it is the name for the join table as well. Maybe just user or user_with_group?

        Fabricate(:user).tap do |u|
          Fabricate(:group_user, user: u, group: group)

I find it hard to understand stand this test because I can’t seem to tell what the subject of this test is. We query for two records and then assert against those two records.

        expect(IncomingEmail.exists?(post_id: eq(false)

Should we also assert that a new post is created here?