diff --git a/master/internal/api_user.go b/master/internal/api_user.go index 5ef1c9ccafc..b97f22fae31 100644 --- a/master/internal/api_user.go +++ b/master/internal/api_user.go @@ -163,8 +163,7 @@ func (a *apiServer) GetUsers( ColumnExpr("h.uid AS agent_uid"). ColumnExpr("h.gid AS agent_gid"). ColumnExpr("h.user_ AS agent_user"). - ColumnExpr("h.group_ AS agent_group"). - ColumnExpr("COALESCE(u.display_name, u.username) AS name") + ColumnExpr("h.group_ AS agent_group") if req.Name != "" { nameFilterExpr := "%" + req.Name + "%" @@ -196,7 +195,12 @@ func (a *apiServer) GetUsers( if !ok { return nil, fmt.Errorf("unsupported sort by %s", req.SortBy) } - query.OrderExpr("? ?", bun.Ident(sortColumn), bun.Safe(orderBy)) + if sortColumn == "name" { + // ensure lexicographical sort: + query.OrderExpr("COALESCE(u.display_name, u.username) COLLATE \"C\" " + orderBy) + } else { + query.OrderExpr("? ?", bun.Ident(sortColumn), bun.Safe(orderBy)) + } if sortColumn != "id" { query.OrderExpr("id asc") } diff --git a/webui/react/src/e2e/models/pages/Admin/UserManagement.ts b/webui/react/src/e2e/models/pages/Admin/UserManagement.ts index 038e440d52e..7a83d913f5c 100644 --- a/webui/react/src/e2e/models/pages/Admin/UserManagement.ts +++ b/webui/react/src/e2e/models/pages/Admin/UserManagement.ts @@ -3,6 +3,7 @@ import { BaseComponent } from 'playwright-page-model-base/BaseComponent'; import { expect } from 'e2e/fixtures/global-fixtures'; import { DropdownMenu } from 'e2e/models/common/hew/Dropdown'; +import { Nameplate } from 'e2e/models/common/hew/Nameplate'; import { Select } from 'e2e/models/common/hew/Select'; import { Toast } from 'e2e/models/common/hew/Toast'; import { AddUsersToGroupsModal } from 'e2e/models/components/AddUsersToGroupsModal'; @@ -150,6 +151,9 @@ class UserRow extends Row { parent: this, selector: '[data-testid="user"]', }); + readonly nameplate = new Nameplate({ + parent: this, + }); readonly status = new BaseComponent({ parent: this, selector: '[data-testid="status"]', diff --git a/webui/react/src/e2e/tests/userManagement.spec.ts b/webui/react/src/e2e/tests/userManagement.spec.ts index 162b80c8345..5d128d7f400 100644 --- a/webui/react/src/e2e/tests/userManagement.spec.ts +++ b/webui/react/src/e2e/tests/userManagement.spec.ts @@ -1,9 +1,14 @@ +import { orderBy } from 'lodash'; + +import { ApiUserFixture } from 'e2e/fixtures/api.user.fixture'; import { expect, test } from 'e2e/fixtures/global-fixtures'; import { UserManagement } from 'e2e/models/pages/Admin/UserManagement'; import { SignIn } from 'e2e/models/pages/SignIn'; -import { sessionRandomHash } from 'e2e/utils/naming'; +import { safeName, sessionRandomHash } from 'e2e/utils/naming'; import { repeatWithFallback } from 'e2e/utils/polling'; +import { isRbacEnabled } from 'e2e/utils/rbac'; import { TestUser } from 'e2e/utils/users'; +import { V1User } from 'services/api-ts-sdk'; test.describe('User Management', () => { // One list of users per test session. This is to encourage a final teardown @@ -124,16 +129,44 @@ test.describe('User Management', () => { }); test.describe('User Management List', () => { - const usernamePrefix = 'test-user-pagination'; + type PageCountOption = 'perPage10' | 'perPage20' | 'perPage50' | 'perPage100'; + const USERNAME_PREFIX = 'test-user-list'; + const listTestUsers: V1User[] = []; + + const setPageCount = async ( + userManagementPage: UserManagement, + pageCountOption: PageCountOption, + ) => { + await expect( + repeatWithFallback( + async () => { + await userManagementPage.table.table.pagination.perPage.openMenu(); + await userManagementPage.table.table.pagination.perPage[ + pageCountOption + ].pwLocator.click(); + }, + async () => { + // BUG [ET-233] + await userManagementPage.goto(); + }, + ), + ).toPass({ timeout: 25_000 }); + }; + test.beforeAll(async ({ backgroundApiUser }) => { - await test.step('Create User', async () => { + await test.step('Create test users', async () => { // pagination will be 10 per page, so create 11 users for (let i = 0; i < 11; i++) { const userResponse = await backgroundApiUser.createUser( - backgroundApiUser.new({ usernamePrefix }), + // adding index prefix allows more specific testing of sorting: + backgroundApiUser.new({ usernamePrefix: `${i}-${USERNAME_PREFIX}` }), ); if (userResponse.user?.id) { testUserIds.push(userResponse.user.id); + listTestUsers.push({ + ...userResponse.user, + displayName: userResponse.user.displayName || undefined, + }); } else { throw new Error('createUser: invalid API response'); } @@ -141,23 +174,11 @@ test.describe('User Management', () => { }); }); - test('[ET-233] Bulk Actions', async ({ page, user, playwright }) => { + test('Bulk Actions', async ({ page, user, playwright }) => { const userManagementPage = new UserManagement(page); await test.step('Setup Table Filters', async () => { - // set pagination to 10 - await expect( - repeatWithFallback( - async () => { - await userManagementPage.table.table.pagination.perPage.openMenu(); - await userManagementPage.table.table.pagination.perPage.perPage10.pwLocator.click(); - }, - async () => { - // BUG [ET-233] - await userManagementPage.goto(); - }, - ), - ).toPass({ timeout: 15_000 }); + await setPageCount(userManagementPage, 'perPage10'); // filter by active users await userManagementPage.filterStatus.openMenu(); await userManagementPage.filterStatus.activeUsers.pwLocator.click(); @@ -169,13 +190,9 @@ test.describe('User Management', () => { ).toHaveLength(10); }).toPass({ timeout: 10_000 }); // search for users created this session and wait for table stable - await userManagementPage.search.pwLocator.fill(usernamePrefix + sessionRandomHash); + await userManagementPage.search.pwLocator.fill(USERNAME_PREFIX + sessionRandomHash); await expect(async () => { - expect( - await userManagementPage.table.table.filterRows(async (row) => { - return (await row.user.name.pwLocator.textContent())?.indexOf(usernamePrefix) === 0; - }), - ).toHaveLength(10); + expect(await userManagementPage.table.table.rows.pwLocator.all()).toHaveLength(10); }).toPass({ timeout: 10_000 }); // go to page 2 to see users await expect(async () => { @@ -227,38 +244,26 @@ test.describe('User Management', () => { return Number(match[1]); }; - const pagination = userManagementPage.table.table.pagination; - for await (const { name, paginationOption } of [ + for await (const { name, pageCountOption } of [ { name: '10', - paginationOption: pagination.perPage.perPage10, + pageCountOption: 'perPage10', }, { name: '20', - paginationOption: pagination.perPage.perPage20, + pageCountOption: 'perPage20', }, { name: '50', - paginationOption: pagination.perPage.perPage50, + pageCountOption: 'perPage50', }, { name: '100', - paginationOption: pagination.perPage.perPage100, + pageCountOption: 'perPage100', }, ]) { await test.step(`Compare Table Rows With Pagination ${name}`, async () => { - await expect( - repeatWithFallback( - async () => { - await pagination.perPage.openMenu(); - await paginationOption.pwLocator.click(); - }, - async () => { - // BUG [ET-233] - await userManagementPage.goto(); - }, - ), - ).toPass({ timeout: 25_000 }); + await setPageCount(userManagementPage, pageCountOption as PageCountOption); await expect(userManagementPage.skeletonTable.pwLocator).not.toBeVisible(); const paginationSelection = Number(name); await expect( @@ -280,5 +285,172 @@ test.describe('User Management', () => { }); } }); + + test.describe('Sort and filter', () => { + const ADMIN_UPDATE_INDEX = 0; + const STATUS_UPDATE_INDEX = 1; + const NAME_UPDATE_INDEX = 2; + + const updateUser = async (index: number, updates: Partial, api: ApiUserFixture) => { + const userId = listTestUsers[index].id; + if (userId === undefined) throw new Error('patchUser: invalid user'); + const updated = await api.patchUser(userId, updates); + Object.assign(listTestUsers[index], updated); + }; + + const getTableUsernames = async (userManagementPage: UserManagement) => { + return await Promise.all( + (await userManagementPage.table.table.rows.nameplate.name.pwLocator.all()).map( + async (username) => { + return await username.textContent(); + }, + ), + ); + }; + + test.beforeAll(async ({ backgroundApiUser }) => { + const testDisplayName = safeName('0-test-display-name'); + + await updateUser(ADMIN_UPDATE_INDEX, { admin: false }, backgroundApiUser); + await updateUser(STATUS_UPDATE_INDEX, { active: false }, backgroundApiUser); + await updateUser(NAME_UPDATE_INDEX, { displayName: testDisplayName }, backgroundApiUser); + }); + + test.beforeEach(async ({ page }) => { + const userManagementPage = new UserManagement(page); + + await setPageCount(userManagementPage, 'perPage10'); + + await userManagementPage.search.pwLocator.fill(USERNAME_PREFIX + sessionRandomHash); + }); + + test.afterAll(async ({ backgroundApiUser }) => { + await updateUser(ADMIN_UPDATE_INDEX, { admin: true }, backgroundApiUser); + await updateUser(STATUS_UPDATE_INDEX, { active: true }, backgroundApiUser); + await updateUser(NAME_UPDATE_INDEX, { displayName: '' }, backgroundApiUser); + }); + + test('Sort', async ({ page }) => { + const userManagementPage = new UserManagement(page); + + const validateSort = async ( + sortBy: 'name' | 'admin' | 'active' | 'modifiedAt', + order: 'asc' | 'desc', + ) => { + const sortedListTestUsers = orderBy( + listTestUsers, + sortBy === 'name' ? (u) => u.displayName || u.username : sortBy, + order, + ); + + await expect(async () => { + expect(await getTableUsernames(userManagementPage)).toEqual( + sortedListTestUsers.slice(0, 10).map((u) => u.username), + ); + }).toPass({ timeout: 10_000 }); + }; + + const testSort = async ( + columnId: 'user' | 'role' | 'status' | 'modified', + sortBy: 'name' | 'admin' | 'active' | 'modifiedAt', + ) => { + const columnHeader = userManagementPage.table.table.headRow[columnId].pwLocator; + const columnSort = await columnHeader.getAttribute('aria-sort'); + + if (columnSort !== 'ascending') await columnHeader.click(); + await validateSort(sortBy, 'asc'); + + await columnHeader.click(); + await validateSort(sortBy, 'desc'); + }; + + await test.step('Sort by user', async () => { + await testSort('user', 'name'); + }); + + if (!isRbacEnabled()) { + await test.step('Sort by role', async () => { + await testSort('role', 'admin'); + }); + } + + await test.step('Sort by status', async () => { + await testSort('status', 'active'); + }); + + await test.step('Sort by last modified', async () => { + await testSort('modified', 'modifiedAt'); + }); + }); + + test('Filter', async ({ page }) => { + const userManagementPage = new UserManagement(page); + + const validateFilter = async (filterFn: (user: V1User) => boolean) => { + await expect(async () => { + expect((await getTableUsernames(userManagementPage)).sort()).toEqual( + listTestUsers + .filter(filterFn) + .map((u) => u.username) + .sort(), + ); + }).toPass({ timeout: 10_000 }); + }; + + const resetFilters = async () => { + await userManagementPage.search.pwLocator.fill(USERNAME_PREFIX + sessionRandomHash); + + if (!isRbacEnabled()) { + await userManagementPage.filterRole.openMenu(); + await userManagementPage.filterRole.allRoles.pwLocator.click(); + } + + await userManagementPage.filterStatus.openMenu(); + await userManagementPage.filterStatus.allStatuses.pwLocator.click(); + }; + + await test.step('Filter by display name', async () => { + await resetFilters(); + + await userManagementPage.search.pwLocator.fill( + listTestUsers[NAME_UPDATE_INDEX].displayName ?? '', + ); + + await validateFilter( + (u) => u.displayName === listTestUsers[NAME_UPDATE_INDEX].displayName, + ); + }); + + if (!isRbacEnabled()) { + await test.step('Filter by role', async () => { + await resetFilters(); + + await userManagementPage.filterRole.openMenu(); + await userManagementPage.filterRole.admin.pwLocator.click(); + + await validateFilter((u) => !!u.admin); + + await userManagementPage.filterRole.openMenu(); + await userManagementPage.filterRole.nonAdmin.pwLocator.click(); + + await validateFilter((u) => !u.admin); + }); + } + + await test.step('Filter by status', async () => { + await resetFilters(); + + await userManagementPage.filterStatus.openMenu(); + await userManagementPage.filterStatus.activeUsers.pwLocator.click(); + + await validateFilter((u) => !!u.active); + + await userManagementPage.filterStatus.openMenu(); + await userManagementPage.filterStatus.deactivatedUsers.pwLocator.click(); + + await validateFilter((u) => !u.active); + }); + }); + }); }); }); diff --git a/webui/react/src/pages/Admin/UserManagement.tsx b/webui/react/src/pages/Admin/UserManagement.tsx index 332d01a7bf2..16b90c8f6ab 100644 --- a/webui/react/src/pages/Admin/UserManagement.tsx +++ b/webui/react/src/pages/Admin/UserManagement.tsx @@ -40,7 +40,7 @@ import { DetailedUser, UserRole as UserRoleType } from 'types'; import handleError, { ErrorType } from 'utils/error'; import { useObservable } from 'utils/observable'; import { validateDetApiEnum } from 'utils/service'; -import { alphaNumericSorter, booleanSorter, numericSorter } from 'utils/sort'; +import { booleanSorter, numericSorter } from 'utils/sort'; import css from './UserManagement.module.scss'; import { @@ -404,8 +404,10 @@ const UserManagement: React.FC = ({ onUserCreate }: Props) => { key: V1GetUsersRequestSortBy.NAME, onCell: () => ({ ...onRightClickableCell(), 'data-testid': 'user' }), render: (_: string, r: DetailedUser) => , - sorter: (a: DetailedUser, b: DetailedUser) => { - return alphaNumericSorter(a.displayName || a.username, b.displayName || b.username); + sorter: () => { + // do not overwrite paginated backend sort order: + return 0; + // TODO: should not overwrite paginated backend sort order anywhere in the app }, title: 'User', }, @@ -428,7 +430,7 @@ const UserManagement: React.FC = ({ onUserCreate }: Props) => { key: V1GetUsersRequestSortBy.ADMIN, onCell: () => ({ ...onRightClickableCell(), 'data-testid': 'role' }), render: (isAdmin: boolean) => <>{isAdmin ? 'Admin' : 'Member'}, - sorter: (a: DetailedUser, b: DetailedUser) => booleanSorter(a.isAdmin, b.isAdmin), + sorter: (a: DetailedUser, b: DetailedUser) => booleanSorter(b.isAdmin, a.isAdmin), title: 'Role', }, { @@ -492,15 +494,17 @@ const UserManagement: React.FC = ({ onUserCreate }: Props) => { width={'100%'} onChange={handleNameSearchApply} /> - + )}