Merge branch 'simplify-emails-content' into 'master'

Streamline the content of notification emails

In notification emails, the actual content of the email is often buried under several blocks of chrome — and may even be truncated or completely missing. Ideally, the notification emails would be like *real emails*: a short message of meaningful text, sent from the author of the change that triggered the notification.

This MR includes the following changes to notification emails:

* Remove much of the chrome (e.g. the "GitLab" header)
* Emphasize the content (no more small, grayed-out content)
* Add missing informations to the emails (issue description in "new issue" email, file name in "diff comment" email)
* Add a consistent "View in GitLab" link in the footer
* The assignee is displayed only if someone is assigned
* Fix a rendering bug when viewing emails with [Zimbra](http://www.zimbra.com/)

We use these patches at [Capitaine Train](http://www.capitainetrain.com), and it has been a surprisingly big productivity boost for us.

![Before and after](http://f.cl.ly/items/3n0P2c2v1P0y011c0D3e/Before%20and%20After.png)

app/mailers/emails/groups.rb

@@ -3,7 +3,7 @@ module Emails
     def group_access_granted_email(user_group_id)
       @membership = UsersGroup.find(user_group_id)
       @group = @membership.group
-
+      @target_url = group_url(@group)
       mail(to: @membership.user.email,
            subject: subject("Access to group was granted"))
     end

app/mailers/emails/issues.rb

@@ -3,6 +3,7 @@ module Emails
     def new_issue_email(recipient_id, issue_id)
       @issue = Issue.find(issue_id)
       @project = @issue.project
+      @target_url = project_issue_url(@project, @issue)
       mail(from: sender(@issue.author_id),
            to: recipient(recipient_id),
            subject: subject("#{@issue.title} (##{@issue.iid})"))
@@ -12,6 +13,7 @@ module Emails
       @issue = Issue.find(issue_id)
       @previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id
       @project = @issue.project
+      @target_url = project_issue_url(@project, @issue)
       mail(from: sender(updated_by_user_id),
            to: recipient(recipient_id),
            subject: subject("#{@issue.title} (##{@issue.iid})"))
@@ -21,6 +23,7 @@ module Emails
       @issue = Issue.find issue_id
       @project = @issue.project
       @updated_by = User.find updated_by_user_id
+      @target_url = project_issue_url(@project, @issue)
       mail(from: sender(updated_by_user_id),
            to: recipient(recipient_id),
            subject: subject("#{@issue.title} (##{@issue.iid})"))
@@ -31,6 +34,7 @@ module Emails
       @issue_status = status
       @project = @issue.project
       @updated_by = User.find updated_by_user_id
+      @target_url = project_issue_url(@project, @issue)
       mail(from: sender(updated_by_user_id),
            to: recipient(recipient_id),
            subject: subject("#{@issue.title} (##{@issue.iid})"))

app/mailers/emails/merge_requests.rb

@@ -3,6 +3,7 @@ module Emails
     def new_merge_request_email(recipient_id, merge_request_id)
       @merge_request = MergeRequest.find(merge_request_id)
       @project = @merge_request.project
+      @target_url = project_merge_request_url(@project, @merge_request)
       mail(from: sender(@merge_request.author_id),
            to: recipient(recipient_id),
            subject: subject("#{@merge_request.title} (!#{@merge_request.iid})"))
@@ -12,6 +13,7 @@ module Emails
       @merge_request = MergeRequest.find(merge_request_id)
       @previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id
       @project = @merge_request.project
+      @target_url = project_merge_request_url(@project, @merge_request)
       mail(from: sender(updated_by_user_id),
            to: recipient(recipient_id),
            subject: subject("#{@merge_request.title} (!#{@merge_request.iid})"))
@@ -21,6 +23,7 @@ module Emails
       @merge_request = MergeRequest.find(merge_request_id)
       @updated_by = User.find updated_by_user_id
       @project = @merge_request.project
+      @target_url = project_merge_request_url(@project, @merge_request)
       mail(from: sender(updated_by_user_id),
            to: recipient(recipient_id),
            subject: subject("#{@merge_request.title} (!#{@merge_request.iid})"))
@@ -29,6 +32,7 @@ module Emails
     def merged_merge_request_email(recipient_id, merge_request_id)
       @merge_request = MergeRequest.find(merge_request_id)
       @project = @merge_request.project
+      @target_url = project_merge_request_url(@project, @merge_request)
       mail(from: sender(@merge_request.author_id_of_changes),
            to: recipient(recipient_id),
            subject: subject("#{@merge_request.title} (!#{@merge_request.iid})"))

app/mailers/emails/notes.rb

@@ -4,6 +4,7 @@ module Emails
       @note = Note.find(note_id)
       @commit = @note.noteable
       @project = @note.project
+      @target_url = project_commit_url(@project, @commit, anchor: "note_#{@note.id}")
       mail(from: sender(@note.author_id),
            to: recipient(recipient_id),
            subject: subject("#{@commit.title} (#{@commit.short_id})"))
@@ -13,6 +14,7 @@ module Emails
       @note = Note.find(note_id)
       @issue = @note.noteable
       @project = @note.project
+      @target_url = project_issue_url(@project, @issue, anchor: "note_#{@note.id}")
       mail(from: sender(@note.author_id),
            to: recipient(recipient_id),
            subject: subject("#{@issue.title} (##{@issue.iid})"))
@@ -22,6 +24,7 @@ module Emails
       @note = Note.find(note_id)
       @merge_request = @note.noteable
       @project = @note.project
+      @target_url = project_merge_request_url(@project, @merge_request, anchor: "note_#{@note.id}")
       mail(from: sender(@note.author_id),
            to: recipient(recipient_id),
            subject: subject("#{@merge_request.title} (!#{@merge_request.iid})"))
@@ -30,6 +33,7 @@ module Emails
     def note_wall_email(recipient_id, note_id)
       @note = Note.find(note_id)
       @project = @note.project
+      @target_url = project_wall_url(@note.project, anchor: "note_#{@note.id}")
       mail(from: sender(@note.author_id),
            to: recipient(recipient_id),
            subject: subject("Note on wall"))

app/mailers/emails/profile.rb

@@ -3,6 +3,7 @@ module Emails
     def new_user_email(user_id, password)
       @user = User.find(user_id)
       @password = password
+      @target_url = user_url(@user)
       mail(to: @user.email, subject: subject("Account was created for you"))
     end
 
@@ -15,6 +16,7 @@ module Emails
     def new_ssh_key_email(key_id)
       @key = Key.find(key_id)
       @user = @key.user
+      @target_url = user_url(@user)
       mail(to: @user.email, subject: subject("SSH key was added to your account"))
     end
   end

app/mailers/emails/projects.rb

@@ -3,6 +3,7 @@ module Emails
     def project_access_granted_email(user_project_id)
       @users_project = UsersProject.find user_project_id
       @project = @users_project.project
+      @target_url = project_url(@project)
       mail(to: @users_project.user.email,
            subject: subject("Access to project was granted"))
     end
@@ -10,6 +11,7 @@ module Emails
     def project_was_moved_email(project_id, user_id)
       @user = User.find user_id
       @project = Project.find project_id
+      @target_url = project_url(@project)
       mail(to: @user.email,
            subject: subject("Project was moved"))
     end
@@ -21,6 +23,11 @@ module Emails
       @commits = Commit.decorate(compare.commits)
       @diffs   = compare.diffs
       @branch  = branch
+      if @commits.length > 1
+        @target_url = project_compare_url(@project, from: @commits.first, to: @commits.last)
+      else
+        @target_url = project_commit_url(@project, @compare.commit)
+      end
 
       mail(from: sender(author_id),
            to: recipient,

app/views/layouts/notify.html.haml

@@ -3,20 +3,24 @@
     %meta{content: "text/html; charset=utf-8", "http-equiv" => "Content-Type"}
       %title
         GitLab
+  :css
+    p.details {
+      font-style:italic;
+      color:#777
+    }
+    .footer p {
+      font-size:small;
+      color:#777
+    }
 
   %body
-    %h1{style: "background: #EEE; border-bottom: 1px solid #DDD; color: #474D57; font: normal 20px Helvetica, Arial, sans-serif; margin: 0; padding: 5px 10px; line-height: 32px; font-size: 16px;"}
+    %div.content
-      GitLab
+      = yield
-      - if @project
+    %div.footer{style: "margin-top: 10px;"}
-        \|
+      %p
-        = link_to @project.name_with_namespace, project_url(@project), style: 'color: #29B; text-decoration: none'
+        \—
-    %table{align: "left", border: "0", cellpadding: "0", cellspacing: "0", style: "padding: 10px 0;", width: "100%"}
+        %br
-      %tr
+        - if @project
-        %td{align: "left", style: "margin: 0; padding: 10px;"}
+          You're receiving this notification because you are a member of the #{link_to @project.name_with_namespace, project_url(@project)} project team.
-          = yield
+        - if @target_url
-          %br
+          #{link_to "View in GitLab", @target_url}
-      %tr
-        %td{align: "left", style: "margin: 0; padding: 10px;"}
-          %p{style: "font-size:small;color:#777"}
-            - if @project
-              You're receiving this notification because you are a member of the #{@project.name_with_namespace} project team.

app/views/notify/_note_message.html.haml

@@ -1,6 +1,2 @@
-%p
+%div
-  %strong #{@note.author_name}
-  wrote:
-
-%cite{style: 'color: #666'}
   = markdown(@note.note)

app/views/notify/closed_issue_email.html.haml

@@ -1,5 +1,2 @@
 %p
   = "Issue was closed by #{@updated_by.name}"
-%p
-  = "Issue ##{@issue.iid}"
-  = link_to_gfm truncate(@issue.title, length: 45), project_issue_url(@issue.project, @issue), title: @issue.title

app/views/notify/closed_merge_request_email.html.haml

@@ -1,9 +1,2 @@
 %p
-  = "Merge Request #{@merge_request.iid} was closed by #{@updated_by.name}"
+  = "Merge Request !#{@merge_request.iid} was closed by #{@updated_by.name}"
-%p
-  = link_to_gfm truncate(@merge_request.title, length: 40), project_merge_request_url(@merge_request.target_project, @merge_request)
-%p
-  != merge_path_description(@merge_request, '→')
-%p
-  Assignee: #{@merge_request.author_name} → #{@merge_request.assignee_name}
-

app/views/notify/group_access_granted_email.html.haml

@@ -1,5 +1,2 @@
 %p
   = "You have been granted #{@membership.human_access} access to group"
-%p
-  = link_to group_url(@group) do
-    = @group.name

app/views/notify/issue_status_changed_email.html.haml

@@ -1,5 +1,2 @@
 %p
   = "Issue was #{@issue_status} by #{@updated_by.name}"
-%p
-  = "Issue ##{@issue.iid}"
-  = link_to_gfm truncate(@issue.title, length: 45), project_issue_url(@issue.project, @issue), title: @issue.title

app/views/notify/merged_merge_request_email.html.haml

@@ -1,9 +1,2 @@
 %p
-  = "Merge Request #{@merge_request.iid} was merged"
+  = "Merge Request !#{@merge_request.iid} was merged"
-%p
-  = link_to_gfm truncate(@merge_request.title, length: 40), project_merge_request_url(@merge_request.target_project, @merge_request)
-%p
-  != merge_path_description(@merge_request, '→')
-%p
-  Assignee: #{@merge_request.author_name} → #{@merge_request.assignee_name}
-

app/views/notify/new_issue_email.html.haml

@@ -1,9 +1,6 @@
-%p
+-if @issue.description
-  New Issue was created.
+  = markdown(@issue.description)
-%p
+
-  = "Issue ##{@issue.iid}"
+- if @issue.assignee_id.present?
-  = link_to_gfm truncate(@issue.title, length: 45), project_issue_url(@issue.project, @issue), title: @issue.title
+  %p
-%p
+    Assignee: #{@issue.assignee_name}
-  Author: #{@issue.author_name}
-%p
-  Assignee: #{@issue.assignee_name}

app/views/notify/new_merge_request_email.html.haml

@@ -1,9 +1,9 @@
-%p
+%p.details
-  = "New Merge Request ##{@merge_request.iid}"
-%p
-  = link_to_gfm truncate(@merge_request.title, length: 40), project_merge_request_url(@merge_request.target_project, @merge_request)
-%p
   != merge_path_description(@merge_request, '→')
-%p
-  Assignee: #{@merge_request.author_name} → #{@merge_request.assignee_name}
 
+- if @merge_request.assignee_id.present?
+  %p
+    Assignee: #{@merge_request.author_name} → #{@merge_request.assignee_name}
+
+-if @merge_request.description
+  = markdown(@merge_request.description)

app/views/notify/note_commit_email.html.haml

@@ -1,5 +1,2 @@
-%p
-  = "New comment for Commit #{@commit.short_id}"
-  = link_to_gfm truncate(@commit.title, length: 16), project_commit_url(@note.project, id: @commit.id, anchor: "note_#{@note.id}")
 = render 'note_message'
 

app/views/notify/note_issue_email.html.haml

@@ -1,4 +1 @@
-%p
-  = "New comment for Issue ##{@issue.iid}"
-  = link_to_gfm truncate(@issue.title, length: 35), project_issue_url(@issue.project, @issue, anchor: "note_#{@note.id}")
 = render 'note_message'

app/views/notify/note_merge_request_email.html.haml

@@ -1,8 +1,7 @@
-%p
+- if @note.diff_file_name
-  - if @note.for_diff_line?
+  %p.details
-    = link_to "New comment on diff", diffs_project_merge_request_url(@merge_request.target_project, @merge_request, anchor: "note_#{@note.id}")
+    New comment on diff for
-  - else
+    = link_to @note.diff_file_name, @target_url
-    = link_to "New comment", project_merge_request_url(@merge_request.target_project, @merge_request, anchor: "note_#{@note.id}")
+    \:
-  for Merge Request ##{@merge_request.iid}
+
-  %cite "#{truncate(@merge_request.title, length: 20)}"
 = render 'note_message'

app/views/notify/note_wall_email.html.haml

@@ -1,5 +1 @@
-%p
-  New message on
-  = link_to "Project Wall", project_wall_url(@note.project, anchor: "note_#{@note.id}")
-
 = render 'note_message'

app/views/notify/reassigned_issue_email.html.haml

@@ -1,6 +1,3 @@
-%p
-  = "Reassigned Issue ##{@issue.iid}"
-  = link_to_gfm truncate(@issue.title, length: 30), project_issue_url(@issue.project, @issue)
 %p
   Assignee changed
   - if @previous_assignee

app/views/notify/reassigned_merge_request_email.html.haml

@@ -1,6 +1,3 @@
-%p
-  = "Reassigned Merge Request ##{@merge_request.iid}"
-  = link_to_gfm truncate(@merge_request.title, length: 30), project_merge_request_url(@merge_request.target_project, @merge_request)
 %p
   Assignee changed
   - if @previous_assignee

spec/mailers/notify_spec.rb

@@ -146,7 +146,8 @@ describe Notify do
       end
 
       context 'for issues' do
-        let(:issue) { create(:issue, author: current_user, assignee: assignee, project: project ) }
+        let(:issue) { create(:issue, author: current_user, assignee: assignee, project: project) }
+        let(:issue_with_description) { create(:issue, author: current_user, assignee: assignee, project: project, description: Faker::Lorem.sentence) }
 
         describe 'that are new' do
           subject { Notify.new_issue_email(issue.assignee_id, issue.id) }
@@ -162,6 +163,14 @@ describe Notify do
           end
         end
 
+        describe 'that are new with a description' do
+          subject { Notify.new_issue_email(issue_with_description.assignee_id, issue_with_description.id) }
+
+          it 'contains the description' do
+            should have_body_text /#{issue_with_description.description}/
+          end
+        end
+
         describe 'that have been reassigned' do
           subject { Notify.reassigned_issue_email(recipient.id, issue.id, previous_assignee.id, current_user) }
 
@@ -221,6 +230,7 @@ describe Notify do
 
       context 'for merge requests' do
         let(:merge_request) { create(:merge_request, author: current_user, assignee: assignee, source_project: project, target_project: project) }
+        let(:merge_request_with_description) { create(:merge_request, author: current_user, assignee: assignee, source_project: project, target_project: project, description: Faker::Lorem.sentence) }
 
         describe 'that are new' do
           subject { Notify.new_merge_request_email(merge_request.assignee_id, merge_request.id) }
@@ -244,6 +254,14 @@ describe Notify do
           end
         end
 
+        describe 'that are new with a description' do
+          subject { Notify.new_merge_request_email(merge_request_with_description.assignee_id, merge_request_with_description.id) }
+
+          it 'contains the description' do
+            should have_body_text /#{merge_request_with_description.description}/
+          end
+        end
+
         describe 'that are reassigned' do
           subject { Notify.reassigned_merge_request_email(recipient.id, merge_request.id, previous_assignee.id, current_user.id) }
 
@@ -335,10 +353,6 @@ describe Notify do
           should deliver_to recipient.email
         end
 
-        it 'contains the name of the note\'s author' do
-          should have_body_text /#{note_author.name}/
-        end
-
         it 'contains the message from the note' do
           should have_body_text /#{note.note}/
         end
@@ -468,6 +482,8 @@ describe Notify do
     let(:example_site_path) { root_path }
     let(:user) { create(:user) }
     let(:compare) { Gitlab::Git::Compare.new(project.repository.raw_repository, 'cd5c4bac', 'b1e6a9db') }
+    let(:commits) { Commit.decorate(compare.commits) }
+    let(:diff_path) { project_compare_path(project, from: commits.first, to: commits.last) }
 
     subject { Notify.repository_push_email(project.id, 'devs@company.name', user.id, 'master', compare) }
 
@@ -492,5 +508,9 @@ describe Notify do
     it 'includes diffs' do
       should have_body_text /Checkout wiki pages for installation information/
     end
+
+    it 'contains a link to the diff' do
+      should have_body_text /#{diff_path}/
+    end
   end
 end