Commit f4ab02b1 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'osw-support-multi-line-suggestions' into 'master'

Support multi-line suggestions

Closes #53310

See merge request gitlab-org/gitlab-ce!25211
parents 30988aec e540c0d7
......@@ -48,10 +48,13 @@ export default {
noteableType: this.noteableType,
noteTargetLine: this.noteTargetLine,
diffViewType: this.diffViewType,
diffFile: this.getDiffFileByHash(this.diffFileHash),
diffFile: this.diffFile,
linePosition: this.linePosition,
};
},
diffFile() {
return this.getDiffFileByHash(this.diffFileHash);
},
},
mounted() {
if (this.isLoggedIn) {
......@@ -102,6 +105,7 @@ export default {
:line-code="line.line_code"
:line="line"
:help-page-path="helpPagePath"
:diff-file="diffFile"
save-button-title="Comment"
class="diff-comment-form"
@handleFormUpdateAddToReview="addToReview"
......
......@@ -61,6 +61,11 @@ export default {
required: false,
default: null,
},
diffFile: {
type: Object,
required: false,
default: null,
},
helpPagePath: {
type: String,
required: false,
......@@ -102,9 +107,42 @@ export default {
}
return '#';
},
diffParams() {
if (this.diffFile) {
return {
filePath: this.diffFile.file_path,
refs: this.diffFile.diff_refs,
};
} else if (this.note && this.note.position) {
return {
filePath: this.note.position.new_path,
refs: this.note.position,
};
} else if (this.discussion && this.discussion.diff_file) {
return {
filePath: this.discussion.diff_file.file_path,
refs: this.discussion.diff_file.diff_refs,
};
}
return null;
},
markdownPreviewPath() {
const notable = this.getNoteableDataByProp('preview_note_path');
return mergeUrlParams({ preview_suggestions: true }, notable);
const previewSuggestions = this.line && this.diffParams;
const params = previewSuggestions
? {
preview_suggestions: previewSuggestions,
line: this.line.new_line,
file_path: this.diffParams.filePath,
base_sha: this.diffParams.refs.base_sha,
start_sha: this.diffParams.refs.start_sha,
head_sha: this.diffParams.refs.head_sha,
}
: {};
return mergeUrlParams(params, notable);
},
markdownDocsPath() {
return this.getNotesDataByProp('markdownDocsPath');
......@@ -234,8 +272,8 @@ export default {
placeholder="Write a comment or drag your files here…"
@keydown.meta.enter="handleKeySubmit()"
@keydown.ctrl.enter="handleKeySubmit()"
@keydown.up="editMyLastNote()"
@keydown.esc="cancelHandler(true)"
@keydown.exact.up="editMyLastNote()"
@keydown.exact.esc="cancelHandler(true)"
@input="onInput"
></textarea>
</markdown-field>
......
/* eslint-disable import/prefer-default-export */
function trimFirstCharOfLineContent(text) {
if (!text) {
return text;
}
return text.replace(/^( |\+|-)/, '');
}
function cleanSuggestionLine(line = {}) {
return {
...line,
text: trimFirstCharOfLineContent(line.text),
};
}
export function selectDiffLines(lines) {
return lines.filter(line => line.type !== 'match').map(line => cleanSuggestionLine(line));
}
......@@ -76,6 +76,7 @@ export default {
hasSuggestion: false,
markdownPreviewLoading: false,
previewMarkdown: false,
suggestions: this.note.suggestions || [],
};
},
computed: {
......@@ -109,9 +110,6 @@ export default {
}
return lineNumber;
},
suggestions() {
return this.note.suggestions || [];
},
lineType() {
return this.line ? this.line.type : '';
},
......@@ -175,6 +173,7 @@ export default {
this.referencedCommands = data.references.commands;
this.referencedUsers = data.references.users;
this.hasSuggestion = data.references.suggestions && data.references.suggestions.length;
this.suggestions = data.references.suggestions;
}
this.$nextTick()
......
......@@ -38,7 +38,7 @@ export default {
].join('\n');
},
mdSuggestion() {
return ['```suggestion', `{text}`, '```'].join('\n');
return ['```suggestion:-0+0', `{text}`, '```'].join('\n');
},
},
mounted() {
......
<script>
import SuggestionDiffHeader from './suggestion_diff_header.vue';
import SuggestionDiffRow from './suggestion_diff_row.vue';
import { selectDiffLines } from '../lib/utils/diff_utils';
export default {
components: {
SuggestionDiffHeader,
SuggestionDiffRow,
},
props: {
newLines: {
type: Array,
required: true,
},
fromContent: {
type: String,
required: false,
default: '',
},
fromLine: {
type: Number,
required: true,
},
suggestion: {
type: Object,
required: true,
......@@ -33,6 +23,11 @@ export default {
required: true,
},
},
computed: {
lines() {
return selectDiffLines(this.suggestion.diff_lines);
},
},
methods: {
applySuggestion(callback) {
this.$emit('apply', { suggestionId: this.suggestion.id, callback });
......@@ -52,22 +47,11 @@ export default {
/>
<table class="mb-3 md-suggestion-diff js-syntax-highlight code">
<tbody>
<!-- Old Line -->
<tr class="line_holder old">
<td class="diff-line-num old_line qa-old-diff-line-number old">{{ fromLine }}</td>
<td class="diff-line-num new_line old"></td>
<td class="line_content old">
<span>{{ fromContent }}</span>
</td>
</tr>
<!-- New Line(s) -->
<tr v-for="(line, key) of newLines" :key="key" class="line_holder new">
<td class="diff-line-num old_line new"></td>
<td class="diff-line-num new_line qa-new-diff-line-number new">{{ line.lineNumber }}</td>
<td class="line_content new">
<span>{{ line.content }}</span>
</td>
</tr>
<suggestion-diff-row
v-for="(line, index) of lines"
:key="`${index}-${line.text}`"
:line="line"
/>
</tbody>
</table>
</div>
......
<script>
export default {
name: 'SuggestionDiffRow',
props: {
line: {
type: Object,
required: true,
},
},
computed: {
lineType() {
return this.line.type;
},
},
};
</script>
<template>
<tr class="line_holder" :class="lineType">
<td class="diff-line-num old_line" :class="lineType">
{{ line.old_line }}
</td>
<td class="diff-line-num new_line" :class="lineType">
{{ line.new_line }}
</td>
<td class="line_content" :class="lineType">
<span v-if="line.text">{{ line.text }}</span>
<!-- TODO: replace this hack with zero-width whitespace when we have rich_text from BE -->
<span v-else>&#8203;</span>
</td>
</tr>
</template>
......@@ -6,16 +6,6 @@ import Flash from '~/flash';
export default {
components: { SuggestionDiff },
props: {
fromLine: {
type: Number,
required: false,
default: 0,
},
fromContent: {
type: String,
required: false,
default: '',
},
lineType: {
type: String,
required: false,
......@@ -71,41 +61,19 @@ export default {
suggestionElements.forEach((suggestionEl, i) => {
const suggestionParentEl = suggestionEl.parentElement;
const newLines = this.extractNewLines(suggestionParentEl);
const diffComponent = this.generateDiff(newLines, i);
const diffComponent = this.generateDiff(i);
diffComponent.$mount(suggestionParentEl);
});
this.isRendered = true;
},
extractNewLines(suggestionEl) {
// extracts the suggested lines from the markdown
// calculates a line number for each line
const newLines = suggestionEl.querySelectorAll('.line');
const fromLine = this.suggestions.length ? this.suggestions[0].from_line : this.fromLine;
const lines = [];
newLines.forEach((line, i) => {
const content = `${line.innerText}\n`;
const lineNumber = fromLine + i;
lines.push({ content, lineNumber });
});
return lines;
},
generateDiff(newLines, suggestionIndex) {
// generates the diff <suggestion-diff /> component
// all `suggestion` markdown will be swapped out by this component
generateDiff(suggestionIndex) {
const { suggestions, disabled, helpPagePath } = this;
const suggestion =
suggestions && suggestions[suggestionIndex] ? suggestions[suggestionIndex] : {};
const fromContent = suggestion.from_content || this.fromContent;
const fromLine = suggestion.from_line || this.fromLine;
const SuggestionDiffComponent = Vue.extend(SuggestionDiff);
const suggestionDiff = new SuggestionDiffComponent({
propsData: { newLines, fromLine, fromContent, disabled, suggestion, helpPagePath },
propsData: { disabled, suggestion, helpPagePath },
});
suggestionDiff.$on('apply', ({ suggestionId, callback }) => {
......
......@@ -20,7 +20,7 @@ module PreviewMarkdown
body: view_context.markdown(result[:text], markdown_params),
references: {
users: result[:users],
suggestions: result[:suggestions],
suggestions: SuggestionSerializer.new.represent_diff(result[:suggestions]),
commands: view_context.markdown(result[:commands])
}
}
......
......@@ -42,6 +42,6 @@ class IssueEntity < IssuableEntity
end
expose :preview_note_path do |issue|
preview_markdown_path(issue.project, quick_actions_target_type: 'Issue', quick_actions_target_id: issue.iid)
preview_markdown_path(issue.project, target_type: 'Issue', target_id: issue.iid)
end
end
......@@ -235,7 +235,7 @@ class MergeRequestWidgetEntity < IssuableEntity
end
expose :preview_note_path do |merge_request|
preview_markdown_path(merge_request.project, quick_actions_target_type: 'MergeRequest', quick_actions_target_id: merge_request.iid)
preview_markdown_path(merge_request.project, target_type: 'MergeRequest', target_id: merge_request.iid)
end
expose :merge_commit_path do |merge_request|
......
......@@ -3,6 +3,8 @@
class SuggestionEntity < API::Entities::Suggestion
include RequestAwareEntity
unexpose :from_line, :to_line, :from_content, :to_content
expose :diff_lines, using: DiffLineEntity
expose :current_user do
expose :can_apply do |suggestion|
Ability.allowed?(current_user, :apply_suggestion, suggestion)
......
# frozen_string_literal: true
class SuggestionSerializer < BaseSerializer
entity SuggestionEntity
def represent_diff(resource)
represent(resource, { only: [:diff_lines] })
end
end
......@@ -2,10 +2,17 @@
module Suggestible
extend ActiveSupport::Concern
include Gitlab::Utils::StrongMemoize
# This translates into limiting suggestion changes to `suggestion:-100+100`.
MAX_LINES_CONTEXT = 100.freeze
def diff_lines
strong_memoize(:diff_lines) do
Gitlab::Diff::SuggestionDiff.new(self).diff_lines
end
end
def fetch_from_content
diff_file.new_blob_lines_between(from_line, to_line).join
end
......
......@@ -17,7 +17,7 @@ class PreviewMarkdownService < BaseService
private
def explain_quick_actions(text)
return text, [] unless %w(Issue MergeRequest Commit).include?(commands_target_type)
return text, [] unless %w(Issue MergeRequest Commit).include?(target_type)
quick_actions_service = QuickActions::InterpretService.new(project, current_user)
quick_actions_service.explain(text, find_commands_target)
......@@ -30,22 +30,34 @@ class PreviewMarkdownService < BaseService
end
def find_suggestions(text)
return [] unless params[:preview_suggestions]
return [] unless preview_sugestions?
Banzai::SuggestionsParser.parse(text)
position = Gitlab::Diff::Position.new(new_path: params[:file_path],
new_line: params[:line].to_i,
base_sha: params[:base_sha],
head_sha: params[:head_sha],
start_sha: params[:start_sha])
Gitlab::Diff::SuggestionsParser.parse(text, position: position, project: project)
end
def preview_sugestions?
params[:preview_suggestions] &&
target_type == 'MergeRequest' &&
Ability.allowed?(current_user, :download_code, project)
end
def find_commands_target
QuickActions::TargetService
.new(project, current_user)
.execute(commands_target_type, commands_target_id)
.execute(target_type, target_id)
end
def commands_target_type
params[:quick_actions_target_type]
def target_type
params[:target_type]
end
def commands_target_id
params[:quick_actions_target_id]
def target_id
params[:target_id]
end
end
......@@ -5,7 +5,7 @@
- supports_quick_actions = model.new_record?
- if supports_quick_actions
- preview_url = preview_markdown_path(project, quick_actions_target_type: model.class.name)
- preview_url = preview_markdown_path(project, target_type: model.class.name)
- else
- preview_url = preview_markdown_path(project)
......
- supports_autocomplete = local_assigns.fetch(:supports_autocomplete, true)
- supports_quick_actions = note_supports_quick_actions?(@note)
- if supports_quick_actions
- preview_url = preview_markdown_path(@project, quick_actions_target_type: @note.noteable_type, quick_actions_target_id: @note.noteable_id)
- preview_url = preview_markdown_path(@project, target_type: @note.noteable_type, target_id: @note.noteable_id)
- else
- preview_url = preview_markdown_path(@project)
......
---
title: Support multi-line suggestions
merge_request: 25211
author:
type: added
......@@ -344,6 +344,24 @@ and push the suggested change directly into the codebase in the merge request's
Custom commit messages will be introduced by
[#54404](https://gitlab.com/gitlab-org/gitlab-ce/issues/54404).
### Multi-line suggestions
> [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/issues/53310) in GitLab 11.10.
Reviewers can also suggest changes to
multiple lines with a single suggestion within Merge Request diff discussions.
![Multi-line suggestion syntax](img/multi-line-suggestion-syntax.png)
In the example above, the suggestion covers three lines above and four lines below the commented diff line.
It'd change from 3 lines _above_ to 4 lines _below_ the commented Diff line.
![Multi-line suggestion preview](img/multi-line-suggestion-preview.png)
NOTE: **Note:**
Suggestions covering multiple lines are limited to 100 lines _above_ and 100 lines _below_
the commented diff line, allowing up to 200 changed lines per suggestion.
## Start a discussion by replying to a standard comment
> [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/issues/30299) in GitLab 11.9
......
# 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.
#
def self.parse(text)
html = Banzai.render(text, project: nil, no_original_data: true)
doc = Nokogiri::HTML(html)
doc.search('pre.suggestion').map { |node| node.text }
end
end
end
......@@ -703,6 +703,16 @@ describe ProjectsController do
expect(JSON.parse(response.body).keys).to match_array(%w(body references))
end
context 'when not authorized' do
let(:private_project) { create(:project, :private) }
it 'returns 404' do
post :preview_markdown, params: { namespace_id: private_project.namespace, id: private_project, text: '*Markdown* text' }
expect(response).to have_gitlab_http_status(404)
end
end
context 'state filter on references' do
let(:issue) { create(:issue, :closed, project: public_project) }
let(:merge_request) { create(:merge_request, :closed, target_project: public_project) }
......
......@@ -6,6 +6,14 @@ describe 'User comments on a diff', :js do
include MergeRequestDiffHelpers
include RepoHelpers
def expect_suggestion_has_content(element, expected_changing_content, expected_suggested_content)
changing_content = element.all(:css, '.line_holder.old').map(&:text)
suggested_content = element.all(:css, '.line_holder.new').map(&:text)
expect(changing_content).to eq(expected_changing_content)
expect(suggested_content).to eq(expected_suggested_content)
end
let(:project) { create(:project, :repository) }
let(:merge_request) do
create(:merge_request_with_diffs, source_project: project, target_project: project, source_branch: 'merge-test')
......@@ -33,8 +41,18 @@ describe 'User comments on a diff', :js do
page.within('.diff-discussions') do
expect(page).to have_button('Apply suggestion')
expect(page).to have_content('Suggested change')
expect(page).to have_content(' url = https://github.com/gitlabhq/gitlab-shell.git')
expect(page).to have_content('# change to a comment')
end
page.within('.md-suggestion-diff') do
expected_changing_content = [
"6 url = https://github.com/gitlabhq/gitlab-shell.git"
]
expected_suggested_content = [
"6 # change to a comment"
]
expect_suggestion_has_content(page, expected_changing_content, expected_suggested_content)
end
end
......@@ -64,7 +82,7 @@ describe 'User comments on a diff', :js do
click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']"))
page.within('.js-discussion-note-form') do
fill_in('note_note', with: "```suggestion\n# change to a comment\n```\n```suggestion\n# or that\n```")
fill_in('note_note', with: "```suggestion\n# change to a comment\n```\n```suggestion:-2\n# or that\n# heh\n```")
click_button('Comment')
end
......@@ -74,11 +92,90 @@ describe 'User comments on a diff', :js do
suggestion_1 = page.all(:css, '.md-suggestion-diff')[0]
suggestion_2 = page.all(:css, '.md-suggestion-diff')[1]
expect(suggestion_1).to have_content(' url = https://github.com/gitlabhq/gitlab-shell.git')
expect(suggestion_1).to have_content('# change to a comment')
suggestion_1_expected_changing_content = [
"6 url = https://github.com/gitlabhq/gitlab-shell.git"
]
suggestion_1_expected_suggested_content = [
"6 # change to a comment"
]
suggestion_2_expected_changing_content = [
"4 [submodule \"gitlab-shell\"]",
"5 path = gitlab-shell",
"6 url = https://github.com/gitlabhq/gitlab-shell.git"
]
suggestion_2_expected_suggested_content = [
"4 # or that",
"5 # heh"
]
expect_suggestion_has_content(suggestion_1,
suggestion_1_expected_changing_content,
suggestion_1_expected_suggested_content)
expect_suggestion_has_content(suggestion_2,
suggestion_2_expected_changing_content,
suggestion_2_expected_suggested_content)
end
end
end
context 'multi-line suggestions' do
it 'suggestion is presented' do
click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']"))
page.within('.js-discussion-note-form') do
fill_in('note_note', with: "```suggestion:-3+5\n# change to a\n# comment\n# with\n# broken\n# lines\n```")
click_button('Comment')
end
expect(suggestion_2).to have_content(' url = https://github.com/gitlabhq/gitlab-shell.git')
expect(suggestion_2).to have_content('# or that')
wait_for_requests
page.within('.diff-discussions') do
expect(page).to have_button('Apply suggestion')
expect(page).to have_content('Suggested change')
end
page.within('.md-suggestion-diff') do
expected_changing_content = [
"3 url = git://github.com/randx/six.git",
"4 [submodule \"gitlab-shell\"]",
"5 path = gitlab-shell",
"6 url = https://github.com/gitlabhq/gitlab-shell.git",
"7 [submodule \"gitlab-grack\"]",
"8 path = gitlab-grack",
"9 url = https://gitlab.com/gitlab-org/gitlab-grack.git"
]
expected_suggested_content = [
"3 # change to a",
"4 # comment",
"5 # with",
"6 # broken",
"7 # lines"
]
expect_suggestion_has_content(page, expected_changing_content, expected_suggested_content)
end
end
it 'suggestion is appliable' do
click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']"))
page.within('.js-discussion-note-form') do
fill_in('note_note', with: "```suggestion:-3+5\n# change to a\n# comment\n# with\n# broken\n# lines\n```")
click_button('Comment')
end
wait_for_requests
page.within('.diff-discussions') do
expect(page).not_to have_content('Applied')
click_button('Apply suggestion')
wait_for_requests
expect(page).to have_content('Applied')
end
end
end
......
import { shallowMount, createLocalVue } from '@vue/test-utils';
import SuggestionDiffRow from '~/vue_shared/components/markdown/suggestion_diff_row.vue';
const oldLine = {
can_receive_suggestion: false,
line_code: null,
meta_data: null,
new_line: null,
old_line: 5,
rich_text: '-oldtext',
text: '-oldtext',
type: 'old',
};
const newLine = {
can_receive_suggestion: false,
line_code: null,
meta_data: null,
new_line: 6,
old_line: null,
rich_text: '-newtext',
text: '-newtext',
type: 'new',
};
describe(SuggestionDiffRow.name, () => {
let wrapper;