summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/models/diff_note.rb99
-rw-r--r--app/models/note.rb8
-rw-r--r--db/migrate/20160508215920_add_positions_to_diff_notes.rb6
-rw-r--r--features/steps/project/merge_requests.rb3
-rw-r--r--spec/factories/notes.rb33
-rw-r--r--spec/lib/gitlab/note_data_builder_spec.rb4
-rw-r--r--spec/lib/gitlab/url_builder_spec.rb8
-rw-r--r--spec/models/legacy_diff_note_spec.rb16
8 files changed, 155 insertions, 22 deletions
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
View Lorry Controller Status