Commit f5ba7ac3 authored by Andreas Brandl's avatar Andreas Brandl

Merge branch 'osw-multi-line-suggestions-creation-strategy' into 'master'

Prepares suggestion implementation for multi-line support

See merge request gitlab-org/gitlab-ce!26057
parents d160e6bc 03e0604d
...@@ -77,8 +77,8 @@ class DiffNote < Note ...@@ -77,8 +77,8 @@ class DiffNote < Note
def supports_suggestion? def supports_suggestion?
return false unless noteable.supports_suggestion? && on_text? return false unless noteable.supports_suggestion? && on_text?
# We don't want to trigger side-effects of `diff_file` call. # We don't want to trigger side-effects of `diff_file` call.
return false unless file = fetch_diff_file return false unless file = latest_diff_file
return false unless line = file.line_for_position(self.original_position) return false unless line = file.line_for_position(self.position)
line&.suggestible? line&.suggestible?
end end
......
...@@ -66,6 +66,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -66,6 +66,7 @@ class MergeRequest < ActiveRecord::Base
has_many :cached_closes_issues, through: :merge_requests_closing_issues, source: :issue 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 :merge_request_pipelines, foreign_key: 'merge_request_id', class_name: 'Ci::Pipeline'
has_many :suggestions, through: :notes
has_many :merge_request_assignees has_many :merge_request_assignees
# Will be deprecated at https://gitlab.com/gitlab-org/gitlab-ce/issues/59457 # Will be deprecated at https://gitlab.com/gitlab-org/gitlab-ce/issues/59457
......
# frozen_string_literal: true # frozen_string_literal: true
class Suggestion < ApplicationRecord class Suggestion < ApplicationRecord
include Suggestible
belongs_to :note, inverse_of: :suggestions belongs_to :note, inverse_of: :suggestions
validates :note, presence: true validates :note, presence: true
validates :commit_id, presence: true, if: :applied? 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 def project
noteable.source_project noteable.source_project
...@@ -19,37 +27,37 @@ class Suggestion < ApplicationRecord ...@@ -19,37 +27,37 @@ class Suggestion < ApplicationRecord
position.file_path position.file_path
end 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 # `from_line_index` and `to_line_index` represents diff/blob line numbers in
# index-like way (N-1). # index-like way (N-1).
def from_line_index def from_line_index
from_line - 1 from_line - 1
end end
alias_method :to_line_index, :from_line_index
def appliable? def to_line_index
return false unless note.supports_suggestion? to_line - 1
end
def appliable?(cached: true)
!applied? && !applied? &&
noteable.opened? && noteable.opened? &&
!outdated?(cached: cached) &&
note.supports_suggestion? &&
different_content? && different_content? &&
note.active? note.active?
end 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 private
def different_content? def different_content?
......
# 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
...@@ -20,6 +20,7 @@ module MergeRequests ...@@ -20,6 +20,7 @@ module MergeRequests
close_upon_missing_source_branch_ref close_upon_missing_source_branch_ref
post_merge_manually_merged post_merge_manually_merged
reload_merge_requests reload_merge_requests
outdate_suggestions
reset_merge_when_pipeline_succeeds reset_merge_when_pipeline_succeeds
mark_pending_todos_done mark_pending_todos_done
cache_merge_requests_closing_issues cache_merge_requests_closing_issues
...@@ -125,6 +126,14 @@ module MergeRequests ...@@ -125,6 +126,14 @@ module MergeRequests
merge_request.source_branch == @push.branch_name merge_request.source_branch == @push.branch_name
end 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 def reset_merge_when_pipeline_succeeds
merge_requests_for_source_branch.each(&:reset_merge_when_pipeline_succeeds) merge_requests_for_source_branch.each(&:reset_merge_when_pipeline_succeeds)
end end
......
...@@ -7,7 +7,7 @@ module Suggestions ...@@ -7,7 +7,7 @@ module Suggestions
end end
def execute(suggestion) def execute(suggestion)
unless suggestion.appliable? unless suggestion.appliable?(cached: false)
return error('Suggestion is not appliable') return error('Suggestion is not appliable')
end end
...@@ -15,7 +15,7 @@ module Suggestions ...@@ -15,7 +15,7 @@ module Suggestions
return error('The file has been changed') return error('The file has been changed')
end end
diff_file = suggestion.note.latest_diff_file diff_file = suggestion.diff_file
unless diff_file unless diff_file
return error('The file was not found') return error('The file was not found')
......
...@@ -9,52 +9,24 @@ module Suggestions ...@@ -9,52 +9,24 @@ module Suggestions
def execute def execute
return unless @note.supports_suggestion? return unless @note.supports_suggestion?
diff_file = @note.latest_diff_file suggestions = Gitlab::Diff::SuggestionsParser.parse(@note.note,
project: @note.project,
return unless diff_file position: @note.position)
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
rows = rows =
suggestions.map.with_index do |suggestion, index| 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 creation_params.merge!(note_id: @note.id, relative_order: index)
# 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
}
end end
rows.in_groups_of(100, false) do |rows| rows.in_groups_of(100, false) do |rows|
Gitlab::Database.bulk_insert('suggestions', rows) Gitlab::Database.bulk_insert('suggestions', rows)
end end
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
end end
# 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
---
title: Implements the creation strategy for multi-line suggestions
merge_request: 26057
author:
type: changed
# frozen_string_literal: true
class AddMultiLineAttributesToSuggestion < ActiveRecord::Migration[5.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
disable_ddl_transaction!
def up
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
remove_columns :suggestions, :outdated, :lines_above, :lines_below
end
end
...@@ -2039,6 +2039,9 @@ ActiveRecord::Schema.define(version: 20190322132835) do ...@@ -2039,6 +2039,9 @@ ActiveRecord::Schema.define(version: 20190322132835) do
t.string "commit_id" t.string "commit_id"
t.text "from_content", null: false t.text "from_content", null: false
t.text "to_content", null: false t.text "to_content", 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 t.index ["note_id", "relative_order"], name: "index_suggestions_on_note_id_and_relative_order", unique: true, using: :btree
end end
......
...@@ -24,8 +24,6 @@ Example response: ...@@ -24,8 +24,6 @@ Example response:
```json ```json
{ {
"id": 36, "id": 36,
"from_original_line": 10,
"to_original_line": 10,
"from_line": 10, "from_line": 10,
"to_line": 10, "to_line": 10,
"appliable": false, "appliable": false,
......
...@@ -1558,8 +1558,6 @@ module API ...@@ -1558,8 +1558,6 @@ module API
class Suggestion < Grape::Entity class Suggestion < Grape::Entity
expose :id expose :id
expose :from_original_line
expose :to_original_line
expose :from_line expose :from_line
expose :to_line expose :to_line
expose :appliable?, as: :appliable expose :appliable?, as: :appliable
......
# frozen_string_literal: true # 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 Banzai
module SuggestionsParser module SuggestionsParser
# Returns the content of each suggestion code block. # Returns the content of each suggestion code block.
......
# 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
...@@ -5,6 +5,47 @@ module Gitlab ...@@ -5,6 +5,47 @@ module Gitlab
class SuggestionsParser class SuggestionsParser
# Matches for instance "-1", "+1" or "-1+2". # Matches for instance "-1", "+1" or "-1+2".
SUGGESTION_CONTEXT = /^(\-(?<above>\d+))?(\+(?<below>\d+))?$/.freeze 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 end
end end
...@@ -33,6 +33,7 @@ project_tree: ...@@ -33,6 +33,7 @@ project_tree:
- :user - :user
- merge_requests: - merge_requests:
- :metrics - :metrics
- :suggestions
- notes: - notes:
- :author - :author
- events: - events:
......
...@@ -16,5 +16,11 @@ FactoryBot.define do ...@@ -16,5 +16,11 @@ FactoryBot.define do
applied true applied true
commit_id { RepoHelpers.sample_commit.id } commit_id { RepoHelpers.sample_commit.id }
end end
trait :content_from_repo do
after(:build) do |suggestion, evaluator|
suggestion.from_content = suggestion.fetch_from_content
end
end
end end
end end
# 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
# 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
...@@ -101,6 +101,7 @@ merge_requests: ...@@ -101,6 +101,7 @@ merge_requests:
- latest_merge_request_diff - latest_merge_request_diff
- merge_request_pipelines - merge_request_pipelines
- merge_request_assignees - merge_request_assignees
- suggestions
merge_request_diff: merge_request_diff:
- merge_request - merge_request
- merge_request_diff_commits - merge_request_diff_commits
...@@ -352,3 +353,5 @@ resource_label_events: ...@@ -352,3 +353,5 @@ resource_label_events:
- label - label
error_tracking_setting: error_tracking_setting:
- project - project
suggestions:
- note
...@@ -608,3 +608,14 @@ ErrorTracking::ProjectErrorTrackingSetting: ...@@ -608,3 +608,14 @@ ErrorTracking::ProjectErrorTrackingSetting:
- project_id - project_id
- project_name - project_name
- organization_name - organization_name
Suggestion:
- id
- note_id
- relative_order
- applied
- commit_id
- from_content
- to_content
- outdated
- lines_above
- lines_below
...@@ -42,8 +42,7 @@ describe API::Suggestions do ...@@ -42,8 +42,7 @@ describe API::Suggestions do
expect(response).to have_gitlab_http_status(200) expect(response).to have_gitlab_http_status(200)
expect(json_response) expect(json_response)
.to include('id', 'from_original_line', 'to_original_line', .to include('id', 'from_line', 'to_line', 'appliable', 'applied',
'from_line', 'to_line', 'appliable', 'applied',
'from_content', 'to_content') 'from_content', 'to_content')
end end
end end
......
...@@ -13,8 +13,8 @@ describe SuggestionEntity do ...@@ -13,8 +13,8 @@ describe SuggestionEntity do
subject { entity.as_json } subject { entity.as_json }
it 'exposes correct attributes' do it 'exposes correct attributes' do
expect(subject).to include(:id, :from_original_line, :to_original_line, :from_line, expect(subject).to include(:id, :from_line, :to_line, :appliable,
:to_line, :appliable, :applied, :from_content, :to_content) :applied, :from_content, :to_content)
end end
it 'exposes current user abilities' do it 'exposes current user abilities' do
......
...@@ -97,6 +97,15 @@ describe MergeRequests::RefreshService do ...@@ -97,6 +97,15 @@ describe MergeRequests::RefreshService do
} }
end 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 context 'when source branch ref does not exists' do
before do before do
DeleteBranchService.new(@project, @user).execute(@merge_request.source_branch) DeleteBranchService.new(@project, @user).execute(@merge_request.source_branch)
...@@ -329,14 +338,16 @@ describe MergeRequests::RefreshService do ...@@ -329,14 +338,16 @@ describe MergeRequests::RefreshService do
context 'push to fork repo source branch' do context 'push to fork repo source branch' do
let(:refresh_service) { service.new(@fork_project, @user) } let(:refresh_service) { service.new(@fork_project, @user) }
context 'open fork merge request' do def refresh
before do
allow(refresh_service).to receive(:execute_hooks) allow(refresh_service).to receive(:execute_hooks)
refresh_service.execute(@oldrev, @newrev, 'refs/heads/master') refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
reload_mrs reload_mrs
end end
context 'open fork merge request' do
it 'executes hooks with update action' do it 'executes hooks with update action' do
refresh
expect(refresh_service).to have_received(:execute_hooks) expect(refresh_service).to have_received(:execute_hooks)
.with(@fork_merge_request, 'update', old_rev: @oldrev) .with(@fork_merge_request, 'update', old_rev: @oldrev)
...@@ -347,21 +358,30 @@ describe MergeRequests::RefreshService do ...@@ -347,21 +358,30 @@ describe MergeRequests::RefreshService do
expect(@build_failed_todo).to be_pending expect(@build_failed_todo).to be_pending
expect(@fork_build_failed_todo).to be_pending expect(@fork_build_failed_todo).to be_pending
end 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 end
context 'closed fork merge request' do context 'closed fork merge request' do
before do before do
@fork_merge_request.close! @fork_merge_request.close!
allow(refresh_service).to receive(:execute_hooks)
refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
reload_mrs
end end
it 'do not execute hooks with update action' do it 'do not execute hooks with update action' do
refresh
expect(refresh_service).not_to have_received(:execute_hooks) expect(refresh_service).not_to have_received(:execute_hooks)
end end
it 'updates merge request to closed state' do it 'updates merge request to closed state' do
refresh
expect(@merge_request.notes).to be_empty expect(@merge_request.notes).to be_empty
expect(@merge_request).to be_open expect(@merge_request).to be_open
expect(@fork_merge_request.notes).to be_empty expect(@fork_merge_request.notes).to be_empty
......
...@@ -5,6 +5,41 @@ require 'spec_helper' ...@@ -5,6 +5,41 @@ require 'spec_helper'
describe Suggestions::ApplyService do describe Suggestions::ApplyService do
include ProjectForksHelper 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(:project) { create(:project, :repository) }
let(:user) { create(:user, :commit_email) } let(:user) { create(:user, :commit_email) }
...@@ -17,8 +52,7 @@ describe Suggestions::ApplyService do ...@@ -17,8 +52,7 @@ describe Suggestions::ApplyService do
end end
let(:suggestion) do let(:suggestion) do
create(:suggestion, note: diff_note, create(:suggestion, :content_from_repo, 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") to_content: " raise RuntimeError, 'Explosion'\n # explosion?\n")
end end
...@@ -84,39 +118,7 @@ describe Suggestions::ApplyService do ...@@ -84,39 +118,7 @@ describe Suggestions::ApplyService do
project.add_maintainer(user) project.add_maintainer(user)
end end
it 'updates the file with the new contents' do it_behaves_like 'successfully creates commit and updates suggestion'
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
context 'when it fails to apply because the file was changed' do context 'when it fails to apply because the file was changed' do
it 'returns error message' do it 'returns error message' do
...@@ -212,11 +214,13 @@ describe Suggestions::ApplyService do ...@@ -212,11 +214,13 @@ describe Suggestions::ApplyService do
end end
def apply_suggestion(suggestion) def apply_suggestion(suggestion)
suggestion.note.reload suggestion.reload
merge_request.reload merge_request.reload
merge_request.clear_memoized_shas merge_request.clear_memoized_shas
result = subject.execute(suggestion) result = subject.execute(suggestion)
expect(result[:status]).to eq(:success)
refresh = MergeRequests::RefreshService.new(project, user) refresh = MergeRequests::RefreshService.new(project, user)
refresh.execute(merge_request.diff_head_sha, refresh.execute(merge_request.diff_head_sha,
suggestion.commit_id, suggestion.commit_id,
...@@ -368,7 +372,18 @@ describe Suggestions::ApplyService do ...@@ -368,7 +372,18 @@ describe Suggestions::ApplyService do
result = subject.execute(suggestion) 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) status: :error)
end end
end end
......
...@@ -40,6 +40,14 @@ describe Suggestions::CreateService do ...@@ -40,6 +40,14 @@ describe Suggestions::CreateService do
```thing ```thing
this is not a suggestion, it's a thing this is not a suggestion, it's a thing
``` ```
```suggestion:-3+2
# multi-line suggestion 1
```
```suggestion:-5
# multi-line suggestion 1
```
MARKDOWN MARKDOWN
end end
...@@ -54,7 +62,7 @@ describe Suggestions::CreateService do ...@@ -54,7 +62,7 @@ describe Suggestions::CreateService do
end end
it 'does not try to parse suggestions' do 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 subject.execute
end end
...@@ -71,7 +79,7 @@ describe Suggestions::CreateService do ...@@ -71,7 +79,7 @@ describe Suggestions::CreateService do
it 'does not try to parse suggestions' do it 'does not try to parse suggestions' do
allow(note).to receive(:on_text?) { false } allow(note).to receive(:on_text?) { false }
expect(Banzai::SuggestionsParser).not_to receive(:parse) expect(Gitlab::Diff::SuggestionsParser).not_to receive(:parse)
subject.execute subject.execute
end end
...@@ -87,7 +95,9 @@ describe Suggestions::CreateService do ...@@ -87,7 +95,9 @@ describe Suggestions::CreateService do
end end
it 'creates no suggestion when diff file is not found' do 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) expect { subject.execute }.not_to change(Suggestion, :count)
end end
...@@ -101,27 +111,30 @@ describe Suggestions::CreateService do ...@@ -101,27 +111,30 @@ describe Suggestions::CreateService do
note: markdown) note: markdown)
end end
context 'single line suggestions' do let(:expected_suggestions) do
Gitlab::Diff::SuggestionsParser.parse(markdown,
project: note.project,
position: note.position)
end
it 'persists suggestion records' do it 'persists suggestion records' do
expect { subject.execute } expect { subject.execute }.to change { note.suggestions.count }
.to change { note.suggestions.count } .from(0).to(expected_suggestions.size)
.from(0)
.to(2)
end end
it 'persists original from_content lines and suggested lines' do it 'persists suggestions data correctly' do
subject.execute subject.execute
suggestions = note.suggestions.order(:relative_order) suggestions = note.suggestions.order(:relative_order)
suggestion_1 = suggestions.first suggestions.zip(expected_suggestions) do |suggestion, expected_suggestion|
suggestion_2 = suggestions.last expected_data = expected_suggestion.to_hash
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", expect(suggestion.from_content).to eq(expected_data[:from_content])
to_content: " xpto\n baz\n") 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 end
context 'outdated position note' do context 'outdated position note' do
...@@ -131,9 +144,8 @@ describe Suggestions::CreateService do ...@@ -131,9 +144,8 @@ describe Suggestions::CreateService do
let(:position) { build_position(diff_refs: latest_diff.diff_refs) } let(:position) { build_position(diff_refs: latest_diff.diff_refs) }
it 'uses the correct position when creating the suggestion' do it 'uses the correct position when creating the suggestion' do
expect(note.position) expect(Gitlab::Diff::SuggestionsParser).to receive(:parse)
.to receive(:diff_file) .with(note.note, project: note.project, position: note.position)
.with(project_with_repo.repository)
.and_call_original .and_call_original
subject.execute subject.execute
...@@ -141,5 +153,4 @@ describe Suggestions::CreateService do ...@@ -141,5 +153,4 @@ describe Suggestions::CreateService do
end end
end end
end end
end
end end
# 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
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