diff options
10 files changed, 283 insertions, 124 deletions
diff --git a/app/assets/javascripts/notes/components/note_body.vue b/app/assets/javascripts/notes/components/note_body.vue index 8ddd5b8514a..88454c3fb4c 100644 --- a/app/assets/javascripts/notes/components/note_body.vue +++ b/app/assets/javascripts/notes/components/note_body.vue @@ -83,10 +83,12 @@ export default { formCancelHandler(shouldConfirm, isDirty) { this.$emit('cancelForm', shouldConfirm, isDirty); }, - applySuggestion({ suggestionId, flashContainer, callback }) { + applySuggestion({ suggestionId, flashContainer, callback = () => {} }) { const { discussion_id: discussionId, id: noteId } = this.note; - this.submitSuggestion({ discussionId, noteId, suggestionId, flashContainer, callback }); + return this.submitSuggestion({ discussionId, noteId, suggestionId, flashContainer }).then( + callback, + ); }, }, }; diff --git a/app/assets/javascripts/notes/stores/actions.js b/app/assets/javascripts/notes/stores/actions.js index 970e6551092..bac124be34c 100644 --- a/app/assets/javascripts/notes/stores/actions.js +++ b/app/assets/javascripts/notes/stores/actions.js @@ -142,6 +142,23 @@ export const createNewNote = ({ commit, dispatch }, { endpoint, data }) => export const removePlaceholderNotes = ({ commit }) => commit(types.REMOVE_PLACEHOLDER_NOTES); +export const resolveDiscussion = ({ state, dispatch, getters }, { discussionId }) => { + const discussion = utils.findNoteObjectById(state.discussions, discussionId); + const isResolved = getters.isDiscussionResolved(discussionId); + + if (!discussion) { + return Promise.reject(); + } else if (isResolved) { + return Promise.resolve(); + } + + return dispatch('toggleResolveNote', { + endpoint: discussion.resolve_path, + isResolved, + discussion: true, + }); +}; + export const toggleResolveNote = ({ commit, dispatch }, { endpoint, isResolved, discussion }) => service .toggleResolveNote(endpoint, isResolved) @@ -420,15 +437,13 @@ export const updateResolvableDiscussonsCounts = ({ commit }) => commit(types.UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS); export const submitSuggestion = ( - { commit }, - { discussionId, noteId, suggestionId, flashContainer, callback }, -) => { + { commit, dispatch }, + { discussionId, noteId, suggestionId, flashContainer }, +) => service .applySuggestion(suggestionId) - .then(() => { - commit(types.APPLY_SUGGESTION, { discussionId, noteId, suggestionId }); - callback(); - }) + .then(() => commit(types.APPLY_SUGGESTION, { discussionId, noteId, suggestionId })) + .then(() => dispatch('resolveDiscussion', { discussionId }).catch(() => {})) .catch(err => { const defaultMessage = __( 'Something went wrong while applying the suggestion. Please try again.', @@ -436,9 +451,7 @@ export const submitSuggestion = ( const flashMessage = err.response.data ? `${err.response.data.message}.` : defaultMessage; Flash(__(flashMessage), 'alert', flashContainer); - callback(); }); -}; export const convertToDiscussion = ({ commit }, noteId) => commit(types.CONVERT_TO_DISCUSSION, noteId); 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 index c5a2aa1f2af..32783b85df4 100644 --- a/app/assets/javascripts/vue_shared/components/markdown/suggestion_diff_header.vue +++ b/app/assets/javascripts/vue_shared/components/markdown/suggestion_diff_header.vue @@ -1,8 +1,10 @@ <script> import Icon from '~/vue_shared/components/icon.vue'; +import { GlButton, GlLoadingIcon, GlTooltipDirective } from '@gitlab/ui'; export default { - components: { Icon }, + components: { Icon, GlButton, GlLoadingIcon }, + directives: { 'gl-tooltip': GlTooltipDirective }, props: { canApply: { type: Boolean, @@ -21,7 +23,6 @@ export default { }, data() { return { - isAppliedSuccessfully: false, isApplying: false, }; }, @@ -47,14 +48,19 @@ export default { </a> </div> <span v-if="isApplied" class="badge badge-success">{{ __('Applied') }}</span> - <button - v-if="canApply" - type="button" - class="btn qa-apply-btn" + <div v-if="isApplying" class="d-flex align-items-center text-secondary"> + <gl-loading-icon class="d-flex-center mr-2" /> + <span>{{ __('Applying suggestion') }}</span> + </div> + <gl-button + v-else-if="canApply" + v-gl-tooltip.viewport="__('This also resolves the discussion')" + class="btn-inverted qa-apply-btn" :disabled="isApplying" + variant="success" @click="applySuggestion" > {{ __('Apply suggestion') }} - </button> + </gl-button> </div> </template> diff --git a/changelogs/unreleased/54405-resolve-discussion-when-applying-a-suggested-change.yml b/changelogs/unreleased/54405-resolve-discussion-when-applying-a-suggested-change.yml new file mode 100644 index 00000000000..862ce623d8c --- /dev/null +++ b/changelogs/unreleased/54405-resolve-discussion-when-applying-a-suggested-change.yml @@ -0,0 +1,5 @@ +--- +title: Resolve discussion when apply suggestion +merge_request: 28160 +author: +type: changed diff --git a/doc/user/discussions/index.md b/doc/user/discussions/index.md index 248f8395db1..9c29265847e 100644 --- a/doc/user/discussions/index.md +++ b/doc/user/discussions/index.md @@ -401,13 +401,10 @@ the Merge Request authored by the user that applied them.  - > **Note:** - Discussions are _not_ automatically resolved. Will be introduced by - [#54405](https://gitlab.com/gitlab-org/gitlab-ce/issues/54405). - Once the author applies a suggestion, it will be marked with the **Applied** label, -and GitLab will create a new commit with the message `Apply suggestion to <file-name>` -and push the suggested change directly into the codebase in the merge request's branch. +the discussion will be automatically resolved, and GitLab will create a new commit +with the message `Apply suggestion to <file-name>` and push the suggested change +directly into the codebase in the merge request's branch. [Developer permission](../permissions.md) is required to do so. > **Note:** diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 445984c7847..50ff8a5e041 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -1024,6 +1024,9 @@ msgstr "" msgid "Applying multiple commands" msgstr "" +msgid "Applying suggestion" +msgstr "" + msgid "Apr" msgstr "" @@ -9517,6 +9520,9 @@ msgstr "" msgid "This action can lead to data loss. To prevent accidental actions we ask you to confirm your intention." msgstr "" +msgid "This also resolves the discussion" +msgstr "" + msgid "This application was created by %{link_to_owner}." msgstr "" diff --git a/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb b/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb index 1b5dd6945e0..04c7f4b6c76 100644 --- a/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb +++ b/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb @@ -121,7 +121,7 @@ describe 'User comments on a diff', :js do end context 'multi-line suggestions' do - it 'suggestion is presented' do + before do click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']")) page.within('.js-discussion-note-form') do @@ -130,7 +130,9 @@ describe 'User comments on a diff', :js do end wait_for_requests + end + it 'suggestion is presented' do page.within('.diff-discussions') do expect(page).to have_button('Apply suggestion') expect(page).to have_content('Suggested change') @@ -160,22 +162,24 @@ describe 'User comments on a diff', :js do end it 'suggestion is appliable' do - click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']")) + page.within('.diff-discussions') do + expect(page).not_to have_content('Applied') - page.within('.js-discussion-note-form') do - fill_in('note_note', with: "```suggestion:-3+5\n# change to a\n# comment\n# with\n# broken\n# lines\n```") - click_button('Comment') - end + click_button('Apply suggestion') + wait_for_requests - wait_for_requests + expect(page).to have_content('Applied') + end + end + it 'resolves discussion when applied' do page.within('.diff-discussions') do - expect(page).not_to have_content('Applied') + expect(page).not_to have_content('Unresolve discussion') click_button('Apply suggestion') wait_for_requests - expect(page).to have_content('Applied') + expect(page).to have_content('Unresolve discussion') end end end diff --git a/spec/frontend/vue_shared/components/markdown/suggestion_diff_header_spec.js b/spec/frontend/vue_shared/components/markdown/suggestion_diff_header_spec.js new file mode 100644 index 00000000000..3b6f67457ad --- /dev/null +++ b/spec/frontend/vue_shared/components/markdown/suggestion_diff_header_spec.js @@ -0,0 +1,103 @@ +import { GlLoadingIcon } from '@gitlab/ui'; +import { shallowMount, createLocalVue } from '@vue/test-utils'; +import SuggestionDiffHeader from '~/vue_shared/components/markdown/suggestion_diff_header.vue'; + +const localVue = createLocalVue(); + +const DEFAULT_PROPS = { + canApply: true, + isApplied: false, + helpPagePath: 'path_to_docs', +}; + +describe('Suggestion Diff component', () => { + let wrapper; + + const createComponent = props => { + wrapper = shallowMount(localVue.extend(SuggestionDiffHeader), { + propsData: { + ...DEFAULT_PROPS, + ...props, + }, + localVue, + sync: false, + }); + }; + + afterEach(() => { + wrapper.destroy(); + }); + + const findApplyButton = () => wrapper.find('.qa-apply-btn'); + const findHeader = () => wrapper.find('.qa-suggestion-diff-header'); + const findHelpButton = () => wrapper.find('.js-help-btn'); + const findLoading = () => wrapper.find(GlLoadingIcon); + + it('renders a suggestion header', () => { + createComponent(); + + const header = findHeader(); + + expect(header.exists()).toBe(true); + expect(header.html().includes('Suggested change')).toBe(true); + }); + + it('renders a help button', () => { + createComponent(); + + expect(findHelpButton().exists()).toBe(true); + }); + + it('renders an apply button', () => { + createComponent(); + + const applyBtn = findApplyButton(); + + expect(applyBtn.exists()).toBe(true); + expect(applyBtn.html().includes('Apply suggestion')).toBe(true); + }); + + it('does not render an apply button if `canApply` is set to false', () => { + createComponent({ canApply: false }); + + expect(findApplyButton().exists()).toBe(false); + }); + + describe('when apply suggestion is clicked', () => { + beforeEach(done => { + createComponent(); + + findApplyButton().vm.$emit('click'); + + wrapper.vm.$nextTick(done); + }); + + it('emits apply', () => { + expect(wrapper.emittedByOrder()).toEqual([{ name: 'apply', args: [expect.any(Function)] }]); + }); + + it('hides apply button', () => { + expect(findApplyButton().exists()).toBe(false); + }); + + it('shows loading', () => { + expect(findLoading().exists()).toBe(true); + expect(wrapper.text()).toContain('Applying suggestion'); + }); + + it('when callback of apply is called, hides loading', done => { + const [callback] = wrapper.emitted().apply[0]; + + callback(); + + wrapper.vm + .$nextTick() + .then(() => { + expect(findApplyButton().exists()).toBe(true); + expect(findLoading().exists()).toBe(false); + }) + .then(done) + .catch(done.fail); + }); + }); +}); diff --git a/spec/javascripts/notes/stores/actions_spec.js b/spec/javascripts/notes/stores/actions_spec.js index 94ce6d8e222..39901276b8c 100644 --- a/spec/javascripts/notes/stores/actions_spec.js +++ b/spec/javascripts/notes/stores/actions_spec.js @@ -3,11 +3,12 @@ import $ from 'jquery'; import _ from 'underscore'; import { TEST_HOST } from 'spec/test_constants'; import { headersInterceptor } from 'spec/helpers/vue_resource_helper'; -import * as actions from '~/notes/stores/actions'; +import actionsModule, * as actions from '~/notes/stores/actions'; import * as mutationTypes from '~/notes/stores/mutation_types'; import * as notesConstants from '~/notes/constants'; import createStore from '~/notes/stores'; import mrWidgetEventHub from '~/vue_merge_request_widget/event_hub'; +import service from '~/notes/services/notes_service'; import testAction from '../../helpers/vuex_action_helper'; import { resetStore } from '../helpers'; import { @@ -18,11 +19,21 @@ import { individualNote, } from '../mock_data'; +const TEST_ERROR_MESSAGE = 'Test error message'; + describe('Actions Notes Store', () => { + let commit; + let dispatch; + let state; let store; + let flashSpy; beforeEach(() => { store = createStore(); + commit = jasmine.createSpy('commit'); + dispatch = jasmine.createSpy('dispatch'); + state = {}; + flashSpy = spyOnDependency(actionsModule, 'Flash'); }); afterEach(() => { @@ -604,21 +615,6 @@ describe('Actions Notes Store', () => { }); describe('updateOrCreateNotes', () => { - let commit; - let dispatch; - let state; - - beforeEach(() => { - commit = jasmine.createSpy('commit'); - dispatch = jasmine.createSpy('dispatch'); - state = {}; - }); - - afterEach(() => { - commit.calls.reset(); - dispatch.calls.reset(); - }); - it('Updates existing note', () => { const note = { id: 1234 }; const getters = { notesById: { 1234: note } }; @@ -751,4 +747,106 @@ describe('Actions Notes Store', () => { ); }); }); + + describe('resolveDiscussion', () => { + let getters; + let discussionId; + + beforeEach(() => { + discussionId = discussionMock.id; + state.discussions = [discussionMock]; + getters = { + isDiscussionResolved: () => false, + }; + }); + + it('when unresolved, dispatches action', done => { + testAction( + actions.resolveDiscussion, + { discussionId }, + { ...state, ...getters }, + [], + [ + { + type: 'toggleResolveNote', + payload: { + endpoint: discussionMock.resolve_path, + isResolved: false, + discussion: true, + }, + }, + ], + done, + ); + }); + + it('when resolved, does nothing', done => { + getters.isDiscussionResolved = id => id === discussionId; + + testAction( + actions.resolveDiscussion, + { discussionId }, + { ...state, ...getters }, + [], + [], + done, + ); + }); + }); + + describe('submitSuggestion', () => { + const discussionId = 'discussion-id'; + const noteId = 'note-id'; + const suggestionId = 'suggestion-id'; + let flashContainer; + + beforeEach(() => { + spyOn(service, 'applySuggestion'); + dispatch.and.returnValue(Promise.resolve()); + service.applySuggestion.and.returnValue(Promise.resolve()); + flashContainer = {}; + }); + + const testSubmitSuggestion = (done, expectFn) => { + actions + .submitSuggestion( + { commit, dispatch }, + { discussionId, noteId, suggestionId, flashContainer }, + ) + .then(expectFn) + .then(done) + .catch(done.fail); + }; + + it('when service success, commits and resolves discussion', done => { + testSubmitSuggestion(done, () => { + expect(commit.calls.allArgs()).toEqual([ + [mutationTypes.APPLY_SUGGESTION, { discussionId, noteId, suggestionId }], + ]); + + expect(dispatch.calls.allArgs()).toEqual([['resolveDiscussion', { discussionId }]]); + expect(flashSpy).not.toHaveBeenCalled(); + }); + }); + + it('when service fails, flashes error message', done => { + const response = { response: { data: { message: TEST_ERROR_MESSAGE } } }; + + service.applySuggestion.and.returnValue(Promise.reject(response)); + + testSubmitSuggestion(done, () => { + expect(commit).not.toHaveBeenCalled(); + expect(dispatch).not.toHaveBeenCalled(); + expect(flashSpy).toHaveBeenCalledWith(`${TEST_ERROR_MESSAGE}.`, 'alert', flashContainer); + }); + }); + + it('when resolve discussion fails, fail gracefully', done => { + dispatch.and.returnValue(Promise.reject()); + + testSubmitSuggestion(done, () => { + expect(flashSpy).not.toHaveBeenCalled(); + }); + }); + }); }); 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 deleted file mode 100644 index 12ee804f668..00000000000 --- a/spec/javascripts/vue_shared/components/markdown/suggestion_diff_header_spec.js +++ /dev/null @@ -1,75 +0,0 @@ -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 a help button', () => { - const helpBtn = vm.$el.querySelector('.js-help-btn'); - - expect(helpBtn).not.toBeNull(); - }); - - 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(); - }); - }); -}); |