Commit b782e7c9 authored by Robert Speicher's avatar Robert Speicher

Merge branch 'rs-note-active-spec' into 'master'

Add unit specs for `Note#active?`

See merge request !3133
parents 961bc41c a63eba9a
...@@ -173,26 +173,29 @@ class Note < ActiveRecord::Base ...@@ -173,26 +173,29 @@ class Note < ActiveRecord::Base
Note.where(noteable_id: noteable_id, noteable_type: noteable_type, line_code: line_code).last.try(:diff) Note.where(noteable_id: noteable_id, noteable_type: noteable_type, line_code: line_code).last.try(:diff)
end end
# Check if such line of code exists in merge request diff # Check if this note is part of an "active" discussion
# If exists - its active discussion #
# If not - its outdated diff # 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? def active?
return true unless self.diff return true unless self.diff
return false unless noteable return false unless noteable
return @active if defined?(@active) return @active if defined?(@active)
diffs = noteable.diffs(Commit.max_diff_options) noteable_diff = find_noteable_diff
notable_diff = diffs.find { |d| d.new_path == self.diff.new_path }
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) @active = parsed_lines.any? { |line_obj| line_obj.text == diff_line }
# We cannot use ||= because @active may be false else
@active = parsed_lines.any? { |line_obj| line_obj.text == diff_line } @active = false
end end
def outdated? @active
!active?
end end
def diff_file_index def diff_file_index
...@@ -380,6 +383,12 @@ class Note < ActiveRecord::Base ...@@ -380,6 +383,12 @@ class Note < ActiveRecord::Base
self.line_code = nil if self.line_code.blank? self.line_code = nil if self.line_code.blank?
end end
# 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? def awards_supported?
(for_issue? || for_merge_request?) && !for_diff_line? (for_issue? || for_merge_request?) && !for_diff_line?
end end
......
...@@ -152,7 +152,7 @@ describe Note, models: true do ...@@ -152,7 +152,7 @@ describe Note, models: true do
end end
end end
describe :grouped_awards do describe '.grouped_awards' do
before do before do
create :note, note: "smile", is_award: true create :note, note: "smile", is_award: true
create :note, note: "smile", is_award: true create :note, note: "smile", is_award: true
...@@ -169,6 +169,66 @@ describe Note, models: true do ...@@ -169,6 +169,66 @@ describe Note, models: true do
end end
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 describe "editable?" do
it "returns true" do it "returns true" do
note = build(:note) note = build(:note)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment