Better notification emails for notes and (diff) discussions

26 files changed, 106 insertions, 84 deletions

diff --git a/app/mailers/emails/notes.rb b/app/mailers/emails/notes.rb
index 46fa6fd9f6d..de58314fef2 100644
--- a/app/mailers/emails/notes.rb
+++ b/app/mailers/emails/notes.rb
@@ -4,7 +4,7 @@ module Emails
       setup_note_mail(note_id, recipient_id)
 
       @commit = @note.noteable
-      @discussion = @note.to_discussion if @note.diff_note?
+      @discussion = @note.discussion if @note.part_of_discussion?
       @target_url = namespace_project_commit_url(*note_target_url_options)
 
       mail_answer_thread(@commit,
@@ -17,6 +17,7 @@ module Emails
       setup_note_mail(note_id, recipient_id)
 
       @issue = @note.noteable
+      @discussion = @note.discussion if @note.part_of_discussion?
       @target_url = namespace_project_issue_url(*note_target_url_options)
       mail_answer_thread(@issue, note_thread_options(recipient_id))
     end
@@ -25,7 +26,7 @@ module Emails
       setup_note_mail(note_id, recipient_id)
 
       @merge_request = @note.noteable
-      @discussion = @note.to_discussion if @note.diff_note?
+      @discussion = @note.discussion if @note.part_of_discussion?
       @target_url = namespace_project_merge_request_url(*note_target_url_options)
       mail_answer_thread(@merge_request, note_thread_options(recipient_id))
     end
@@ -34,6 +35,7 @@ module Emails
       setup_note_mail(note_id, recipient_id)
 
       @snippet = @note.noteable
+      @discussion = @note.discussion if @note.part_of_discussion?
       @target_url = namespace_project_snippet_url(*note_target_url_options)
       mail_answer_thread(@snippet, note_thread_options(recipient_id))
     end
@@ -42,6 +44,7 @@ module Emails
       setup_note_mail(note_id, recipient_id)
 
       @snippet = @note.noteable
+      @discussion = @note.discussion if @note.part_of_discussion?
       @target_url = snippet_url(@note.noteable)
       mail_answer_thread(@snippet, note_thread_options(recipient_id))
     end
diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb
index 9a934c7520c..9c80b190b74 100644
--- a/app/models/diff_discussion.rb
+++ b/app/models/diff_discussion.rb
@@ -4,12 +4,14 @@ class DiffDiscussion < Discussion
   delegate  :line_code,
             :original_line_code,
             :diff_file,
+            :diff_line,
             :for_line?,
             :active?,
 
             to: :first_note
 
-  delegate  :blob,
+  delegate  :file_path,
+            :blob,
             :highlighted_diff_lines,
             :diff_lines,
 
diff --git a/app/models/discussion.rb b/app/models/discussion.rb
index 314aea2c63a..8ab9031e42c 100644
--- a/app/models/discussion.rb
+++ b/app/models/discussion.rb
@@ -87,6 +87,10 @@ class Discussion
     false
   end
 
+  def new_discussion?
+    notes.length == 1
+  end
+
   def potentially_resolvable?
     first_note.for_merge_request?
   end
diff --git a/app/models/note.rb b/app/models/note.rb
index 6385747b571..06ceb60b982 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -261,7 +261,7 @@ class Note < ActiveRecord::Base
   # Returns the entire discussion this note is part of
   def discussion
     if part_of_discussion?
-      self.noteable.notes.find_discussion(self.discussion_id)
+      self.noteable.notes.find_discussion(self.discussion_id) || to_discussion
     else
       to_discussion
     end
diff --git a/app/views/layouts/mailer.text.erb b/app/views/layouts/mailer.text.erb
new file mode 100644
index 00000000000..198f30a1dc4
--- /dev/null
+++ b/app/views/layouts/mailer.text.erb
@@ -0,0 +1,4 @@
+<%= yield -%>
+
+---
+You're receiving this email because of your account on <%= Gitlab.config.gitlab.host %>.
diff --git a/app/views/layouts/mailer.text.haml b/app/views/layouts/mailer.text.haml
deleted file mode 100644
index 6a9c6ced9cc..00000000000
--- a/app/views/layouts/mailer.text.haml
+++ /dev/null
@@ -1,5 +0,0 @@
-= yield
-
-You're receiving this email because of your account on #{Gitlab.config.gitlab.host}.
-Manage all notifications: #{profile_notifications_url}
-Help: #{help_url}
diff --git a/app/views/layouts/notify.text.erb b/app/views/layouts/notify.text.erb
new file mode 100644
index 00000000000..b4ce02eead8
--- /dev/null
+++ b/app/views/layouts/notify.text.erb
@@ -0,0 +1,12 @@
+<%= yield -%>
+
+---
+<%  if @target_url -%>
+<%    if @reply_by_email -%>
+<%=     "Reply to this email directly or view it on GitLab: #{@target_url}" -%>
+<%    else -%>
+<%=     "View it on GitLab: #{@target_url}" -%>
+<%    end -%>
+<%  end -%>
+
+You're receiving this email because of your account on <%= Gitlab.config.gitlab.host %>.
diff --git a/app/views/notify/_note_email.html.haml b/app/views/notify/_note_email.html.haml
new file mode 100644
index 00000000000..8b139a150b7
--- /dev/null
+++ b/app/views/notify/_note_email.html.haml
@@ -0,0 +1,36 @@
+- if @discussion
+  %p.details
+    = succeed ':' do
+      = link_to @note.author_name, user_url(@note.author)
+
+      - if @discussion.diff_discussion?
+        - if @discussion.new_discussion?
+          started a new discussion
+        - else
+          commented on a discussion
+
+        on #{link_to @discussion.file_path, @target_url}
+      - else
+        - if @discussion.new_discussion?
+          started a new discussion
+        - else
+          commented on a #{link_to 'discussion', @target_url}
+
+- elsif current_application_settings.email_author_in_body
+  %p.details
+    #{link_to @note.author_name, user_url(@note.author)} commented:
+
+- if @discussion&.diff_discussion?
+  = content_for :head do
+    = stylesheet_link_tag 'mailers/highlighted_diff_email'
+
+  %table
+    = render partial: "projects/diffs/line",
+      collection: @discussion.truncated_diff_lines,
+      as: :line,
+      locals: { diff_file: @discussion.diff_file,
+        plain: true,
+        email: true }
+
+%div
+  = markdown(@note.note, pipeline: :email, author: @note.author)
diff --git a/app/views/notify/_note_email.text.erb b/app/views/notify/_note_email.text.erb
new file mode 100644
index 00000000000..f2db321859f
--- /dev/null
+++ b/app/views/notify/_note_email.text.erb
@@ -0,0 +1,25 @@
+<%  if @discussion -%>
+<%=   @note.author_name -%>
+<%    if @discussion.new_discussion? -%>
+<%=     " started a new discussion" -%>
+<%    else -%>
+<%=     " commented on a discussion" -%>
+<%    end -%>
+<%    if @discussion.diff_discussion? -%>
+<%=     " on #{@discussion.file_path}" -%>
+<%    end -%>
+<%=   ":" -%>
+
+
+<%  elsif current_application_settings.email_author_in_body -%>
+<%=     "#{@note.author_name} commented:" -%>
+
+
+<%  end -%>
+<%  if @discussion&.diff_discussion? -%>
+<%    @discussion.truncated_diff_lines(highlight: false).each do |line| -%>
+<%=     "> #{line.text}\n" -%>
+<%    end -%>
+
+<%  end -%>
+<%= @note.note -%>
diff --git a/app/views/notify/_note_message.html.haml b/app/views/notify/_note_message.html.haml
deleted file mode 100644
index e9c66170877..00000000000
--- a/app/views/notify/_note_message.html.haml
+++ /dev/null
@@ -1,5 +0,0 @@
-- if current_application_settings.email_author_in_body
-  %div
-    #{link_to @note.author_name, user_url(@note.author)} wrote:
-%div
-  = markdown(@note.note, pipeline: :email, author: @note.author)
diff --git a/app/views/notify/_note_message.text.erb b/app/views/notify/_note_message.text.erb
deleted file mode 100644
index f82cbc9a3fc..00000000000
--- a/app/views/notify/_note_message.text.erb
+++ /dev/null
@@ -1,5 +0,0 @@
-<% if current_application_settings.email_author_in_body %>
-  <%= @note.author_name %> wrote:
-<% end -%>
-
-<%= @note.note %>
diff --git a/app/views/notify/_note_mr_or_commit_email.html.haml b/app/views/notify/_note_mr_or_commit_email.html.haml
deleted file mode 100644
index edf8dfe7e9e..00000000000
--- a/app/views/notify/_note_mr_or_commit_email.html.haml
+++ /dev/null
@@ -1,18 +0,0 @@
-= content_for :head do
-  = stylesheet_link_tag 'mailers/highlighted_diff_email'
-
-New comment
-
-- if @discussion && @discussion.diff_file
-  on
-  = link_to @note.diff_file.file_path, @target_url, class: 'details'
-  \:
-  %table
-    = render partial: "projects/diffs/line",
-      collection: @discussion.truncated_diff_lines,
-      as: :line,
-      locals: { diff_file: @note.diff_file,
-        plain: true,
-        email: true }
-
-= render 'note_message'
diff --git a/app/views/notify/_note_mr_or_commit_email.text.erb b/app/views/notify/_note_mr_or_commit_email.text.erb
deleted file mode 100644
index b4fcdf6b1e9..00000000000
--- a/app/views/notify/_note_mr_or_commit_email.text.erb
+++ /dev/null
@@ -1,8 +0,0 @@
-<% if @discussion && @discussion.diff_file -%>
- on <%= @note.diff_file.file_path -%>
-<% end -%>:
-
-<%= url %>
-
-<%= render 'simple_diff' if @discussion -%>
-<%= render 'note_message' %>
diff --git a/app/views/notify/_simple_diff.text.erb b/app/views/notify/_simple_diff.text.erb
deleted file mode 100644
index c28d1cc34d3..00000000000
--- a/app/views/notify/_simple_diff.text.erb
+++ /dev/null
@@ -1,3 +0,0 @@
-<% @discussion.truncated_diff_lines(highlight: false).each do |line| %>
-> <%= line.text %>
-<% end %>
diff --git a/app/views/notify/note_commit_email.html.haml b/app/views/notify/note_commit_email.html.haml
index 0a650e3b2ca..5e69f01a486 100644
--- a/app/views/notify/note_commit_email.html.haml
+++ b/app/views/notify/note_commit_email.html.haml
@@ -1,2 +1 @@
-%p.details
-  = render 'note_mr_or_commit_email'
+= render 'note_email'
diff --git a/app/views/notify/note_commit_email.text.erb b/app/views/notify/note_commit_email.text.erb
index 6aa085a172e..5585e7c3ee8 100644
--- a/app/views/notify/note_commit_email.text.erb
+++ b/app/views/notify/note_commit_email.text.erb
@@ -1,2 +1 @@
-New comment for Commit <%= @commit.short_id -%>
-<%= render partial: 'note_mr_or_commit_email', locals: { url: @target_url } %>
+<%= render partial: 'note_email' %>
diff --git a/app/views/notify/note_issue_email.html.haml b/app/views/notify/note_issue_email.html.haml
index 2fa2f784661..5e69f01a486 100644
--- a/app/views/notify/note_issue_email.html.haml
+++ b/app/views/notify/note_issue_email.html.haml
@@ -1 +1 @@
-= render 'note_message'
+= render 'note_email'
diff --git a/app/views/notify/note_issue_email.text.erb b/app/views/notify/note_issue_email.text.erb
index e33cbcd70f2..5585e7c3ee8 100644
--- a/app/views/notify/note_issue_email.text.erb
+++ b/app/views/notify/note_issue_email.text.erb
@@ -1,9 +1 @@
-New comment for Issue <%= @issue.iid %>
-
-<%= url_for(namespace_project_issue_url(@issue.project.namespace, @issue.project, @issue, anchor: "note_#{@note.id}")) %>
-
-
-Author: <%= @note.author_name %>
-
-<%= @note.note %>
-
+<%= render partial: 'note_email' %>
diff --git a/app/views/notify/note_merge_request_email.html.haml b/app/views/notify/note_merge_request_email.html.haml
index 0a650e3b2ca..5e69f01a486 100644
--- a/app/views/notify/note_merge_request_email.html.haml
+++ b/app/views/notify/note_merge_request_email.html.haml
@@ -1,2 +1 @@
-%p.details
-  = render 'note_mr_or_commit_email'
+= render 'note_email'
diff --git a/app/views/notify/note_merge_request_email.text.erb b/app/views/notify/note_merge_request_email.text.erb
index 2ce64c494cf..381f0366971 100644
--- a/app/views/notify/note_merge_request_email.text.erb
+++ b/app/views/notify/note_merge_request_email.text.erb
@@ -1,2 +1 @@
-New comment for Merge Request <%= @merge_request.to_reference -%>
-<%= render partial: 'note_mr_or_commit_email', locals: { url: @target_url }%>
+<%= render partial: 'note_email'%>
diff --git a/app/views/notify/note_personal_snippet_email.html.haml b/app/views/notify/note_personal_snippet_email.html.haml
index 2fa2f784661..5e69f01a486 100644
--- a/app/views/notify/note_personal_snippet_email.html.haml
+++ b/app/views/notify/note_personal_snippet_email.html.haml
@@ -1 +1 @@
-= render 'note_message'
+= render 'note_email'
diff --git a/app/views/notify/note_personal_snippet_email.text.erb b/app/views/notify/note_personal_snippet_email.text.erb
index b2a8809a23b..5585e7c3ee8 100644
--- a/app/views/notify/note_personal_snippet_email.text.erb
+++ b/app/views/notify/note_personal_snippet_email.text.erb
@@ -1,8 +1 @@
-New comment for Snippet <%= @snippet.id %>
-
-<%= url_for(snippet_url(@snippet, anchor: "note_#{@note.id}")) %>
-
-
-Author: <%= @note.author_name %>
-
-<%= @note.note %>
+<%= render partial: 'note_email' %>
diff --git a/app/views/notify/note_snippet_email.html.haml b/app/views/notify/note_snippet_email.html.haml
index 2fa2f784661..5e69f01a486 100644
--- a/app/views/notify/note_snippet_email.html.haml
+++ b/app/views/notify/note_snippet_email.html.haml
@@ -1 +1 @@
-= render 'note_message'
+= render 'note_email'
diff --git a/app/views/notify/note_snippet_email.text.erb b/app/views/notify/note_snippet_email.text.erb
index 4d5a406f4b0..5585e7c3ee8 100644
--- a/app/views/notify/note_snippet_email.text.erb
+++ b/app/views/notify/note_snippet_email.text.erb
@@ -1,8 +1 @@
-New comment for Snippet <%= @snippet.id %>
-
-<%= url_for(namespace_project_snippet_url(@snippet.project.namespace, @snippet.project, @snippet, anchor: "note_#{@note.id}")) %>
-
-
-Author: <%= @note.author_name %>
-
-<%= @note.note %>
+<%= render partial: 'note_email' %>
diff --git a/lib/gitlab/diff/line.rb b/lib/gitlab/diff/line.rb
index 114656958e3..0a15c6d9358 100644
--- a/lib/gitlab/diff/line.rb
+++ b/lib/gitlab/diff/line.rb
@@ -33,6 +33,10 @@ module Gitlab
         new_pos unless removed? || meta?
       end
 
+      def line
+        new_line || old_line
+      end
+
       def unchanged?
         type.nil?
       end
diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb
index 6a89b007f96..01246f87dff 100644
--- a/spec/mailers/notify_spec.rb
+++ b/spec/mailers/notify_spec.rb
@@ -536,6 +536,8 @@ describe Notify do
         allow(Note).to receive(:find).with(note.id).and_return(note)
       end
 
+      # TODO: Test discussions
+
       shared_examples 'a note email' do
         it_behaves_like 'it should have Gmail Actions links'