Merge branch 'osw-fetch-latest-version-when-creating-suggestions' into 'master'

Enforce using the latest diff version when creating suggestions

Closes gitlab-ee#9876

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

app/models/diff_note.rb

@@ -41,6 +41,14 @@ class DiffNote < Note
     create_note_diff_file(creation_params)
   end
 
+  # Returns the diff file from `position`
+  def latest_diff_file
+    strong_memoize(:latest_diff_file) do
+      position.diff_file(project.repository)
+    end
+  end
+
+  # Returns the diff file from `original_position`
   def diff_file
     strong_memoize(:diff_file) do
       enqueue_diff_file_creation_job if should_create_diff_file?

app/models/suggestion.rb

@@ -19,11 +19,6 @@ class Suggestion < ApplicationRecord
     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.

app/services/suggestions/apply_service.rb

@@ -15,7 +15,13 @@ module Suggestions
         return error('The file has been changed')
       end
 
-      params = file_update_params(suggestion)
+      diff_file = suggestion.note.latest_diff_file
+
+      unless diff_file
+        return error('The file was not found')
+      end
+
+      params = file_update_params(suggestion, diff_file)
       result = ::Files::UpdateService.new(suggestion.project, @current_user, params).execute
 
       if result[:status] == :success
@@ -38,8 +44,8 @@ module Suggestions
       suggestion.position.head_sha == suggestion.noteable.source_branch_sha
     end
 
-    def file_update_params(suggestion)
-      blob = suggestion.diff_file.new_blob
+    def file_update_params(suggestion, diff_file)
+      blob = diff_file.new_blob
       file_path = suggestion.file_path
       branch_name = suggestion.branch
       file_content = new_file_content(suggestion, blob)

app/services/suggestions/create_service.rb

@@ -9,6 +9,10 @@ module Suggestions
     def execute
       return unless @note.supports_suggestion?
 
+      diff_file = @note.latest_diff_file
+
+      return unless diff_file
+
       suggestions = Banzai::SuggestionsParser.parse(@note.note)
 
       # For single line suggestion we're only looking forward to
@@ -20,7 +24,7 @@ module Suggestions
 
       rows =
         suggestions.map.with_index do |suggestion, index|
-          from_content = changing_lines(comment_line, comment_line)
+          from_content = changing_lines(diff_file, comment_line, comment_line)
 
           # The parsed suggestion doesn't have information about the correct
           # ending characters (we may have a line break, or not), so we take
@@ -44,8 +48,8 @@ module Suggestions
 
     private
 
-    def changing_lines(from_line, to_line)
-      @note.diff_file.new_blob_lines_between(from_line, to_line).join
+    def changing_lines(diff_file, from_line, to_line)
+      diff_file.new_blob_lines_between(from_line, to_line).join
     end
 
     def line_break_chars(line)

changelogs/unreleased/osw-fetch-latest-version-when-creating-suggestions.yml

+---
+title: Always fetch MR latest version when creating suggestions
+merge_request: 25441
+author:
+type: fixed

spec/services/suggestions/apply_service_spec.rb

@@ -362,6 +362,17 @@ describe Suggestions::ApplyService do
       project.add_maintainer(user)
     end
 
+    context 'diff file was not found' do
+      it 'returns error message' do
+        expect(suggestion.note).to receive(:latest_diff_file) { nil }
+
+        result = subject.execute(suggestion)
+
+        expect(result).to eq(message: 'The file was not found',
+                             status: :error)
+      end
+    end
+
     context 'suggestion was already applied' do
       it 'returns success status' do
         result = subject.execute(suggestion)

spec/services/suggestions/create_service_spec.rb

@@ -9,14 +9,18 @@ describe Suggestions::CreateService do
                            target_project: project_with_repo)
   end
 
-  let(:position) do
-    Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb",
-                               new_path: "files/ruby/popen.rb",
-                               old_line: nil,
-                               new_line: 14,
-                               diff_refs: merge_request.diff_refs)
+  def build_position(args = {})
+    default_args = { old_path: "files/ruby/popen.rb",
+                     new_path: "files/ruby/popen.rb",
+                     old_line: nil,
+                     new_line: 14,
+                     diff_refs: merge_request.diff_refs }
+
+    Gitlab::Diff::Position.new(default_args.merge(args))
   end
 
+  let(:position) { build_position }
+
   let(:markdown) do
     <<-MARKDOWN.strip_heredoc
         ```suggestion
@@ -74,6 +78,21 @@ describe Suggestions::CreateService do
       end
     end
 
+    context 'should not create suggestions' do
+      let(:note) do
+        create(:diff_note_on_merge_request, project: project_with_repo,
+                                            noteable: merge_request,
+                                            position: position,
+                                            note: markdown)
+      end
+
+      it 'creates no suggestion when diff file is not found' do
+        expect(note).to receive(:latest_diff_file) { nil }
+
+        expect { subject.execute }.not_to change(Suggestion, :count)
+      end
+    end
+
     context 'should create suggestions' do
       let(:note) do
         create(:diff_note_on_merge_request, project: project_with_repo,
@@ -104,6 +123,22 @@ describe Suggestions::CreateService do
           expect(suggestion_2).to have_attributes(from_content: "    vars = {\n",
                                                   to_content: "  xpto\n  baz\n")
         end
+
+        context 'outdated position note' do
+          let!(:outdated_diff) { merge_request.merge_request_diff }
+          let!(:latest_diff) { merge_request.create_merge_request_diff }
+          let(:outdated_position) { build_position(diff_refs: outdated_diff.diff_refs) }
+          let(:position) { build_position(diff_refs: latest_diff.diff_refs) }
+
+          it 'uses the correct position when creating the suggestion' do
+            expect(note.position)
+              .to receive(:diff_file)
+              .with(project_with_repo.repository)
+              .and_call_original
+
+            subject.execute
+          end
+        end
       end
     end
   end