From a63eba9a2bebd6e33c3d1051a0d2fd08e024f546 Mon Sep 17 00:00:00 2001
From: Robert Speicher <rspeicher@gmail.com>
Date: Tue, 8 Mar 2016 20:04:13 -0500
Subject: Add unit specs for `Note#active?`

---
 app/models/note.rb       | 33 ++++++++++++++++----------
 spec/models/note_spec.rb | 62 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 82 insertions(+), 13 deletions(-)

diff --git a/app/models/note.rb b/app/models/note.rb
index 8b0610ff77e..0ea61b24955 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -172,26 +172,29 @@ class Note < ActiveRecord::Base
     Note.where(noteable_id: noteable_id, noteable_type: noteable_type, line_code: line_code).last.try(:diff)
   end
 
-  # Check if such line of code exists in merge request diff
-  # If exists - its active discussion
-  # If not - its outdated diff
+  # Check if this note is part of an "active" discussion
+  #
+  # This will always return true for anything except MergeRequest noteables,
+  # which have special logic.
+  #
+  # If the note's current diff cannot be matched in the MergeRequest's current
+  # diff, it's considered inactive.
   def active?
     return true unless self.diff
     return false unless noteable
     return @active if defined?(@active)
 
-    diffs = noteable.diffs(Commit.max_diff_options)
-    notable_diff = diffs.find { |d| d.new_path == self.diff.new_path }
+    noteable_diff = find_noteable_diff
 
-    return @active = false if notable_diff.nil?
+    if noteable_diff
+      parsed_lines = Gitlab::Diff::Parser.new.parse(noteable_diff.diff.each_line)
 
-    parsed_lines = Gitlab::Diff::Parser.new.parse(notable_diff.diff.each_line)
-    # We cannot use ||= because @active may be false
-    @active = parsed_lines.any? { |line_obj| line_obj.text == diff_line }
-  end
+      @active = parsed_lines.any? { |line_obj| line_obj.text == diff_line }
+    else
+      @active = false
+    end
 
-  def outdated?
-    !active?
+    @active
   end
 
   def diff_file_index
@@ -375,6 +378,12 @@ class Note < ActiveRecord::Base
 
   private
 
+  # Find the diff on noteable that matches our own
+  def find_noteable_diff
+    diffs = noteable.diffs(Commit.max_diff_options)
+    diffs.find { |d| d.new_path == self.diff.new_path }
+  end
+
   def awards_supported?
     (for_issue? || for_merge_request?) && !for_diff_line?
   end
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index cd620ea5440..b3ed7d6f8bd 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -152,7 +152,7 @@ describe Note, models: true do
     end
   end
 
-  describe :grouped_awards do
+  describe '.grouped_awards' do
     before do
       create :note, note: "smile", is_award: true
       create :note, note: "smile", is_award: true
@@ -169,6 +169,66 @@ describe Note, models: true do
     end
   end
 
+  describe '#active?' do
+    it 'is always true when the note has no associated diff' do
+      note = build(:note)
+
+      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)
+
+      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)
+
+      expect(note).to receive(:diff).and_return(double)
+      expect(note).to receive(:noteable).and_return(double)
+
+      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, 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, 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
+
   describe "editable?" do
     it "returns true" do
       note = build(:note)
-- 
cgit v1.2.1