diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb new file mode 100644 index 00000000000..94c07cc535c --- /dev/null +++ b/app/models/diff_note.rb @@ -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 diff --git a/app/models/note.rb b/app/models/note.rb index 0c265064630..ffffd0c0838 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -56,7 +56,7 @@ class Note < ActiveRecord::Base scope :inc_author, ->{ includes(:author) } 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 :with_associations, -> do @@ -82,7 +82,7 @@ class Note < ActiveRecord::Base end 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 # Searches for notes matching the given query. @@ -115,6 +115,10 @@ class Note < ActiveRecord::Base false end + def new_diff_note? + false + end + def active? true end diff --git a/db/migrate/20160508215920_add_positions_to_diff_notes.rb b/db/migrate/20160508215920_add_positions_to_diff_notes.rb new file mode 100644 index 00000000000..2952c25004e --- /dev/null +++ b/db/migrate/20160508215920_add_positions_to_diff_notes.rb @@ -0,0 +1,6 @@ +class AddPositionsToDiffNotes < ActiveRecord::Migration + def change + add_column :notes, :position, :text + add_column :notes, :original_position, :text + end +end diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb index 3611c187202..da848afd48e 100644 --- a/features/steps/project/merge_requests.rb +++ b/features/steps/project/merge_requests.rb @@ -272,10 +272,9 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps step 'user "John Doe" leaves a comment like "Line is wrong" on diff' do 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, author: user_exists("John Doe"), - line_code: sample_commit.line_code, note: 'Line is wrong') end diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb index 696cf276e57..83e38095feb 100644 --- a/spec/factories/notes.rb +++ b/spec/factories/notes.rb @@ -10,21 +10,46 @@ FactoryGirl.define do on_issue 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_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 :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 noteable nil - noteable_id nil noteable_type 'Commit' + noteable_id nil commit_id RepoHelpers.sample_commit.id end - trait :on_diff do + trait :legacy_diff_note do line_code "0_184_184" end diff --git a/spec/lib/gitlab/note_data_builder_spec.rb b/spec/lib/gitlab/note_data_builder_spec.rb index e848d88182f..3d6bcdfd873 100644 --- a/spec/lib/gitlab/note_data_builder_spec.rb +++ b/spec/lib/gitlab/note_data_builder_spec.rb @@ -27,7 +27,7 @@ describe 'Gitlab::NoteDataBuilder', lib: true do end 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 expect(data).to have_key(:commit) @@ -90,7 +90,7 @@ describe 'Gitlab::NoteDataBuilder', lib: true do end let(:note) do - create(:note_on_merge_request_diff, noteable: merge_request, + create(:diff_note_on_merge_request, noteable: merge_request, project: project) end diff --git a/spec/lib/gitlab/url_builder_spec.rb b/spec/lib/gitlab/url_builder_spec.rb index bf11472407a..a826b24419a 100644 --- a/spec/lib/gitlab/url_builder_spec.rb +++ b/spec/lib/gitlab/url_builder_spec.rb @@ -43,9 +43,9 @@ describe Gitlab::UrlBuilder, lib: true do end end - context 'on a CommitDiff' do + context 'on a Commit Diff' 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) @@ -75,10 +75,10 @@ describe Gitlab::UrlBuilder, lib: true do end end - context 'on a MergeRequestDiff' do + context 'on a MergeRequest Diff' do it 'returns a proper URL' do 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) diff --git a/spec/models/legacy_diff_note_spec.rb b/spec/models/legacy_diff_note_spec.rb index b2d06853886..d64d89edbd3 100644 --- a/spec/models/legacy_diff_note_spec.rb +++ b/spec/models/legacy_diff_note_spec.rb @@ -2,7 +2,7 @@ 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!(:note) { create(:legacy_diff_note_on_commit, note: "+1 from me") } let!(:commit) { note.noteable } it "should save a valid note" do @@ -17,7 +17,7 @@ describe LegacyDiffNote, models: true do describe '#active?' 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) @@ -25,7 +25,7 @@ describe LegacyDiffNote, models: true do end 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(:noteable).and_return(nil) @@ -34,7 +34,7 @@ describe LegacyDiffNote, models: true do end 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') expect(note).not_to receive(:find_noteable_diff) @@ -45,7 +45,7 @@ describe LegacyDiffNote, models: true do 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) + note = build(:legacy_diff_note_on_merge_request, noteable: merge) allow(note).to receive(:diff).and_return(double) 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) # We're persisting in order to trigger the set_diff callback - note = create(:note_on_merge_request_diff, noteable: merge, - line_code: code, - project: merge.source_project) + note = create(:legacy_diff_note_on_merge_request, noteable: merge, + line_code: code, + project: merge.source_project) # Make sure we don't get a false positive from a guard clause expect(note).to receive(:find_noteable_diff).and_call_original