From 2acf3a564c4d042b4cf5463867bd5d37723509f5 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Fri, 15 Sep 2017 08:58:21 +0200 Subject: Make mail notifications of discussion notes In-Reply-To of each other When a note is part of a discussion, the email sent out should be `In-Reply-To` the previous note in that discussion. Closes gitlab-org/gitlab-ce#36054 --- app/mailers/emails/notes.rb | 10 ++++----- app/mailers/notify.rb | 12 +++++++++++ app/models/note.rb | 9 ++++++++ .../unreleased/tc-correct-email-in-reply-to.yml | 5 +++++ spec/models/note_spec.rb | 25 ++++++++++++++++++++++ 5 files changed, 56 insertions(+), 5 deletions(-) create mode 100644 changelogs/unreleased/tc-correct-email-in-reply-to.yml diff --git a/app/mailers/emails/notes.rb b/app/mailers/emails/notes.rb index 77a82b895ce..50e17fe7717 100644 --- a/app/mailers/emails/notes.rb +++ b/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 diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb index 9efabe3f44e..250583d2d28 100644 --- a/app/mailers/notify.rb +++ b/app/mailers/notify.rb @@ -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.replies_to) + headers['References'] = message_id(model) + + 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 diff --git a/app/models/note.rb b/app/models/note.rb index 02f9fd61e49..ea31bc45c61 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -360,6 +360,15 @@ class Note < ActiveRecord::Base end end + def replies_to + if part_of_discussion? + previous_note = discussion.notes.take_while { |n| n.id < id }.last + return previous_note if previous_note + end + + noteable + end + def expire_etag_cache return unless noteable&.discussions_rendered_on_frontend? diff --git a/changelogs/unreleased/tc-correct-email-in-reply-to.yml b/changelogs/unreleased/tc-correct-email-in-reply-to.yml new file mode 100644 index 00000000000..1c8043f6a5c --- /dev/null +++ b/changelogs/unreleased/tc-correct-email-in-reply-to.yml @@ -0,0 +1,5 @@ +--- +title: Make mail notifications of discussion notes In-Reply-To of each other +merge_request: 14289 +author: +type: changed diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index e1a0c55b6a6..cf79ecf00c2 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -756,6 +756,31 @@ describe Note do end end + describe '#replies_to' do + context 'when part of a discussion' do + let(:note) { create(:discussion_note_on_issue) } + + 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) + 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.replies_to).to eq(note.noteable) + end + end + end + describe 'expiring ETag cache' do let(:note) { build(:note_on_issue) } -- cgit v1.2.3 From f55aaca561986757343fb410a9e6c09cfcc61385 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Fri, 8 Dec 2017 22:45:57 +0100 Subject: Make discussion mail References all notes in the discussion 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. --- app/mailers/notify.rb | 8 ++++---- app/models/note.rb | 9 +++++---- spec/mailers/notify_spec.rb | 40 ++++++++++++++++++++++++++++++++++++++++ spec/models/note_spec.rb | 19 ++++++++----------- 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 = "".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 -- cgit v1.2.3