summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorToon Claes <toon@iotcl.com>2017-12-08 22:45:57 +0100
committerToon Claes <toon@iotcl.com>2017-12-13 21:26:01 +0100
commitf55aaca561986757343fb410a9e6c09cfcc61385 (patch)
tree1c84b54d8cefa3af4768af4cf31740107f188cb2
parent2acf3a564c4d042b4cf5463867bd5d37723509f5 (diff)
downloadgitlab-ce-f55aaca561986757343fb410a9e6c09cfcc61385.tar.gz
Make discussion mail References all notes in the discussiontc-correct-email-in-reply-to
When a note is part of a discussion, the email sent out will be `In-Reply-To` the previous note in that discussion. It also `References` all the previous notes in that discussion, and the original issue. Closes gitlab-org/gitlab-ce#36054.
-rw-r--r--app/mailers/notify.rb8
-rw-r--r--app/models/note.rb9
-rw-r--r--spec/mailers/notify_spec.rb40
-rw-r--r--spec/models/note_spec.rb19
4 files changed, 57 insertions, 19 deletions
diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb
index 250583d2d28..ec886e993c3 100644
--- a/app/mailers/notify.rb
+++ b/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
@@ -158,8 +158,8 @@ class Notify < BaseMailer
def mail_answer_note_thread(model, note, headers = {})
headers['Message-ID'] = message_id(note)
- headers['In-Reply-To'] = message_id(note.replies_to)
- headers['References'] = message_id(model)
+ 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?
diff --git a/app/models/note.rb b/app/models/note.rb
index ea31bc45c61..184fbd5f5ae 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -360,13 +360,14 @@ class Note < ActiveRecord::Base
end
end
- def replies_to
+ def references
+ refs = [noteable]
+
if part_of_discussion?
- previous_note = discussion.notes.take_while { |n| n.id < id }.last
- return previous_note if previous_note
+ refs += discussion.notes.take_while { |n| n.id < id }
end
- noteable
+ refs
end
def expire_etag_cache
diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb
index e1d71a9573b..4d0a3942996 100644
--- a/spec/mailers/notify_spec.rb
+++ b/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) }
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index cf79ecf00c2..cefbf60b28c 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -756,18 +756,15 @@ describe Note do
end
end
- describe '#replies_to' do
+ describe '#references' do
context 'when part of a discussion' do
- let(:note) { create(:discussion_note_on_issue) }
+ 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)
- it 'returns noteable when there are not earlier notes in the discussion' do
- expect(note.replies_to).to eq(note.noteable)
- end
-
- it 'returns previous note in discussion' do
- reply = create(:discussion_note_on_issue, in_reply_to: note)
-
- expect(reply.replies_to).to eq(note)
+ expect(third_note.references).to eq([first_note.noteable, first_note, second_note])
end
end
@@ -776,7 +773,7 @@ describe Note do
let(:note) { create(:note, in_reply_to: subject) }
it 'returns the noteable' do
- expect(note.replies_to).to eq(note.noteable)
+ expect(note.references).to eq([note.noteable])
end
end
end