Better notification emails for notes and (diff) discussions

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

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,
 

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

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

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

app/views/layouts/mailer.text.haml

@@ -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}

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

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)

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 -%>

app/views/notify/_note_message.html.haml

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

app/views/notify/_note_message.text.erb

@@ -1,5 +0,0 @@
-<% if current_application_settings.email_author_in_body %>
-  <%= @note.author_name %> wrote:
-<% end -%>
-
-<%= @note.note %>

app/views/notify/_note_mr_or_commit_email.html.haml

@@ -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'

app/views/notify/_note_mr_or_commit_email.text.erb

@@ -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' %>

app/views/notify/_simple_diff.text.erb

@@ -1,3 +0,0 @@
-<% @discussion.truncated_diff_lines(highlight: false).each do |line| %>
-> <%= line.text %>
-<% end %>

app/views/notify/note_commit_email.html.haml

@@ -1,2 +1 @@
-%p.details
-  = render 'note_mr_or_commit_email'
+= render 'note_email'

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' %>

app/views/notify/note_issue_email.html.haml

@@ -1 +1 @@
-= render 'note_message'
+= render 'note_email'

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' %>

app/views/notify/note_merge_request_email.html.haml

@@ -1,2 +1 @@
-%p.details
-  = render 'note_mr_or_commit_email'
+= render 'note_email'

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'%>

app/views/notify/note_personal_snippet_email.html.haml

@@ -1 +1 @@
-= render 'note_message'
+= render 'note_email'

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' %>

app/views/notify/note_snippet_email.html.haml

@@ -1 +1 @@
-= render 'note_message'
+= render 'note_email'

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' %>

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

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'