Clean up LegacyDiffNote somewhat

app/models/legacy_diff_note.rb

@@ -23,42 +23,65 @@ class LegacyDiffNote < Note
     @discussion_id ||= self.class.build_discussion_id(noteable_type, noteable_id || commit_id, line_code, active?)
   end
-  def find_diff
-    return nil unless noteable
-    return @diff if defined?(@diff)
-
-    # Don't use ||= because nil is a valid value for @diff
-    @diff = noteable.diffs(Commit.max_diff_options).find do |d|
-      Digest::SHA1.hexdigest(d.new_path) == diff_file_index if d.new_path
-    end
+  def diff_file_hash
+    line_code.split('_')[0] if line_code
   end
-  def set_diff
-    # First lets find notes with same diff
-    # before iterating over all mr diffs
-    diff = diff_for_line_code unless for_merge_request?
-    diff ||= find_diff
-    self.st_diff = diff.to_hash if diff
+  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_for_line_code
-    attributes = {
-      noteable_type: noteable_type,
-      line_code: line_code
-    }
-    if for_commit?
-      attributes[:commit_id] = commit_id
+  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)
+  end
+
+  def diff_line
+    @diff_line ||= diff_lines.find { |line| generate_line_code(line) == 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
-      attributes[:noteable_id] = noteable_id
+        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
-    self.class.where(attributes).last.try(:diff)
+    prev_lines
   end
   # Check if this note is part of an "active" discussion
@@ -69,17 +92,17 @@ class LegacyDiffNote < Note
   # If the note's current diff cannot be matched in the MergeRequest's current
   # diff, it's considered inactive.
   def active?
+    return @active if defined?(@active)
     return true if for_commit?
     return true unless self.diff
     return false unless noteable
-    return @active if defined?(@active)
     noteable_diff = find_noteable_diff
     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 }
+      @active = parsed_lines.any? { |line_obj| line_obj.text == diff_line_text }
     else
       @active = false
     end
@@ -87,93 +110,45 @@ class LegacyDiffNote < Note
     @active
   end
-  def diff_file_index
-    line_code.split('_')[0] if line_code
-  end
-
-  def diff_file_name
-    diff.new_path if diff
-  end
-
-  def file_path
-    if diff.new_path.present?
-      diff.new_path
-    elsif diff.old_path.present?
-      diff.old_path
-    end
-  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 generate_line_code(line)
-    Gitlab::Diff::LineCode.generate(file_path, line.new_pos, line.old_pos)
-  end
-  def diff_line
-    return @diff_line if @diff_line
-    if diff
-      diff_lines.each do |line|
-        if generate_line_code(line) == self.line_code
-          @diff_line = line.text
-        end
-      end
+  private
+  def find_diff
+    return nil unless noteable
+    return @diff if defined?(@diff)
+    @diff = noteable.diffs(Commit.max_diff_options).find do |d|
+      d.new_path && Digest::SHA1.hexdigest(d.new_path) == diff_file_hash
     end
-
-    @diff_line
   end
-  def diff_line_type
-    return @diff_line_type if @diff_line_type
-
-    if diff
-      diff_lines.each do |line|
-        if generate_line_code(line) == self.line_code
-          @diff_line_type = line.type
-        end
-      end
-    end
-    @diff_line_type
+  def set_diff
+    # First lets find notes with same diff
+    # before iterating over all mr diffs
+    diff = diff_for_line_code unless for_merge_request?
+    diff ||= find_diff
+    self.st_diff = diff.to_hash if diff
   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
+  def diff_for_line_code
+    attributes = {
+      noteable_type: noteable_type,
+      line_code: line_code
+    }
+    if for_commit?
+      attributes[:commit_id] = commit_id
     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
+      attributes[:noteable_id] = noteable_id
     end
-  def diff_lines
-    @diff_lines ||= Gitlab::Diff::Parser.new.parse(diff.diff.each_line)
+    self.class.where(attributes).last.try(:diff)
   end
-  def highlighted_diff_lines
-    Gitlab::Diff::Highlight.new(diff_lines).highlight
+  def generate_line_code(line)
+    Gitlab::Diff::LineCode.generate(diff_file_path, line.new_pos, line.old_pos)
   end
-  private
-
   # Find the diff on noteable that matches our own
   def find_noteable_diff
     diffs = noteable.diffs(Commit.max_diff_options)

app/models/note.rb

@@ -112,6 +112,10 @@ class Note < ActiveRecord::Base
     false
   end
+  def active?
+    true
+  end
+
   def discussion_id
     @discussion_id ||=
       if for_merge_request?

app/views/notify/note_merge_request_email.html.haml

 - if @note.legacy_diff_note?
   %p.details
     New comment on diff for
-    = link_to @note.diff_file_name, @target_url
+    = link_to @note.diff_file_path, @target_url
     \:
 = render 'note_message'

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

@@ -15,7 +15,7 @@
     %table
       - note.truncated_diff_lines.each do |line|
         - type = line.type
-        - line_code = generate_line_code(note.file_path, line)
+        - line_code = generate_line_code(note.diff_file_path, line)
         %tr.line_holder{ id: line_code, class: "#{type}" }
           - if type == "match"
             %td.old_line.diff-line-num= "..."

lib/api/entities.rb

@@ -227,7 +227,7 @@ module API
     class CommitNote < Grape::Entity
       expose :note
-      expose(:path) { |note| note.diff_file_name if note.legacy_diff_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 :author, using: Entities::UserBasic

spec/models/legacy_diff_note_spec.rb

+require 'spec_helper'
+
+describe LegacyDiffNote, models: true do
+  describe "Commit diff line notes" do
+    let!(:note) { create(:note_on_commit_diff, note: "+1 from me") }
+    let!(:commit) { note.noteable }
+
+    it "should save a valid note" do
+      expect(note.commit_id).to eq(commit.id)
+      expect(note.noteable.id).to eq(commit.id)
+    end
+
+    it "should be recognized by #legacy_diff_note?" do
+      expect(note).to be_legacy_diff_note
+    end
+  end
+
+  describe '#active?' do
+    it 'is always true when the note has no associated diff' do
+      note = build(:note_on_merge_request_diff)
+
+      expect(note).to receive(:diff).and_return(nil)
+
+      expect(note).to be_active
+    end
+
+    it 'is never true when the note has no noteable associated' do
+      note = build(:note_on_merge_request_diff)
+
+      expect(note).to receive(:diff).and_return(double)
+      expect(note).to receive(:noteable).and_return(nil)
+
+      expect(note).not_to be_active
+    end
+
+    it 'returns the memoized value if defined' do
+      note = build(:note_on_merge_request_diff)
+
+      note.instance_variable_set(:@active, 'foo')
+      expect(note).not_to receive(:find_noteable_diff)
+
+      expect(note.active?).to eq 'foo'
+    end
+
+    context 'for a merge request noteable' do
+      it 'is false when noteable has no matching diff' do
+        merge = build_stubbed(:merge_request, :simple)
+        note = build(:note_on_merge_request_diff, noteable: merge)
+
+        allow(note).to receive(:diff).and_return(double)
+        expect(note).to receive(:find_noteable_diff).and_return(nil)
+
+        expect(note).not_to be_active
+      end
+
+      it 'is true when noteable has a matching diff' do
+        merge = create(:merge_request, :simple)
+
+        # Generate a real line_code value so we know it will match. We use a
+        # random line from a random diff just for funsies.
+        diff = merge.diffs.to_a.sample
+        line = Gitlab::Diff::Parser.new.parse(diff.diff.each_line).to_a.sample
+        code = Gitlab::Diff::LineCode.generate(diff.new_path, line.new_pos, line.old_pos)
+
+        # We're persisting in order to trigger the set_diff callback
+        note = create(:note_on_merge_request_diff, noteable: merge, line_code: code)
+
+        # Make sure we don't get a false positive from a guard clause
+        expect(note).to receive(:find_noteable_diff).and_call_original
+        expect(note).to be_active
+      end
+    end
+  end
+end