Commit c3a3f80af9277ca22a186a6ac8feafe9ddfda97c
Exists in
spb-stable
and in
3 other branches
Merge branch 'cleaner-email-headers' into 'master'
Cleaner headers in Notification Emails Make the informations available in the notification email headers (sender, recipient, subject, etc.) more readable and meaningful. * Remove the email subject prefix * Don't write the project namespace in email subjects * Write the issue/merge request title in the notification email subject * Make the email appear as sent from the action author (the actual email address is still `gitlab@gitlab.com`) For instance, this is the notification email for a new issue comment before: > From: gitlab@gitlab.com > To: myemailaddress@gmail.com > Subject: GitLab | GitLab HQ / GitLab-Shell | New note for issue #1234 And after : > From: Nick Brown <gitlab@gitlab.com> > To: myemailaddress@gmail.com > Subject: GitLab-Shell | Add local update hook (#1234) The recipient of the notification can easily get the gist of the message without even opening it — just by looking at how it appears in her inbox. None of the actual email addresses (From, To, Reply-to) changes, just the display name. Having a consistent subject for all notification emails sent about some resource also allow good email clients to group the discussion by thread (although grouping in Mail.app still needs some work).
Showing
9 changed files
with
149 additions
and
52 deletions
Show diff stats
app/mailers/emails/issues.rb
@@ -3,22 +3,27 @@ module Emails | @@ -3,22 +3,27 @@ module Emails | ||
3 | def new_issue_email(recipient_id, issue_id) | 3 | def new_issue_email(recipient_id, issue_id) |
4 | @issue = Issue.find(issue_id) | 4 | @issue = Issue.find(issue_id) |
5 | @project = @issue.project | 5 | @project = @issue.project |
6 | - mail(to: recipient(recipient_id), subject: subject("New issue ##{@issue.iid}", @issue.title)) | 6 | + mail(from: sender(@issue.author_id), |
7 | + to: recipient(recipient_id), | ||
8 | + subject: subject("#{@issue.title} (##{@issue.iid})")) | ||
7 | end | 9 | end |
8 | 10 | ||
9 | - def reassigned_issue_email(recipient_id, issue_id, previous_assignee_id) | 11 | + def reassigned_issue_email(recipient_id, issue_id, previous_assignee_id, updated_by_user_id) |
10 | @issue = Issue.find(issue_id) | 12 | @issue = Issue.find(issue_id) |
11 | @previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id | 13 | @previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id |
12 | @project = @issue.project | 14 | @project = @issue.project |
13 | - mail(to: recipient(recipient_id), subject: subject("Changed issue ##{@issue.iid}", @issue.title)) | 15 | + mail(from: sender(updated_by_user_id), |
16 | + to: recipient(recipient_id), | ||
17 | + subject: subject("#{@issue.title} (##{@issue.iid})")) | ||
14 | end | 18 | end |
15 | 19 | ||
16 | def closed_issue_email(recipient_id, issue_id, updated_by_user_id) | 20 | def closed_issue_email(recipient_id, issue_id, updated_by_user_id) |
17 | @issue = Issue.find issue_id | 21 | @issue = Issue.find issue_id |
18 | @project = @issue.project | 22 | @project = @issue.project |
19 | @updated_by = User.find updated_by_user_id | 23 | @updated_by = User.find updated_by_user_id |
20 | - mail(to: recipient(recipient_id), | ||
21 | - subject: subject("Closed issue ##{@issue.iid}", @issue.title)) | 24 | + mail(from: sender(updated_by_user_id), |
25 | + to: recipient(recipient_id), | ||
26 | + subject: subject("#{@issue.title} (##{@issue.iid})")) | ||
22 | end | 27 | end |
23 | 28 | ||
24 | def issue_status_changed_email(recipient_id, issue_id, status, updated_by_user_id) | 29 | def issue_status_changed_email(recipient_id, issue_id, status, updated_by_user_id) |
@@ -26,8 +31,9 @@ module Emails | @@ -26,8 +31,9 @@ module Emails | ||
26 | @issue_status = status | 31 | @issue_status = status |
27 | @project = @issue.project | 32 | @project = @issue.project |
28 | @updated_by = User.find updated_by_user_id | 33 | @updated_by = User.find updated_by_user_id |
29 | - mail(to: recipient(recipient_id), | ||
30 | - subject: subject("Changed issue ##{@issue.iid}", @issue.title)) | 34 | + mail(from: sender(updated_by_user_id), |
35 | + to: recipient(recipient_id), | ||
36 | + subject: subject("#{@issue.title} (##{@issue.iid})")) | ||
31 | end | 37 | end |
32 | end | 38 | end |
33 | end | 39 | end |
app/mailers/emails/merge_requests.rb
@@ -3,27 +3,35 @@ module Emails | @@ -3,27 +3,35 @@ module Emails | ||
3 | def new_merge_request_email(recipient_id, merge_request_id) | 3 | def new_merge_request_email(recipient_id, merge_request_id) |
4 | @merge_request = MergeRequest.find(merge_request_id) | 4 | @merge_request = MergeRequest.find(merge_request_id) |
5 | @project = @merge_request.project | 5 | @project = @merge_request.project |
6 | - mail(to: recipient(recipient_id), subject: subject("New merge request ##{@merge_request.iid}", @merge_request.title)) | 6 | + mail(from: sender(@merge_request.author_id), |
7 | + to: recipient(recipient_id), | ||
8 | + subject: subject("#{@merge_request.title} (!#{@merge_request.iid})")) | ||
7 | end | 9 | end |
8 | 10 | ||
9 | - def reassigned_merge_request_email(recipient_id, merge_request_id, previous_assignee_id) | 11 | + def reassigned_merge_request_email(recipient_id, merge_request_id, previous_assignee_id, updated_by_user_id) |
10 | @merge_request = MergeRequest.find(merge_request_id) | 12 | @merge_request = MergeRequest.find(merge_request_id) |
11 | @previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id | 13 | @previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id |
12 | @project = @merge_request.project | 14 | @project = @merge_request.project |
13 | - mail(to: recipient(recipient_id), subject: subject("Changed merge request ##{@merge_request.iid}", @merge_request.title)) | 15 | + mail(from: sender(updated_by_user_id), |
16 | + to: recipient(recipient_id), | ||
17 | + subject: subject("#{@merge_request.title} (!#{@merge_request.iid})")) | ||
14 | end | 18 | end |
15 | 19 | ||
16 | def closed_merge_request_email(recipient_id, merge_request_id, updated_by_user_id) | 20 | def closed_merge_request_email(recipient_id, merge_request_id, updated_by_user_id) |
17 | @merge_request = MergeRequest.find(merge_request_id) | 21 | @merge_request = MergeRequest.find(merge_request_id) |
18 | @updated_by = User.find updated_by_user_id | 22 | @updated_by = User.find updated_by_user_id |
19 | @project = @merge_request.project | 23 | @project = @merge_request.project |
20 | - mail(to: recipient(recipient_id), subject: subject("Closed merge request ##{@merge_request.iid}", @merge_request.title)) | 24 | + mail(from: sender(updated_by_user_id), |
25 | + to: recipient(recipient_id), | ||
26 | + subject: subject("#{@merge_request.title} (!#{@merge_request.iid})")) | ||
21 | end | 27 | end |
22 | 28 | ||
23 | def merged_merge_request_email(recipient_id, merge_request_id) | 29 | def merged_merge_request_email(recipient_id, merge_request_id) |
24 | @merge_request = MergeRequest.find(merge_request_id) | 30 | @merge_request = MergeRequest.find(merge_request_id) |
25 | @project = @merge_request.project | 31 | @project = @merge_request.project |
26 | - mail(to: recipient(recipient_id), subject: subject("Accepted merge request ##{@merge_request.iid}", @merge_request.title)) | 32 | + mail(from: sender(@merge_request.author_id_of_changes), |
33 | + to: recipient(recipient_id), | ||
34 | + subject: subject("#{@merge_request.title} (!#{@merge_request.iid})")) | ||
27 | end | 35 | end |
28 | end | 36 | end |
29 | 37 | ||
@@ -57,7 +65,7 @@ module Emails | @@ -57,7 +65,7 @@ module Emails | ||
57 | # >> subject('Lorem ipsum', 'Dolor sit amet') | 65 | # >> subject('Lorem ipsum', 'Dolor sit amet') |
58 | # => "GitLab Merge Request | Lorem ipsum | Dolor sit amet" | 66 | # => "GitLab Merge Request | Lorem ipsum | Dolor sit amet" |
59 | def subject(*extra) | 67 | def subject(*extra) |
60 | - subject = "GitLab Merge Request |" | 68 | + subject = "Merge Request | " |
61 | if @merge_request.for_fork? | 69 | if @merge_request.for_fork? |
62 | subject << "#{@merge_request.source_project.name_with_namespace}:#{merge_request.source_branch} >> #{@merge_request.target_project.name_with_namespace}:#{merge_request.target_branch}" | 70 | subject << "#{@merge_request.source_project.name_with_namespace}:#{merge_request.source_branch} >> #{@merge_request.target_project.name_with_namespace}:#{merge_request.target_branch}" |
63 | else | 71 | else |
app/mailers/emails/notes.rb
@@ -4,27 +4,35 @@ module Emails | @@ -4,27 +4,35 @@ module Emails | ||
4 | @note = Note.find(note_id) | 4 | @note = Note.find(note_id) |
5 | @commit = @note.noteable | 5 | @commit = @note.noteable |
6 | @project = @note.project | 6 | @project = @note.project |
7 | - mail(to: recipient(recipient_id), subject: subject("Note for commit #{@commit.short_id}", @commit.title)) | 7 | + mail(from: sender(@note.author_id), |
8 | + to: recipient(recipient_id), | ||
9 | + subject: subject("#{@commit.title} (#{@commit.short_id})")) | ||
8 | end | 10 | end |
9 | 11 | ||
10 | def note_issue_email(recipient_id, note_id) | 12 | def note_issue_email(recipient_id, note_id) |
11 | @note = Note.find(note_id) | 13 | @note = Note.find(note_id) |
12 | @issue = @note.noteable | 14 | @issue = @note.noteable |
13 | @project = @note.project | 15 | @project = @note.project |
14 | - mail(to: recipient(recipient_id), subject: subject("Note for issue ##{@issue.iid}")) | 16 | + mail(from: sender(@note.author_id), |
17 | + to: recipient(recipient_id), | ||
18 | + subject: subject("#{@issue.title} (##{@issue.iid})")) | ||
15 | end | 19 | end |
16 | 20 | ||
17 | def note_merge_request_email(recipient_id, note_id) | 21 | def note_merge_request_email(recipient_id, note_id) |
18 | @note = Note.find(note_id) | 22 | @note = Note.find(note_id) |
19 | @merge_request = @note.noteable | 23 | @merge_request = @note.noteable |
20 | @project = @note.project | 24 | @project = @note.project |
21 | - mail(to: recipient(recipient_id), subject: subject("Note for merge request ##{@merge_request.iid}")) | 25 | + mail(from: sender(@note.author_id), |
26 | + to: recipient(recipient_id), | ||
27 | + subject: subject("#{@merge_request.title} (!#{@merge_request.iid})")) | ||
22 | end | 28 | end |
23 | 29 | ||
24 | def note_wall_email(recipient_id, note_id) | 30 | def note_wall_email(recipient_id, note_id) |
25 | @note = Note.find(note_id) | 31 | @note = Note.find(note_id) |
26 | @project = @note.project | 32 | @project = @note.project |
27 | - mail(to: recipient(recipient_id), subject: subject("Note on wall")) | 33 | + mail(from: sender(@note.author_id), |
34 | + to: recipient(recipient_id), | ||
35 | + subject: subject("Note on wall")) | ||
28 | end | 36 | end |
29 | end | 37 | end |
30 | end | 38 | end |
app/mailers/emails/projects.rb
@@ -22,7 +22,9 @@ module Emails | @@ -22,7 +22,9 @@ module Emails | ||
22 | @diffs = compare.diffs | 22 | @diffs = compare.diffs |
23 | @branch = branch | 23 | @branch = branch |
24 | 24 | ||
25 | - mail(to: recipient, subject: subject("New push to repository")) | 25 | + mail(from: sender(author_id), |
26 | + to: recipient, | ||
27 | + subject: subject("New push to repository")) | ||
26 | end | 28 | end |
27 | end | 29 | end |
28 | end | 30 | end |
app/mailers/notify.rb
@@ -15,16 +15,33 @@ class Notify < ActionMailer::Base | @@ -15,16 +15,33 @@ class Notify < ActionMailer::Base | ||
15 | default_url_options[:port] = Gitlab.config.gitlab.port unless Gitlab.config.gitlab_on_standard_port? | 15 | default_url_options[:port] = Gitlab.config.gitlab.port unless Gitlab.config.gitlab_on_standard_port? |
16 | default_url_options[:script_name] = Gitlab.config.gitlab.relative_url_root | 16 | default_url_options[:script_name] = Gitlab.config.gitlab.relative_url_root |
17 | 17 | ||
18 | - default from: Gitlab.config.gitlab.email_from | 18 | + default from: Proc.new { default_sender_address.format } |
19 | default reply_to: "noreply@#{Gitlab.config.gitlab.host}" | 19 | default reply_to: "noreply@#{Gitlab.config.gitlab.host}" |
20 | 20 | ||
21 | - # Just send email with 3 seconds delay | 21 | + # Just send email with 2 seconds delay |
22 | def self.delay | 22 | def self.delay |
23 | delay_for(2.seconds) | 23 | delay_for(2.seconds) |
24 | end | 24 | end |
25 | 25 | ||
26 | private | 26 | private |
27 | 27 | ||
28 | + # The default email address to send emails from | ||
29 | + def default_sender_address | ||
30 | + address = Mail::Address.new(Gitlab.config.gitlab.email_from) | ||
31 | + address.display_name = "GitLab" | ||
32 | + address | ||
33 | + end | ||
34 | + | ||
35 | + # Return an email address that displays the name of the sender. | ||
36 | + # Only the displayed name changes; the actual email address is always the same. | ||
37 | + def sender(sender_id) | ||
38 | + if sender = User.find(sender_id) | ||
39 | + address = default_sender_address | ||
40 | + address.display_name = sender.name | ||
41 | + address.format | ||
42 | + end | ||
43 | + end | ||
44 | + | ||
28 | # Look up a User by their ID and return their email address | 45 | # Look up a User by their ID and return their email address |
29 | # | 46 | # |
30 | # recipient_id - User ID | 47 | # recipient_id - User ID |
@@ -43,21 +60,21 @@ class Notify < ActionMailer::Base | @@ -43,21 +60,21 @@ class Notify < ActionMailer::Base | ||
43 | # Examples | 60 | # Examples |
44 | # | 61 | # |
45 | # >> subject('Lorem ipsum') | 62 | # >> subject('Lorem ipsum') |
46 | - # => "GitLab | Lorem ipsum" | 63 | + # => "Lorem ipsum" |
47 | # | 64 | # |
48 | # # Automatically inserts Project name when @project is set | 65 | # # Automatically inserts Project name when @project is set |
49 | # >> @project = Project.last | 66 | # >> @project = Project.last |
50 | # => #<Project id: 1, name: "Ruby on Rails", path: "ruby_on_rails", ...> | 67 | # => #<Project id: 1, name: "Ruby on Rails", path: "ruby_on_rails", ...> |
51 | # >> subject('Lorem ipsum') | 68 | # >> subject('Lorem ipsum') |
52 | - # => "GitLab | Ruby on Rails | Lorem ipsum " | 69 | + # => "Ruby on Rails | Lorem ipsum " |
53 | # | 70 | # |
54 | # # Accepts multiple arguments | 71 | # # Accepts multiple arguments |
55 | # >> subject('Lorem ipsum', 'Dolor sit amet') | 72 | # >> subject('Lorem ipsum', 'Dolor sit amet') |
56 | - # => "GitLab | Lorem ipsum | Dolor sit amet" | 73 | + # => "Lorem ipsum | Dolor sit amet" |
57 | def subject(*extra) | 74 | def subject(*extra) |
58 | - subject = "GitLab" | ||
59 | - subject << (@project ? " | #{@project.name_with_namespace}" : "") | ||
60 | - subject << " | " + extra.join(' | ') if extra.present? | 75 | + subject = "" |
76 | + subject << "#{@project.name} | " if @project | ||
77 | + subject << extra.join(' | ') if extra.present? | ||
61 | subject | 78 | subject |
62 | end | 79 | end |
63 | end | 80 | end |
app/services/notification_service.rb
@@ -257,7 +257,7 @@ class NotificationService | @@ -257,7 +257,7 @@ class NotificationService | ||
257 | recipients.delete(current_user) | 257 | recipients.delete(current_user) |
258 | 258 | ||
259 | recipients.each do |recipient| | 259 | recipients.each do |recipient| |
260 | - mailer.send(method, recipient.id, target.id, target.assignee_id_was) | 260 | + mailer.send(method, recipient.id, target.id, target.assignee_id_was, current_user.id) |
261 | end | 261 | end |
262 | end | 262 | end |
263 | 263 |
config/initializers/devise.rb
@@ -4,7 +4,7 @@ Devise.setup do |config| | @@ -4,7 +4,7 @@ Devise.setup do |config| | ||
4 | # ==> Mailer Configuration | 4 | # ==> Mailer Configuration |
5 | # Configure the e-mail address which will be shown in Devise::Mailer, | 5 | # Configure the e-mail address which will be shown in Devise::Mailer, |
6 | # note that it will be overwritten if you use your own mailer class with default "from" parameter. | 6 | # note that it will be overwritten if you use your own mailer class with default "from" parameter. |
7 | - config.mailer_sender = Gitlab.config.gitlab.email_from | 7 | + config.mailer_sender = "GitLab <#{Gitlab.config.gitlab.email_from}>" |
8 | 8 | ||
9 | 9 | ||
10 | # Configure the class responsible to send e-mails. | 10 | # Configure the class responsible to send e-mails. |
spec/mailers/notify_spec.rb
@@ -4,6 +4,7 @@ describe Notify do | @@ -4,6 +4,7 @@ describe Notify do | ||
4 | include EmailSpec::Helpers | 4 | include EmailSpec::Helpers |
5 | include EmailSpec::Matchers | 5 | include EmailSpec::Matchers |
6 | 6 | ||
7 | + let(:gitlab_sender) { Gitlab.config.gitlab.email_from } | ||
7 | let(:recipient) { create(:user, email: 'recipient@example.com') } | 8 | let(:recipient) { create(:user, email: 'recipient@example.com') } |
8 | let(:project) { create(:project) } | 9 | let(:project) { create(:project) } |
9 | 10 | ||
@@ -13,18 +14,28 @@ describe Notify do | @@ -13,18 +14,28 @@ describe Notify do | ||
13 | end | 14 | end |
14 | end | 15 | end |
15 | 16 | ||
17 | + shared_examples 'an email sent from GitLab' do | ||
18 | + it 'is sent from GitLab' do | ||
19 | + sender = subject.header[:from].addrs[0] | ||
20 | + sender.display_name.should eq('GitLab') | ||
21 | + sender.address.should eq(gitlab_sender) | ||
22 | + end | ||
23 | + end | ||
24 | + | ||
16 | describe 'for new users, the email' do | 25 | describe 'for new users, the email' do |
17 | let(:example_site_path) { root_path } | 26 | let(:example_site_path) { root_path } |
18 | let(:new_user) { create(:user, email: 'newguy@example.com', created_by_id: 1) } | 27 | let(:new_user) { create(:user, email: 'newguy@example.com', created_by_id: 1) } |
19 | 28 | ||
20 | subject { Notify.new_user_email(new_user.id, new_user.password) } | 29 | subject { Notify.new_user_email(new_user.id, new_user.password) } |
21 | 30 | ||
31 | + it_behaves_like 'an email sent from GitLab' | ||
32 | + | ||
22 | it 'is sent to the new user' do | 33 | it 'is sent to the new user' do |
23 | should deliver_to new_user.email | 34 | should deliver_to new_user.email |
24 | end | 35 | end |
25 | 36 | ||
26 | it 'has the correct subject' do | 37 | it 'has the correct subject' do |
27 | - should have_subject /^gitlab \| Account was created for you$/i | 38 | + should have_subject /^Account was created for you$/i |
28 | end | 39 | end |
29 | 40 | ||
30 | it 'contains the new user\'s login name' do | 41 | it 'contains the new user\'s login name' do |
@@ -47,12 +58,14 @@ describe Notify do | @@ -47,12 +58,14 @@ describe Notify do | ||
47 | 58 | ||
48 | subject { Notify.new_user_email(new_user.id, new_user.password) } | 59 | subject { Notify.new_user_email(new_user.id, new_user.password) } |
49 | 60 | ||
61 | + it_behaves_like 'an email sent from GitLab' | ||
62 | + | ||
50 | it 'is sent to the new user' do | 63 | it 'is sent to the new user' do |
51 | should deliver_to new_user.email | 64 | should deliver_to new_user.email |
52 | end | 65 | end |
53 | 66 | ||
54 | it 'has the correct subject' do | 67 | it 'has the correct subject' do |
55 | - should have_subject /^gitlab \| Account was created for you$/i | 68 | + should have_subject /^Account was created for you$/i |
56 | end | 69 | end |
57 | 70 | ||
58 | it 'contains the new user\'s login name' do | 71 | it 'contains the new user\'s login name' do |
@@ -73,12 +86,14 @@ describe Notify do | @@ -73,12 +86,14 @@ describe Notify do | ||
73 | 86 | ||
74 | subject { Notify.new_ssh_key_email(key.id) } | 87 | subject { Notify.new_ssh_key_email(key.id) } |
75 | 88 | ||
89 | + it_behaves_like 'an email sent from GitLab' | ||
90 | + | ||
76 | it 'is sent to the new user' do | 91 | it 'is sent to the new user' do |
77 | should deliver_to key.user.email | 92 | should deliver_to key.user.email |
78 | end | 93 | end |
79 | 94 | ||
80 | it 'has the correct subject' do | 95 | it 'has the correct subject' do |
81 | - should have_subject /^gitlab \| SSH key was added to your account$/i | 96 | + should have_subject /^SSH key was added to your account$/i |
82 | end | 97 | end |
83 | 98 | ||
84 | it 'contains the new ssh key title' do | 99 | it 'contains the new ssh key title' do |
@@ -114,17 +129,24 @@ describe Notify do | @@ -114,17 +129,24 @@ describe Notify do | ||
114 | 129 | ||
115 | context 'for a project' do | 130 | context 'for a project' do |
116 | describe 'items that are assignable, the email' do | 131 | describe 'items that are assignable, the email' do |
132 | + let(:current_user) { create(:user, email: "current@email.com") } | ||
117 | let(:assignee) { create(:user, email: 'assignee@example.com') } | 133 | let(:assignee) { create(:user, email: 'assignee@example.com') } |
118 | let(:previous_assignee) { create(:user, name: 'Previous Assignee') } | 134 | let(:previous_assignee) { create(:user, name: 'Previous Assignee') } |
119 | 135 | ||
120 | shared_examples 'an assignee email' do | 136 | shared_examples 'an assignee email' do |
137 | + it 'is sent as the author' do | ||
138 | + sender = subject.header[:from].addrs[0] | ||
139 | + sender.display_name.should eq(current_user.name) | ||
140 | + sender.address.should eq(gitlab_sender) | ||
141 | + end | ||
142 | + | ||
121 | it 'is sent to the assignee' do | 143 | it 'is sent to the assignee' do |
122 | should deliver_to assignee.email | 144 | should deliver_to assignee.email |
123 | end | 145 | end |
124 | end | 146 | end |
125 | 147 | ||
126 | context 'for issues' do | 148 | context 'for issues' do |
127 | - let(:issue) { create(:issue, assignee: assignee, project: project ) } | 149 | + let(:issue) { create(:issue, author: current_user, assignee: assignee, project: project ) } |
128 | 150 | ||
129 | describe 'that are new' do | 151 | describe 'that are new' do |
130 | subject { Notify.new_issue_email(issue.assignee_id, issue.id) } | 152 | subject { Notify.new_issue_email(issue.assignee_id, issue.id) } |
@@ -132,7 +154,7 @@ describe Notify do | @@ -132,7 +154,7 @@ describe Notify do | ||
132 | it_behaves_like 'an assignee email' | 154 | it_behaves_like 'an assignee email' |
133 | 155 | ||
134 | it 'has the correct subject' do | 156 | it 'has the correct subject' do |
135 | - should have_subject /#{project.name} \| New issue ##{issue.iid} \| #{issue.title}/ | 157 | + should have_subject /#{project.name} \| #{issue.title} \(##{issue.iid}\)/ |
136 | end | 158 | end |
137 | 159 | ||
138 | it 'contains a link to the new issue' do | 160 | it 'contains a link to the new issue' do |
@@ -141,14 +163,18 @@ describe Notify do | @@ -141,14 +163,18 @@ describe Notify do | ||
141 | end | 163 | end |
142 | 164 | ||
143 | describe 'that have been reassigned' do | 165 | describe 'that have been reassigned' do |
144 | - before(:each) { issue.stub(:assignee_id_was).and_return(previous_assignee.id) } | ||
145 | - | ||
146 | - subject { Notify.reassigned_issue_email(recipient.id, issue.id, previous_assignee.id) } | 166 | + subject { Notify.reassigned_issue_email(recipient.id, issue.id, previous_assignee.id, current_user) } |
147 | 167 | ||
148 | it_behaves_like 'a multiple recipients email' | 168 | it_behaves_like 'a multiple recipients email' |
149 | 169 | ||
170 | + it 'is sent as the author' do | ||
171 | + sender = subject.header[:from].addrs[0] | ||
172 | + sender.display_name.should eq(current_user.name) | ||
173 | + sender.address.should eq(gitlab_sender) | ||
174 | + end | ||
175 | + | ||
150 | it 'has the correct subject' do | 176 | it 'has the correct subject' do |
151 | - should have_subject /Changed issue ##{issue.iid} \| #{issue.title}/ | 177 | + should have_subject /#{issue.title} \(##{issue.iid}\)/ |
152 | end | 178 | end |
153 | 179 | ||
154 | it 'contains the name of the previous assignee' do | 180 | it 'contains the name of the previous assignee' do |
@@ -165,12 +191,17 @@ describe Notify do | @@ -165,12 +191,17 @@ describe Notify do | ||
165 | end | 191 | end |
166 | 192 | ||
167 | describe 'status changed' do | 193 | describe 'status changed' do |
168 | - let(:current_user) { create(:user, email: "current@email.com") } | ||
169 | let(:status) { 'closed' } | 194 | let(:status) { 'closed' } |
170 | subject { Notify.issue_status_changed_email(recipient.id, issue.id, status, current_user) } | 195 | subject { Notify.issue_status_changed_email(recipient.id, issue.id, status, current_user) } |
171 | 196 | ||
197 | + it 'is sent as the author' do | ||
198 | + sender = subject.header[:from].addrs[0] | ||
199 | + sender.display_name.should eq(current_user.name) | ||
200 | + sender.address.should eq(gitlab_sender) | ||
201 | + end | ||
202 | + | ||
172 | it 'has the correct subject' do | 203 | it 'has the correct subject' do |
173 | - should have_subject /Changed issue ##{issue.iid} \| #{issue.title}/i | 204 | + should have_subject /#{issue.title} \(##{issue.iid}\)/i |
174 | end | 205 | end |
175 | 206 | ||
176 | it 'contains the new status' do | 207 | it 'contains the new status' do |
@@ -189,7 +220,7 @@ describe Notify do | @@ -189,7 +220,7 @@ describe Notify do | ||
189 | end | 220 | end |
190 | 221 | ||
191 | context 'for merge requests' do | 222 | context 'for merge requests' do |
192 | - let(:merge_request) { create(:merge_request, assignee: assignee, source_project: project, target_project: project) } | 223 | + let(:merge_request) { create(:merge_request, author: current_user, assignee: assignee, source_project: project, target_project: project) } |
193 | 224 | ||
194 | describe 'that are new' do | 225 | describe 'that are new' do |
195 | subject { Notify.new_merge_request_email(merge_request.assignee_id, merge_request.id) } | 226 | subject { Notify.new_merge_request_email(merge_request.assignee_id, merge_request.id) } |
@@ -197,7 +228,7 @@ describe Notify do | @@ -197,7 +228,7 @@ describe Notify do | ||
197 | it_behaves_like 'an assignee email' | 228 | it_behaves_like 'an assignee email' |
198 | 229 | ||
199 | it 'has the correct subject' do | 230 | it 'has the correct subject' do |
200 | - should have_subject /New merge request ##{merge_request.iid}/ | 231 | + should have_subject /#{merge_request.title} \(!#{merge_request.iid}\)/ |
201 | end | 232 | end |
202 | 233 | ||
203 | it 'contains a link to the new merge request' do | 234 | it 'contains a link to the new merge request' do |
@@ -214,14 +245,18 @@ describe Notify do | @@ -214,14 +245,18 @@ describe Notify do | ||
214 | end | 245 | end |
215 | 246 | ||
216 | describe 'that are reassigned' do | 247 | describe 'that are reassigned' do |
217 | - before(:each) { merge_request.stub(:assignee_id_was).and_return(previous_assignee.id) } | ||
218 | - | ||
219 | - subject { Notify.reassigned_merge_request_email(recipient.id, merge_request.id, previous_assignee.id) } | 248 | + subject { Notify.reassigned_merge_request_email(recipient.id, merge_request.id, previous_assignee.id, current_user.id) } |
220 | 249 | ||
221 | it_behaves_like 'a multiple recipients email' | 250 | it_behaves_like 'a multiple recipients email' |
222 | 251 | ||
252 | + it 'is sent as the author' do | ||
253 | + sender = subject.header[:from].addrs[0] | ||
254 | + sender.display_name.should eq(current_user.name) | ||
255 | + sender.address.should eq(gitlab_sender) | ||
256 | + end | ||
257 | + | ||
223 | it 'has the correct subject' do | 258 | it 'has the correct subject' do |
224 | - should have_subject /Changed merge request ##{merge_request.iid}/ | 259 | + should have_subject /#{merge_request.title} \(!#{merge_request.iid}\)/ |
225 | end | 260 | end |
226 | 261 | ||
227 | it 'contains the name of the previous assignee' do | 262 | it 'contains the name of the previous assignee' do |
@@ -245,6 +280,8 @@ describe Notify do | @@ -245,6 +280,8 @@ describe Notify do | ||
245 | let(:user) { create(:user) } | 280 | let(:user) { create(:user) } |
246 | subject { Notify.project_was_moved_email(project.id, user.id) } | 281 | subject { Notify.project_was_moved_email(project.id, user.id) } |
247 | 282 | ||
283 | + it_behaves_like 'an email sent from GitLab' | ||
284 | + | ||
248 | it 'has the correct subject' do | 285 | it 'has the correct subject' do |
249 | should have_subject /Project was moved/ | 286 | should have_subject /Project was moved/ |
250 | end | 287 | end |
@@ -265,6 +302,9 @@ describe Notify do | @@ -265,6 +302,9 @@ describe Notify do | ||
265 | project: project, | 302 | project: project, |
266 | user: user) } | 303 | user: user) } |
267 | subject { Notify.project_access_granted_email(users_project.id) } | 304 | subject { Notify.project_access_granted_email(users_project.id) } |
305 | + | ||
306 | + it_behaves_like 'an email sent from GitLab' | ||
307 | + | ||
268 | it 'has the correct subject' do | 308 | it 'has the correct subject' do |
269 | should have_subject /Access to project was granted/ | 309 | should have_subject /Access to project was granted/ |
270 | end | 310 | end |
@@ -285,6 +325,12 @@ describe Notify do | @@ -285,6 +325,12 @@ describe Notify do | ||
285 | end | 325 | end |
286 | 326 | ||
287 | shared_examples 'a note email' do | 327 | shared_examples 'a note email' do |
328 | + it 'is sent as the author' do | ||
329 | + sender = subject.header[:from].addrs[0] | ||
330 | + sender.display_name.should eq(note_author.name) | ||
331 | + sender.address.should eq(gitlab_sender) | ||
332 | + end | ||
333 | + | ||
288 | it 'is sent to the given recipient' do | 334 | it 'is sent to the given recipient' do |
289 | should deliver_to recipient.email | 335 | should deliver_to recipient.email |
290 | end | 336 | end |
@@ -324,7 +370,7 @@ describe Notify do | @@ -324,7 +370,7 @@ describe Notify do | ||
324 | it_behaves_like 'a note email' | 370 | it_behaves_like 'a note email' |
325 | 371 | ||
326 | it 'has the correct subject' do | 372 | it 'has the correct subject' do |
327 | - should have_subject /Note for commit #{commit.short_id}/ | 373 | + should have_subject /#{commit.title} \(#{commit.short_id}\)/ |
328 | end | 374 | end |
329 | 375 | ||
330 | it 'contains a link to the commit' do | 376 | it 'contains a link to the commit' do |
@@ -342,7 +388,7 @@ describe Notify do | @@ -342,7 +388,7 @@ describe Notify do | ||
342 | it_behaves_like 'a note email' | 388 | it_behaves_like 'a note email' |
343 | 389 | ||
344 | it 'has the correct subject' do | 390 | it 'has the correct subject' do |
345 | - should have_subject /Note for merge request ##{merge_request.iid}/ | 391 | + should have_subject /#{merge_request.title} \(!#{merge_request.iid}\)/ |
346 | end | 392 | end |
347 | 393 | ||
348 | it 'contains a link to the merge request note' do | 394 | it 'contains a link to the merge request note' do |
@@ -360,7 +406,7 @@ describe Notify do | @@ -360,7 +406,7 @@ describe Notify do | ||
360 | it_behaves_like 'a note email' | 406 | it_behaves_like 'a note email' |
361 | 407 | ||
362 | it 'has the correct subject' do | 408 | it 'has the correct subject' do |
363 | - should have_subject /Note for issue ##{issue.iid}/ | 409 | + should have_subject /#{issue.title} \(##{issue.iid}\)/ |
364 | end | 410 | end |
365 | 411 | ||
366 | it 'contains a link to the issue note' do | 412 | it 'contains a link to the issue note' do |
@@ -377,6 +423,8 @@ describe Notify do | @@ -377,6 +423,8 @@ describe Notify do | ||
377 | 423 | ||
378 | subject { Notify.group_access_granted_email(membership.id) } | 424 | subject { Notify.group_access_granted_email(membership.id) } |
379 | 425 | ||
426 | + it_behaves_like 'an email sent from GitLab' | ||
427 | + | ||
380 | it 'has the correct subject' do | 428 | it 'has the correct subject' do |
381 | should have_subject /Access to group was granted/ | 429 | should have_subject /Access to group was granted/ |
382 | end | 430 | end |
@@ -401,6 +449,8 @@ describe Notify do | @@ -401,6 +449,8 @@ describe Notify do | ||
401 | 449 | ||
402 | subject { ActionMailer::Base.deliveries.last } | 450 | subject { ActionMailer::Base.deliveries.last } |
403 | 451 | ||
452 | + it_behaves_like 'an email sent from GitLab' | ||
453 | + | ||
404 | it 'is sent to the new user' do | 454 | it 'is sent to the new user' do |
405 | should deliver_to 'new-email@mail.com' | 455 | should deliver_to 'new-email@mail.com' |
406 | end | 456 | end |
@@ -421,6 +471,12 @@ describe Notify do | @@ -421,6 +471,12 @@ describe Notify do | ||
421 | 471 | ||
422 | subject { Notify.repository_push_email(project.id, 'devs@company.name', user.id, 'master', compare) } | 472 | subject { Notify.repository_push_email(project.id, 'devs@company.name', user.id, 'master', compare) } |
423 | 473 | ||
474 | + it 'is sent as the author' do | ||
475 | + sender = subject.header[:from].addrs[0] | ||
476 | + sender.display_name.should eq(user.name) | ||
477 | + sender.address.should eq(gitlab_sender) | ||
478 | + end | ||
479 | + | ||
424 | it 'is sent to recipient' do | 480 | it 'is sent to recipient' do |
425 | should deliver_to 'devs@company.name' | 481 | should deliver_to 'devs@company.name' |
426 | end | 482 | end |
spec/services/notification_service_spec.rb
@@ -137,11 +137,11 @@ describe NotificationService do | @@ -137,11 +137,11 @@ describe NotificationService do | ||
137 | end | 137 | end |
138 | 138 | ||
139 | def should_email(user_id) | 139 | def should_email(user_id) |
140 | - Notify.should_receive(:reassigned_issue_email).with(user_id, issue.id, issue.assignee_id) | 140 | + Notify.should_receive(:reassigned_issue_email).with(user_id, issue.id, issue.assignee_id, @u_disabled.id) |
141 | end | 141 | end |
142 | 142 | ||
143 | def should_not_email(user_id) | 143 | def should_not_email(user_id) |
144 | - Notify.should_not_receive(:reassigned_issue_email).with(user_id, issue.id, issue.assignee_id) | 144 | + Notify.should_not_receive(:reassigned_issue_email).with(user_id, issue.id, issue.assignee_id, @u_disabled.id) |
145 | end | 145 | end |
146 | end | 146 | end |
147 | 147 | ||
@@ -201,11 +201,11 @@ describe NotificationService do | @@ -201,11 +201,11 @@ describe NotificationService do | ||
201 | end | 201 | end |
202 | 202 | ||
203 | def should_email(user_id) | 203 | def should_email(user_id) |
204 | - Notify.should_receive(:reassigned_merge_request_email).with(user_id, merge_request.id, merge_request.assignee_id) | 204 | + Notify.should_receive(:reassigned_merge_request_email).with(user_id, merge_request.id, merge_request.assignee_id, merge_request.author_id) |
205 | end | 205 | end |
206 | 206 | ||
207 | def should_not_email(user_id) | 207 | def should_not_email(user_id) |
208 | - Notify.should_not_receive(:reassigned_merge_request_email).with(user_id, merge_request.id, merge_request.assignee_id) | 208 | + Notify.should_not_receive(:reassigned_merge_request_email).with(user_id, merge_request.id, merge_request.assignee_id, merge_request.author_id) |
209 | end | 209 | end |
210 | end | 210 | end |
211 | 211 |