Download as
Browse Code »

Commit de90b572d8f22708ea76ffbea9c513143fdeea2e

Authored by Pierre de La Morinerie
1 parent 466b768b
Exists in spb-stable and in 2 other branches 7-0-stable, 7-0-stable-spb

Allow more mail clients to group emails by thread 

* 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.
Showing 5 changed files with 116 additions and 68 deletions   Show diff stats
app/mailers/emails/issues.rb
  Diff comments   View file @de90b57
... ... @@ -4,10 +4,10 @@ module Emails
4 4 @issue = Issue.find(issue_id)
5 5 @project = @issue.project
6 6 @target_url = project_issue_url(@project, @issue)
7   - set_message_id("issue_#{issue_id}")
8   - mail(from: sender(@issue.author_id),
9   - cc: recipient(recipient_id),
10   - subject: subject("#{@issue.title} (##{@issue.iid})"))
  7 + mail_new_thread(@issue,
  8 + from: sender(@issue.author_id),
  9 + cc: recipient(recipient_id),
  10 + subject: subject("#{@issue.title} (##{@issue.iid})"))
11 11 end
12 12  
13 13 def reassigned_issue_email(recipient_id, issue_id, previous_assignee_id, updated_by_user_id)
... ... @@ -15,10 +15,10 @@ module Emails
15 15 @previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id
16 16 @project = @issue.project
17 17 @target_url = project_issue_url(@project, @issue)
18   - set_reference("issue_#{issue_id}")
19   - mail(from: sender(updated_by_user_id),
20   - cc: recipient(recipient_id),
21   - subject: subject("#{@issue.title} (##{@issue.iid})"))
  18 + mail_answer_thread(@issue,
  19 + from: sender(updated_by_user_id),
  20 + cc: recipient(recipient_id),
  21 + subject: subject("#{@issue.title} (##{@issue.iid})"))
22 22 end
23 23  
24 24 def closed_issue_email(recipient_id, issue_id, updated_by_user_id)
... ... @@ -26,10 +26,10 @@ module Emails
26 26 @project = @issue.project
27 27 @updated_by = User.find updated_by_user_id
28 28 @target_url = project_issue_url(@project, @issue)
29   - set_reference("issue_#{issue_id}")
30   - mail(from: sender(updated_by_user_id),
31   - cc: recipient(recipient_id),
32   - subject: subject("#{@issue.title} (##{@issue.iid})"))
  29 + mail_answer_thread(@issue,
  30 + from: sender(updated_by_user_id),
  31 + cc: recipient(recipient_id),
  32 + subject: subject("#{@issue.title} (##{@issue.iid})"))
33 33 end
34 34  
35 35 def issue_status_changed_email(recipient_id, issue_id, status, updated_by_user_id)
... ... @@ -38,10 +38,10 @@ module Emails
38 38 @project = @issue.project
39 39 @updated_by = User.find updated_by_user_id
40 40 @target_url = project_issue_url(@project, @issue)
41   - set_reference("issue_#{issue_id}")
42   - mail(from: sender(updated_by_user_id),
43   - cc: recipient(recipient_id),
44   - subject: subject("#{@issue.title} (##{@issue.iid})"))
  41 + mail_answer_thread(@issue,
  42 + from: sender(updated_by_user_id),
  43 + cc: recipient(recipient_id),
  44 + subject: subject("#{@issue.title} (##{@issue.iid})"))
45 45 end
46 46 end
47 47 end
... ...
app/mailers/emails/merge_requests.rb
  Diff comments   View file @de90b57
... ... @@ -4,10 +4,10 @@ module Emails
4 4 @merge_request = MergeRequest.find(merge_request_id)
5 5 @project = @merge_request.project
6 6 @target_url = project_merge_request_url(@project, @merge_request)
7   - set_message_id("merge_request_#{merge_request_id}")
8   - mail(from: sender(@merge_request.author_id),
9   - cc: recipient(recipient_id),
10   - subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
  7 + mail_new_thread(@merge_request,
  8 + from: sender(@merge_request.author_id),
  9 + cc: recipient(recipient_id),
  10 + subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
11 11 end
12 12  
13 13 def reassigned_merge_request_email(recipient_id, merge_request_id, previous_assignee_id, updated_by_user_id)
... ... @@ -15,10 +15,10 @@ module Emails
15 15 @previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id
16 16 @project = @merge_request.project
17 17 @target_url = project_merge_request_url(@project, @merge_request)
18   - set_reference("merge_request_#{merge_request_id}")
19   - mail(from: sender(updated_by_user_id),
20   - cc: recipient(recipient_id),
21   - subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
  18 + mail_answer_thread(@merge_request,
  19 + from: sender(updated_by_user_id),
  20 + cc: recipient(recipient_id),
  21 + subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
22 22 end
23 23  
24 24 def closed_merge_request_email(recipient_id, merge_request_id, updated_by_user_id)
... ... @@ -26,20 +26,20 @@ module Emails
26 26 @updated_by = User.find updated_by_user_id
27 27 @project = @merge_request.project
28 28 @target_url = project_merge_request_url(@project, @merge_request)
29   - set_reference("merge_request_#{merge_request_id}")
30   - mail(from: sender(updated_by_user_id),
31   - cc: recipient(recipient_id),
32   - subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
  29 + mail_answer_thread(@merge_request,
  30 + from: sender(updated_by_user_id),
  31 + cc: recipient(recipient_id),
  32 + subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
33 33 end
34 34  
35 35 def merged_merge_request_email(recipient_id, merge_request_id, updated_by_user_id)
36 36 @merge_request = MergeRequest.find(merge_request_id)
37 37 @project = @merge_request.project
38 38 @target_url = project_merge_request_url(@project, @merge_request)
39   - set_reference("merge_request_#{merge_request_id}")
40   - mail(from: sender(updated_by_user_id),
41   - cc: recipient(recipient_id),
42   - subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
  39 + mail_answer_thread(@merge_request,
  40 + from: sender(updated_by_user_id),
  41 + cc: recipient(recipient_id),
  42 + subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
43 43 end
44 44 end
45 45  
... ...
app/mailers/emails/notes.rb
  Diff comments   View file @de90b57
... ... @@ -5,9 +5,10 @@ module Emails
5 5 @commit = @note.noteable
6 6 @project = @note.project
7 7 @target_url = project_commit_url(@project, @commit, anchor: "note_#{@note.id}")
8   - mail(from: sender(@note.author_id),
9   - cc: recipient(recipient_id),
10   - subject: subject("#{@commit.title} (#{@commit.short_id})"))
  8 + mail_answer_thread(@commit,
  9 + from: sender(@note.author_id),
  10 + cc: recipient(recipient_id),
  11 + subject: subject("#{@commit.title} (#{@commit.short_id})"))
11 12 end
12 13  
13 14 def note_issue_email(recipient_id, note_id)
... ... @@ -15,10 +16,10 @@ module Emails
15 16 @issue = @note.noteable
16 17 @project = @note.project
17 18 @target_url = project_issue_url(@project, @issue, anchor: "note_#{@note.id}")
18   - set_reference("issue_#{@issue.id}")
19   - mail(from: sender(@note.author_id),
20   - cc: recipient(recipient_id),
21   - subject: subject("#{@issue.title} (##{@issue.iid})"))
  19 + mail_answer_thread(@issue,
  20 + from: sender(@note.author_id),
  21 + cc: recipient(recipient_id),
  22 + subject: subject("#{@issue.title} (##{@issue.iid})"))
22 23 end
23 24  
24 25 def note_merge_request_email(recipient_id, note_id)
... ... @@ -26,10 +27,10 @@ module Emails
26 27 @merge_request = @note.noteable
27 28 @project = @note.project
28 29 @target_url = project_merge_request_url(@project, @merge_request, anchor: "note_#{@note.id}")
29   - set_reference("merge_request_#{@merge_request.id}")
30   - mail(from: sender(@note.author_id),
31   - cc: recipient(recipient_id),
32   - subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
  30 + mail_answer_thread(@merge_request,
  31 + from: sender(@note.author_id),
  32 + cc: recipient(recipient_id),
  33 + subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
33 34 end
34 35  
35 36 def note_wall_email(recipient_id, note_id)
... ...
app/mailers/notify.rb
  Diff comments   View file @de90b57
... ... @@ -67,14 +67,6 @@ class Notify < ActionMailer::Base
67 67 end
68 68 end
69 69  
70   - # Set the Message-ID header field
71   - #
72   - # local_part - The local part of the message ID
73   - #
74   - def set_message_id(local_part)
75   - headers["Message-ID"] = "<#{local_part}@#{Gitlab.config.gitlab.host}>"
76   - end
77   -
78 70 # Set the References header field
79 71 #
80 72 # local_part - The local part of the referenced message ID
... ... @@ -107,4 +99,48 @@ class Notify &lt; ActionMailer::Base
107 99 subject << extra.join(' | ') if extra.present?
108 100 subject
109 101 end
  102 +
  103 + # Return a string suitable for inclusion in the 'Message-Id' mail header.
  104 + #
  105 + # The message-id is generated from the unique URL to a model object.
  106 + def message_id(model)
  107 + model_name = model.class.model_name.singular_route_key
  108 + "<#{model_name}_#{model.id}@#{Gitlab.config.gitlab.host}>"
  109 + end
  110 +
  111 + # Send an email that starts a new conversation thread,
  112 + # with headers suitable for grouping by thread in email clients.
  113 + #
  114 + # See: mail_answer_thread
  115 + def mail_new_thread(model, headers = {}, &block)
  116 + raise ArgumentError, '"To:" header will be overwritten; use "Cc:" or "Bcc:"' unless headers[:to].nil?
  117 + headers[:to] = project_sender_address.format
  118 +
  119 + headers['Message-ID'] = message_id(model)
  120 +
  121 + mail(headers, &block)
  122 + end
  123 +
  124 + # Send an email that responds to an existing conversation thread,
  125 + # with headers suitable for grouping by thread in email clients.
  126 + #
  127 + # For grouping emails by thread, email clients heuristics require the answers to:
  128 + #
  129 + # * have a subject that begin by 'Re: '
  130 + # * have a 'In-Reply-To' or 'References' header that references the original 'Message-ID'
  131 + # * have stable 'From' and 'To' headers between messages of the same thread
  132 + #
  133 + def mail_answer_thread(model, headers = {}, &block)
  134 + raise ArgumentError, '"To:" header will be overwritten; use "Cc:" or "Bcc:"' unless headers[:to].nil?
  135 + headers[:to] = project_sender_address.format
  136 +
  137 + headers['In-Reply-To'] = message_id(model)
  138 + headers['References'] = message_id(model)
  139 +
  140 + if (headers[:subject])
  141 + headers[:subject].prepend('Re: ')
  142 + end
  143 +
  144 + mail(headers, &block)
  145 + end
110 146 end
... ...
spec/mailers/notify_spec.rb
  Diff comments   View file @de90b57
... ... @@ -22,6 +22,23 @@ describe Notify do
22 22 end
23 23 end
24 24  
  25 + shared_examples 'an email starting a new thread' do |message_id_prefix|
  26 + it 'has a discussion identifier' do
  27 + should have_header 'Message-ID', /<#{message_id_prefix}(.*)@#{Gitlab.config.gitlab.host}>/
  28 + end
  29 + end
  30 +
  31 + shared_examples 'an answer to an existing thread' do |thread_id_prefix|
  32 + it 'has a subject that begins with Re: ' do
  33 + should have_subject /^Re: /
  34 + end
  35 +
  36 + it 'has headers that reference an existing thread' do
  37 + should have_header 'References', /<#{thread_id_prefix}(.*)@#{Gitlab.config.gitlab.host}>/
  38 + should have_header 'In-Reply-To', /<#{thread_id_prefix}(.*)@#{Gitlab.config.gitlab.host}>/
  39 + end
  40 + end
  41 +
25 42 describe 'for new users, the email' do
26 43 let(:example_site_path) { root_path }
27 44 let(:new_user) { create(:user, email: 'newguy@example.com', created_by_id: 1) }
... ... @@ -153,6 +170,7 @@ describe Notify do
153 170 subject { Notify.new_issue_email(issue.assignee_id, issue.id) }
154 171  
155 172 it_behaves_like 'an assignee email'
  173 + it_behaves_like 'an email starting a new thread', 'issue'
156 174  
157 175 it 'has the correct subject' do
158 176 should have_subject /#{project.name} \| #{issue.title} \(##{issue.iid}\)/
... ... @@ -161,10 +179,6 @@ describe Notify do
161 179 it 'contains a link to the new issue' do
162 180 should have_body_text /#{project_issue_path project, issue}/
163 181 end
164   -
165   - it 'has the correct message-id set' do
166   - should have_header 'Message-ID', "<issue_#{issue.id}@#{Gitlab.config.gitlab.host}>"
167   - end
168 182 end
169 183  
170 184 describe 'that are new with a description' do
... ... @@ -179,6 +193,7 @@ describe Notify do
179 193 subject { Notify.reassigned_issue_email(recipient.id, issue.id, previous_assignee.id, current_user) }
180 194  
181 195 it_behaves_like 'a multiple recipients email'
  196 + it_behaves_like 'an answer to an existing thread', 'issue'
182 197  
183 198 it 'is sent as the author' do
184 199 sender = subject.header[:from].addrs[0]
... ... @@ -201,16 +216,14 @@ describe Notify do
201 216 it 'contains a link to the issue' do
202 217 should have_body_text /#{project_issue_path project, issue}/
203 218 end
204   -
205   - it 'has the correct reference set' do
206   - should have_header 'References', "<issue_#{issue.id}@#{Gitlab.config.gitlab.host}>"
207   - end
208 219 end
209 220  
210 221 describe 'status changed' do
211 222 let(:status) { 'closed' }
212 223 subject { Notify.issue_status_changed_email(recipient.id, issue.id, status, current_user) }
213 224  
  225 + it_behaves_like 'an answer to an existing thread', 'issue'
  226 +
214 227 it 'is sent as the author' do
215 228 sender = subject.header[:from].addrs[0]
216 229 sender.display_name.should eq(current_user.name)
... ... @@ -232,10 +245,6 @@ describe Notify do
232 245 it 'contains a link to the issue' do
233 246 should have_body_text /#{project_issue_path project, issue}/
234 247 end
235   -
236   - it 'has the correct reference set' do
237   - should have_header 'References', "<issue_#{issue.id}@#{Gitlab.config.gitlab.host}>"
238   - end
239 248 end
240 249  
241 250 end
... ... @@ -249,6 +258,7 @@ describe Notify do
249 258 subject { Notify.new_merge_request_email(merge_request.assignee_id, merge_request.id) }
250 259  
251 260 it_behaves_like 'an assignee email'
  261 + it_behaves_like 'an email starting a new thread', 'merge_request'
252 262  
253 263 it 'has the correct subject' do
254 264 should have_subject /#{merge_request.title} \(##{merge_request.iid}\)/
... ... @@ -283,6 +293,7 @@ describe Notify do
283 293 subject { Notify.reassigned_merge_request_email(recipient.id, merge_request.id, previous_assignee.id, current_user.id) }
284 294  
285 295 it_behaves_like 'a multiple recipients email'
  296 + it_behaves_like 'an answer to an existing thread', 'merge_request'
286 297  
287 298 it 'is sent as the author' do
288 299 sender = subject.header[:from].addrs[0]
... ... @@ -311,6 +322,7 @@ describe Notify do
311 322 subject { Notify.merged_merge_request_email(recipient.id, merge_request.id, merge_author.id) }
312 323  
313 324 it_behaves_like 'a multiple recipients email'
  325 + it_behaves_like 'an answer to an existing thread', 'merge_request'
314 326  
315 327 it 'is sent as the merge author' do
316 328 sender = subject.header[:from].addrs[0]
... ... @@ -329,10 +341,6 @@ describe Notify do
329 341 it 'contains a link to the merge request' do
330 342 should have_body_text /#{project_merge_request_path project, merge_request}/
331 343 end
332   -
333   - it 'has the correct reference set' do
334   - should have_header 'References', "<merge_request_#{merge_request.id}@#{Gitlab.config.gitlab.host}>"
335   - end
336 344 end
337 345 end
338 346 end
... ... @@ -426,6 +434,7 @@ describe Notify do
426 434 subject { Notify.note_commit_email(recipient.id, note.id) }
427 435  
428 436 it_behaves_like 'a note email'
  437 + it_behaves_like 'an answer to an existing thread', 'commits'
429 438  
430 439 it 'has the correct subject' do
431 440 should have_subject /#{commit.title} \(#{commit.short_id}\)/
... ... @@ -444,6 +453,7 @@ describe Notify do
444 453 subject { Notify.note_merge_request_email(recipient.id, note.id) }
445 454  
446 455 it_behaves_like 'a note email'
  456 + it_behaves_like 'an answer to an existing thread', 'merge_request'
447 457  
448 458 it 'has the correct subject' do
449 459 should have_subject /#{merge_request.title} \(##{merge_request.iid}\)/
... ... @@ -462,6 +472,7 @@ describe Notify do
462 472 subject { Notify.note_issue_email(recipient.id, note.id) }
463 473  
464 474 it_behaves_like 'a note email'
  475 + it_behaves_like 'an answer to an existing thread', 'issue'
465 476  
466 477 it 'has the correct subject' do
467 478 should have_subject /#{issue.title} \(##{issue.iid}\)/
... ... @@ -574,7 +585,7 @@ describe Notify do
574 585 end
575 586  
576 587 it 'is sent to recipient' do
577   - should deliver_to 'devs@company.name'
  588 + should cc_to 'devs@company.name'
578 589 end
579 590  
580 591 it 'has the correct subject' do
... ...