diff --git a/specifyweb/frontend/js_src/lib/components/Security/Collection.tsx b/specifyweb/frontend/js_src/lib/components/Security/Collection.tsx index 78e92b788bf..8990d603b9a 100644 --- a/specifyweb/frontend/js_src/lib/components/Security/Collection.tsx +++ b/specifyweb/frontend/js_src/lib/components/Security/Collection.tsx @@ -9,7 +9,7 @@ import { commonText } from '../../localization/common'; import { userText } from '../../localization/user'; import type { GetOrSet, IR, RA } from '../../utils/types'; import { defined, localized } from '../../utils/types'; -import { index } from '../../utils/utils'; +import { index, sortFunction } from '../../utils/utils'; import { Container, Ul } from '../Atoms'; import { formatConjunction } from '../Atoms/Internationalization'; import { Link } from '../Atoms/Link'; @@ -32,6 +32,7 @@ import { useCollectionUserRoles, useCollectionUsersWithPolicies, } from './CollectionHooks'; +import { useAdmins } from './Institution'; import type { Role } from './Role'; import { fetchRoles } from './utils'; @@ -52,6 +53,56 @@ export type SecurityCollectionOutlet = SecurityOutlet & { readonly getSetUserRoles: GetOrSet; }; +export type AdminsShape = { + readonly admins: ReadonlySet; + readonly adminUsers: ReadonlyArray<{ + readonly userId: number; + readonly userName: LocalizedString; + }>; +}; + +/** + * Computes the displayUsers by merging collection users with institution admin users. + * Deduplicates and sorts alphabetically by userName. + * Returns mergedUsers unchanged if either argument is undefined. + */ +export function computeDisplayUsers( + mergedUsers: UserRoles | undefined, + admins: AdminsShape | undefined +): UserRoles | undefined { + return typeof mergedUsers === 'object' && typeof admins === 'object' + ? [ + ...mergedUsers, + ...admins.adminUsers + .filter( + ({ userId }) => + !mergedUsers.some((user) => user.userId === userId) + ) + .map(({ userId, userName }) => ({ + userId, + userName, + roles: [] as RA, + })), + ].sort(sortFunction(({ userName }) => userName)) + : mergedUsers; +} + +/** + * Computes the labels for a user, prepending the institution admin label if applicable. + */ +export function computeLabels( + admins: AdminsShape | undefined, + userId: number, + roles: RA, + institutionAdminLabel: LocalizedString +): ReadonlyArray { + const isAdmin = admins?.admins.has(userId) === true; + return [ + ...(isAdmin ? [institutionAdminLabel] : []), + ...roles.map(({ roleName }) => roleName), + ]; +} + export function SecurityCollection(): JSX.Element { const { collectionId = '' } = useParams(); const availableCollections = useAvailableCollections(); @@ -99,7 +150,15 @@ export function CollectionView({ const [userRoles] = getSetUserRoles; useErrorContext('userRoles', userRoles); + const admins = useAdmins(); + + /* + * Include institution admin users in the collection user list so it is + * clear they have power in every collection, even if they don't have + * any explicit collection-level roles or policies assigned. + */ const mergedUsers = mergeCollectionUsers(userRoles, usersWithPolicies); + const displayUsers = computeDisplayUsers(mergedUsers, admins); const navigate = useNavigate(); const location = useLocation(); @@ -152,24 +211,28 @@ export function CollectionView({ collectionTable: tables.Collection.label, })} - {typeof mergedUsers === 'object' ? ( - mergedUsers.length === 0 ? ( + {typeof displayUsers === 'object' ? ( + displayUsers.length === 0 ? ( commonText.none() ) : ( <>
    - {mergedUsers.map(({ userId, userName, roles }) => { + {displayUsers.map(({ userId, userName, roles }) => { const canRead = userId === userInformation.id || hasTablePermission('SpecifyUser', 'read'); + const labels = computeLabels( + admins, + userId, + roles, + userText.institutionAdmin() + ); const children = ( <> {userName} - {roles.length > 0 && ( + {labels.length > 0 && ( - {`(${formatConjunction( - roles.map(({ roleName }) => roleName) - )})`} + {` (${formatConjunction(labels)})`} )} diff --git a/specifyweb/frontend/js_src/lib/components/Security/Institution.tsx b/specifyweb/frontend/js_src/lib/components/Security/Institution.tsx index 52771b16e6b..4919afef93d 100644 --- a/specifyweb/frontend/js_src/lib/components/Security/Institution.tsx +++ b/specifyweb/frontend/js_src/lib/components/Security/Institution.tsx @@ -206,6 +206,10 @@ function InstitutionView({ export function useAdmins(): | { readonly admins: ReadonlySet; + readonly adminUsers: RA<{ + readonly userId: number; + readonly userName: LocalizedString; + }>; readonly legacyAdmins: ReadonlySet; } | undefined { @@ -227,11 +231,19 @@ export function useAdmins(): errorMode: 'dismissible', }).then(({ data }) => ({ admins: new Set(data.sp7_admins.map(({ userid }) => userid)), + adminUsers: data.sp7_admins.map(({ userid, username }) => ({ + userId: userid, + userName: username, + })), legacyAdmins: new Set( data.sp6_admins.map(({ userid }) => userid) ), })) - : { admins: new Set(), legacyAdmins: new Set() }, + : { + admins: new Set(), + adminUsers: [], + legacyAdmins: new Set(), + }, [] ), false diff --git a/specifyweb/frontend/js_src/lib/components/Security/__tests__/Collection.test.ts b/specifyweb/frontend/js_src/lib/components/Security/__tests__/Collection.test.ts new file mode 100644 index 00000000000..45ab358e7f7 --- /dev/null +++ b/specifyweb/frontend/js_src/lib/components/Security/__tests__/Collection.test.ts @@ -0,0 +1,238 @@ +/** + * Tests for the displayUsers merging logic and user label construction + * introduced in CollectionView (Collection.tsx). + * + * The PR change computes displayUsers by merging mergeCollectionUsers output + * with institution admin users (from useAdmins), deduplicating, and sorting. + * It also computes per-user labels that prepend "Institution Admin" for admins. + */ + +import type { LocalizedString } from 'typesafe-i18n'; + +import { + computeDisplayUsers, + computeLabels, + type AdminsShape, + type RoleBase, + type UserRoles, +} from '../Collection'; + +// --------------------------------------------------------------------------- +// Test fixtures +// --------------------------------------------------------------------------- + +const L = (s: string): LocalizedString => s as LocalizedString; + +const INST_ADMIN_LABEL = L('Institution Admin'); + +const alice = { userId: 1, userName: L('alice'), roles: [] as RoleBase[] }; +const bob = { + userId: 2, + userName: L('bob'), + roles: [{ roleId: 10, roleName: L('Cataloger') }], +}; +const carol = { userId: 3, userName: L('carol'), roles: [] as RoleBase[] }; + +const adminsWithAliceAndCarol: AdminsShape = { + admins: new Set([1, 3]), + adminUsers: [ + { userId: 1, userName: L('alice') }, + { userId: 3, userName: L('carol') }, + ], +}; + +// --------------------------------------------------------------------------- +// displayUsers merging logic +// --------------------------------------------------------------------------- + +describe('displayUsers computation', () => { + test('returns undefined when mergedUsers is undefined', () => { + const result = computeDisplayUsers(undefined, adminsWithAliceAndCarol); + expect(result).toBeUndefined(); + }); + + test('returns mergedUsers unchanged when admins is undefined', () => { + const mergedUsers: UserRoles = [alice, bob]; + const result = computeDisplayUsers(mergedUsers, undefined); + // Should be exactly the same reference since the branch just returns mergedUsers + expect(result).toEqual(mergedUsers); + }); + + test('returns mergedUsers unchanged when both args are undefined', () => { + const result = computeDisplayUsers(undefined, undefined); + expect(result).toBeUndefined(); + }); + + test('includes all existing mergedUsers in output', () => { + const mergedUsers: UserRoles = [alice, bob]; + const result = computeDisplayUsers(mergedUsers, adminsWithAliceAndCarol); + expect(result?.some((u) => u.userId === alice.userId)).toBe(true); + expect(result?.some((u) => u.userId === bob.userId)).toBe(true); + }); + + test('adds admin user not already in mergedUsers', () => { + // carol (userId=3) is an admin but not in mergedUsers + const mergedUsers: UserRoles = [alice, bob]; + const result = computeDisplayUsers(mergedUsers, adminsWithAliceAndCarol); + expect(result?.some((u) => u.userId === carol.userId)).toBe(true); + }); + + test('does not duplicate an admin user already in mergedUsers', () => { + // alice (userId=1) is in both mergedUsers and adminUsers + const mergedUsers: UserRoles = [alice, bob]; + const result = computeDisplayUsers(mergedUsers, adminsWithAliceAndCarol); + const aliceEntries = result?.filter((u) => u.userId === alice.userId) ?? []; + expect(aliceEntries).toHaveLength(1); + }); + + test('added admin users have empty roles array', () => { + // carol is added from admins and should have no roles + const mergedUsers: UserRoles = [alice, bob]; + const result = computeDisplayUsers(mergedUsers, adminsWithAliceAndCarol); + const carolEntry = result?.find((u) => u.userId === carol.userId); + expect(carolEntry?.roles).toEqual([]); + }); + + test('result is sorted alphabetically by userName', () => { + // alice < bob < carol + const mergedUsers: UserRoles = [bob, alice]; // unsorted input + const result = computeDisplayUsers(mergedUsers, adminsWithAliceAndCarol); + const userNames = result?.map((u) => u.userName); + expect(userNames).toEqual(['alice', 'bob', 'carol']); + }); + + test('sort is stable and handles already-sorted input', () => { + const mergedUsers: UserRoles = [alice, bob]; + const adminsNoExtra: AdminsShape = { + admins: new Set([1]), + adminUsers: [{ userId: 1, userName: L('alice') }], + }; + const result = computeDisplayUsers(mergedUsers, adminsNoExtra); + const userNames = result?.map((u) => u.userName); + expect(userNames).toEqual(['alice', 'bob']); + }); + + test('handles empty mergedUsers with admin users to add', () => { + const result = computeDisplayUsers([], adminsWithAliceAndCarol); + expect(result).toHaveLength(2); + const userNames = result?.map((u) => u.userName); + expect(userNames).toEqual(['alice', 'carol']); // sorted + }); + + test('handles empty mergedUsers and empty adminUsers', () => { + const emptyAdmins: AdminsShape = { + admins: new Set(), + adminUsers: [], + }; + const result = computeDisplayUsers([], emptyAdmins); + expect(result).toEqual([]); + }); + + test('handles mergedUsers with users and no admin users to add', () => { + const noAdmins: AdminsShape = { + admins: new Set(), + adminUsers: [], + }; + const mergedUsers: UserRoles = [alice, bob]; + const result = computeDisplayUsers(mergedUsers, noAdmins); + expect(result).toHaveLength(2); + }); + + test('preserves roles on existing mergedUsers entries', () => { + const mergedUsers: UserRoles = [bob]; // bob has roles + const result = computeDisplayUsers(mergedUsers, adminsWithAliceAndCarol); + const bobEntry = result?.find((u) => u.userId === bob.userId); + expect(bobEntry?.roles).toEqual(bob.roles); + }); +}); + +// --------------------------------------------------------------------------- +// Per-user label construction +// --------------------------------------------------------------------------- + +describe('user label computation', () => { + test('admin user with no roles gets only the institution admin label', () => { + const labels = computeLabels( + adminsWithAliceAndCarol, + 1, // alice is an admin + [], + INST_ADMIN_LABEL + ); + expect(labels).toEqual([INST_ADMIN_LABEL]); + }); + + test('admin user with roles gets institution admin label prepended', () => { + const roles: RoleBase[] = [ + { roleId: 10, roleName: L('Cataloger') }, + { roleId: 11, roleName: L('Manager') }, + ]; + const labels = computeLabels( + adminsWithAliceAndCarol, + 1, // alice is an admin + roles, + INST_ADMIN_LABEL + ); + expect(labels).toEqual([INST_ADMIN_LABEL, L('Cataloger'), L('Manager')]); + }); + + test('non-admin user with roles gets only role names', () => { + const roles: RoleBase[] = [{ roleId: 10, roleName: L('Cataloger') }]; + const labels = computeLabels( + adminsWithAliceAndCarol, + 2, // bob is NOT an admin + roles, + INST_ADMIN_LABEL + ); + expect(labels).toEqual([L('Cataloger')]); + }); + + test('non-admin user with no roles produces empty labels', () => { + const labels = computeLabels( + adminsWithAliceAndCarol, + 2, // bob is NOT an admin + [], + INST_ADMIN_LABEL + ); + expect(labels).toEqual([]); + }); + + test('institution admin label is always first in the list', () => { + const roles: RoleBase[] = [ + { roleId: 5, roleName: L('Data Entry') }, + { roleId: 6, roleName: L('Viewer') }, + ]; + const labels = computeLabels( + adminsWithAliceAndCarol, + 3, // carol is an admin + roles, + INST_ADMIN_LABEL + ); + expect(labels[0]).toBe(INST_ADMIN_LABEL); + }); + + test('admins=undefined results in no admin label for any user', () => { + const labels = computeLabels( + undefined, + 1, // alice would be an admin, but admins is undefined + [], + INST_ADMIN_LABEL + ); + expect(labels).toEqual([]); + }); + + test('admins=undefined user with roles gets only role names', () => { + const roles: RoleBase[] = [{ roleId: 10, roleName: L('Cataloger') }]; + const labels = computeLabels(undefined, 1, roles, INST_ADMIN_LABEL); + expect(labels).toEqual([L('Cataloger')]); + }); + + test('user in admins.admins but not an adminUser still gets the label', () => { + // admins.admins is the Set used for label display; it's independent of adminUsers + const adminsOnlySet: AdminsShape = { + admins: new Set([99]), + adminUsers: [], + }; + const labels = computeLabels(adminsOnlySet, 99, [], INST_ADMIN_LABEL); + expect(labels).toEqual([INST_ADMIN_LABEL]); + }); +}); diff --git a/specifyweb/frontend/js_src/lib/components/Security/__tests__/Institution.test.ts b/specifyweb/frontend/js_src/lib/components/Security/__tests__/Institution.test.ts new file mode 100644 index 00000000000..37dfc97cb4d --- /dev/null +++ b/specifyweb/frontend/js_src/lib/components/Security/__tests__/Institution.test.ts @@ -0,0 +1,195 @@ +import { renderHook, waitFor } from '@testing-library/react'; +import type { LocalizedString } from 'typesafe-i18n'; + +import { useAsyncStateMock } from '../../../hooks/useAsyncStateMock'; +import { overrideAjax } from '../../../tests/ajax'; +import { hasPermission } from '../../Permissions/helpers'; +import { useAdmins } from '../Institution'; + +jest.mock('../../Permissions/helpers', () => ({ + hasPermission: jest.fn(), +})); + +const mockSetState = jest.fn(); + +jest.mock('../../../hooks/useAsyncState', () => { + const module = jest.requireActual('../../../hooks/useAsyncState'); + return { + ...module, + useAsyncState: (callback: () => Promise | undefined) => { + useAsyncStateMock(callback, mockSetState); + return [undefined, () => undefined]; + }, + }; +}); + +beforeEach(() => { + mockSetState.mockClear(); +}); + +describe('useAdmins - permission denied', () => { + beforeEach(() => { + (hasPermission as jest.Mock).mockReturnValue(false); + }); + + test('returns empty admins Set when permission is denied', async () => { + renderHook(() => useAdmins()); + + await waitFor(() => { + expect(mockSetState).toHaveBeenCalledTimes(1); + }); + + const result = mockSetState.mock.lastCall?.[0]; + expect(result?.admins).toEqual(new Set()); + }); + + test('returns empty adminUsers array when permission is denied', async () => { + renderHook(() => useAdmins()); + + await waitFor(() => { + expect(mockSetState).toHaveBeenCalledTimes(1); + }); + + const result = mockSetState.mock.lastCall?.[0]; + expect(result?.adminUsers).toEqual([]); + }); + + test('returns empty legacyAdmins Set when permission is denied', async () => { + renderHook(() => useAdmins()); + + await waitFor(() => { + expect(mockSetState).toHaveBeenCalledTimes(1); + }); + + const result = mockSetState.mock.lastCall?.[0]; + expect(result?.legacyAdmins).toEqual(new Set()); + }); +}); + +describe('useAdmins - permission granted with admin data', () => { + beforeEach(() => { + (hasPermission as jest.Mock).mockReturnValue(true); + }); + + overrideAjax('/permissions/list_admins/', { + sp7_admins: [ + { userid: 1, username: 'alice' as LocalizedString }, + { userid: 2, username: 'bob' as LocalizedString }, + ], + sp6_admins: [{ userid: 3, username: 'charlie' as LocalizedString }], + }); + + test('admins Set contains sp7_admin userids', async () => { + renderHook(() => useAdmins()); + + await waitFor(() => { + expect(mockSetState).toHaveBeenCalledTimes(1); + }); + + const result = mockSetState.mock.lastCall?.[0]; + expect(result?.admins.has(1)).toBe(true); + expect(result?.admins.has(2)).toBe(true); + expect(result?.admins.has(3)).toBe(false); + }); + + test('adminUsers maps sp7_admins using camelCase keys', async () => { + renderHook(() => useAdmins()); + + await waitFor(() => { + expect(mockSetState).toHaveBeenCalledTimes(1); + }); + + const result = mockSetState.mock.lastCall?.[0]; + expect(result?.adminUsers).toEqual([ + { userId: 1, userName: 'alice' }, + { userId: 2, userName: 'bob' }, + ]); + }); + + test('adminUsers does not include sp6 (legacy) admins', async () => { + renderHook(() => useAdmins()); + + await waitFor(() => { + expect(mockSetState).toHaveBeenCalledTimes(1); + }); + + const result = mockSetState.mock.lastCall?.[0]; + const hasCharlie = result?.adminUsers.some( + (u: { userId: number }) => u.userId === 3 + ); + expect(hasCharlie).toBe(false); + }); + + test('legacyAdmins Set contains sp6_admin userids', async () => { + renderHook(() => useAdmins()); + + await waitFor(() => { + expect(mockSetState).toHaveBeenCalledTimes(1); + }); + + const result = mockSetState.mock.lastCall?.[0]; + expect(result?.legacyAdmins.has(3)).toBe(true); + expect(result?.legacyAdmins.has(1)).toBe(false); + expect(result?.legacyAdmins.has(2)).toBe(false); + }); + + test('admins and legacyAdmins are disjoint Sets', async () => { + renderHook(() => useAdmins()); + + await waitFor(() => { + expect(mockSetState).toHaveBeenCalledTimes(1); + }); + + const result = mockSetState.mock.lastCall?.[0]; + const adminsArray = [...result.admins]; + const legacyAdminsArray = [...result.legacyAdmins]; + const overlap = adminsArray.filter((id: number) => + legacyAdminsArray.includes(id) + ); + expect(overlap).toEqual([]); + }); +}); + +describe('useAdmins - permission granted with empty admin lists', () => { + beforeEach(() => { + (hasPermission as jest.Mock).mockReturnValue(true); + }); + + overrideAjax('/permissions/list_admins/', { + sp7_admins: [], + sp6_admins: [], + }); + + test('admins Set is empty when no sp7 admins exist', async () => { + renderHook(() => useAdmins()); + + await waitFor(() => { + expect(mockSetState).toHaveBeenCalledTimes(1); + }); + + const result = mockSetState.mock.lastCall?.[0]; + expect(result?.admins).toEqual(new Set()); + }); + + test('adminUsers array is empty when no sp7 admins exist', async () => { + renderHook(() => useAdmins()); + + await waitFor(() => { + expect(mockSetState).toHaveBeenCalledTimes(1); + }); + + const result = mockSetState.mock.lastCall?.[0]; + expect(result?.adminUsers).toEqual([]); + }); + + test('legacyAdmins Set is empty when no sp6 admins exist', async () => { + renderHook(() => useAdmins()); + + await waitFor(() => { + expect(mockSetState).toHaveBeenCalledTimes(1); + }); + + const result = mockSetState.mock.lastCall?.[0]; + expect(result?.legacyAdmins).toEqual(new Set()); + }); +});