From f21ecf29f05b83d93aa3c9bd0a341b89f708cffc Mon Sep 17 00:00:00 2001
From: Andrew Fontaine <afontaine@gitlab.com>
Date: Wed, 31 May 2023 17:11:40 +0000
Subject: [PATCH] Add delete option to selected approvers
Instead of making users open the approvers dropdown and deselecting
options, it is a lot smoother to have a small trash button next to each
approver row to remove it from the selected options.
Changelog: added
EE: true
---
.../settings/components/access_dropdown.vue | 31 +++++++++
.../protected_environments/add_approvers.vue | 41 +++++++++--
.../page_bundles/ci_cd_settings.scss | 2 +-
.../add_approvers_spec.js | 68 +++++++++++++++++--
.../create_protected_environment_spec.js | 2 +-
locale/gitlab.pot | 3 +
.../components/new_access_dropdown_spec.js | 34 +++++++++-
7 files changed, 168 insertions(+), 13 deletions(-)
diff --git a/app/assets/javascripts/projects/settings/components/access_dropdown.vue b/app/assets/javascripts/projects/settings/components/access_dropdown.vue
index 08a1c586f6957..a2e4827cbfa2b 100644
--- a/app/assets/javascripts/projects/settings/components/access_dropdown.vue
+++ b/app/assets/javascripts/projects/settings/components/access_dropdown.vue
@@ -63,6 +63,11 @@ export default {
required: false,
default: () => [],
},
+ items: {
+ type: Array,
+ required: false,
+ default: () => [],
+ },
},
data() {
return {
@@ -143,11 +148,37 @@ export default {
query: debounce(function debouncedSearch() {
return this.getData();
}, 500),
+ items(items) {
+ this.setDataForSave(items);
+ },
},
created() {
this.getData({ initial: true });
},
methods: {
+ setDataForSave(items) {
+ this.selected = items.reduce(
+ (selected, item) => {
+ if (item.group_id) {
+ selected[LEVEL_TYPES.GROUP].push({ id: item.group_id, ...item });
+ } else if (item.user_id) {
+ selected[LEVEL_TYPES.USER].push({ id: item.user_id, ...item });
+ } else if (item.access_level) {
+ const level = this.accessLevelsData.find(({ id }) => item.access_level === id);
+ selected[LEVEL_TYPES.ROLE].push(level);
+ } else if (item.deploy_key_id) {
+ selected[LEVEL_TYPES.DEPLOY_KEY].push({ id: item.deploy_key_id, ...item });
+ }
+ return selected;
+ },
+ {
+ [LEVEL_TYPES.GROUP]: [],
+ [LEVEL_TYPES.USER]: [],
+ [LEVEL_TYPES.ROLE]: [],
+ [LEVEL_TYPES.DEPLOY_KEY]: [],
+ },
+ );
+ },
focusInput() {
this.$refs.search.focusInput();
},
diff --git a/ee/app/assets/javascripts/protected_environments/add_approvers.vue b/ee/app/assets/javascripts/protected_environments/add_approvers.vue
index 7f972fc7f22f9..00e7b5f3dc692 100644
--- a/ee/app/assets/javascripts/protected_environments/add_approvers.vue
+++ b/ee/app/assets/javascripts/protected_environments/add_approvers.vue
@@ -1,5 +1,13 @@
<script>
-import { GlFormGroup, GlCollapse, GlAvatar, GlLink, GlFormInput } from '@gitlab/ui';
+import {
+ GlFormGroup,
+ GlCollapse,
+ GlAvatar,
+ GlButton,
+ GlLink,
+ GlFormInput,
+ GlTooltipDirective as GlTooltip,
+} from '@gitlab/ui';
import * as Sentry from '@sentry/browser';
import { uniqueId } from 'lodash';
import Api from 'ee/api';
@@ -31,6 +39,12 @@ const mapGroupToApprover = (group) => ({
type: 'group',
});
+const ID_FOR_TYPE = {
+ user: 'user_id',
+ group: 'group_id',
+ access: 'access_level',
+};
+
const MIN_APPROVALS_COUNT = 1;
const MAX_APPROVALS_COUNT = 5;
@@ -41,10 +55,12 @@ export default {
GlFormGroup,
GlCollapse,
GlAvatar,
+ GlButton,
GlLink,
GlFormInput,
AccessDropdown,
},
+ directives: { GlTooltip },
inject: { accessLevelsData: { default: [] } },
props: {
disabled: {
@@ -95,6 +111,7 @@ export default {
return Promise.resolve({
accessLevel: approver.access_level,
+ id: approver.access_level,
name: this.accessLevelsData.find(({ id }) => id === approver.access_level).text,
approvals: 1,
type: 'access',
@@ -119,6 +136,11 @@ export default {
updateApprovers(permissions) {
this.approvers = permissions;
},
+ removeApprover({ type, id }) {
+ const key = ID_FOR_TYPE[type];
+ const index = this.approvers.findIndex(({ [key]: i }) => id === i);
+ this.$delete(this.approvers, index);
+ },
isApprovalValid(approvals) {
const count = parseFloat(approvals);
return count >= MIN_APPROVALS_COUNT && count <= MAX_APPROVALS_COUNT;
@@ -134,6 +156,7 @@ export default {
),
approvalRulesLabel: s__('ProtectedEnvironments|Approval rules'),
approvalsInvalid: s__('ProtectedEnvironments|Number of approvals must be between 1 and 5'),
+ removeApprover: s__('ProtectedEnvironments|Remove approval rule'),
},
};
</script>
@@ -152,8 +175,8 @@ export default {
:access-levels-data="accessLevelsData"
:access-level="$options.ACCESS_LEVELS.DEPLOY"
:disabled="disabled"
- :preselected-items="approvers"
- @hidden="updateApprovers"
+ :items="approvers"
+ @select="updateApprovers"
/>
</gl-form-group>
<gl-collapse :visible="hasSelectedApprovers">
@@ -163,7 +186,7 @@ export default {
class="protected-environment-approvers gl-display-grid gl-gap-5 gl-align-items-center"
>
<span class="protected-environment-approvers-label">{{ __('Approvers') }}</span>
- <span>{{ __('Approvals required') }}</span>
+ <span class="protected-environment-approvers-label">{{ __('Approvals required') }}</span>
<template v-for="(approver, index) in approverInfo">
<gl-avatar
v-if="approver.avatarShape"
@@ -186,6 +209,7 @@ export default {
:label="$options.i18n.approverLabel"
:label-for="approvalsId(index)"
label-sr-only
+ class="gl-mb-0"
>
<gl-form-input
:id="approvalsId(index)"
@@ -199,6 +223,15 @@ export default {
{{ $options.i18n.approvalsInvalid }}
</template>
</gl-form-group>
+ <gl-button
+ :key="`${index}-remove`"
+ v-gl-tooltip
+ :title="$options.i18n.removeApprover"
+ :aria-label="$options.i18n.removeApprover"
+ icon="remove"
+ :data-testid="`remove-approver-${approver.name}`"
+ @click="removeApprover(approver)"
+ />
</template>
</div>
</gl-collapse>
diff --git a/ee/app/assets/stylesheets/page_bundles/ci_cd_settings.scss b/ee/app/assets/stylesheets/page_bundles/ci_cd_settings.scss
index 67445f244c43f..062bfbeb46446 100644
--- a/ee/app/assets/stylesheets/page_bundles/ci_cd_settings.scss
+++ b/ee/app/assets/stylesheets/page_bundles/ci_cd_settings.scss
@@ -1,5 +1,5 @@
.protected-environment-approvers {
- grid-template-columns: repeat(3, max-content);
+ grid-template-columns: repeat(4, max-content);
}
.protected-environment-approvers-label {
diff --git a/ee/spec/frontend/protected_environments/add_approvers_spec.js b/ee/spec/frontend/protected_environments/add_approvers_spec.js
index e33339370b0b4..872505b649daa 100644
--- a/ee/spec/frontend/protected_environments/add_approvers_spec.js
+++ b/ee/spec/frontend/protected_environments/add_approvers_spec.js
@@ -47,6 +47,9 @@ describe('ee/protected_environments/add_approvers.vue', () => {
.findAllComponents(GlFormInput)
.wrappers.find((w) => w.attributes('name') === `approval-count-${name}`);
+ const findRemoveApproverButton = (name) =>
+ wrapper.findComponentByTestId(`remove-approver-${name}`);
+
beforeEach(() => {
window.gon = {
api_version: 'v4',
@@ -72,7 +75,7 @@ describe('ee/protected_environments/add_approvers.vue', () => {
mockAxios.onGet().replyOnce(HTTP_STATUS_BAD_REQUEST);
createComponent();
- findApproverDropdown().vm.$emit('hidden', [{ group_id: 1 }]);
+ findApproverDropdown().vm.$emit('select', [{ group_id: 1 }]);
await waitForPromises();
@@ -86,7 +89,7 @@ describe('ee/protected_environments/add_approvers.vue', () => {
it('emits an empty error value when fetching new details', async () => {
createComponent();
- findApproverDropdown().vm.$emit('hidden', [{ group_id: 1 }]);
+ findApproverDropdown().vm.$emit('select', [{ group_id: 1 }]);
await waitForPromises();
@@ -96,7 +99,7 @@ describe('ee/protected_environments/add_approvers.vue', () => {
avatar_url: '/root.png',
id: 1,
});
- findApproverDropdown().vm.$emit('hidden', [{ user_id: 1 }]);
+ findApproverDropdown().vm.$emit('select', [{ user_id: 1 }]);
await waitForPromises();
@@ -104,6 +107,63 @@ describe('ee/protected_environments/add_approvers.vue', () => {
expect(event).toBe('');
});
+ it('shows a remove button for approvers', async () => {
+ createComponent();
+ mockAxios.onGet('/api/v4/groups/1').reply(HTTP_STATUS_OK, {
+ full_name: 'root / group',
+ name: 'group',
+ web_url: `${TEST_HOST}/root/group`,
+ avatar_url: '/root/group.png',
+ id: 1,
+ });
+ findApproverDropdown().vm.$emit('select', [{ group_id: 1 }]);
+
+ await nextTick();
+ await waitForPromises();
+
+ const button = findRemoveApproverButton('root / group');
+
+ expect(button.props('icon')).toBe('remove');
+ expect(button.attributes()).toMatchObject({
+ title: s__('ProtectedEnvironments|Remove approval rule'),
+ 'aria-label': s__('ProtectedEnvironments|Remove approval rule'),
+ });
+ });
+
+ it('removes approvers if remove is clicked', async () => {
+ createComponent();
+ mockAxios
+ .onGet('/api/v4/users/1')
+ .reply(HTTP_STATUS_OK, {
+ name: 'root',
+ web_url: `${TEST_HOST}/root`,
+ avatar_url: '/root.png',
+ id: 1,
+ })
+ .onGet('/api/v4/groups/1')
+ .reply(HTTP_STATUS_OK, {
+ full_name: 'root / group',
+ name: 'group',
+ web_url: `${TEST_HOST}/root/group`,
+ avatar_url: '/root/group.png',
+ id: 1,
+ });
+ findApproverDropdown().vm.$emit('select', [{ user_id: 1 }, { group_id: 1 }]);
+
+ await nextTick();
+ await waitForPromises();
+
+ const button = findRemoveApproverButton('root');
+
+ await button.vm.$emit('click');
+
+ await waitForPromises();
+
+ const approvalInputs = wrapper.findByTestId('approval-rules').findAllComponents(GlFormInput);
+
+ expect(approvalInputs).toHaveLength(1);
+ });
+
describe('information for approvers', () => {
beforeEach(() => {
mockAxios.onGet('/api/v4/users/1').replyOnce(HTTP_STATUS_OK, {
@@ -129,7 +189,7 @@ describe('ee/protected_environments/add_approvers.vue', () => {
`('it displays correct information for $type', ({ access, details }) => {
beforeEach(async () => {
createComponent();
- findApproverDropdown().vm.$emit('hidden', [access]);
+ findApproverDropdown().vm.$emit('select', [access]);
await nextTick();
await waitForPromises();
});
diff --git a/ee/spec/frontend/protected_environments/create_protected_environment_spec.js b/ee/spec/frontend/protected_environments/create_protected_environment_spec.js
index ecd4128d1d697..12c347f72d732 100644
--- a/ee/spec/frontend/protected_environments/create_protected_environment_spec.js
+++ b/ee/spec/frontend/protected_environments/create_protected_environment_spec.js
@@ -94,7 +94,7 @@ describe('ee/protected_environments/create_protected_environment.vue', () => {
});
findAccessDropdown().vm.$emit('hidden', deployAccessLevels);
findEnvironmentsListbox().vm.$emit('select', name);
- findApproverDropdown().vm.$emit('hidden', deployAccessLevels);
+ findApproverDropdown().vm.$emit('select', deployAccessLevels);
await waitForPromises();
findRequiredCountForApprover('root').vm.$emit('input', requiredApprovalCount);
await nextTick();
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index d06749ed65faf..9c6e0c956112c 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -36659,6 +36659,9 @@ msgstr ""
msgid "ProtectedEnvironments|Number of approvals must be between 1 and 5"
msgstr ""
+msgid "ProtectedEnvironments|Remove approval rule"
+msgstr ""
+
msgid "ProtectedEnvironments|Required approval count"
msgstr ""
diff --git a/spec/frontend/projects/settings/components/new_access_dropdown_spec.js b/spec/frontend/projects/settings/components/new_access_dropdown_spec.js
index f3e536de70305..ce696ee321b85 100644
--- a/spec/frontend/projects/settings/components/new_access_dropdown_spec.js
+++ b/spec/frontend/projects/settings/components/new_access_dropdown_spec.js
@@ -99,6 +99,9 @@ describe('Access Level Dropdown', () => {
const findDropdownItemWithText = (items, text) =>
items.filter((item) => item.text().includes(text)).at(0);
+ const findSelected = (type) =>
+ wrapper.findAllByTestId(`${type}-dropdown-item`).filter((w) => w.props('isChecked'));
+
describe('data request', () => {
it('should make an api call for users, groups && deployKeys when user has a license', () => {
createComponent();
@@ -305,9 +308,6 @@ describe('Access Level Dropdown', () => {
{ id: 122, type: 'deploy_key', deploy_key_id: 12 },
];
- const findSelected = (type) =>
- wrapper.findAllByTestId(`${type}-dropdown-item`).filter((w) => w.props('isChecked'));
-
beforeEach(async () => {
createComponent({ preselectedItems });
await waitForPromises();
@@ -339,6 +339,34 @@ describe('Access Level Dropdown', () => {
});
});
+ describe('handling two-way data binding', () => {
+ it('emits a formatted update on selection', async () => {
+ createComponent();
+ await waitForPromises();
+ const dropdownItems = findAllDropdownItems();
+ // select new item from each group
+ findDropdownItemWithText(dropdownItems, 'role1').trigger('click');
+ findDropdownItemWithText(dropdownItems, 'group4').trigger('click');
+ findDropdownItemWithText(dropdownItems, 'user7').trigger('click');
+ findDropdownItemWithText(dropdownItems, 'key10').trigger('click');
+
+ await wrapper.setProps({ items: [{ user_id: 7 }] });
+
+ const selectedUsers = findSelected(LEVEL_TYPES.USER);
+ expect(selectedUsers).toHaveLength(1);
+ expect(selectedUsers.at(0).text()).toBe('user7');
+
+ const selectedRoles = findSelected(LEVEL_TYPES.ROLE);
+ expect(selectedRoles).toHaveLength(0);
+
+ const selectedGroups = findSelected(LEVEL_TYPES.GROUP);
+ expect(selectedGroups).toHaveLength(0);
+
+ const selectedDeployKeys = findSelected(LEVEL_TYPES.DEPLOY_KEY);
+ expect(selectedDeployKeys).toHaveLength(0);
+ });
+ });
+
describe('on dropdown open', () => {
beforeEach(() => {
createComponent();
--
GitLab