Merge branch '292234-unable-to-close-epics-using-the-comment-button' into 'master'

Fix `Close epic` button not working on epic page

See merge request gitlab-org/gitlab!49597

app/assets/javascripts/notes/components/comment_form.vue

@@ -138,8 +138,8 @@ export default {
         ? __('merge request')
         : __('issue');
     },
-    isMergeRequest() {
-      return this.noteableType === constants.MERGE_REQUEST_NOTEABLE_TYPE;
+    isIssue() {
+      return this.noteableDisplayName === constants.ISSUE_NOTEABLE_TYPE;
     },
     trackingLabel() {
       return slugifyWithUnderscore(`${this.commentButtonTitle} button`);
@@ -167,8 +167,8 @@ export default {
       'stopPolling',
       'restartPolling',
       'removePlaceholderNotes',
-      'closeMergeRequest',
-      'reopenMergeRequest',
+      'closeIssuable',
+      'reopenIssuable',
       'toggleIssueLocalState',
     ]),
     setIsSubmitButtonDisabled(note, isSubmitting) {
@@ -229,22 +229,18 @@ export default {
       }
     },
     toggleIssueState() {
-      if (!this.isMergeRequest) {
+      if (this.isIssue) {
+        // We want to invoke the close/reopen logic in the issue header
+        // since that is where the blocked-by issues modal logic is also defined
         eventHub.$emit('toggle.issuable.state');
         return;
       }
 
-      const toggleMergeRequestState = this.isOpen
-        ? this.closeMergeRequest
-        : this.reopenMergeRequest;
+      const toggleState = this.isOpen ? this.closeIssuable : this.reopenIssuable;
 
-      const errorMessage = this.isOpen
-        ? __('Something went wrong while closing the merge request. Please try again later')
-        : __('Something went wrong while reopening the merge request. Please try again later');
-
-      toggleMergeRequestState()
+      toggleState()
         .then(refreshUserMergeRequestCounts)
-        .catch(() => Flash(errorMessage));
+        .catch(() => Flash(constants.toggleStateErrorMessage[this.noteableType][this.openState]));
     },
     discard(shouldClear = true) {
       // `blur` is needed to clear slash commands autocomplete cache if event fired.

app/assets/javascripts/notes/constants.js

+import { __ } from '~/locale';
+
 export const DISCUSSION_NOTE = 'DiscussionNote';
 export const DIFF_NOTE = 'DiffNote';
 export const DISCUSSION = 'discussion';
@@ -36,3 +38,16 @@ export const DISCUSSION_FILTER_TYPES = {
   COMMENTS: 'comments',
   HISTORY: 'history',
 };
+
+export const toggleStateErrorMessage = {
+  Epic: {
+    [CLOSED]: __('Something went wrong while reopening the epic. Please try again later.'),
+    [OPENED]: __('Something went wrong while closing the epic. Please try again later.'),
+    [REOPENED]: __('Something went wrong while closing the epic. Please try again later.'),
+  },
+  MergeRequest: {
+    [CLOSED]: __('Something went wrong while reopening the merge request. Please try again later.'),
+    [OPENED]: __('Something went wrong while closing the merge request. Please try again later.'),
+    [REOPENED]: __('Something went wrong while closing the merge request. Please try again later.'),
+  },
+};

app/assets/javascripts/notes/stores/actions.js

@@ -244,7 +244,7 @@ export const toggleResolveNote = ({ commit, dispatch }, { endpoint, isResolved,
   });
 };
 
-export const closeMergeRequest = ({ commit, dispatch, state }) => {
+export const closeIssuable = ({ commit, dispatch, state }) => {
   dispatch('toggleStateButtonLoading', true);
   return axios.put(state.notesData.closePath).then(({ data }) => {
     commit(types.CLOSE_ISSUE);
@@ -253,7 +253,7 @@ export const closeMergeRequest = ({ commit, dispatch, state }) => {
   });
 };
 
-export const reopenMergeRequest = ({ commit, dispatch, state }) => {
+export const reopenIssuable = ({ commit, dispatch, state }) => {
   dispatch('toggleStateButtonLoading', true);
   return axios.put(state.notesData.reopenPath).then(({ data }) => {
     commit(types.REOPEN_ISSUE);

ee/changelogs/unreleased/292234-unable-to-close-epics-using-the-comment-button.yml

+---
+title: Fix `Close epic` button not working on epic page
+merge_request: 49597
+author:
+type: fixed

ee/spec/features/epics/epic_show_spec.rb

@@ -31,11 +31,13 @@ RSpec.describe 'Epic show', :js do
     group.add_developer(user)
     stub_licensed_features(epics: true, subepics: true)
     sign_in(user)
-
-    visit group_epic_path(group, epic)
   end
 
   describe 'when sub-epics feature is available' do
+    before do
+      visit group_epic_path(group, epic)
+    end
+
     describe 'Epic metadata' do
       it 'shows epic tabs `Epics and Issues` and `Roadmap`' do
         page.within('.js-epic-tabs-container') do
@@ -89,8 +91,6 @@ RSpec.describe 'Epic show', :js do
 
   describe 'when sub-epics feature not is available' do
     before do
-      stub_licensed_features(epics: true, subepics: false)
-
       visit group_epic_path(group, epic)
     end
 
@@ -116,6 +116,10 @@ RSpec.describe 'Epic show', :js do
   end
 
   describe 'Epic metadata' do
+    before do
+      visit group_epic_path(group, epic)
+    end
+
     it_behaves_like 'page meta description', 'Lorem ipsum dolor sit amet, consectetur adipiscing elit. Nos commodius agimus. Ex rebus enim timiditas, non ex vocabulis nascitur. Ita prorsus, inquam; Duo...'
 
     it 'shows epic status, date and author in header' do
@@ -175,6 +179,10 @@ RSpec.describe 'Epic show', :js do
   end
 
   describe 'Epic sidebar' do
+    before do
+      visit group_epic_path(group, epic)
+    end
+
     describe 'Labels select' do
       it 'opens dropdown when `Edit` is clicked' do
         page.within('aside.right-sidebar') do
@@ -257,4 +265,74 @@ RSpec.describe 'Epic show', :js do
       end
     end
   end
+
+  describe 'epic actions' do
+    shared_examples 'epic closed' do |selector|
+      it 'can close an epic' do
+        expect(find('.status-box')).to have_content 'Open'
+
+        within selector do
+          click_button 'Close epic'
+        end
+
+        expect(find('.status-box')).to have_content 'Closed'
+      end
+    end
+
+    shared_examples 'epic reopened' do |selector|
+      it 'can reopen an epic' do
+        expect(find('.status-box')).to have_content 'Closed'
+
+        within selector do
+          click_button 'Reopen epic'
+        end
+
+        expect(find('.status-box')).to have_content 'Open'
+      end
+    end
+
+    describe 'when open' do
+      context 'when clicking the top `Close epic` button', :aggregate_failures do
+        let(:open_epic) { create(:epic, group: group) }
+
+        before do
+          visit group_epic_path(group, open_epic)
+        end
+
+        it_behaves_like 'epic closed', '.detail-page-header'
+      end
+
+      context 'when clicking the bottom `Close epic` button', :aggregate_failures do
+        let(:open_epic) { create(:epic, group: group) }
+
+        before do
+          visit group_epic_path(group, open_epic)
+        end
+
+        it_behaves_like 'epic closed', '.timeline-content-form'
+      end
+    end
+
+    describe 'when closed' do
+      context 'when clicking the top `Reopen epic` button', :aggregate_failures do
+        let(:closed_epic) { create(:epic, group: group, state: 'closed') }
+
+        before do
+          visit group_epic_path(group, closed_epic)
+        end
+
+        it_behaves_like 'epic reopened', '.detail-page-header'
+      end
+
+      context 'when clicking the bottom `Reopen epic` button', :aggregate_failures do
+        let(:closed_epic) { create(:epic, group: group, state: 'closed') }
+
+        before do
+          visit group_epic_path(group, closed_epic)
+        end
+
+        it_behaves_like 'epic reopened', '.timeline-content-form'
+      end
+    end
+  end
 end

locale/gitlab.pot

@@ -25688,7 +25688,10 @@ msgstr ""
 msgid "Something went wrong while archiving a requirement."
 msgstr ""
 
-msgid "Something went wrong while closing the merge request. Please try again later"
+msgid "Something went wrong while closing the epic. Please try again later."
+msgstr ""
+
+msgid "Something went wrong while closing the merge request. Please try again later."
 msgstr ""
 
 msgid "Something went wrong while creating a requirement."
@@ -25775,7 +25778,10 @@ msgstr ""
 msgid "Something went wrong while reopening a requirement."
 msgstr ""
 
-msgid "Something went wrong while reopening the merge request. Please try again later"
+msgid "Something went wrong while reopening the epic. Please try again later."
+msgstr ""
+
+msgid "Something went wrong while reopening the merge request. Please try again later."
 msgstr ""
 
 msgid "Something went wrong while resolving this discussion. Please try again."

spec/features/issues/issue_state_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'issue state', :js do
+  let_it_be(:project) { create(:project) }
+  let_it_be(:user) { create(:user) }
+
+  before do
+    project.add_developer(user)
+    sign_in(user)
+  end
+
+  shared_examples 'issue closed' do |selector|
+    it 'can close an issue' do
+      expect(find('.status-box')).to have_content 'Open'
+
+      within selector do
+        click_button 'Close issue'
+      end
+
+      expect(find('.status-box')).to have_content 'Closed'
+    end
+  end
+
+  shared_examples 'issue reopened' do |selector|
+    it 'can reopen an issue' do
+      expect(find('.status-box')).to have_content 'Closed'
+
+      within selector do
+        click_button 'Reopen issue'
+      end
+
+      expect(find('.status-box')).to have_content 'Open'
+    end
+  end
+
+  describe 'when open' do
+    context 'when clicking the top `Close issue` button', :aggregate_failures do
+      let(:open_issue) { create(:issue, project: project) }
+
+      before do
+        visit project_issue_path(project, open_issue)
+      end
+
+      it_behaves_like 'issue closed', '.detail-page-header'
+    end
+
+    context 'when clicking the bottom `Close issue` button', :aggregate_failures do
+      let(:open_issue) { create(:issue, project: project) }
+
+      before do
+        visit project_issue_path(project, open_issue)
+      end
+
+      it_behaves_like 'issue closed', '.timeline-content-form'
+    end
+  end
+
+  describe 'when closed' do
+    context 'when clicking the top `Reopen issue` button', :aggregate_failures do
+      let(:closed_issue) { create(:issue, project: project, state: 'closed') }
+
+      before do
+        visit project_issue_path(project, closed_issue)
+      end
+
+      it_behaves_like 'issue reopened', '.detail-page-header'
+    end
+
+    context 'when clicking the bottom `Reopen issue` button', :aggregate_failures do
+      let(:closed_issue) { create(:issue, project: project, state: 'closed') }
+
+      before do
+        visit project_issue_path(project, closed_issue)
+      end
+
+      it_behaves_like 'issue reopened', '.timeline-content-form'
+    end
+  end
+end

spec/features/merge_request/user_closes_merge_request_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe 'User closes a merge requests', :js do
-  let(:project) { create(:project, :repository) }
-  let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
-  let(:user) { create(:user) }
-
-  before do
-    project.add_maintainer(user)
-    sign_in(user)
-
-    visit(merge_request_path(merge_request))
-  end
-
-  it 'closes a merge request' do
-    click_button('Close merge request', match: :first)
-
-    expect(page).to have_content(merge_request.title)
-    expect(page).to have_content('Closed by')
-  end
-end

spec/features/merge_request/user_closes_reopens_merge_request_state_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'User closes/reopens a merge request', :js do
+  let_it_be(:project) { create(:project, :repository) }
+  let_it_be(:user) { create(:user) }
+
+  before do
+    project.add_developer(user)
+    sign_in(user)
+  end
+
+  describe 'when open' do
+    context 'when clicking the top `Close merge request` link', :aggregate_failures do
+      let(:open_merge_request) { create(:merge_request, source_project: project, target_project: project) }
+
+      before do
+        visit merge_request_path(open_merge_request)
+      end
+
+      it 'can close a merge request' do
+        expect(find('.status-box')).to have_content 'Open'
+
+        within '.detail-page-header' do
+          click_button 'Toggle dropdown'
+          click_link 'Close merge request'
+        end
+
+        wait_for_requests
+
+        expect(find('.status-box')).to have_content 'Closed'
+      end
+    end
+
+    context 'when clicking the bottom `Close merge request` button', :aggregate_failures do
+      let(:open_merge_request) { create(:merge_request, source_project: project, target_project: project) }
+
+      before do
+        visit merge_request_path(open_merge_request)
+      end
+
+      it 'can close a merge request' do
+        expect(find('.status-box')).to have_content 'Open'
+
+        within '.timeline-content-form' do
+          click_button 'Close merge request'
+
+          # Clicking the bottom `Close merge request` button does not yet update
+          # the header status so for now we'll check that the button text changes
+          expect(page).not_to have_button 'Close merge request'
+          expect(page).to have_button 'Reopen merge request'
+        end
+      end
+    end
+  end
+
+  describe 'when closed' do
+    context 'when clicking the top `Reopen merge request` link', :aggregate_failures do
+      let(:closed_merge_request) { create(:merge_request, source_project: project, target_project: project, state: 'closed') }
+
+      before do
+        visit merge_request_path(closed_merge_request)
+      end
+
+      it 'can reopen a merge request' do
+        expect(find('.status-box')).to have_content 'Closed'
+
+        within '.detail-page-header' do
+          click_button 'Toggle dropdown'
+          click_link 'Reopen merge request'
+        end
+
+        wait_for_requests
+
+        expect(find('.status-box')).to have_content 'Open'
+      end
+    end
+
+    context 'when clicking the bottom `Reopen merge request` button', :aggregate_failures do
+      let(:closed_merge_request) { create(:merge_request, source_project: project, target_project: project, state: 'closed') }
+
+      before do
+        visit merge_request_path(closed_merge_request)
+      end
+
+      it 'can reopen a merge request' do
+        expect(find('.status-box')).to have_content 'Closed'
+
+        within '.timeline-content-form' do
+          click_button 'Reopen merge request'
+
+          # Clicking the bottom `Reopen merge request` button does not yet update
+          # the header status so for now we'll check that the button text changes
+          expect(page).not_to have_button 'Reopen merge request'
+          expect(page).to have_button 'Close merge request'
+        end
+      end
+    end
+  end
+end

spec/features/merge_request/user_reopens_merge_request_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe 'User reopens a merge requests', :js do
-  let(:project) { create(:project, :public, :repository) }
-  let!(:merge_request) { create(:closed_merge_request, source_project: project, target_project: project) }
-  let(:user) { create(:user) }
-
-  before do
-    project.add_maintainer(user)
-    sign_in(user)
-
-    visit(merge_request_path(merge_request))
-  end
-
-  it 'reopens a merge request' do
-    find('.detail-page-header .dropdown-toggle').click
-
-    click_link('Reopen merge request', match: :first)
-
-    wait_for_requests
-
-    page.within('.status-box') do
-      expect(page).to have_content('Open')
-    end
-  end
-end

spec/frontend/notes/components/comment_form_spec.js

@@ -2,6 +2,7 @@ import { nextTick } from 'vue';
 import { mount, shallowMount } from '@vue/test-utils';
 import MockAdapter from 'axios-mock-adapter';
 import Autosize from 'autosize';
+import { deprecatedCreateFlash as flash } from '~/flash';
 import axios from '~/lib/utils/axios_utils';
 import createStore from '~/notes/stores';
 import CommentForm from '~/notes/components/comment_form.vue';
@@ -13,6 +14,7 @@ import { loggedOutnoteableData, notesDataMock, userDataMock, noteableDataMock }
 
 jest.mock('autosize');
 jest.mock('~/commons/nav/user_merge_requests');
+jest.mock('~/flash');
 jest.mock('~/gl_form');
 
 describe('issue_comment_form component', () => {
@@ -28,7 +30,7 @@ describe('issue_comment_form component', () => {
 
   const mountComponent = ({
     initialData = {},
-    noteableType = 'issue',
+    noteableType = 'Issue',
     noteableData = noteableDataMock,
     notesData = notesDataMock,
     userData = userDataMock,
@@ -278,52 +280,92 @@ describe('issue_comment_form component', () => {
           });
         });
 
-        describe('when merge request', () => {
+        describe.each`
+          type               | noteableType
+          ${'merge request'} | ${'MergeRequest'}
+          ${'epic'}          | ${'Epic'}
+        `('when $type', ({ type, noteableType }) => {
           describe('when open', () => {
-            it('makes an API call to open the merge request', () => {
+            it(`makes an API call to open it`, () => {
               mountComponent({
-                noteableType: constants.MERGE_REQUEST_NOTEABLE_TYPE,
+                noteableType,
                 noteableData: { ...noteableDataMock, state: constants.OPENED },
                 mountFunction: mount,
               });
 
-              jest.spyOn(wrapper.vm, 'closeMergeRequest').mockResolvedValue();
+              jest.spyOn(wrapper.vm, 'closeIssuable').mockResolvedValue();
 
               findCloseReopenButton().trigger('click');
 
-              expect(wrapper.vm.closeMergeRequest).toHaveBeenCalled();
+              expect(wrapper.vm.closeIssuable).toHaveBeenCalled();
+            });
+
+            it(`shows an error when the API call fails`, async () => {
+              mountComponent({
+                noteableType,
+                noteableData: { ...noteableDataMock, state: constants.OPENED },
+                mountFunction: mount,
+              });
+
+              jest.spyOn(wrapper.vm, 'closeIssuable').mockRejectedValue();
+
+              await findCloseReopenButton().trigger('click');
+
+              await wrapper.vm.$nextTick;
+
+              expect(flash).toHaveBeenCalledWith(
+                `Something went wrong while closing the ${type}. Please try again later.`,
+              );
             });
           });
 
           describe('when closed', () => {
-            it('makes an API call to close the merge request', () => {
+            it('makes an API call to close it', () => {
               mountComponent({
-                noteableType: constants.MERGE_REQUEST_NOTEABLE_TYPE,
+                noteableType,
                 noteableData: { ...noteableDataMock, state: constants.CLOSED },
                 mountFunction: mount,
               });
 
-              jest.spyOn(wrapper.vm, 'reopenMergeRequest').mockResolvedValue();
+              jest.spyOn(wrapper.vm, 'reopenIssuable').mockResolvedValue();
 
               findCloseReopenButton().trigger('click');
 
-              expect(wrapper.vm.reopenMergeRequest).toHaveBeenCalled();
+              expect(wrapper.vm.reopenIssuable).toHaveBeenCalled();
             });
           });
 
-          it('should update MR count', async () => {
+          it(`shows an error when the API call fails`, async () => {
             mountComponent({
-              noteableType: constants.MERGE_REQUEST_NOTEABLE_TYPE,
+              noteableType,
+              noteableData: { ...noteableDataMock, state: constants.CLOSED },
               mountFunction: mount,
             });
 
-            jest.spyOn(wrapper.vm, 'closeMergeRequest').mockResolvedValue();
+            jest.spyOn(wrapper.vm, 'reopenIssuable').mockRejectedValue();
 
             await findCloseReopenButton().trigger('click');
 
-            expect(refreshUserMergeRequestCounts).toHaveBeenCalled();
+            await wrapper.vm.$nextTick;
+
+            expect(flash).toHaveBeenCalledWith(
+              `Something went wrong while reopening the ${type}. Please try again later.`,
+            );
           });
         });
+
+        it('when merge request, should update MR count', async () => {
+          mountComponent({
+            noteableType: constants.MERGE_REQUEST_NOTEABLE_TYPE,
+            mountFunction: mount,
+          });
+
+          jest.spyOn(wrapper.vm, 'closeIssuable').mockResolvedValue();
+
+          await findCloseReopenButton().trigger('click');
+
+          expect(refreshUserMergeRequestCounts).toHaveBeenCalled();
+        });
       });
     });
 

spec/frontend/notes/stores/actions_spec.js

@@ -177,7 +177,7 @@ describe('Actions Notes Store', () => {
     describe('closeMergeRequest', () => {
       it('sets state as closed', done => {
         store
-          .dispatch('closeMergeRequest', { notesData: { closeIssuePath: '' } })
+          .dispatch('closeIssuable', { notesData: { closeIssuePath: '' } })
           .then(() => {
             expect(store.state.noteableData.state).toEqual('closed');
             expect(store.state.isToggleStateButtonLoading).toEqual(false);
@@ -190,7 +190,7 @@ describe('Actions Notes Store', () => {
     describe('reopenMergeRequest', () => {
       it('sets state as reopened', done => {
         store
-          .dispatch('reopenMergeRequest', { notesData: { reopenIssuePath: '' } })
+          .dispatch('reopenIssuable', { notesData: { reopenIssuePath: '' } })
           .then(() => {
             expect(store.state.noteableData.state).toEqual('reopened');
             expect(store.state.isToggleStateButtonLoading).toEqual(false);