summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorPhil Hughes <me@iamphill.com>2019-05-07 15:26:42 +0000
committerPhil Hughes <me@iamphill.com>2019-05-07 15:26:42 +0000
commit91acefb1f4ee4f747d4679d9c395a8c8dc33d6de (patch)
tree3ad8e6f5d0237539b51246cdda1e7c9cffa8ccb0
parent3e6e02a1d771d25d90da187ba414a434807a5b51 (diff)
parentd24d77a93a64e4698163c5ca579954f61ad9248a (diff)
downloadgitlab-ce-91acefb1f4ee4f747d4679d9c395a8c8dc33d6de.tar.gz
Merge branch '54405-resolve-discussion-when-applying-a-suggested-change' into 'master'
Resolve "Resolve discussion when suggestion is applied" Closes #54405 See merge request gitlab-org/gitlab-ce!28160
-rw-r--r--app/assets/javascripts/notes/components/note_body.vue6
-rw-r--r--app/assets/javascripts/notes/stores/actions.js31
-rw-r--r--app/assets/javascripts/vue_shared/components/markdown/suggestion_diff_header.vue20
-rw-r--r--changelogs/unreleased/54405-resolve-discussion-when-applying-a-suggested-change.yml5
-rw-r--r--doc/user/discussions/index.md9
-rw-r--r--locale/gitlab.pot6
-rw-r--r--spec/features/merge_request/user_suggests_changes_on_diff_spec.rb22
-rw-r--r--spec/frontend/vue_shared/components/markdown/suggestion_diff_header_spec.js103
-rw-r--r--spec/javascripts/notes/stores/actions_spec.js130
-rw-r--r--spec/javascripts/vue_shared/components/markdown/suggestion_diff_header_spec.js75
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 9327865c682..5d69efc3600 100644
--- a/doc/user/discussions/index.md
+++ b/doc/user/discussions/index.md
@@ -396,13 +396,10 @@ the Merge Request authored by the user that applied them.
![Apply suggestions](img/suggestion.png)
- > **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 05e56f2b89d..5aa048c28a3 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -1027,6 +1027,9 @@ msgstr ""
msgid "Applying multiple commands"
msgstr ""
+msgid "Applying suggestion"
+msgstr ""
+
msgid "Apr"
msgstr ""
@@ -9586,6 +9589,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();
- });
- });
-});