Merge branch 'tc-correct-email-in-reply-to' into 'master'

Make mail notifications of discussion notes In-Reply-To of each other

Closes #36054

See merge request gitlab-org/gitlab-ce!14289

app/mailers/emails/notes.rb

@@ -5,7 +5,7 @@ module Emails
 
       @commit = @note.noteable
       @target_url = project_commit_url(*note_target_url_options)
-      mail_answer_thread(@commit, note_thread_options(recipient_id))
+      mail_answer_note_thread(@commit, @note, note_thread_options(recipient_id))
     end
 
     def note_issue_email(recipient_id, note_id)
@@ -13,7 +13,7 @@ module Emails
 
       @issue = @note.noteable
       @target_url = project_issue_url(*note_target_url_options)
-      mail_answer_thread(@issue, note_thread_options(recipient_id))
+      mail_answer_note_thread(@issue, @note, note_thread_options(recipient_id))
     end
 
     def note_merge_request_email(recipient_id, note_id)
@@ -21,7 +21,7 @@ module Emails
 
       @merge_request = @note.noteable
       @target_url = project_merge_request_url(*note_target_url_options)
-      mail_answer_thread(@merge_request, note_thread_options(recipient_id))
+      mail_answer_note_thread(@merge_request, @note, note_thread_options(recipient_id))
     end
 
     def note_snippet_email(recipient_id, note_id)
@@ -29,7 +29,7 @@ module Emails
 
       @snippet = @note.noteable
       @target_url = project_snippet_url(*note_target_url_options)
-      mail_answer_thread(@snippet, note_thread_options(recipient_id))
+      mail_answer_note_thread(@snippet, @note, note_thread_options(recipient_id))
     end
 
     def note_personal_snippet_email(recipient_id, note_id)
@@ -37,7 +37,7 @@ module Emails
 
       @snippet = @note.noteable
       @target_url = snippet_url(@note.noteable)
-      mail_answer_thread(@snippet, note_thread_options(recipient_id))
+      mail_answer_note_thread(@snippet, @note, note_thread_options(recipient_id))
     end
 
     private

app/mailers/notify.rb

@@ -119,8 +119,8 @@ class Notify < BaseMailer
       headers['Reply-To'] = address
 
       fallback_reply_message_id = "<reply-#{reply_key}@#{Gitlab.config.gitlab.host}>".freeze
-      headers['References'] ||= ''
-      headers['References'] << ' ' << fallback_reply_message_id
+      headers['References'] ||= []
+      headers['References'] << fallback_reply_message_id
 
       @reply_by_email = true
     end
@@ -156,6 +156,18 @@ class Notify < BaseMailer
     mail_thread(model, headers)
   end
 
+  def mail_answer_note_thread(model, note, headers = {})
+    headers['Message-ID'] = message_id(note)
+    headers['In-Reply-To'] = message_id(note.references.last)
+    headers['References'] = note.references.map { |ref| message_id(ref) }
+
+    headers['X-GitLab-Discussion-ID'] = note.discussion.id if note.part_of_discussion?
+
+    headers[:subject]&.prepend('Re: ')
+
+    mail_thread(model, headers)
+  end
+
   def reply_key
     @reply_key ||= SentNotification.reply_key
   end

app/models/note.rb

@@ -360,6 +360,16 @@ class Note < ActiveRecord::Base
     end
   end
 
+  def references
+    refs = [noteable]
+
+    if part_of_discussion?
+      refs += discussion.notes.take_while { |n| n.id < id }
+    end
+
+    refs
+  end
+
   def expire_etag_cache
     return unless noteable&.discussions_rendered_on_frontend?
 

changelogs/unreleased/tc-correct-email-in-reply-to.yml

+---
+title: Make mail notifications of discussion notes In-Reply-To of each other
+merge_request: 14289
+author:
+type: changed

spec/mailers/notify_spec.rb

@@ -342,6 +342,46 @@ describe Notify do
       end
     end
 
+    context 'for issue notes' do
+      let(:host) { Gitlab.config.gitlab.host }
+
+      context 'in discussion' do
+        set(:first_note) { create(:discussion_note_on_issue) }
+        set(:second_note) { create(:discussion_note_on_issue, in_reply_to: first_note) }
+        set(:third_note) { create(:discussion_note_on_issue, in_reply_to: second_note) }
+
+        subject { described_class.note_issue_email(recipient.id, third_note.id) }
+
+        it 'has In-Reply-To header pointing to previous note in discussion' do
+          expect(subject.header['In-Reply-To'].message_ids).to eq(["note_#{second_note.id}@#{host}"])
+        end
+
+        it 'has References header including the notes and issue of the discussion' do
+          expect(subject.header['References'].message_ids).to include("issue_#{first_note.noteable.id}@#{host}",
+                                                                   "note_#{first_note.id}@#{host}",
+                                                                   "note_#{second_note.id}@#{host}")
+        end
+
+        it 'has X-GitLab-Discussion-ID header' do
+          expect(subject.header['X-GitLab-Discussion-ID'].value).to eq(third_note.discussion.id)
+        end
+      end
+
+      context 'individual issue comments' do
+        set(:note) { create(:note_on_issue) }
+
+        subject { described_class.note_issue_email(recipient.id, note.id) }
+
+        it 'has In-Reply-To header pointing to the issue' do
+          expect(subject.header['In-Reply-To'].message_ids).to eq(["issue_#{note.noteable.id}@#{host}"])
+        end
+
+        it 'has References header including the notes and issue of the discussion' do
+          expect(subject.header['References'].message_ids).to include("issue_#{note.noteable.id}@#{host}")
+        end
+      end
+    end
+
     context 'for snippet notes' do
       let(:project_snippet) { create(:project_snippet, project: project) }
       let(:project_snippet_note) { create(:note_on_project_snippet, project: project, noteable: project_snippet) }

spec/models/note_spec.rb

@@ -756,6 +756,28 @@ describe Note do
     end
   end
 
+  describe '#references' do
+    context 'when part of a discussion' do
+      it 'references all earlier notes in the discussion' do
+        first_note = create(:discussion_note_on_issue)
+        second_note = create(:discussion_note_on_issue, in_reply_to: first_note)
+        third_note = create(:discussion_note_on_issue, in_reply_to: second_note)
+        create(:discussion_note_on_issue, in_reply_to: third_note)
+
+        expect(third_note.references).to eq([first_note.noteable, first_note, second_note])
+      end
+    end
+
+    context 'when not part of a discussion' do
+      subject { create(:note) }
+      let(:note) { create(:note, in_reply_to: subject) }
+
+      it 'returns the noteable' do
+        expect(note.references).to eq([note.noteable])
+      end
+    end
+  end
+
   describe 'expiring ETag cache' do
     let(:note) { build(:note_on_issue) }