Send emails from the author

This changes the email "From" field from "gitlab@example.com" to either: * "John Doe <gitlab@example.com>" if the author of the action is known, * "GitLab <gitlab@example.com>" otherwise. Rationale: this allow mails to appear as if they were sent by the author. It appears in the mailbox more like a real discussion between the sender and the receiver ("John sent: we should refactor this") and less like a robot notifying about something.

Send emails from the author
This changes the email "From" field from "gitlab@example.com" to either: * "John Doe <gitlab@example.com>" if the author of the action is known, * "GitLab <gitlab@example.com>" otherwise. Rationale: this allow mails to appear as if they were sent by the author. It appears in the mailbox more like a real discussion between the sender and the receiver ("John sent: we should refactor this") and less like a robot notifying about something.
Pierre de La Morinerie
1 parent 57cb1ca7
Showing 9 changed files with 126 additions and 39 deletions Show diff stats
app/mailers/emails/issues.rb
app/mailers/emails/merge_requests.rb
app/mailers/emails/notes.rb
app/mailers/emails/projects.rb
app/mailers/notify.rb
app/services/notification_service.rb
config/initializers/devise.rb
spec/mailers/notify_spec.rb
spec/services/notification_service_spec.rb
@@ -3,15 +3,17 @@ module Emails
     def new_issue_email(recipient_id, issue_id)
       @issue = Issue.find(issue_id)
       @project = @issue.project
-      mail(to: recipient(recipient_id),
+      mail(from: sender(@issue.author_id),
+           to: recipient(recipient_id),
            subject: subject("#{@issue.title} (##{@issue.iid})"))
     end
-    def reassigned_issue_email(recipient_id, issue_id, previous_assignee_id)
+    def reassigned_issue_email(recipient_id, issue_id, previous_assignee_id, updated_by_user_id)
       @issue = Issue.find(issue_id)
       @previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id
       @project = @issue.project
-      mail(to: recipient(recipient_id),
+      mail(from: sender(updated_by_user_id),
+           to: recipient(recipient_id),
            subject: subject("#{@issue.title} (##{@issue.iid})"))
     end
@@ -19,7 +21,8 @@ module Emails
       @issue = Issue.find issue_id
       @project = @issue.project
       @updated_by = User.find updated_by_user_id
-      mail(to: recipient(recipient_id),
+      mail(from: sender(updated_by_user_id),
+           to: recipient(recipient_id),
            subject: subject("#{@issue.title} (##{@issue.iid})"))
     end
@@ -28,7 +31,8 @@ module Emails
       @issue_status = status
       @project = @issue.project
       @updated_by = User.find updated_by_user_id
-      mail(to: recipient(recipient_id),
+      mail(from: sender(updated_by_user_id),
+           to: recipient(recipient_id),
            subject: subject("#{@issue.title} (##{@issue.iid})"))
     end
   end
@@ -3,15 +3,17 @@ module Emails
     def new_merge_request_email(recipient_id, merge_request_id)
       @merge_request = MergeRequest.find(merge_request_id)
       @project = @merge_request.project
-      mail(to: recipient(recipient_id),
+      mail(from: sender(@merge_request.author_id),
+           to: recipient(recipient_id),
            subject: subject("#{@merge_request.title} (!#{@merge_request.iid})"))
     end
-    def reassigned_merge_request_email(recipient_id, merge_request_id, previous_assignee_id)
+    def reassigned_merge_request_email(recipient_id, merge_request_id, previous_assignee_id, updated_by_user_id)
       @merge_request = MergeRequest.find(merge_request_id)
       @previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id
       @project = @merge_request.project
-      mail(to: recipient(recipient_id),
+      mail(from: sender(updated_by_user_id),
+           to: recipient(recipient_id),
            subject: subject("#{@merge_request.title} (!#{@merge_request.iid})"))
     end
@@ -19,14 +21,16 @@ module Emails
       @merge_request = MergeRequest.find(merge_request_id)
       @updated_by = User.find updated_by_user_id
       @project = @merge_request.project
-      mail(to: recipient(recipient_id),
+      mail(from: sender(updated_by_user_id),
+           to: recipient(recipient_id),
            subject: subject("#{@merge_request.title} (!#{@merge_request.iid})"))
     end
     def merged_merge_request_email(recipient_id, merge_request_id)
       @merge_request = MergeRequest.find(merge_request_id)
       @project = @merge_request.project
-      mail(to: recipient(recipient_id),
+      mail(from: sender(@merge_request.author_id_of_changes),
+           to: recipient(recipient_id),
            subject: subject("#{@merge_request.title} (!#{@merge_request.iid})"))
     end
   end
@@ -4,7 +4,8 @@ module Emails
       @note = Note.find(note_id)
       @commit = @note.noteable
       @project = @note.project
-      mail(to: recipient(recipient_id),
+      mail(from: sender(@note.author_id),
+           to: recipient(recipient_id),
            subject: subject("#{@commit.title} (#{@commit.short_id})"))
     end
@@ -12,7 +13,8 @@ module Emails
       @note = Note.find(note_id)
       @issue = @note.noteable
       @project = @note.project
-      mail(to: recipient(recipient_id),
+      mail(from: sender(@note.author_id),
+           to: recipient(recipient_id),
            subject: subject("#{@issue.title} (##{@issue.iid})"))
     end
@@ -20,14 +22,16 @@ module Emails
       @note = Note.find(note_id)
       @merge_request = @note.noteable
       @project = @note.project
-      mail(to: recipient(recipient_id),
+      mail(from: sender(@note.author_id),
+           to: recipient(recipient_id),
            subject: subject("#{@merge_request.title} (!#{@merge_request.iid})"))
     end
     def note_wall_email(recipient_id, note_id)
       @note = Note.find(note_id)
       @project = @note.project
-      mail(to: recipient(recipient_id),
+      mail(from: sender(@note.author_id),
+           to: recipient(recipient_id),
            subject: subject("Note on wall"))
     end
   end
@@ -22,7 +22,9 @@ module Emails
       @diffs   = compare.diffs
       @branch  = branch
-      mail(to: recipient, subject: subject("New push to repository"))
+      mail(from: sender(author_id),
+           to: recipient,
+           subject: subject("New push to repository"))
     end
   end
 end
@@ -15,7 +15,7 @@ class Notify &lt; ActionMailer::Base
   default_url_options[:port]     = Gitlab.config.gitlab.port unless Gitlab.config.gitlab_on_standard_port?
   default_url_options[:script_name] = Gitlab.config.gitlab.relative_url_root
-  default from: Gitlab.config.gitlab.email_from
+  default from: Proc.new { default_sender_address.format }
   default reply_to: "noreply@#{Gitlab.config.gitlab.host}"
   # Just send email with 2 seconds delay
@@ -25,6 +25,23 @@ class Notify &lt; ActionMailer::Base
   private
+  # The default email address to send emails from
+  def default_sender_address
+    address = Mail::Address.new(Gitlab.config.gitlab.email_from)
+    address.display_name = "GitLab"
+    address
+  end
+
+  # Return an email address that displays the name of the sender.
+  # Only the displayed name changes; the actual email address is always the same.
+  def sender(sender_id)
+    if sender = User.find(sender_id)
+      address = default_sender_address
+      address.display_name = sender.name
+      address.format
+    end
+  end
+
   # Look up a User by their ID and return their email address
   #
   # recipient_id - User ID
@@ -257,7 +257,7 @@ class NotificationService
     recipients.delete(current_user)
     recipients.each do |recipient|
-      mailer.send(method, recipient.id, target.id, target.assignee_id_was)
+      mailer.send(method, recipient.id, target.id, target.assignee_id_was, current_user.id)
     end
   end
@@ -4,7 +4,7 @@ Devise.setup do |config|
   # ==> Mailer Configuration
   # Configure the e-mail address which will be shown in Devise::Mailer,
   # note that it will be overwritten if you use your own mailer class with default "from" parameter.
-  config.mailer_sender = Gitlab.config.gitlab.email_from
+  config.mailer_sender = "GitLab <#{Gitlab.config.gitlab.email_from}>"
   # Configure the class responsible to send e-mails.
@@ -4,6 +4,7 @@ describe Notify do
   include EmailSpec::Helpers
   include EmailSpec::Matchers
+  let(:gitlab_sender) { Gitlab.config.gitlab.email_from }
   let(:recipient) { create(:user, email: 'recipient@example.com') }
   let(:project) { create(:project) }
@@ -13,12 +14,22 @@ describe Notify do
     end
   end
+  shared_examples 'an email sent from GitLab' do
+    it 'is sent from GitLab' do
+      sender = subject.header[:from].addrs[0]
+      sender.display_name.should eq('GitLab')
+      sender.address.should eq(gitlab_sender)
+    end
+  end
+
   describe 'for new users, the email' do
     let(:example_site_path) { root_path }
     let(:new_user) { create(:user, email: 'newguy@example.com', created_by_id: 1) }
     subject { Notify.new_user_email(new_user.id, new_user.password) }
+    it_behaves_like 'an email sent from GitLab'
+
     it 'is sent to the new user' do
       should deliver_to new_user.email
     end
@@ -47,6 +58,8 @@ describe Notify do
     subject { Notify.new_user_email(new_user.id, new_user.password) }
+    it_behaves_like 'an email sent from GitLab'
+
     it 'is sent to the new user' do
       should deliver_to new_user.email
     end
@@ -73,6 +86,8 @@ describe Notify do
     subject { Notify.new_ssh_key_email(key.id) }
+    it_behaves_like 'an email sent from GitLab'
+
     it 'is sent to the new user' do
       should deliver_to key.user.email
     end
@@ -114,17 +129,24 @@ describe Notify do
   context 'for a project' do
     describe 'items that are assignable, the email' do
+      let(:current_user) { create(:user, email: "current@email.com") }
       let(:assignee) { create(:user, email: 'assignee@example.com') }
       let(:previous_assignee) { create(:user, name: 'Previous Assignee') }
       shared_examples 'an assignee email' do
+        it 'is sent as the author' do
+          sender = subject.header[:from].addrs[0]
+          sender.display_name.should eq(current_user.name)
+          sender.address.should eq(gitlab_sender)
+        end
+
         it 'is sent to the assignee' do
           should deliver_to assignee.email
         end
       end
       context 'for issues' do
-        let(:issue) { create(:issue, assignee: assignee, project: project ) }
+        let(:issue) { create(:issue, author: current_user, assignee: assignee, project: project ) }
         describe 'that are new' do
           subject { Notify.new_issue_email(issue.assignee_id, issue.id) }
@@ -132,7 +154,7 @@ describe Notify do
           it_behaves_like 'an assignee email'
           it 'has the correct subject' do
-            should have_subject /#{project.name} \| New issue ##{issue.iid} \| #{issue.title}/
+            should have_subject /#{project.name} \| #{issue.title} \(##{issue.iid}\)/
           end
           it 'contains a link to the new issue' do
@@ -141,14 +163,18 @@ describe Notify do
         end
         describe 'that have been reassigned' do
-          before(:each) { issue.stub(:assignee_id_was).and_return(previous_assignee.id) }
-
-          subject { Notify.reassigned_issue_email(recipient.id, issue.id, previous_assignee.id) }
+          subject { Notify.reassigned_issue_email(recipient.id, issue.id, previous_assignee.id, current_user) }
           it_behaves_like 'a multiple recipients email'
+          it 'is sent as the author' do
+            sender = subject.header[:from].addrs[0]
+            sender.display_name.should eq(current_user.name)
+            sender.address.should eq(gitlab_sender)
+          end
+
           it 'has the correct subject' do
-            should have_subject /Changed issue ##{issue.iid} \| #{issue.title}/
+            should have_subject /#{issue.title} \(##{issue.iid}\)/
           end
           it 'contains the name of the previous assignee' do
@@ -165,12 +191,17 @@ describe Notify do
         end
         describe 'status changed' do
-          let(:current_user) { create(:user, email: "current@email.com") }
           let(:status) { 'closed' }
           subject { Notify.issue_status_changed_email(recipient.id, issue.id, status, current_user) }
+          it 'is sent as the author' do
+            sender = subject.header[:from].addrs[0]
+            sender.display_name.should eq(current_user.name)
+            sender.address.should eq(gitlab_sender)
+          end
+
           it 'has the correct subject' do
-            should have_subject /Changed issue ##{issue.iid} \| #{issue.title}/i
+            should have_subject /#{issue.title} \(##{issue.iid}\)/i
           end
           it 'contains the new status' do
@@ -189,7 +220,7 @@ describe Notify do
       end
       context 'for merge requests' do
-        let(:merge_request) { create(:merge_request, assignee: assignee, source_project: project, target_project: project) }
+        let(:merge_request) { create(:merge_request, author: current_user, assignee: assignee, source_project: project, target_project: project) }
         describe 'that are new' do
           subject { Notify.new_merge_request_email(merge_request.assignee_id, merge_request.id) }
@@ -197,7 +228,7 @@ describe Notify do
           it_behaves_like 'an assignee email'
           it 'has the correct subject' do
-            should have_subject /New merge request ##{merge_request.iid}/
+            should have_subject /#{merge_request.title} \(!#{merge_request.iid}\)/
           end
           it 'contains a link to the new merge request' do
@@ -214,14 +245,18 @@ describe Notify do
         end
         describe 'that are reassigned' do
-          before(:each) { merge_request.stub(:assignee_id_was).and_return(previous_assignee.id) }
-
-          subject { Notify.reassigned_merge_request_email(recipient.id, merge_request.id, previous_assignee.id) }
+          subject { Notify.reassigned_merge_request_email(recipient.id, merge_request.id, previous_assignee.id, current_user.id) }
           it_behaves_like 'a multiple recipients email'
+          it 'is sent as the author' do
+            sender = subject.header[:from].addrs[0]
+            sender.display_name.should eq(current_user.name)
+            sender.address.should eq(gitlab_sender)
+          end
+
           it 'has the correct subject' do
-            should have_subject /Changed merge request ##{merge_request.iid}/
+            should have_subject /#{merge_request.title} \(!#{merge_request.iid}\)/
           end
           it 'contains the name of the previous assignee' do
@@ -245,6 +280,8 @@ describe Notify do
       let(:user) { create(:user) }
       subject { Notify.project_was_moved_email(project.id, user.id) }
+      it_behaves_like 'an email sent from GitLab'
+
       it 'has the correct subject' do
         should have_subject /Project was moved/
       end
@@ -265,6 +302,9 @@ describe Notify do
                                    project: project,
                                    user: user) }
       subject { Notify.project_access_granted_email(users_project.id) }
+
+      it_behaves_like 'an email sent from GitLab'
+
       it 'has the correct subject' do
         should have_subject /Access to project was granted/
       end
@@ -285,6 +325,12 @@ describe Notify do
       end
       shared_examples 'a note email' do
+        it 'is sent as the author' do
+          sender = subject.header[:from].addrs[0]
+          sender.display_name.should eq(note_author.name)
+          sender.address.should eq(gitlab_sender)
+        end
+
         it 'is sent to the given recipient' do
           should deliver_to recipient.email
         end
@@ -324,7 +370,7 @@ describe Notify do
         it_behaves_like 'a note email'
         it 'has the correct subject' do
-          should have_subject /Note for commit #{commit.short_id}/
+          should have_subject /#{commit.title} \(#{commit.short_id}\)/
         end
         it 'contains a link to the commit' do
@@ -342,7 +388,7 @@ describe Notify do
         it_behaves_like 'a note email'
         it 'has the correct subject' do
-          should have_subject /Note for merge request ##{merge_request.iid}/
+          should have_subject /#{merge_request.title} \(!#{merge_request.iid}\)/
         end
         it 'contains a link to the merge request note' do
@@ -360,7 +406,7 @@ describe Notify do
         it_behaves_like 'a note email'
         it 'has the correct subject' do
-          should have_subject /Note for issue ##{issue.iid}/
+          should have_subject /#{issue.title} \(##{issue.iid}\)/
         end
         it 'contains a link to the issue note' do
@@ -377,6 +423,8 @@ describe Notify do
     subject { Notify.group_access_granted_email(membership.id) }
+    it_behaves_like 'an email sent from GitLab'
+
     it 'has the correct subject' do
       should have_subject /Access to group was granted/
     end
@@ -401,6 +449,8 @@ describe Notify do
     subject { ActionMailer::Base.deliveries.last }
+    it_behaves_like 'an email sent from GitLab'
+
     it 'is sent to the new user' do
       should deliver_to 'new-email@mail.com'
     end
@@ -421,6 +471,12 @@ describe Notify do
     subject { Notify.repository_push_email(project.id, 'devs@company.name', user.id, 'master', compare) }
+    it 'is sent as the author' do
+      sender = subject.header[:from].addrs[0]
+      sender.display_name.should eq(user.name)
+      sender.address.should eq(gitlab_sender)
+    end
+
     it 'is sent to recipient' do
       should deliver_to 'devs@company.name'
     end
@@ -137,11 +137,11 @@ describe NotificationService do
       end
       def should_email(user_id)
-        Notify.should_receive(:reassigned_issue_email).with(user_id, issue.id, issue.assignee_id)
+        Notify.should_receive(:reassigned_issue_email).with(user_id, issue.id, issue.assignee_id, @u_disabled.id)
       end
       def should_not_email(user_id)
-        Notify.should_not_receive(:reassigned_issue_email).with(user_id, issue.id, issue.assignee_id)
+        Notify.should_not_receive(:reassigned_issue_email).with(user_id, issue.id, issue.assignee_id, @u_disabled.id)
       end
     end
@@ -201,11 +201,11 @@ describe NotificationService do
       end
       def should_email(user_id)
-        Notify.should_receive(:reassigned_merge_request_email).with(user_id, merge_request.id, merge_request.assignee_id)
+        Notify.should_receive(:reassigned_merge_request_email).with(user_id, merge_request.id, merge_request.assignee_id, merge_request.author_id)
       end
       def should_not_email(user_id)
-        Notify.should_not_receive(:reassigned_merge_request_email).with(user_id, merge_request.id, merge_request.assignee_id)
+        Notify.should_not_receive(:reassigned_merge_request_email).with(user_id, merge_request.id, merge_request.assignee_id, merge_request.author_id)
       end
     end
	@@ -15,7 +15,7 @@ class Notify < ActionMailer::Base		@@ -15,7 +15,7 @@ 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 2 seconds delay	21	# Just send email with 2 seconds delay
	@@ -25,6 +25,23 @@ class Notify < ActionMailer::Base		@@ -25,6 +25,23 @@ class Notify < ActionMailer::Base
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
	@@ -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
	@@ -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.
	@@ -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