Merge pull request #384 from triadgit/fix/thread-on-in-reply-to

fix: thread an IMAP message on In-Reply-To only, never on the sender's Reply-To

internal/app/worker/wmail/sync_imap.go

@@ -326,18 +326,33 @@ func (w *WMail) imapApply(ctx context.Context, fetched []*imap.Fetched, backfill
 	return all, nil
 }
 
+// threadParentID is the message this one answers, and the key its thread is
+// built on. Only In-Reply-To carries that.
+//
+// Reply-To must not be used here. It is an address header -- "send replies to
+// this mailbox" -- not a message identifier, so keying a thread on it puts
+// every message a sender ever sent into one strand. On a production instance
+// that collapsed 484 of 1431 stored messages into 62 threads: 76 unrelated
+// DMARC aggregate reports from one reporter arrived as a single 76-message
+// conversation, and a mailbox's own test sends and live outreach merged
+// together.
+//
+// A message that answers nothing has no parent, and the caller roots its
+// thread on its own Message-ID.
+func threadParentID(msg *models.EmailMessageData) string {
+	if msg == nil || len(msg.InReplyTo) == 0 {
+		return ""
+	}
+	return msg.InReplyTo[len(msg.InReplyTo)-1]
+}
+
 // imapStore threads a new message and hands it to storeNew.
 func (w *WMail) imapStore(ctx context.Context, msg *models.EmailMessageData) error {
 	msg.ID = uuid.New()
 	now := time.Now()
 
 	var threadID string
-	var parentID string
-	if len(msg.InReplyTo) > 0 {
-		parentID = msg.InReplyTo[len(msg.InReplyTo)-1]
-	} else if len(msg.ReplyTo) > 0 {
-		parentID = msg.ReplyTo[len(msg.ReplyTo)-1]
-	}
+	parentID := threadParentID(msg)
 
 	if parentID != "" {
 		internalParent, _ := w.EmailMessageMapRepository.Get(ctx, w.UserID, w.ID, parentID)

internal/app/worker/wmail/thread_parent_test.go

@@ -0,0 +1,80 @@
+package wmail
+
+import (
+	"testing"
+
+	"github.com/warmbly/warmbly/internal/models"
+)
+
+// threadParentID decides which conversation an inbound message joins. A wrong
+// answer does not lose the message, it files it under the wrong conversation,
+// which is harder to notice and worse to work in.
+//
+// The case this pins: Reply-To as a fallback parent. It is an address header,
+// not a message identifier, so keying on it puts every message a sender ever
+// sent into one thread.
+func TestThreadParentIDIgnoresReplyTo(t *testing.T) {
+	cases := []struct {
+		name string
+		msg  *models.EmailMessageData
+		want string
+	}{
+		{
+			name: "a genuine reply threads on In-Reply-To",
+			msg:  &models.EmailMessageData{InReplyTo: []string{"<parent@example.com>"}},
+			want: "<parent@example.com>",
+		},
+		{
+			name: "the last In-Reply-To wins on a deep chain",
+			msg: &models.EmailMessageData{InReplyTo: []string{
+				"<root@example.com>", "<latest@example.com>",
+			}},
+			want: "<latest@example.com>",
+		},
+		{
+			name: "Reply-To alone is not a parent",
+			msg:  &models.EmailMessageData{ReplyTo: []string{"reports@example.net"}},
+			want: "",
+		},
+		{
+			name: "Reply-To never overrides In-Reply-To",
+			msg: &models.EmailMessageData{
+				InReplyTo: []string{"<parent@example.com>"},
+				ReplyTo:   []string{"someone@example.net"},
+			},
+			want: "<parent@example.com>",
+		},
+		{
+			name: "a message that answers nothing has no parent",
+			msg:  &models.EmailMessageData{},
+			want: "",
+		},
+		{
+			name: "nil is not a panic",
+			msg:  nil,
+			want: "",
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			if got := threadParentID(tc.msg); got != tc.want {
+				t.Errorf("threadParentID() = %q, want %q", got, tc.want)
+			}
+		})
+	}
+}
+
+// Two unrelated messages from one sender must not share a parent, which is the
+// shape of the bug: same Reply-To, different conversations.
+func TestThreadParentIDKeepsUnrelatedMessagesApart(t *testing.T) {
+	sender := []string{"Aggregate Reports <reports@example.net>"}
+	first := &models.EmailMessageData{MessageID: "<a@example.net>", ReplyTo: sender}
+	second := &models.EmailMessageData{MessageID: "<b@example.net>", ReplyTo: sender}
+
+	if p := threadParentID(first); p != "" {
+		t.Fatalf("first message got parent %q, want none", p)
+	}
+	if p := threadParentID(second); p != "" {
+		t.Fatalf("second message got parent %q, want none", p)
+	}
+}