Add DiffNote model
This commit is contained in:
parent
db65954d78
commit
2f30d00432
|
@ -0,0 +1,99 @@
|
||||||
|
class DiffNote < Note
|
||||||
|
include NoteOnDiff
|
||||||
|
|
||||||
|
serialize :original_position, Gitlab::Diff::Position
|
||||||
|
serialize :position, Gitlab::Diff::Position
|
||||||
|
|
||||||
|
validates :original_position, presence: true
|
||||||
|
validates :position, presence: true
|
||||||
|
validates :diff_line, presence: true
|
||||||
|
validates :line_code, presence: true, line_code: true
|
||||||
|
validates :noteable_type, inclusion: { in: ['Commit', 'MergeRequest'] }
|
||||||
|
validate :positions_complete
|
||||||
|
validate :verify_supported
|
||||||
|
|
||||||
|
before_validation :set_original_position, on: :create
|
||||||
|
before_validation :set_line_code
|
||||||
|
|
||||||
|
class << self
|
||||||
|
def build_discussion_id(noteable_type, noteable_id, position)
|
||||||
|
[super(noteable_type, noteable_id), *position.key].join("-")
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
def new_diff_note?
|
||||||
|
true
|
||||||
|
end
|
||||||
|
|
||||||
|
def diff_attributes
|
||||||
|
{ position: position.to_json }
|
||||||
|
end
|
||||||
|
|
||||||
|
def discussion_id
|
||||||
|
@discussion_id ||= self.class.build_discussion_id(noteable_type, noteable_id || commit_id, position)
|
||||||
|
end
|
||||||
|
|
||||||
|
def original_discussion_id
|
||||||
|
@original_discussion_id ||= self.class.build_discussion_id(noteable_type, noteable_id || commit_id, original_position)
|
||||||
|
end
|
||||||
|
|
||||||
|
def position=(new_position)
|
||||||
|
if new_position.is_a?(String)
|
||||||
|
new_position = JSON.parse(new_position) rescue nil
|
||||||
|
end
|
||||||
|
|
||||||
|
if new_position.is_a?(Hash)
|
||||||
|
new_position = new_position.with_indifferent_access
|
||||||
|
new_position = Gitlab::Diff::Position.new(new_position)
|
||||||
|
end
|
||||||
|
|
||||||
|
super(new_position)
|
||||||
|
end
|
||||||
|
|
||||||
|
def diff_file
|
||||||
|
@diff_file ||= self.original_position.diff_file(self.project.repository)
|
||||||
|
end
|
||||||
|
|
||||||
|
def diff_line
|
||||||
|
@diff_line ||= diff_file.line_for_position(self.original_position) if diff_file
|
||||||
|
end
|
||||||
|
|
||||||
|
def for_line?(line)
|
||||||
|
diff_file.position(line) == self.original_position
|
||||||
|
end
|
||||||
|
|
||||||
|
def active?(diff_refs = nil)
|
||||||
|
return false unless supported?
|
||||||
|
return true if for_commit?
|
||||||
|
|
||||||
|
diff_refs ||= self.noteable.diff_refs
|
||||||
|
|
||||||
|
self.position.diff_refs == diff_refs
|
||||||
|
end
|
||||||
|
|
||||||
|
private
|
||||||
|
|
||||||
|
def supported?
|
||||||
|
!self.for_merge_request?
|
||||||
|
end
|
||||||
|
|
||||||
|
def set_original_position
|
||||||
|
self.original_position = self.position.dup
|
||||||
|
end
|
||||||
|
|
||||||
|
def set_line_code
|
||||||
|
self.line_code = self.position.line_code(self.project.repository)
|
||||||
|
end
|
||||||
|
|
||||||
|
def verify_supported
|
||||||
|
return if supported?
|
||||||
|
|
||||||
|
errors.add(:noteable, "doesn't support new-style diff notes")
|
||||||
|
end
|
||||||
|
|
||||||
|
def positions_complete
|
||||||
|
return if self.original_position.complete? && self.position.complete?
|
||||||
|
|
||||||
|
errors.add(:position, "is invalid")
|
||||||
|
end
|
||||||
|
end
|
|
@ -56,7 +56,7 @@ class Note < ActiveRecord::Base
|
||||||
scope :inc_author, ->{ includes(:author) }
|
scope :inc_author, ->{ includes(:author) }
|
||||||
scope :inc_author_project_award_emoji, ->{ includes(:project, :author, :award_emoji) }
|
scope :inc_author_project_award_emoji, ->{ includes(:project, :author, :award_emoji) }
|
||||||
|
|
||||||
scope :legacy_diff_notes, ->{ where(type: 'LegacyDiffNote') }
|
scope :diff_notes, ->{ where(type: ['LegacyDiffNote', 'DiffNote']) }
|
||||||
scope :non_diff_notes, ->{ where(type: ['Note', nil]) }
|
scope :non_diff_notes, ->{ where(type: ['Note', nil]) }
|
||||||
|
|
||||||
scope :with_associations, -> do
|
scope :with_associations, -> do
|
||||||
|
@ -82,7 +82,7 @@ class Note < ActiveRecord::Base
|
||||||
end
|
end
|
||||||
|
|
||||||
def grouped_diff_notes
|
def grouped_diff_notes
|
||||||
legacy_diff_notes.select(&:active?).sort_by(&:created_at).group_by(&:line_code)
|
diff_notes.select(&:active?).sort_by(&:created_at).group_by(&:line_code)
|
||||||
end
|
end
|
||||||
|
|
||||||
# Searches for notes matching the given query.
|
# Searches for notes matching the given query.
|
||||||
|
@ -115,6 +115,10 @@ class Note < ActiveRecord::Base
|
||||||
false
|
false
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def new_diff_note?
|
||||||
|
false
|
||||||
|
end
|
||||||
|
|
||||||
def active?
|
def active?
|
||||||
true
|
true
|
||||||
end
|
end
|
||||||
|
|
|
@ -0,0 +1,6 @@
|
||||||
|
class AddPositionsToDiffNotes < ActiveRecord::Migration
|
||||||
|
def change
|
||||||
|
add_column :notes, :position, :text
|
||||||
|
add_column :notes, :original_position, :text
|
||||||
|
end
|
||||||
|
end
|
|
@ -272,10 +272,9 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
|
||||||
|
|
||||||
step 'user "John Doe" leaves a comment like "Line is wrong" on diff' do
|
step 'user "John Doe" leaves a comment like "Line is wrong" on diff' do
|
||||||
mr = MergeRequest.find_by(title: "Bug NS-05")
|
mr = MergeRequest.find_by(title: "Bug NS-05")
|
||||||
create(:note_on_merge_request_diff, project: project,
|
create(:diff_note_on_merge_request, project: project,
|
||||||
noteable: mr,
|
noteable: mr,
|
||||||
author: user_exists("John Doe"),
|
author: user_exists("John Doe"),
|
||||||
line_code: sample_commit.line_code,
|
|
||||||
note: 'Line is wrong')
|
note: 'Line is wrong')
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
|
@ -10,21 +10,46 @@ FactoryGirl.define do
|
||||||
on_issue
|
on_issue
|
||||||
|
|
||||||
factory :note_on_commit, traits: [:on_commit]
|
factory :note_on_commit, traits: [:on_commit]
|
||||||
factory :note_on_commit_diff, traits: [:on_commit, :on_diff], class: LegacyDiffNote
|
|
||||||
factory :note_on_issue, traits: [:on_issue], aliases: [:votable_note]
|
factory :note_on_issue, traits: [:on_issue], aliases: [:votable_note]
|
||||||
factory :note_on_merge_request, traits: [:on_merge_request]
|
factory :note_on_merge_request, traits: [:on_merge_request]
|
||||||
factory :note_on_merge_request_diff, traits: [:on_merge_request, :on_diff], class: LegacyDiffNote
|
|
||||||
factory :note_on_project_snippet, traits: [:on_project_snippet]
|
factory :note_on_project_snippet, traits: [:on_project_snippet]
|
||||||
factory :system_note, traits: [:system]
|
factory :system_note, traits: [:system]
|
||||||
|
|
||||||
|
factory :legacy_diff_note_on_commit, traits: [:on_commit, :legacy_diff_note], class: LegacyDiffNote
|
||||||
|
factory :legacy_diff_note_on_merge_request, traits: [:on_merge_request, :legacy_diff_note], class: LegacyDiffNote
|
||||||
|
|
||||||
|
factory :diff_note_on_merge_request, traits: [:on_merge_request], class: DiffNote do
|
||||||
|
position do
|
||||||
|
Gitlab::Diff::Position.new(
|
||||||
|
old_path: "files/ruby/popen.rb",
|
||||||
|
new_path: "files/ruby/popen.rb",
|
||||||
|
old_line: nil,
|
||||||
|
new_line: 14,
|
||||||
|
diff_refs: noteable.diff_refs
|
||||||
|
)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
factory :diff_note_on_commit, traits: [:on_commit], class: DiffNote do
|
||||||
|
position do
|
||||||
|
Gitlab::Diff::Position.new(
|
||||||
|
old_path: "files/ruby/popen.rb",
|
||||||
|
new_path: "files/ruby/popen.rb",
|
||||||
|
old_line: nil,
|
||||||
|
new_line: 14,
|
||||||
|
diff_refs: project.commit(commit_id).try(:diff_refs)
|
||||||
|
)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
trait :on_commit do
|
trait :on_commit do
|
||||||
noteable nil
|
noteable nil
|
||||||
noteable_id nil
|
|
||||||
noteable_type 'Commit'
|
noteable_type 'Commit'
|
||||||
|
noteable_id nil
|
||||||
commit_id RepoHelpers.sample_commit.id
|
commit_id RepoHelpers.sample_commit.id
|
||||||
end
|
end
|
||||||
|
|
||||||
trait :on_diff do
|
trait :legacy_diff_note do
|
||||||
line_code "0_184_184"
|
line_code "0_184_184"
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
|
@ -27,7 +27,7 @@ describe 'Gitlab::NoteDataBuilder', lib: true do
|
||||||
end
|
end
|
||||||
|
|
||||||
describe 'When asking for a note on commit diff' do
|
describe 'When asking for a note on commit diff' do
|
||||||
let(:note) { create(:note_on_commit_diff, project: project) }
|
let(:note) { create(:diff_note_on_commit, project: project) }
|
||||||
|
|
||||||
it 'returns the note and commit-specific data' do
|
it 'returns the note and commit-specific data' do
|
||||||
expect(data).to have_key(:commit)
|
expect(data).to have_key(:commit)
|
||||||
|
@ -90,7 +90,7 @@ describe 'Gitlab::NoteDataBuilder', lib: true do
|
||||||
end
|
end
|
||||||
|
|
||||||
let(:note) do
|
let(:note) do
|
||||||
create(:note_on_merge_request_diff, noteable: merge_request,
|
create(:diff_note_on_merge_request, noteable: merge_request,
|
||||||
project: project)
|
project: project)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
|
@ -43,9 +43,9 @@ describe Gitlab::UrlBuilder, lib: true do
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
context 'on a CommitDiff' do
|
context 'on a Commit Diff' do
|
||||||
it 'returns a proper URL' do
|
it 'returns a proper URL' do
|
||||||
note = build_stubbed(:note_on_commit_diff)
|
note = build_stubbed(:diff_note_on_commit)
|
||||||
|
|
||||||
url = described_class.build(note)
|
url = described_class.build(note)
|
||||||
|
|
||||||
|
@ -75,10 +75,10 @@ describe Gitlab::UrlBuilder, lib: true do
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
context 'on a MergeRequestDiff' do
|
context 'on a MergeRequest Diff' do
|
||||||
it 'returns a proper URL' do
|
it 'returns a proper URL' do
|
||||||
merge_request = create(:merge_request, iid: 42)
|
merge_request = create(:merge_request, iid: 42)
|
||||||
note = build_stubbed(:note_on_merge_request_diff, noteable: merge_request)
|
note = build_stubbed(:diff_note_on_merge_request, noteable: merge_request)
|
||||||
|
|
||||||
url = described_class.build(note)
|
url = described_class.build(note)
|
||||||
|
|
||||||
|
|
|
@ -2,7 +2,7 @@ require 'spec_helper'
|
||||||
|
|
||||||
describe LegacyDiffNote, models: true do
|
describe LegacyDiffNote, models: true do
|
||||||
describe "Commit diff line notes" do
|
describe "Commit diff line notes" do
|
||||||
let!(:note) { create(:note_on_commit_diff, note: "+1 from me") }
|
let!(:note) { create(:legacy_diff_note_on_commit, note: "+1 from me") }
|
||||||
let!(:commit) { note.noteable }
|
let!(:commit) { note.noteable }
|
||||||
|
|
||||||
it "should save a valid note" do
|
it "should save a valid note" do
|
||||||
|
@ -17,7 +17,7 @@ describe LegacyDiffNote, models: true do
|
||||||
|
|
||||||
describe '#active?' do
|
describe '#active?' do
|
||||||
it 'is always true when the note has no associated diff' do
|
it 'is always true when the note has no associated diff' do
|
||||||
note = build(:note_on_merge_request_diff)
|
note = build(:legacy_diff_note_on_merge_request)
|
||||||
|
|
||||||
expect(note).to receive(:diff).and_return(nil)
|
expect(note).to receive(:diff).and_return(nil)
|
||||||
|
|
||||||
|
@ -25,7 +25,7 @@ describe LegacyDiffNote, models: true do
|
||||||
end
|
end
|
||||||
|
|
||||||
it 'is never true when the note has no noteable associated' do
|
it 'is never true when the note has no noteable associated' do
|
||||||
note = build(:note_on_merge_request_diff)
|
note = build(:legacy_diff_note_on_merge_request)
|
||||||
|
|
||||||
expect(note).to receive(:diff).and_return(double)
|
expect(note).to receive(:diff).and_return(double)
|
||||||
expect(note).to receive(:noteable).and_return(nil)
|
expect(note).to receive(:noteable).and_return(nil)
|
||||||
|
@ -34,7 +34,7 @@ describe LegacyDiffNote, models: true do
|
||||||
end
|
end
|
||||||
|
|
||||||
it 'returns the memoized value if defined' do
|
it 'returns the memoized value if defined' do
|
||||||
note = build(:note_on_merge_request_diff)
|
note = build(:legacy_diff_note_on_merge_request)
|
||||||
|
|
||||||
note.instance_variable_set(:@active, 'foo')
|
note.instance_variable_set(:@active, 'foo')
|
||||||
expect(note).not_to receive(:find_noteable_diff)
|
expect(note).not_to receive(:find_noteable_diff)
|
||||||
|
@ -45,7 +45,7 @@ describe LegacyDiffNote, models: true do
|
||||||
context 'for a merge request noteable' do
|
context 'for a merge request noteable' do
|
||||||
it 'is false when noteable has no matching diff' do
|
it 'is false when noteable has no matching diff' do
|
||||||
merge = build_stubbed(:merge_request, :simple)
|
merge = build_stubbed(:merge_request, :simple)
|
||||||
note = build(:note_on_merge_request_diff, noteable: merge)
|
note = build(:legacy_diff_note_on_merge_request, noteable: merge)
|
||||||
|
|
||||||
allow(note).to receive(:diff).and_return(double)
|
allow(note).to receive(:diff).and_return(double)
|
||||||
expect(note).to receive(:find_noteable_diff).and_return(nil)
|
expect(note).to receive(:find_noteable_diff).and_return(nil)
|
||||||
|
@ -63,9 +63,9 @@ describe LegacyDiffNote, models: true do
|
||||||
code = Gitlab::Diff::LineCode.generate(diff.new_path, line.new_pos, line.old_pos)
|
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
|
# We're persisting in order to trigger the set_diff callback
|
||||||
note = create(:note_on_merge_request_diff, noteable: merge,
|
note = create(:legacy_diff_note_on_merge_request, noteable: merge,
|
||||||
line_code: code,
|
line_code: code,
|
||||||
project: merge.source_project)
|
project: merge.source_project)
|
||||||
|
|
||||||
# Make sure we don't get a false positive from a guard clause
|
# 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 receive(:find_noteable_diff).and_call_original
|
||||||
|
|
Loading…
Reference in New Issue