From bf4adbcd0af24afd19799b878d8b8b47b34a992d Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 28 Jan 2021 15:10:46 -0700 Subject: [PATCH 1/5] Add a utility for getting a suggestion commit message By default, the commit message has a bunch of variables in it, like `%{some_var}`. We want to be able to replace those variables with actual values. Rather than doing it ad hoc (maybe in some leaf component), this utility provides a central place where that logic lives. --- .../javascripts/diffs/utils/suggestions.js | 28 +++++++++++++++++++ spec/frontend/diffs/utils/suggestions_spec.js | 15 ++++++++++ 2 files changed, 43 insertions(+) create mode 100644 app/assets/javascripts/diffs/utils/suggestions.js create mode 100644 spec/frontend/diffs/utils/suggestions_spec.js diff --git a/app/assets/javascripts/diffs/utils/suggestions.js b/app/assets/javascripts/diffs/utils/suggestions.js new file mode 100644 index 00000000000000..a272f7f32571f7 --- /dev/null +++ b/app/assets/javascripts/diffs/utils/suggestions.js @@ -0,0 +1,28 @@ +function removeEmptyProperties(dict) { + const noBlanks = Object.entries(dict).reduce((final, [key, value]) => { + const upd = { ...final }; + + // The number 0 shouldn't be falsey when we're printing variables + if (value || value === 0) { + upd[key] = value; + } + + return upd; + }, {}); + + return noBlanks; +} + +export function computeSuggestionCommitMessage({ message, values = {} } = {}) { + const noEmpties = removeEmptyProperties(values); + const matchPhrases = Object.keys(noEmpties) + .map((key) => `%{${key}}`) + .join('|'); + const replacementExpression = new RegExp(`(${matchPhrases})`, 'gm'); + + return message.replace(replacementExpression, (match) => { + const key = match.replace(/(^%{|}$)/gm, ''); + + return noEmpties[key]; + }); +} diff --git a/spec/frontend/diffs/utils/suggestions_spec.js b/spec/frontend/diffs/utils/suggestions_spec.js new file mode 100644 index 00000000000000..fbfe9cef85775d --- /dev/null +++ b/spec/frontend/diffs/utils/suggestions_spec.js @@ -0,0 +1,15 @@ +import { computeSuggestionCommitMessage } from '~/diffs/utils/suggestions'; + +describe('Diff Suggestions utilities', () => { + describe('computeSuggestionCommitMessage', () => { + it.each` + description | input | values | output + ${'makes the appropriate replacements'} | ${'%{foo} %{bar}'} | ${{ foo: 'foo', bar: 'bar' }} | ${'foo bar'} + ${"skips replacing values that aren't passed"} | ${'%{foo} %{bar}'} | ${{ foo: 'foo' }} | ${'foo %{bar}'} + ${'treats the number 0 as a valid value (not falsey)'} | ${'%{foo} %{bar}'} | ${{ foo: 'foo', bar: 0 }} | ${'foo 0'} + ${"works when the variables don't have any space between them"} | ${'%{foo}%{bar}'} | ${{ foo: 'foo', bar: 'bar' }} | ${'foobar'} + `('$description', ({ input, output, values }) => { + expect(computeSuggestionCommitMessage({ message: input, values })).toBe(output); + }); + }); +}); -- GitLab From 6154fb9e220d6009b77a544ad06ee8b5c16f2039 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 1 Feb 2021 21:46:54 -0700 Subject: [PATCH 2/5] Add getter for the complete suggestion commit message --- app/assets/javascripts/diffs/store/getters.js | 16 +++++ spec/frontend/diffs/store/getters_spec.js | 60 +++++++++++++++++++ 2 files changed, 76 insertions(+) diff --git a/app/assets/javascripts/diffs/store/getters.js b/app/assets/javascripts/diffs/store/getters.js index 149afc01056a83..1fc2a684e951b1 100644 --- a/app/assets/javascripts/diffs/store/getters.js +++ b/app/assets/javascripts/diffs/store/getters.js @@ -4,6 +4,7 @@ import { INLINE_DIFF_VIEW_TYPE, INLINE_DIFF_LINES_KEY, } from '../constants'; +import { computeSuggestionCommitMessage } from '../utils/suggestions'; import { parallelizeDiffLines } from './utils'; export * from './getters_versions_dropdowns'; @@ -154,3 +155,18 @@ export const diffLines = (state) => (file, unifiedDiffComponents) => { state.diffViewType === INLINE_DIFF_VIEW_TYPE, ); }; + +export function suggestionCommitMessage(state) { + return (values = {}) => + computeSuggestionCommitMessage({ + message: state.defaultSuggestionCommitMessage, + values: { + branch_name: state.branchName, + project_path: state.projectPath, + project_name: state.projectName, + username: state.username, + user_full_name: state.userFullName, + ...values, + }, + }); +} diff --git a/spec/frontend/diffs/store/getters_spec.js b/spec/frontend/diffs/store/getters_spec.js index 85dc52f8bf70f2..56d6bc844c9260 100644 --- a/spec/frontend/diffs/store/getters_spec.js +++ b/spec/frontend/diffs/store/getters_spec.js @@ -375,4 +375,64 @@ describe('Diffs Module Getters', () => { }); }); }); + + describe('suggestionCommitMessage', () => { + beforeEach(() => { + Object.assign(localState, { + defaultSuggestionCommitMessage: + '%{branch_name}%{project_path}%{project_name}%{username}%{user_full_name}%{file_paths}%{suggestions_count}%{files_count}', + branchName: 'branch', + projectPath: '/path', + projectName: 'name', + username: 'user', + userFullName: 'user userton', + }); + }); + + it.each` + specialState | output + ${{}} | ${'branch/pathnameuseruser userton%{file_paths}%{suggestions_count}%{files_count}'} + ${{ userFullName: null }} | ${'branch/pathnameuser%{user_full_name}%{file_paths}%{suggestions_count}%{files_count}'} + ${{ username: null }} | ${'branch/pathname%{username}user userton%{file_paths}%{suggestions_count}%{files_count}'} + ${{ projectName: null }} | ${'branch/path%{project_name}useruser userton%{file_paths}%{suggestions_count}%{files_count}'} + ${{ projectPath: null }} | ${'branch%{project_path}nameuseruser userton%{file_paths}%{suggestions_count}%{files_count}'} + ${{ branchName: null }} | ${'%{branch_name}/pathnameuseruser userton%{file_paths}%{suggestions_count}%{files_count}'} + `( + 'provides the correct "base" default commit message based on state ($specialState)', + ({ specialState, output }) => { + Object.assign(localState, specialState); + + expect(getters.suggestionCommitMessage(localState)()).toBe(output); + }, + ); + + it.each` + stateOverrides | output + ${{}} | ${'branch/pathnameuseruser userton%{file_paths}%{suggestions_count}%{files_count}'} + ${{ user_full_name: null }} | ${'branch/pathnameuser%{user_full_name}%{file_paths}%{suggestions_count}%{files_count}'} + ${{ username: null }} | ${'branch/pathname%{username}user userton%{file_paths}%{suggestions_count}%{files_count}'} + ${{ project_name: null }} | ${'branch/path%{project_name}useruser userton%{file_paths}%{suggestions_count}%{files_count}'} + ${{ project_path: null }} | ${'branch%{project_path}nameuseruser userton%{file_paths}%{suggestions_count}%{files_count}'} + ${{ branch_name: null }} | ${'%{branch_name}/pathnameuseruser userton%{file_paths}%{suggestions_count}%{files_count}'} + `( + "properly overrides state values ($stateOverrides) if they're provided", + ({ stateOverrides, output }) => { + expect(getters.suggestionCommitMessage(localState)(stateOverrides)).toBe(output); + }, + ); + + it.each` + providedValues | output + ${{ file_paths: 'path1, path2', suggestions_count: 1, files_count: 1 }} | ${'branch/pathnameuseruser usertonpath1, path211'} + ${{ suggestions_count: 1, files_count: 1 }} | ${'branch/pathnameuseruser userton%{file_paths}11'} + ${{ file_paths: 'path1, path2', files_count: 1 }} | ${'branch/pathnameuseruser usertonpath1, path2%{suggestions_count}1'} + ${{ file_paths: 'path1, path2', suggestions_count: 1 }} | ${'branch/pathnameuseruser usertonpath1, path21%{files_count}'} + ${{ something_unused: 'CrAzY TeXt' }} | ${'branch/pathnameuseruser userton%{file_paths}%{suggestions_count}%{files_count}'} + `( + "fills in any missing interpolations ($providedValues) when they're provided at the getter callsite", + ({ providedValues, output }) => { + expect(getters.suggestionCommitMessage(localState)(providedValues)).toBe(output); + }, + ); + }); }); -- GitLab From b971150b25a9602c08c5a8e75df03fead5b82bc7 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 28 Jan 2021 17:10:24 -0700 Subject: [PATCH 3/5] Pass the file to the noteable note, since it's needed The file is needed to compute the suggestion commit message --- app/assets/javascripts/notes/components/noteable_note.vue | 1 + 1 file changed, 1 insertion(+) diff --git a/app/assets/javascripts/notes/components/noteable_note.vue b/app/assets/javascripts/notes/components/noteable_note.vue index a1738b993d7476..22941857f939e7 100644 --- a/app/assets/javascripts/notes/components/noteable_note.vue +++ b/app/assets/javascripts/notes/components/noteable_note.vue @@ -431,6 +431,7 @@ export default { ref="noteBody" :note="note" :line="line" + :file="diffFile" :can-edit="note.current_user.can_edit" :is-editing="isEditing" :help-page-path="helpPagePath" -- GitLab From bd5cb612c4cbacf5a74ce31d3fd32d7b53c3d331 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 3 Feb 2021 10:19:00 -0700 Subject: [PATCH 4/5] Get the complete suggestion commit message This replaces the default value that's provided by the Vuex store and the user settings. The computed value takes the user's setting and fills in all of the values. --- .../notes/components/note_body.vue | 26 ++++++++- .../notes/components/note_body_spec.js | 54 +++++++++++++++++++ 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/notes/components/note_body.vue b/app/assets/javascripts/notes/components/note_body.vue index 03a8a8f93766a7..df56e6fd8b01c8 100644 --- a/app/assets/javascripts/notes/components/note_body.vue +++ b/app/assets/javascripts/notes/components/note_body.vue @@ -2,6 +2,8 @@ /* eslint-disable vue/no-v-html */ import { mapActions, mapGetters, mapState } from 'vuex'; import $ from 'jquery'; +import { escape } from 'lodash'; + import '~/behaviors/markdown/render_gfm'; import Suggestions from '~/vue_shared/components/markdown/suggestions.vue'; import autosave from '../mixins/autosave'; @@ -29,6 +31,11 @@ export default { required: false, default: null, }, + file: { + type: Object, + required: false, + default: null, + }, canEdit: { type: Boolean, required: true, @@ -46,6 +53,7 @@ export default { }, computed: { ...mapGetters(['getDiscussion', 'suggestionsCount']), + ...mapGetters('diffs', ['suggestionCommitMessage']), discussion() { if (!this.note.isDraft) return {}; @@ -54,7 +62,6 @@ export default { ...mapState({ batchSuggestionsInfo: (state) => state.notes.batchSuggestionsInfo, }), - ...mapState('diffs', ['defaultSuggestionCommitMessage']), noteBody() { return this.note.note; }, @@ -64,6 +71,21 @@ export default { lineType() { return this.line ? this.line.type : null; }, + commitMessage() { + // Please see this issue comment for why these + // are hard-coded to 1: + // https://gitlab.com/gitlab-org/gitlab/-/issues/291027#note_468308022 + const suggestionsCount = 1; + const filesCount = 1; + const filePaths = this.file ? [this.file.file_path] : []; + const suggestion = this.suggestionCommitMessage({ + file_paths: filePaths.join(', '), + suggestions_count: suggestionsCount, + files_count: filesCount, + }); + + return escape(suggestion); + }, }, mounted() { this.renderGFM(); @@ -135,7 +157,7 @@ export default { :note-html="note.note_html" :line-type="lineType" :help-page-path="helpPagePath" - :default-commit-message="defaultSuggestionCommitMessage" + :default-commit-message="commitMessage" @apply="applySuggestion" @applyBatch="applySuggestionBatch" @addToBatch="addSuggestionToBatch" diff --git a/spec/frontend/notes/components/note_body_spec.js b/spec/frontend/notes/components/note_body_spec.js index 3c11c266f90068..07b8d99a79a128 100644 --- a/spec/frontend/notes/components/note_body_spec.js +++ b/spec/frontend/notes/components/note_body_spec.js @@ -1,6 +1,14 @@ import Vue from 'vue'; +import { shallowMount } from '@vue/test-utils'; +import Vuex from 'vuex'; + +import notes from '~/notes/stores/modules/index'; import createStore from '~/notes/stores'; +import { suggestionCommitMessage } from '~/diffs/store/getters'; + +import Suggestions from '~/vue_shared/components/markdown/suggestions.vue'; import noteBody from '~/notes/components/note_body.vue'; + import { noteableDataMock, notesDataMock, note } from '../mock_data'; describe('issue_note_body component', () => { @@ -54,4 +62,50 @@ describe('issue_note_body component', () => { expect(vm.autosave.key).toEqual(autosaveKey); }); }); + + describe('commitMessage', () => { + let wrapper; + + Vue.use(Vuex); + + beforeEach(() => { + const notesStore = notes(); + + notesStore.state.notes = {}; + + store = new Vuex.Store({ + modules: { + notes: notesStore, + diffs: { + namespaced: true, + state: { + defaultSuggestionCommitMessage: + '%{branch_name}%{project_path}%{project_name}%{username}%{user_full_name}%{file_paths}%{suggestions_count}%{files_count}', + branchName: 'branch', + projectPath: '/path', + projectName: 'name', + username: 'user', + userFullName: 'user userton', + }, + getters: { suggestionCommitMessage }, + }, + }, + }); + + wrapper = shallowMount(noteBody, { + store, + propsData: { + note: { ...note, suggestions: [12345] }, + canEdit: true, + file: { file_path: 'abc' }, + }, + }); + }); + + it('passes the correct default placeholder commit message for a suggestion to the suggestions component', () => { + const commitMessage = wrapper.find(Suggestions).attributes('defaultcommitmessage'); + + expect(commitMessage).toBe('branch/pathnameuseruser usertonabc11'); + }); + }); }); -- GitLab From 2378eaf3ca305bc1d8fd491ae51de11c8a8de5be Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 1 Feb 2021 18:51:29 -0700 Subject: [PATCH 5/5] Add changelog for variable-filled default commit message placeholder --- .../unreleased/tor-defect-commit-message-placeholders.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelogs/unreleased/tor-defect-commit-message-placeholders.yml diff --git a/changelogs/unreleased/tor-defect-commit-message-placeholders.yml b/changelogs/unreleased/tor-defect-commit-message-placeholders.yml new file mode 100644 index 00000000000000..093dfdda81a029 --- /dev/null +++ b/changelogs/unreleased/tor-defect-commit-message-placeholders.yml @@ -0,0 +1,6 @@ +--- +title: Fill default commit message values in the placeholder instead of showing the + variable slugs +merge_request: 52851 +author: +type: fixed -- GitLab