From 09838ac626df739f0ab9852e3c7d862c7fd707e5 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 31 May 2017 14:31:33 -0500 Subject: Update diff discussion position per discussion instead of per note --- app/models/diff_discussion.rb | 1 + app/models/diff_note.rb | 18 +- app/models/merge_request.rb | 22 +-- .../discussions/update_diff_position_service.rb | 41 +++++ app/services/notes/diff_position_update_service.rb | 33 ---- spec/models/diff_note_spec.rb | 27 +-- spec/models/merge_request_spec.rb | 10 +- .../update_diff_position_service_spec.rb | 193 +++++++++++++++++++++ .../notes/diff_position_update_service_spec.rb | 193 --------------------- 9 files changed, 266 insertions(+), 272 deletions(-) create mode 100644 app/services/discussions/update_diff_position_service.rb delete mode 100644 app/services/notes/diff_position_update_service.rb create mode 100644 spec/services/discussions/update_diff_position_service_spec.rb delete mode 100644 spec/services/notes/diff_position_update_service_spec.rb diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb index 800574d8be0..07c4846e2ac 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 2a4cff37566..6fbc6a9f58b 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 356af776b8d..1c4bb119bbe 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 00000000000..1ef8d9edbe1 --- /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 eff7b287269..00000000000 --- 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 96f075d4f7d..297c2108dc2 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 712470d6bf5..6383f112543 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/discussions/update_diff_position_service_spec.rb b/spec/services/discussions/update_diff_position_service_spec.rb new file mode 100644 index 00000000000..177e32e13bd --- /dev/null +++ b/spec/services/discussions/update_diff_position_service_spec.rb @@ -0,0 +1,193 @@ +require 'spec_helper' + +describe Discussions::UpdateDiffPositionService, services: true do + let(:project) { create(:project, :repository) } + let(:current_user) { project.owner } + let(:create_commit) { project.commit("913c66a37b4a45b9769037c55c2d238bd0942d2e") } + let(:modify_commit) { project.commit("874797c3a73b60d2187ed6e2fcabd289ff75171e") } + let(:edit_commit) { project.commit("570e7b2abdd848b95f2f578043fc23bd6f6fd24d") } + + let(:path) { "files/ruby/popen.rb" } + + let(:old_diff_refs) do + Gitlab::Diff::DiffRefs.new( + base_sha: create_commit.parent_id, + head_sha: modify_commit.sha + ) + end + + let(:new_diff_refs) do + Gitlab::Diff::DiffRefs.new( + base_sha: create_commit.parent_id, + head_sha: edit_commit.sha + ) + end + + subject do + described_class.new( + project, + current_user, + old_diff_refs: old_diff_refs, + new_diff_refs: new_diff_refs, + paths: [path] + ) + end + + # old diff: + # 1 + require 'fileutils' + # 2 + require 'open3' + # 3 + + # 4 + module Popen + # 5 + extend self + # 6 + + # 7 + def popen(cmd, path=nil) + # 8 + unless cmd.is_a?(Array) + # 9 + raise "System commands must be given as an array of strings" + # 10 + end + # 11 + + # 12 + path ||= Dir.pwd + # 13 + vars = { "PWD" => path } + # 14 + options = { chdir: path } + # 15 + + # 16 + unless File.directory?(path) + # 17 + FileUtils.mkdir_p(path) + # 18 + end + # 19 + + # 20 + @cmd_output = "" + # 21 + @cmd_status = 0 + # 22 + Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| + # 23 + @cmd_output << stdout.read + # 24 + @cmd_output << stderr.read + # 25 + @cmd_status = wait_thr.value.exitstatus + # 26 + end + # 27 + + # 28 + return @cmd_output, @cmd_status + # 29 + end + # 30 + end + # + # new diff: + # 1 + require 'fileutils' + # 2 + require 'open3' + # 3 + + # 4 + module Popen + # 5 + extend self + # 6 + + # 7 + def popen(cmd, path=nil) + # 8 + unless cmd.is_a?(Array) + # 9 + raise RuntimeError, "System commands must be given as an array of strings" + # 10 + end + # 11 + + # 12 + path ||= Dir.pwd + # 13 + + # 14 + vars = { + # 15 + "PWD" => path + # 16 + } + # 17 + + # 18 + options = { + # 19 + chdir: path + # 20 + } + # 21 + + # 22 + unless File.directory?(path) + # 23 + FileUtils.mkdir_p(path) + # 24 + end + # 25 + + # 26 + @cmd_output = "" + # 27 + @cmd_status = 0 + # 28 + + # 29 + Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| + # 30 + @cmd_output << stdout.read + # 31 + @cmd_output << stderr.read + # 32 + @cmd_status = wait_thr.value.exitstatus + # 33 + end + # 34 + + # 35 + return @cmd_output, @cmd_status + # 36 + end + # 37 + end + # + # old->new diff: + # .. .. @@ -6,12 +6,18 @@ module Popen + # 6 6 + # 7 7 def popen(cmd, path=nil) + # 8 8 unless cmd.is_a?(Array) + # 9 - raise "System commands must be given as an array of strings" + # 9 + raise RuntimeError, "System commands must be given as an array of strings" + # 10 10 end + # 11 11 + # 12 12 path ||= Dir.pwd + # 13 - vars = { "PWD" => path } + # 14 - options = { chdir: path } + # 13 + + # 14 + vars = { + # 15 + "PWD" => path + # 16 + } + # 17 + + # 18 + options = { + # 19 + chdir: path + # 20 + } + # 15 21 + # 16 22 unless File.directory?(path) + # 17 23 FileUtils.mkdir_p(path) + # 18 24 end + # 19 25 + # 20 26 @cmd_output = "" + # 21 27 @cmd_status = 0 + # 28 + + # 22 29 Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| + # 23 30 @cmd_output << stdout.read + # 24 31 @cmd_output << stderr.read + # .. .. + + describe "#execute" do + let(:discussion) { create(:diff_note_on_merge_request, project: project, position: old_position).to_discussion } + + let(:old_position) do + Gitlab::Diff::Position.new( + old_path: path, + new_path: path, + old_line: nil, + new_line: line, + diff_refs: old_diff_refs + ) + end + + context "when the diff line is the same" do + let(:line) { 16 } + + it "updates the position" do + subject.execute(discussion) + + 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 + + context "when the diff line has changed" do + let(:line) { 9 } + + it "doesn't update the position" do + subject.execute(discussion) + + expect(discussion.original_position).to eq(old_position) + expect(discussion.position).to eq(old_position) + end + + it 'sets the change position' do + subject.execute(discussion) + + 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 discussion' do + expect(SystemNoteService).to receive(:diff_discussion_outdated).with( + discussion, project, current_user, instance_of(Gitlab::Diff::Position)) + + subject.execute(discussion) + end + end + end +end diff --git a/spec/services/notes/diff_position_update_service_spec.rb b/spec/services/notes/diff_position_update_service_spec.rb deleted file mode 100644 index 380c296fd3a..00000000000 --- a/spec/services/notes/diff_position_update_service_spec.rb +++ /dev/null @@ -1,193 +0,0 @@ -require 'spec_helper' - -describe Notes::DiffPositionUpdateService, services: true do - let(:project) { create(:project, :repository) } - let(:current_user) { project.owner } - let(:create_commit) { project.commit("913c66a37b4a45b9769037c55c2d238bd0942d2e") } - let(:modify_commit) { project.commit("874797c3a73b60d2187ed6e2fcabd289ff75171e") } - let(:edit_commit) { project.commit("570e7b2abdd848b95f2f578043fc23bd6f6fd24d") } - - let(:path) { "files/ruby/popen.rb" } - - let(:old_diff_refs) do - Gitlab::Diff::DiffRefs.new( - base_sha: create_commit.parent_id, - head_sha: modify_commit.sha - ) - end - - let(:new_diff_refs) do - Gitlab::Diff::DiffRefs.new( - base_sha: create_commit.parent_id, - head_sha: edit_commit.sha - ) - end - - subject do - described_class.new( - project, - current_user, - old_diff_refs: old_diff_refs, - new_diff_refs: new_diff_refs, - paths: [path] - ) - end - - # old diff: - # 1 + require 'fileutils' - # 2 + require 'open3' - # 3 + - # 4 + module Popen - # 5 + extend self - # 6 + - # 7 + def popen(cmd, path=nil) - # 8 + unless cmd.is_a?(Array) - # 9 + raise "System commands must be given as an array of strings" - # 10 + end - # 11 + - # 12 + path ||= Dir.pwd - # 13 + vars = { "PWD" => path } - # 14 + options = { chdir: path } - # 15 + - # 16 + unless File.directory?(path) - # 17 + FileUtils.mkdir_p(path) - # 18 + end - # 19 + - # 20 + @cmd_output = "" - # 21 + @cmd_status = 0 - # 22 + Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| - # 23 + @cmd_output << stdout.read - # 24 + @cmd_output << stderr.read - # 25 + @cmd_status = wait_thr.value.exitstatus - # 26 + end - # 27 + - # 28 + return @cmd_output, @cmd_status - # 29 + end - # 30 + end - # - # new diff: - # 1 + require 'fileutils' - # 2 + require 'open3' - # 3 + - # 4 + module Popen - # 5 + extend self - # 6 + - # 7 + def popen(cmd, path=nil) - # 8 + unless cmd.is_a?(Array) - # 9 + raise RuntimeError, "System commands must be given as an array of strings" - # 10 + end - # 11 + - # 12 + path ||= Dir.pwd - # 13 + - # 14 + vars = { - # 15 + "PWD" => path - # 16 + } - # 17 + - # 18 + options = { - # 19 + chdir: path - # 20 + } - # 21 + - # 22 + unless File.directory?(path) - # 23 + FileUtils.mkdir_p(path) - # 24 + end - # 25 + - # 26 + @cmd_output = "" - # 27 + @cmd_status = 0 - # 28 + - # 29 + Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| - # 30 + @cmd_output << stdout.read - # 31 + @cmd_output << stderr.read - # 32 + @cmd_status = wait_thr.value.exitstatus - # 33 + end - # 34 + - # 35 + return @cmd_output, @cmd_status - # 36 + end - # 37 + end - # - # old->new diff: - # .. .. @@ -6,12 +6,18 @@ module Popen - # 6 6 - # 7 7 def popen(cmd, path=nil) - # 8 8 unless cmd.is_a?(Array) - # 9 - raise "System commands must be given as an array of strings" - # 9 + raise RuntimeError, "System commands must be given as an array of strings" - # 10 10 end - # 11 11 - # 12 12 path ||= Dir.pwd - # 13 - vars = { "PWD" => path } - # 14 - options = { chdir: path } - # 13 + - # 14 + vars = { - # 15 + "PWD" => path - # 16 + } - # 17 + - # 18 + options = { - # 19 + chdir: path - # 20 + } - # 15 21 - # 16 22 unless File.directory?(path) - # 17 23 FileUtils.mkdir_p(path) - # 18 24 end - # 19 25 - # 20 26 @cmd_output = "" - # 21 27 @cmd_status = 0 - # 28 + - # 22 29 Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| - # 23 30 @cmd_output << stdout.read - # 24 31 @cmd_output << stderr.read - # .. .. - - describe "#execute" do - let(:note) { create(:diff_note_on_merge_request, project: project, position: old_position) } - - let(:old_position) do - Gitlab::Diff::Position.new( - old_path: path, - new_path: path, - old_line: nil, - new_line: line, - diff_refs: old_diff_refs - ) - end - - context "when the diff line is the same" do - let(:line) { 16 } - - it "updates the position" do - subject.execute(note) - - expect(note.original_position).to eq(old_position) - expect(note.position).not_to eq(old_position) - expect(note.position.new_line).to eq(22) - end - end - - context "when the diff line has changed" do - let(:line) { 9 } - - it "doesn't update the position" do - subject.execute(note) - - expect(note.original_position).to eq(old_position) - expect(note.position).to eq(old_position) - end - - it 'sets the change position' do - subject.execute(note) - - change_position = note.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 - expect(SystemNoteService).to receive(:diff_discussion_outdated).with( - note.to_discussion, project, current_user, instance_of(Gitlab::Diff::Position)) - - subject.execute(note) - end - end - end -end -- cgit v1.2.3