Automatically update diff note positions when MR is pushed to

5 files changed, 94 insertions, 9 deletions

diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb
index 70fbcb4c4e9..881ae5d1cad 100644
--- a/app/models/diff_note.rb
+++ b/app/models/diff_note.rb
@@ -12,7 +12,7 @@ class DiffNote < Note
   validate :positions_complete
   validate :verify_supported
 
-  before_validation :set_original_position, on: :create
+  before_validation :set_original_position, :update_position, on: :create
   before_validation :set_line_code
 
   class << self
@@ -71,6 +71,26 @@ class DiffNote < Note
     self.position.diff_refs == diff_refs
   end
 
+  def update_position
+    return unless supported?
+    return if for_commit?
+
+    return if active?
+
+    Notes::DiffPositionUpdateService.new(
+      self.project,
+      nil,
+      old_diff_refs: self.position.diff_refs,
+      new_diff_refs: self.noteable.diff_refs,
+      paths: self.position.paths
+    ).execute(self)
+  end
+
+  def update_position!
+    update_position &&
+      Gitlab::Timeless.timeless(self, &:save)
+  end
+
   private
 
   def supported?
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 82e0f5a0b53..4624e9d36e9 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -288,14 +288,23 @@ class MergeRequest < ActiveRecord::Base
 
   def update_merge_request_diff
     if source_branch_changed? || target_branch_changed?
-      reload_code
+      reload_diff
     end
   end
 
-  def reload_code
-    if merge_request_diff && open?
-      merge_request_diff.reload_content
-    end
+  def reload_diff
+    return unless merge_request_diff && open?
+
+    old_diff_refs = self.diff_refs
+
+    merge_request_diff.reload_content
+
+    new_diff_refs = self.diff_refs
+
+    update_diff_notes_positions(
+      old_diff_refs: old_diff_refs,
+      new_diff_refs: new_diff_refs
+    )
   end
 
   def check_if_can_be_merged
@@ -646,6 +655,32 @@ class MergeRequest < ActiveRecord::Base
     diff_refs && diff_refs.complete?
   end
 
+  def update_diff_notes_positions(old_diff_refs:, new_diff_refs:)
+    return unless support_new_diff_notes?
+    return if new_diff_refs == old_diff_refs
+
+    active_diff_notes = self.notes.diff_notes.select do |note|
+      note.new_diff_note? && note.active?(old_diff_refs)
+    end
+
+    return if active_diff_notes.empty?
+
+    paths = active_diff_notes.flat_map { |n| n.diff_file.paths }.uniq
+
+    service = Notes::DiffPositionUpdateService.new(
+      self.project,
+      nil,
+      old_diff_refs: old_diff_refs,
+      new_diff_refs: new_diff_refs,
+      paths: paths
+    )
+
+    active_diff_notes.each do |note|
+      service.execute(note)
+      Gitlab::Timeless.timeless(note, &:save)
+    end
+  end
+
   def keep_around_commit
     project.repository.keep_around(self.merge_commit_sha)
   end
diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb
index de79c024428..21490ac77ea 100644
--- a/app/services/merge_requests/refresh_service.rb
+++ b/app/services/merge_requests/refresh_service.rb
@@ -60,7 +60,7 @@ module MergeRequests
 
       merge_requests.each do |merge_request|
         if merge_request.source_branch == @branch_name || force_push?
-          merge_request.reload_code
+          merge_request.reload_diff
           merge_request.mark_as_unchecked
         else
           mr_commit_ids = merge_request.commits.map(&:id)
@@ -68,7 +68,7 @@ module MergeRequests
           matches = mr_commit_ids & push_commit_ids
 
           if matches.any?
-            merge_request.reload_code
+            merge_request.reload_diff
             merge_request.mark_as_unchecked
           else
             merge_request.mark_as_unchecked
diff --git a/app/services/merge_requests/reopen_service.rb b/app/services/merge_requests/reopen_service.rb
index 8279ad2001b..eb88ae9d11c 100644
--- a/app/services/merge_requests/reopen_service.rb
+++ b/app/services/merge_requests/reopen_service.rb
@@ -6,7 +6,7 @@ module MergeRequests
         create_note(merge_request)
         notification_service.reopen_mr(merge_request, current_user)
         execute_hooks(merge_request, 'reopen')
-        merge_request.reload_code
+        merge_request.reload_diff
         merge_request.mark_as_unchecked
       end
 
diff --git a/app/services/notes/diff_position_update_service.rb b/app/services/notes/diff_position_update_service.rb
new file mode 100644
index 00000000000..0cb731f5bc3
--- /dev/null
+++ b/app/services/notes/diff_position_update_service.rb
@@ -0,0 +1,30 @@
+module Notes
+  class DiffPositionUpdateService < BaseService
+    def execute(note)
+      new_position = tracer.trace(note.position)
+
+      # Don't update the position if the type doesn't match, since that means
+      # the diff line commented on was changed, and the comment is now outdated
+      old_position = note.position
+      if new_position &&
+          new_position != old_position &&
+          new_position.type == old_position.type
+
+        note.position = new_position
+      end
+
+      note
+    end
+
+    private
+
+    def tracer
+      @tracer ||= Gitlab::Diff::PositionTracer.new(
+        repository: project.repository,
+        old_diff_refs: params[:old_diff_refs],
+        new_diff_refs: params[:new_diff_refs],
+        paths: params[:paths]
+      )
+    end
+  end
+end