diff options
author | Oswaldo Ferreira <oswaldo@gitlab.com> | 2018-12-13 19:17:19 +0000 |
---|---|---|
committer | Phil Hughes <me@iamphill.com> | 2018-12-13 19:17:19 +0000 |
commit | ed3034bbb71d43b12944a9da29b5264cb3ff3312 (patch) | |
tree | 3110713f4455a4b3a830e177422663e082fc0eb9 | |
parent | eb81c1239ef86561b4304339609be32318419dbb (diff) | |
download | gitlab-ce-ed3034bbb71d43b12944a9da29b5264cb3ff3312.tar.gz |
Allow suggesting single line changes in diffs
86 files changed, 2150 insertions, 25 deletions
diff --git a/app/assets/javascripts/api.js b/app/assets/javascripts/api.js index e2740981a4b..7607c4b3b79 100644 --- a/app/assets/javascripts/api.js +++ b/app/assets/javascripts/api.js @@ -25,6 +25,7 @@ const Api = { userStatusPath: '/api/:version/users/:id/status', userPostStatusPath: '/api/:version/user/status', commitPath: '/api/:version/projects/:id/repository/commits', + applySuggestionPath: '/api/:version/suggestions/:id/apply', commitPipelinesPath: '/:project_id/commit/:sha/pipelines', branchSinglePath: '/api/:version/projects/:id/repository/branches/:branch', createBranchPath: '/api/:version/projects/:id/repository/branches', @@ -185,6 +186,12 @@ const Api = { }); }, + applySuggestion(id) { + const url = Api.buildUrl(Api.applySuggestionPath).replace(':id', encodeURIComponent(id)); + + return axios.put(url); + }, + commitPipelines(projectId, sha) { const encodedProjectId = projectId .split('/') diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index f0e82b1ed27..d4c1b07093d 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -42,6 +42,11 @@ export default { type: Object, required: true, }, + helpPagePath: { + type: String, + required: false, + default: '', + }, changesEmptyStateIllustration: { type: String, required: false, @@ -208,6 +213,7 @@ export default { v-for="file in diffFiles" :key="file.newPath" :file="file" + :help-page-path="helpPagePath" :can-current-user-fork="canCurrentUserFork" /> </template> diff --git a/app/assets/javascripts/diffs/components/diff_content.vue b/app/assets/javascripts/diffs/components/diff_content.vue index 11cc4c09fed..ac963f2971e 100644 --- a/app/assets/javascripts/diffs/components/diff_content.vue +++ b/app/assets/javascripts/diffs/components/diff_content.vue @@ -23,6 +23,11 @@ export default { type: Object, required: true, }, + helpPagePath: { + type: String, + required: false, + default: '', + }, }, computed: { ...mapState({ @@ -74,11 +79,13 @@ export default { v-if="isInlineView" :diff-file="diffFile" :diff-lines="diffFile.highlighted_diff_lines || []" + :help-page-path="helpPagePath" /> <parallel-diff-view v-if="isParallelView" :diff-file="diffFile" :diff-lines="diffFile.parallel_diff_lines || []" + :help-page-path="helpPagePath" /> </template> <diff-viewer diff --git a/app/assets/javascripts/diffs/components/diff_discussions.vue b/app/assets/javascripts/diffs/components/diff_discussions.vue index bee29b04e92..b2021cd6061 100644 --- a/app/assets/javascripts/diffs/components/diff_discussions.vue +++ b/app/assets/javascripts/diffs/components/diff_discussions.vue @@ -13,6 +13,11 @@ export default { type: Array, required: true, }, + line: { + type: Object, + required: false, + default: null, + }, shouldCollapseDiscussions: { type: Boolean, required: false, @@ -23,6 +28,11 @@ export default { required: false, default: false, }, + helpPagePath: { + type: String, + required: false, + default: '', + }, }, methods: { ...mapActions(['toggleDiscussion']), @@ -72,6 +82,8 @@ export default { :render-diff-file="false" :always-expanded="true" :discussions-by-diff-order="true" + :line="line" + :help-page-path="helpPagePath" @noteDeleted="deleteNoteHandler" > <span v-if="renderAvatarBadge" slot="avatar-badge" class="badge badge-pill"> diff --git a/app/assets/javascripts/diffs/components/diff_file.vue b/app/assets/javascripts/diffs/components/diff_file.vue index bed29efb253..449f7007077 100644 --- a/app/assets/javascripts/diffs/components/diff_file.vue +++ b/app/assets/javascripts/diffs/components/diff_file.vue @@ -23,6 +23,11 @@ export default { type: Boolean, required: true, }, + helpPagePath: { + type: String, + required: false, + default: '', + }, }, data() { return { @@ -164,6 +169,7 @@ export default { v-if="!isCollapsed && file.renderIt" :class="{ hidden: isCollapsed || file.too_large }" :diff-file="file" + :help-page-path="helpPagePath" /> <gl-loading-icon v-if="showLoadingIcon" class="diff-content loading" /> <div v-else-if="showExpandMessage" class="nothing-here-block diff-collapsed"> 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 9fd02acbd6e..e7569ba7b84 100644 --- a/app/assets/javascripts/diffs/components/diff_line_note_form.vue +++ b/app/assets/javascripts/diffs/components/diff_line_note_form.vue @@ -94,6 +94,7 @@ export default { ref="noteForm" :is-editing="true" :line-code="line.line_code" + :line="line" save-button-title="Comment" class="diff-comment-form" @cancelForm="handleCancelCommentForm" diff --git a/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue b/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue index aa40b24950a..814ee0b7c02 100644 --- a/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue +++ b/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue @@ -16,6 +16,11 @@ export default { type: String, required: true, }, + helpPagePath: { + type: String, + required: false, + default: '', + }, }, computed: { className() { @@ -38,7 +43,12 @@ export default { <tr v-if="shouldRender" :class="className" class="notes_holder"> <td class="notes_content" colspan="3"> <div class="content"> - <diff-discussions v-if="line.discussions.length" :discussions="line.discussions" /> + <diff-discussions + v-if="line.discussions.length" + :line="line" + :discussions="line.discussions" + :help-page-path="helpPagePath" + /> <diff-line-note-form v-if="line.hasForm" :diff-file-hash="diffFileHash" diff --git a/app/assets/javascripts/diffs/components/inline_diff_view.vue b/app/assets/javascripts/diffs/components/inline_diff_view.vue index 6a0ce760e6d..9310e2b7ca9 100644 --- a/app/assets/javascripts/diffs/components/inline_diff_view.vue +++ b/app/assets/javascripts/diffs/components/inline_diff_view.vue @@ -17,6 +17,11 @@ export default { type: Array, required: true, }, + helpPagePath: { + type: String, + required: false, + default: '', + }, }, computed: { ...mapGetters('diffs', ['commitId']), @@ -47,6 +52,7 @@ export default { :key="`icr-${index}`" :diff-file-hash="diffFile.file_hash" :line="line" + :help-page-path="helpPagePath" /> </template> </tbody> diff --git a/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue b/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue index b98463d3dd3..a65cf025cde 100644 --- a/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue +++ b/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue @@ -20,6 +20,11 @@ export default { type: Number, required: true, }, + helpPagePath: { + type: String, + required: false, + default: '', + }, }, computed: { hasExpandedDiscussionOnLeft() { @@ -87,6 +92,8 @@ export default { <diff-discussions v-if="line.left.discussions.length" :discussions="line.left.discussions" + :line="line.left" + :help-page-path="helpPagePath" /> </div> <diff-line-note-form @@ -102,6 +109,8 @@ export default { <diff-discussions v-if="line.right.discussions.length" :discussions="line.right.discussions" + :line="line.right" + :help-page-path="helpPagePath" /> </div> <diff-line-note-form diff --git a/app/assets/javascripts/diffs/components/parallel_diff_view.vue b/app/assets/javascripts/diffs/components/parallel_diff_view.vue index 9a6e0e82529..e6bc0daebb3 100644 --- a/app/assets/javascripts/diffs/components/parallel_diff_view.vue +++ b/app/assets/javascripts/diffs/components/parallel_diff_view.vue @@ -17,6 +17,11 @@ export default { type: Array, required: true, }, + helpPagePath: { + type: String, + required: false, + default: '', + }, }, computed: { ...mapGetters('diffs', ['commitId']), @@ -49,6 +54,7 @@ export default { :line="line" :diff-file-hash="diffFile.file_hash" :line-index="index" + :help-page-path="helpPagePath" /> </template> </tbody> diff --git a/app/assets/javascripts/diffs/index.js b/app/assets/javascripts/diffs/index.js index 915cacb374f..b130cedc24c 100644 --- a/app/assets/javascripts/diffs/index.js +++ b/app/assets/javascripts/diffs/index.js @@ -16,6 +16,7 @@ export default function initDiffsApp(store) { return { endpoint: dataset.endpoint, projectPath: dataset.projectPath, + helpPagePath: dataset.helpPagePath, currentUser: JSON.parse(dataset.currentUserData) || {}, changesEmptyStateIllustration: dataset.changesEmptyStateIllustration, }; @@ -31,6 +32,7 @@ export default function initDiffsApp(store) { endpoint: this.endpoint, currentUser: this.currentUser, projectPath: this.projectPath, + helpPagePath: this.helpPagePath, shouldShow: this.activeTab === 'diffs', changesEmptyStateIllustration: this.changesEmptyStateIllustration, }, diff --git a/app/assets/javascripts/lib/utils/text_markdown.js b/app/assets/javascripts/lib/utils/text_markdown.js index 3618c6af7e2..c095a017866 100644 --- a/app/assets/javascripts/lib/utils/text_markdown.js +++ b/app/assets/javascripts/lib/utils/text_markdown.js @@ -39,7 +39,14 @@ function blockTagText(text, textArea, blockTag, selected) { } } -function moveCursor({ textArea, tag, positionBetweenTags, removedLastNewLine, select }) { +function moveCursor({ + textArea, + tag, + cursorOffset, + positionBetweenTags, + removedLastNewLine, + select, +}) { var pos; if (!textArea.setSelectionRange) { return; @@ -61,11 +68,24 @@ function moveCursor({ textArea, tag, positionBetweenTags, removedLastNewLine, se pos -= 1; } + if (cursorOffset) { + pos -= cursorOffset; + } + return textArea.setSelectionRange(pos, pos); } } -export function insertMarkdownText({ textArea, text, tag, blockTag, selected, wrap, select }) { +export function insertMarkdownText({ + textArea, + text, + tag, + cursorOffset, + blockTag, + selected, + wrap, + select, +}) { var textToInsert, selectedSplit, startChar, @@ -154,20 +174,30 @@ export function insertMarkdownText({ textArea, text, tag, blockTag, selected, wr return moveCursor({ textArea, tag: tag.replace(textPlaceholder, selected), + cursorOffset, positionBetweenTags: wrap && selected.length === 0, removedLastNewLine, select, }); } -function updateText({ textArea, tag, blockTag, wrap, select }) { +function updateText({ textArea, tag, cursorOffset, blockTag, wrap, select, tagContent }) { var $textArea, selected, text; $textArea = $(textArea); textArea = $textArea.get(0); text = $textArea.val(); - selected = selectedText(text, textArea); + selected = selectedText(text, textArea) || tagContent; $textArea.focus(); - return insertMarkdownText({ textArea, text, tag, blockTag, selected, wrap, select }); + return insertMarkdownText({ + textArea, + text, + tag, + cursorOffset, + blockTag, + selected, + wrap, + select, + }); } export function addMarkdownListeners(form) { @@ -178,9 +208,11 @@ export function addMarkdownListeners(form) { return updateText({ textArea: $this.closest('.md-area').find('textarea'), tag: $this.data('mdTag'), + cursorOffset: $this.data('mdCursorOffset'), blockTag: $this.data('mdBlock'), wrap: !$this.data('mdPrepend'), select: $this.data('mdSelect'), + tagContent: $this.data('mdTagContent').toString(), }); }); } diff --git a/app/assets/javascripts/mr_notes/index.js b/app/assets/javascripts/mr_notes/index.js index 1c98683c597..e4d72eb8318 100644 --- a/app/assets/javascripts/mr_notes/index.js +++ b/app/assets/javascripts/mr_notes/index.js @@ -33,6 +33,7 @@ export default function initMrNotes() { noteableData, currentUserData: JSON.parse(notesDataset.currentUserData), notesData: JSON.parse(notesDataset.notesData), + helpPagePath: notesDataset.helpPagePath, }; }, computed: { @@ -71,6 +72,7 @@ export default function initMrNotes() { notesData: this.notesData, userData: this.currentUserData, shouldShow: this.activeTab === 'show', + helpPagePath: this.helpPagePath, }, }); }, diff --git a/app/assets/javascripts/notes/components/note_body.vue b/app/assets/javascripts/notes/components/note_body.vue index c0bee600181..bcf5d334da4 100644 --- a/app/assets/javascripts/notes/components/note_body.vue +++ b/app/assets/javascripts/notes/components/note_body.vue @@ -1,10 +1,12 @@ <script> +import { mapActions } from 'vuex'; import $ from 'jquery'; import noteEditedText from './note_edited_text.vue'; import noteAwardsList from './note_awards_list.vue'; import noteAttachment from './note_attachment.vue'; import noteForm from './note_form.vue'; import autosave from '../mixins/autosave'; +import Suggestions from '~/vue_shared/components/markdown/suggestions.vue'; export default { components: { @@ -12,6 +14,7 @@ export default { noteAwardsList, noteAttachment, noteForm, + Suggestions, }, mixins: [autosave], props: { @@ -19,6 +22,11 @@ export default { type: Object, required: true, }, + line: { + type: Object, + required: false, + default: null, + }, canEdit: { type: Boolean, required: true, @@ -28,11 +36,22 @@ export default { required: false, default: false, }, + helpPagePath: { + type: String, + required: false, + default: '', + }, }, computed: { noteBody() { return this.note.note; }, + hasSuggestion() { + return this.note.suggestions && this.note.suggestions.length; + }, + lineType() { + return this.line ? this.line.type : null; + }, }, mounted() { this.renderGFM(); @@ -53,6 +72,7 @@ export default { } }, methods: { + ...mapActions(['submitSuggestion']), renderGFM() { $(this.$refs['note-body']).renderGFM(); }, @@ -62,19 +82,35 @@ export default { formCancelHandler(shouldConfirm, isDirty) { this.$emit('cancelForm', shouldConfirm, isDirty); }, + applySuggestion({ suggestionId, flashContainer, callback }) { + const { discussion_id: discussionId, id: noteId } = this.note; + + this.submitSuggestion({ discussionId, noteId, suggestionId, flashContainer, callback }); + }, }, }; </script> <template> <div ref="note-body" :class="{ 'js-task-list-container': canEdit }" class="note-body"> - <div class="note-text md" v-html="note.note_html"></div> + <suggestions + v-if="hasSuggestion && !isEditing" + :suggestions="note.suggestions" + :note-html="note.note_html" + :line-type="lineType" + :help-page-path="helpPagePath" + @apply="applySuggestion" + /> + <div v-else class="note-text md" v-html="note.note_html"></div> <note-form v-if="isEditing" ref="noteForm" :is-editing="isEditing" :note-body="noteBody" :note-id="note.id" + :line="line" + :note="note" + :help-page-path="helpPagePath" :markdown-version="note.cached_markdown_version" @handleFormUpdate="handleFormUpdate" @cancelForm="formCancelHandler" diff --git a/app/assets/javascripts/notes/components/note_form.vue b/app/assets/javascripts/notes/components/note_form.vue index 95164183ccb..9b7f3d3588d 100644 --- a/app/assets/javascripts/notes/components/note_form.vue +++ b/app/assets/javascripts/notes/components/note_form.vue @@ -1,4 +1,5 @@ <script> +import { mergeUrlParams } from '~/lib/utils/url_utility'; import { mapGetters, mapActions } from 'vuex'; import eventHub from '../event_hub'; import issueWarning from '../../vue_shared/components/issue/issue_warning.vue'; @@ -53,6 +54,21 @@ export default { required: false, default: false, }, + line: { + type: Object, + required: false, + default: null, + }, + note: { + type: Object, + required: false, + default: null, + }, + helpPagePath: { + type: String, + required: false, + default: '', + }, }, data() { return { @@ -79,7 +95,8 @@ export default { return '#'; }, markdownPreviewPath() { - return this.getNoteableDataByProp('preview_note_path'); + const notable = this.getNoteableDataByProp('preview_note_path'); + return mergeUrlParams({ preview_suggestions: true }, notable); }, markdownDocsPath() { return this.getNotesDataByProp('markdownDocsPath'); @@ -93,6 +110,18 @@ export default { isDisabled() { return !this.updatedNoteBody.length || this.isSubmitting; }, + discussionNote() { + const discussionNote = this.discussion.id + ? this.getDiscussionLastNote(this.discussion) + : this.note; + return discussionNote || {}; + }, + canSuggest() { + return ( + this.getNoteableData.can_receive_suggestion && + (this.line && this.line.can_receive_suggestion) + ); + }, }, watch: { noteBody() { @@ -171,7 +200,11 @@ export default { :markdown-docs-path="markdownDocsPath" :markdown-version="markdownVersion" :quick-actions-docs-path="quickActionsDocsPath" + :line="line" + :note="discussionNote" + :can-suggest="canSuggest" :add-spacing-classes="false" + :help-page-path="helpPagePath" > <textarea id="note_note" diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue b/app/assets/javascripts/notes/components/noteable_discussion.vue index 5c9a28b8512..4156fe0d229 100644 --- a/app/assets/javascripts/notes/components/noteable_discussion.vue +++ b/app/assets/javascripts/notes/components/noteable_discussion.vue @@ -49,6 +49,11 @@ export default { type: Object, required: true, }, + line: { + type: Object, + required: false, + default: null, + }, renderDiffFile: { type: Boolean, required: false, @@ -64,6 +69,11 @@ export default { required: false, default: false, }, + helpPagePath: { + type: String, + required: false, + default: '', + }, }, data() { const { diff_discussion: isDiffDiscussion, resolved } = this.discussion; @@ -194,6 +204,13 @@ export default { false, ); }, + diffLine() { + if (this.discussion.diff_discussion && this.discussion.truncated_diff_lines) { + return this.discussion.truncated_diff_lines.slice(-1)[0]; + } + + return this.line; + }, }, watch: { isReplying() { @@ -357,6 +374,8 @@ Please check your network connection and try again.`; <component :is="componentName(initialDiscussion)" :note="componentData(initialDiscussion)" + :line="line" + :help-page-path="helpPagePath" @handleDeleteNote="deleteNoteHandler" > <slot slot="avatar-badge" name="avatar-badge"></slot> @@ -373,6 +392,8 @@ Please check your network connection and try again.`; v-for="note in replies" :key="note.id" :note="componentData(note)" + :help-page-path="helpPagePath" + :line="line" @handleDeleteNote="deleteNoteHandler" /> </template> @@ -383,6 +404,8 @@ Please check your network connection and try again.`; v-for="(note, index) in discussion.notes" :key="note.id" :note="componentData(note)" + :help-page-path="helpPagePath" + :line="diffLine" @handleDeleteNote="deleteNoteHandler" > <slot v-if="index === 0" slot="avatar-badge" name="avatar-badge"></slot> @@ -447,6 +470,7 @@ Please check your network connection and try again.`; ref="noteForm" :discussion="discussion" :is-editing="false" + :line="diffLine" save-button-title="Comment" @handleFormUpdate="saveReply" @cancelForm="cancelReplyForm" diff --git a/app/assets/javascripts/notes/components/noteable_note.vue b/app/assets/javascripts/notes/components/noteable_note.vue index a17be51353e..57e9c40bd61 100644 --- a/app/assets/javascripts/notes/components/noteable_note.vue +++ b/app/assets/javascripts/notes/components/noteable_note.vue @@ -27,6 +27,16 @@ export default { type: Object, required: true, }, + line: { + type: Object, + required: false, + default: null, + }, + helpPagePath: { + type: String, + required: false, + default: '', + }, }, data() { return { @@ -220,8 +230,10 @@ export default { <note-body ref="noteBody" :note="note" + :line="line" :can-edit="note.current_user.can_edit" :is-editing="isEditing" + :help-page-path="helpPagePath" @handleFormUpdate="formUpdateHandler" @cancelForm="formCancelHandler" /> diff --git a/app/assets/javascripts/notes/components/notes_app.vue b/app/assets/javascripts/notes/components/notes_app.vue index 27f896cee35..f3fcfdfda05 100644 --- a/app/assets/javascripts/notes/components/notes_app.vue +++ b/app/assets/javascripts/notes/components/notes_app.vue @@ -49,6 +49,11 @@ export default { required: false, default: 0, }, + helpPagePath: { + type: String, + required: false, + default: '', + }, }, data() { return { @@ -206,6 +211,7 @@ export default { :key="discussion.id" :discussion="discussion" :render-diff-file="true" + :help-page-path="helpPagePath" /> </template> </ul> diff --git a/app/assets/javascripts/notes/services/notes_service.js b/app/assets/javascripts/notes/services/notes_service.js index 47a6f07cce2..237e70c0a4c 100644 --- a/app/assets/javascripts/notes/services/notes_service.js +++ b/app/assets/javascripts/notes/services/notes_service.js @@ -1,4 +1,5 @@ import Vue from 'vue'; +import Api from '~/api'; import VueResource from 'vue-resource'; import * as constants from '../constants'; @@ -44,4 +45,7 @@ export default { toggleIssueState(endpoint, data) { return Vue.http.put(endpoint, data); }, + applySuggestion(id) { + return Api.applySuggestion(id); + }, }; diff --git a/app/assets/javascripts/notes/stores/actions.js b/app/assets/javascripts/notes/stores/actions.js index 4716ab52333..65f85314fa0 100644 --- a/app/assets/javascripts/notes/stores/actions.js +++ b/app/assets/javascripts/notes/stores/actions.js @@ -405,5 +405,25 @@ export const startTaskList = ({ dispatch }) => export const updateResolvableDiscussonsCounts = ({ commit }) => commit(types.UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS); +export const submitSuggestion = ( + { commit }, + { discussionId, noteId, suggestionId, flashContainer, callback }, +) => { + service + .applySuggestion(suggestionId) + .then(() => { + commit(types.APPLY_SUGGESTION, { discussionId, noteId, suggestionId }); + callback(); + }) + .catch(() => { + Flash( + __('Something went wrong while applying the suggestion. Please try again.'), + 'alert', + flashContainer, + ); + callback(); + }); +}; + // prevent babel-plugin-rewire from generating an invalid default during karma tests export default () => {}; diff --git a/app/assets/javascripts/notes/stores/modules/index.js b/app/assets/javascripts/notes/stores/modules/index.js index b5fe8bdb1d3..887e6d22b06 100644 --- a/app/assets/javascripts/notes/stores/modules/index.js +++ b/app/assets/javascripts/notes/stores/modules/index.js @@ -20,6 +20,7 @@ export default () => ({ userData: {}, noteableData: { current_user: {}, + preview_note_path: 'path/to/preview', }, commentsDisabled: false, resolvableDiscussionsCount: 0, diff --git a/app/assets/javascripts/notes/stores/mutation_types.js b/app/assets/javascripts/notes/stores/mutation_types.js index 9c68ab67a8c..df943c155f4 100644 --- a/app/assets/javascripts/notes/stores/mutation_types.js +++ b/app/assets/javascripts/notes/stores/mutation_types.js @@ -16,6 +16,7 @@ export const SET_DISCUSSION_DIFF_LINES = 'SET_DISCUSSION_DIFF_LINES'; export const SET_NOTES_FETCHED_STATE = 'SET_NOTES_FETCHED_STATE'; export const SET_NOTES_LOADING_STATE = 'SET_NOTES_LOADING_STATE'; export const DISABLE_COMMENTS = 'DISABLE_COMMENTS'; +export const APPLY_SUGGESTION = 'APPLY_SUGGESTION'; // DISCUSSION export const COLLAPSE_DISCUSSION = 'COLLAPSE_DISCUSSION'; diff --git a/app/assets/javascripts/notes/stores/mutations.js b/app/assets/javascripts/notes/stores/mutations.js index 39ff0ff73d7..8992454be2e 100644 --- a/app/assets/javascripts/notes/stores/mutations.js +++ b/app/assets/javascripts/notes/stores/mutations.js @@ -197,6 +197,17 @@ export default { } }, + [types.APPLY_SUGGESTION](state, { noteId, discussionId, suggestionId }) { + const noteObj = utils.findNoteObjectById(state.discussions, discussionId); + const comment = utils.findNoteObjectById(noteObj.notes, noteId); + + comment.suggestions = comment.suggestions.map(suggestion => ({ + ...suggestion, + applied: suggestion.applied || suggestion.id === suggestionId, + appliable: false, + })); + }, + [types.UPDATE_DISCUSSION](state, noteData) { const note = noteData; const selectedDiscussion = state.discussions.find(disc => disc.id === note.id); diff --git a/app/assets/javascripts/vue_shared/components/markdown/field.vue b/app/assets/javascripts/vue_shared/components/markdown/field.vue index 43def2673eb..2f7ed4a982c 100644 --- a/app/assets/javascripts/vue_shared/components/markdown/field.vue +++ b/app/assets/javascripts/vue_shared/components/markdown/field.vue @@ -1,17 +1,21 @@ <script> import $ from 'jquery'; +import _ from 'underscore'; import { __ } from '~/locale'; +import { stripHtml } from '~/lib/utils/text_utility'; import Flash from '../../../flash'; import GLForm from '../../../gl_form'; import markdownHeader from './header.vue'; import markdownToolbar from './toolbar.vue'; import icon from '../icon.vue'; +import Suggestions from '~/vue_shared/components/markdown/suggestions.vue'; export default { components: { markdownHeader, markdownToolbar, icon, + Suggestions, }, props: { markdownPreviewPath: { @@ -48,12 +52,33 @@ export default { required: false, default: true, }, + line: { + type: Object, + required: false, + default: null, + }, + note: { + type: Object, + required: false, + default: () => ({}), + }, + canSuggest: { + type: Boolean, + required: false, + default: false, + }, + helpPagePath: { + type: String, + required: false, + default: '', + }, }, data() { return { markdownPreview: '', referencedCommands: '', referencedUsers: '', + hasSuggestion: false, markdownPreviewLoading: false, previewMarkdown: false, }; @@ -63,6 +88,39 @@ export default { const referencedUsersThreshold = 10; return this.referencedUsers.length >= referencedUsersThreshold; }, + lineContent() { + const FIRST_CHAR_REGEX = /^(\+|-)/; + const [firstSuggestion] = this.suggestions; + if (firstSuggestion) { + return firstSuggestion.from_content; + } + + if (this.line) { + const { rich_text: richText, text } = this.line; + + if (text) { + return text.replace(FIRST_CHAR_REGEX, ''); + } + + return _.unescape(stripHtml(richText).replace(/\n/g, '')); + } + + return ''; + }, + lineNumber() { + let lineNumber; + if (this.line) { + const { new_line: newLine, old_line: oldLine } = this.line; + lineNumber = newLine || oldLine; + } + return lineNumber; + }, + suggestions() { + return this.note.suggestions || []; + }, + lineType() { + return this.line ? this.line.type : ''; + }, }, mounted() { /* @@ -122,6 +180,7 @@ export default { if (data.references) { this.referencedCommands = data.references.commands; this.referencedUsers = data.references.users; + this.hasSuggestion = data.references.suggestions && data.references.suggestions.length; } this.$nextTick(() => { @@ -147,6 +206,8 @@ export default { > <markdown-header :preview-markdown="previewMarkdown" + :line-content="lineContent" + :can-suggest="canSuggest" @preview-markdown="showPreviewTab" @write-markdown="showWriteTab" /> @@ -163,19 +224,39 @@ export default { /> </div> </div> - <div - v-show="previewMarkdown" - ref="markdown-preview" - class="md-preview js-vue-md-preview md md-preview-holder" - v-html="markdownPreview" - ></div> + <template v-if="hasSuggestion"> + <div + v-show="previewMarkdown" + ref="markdown-preview" + class="md-preview js-vue-md-preview md md-preview-holder" + > + <suggestions + v-if="hasSuggestion" + :note-html="markdownPreview" + :from-line="lineNumber" + :from-content="lineContent" + :line-type="lineType" + :disabled="true" + :suggestions="suggestions" + :help-page-path="helpPagePath" + /> + </div> + </template> + <template v-else> + <div + v-show="previewMarkdown" + ref="markdown-preview" + class="md-preview js-vue-md-preview md md-preview-holder" + v-html="markdownPreview" + ></div> + </template> <template v-if="previewMarkdown && !markdownPreviewLoading"> <div v-if="referencedCommands" class="referenced-commands" v-html="referencedCommands"></div> <div v-if="shouldShowReferencedUsers" class="referenced-users"> <span> - <i class="fa fa-exclamation-triangle" aria-hidden="true"> </i> You are about to add + <i class="fa fa-exclamation-triangle" aria-hidden="true"></i> You are about to add <strong> - <span class="js-referenced-users-count"> {{ referencedUsers.length }} </span> + <span class="js-referenced-users-count">{{ referencedUsers.length }}</span> </strong> people to the discussion. Proceed with caution. </span> diff --git a/app/assets/javascripts/vue_shared/components/markdown/header.vue b/app/assets/javascripts/vue_shared/components/markdown/header.vue index 4c4ba537065..bf4d42670ee 100644 --- a/app/assets/javascripts/vue_shared/components/markdown/header.vue +++ b/app/assets/javascripts/vue_shared/components/markdown/header.vue @@ -17,6 +17,16 @@ export default { type: Boolean, required: true, }, + lineContent: { + type: String, + required: false, + default: '', + }, + canSuggest: { + type: Boolean, + required: false, + default: true, + }, }, computed: { mdTable() { @@ -27,6 +37,9 @@ export default { '| cell | cell |', ].join('\n'); }, + mdSuggestion() { + return ['```suggestion', `{text}`, '```'].join('\n'); + }, }, mounted() { $(document).on('markdown-preview:show.vue', this.previewMarkdownTab); @@ -119,6 +132,16 @@ export default { :button-title="__('Add a table')" icon="table" /> + <toolbar-button + v-if="canSuggest" + :tag="mdSuggestion" + :prepend="true" + :button-title="__('Insert suggestion')" + :cursor-offset="4" + :tag-content="lineContent" + icon="doc-code" + class="qa-suggestion-btn" + /> <button v-gl-tooltip aria-label="Go full screen" diff --git a/app/assets/javascripts/vue_shared/components/markdown/suggestion_diff.vue b/app/assets/javascripts/vue_shared/components/markdown/suggestion_diff.vue new file mode 100644 index 00000000000..f98560f7336 --- /dev/null +++ b/app/assets/javascripts/vue_shared/components/markdown/suggestion_diff.vue @@ -0,0 +1,74 @@ +<script> +import SuggestionDiffHeader from './suggestion_diff_header.vue'; + +export default { + components: { + SuggestionDiffHeader, + }, + props: { + newLines: { + type: Array, + required: true, + }, + fromContent: { + type: String, + required: false, + default: '', + }, + fromLine: { + type: Number, + required: true, + }, + suggestion: { + type: Object, + required: true, + }, + disabled: { + type: Boolean, + required: false, + default: false, + }, + helpPagePath: { + type: String, + required: true, + }, + }, + methods: { + applySuggestion(callback) { + this.$emit('apply', { suggestionId: this.suggestion.id, callback }); + }, + }, +}; +</script> + +<template> + <div> + <suggestion-diff-header + class="qa-suggestion-diff-header" + :can-apply="suggestion.appliable && suggestion.current_user.can_apply && !disabled" + :is-applied="suggestion.applied" + :help-page-path="helpPagePath" + @apply="applySuggestion" + /> + <table class="mb-3 md-suggestion-diff"> + <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> + </tbody> + </table> + </div> +</template> diff --git a/app/assets/javascripts/vue_shared/components/markdown/suggestion_diff_header.vue b/app/assets/javascripts/vue_shared/components/markdown/suggestion_diff_header.vue new file mode 100644 index 00000000000..563e2f94fcc --- /dev/null +++ b/app/assets/javascripts/vue_shared/components/markdown/suggestion_diff_header.vue @@ -0,0 +1,60 @@ +<script> +import Icon from '~/vue_shared/components/icon.vue'; + +export default { + components: { Icon }, + props: { + canApply: { + type: Boolean, + required: false, + default: false, + }, + isApplied: { + type: Boolean, + required: true, + default: false, + }, + helpPagePath: { + type: String, + required: true, + }, + }, + data() { + return { + isAppliedSuccessfully: false, + isApplying: false, + }; + }, + methods: { + applySuggestion() { + if (!this.canApply) return; + this.isApplying = true; + this.$emit('apply', this.applySuggestionCallback); + }, + applySuggestionCallback() { + this.isApplying = false; + }, + }, +}; +</script> + +<template> + <div class="md-suggestion-header border-bottom-0 mt-2"> + <div class="qa-suggestion-diff-header font-weight-bold"> + {{ __('Suggested change') }} + <a v-if="helpPagePath" :href="helpPagePath" :aria-label="__('Help')"> + <icon name="question-o" css-classes="link-highlight" /> + </a> + </div> + <span v-if="isApplied" class="badge badge-success">{{ __('Applied') }}</span> + <button + v-if="canApply" + type="button" + class="btn qa-apply-btn" + :disabled="isApplying" + @click="applySuggestion" + > + {{ __('Apply suggestion') }} + </button> + </div> +</template> diff --git a/app/assets/javascripts/vue_shared/components/markdown/suggestions.vue b/app/assets/javascripts/vue_shared/components/markdown/suggestions.vue new file mode 100644 index 00000000000..7c6dbee3e19 --- /dev/null +++ b/app/assets/javascripts/vue_shared/components/markdown/suggestions.vue @@ -0,0 +1,136 @@ +<script> +import Vue from 'vue'; +import SuggestionDiff from './suggestion_diff.vue'; +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, + default: '', + }, + suggestions: { + type: Array, + required: false, + default: () => [], + }, + noteHtml: { + type: String, + required: true, + }, + disabled: { + type: Boolean, + required: false, + default: false, + }, + helpPagePath: { + type: String, + required: true, + }, + }, + data() { + return { + isRendered: false, + }; + }, + watch: { + suggestions() { + this.reset(); + }, + noteHtml() { + this.reset(); + }, + }, + mounted() { + this.renderSuggestions(); + }, + methods: { + renderSuggestions() { + // swaps out suggestion(s) markdown with rich diff components + // (while still keeping non-suggestion markdown in place) + + if (!this.noteHtml) return; + const { container } = this.$refs; + const suggestionElements = container.querySelectorAll('.js-render-suggestion'); + + if (this.lineType === 'old') { + Flash('Unable to apply suggestions to a deleted line.', 'alert', this.$el); + } + + suggestionElements.forEach((suggestionEl, i) => { + const suggestionParentEl = suggestionEl.parentElement; + const newLines = this.extractNewLines(suggestionParentEl); + const diffComponent = this.generateDiff(newLines, i); + diffComponent.$mount(suggestionParentEl); + }); + + this.isRendered = true; + }, + extractNewLines(suggestionEl) { + // extracts the suggested lines from the markdown + // calculates a line number for each line + + const FIRST_CHAR_REGEX = /^(\+|-)/; + 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.replace(FIRST_CHAR_REGEX, '')}\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 + + 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 }, + }); + + suggestionDiff.$on('apply', ({ suggestionId, callback }) => { + this.$emit('apply', { suggestionId, callback, flashContainer: this.$el }); + }); + + return suggestionDiff; + }, + reset() { + // resets the container HTML (replaces it with the updated noteHTML) + // calls `renderSuggestions` once the updated noteHTML is added to the DOM + + this.$refs.container.innerHTML = this.noteHtml; + this.isRendered = false; + this.renderSuggestions(); + this.$nextTick(() => this.renderSuggestions()); + }, + }, +}; +</script> + +<template> + <div> + <div class="flash-container mt-3"></div> + <div v-show="isRendered" ref="container" v-html="noteHtml"></div> + </div> +</template> diff --git a/app/assets/javascripts/vue_shared/components/markdown/toolbar_button.vue b/app/assets/javascripts/vue_shared/components/markdown/toolbar_button.vue index a6d2cecdf7e..4572caa907b 100644 --- a/app/assets/javascripts/vue_shared/components/markdown/toolbar_button.vue +++ b/app/assets/javascripts/vue_shared/components/markdown/toolbar_button.vue @@ -37,6 +37,16 @@ export default { required: false, default: false, }, + tagContent: { + type: String, + required: false, + default: '', + }, + cursorOffset: { + type: Number, + required: false, + default: 0, + }, }, }; </script> @@ -45,8 +55,10 @@ export default { <button v-gl-tooltip :data-md-tag="tag" + :data-md-cursor-offset="cursorOffset" :data-md-select="tagSelect" :data-md-block="tagBlock" + :data-md-tag-content="tagContent" :data-md-prepend="prepend" :title="buttonTitle" :aria-label="buttonTitle" diff --git a/app/assets/stylesheets/framework/markdown_area.scss b/app/assets/stylesheets/framework/markdown_area.scss index 2b110e23fb8..5609a2086e6 100644 --- a/app/assets/stylesheets/framework/markdown_area.scss +++ b/app/assets/stylesheets/framework/markdown_area.scss @@ -277,6 +277,27 @@ } } +.md-suggestion-diff { + display: table !important; + border: 1px solid $border-color !important; +} + +.md-suggestion-header { + height: $suggestion-header-height; + display: flex; + align-items: center; + justify-content: space-between; + background-color: $gray-light; + border: 1px solid $border-color; + padding: $gl-padding; + border-radius: $border-radius-default $border-radius-default 0 0; + + svg { + vertical-align: middle; + margin-bottom: 3px; + } +} + @include media-breakpoint-down(xs) { .atwho-view-ul { width: 350px; diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss index a92481b3ebb..c0bba30944a 100644 --- a/app/assets/stylesheets/framework/variables.scss +++ b/app/assets/stylesheets/framework/variables.scss @@ -252,6 +252,7 @@ $browserScrollbarSize: 10px; * Misc */ $header-height: 40px; +$suggestion-header-height: 46px; $ide-statusbar-height: 25px; $fixed-layout-width: 1280px; $limited-layout-width: 990px; diff --git a/app/controllers/concerns/preview_markdown.rb b/app/controllers/concerns/preview_markdown.rb index c61b9fabe9e..4b0f0b8255c 100644 --- a/app/controllers/concerns/preview_markdown.rb +++ b/app/controllers/concerns/preview_markdown.rb @@ -12,7 +12,7 @@ module PreviewMarkdown when 'wikis' then { pipeline: :wiki, project_wiki: @project_wiki, page_slug: params[:id] } when 'snippets' then { skip_project_check: true } when 'groups' then { group: group } - when 'projects' then { issuable_state_filter_enabled: true } + when 'projects' then projects_filter_params else {} end @@ -22,9 +22,17 @@ module PreviewMarkdown body: view_context.markdown(result[:text], markdown_params), references: { users: result[:users], + suggestions: result[:suggestions], commands: view_context.markdown(result[:commands]) } } end + + def projects_filter_params + { + issuable_state_filter_enabled: true, + suggestions_filter_enabled: params[:preview_suggestions].present? + } + end # rubocop:enable Gitlab/ModuleWithInstanceVariables end diff --git a/app/models/concerns/noteable.rb b/app/models/concerns/noteable.rb index eb315058c3a..f2cad09e779 100644 --- a/app/models/concerns/noteable.rb +++ b/app/models/concerns/noteable.rb @@ -26,6 +26,10 @@ module Noteable DiscussionNote.noteable_types.include?(base_class_name) end + def supports_suggestion? + false + end + def discussions_rendered_on_frontend? false end diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb index c32008aa9c7..279603496b0 100644 --- a/app/models/diff_note.rb +++ b/app/models/diff_note.rb @@ -66,10 +66,23 @@ class DiffNote < Note self.original_position.diff_refs == diff_refs end + def supports_suggestion? + return false unless noteable.supports_suggestion? && on_text? + # We don't want to trigger side-effects of `diff_file` call. + return false unless file = fetch_diff_file + return false unless line = file.line_for_position(self.original_position) + + line&.suggestible? + end + def discussion_first_note? self == discussion.first_note end + def banzai_render_context(field) + super.merge(suggestions_filter_enabled: supports_suggestion?) + end + private def enqueue_diff_file_creation_job diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index a13cac73d04..8052a54c504 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -363,6 +363,11 @@ class MergeRequest < ActiveRecord::Base end end + def supports_suggestion? + # Should be `true` when removing the FF. + Suggestion.feature_enabled? + end + # Calls `MergeWorker` to proceed with the merge process and # updates `merge_jid` with the MergeWorker#jid. # This helps tracking enqueued and ongoing merge jobs. diff --git a/app/models/note.rb b/app/models/note.rb index 17c7d97fa0a..becf14e9785 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -69,6 +69,12 @@ class Note < ActiveRecord::Base belongs_to :last_edited_by, class_name: 'User' has_many :todos + + # The delete_all definition is required here in order + # to generate the correct DELETE sql for + # suggestions.delete_all calls + has_many :suggestions, -> { order(:relative_order) }, + inverse_of: :note, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent has_many :events, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_one :system_note_metadata has_one :note_diff_file, inverse_of: :diff_note, foreign_key: :diff_note_id @@ -110,7 +116,7 @@ class Note < ActiveRecord::Base scope :inc_author, -> { includes(:author) } scope :inc_relations_for_view, -> do includes(:project, { author: :status }, :updated_by, :resolved_by, :award_emoji, - :system_note_metadata, :note_diff_file) + :system_note_metadata, :note_diff_file, :suggestions) end scope :with_notes_filter, -> (notes_filter) do @@ -226,6 +232,10 @@ class Note < ActiveRecord::Base Gitlab::HookData::NoteBuilder.new(self).build end + def supports_suggestion? + false + end + def for_commit? noteable_type == "Commit" end diff --git a/app/models/suggestion.rb b/app/models/suggestion.rb new file mode 100644 index 00000000000..cec5ea30f9d --- /dev/null +++ b/app/models/suggestion.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +class Suggestion < ApplicationRecord + FEATURE_FLAG = :diff_suggestions + + belongs_to :note, inverse_of: :suggestions + validates :note, presence: true + validates :commit_id, presence: true, if: :applied? + + delegate :original_position, :position, :diff_file, + :noteable, to: :note + + def self.feature_enabled? + Feature.enabled?(FEATURE_FLAG) + end + + def project + noteable.source_project + end + + def branch + noteable.source_branch + end + + # For now, suggestions only serve as a way to send patches that + # will change a single line (being able to apply multiple in the same place), + # which explains `from_line` and `to_line` being the same line. + # We'll iterate on that in https://gitlab.com/gitlab-org/gitlab-ce/issues/53310 + # when allowing multi-line suggestions. + def from_line + position.new_line + end + alias_method :to_line, :from_line + + def from_original_line + original_position.new_line + end + alias_method :to_original_line, :from_original_line + + # `from_line_index` and `to_line_index` represents diff/blob line numbers in + # index-like way (N-1). + def from_line_index + from_line - 1 + end + alias_method :to_line_index, :from_line_index + + def appliable? + return false unless note.supports_suggestion? + + !applied? && + noteable.opened? && + different_content? && + note.active? + end + + private + + def different_content? + from_content != to_content + end +end diff --git a/app/policies/suggestion_policy.rb b/app/policies/suggestion_policy.rb new file mode 100644 index 00000000000..301b7d965f5 --- /dev/null +++ b/app/policies/suggestion_policy.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class SuggestionPolicy < BasePolicy + delegate { @subject.project } + + condition(:can_push_to_branch) do + Gitlab::UserAccess.new(@user, project: @subject.project).can_push_to_branch?(@subject.branch) + end + + rule { can_push_to_branch }.enable :apply_suggestion +end diff --git a/app/serializers/diff_line_entity.rb b/app/serializers/diff_line_entity.rb index 942714b7787..bfef6d3bde8 100644 --- a/app/serializers/diff_line_entity.rb +++ b/app/serializers/diff_line_entity.rb @@ -11,4 +11,6 @@ class DiffLineEntity < Grape::Entity expose :rich_text do |line| ERB::Util.html_escape(line.rich_text || line.text) end + + expose :suggestible?, as: :can_receive_suggestion end diff --git a/app/serializers/merge_request_widget_entity.rb b/app/serializers/merge_request_widget_entity.rb index f33a1654d5e..9731b52f1ad 100644 --- a/app/serializers/merge_request_widget_entity.rb +++ b/app/serializers/merge_request_widget_entity.rb @@ -238,6 +238,8 @@ class MergeRequestWidgetEntity < IssuableEntity end end + expose :supports_suggestion?, as: :can_receive_suggestion + private delegate :current_user, to: :request diff --git a/app/serializers/note_entity.rb b/app/serializers/note_entity.rb index c6d27817411..1d3b59eb1b7 100644 --- a/app/serializers/note_entity.rb +++ b/app/serializers/note_entity.rb @@ -36,6 +36,7 @@ class NoteEntity < API::Entities::Note end end + expose :suggestions, using: SuggestionEntity expose :resolved?, as: :resolved expose :resolvable?, as: :resolvable diff --git a/app/serializers/suggestion_entity.rb b/app/serializers/suggestion_entity.rb new file mode 100644 index 00000000000..4d0d4da10be --- /dev/null +++ b/app/serializers/suggestion_entity.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class SuggestionEntity < API::Entities::Suggestion + include RequestAwareEntity + + expose :current_user do + expose :can_apply do |suggestion| + Ability.allowed?(current_user, :apply_suggestion, suggestion) + end + end + + private + + def current_user + request.current_user + end +end diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb index e03789e3ca9..c4546f30235 100644 --- a/app/services/notes/create_service.rb +++ b/app/services/notes/create_service.rb @@ -36,6 +36,7 @@ module Notes if !only_commands && note.save todo_service.new_note(note, current_user) clear_noteable_diffs_cache(note) + Suggestions::CreateService.new(note).execute end if command_params.present? diff --git a/app/services/notes/update_service.rb b/app/services/notes/update_service.rb index 35db409eb27..d2052bed646 100644 --- a/app/services/notes/update_service.rb +++ b/app/services/notes/update_service.rb @@ -14,6 +14,17 @@ module Notes TodoService.new.update_note(note, current_user, old_mentioned_users) end + if note.supports_suggestion? + Suggestion.transaction do + note.suggestions.delete_all + Suggestions::CreateService.new(note).execute + end + + # We need to refresh the previous suggestions call cache + # in order to get the new records. + note.reload + end + note end end diff --git a/app/services/preview_markdown_service.rb b/app/services/preview_markdown_service.rb index de8757006f1..a449a5dc3e9 100644 --- a/app/services/preview_markdown_service.rb +++ b/app/services/preview_markdown_service.rb @@ -4,10 +4,12 @@ class PreviewMarkdownService < BaseService def execute text, commands = explain_quick_actions(params[:text]) users = find_user_references(text) + suggestions = find_suggestions(text) success( text: text, users: users, + suggestions: suggestions, commands: commands.join(' '), markdown_engine: markdown_engine ) @@ -28,6 +30,12 @@ class PreviewMarkdownService < BaseService extractor.users.map(&:username) end + def find_suggestions(text) + return [] unless params[:preview_suggestions] + + Banzai::SuggestionsParser.parse(text) + end + def find_commands_target QuickActions::TargetService .new(project, current_user) diff --git a/app/services/suggestions/apply_service.rb b/app/services/suggestions/apply_service.rb new file mode 100644 index 00000000000..d931d528c86 --- /dev/null +++ b/app/services/suggestions/apply_service.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +module Suggestions + class ApplyService < ::BaseService + def initialize(current_user) + @current_user = current_user + end + + def execute(suggestion) + unless suggestion.appliable? + return error('Suggestion is not appliable') + end + + params = file_update_params(suggestion) + result = ::Files::UpdateService.new(suggestion.project, @current_user, params).execute + + if result[:status] == :success + suggestion.update(commit_id: result[:result], applied: true) + end + + result + end + + private + + def file_update_params(suggestion) + diff_file = suggestion.diff_file + + file_path = diff_file.file_path + branch_name = suggestion.noteable.source_branch + file_content = new_file_content(suggestion) + commit_message = "Apply suggestion to #{file_path}" + + { + file_path: file_path, + branch_name: branch_name, + start_branch: branch_name, + commit_message: commit_message, + file_content: file_content + } + end + + def new_file_content(suggestion) + range = suggestion.from_line_index..suggestion.to_line_index + blob = suggestion.diff_file.new_blob + + blob.load_all_data! + content = blob.data.lines + content[range] = suggestion.to_content + + content.join + end + end +end diff --git a/app/services/suggestions/create_service.rb b/app/services/suggestions/create_service.rb new file mode 100644 index 00000000000..77e958cbe0c --- /dev/null +++ b/app/services/suggestions/create_service.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +module Suggestions + class CreateService + def initialize(note) + @note = note + end + + def execute + return unless @note.supports_suggestion? + + 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 = + suggestions.map.with_index do |suggestion, index| + from_content = changing_lines(comment_line, comment_line) + + # The parsed suggestion doesn't have information about the correct + # ending characters (we may have a line break, or not), so we take + # 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 + + rows.in_groups_of(100, false) do |rows| + Gitlab::Database.bulk_insert('suggestions', rows) + end + end + + private + + def changing_lines(from_line, to_line) + @note.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 diff --git a/app/views/projects/merge_requests/show.html.haml b/app/views/projects/merge_requests/show.html.haml index c178206dda4..3f2e59d05e3 100644 --- a/app/views/projects/merge_requests/show.html.haml +++ b/app/views/projects/merge_requests/show.html.haml @@ -67,6 +67,7 @@ noteable_data: serialize_issuable(@merge_request), noteable_type: 'MergeRequest', target_type: 'merge_request', + help_page_path: nil, current_user_data: UserSerializer.new(project: @project).represent(current_user, {}, MergeRequestUserEntity).to_json} } #commits.commits.tab-pane @@ -76,6 +77,7 @@ = render 'projects/commit/pipelines_list', disable_initialization: true, endpoint: pipelines_project_merge_request_path(@project, @merge_request) #js-diffs-app.diffs.tab-pane{ data: { "is-locked" => @merge_request.discussion_locked?, endpoint: diffs_project_merge_request_path(@project, @merge_request, 'json', request.query_parameters), + help_page_path: nil, current_user_data: UserSerializer.new(project: @project).represent(current_user, {}, MergeRequestUserEntity).to_json, project_path: project_path(@merge_request.project), changes_empty_state_illustration: image_path('illustrations/merge_request_changes_empty.svg') } } diff --git a/changelogs/unreleased/suggest-change-to-diff-line.yml b/changelogs/unreleased/suggest-change-to-diff-line.yml new file mode 100644 index 00000000000..cb949f14e8c --- /dev/null +++ b/changelogs/unreleased/suggest-change-to-diff-line.yml @@ -0,0 +1,5 @@ +--- +title: Add ability to render suggestions +merge_request: 23147 +author: +type: added diff --git a/db/migrate/20181123144235_create_suggestions.rb b/db/migrate/20181123144235_create_suggestions.rb new file mode 100644 index 00000000000..bcc4d8c538b --- /dev/null +++ b/db/migrate/20181123144235_create_suggestions.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +class CreateSuggestions < ActiveRecord::Migration + DOWNTIME = false + + def change + create_table :suggestions, id: :bigserial do |t| + t.references :note, foreign_key: { on_delete: :cascade }, null: false + t.integer :relative_order, null: false, limit: 2 + t.boolean :applied, null: false, default: false + t.string :commit_id + t.text :from_content, null: false + t.text :to_content, null: false + + t.index [:note_id, :relative_order], + name: 'index_suggestions_on_note_id_and_relative_order', + unique: true + end + end +end diff --git a/db/schema.rb b/db/schema.rb index e5e19eb7745..10b7aa8a99f 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1956,6 +1956,16 @@ ActiveRecord::Schema.define(version: 20181204154019) do t.index ["subscribable_id", "subscribable_type", "user_id", "project_id"], name: "index_subscriptions_on_subscribable_and_user_id_and_project_id", unique: true, using: :btree end + create_table "suggestions", id: :bigserial, force: :cascade do |t| + t.integer "note_id", null: false + t.integer "relative_order", limit: 2, null: false + t.boolean "applied", default: false, null: false + t.string "commit_id" + t.text "from_content", null: false + t.text "to_content", null: false + t.index ["note_id", "relative_order"], name: "index_suggestions_on_note_id_and_relative_order", unique: true, using: :btree + end + create_table "system_note_metadata", force: :cascade do |t| t.integer "note_id", null: false t.integer "commit_count" @@ -2432,6 +2442,7 @@ ActiveRecord::Schema.define(version: 20181204154019) do add_foreign_key "services", "projects", name: "fk_71cce407f9", on_delete: :cascade add_foreign_key "snippets", "projects", name: "fk_be41fd4bb7", on_delete: :cascade add_foreign_key "subscriptions", "projects", on_delete: :cascade + add_foreign_key "suggestions", "notes", on_delete: :cascade add_foreign_key "system_note_metadata", "notes", name: "fk_d83a918cb1", on_delete: :cascade add_foreign_key "term_agreements", "application_setting_terms", column: "term_id" add_foreign_key "term_agreements", "users", on_delete: :cascade diff --git a/lib/api/api.rb b/lib/api/api.rb index 8abb24e6f69..f1448da7403 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -149,6 +149,7 @@ module API mount ::API::Snippets mount ::API::Submodules mount ::API::Subscriptions + mount ::API::Suggestions mount ::API::SystemHooks mount ::API::Tags mount ::API::Templates diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 5dbfbb85e9e..b83a5c14190 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -1495,5 +1495,17 @@ module API expose :label, using: Entities::LabelBasic expose :action end + + class Suggestion < Grape::Entity + expose :id + expose :from_original_line + expose :to_original_line + expose :from_line + expose :to_line + expose :appliable?, as: :appliable + expose :applied + expose :from_content + expose :to_content + end end end diff --git a/lib/api/suggestions.rb b/lib/api/suggestions.rb new file mode 100644 index 00000000000..d008d1b9e97 --- /dev/null +++ b/lib/api/suggestions.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +module API + class Suggestions < Grape::API + before { authenticate! } + + resource :suggestions do + desc 'Apply suggestion patch in the Merge Request it was created' do + success Entities::Suggestion + end + params do + requires :id, type: String, desc: 'The suggestion ID' + end + put ':id/apply' do + suggestion = Suggestion.find_by_id(params[:id]) + + not_found! unless suggestion + authorize! :apply_suggestion, suggestion + + result = ::Suggestions::ApplyService.new(current_user).execute(suggestion) + + if result[:status] == :success + present suggestion, with: Entities::Suggestion, current_user: current_user + else + http_status = result[:http_status] || 400 + render_api_error!(result[:message], http_status) + end + end + end + end +end diff --git a/lib/banzai/filter/suggestion_filter.rb b/lib/banzai/filter/suggestion_filter.rb new file mode 100644 index 00000000000..822db7cf26e --- /dev/null +++ b/lib/banzai/filter/suggestion_filter.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module Banzai + module Filter + class SuggestionFilter < HTML::Pipeline::Filter + # Class used for tagging elements that should be rendered + TAG_CLASS = 'js-render-suggestion'.freeze + + def call + return doc unless Suggestion.feature_enabled? + return doc unless suggestions_filter_enabled? + + doc.search('pre.suggestion > code').each do |node| + node.add_class(TAG_CLASS) + end + + doc + end + + def suggestions_filter_enabled? + context[:suggestions_filter_enabled] + end + end + end +end diff --git a/lib/banzai/filter/syntax_highlight_filter.rb b/lib/banzai/filter/syntax_highlight_filter.rb index 8a7f9045c24..18e5e9185de 100644 --- a/lib/banzai/filter/syntax_highlight_filter.rb +++ b/lib/banzai/filter/syntax_highlight_filter.rb @@ -69,7 +69,7 @@ module Banzai end def use_rouge?(language) - %w(math mermaid plantuml).exclude?(language) + %w(math mermaid plantuml suggestion).exclude?(language) end end end diff --git a/lib/banzai/pipeline/gfm_pipeline.rb b/lib/banzai/pipeline/gfm_pipeline.rb index 96bea7ca935..5f13a6d6cde 100644 --- a/lib/banzai/pipeline/gfm_pipeline.rb +++ b/lib/banzai/pipeline/gfm_pipeline.rb @@ -29,6 +29,7 @@ module Banzai Filter::TableOfContentsFilter, Filter::AutolinkFilter, Filter::ExternalLinkFilter, + Filter::SuggestionFilter, *reference_filters, diff --git a/lib/banzai/pipeline/post_process_pipeline.rb b/lib/banzai/pipeline/post_process_pipeline.rb index 63a998a2c1f..7eaad6d7560 100644 --- a/lib/banzai/pipeline/post_process_pipeline.rb +++ b/lib/banzai/pipeline/post_process_pipeline.rb @@ -14,7 +14,8 @@ module Banzai [ Filter::RedactorFilter, Filter::RelativeLinkFilter, - Filter::IssuableStateFilter + Filter::IssuableStateFilter, + Filter::SuggestionFilter ] end diff --git a/lib/banzai/suggestions_parser.rb b/lib/banzai/suggestions_parser.rb new file mode 100644 index 00000000000..09f36635020 --- /dev/null +++ b/lib/banzai/suggestions_parser.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +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/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index 8ba44dff06f..a85c5386d98 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -122,6 +122,16 @@ module Gitlab old_blob_lazy&.itself end + def new_blob_lines_between(from_line, to_line) + return [] unless new_blob + + from_index = from_line - 1 + to_index = to_line - 1 + + new_blob.load_all_data! + new_blob.data.lines[from_index..to_index] + end + def content_sha new_content_sha || old_content_sha end diff --git a/lib/gitlab/diff/line.rb b/lib/gitlab/diff/line.rb index f0c4977fc50..001748afb41 100644 --- a/lib/gitlab/diff/line.rb +++ b/lib/gitlab/diff/line.rb @@ -73,6 +73,10 @@ module Gitlab !meta? end + def suggestible? + !removed? + end + def rich_text @parent_file.try(:highlight_lines!) if @parent_file && !@rich_text diff --git a/lib/gitlab/usage_data.rb b/lib/gitlab/usage_data.rb index 008e9cd1d24..083c620267a 100644 --- a/lib/gitlab/usage_data.rb +++ b/lib/gitlab/usage_data.rb @@ -85,6 +85,8 @@ module Gitlab releases: count(Release), remote_mirrors: count(RemoteMirror), snippets: count(Snippet), + suggestions: count(Suggestion), + todos: count(Todo), uploads: count(Upload), web_hooks: count(WebHook) }.merge(services_usage).merge(approximate_counts) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index abd1ce4a13a..b85eb6bdc88 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -657,6 +657,12 @@ msgstr "" msgid "Applications" msgstr "" +msgid "Applied" +msgstr "" + +msgid "Apply suggestion" +msgstr "" + msgid "Apr" msgstr "" @@ -3597,6 +3603,9 @@ msgstr "" msgid "Input your repository URL" msgstr "" +msgid "Insert suggestion" +msgstr "" + msgid "Install GitLab Runner" msgstr "" @@ -6080,6 +6089,9 @@ msgstr "" msgid "Something went wrong when toggling the button" msgstr "" +msgid "Something went wrong while applying the suggestion. Please try again." +msgstr "" + msgid "Something went wrong while closing the %{issuable}. Please try again later" msgstr "" @@ -6347,6 +6359,9 @@ msgstr "" msgid "Subscribed" msgstr "" +msgid "Suggested change" +msgstr "" + msgid "Switch branch/tag" msgstr "" diff --git a/spec/db/schema_spec.rb b/spec/db/schema_spec.rb index e8584846b56..7c505ee0d43 100644 --- a/spec/db/schema_spec.rb +++ b/spec/db/schema_spec.rb @@ -54,7 +54,8 @@ describe 'Database schema' do user_agent_details: %w[subject_id], users: %w[color_scheme_id created_by_id theme_id], users_star_projects: %w[user_id], - web_hooks: %w[service_id] + web_hooks: %w[service_id], + suggestions: %w[commit_id] }.with_indifferent_access.freeze context 'for table' do diff --git a/spec/factories/suggestions.rb b/spec/factories/suggestions.rb new file mode 100644 index 00000000000..307523cc061 --- /dev/null +++ b/spec/factories/suggestions.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :suggestion do + relative_order 0 + association :note, factory: :diff_note_on_merge_request + from_content " vars = {\n" + to_content " vars = [\n" + + trait :unappliable do + from_content "foo" + to_content "foo" + end + + trait :applied do + applied true + commit_id { RepoHelpers.sample_commit.id } + end + end +end 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 new file mode 100644 index 00000000000..c19e299097e --- /dev/null +++ b/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb @@ -0,0 +1,85 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe 'User comments on a diff', :js do + include MergeRequestDiffHelpers + include RepoHelpers + + let(:project) { create(:project, :repository) } + let(:merge_request) do + create(:merge_request_with_diffs, source_project: project, target_project: project, source_branch: 'merge-test') + end + let(:user) { create(:user) } + + before do + project.add_maintainer(user) + sign_in(user) + + visit(diffs_project_merge_request_path(project, merge_request)) + end + + context 'single suggestion note' 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\n# change to a comment\n```") + click_button('Comment') + end + + wait_for_requests + + 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 + 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\n# change to a comment\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 + + context 'multiple suggestions in a single note' do + it 'suggestions are 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\n# change to a comment\n```\n```suggestion\n# or that\n```") + click_button('Comment') + end + + wait_for_requests + + page.within('.diff-discussions') 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') + + expect(suggestion_2).to have_content(' url = https://github.com/gitlabhq/gitlab-shell.git') + expect(suggestion_2).to have_content('# or that') + end + end + end +end diff --git a/spec/fixtures/api/schemas/entities/diff_line.json b/spec/fixtures/api/schemas/entities/diff_line.json index 66e8b443e1b..9657004cd2d 100644 --- a/spec/fixtures/api/schemas/entities/diff_line.json +++ b/spec/fixtures/api/schemas/entities/diff_line.json @@ -8,7 +8,8 @@ "new_line": { "type": ["integer", "null"] }, "text": { "type": ["string"] }, "rich_text": { "type": ["string"] }, - "meta_data": { "type": ["object", "null"] } + "meta_data": { "type": ["object", "null"] }, + "can_receive_suggestion": { "type": "boolean" } }, "additionalProperties": false } diff --git a/spec/fixtures/api/schemas/entities/merge_request_widget.json b/spec/fixtures/api/schemas/entities/merge_request_widget.json index 35971d564d5..193ab6821a5 100644 --- a/spec/fixtures/api/schemas/entities/merge_request_widget.json +++ b/spec/fixtures/api/schemas/entities/merge_request_widget.json @@ -119,7 +119,8 @@ "can_push_to_source_branch": { "type": "boolean" }, "rebase_path": { "type": ["string", "null"] }, "squash": { "type": "boolean" }, - "test_reports_path": { "type": ["string", "null"] } + "test_reports_path": { "type": ["string", "null"] }, + "can_receive_suggestion": { "type": "boolean" } }, "additionalProperties": false } diff --git a/spec/javascripts/diffs/components/diff_content_spec.js b/spec/javascripts/diffs/components/diff_content_spec.js index c25f6167163..d688560a260 100644 --- a/spec/javascripts/diffs/components/diff_content_spec.js +++ b/spec/javascripts/diffs/components/diff_content_spec.js @@ -17,6 +17,7 @@ describe('DiffContent', () => { current_user: { can_create_note: false, }, + preview_note_path: 'path/to/preview', }; vm = mountComponentWithStore(Component, { diff --git a/spec/javascripts/diffs/mock_data/diff_discussions.js b/spec/javascripts/diffs/mock_data/diff_discussions.js index 44313caba29..c1e9f791925 100644 --- a/spec/javascripts/diffs/mock_data/diff_discussions.js +++ b/spec/javascripts/diffs/mock_data/diff_discussions.js @@ -487,8 +487,19 @@ export default { ], }, diff_discussion: true, - truncated_diff_lines: - '<tr class="line_holder new" id="">\n<td class="diff-line-num new old_line" data-linenumber="1">\n \n</td>\n<td class="diff-line-num new new_line" data-linenumber="1">\n1\n</td>\n<td class="line_content new noteable_line"><span id="LC1" class="line" lang="plaintext"> - Bad dates</span>\n</td>\n</tr>\n<tr class="line_holder new" id="">\n<td class="diff-line-num new old_line" data-linenumber="1">\n \n</td>\n<td class="diff-line-num new new_line" data-linenumber="2">\n2\n</td>\n<td class="line_content new noteable_line"><span id="LC2" class="line" lang="plaintext"></span>\n</td>\n</tr>\n', + truncated_diff_lines: [ + { + text: 'line', + rich_text: + '<tr class="line_holder new" id="">\n<td class="diff-line-num new old_line" data-linenumber="1">\n \n</td>\n<td class="diff-line-num new new_line" data-linenumber="1">\n1\n</td>\n<td class="line_content new noteable_line"><span id="LC1" class="line" lang="plaintext"> - Bad dates</span>\n</td>\n</tr>\n<tr class="line_holder new" id="">\n<td class="diff-line-num new old_line" data-linenumber="1">\n \n</td>\n<td class="diff-line-num new new_line" data-linenumber="2">\n2\n</td>\n<td class="line_content new noteable_line"><span id="LC2" class="line" lang="plaintext"></span>\n</td>\n</tr>\n', + can_receive_suggestion: true, + line_code: '6f209374f7e565f771b95720abf46024c41d1885_1_1', + type: 'new', + old_line: null, + new_line: 1, + meta_data: null, + }, + ], }; export const imageDiffDiscussions = [ diff --git a/spec/javascripts/vue_shared/components/markdown/header_spec.js b/spec/javascripts/vue_shared/components/markdown/header_spec.js index 59613faa49f..e733a95288e 100644 --- a/spec/javascripts/vue_shared/components/markdown/header_spec.js +++ b/spec/javascripts/vue_shared/components/markdown/header_spec.js @@ -28,6 +28,7 @@ describe('Markdown field header component', () => { 'Add a numbered list', 'Add a task list', 'Add a table', + 'Insert suggestion', 'Go full screen', ]; const elements = vm.$el.querySelectorAll('.toolbar-btn'); @@ -93,4 +94,18 @@ describe('Markdown field header component', () => { '| header | header |\n| ------ | ------ |\n| cell | cell |\n| cell | cell |', ); }); + + it('renders suggestion template', () => { + vm.lineContent = 'Some content'; + + expect(vm.mdSuggestion).toEqual('```suggestion\n{text}\n```'); + }); + + it('does not render suggestion button if `canSuggest` is set to false', () => { + vm.canSuggest = false; + + Vue.nextTick(() => { + expect(vm.$el.querySelector('.qa-suggestion-btn')).toBe(null); + }); + }); }); diff --git a/spec/javascripts/vue_shared/components/markdown/suggestion_diff_header_spec.js b/spec/javascripts/vue_shared/components/markdown/suggestion_diff_header_spec.js new file mode 100644 index 00000000000..8187b3204b1 --- /dev/null +++ b/spec/javascripts/vue_shared/components/markdown/suggestion_diff_header_spec.js @@ -0,0 +1,69 @@ +import Vue from 'vue'; +import SuggestionDiffHeaderComponent from '~/vue_shared/components/markdown/suggestion_diff_header.vue'; + +const MOCK_DATA = { + canApply: true, + isApplied: false, + helpPagePath: 'path_to_docs', +}; + +describe('Suggestion Diff component', () => { + let vm; + + function createComponent(propsData) { + const Component = Vue.extend(SuggestionDiffHeaderComponent); + + return new Component({ + propsData, + }).$mount(); + } + + beforeEach(done => { + vm = createComponent(MOCK_DATA); + Vue.nextTick(done); + }); + + describe('init', () => { + it('renders a suggestion header', () => { + const header = vm.$el.querySelector('.qa-suggestion-diff-header'); + + expect(header).not.toBeNull(); + expect(header.innerHTML.includes('Suggested change')).toBe(true); + }); + + it('renders an apply button', () => { + const applyBtn = vm.$el.querySelector('.qa-apply-btn'); + + expect(applyBtn).not.toBeNull(); + expect(applyBtn.innerHTML.includes('Apply suggestion')).toBe(true); + }); + + it('does not render an apply button if `canApply` is set to false', () => { + const props = Object.assign(MOCK_DATA, { canApply: false }); + + vm = createComponent(props); + + expect(vm.$el.querySelector('.qa-apply-btn')).toBeNull(); + }); + }); + + describe('applySuggestion', () => { + it('emits when the apply button is clicked', () => { + const props = Object.assign(MOCK_DATA, { canApply: true }); + + vm = createComponent(props); + spyOn(vm, '$emit'); + vm.applySuggestion(); + + expect(vm.$emit).toHaveBeenCalled(); + }); + + it('does not emit when the canApply is set to false', () => { + spyOn(vm, '$emit'); + vm.canApply = false; + vm.applySuggestion(); + + expect(vm.$emit).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/spec/javascripts/vue_shared/components/markdown/suggestion_diff_spec.js b/spec/javascripts/vue_shared/components/markdown/suggestion_diff_spec.js new file mode 100644 index 00000000000..d4ed8f2f7a4 --- /dev/null +++ b/spec/javascripts/vue_shared/components/markdown/suggestion_diff_spec.js @@ -0,0 +1,79 @@ +import Vue from 'vue'; +import SuggestionDiffComponent from '~/vue_shared/components/markdown/suggestion_diff.vue'; + +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, + }, + helpPagePath: 'path_to_docs', +}; + +describe('Suggestion Diff component', () => { + let vm; + + beforeEach(done => { + const Component = Vue.extend(SuggestionDiffComponent); + + vm = new Component({ + propsData: MOCK_DATA, + }).$mount(); + + Vue.nextTick(done); + }); + + describe('init', () => { + it('renders a suggestion header', () => { + expect(vm.$el.querySelector('.qa-suggestion-diff-header')).not.toBeNull(); + }); + + it('renders a diff table', () => { + expect(vm.$el.querySelector('table.md-suggestion-diff')).not.toBeNull(); + }); + + it('renders the oldLineNumber', () => { + const fromLine = vm.$el.querySelector('.qa-old-diff-line-number').innerHTML; + + expect(parseInt(fromLine, 10)).toBe(vm.fromLine); + }); + + 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); + }); + }); + + it('renders a line number for each line', () => { + const newLineNumbers = vm.$el.querySelectorAll('.qa-new-diff-line-number'); + + newLineNumbers.forEach((line, i) => { + expect(newLineNumbers[i].innerHTML.includes(vm.newLines[i].lineNumber)).toBe(true); + }); + }); + }); + + describe('applySuggestion', () => { + it('emits apply event when applySuggestion is called', () => { + const callback = () => {}; + spyOn(vm, '$emit'); + vm.applySuggestion(callback); + + expect(vm.$emit).toHaveBeenCalledWith('apply', { suggestionId: vm.suggestion.id, callback }); + }); + }); +}); diff --git a/spec/javascripts/vue_shared/components/markdown/suggestions_spec.js b/spec/javascripts/vue_shared/components/markdown/suggestions_spec.js new file mode 100644 index 00000000000..ab1b747c360 --- /dev/null +++ b/spec/javascripts/vue_shared/components/markdown/suggestions_spec.js @@ -0,0 +1,125 @@ +import Vue from 'vue'; +import SuggestionsComponent from '~/vue_shared/components/markdown/suggestions.vue'; + +const MOCK_DATA = { + fromLine: 1, + fromContent: 'Old content', + suggestions: [], + noteHtml: ` + <div class="suggestion"> + <div class="line">Suggestion 1</div> + </div> + + <div class="suggestion"> + <div class="line">Suggestion 2</div> + </div> + `, + 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 => { + const Component = Vue.extend(SuggestionsComponent); + + vm = new Component({ + propsData: MOCK_DATA, + }).$mount(); + + extractedLines = vm.extractNewLines(generateMockLines()); + diffTable = vm.generateDiff(extractedLines).$mount().$el; + + spyOn(vm, 'renderSuggestions'); + vm.renderSuggestions(); + Vue.nextTick(done); + }); + + describe('mounted', () => { + it('renders a flash container', () => { + expect(vm.$el.querySelector('.flash-container')).not.toBeNull(); + }); + + it('renders a container for suggestions', () => { + expect(vm.$refs.container).not.toBeNull(); + }); + + 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([]); + }); + }); + + describe('generateDiff', () => { + it('generates a diff table', () => { + expect(diffTable.querySelector('.md-suggestion-diff')).not.toBeNull(); + }); + + it('generates a diff table that contains contents of `oldLineContent`', () => { + expect(diffTable.innerHTML.includes(vm.fromContent)).toBe(true); + }); + + it('generates a diff table that contains contents the suggested lines', () => { + extractedLines.forEach((line, i) => { + expect(diffTable.innerHTML.includes(extractedLines[i].content)).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'); + + expect([...lines][0].innerHTML).toBe('1'); + expect([...lines][1].innerHTML).toBe('2'); + expect([...lines][2].innerHTML).toBe('3'); + }); + }); +}); diff --git a/spec/lib/banzai/filter/suggestion_filter_spec.rb b/spec/lib/banzai/filter/suggestion_filter_spec.rb new file mode 100644 index 00000000000..55a141bf315 --- /dev/null +++ b/spec/lib/banzai/filter/suggestion_filter_spec.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Banzai::Filter::SuggestionFilter do + include FilterSpecHelper + + let(:input) { "<pre class='code highlight js-syntax-highlight suggestion'><code>foo\n</code></pre>" } + let(:default_context) do + { suggestions_filter_enabled: true } + end + + it 'includes `js-render-suggestion` class' do + doc = filter(input, default_context) + result = doc.css('code').first + + expect(result[:class]).to include('js-render-suggestion') + end + + it 'includes no `js-render-suggestion` when feature disabled' do + stub_feature_flags(diff_suggestions: false) + + doc = filter(input, default_context) + result = doc.css('code').first + + expect(result[:class]).to be_nil + end + + it 'includes no `js-render-suggestion` when filter is disabled' do + doc = filter(input) + result = doc.css('code').first + + expect(result[:class]).to be_nil + end +end diff --git a/spec/lib/banzai/suggestions_parser_spec.rb b/spec/lib/banzai/suggestions_parser_spec.rb new file mode 100644 index 00000000000..79658d710ce --- /dev/null +++ b/spec/lib/banzai/suggestions_parser_spec.rb @@ -0,0 +1,32 @@ +# 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/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index bae5b21c26f..72abd8973cb 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -37,6 +37,7 @@ notes: - events - system_note_metadata - note_diff_file +- suggestions label_links: - target - label diff --git a/spec/lib/gitlab/usage_data_spec.rb b/spec/lib/gitlab/usage_data_spec.rb index deb19fe1a4b..2a09f581f68 100644 --- a/spec/lib/gitlab/usage_data_spec.rb +++ b/spec/lib/gitlab/usage_data_spec.rb @@ -117,6 +117,7 @@ describe Gitlab::UsageData do releases remote_mirrors snippets + suggestions todos uploads web_hooks diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb index 8624f0daa4d..40ce8ab736a 100644 --- a/spec/models/diff_note_spec.rb +++ b/spec/models/diff_note_spec.rb @@ -318,6 +318,24 @@ describe DiffNote do end end + describe '#supports_suggestion?' do + context 'when noteable does not support suggestions' do + it 'returns false' do + allow(subject.noteable).to receive(:supports_suggestion?) { false } + + expect(subject.supports_suggestion?).to be(false) + end + end + + context 'when line is not suggestible' do + it 'returns false' do + allow_any_instance_of(Gitlab::Diff::Line).to receive(:suggestible?) { false } + + expect(subject.supports_suggestion?).to be(false) + end + end + end + describe "image diff notes" do let(:path) { "files/images/any_image.png" } diff --git a/spec/models/suggestion_spec.rb b/spec/models/suggestion_spec.rb new file mode 100644 index 00000000000..cafc725dddb --- /dev/null +++ b/spec/models/suggestion_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Suggestion do + let(:suggestion) { create(:suggestion) } + + describe 'associations' do + it { is_expected.to belong_to(:note) } + end + + describe 'validations' do + it { is_expected.to validate_presence_of(:note) } + + context 'when suggestion is applied' do + before do + allow(subject).to receive(:applied?).and_return(true) + end + + it { is_expected.to validate_presence_of(:commit_id) } + end + end + + describe '#appliable?' do + context 'when note does not support suggestions' do + it 'returns false' do + expect_next_instance_of(DiffNote) do |note| + allow(note).to receive(:supports_suggestion?) { false } + end + + expect(suggestion).not_to be_appliable + end + end + + context 'when patch is already applied' do + let(:suggestion) { create(:suggestion, :applied) } + + it 'returns false' do + expect(suggestion).not_to be_appliable + end + end + + context 'when merge request is not opened' do + let(:merge_request) { create(:merge_request, :merged) } + let(:note) do + create(:diff_note_on_merge_request, project: merge_request.project, + noteable: merge_request) + end + + let(:suggestion) { create(:suggestion, note: note) } + + it 'returns false' do + expect(suggestion).not_to be_appliable + end + end + end +end diff --git a/spec/requests/api/suggestions_spec.rb b/spec/requests/api/suggestions_spec.rb new file mode 100644 index 00000000000..8e9f737fbd5 --- /dev/null +++ b/spec/requests/api/suggestions_spec.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe API::Suggestions do + let(:project) { create(:project, :repository) } + let(:user) { create(:user) } + + let(:merge_request) do + create(:merge_request, source_project: project, + target_project: project) + end + + let(:position) do + Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb", + new_path: "files/ruby/popen.rb", + old_line: nil, + new_line: 9, + 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 + + describe "PUT /suggestions/:id/apply" do + let(:url) { "/suggestions/#{suggestion.id}/apply" } + + context 'when successfully applies patch' do + let(:suggestion) do + create(:suggestion, note: diff_note, + from_content: " raise RuntimeError, \"System commands must be given as an array of strings\"\n", + to_content: " raise RuntimeError, 'Explosion'\n # explosion?") + end + + it 'returns 200 with json content' do + project.add_maintainer(user) + + put api(url, user), id: suggestion.id + + expect(response).to have_gitlab_http_status(200) + expect(json_response) + .to include('id', 'from_original_line', 'to_original_line', + 'from_line', 'to_line', 'appliable', 'applied', + 'from_content', 'to_content') + end + end + + context 'when not able to apply patch' do + let(:suggestion) do + create(:suggestion, :unappliable, note: diff_note) + end + + it 'returns 400 with json content' do + project.add_maintainer(user) + + put api(url, user), id: suggestion.id + + expect(response).to have_gitlab_http_status(400) + expect(json_response).to eq({ 'message' => 'Suggestion is not appliable' }) + end + end + + context 'when unauthorized' do + let(:suggestion) do + create(:suggestion, note: diff_note, + from_content: " raise RuntimeError, \"System commands must be given as an array of strings\"\n", + to_content: " raise RuntimeError, 'Explosion'\n # explosion?") + end + + it 'returns 403 with json content' do + project.add_reporter(user) + + put api(url, user), id: suggestion.id + + expect(response).to have_gitlab_http_status(403) + expect(json_response).to eq({ 'message' => '403 Forbidden' }) + end + end + end +end diff --git a/spec/serializers/suggestion_entity_spec.rb b/spec/serializers/suggestion_entity_spec.rb new file mode 100644 index 00000000000..047571f161c --- /dev/null +++ b/spec/serializers/suggestion_entity_spec.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe SuggestionEntity do + include RepoHelpers + + let(:user) { create(:user) } + let(:request) { double('request', current_user: user) } + let(:suggestion) { create(:suggestion) } + let(:entity) { described_class.new(suggestion, request: request) } + + subject { entity.as_json } + + it 'exposes correct attributes' do + expect(subject).to include(:id, :from_original_line, :to_original_line, :from_line, + :to_line, :appliable, :applied, :from_content, :to_content) + end + + it 'exposes current user abilities' do + expect(subject[:current_user]).to include(:can_apply) + end +end diff --git a/spec/services/notes/update_service_spec.rb b/spec/services/notes/update_service_spec.rb index 533dcdcd6cd..fd9bff46a06 100644 --- a/spec/services/notes/update_service_spec.rb +++ b/spec/services/notes/update_service_spec.rb @@ -20,6 +20,29 @@ describe Notes::UpdateService do @note.reload end + context 'suggestions' do + it 'refreshes note suggestions' do + markdown = <<-MARKDOWN.strip_heredoc + ```suggestion + foo + ``` + + ```suggestion + bar + ``` + MARKDOWN + + suggestion = create(:suggestion) + note = suggestion.note + + expect { described_class.new(project, user, note: markdown).execute(note) } + .to change { note.suggestions.count }.from(1).to(2) + + expect(note.suggestions.order(:relative_order).map(&:to_content)) + .to eq([" foo\n", " bar\n"]) + end + end + context 'todos' do let!(:todo) { create(:todo, :assigned, user: user, project: project, target: issue, author: user2) } diff --git a/spec/services/preview_markdown_service_spec.rb b/spec/services/preview_markdown_service_spec.rb index b69977c812a..458cb8f1f31 100644 --- a/spec/services/preview_markdown_service_spec.rb +++ b/spec/services/preview_markdown_service_spec.rb @@ -19,6 +19,31 @@ describe PreviewMarkdownService do end end + describe 'suggestions' do + let(:params) { { text: "```suggestion\nfoo\n```", preview_suggestions: preview_suggestions } } + let(:service) { described_class.new(project, user, params) } + + context 'when preview markdown param is present' do + let(:preview_suggestions) { true } + + it 'returns users referenced in text' do + result = service.execute + + expect(result[:suggestions]).to eq(['foo']) + end + end + + context 'when preview markdown param is not present' do + let(:preview_suggestions) { false } + + it 'returns users referenced in text' do + result = service.execute + + expect(result[:suggestions]).to eq([]) + end + end + end + context 'new note with quick actions' do let(:issue) { create(:issue, project: project) } let(:params) do diff --git a/spec/services/suggestions/apply_service_spec.rb b/spec/services/suggestions/apply_service_spec.rb new file mode 100644 index 00000000000..3a483717756 --- /dev/null +++ b/spec/services/suggestions/apply_service_spec.rb @@ -0,0 +1,229 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Suggestions::ApplyService do + include ProjectForksHelper + + let(:project) { create(:project, :repository) } + let(:user) { create(:user, :commit_email) } + + 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(:suggestion) do + create(:suggestion, note: diff_note, + from_content: " raise RuntimeError, \"System commands must be given as an array of strings\"\n", + to_content: " raise RuntimeError, 'Explosion'\n # explosion?\n") + end + + subject { described_class.new(user) } + + context 'patch is appliable' do + let(:expected_content) do + <<-CONTENT.strip_heredoc + require 'fileutils' + require 'open3' + + module Popen + extend self + + def popen(cmd, path=nil) + unless cmd.is_a?(Array) + raise RuntimeError, 'Explosion' + # explosion? + end + + path ||= Dir.pwd + + 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 + + context 'non-fork project' do + let(:merge_request) do + create(:merge_request, source_project: project, + 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 + + it 'updates the file with the new contents' do + subject.execute(suggestion) + + blob = project.repository.blob_at_branch(merge_request.source_branch, + position.new_path) + + expect(blob.data).to eq(expected_content) + end + + it 'returns success status' do + result = subject.execute(suggestion) + + expect(result[:status]).to eq(:success) + end + + it 'updates suggestion applied and commit_id columns' do + expect { subject.execute(suggestion) } + .to change(suggestion, :applied) + .from(false).to(true) + .and change(suggestion, :commit_id) + .from(nil) + end + + it 'created commit has users email and name' do + subject.execute(suggestion) + + commit = project.repository.commit + + expect(user.commit_email).not_to eq(user.email) + expect(commit.author_email).to eq(user.commit_email) + expect(commit.committer_email).to eq(user.commit_email) + expect(commit.author_name).to eq(user.name) + end + end + + context 'fork-project' do + let(:project) { create(:project, :public, :repository) } + + let(:forked_project) do + fork_project_with_submodules(project, user) + end + + let(:merge_request) do + create(:merge_request, + source_branch: 'conflict-resolvable-fork', source_project: forked_project, + target_branch: 'conflict-start', 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 + + it 'updates file in the source project' do + expect(Files::UpdateService).to receive(:new) + .with(merge_request.source_project, user, anything) + .and_call_original + + subject.execute(suggestion) + end + end + end + + context 'no permission' do + let(:merge_request) do + create(:merge_request, source_project: project, + target_project: project) + end + + let(:diff_note) do + create(:diff_note_on_merge_request, noteable: merge_request, + position: position, + project: project) + end + + context 'user cannot write in project repo' do + before do + project.add_reporter(user) + end + + it 'returns error' do + result = subject.execute(suggestion) + + expect(result).to eq(message: "You are not allowed to push into this branch", + status: :error) + end + end + end + + context 'patch is not appliable' do + let(:merge_request) do + create(:merge_request, source_project: project, + 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 + + context 'suggestion was already applied' do + it 'returns success status' do + result = subject.execute(suggestion) + + expect(result[:status]).to eq(:success) + end + end + + context 'note is outdated' do + before do + allow(diff_note).to receive(:active?) { false } + end + + it 'returns error message' do + result = subject.execute(suggestion) + + expect(result).to eq(message: 'Suggestion is not appliable', + status: :error) + end + end + + context 'suggestion was already applied' do + before do + suggestion.update!(applied: true, commit_id: 'sha') + end + + it 'returns error message' do + result = subject.execute(suggestion) + + expect(result).to eq(message: 'Suggestion is not appliable', + status: :error) + end + end + end +end diff --git a/spec/services/suggestions/create_service_spec.rb b/spec/services/suggestions/create_service_spec.rb new file mode 100644 index 00000000000..f1142c88a69 --- /dev/null +++ b/spec/services/suggestions/create_service_spec.rb @@ -0,0 +1,110 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Suggestions::CreateService do + let(:project_with_repo) { create(:project, :repository) } + let(:merge_request) do + create(:merge_request, source_project: project_with_repo, + target_project: project_with_repo) + end + + let(:position) do + Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb", + new_path: "files/ruby/popen.rb", + old_line: nil, + new_line: 14, + diff_refs: merge_request.diff_refs) + end + + 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 + + subject { described_class.new(note) } + + describe '#execute' do + context 'should not try to parse suggestions' do + context 'when not a diff note for merge requests' do + let(:note) do + create(:diff_note_on_commit, project: project_with_repo, + note: markdown) + end + + it 'does not try to parse suggestions' do + expect(Banzai::SuggestionsParser).not_to receive(:parse) + + subject.execute + end + end + + context 'when diff note is not for text' do + let(:note) do + create(:diff_note_on_merge_request, project: project_with_repo, + noteable: merge_request, + position: position, + note: markdown) + end + + it 'does not try to parse suggestions' do + allow(note).to receive(:on_text?) { false } + + expect(Banzai::SuggestionsParser).not_to receive(:parse) + + subject.execute + end + end + end + + context 'should create suggestions' do + let(:note) do + create(:diff_note_on_merge_request, project: project_with_repo, + noteable: merge_request, + position: position, + note: markdown) + end + + context 'single line suggestions' do + it 'persists suggestion records' do + expect { subject.execute } + .to change { note.suggestions.count } + .from(0) + .to(2) + end + + it 'persists original from_content lines and suggested lines' do + subject.execute + + suggestions = note.suggestions.order(:relative_order) + + suggestion_1 = suggestions.first + suggestion_2 = suggestions.last + + expect(suggestion_1).to have_attributes(from_content: " vars = {\n", + to_content: " foo\n bar\n") + + expect(suggestion_2).to have_attributes(from_content: " vars = {\n", + to_content: " xpto\n baz\n") + end + end + end + end +end |