Commit 97fd990ecde387290be269ef7daafa5761f94af6

Authored by Dmitriy Zaporozhets
2 parents f451a697 de90b572

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

Fix broken email threading

The email threading support introduced in GitLab CE 6.9 is broken on several popular email clients (including Mail.app and Airmail on Mac OS X).

This MR makes the following changes to improve email threading compatibility:

* Subject of answers to an existing thread begins with `Re: ` (required by Mail.app)
* The recipient of every email in a thread is stable (required by Mail.app ; otherwise it groups emails by sender)
* Send a ‘In-Reply-To’ header along the ‘References’ header (for compatibility with the spec)

In order to do this, these commits:

* Change the `To:` field to `namespace/project` ; the actual receiver is now in the `Cc:` field.
* Introduce the `mail_new_thread` and `mail_answer_thread` methods ; they format the message correctly for threading, and can generate the `Message-ID` automatically from a model instance.
* Refactor the tests to shared behaviors for email threading.

We've been using these patches at @capitainetrain for a few months now ; I just ported them to work nicely with the recent threading commits.
app/mailers/emails/groups.rb
... ... @@ -4,7 +4,7 @@ module Emails
4 4 @membership = UsersGroup.find(user_group_id)
5 5 @group = @membership.group
6 6 @target_url = group_url(@group)
7   - mail(to: @membership.user.email,
  7 + mail(cc: @membership.user.email,
8 8 subject: subject("Access to group was granted"))
9 9 end
10 10 end
... ...
app/mailers/emails/issues.rb
... ... @@ -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   - to: 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   - to: 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   - to: 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   - to: 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
... ... @@ -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   - to: 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   - to: 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   - to: 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   - to: 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
... ... @@ -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   - to: 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   - to: 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   - to: 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)
... ... @@ -37,7 +38,7 @@ module Emails
37 38 @project = @note.project
38 39 @target_url = project_wall_url(@note.project, anchor: "note_#{@note.id}")
39 40 mail(from: sender(@note.author_id),
40   - to: recipient(recipient_id),
  41 + cc: recipient(recipient_id),
41 42 subject: subject("Note on wall"))
42 43 end
43 44 end
... ...
app/mailers/emails/projects.rb
... ... @@ -4,7 +4,7 @@ module Emails
4 4 @users_project = UsersProject.find user_project_id
5 5 @project = @users_project.project
6 6 @target_url = project_url(@project)
7   - mail(to: @users_project.user.email,
  7 + mail(cc: @users_project.user.email,
8 8 subject: subject("Access to project was granted"))
9 9 end
10 10  
... ... @@ -12,7 +12,7 @@ module Emails
12 12 @user = User.find user_id
13 13 @project = Project.find project_id
14 14 @target_url = project_url(@project)
15   - mail(to: @user.email,
  15 + mail(cc: @user.email,
16 16 subject: subject("Project was moved"))
17 17 end
18 18  
... ... @@ -30,7 +30,7 @@ module Emails
30 30 end
31 31  
32 32 mail(from: sender(author_id),
33   - to: recipient,
  33 + cc: recipient,
34 34 subject: subject("New push to repository"))
35 35 end
36 36 end
... ...
app/mailers/notify.rb
1 1 class Notify < ActionMailer::Base
  2 + include ActionDispatch::Routing::PolymorphicRoutes
  3 +
2 4 include Emails::Issues
3 5 include Emails::MergeRequests
4 6 include Emails::Notes
... ... @@ -16,6 +18,7 @@ class Notify &lt; ActionMailer::Base
16 18 default_url_options[:script_name] = Gitlab.config.gitlab.relative_url_root
17 19  
18 20 default from: Proc.new { default_sender_address.format }
  21 + default to: Proc.new { project_sender_address.format }
19 22 default reply_to: "noreply@#{Gitlab.config.gitlab.host}"
20 23  
21 24 # Just send email with 2 seconds delay
... ... @@ -32,6 +35,17 @@ class Notify &lt; ActionMailer::Base
32 35 address
33 36 end
34 37  
  38 + # The default email address to send emails to. Includes the project name if possible.
  39 + def project_sender_address
  40 + if @project
  41 + address = default_sender_address
  42 + address.display_name = @project.name_with_namespace
  43 + address
  44 + else
  45 + default_sender_address
  46 + end
  47 + end
  48 +
35 49 # Return an email address that displays the name of the sender.
36 50 # Only the displayed name changes; the actual email address is always the same.
37 51 def sender(sender_id)
... ... @@ -53,14 +67,6 @@ class Notify &lt; ActionMailer::Base
53 67 end
54 68 end
55 69  
56   - # Set the Message-ID header field
57   - #
58   - # local_part - The local part of the message ID
59   - #
60   - def set_message_id(local_part)
61   - headers["Message-ID"] = "<#{local_part}@#{Gitlab.config.gitlab.host}>"
62   - end
63   -
64 70 # Set the References header field
65 71 #
66 72 # local_part - The local part of the referenced message ID
... ... @@ -93,4 +99,48 @@ class Notify &lt; ActionMailer::Base
93 99 subject << extra.join(' | ') if extra.present?
94 100 subject
95 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
96 146 end
... ...
spec/mailers/notify_spec.rb
... ... @@ -10,7 +10,7 @@ describe Notify do
10 10  
11 11 shared_examples 'a multiple recipients email' do
12 12 it 'is sent to the given recipient' do
13   - should deliver_to recipient.email
  13 + should cc_to recipient.email
14 14 end
15 15 end
16 16  
... ... @@ -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) }
... ... @@ -141,7 +158,7 @@ describe Notify do
141 158 end
142 159  
143 160 it 'is sent to the assignee' do
144   - should deliver_to assignee.email
  161 + should cc_to assignee.email
145 162 end
146 163 end
147 164  
... ... @@ -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
... ... @@ -394,7 +402,7 @@ describe Notify do
394 402 end
395 403  
396 404 it 'is sent to the given recipient' do
397   - should deliver_to recipient.email
  405 + should cc_to recipient.email
398 406 end
399 407  
400 408 it 'contains the message from the note' do
... ... @@ -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}\)/
... ... @@ -538,7 +549,7 @@ describe Notify do
538 549 end
539 550  
540 551 it 'is sent to recipient' do
541   - should deliver_to 'devs@company.name'
  552 + should cc_to 'devs@company.name'
542 553 end
543 554  
544 555 it 'has the correct subject' do
... ... @@ -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
... ...
spec/services/issues/close_service_spec.rb
... ... @@ -22,7 +22,7 @@ describe Issues::CloseService do
22 22  
23 23 it 'should send email to user2 about assign of new issue' do
24 24 email = ActionMailer::Base.deliveries.last
25   - email.to.first.should == user2.email
  25 + email.cc.first.should == user2.email
26 26 email.subject.should include(issue.title)
27 27 end
28 28  
... ...
spec/services/issues/update_service_spec.rb
... ... @@ -31,7 +31,7 @@ describe Issues::UpdateService do
31 31  
32 32 it 'should send email to user2 about assign of new issue' do
33 33 email = ActionMailer::Base.deliveries.last
34   - email.to.first.should == user2.email
  34 + email.cc.first.should == user2.email
35 35 email.subject.should include(issue.title)
36 36 end
37 37  
... ...
spec/services/merge_requests/close_service_spec.rb
... ... @@ -22,7 +22,7 @@ describe MergeRequests::CloseService do
22 22  
23 23 it 'should send email to user2 about assign of new merge_request' do
24 24 email = ActionMailer::Base.deliveries.last
25   - email.to.first.should == user2.email
  25 + email.cc.first.should == user2.email
26 26 email.subject.should include(merge_request.title)
27 27 end
28 28  
... ...
spec/services/merge_requests/update_service_spec.rb
... ... @@ -31,7 +31,7 @@ describe MergeRequests::UpdateService do
31 31  
32 32 it 'should send email to user2 about assign of new merge_request' do
33 33 email = ActionMailer::Base.deliveries.last
34   - email.to.first.should == user2.email
  34 + email.cc.first.should == user2.email
35 35 email.subject.should include(merge_request.title)
36 36 end
37 37  
... ...