Download as
Browse Code »

Commit 9d09d616af77936f0e278b991253ac7a2b0fccf3

Authored by Dmitriy Zaporozhets
2 parents 0a1b4ba2 97fd990e
Exists in spb-stable and in 2 other branches 7-0-stable, 7-0-stable-spb

Merge branch 'master' of gitlab.com:gitlab-org/gitlab-ce

Showing 11 changed files with 143 additions and 81 deletions   Show diff stats
app/mailers/emails/groups.rb
  Diff comments   View file @9d09d61
... ... @@ -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
  Diff comments   View file @9d09d61
... ... @@ -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
  Diff comments   View file @9d09d61
... ... @@ -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
  Diff comments   View file @9d09d61
... ... @@ -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
  Diff comments   View file @9d09d61
... ... @@ -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
  Diff comments   View file @9d09d61
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
  Diff comments   View file @9d09d61
... ... @@ -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
  Diff comments   View file @9d09d61
... ... @@ -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
  Diff comments   View file @9d09d61
... ... @@ -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
  Diff comments   View file @9d09d61
... ... @@ -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
  Diff comments   View file @9d09d61
... ... @@ -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  
... ...