Merge branch 'improve-email-threading' into 'master'

Improve email threading

This is a follow-up to !112 : it improves email threading, without changing the receiver of the emails.

* Subject of answers to an existing thread begins with `Re: ` (required by Mail.app)
* Send a 'In-Reply-To' header along the 'References' header

4 files changed, 82 insertions, 51 deletions

diff --git a/app/mailers/emails/issues.rb b/app/mailers/emails/issues.rb
index a096df9dc0d..e5346235963 100644
--- a/app/mailers/emails/issues.rb
+++ b/app/mailers/emails/issues.rb
@@ -4,10 +4,10 @@ module Emails
       @issue = Issue.find(issue_id)
       @project = @issue.project
       @target_url = project_issue_url(@project, @issue)
-      set_message_id("issue_#{issue_id}")
-      mail(from: sender(@issue.author_id),
-           to: recipient(recipient_id),
-           subject: subject("#{@issue.title} (##{@issue.iid})"))
+      mail_new_thread(@issue,
+                      from: sender(@issue.author_id),
+                      to: recipient(recipient_id),
+                      subject: subject("#{@issue.title} (##{@issue.iid})"))
     end
 
     def reassigned_issue_email(recipient_id, issue_id, previous_assignee_id, updated_by_user_id)
@@ -15,10 +15,10 @@ module Emails
       @previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id
       @project = @issue.project
       @target_url = project_issue_url(@project, @issue)
-      set_reference("issue_#{issue_id}")
-      mail(from: sender(updated_by_user_id),
-           to: recipient(recipient_id),
-           subject: subject("#{@issue.title} (##{@issue.iid})"))
+      mail_answer_thread(@issue,
+                         from: sender(updated_by_user_id),
+                         to: recipient(recipient_id),
+                         subject: subject("#{@issue.title} (##{@issue.iid})"))
     end
 
     def closed_issue_email(recipient_id, issue_id, updated_by_user_id)
@@ -26,10 +26,10 @@ module Emails
       @project = @issue.project
       @updated_by = User.find updated_by_user_id
       @target_url = project_issue_url(@project, @issue)
-      set_reference("issue_#{issue_id}")
-      mail(from: sender(updated_by_user_id),
-           to: recipient(recipient_id),
-           subject: subject("#{@issue.title} (##{@issue.iid})"))
+      mail_answer_thread(@issue,
+                         from: sender(updated_by_user_id),
+                         to: recipient(recipient_id),
+                         subject: subject("#{@issue.title} (##{@issue.iid})"))
     end
 
     def issue_status_changed_email(recipient_id, issue_id, status, updated_by_user_id)
@@ -38,10 +38,10 @@ module Emails
       @project = @issue.project
       @updated_by = User.find updated_by_user_id
       @target_url = project_issue_url(@project, @issue)
-      set_reference("issue_#{issue_id}")
-      mail(from: sender(updated_by_user_id),
-           to: recipient(recipient_id),
-           subject: subject("#{@issue.title} (##{@issue.iid})"))
+      mail_answer_thread(@issue,
+                         from: sender(updated_by_user_id),
+                         to: recipient(recipient_id),
+                         subject: subject("#{@issue.title} (##{@issue.iid})"))
     end
   end
 end
diff --git a/app/mailers/emails/merge_requests.rb b/app/mailers/emails/merge_requests.rb
index 2b5af09a576..935987e2ed1 100644
--- a/app/mailers/emails/merge_requests.rb
+++ b/app/mailers/emails/merge_requests.rb
@@ -4,10 +4,10 @@ module Emails
       @merge_request = MergeRequest.find(merge_request_id)
       @project = @merge_request.project
       @target_url = project_merge_request_url(@project, @merge_request)
-      set_message_id("merge_request_#{merge_request_id}")
-      mail(from: sender(@merge_request.author_id),
-           to: recipient(recipient_id),
-           subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
+      mail_new_thread(@merge_request,
+                      from: sender(@merge_request.author_id),
+                      to: recipient(recipient_id),
+                      subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
     end
 
     def reassigned_merge_request_email(recipient_id, merge_request_id, previous_assignee_id, updated_by_user_id)
@@ -15,10 +15,10 @@ module Emails
       @previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id
       @project = @merge_request.project
       @target_url = project_merge_request_url(@project, @merge_request)
-      set_reference("merge_request_#{merge_request_id}")
-      mail(from: sender(updated_by_user_id),
-           to: recipient(recipient_id),
-           subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
+      mail_answer_thread(@merge_request,
+                         from: sender(updated_by_user_id),
+                         to: recipient(recipient_id),
+                         subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
     end
 
     def closed_merge_request_email(recipient_id, merge_request_id, updated_by_user_id)
@@ -26,20 +26,20 @@ module Emails
       @updated_by = User.find updated_by_user_id
       @project = @merge_request.project
       @target_url = project_merge_request_url(@project, @merge_request)
-      set_reference("merge_request_#{merge_request_id}")
-      mail(from: sender(updated_by_user_id),
-           to: recipient(recipient_id),
-           subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
+      mail_answer_thread(@merge_request,
+                         from: sender(updated_by_user_id),
+                         to: recipient(recipient_id),
+                         subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
     end
 
     def merged_merge_request_email(recipient_id, merge_request_id, updated_by_user_id)
       @merge_request = MergeRequest.find(merge_request_id)
       @project = @merge_request.project
       @target_url = project_merge_request_url(@project, @merge_request)
-      set_reference("merge_request_#{merge_request_id}")
-      mail(from: sender(updated_by_user_id),
-           to: recipient(recipient_id),
-           subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
+      mail_answer_thread(@merge_request,
+                         from: sender(updated_by_user_id),
+                         to: recipient(recipient_id),
+                         subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
     end
 
     def merge_request_status_email(recipient_id, merge_request_id, status, updated_by_user_id)
diff --git a/app/mailers/emails/notes.rb b/app/mailers/emails/notes.rb
index b7fedbd3707..ef9af726a6c 100644
--- a/app/mailers/emails/notes.rb
+++ b/app/mailers/emails/notes.rb
@@ -5,9 +5,10 @@ module Emails
       @commit = @note.noteable
       @project = @note.project
       @target_url = project_commit_url(@project, @commit, anchor: "note_#{@note.id}")
-      mail(from: sender(@note.author_id),
-           to: recipient(recipient_id),
-           subject: subject("#{@commit.title} (#{@commit.short_id})"))
+      mail_answer_thread(@commit,
+                         from: sender(@note.author_id),
+                         to: recipient(recipient_id),
+                         subject: subject("#{@commit.title} (#{@commit.short_id})"))
     end
 
     def note_issue_email(recipient_id, note_id)
@@ -15,10 +16,10 @@ module Emails
       @issue = @note.noteable
       @project = @note.project
       @target_url = project_issue_url(@project, @issue, anchor: "note_#{@note.id}")
-      set_reference("issue_#{@issue.id}")
-      mail(from: sender(@note.author_id),
-           to: recipient(recipient_id),
-           subject: subject("#{@issue.title} (##{@issue.iid})"))
+      mail_answer_thread(@issue,
+                         from: sender(@note.author_id),
+                         to: recipient(recipient_id),
+                         subject: subject("#{@issue.title} (##{@issue.iid})"))
     end
 
     def note_merge_request_email(recipient_id, note_id)
@@ -26,10 +27,10 @@ module Emails
       @merge_request = @note.noteable
       @project = @note.project
       @target_url = project_merge_request_url(@project, @merge_request, anchor: "note_#{@note.id}")
-      set_reference("merge_request_#{@merge_request.id}")
-      mail(from: sender(@note.author_id),
-           to: recipient(recipient_id),
-           subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
+      mail_answer_thread(@merge_request,
+                         from: sender(@note.author_id),
+                         to: recipient(recipient_id),
+                         subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
     end
   end
 end
diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb
index 84a0da0129d..bd438bab89a 100644
--- a/app/mailers/notify.rb
+++ b/app/mailers/notify.rb
@@ -1,4 +1,6 @@
 class Notify < ActionMailer::Base
+  include ActionDispatch::Routing::PolymorphicRoutes
+
   include Emails::Issues
   include Emails::MergeRequests
   include Emails::Notes
@@ -53,14 +55,6 @@ class Notify < ActionMailer::Base
     end
   end
 
-  # Set the Message-ID header field
-  #
-  # local_part - The local part of the message ID
-  #
-  def set_message_id(local_part)
-    headers["Message-ID"] = "<#{local_part}@#{Gitlab.config.gitlab.host}>"
-  end
-
   # Set the References header field
   #
   # local_part - The local part of the referenced message ID
@@ -93,4 +87,40 @@ class Notify < ActionMailer::Base
     subject << extra.join(' | ') if extra.present?
     subject
   end
+
+  # Return a string suitable for inclusion in the 'Message-Id' mail header.
+  #
+  # The message-id is generated from the unique URL to a model object.
+  def message_id(model)
+    model_name = model.class.model_name.singular_route_key
+    "<#{model_name}_#{model.id}@#{Gitlab.config.gitlab.host}>"
+  end
+
+  # Send an email that starts a new conversation thread,
+  # with headers suitable for grouping by thread in email clients.
+  #
+  # See: mail_answer_thread
+  def mail_new_thread(model, headers = {}, &block)
+    headers['Message-ID'] = message_id(model)
+    mail(headers, &block)
+  end
+
+  # Send an email that responds to an existing conversation thread,
+  # with headers suitable for grouping by thread in email clients.
+  #
+  # For grouping emails by thread, email clients heuristics require the answers to:
+  #
+  #  * have a subject that begin by 'Re: '
+  #  * have a 'In-Reply-To' or 'References' header that references the original 'Message-ID'
+  #
+  def mail_answer_thread(model, headers = {}, &block)
+    headers['In-Reply-To'] = message_id(model)
+    headers['References'] = message_id(model)
+
+    if (headers[:subject])
+      headers[:subject].prepend('Re: ')
+    end
+
+    mail(headers, &block)
+  end
 end