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).

2 files changed, 80 insertions, 24 deletions

diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb
index 88cae0bb756..26b7ca876fc 100644
--- a/spec/mailers/notify_spec.rb
+++ b/spec/mailers/notify_spec.rb
@@ -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,18 +14,28 @@ 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
 
     it 'has the correct subject' do
-      should have_subject /^gitlab \| Account was created for you$/i
+      should have_subject /^Account was created for you$/i
     end
 
     it 'contains the new user\'s login name' do
@@ -47,12 +58,14 @@ 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
 
     it 'has the correct subject' do
-      should have_subject /^gitlab \| Account was created for you$/i
+      should have_subject /^Account was created for you$/i
     end
 
     it 'contains the new user\'s login name' do
@@ -73,12 +86,14 @@ 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
 
     it 'has the correct subject' do
-      should have_subject /^gitlab \| SSH key was added to your account$/i
+      should have_subject /^SSH key was added to your account$/i
     end
 
     it 'contains the new ssh key title' do
@@ -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
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index e378be04255..077ad8b6e12 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -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