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

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,
+      );
     },
   },
 };

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);

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>

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

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:**

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 ""
 

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,15 +162,6 @@ 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('.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
-
-      wait_for_requests
-
       page.within('.diff-discussions') do
         expect(page).not_to have_content('Applied')
 
@@ -178,5 +171,16 @@ describe 'User comments on a diff', :js do
         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('Unresolve discussion')
+
+        click_button('Apply suggestion')
+        wait_for_requests
+
+        expect(page).to have_content('Unresolve discussion')
+      end
+    end
   end
 end

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);
+    });
+  });
+});

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();
+      });
+    });
+  });
 });

spec/javascripts/vue_shared/components/markdown/suggestion_diff_header_spec.js

@@ -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();
-    });
-  });
-});