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

[EE] Enforce using the latest diff version when creating suggestions

Closes #9876

See merge request gitlab-org/gitlab-ee!9725

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

ee/spec/services/draft_notes/publish_service_spec.rb

@@ -74,6 +74,82 @@ describe DraftNotes::PublishService do
     end
   end
 
+  context 'draft notes with suggestions' do
+    let(:project) { create(:project, :repository) }
+    let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
+
+    let(:suggestion_note) do
+      <<-MARKDOWN.strip_heredoc
+        ```suggestion
+          foo
+        ```
+      MARKDOWN
+    end
+
+    let!(:draft) { create(:draft_note_on_text_diff, note: suggestion_note, merge_request: merge_request, author: user) }
+
+    it 'creates a suggestion with correct content' do
+      expect { publish(draft: draft) }.to change { Suggestion.count }.by(1)
+        .and change { DiffNote.count }.from(0).to(1)
+
+      suggestion = Suggestion.last
+
+      expect(suggestion.from_line).to eq(14)
+      expect(suggestion.to_line).to eq(14)
+      expect(suggestion.from_content).to eq("    vars = {\n")
+      expect(suggestion.to_content).to eq("  foo\n")
+    end
+
+    context 'when the diff is changed' do
+      let(:file_path) { 'files/ruby/popen.rb' }
+      let(:branch_name) { project.default_branch }
+      let(:commit) { project.repository.commit }
+
+      def update_file(file_path, new_content)
+        params = {
+          file_path: file_path,
+          commit_message: "Update File",
+          file_content: new_content,
+          start_project: project,
+          start_branch: project.default_branch,
+          branch_name: branch_name
+        }
+
+        Files::UpdateService.new(project, user, params).execute
+      end
+
+      before do
+        project.add_developer(user)
+      end
+
+      it 'creates a suggestion based on the latest diff content and positions' do
+        diff_file = merge_request.diffs(paths: [file_path]).diff_files.first
+        raw_data = diff_file.new_blob.data
+
+        # Add a line break to the beginning of the file
+        result = update_file(file_path, raw_data.prepend("\n"))
+        oldrev = merge_request.diff_head_sha
+        newrev = result[:result]
+
+        expect(newrev).to be_present
+
+        # Generates new MR revision at DB level
+        refresh = MergeRequests::RefreshService.new(project, user)
+        refresh.execute(oldrev, newrev, merge_request.source_branch_ref)
+
+        expect { publish(draft: draft) }.to change { Suggestion.count }.by(1)
+          .and change { DiffNote.count }.from(0).to(1)
+
+        suggestion = Suggestion.last
+
+        expect(suggestion.from_line).to eq(15)
+        expect(suggestion.to_line).to eq(15)
+        expect(suggestion.from_content).to eq("    vars = {\n")
+        expect(suggestion.to_content).to eq("  foo\n")
+      end
+    end
+  end
+
   it 'only publishes the draft notes belonging to the current user' do
     other_user = create(:user)
     project.add_maintainer(other_user)

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",
+  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)
+                     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