Merge branch 'dm-update-discussion-diff-position' into 'master'

Update diff discussion position per discussion instead of per note

Closes #33157

See merge request !11833

app/models/diff_discussion.rb

@@ -10,6 +10,7 @@ class DiffDiscussion < Discussion
 
   delegate  :position,
             :original_position,
+            :change_position,
 
             to: :first_note
 

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

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
 

app/services/discussions/update_diff_position_service.rb

+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

app/services/notes/diff_position_update_service.rb

-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

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

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)

spec/services/notes/diff_position_update_service_spec.rb → spec/services/discussions/update_diff_position_service_spec.rb

 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