kotovalexarian-likes-gitlab/gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

Prepare suggestion implementation for multi-line

Browse source 
Adds the groundwork needed in order to persist multi-line suggestions,
while providing the parsing strategy which will be reused for the
**Preview** as well.
This commit is contained in:
Oswaldo Ferreira 2019-03-15 14:35:34 -03:00
parent 1db3926dd2
commit 03e0604d5d
28 changed files with 620 additions and 156 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
app/models/diff_note.rb
View file

@ -77,8 +77,8 @@ class DiffNote < Note
def supports_suggestion?
return false unless noteable.supports_suggestion? && on_text?
# We don't want to trigger side-effects of `diff_file` call.
return false unless file = fetch_diff_file
return false unless line = file.line_for_position(self.original_position)
return false unless file = latest_diff_file
return false unless line = file.line_for_position(self.position)
line&.suggestible?
end

1
app/models/merge_request.rb
View file

@ -66,6 +66,7 @@ class MergeRequest < ActiveRecord::Base
has_many :cached_closes_issues, through: :merge_requests_closing_issues, source: :issue
has_many :merge_request_pipelines, foreign_key: 'merge_request_id', class_name: 'Ci::Pipeline'
has_many :suggestions, through: :notes
has_many :merge_request_assignees
# Will be deprecated at https://gitlab.com/gitlab-org/gitlab-ce/issues/59457

46
app/models/suggestion.rb
View file

@ -1,11 +1,19 @@
# frozen_string_literal: true
class Suggestion < ApplicationRecord
include Suggestible
belongs_to :note, inverse_of: :suggestions
validates :note, presence: true
validates :commit_id, presence: true, if: :applied?
delegate :original_position, :position, :noteable, to: :note
delegate :position, :noteable, to: :note
scope :active, -> { where(outdated: false) }
def diff_file
note.latest_diff_file
end
def project
noteable.source_project
@ -19,37 +27,37 @@ class Suggestion < ApplicationRecord
position.file_path
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.
# We'll iterate on that in https://gitlab.com/gitlab-org/gitlab-ce/issues/53310
# when allowing multi-line suggestions.
def from_line
position.new_line
end
alias_method :to_line, :from_line
def from_original_line
original_position.new_line
end
alias_method :to_original_line, :from_original_line
# `from_line_index` and `to_line_index` represents diff/blob line numbers in
# index-like way (N-1).
def from_line_index
from_line - 1
end
alias_method :to_line_index, :from_line_index
def appliable?
return false unless note.supports_suggestion?
def to_line_index
to_line - 1
end
def appliable?(cached: true)
!applied? &&
noteable.opened? &&
!outdated?(cached: cached) &&
note.supports_suggestion? &&
different_content? &&
note.active?
end
# Overwrites outdated column
def outdated?(cached: true)
return super() if cached
return true unless diff_file
from_content != fetch_from_content
end
def target_line
position.new_line
end
private
def different_content?

30
app/services/concerns/suggestible.rb Normal file
View file

@ -0,0 +1,30 @@
# frozen_string_literal: true
module Suggestible
extend ActiveSupport::Concern
# This translates into limiting suggestion changes to `suggestion:-100+100`.
MAX_LINES_CONTEXT = 100.freeze
def fetch_from_content
diff_file.new_blob_lines_between(from_line, to_line).join
end
def from_line
real_above = [lines_above, MAX_LINES_CONTEXT].min
[target_line - real_above, 1].max
end
def to_line
real_below = [lines_below, MAX_LINES_CONTEXT].min
target_line + real_below
end
def diff_file
raise NotImplementedError
end
def target_line
raise NotImplementedError
end
end

9
app/services/merge_requests/refresh_service.rb
View file

@ -20,6 +20,7 @@ module MergeRequests
close_upon_missing_source_branch_ref
post_merge_manually_merged
reload_merge_requests
outdate_suggestions
reset_merge_when_pipeline_succeeds
mark_pending_todos_done
cache_merge_requests_closing_issues
@ -125,6 +126,14 @@ module MergeRequests
merge_request.source_branch == @push.branch_name
end
def outdate_suggestions
outdate_service = Suggestions::OutdateService.new
merge_requests_for_source_branch.each do |merge_request|
outdate_service.execute(merge_request)
end
end
def reset_merge_when_pipeline_succeeds
merge_requests_for_source_branch.each(&:reset_merge_when_pipeline_succeeds)
end

4
app/services/suggestions/apply_service.rb
View file

@ -7,7 +7,7 @@ module Suggestions
end
def execute(suggestion)
unless suggestion.appliable?
unless suggestion.appliable?(cached: false)
return error('Suggestion is not appliable')
end
@ -15,7 +15,7 @@ module Suggestions
return error('The file has been changed')
end
diff_file = suggestion.note.latest_diff_file
diff_file = suggestion.diff_file
unless diff_file
return error('The file was not found')

46
app/services/suggestions/create_service.rb
View file

@ -9,52 +9,24 @@ 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
# change the line receiving the comment. Though, in
# https://gitlab.com/gitlab-org/gitlab-ce/issues/53310
# we'll introduce a ```suggestion:L<x>-<y>, so this will
# slightly change.
comment_line = @note.position.new_line
suggestions = Gitlab::Diff::SuggestionsParser.parse(@note.note,
project: @note.project,
position: @note.position)
rows =
suggestions.map.with_index do |suggestion, index|
from_content = changing_lines(diff_file, comment_line, comment_line)
creation_params =
suggestion.to_hash.slice(:from_content,
:to_content,
:lines_above,
:lines_below)
# The parsed suggestion doesn't have information about the correct
# ending characters (we may have a line break, or not), so we take
# this information from the last line being changed (last
# characters).
endline_chars = line_break_chars(from_content.lines.last)
to_content = "#{suggestion}#{endline_chars}"
{
note_id: @note.id,
from_content: from_content,
to_content: to_content,
relative_order: index
}
creation_params.merge!(note_id: @note.id, relative_order: index)
end
rows.in_groups_of(100, false) do |rows|
Gitlab::Database.bulk_insert('suggestions', rows)
end
end
private
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)
match = /\r\n|\r|\n/.match(line)
match[0] if match
end
end
end

19
app/services/suggestions/outdate_service.rb Normal file
View file

@ -0,0 +1,19 @@
# frozen_string_literal: true
module Suggestions
class OutdateService
def execute(merge_request)
# rubocop: disable CodeReuse/ActiveRecord
suggestions = merge_request.suggestions.active.includes(:note)
suggestions.find_in_batches(batch_size: 100) do |group|
outdatable_suggestion_ids = group.select do |suggestion|
suggestion.outdated?(cached: false)
end.map(&:id)
Suggestion.where(id: outdatable_suggestion_ids).update_all(outdated: true)
end
# rubocop: enable CodeReuse/ActiveRecord
end
end
end

5
changelogs/unreleased/osw-multi-line-suggestions-creation-strategy.yml Normal file
View file

@ -0,0 +1,5 @@
---
title: Implements the creation strategy for multi-line suggestions
merge_request: 26057
author:
type: changed

2
db/migrate/20190228192410_add_multi_line_attributes_to_suggestion.rb
View file

@ -8,9 +8,9 @@ class AddMultiLineAttributesToSuggestion < ActiveRecord::Migration[5.0]
disable_ddl_transaction!
def up
add_column_with_default :suggestions, :outdated, :boolean, default: false, allow_null: false
add_column_with_default :suggestions, :lines_above, :integer, default: 0, allow_null: false
add_column_with_default :suggestions, :lines_below, :integer, default: 0, allow_null: false
add_column_with_default :suggestions, :outdated, :boolean, default: false, allow_null: false
end
def down

2
db/schema.rb
View file

@ -2039,9 +2039,9 @@ ActiveRecord::Schema.define(version: 20190322132835) do
t.string "commit_id"
t.text "from_content", null: false
t.text "to_content", null: false
t.boolean "outdated", default: false, null: false
t.integer "lines_above", default: 0, null: false
t.integer "lines_below", default: 0, null: false
t.boolean "outdated", default: false, null: false
t.index ["note_id", "relative_order"], name: "index_suggestions_on_note_id_and_relative_order", unique: true, using: :btree
end

2
doc/api/suggestions.md
View file

@ -24,8 +24,6 @@ Example response:
```json
{
"id": 36,
"from_original_line": 10,
"to_original_line": 10,
"from_line": 10,
"to_line": 10,
"appliable": false,

2
lib/api/entities.rb
View file

@ -1558,8 +1558,6 @@ module API
class Suggestion < Grape::Entity
expose :id
expose :from_original_line
expose :to_original_line
expose :from_line
expose :to_line
expose :appliable?, as: :appliable

2
lib/banzai/suggestions_parser.rb
View file

@ -1,5 +1,7 @@
# frozen_string_literal: true
# TODO: Delete when https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/26107
# exchange this parser by `Gitlab::Diff::SuggestionsParser`.
module Banzai
module SuggestionsParser
# Returns the content of each suggestion code block.

52
lib/gitlab/diff/suggestion.rb Normal file
View file

@ -0,0 +1,52 @@
# frozen_string_literal: true
module Gitlab
module Diff
class Suggestion
include Suggestible
include Gitlab::Utils::StrongMemoize
attr_reader :diff_file, :lines_above, :lines_below,
:target_line
def initialize(text, line:, above:, below:, diff_file:)
@text = text
@target_line = line
@lines_above = above.to_i
@lines_below = below.to_i
@diff_file = diff_file
end
def to_hash
{
from_content: from_content,
to_content: to_content,
lines_above: @lines_above,
lines_below: @lines_below
}
end
def from_content
strong_memoize(:from_content) do
fetch_from_content
end
end
def to_content
# The parsed suggestion doesn't have information about the correct
# ending characters (we may have a line break, or not), so we take
# this information from the last line being changed (last
# characters).
endline_chars = line_break_chars(from_content.lines.last)
"#{@text}#{endline_chars}"
end
private
def line_break_chars(line)
match = /\r\n|\r|\n/.match(line)
match[0] if match
end
end
end
end

41
lib/gitlab/diff/suggestions_parser.rb
View file

@ -5,6 +5,47 @@ module Gitlab
class SuggestionsParser
# Matches for instance "-1", "+1" or "-1+2".
SUGGESTION_CONTEXT = /^(\-(?<above>\d+))?(\+(?<below>\d+))?$/.freeze
class << self
# Returns an array of Gitlab::Diff::Suggestion which represents each
# suggestion in the given text.
#
def parse(text, position:, project:)
return [] unless position.complete?
html = Banzai.render(text, project: nil, no_original_data: true)
doc = Nokogiri::HTML(html)
suggestion_nodes = doc.search('pre.suggestion')
return [] if suggestion_nodes.empty?
diff_file = position.diff_file(project.repository)
suggestion_nodes.map do |node|
lang_param = node['data-lang-params']
lines_above, lines_below = nil
if lang_param && suggestion_params = fetch_suggestion_params(lang_param)
lines_above, lines_below =
suggestion_params[:above],
suggestion_params[:below]
end
Gitlab::Diff::Suggestion.new(node.text,
line: position.new_line,
above: lines_above.to_i,
below: lines_below.to_i,
diff_file: diff_file)
end
end
private
def fetch_suggestion_params(lang_param)
lang_param.match(SUGGESTION_CONTEXT)
end
end
end
end
end

1
lib/gitlab/import_export/import_export.yml
View file

@ -33,6 +33,7 @@ project_tree:
- :user
- merge_requests:
- :metrics
- :suggestions
- notes:
- :author
- events:

6
spec/factories/suggestions.rb
View file

@ -16,5 +16,11 @@ FactoryBot.define do
applied true
commit_id { RepoHelpers.sample_commit.id }
end
trait :content_from_repo do
after(:build) do |suggestion, evaluator|
suggestion.from_content = suggestion.fetch_from_content
end
end
end
end

88
spec/lib/gitlab/diff/suggestion_spec.rb Normal file
View file

@ -0,0 +1,88 @@
# frozen_string_literal: true
require 'spec_helper'
describe Gitlab::Diff::Suggestion do
shared_examples 'correct suggestion raw content' do
it 'returns correct raw data' do
expect(suggestion.to_hash).to include(from_content: expected_lines.join,
to_content: "#{text}\n",
lines_above: above,
lines_below: below)
end
end
let(:merge_request) { create(:merge_request) }
let(:project) { merge_request.project }
let(:position) do
Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb",
new_path: "files/ruby/popen.rb",
old_line: nil,
new_line: 9,
diff_refs: merge_request.diff_refs)
end
let(:diff_file) do
position.diff_file(project.repository)
end
let(:text) { "# parsed suggestion content\n# with comments" }
def blob_lines_data(from_line, to_line)
diff_file.new_blob_lines_between(from_line, to_line)
end
def blob_data
blob = diff_file.new_blob
blob.load_all_data!
blob.data
end
let(:suggestion) do
described_class.new(text, line: line, above: above, below: below, diff_file: diff_file)
end
describe '#to_hash' do
context 'when changing content surpasses the top limit' do
let(:line) { 4 }
let(:above) { 5 }
let(:below) { 2 }
let(:expected_above) { line - 1 }
let(:expected_below) { below }
let(:expected_lines) { blob_lines_data(line - expected_above, line + expected_below) }
it_behaves_like 'correct suggestion raw content'
end
context 'when changing content surpasses the amount of lines in the blob (bottom)' do
let(:line) { 5 }
let(:above) { 1 }
let(:below) { blob_data.lines.size + 10 }
let(:expected_below) { below }
let(:expected_above) { above }
let(:expected_lines) { blob_lines_data(line - expected_above, line + expected_below) }
it_behaves_like 'correct suggestion raw content'
end
context 'when lines are within blob lines boundary' do
let(:line) { 5 }
let(:above) { 2 }
let(:below) { 3 }
let(:expected_below) { below }
let(:expected_above) { above }
let(:expected_lines) { blob_lines_data(line - expected_above, line + expected_below) }
it_behaves_like 'correct suggestion raw content'
end
context 'when no extra lines (single-line suggestion)' do
let(:line) { 5 }
let(:above) { 0 }
let(:below) { 0 }
let(:expected_below) { below }
let(:expected_above) { above }
let(:expected_lines) { blob_lines_data(line - expected_above, line + expected_below) }
it_behaves_like 'correct suggestion raw content'
end
end
end

73
spec/lib/gitlab/diff/suggestions_parser_spec.rb Normal file
View file

@ -0,0 +1,73 @@
# frozen_string_literal: true
require 'spec_helper'
describe Gitlab::Diff::SuggestionsParser do
describe '.parse' do
let(:merge_request) { create(:merge_request) }
let(:project) { merge_request.project }
let(:position) do
Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb",
new_path: "files/ruby/popen.rb",
old_line: nil,
new_line: 9,
diff_refs: merge_request.diff_refs)
end
let(:diff_file) do
position.diff_file(project.repository)
end
subject do
described_class.parse(markdown, project: merge_request.project,
position: position)
end
def blob_lines_data(from_line, to_line)
diff_file.new_blob_lines_between(from_line, to_line).join
end
context 'single-line suggestions' do
let(:markdown) do
<<-MARKDOWN.strip_heredoc
```suggestion
foo
bar
```
```
nothing
```
```suggestion
xpto
baz
```
```thing
this is not a suggestion, it's a thing
```
MARKDOWN
end
it 'returns a list of Gitlab::Diff::Suggestion' do
expect(subject).to all(be_a(Gitlab::Diff::Suggestion))
expect(subject.size).to eq(2)
end
it 'parsed suggestion has correct data' do
from_line, to_line = position.new_line, position.new_line
expect(subject.first.to_hash).to include(from_content: blob_lines_data(from_line, to_line),
to_content: " foo\n bar\n",
lines_above: 0,
lines_below: 0)
expect(subject.second.to_hash).to include(from_content: blob_lines_data(from_line, to_line),
to_content: " xpto\n baz\n",
lines_above: 0,
lines_below: 0)
end
end
end
end

3
spec/lib/gitlab/import_export/all_models.yml
View file

@ -101,6 +101,7 @@ merge_requests:
- latest_merge_request_diff
- merge_request_pipelines
- merge_request_assignees
- suggestions
merge_request_diff:
- merge_request
- merge_request_diff_commits
@ -352,3 +353,5 @@ resource_label_events:
- label
error_tracking_setting:
- project
suggestions:
- note

11
spec/lib/gitlab/import_export/safe_model_attributes.yml
View file

@ -608,3 +608,14 @@ ErrorTracking::ProjectErrorTrackingSetting:
- project_id
- project_name
- organization_name
Suggestion:
- id
- note_id
- relative_order
- applied
- commit_id
- from_content
- to_content
- outdated
- lines_above
- lines_below

3
spec/requests/api/suggestions_spec.rb
View file

@ -42,8 +42,7 @@ describe API::Suggestions do
expect(response).to have_gitlab_http_status(200)
expect(json_response)
.to include('id', 'from_original_line', 'to_original_line',
'from_line', 'to_line', 'appliable', 'applied',
.to include('id', 'from_line', 'to_line', 'appliable', 'applied',
'from_content', 'to_content')
end
end

4
spec/serializers/suggestion_entity_spec.rb
View file

@ -13,8 +13,8 @@ describe SuggestionEntity do
subject { entity.as_json }
it 'exposes correct attributes' do
expect(subject).to include(:id, :from_original_line, :to_original_line, :from_line,
:to_line, :appliable, :applied, :from_content, :to_content)
expect(subject).to include(:id, :from_line, :to_line, :appliable,
:applied, :from_content, :to_content)
end
it 'exposes current user abilities' do

38
spec/services/merge_requests/refresh_service_spec.rb
View file

@ -97,6 +97,15 @@ describe MergeRequests::RefreshService do
}
end
it 'outdates MR suggestions' do
expect_next_instance_of(Suggestions::OutdateService) do |service|
expect(service).to receive(:execute).with(@merge_request).and_call_original
expect(service).to receive(:execute).with(@another_merge_request).and_call_original
end
refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
end
context 'when source branch ref does not exists' do
before do
DeleteBranchService.new(@project, @user).execute(@merge_request.source_branch)
@ -329,14 +338,16 @@ describe MergeRequests::RefreshService do
context 'push to fork repo source branch' do
let(:refresh_service) { service.new(@fork_project, @user) }
context 'open fork merge request' do
before do
allow(refresh_service).to receive(:execute_hooks)
refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
reload_mrs
end
def refresh
allow(refresh_service).to receive(:execute_hooks)
refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
reload_mrs
end
context 'open fork merge request' do
it 'executes hooks with update action' do
refresh
expect(refresh_service).to have_received(:execute_hooks)
.with(@fork_merge_request, 'update', old_rev: @oldrev)
@ -347,21 +358,30 @@ describe MergeRequests::RefreshService do
expect(@build_failed_todo).to be_pending
expect(@fork_build_failed_todo).to be_pending
end
it 'outdates opened forked MR suggestions' do
expect_next_instance_of(Suggestions::OutdateService) do |service|
expect(service).to receive(:execute).with(@fork_merge_request).and_call_original
end
refresh
end
end
context 'closed fork merge request' do
before do
@fork_merge_request.close!
allow(refresh_service).to receive(:execute_hooks)
refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
reload_mrs
end
it 'do not execute hooks with update action' do
refresh
expect(refresh_service).not_to have_received(:execute_hooks)
end
it 'updates merge request to closed state' do
refresh
expect(@merge_request.notes).to be_empty
expect(@merge_request).to be_open
expect(@fork_merge_request.notes).to be_empty

93
spec/services/suggestions/apply_service_spec.rb
View file

@ -5,6 +5,41 @@ require 'spec_helper'
describe Suggestions::ApplyService do
include ProjectForksHelper
shared_examples 'successfully creates commit and updates suggestion' do
def apply(suggestion)
result = subject.execute(suggestion)
expect(result[:status]).to eq(:success)
end
it 'updates the file with the new contents' do
apply(suggestion)
blob = project.repository.blob_at_branch(merge_request.source_branch,
position.new_path)
expect(blob.data).to eq(expected_content)
end
it 'updates suggestion applied and commit_id columns' do
expect { apply(suggestion) }
.to change(suggestion, :applied)
.from(false).to(true)
.and change(suggestion, :commit_id)
.from(nil)
end
it 'created commit has users email and name' do
apply(suggestion)
commit = project.repository.commit
expect(user.commit_email).not_to eq(user.email)
expect(commit.author_email).to eq(user.commit_email)
expect(commit.committer_email).to eq(user.commit_email)
expect(commit.author_name).to eq(user.name)
end
end
let(:project) { create(:project, :repository) }
let(:user) { create(:user, :commit_email) }
@ -17,9 +52,8 @@ describe Suggestions::ApplyService do
end
let(:suggestion) do
create(:suggestion, note: diff_note,
from_content: " raise RuntimeError, \"System commands must be given as an array of strings\"\n",
to_content: " raise RuntimeError, 'Explosion'\n # explosion?\n")
create(:suggestion, :content_from_repo, note: diff_note,
to_content: " raise RuntimeError, 'Explosion'\n # explosion?\n")
end
subject { described_class.new(user) }
@ -84,39 +118,7 @@ describe Suggestions::ApplyService do
project.add_maintainer(user)
end
it 'updates the file with the new contents' do
subject.execute(suggestion)
blob = project.repository.blob_at_branch(merge_request.source_branch,
position.new_path)
expect(blob.data).to eq(expected_content)
end
it 'returns success status' do
result = subject.execute(suggestion)
expect(result[:status]).to eq(:success)
end
it 'updates suggestion applied and commit_id columns' do
expect { subject.execute(suggestion) }
.to change(suggestion, :applied)
.from(false).to(true)
.and change(suggestion, :commit_id)
.from(nil)
end
it 'created commit has users email and name' do
subject.execute(suggestion)
commit = project.repository.commit
expect(user.commit_email).not_to eq(user.email)
expect(commit.author_email).to eq(user.commit_email)
expect(commit.committer_email).to eq(user.commit_email)
expect(commit.author_name).to eq(user.name)
end
it_behaves_like 'successfully creates commit and updates suggestion'
context 'when it fails to apply because the file was changed' do
it 'returns error message' do
@ -212,11 +214,13 @@ describe Suggestions::ApplyService do
end
def apply_suggestion(suggestion)
suggestion.note.reload
suggestion.reload
merge_request.reload
merge_request.clear_memoized_shas
result = subject.execute(suggestion)
expect(result[:status]).to eq(:success)
refresh = MergeRequests::RefreshService.new(project, user)
refresh.execute(merge_request.diff_head_sha,
suggestion.commit_id,
@ -241,7 +245,7 @@ describe Suggestions::ApplyService do
suggestion_2_changes = { old_line: 24,
new_line: 31,
from_content: " @cmd_output << stderr.read\n",
from_content: " @cmd_output << stderr.read\n",
to_content: "# v2 change\n",
path: path }
@ -368,7 +372,18 @@ describe Suggestions::ApplyService do
result = subject.execute(suggestion)
expect(result).to eq(message: 'The file was not found',
expect(result).to eq(message: 'Suggestion is not appliable',
status: :error)
end
end
context 'suggestion is eligible to be outdated' do
it 'returns error message' do
expect(suggestion).to receive(:outdated?) { true }
result = subject.execute(suggestion)
expect(result).to eq(message: 'Suggestion is not appliable',
status: :error)
end
end

87
spec/services/suggestions/create_service_spec.rb
View file

@ -40,6 +40,14 @@ describe Suggestions::CreateService do
```thing
this is not a suggestion, it's a thing
```
```suggestion:-3+2
# multi-line suggestion 1
```
```suggestion:-5
# multi-line suggestion 1
```
MARKDOWN
end
@ -54,7 +62,7 @@ describe Suggestions::CreateService do
end
it 'does not try to parse suggestions' do
expect(Banzai::SuggestionsParser).not_to receive(:parse)
expect(Gitlab::Diff::SuggestionsParser).not_to receive(:parse)
subject.execute
end
@ -71,7 +79,7 @@ describe Suggestions::CreateService do
it 'does not try to parse suggestions' do
allow(note).to receive(:on_text?) { false }
expect(Banzai::SuggestionsParser).not_to receive(:parse)
expect(Gitlab::Diff::SuggestionsParser).not_to receive(:parse)
subject.execute
end
@ -87,7 +95,9 @@ describe Suggestions::CreateService do
end
it 'creates no suggestion when diff file is not found' do
expect(note).to receive(:latest_diff_file) { nil }
expect_next_instance_of(DiffNote) do |diff_note|
expect(diff_note).to receive(:latest_diff_file).twice { nil }
end
expect { subject.execute }.not_to change(Suggestion, :count)
end
@ -101,43 +111,44 @@ describe Suggestions::CreateService do
note: markdown)
end
context 'single line suggestions' do
it 'persists suggestion records' do
expect { subject.execute }
.to change { note.suggestions.count }
.from(0)
.to(2)
end
let(:expected_suggestions) do
Gitlab::Diff::SuggestionsParser.parse(markdown,
project: note.project,
position: note.position)
end
it 'persists suggestion records' do
expect { subject.execute }.to change { note.suggestions.count }
.from(0).to(expected_suggestions.size)
end
it 'persists suggestions data correctly' do
subject.execute
suggestions = note.suggestions.order(:relative_order)
suggestions.zip(expected_suggestions) do |suggestion, expected_suggestion|
expected_data = expected_suggestion.to_hash
expect(suggestion.from_content).to eq(expected_data[:from_content])
expect(suggestion.to_content).to eq(expected_data[:to_content])
expect(suggestion.lines_above).to eq(expected_data[:lines_above])
expect(suggestion.lines_below).to eq(expected_data[:lines_below])
end
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(Gitlab::Diff::SuggestionsParser).to receive(:parse)
.with(note.note, project: note.project, position: note.position)
.and_call_original
it 'persists original from_content lines and suggested lines' do
subject.execute
suggestions = note.suggestions.order(:relative_order)
suggestion_1 = suggestions.first
suggestion_2 = suggestions.last
expect(suggestion_1).to have_attributes(from_content: " vars = {\n",
to_content: " foo\n bar\n")
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

102
spec/services/suggestions/outdate_service_spec.rb Normal file
View file

@ -0,0 +1,102 @@
# frozen_string_literal: true
require 'spec_helper'
describe Suggestions::OutdateService do
describe '#execute' do
let(:merge_request) { create(:merge_request) }
let(:project) { merge_request.target_project }
let(:user) { merge_request.author }
let(:file_path) { 'files/ruby/popen.rb' }
let(:branch_name) { project.default_branch }
let(:diff_file) { suggestion.diff_file }
let(:position) { build_position(file_path, comment_line) }
let(:note) do
create(:diff_note_on_merge_request, noteable: merge_request,
position: position,
project: project)
end
def build_position(path, line)
Gitlab::Diff::Position.new(old_path: path,
new_path: path,
old_line: nil,
new_line: line,
diff_refs: merge_request.diff_refs)
end
def commit_changes(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
def update_file_line(diff_file, change_line, content)
new_lines = diff_file.new_blob.data.lines
new_lines[change_line..change_line] = content
result = commit_changes(diff_file.file_path, new_lines.join)
newrev = result[:result]
expect(result[:status]).to eq(:success)
expect(newrev).to be_present
# Ensure all memoized data is cleared in order
# to generate the new merge_request_diff.
MergeRequest.find(merge_request.id).reload_diff(user)
note.reload
end
before do
project.add_maintainer(user)
end
subject { described_class.new.execute(merge_request) }
context 'when there is a change within multi-line suggestion range' do
let(:comment_line) { 9 }
let(:lines_above) { 8 } # suggesting to change lines 1..9
let(:change_line) { 2 } # line 2 is within the range
let!(:suggestion) do
create(:suggestion, :content_from_repo, note: note, lines_above: lines_above)
end
it 'updates the outdatable suggestion record' do
update_file_line(diff_file, change_line, "# foo\nbar\n")
# Make sure note is still active
expect(note.active?).to be(true)
expect { subject }.to change { suggestion.reload.outdated }
.from(false).to(true)
end
end
context 'when there is no change within multi-line suggestion range' do
let(:comment_line) { 9 }
let(:lines_above) { 3 } # suggesting to change lines 6..9
let(:change_line) { 2 } # line 2 is not within the range
let!(:suggestion) do
create(:suggestion, :content_from_repo, note: note, lines_above: lines_above)
end
subject { described_class.new.execute(merge_request) }
it 'does not outdates suggestion record' do
update_file_line(diff_file, change_line, "# foo\nbar\n")
# Make sure note is still active
expect(note.active?).to be(true)
expect { subject }.not_to change { suggestion.reload.outdated }.from(false)
end
end
end
end