Commit e116fcab authored by Oswaldo Ferreira's avatar Oswaldo Ferreira

Always fetch MR latest version when creating suggestions

This is an issue that can only be seen through EE. Further
details can be seen on
https://gitlab.com/gitlab-org/gitlab-ee/issues/9876. In general
we should always use the latest diff version of a file in order
to both create and apply suggestions.
parent 3395eacb
......@@ -41,6 +41,15 @@ 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
repository = project.repository
position.diff_file(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?
......
......@@ -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.
......
......@@ -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)
......
......@@ -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)
......
---
title: Always fetch MR latest version when creating suggestions
merge_request: 25441
author:
type: fixed
......@@ -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)
......
......@@ -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
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment