Add keyword arguments to truncated_diff method

* Added keyword arguments to truncated_diff_lines method to allow for using highlighting or not (html templates vs. text)
* Tweaked templates for consistency and format appropriateness

13 files changed, 38 insertions, 55 deletions

diff --git a/app/models/discussion.rb b/app/models/discussion.rb
index 486bfd2c766..9bd37fe6d89 100644
--- a/app/models/discussion.rb
+++ b/app/models/discussion.rb
@@ -27,7 +27,7 @@ class Discussion
 
   delegate  :blob,
             :highlighted_diff_lines,
-            :text_parsed_diff_lines,
+            :diff_lines,
 
             to: :diff_file,
             allow_nil: true
@@ -164,10 +164,11 @@ class Discussion
   end
 
   # Returns an array of at most 16 highlighted lines above a diff note
-  def truncated_diff_lines
+  def truncated_diff_lines(highlight: true)
+    initial_lines = highlight ? highlighted_diff_lines : diff_lines
     prev_lines = []
 
-    diff_file.diff_lines.each do |line|
+    initial_lines.each do |line|
       if line.meta?
         prev_lines.clear
       else
diff --git a/app/views/discussions/_diff_with_notes.html.haml b/app/views/discussions/_diff_with_notes.html.haml
index 06493ba0105..5c667e4842b 100644
--- a/app/views/discussions/_diff_with_notes.html.haml
+++ b/app/views/discussions/_diff_with_notes.html.haml
@@ -9,7 +9,7 @@
     %table
       - discussions = { discussion.original_line_code => discussion }
       = render partial: "projects/diffs/line",
-        collection: discussion.highlighted_diff_lines(discussion.truncated_diff_lines),
+        collection: discussion.highlighted_diff_lines,
         as: :line,
         locals: { diff_file: diff_file,
           discussions: discussions,
diff --git a/app/views/notify/_note_message.text.erb b/app/views/notify/_note_message.text.erb
new file mode 100644
index 00000000000..f82cbc9a3fc
--- /dev/null
+++ b/app/views/notify/_note_message.text.erb
@@ -0,0 +1,5 @@
+<% 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
index 7033842b557..15e92c42b14 100644
--- a/app/views/notify/_note_mr_or_commit_email.html.haml
+++ b/app/views/notify/_note_mr_or_commit_email.html.haml
@@ -1,17 +1,20 @@
 = content_for :head do
   = stylesheet_link_tag 'mailers/highlighted_diff_email'
 
+New comment
+
 - if @note.diff_note? && @note.diff_file
   on
   = link_to @note.diff_file.file_path, @target_url, class: 'details'
-  \:
+\:
 
+- if @discussion
   %table
     = render partial: "projects/diffs/line",
-      collection: @discussion.highlighted_diff_lines(@discussion.truncated_diff_lines),
+      collection: @discussion.truncated_diff_lines,
       as: :line,
       locals: { diff_file: @note.diff_file,
-      plain: true,
-      email: true }
+        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
new file mode 100644
index 00000000000..3dd1b4d4c0e
--- /dev/null
+++ b/app/views/notify/_note_mr_or_commit_email.text.erb
@@ -0,0 +1,8 @@
+<% if @note.diff_note? && @note.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
index a5a796bc168..58b0018c0ca 100644
--- a/app/views/notify/_simple_diff.text.erb
+++ b/app/views/notify/_simple_diff.text.erb
@@ -1,4 +1,3 @@
-<% lines = @discussion.truncated_diff_lines %>
-<% @discussion.text_parsed_diff_lines(lines).each do |line| %>
-  <%= line %>
+<% @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 17dcf36689f..0a650e3b2ca 100644
--- a/app/views/notify/note_commit_email.html.haml
+++ b/app/views/notify/note_commit_email.html.haml
@@ -1,5 +1,2 @@
 %p.details
-  New comment for Commit
-  = @commit.short_id
-
   = render 'note_mr_or_commit_email'
diff --git a/app/views/notify/note_commit_email.text.erb b/app/views/notify/note_commit_email.text.erb
index 715e58af61c..dc764b61659 100644
--- a/app/views/notify/note_commit_email.text.erb
+++ b/app/views/notify/note_commit_email.text.erb
@@ -1,11 +1,4 @@
-New comment for Commit <%= @commit.short_id %>
+<% url = url_for(namespace_project_commit_url(@note.project.namespace, @note.project, id: @commit.id, anchor: "note_#{@note.id}")) %>
 
-<%= url_for(namespace_project_commit_url(@note.project.namespace, @note.project, id: @commit.id, anchor: "note_#{@note.id}")) %>
-
-<%= render 'simple_diff' if @discussion %>
-
-<% if current_application_settings.email_author_in_body %>
-  <%= @note.author_name %> wrote:
-<% end %>
-
-<%= @note.note %>
+New comment for Commit <%= @commit.short_id -%>
+<%= render partial: 'note_mr_or_commit_email', locals: { url: url } %>
diff --git a/app/views/notify/note_merge_request_email.html.haml b/app/views/notify/note_merge_request_email.html.haml
index b7758f191dc..0a650e3b2ca 100644
--- a/app/views/notify/note_merge_request_email.html.haml
+++ b/app/views/notify/note_merge_request_email.html.haml
@@ -1,5 +1,2 @@
 %p.details
-  New comment for Merge Request
-  = @merge_request.to_reference
-
   = render 'note_mr_or_commit_email'
diff --git a/app/views/notify/note_merge_request_email.text.erb b/app/views/notify/note_merge_request_email.text.erb
index d24e15af91f..e33d15daded 100644
--- a/app/views/notify/note_merge_request_email.text.erb
+++ b/app/views/notify/note_merge_request_email.text.erb
@@ -1,11 +1,4 @@
-New comment for Merge Request <%= @merge_request.to_reference %>
+<% url = url_for(namespace_project_merge_request_url(@merge_request.target_project.namespace, @merge_request.target_project, @merge_request, anchor: "note_#{@note.id}")) %>
 
-<%= url_for(namespace_project_merge_request_url(@merge_request.target_project.namespace, @merge_request.target_project, @merge_request, anchor: "note_#{@note.id}")) %>
-
-<%= render 'simple_diff' if @discussion %>
-
-<% if current_application_settings.email_author_in_body %>
-  <%= @note.author_name %> wrote:
-<% end %>
-
-<%= @note.note %>
+New comment for Merge Request <%= @merge_request.to_reference -%>
+<%= render partial: 'note_mr_or_commit_email', locals: { url: url }%>
diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb
index 9b60102947a..84784aaf2fd 100644
--- a/lib/gitlab/diff/file.rb
+++ b/lib/gitlab/diff/file.rb
@@ -76,15 +76,8 @@ module Gitlab
         @diff_lines ||= Gitlab::Diff::Parser.new.parse(raw_diff.each_line).to_a
       end
 
-      def highlighted_diff_lines(lines = self)
-        @highlighted_diff_lines ||= Gitlab::Diff::Highlight.new(lines, repository: self.repository).highlight
-      end
-
-      def text_parsed_diff_lines(lines)
-        @text_parsed_diff_lines ||=
-        lines.map do | line |
-          "> " + line.text
-        end
+      def highlighted_diff_lines
+        @highlighted_diff_lines ||= Gitlab::Diff::Highlight.new(self, repository: self.repository).highlight
       end
 
       # Array[<Hash>] with right/left keys that contains Gitlab::Diff::Line objects which text is hightlighted
diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb
index a80eb114c17..824b516f856 100644
--- a/spec/mailers/notify_spec.rb
+++ b/spec/mailers/notify_spec.rb
@@ -697,7 +697,7 @@ describe Notify do
         let(:note) { create(model, project: project, author: note_author) }
 
         it "includes diffs with character-level highlighting" do
-          is_expected.to have_body_text /\<span class='idiff left right'>vars = {<\/span>/
+          is_expected.to have_body_text /<span class=\"p\">}<\/span><\/span>/
         end
 
         it 'contains a link to the diff file' do
@@ -743,9 +743,6 @@ describe Notify do
         subject { Notify.note_commit_email(recipient.id, note.id) }
 
         it_behaves_like 'a note email on a diff', :diff_note_on_commit
-        # it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do
-        #   let(:model) { commit }
-        # end
         it_behaves_like 'it should show Gmail Actions View Commit link'
         it_behaves_like 'a user cannot unsubscribe through footer link'
       end
@@ -757,9 +754,6 @@ describe Notify do
         subject { Notify.note_merge_request_email(recipient.id, note.id) }
 
         it_behaves_like 'a note email on a diff', :diff_note_on_merge_request
-        # it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do
-        #   let(:model) { merge_request }
-        # end
         it_behaves_like 'it should show Gmail Actions View Merge request link'
         it_behaves_like 'an unsubscribeable thread'
       end
diff --git a/spec/models/discussion_spec.rb b/spec/models/discussion_spec.rb
index d4b1f480c56..187d0bc0150 100644
--- a/spec/models/discussion_spec.rb
+++ b/spec/models/discussion_spec.rb
@@ -595,7 +595,7 @@ describe Discussion, model: true do
     let(:truncated_lines) { subject.truncated_diff_lines }
 
     context "when diff is greater than allowed number of truncated diff lines " do
-      let(:initial_line_count) { subject.diff_file.diff_lines.count }
+      let(:initial_line_count) { subject.diff_lines.count }
       let(:truncated_line_count) { truncated_lines.count }
 
       it "returns fewer lines"  do
@@ -606,7 +606,7 @@ describe Discussion, model: true do
     end
 
     context "when some diff lines are meta" do
-      let(:initial_meta_lines?) { subject.diff_file.diff_lines.any?(&:meta?) }
+      let(:initial_meta_lines?) { subject.diff_lines.any?(&:meta?) }
       let(:truncated_meta_lines?) { truncated_lines.any?(&:meta?) }
 
       it "returns no meta lines"  do