Merge branch 'osw-fix-quick-suggestion-application-being-reverted' into 'master'

Adjust applied suggestion reverting previous changes

Closes #56017

See merge request gitlab-org/gitlab-ce!24250

(cherry picked from commit 8c1991b9bb7e3b8d606481fdea316d633cfa5eb7)

6e293681 Adjust applied suggestion reverting previous changes

3 files changed, 49 insertions, 19 deletions

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 613860ec31a..f220fa705ce 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -550,15 +550,19 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def diff_refs
-    if persisted?
-      merge_request_diff.diff_refs
-    else
-      Gitlab::Diff::DiffRefs.new(
-        base_sha:  diff_base_sha,
-        start_sha: diff_start_sha,
-        head_sha:  diff_head_sha
-      )
-    end
+    persisted? ? merge_request_diff.diff_refs : repository_diff_refs
+  end
+
+  # Instead trying to fetch the
+  # persisted diff_refs, this method goes
+  # straight to the repository to get the
+  # most recent data possible.
+  def repository_diff_refs
+    Gitlab::Diff::DiffRefs.new(
+      base_sha:  branch_merge_base_sha,
+      start_sha: target_branch_sha,
+      head_sha:  source_branch_sha
+    )
   end
 
   def branch_merge_base_sha
diff --git a/app/models/suggestion.rb b/app/models/suggestion.rb
index c76b8e71507..7eee4fbbe5f 100644
--- a/app/models/suggestion.rb
+++ b/app/models/suggestion.rb
@@ -5,8 +5,7 @@ class Suggestion < ApplicationRecord
   validates :note, presence: true
   validates :commit_id, presence: true, if: :applied?
 
-  delegate :original_position, :position, :diff_file,
-    :noteable, to: :note
+  delegate :original_position, :position, :noteable, to: :note
 
   def project
     noteable.source_project
@@ -16,6 +15,15 @@ class Suggestion < ApplicationRecord
     noteable.source_branch
   end
 
+  def file_path
+    position.file_path
+  end
+
+  def diff_file
+    repository = project.repository
+    position.diff_file(repository)
+  end
+
   # For now, suggestions only serve as a way to send patches that
   # will change a single line (being able to apply multiple in the same place),
   # which explains `from_line` and `to_line` being the same line.
diff --git a/app/services/suggestions/apply_service.rb b/app/services/suggestions/apply_service.rb
index d931d528c86..cc47b46b527 100644
--- a/app/services/suggestions/apply_service.rb
+++ b/app/services/suggestions/apply_service.rb
@@ -11,6 +11,10 @@ module Suggestions
         return error('Suggestion is not appliable')
       end
 
+      unless latest_diff_refs?(suggestion)
+        return error('The file has been changed')
+      end
+
       params = file_update_params(suggestion)
       result = ::Files::UpdateService.new(suggestion.project, @current_user, params).execute
 
@@ -19,30 +23,44 @@ module Suggestions
       end
 
       result
+    rescue Files::UpdateService::FileChangedError
+      error('The file has been changed')
     end
 
     private
 
-    def file_update_params(suggestion)
-      diff_file = suggestion.diff_file
+    # Checks whether the latest diff refs for the branch matches with
+    # the position refs we're using to update the file content. Since
+    # the persisted refs are updated async (for MergeRequest),
+    # it's more consistent to fetch this data directly from the repository.
+    def latest_diff_refs?(suggestion)
+      suggestion.position.diff_refs == suggestion.noteable.repository_diff_refs
+    end
 
-      file_path = diff_file.file_path
-      branch_name = suggestion.noteable.source_branch
-      file_content = new_file_content(suggestion)
+    def file_update_params(suggestion)
+      blob = suggestion.diff_file.new_blob
+      file_path = suggestion.file_path
+      branch_name = suggestion.branch
+      file_content = new_file_content(suggestion, blob)
       commit_message = "Apply suggestion to #{file_path}"
 
+      file_last_commit =
+        Gitlab::Git::Commit.last_for_path(suggestion.project.repository,
+                                          blob.commit_id,
+                                          blob.path)
+
       {
         file_path: file_path,
         branch_name: branch_name,
         start_branch: branch_name,
         commit_message: commit_message,
-        file_content: file_content
+        file_content: file_content,
+        last_commit_sha: file_last_commit&.id
       }
     end
 
-    def new_file_content(suggestion)
+    def new_file_content(suggestion, blob)
       range = suggestion.from_line_index..suggestion.to_line_index
-      blob = suggestion.diff_file.new_blob
 
       blob.load_all_data!
       content = blob.data.lines