Add "Delete" group button to the groups dashboard

Group owners can now delete a group and its subgroups from the
parent group level. Until now, group owners had to go into each
individual group to delete them, which was timely and inefficient.

Group owners can now view all groups and delete them from a single
place. The groups dashboard now shows a "Delete" group button in
the dropdown to the group owners.

Changelog: added

app/assets/javascripts/groups/components/app.vue

@@ -41,6 +41,7 @@ export default {
   },
   data() {
     return {
+      isModalVisible: false,
       isLoading: true,
       isSearchEmpty: false,
       searchEmptyMessage: '',
@@ -101,6 +102,12 @@ export default {
     eventHub.$off(`${this.action}updateGroups`, this.updateGroups);
   },
   methods: {
+    hideModal() {
+      this.isModalVisible = false;
+    },
+    showModal() {
+      this.isModalVisible = true;
+    },
     fetchGroups({ parentId, page, filterGroupsBy, sortBy, archived, updatePagination }) {
       return this.service
         .getGroups(parentId, page, filterGroupsBy, sortBy, archived)
@@ -185,6 +192,7 @@ export default {
     showLeaveGroupModal(group, parentGroup) {
       this.targetGroup = group;
       this.targetParentGroup = parentGroup;
+      this.showModal();
     },
     leaveGroup() {
       this.targetGroup.isBeingRemoved = true;
@@ -256,10 +264,12 @@ export default {
     />
     <gl-modal
       modal-id="leave-group-modal"
+      :visible="isModalVisible"
       :title="__('Are you sure?')"
       :action-primary="primaryProps"
       :action-cancel="cancelProps"
       @primary="leaveGroup"
+      @hide="hideModal"
     >
       {{ groupLeaveConfirmationMessage }}
     </gl-modal>

app/assets/javascripts/groups/components/group_item.vue

@@ -34,8 +34,8 @@ export default {
       ),
     itemCaret,
     itemTypeIcon,
-    itemStats,
     itemActions,
+    itemStats,
   },
   props: {
     parentGroup: {
@@ -92,6 +92,9 @@ export default {
     complianceFramework() {
       return this.group.complianceFramework;
     },
+    showActionsMenu() {
+      return this.isGroup && (this.group.canEdit || this.group.canRemove || this.group.canLeave);
+    },
   },
   methods: {
     onClickRowGroup(e) {
@@ -197,17 +200,19 @@ export default {
         <div v-if="isGroupPendingRemoval">
           <gl-badge variant="warning">{{ __('pending deletion') }}</gl-badge>
         </div>
-        <div class="metadata d-flex flex-grow-1 flex-shrink-0 flex-wrap justify-content-md-between">
+        <div
+          class="metadata gl-display-flex gl-flex-grow-1 gl-flex-shrink-0 gl-flex-wrap justify-content-md-between"
+        >
+          <item-stats
+            :item="group"
+            class="group-stats gl-mt-2 gl-display-none gl-md-display-flex gl-align-items-center"
+          />
           <item-actions
-            v-if="isGroup"
+            v-if="showActionsMenu"
             :group="group"
             :parent-group="parentGroup"
             :action="action"
           />
-          <item-stats
-            :item="group"
-            class="group-stats gl-mt-2 d-none d-md-flex gl-align-items-center"
-          />
         </div>
       </div>
     </div>

app/assets/javascripts/groups/components/item_actions.vue

 <script>
-import { GlTooltipDirective, GlButton, GlModalDirective } from '@gitlab/ui';
+import { GlTooltipDirective, GlDropdown, GlDropdownItem } from '@gitlab/ui';
 import { COMMON_STR } from '../constants';
 import eventHub from '../event_hub';
 
+const { LEAVE_BTN_TITLE, EDIT_BTN_TITLE, REMOVE_BTN_TITLE, OPTIONS_DROPDOWN_TITLE } = COMMON_STR;
+
 export default {
   components: {
-    GlButton,
+    GlDropdown,
+    GlDropdownItem,
   },
   directives: {
     GlTooltip: GlTooltipDirective,
-    GlModal: GlModalDirective,
   },
   props: {
     parentGroup: {
@@ -28,11 +30,8 @@ export default {
     },
   },
   computed: {
-    leaveBtnTitle() {
-      return COMMON_STR.LEAVE_BTN_TITLE;
-    },
-    editBtnTitle() {
-      return COMMON_STR.EDIT_BTN_TITLE;
+    removeButtonHref() {
+      return `${this.group.editPath}#js-remove-group-form`;
     },
   },
   methods: {
@@ -40,33 +39,51 @@ export default {
       eventHub.$emit(`${this.action}showLeaveGroupModal`, this.group, this.parentGroup);
     },
   },
+  i18n: {
+    leaveBtnTitle: LEAVE_BTN_TITLE,
+    editBtnTitle: EDIT_BTN_TITLE,
+    removeBtnTitle: REMOVE_BTN_TITLE,
+    optionsDropdownTitle: OPTIONS_DROPDOWN_TITLE,
+  },
 };
 </script>
 
 <template>
-  <div class="controls d-flex justify-content-end">
-    <gl-button
-      v-if="group.canLeave"
-      v-gl-tooltip.top
-      v-gl-modal.leave-group-modal
-      :title="leaveBtnTitle"
-      :aria-label="leaveBtnTitle"
-      data-testid="leave-group-btn"
-      size="small"
-      icon="leave"
-      class="leave-group gl-ml-3"
-      @click.stop="onLeaveGroup"
-    />
-    <gl-button
+  <div class="gl-display-flex gl-justify-content-end gl-ml-5">
+    <gl-dropdown
+      v-gl-tooltip.hover.focus="$options.i18n.optionsDropdownTitle"
+      right
+      category="tertiary"
+      icon="ellipsis_v"
+      no-caret
+      :data-testid="`group-${group.id}-dropdown-button`"
+      data-qa-selector="group_dropdown_button"
+      :data-qa-group-id="group.id"
+    >
+      <gl-dropdown-item
         v-if="group.canEdit"
-      v-gl-tooltip.top
+        :data-testid="`edit-group-${group.id}-btn`"
         :href="group.editPath"
-      :title="editBtnTitle"
-      :aria-label="editBtnTitle"
-      data-testid="edit-group-btn"
-      size="small"
-      icon="pencil"
-      class="edit-group gl-ml-3"
-    />
+        @click.stop
+      >
+        {{ $options.i18n.editBtnTitle }}
+      </gl-dropdown-item>
+      <gl-dropdown-item
+        v-if="group.canLeave"
+        :data-testid="`leave-group-${group.id}-btn`"
+        @click.stop="onLeaveGroup"
+      >
+        {{ $options.i18n.leaveBtnTitle }}
+      </gl-dropdown-item>
+      <gl-dropdown-item
+        v-if="group.canRemove"
+        :href="removeButtonHref"
+        :data-testid="`remove-group-${group.id}-btn`"
+        variant="danger"
+        @click.stop
+      >
+        {{ $options.i18n.removeBtnTitle }}
+      </gl-dropdown-item>
+    </gl-dropdown>
   </div>
 </template>

app/assets/javascripts/groups/constants.js

@@ -15,8 +15,10 @@ export const COMMON_STR = {
   LEAVE_FORBIDDEN: s__(
     'GroupsTree|Failed to leave the group. Please make sure you are not the only owner.',
   ),
-  LEAVE_BTN_TITLE: s__('GroupsTree|Leave this group'),
-  EDIT_BTN_TITLE: s__('GroupsTree|Edit group'),
+  LEAVE_BTN_TITLE: s__('GroupsTree|Leave group'),
+  EDIT_BTN_TITLE: s__('GroupsTree|Edit'),
+  REMOVE_BTN_TITLE: s__('GroupsTree|Delete'),
+  OPTIONS_DROPDOWN_TITLE: s__('GroupsTree|Options'),
   GROUP_SEARCH_EMPTY: s__('GroupsTree|No groups matched your search'),
   GROUP_PROJECT_SEARCH_EMPTY: s__('GroupsTree|No groups or projects matched your search'),
 };

app/assets/javascripts/groups/store/groups_store.js

@@ -83,6 +83,7 @@ export default class GroupsStore {
       leavePath: rawGroupItem.leave_path,
       canEdit: rawGroupItem.can_edit,
       canLeave: rawGroupItem.can_leave,
+      canRemove: rawGroupItem.can_remove,
       type: rawGroupItem.type,
       permission: rawGroupItem.permission,
       children: groupChildren,

app/serializers/group_child_entity.rb

@@ -58,6 +58,10 @@ class GroupChildEntity < Grape::Entity
     end
   end
 
+  expose :can_remove, unless: lambda { |_instance, _options| project? } do |group|
+    can?(request.current_user, :admin_group, group)
+  end
+
   expose :number_users_with_delimiter, unless: lambda { |_instance, _options| project? } do |instance|
     number_with_delimiter(instance.member_count)
   end

doc/user/group/index.md

@@ -422,10 +422,22 @@ for the group's projects to meet your group's needs.
 
 To remove a group and its contents:
 
-1. Go to your group's **Settings > General** page.
-1. Expand the **Path, transfer, remove** section.
+1. On the top bar, select **Menu > Groups** and find your group.
+1. On the left sidebar, select **Settings > General**.
+1. Expand the **Advanced** section.
+1. In the **Remove group** section, select **Remove group**.
+1. Type the group name.
+1. Select **Confirm**.
+
+A group can also be removed from the groups dashboard:
+
+1. On the top bar, select **Menu > Groups**.
+1. Select **Your Groups**.
+1. Select (**{ellipsis_v}**) for the group you want to delete.
+1. Select **Delete**.
 1. In the Remove group section, select **Remove group**.
-1. Confirm the action.
+1. Type the group name.
+1. Select **Confirm**.
 
 This action removes the group. It also adds a background job to delete all projects in the group.
 

locale/gitlab.pot

@@ -17695,13 +17695,16 @@ msgstr ""
 msgid "GroupsTree|Are you sure you want to leave the \"%{fullName}\" group?"
 msgstr ""
 
-msgid "GroupsTree|Edit group"
+msgid "GroupsTree|Delete"
+msgstr ""
+
+msgid "GroupsTree|Edit"
 msgstr ""
 
 msgid "GroupsTree|Failed to leave the group. Please make sure you are not the only owner."
 msgstr ""
 
-msgid "GroupsTree|Leave this group"
+msgid "GroupsTree|Leave group"
 msgstr ""
 
 msgid "GroupsTree|Loading groups"
@@ -17713,6 +17716,9 @@ msgstr ""
 msgid "GroupsTree|No groups or projects matched your search"
 msgstr ""
 
+msgid "GroupsTree|Options"
+msgstr ""
+
 msgid "GroupsTree|Search by name"
 msgstr ""
 

spec/features/dashboard/groups_list_spec.rb

@@ -15,6 +15,10 @@ RSpec.describe 'Dashboard Groups page', :js do
     wait_for_requests
   end
 
+  def click_options_menu(group)
+    page.find("[data-testid='group-#{group.id}-dropdown-button'").click
+  end
+
   it 'shows groups user is member of' do
     group.add_owner(user)
     nested_group.add_owner(user)
@@ -112,6 +116,67 @@ RSpec.describe 'Dashboard Groups page', :js do
     end
   end
 
+  context 'group actions dropdown' do
+    let!(:subgroup) { create(:group, :public, parent: group) }
+
+    context 'user with subgroup ownership' do
+      before do
+        subgroup.add_owner(user)
+        sign_in(user)
+
+        visit dashboard_groups_path
+      end
+
+      it 'cannot remove parent group' do
+        expect(page).not_to have_selector("[data-testid='group-#{group.id}-dropdown-button'")
+      end
+    end
+
+    context 'user with parent group ownership' do
+      before do
+        group.add_owner(user)
+        sign_in(user)
+
+        visit dashboard_groups_path
+      end
+
+      it 'can remove parent group' do
+        click_options_menu(group)
+
+        expect(page).to have_selector("[data-testid='remove-group-#{group.id}-btn']")
+      end
+
+      it 'can remove subgroups' do
+        click_group_caret(group)
+        click_options_menu(subgroup)
+
+        expect(page).to have_selector("[data-testid='remove-group-#{subgroup.id}-btn']")
+      end
+    end
+
+    context 'user is a maintainer' do
+      before do
+        group.add_maintainer(user)
+        sign_in(user)
+
+        visit dashboard_groups_path
+        click_options_menu(group)
+      end
+
+      it 'cannot remove the group' do
+        expect(page).not_to have_selector("[data-testid='remove-group-#{group.id}-btn']")
+      end
+
+      it 'cannot edit the group' do
+        expect(page).not_to have_selector("[data-testid='edit-group-#{group.id}-btn']")
+      end
+
+      it 'can leave the group' do
+        expect(page).to have_selector("[data-testid='leave-group-#{group.id}-btn']")
+      end
+    end
+  end
+
   context 'when using pagination' do
     let(:group)  { create(:group, created_at: 5.days.ago) }
     let(:group2) { create(:group, created_at: 2.days.ago) }

spec/frontend/groups/components/app_spec.js

@@ -280,6 +280,7 @@ describe('AppComponent', () => {
         expect(vm.targetParentGroup).toBe(null);
         vm.showLeaveGroupModal(group, mockParentGroupItem);
 
+        expect(vm.isModalVisible).toBe(true);
         expect(vm.targetGroup).not.toBe(null);
         expect(vm.targetParentGroup).not.toBe(null);
       });
@@ -290,6 +291,7 @@ describe('AppComponent', () => {
         expect(vm.groupLeaveConfirmationMessage).toBe('');
         vm.showLeaveGroupModal(group, mockParentGroupItem);
 
+        expect(vm.isModalVisible).toBe(true);
         expect(vm.groupLeaveConfirmationMessage).toBe(
           `Are you sure you want to leave the "${group.fullName}" group?`,
         );

spec/frontend/groups/components/item_actions_spec.js

-import { shallowMount } from '@vue/test-utils';
+import { shallowMountExtended } from 'helpers/vue_test_utils_helper';
 import ItemActions from '~/groups/components/item_actions.vue';
 import eventHub from '~/groups/event_hub';
 import { mockParentGroupItem, mockChildren } from '../mock_data';
@@ -13,7 +13,7 @@ describe('ItemActions', () => {
   };
 
   const createComponent = (props = {}) => {
-    wrapper = shallowMount(ItemActions, {
+    wrapper = shallowMountExtended(ItemActions, {
       propsData: { ...defaultProps, ...props },
     });
   };
@@ -23,8 +23,10 @@ describe('ItemActions', () => {
     wrapper = null;
   });
 
-  const findEditGroupBtn = () => wrapper.find('[data-testid="edit-group-btn"]');
-  const findLeaveGroupBtn = () => wrapper.find('[data-testid="leave-group-btn"]');
+  const findEditGroupBtn = () => wrapper.findByTestId(`edit-group-${mockParentGroupItem.id}-btn`);
+  const findLeaveGroupBtn = () => wrapper.findByTestId(`leave-group-${mockParentGroupItem.id}-btn`);
+  const findRemoveGroupBtn = () =>
+    wrapper.findByTestId(`remove-group-${mockParentGroupItem.id}-btn`);
 
   describe('template', () => {
     let group;
@@ -34,6 +36,7 @@ describe('ItemActions', () => {
         ...mockParentGroupItem,
         canEdit: true,
         canLeave: true,
+        canRemove: true,
       };
       createComponent({ group });
     });
@@ -41,21 +44,21 @@ describe('ItemActions', () => {
     it('renders component template correctly', () => {
       createComponent();
 
-      expect(wrapper.classes()).toContain('controls');
+      expect(wrapper.classes()).toContain('gl-display-flex', 'gl-justify-content-end', 'gl-ml-5');
     });
 
-    it('renders "Edit group" button with correct attribute values', () => {
+    it('renders "Edit" group button with correct attribute values', () => {
       const button = findEditGroupBtn();
       expect(button.exists()).toBe(true);
-      expect(button.props('icon')).toBe('pencil');
-      expect(button.attributes('aria-label')).toBe('Edit group');
+      expect(button.attributes('href')).toBe(mockParentGroupItem.editPath);
     });
 
-    it('renders "Leave this group" button with correct attribute values', () => {
-      const button = findLeaveGroupBtn();
+    it('renders "Delete" group button with correct attribute values', () => {
+      const button = findRemoveGroupBtn();
       expect(button.exists()).toBe(true);
-      expect(button.props('icon')).toBe('leave');
-      expect(button.attributes('aria-label')).toBe('Leave this group');
+      expect(button.attributes('href')).toBe(
+        `${mockParentGroupItem.editPath}#js-remove-group-form`,
+      );
     });
 
     it('emits `showLeaveGroupModal` event in the event hub', () => {
@@ -103,4 +106,15 @@ describe('ItemActions', () => {
 
     expect(findEditGroupBtn().exists()).toBe(false);
   });
+
+  it('does not render delete button if group can not be edited', () => {
+    createComponent({
+      group: {
+        ...mockParentGroupItem,
+        canRemove: false,
+      },
+    });
+
+    expect(findRemoveGroupBtn().exists()).toBe(false);
+  });
 });

spec/serializers/group_child_entity_spec.rb

@@ -6,7 +6,8 @@ RSpec.describe GroupChildEntity do
   include ExternalAuthorizationServiceHelpers
   include Gitlab::Routing.url_helpers
 
-  let(:user) { create(:user) }
+  let_it_be(:user) { create(:user) }
+
   let(:request) { double('request') }
   let(:entity) { described_class.new(object, request: request) }
 
@@ -103,6 +104,22 @@ RSpec.describe GroupChildEntity do
       expect(json[:can_leave]).to be_truthy
     end
 
+    it 'allows an owner to delete the group' do
+      expect(json[:can_remove]).to be_truthy
+    end
+
+    it 'allows admin to delete the group', :enable_admin_mode do
+      allow(request).to receive(:current_user).and_return(create(:admin))
+
+      expect(json[:can_remove]).to be_truthy
+    end
+
+    it 'disallows a maintainer to delete the group' do
+      object.add_maintainer(user)
+
+      expect(json[:can_remove]).to be_falsy
+    end
+
     it 'has the correct edit path' do
       expect(json[:edit_path]).to eq(edit_group_path(object))
     end