diff options
| -rw-r--r-- | app/models/diff_note.rb | 8 | ||||
| -rw-r--r-- | app/models/suggestion.rb | 5 | ||||
| -rw-r--r-- | app/services/suggestions/apply_service.rb | 12 | ||||
| -rw-r--r-- | app/services/suggestions/create_service.rb | 10 | ||||
| -rw-r--r-- | changelogs/unreleased/osw-fetch-latest-version-when-creating-suggestions.yml | 5 | ||||
| -rw-r--r-- | spec/services/suggestions/apply_service_spec.rb | 11 | ||||
| -rw-r--r-- | spec/services/suggestions/create_service_spec.rb | 47 |
7 files changed, 81 insertions, 17 deletions
diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb index 279603496b0..805092e527a 100644 --- a/app/models/diff_note.rb +++ b/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? diff --git a/app/models/suggestion.rb b/app/models/suggestion.rb index 7eee4fbbe5f..09034646bff 100644 --- a/app/models/suggestion.rb +++ b/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. diff --git a/app/services/suggestions/apply_service.rb b/app/services/suggestions/apply_service.rb index 1f720fc835f..f778c5aa5f5 100644 --- a/app/services/suggestions/apply_service.rb +++ b/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) diff --git a/app/services/suggestions/create_service.rb b/app/services/suggestions/create_service.rb index 77e958cbe0c..c7ac2452c53 100644 --- a/app/services/suggestions/create_service.rb +++ b/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) diff --git a/changelogs/unreleased/osw-fetch-latest-version-when-creating-suggestions.yml b/changelogs/unreleased/osw-fetch-latest-version-when-creating-suggestions.yml new file mode 100644 index 00000000000..4e01a13d781 --- /dev/null +++ b/changelogs/unreleased/osw-fetch-latest-version-when-creating-suggestions.yml @@ -0,0 +1,5 @@ +--- +title: Always fetch MR latest version when creating suggestions +merge_request: 25441 +author: +type: fixed diff --git a/spec/services/suggestions/apply_service_spec.rb b/spec/services/suggestions/apply_service_spec.rb index 8e77d582eb4..fe85b5c9065 100644 --- a/spec/services/suggestions/apply_service_spec.rb +++ b/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) diff --git a/spec/services/suggestions/create_service_spec.rb b/spec/services/suggestions/create_service_spec.rb index f1142c88a69..1b4b15b8eaa 100644 --- a/spec/services/suggestions/create_service_spec.rb +++ b/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 |