diff --git a/ee/app/assets/javascripts/roles_and_permissions/components/create_member_role.vue b/ee/app/assets/javascripts/roles_and_permissions/components/create_member_role.vue
index 7823c6e00f35a4f25ce59131c57ced0e48d10452..06498cda889459c9f3c2c2f54b54c0e6ea5d8628 100644
--- a/ee/app/assets/javascripts/roles_and_permissions/components/create_member_role.vue
+++ b/ee/app/assets/javascripts/roles_and_permissions/components/create_member_role.vue
@@ -277,7 +277,7 @@ export default {
<div class="gl-mb-3">
<gl-sprintf :message="$options.i18n.baseRoleHelpText">
<template #link="{ content }">
- <gl-link :href="staticRolesHelpPagePath">{{ content }}</gl-link>
+ <gl-link :href="staticRolesHelpPagePath" target="_blank">{{ content }}</gl-link>
</template>
</gl-sprintf>
</div>
@@ -292,7 +292,7 @@ export default {
/>
</gl-form-group>
- <permissions-selector :permissions.sync="memberRole.permissions" :state="isPermissionsValid" />
+ <permissions-selector v-model="memberRole.permissions" :is-valid="isPermissionsValid" />
<div class="gl-display-flex gl-flex-wrap gl-gap-3">
<gl-button
diff --git a/ee/app/assets/javascripts/roles_and_permissions/components/permissions_selector.vue b/ee/app/assets/javascripts/roles_and_permissions/components/permissions_selector.vue
index db6c246b5c8d91c620d45dee72b24763cc3ca362..bf838e33fb97220137e43d33aaa7e65cf02ccd0e 100644
--- a/ee/app/assets/javascripts/roles_and_permissions/components/permissions_selector.vue
+++ b/ee/app/assets/javascripts/roles_and_permissions/components/permissions_selector.vue
@@ -1,30 +1,44 @@
<script>
-import { GlFormCheckbox, GlFormCheckboxGroup, GlFormGroup, GlSkeletonLoader } from '@gitlab/ui';
-import { difference, pull } from 'lodash';
-import { createAlert } from '~/alert';
+import { GlFormCheckbox, GlLoadingIcon, GlTable, GlSprintf, GlLink, GlAlert } from '@gitlab/ui';
+import { pull } from 'lodash';
import { s__ } from '~/locale';
import memberRolePermissionsQuery from 'ee/roles_and_permissions/graphql/member_role_permissions.query.graphql';
+import { helpPagePath } from '~/helpers/help_page_helper';
+
+export const FIELDS = [
+ { key: 'checkbox', label: '' },
+ { key: 'name', label: s__('MemberRole|Permission') },
+ { key: 'description', label: s__('MemberRole|Description') },
+];
export default {
i18n: {
customPermissionsLabel: s__('MemberRole|Custom permissions'),
customPermissionsDescription: s__(
- 'MemberRole|Add at least one custom permission to the base role.',
+ 'MemberRole|Learn more about %{linkStart}available custom permissions%{linkEnd}.',
),
permissionsFetchError: s__('MemberRole|Could not fetch available permissions.'),
+ permissionsSelected: s__('MemberRole|%{count} of %{total} permissions selected'),
+ permissionsSelectionError: s__('MemberRole|Select at least one permission.'),
},
components: {
- GlFormCheckboxGroup,
GlFormCheckbox,
- GlFormGroup,
- GlSkeletonLoader,
+ GlLoadingIcon,
+ GlLink,
+ GlSprintf,
+ GlTable,
+ GlAlert,
+ },
+ model: {
+ prop: 'permissions',
+ event: 'change',
},
props: {
permissions: {
type: Array,
required: true,
},
- state: {
+ isValid: {
type: Boolean,
required: true,
},
@@ -41,16 +55,30 @@ export default {
return data.memberRolePermissions?.nodes || [];
},
error() {
- createAlert({ message: this.$options.i18n.permissionsFetchError });
+ this.availablePermissions = [];
},
},
},
computed: {
+ docsPath() {
+ return helpPagePath('user/custom_roles/abilities');
+ },
isLoadingPermissions() {
return this.$apollo.queries.availablePermissions.loading;
},
- permissionsState() {
- return this.state ? null : false;
+ isErrorLoadingPermissions() {
+ return !this.isLoadingPermissions && !this.hasAvailablePermissions;
+ },
+ hasAvailablePermissions() {
+ return this.availablePermissions.length > 0;
+ },
+ isSomePermissionsSelected() {
+ return this.permissions.length > 0 && !this.isAllPermissionsSelected;
+ },
+ isAllPermissionsSelected() {
+ return (
+ !this.isLoadingPermissions && this.permissions.length >= this.availablePermissions.length
+ );
},
parentPermissionsLookup() {
return this.availablePermissions.reduce((acc, { value, requirements }) => {
@@ -74,20 +102,37 @@ export default {
},
},
methods: {
- emitSelectedPermissions(selected) {
- const added = difference(selected, this.permissions);
- const removed = difference(this.permissions, selected);
- // Check/uncheck any dependent permissions based on what permissions are selected.
- added.forEach((permission) => this.selectParentPermissions(permission, selected));
- removed.forEach((permission) => this.deselectChildPermissions(permission, selected));
+ isSelected({ value }) {
+ return this.permissions.includes(value);
+ },
+ updatePermissions({ value }) {
+ const selected = [...this.permissions];
+
+ if (selected.includes(value)) {
+ // Permission is being removed, remove it and deselect any child permissions.
+ pull(selected, value);
+ this.deselectChildPermissions(value, selected);
+ } else {
+ // Permission is being added, select it and select any parent permissions.
+ selected.push(value);
+ this.selectParentPermissions(value, selected);
+ }
- this.$emit('update:permissions', selected);
+ this.emitPermissionsUpdate(selected);
+ },
+ toggleAllPermissions() {
+ const permissions = this.isAllPermissionsSelected ? [] : this.availablePermissions;
+ this.emitPermissionsUpdate(permissions.map(({ value }) => value));
+ },
+ emitPermissionsUpdate(permissions) {
+ this.$emit('change', permissions);
},
selectParentPermissions(permission, selected) {
const parentPermissions = this.parentPermissionsLookup[permission];
parentPermissions?.forEach((parent) => {
- // Only select the parent permission if it's not already selected.
+ // Only select the parent permission if it's not selected. This prevents an infinite loop if there are
+ // circular dependencies, i.e. A depends on B and B depends on A.
if (!selected.includes(parent)) {
selected.push(parent);
this.selectParentPermissions(parent, selected);
@@ -98,7 +143,8 @@ export default {
const childPermissions = this.childPermissionsLookup[permission];
childPermissions?.forEach((child) => {
- // Only remove the child permission if it's selected.
+ // Only unselect the child permission if it's already selected. This prevents an infinite loop if there are
+ // circular dependencies, i.e. A depends on B and B depends on A.
if (selected.includes(child)) {
pull(selected, child);
this.deselectChildPermissions(child, selected);
@@ -106,38 +152,78 @@ export default {
});
},
},
+ FIELDS,
};
</script>
<template>
- <gl-form-group :label="$options.i18n.customPermissionsLabel" label-class="gl-pb-1!">
- <template #label-description>
- <div v-if="!isLoadingPermissions" class="gl-mb-6">
- {{ $options.i18n.customPermissionsDescription }}
- </div>
- </template>
-
- <div v-if="isLoadingPermissions" class="gl-mt-5">
- <gl-skeleton-loader />
- </div>
-
- <gl-form-checkbox-group
- v-else
- :checked="permissions"
- :state="permissionsState"
- @input="emitSelectedPermissions"
- >
- <gl-form-checkbox
- v-for="permission in availablePermissions"
- :key="permission.value"
- :value="permission.value"
- :data-testid="permission.value"
+ <fieldset>
+ <legend class="gl-mb-1 gl-border-b-0 gl-font-base">
+ <span class="gl-font-weight-bold">{{ $options.i18n.customPermissionsLabel }}</span>
+ <span
+ v-if="hasAvailablePermissions"
+ class="gl-text-gray-400 gl-ml-3"
+ data-testid="permissions-selected-message"
>
- {{ permission.name }}
- <template v-if="permission.description" #help>
- {{ permission.description }}
+ <gl-sprintf :message="$options.i18n.permissionsSelected">
+ <template #count>{{ permissions.length }}</template>
+ <template #total>{{ availablePermissions.length }}</template>
+ </gl-sprintf>
+ </span>
+ </legend>
+
+ <p class="gl-mb-4">
+ <gl-sprintf :message="$options.i18n.customPermissionsDescription">
+ <template #link="{ content }">
+ <gl-link :href="docsPath" target="_blank">{{ content }}</gl-link>
</template>
- </gl-form-checkbox>
- </gl-form-checkbox-group>
- </gl-form-group>
+ </gl-sprintf>
+ </p>
+
+ <p v-if="!isValid" class="gl-text-red-500">{{ $options.i18n.permissionsSelectionError }}</p>
+
+ <gl-alert
+ v-if="isErrorLoadingPermissions"
+ :dismissible="false"
+ variant="danger"
+ class="gl-mb-6 gl-display-inline-block"
+ >
+ {{ $options.i18n.permissionsFetchError }}
+ </gl-alert>
+
+ <gl-table
+ v-else
+ :items="availablePermissions"
+ :fields="$options.FIELDS"
+ :busy="isLoadingPermissions"
+ hover
+ selected-variant=""
+ selectable
+ class="gl-my-8"
+ @row-clicked="updatePermissions"
+ >
+ <template #table-busy>
+ <gl-loading-icon size="md" />
+ </template>
+
+ <template #head(checkbox)>
+ <gl-form-checkbox
+ :disabled="isLoadingPermissions"
+ :checked="isAllPermissionsSelected"
+ :indeterminate="isSomePermissionsSelected"
+ @change="toggleAllPermissions"
+ />
+ </template>
+
+ <template #cell(checkbox)="{ item }">
+ <gl-form-checkbox :checked="isSelected(item)" @change="updatePermissions(item)" />
+ </template>
+
+ <template #cell(name)="{ item }">
+ <span :class="{ 'gl-text-red-500': !isValid }" class="gl-white-space-nowrap">
+ {{ item.name }}
+ </span>
+ </template>
+ </gl-table>
+ </fieldset>
</template>
diff --git a/ee/spec/features/admin/member_roles_spec.rb b/ee/spec/features/admin/member_roles_spec.rb
index f1803f61df1ec7d0128fd3efe33725decdb10e44..e20063eb6e87d2e9ce765d192fd9d58afe7e3a35 100644
--- a/ee/spec/features/admin/member_roles_spec.rb
+++ b/ee/spec/features/admin/member_roles_spec.rb
@@ -22,8 +22,9 @@ def create_role(access_level, name, description, permissions)
fill_in 'Name', with: name
fill_in 'Description', with: description
select access_level, from: 'Base role'
+
permissions.each do |permission|
- page.check permission
+ page.find('tr', text: permission).click
end
click_button s_('MemberRole|Create role')
diff --git a/ee/spec/features/groups/member_roles_spec.rb b/ee/spec/features/groups/member_roles_spec.rb
index b67df583d4e62f661c06a44acc921b98424d3d5b..29220e329cf2debb04470d7e5627215d8750c896 100644
--- a/ee/spec/features/groups/member_roles_spec.rb
+++ b/ee/spec/features/groups/member_roles_spec.rb
@@ -26,7 +26,7 @@ def create_role(access_level, name, description, permissions)
select access_level, from: 'Base role'
permissions.each do |permission|
- page.check(permission)
+ page.find('tr', text: permission).click
end
click_button s_('MemberRole|Create role')
diff --git a/ee/spec/frontend/roles_and_permissions/components/create_member_role_spec.js b/ee/spec/frontend/roles_and_permissions/components/create_member_role_spec.js
index 117372e24c0e3bc0a1880f76cfe03b4857d8f015..48a41ba394f3c0c374c56cbdad7b31848baad77a 100644
--- a/ee/spec/frontend/roles_and_permissions/components/create_member_role_spec.js
+++ b/ee/spec/frontend/roles_and_permissions/components/create_member_role_spec.js
@@ -72,7 +72,7 @@ describe('CreateMemberRole', () => {
findSelect().setValue('GUEST');
findNameField().setValue('My role name');
findDescriptionField().setValue('My description');
- findPermissionsSelector().vm.$emit('update:permissions', ['A']);
+ findPermissionsSelector().vm.$emit('change', ['A']);
return nextTick();
};
@@ -143,12 +143,12 @@ describe('CreateMemberRole', () => {
});
it('shows a warning if no permissions are selected', async () => {
- expect(findPermissionsSelector().props('state')).toBe(true);
+ expect(findPermissionsSelector().props('isValid')).toBe(true);
findPermissionsSelector().vm.$emit('update:permissions', []);
await submitForm();
- expect(findPermissionsSelector().props('state')).toBe(false);
+ expect(findPermissionsSelector().props('isValid')).toBe(false);
});
});
diff --git a/ee/spec/frontend/roles_and_permissions/components/permissions_selector_spec.js b/ee/spec/frontend/roles_and_permissions/components/permissions_selector_spec.js
index 96206ce904d55f597424f03ec84e0bcc1e088676..d67223c53577bee40b2a2f23397ea5def065740a 100644
--- a/ee/spec/frontend/roles_and_permissions/components/permissions_selector_spec.js
+++ b/ee/spec/frontend/roles_and_permissions/components/permissions_selector_spec.js
@@ -1,85 +1,117 @@
-import { GlFormCheckbox, GlFormCheckboxGroup, GlSkeletonLoader } from '@gitlab/ui';
+import { GlTable, GlFormCheckbox, GlAlert, GlSprintf } from '@gitlab/ui';
import Vue from 'vue';
import VueApollo from 'vue-apollo';
-import { createAlert } from '~/alert';
import createMockApollo from 'helpers/mock_apollo_helper';
-import PermissionsSelector from 'ee/roles_and_permissions/components/permissions_selector.vue';
-import { mountExtended, shallowMountExtended } from 'helpers/vue_test_utils_helper';
+import PermissionsSelector, {
+ FIELDS,
+} from 'ee/roles_and_permissions/components/permissions_selector.vue';
+import { shallowMountExtended, mountExtended } from 'helpers/vue_test_utils_helper';
+import { stubComponent } from 'helpers/stub_component';
import waitForPromises from 'helpers/wait_for_promises';
import memberRolePermissionsQuery from 'ee/roles_and_permissions/graphql/member_role_permissions.query.graphql';
import { mockPermissions, mockDefaultPermissions } from '../mock_data';
Vue.use(VueApollo);
-jest.mock('~/alert');
describe('Permissions Selector component', () => {
let wrapper;
const defaultAvailablePermissionsHandler = jest.fn().mockResolvedValue(mockPermissions);
+ const glTableStub = stubComponent(GlTable, { props: ['items', 'fields', 'busy'] });
const createComponent = ({
+ mountFn = shallowMountExtended,
permissions = [],
- state = true,
+ isValid = true,
availablePermissionsHandler = defaultAvailablePermissionsHandler,
- mountFn = shallowMountExtended,
} = {}) => {
wrapper = mountFn(PermissionsSelector, {
- propsData: { permissions, state },
+ propsData: { permissions, isValid },
apolloProvider: createMockApollo([[memberRolePermissionsQuery, availablePermissionsHandler]]),
+ stubs: {
+ GlSprintf,
+ ...(mountFn === shallowMountExtended ? { GlTable: glTableStub } : {}),
+ },
});
return waitForPromises();
};
- const findCheckboxes = () => wrapper.findAllComponents(GlFormCheckbox);
- const findCheckboxGroup = () => wrapper.findComponent(GlFormCheckboxGroup);
- const findSkeletonLoader = () => wrapper.findComponent(GlSkeletonLoader);
+ const findTable = () => wrapper.findComponent(GlTable);
+ const findPermissionsSelectedMessage = () => wrapper.findByTestId('permissions-selected-message');
+ const findAlert = () => wrapper.findComponent(GlAlert);
- const checkPermissions = (permissions) => {
- findCheckboxGroup().vm.$emit('input', permissions);
+ const checkPermission = (value) => {
+ findTable().vm.$emit('row-clicked', { value });
};
- const expectCheckedPermissions = (expected) => {
- const permissions = wrapper.emitted('update:permissions')[0][0];
+ const expectSelectedPermissions = (expected) => {
+ const permissions = wrapper.emitted('change')[0][0];
expect(permissions.sort()).toEqual(expected.sort());
};
describe('available permissions', () => {
- it('loads the available permissions', () => {
- createComponent();
+ describe('when loading', () => {
+ beforeEach(() => {
+ createComponent();
+ });
- expect(defaultAvailablePermissionsHandler).toHaveBeenCalledTimes(1);
- });
+ it('calls the query', () => {
+ expect(defaultAvailablePermissionsHandler).toHaveBeenCalledTimes(1);
+ });
+
+ it('shows the table as busy', () => {
+ expect(findTable().props('busy')).toBe(true);
+ });
- it('shows the GlSkeletonLoader when the query is loading', () => {
- createComponent();
+ it('does not show the permissions selected message', () => {
+ expect(findPermissionsSelectedMessage().exists()).toBe(false);
+ });
- expect(findSkeletonLoader().exists()).toBe(true);
- expect(findCheckboxes()).toHaveLength(0);
+ it('does not show the error message', () => {
+ expect(findAlert().exists()).toBe(false);
+ });
});
- it('shows the expected permissions when loaded', async () => {
- await createComponent();
+ describe('after data is loaded', () => {
+ beforeEach(() => {
+ return createComponent();
+ });
- expect(findSkeletonLoader().exists()).toBe(false);
- expect(findCheckboxes()).toHaveLength(mockDefaultPermissions.length);
+ it('shows the table with the expected permissions', () => {
+ expect(findTable().props('busy')).toBe(false);
+ expect(findTable().props()).toMatchObject({
+ fields: FIELDS,
+ items: mockDefaultPermissions,
+ });
+ });
- mockDefaultPermissions.forEach((permission, index) => {
- const checkboxText = findCheckboxes().at(index).text();
+ it('shows the permissions selected message', () => {
+ expect(findPermissionsSelectedMessage().text()).toBe('0 of 7 permissions selected');
+ });
- expect(checkboxText).toContain(permission.name);
- expect(checkboxText).toContain(permission.description);
+ it('does not show the error message', () => {
+ expect(findAlert().exists()).toBe(false);
});
});
- it('shows an error if the query fails', async () => {
- const availablePermissionsHandler = jest.fn().mockRejectedValue(new Error('failed'));
- await createComponent({ availablePermissionsHandler });
+ describe('on query error', () => {
+ beforeEach(() => {
+ const availablePermissionsHandler = jest.fn().mockRejectedValue();
+ return createComponent({ availablePermissionsHandler });
+ });
+
+ it('shows the error message', () => {
+ expect(findAlert().text()).toBe('Could not fetch available permissions.');
+ });
- expect(findCheckboxes()).toHaveLength(0);
- expect(createAlert).toHaveBeenCalledWith({
- message: 'Could not fetch available permissions.',
+ it('does not show the table', () => {
+ expect(findTable().exists()).toBe(false);
+ });
+
+ it('does not show the permissions selected message', () => {
+ expect(findPermissionsSelectedMessage().exists()).toBe(false);
});
});
});
@@ -96,9 +128,9 @@ describe('Permissions Selector component', () => {
${'G'} | ${['A', 'B', 'C', 'G']}
`('selects $expected when $permission is selected', async ({ permission, expected }) => {
await createComponent();
- checkPermissions([permission]);
+ checkPermission(permission);
- expectCheckedPermissions(expected);
+ expectSelectedPermissions(expected);
});
it.each`
@@ -114,28 +146,34 @@ describe('Permissions Selector component', () => {
'selects $expected when all permissions start off selected and $permission is unselected',
async ({ permission, expected }) => {
const permissions = mockDefaultPermissions.map((p) => p.value);
- const selectedPermissions = permissions.filter((v) => v !== permission);
await createComponent({ permissions });
// Uncheck the permission by removing it from all permissions.
- checkPermissions(selectedPermissions);
+ checkPermission(permission);
- expectCheckedPermissions(expected);
+ expectSelectedPermissions(expected);
},
);
+
+ it('checks the permission when the table row is clicked', async () => {
+ await createComponent({ mountFn: mountExtended });
+ findTable().find('tbody tr').trigger('click');
+
+ expectSelectedPermissions(['A']);
+ });
+
+ it('checks the permission when the checkbox is clicked', async () => {
+ await createComponent({ mountFn: mountExtended });
+ wrapper.findAllComponents(GlFormCheckbox).at(1).trigger('click');
+
+ expectSelectedPermissions(['A']);
+ });
});
describe('validation state', () => {
- it.each`
- state | expectedIsInvalid
- ${true} | ${false}
- ${false} | ${true}
- `('shows validation as $state when state is $state', async ({ state, expectedIsInvalid }) => {
- await createComponent({ state, mountFn: mountExtended });
-
- const input = findCheckboxes().at(0).find('input');
- // is-valid should always be false, otherwise the text color will be green instead of black.
- expect(input.classes('is-valid')).toBe(false);
- expect(input.classes('is-invalid')).toBe(expectedIsInvalid);
+ it.each([true, false])('shows the expected text when isValid prop is %s', async (isValid) => {
+ await createComponent({ mountFn: mountExtended, isValid });
+
+ expect(wrapper.find('tbody td span').classes('gl-text-red-500')).toBe(!isValid);
});
});
});
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index c13771d98028b7df81fd033b96e27dcb6a3f3367..1f47d556d4c448a423e27cf8b3dfda3a28c7e15f 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -31370,13 +31370,13 @@ msgstr ""
msgid "MemberInviteEmail|Invitation to join the %{project_or_group} %{project_or_group_name}"
msgstr ""
-msgid "MemberRole|%{requirement} has to be enabled in order to enable %{permission}"
+msgid "MemberRole|%{count} of %{total} permissions selected"
msgstr ""
-msgid "MemberRole|Actions"
+msgid "MemberRole|%{requirement} has to be enabled in order to enable %{permission}"
msgstr ""
-msgid "MemberRole|Add at least one custom permission to the base role."
+msgid "MemberRole|Actions"
msgstr ""
msgid "MemberRole|Are you sure you want to delete this custom role? Before you delete this custom role, make sure no group member has this role."
@@ -31454,6 +31454,9 @@ msgstr ""
msgid "MemberRole|ID"
msgstr ""
+msgid "MemberRole|Learn more about %{linkStart}available custom permissions%{linkEnd}."
+msgstr ""
+
msgid "MemberRole|Manage roles"
msgstr ""
@@ -31472,6 +31475,9 @@ msgstr ""
msgid "MemberRole|No description"
msgstr ""
+msgid "MemberRole|Permission"
+msgstr ""
+
msgid "MemberRole|Permissions"
msgstr ""
@@ -31490,6 +31496,9 @@ msgstr ""
msgid "MemberRole|Select a %{linkStart}pre-existing static role%{linkEnd} to predefine a set of permissions."
msgstr ""
+msgid "MemberRole|Select at least one permission."
+msgstr ""
+
msgid "MemberRole|Standard roles"
msgstr ""