diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb
index 800574d8be0eb8a4ec45e386085ea96c2f85424a..07c4846e2ac5900e41f572ead8db93dc8c865f4a 100644
--- a/app/models/diff_discussion.rb
+++ b/app/models/diff_discussion.rb
@@ -10,6 +10,7 @@ class DiffDiscussion < Discussion
delegate :position,
:original_position,
+ :change_position,
to: :first_note
diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb
index 6cd0502069ae656008b0def55385086705cdda2f..20ef137850095dc56786623c3cadc41adaf2bd73 100644
--- a/app/models/diff_note.rb
+++ b/app/models/diff_note.rb
@@ -95,13 +95,21 @@ class DiffNote < Note
return if active?
- Notes::DiffPositionUpdateService.new(
- self.project,
- nil,
+ tracer = Gitlab::Diff::PositionTracer.new(
+ project: self.project,
old_diff_refs: self.position.diff_refs,
- new_diff_refs: noteable.diff_refs,
+ new_diff_refs: self.noteable.diff_refs,
paths: self.position.paths
- ).execute(self)
+ )
+
+ result = tracer.trace(self.position)
+ return unless result
+
+ if result[:outdated]
+ self.change_position = result[:position]
+ else
+ self.position = result[:position]
+ end
end
def verify_supported
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 4bccda8a732b679b7ccdd0e48c61032f098cb3ab..dd155252ad5f8049853f92754e72919436d366db 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -421,7 +421,7 @@ class MergeRequest < ActiveRecord::Base
MergeRequests::MergeRequestDiffCacheService.new.execute(self)
new_diff_refs = self.diff_refs
- update_diff_notes_positions(
+ update_diff_discussion_positions(
old_diff_refs: old_diff_refs,
new_diff_refs: new_diff_refs,
current_user: current_user
@@ -853,19 +853,18 @@ class MergeRequest < ActiveRecord::Base
diff_refs && diff_refs.complete?
end
- def update_diff_notes_positions(old_diff_refs:, new_diff_refs:, current_user: nil)
+ def update_diff_discussion_positions(old_diff_refs:, new_diff_refs:, current_user: nil)
return unless has_complete_diff_refs?
return if new_diff_refs == old_diff_refs
- active_diff_notes = self.notes.new_diff_notes.select do |note|
- note.active?(old_diff_refs)
+ active_diff_discussions = self.notes.new_diff_notes.discussions.select do |discussion|
+ discussion.active?(old_diff_refs)
end
+ return if active_diff_discussions.empty?
- return if active_diff_notes.empty?
+ paths = active_diff_discussions.flat_map { |n| n.diff_file.paths }.uniq
- paths = active_diff_notes.flat_map { |n| n.diff_file.paths }.uniq
-
- service = Notes::DiffPositionUpdateService.new(
+ service = Discussions::UpdateDiffPositionService.new(
self.project,
current_user,
old_diff_refs: old_diff_refs,
@@ -873,11 +872,8 @@ class MergeRequest < ActiveRecord::Base
paths: paths
)
- transaction do
- active_diff_notes.each do |note|
- service.execute(note)
- Gitlab::Timeless.timeless(note, &:save)
- end
+ active_diff_discussions.each do |discussion|
+ service.execute(discussion)
end
end
diff --git a/app/services/discussions/update_diff_position_service.rb b/app/services/discussions/update_diff_position_service.rb
new file mode 100644
index 0000000000000000000000000000000000000000..1ef8d9edbe1712fa1a09f77545f38eb89cc31c4c
--- /dev/null
+++ b/app/services/discussions/update_diff_position_service.rb
@@ -0,0 +1,41 @@
+module Discussions
+ class UpdateDiffPositionService < BaseService
+ def execute(discussion)
+ result = tracer.trace(discussion.position)
+ return unless result
+
+ position = result[:position]
+ outdated = result[:outdated]
+
+ discussion.notes.each do |note|
+ if outdated
+ note.change_position = position
+ else
+ note.position = position
+ note.change_position = nil
+ end
+ end
+
+ Note.transaction do
+ discussion.notes.each do |note|
+ Gitlab::Timeless.timeless(note, &:save)
+ end
+
+ if outdated && current_user
+ SystemNoteService.diff_discussion_outdated(discussion, project, current_user, position)
+ end
+ end
+ end
+
+ private
+
+ def tracer
+ @tracer ||= Gitlab::Diff::PositionTracer.new(
+ project: project,
+ old_diff_refs: params[:old_diff_refs],
+ new_diff_refs: params[:new_diff_refs],
+ paths: params[:paths]
+ )
+ end
+ end
+end
diff --git a/app/services/notes/diff_position_update_service.rb b/app/services/notes/diff_position_update_service.rb
deleted file mode 100644
index eff7b2872699e07323b32c65afa7c4115260f137..0000000000000000000000000000000000000000
--- a/app/services/notes/diff_position_update_service.rb
+++ /dev/null
@@ -1,33 +0,0 @@
-module Notes
- class DiffPositionUpdateService < BaseService
- def execute(note)
- results = tracer.trace(note.position)
- return unless results
-
- position = results[:position]
- outdated = results[:outdated]
-
- if outdated
- note.change_position = position
-
- if note.persisted? && current_user
- SystemNoteService.diff_discussion_outdated(note.to_discussion, project, current_user, position)
- end
- else
- note.position = position
- note.change_position = nil
- end
- end
-
- private
-
- def tracer
- @tracer ||= Gitlab::Diff::PositionTracer.new(
- project: project,
- old_diff_refs: params[:old_diff_refs],
- new_diff_refs: params[:new_diff_refs],
- paths: params[:paths]
- )
- end
- end
-end
diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb
index 96f075d4f7d52780df32b4287ea563aefa6f902c..297c2108dc289fc9a056cd0ff01279c8658aac44 100644
--- a/spec/models/diff_note_spec.rb
+++ b/spec/models/diff_note_spec.rb
@@ -160,12 +160,6 @@ describe DiffNote, models: true do
context "when noteable is a commit" do
let(:diff_note) { create(:diff_note_on_commit, project: project, position: position) }
- it "doesn't use the DiffPositionUpdateService" do
- expect(Notes::DiffPositionUpdateService).not_to receive(:new)
-
- diff_note
- end
-
it "doesn't update the position" do
diff_note
@@ -178,12 +172,6 @@ describe DiffNote, models: true do
let(:diff_note) { create(:diff_note_on_merge_request, project: project, position: position, noteable: merge_request) }
context "when the note is active" do
- it "doesn't use the DiffPositionUpdateService" do
- expect(Notes::DiffPositionUpdateService).not_to receive(:new)
-
- diff_note
- end
-
it "doesn't update the position" do
diff_note
@@ -197,18 +185,11 @@ describe DiffNote, models: true do
allow(merge_request).to receive(:diff_refs).and_return(commit.diff_refs)
end
- it "uses the DiffPositionUpdateService" do
- service = instance_double("Notes::DiffPositionUpdateService")
- expect(Notes::DiffPositionUpdateService).to receive(:new).with(
- project,
- nil,
- old_diff_refs: position.diff_refs,
- new_diff_refs: commit.diff_refs,
- paths: [path]
- ).and_return(service)
- expect(service).to receive(:execute)
-
+ it "updates the position" do
diff_note
+
+ expect(diff_note.original_position).to eq(position)
+ expect(diff_note.position).not_to eq(position)
end
end
end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 58f273ade28a3e769e890a24a0f2a13ccc8671d1..060754fab633db345a5fbd3481cdf2b27b0882ea 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1178,7 +1178,7 @@ describe MergeRequest, models: true do
end
describe "#reload_diff" do
- let(:note) { create(:diff_note_on_merge_request, project: subject.project, noteable: subject) }
+ let(:discussion) { create(:diff_note_on_merge_request, project: subject.project, noteable: subject).to_discussion }
let(:commit) { subject.project.commit(sample_commit.id) }
@@ -1197,7 +1197,7 @@ describe MergeRequest, models: true do
subject.reload_diff
end
- it "updates diff note positions" do
+ it "updates diff discussion positions" do
old_diff_refs = subject.diff_refs
# Update merge_request_diff so that #diff_refs will return commit.diff_refs
@@ -1211,15 +1211,15 @@ describe MergeRequest, models: true do
subject.merge_request_diff(true)
end
- expect(Notes::DiffPositionUpdateService).to receive(:new).with(
+ expect(Discussions::UpdateDiffPositionService).to receive(:new).with(
subject.project,
subject.author,
old_diff_refs: old_diff_refs,
new_diff_refs: commit.diff_refs,
- paths: note.position.paths
+ paths: discussion.position.paths
).and_call_original
- expect_any_instance_of(Notes::DiffPositionUpdateService).to receive(:execute).with(note)
+ expect_any_instance_of(Discussions::UpdateDiffPositionService).to receive(:execute).with(discussion).and_call_original
expect_any_instance_of(DiffNote).to receive(:save).once
subject.reload_diff(subject.author)
diff --git a/spec/services/notes/diff_position_update_service_spec.rb b/spec/services/discussions/update_diff_position_service_spec.rb
similarity index 85%
rename from spec/services/notes/diff_position_update_service_spec.rb
rename to spec/services/discussions/update_diff_position_service_spec.rb
index 380c296fd3ada7f09b641797351f4561986c47d1..177e32e13bd826e6f794f652cba01f2894997e5f 100644
--- a/spec/services/notes/diff_position_update_service_spec.rb
+++ b/spec/services/discussions/update_diff_position_service_spec.rb
@@ -1,6 +1,6 @@
require 'spec_helper'
-describe Notes::DiffPositionUpdateService, services: true do
+describe Discussions::UpdateDiffPositionService, services: true do
let(:project) { create(:project, :repository) }
let(:current_user) { project.owner }
let(:create_commit) { project.commit("913c66a37b4a45b9769037c55c2d238bd0942d2e") }
@@ -138,7 +138,7 @@ describe Notes::DiffPositionUpdateService, services: true do
# .. ..
describe "#execute" do
- let(:note) { create(:diff_note_on_merge_request, project: project, position: old_position) }
+ let(:discussion) { create(:diff_note_on_merge_request, project: project, position: old_position).to_discussion }
let(:old_position) do
Gitlab::Diff::Position.new(
@@ -154,11 +154,11 @@ describe Notes::DiffPositionUpdateService, services: true do
let(:line) { 16 }
it "updates the position" do
- subject.execute(note)
+ subject.execute(discussion)
- expect(note.original_position).to eq(old_position)
- expect(note.position).not_to eq(old_position)
- expect(note.position.new_line).to eq(22)
+ expect(discussion.original_position).to eq(old_position)
+ expect(discussion.position).not_to eq(old_position)
+ expect(discussion.position.new_line).to eq(22)
end
end
@@ -166,27 +166,27 @@ describe Notes::DiffPositionUpdateService, services: true do
let(:line) { 9 }
it "doesn't update the position" do
- subject.execute(note)
+ subject.execute(discussion)
- expect(note.original_position).to eq(old_position)
- expect(note.position).to eq(old_position)
+ expect(discussion.original_position).to eq(old_position)
+ expect(discussion.position).to eq(old_position)
end
it 'sets the change position' do
- subject.execute(note)
+ subject.execute(discussion)
- change_position = note.change_position
+ change_position = discussion.change_position
expect(change_position.start_sha).to eq(old_diff_refs.head_sha)
expect(change_position.head_sha).to eq(new_diff_refs.head_sha)
expect(change_position.old_line).to eq(9)
expect(change_position.new_line).to be_nil
end
- it 'creates a system note' do
+ it 'creates a system discussion' do
expect(SystemNoteService).to receive(:diff_discussion_outdated).with(
- note.to_discussion, project, current_user, instance_of(Gitlab::Diff::Position))
+ discussion, project, current_user, instance_of(Gitlab::Diff::Position))
- subject.execute(note)
+ subject.execute(discussion)
end
end
end