From 7848d54f5b0024f58d8ce2b0259347816be5bdbc Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 13 May 2016 14:53:31 -0500 Subject: Clean up LegacyDiffNote somewhat --- app/models/legacy_diff_note.rb | 171 +++++++++------------ app/models/note.rb | 4 + .../notify/note_merge_request_email.html.haml | 2 +- .../discussions/_legacy_diff_with_notes.html.haml | 2 +- lib/api/entities.rb | 2 +- spec/models/legacy_diff_note_spec.rb | 74 +++++++++ 6 files changed, 154 insertions(+), 101 deletions(-) create mode 100644 spec/models/legacy_diff_note_spec.rb diff --git a/app/models/legacy_diff_note.rb b/app/models/legacy_diff_note.rb index b5de85df99f..bbefc911b29 100644 --- a/app/models/legacy_diff_note.rb +++ b/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 + def diff_old_line + line_code.split('_')[1].to_i if line_code + end - self.st_diff = diff.to_hash if diff + 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 - } + def diff_file_path + diff.new_path.presence || diff.old_path + end - if for_commit? - attributes[:commit_id] = commit_id - else - attributes[:noteable_id] = noteable_id + 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 + 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 + private - def diff_line - return @diff_line if @diff_line + def find_diff + return nil unless noteable + return @diff if defined?(@diff) - if diff - diff_lines.each do |line| - if generate_line_code(line) == self.line_code - @diff_line = line.text - end - end + @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 + 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 - @diff_line_type + 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 - else - prev_lines << line - - break if generate_line_code(line) == self.line_code + def diff_for_line_code + attributes = { + noteable_type: noteable_type, + line_code: line_code + } - prev_lines.shift if prev_lines.length >= max_number_of_lines - end + if for_commit? + attributes[:commit_id] = commit_id + else + attributes[:noteable_id] = noteable_id end - prev_lines - 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) diff --git a/app/models/note.rb b/app/models/note.rb index 3bc55870704..7e5bdc09a84 100644 --- a/app/models/note.rb +++ b/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? diff --git a/app/views/notify/note_merge_request_email.html.haml b/app/views/notify/note_merge_request_email.html.haml index 27e8ea5f5a1..a3643a00cfe 100644 --- a/app/views/notify/note_merge_request_email.html.haml +++ b/app/views/notify/note_merge_request_email.html.haml @@ -1,7 +1,7 @@ - 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' diff --git a/app/views/projects/notes/discussions/_legacy_diff_with_notes.html.haml b/app/views/projects/notes/discussions/_legacy_diff_with_notes.html.haml index 3ab11f6461d..6401245bf73 100644 --- a/app/views/projects/notes/discussions/_legacy_diff_with_notes.html.haml +++ b/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= "..." diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 1619199b0a7..93a5798e21e 100644 --- a/lib/api/entities.rb +++ b/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 diff --git a/spec/models/legacy_diff_note_spec.rb b/spec/models/legacy_diff_note_spec.rb new file mode 100644 index 00000000000..7c29bef54e4 --- /dev/null +++ b/spec/models/legacy_diff_note_spec.rb @@ -0,0 +1,74 @@ +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 -- cgit v1.2.1