From de90b572d8f22708ea76ffbea9c513143fdeea2e Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Mon, 24 Feb 2014 12:12:29 +0100 Subject: [PATCH] Allow more mail clients to group emails by thread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * send a ‘In-Reply-To’ header along the ‘References’ header * subject of answers to an existing thread begins with ‘Re: ’ This fixes threading with at least Mail.app and Airmail. --- app/mailers/emails/issues.rb | 32 ++++++++--------- app/mailers/emails/merge_requests.rb | 32 ++++++++--------- app/mailers/emails/notes.rb | 23 ++++++------ app/mailers/notify.rb | 52 +++++++++++++++++++++++----- spec/mailers/notify_spec.rb | 45 +++++++++++++++--------- 5 files changed, 116 insertions(+), 68 deletions(-) diff --git a/app/mailers/emails/issues.rb b/app/mailers/emails/issues.rb index 50676ac10cb..516e3da84c8 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), - cc: recipient(recipient_id), - subject: subject("#{@issue.title} (##{@issue.iid})")) + mail_new_thread(@issue, + from: sender(@issue.author_id), + cc: 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), - cc: recipient(recipient_id), - subject: subject("#{@issue.title} (##{@issue.iid})")) + mail_answer_thread(@issue, + from: sender(updated_by_user_id), + cc: 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), - cc: recipient(recipient_id), - subject: subject("#{@issue.title} (##{@issue.iid})")) + mail_answer_thread(@issue, + from: sender(updated_by_user_id), + cc: 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), - cc: recipient(recipient_id), - subject: subject("#{@issue.title} (##{@issue.iid})")) + mail_answer_thread(@issue, + from: sender(updated_by_user_id), + cc: 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 6b46cc90e08..c4ca3a3b69b 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), - cc: recipient(recipient_id), - subject: subject("#{@merge_request.title} (##{@merge_request.iid})")) + mail_new_thread(@merge_request, + from: sender(@merge_request.author_id), + cc: 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), - cc: recipient(recipient_id), - subject: subject("#{@merge_request.title} (##{@merge_request.iid})")) + mail_answer_thread(@merge_request, + from: sender(updated_by_user_id), + cc: 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), - cc: recipient(recipient_id), - subject: subject("#{@merge_request.title} (##{@merge_request.iid})")) + mail_answer_thread(@merge_request, + from: sender(updated_by_user_id), + cc: 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), - cc: recipient(recipient_id), - subject: subject("#{@merge_request.title} (##{@merge_request.iid})")) + mail_answer_thread(@merge_request, + from: sender(updated_by_user_id), + cc: recipient(recipient_id), + subject: subject("#{@merge_request.title} (##{@merge_request.iid})")) end end diff --git a/app/mailers/emails/notes.rb b/app/mailers/emails/notes.rb index d6ab63fe68c..7848d34ab2b 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), - cc: recipient(recipient_id), - subject: subject("#{@commit.title} (#{@commit.short_id})")) + mail_answer_thread(@commit, + from: sender(@note.author_id), + cc: 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), - cc: recipient(recipient_id), - subject: subject("#{@issue.title} (##{@issue.iid})")) + mail_answer_thread(@issue, + from: sender(@note.author_id), + cc: 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), - cc: recipient(recipient_id), - subject: subject("#{@merge_request.title} (##{@merge_request.iid})")) + mail_answer_thread(@merge_request, + from: sender(@note.author_id), + cc: recipient(recipient_id), + subject: subject("#{@merge_request.title} (##{@merge_request.iid})")) end def note_wall_email(recipient_id, note_id) diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb index 8eea8cf25e0..d65bead9cfe 100644 --- a/app/mailers/notify.rb +++ b/app/mailers/notify.rb @@ -67,14 +67,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 @@ -107,4 +99,48 @@ 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) + raise ArgumentError, '"To:" header will be overwritten; use "Cc:" or "Bcc:"' unless headers[:to].nil? + headers[:to] = project_sender_address.format + + 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' + # * have stable 'From' and 'To' headers between messages of the same thread + # + def mail_answer_thread(model, headers = {}, &block) + raise ArgumentError, '"To:" header will be overwritten; use "Cc:" or "Bcc:"' unless headers[:to].nil? + headers[:to] = project_sender_address.format + + 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 diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 6725e350898..32ba6c9b863 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -22,6 +22,23 @@ describe Notify do end end + shared_examples 'an email starting a new thread' do |message_id_prefix| + it 'has a discussion identifier' do + should have_header 'Message-ID', /<#{message_id_prefix}(.*)@#{Gitlab.config.gitlab.host}>/ + end + end + + shared_examples 'an answer to an existing thread' do |thread_id_prefix| + it 'has a subject that begins with Re: ' do + should have_subject /^Re: / + end + + it 'has headers that reference an existing thread' do + should have_header 'References', /<#{thread_id_prefix}(.*)@#{Gitlab.config.gitlab.host}>/ + should have_header 'In-Reply-To', /<#{thread_id_prefix}(.*)@#{Gitlab.config.gitlab.host}>/ + end + end + describe 'for new users, the email' do let(:example_site_path) { root_path } let(:new_user) { create(:user, email: 'newguy@example.com', created_by_id: 1) } @@ -153,6 +170,7 @@ describe Notify do subject { Notify.new_issue_email(issue.assignee_id, issue.id) } it_behaves_like 'an assignee email' + it_behaves_like 'an email starting a new thread', 'issue' it 'has the correct subject' do should have_subject /#{project.name} \| #{issue.title} \(##{issue.iid}\)/ @@ -161,10 +179,6 @@ describe Notify do it 'contains a link to the new issue' do should have_body_text /#{project_issue_path project, issue}/ end - - it 'has the correct message-id set' do - should have_header 'Message-ID', "" - end end describe 'that are new with a description' do @@ -179,6 +193,7 @@ describe Notify do subject { Notify.reassigned_issue_email(recipient.id, issue.id, previous_assignee.id, current_user) } it_behaves_like 'a multiple recipients email' + it_behaves_like 'an answer to an existing thread', 'issue' it 'is sent as the author' do sender = subject.header[:from].addrs[0] @@ -201,16 +216,14 @@ describe Notify do it 'contains a link to the issue' do should have_body_text /#{project_issue_path project, issue}/ end - - it 'has the correct reference set' do - should have_header 'References', "" - end end describe 'status changed' do let(:status) { 'closed' } subject { Notify.issue_status_changed_email(recipient.id, issue.id, status, current_user) } + it_behaves_like 'an answer to an existing thread', 'issue' + it 'is sent as the author' do sender = subject.header[:from].addrs[0] sender.display_name.should eq(current_user.name) @@ -232,10 +245,6 @@ describe Notify do it 'contains a link to the issue' do should have_body_text /#{project_issue_path project, issue}/ end - - it 'has the correct reference set' do - should have_header 'References', "" - end end end @@ -249,6 +258,7 @@ describe Notify do subject { Notify.new_merge_request_email(merge_request.assignee_id, merge_request.id) } it_behaves_like 'an assignee email' + it_behaves_like 'an email starting a new thread', 'merge_request' it 'has the correct subject' do should have_subject /#{merge_request.title} \(##{merge_request.iid}\)/ @@ -283,6 +293,7 @@ describe Notify do subject { Notify.reassigned_merge_request_email(recipient.id, merge_request.id, previous_assignee.id, current_user.id) } it_behaves_like 'a multiple recipients email' + it_behaves_like 'an answer to an existing thread', 'merge_request' it 'is sent as the author' do sender = subject.header[:from].addrs[0] @@ -311,6 +322,7 @@ describe Notify do subject { Notify.merged_merge_request_email(recipient.id, merge_request.id, merge_author.id) } it_behaves_like 'a multiple recipients email' + it_behaves_like 'an answer to an existing thread', 'merge_request' it 'is sent as the merge author' do sender = subject.header[:from].addrs[0] @@ -329,10 +341,6 @@ describe Notify do it 'contains a link to the merge request' do should have_body_text /#{project_merge_request_path project, merge_request}/ end - - it 'has the correct reference set' do - should have_header 'References', "" - end end end end @@ -426,6 +434,7 @@ describe Notify do subject { Notify.note_commit_email(recipient.id, note.id) } it_behaves_like 'a note email' + it_behaves_like 'an answer to an existing thread', 'commits' it 'has the correct subject' do should have_subject /#{commit.title} \(#{commit.short_id}\)/ @@ -444,6 +453,7 @@ describe Notify do subject { Notify.note_merge_request_email(recipient.id, note.id) } it_behaves_like 'a note email' + it_behaves_like 'an answer to an existing thread', 'merge_request' it 'has the correct subject' do should have_subject /#{merge_request.title} \(##{merge_request.iid}\)/ @@ -462,6 +472,7 @@ describe Notify do subject { Notify.note_issue_email(recipient.id, note.id) } it_behaves_like 'a note email' + it_behaves_like 'an answer to an existing thread', 'issue' it 'has the correct subject' do should have_subject /#{issue.title} \(##{issue.iid}\)/ @@ -574,7 +585,7 @@ describe Notify do end it 'is sent to recipient' do - should deliver_to 'devs@company.name' + should cc_to 'devs@company.name' end it 'has the correct subject' do