Merge branch '220867-prevent-editing-closed-health-status' into 'master'

Prevent editing closed health status

See merge request gitlab-org/gitlab!38372

app/views/shared/issuable/_close_reopen_report_toggle.html.haml

@@ -9,7 +9,7 @@
   - add_blocked_class = !issuable.closed? && warn_before_close
 
 .float-left.btn-group.gl-ml-3.issuable-close-dropdown.droplab-dropdown.js-issuable-close-dropdown
-  %button{ class: "#{button_class} btn-#{button_action} #{(add_blocked_class ? 'btn-issue-blocked' : '')}", data: { qa_selector: 'close_issue_button', endpoint: close_reopen_issuable_path(issuable) } }
+  %button{ class: "#{button_class} btn-#{button_action} #{(add_blocked_class ? 'btn-issue-blocked' : '')}", data: { testid: 'close-issue-button', qa_selector: 'close_issue_button', endpoint: close_reopen_issuable_path(issuable) } }
     #{display_button_action} #{display_issuable_type}
 
   = button_tag type: 'button', class: "#{toggle_class} btn-#{button_action}-color",

doc/user/project/issues/index.md

@@ -186,8 +186,8 @@ requires [GraphQL](../../../api/graphql/index.md) to be enabled.
 
 ### Health status **(ULTIMATE)**
 
-> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/36427) in [GitLab Ultimate](https://about.gitlab.com/pricing/) 12.10.
-
+> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/36427) in [GitLab Ultimate](https://about.gitlab.com/pricing/) 12.10.
+> - Health status of closed issues [can't be edited](https://gitlab.com/gitlab-org/gitlab/-/issues/220867) in [GitLab Ultimate](https://about.gitlab.com/pricing/) 13.4 and later.
 To help you track the status of your issues, you can assign a status to each issue to flag work
 that's progressing as planned or needs attention to keep on schedule:
 
@@ -197,6 +197,9 @@ that's progressing as planned or needs attention to keep on schedule:
 
 !["On track" health status on an issue](img/issue_health_status_dropdown_v12_10.png)
 
+After an issue is closed, its health status can't be edited and the "Edit" button becomes disabled
+until the issue is reopened.
+
 You can then see issue statuses on the
 [Epic tree](../../group/epics/index.md#issue-health-status-in-epic-tree-ultimate).
 

ee/app/assets/javascripts/sidebar/components/status/sidebar_status.vue

 <script>
+import { mapGetters } from 'vuex';
 import { deprecatedCreateFlash as Flash } from '~/flash';
 import { __ } from '~/locale';
 import Status from './status.vue';
+import { OPENED, REOPENED } from '~/notes/constants';
 
 export default {
   components: {
@@ -16,6 +18,12 @@ export default {
       },
     },
   },
+  computed: {
+    ...mapGetters(['getNoteableData']),
+    isOpen() {
+      return this.getNoteableData.state === OPENED || this.getNoteableData.state === REOPENED;
+    },
+  },
   methods: {
     handleDropdownClick(status) {
       this.mediator.updateStatus(status).catch(() => {
@@ -28,7 +36,7 @@ export default {
 
 <template>
   <status
-    :is-editable="mediator.store.editable"
+    :is-editable="mediator.store.editable && isOpen"
     :is-fetching="mediator.store.isFetching.status"
     :status="mediator.store.status"
     @onDropdownClick="handleDropdownClick"

ee/app/assets/javascripts/sidebar/components/status/status.vue

@@ -3,24 +3,26 @@ import {
   GlIcon,
   GlDeprecatedButton as GlButton,
   GlLoadingIcon,
-  GlTooltip,
-  GlDeprecatedDropdownItem,
-  GlDeprecatedDropdown,
-  GlDeprecatedDropdownDivider,
+  GlTooltipDirective as GlTooltip,
+  GlDropdownItem,
+  GlDropdown,
+  GlDropdownDivider,
 } from '@gitlab/ui';
 import Tracking from '~/tracking';
 import { s__ } from '~/locale';
 import { healthStatusTextMap } from '../../constants';
 
 export default {
+  directives: {
+    GlTooltip,
+  },
   components: {
     GlIcon,
     GlButton,
     GlLoadingIcon,
-    GlTooltip,
-    GlDeprecatedDropdown,
-    GlDeprecatedDropdownItem,
-    GlDeprecatedDropdownDivider,
+    GlDropdown,
+    GlDropdownItem,
+    GlDropdownDivider,
   },
   mixins: [Tracking.mixin()],
   props: {
@@ -60,14 +62,26 @@ export default {
     dropdownText() {
       return this.status ? healthStatusTextMap[this.status] : s__('Select health status');
     },
-    tooltipText() {
+    statusTooltip() {
       let tooltipText = s__('Sidebar|Health status');
 
       if (this.status) {
         tooltipText += `: ${this.statusText}`;
       }
 
-      return tooltipText;
+      return {
+        title: tooltipText,
+      };
+    },
+    editTooltip() {
+      const tooltipText = !this.isEditable
+        ? s__('Health status cannot be edited because this issue is closed')
+        : '';
+
+      return {
+        title: tooltipText,
+        offset: -80,
+      };
     },
   },
   watch: {
@@ -75,6 +89,12 @@ export default {
       this.selectedStatus = status;
     },
   },
+  mounted() {
+    document.addEventListener('click', this.handleOffClick);
+  },
+  beforeDestroy() {
+    document.removeEventListener('click', this.handleOffClick);
+  },
   methods: {
     handleDropdownClick(status) {
       this.selectedStatus = status;
@@ -94,7 +114,7 @@ export default {
        */
       const { dropdown } = this.$refs.dropdown.$refs;
 
-      if (dropdown && this.isDropdownShowing) {
+      if (dropdown && !this.isDropdownShowing) {
         dropdown.show();
       }
     },
@@ -104,44 +124,51 @@ export default {
     isSelected(status) {
       return this.status === status;
     },
+    handleOffClick(event) {
+      if (!this.isDropdownShowing) return;
+
+      if (!this.$refs.dropdown.$el.contains(event.target)) {
+        this.toggleFormDropdown();
+      }
+    },
   },
 };
 </script>
 
 <template>
   <div class="block health-status">
-    <div ref="status" class="sidebar-collapsed-icon">
+    <div ref="status" v-gl-tooltip.left="statusTooltip" class="sidebar-collapsed-icon">
       <gl-icon name="status-health" :size="14" />
 
       <gl-loading-icon v-if="isFetching" />
-      <p v-else class="collapse-truncated-title px-1">{{ statusText }}</p>
+      <p v-else class="collapse-truncated-title gl-px-2">{{ statusText }}</p>
     </div>
-    <gl-tooltip :target="() => $refs.status" boundary="viewport" placement="left">
-      {{ tooltipText }}
-    </gl-tooltip>
 
     <div class="hide-collapsed">
-      <p class="title d-flex justify-content-between">
-        {{ s__('Sidebar|Health status') }}
-        <a
-          v-if="isEditable"
-          ref="editButton"
-          class="btn-link"
-          href="#"
-          @click="toggleFormDropdown"
-          @keydown.esc="hideDropdown"
-        >
-          {{ __('Edit') }}
-        </a>
+      <p class="title gl-display-flex justify-content-between">
+        <span data-testid="statusTitle">{{ s__('Sidebar|Health status') }}</span>
+        <span v-gl-tooltip.topleft="editTooltip" data-testid="editButtonTooltip" tabindex="0">
+          <gl-button
+            ref="editButton"
+            variant="link"
+            class="edit-link btn-link-hover"
+            :disabled="!isEditable"
+            @click.stop="toggleFormDropdown"
+            @keydown.esc="hideDropdown"
+          >
+            {{ __('Edit') }}
+          </gl-button>
+        </span>
       </p>
 
       <div
+        data-testid="dropdownWrapper"
         class="dropdown dropdown-menu-selectable"
-        :class="{ show: isDropdownShowing, 'd-none': !isDropdownShowing }"
+        :class="{ show: isDropdownShowing, 'gl-display-none': !isDropdownShowing }"
       >
-        <gl-deprecated-dropdown
+        <gl-dropdown
           ref="dropdown"
-          class="w-100"
+          class="gl-w-full"
           :text="dropdownText"
           @keydown.esc.native="hideDropdown"
           @hide="hideDropdown"
@@ -158,7 +185,7 @@ export default {
           </div>
 
           <div class="dropdown-content dropdown-body">
-            <gl-deprecated-dropdown-item @click="handleDropdownClick(null)">
+            <gl-dropdown-item @click="handleDropdownClick(null)">
               <gl-button
                 variant="link"
                 class="dropdown-item health-dropdown-item"
@@ -166,11 +193,11 @@ export default {
               >
                 {{ s__('Sidebar|No status') }}
               </gl-button>
-            </gl-deprecated-dropdown-item>
+            </gl-dropdown-item>
 
-            <gl-deprecated-dropdown-divider class="divider health-divider" />
+            <gl-dropdown-divider class="divider health-divider" />
 
-            <gl-deprecated-dropdown-item
+            <gl-dropdown-item
               v-for="option in statusOptions"
               :key="option.key"
               @click="handleDropdownClick(option.key)"
@@ -182,14 +209,14 @@ export default {
               >
                 {{ option.value }}
               </gl-button>
-            </gl-deprecated-dropdown-item>
+            </gl-dropdown-item>
           </div>
-        </gl-deprecated-dropdown>
+        </gl-dropdown>
       </div>
 
       <gl-loading-icon v-if="isFetching" :inline="true" />
-      <p v-else-if="!isDropdownShowing" class="value m-0" :class="{ 'no-value': !status }">
-        <span v-if="status" class="text-plain bold">{{ statusText }}</span>
+      <p v-else-if="!isDropdownShowing" class="value gl-m-0" :class="{ 'no-value': !status }">
+        <span v-if="status" class="text-plain gl-font-weight-bold">{{ statusText }}</span>
         <span v-else>{{ __('None') }}</span>
       </p>
     </div>

ee/app/assets/javascripts/sidebar/mount_sidebar.js

@@ -8,6 +8,7 @@ import SidebarWeight from './components/weight/sidebar_weight.vue';
 import IterationSelect from './components/iteration_select.vue';
 import SidebarStore from './stores/sidebar_store';
 import createDefaultClient from '~/lib/graphql';
+import { store } from '~/notes/stores';
 
 Vue.use(VueApollo);
 
@@ -39,6 +40,7 @@ const mountStatusComponent = mediator => {
 
   return new Vue({
     el,
+    store,
     components: {
       SidebarStatus,
     },

ee/changelogs/unreleased/220867-prevent-editing-closed-health-status.yml

+---
+title: Disable editing health status for closed issues and show informative tooltip
+merge_request: 38372
+author:
+type: changed

ee/spec/features/issues/issue_sidebar_spec.rb

@@ -84,13 +84,25 @@ RSpec.describe 'Issue Sidebar' do
     end
 
     context 'when health status feature is available' do
-      it 'shows health status on sidebar' do
+      before do
         stub_licensed_features(issuable_health_status: true)
 
         visit_issue(project, issue)
+      end
 
+      it 'shows health status on sidebar' do
         expect(page).to have_selector('.block.health-status')
       end
+
+      context 'when user closes an issue' do
+        it 'disables the edit button' do
+          page.find('[data-testid="close-issue-button"]').click
+
+          page.within('.health-status') do
+            expect(page).to have_button('Edit', disabled: true)
+          end
+        end
+      end
     end
 
     context 'when health status feature is not available' do

ee/spec/frontend/sidebar/components/status/sidebar_status_spec.js

@@ -3,21 +3,23 @@ import SidebarStatus from 'ee/sidebar/components/status/sidebar_status.vue';
 import Status from 'ee/sidebar/components/status/status.vue';
 
 describe('SidebarStatus', () => {
+  let mediator;
   let wrapper;
   let handleDropdownClickMock;
 
-  beforeEach(() => {
-    const mediator = {
+  const createMediator = states => {
+    mediator = {
       store: {
         isFetching: {
           status: true,
         },
         status: '',
+        ...states,
       },
     };
+  };
 
-    handleDropdownClickMock = jest.fn();
-
+  const createWrapper = (mockStore = {}) => {
     wrapper = shallowMount(SidebarStatus, {
       propsData: {
         mediator,
@@ -25,7 +27,16 @@ describe('SidebarStatus', () => {
       methods: {
         handleDropdownClick: handleDropdownClickMock,
       },
+      mocks: {
+        $store: mockStore,
+      },
     });
+  };
+
+  beforeEach(() => {
+    handleDropdownClickMock = jest.fn();
+    createMediator();
+    createWrapper();
   });
 
   afterEach(() => {
@@ -33,6 +44,31 @@ describe('SidebarStatus', () => {
     wrapper = null;
   });
 
+  describe('computed', () => {
+    describe.each`
+      noteableState | isOpen
+      ${'opened'}   | ${true}
+      ${'reopened'} | ${true}
+      ${'closed'}   | ${false}
+    `('isOpen', ({ noteableState, isOpen }) => {
+      beforeEach(() => {
+        const mockStore = {
+          getters: {
+            getNoteableData: {
+              state: noteableState,
+            },
+          },
+        };
+        createMediator({ editable: true });
+        createWrapper(mockStore);
+      });
+
+      it(`returns ${isOpen} when issue is ${noteableState}`, () => {
+        expect(wrapper.vm.isOpen).toBe(isOpen);
+      });
+    });
+  });
+
   describe('Status child component', () => {
     beforeEach(() => {});
 

ee/spec/frontend/sidebar/components/status/status_spec.js

-import {
-  GlDeprecatedDropdown,
-  GlDeprecatedDropdownItem,
-  GlLoadingIcon,
-  GlTooltip,
-} from '@gitlab/ui';
+import { GlDropdown, GlDropdownItem, GlLoadingIcon } from '@gitlab/ui';
 import { mount, shallowMount } from '@vue/test-utils';
+import { createMockDirective, getBinding } from 'helpers/vue_mock_directive';
 import Vue from 'vue';
 import Status from 'ee/sidebar/components/status/status.vue';
 import { healthStatus, healthStatusTextMap } from 'ee/sidebar/constants';
 
 const getStatusText = wrapper => wrapper.find('.value .text-plain').text();
 
-const getTooltipText = wrapper => wrapper.find(GlTooltip).text();
+const getStatusTitleText = wrapper => wrapper.find('[data-testid="statusTitle"]').text();
+
+const getStatusTooltipValue = wrapper =>
+  getBinding(wrapper.find({ ref: 'status' }).element, 'gl-tooltip').value;
+
+const getEditButtonTooltipValue = wrapper =>
+  getBinding(wrapper.find('[data-testid="editButtonTooltip"]').element, 'gl-tooltip').value;
 
 const getEditButton = wrapper => wrapper.find({ ref: 'editButton' });
 
-const getDropdownElement = wrapper => wrapper.find(GlDeprecatedDropdown);
+const getDropdownClasses = wrapper => wrapper.find('[data-testid="dropdownWrapper"]').classes();
 
-const getRemoveStatusItem = wrapper => wrapper.find(GlDeprecatedDropdownItem);
+const getDropdownElement = wrapper => wrapper.find(GlDropdown);
+
+const getRemoveStatusItem = wrapper => wrapper.find(GlDropdownItem);
 
 describe('Status', () => {
   let wrapper;
@@ -25,12 +29,18 @@ describe('Status', () => {
   function shallowMountStatus(propsData) {
     wrapper = shallowMount(Status, {
       propsData,
+      directives: {
+        GlTooltip: createMockDirective(),
+      },
     });
   }
 
   function mountStatus(propsData) {
     wrapper = mount(Status, {
       propsData,
+      directives: {
+        GlTooltip: createMockDirective(),
+      },
     });
   }
 
@@ -40,7 +50,7 @@ describe('Status', () => {
 
   it('shows the text "Status"', () => {
     shallowMountStatus();
-    expect(wrapper.find('.title').text()).toBe('Health status');
+    expect(getStatusTitleText(wrapper)).toBe('Health status');
   });
 
   describe('loading icon', () => {
@@ -76,14 +86,22 @@ describe('Status', () => {
       expect(getEditButton(wrapper).exists()).toBe(true);
     });
 
-    it('is hidden when user cannot edit', () => {
-      const props = {
-        isEditable: false,
-      };
+    describe('when disabled', () => {
+      beforeEach(() => {
+        const props = {
+          isEditable: false,
+        };
 
-      shallowMountStatus(props);
+        shallowMountStatus(props);
+      });
+      it('is disabled when user cannot edit', () => {
+        expect(getEditButton(wrapper).attributes().disabled).toBe('true');
+      });
 
-      expect(getEditButton(wrapper).exists()).toBe(false);
+      it('will render a tooltip with an informative message', () => {
+        const tooltipTitle = 'Health status cannot be edited because this issue is closed';
+        expect(getEditButtonTooltipValue(wrapper).title).toBe(tooltipTitle);
+      });
     });
   });
 
@@ -136,7 +154,7 @@ describe('Status', () => {
       });
 
       it('shows "Status" in the tooltip', () => {
-        expect(getTooltipText(wrapper)).toBe('Health status');
+        expect(getStatusTooltipValue(wrapper).title).toBe('Health status');
       });
     });
 
@@ -154,7 +172,9 @@ describe('Status', () => {
       });
 
       it(`shows "Status: ${healthStatusTextMap[statusValue]}" in the tooltip`, () => {
-        expect(getTooltipText(wrapper)).toBe(`Health status: ${healthStatusTextMap[statusValue]}`);
+        expect(getStatusTooltipValue(wrapper).title).toBe(
+          `Health status: ${healthStatusTextMap[statusValue]}`,
+        );
       });
     });
   });
@@ -169,7 +189,7 @@ describe('Status', () => {
 
       const dropdown = wrapper.find('.dropdown');
 
-      expect(dropdown.classes()).toContain('d-none');
+      expect(dropdown.classes()).toContain('gl-display-none');
     });
 
     describe('when hidden', () => {
@@ -185,7 +205,7 @@ describe('Status', () => {
         getEditButton(wrapper).trigger('click');
 
         return Vue.nextTick().then(() => {
-          expect(wrapper.find('.dropdown').classes()).toContain('show');
+          expect(getDropdownClasses(wrapper)).toContain('show');
         });
       });
     });
@@ -196,7 +216,7 @@ describe('Status', () => {
           isEditable: true,
         };
 
-        shallowMountStatus(props);
+        mountStatus(props);
 
         wrapper.setData({ isDropdownShowing: true });
       });
@@ -215,17 +235,17 @@ describe('Status', () => {
         getEditButton(wrapper).trigger('click');
 
         return Vue.nextTick().then(() => {
-          expect(wrapper.find('.dropdown').classes()).toContain('d-none');
+          expect(getDropdownClasses(wrapper)).toContain('gl-display-none');
         });
       });
 
       it('hides form when a dropdown item is clicked', () => {
-        const dropdownItem = wrapper.findAll(GlDeprecatedDropdownItem).at(1);
+        const dropdownItem = wrapper.findAll(GlDropdownItem).at(1);
 
         dropdownItem.vm.$emit('click');
 
         return wrapper.vm.$nextTick().then(() => {
-          expect(wrapper.find('.dropdown').classes()).toContain('d-none');
+          expect(getDropdownClasses(wrapper)).toContain('gl-display-none');
         });
       });
     });
@@ -246,7 +266,7 @@ describe('Status', () => {
       });
 
       it('shows 4 dropdown items', () => {
-        expect(wrapper.findAll(GlDeprecatedDropdownItem)).toHaveLength(4);
+        expect(wrapper.findAll(GlDropdownItem)).toHaveLength(4);
       });
 
       // Test that "On track", "Needs attention", and "At risk" are displayed
@@ -255,7 +275,7 @@ describe('Status', () => {
         (statusText, index) => {
           expect(
             wrapper
-              .findAll(GlDeprecatedDropdownItem)
+              .findAll(GlDropdownItem)
               .at(index + 1) // +1 in index to account for 1st item as `No status`
               .text(),
           ).toContain(statusText);
@@ -267,7 +287,7 @@ describe('Status', () => {
         'emits onFormSubmit event with argument "%s" when user selects the option and submits form',
         (status, index) => {
           wrapper
-            .findAll(GlDeprecatedDropdownItem)
+            .findAll(GlDropdownItem)
             .at(index + 1)
             .vm.$emit('click', { preventDefault: () => null });
 

locale/gitlab.pot

@@ -12451,6 +12451,9 @@ msgstr ""
 msgid "Health status"
 msgstr ""
 
+msgid "Health status cannot be edited because this issue is closed"
+msgstr ""
+
 msgid "HealthCheck|Access token is"
 msgstr ""