diff --git a/app/assets/javascripts/diffs/components/diff_line_note_form.vue b/app/assets/javascripts/diffs/components/diff_line_note_form.vue
index bb66ab3628323e618a65004529da6194ba6d5dbb..41670b4579800da033769fb7cd8bc92fcbd3c95a 100644
--- a/app/assets/javascripts/diffs/components/diff_line_note_form.vue
+++ b/app/assets/javascripts/diffs/components/diff_line_note_form.vue
@@ -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"
diff --git a/app/assets/javascripts/notes/components/note_form.vue b/app/assets/javascripts/notes/components/note_form.vue
index 57d6b181bd7bd6b02f8f1ed6df4b9611e20a1f2c..471323bfc8336acb5fb603ad9b1c2b0a2ff5c321 100644
--- a/app/assets/javascripts/notes/components/note_form.vue
+++ b/app/assets/javascripts/notes/components/note_form.vue
@@ -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"
>
diff --git a/app/assets/javascripts/vue_shared/components/lib/utils/diff_utils.js b/app/assets/javascripts/vue_shared/components/lib/utils/diff_utils.js
new file mode 100644
index 0000000000000000000000000000000000000000..d1aba99ac22f986b4a68c244feab3db7ad9fc536
--- /dev/null
+++ b/app/assets/javascripts/vue_shared/components/lib/utils/diff_utils.js
@@ -0,0 +1,20 @@
+/* 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));
+}
diff --git a/app/assets/javascripts/vue_shared/components/markdown/field.vue b/app/assets/javascripts/vue_shared/components/markdown/field.vue
index eccf73e227c8c98977f3302ddba68e0312a92fb4..0f3b35684141a21e2a755f9764ee57f232751ed0 100644
--- a/app/assets/javascripts/vue_shared/components/markdown/field.vue
+++ b/app/assets/javascripts/vue_shared/components/markdown/field.vue
@@ -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()
diff --git a/app/assets/javascripts/vue_shared/components/markdown/header.vue b/app/assets/javascripts/vue_shared/components/markdown/header.vue
index cc6ecdb0395d67193ef93edd4e93df9f9ecf003b..a5a5b2ef415b33e1134645e4c835c2e14809d483 100644
--- a/app/assets/javascripts/vue_shared/components/markdown/header.vue
+++ b/app/assets/javascripts/vue_shared/components/markdown/header.vue
@@ -38,7 +38,7 @@ export default {
].join('\n');
},
mdSuggestion() {
- return ['```suggestion', `{text}`, '```'].join('\n');
+ return ['```suggestion:-0+0', `{text}`, '```'].join('\n');
},
},
mounted() {
diff --git a/app/assets/javascripts/vue_shared/components/markdown/suggestion_diff.vue b/app/assets/javascripts/vue_shared/components/markdown/suggestion_diff.vue
index a351ca62c94bbfa38ebcf3017140a204d95552f0..2eb4ec12a4a52d36b9c7ece25338108eadc0f8aa 100644
--- a/app/assets/javascripts/vue_shared/components/markdown/suggestion_diff.vue
+++ b/app/assets/javascripts/vue_shared/components/markdown/suggestion_diff.vue
@@ -1,24 +1,14 @@
+
+
+
+ |
+ {{ line.old_line }}
+ |
+
+ {{ line.new_line }}
+ |
+
+ {{ line.text }}
+
+
+ |
+
+
diff --git a/app/assets/javascripts/vue_shared/components/markdown/suggestions.vue b/app/assets/javascripts/vue_shared/components/markdown/suggestions.vue
index 177d78cb904a6c0e49309b00ee91d26edb040744..8d3705e1e4a114ec7258e9aabfba9e4d7fbedd9d 100644
--- a/app/assets/javascripts/vue_shared/components/markdown/suggestions.vue
+++ b/app/assets/javascripts/vue_shared/components/markdown/suggestions.vue
@@ -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 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 }) => {
diff --git a/app/controllers/concerns/preview_markdown.rb b/app/controllers/concerns/preview_markdown.rb
index f72d25fc54c4410668e66776aae7e0a1a9ffafb3..2a9729b6ffd7d9e7ca3366db08e9d070dada5d34 100644
--- a/app/controllers/concerns/preview_markdown.rb
+++ b/app/controllers/concerns/preview_markdown.rb
@@ -20,7 +20,7 @@ def preview_markdown
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])
}
}
diff --git a/app/serializers/issue_entity.rb b/app/serializers/issue_entity.rb
index c3f7d4651fb8f76d9676949db641514d36013b87..914ad628a99e5d9e9a81926cf7bf4b4e8cbbcd95 100644
--- a/app/serializers/issue_entity.rb
+++ b/app/serializers/issue_entity.rb
@@ -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
diff --git a/app/serializers/merge_request_widget_entity.rb b/app/serializers/merge_request_widget_entity.rb
index d673f8ae89654fbe30ab97085f5f2391778290a7..4831eb32c9650fd8559156252d3737149a6511bd 100644
--- a/app/serializers/merge_request_widget_entity.rb
+++ b/app/serializers/merge_request_widget_entity.rb
@@ -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|
diff --git a/app/serializers/suggestion_entity.rb b/app/serializers/suggestion_entity.rb
index 4d0d4da10bea944d3f598158dc84742fa13534b4..2dd62e19e29468edc0b6fd117e7f0c2cb6ab6d6b 100644
--- a/app/serializers/suggestion_entity.rb
+++ b/app/serializers/suggestion_entity.rb
@@ -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)
diff --git a/app/serializers/suggestion_serializer.rb b/app/serializers/suggestion_serializer.rb
new file mode 100644
index 0000000000000000000000000000000000000000..010344f9fcd4866e0a90d6055dc581d2593fa6e9
--- /dev/null
+++ b/app/serializers/suggestion_serializer.rb
@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+class SuggestionSerializer < BaseSerializer
+ entity SuggestionEntity
+
+ def represent_diff(resource)
+ represent(resource, { only: [:diff_lines] })
+ end
+end
diff --git a/app/services/concerns/suggestible.rb b/app/services/concerns/suggestible.rb
index 0b9822b19091b87fd2ceabedf870cff115ab6659..0cba9bf1b8acc0c0b84919f655c9a267e3768b67 100644
--- a/app/services/concerns/suggestible.rb
+++ b/app/services/concerns/suggestible.rb
@@ -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
diff --git a/app/services/preview_markdown_service.rb b/app/services/preview_markdown_service.rb
index c1655c38095350a5ae67179b018bd35668fe1d32..7386530f45ff0c20dec30c4ad1cd694381c9c762 100644
--- a/app/services/preview_markdown_service.rb
+++ b/app/services/preview_markdown_service.rb
@@ -17,7 +17,7 @@ def execute
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 @@ def find_user_references(text)
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
diff --git a/app/views/shared/form_elements/_description.html.haml b/app/views/shared/form_elements/_description.html.haml
index 25df2fe5cd6c57644a1181f99fd6a3cee8b002a2..b11cb8a3076d72eea1b5cbb714c3dfbed4d25bee 100644
--- a/app/views/shared/form_elements/_description.html.haml
+++ b/app/views/shared/form_elements/_description.html.haml
@@ -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)
diff --git a/app/views/shared/notes/_form.html.haml b/app/views/shared/notes/_form.html.haml
index 6a1eea85fded944d6e980cdff84806df43d59613..d91bc6e57c92b35106a01db24468dc692da684c4 100644
--- a/app/views/shared/notes/_form.html.haml
+++ b/app/views/shared/notes/_form.html.haml
@@ -1,7 +1,7 @@
- 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)
diff --git a/changelogs/unreleased/osw-support-multi-line-suggestions.yml b/changelogs/unreleased/osw-support-multi-line-suggestions.yml
new file mode 100644
index 0000000000000000000000000000000000000000..8c8206c3822ffb70603cd454fc3f8d33fd5a0c50
--- /dev/null
+++ b/changelogs/unreleased/osw-support-multi-line-suggestions.yml
@@ -0,0 +1,5 @@
+---
+title: Support multi-line suggestions
+merge_request: 25211
+author:
+type: added
diff --git a/doc/user/discussions/img/multi-line-suggestion-preview.png b/doc/user/discussions/img/multi-line-suggestion-preview.png
new file mode 100644
index 0000000000000000000000000000000000000000..4288d0ba034b29fec5c2ebb03f905aa7cede7bd2
Binary files /dev/null and b/doc/user/discussions/img/multi-line-suggestion-preview.png differ
diff --git a/doc/user/discussions/img/multi-line-suggestion-syntax.png b/doc/user/discussions/img/multi-line-suggestion-syntax.png
new file mode 100644
index 0000000000000000000000000000000000000000..df0c99b84ef48aaa60b1f8dbdaa7afb574116efc
Binary files /dev/null and b/doc/user/discussions/img/multi-line-suggestion-syntax.png differ
diff --git a/doc/user/discussions/index.md b/doc/user/discussions/index.md
index 23b9604a45687a54d769cae620f1890a6bceb8c9..bf41fdecd10ba69a39e25436d10c644893a343bf 100644
--- a/doc/user/discussions/index.md
+++ b/doc/user/discussions/index.md
@@ -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.
+
+
+
+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.
+
+
+
+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
diff --git a/lib/banzai/suggestions_parser.rb b/lib/banzai/suggestions_parser.rb
deleted file mode 100644
index 0d7f751bfc1aac66c2b30cde024009f2db2c8be2..0000000000000000000000000000000000000000
--- a/lib/banzai/suggestions_parser.rb
+++ /dev/null
@@ -1,16 +0,0 @@
-# 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
diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb
index 356d606d5c5d68e61e2885c11a03e59758253856..56d38b9475e0e2c04f616a91e46bf2e3c0c7bb25 100644
--- a/spec/controllers/projects_controller_spec.rb
+++ b/spec/controllers/projects_controller_spec.rb
@@ -703,6 +703,16 @@ def update_project(**parameters)
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) }
diff --git a/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb b/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb
index c19e299097e3b2a1c847162eb9b32cf4614611f5..1b5dd6945e0b7b399340068b37e98a6ae88ed3a3 100644
--- a/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb
+++ b/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb
@@ -6,6 +6,14 @@
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 @@
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 @@
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 @@
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
diff --git a/spec/frontend/vue_shared/components/markdown/suggestion_diff_row_spec.js b/spec/frontend/vue_shared/components/markdown/suggestion_diff_row_spec.js
new file mode 100644
index 0000000000000000000000000000000000000000..866d6eb05c636d8e5b9064f75bdeb8357f349ee3
--- /dev/null
+++ b/spec/frontend/vue_shared/components/markdown/suggestion_diff_row_spec.js
@@ -0,0 +1,98 @@
+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;
+
+ const factory = (options = {}) => {
+ const localVue = createLocalVue();
+
+ wrapper = shallowMount(SuggestionDiffRow, {
+ localVue,
+ ...options,
+ });
+ };
+
+ const findOldLineWrapper = () => wrapper.find('.old_line');
+ const findNewLineWrapper = () => wrapper.find('.new_line');
+
+ afterEach(() => {
+ wrapper.destroy();
+ });
+
+ it('renders correctly', () => {
+ factory({
+ propsData: {
+ line: oldLine,
+ },
+ });
+
+ expect(wrapper.is('.line_holder')).toBe(true);
+ });
+
+ describe('when passed line has type old', () => {
+ beforeEach(() => {
+ factory({
+ propsData: {
+ line: oldLine,
+ },
+ });
+ });
+
+ it('has old class when line has type old', () => {
+ expect(wrapper.find('td').classes()).toContain('old');
+ });
+
+ it('has old line number rendered', () => {
+ expect(findOldLineWrapper().text()).toBe('5');
+ });
+
+ it('has no new line number rendered', () => {
+ expect(findNewLineWrapper().text()).toBe('');
+ });
+ });
+
+ describe('when passed line has type new', () => {
+ beforeEach(() => {
+ factory({
+ propsData: {
+ line: newLine,
+ },
+ });
+ });
+
+ it('has new class when line has type new', () => {
+ expect(wrapper.find('td').classes()).toContain('new');
+ });
+
+ it('has no old line number rendered', () => {
+ expect(findOldLineWrapper().text()).toBe('');
+ });
+
+ it('has no new line number rendered', () => {
+ expect(findNewLineWrapper().text()).toBe('6');
+ });
+ });
+});
diff --git a/spec/javascripts/notes/mock_data.js b/spec/javascripts/notes/mock_data.js
index 348743081eb5d688107014f2c323e399aa463065..1df5cf9ef684774005c30ca9c5a09be462a1eea9 100644
--- a/spec/javascripts/notes/mock_data.js
+++ b/spec/javascripts/notes/mock_data.js
@@ -44,8 +44,7 @@ export const noteableDataMock = {
milestone: null,
milestone_id: null,
moved_to_id: null,
- preview_note_path:
- '/gitlab-org/gitlab-ce/preview_markdown?quick_actions_target_id=98&quick_actions_target_type=Issue',
+ preview_note_path: '/gitlab-org/gitlab-ce/preview_markdown?target_id=98&target_type=Issue',
project_id: 2,
state: 'opened',
time_estimate: 0,
@@ -347,8 +346,7 @@ export const loggedOutnoteableData = {
},
noteable_note_url: '/group/project/merge_requests/1#note_1',
create_note_path: '/gitlab-org/gitlab-ce/notes?target_id=98&target_type=issue',
- preview_note_path:
- '/gitlab-org/gitlab-ce/preview_markdown?quick_actions_target_id=98&quick_actions_target_type=Issue',
+ preview_note_path: '/gitlab-org/gitlab-ce/preview_markdown?target_id=98&target_type=Issue',
};
export const collapseNotesMock = [
diff --git a/spec/javascripts/vue_shared/components/markdown/header_spec.js b/spec/javascripts/vue_shared/components/markdown/header_spec.js
index e733a95288ec18f15eb534d78f078b8e40425514..d4be2451f0b9f9e2c25fda31265c1058153d6818 100644
--- a/spec/javascripts/vue_shared/components/markdown/header_spec.js
+++ b/spec/javascripts/vue_shared/components/markdown/header_spec.js
@@ -98,7 +98,7 @@ describe('Markdown field header component', () => {
it('renders suggestion template', () => {
vm.lineContent = 'Some content';
- expect(vm.mdSuggestion).toEqual('```suggestion\n{text}\n```');
+ expect(vm.mdSuggestion).toEqual('```suggestion:-0+0\n{text}\n```');
});
it('does not render suggestion button if `canSuggest` is set to false', () => {
diff --git a/spec/javascripts/vue_shared/components/markdown/suggestion_diff_spec.js b/spec/javascripts/vue_shared/components/markdown/suggestion_diff_spec.js
index f87c2a92f472b34c2b104892b5f752874065b6a3..ea74cb9eb21c3839a2b43474f5358a9918a48668 100644
--- a/spec/javascripts/vue_shared/components/markdown/suggestion_diff_spec.js
+++ b/spec/javascripts/vue_shared/components/markdown/suggestion_diff_spec.js
@@ -1,21 +1,50 @@
import Vue from 'vue';
import SuggestionDiffComponent from '~/vue_shared/components/markdown/suggestion_diff.vue';
+import { selectDiffLines } from '~/vue_shared/components/lib/utils/diff_utils';
const MOCK_DATA = {
canApply: true,
- newLines: [
- { content: 'Line 1\n', lineNumber: 1 },
- { content: 'Line 2\n', lineNumber: 2 },
- { content: 'Line 3\n', lineNumber: 3 },
- ],
- fromLine: 1,
- fromContent: 'Old content',
suggestion: {
id: 1,
+ diff_lines: [
+ {
+ can_receive_suggestion: false,
+ line_code: null,
+ meta_data: null,
+ new_line: null,
+ old_line: 5,
+ rich_text: '-test',
+ text: '-test',
+ type: 'old',
+ },
+ {
+ can_receive_suggestion: true,
+ line_code: null,
+ meta_data: null,
+ new_line: 5,
+ old_line: null,
+ rich_text: '+new test',
+ text: '+new test',
+ type: 'new',
+ },
+ {
+ can_receive_suggestion: true,
+ line_code: null,
+ meta_data: null,
+ new_line: 5,
+ old_line: null,
+ rich_text: '+new test2',
+ text: '+new test2',
+ type: 'new',
+ },
+ ],
},
helpPagePath: 'path_to_docs',
};
+const lines = selectDiffLines(MOCK_DATA.suggestion.diff_lines);
+const newLines = lines.filter(line => line.type === 'new');
+
describe('Suggestion Diff component', () => {
let vm;
@@ -39,30 +68,23 @@ describe('Suggestion Diff component', () => {
});
it('renders the oldLineNumber', () => {
- const fromLine = vm.$el.querySelector('.qa-old-diff-line-number').innerHTML;
+ const fromLine = vm.$el.querySelector('.old_line').innerHTML;
- expect(parseInt(fromLine, 10)).toBe(vm.fromLine);
+ expect(parseInt(fromLine, 10)).toBe(lines[0].old_line);
});
it('renders the oldLineContent', () => {
const fromContent = vm.$el.querySelector('.line_content.old').innerHTML;
- expect(fromContent.includes(vm.fromContent)).toBe(true);
- });
-
- it('renders the contents of newLines', () => {
- const newLines = vm.$el.querySelectorAll('.line_holder.new');
-
- newLines.forEach((line, i) => {
- expect(newLines[i].innerHTML.includes(vm.newLines[i].content)).toBe(true);
- });
+ expect(fromContent.includes(lines[0].text)).toBe(true);
});
- it('renders a line number for each line', () => {
- const newLineNumbers = vm.$el.querySelectorAll('.qa-new-diff-line-number');
+ it('renders new lines', () => {
+ const newLinesElements = vm.$el.querySelectorAll('.line_holder.new');
- newLineNumbers.forEach((line, i) => {
- expect(newLineNumbers[i].innerHTML.includes(vm.newLines[i].lineNumber)).toBe(true);
+ newLinesElements.forEach((line, i) => {
+ expect(newLinesElements[i].innerHTML.includes(newLines[i].new_line)).toBe(true);
+ expect(newLinesElements[i].innerHTML.includes(newLines[i].text)).toBe(true);
});
});
});
diff --git a/spec/javascripts/vue_shared/components/markdown/suggestions_spec.js b/spec/javascripts/vue_shared/components/markdown/suggestions_spec.js
index 33be63a3a1e48ad744f22231d3047a78c7a5bfbf..b7de40b483120ade6916319413bb482ffccf3d59 100644
--- a/spec/javascripts/vue_shared/components/markdown/suggestions_spec.js
+++ b/spec/javascripts/vue_shared/components/markdown/suggestions_spec.js
@@ -2,46 +2,52 @@ import Vue from 'vue';
import SuggestionsComponent from '~/vue_shared/components/markdown/suggestions.vue';
const MOCK_DATA = {
- fromLine: 1,
- fromContent: 'Old content',
- suggestions: [],
+ suggestions: [
+ {
+ id: 1,
+ appliable: true,
+ applied: false,
+ current_user: {
+ can_apply: true,
+ },
+ diff_lines: [
+ {
+ can_receive_suggestion: false,
+ line_code: null,
+ meta_data: null,
+ new_line: null,
+ old_line: 5,
+ rich_text: '-test',
+ text: '-test',
+ type: 'old',
+ },
+ {
+ can_receive_suggestion: true,
+ line_code: null,
+ meta_data: null,
+ new_line: 5,
+ old_line: null,
+ rich_text: '+new test',
+ text: '+new test',
+ type: 'new',
+ },
+ ],
+ },
+ ],
noteHtml: `
+
-
Suggestion 1
+
+newtest
-
-
`,
isApplied: false,
helpPagePath: 'path_to_docs',
};
-const generateLine = content => {
- const line = document.createElement('div');
- line.className = 'line';
- line.innerHTML = content;
-
- return line;
-};
-
-const generateMockLines = () => {
- const line1 = generateLine('Line 1');
- const line2 = generateLine('Line 2');
- const line3 = generateLine('- Line 3');
- const container = document.createElement('div');
-
- container.appendChild(line1);
- container.appendChild(line2);
- container.appendChild(line3);
-
- return container;
-};
-
describe('Suggestion component', () => {
let vm;
- let extractedLines;
let diffTable;
beforeEach(done => {
@@ -51,8 +57,7 @@ describe('Suggestion component', () => {
propsData: MOCK_DATA,
}).$mount();
- extractedLines = vm.extractNewLines(generateMockLines());
- diffTable = vm.generateDiff(extractedLines).$mount().$el;
+ diffTable = vm.generateDiff(0).$mount().$el;
spyOn(vm, 'renderSuggestions');
vm.renderSuggestions();
@@ -70,32 +75,8 @@ describe('Suggestion component', () => {
it('renders suggestions', () => {
expect(vm.renderSuggestions).toHaveBeenCalled();
- expect(vm.$el.innerHTML.includes('Suggestion 1')).toBe(true);
- expect(vm.$el.innerHTML.includes('Suggestion 2')).toBe(true);
- });
- });
-
- describe('extractNewLines', () => {
- it('extracts suggested lines', () => {
- const expectedReturn = [
- { content: 'Line 1\n', lineNumber: 1 },
- { content: 'Line 2\n', lineNumber: 2 },
- { content: '- Line 3\n', lineNumber: 3 },
- ];
-
- expect(vm.extractNewLines(generateMockLines())).toEqual(expectedReturn);
- });
-
- it('increments line number for each extracted line', () => {
- expect(extractedLines[0].lineNumber).toEqual(1);
- expect(extractedLines[1].lineNumber).toEqual(2);
- expect(extractedLines[2].lineNumber).toEqual(3);
- });
-
- it('returns empty array if no lines are found', () => {
- const el = document.createElement('div');
-
- expect(vm.extractNewLines(el)).toEqual([]);
+ expect(vm.$el.innerHTML.includes('oldtest')).toBe(true);
+ expect(vm.$el.innerHTML.includes('newtest')).toBe(true);
});
});
@@ -109,17 +90,17 @@ describe('Suggestion component', () => {
});
it('generates a diff table that contains contents the suggested lines', () => {
- extractedLines.forEach((line, i) => {
- expect(diffTable.innerHTML.includes(extractedLines[i].content)).toBe(true);
+ MOCK_DATA.suggestions[0].diff_lines.forEach(line => {
+ const text = line.text.substring(1);
+
+ expect(diffTable.innerHTML.includes(text)).toBe(true);
});
});
it('generates a diff table with the correct line number for each suggested line', () => {
- const lines = diffTable.getElementsByClassName('qa-new-diff-line-number');
+ const lines = diffTable.querySelectorAll('.old_line');
- expect([...lines][0].innerHTML).toBe('1');
- expect([...lines][1].innerHTML).toBe('2');
- expect([...lines][2].innerHTML).toBe('3');
+ expect(parseInt([...lines][0].innerHTML, 10)).toBe(5);
});
});
});
diff --git a/spec/lib/banzai/suggestions_parser_spec.rb b/spec/lib/banzai/suggestions_parser_spec.rb
deleted file mode 100644
index 79658d710ce2f9afb82328412f4ebfa324d37cc1..0000000000000000000000000000000000000000
--- a/spec/lib/banzai/suggestions_parser_spec.rb
+++ /dev/null
@@ -1,32 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-describe Banzai::SuggestionsParser do
- describe '.parse' do
- it 'returns a list of suggestion contents' do
- markdown = <<-MARKDOWN.strip_heredoc
- ```suggestion
- foo
- bar
- ```
-
- ```
- nothing
- ```
-
- ```suggestion
- xpto
- baz
- ```
-
- ```thing
- this is not a suggestion, it's a thing
- ```
- MARKDOWN
-
- expect(described_class.parse(markdown)).to eq([" foo\n bar",
- " xpto\n baz"])
- end
- end
-end
diff --git a/spec/lib/gitlab/diff/suggestion_spec.rb b/spec/lib/gitlab/diff/suggestion_spec.rb
index 71fd25df6981a673830877b08c374f3a18552e12..d7ca0e0a52291a02864a429d55d590921f87df71 100644
--- a/spec/lib/gitlab/diff/suggestion_spec.rb
+++ b/spec/lib/gitlab/diff/suggestion_spec.rb
@@ -10,6 +10,16 @@
lines_above: above,
lines_below: below)
end
+
+ it 'returns diff lines with correct line numbers' do
+ diff_lines = suggestion.diff_lines
+
+ expect(diff_lines).to all(be_a(Gitlab::Diff::Line))
+
+ expected_diff_lines.each_with_index do |expected_line, index|
+ expect(diff_lines[index].to_hash).to include(expected_line)
+ end
+ end
end
let(:merge_request) { create(:merge_request) }
@@ -48,6 +58,18 @@ def blob_data
let(:expected_above) { line - 1 }
let(:expected_below) { below }
let(:expected_lines) { blob_lines_data(line - expected_above, line + expected_below) }
+ let(:expected_diff_lines) do
+ [
+ { old_pos: 1, new_pos: 1, type: 'old', text: "-require 'fileutils'" },
+ { old_pos: 2, new_pos: 1, type: 'old', text: "-require 'open3'" },
+ { old_pos: 3, new_pos: 1, type: 'old', text: "-" },
+ { old_pos: 4, new_pos: 1, type: 'old', text: "-module Popen" },
+ { old_pos: 5, new_pos: 1, type: 'old', text: "- extend self" },
+ { old_pos: 6, new_pos: 1, type: 'old', text: "-" },
+ { old_pos: 7, new_pos: 1, type: 'new', text: "+# parsed suggestion content" },
+ { old_pos: 7, new_pos: 2, type: 'new', text: "+# with comments" }
+ ]
+ end
it_behaves_like 'correct suggestion raw content'
end
@@ -59,6 +81,47 @@ def blob_data
let(:expected_below) { below }
let(:expected_above) { above }
let(:expected_lines) { blob_lines_data(line - expected_above, line + expected_below) }
+ let(:expected_diff_lines) do
+ [
+ { old_pos: 4, new_pos: 4, type: "match", text: "@@ -4 +4" },
+ { old_pos: 4, new_pos: 4, type: "old", text: "-module Popen" },
+ { old_pos: 5, new_pos: 4, type: "old", text: "- extend self" },
+ { old_pos: 6, new_pos: 4, type: "old", text: "-" },
+ { old_pos: 7, new_pos: 4, type: "old", text: "- def popen(cmd, path=nil)" },
+ { old_pos: 8, new_pos: 4, type: "old", text: "- unless cmd.is_a?(Array)" },
+ { old_pos: 9, new_pos: 4, type: "old", text: "- raise RuntimeError, \"System commands must be given as an array of strings\"" },
+ { old_pos: 10, new_pos: 4, type: "old", text: "- end" },
+ { old_pos: 11, new_pos: 4, type: "old", text: "-" },
+ { old_pos: 12, new_pos: 4, type: "old", text: "- path ||= Dir.pwd" },
+ { old_pos: 13, new_pos: 4, type: "old", text: "-" },
+ { old_pos: 14, new_pos: 4, type: "old", text: "- vars = {" },
+ { old_pos: 15, new_pos: 4, type: "old", text: "- \"PWD\" => path" },
+ { old_pos: 16, new_pos: 4, type: "old", text: "- }" },
+ { old_pos: 17, new_pos: 4, type: "old", text: "-" },
+ { old_pos: 18, new_pos: 4, type: "old", text: "- options = {" },
+ { old_pos: 19, new_pos: 4, type: "old", text: "- chdir: path" },
+ { old_pos: 20, new_pos: 4, type: "old", text: "- }" },
+ { old_pos: 21, new_pos: 4, type: "old", text: "-" },
+ { old_pos: 22, new_pos: 4, type: "old", text: "- unless File.directory?(path)" },
+ { old_pos: 23, new_pos: 4, type: "old", text: "- FileUtils.mkdir_p(path)" },
+ { old_pos: 24, new_pos: 4, type: "old", text: "- end" },
+ { old_pos: 25, new_pos: 4, type: "old", text: "-" },
+ { old_pos: 26, new_pos: 4, type: "old", text: "- @cmd_output = \"\"" },
+ { old_pos: 27, new_pos: 4, type: "old", text: "- @cmd_status = 0" },
+ { old_pos: 28, new_pos: 4, type: "old", text: "-" },
+ { old_pos: 29, new_pos: 4, type: "old", text: "- Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|" },
+ { old_pos: 30, new_pos: 4, type: "old", text: "- @cmd_output << stdout.read" },
+ { old_pos: 31, new_pos: 4, type: "old", text: "- @cmd_output << stderr.read" },
+ { old_pos: 32, new_pos: 4, type: "old", text: "- @cmd_status = wait_thr.value.exitstatus" },
+ { old_pos: 33, new_pos: 4, type: "old", text: "- end" },
+ { old_pos: 34, new_pos: 4, type: "old", text: "-" },
+ { old_pos: 35, new_pos: 4, type: "old", text: "- return @cmd_output, @cmd_status" },
+ { old_pos: 36, new_pos: 4, type: "old", text: "- end" },
+ { old_pos: 37, new_pos: 4, type: "old", text: "-end" },
+ { old_pos: 38, new_pos: 4, type: "new", text: "+# parsed suggestion content" },
+ { old_pos: 38, new_pos: 5, type: "new", text: "+# with comments" }
+ ]
+ end
it_behaves_like 'correct suggestion raw content'
end
@@ -70,17 +133,19 @@ def blob_data
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) }
+ let(:expected_diff_lines) do
+ [
+ { old_pos: 3, new_pos: 3, type: "match", text: "@@ -3 +3" },
+ { old_pos: 3, new_pos: 3, type: "old", text: "-" },
+ { old_pos: 4, new_pos: 3, type: "old", text: "-module Popen" },
+ { old_pos: 5, new_pos: 3, type: "old", text: "- extend self" },
+ { old_pos: 6, new_pos: 3, type: "old", text: "-" },
+ { old_pos: 7, new_pos: 3, type: "old", text: "- def popen(cmd, path=nil)" },
+ { old_pos: 8, new_pos: 3, type: "old", text: "- unless cmd.is_a?(Array)" },
+ { old_pos: 9, new_pos: 3, type: "new", text: "+# parsed suggestion content" },
+ { old_pos: 9, new_pos: 4, type: "new", text: "+# with comments" }
+ ]
+ end
it_behaves_like 'correct suggestion raw content'
end
diff --git a/spec/lib/gitlab/diff/suggestions_parser_spec.rb b/spec/lib/gitlab/diff/suggestions_parser_spec.rb
index 1119ea04995756dd3b426c86b0ba534ad4cc67c9..1f2af42f6e785556a768630f06ebd73a50bc6942 100644
--- a/spec/lib/gitlab/diff/suggestions_parser_spec.rb
+++ b/spec/lib/gitlab/diff/suggestions_parser_spec.rb
@@ -69,5 +69,66 @@ def blob_lines_data(from_line, to_line)
lines_below: 0)
end
end
+
+ context 'multi-line suggestions' do
+ let(:markdown) do
+ <<-MARKDOWN.strip_heredoc
+ ```suggestion:-2+1
+ # above and below
+ ```
+
+ ```
+ nothing
+ ```
+
+ ```suggestion:-3
+ # only above
+ ```
+
+ ```suggestion:+3
+ # only below
+ ```
+
+ ```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(3)
+ end
+
+ it 'suggestion with above and below param has correct data' do
+ from_line = position.new_line - 2
+ to_line = position.new_line + 1
+
+ expect(subject.first.to_hash).to include(from_content: blob_lines_data(from_line, to_line),
+ to_content: " # above and below\n",
+ lines_above: 2,
+ lines_below: 1)
+ end
+
+ it 'suggestion with above param has correct data' do
+ from_line = position.new_line - 3
+ to_line = position.new_line
+
+ expect(subject.second.to_hash).to eq(from_content: blob_lines_data(from_line, to_line),
+ to_content: " # only above\n",
+ lines_above: 3,
+ lines_below: 0)
+ end
+
+ it 'suggestion with below param has correct data' do
+ from_line = position.new_line
+ to_line = position.new_line + 3
+
+ expect(subject.third.to_hash).to eq(from_content: blob_lines_data(from_line, to_line),
+ to_content: " # only below\n",
+ lines_above: 0,
+ lines_below: 3)
+ end
+ end
end
end
diff --git a/spec/models/suggestion_spec.rb b/spec/models/suggestion_spec.rb
index cafc725dddb2ef414066f1ea94aa79541f3317fb..8d4e9070b19c8b885c5ea1013d6b606fa370a1d5 100644
--- a/spec/models/suggestion_spec.rb
+++ b/spec/models/suggestion_spec.rb
@@ -21,6 +21,22 @@
end
end
+ describe '#diff_lines' do
+ let(:suggestion) { create(:suggestion, :content_from_repo) }
+
+ it 'returns parsed diff lines' do
+ expected_diff_lines = Gitlab::Diff::SuggestionDiff.new(suggestion).diff_lines
+ diff_lines = suggestion.diff_lines
+
+ expect(diff_lines.size).to eq(expected_diff_lines.size)
+ expect(diff_lines).to all(be_a(Gitlab::Diff::Line))
+
+ expected_diff_lines.each_with_index do |expected_line, index|
+ expect(diff_lines[index].to_hash).to eq(expected_line.to_hash)
+ end
+ end
+ end
+
describe '#appliable?' do
context 'when note does not support suggestions' do
it 'returns false' do
diff --git a/spec/serializers/suggestion_entity_spec.rb b/spec/serializers/suggestion_entity_spec.rb
index d38fc2b132b44cf9e2c96235bb27cd57685b76bf..d282a7f9c7abac2317a915998561d5e6417a9390 100644
--- a/spec/serializers/suggestion_entity_spec.rb
+++ b/spec/serializers/suggestion_entity_spec.rb
@@ -13,8 +13,7 @@
subject { entity.as_json }
it 'exposes correct attributes' do
- expect(subject).to include(:id, :from_line, :to_line, :appliable,
- :applied, :from_content, :to_content)
+ expect(subject.keys).to match_array([:id, :appliable, :applied, :diff_lines, :current_user])
end
it 'exposes current user abilities' do
diff --git a/spec/services/preview_markdown_service_spec.rb b/spec/services/preview_markdown_service_spec.rb
index 85515d548a73e10b968698661ff9ddb8022dd7ec..a1d31464e07e5ace5917a818a4b62c10a7b298c4 100644
--- a/spec/services/preview_markdown_service_spec.rb
+++ b/spec/services/preview_markdown_service_spec.rb
@@ -2,7 +2,7 @@
describe PreviewMarkdownService do
let(:user) { create(:user) }
- let(:project) { create(:project) }
+ let(:project) { create(:project, :repository) }
before do
project.add_developer(user)
@@ -20,23 +20,72 @@
end
describe 'suggestions' do
- let(:params) { { text: "```suggestion\nfoo\n```", preview_suggestions: preview_suggestions } }
+ let(:merge_request) do
+ create(:merge_request, target_project: project, source_project: project)
+ end
+ let(:text) { "```suggestion\nfoo\n```" }
+ let(:params) do
+ suggestion_params.merge(text: text,
+ target_type: 'MergeRequest',
+ target_id: merge_request.iid)
+ end
let(:service) { described_class.new(project, user, params) }
context 'when preview markdown param is present' do
- let(:preview_suggestions) { true }
+ let(:path) { "files/ruby/popen.rb" }
+ let(:line) { 10 }
+ let(:diff_refs) { merge_request.diff_refs }
+
+ let(:suggestion_params) do
+ {
+ preview_suggestions: true,
+ file_path: path,
+ line: line,
+ base_sha: diff_refs.base_sha,
+ start_sha: diff_refs.start_sha,
+ head_sha: diff_refs.head_sha
+ }
+ end
+
+ it 'returns suggestions referenced in text' do
+ position = Gitlab::Diff::Position.new(new_path: path,
+ new_line: line,
+ diff_refs: diff_refs)
+
+ expect(Gitlab::Diff::SuggestionsParser)
+ .to receive(:parse)
+ .with(text, position: position, project: merge_request.project)
+ .and_call_original
- it 'returns users referenced in text' do
result = service.execute
- expect(result[:suggestions]).to eq(['foo'])
+ expect(result[:suggestions]).to all(be_a(Gitlab::Diff::Suggestion))
+ end
+
+ context 'when user is not authorized' do
+ let(:another_user) { create(:user) }
+ let(:service) { described_class.new(project, another_user, params) }
+
+ before do
+ project.add_guest(another_user)
+ end
+
+ it 'returns no suggestions' do
+ result = service.execute
+
+ expect(result[:suggestions]).to be_empty
+ end
end
end
context 'when preview markdown param is not present' do
- let(:preview_suggestions) { false }
+ let(:suggestion_params) do
+ {
+ preview_suggestions: false
+ }
+ end
- it 'returns users referenced in text' do
+ it 'returns suggestions referenced in text' do
result = service.execute
expect(result[:suggestions]).to eq([])
@@ -49,8 +98,8 @@
let(:params) do
{
text: "Please do it\n/assign #{user.to_reference}",
- quick_actions_target_type: 'Issue',
- quick_actions_target_id: issue.id
+ target_type: 'Issue',
+ target_id: issue.id
}
end
let(:service) { described_class.new(project, user, params) }
@@ -72,7 +121,7 @@
let(:params) do
{
text: "My work\n/estimate 2y",
- quick_actions_target_type: 'MergeRequest'
+ target_type: 'MergeRequest'
}
end
let(:service) { described_class.new(project, user, params) }
@@ -96,8 +145,8 @@
let(:params) do
{
text: "My work\n/tag v1.2.3 Stable release",
- quick_actions_target_type: 'Commit',
- quick_actions_target_id: commit.id
+ target_type: 'Commit',
+ target_id: commit.id
}
end
let(:service) { described_class.new(project, user, params) }
diff --git a/spec/services/suggestions/apply_service_spec.rb b/spec/services/suggestions/apply_service_spec.rb
index 80b5dcac6c77298736e7363719e36633213f47ce..7732767137c809fc57924fa92d096d2ae3f2f6f2 100644
--- a/spec/services/suggestions/apply_service_spec.rb
+++ b/spec/services/suggestions/apply_service_spec.rb
@@ -51,6 +51,10 @@ def apply(suggestion)
diff_refs: merge_request.diff_refs)
end
+ let(:diff_note) do
+ create(:diff_note_on_merge_request, noteable: merge_request, position: position, project: project)
+ end
+
let(:suggestion) do
create(:suggestion, :content_from_repo, note: diff_note,
to_content: " raise RuntimeError, 'Explosion'\n # explosion?\n")
@@ -108,12 +112,6 @@ def popen(cmd, path=nil)
target_project: project)
end
- let!(:diff_note) do
- create(:diff_note_on_merge_request, noteable: merge_request,
- position: position,
- project: project)
- end
-
before do
project.add_maintainer(user)
end
@@ -192,11 +190,6 @@ def popen(cmd, path=nil)
CONTENT
end
- let(:merge_request) do
- create(:merge_request, source_project: project,
- target_project: project)
- end
-
def create_suggestion(diff, old_line: nil, new_line: nil, from_content:, to_content:, path:)
position = Gitlab::Diff::Position.new(old_path: path,
new_path: path,
@@ -291,6 +284,55 @@ def fetch_raw_diff(suggestion)
expect(suggestion_2_diff.strip).to eq(expected_suggestion_2_diff.strip)
end
end
+
+ context 'multi-line suggestion' do
+ let(:expected_content) do
+ <<~CONTENT
+ require 'fileutils'
+ require 'open3'
+
+ module Popen
+ extend self
+
+ # multi
+ # line
+
+ vars = {
+ "PWD" => path
+ }
+
+ options = {
+ chdir: path
+ }
+
+ unless File.directory?(path)
+ FileUtils.mkdir_p(path)
+ end
+
+ @cmd_output = ""
+ @cmd_status = 0
+
+ Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|
+ @cmd_output << stdout.read
+ @cmd_output << stderr.read
+ @cmd_status = wait_thr.value.exitstatus
+ end
+
+ return @cmd_output, @cmd_status
+ end
+ end
+ CONTENT
+ end
+
+ let(:suggestion) do
+ create(:suggestion, :content_from_repo, note: diff_note,
+ lines_above: 2,
+ lines_below: 3,
+ to_content: "# multi\n# line\n")
+ end
+
+ it_behaves_like 'successfully creates commit and updates suggestion'
+ end
end
context 'fork-project' do