From ed3034bbb71d43b12944a9da29b5264cb3ff3312 Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Thu, 13 Dec 2018 19:17:19 +0000 Subject: Allow suggesting single line changes in diffs --- app/assets/javascripts/api.js | 7 + app/assets/javascripts/diffs/components/app.vue | 6 + .../javascripts/diffs/components/diff_content.vue | 7 + .../diffs/components/diff_discussions.vue | 12 ++ .../javascripts/diffs/components/diff_file.vue | 6 + .../diffs/components/diff_line_note_form.vue | 1 + .../diffs/components/inline_diff_comment_row.vue | 12 +- .../diffs/components/inline_diff_view.vue | 6 + .../diffs/components/parallel_diff_comment_row.vue | 9 + .../diffs/components/parallel_diff_view.vue | 6 + app/assets/javascripts/diffs/index.js | 2 + app/assets/javascripts/lib/utils/text_markdown.js | 42 +++- app/assets/javascripts/mr_notes/index.js | 2 + .../javascripts/notes/components/note_body.vue | 38 +++- .../javascripts/notes/components/note_form.vue | 35 +++- .../notes/components/noteable_discussion.vue | 24 +++ .../javascripts/notes/components/noteable_note.vue | 12 ++ .../javascripts/notes/components/notes_app.vue | 6 + .../javascripts/notes/services/notes_service.js | 4 + app/assets/javascripts/notes/stores/actions.js | 20 ++ .../javascripts/notes/stores/modules/index.js | 1 + .../javascripts/notes/stores/mutation_types.js | 1 + app/assets/javascripts/notes/stores/mutations.js | 11 + .../vue_shared/components/markdown/field.vue | 97 ++++++++- .../vue_shared/components/markdown/header.vue | 23 +++ .../components/markdown/suggestion_diff.vue | 74 +++++++ .../components/markdown/suggestion_diff_header.vue | 60 ++++++ .../vue_shared/components/markdown/suggestions.vue | 136 ++++++++++++ .../components/markdown/toolbar_button.vue | 12 ++ .../stylesheets/framework/markdown_area.scss | 21 ++ app/assets/stylesheets/framework/variables.scss | 1 + app/controllers/concerns/preview_markdown.rb | 10 +- app/models/concerns/noteable.rb | 4 + app/models/diff_note.rb | 13 ++ app/models/merge_request.rb | 5 + app/models/note.rb | 12 +- app/models/suggestion.rb | 61 ++++++ app/policies/suggestion_policy.rb | 11 + app/serializers/diff_line_entity.rb | 2 + app/serializers/merge_request_widget_entity.rb | 2 + app/serializers/note_entity.rb | 1 + app/serializers/suggestion_entity.rb | 17 ++ app/services/notes/create_service.rb | 1 + app/services/notes/update_service.rb | 11 + app/services/preview_markdown_service.rb | 8 + app/services/suggestions/apply_service.rb | 54 +++++ app/services/suggestions/create_service.rb | 56 +++++ app/views/projects/merge_requests/show.html.haml | 2 + .../unreleased/suggest-change-to-diff-line.yml | 5 + db/migrate/20181123144235_create_suggestions.rb | 20 ++ db/schema.rb | 11 + lib/api/api.rb | 1 + lib/api/entities.rb | 12 ++ lib/api/suggestions.rb | 31 +++ lib/banzai/filter/suggestion_filter.rb | 25 +++ lib/banzai/filter/syntax_highlight_filter.rb | 2 +- lib/banzai/pipeline/gfm_pipeline.rb | 1 + lib/banzai/pipeline/post_process_pipeline.rb | 3 +- lib/banzai/suggestions_parser.rb | 14 ++ lib/gitlab/diff/file.rb | 10 + lib/gitlab/diff/line.rb | 4 + lib/gitlab/usage_data.rb | 2 + locale/gitlab.pot | 15 ++ spec/db/schema_spec.rb | 3 +- spec/factories/suggestions.rb | 20 ++ .../user_suggests_changes_on_diff_spec.rb | 85 ++++++++ spec/fixtures/api/schemas/entities/diff_line.json | 3 +- .../api/schemas/entities/merge_request_widget.json | 3 +- .../diffs/components/diff_content_spec.js | 1 + .../diffs/mock_data/diff_discussions.js | 15 +- .../vue_shared/components/markdown/header_spec.js | 15 ++ .../markdown/suggestion_diff_header_spec.js | 69 +++++++ .../components/markdown/suggestion_diff_spec.js | 79 +++++++ .../components/markdown/suggestions_spec.js | 125 +++++++++++ spec/lib/banzai/filter/suggestion_filter_spec.rb | 35 ++++ spec/lib/banzai/suggestions_parser_spec.rb | 32 +++ spec/lib/gitlab/import_export/all_models.yml | 1 + spec/lib/gitlab/usage_data_spec.rb | 1 + spec/models/diff_note_spec.rb | 18 ++ spec/models/suggestion_spec.rb | 57 +++++ spec/requests/api/suggestions_spec.rb | 83 ++++++++ spec/serializers/suggestion_entity_spec.rb | 23 +++ spec/services/notes/update_service_spec.rb | 23 +++ spec/services/preview_markdown_service_spec.rb | 25 +++ spec/services/suggestions/apply_service_spec.rb | 229 +++++++++++++++++++++ spec/services/suggestions/create_service_spec.rb | 110 ++++++++++ 86 files changed, 2150 insertions(+), 25 deletions(-) create mode 100644 app/assets/javascripts/vue_shared/components/markdown/suggestion_diff.vue create mode 100644 app/assets/javascripts/vue_shared/components/markdown/suggestion_diff_header.vue create mode 100644 app/assets/javascripts/vue_shared/components/markdown/suggestions.vue create mode 100644 app/models/suggestion.rb create mode 100644 app/policies/suggestion_policy.rb create mode 100644 app/serializers/suggestion_entity.rb create mode 100644 app/services/suggestions/apply_service.rb create mode 100644 app/services/suggestions/create_service.rb create mode 100644 changelogs/unreleased/suggest-change-to-diff-line.yml create mode 100644 db/migrate/20181123144235_create_suggestions.rb create mode 100644 lib/api/suggestions.rb create mode 100644 lib/banzai/filter/suggestion_filter.rb create mode 100644 lib/banzai/suggestions_parser.rb create mode 100644 spec/factories/suggestions.rb create mode 100644 spec/features/merge_request/user_suggests_changes_on_diff_spec.rb create mode 100644 spec/javascripts/vue_shared/components/markdown/suggestion_diff_header_spec.js create mode 100644 spec/javascripts/vue_shared/components/markdown/suggestion_diff_spec.js create mode 100644 spec/javascripts/vue_shared/components/markdown/suggestions_spec.js create mode 100644 spec/lib/banzai/filter/suggestion_filter_spec.rb create mode 100644 spec/lib/banzai/suggestions_parser_spec.rb create mode 100644 spec/models/suggestion_spec.rb create mode 100644 spec/requests/api/suggestions_spec.rb create mode 100644 spec/serializers/suggestion_entity_spec.rb create mode 100644 spec/services/suggestions/apply_service_spec.rb create mode 100644 spec/services/suggestions/create_service_spec.rb 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" /> 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" /> 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" />
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 {
- + 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 --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 @@