Extract parts of LegacyDiffNote into DiffOnNote concern and move part of...

Extract parts of LegacyDiffNote into DiffOnNote concern and move part of responsibility to other classes

app/helpers/diff_helper.rb

@@ -34,10 +34,6 @@ module DiffHelper
     diffs.decorate! { |diff| Gitlab::Diff::File.new(diff, diff_refs: diff_refs, repository: repository) }
   end
 
-  def generate_line_code(file_path, line)
-    Gitlab::Diff::LineCode.generate(file_path, line.new_pos, line.old_pos)
-  end
-
   def unfold_bottom_class(bottom)
     bottom ? 'js-unfold-bottom' : ''
   end

app/helpers/notes_helper.rb

@@ -60,10 +60,9 @@ module NotesHelper
     }
 
     if note.diff_note?
-      data.merge!(
-        line_code: note.line_code,
-        note_type: LegacyDiffNote.name
-      )
+      data[:note_type] = note.type
+
+      data.merge!(note.diff_attributes)
     end
 
     button_tag 'Reply...', class: 'btn btn-text-field js-discussion-reply-button',

app/models/concerns/note_on_diff.rb

+module NoteOnDiff
+  extend ActiveSupport::Concern
+
+  NUMBER_OF_TRUNCATED_DIFF_LINES = 16
+
+  included do
+    delegate :blob, :highlighted_diff_lines, to: :diff_file, allow_nil: true
+  end
+
+  def diff_note?
+    true
+  end
+
+  def diff_file
+    raise NotImplementedError
+  end
+
+  def diff_line
+    raise NotImplementedError
+  end
+
+  def for_line?(line)
+    raise NotImplementedError
+  end
+
+  def diff_attributes
+    raise NotImplementedError
+  end
+
+  def can_be_award_emoji?
+    false
+  end
+
+  def truncated_diff_lines
+    prev_match_line = nil
+    prev_lines = []
+
+    highlighted_diff_lines.each do |line|
+      if line.meta?
+        prev_lines.clear
+        prev_match_line = line
+      else
+        prev_lines << line
+
+        break if for_line?(line)
+
+        prev_lines.shift if prev_lines.length >= NUMBER_OF_TRUNCATED_DIFF_LINES
+      end
+    end
+
+    prev_lines
+  end
+end

app/models/legacy_diff_note.rb

 class LegacyDiffNote < Note
+  include NoteOnDiff
+
   serialize :st_diff
 
   validates :line_code, presence: true, line_code: true
@@ -11,12 +13,12 @@ class LegacyDiffNote < Note
     end
   end
 
-  def diff_note?
+  def legacy_diff_note?
     true
   end
 
-  def legacy_diff_note?
-    true
+  def diff_attributes
+    { line_code: line_code }
   end
 
   def discussion_id
@@ -27,61 +29,20 @@ class LegacyDiffNote < Note
     line_code.split('_')[0] if line_code
   end
 
-  def diff_old_line
-    line_code.split('_')[1].to_i if line_code
-  end
-
-  def diff_new_line
-    line_code.split('_')[2].to_i if line_code
-  end
-
   def diff
     @diff ||= Gitlab::Git::Diff.new(st_diff) if st_diff.respond_to?(:map)
   end
 
-  def diff_file_path
-    diff.new_path.presence || diff.old_path
-  end
-
-  def diff_lines
-    @diff_lines ||= Gitlab::Diff::Parser.new.parse(diff.diff.each_line)
+  def diff_file
+    @diff_file ||= Gitlab::Diff::File.new(diff, repository: self.project.repository) if diff
   end
 
   def diff_line
-    @diff_line ||= diff_lines.find { |line| generate_line_code(line) == self.line_code }
+    @diff_line ||= diff_file.line_for_line_code(self.line_code)
   end
 
-  def diff_line_text
-    diff_line.try(:text)
-  end
-
-  def diff_line_type
-    diff_line.try(:type)
-  end
-
-  def highlighted_diff_lines
-    Gitlab::Diff::Highlight.new(diff_lines).highlight
-  end
-
-  def truncated_diff_lines
-    max_number_of_lines = 16
-    prev_match_line = nil
-    prev_lines = []
-
-    highlighted_diff_lines.each do |line|
-      if line.type == "match"
-        prev_lines.clear
-        prev_match_line = line
-      else
-        prev_lines << line
-
-        break if generate_line_code(line) == self.line_code
-
-        prev_lines.shift if prev_lines.length >= max_number_of_lines
-      end
-    end
-
-    prev_lines
+  def for_line?(line)
+    !line.meta? && diff_file.line_code(line) == self.line_code
   end
 
   # Check if this note is part of an "active" discussion
@@ -102,7 +63,7 @@ class LegacyDiffNote < Note
     if noteable_diff
       parsed_lines = Gitlab::Diff::Parser.new.parse(noteable_diff.diff.each_line)
 
-      @active = parsed_lines.any? { |line_obj| line_obj.text == diff_line_text }
+      @active = parsed_lines.any? { |line_obj| line_obj.text == diff_line.text }
     else
       @active = false
     end
@@ -110,10 +71,6 @@ class LegacyDiffNote < Note
     @active
   end
 
-  def award_emoji_supported?
-    false
-  end
-
   private
 
   def find_diff
@@ -149,10 +106,6 @@ class LegacyDiffNote < Note
     self.class.where(attributes).last.try(:diff)
   end
 
-  def generate_line_code(line)
-    Gitlab::Diff::LineCode.generate(diff_file_path, line.new_pos, line.old_pos)
-  end
-
   # Find the diff on noteable that matches our own
   def find_noteable_diff
     diffs = noteable.diffs(Commit.max_diff_options)

app/models/note.rb

@@ -193,7 +193,7 @@ class Note < ActiveRecord::Base
   end
 
   def award_emoji?
-    award_emoji_supported? && contains_emoji_only?
+    can_be_award_emoji? && contains_emoji_only?
   end
 
   def emoji_awardable?
@@ -204,7 +204,7 @@ class Note < ActiveRecord::Base
     self.line_code = nil if self.line_code.blank?
   end
 
-  def award_emoji_supported?
+  def can_be_award_emoji?
     noteable.is_a?(Awardable)
   end
 

app/models/sent_notification.rb

@@ -20,7 +20,7 @@ class SentNotification < ActiveRecord::Base
       find_by(reply_key: reply_key)
     end
 
-    def record(noteable, recipient_id, reply_key, params = {})
+    def record(noteable, recipient_id, reply_key, attrs = {})
       return unless reply_key
 
       noteable_id = nil
@@ -31,7 +31,7 @@ class SentNotification < ActiveRecord::Base
         noteable_id = noteable.id
       end
 
-      params.reverse_merge!(
+      attrs.reverse_merge!(
         project:        noteable.project,
         noteable_type:  noteable.class.name,
         noteable_id:    noteable_id,
@@ -40,13 +40,17 @@ class SentNotification < ActiveRecord::Base
         reply_key:      reply_key
       )
 
-      create(params)
+      create(attrs)
     end
 
-    def record_note(note, recipient_id, reply_key, params = {})
-      params[:line_code] = note.line_code
+    def record_note(note, recipient_id, reply_key, attrs = {})
+      if note.diff_note?
+        attrs[:note_type] = note.type
 
-      record(note.noteable, recipient_id, reply_key, params)
+        attrs.merge!(note.diff_attributes)
+      end
+
+      record(note.noteable, recipient_id, reply_key, attrs)
     end
   end
 

app/views/projects/diffs/_line.html.haml

+- line_code = diff_file.line_code(line)
 - type = line.type
 %tr.line_holder{ id: line_code, class: type }
   - case type

app/views/projects/notes/discussions/_diff_with_notes.html.haml

@@ -12,7 +12,7 @@
     %table
       - note.truncated_diff_lines.each do |line|
         - type = line.type
-        - line_code = generate_line_code(note.diff_file_path, line)
+        - line_code = diff_file.line_code(line)
         %tr.line_holder{ id: line_code, class: "#{type}" }
           - if type == "match"
             %td.old_line.diff-line-num= "..."
@@ -23,5 +23,5 @@
             %td.new_line.diff-line-num{ data: { linenumber: type == "old" ? " ".html_safe : line.new_pos } }
             %td.line_content{ class: ['noteable_line', type, line_code], line_code: line_code }= diff_line_content(line.text, type)
 
-            - if line_code == note.line_code
+            - if note.for_line?(line)
               = render "projects/notes/diff_notes_with_reply", notes: discussion_notes

lib/api/entities.rb

@@ -240,9 +240,9 @@ module API
 
     class CommitNote < Grape::Entity
       expose :note
-      expose(:path) { |note| note.diff_file_path if note.legacy_diff_note? }
-      expose(:line) { |note| note.diff_new_line if note.legacy_diff_note? }
-      expose(:line_type) { |note| note.diff_line_type if note.legacy_diff_note? }
+      expose(:path) { |note| note.diff_file.try(:file_path) if note.diff_note? }
+      expose(:line) { |note| note.diff_line.try(:new_line) if note.diff_note? }
+      expose(:line_type) { |note| note.diff_line.try(:type) if note.diff_note? }
       expose :author, using: Entities::UserBasic
       expose :created_at
     end

lib/gitlab/diff/file.rb

@@ -13,6 +13,16 @@ module Gitlab
         @diff_refs = diff_refs
       end
 
+      def line_code(line)
+        return if line.meta?
+
+        Gitlab::Diff::LineCode.generate(file_path, line.new_pos, line.old_pos)
+      end
+
+      def line_for_line_code(code)
+        diff_lines.find { |line| line_code(line) == code }
+      end
+
       def content_commit
         return unless diff_refs
         

lib/gitlab/diff/highlight.rb

@@ -42,10 +42,9 @@ module Gitlab
 
         line_prefix = diff_line.text.match(/\A(.)/) ? $1 : ' '
 
-        case diff_line.type
-        when 'new', nil
+        if diff_line.unchanged? || diff_line.added?
           rich_line = new_lines[diff_line.new_pos - 1]
-        when 'old'
+        elsif diff_line.removed?
           rich_line = old_lines[diff_line.old_pos - 1]
         end
 
@@ -60,19 +59,12 @@ module Gitlab
 
       def old_lines
         return unless diff_file
-        @old_lines ||= Gitlab::Highlight.highlight_lines(*processing_args(:old))
+        @old_lines ||= Gitlab::Highlight.highlight_lines(self.repository, diff_old_ref, diff_old_path)
       end
 
       def new_lines
         return unless diff_file
-        @new_lines ||= Gitlab::Highlight.highlight_lines(*processing_args(:new))
-      end
-
-      def processing_args(version)
-        ref  = send("diff_#{version}_ref")
-        path = send("diff_#{version}_path")
-
-        [self.repository, ref, path]
+        @new_lines ||= Gitlab::Highlight.highlight_lines(self.repository, diff_new_ref, diff_new_path)
       end
     end
   end

lib/gitlab/diff/line.rb

@@ -9,6 +9,18 @@ module Gitlab
         @old_pos, @new_pos = old_pos, new_pos
       end
 
+      def old_line
+        old_pos unless added? || meta?
+      end
+
+      def new_line
+        new_pos unless removed? || meta?
+      end
+
+      def unchanged?
+        type.nil?
+      end
+
       def added?
         type == 'new'
       end
@@ -16,6 +28,10 @@ module Gitlab
       def removed?
         type == 'old'
       end
+
+      def meta?
+        type == 'match' || type == 'nonewline'
+      end
     end
   end
 end

lib/gitlab/diff/parallel_diff.rb

@@ -15,7 +15,7 @@ module Gitlab
         highlighted_diff_lines.each do |line|
           full_line = line.text
           type = line.type
-          line_code = generate_line_code(diff_file.file_path, line)
+          line_code = diff_file.line_code(line)
           line_new = line.new_pos
           line_old = line.old_pos
 
@@ -23,9 +23,9 @@ module Gitlab
 
           if next_line
             next_line = highlighted_diff_lines[next_line.index]
-            next_line_code = generate_line_code(diff_file.file_path, next_line)
+            full_next_line = next_line.text
+            next_line_code = diff_file.line_code(next_line)
             next_type = next_line.type
-            next_line = next_line.text
           end
 
           case type
@@ -59,8 +59,8 @@ module Gitlab
                 right: {
                   type:       next_type,
                   number:     line_new,
-                  text:       next_line,
-                  line_code:  next_line_code
+                  text:       full_next_line,
+                  line_code:  next_line_code,
                 }
               }
               skip_next = true
@@ -108,12 +108,6 @@ module Gitlab
         end
         lines
       end
-
-      private
-
-      def generate_line_code(file_path, line)
-        Gitlab::Diff::LineCode.generate(file_path, line.new_pos, line.old_pos)
-      end
     end
   end
 end