Commit 09838ac6 authored by Douwe Maan's avatar Douwe Maan

Update diff discussion position per discussion instead of per note

parent 8039b9c3
...@@ -10,6 +10,7 @@ class DiffDiscussion < Discussion ...@@ -10,6 +10,7 @@ class DiffDiscussion < Discussion
delegate :position, delegate :position,
:original_position, :original_position,
:change_position,
to: :first_note to: :first_note
......
...@@ -95,13 +95,21 @@ class DiffNote < Note ...@@ -95,13 +95,21 @@ class DiffNote < Note
return if active? return if active?
Notes::DiffPositionUpdateService.new( tracer = Gitlab::Diff::PositionTracer.new(
self.project, project: self.project,
nil,
old_diff_refs: self.position.diff_refs, old_diff_refs: self.position.diff_refs,
new_diff_refs: noteable.diff_refs, new_diff_refs: self.noteable.diff_refs,
paths: self.position.paths 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 end
def verify_supported def verify_supported
......
...@@ -421,7 +421,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -421,7 +421,7 @@ class MergeRequest < ActiveRecord::Base
MergeRequests::MergeRequestDiffCacheService.new.execute(self) MergeRequests::MergeRequestDiffCacheService.new.execute(self)
new_diff_refs = self.diff_refs new_diff_refs = self.diff_refs
update_diff_notes_positions( update_diff_discussion_positions(
old_diff_refs: old_diff_refs, old_diff_refs: old_diff_refs,
new_diff_refs: new_diff_refs, new_diff_refs: new_diff_refs,
current_user: current_user current_user: current_user
...@@ -853,19 +853,18 @@ class MergeRequest < ActiveRecord::Base ...@@ -853,19 +853,18 @@ class MergeRequest < ActiveRecord::Base
diff_refs && diff_refs.complete? diff_refs && diff_refs.complete?
end 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 unless has_complete_diff_refs?
return if new_diff_refs == old_diff_refs return if new_diff_refs == old_diff_refs
active_diff_notes = self.notes.new_diff_notes.select do |note| active_diff_discussions = self.notes.new_diff_notes.discussions.select do |discussion|
note.active?(old_diff_refs) discussion.active?(old_diff_refs)
end 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 = Discussions::UpdateDiffPositionService.new(
service = Notes::DiffPositionUpdateService.new(
self.project, self.project,
current_user, current_user,
old_diff_refs: old_diff_refs, old_diff_refs: old_diff_refs,
...@@ -873,11 +872,8 @@ class MergeRequest < ActiveRecord::Base ...@@ -873,11 +872,8 @@ class MergeRequest < ActiveRecord::Base
paths: paths paths: paths
) )
transaction do active_diff_discussions.each do |discussion|
active_diff_notes.each do |note| service.execute(discussion)
service.execute(note)
Gitlab::Timeless.timeless(note, &:save)
end
end end
end end
......
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
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
...@@ -160,12 +160,6 @@ describe DiffNote, models: true do ...@@ -160,12 +160,6 @@ describe DiffNote, models: true do
context "when noteable is a commit" do context "when noteable is a commit" do
let(:diff_note) { create(:diff_note_on_commit, project: project, position: position) } 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 it "doesn't update the position" do
diff_note diff_note
...@@ -178,12 +172,6 @@ describe DiffNote, models: true do ...@@ -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) } let(:diff_note) { create(:diff_note_on_merge_request, project: project, position: position, noteable: merge_request) }
context "when the note is active" do 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 it "doesn't update the position" do
diff_note diff_note
...@@ -197,18 +185,11 @@ describe DiffNote, models: true do ...@@ -197,18 +185,11 @@ describe DiffNote, models: true do
allow(merge_request).to receive(:diff_refs).and_return(commit.diff_refs) allow(merge_request).to receive(:diff_refs).and_return(commit.diff_refs)
end end
it "uses the DiffPositionUpdateService" do it "updates the position" 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)
diff_note diff_note
expect(diff_note.original_position).to eq(position)
expect(diff_note.position).not_to eq(position)
end end
end end
end end
......
...@@ -1178,7 +1178,7 @@ describe MergeRequest, models: true do ...@@ -1178,7 +1178,7 @@ describe MergeRequest, models: true do
end end
describe "#reload_diff" do 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) } let(:commit) { subject.project.commit(sample_commit.id) }
...@@ -1197,7 +1197,7 @@ describe MergeRequest, models: true do ...@@ -1197,7 +1197,7 @@ describe MergeRequest, models: true do
subject.reload_diff subject.reload_diff
end end
it "updates diff note positions" do it "updates diff discussion positions" do
old_diff_refs = subject.diff_refs old_diff_refs = subject.diff_refs
# Update merge_request_diff so that #diff_refs will return commit.diff_refs # Update merge_request_diff so that #diff_refs will return commit.diff_refs
...@@ -1211,15 +1211,15 @@ describe MergeRequest, models: true do ...@@ -1211,15 +1211,15 @@ describe MergeRequest, models: true do
subject.merge_request_diff(true) subject.merge_request_diff(true)
end end
expect(Notes::DiffPositionUpdateService).to receive(:new).with( expect(Discussions::UpdateDiffPositionService).to receive(:new).with(
subject.project, subject.project,
subject.author, subject.author,
old_diff_refs: old_diff_refs, old_diff_refs: old_diff_refs,
new_diff_refs: commit.diff_refs, new_diff_refs: commit.diff_refs,
paths: note.position.paths paths: discussion.position.paths
).and_call_original ).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 expect_any_instance_of(DiffNote).to receive(:save).once
subject.reload_diff(subject.author) subject.reload_diff(subject.author)
......
require 'spec_helper' require 'spec_helper'
describe Notes::DiffPositionUpdateService, services: true do describe Discussions::UpdateDiffPositionService, services: true do
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository) }
let(:current_user) { project.owner } let(:current_user) { project.owner }
let(:create_commit) { project.commit("913c66a37b4a45b9769037c55c2d238bd0942d2e") } let(:create_commit) { project.commit("913c66a37b4a45b9769037c55c2d238bd0942d2e") }
...@@ -138,7 +138,7 @@ describe Notes::DiffPositionUpdateService, services: true do ...@@ -138,7 +138,7 @@ describe Notes::DiffPositionUpdateService, services: true do
# .. .. # .. ..
describe "#execute" 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 let(:old_position) do
Gitlab::Diff::Position.new( Gitlab::Diff::Position.new(
...@@ -154,11 +154,11 @@ describe Notes::DiffPositionUpdateService, services: true do ...@@ -154,11 +154,11 @@ describe Notes::DiffPositionUpdateService, services: true do
let(:line) { 16 } let(:line) { 16 }
it "updates the position" do it "updates the position" do
subject.execute(note) subject.execute(discussion)
expect(note.original_position).to eq(old_position) expect(discussion.original_position).to eq(old_position)
expect(note.position).not_to eq(old_position) expect(discussion.position).not_to eq(old_position)
expect(note.position.new_line).to eq(22) expect(discussion.position.new_line).to eq(22)
end end
end end
...@@ -166,27 +166,27 @@ describe Notes::DiffPositionUpdateService, services: true do ...@@ -166,27 +166,27 @@ describe Notes::DiffPositionUpdateService, services: true do
let(:line) { 9 } let(:line) { 9 }
it "doesn't update the position" do it "doesn't update the position" do
subject.execute(note) subject.execute(discussion)
expect(note.original_position).to eq(old_position) expect(discussion.original_position).to eq(old_position)
expect(note.position).to eq(old_position) expect(discussion.position).to eq(old_position)
end end
it 'sets the change position' do 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.start_sha).to eq(old_diff_refs.head_sha)
expect(change_position.head_sha).to eq(new_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.old_line).to eq(9)
expect(change_position.new_line).to be_nil expect(change_position.new_line).to be_nil
end end
it 'creates a system note' do it 'creates a system discussion' do
expect(SystemNoteService).to receive(:diff_discussion_outdated).with( 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 end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment