Merge branch '349863-ability-to-remove-groups-at-the-parent-group-level' into 'master'

Add Ability to Remove groups at the parent group level

See merge request gitlab-org/gitlab!79312

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
-      v-if="group.canEdit"
-      v-gl-tooltip.top
-      :href="group.editPath"
-      :title="editBtnTitle"
-      :aria-label="editBtnTitle"
-      data-testid="edit-group-btn"
-      size="small"
-      icon="pencil"
-      class="edit-group gl-ml-3"
-    />
+  <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"
+        :data-testid="`edit-group-${group.id}-btn`"
+        :href="group.editPath"
+        @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

@@ -17701,13 +17701,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"
@@ -17719,6 +17722,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