From 03bece8ea5deaea07093319ec1529e28a8b1ffa2 Mon Sep 17 00:00:00 2001 From: Alexander Ackermann Date: Tue, 20 Oct 2026 00:08:15 +0200 Subject: [PATCH] fix: sort space members and managers by name within a role --- .../Spaces/SideBar/MembersPanel.vue | 17 ++++- .../Spaces/SideBar/MembersPanel.spec.ts | 72 ++++++++++++++++++- .../web-client/src/helpers/space/functions.ts | 18 ++++- .../unit/helpers/space/functions.spec.ts | 38 +++++++++- .../src/composables/piniaStores/spaces.ts | 14 +++- .../composables/piniaStores/spaces.spec.ts | 52 ++++++++++++++ 6 files changed, 200 insertions(+), 11 deletions(-) diff --git a/packages/web-app-admin-settings/src/components/Spaces/SideBar/MembersPanel.vue b/packages/web-app-admin-settings/src/components/Spaces/SideBar/MembersPanel.vue index a1d011adcbd..92326a16cbd 100644 --- a/packages/web-app-admin-settings/src/components/Spaces/SideBar/MembersPanel.vue +++ b/packages/web-app-admin-settings/src/components/Spaces/SideBar/MembersPanel.vue @@ -84,15 +84,28 @@ const availableRoles = computed(() => { }) }) -// group members by role before paginating, so a role section is not scattered across pages +// group members by role before paginating, so a role section is not scattered across pages. +// sort by name within a role, the server returns members in a different order on every request. const sortedPermissions = computed(() => { const roleIds = unref(availableRoles).map(({ id }) => id) function roleIndex(permission: Permission) { const index = roleIds.findIndex((id) => permission.roles.includes(id)) return index === -1 ? roleIds.length : index } + function displayName({ grantedToV2 }: Permission) { + return grantedToV2?.user?.displayName || grantedToV2?.group?.displayName || '' + } - return [...unref(filteredPermissions)].sort((a, b) => roleIndex(a) - roleIndex(b)) + return [...unref(filteredPermissions)].sort((a, b) => { + return ( + roleIndex(a) - roleIndex(b) || + displayName(a).localeCompare(displayName(b), undefined, { + sensitivity: 'base', + numeric: true + }) || + (a.id || '').localeCompare(b.id || '') + ) + }) }) const { diff --git a/packages/web-app-admin-settings/tests/unit/components/Spaces/SideBar/MembersPanel.spec.ts b/packages/web-app-admin-settings/tests/unit/components/Spaces/SideBar/MembersPanel.spec.ts index c8787292142..173b436b631 100644 --- a/packages/web-app-admin-settings/tests/unit/components/Spaces/SideBar/MembersPanel.spec.ts +++ b/packages/web-app-admin-settings/tests/unit/components/Spaces/SideBar/MembersPanel.spec.ts @@ -13,10 +13,16 @@ import MembersRoleSection from '../../../../../src/components/Spaces/SideBar/Mem import { Permission } from '@opencloud-eu/web-client/graph/generated' import { OcPaginationInline } from '@opencloud-eu/design-system/components' +function roleWithActions(id: string, displayName: string, amountOfActions: number) { + const allowedResourceActions = Array.from({ length: amountOfActions }, (_, i) => `action-${i}`) + return mock({ id, displayName, rolePermissions: [{ allowedResourceActions }] }) +} + +// roles are sorted by their amount of allowed actions const graphRoles = { - '1': mock({ id: '1', displayName: 'Managers', rolePermissions: [] }), - '2': mock({ id: '2', displayName: 'Editors', rolePermissions: [] }), - '3': mock({ id: '3', displayName: 'Viewers', rolePermissions: [] }) + '1': roleWithActions('1', 'Managers', 3), + '2': roleWithActions('2', 'Editors', 2), + '3': roleWithActions('3', 'Viewers', 1) } const spaceMock = { @@ -65,6 +71,66 @@ describe('MembersPanel', () => { .map((section) => section.props().permissions[0].grantedToV2.user.displayName) ).toEqual(['admin', 'marie', 'einstein']) }) + it('should sort members by name within a role, independent of the order returned by the server', () => { + const permissions = [ + mock({ + id: 'p-zoe', + grantedToV2: { user: { displayName: 'Zoe' } }, + roles: ['1'] + }), + mock({ + id: 'p-marie', + grantedToV2: { user: { displayName: 'marie' } }, + roles: ['3'] + }), + mock({ + id: 'p-group', + grantedToV2: { group: { displayName: 'Physics' } }, + roles: ['3'] + }), + mock({ + id: 'p-admin-2', + grantedToV2: { user: { displayName: 'Admin' } }, + roles: ['3'] + }), + mock({ + id: 'p-einstein', + grantedToV2: { user: { displayName: 'Einstein' } }, + roles: ['1'] + }), + mock({ + id: 'p-admin-1', + grantedToV2: { user: { displayName: 'admin' } }, + roles: ['3'] + }), + mock({ + id: 'p-user-10', + grantedToV2: { user: { displayName: 'User 10' } }, + roles: ['3'] + }), + mock({ + id: 'p-user-2', + grantedToV2: { user: { displayName: 'User 2' } }, + roles: ['3'] + }) + ] + + for (const order of [permissions, [...permissions].reverse()]) { + const { wrapper } = getWrapper({ + spaceResource: { root: { permissions: order } } as undefined as SpaceResource + }) + + expect(rolesOnPage(wrapper)).toEqual(['Managers', 'Viewers']) + expect( + wrapper + .findAllComponents(selectors.membersRolePanelStub) + .map((section) => section.props().permissions.map(({ id }: Permission) => id)) + ).toEqual([ + ['p-einstein', 'p-zoe'], + ['p-admin-1', 'p-admin-2', 'p-marie', 'p-group', 'p-user-2', 'p-user-10'] + ]) + } + }) it('should filter members accordingly to the entered search term', async () => { const userToFilterFor = spaceMock.root.permissions[2] const { wrapper } = getWrapper() diff --git a/packages/web-client/src/helpers/space/functions.ts b/packages/web-client/src/helpers/space/functions.ts index 278d8f3d20e..f6fb3a316a4 100644 --- a/packages/web-client/src/helpers/space/functions.ts +++ b/packages/web-client/src/helpers/space/functions.ts @@ -16,7 +16,7 @@ import { import { DavProperty } from '../../webdav/constants' import { buildWebDavPublicPath, buildWebDavOcmPath } from '../publicLink' import { urlJoin } from '../../utils' -import { Drive, DriveItem } from '@opencloud-eu/web-client/graph/generated' +import { Drive, DriveItem, Permission } from '@opencloud-eu/web-client/graph/generated' import { CollaboratorShare, GraphSharePermission, ShareRole } from '../share' export function buildWebDavSpacesPath(storageId: string, path?: string) { @@ -51,7 +51,7 @@ export function isManager(share: CollaboratorShare) { } export const getSpaceManagers = (space: SpaceResource, graphRoles: Record) => { - return space.root.permissions?.filter((p) => { + const managers = space.root.permissions?.filter((p) => { let permissionActions: string[] = [] if (p['@libre.graph.permissions.actions']) { permissionActions = p['@libre.graph.permissions.actions'] @@ -66,6 +66,20 @@ export const getSpaceManagers = (space: SpaceResource, graphRoles: Record { + return ( + displayName(a).localeCompare(displayName(b), undefined, { + sensitivity: 'base', + numeric: true + }) || (a.id || '').localeCompare(b.id || '') + ) + }) } export type PublicLinkType = 'ocm' | 'public-link' diff --git a/packages/web-client/tests/unit/helpers/space/functions.spec.ts b/packages/web-client/tests/unit/helpers/space/functions.spec.ts index a72e6e65a8e..dff26d83f5f 100644 --- a/packages/web-client/tests/unit/helpers/space/functions.spec.ts +++ b/packages/web-client/tests/unit/helpers/space/functions.spec.ts @@ -1,7 +1,7 @@ -import { buildSpace, SpaceResource } from '../../../../src/helpers/space' +import { buildSpace, getSpaceManagers, SpaceResource } from '../../../../src/helpers/space' import { mock } from 'vitest-mock-extended' import { Ability, GraphSharePermission } from '@opencloud-eu/web-client' -import { Drive, User } from '@opencloud-eu/web-client/graph/generated' +import { Drive, Permission, User } from '@opencloud-eu/web-client/graph/generated' describe('buildSpace', () => { const id = '1' @@ -285,3 +285,37 @@ describe('buildSpace', () => { ) }) }) + +describe('getSpaceManagers', () => { + function permission(id: string, grantedToV2: Permission['grantedToV2'], isManager = true) { + return mock({ + id, + grantedToV2, + roles: [], + '@libre.graph.permissions.actions': isManager + ? [GraphSharePermission.deletePermissions] + : [GraphSharePermission.readBasic] + }) + } + + it('returns the managers sorted by name, independent of the order returned by the server', () => { + const permissions = [ + permission('p-user-10', { user: { id: 'u10', displayName: 'User 10' } }), + permission('p-viewer', { user: { id: 'viewer', displayName: 'Anna' } }, false), + permission('p-admin-2', { user: { id: 'admin2', displayName: 'Admin' } }), + permission('p-group', { group: { id: 'group', displayName: 'physics' } }), + permission('p-user-2', { user: { id: 'u2', displayName: 'User 2' } }), + permission('p-admin-1', { user: { id: 'admin1', displayName: 'admin' } }) + ] + const expectedOrder = ['p-admin-1', 'p-admin-2', 'p-group', 'p-user-2', 'p-user-10'] + + for (const order of [permissions, [...permissions].reverse()]) { + const space = mock({ root: { permissions: order } }) + const serverOrder = order.map(({ id }) => id) + + expect(getSpaceManagers(space, {}).map(({ id }) => id)).toEqual(expectedOrder) + // the permissions of the space itself are not reordered + expect(space.root.permissions.map(({ id }) => id)).toEqual(serverOrder) + } + }) +}) diff --git a/packages/web-pkg/src/composables/piniaStores/spaces.ts b/packages/web-pkg/src/composables/piniaStores/spaces.ts index 9965aeef489..bb0b86e9478 100644 --- a/packages/web-pkg/src/composables/piniaStores/spaces.ts +++ b/packages/web-pkg/src/composables/piniaStores/spaces.ts @@ -19,9 +19,19 @@ import { useExtensionRegistry } from './extensionRegistry' import { markSpaceVaultStatus } from '../../helpers/vault' import { ListPermissionsSpaceRootSelectEnum } from '@opencloud-eu/web-client/graph/generated' -// sort space members with higher permissions (managers) at the top +// sort space members with higher permissions (managers) at the top, then by name. +// the server returns members in a different order on every request, so the order must not depend on it. export const sortSpaceMembers = (shares: CollaboratorShare[]) => { - return shares.sort((a, b) => b.permissions.length - a.permissions.length) + return shares.sort((a, b) => { + return ( + b.permissions.length - a.permissions.length || + (a.sharedWith.displayName || '').localeCompare(b.sharedWith.displayName || '', undefined, { + sensitivity: 'base', + numeric: true + }) || + (a.id || '').localeCompare(b.id || '') + ) + }) } export const getSpacesByType = async ({ diff --git a/packages/web-pkg/tests/unit/composables/piniaStores/spaces.spec.ts b/packages/web-pkg/tests/unit/composables/piniaStores/spaces.spec.ts index 001f7e6fd38..37b317890ef 100644 --- a/packages/web-pkg/tests/unit/composables/piniaStores/spaces.spec.ts +++ b/packages/web-pkg/tests/unit/composables/piniaStores/spaces.spec.ts @@ -32,6 +32,58 @@ describe('spaces', () => { sortedMembers[0].permissions.includes(GraphSharePermission.updatePermissions) ).toBeTruthy() }) + it('sorts space members with the same permissions by name (numeric aware), independent of the input order', () => { + const manager = mock({ + id: 'share-zoe', + permissions: [GraphSharePermission.updatePermissions], + sharedWith: { displayName: 'Zoe' } + }) + const members = [ + manager, + mock({ + id: 'share-marie', + permissions: [], + sharedWith: { displayName: 'marie' } + }), + mock({ + id: 'share-admin-2', + permissions: [], + sharedWith: { displayName: 'Admin' } + }), + mock({ + id: 'share-einstein', + permissions: [], + sharedWith: { displayName: 'Einstein' } + }), + mock({ + id: 'share-admin-1', + permissions: [], + sharedWith: { displayName: 'admin' } + }), + mock({ + id: 'share-user-10', + permissions: [], + sharedWith: { displayName: 'User 10' } + }), + mock({ + id: 'share-user-2', + permissions: [], + sharedWith: { displayName: 'User 2' } + }) + ] + const expectedOrder = [ + 'share-zoe', + 'share-admin-1', + 'share-admin-2', + 'share-einstein', + 'share-marie', + 'share-user-2', + 'share-user-10' + ] + + expect(sortSpaceMembers([...members]).map(({ id }) => id)).toEqual(expectedOrder) + expect(sortSpaceMembers([...members].reverse()).map(({ id }) => id)).toEqual(expectedOrder) + }) }) describe('computed "personalSpace"', () => {