Skip to content

Commit de90b57

Browse files
author
Pierre de La Morinerie
committed
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.
1 parent 466b768 commit de90b57

File tree

5 files changed

+116
-68
lines changed

5 files changed

+116
-68
lines changed

‎app/mailers/emails/issues.rb

+16-16
Original file line numberDiff line numberDiff line change
@@ -4,32 +4,32 @@ def new_issue_email(recipient_id, issue_id)
44
@issue = Issue.find(issue_id)
55
@project = @issue.project
66
@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})"))
1111
end
1212

1313
def reassigned_issue_email(recipient_id, issue_id, previous_assignee_id, updated_by_user_id)
1414
@issue = Issue.find(issue_id)
1515
@previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id
1616
@project = @issue.project
1717
@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})"))
2222
end
2323

2424
def closed_issue_email(recipient_id, issue_id, updated_by_user_id)
2525
@issue = Issue.find issue_id
2626
@project = @issue.project
2727
@updated_by = User.find updated_by_user_id
2828
@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})"))
3333
end
3434

3535
def issue_status_changed_email(recipient_id, issue_id, status, updated_by_user_id)
@@ -38,10 +38,10 @@ def issue_status_changed_email(recipient_id, issue_id, status, updated_by_user_i
3838
@project = @issue.project
3939
@updated_by = User.find updated_by_user_id
4040
@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})"))
4545
end
4646
end
4747
end

‎app/mailers/emails/merge_requests.rb

+16-16
Original file line numberDiff line numberDiff line change
@@ -4,42 +4,42 @@ def new_merge_request_email(recipient_id, merge_request_id)
44
@merge_request = MergeRequest.find(merge_request_id)
55
@project = @merge_request.project
66
@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})"))
1111
end
1212

1313
def reassigned_merge_request_email(recipient_id, merge_request_id, previous_assignee_id, updated_by_user_id)
1414
@merge_request = MergeRequest.find(merge_request_id)
1515
@previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id
1616
@project = @merge_request.project
1717
@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})"))
2222
end
2323

2424
def closed_merge_request_email(recipient_id, merge_request_id, updated_by_user_id)
2525
@merge_request = MergeRequest.find(merge_request_id)
2626
@updated_by = User.find updated_by_user_id
2727
@project = @merge_request.project
2828
@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})"))
3333
end
3434

3535
def merged_merge_request_email(recipient_id, merge_request_id, updated_by_user_id)
3636
@merge_request = MergeRequest.find(merge_request_id)
3737
@project = @merge_request.project
3838
@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})"))
4343
end
4444
end
4545

‎app/mailers/emails/notes.rb

+12-11
Original file line numberDiff line numberDiff line change
@@ -5,31 +5,32 @@ def note_commit_email(recipient_id, note_id)
55
@commit = @note.noteable
66
@project = @note.project
77
@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})"))
1112
end
1213

1314
def note_issue_email(recipient_id, note_id)
1415
@note = Note.find(note_id)
1516
@issue = @note.noteable
1617
@project = @note.project
1718
@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})"))
2223
end
2324

2425
def note_merge_request_email(recipient_id, note_id)
2526
@note = Note.find(note_id)
2627
@merge_request = @note.noteable
2728
@project = @note.project
2829
@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})"))
3334
end
3435

3536
def note_wall_email(recipient_id, note_id)

‎app/mailers/notify.rb

+44-8
Original file line numberDiff line numberDiff line change
@@ -67,14 +67,6 @@ def recipient(recipient_id)
6767
end
6868
end
6969

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-
7870
# Set the References header field
7971
#
8072
# local_part - The local part of the referenced message ID
@@ -107,4 +99,48 @@ def subject(*extra)
10799
subject << extra.join(' | ') if extra.present?
108100
subject
109101
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
110146
end

‎spec/mailers/notify_spec.rb

+28-17
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,23 @@
2222
end
2323
end
2424

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+
2542
describe 'for new users, the email' do
2643
let(:example_site_path) { root_path }
2744
let(:new_user) { create(:user, email: '[email protected]', created_by_id: 1) }
@@ -153,6 +170,7 @@
153170
subject { Notify.new_issue_email(issue.assignee_id, issue.id) }
154171

155172
it_behaves_like 'an assignee email'
173+
it_behaves_like 'an email starting a new thread', 'issue'
156174

157175
it 'has the correct subject' do
158176
should have_subject /#{project.name} \| #{issue.title} \(##{issue.iid}\)/
@@ -161,10 +179,6 @@
161179
it 'contains a link to the new issue' do
162180
should have_body_text /#{project_issue_path project, issue}/
163181
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
168182
end
169183

170184
describe 'that are new with a description' do
@@ -179,6 +193,7 @@
179193
subject { Notify.reassigned_issue_email(recipient.id, issue.id, previous_assignee.id, current_user) }
180194

181195
it_behaves_like 'a multiple recipients email'
196+
it_behaves_like 'an answer to an existing thread', 'issue'
182197

183198
it 'is sent as the author' do
184199
sender = subject.header[:from].addrs[0]
@@ -201,16 +216,14 @@
201216
it 'contains a link to the issue' do
202217
should have_body_text /#{project_issue_path project, issue}/
203218
end
204-
205-
it 'has the correct reference set' do
206-
should have_header 'References', "<issue_#{issue.id}@#{Gitlab.config.gitlab.host}>"
207-
end
208219
end
209220

210221
describe 'status changed' do
211222
let(:status) { 'closed' }
212223
subject { Notify.issue_status_changed_email(recipient.id, issue.id, status, current_user) }
213224

225+
it_behaves_like 'an answer to an existing thread', 'issue'
226+
214227
it 'is sent as the author' do
215228
sender = subject.header[:from].addrs[0]
216229
sender.display_name.should eq(current_user.name)
@@ -232,10 +245,6 @@
232245
it 'contains a link to the issue' do
233246
should have_body_text /#{project_issue_path project, issue}/
234247
end
235-
236-
it 'has the correct reference set' do
237-
should have_header 'References', "<issue_#{issue.id}@#{Gitlab.config.gitlab.host}>"
238-
end
239248
end
240249

241250
end
@@ -249,6 +258,7 @@
249258
subject { Notify.new_merge_request_email(merge_request.assignee_id, merge_request.id) }
250259

251260
it_behaves_like 'an assignee email'
261+
it_behaves_like 'an email starting a new thread', 'merge_request'
252262

253263
it 'has the correct subject' do
254264
should have_subject /#{merge_request.title} \(##{merge_request.iid}\)/
@@ -283,6 +293,7 @@
283293
subject { Notify.reassigned_merge_request_email(recipient.id, merge_request.id, previous_assignee.id, current_user.id) }
284294

285295
it_behaves_like 'a multiple recipients email'
296+
it_behaves_like 'an answer to an existing thread', 'merge_request'
286297

287298
it 'is sent as the author' do
288299
sender = subject.header[:from].addrs[0]
@@ -311,6 +322,7 @@
311322
subject { Notify.merged_merge_request_email(recipient.id, merge_request.id, merge_author.id) }
312323

313324
it_behaves_like 'a multiple recipients email'
325+
it_behaves_like 'an answer to an existing thread', 'merge_request'
314326

315327
it 'is sent as the merge author' do
316328
sender = subject.header[:from].addrs[0]
@@ -329,10 +341,6 @@
329341
it 'contains a link to the merge request' do
330342
should have_body_text /#{project_merge_request_path project, merge_request}/
331343
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
336344
end
337345
end
338346
end
@@ -426,6 +434,7 @@
426434
subject { Notify.note_commit_email(recipient.id, note.id) }
427435

428436
it_behaves_like 'a note email'
437+
it_behaves_like 'an answer to an existing thread', 'commits'
429438

430439
it 'has the correct subject' do
431440
should have_subject /#{commit.title} \(#{commit.short_id}\)/
@@ -444,6 +453,7 @@
444453
subject { Notify.note_merge_request_email(recipient.id, note.id) }
445454

446455
it_behaves_like 'a note email'
456+
it_behaves_like 'an answer to an existing thread', 'merge_request'
447457

448458
it 'has the correct subject' do
449459
should have_subject /#{merge_request.title} \(##{merge_request.iid}\)/
@@ -462,6 +472,7 @@
462472
subject { Notify.note_issue_email(recipient.id, note.id) }
463473

464474
it_behaves_like 'a note email'
475+
it_behaves_like 'an answer to an existing thread', 'issue'
465476

466477
it 'has the correct subject' do
467478
should have_subject /#{issue.title} \(##{issue.iid}\)/
@@ -574,7 +585,7 @@
574585
end
575586

576587
it 'is sent to recipient' do
577-
should deliver_to '[email protected]'
588+
should cc_to '[email protected]'
578589
end
579590

580591
it 'has the correct subject' do

0 commit comments

Comments
 (0)