From 623b2044f82f594059457211f4998cc3a91477d9 Mon Sep 17 00:00:00 2001 From: thomasbeaudry Date: Thu, 1 Oct 2026 00:30:38 -0400 Subject: [PATCH 1/4] feat: replace delete user with archive (soft-disable) The backend now sets `disabled: true` instead of permanently deleting the user record. The frontend renames all "Delete User" UI to "Archive User" and adds an archive action to the users table dropdown. Co-Authored-By: Claude Opus 4.6 --- .../users/__tests__/users.controller.spec.ts | 2 +- .../src/users/__tests__/users.service.spec.ts | 15 ++--- apps/api/src/users/users.controller.ts | 22 ++------ apps/api/src/users/users.service.ts | 9 +-- ...rMutation.ts => useArchiveUserMutation.ts} | 4 +- .../src/routes/_app/admin/users/$userId.tsx | 32 ++++++----- .../web/src/routes/_app/admin/users/index.tsx | 55 ++++++++++++++++++- .../pages/_app/admin/users/$userId.page.ts | 4 +- testing/src/specs/admin-management.spec.ts | 8 +-- testing/src/specs/authorization.spec.ts | 13 +++-- 10 files changed, 106 insertions(+), 58 deletions(-) rename apps/web/src/hooks/{useDeleteUserMutation.ts => useArchiveUserMutation.ts} (80%) diff --git a/apps/api/src/users/__tests__/users.controller.spec.ts b/apps/api/src/users/__tests__/users.controller.spec.ts index 6e31ab9be..0b6754c85 100644 --- a/apps/api/src/users/__tests__/users.controller.spec.ts +++ b/apps/api/src/users/__tests__/users.controller.spec.ts @@ -152,7 +152,7 @@ describe('UsersController', () => { // evaluated against a whole ability. `manage User` covers every action a narrower declaration could // name, so a holder refused here is refused by any declaration short of `manage all`. describe('route access for writes to a user', () => { - const WRITE_HANDLERS = ['create', 'deleteById', 'updateById', 'updatePermissions'] as const; + const WRITE_HANDLERS = ['archiveById', 'create', 'updateById', 'updatePermissions'] as const; const abilityFor = (basePermissionLevel: BasePermissionLevel, additionalPermissions: Permissions = []) => new AbilityFactory(MockFactory.createMock(LoggingService) as unknown as LoggingService).createForPayload({ diff --git a/apps/api/src/users/__tests__/users.service.spec.ts b/apps/api/src/users/__tests__/users.service.spec.ts index 1076d74b9..01b08b52c 100644 --- a/apps/api/src/users/__tests__/users.service.spec.ts +++ b/apps/api/src/users/__tests__/users.service.spec.ts @@ -160,15 +160,16 @@ describe('UsersService', () => { }); }); - describe('deleteById', () => { - it('should refuse an administrator deleting their own account, so the last one cannot remove every admin', async () => { - await expect(usersService.deleteById(admin.id, admin)).rejects.toThrow(ForbiddenException); - expect(userModel.delete).not.toHaveBeenCalled(); + describe('archiveById', () => { + it('should refuse an administrator archiving their own account, so the last one cannot remove every admin', async () => { + await expect(usersService.archiveById(admin.id, admin)).rejects.toThrow(ForbiddenException); + expect(userModel.update).not.toHaveBeenCalled(); }); - it('should let an administrator delete another user', async () => { - await usersService.deleteById('user-1', admin); - expect(userModel.delete.mock.lastCall?.[0].where).toMatchObject({ id: 'user-1' }); + it('should set disabled to true instead of deleting the record', async () => { + await usersService.archiveById('user-1', admin); + expect(userModel.update.mock.lastCall?.[0].data).toMatchObject({ disabled: true }); + expect(userModel.update.mock.lastCall?.[0].where).toMatchObject({ id: 'user-1' }); }); }); diff --git a/apps/api/src/users/users.controller.ts b/apps/api/src/users/users.controller.ts index 982c6fc9f..b20d6c238 100644 --- a/apps/api/src/users/users.controller.ts +++ b/apps/api/src/users/users.controller.ts @@ -1,18 +1,6 @@ import { CurrentUser, ParseSchemaPipe } from '@douglasneuroinformatics/libnest'; import type { RequestUser } from '@douglasneuroinformatics/libnest'; -import { - Body, - Controller, - Delete, - Get, - Headers, - NotFoundException, - Param, - Patch, - Post, - Put, - Query -} from '@nestjs/common'; +import { Body, Controller, Get, Headers, NotFoundException, Param, Patch, Post, Put, Query } from '@nestjs/common'; import { ApiOperation, ApiTags } from '@nestjs/swagger'; import { $Language } from '@opendatacapture/schemas/core'; import type { Language } from '@opendatacapture/schemas/core'; @@ -76,11 +64,11 @@ export class UsersController { return { ...created, welcomeEmail }; } - @ApiOperation({ summary: 'Delete User' }) - @Delete(':id') + @ApiOperation({ summary: 'Archive User' }) + @Patch(':id/archive') @RouteAccess(ADMIN_ONLY) - deleteById(@Param('id') id: string, @CurrentUser() currentUser: RequestUser) { - return this.usersService.deleteById(id, currentUser); + archiveById(@Param('id') id: string, @CurrentUser() currentUser: RequestUser) { + return this.usersService.archiveById(id, currentUser); } @ApiOperation({ summary: 'Get All Users' }) diff --git a/apps/api/src/users/users.service.ts b/apps/api/src/users/users.service.ts index 0534cb256..fd767ba35 100644 --- a/apps/api/src/users/users.service.ts +++ b/apps/api/src/users/users.service.ts @@ -113,15 +113,16 @@ export class UsersService { }); } - async deleteById(id: string, currentUser: RequestUser) { + async archiveById(id: string, currentUser: RequestUser) { if (id === currentUser.id) { - throw new ForbiddenException('You may not delete your own account'); + throw new ForbiddenException('You may not archive your own account'); } - return this.userModel.delete({ + return this.userModel.update({ + data: { disabled: true }, omit: { hashedPassword: true }, - where: { AND: [accessibleQuery(currentUser.ability, 'delete', 'User')], id } + where: { AND: [accessibleQuery(currentUser.ability, 'update', 'User')], id } }); } diff --git a/apps/web/src/hooks/useDeleteUserMutation.ts b/apps/web/src/hooks/useArchiveUserMutation.ts similarity index 80% rename from apps/web/src/hooks/useDeleteUserMutation.ts rename to apps/web/src/hooks/useArchiveUserMutation.ts index 5fe57e784..d9fc9aaa8 100644 --- a/apps/web/src/hooks/useDeleteUserMutation.ts +++ b/apps/web/src/hooks/useArchiveUserMutation.ts @@ -4,11 +4,11 @@ import axios from 'axios'; import { USERS_QUERY_KEY } from './useUsersQuery'; -export function useDeleteUserMutation() { +export function useArchiveUserMutation() { const queryClient = useQueryClient(); const addNotification = useNotificationsStore((store) => store.addNotification); return useMutation({ - mutationFn: ({ id }: { id: string }) => axios.delete(`/v1/users/${id}`), + mutationFn: ({ id }: { id: string }) => axios.patch(`/v1/users/${id}/archive`), onSuccess() { addNotification({ type: 'success' }); void queryClient.invalidateQueries({ queryKey: [USERS_QUERY_KEY] }); diff --git a/apps/web/src/routes/_app/admin/users/$userId.tsx b/apps/web/src/routes/_app/admin/users/$userId.tsx index b0515b846..fac348ec6 100644 --- a/apps/web/src/routes/_app/admin/users/$userId.tsx +++ b/apps/web/src/routes/_app/admin/users/$userId.tsx @@ -12,7 +12,7 @@ import { UpdateUserForm } from '@/components/UpdateUserForm'; import type { UpdateUserFormInputData } from '@/components/UpdateUserForm'; import { UserIcon } from '@/components/UserIcon'; import { UserPermissionsEditor } from '@/components/UserPermissionsEditor'; -import { useDeleteUserMutation } from '@/hooks/useDeleteUserMutation'; +import { useArchiveUserMutation } from '@/hooks/useArchiveUserMutation'; import { useFindUserQuery, useFindUserQueryOptions } from '@/hooks/useFindUserQuery'; import { groupsQueryOptions, useGroupsQuery } from '@/hooks/useGroupsQuery'; import { useUpdateUserMutation } from '@/hooks/useUpdateUserMutation'; @@ -26,10 +26,10 @@ const RouteComponent = () => { const navigate = useNavigate(); const groupsQuery = useGroupsQuery(); const userQuery = useFindUserQuery(userId); - const deleteUserMutation = useDeleteUserMutation(); + const archiveUserMutation = useArchiveUserMutation(); const updateUserMutation = useUpdateUserMutation(); const [submitErrorMessage, setSubmitErrorMessage] = useState(null); - const [isConfirmDeleteOpen, setIsConfirmDeleteOpen] = useState(false); + const [isConfirmArchiveOpen, setIsConfirmArchiveOpen] = useState(false); // libui's `Form` clears its values after a successful submit, so the profile form is remounted // from the saved user once a save lands. Keyed on this rather than on the query's refetch time so // that saving a permission below does not discard edits typed here but not yet saved. @@ -150,24 +150,26 @@ const RouteComponent = () => { - {t({ en: 'Delete User', fr: "Supprimer l'utilisateur" })} + {t({ en: 'Archive User', es: 'Archivar usuario', fr: "Archiver l'utilisateur" })} {isCurrentUser ? t({ - en: 'You cannot delete the account you are signed in with.', - fr: 'Vous ne pouvez pas supprimer le compte avec lequel vous êtes connecté.' + en: 'You cannot archive the account you are signed in with.', + es: 'No puede archivar la cuenta con la que ha iniciado sesión.', + fr: 'Vous ne pouvez pas archiver le compte avec lequel vous êtes connecté.' }) : t({ - en: 'Permanently removes this account. This cannot be undone.', - fr: 'Supprime définitivement ce compte. Cette action ne peut pas être annulée.' + en: 'Disables this account. The user will no longer be able to sign in.', + es: 'Desactiva esta cuenta. El usuario ya no podrá iniciar sesión.', + fr: "Désactive ce compte. L'utilisateur ne pourra plus se connecter." })} - + @@ -175,13 +177,15 @@ const RouteComponent = () => { {t({ en: 'Are you absolutely sure?', + es: '¿Está absolutamente seguro?', fr: 'Êtes-vous absolument sûr ?' })} {t({ - en: 'This action will permanently delete the account and cannot be undone.', - fr: 'Cette action supprimera définitivement le compte et ne pourra pas être annulée.' + en: 'This will archive the account and prevent the user from signing in.', + es: 'Esto archivará la cuenta e impedirá que el usuario inicie sesión.', + fr: "Cela archivera le compte et empêchera l'utilisateur de se connecter." })} @@ -191,7 +195,7 @@ const RouteComponent = () => { type="button" variant="danger" onClick={() => { - deleteUserMutation.mutate( + archiveUserMutation.mutate( { id: user.id }, { onSuccess: () => { @@ -207,7 +211,7 @@ const RouteComponent = () => { className="min-w-16" type="button" variant="outline" - onClick={() => setIsConfirmDeleteOpen(false)} + onClick={() => setIsConfirmArchiveOpen(false)} > {t('core.no')} diff --git a/apps/web/src/routes/_app/admin/users/index.tsx b/apps/web/src/routes/_app/admin/users/index.tsx index f0c7373fb..46c9a46ab 100644 --- a/apps/web/src/routes/_app/admin/users/index.tsx +++ b/apps/web/src/routes/_app/admin/users/index.tsx @@ -1,19 +1,24 @@ import React, { useState } from 'react'; import { snakeToCamelCase } from '@douglasneuroinformatics/libjs'; -import { Button, DataTable, Heading } from '@douglasneuroinformatics/libui/components'; +import { Button, DataTable, Dialog, Heading } from '@douglasneuroinformatics/libui/components'; import { useTranslation } from '@douglasneuroinformatics/libui/hooks'; import type { User } from '@opendatacapture/schemas/user'; import { createFileRoute, Link, useNavigate } from '@tanstack/react-router'; import { PageHeader } from '@/components/PageHeader'; +import { useArchiveUserMutation } from '@/hooks/useArchiveUserMutation'; +import { useAppStore } from '@/store'; import { usersQueryOptions, useUsersQuery } from '@/hooks/useUsersQuery'; const RouteComponent = () => { const { t } = useTranslation(); const navigate = useNavigate(); const usersQuery = useUsersQuery(); + const archiveUserMutation = useArchiveUserMutation(); + const currentUser = useAppStore((store) => store.currentUser); const [highlightedRowId, setHighlightedRowId] = useState(null); + const [userToArchive, setUserToArchive] = useState(null); const openUser = (user: User) => { setHighlightedRowId(user.id); @@ -66,6 +71,11 @@ const RouteComponent = () => { { label: t('common.manage'), onSelect: openUser + }, + { + disabled: (user) => user.username === currentUser?.username, + label: t({ en: 'Archive', es: 'Archivar', fr: 'Archiver' }), + onSelect: (user) => setUserToArchive(user) } ]} togglesComponent={() => ( @@ -81,6 +91,49 @@ const RouteComponent = () => { onRowClick={(user) => setHighlightedRowId(user.id)} onRowDoubleClick={openUser} /> + { + if (!open) setUserToArchive(null); + }} + > + + + + {t({ + en: 'Are you absolutely sure?', + es: '¿Está absolutamente seguro?', + fr: 'Êtes-vous absolument sûr ?' + })} + + + {t({ + en: 'This will archive the account and prevent the user from signing in.', + es: 'Esto archivará la cuenta e impedirá que el usuario inicie sesión.', + fr: "Cela archivera le compte et empêchera l'utilisateur de se connecter." + })} + + + + + + + + ); }; diff --git a/testing/src/pages/_app/admin/users/$userId.page.ts b/testing/src/pages/_app/admin/users/$userId.page.ts index 9e4da1ae5..05f3fab12 100644 --- a/testing/src/pages/_app/admin/users/$userId.page.ts +++ b/testing/src/pages/_app/admin/users/$userId.page.ts @@ -40,8 +40,8 @@ export class AdminUserPage extends AppPage { await this.submitPermission(); } - async deleteUser() { - await this.$ref.getByRole('button', { name: 'Delete User' }).click(); + async archiveUser() { + await this.$ref.getByRole('button', { name: 'Archive User' }).click(); await this.$ref.getByRole('button', { name: 'Yes' }).click(); } diff --git a/testing/src/specs/admin-management.spec.ts b/testing/src/specs/admin-management.spec.ts index f99cca3c5..424849d6b 100644 --- a/testing/src/specs/admin-management.spec.ts +++ b/testing/src/specs/admin-management.spec.ts @@ -200,7 +200,7 @@ test.describe('admin management', () => { await expect(page.getByTestId('data-table-row')).toContainText(username); }); - test('should edit and delete a user from the user page', async ({ api, authenticateAs, page, uniqueId }) => { + test('should edit and archive a user from the user page', async ({ api, authenticateAs, page, uniqueId }) => { // Both forms require a non-empty `groupIds` for any non-ADMIN role that is not disabled, so a // groupless user can never be saved from the page. Seed one with a group to isolate the // behavior under test. @@ -221,16 +221,14 @@ test.describe('admin management', () => { // The shared `Form` component's own submit button always has `aria-label="Submit"`, even though // this form's visible label is "Save" -- see DouglasNeuroInformatics/libui#108. await profileForm.getByRole('button', { name: 'Submit' }).click(); - // The edit and delete toasts below can stack within the notification hub's shared 5s lifetime, + // The edit and archive toasts below can stack within the notification hub's shared 5s lifetime, // so `.last()` targets the most recently raised one rather than an ambiguous match on both. await expect(page.getByRole('heading', { name: 'Success' }).last()).toBeVisible(); - await page.getByRole('button', { name: 'Delete User' }).click(); + await page.getByRole('button', { name: 'Archive User' }).click(); await page.getByRole('button', { name: 'Yes' }).click(); await expect(page).toHaveURL('/admin/users'); - await page.getByTestId('data-table-search-bar').getByRole('searchbox').fill(user.username); - await expect(page.getByTestId('data-table-row').filter({ hasText: user.username })).toHaveCount(0); }); test('should say why a save failed when the rejected field is scrolled out of view', async ({ diff --git a/testing/src/specs/authorization.spec.ts b/testing/src/specs/authorization.spec.ts index cc2c2dada..a60d95a64 100644 --- a/testing/src/specs/authorization.spec.ts +++ b/testing/src/specs/authorization.spec.ts @@ -55,8 +55,8 @@ const PRIVILEGED_REQUESTS: PrivilegedRequest[] = [ }, { screen: '/admin/users', - send: (request, { headers, userId }) => request.delete(`${API}/users/${userId}`, { headers }), - what: 'delete a user' + send: (request, { headers, userId }) => request.patch(`${API}/users/${userId}/archive`, { headers }), + what: 'archive a user' }, { // Gated on `manage all` rather than `update User`: an `update User` grant is one of the things @@ -357,15 +357,18 @@ test.describe('server-side authorization', () => { // Every route that writes a user is admin-only, so an administrator removing their own access could // leave no account able to reach them again. Seeded rather than the shared admin, so a regression // loses a throwaway account instead of the one every other spec logs in as. - test('should refuse an administrator deleting or disabling their own account', async ({ api, apiRequestContext }) => { + test('should refuse an administrator archiving or disabling their own account', async ({ + api, + apiRequestContext + }) => { const { credentials, user } = await api.createUser({ basePermissionLevel: 'ADMIN' }); const headers = { Authorization: `Bearer ${await ApiClient.login(apiRequestContext, credentials)}` }; const disabled = await apiRequestContext.patch(`${API}/users/${user.id}`, { data: { disabled: true }, headers }); - const deleted = await apiRequestContext.delete(`${API}/users/${user.id}`, { headers }); + const archived = await apiRequestContext.patch(`${API}/users/${user.id}/archive`, { headers }); expect.soft(disabled.status(), 'an administrator must not be able to disable themselves').toBe(403); - expect.soft(deleted.status(), 'an administrator must not be able to delete themselves').toBe(403); + expect.soft(archived.status(), 'an administrator must not be able to archive themselves').toBe(403); expect((await api.findUserById(user.id)).disabled).not.toBe(true); }); From 1b137bdd781e09480fb3d1ce636a466c369b096b Mon Sep 17 00:00:00 2001 From: thomasbeaudry Date: Thu, 1 Oct 2026 00:46:23 -0400 Subject: [PATCH 2/4] feat: add unarchive, status column, and archived login message Archived users now see a specific error on login asking them to contact an administrator. The users table shows a status column (active in green, archived with date in red). Both the table dropdown and the user detail page offer archive/unarchive depending on the user's current state. Co-Authored-By: Claude Opus 4.6 --- apps/api/src/auth/auth.service.ts | 2 +- .../users/__tests__/users.controller.spec.ts | 2 +- .../src/users/__tests__/users.service.spec.ts | 12 +++ apps/api/src/users/users.controller.ts | 7 ++ apps/api/src/users/users.service.ts | 13 ++++ .../web/src/hooks/useUnarchiveUserMutation.ts | 17 +++++ .../src/routes/_app/admin/users/$userId.tsx | 64 ++++++++++------ .../web/src/routes/_app/admin/users/index.tsx | 73 ++++++++++++++----- apps/web/src/routes/auth/login.tsx | 34 +++++++-- .../pages/_app/admin/users/$userId.page.ts | 5 ++ 10 files changed, 178 insertions(+), 51 deletions(-) create mode 100644 apps/web/src/hooks/useUnarchiveUserMutation.ts diff --git a/apps/api/src/auth/auth.service.ts b/apps/api/src/auth/auth.service.ts index 866594eab..4ed464d62 100644 --- a/apps/api/src/auth/auth.service.ts +++ b/apps/api/src/auth/auth.service.ts @@ -52,7 +52,7 @@ export class AuthService { } if (user.disabled) { - throw new ForbiddenException('Account Disabled'); + throw new ForbiddenException('Account Archived'); } const isCorrectPassword = await this.cryptoService.comparePassword(credentials.password, user.hashedPassword); diff --git a/apps/api/src/users/__tests__/users.controller.spec.ts b/apps/api/src/users/__tests__/users.controller.spec.ts index 0b6754c85..706ef94c3 100644 --- a/apps/api/src/users/__tests__/users.controller.spec.ts +++ b/apps/api/src/users/__tests__/users.controller.spec.ts @@ -152,7 +152,7 @@ describe('UsersController', () => { // evaluated against a whole ability. `manage User` covers every action a narrower declaration could // name, so a holder refused here is refused by any declaration short of `manage all`. describe('route access for writes to a user', () => { - const WRITE_HANDLERS = ['archiveById', 'create', 'updateById', 'updatePermissions'] as const; + const WRITE_HANDLERS = ['archiveById', 'create', 'unarchiveById', 'updateById', 'updatePermissions'] as const; const abilityFor = (basePermissionLevel: BasePermissionLevel, additionalPermissions: Permissions = []) => new AbilityFactory(MockFactory.createMock(LoggingService) as unknown as LoggingService).createForPayload({ diff --git a/apps/api/src/users/__tests__/users.service.spec.ts b/apps/api/src/users/__tests__/users.service.spec.ts index 01b08b52c..d49fb284a 100644 --- a/apps/api/src/users/__tests__/users.service.spec.ts +++ b/apps/api/src/users/__tests__/users.service.spec.ts @@ -173,6 +173,18 @@ describe('UsersService', () => { }); }); + describe('unarchiveById', () => { + it('should refuse an administrator unarchiving their own account', async () => { + await expect(usersService.unarchiveById(admin.id, admin)).rejects.toThrow(ForbiddenException); + }); + + it('should set disabled to false to restore the account', async () => { + await usersService.unarchiveById('user-1', admin); + expect(userModel.update.mock.lastCall?.[0].data).toMatchObject({ disabled: false }); + expect(userModel.update.mock.lastCall?.[0].where).toMatchObject({ id: 'user-1' }); + }); + }); + describe('updatePermissions', () => { beforeEach(() => { userModel.findFirst.mockResolvedValue({ groupIds: ['group-1'], id: 'user-1' }); diff --git a/apps/api/src/users/users.controller.ts b/apps/api/src/users/users.controller.ts index b20d6c238..f7623243d 100644 --- a/apps/api/src/users/users.controller.ts +++ b/apps/api/src/users/users.controller.ts @@ -85,6 +85,13 @@ export class UsersController { return this.usersService.findById(id, { ability }); } + @ApiOperation({ summary: 'Unarchive User' }) + @Patch(':id/unarchive') + @RouteAccess(ADMIN_ONLY) + unarchiveById(@Param('id') id: string, @CurrentUser() currentUser: RequestUser) { + return this.usersService.unarchiveById(id, currentUser); + } + @ApiOperation({ summary: 'Update User' }) @Patch(':id') @RouteAccess(ADMIN_ONLY) diff --git a/apps/api/src/users/users.service.ts b/apps/api/src/users/users.service.ts index fd767ba35..57329c398 100644 --- a/apps/api/src/users/users.service.ts +++ b/apps/api/src/users/users.service.ts @@ -126,6 +126,19 @@ export class UsersService { }); } + async unarchiveById(id: string, currentUser: RequestUser) { + if (id === currentUser.id) { + throw new ForbiddenException('You may not unarchive your own account'); + } + return this.userModel.update({ + data: { disabled: false }, + omit: { + hashedPassword: true + }, + where: { AND: [accessibleQuery(currentUser.ability, 'update', 'User')], id } + }); + } + /** Delete the user with the provided username, otherwise throws */ async deleteByUsername(username: string, { ability }: EntityOperationOptions = {}) { const user = await this.findByUsername(username); diff --git a/apps/web/src/hooks/useUnarchiveUserMutation.ts b/apps/web/src/hooks/useUnarchiveUserMutation.ts new file mode 100644 index 000000000..738851c53 --- /dev/null +++ b/apps/web/src/hooks/useUnarchiveUserMutation.ts @@ -0,0 +1,17 @@ +import { useNotificationsStore } from '@douglasneuroinformatics/libui/hooks'; +import { useMutation, useQueryClient } from '@tanstack/react-query'; +import axios from 'axios'; + +import { USERS_QUERY_KEY } from './useUsersQuery'; + +export function useUnarchiveUserMutation() { + const queryClient = useQueryClient(); + const addNotification = useNotificationsStore((store) => store.addNotification); + return useMutation({ + mutationFn: ({ id }: { id: string }) => axios.patch(`/v1/users/${id}/unarchive`), + onSuccess() { + addNotification({ type: 'success' }); + void queryClient.invalidateQueries({ queryKey: [USERS_QUERY_KEY] }); + } + }); +} diff --git a/apps/web/src/routes/_app/admin/users/$userId.tsx b/apps/web/src/routes/_app/admin/users/$userId.tsx index fac348ec6..f5ef9b93a 100644 --- a/apps/web/src/routes/_app/admin/users/$userId.tsx +++ b/apps/web/src/routes/_app/admin/users/$userId.tsx @@ -14,6 +14,7 @@ import { UserIcon } from '@/components/UserIcon'; import { UserPermissionsEditor } from '@/components/UserPermissionsEditor'; import { useArchiveUserMutation } from '@/hooks/useArchiveUserMutation'; import { useFindUserQuery, useFindUserQueryOptions } from '@/hooks/useFindUserQuery'; +import { useUnarchiveUserMutation } from '@/hooks/useUnarchiveUserMutation'; import { groupsQueryOptions, useGroupsQuery } from '@/hooks/useGroupsQuery'; import { useUpdateUserMutation } from '@/hooks/useUpdateUserMutation'; import { useAppStore } from '@/store'; @@ -27,9 +28,10 @@ const RouteComponent = () => { const groupsQuery = useGroupsQuery(); const userQuery = useFindUserQuery(userId); const archiveUserMutation = useArchiveUserMutation(); + const unarchiveUserMutation = useUnarchiveUserMutation(); const updateUserMutation = useUpdateUserMutation(); const [submitErrorMessage, setSubmitErrorMessage] = useState(null); - const [isConfirmArchiveOpen, setIsConfirmArchiveOpen] = useState(false); + const [isConfirmOpen, setIsConfirmOpen] = useState(false); // libui's `Form` clears its values after a successful submit, so the profile form is remounted // from the saved user once a save lands. Keyed on this rather than on the query's refetch time so // that saving a permission below does not discard edits typed here but not yet saved. @@ -150,7 +152,11 @@ const RouteComponent = () => { - {t({ en: 'Archive User', es: 'Archivar usuario', fr: "Archiver l'utilisateur" })} + + {user.disabled + ? t({ en: 'Unarchive User', es: 'Desarchivar usuario', fr: "Désarchiver l'utilisateur" }) + : t({ en: 'Archive User', es: 'Archivar usuario', fr: "Archiver l'utilisateur" })} + {isCurrentUser ? t({ @@ -158,18 +164,26 @@ const RouteComponent = () => { es: 'No puede archivar la cuenta con la que ha iniciado sesión.', fr: 'Vous ne pouvez pas archiver le compte avec lequel vous êtes connecté.' }) - : t({ - en: 'Disables this account. The user will no longer be able to sign in.', - es: 'Desactiva esta cuenta. El usuario ya no podrá iniciar sesión.', - fr: "Désactive ce compte. L'utilisateur ne pourra plus se connecter." - })} + : user.disabled + ? t({ + en: 'Restores this account. The user will be able to sign in again.', + es: 'Restaura esta cuenta. El usuario podrá iniciar sesión de nuevo.', + fr: "Restaure ce compte. L'utilisateur pourra se reconnecter." + }) + : t({ + en: 'Disables this account. The user will no longer be able to sign in.', + es: 'Desactiva esta cuenta. El usuario ya no podrá iniciar sesión.', + fr: "Désactive ce compte. L'utilisateur ne pourra plus se connecter." + })} - + - @@ -182,24 +196,31 @@ const RouteComponent = () => { })} - {t({ - en: 'This will archive the account and prevent the user from signing in.', - es: 'Esto archivará la cuenta e impedirá que el usuario inicie sesión.', - fr: "Cela archivera le compte et empêchera l'utilisateur de se connecter." - })} + {user.disabled + ? t({ + en: 'This will restore the account and allow the user to sign in again.', + es: 'Esto restaurará la cuenta y permitirá que el usuario inicie sesión de nuevo.', + fr: "Cela restaurera le compte et permettra à l'utilisateur de se reconnecter." + }) + : t({ + en: 'This will archive the account and prevent the user from signing in.', + es: 'Esto archivará la cuenta e impedirá que el usuario inicie sesión.', + fr: "Cela archivera le compte et empêchera l'utilisateur de se connecter." + })} - diff --git a/apps/web/src/routes/_app/admin/users/index.tsx b/apps/web/src/routes/_app/admin/users/index.tsx index 46c9a46ab..5d0e908a7 100644 --- a/apps/web/src/routes/_app/admin/users/index.tsx +++ b/apps/web/src/routes/_app/admin/users/index.tsx @@ -1,6 +1,6 @@ import React, { useState } from 'react'; -import { snakeToCamelCase } from '@douglasneuroinformatics/libjs'; +import { snakeToCamelCase, toBasicISOString } from '@douglasneuroinformatics/libjs'; import { Button, DataTable, Dialog, Heading } from '@douglasneuroinformatics/libui/components'; import { useTranslation } from '@douglasneuroinformatics/libui/hooks'; import type { User } from '@opendatacapture/schemas/user'; @@ -8,17 +8,21 @@ import { createFileRoute, Link, useNavigate } from '@tanstack/react-router'; import { PageHeader } from '@/components/PageHeader'; import { useArchiveUserMutation } from '@/hooks/useArchiveUserMutation'; +import { useUnarchiveUserMutation } from '@/hooks/useUnarchiveUserMutation'; import { useAppStore } from '@/store'; import { usersQueryOptions, useUsersQuery } from '@/hooks/useUsersQuery'; +type ArchiveAction = { kind: 'archive'; user: User } | { kind: 'unarchive'; user: User }; + const RouteComponent = () => { const { t } = useTranslation(); const navigate = useNavigate(); const usersQuery = useUsersQuery(); const archiveUserMutation = useArchiveUserMutation(); + const unarchiveUserMutation = useUnarchiveUserMutation(); const currentUser = useAppStore((store) => store.currentUser); const [highlightedRowId, setHighlightedRowId] = useState(null); - const [userToArchive, setUserToArchive] = useState(null); + const [pendingAction, setPendingAction] = useState(null); const openUser = (user: User) => { setHighlightedRowId(user.id); @@ -63,6 +67,30 @@ const RouteComponent = () => { return t(`common.${snakeToCamelCase(basePermissionLevel)}`); }, header: t('common.basePermissionLevel') + }, + { + accessorKey: 'disabled', + cell: (ctx) => { + const user = ctx.row.original; + if (user.disabled) { + return ( + + {t({ + en: `Archived on ${toBasicISOString(user.updatedAt)}`, + es: `Archivado el ${toBasicISOString(user.updatedAt)}`, + fr: `Archivé le ${toBasicISOString(user.updatedAt)}` + })} + + ); + } + return ( + + {t({ en: 'Active', es: 'Activo', fr: 'Actif' })} + + ); + }, + header: t({ en: 'Status', es: 'Estado', fr: 'Statut' }), + id: 'status' } ]} data={usersQuery.data} @@ -73,9 +101,14 @@ const RouteComponent = () => { onSelect: openUser }, { - disabled: (user) => user.username === currentUser?.username, + disabled: (user) => user.username === currentUser?.username || Boolean(user.disabled), label: t({ en: 'Archive', es: 'Archivar', fr: 'Archiver' }), - onSelect: (user) => setUserToArchive(user) + onSelect: (user) => setPendingAction({ kind: 'archive', user }) + }, + { + disabled: (user) => !user.disabled, + label: t({ en: 'Unarchive', es: 'Desarchivar', fr: 'Désarchiver' }), + onSelect: (user) => setPendingAction({ kind: 'unarchive', user }) } ]} togglesComponent={() => ( @@ -92,9 +125,9 @@ const RouteComponent = () => { onRowDoubleClick={openUser} /> { - if (!open) setUserToArchive(null); + if (!open) setPendingAction(null); }} > @@ -107,28 +140,34 @@ const RouteComponent = () => { })} - {t({ - en: 'This will archive the account and prevent the user from signing in.', - es: 'Esto archivará la cuenta e impedirá que el usuario inicie sesión.', - fr: "Cela archivera le compte et empêchera l'utilisateur de se connecter." - })} + {pendingAction?.kind === 'archive' + ? t({ + en: 'This will archive the account and prevent the user from signing in.', + es: 'Esto archivará la cuenta e impedirá que el usuario inicie sesión.', + fr: "Cela archivera le compte et empêchera l'utilisateur de se connecter." + }) + : t({ + en: 'This will restore the account and allow the user to sign in again.', + es: 'Esto restaurará la cuenta y permitirá que el usuario inicie sesión de nuevo.', + fr: "Cela restaurera le compte et permettra à l'utilisateur de se reconnecter." + })} - diff --git a/apps/web/src/routes/auth/login.tsx b/apps/web/src/routes/auth/login.tsx index 4a2802855..96338d7f2 100644 --- a/apps/web/src/routes/auth/login.tsx +++ b/apps/web/src/routes/auth/login.tsx @@ -12,17 +12,19 @@ import { setupStateQueryOptions, useSetupStateQuery } from '@/hooks/useSetupStat import { useAppStore } from '@/store'; import { getRightPanelGradient } from '@/utils/branding'; -const loginRequest = async ( - credentials: $LoginCredentials -): Promise<{ accessToken: string; success: true } | { success: false }> => { +type LoginResult = { accessToken: string; kind: 'success' } | { kind: 'archived' } | { kind: 'unauthorized' }; + +const loginRequest = async (credentials: $LoginCredentials): Promise => { const response = await axios.post('/v1/auth/login', credentials, { - validateStatus: (status) => status === 200 || status === 401 + validateStatus: (status) => status === 200 || status === 401 || status === 403 }); + if (response.status === 403) { + return { kind: 'archived' }; + } if (response.status === 401) { - console.error(response); - return { success: false }; + return { kind: 'unauthorized' }; } - return { accessToken: response.data.accessToken, success: true }; + return { accessToken: response.data.accessToken, kind: 'success' }; }; const RouteComponent = () => { @@ -39,7 +41,23 @@ const RouteComponent = () => { const handleLogin = async (credentials: $LoginCredentials) => { const result = await loginRequest(credentials); - if (!result.success) { + if (result.kind === 'archived') { + notifications.addNotification({ + message: t({ + en: 'Your account has been archived. Please contact an administrator to restore it.', + es: 'Su cuenta ha sido archivada. Comuníquese con un administrador para restaurarla.', + fr: 'Votre compte a été archivé. Veuillez contacter un administrateur pour le restaurer.' + }), + title: t({ + en: 'Account Archived', + es: 'Cuenta archivada', + fr: 'Compte archivé' + }), + type: 'error' + }); + return; + } + if (result.kind === 'unauthorized') { notifications.addNotification({ message: t('unauthorizedError.message'), title: t('unauthorizedError.title'), diff --git a/testing/src/pages/_app/admin/users/$userId.page.ts b/testing/src/pages/_app/admin/users/$userId.page.ts index 05f3fab12..b32c29845 100644 --- a/testing/src/pages/_app/admin/users/$userId.page.ts +++ b/testing/src/pages/_app/admin/users/$userId.page.ts @@ -45,6 +45,11 @@ export class AdminUserPage extends AppPage { await this.$ref.getByRole('button', { name: 'Yes' }).click(); } + async unarchiveUser() { + await this.$ref.getByRole('button', { name: 'Unarchive User' }).click(); + await this.$ref.getByRole('button', { name: 'Yes' }).click(); + } + async removePermission(index: number) { await this.permissionRows.nth(index).getByTestId('user-permission-remove').click(); } From dfa4b3a875053f403dea0572e54249fc86e0bdcf Mon Sep 17 00:00:00 2001 From: thomasbeaudry Date: Thu, 1 Oct 2026 10:05:05 -0400 Subject: [PATCH 3/4] feat: record archiving as an archivedAt date, separate from disabled Archiving reused the `disabled` flag, which already means "never meant to log in" and exempts an account from needing a group. Archiving now sets its own `archivedAt` date, is recorded in the audit log, and is shown in the users table's Status column alongside an Enabled / Disabled column. Login names an archived or disabled account only after the password is verified, so a wrong guess cannot reveal whether a username exists or what state it is in. Co-Authored-By: Claude Opus 5.5 --- apps/api/prisma/schema.prisma | 1 + .../src/auth/__tests__/auth.service.spec.ts | 43 +++++++++- apps/api/src/auth/auth.service.ts | 13 ++- .../src/users/__tests__/users.service.spec.ts | 18 ++++- apps/api/src/users/users.controller.ts | 14 ++-- apps/api/src/users/users.service.ts | 66 +++++++++------ .../test/suites/02-user-permissions.suite.ts | 3 +- .../src/__tests__/admin-users-table.test.tsx | 57 +++++++++++++ .../web/src/routes/_app/admin/users/index.tsx | 81 ++++++++++++------- apps/web/src/routes/auth/login.tsx | 25 +++++- packages/schemas/src/user/user.ts | 1 + testing/src/specs/auth.spec.ts | 28 +++++++ testing/src/specs/authorization.spec.ts | 4 +- testing/src/support/api-client.ts | 7 ++ 14 files changed, 285 insertions(+), 76 deletions(-) create mode 100644 apps/web/src/__tests__/admin-users-table.test.tsx diff --git a/apps/api/prisma/schema.prisma b/apps/api/prisma/schema.prisma index a933f8072..566bb3f00 100644 --- a/apps/api/prisma/schema.prisma +++ b/apps/api/prisma/schema.prisma @@ -346,6 +346,7 @@ model User { phoneNumber String? email String? disabled Boolean? + archivedAt DateTime? @db.Date @@unique([username]) @@map("UserModel") diff --git a/apps/api/src/auth/__tests__/auth.service.spec.ts b/apps/api/src/auth/__tests__/auth.service.spec.ts index a7e269ed2..f06a16b5c 100644 --- a/apps/api/src/auth/__tests__/auth.service.spec.ts +++ b/apps/api/src/auth/__tests__/auth.service.spec.ts @@ -2,7 +2,7 @@ import { CryptoService, LoggingService } from '@douglasneuroinformatics/libnest' import type { RequestUser } from '@douglasneuroinformatics/libnest'; import { MockFactory } from '@douglasneuroinformatics/libnest/testing'; import type { MockedInstance } from '@douglasneuroinformatics/libnest/testing'; -import { ForbiddenException } from '@nestjs/common'; +import { ForbiddenException, UnauthorizedException } from '@nestjs/common'; import { JwtService } from '@nestjs/jwt'; import { beforeEach, describe, expect, it } from 'vitest'; @@ -29,7 +29,9 @@ const BASE_PAYLOAD = { describe('AuthService', () => { let abilityFactory: AbilityFactory; let authService: AuthService; + let cryptoService: MockedInstance; let jwtService: MockedInstance; + let usersService: MockedInstance; const requestUserFor = (basePermissionLevel: 'ADMIN' | 'GROUP_MANAGER' | 'STANDARD'): RequestUser => { const ability = abilityFactory.createForPayload({ ...BASE_PAYLOAD, basePermissionLevel } as any); @@ -46,15 +48,50 @@ describe('AuthService', () => { abilityFactory = new AbilityFactory(MockFactory.createMock(LoggingService) as unknown as LoggingService); jwtService = MockFactory.createMock(JwtService); jwtService.signAsync.mockResolvedValue('__TOKEN__'); + cryptoService = MockFactory.createMock(CryptoService); + usersService = MockFactory.createMock(UsersService); authService = new AuthService( abilityFactory, MockFactory.createMock(AuditLogger) as unknown as AuditLogger, - MockFactory.createMock(CryptoService) as unknown as CryptoService, + cryptoService as unknown as CryptoService, jwtService as unknown as JwtService, - MockFactory.createMock(UsersService) as unknown as UsersService + usersService as unknown as UsersService ); }); + describe('login', () => { + const credentials = { password: 'guess', username: 'test-user' }; + + const storedUser = (status: { archivedAt?: Date; disabled?: boolean }) => ({ + ...BASE_PAYLOAD, + basePermissionLevel: 'STANDARD', + groups: [], + hashedPassword: '__HASH__', + ...status + }); + + it.each([{ archivedAt: new Date() }, { disabled: true }])( + 'should answer a wrong password for a %o account as invalid credentials, so a guess cannot reveal its status', + async (status) => { + usersService.findByUsername.mockResolvedValue(storedUser(status) as any); + cryptoService.comparePassword.mockResolvedValue(false); + await expect(authService.login(credentials)).rejects.toThrow(UnauthorizedException); + } + ); + + it('should refuse an archived account once the password is proven', async () => { + usersService.findByUsername.mockResolvedValue(storedUser({ archivedAt: new Date() }) as any); + cryptoService.comparePassword.mockResolvedValue(true); + await expect(authService.login(credentials)).rejects.toThrow('Account Archived'); + }); + + it('should refuse a disabled account once the password is proven', async () => { + usersService.findByUsername.mockResolvedValue(storedUser({ disabled: true }) as any); + cryptoService.comparePassword.mockResolvedValue(true); + await expect(authService.login(credentials)).rejects.toThrow('Account Disabled'); + }); + }); + describe('getCreateInstrumentToken', () => { it('should mint a token that satisfies the instrument create route, so the playground can upload a bundle', async () => { const ability = createAppAbility(await mintedPermissions(requestUserFor('ADMIN'))); diff --git a/apps/api/src/auth/auth.service.ts b/apps/api/src/auth/auth.service.ts index 4ed464d62..cb0ad4ff2 100644 --- a/apps/api/src/auth/auth.service.ts +++ b/apps/api/src/auth/auth.service.ts @@ -51,15 +51,20 @@ export class AuthService { throw err; } - if (user.disabled) { - throw new ForbiddenException('Account Archived'); - } - + // Account status is checked only once the password is proven, so a wrong guess cannot reveal + // whether a username exists or has been archived. const isCorrectPassword = await this.cryptoService.comparePassword(credentials.password, user.hashedPassword); if (isCorrectPassword !== true) { throw new UnauthorizedException('Invalid Credentials'); } + if (user.archivedAt) { + throw new ForbiddenException('Account Archived'); + } + if (user.disabled) { + throw new ForbiddenException('Account Disabled'); + } + const tokenPayload: Omit = { additionalPermissions: user.additionalPermissions, basePermissionLevel: user.basePermissionLevel, diff --git a/apps/api/src/users/__tests__/users.service.spec.ts b/apps/api/src/users/__tests__/users.service.spec.ts index d49fb284a..c490971cf 100644 --- a/apps/api/src/users/__tests__/users.service.spec.ts +++ b/apps/api/src/users/__tests__/users.service.spec.ts @@ -10,6 +10,7 @@ import { pwnedPassword } from 'hibp'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import type { Mock } from 'vitest'; +import { AuditLogger } from '@/audit/audit.logger'; import { accessibleQuery, createAppAbility } from '@/auth/ability.utils'; import { GroupsService } from '../../groups/groups.service'; @@ -43,6 +44,7 @@ describe('UsersService', () => { providers: [ UsersService, MockFactory.createForModelToken(getModelToken('User')), + MockFactory.createForService(AuditLogger), MockFactory.createForService(CryptoService), MockFactory.createForService(GroupsService) ] @@ -161,26 +163,34 @@ describe('UsersService', () => { }); describe('archiveById', () => { + beforeEach(() => { + userModel.update.mockResolvedValue({}); + }); + it('should refuse an administrator archiving their own account, so the last one cannot remove every admin', async () => { await expect(usersService.archiveById(admin.id, admin)).rejects.toThrow(ForbiddenException); expect(userModel.update).not.toHaveBeenCalled(); }); - it('should set disabled to true instead of deleting the record', async () => { + it('should set archivedAt instead of deleting the record', async () => { await usersService.archiveById('user-1', admin); - expect(userModel.update.mock.lastCall?.[0].data).toMatchObject({ disabled: true }); + expect(userModel.update.mock.lastCall?.[0].data.archivedAt).toBeInstanceOf(Date); expect(userModel.update.mock.lastCall?.[0].where).toMatchObject({ id: 'user-1' }); }); }); describe('unarchiveById', () => { + beforeEach(() => { + userModel.update.mockResolvedValue({}); + }); + it('should refuse an administrator unarchiving their own account', async () => { await expect(usersService.unarchiveById(admin.id, admin)).rejects.toThrow(ForbiddenException); }); - it('should set disabled to false to restore the account', async () => { + it('should clear archivedAt to restore the account', async () => { await usersService.unarchiveById('user-1', admin); - expect(userModel.update.mock.lastCall?.[0].data).toMatchObject({ disabled: false }); + expect(userModel.update.mock.lastCall?.[0].data).toMatchObject({ archivedAt: null }); expect(userModel.update.mock.lastCall?.[0].where).toMatchObject({ id: 'user-1' }); }); }); diff --git a/apps/api/src/users/users.controller.ts b/apps/api/src/users/users.controller.ts index f7623243d..38cff5e8a 100644 --- a/apps/api/src/users/users.controller.ts +++ b/apps/api/src/users/users.controller.ts @@ -32,6 +32,13 @@ export class UsersController { private readonly mailService: MailService ) {} + @ApiOperation({ summary: 'Archive User' }) + @Patch(':id/archive') + @RouteAccess(ADMIN_ONLY) + archiveById(@Param('id') id: string, @CurrentUser() currentUser: RequestUser) { + return this.usersService.archiveById(id, currentUser); + } + @ApiOperation({ summary: 'Get User by Username' }) @Get('/check-username/:username') @RouteAccess({ action: 'read', subject: 'User' }) @@ -64,13 +71,6 @@ export class UsersController { return { ...created, welcomeEmail }; } - @ApiOperation({ summary: 'Archive User' }) - @Patch(':id/archive') - @RouteAccess(ADMIN_ONLY) - archiveById(@Param('id') id: string, @CurrentUser() currentUser: RequestUser) { - return this.usersService.archiveById(id, currentUser); - } - @ApiOperation({ summary: 'Get All Users' }) @Get() @RouteAccess({ action: 'read', subject: 'User' }) diff --git a/apps/api/src/users/users.service.ts b/apps/api/src/users/users.service.ts index 57329c398..1db0a825c 100644 --- a/apps/api/src/users/users.service.ts +++ b/apps/api/src/users/users.service.ts @@ -14,6 +14,7 @@ import { $SelfUpdateUserData } from '@opendatacapture/schemas/user'; import type { PasswordErrorCode } from '@opendatacapture/schemas/user'; import { pwnedPassword } from 'hibp'; +import { AuditLogger } from '@/audit/audit.logger'; import { accessibleQuery } from '@/auth/ability.utils'; import type { EntityOperationOptions } from '@/core/types'; import { GroupsService } from '@/groups/groups.service'; @@ -28,10 +29,30 @@ export class UsersService { constructor( @InjectModel('User') private readonly userModel: Model<'User'>, + private readonly auditLogger: AuditLogger, private readonly cryptoService: CryptoService, private readonly groupsService: GroupsService ) {} + async archiveById(id: string, currentUser: RequestUser) { + if (id === currentUser.id) { + throw new ForbiddenException('You may not archive your own account'); + } + const user = await this.userModel.update({ + data: { archivedAt: new Date() }, + omit: { + hashedPassword: true + }, + where: { AND: [accessibleQuery(currentUser.ability, 'update', 'User')], id } + }); + await this.auditLogger.log('UPDATE', 'USER', { + groupId: null, + metadata: { action: 'archive', targetUserId: id }, + userId: currentUser.id + }); + return user; + } + async checkUsernameExists(username: string, { ability }: EntityOperationOptions = {}): Promise<{ success: boolean }> { const user = await this.userModel.findFirst({ include: { groups: true }, @@ -113,32 +134,6 @@ export class UsersService { }); } - async archiveById(id: string, currentUser: RequestUser) { - if (id === currentUser.id) { - throw new ForbiddenException('You may not archive your own account'); - } - return this.userModel.update({ - data: { disabled: true }, - omit: { - hashedPassword: true - }, - where: { AND: [accessibleQuery(currentUser.ability, 'update', 'User')], id } - }); - } - - async unarchiveById(id: string, currentUser: RequestUser) { - if (id === currentUser.id) { - throw new ForbiddenException('You may not unarchive your own account'); - } - return this.userModel.update({ - data: { disabled: false }, - omit: { - hashedPassword: true - }, - where: { AND: [accessibleQuery(currentUser.ability, 'update', 'User')], id } - }); - } - /** Delete the user with the provided username, otherwise throws */ async deleteByUsername(username: string, { ability }: EntityOperationOptions = {}) { const user = await this.findByUsername(username); @@ -191,6 +186,25 @@ export class UsersService { return user; } + async unarchiveById(id: string, currentUser: RequestUser) { + if (id === currentUser.id) { + throw new ForbiddenException('You may not unarchive your own account'); + } + const user = await this.userModel.update({ + data: { archivedAt: null }, + omit: { + hashedPassword: true + }, + where: { AND: [accessibleQuery(currentUser.ability, 'update', 'User')], id } + }); + await this.auditLogger.log('UPDATE', 'USER', { + groupId: null, + metadata: { action: 'unarchive', targetUserId: id }, + userId: currentUser.id + }); + return user; + } + async updateById(id: string, { groupIds, password, ...data }: UpdateUserDto, currentUser: RequestUser) { const { ability } = currentUser; const isDemotion = data.basePermissionLevel !== undefined && data.basePermissionLevel !== 'ADMIN'; diff --git a/apps/api/test/suites/02-user-permissions.suite.ts b/apps/api/test/suites/02-user-permissions.suite.ts index 74be2f0b9..2f55d8a9a 100644 --- a/apps/api/test/suites/02-user-permissions.suite.ts +++ b/apps/api/test/suites/02-user-permissions.suite.ts @@ -139,7 +139,8 @@ export default defineSuite('user permissions', function () { { method: 'PATCH', payload: { basePermissionLevel: 'ADMIN' }, url: `/v1/users/${grantee.id}` }, { method: 'PATCH', payload: { groupIds: [group.id, otherGroup.id] }, url: `/v1/users/${grantee.id}` }, { method: 'PATCH', payload: { password: PASSWORD }, url: `/v1/users/${teammateAdmin.id}` }, - { method: 'DELETE', url: `/v1/users/${teammateAdmin.id}` }, + { method: 'PATCH', url: `/v1/users/${teammateAdmin.id}/archive` }, + { method: 'PATCH', url: `/v1/users/${teammateAdmin.id}/unarchive` }, { method: 'POST', payload: { diff --git a/apps/web/src/__tests__/admin-users-table.test.tsx b/apps/web/src/__tests__/admin-users-table.test.tsx new file mode 100644 index 000000000..02df6bcfe --- /dev/null +++ b/apps/web/src/__tests__/admin-users-table.test.tsx @@ -0,0 +1,57 @@ +import type { PropsWithChildren } from 'react'; + +import type { User } from '@opendatacapture/schemas/user'; +import { cleanup, render, screen, within } from '@testing-library/react'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +import { Route } from '@/routes/_app/admin/users/index'; + +import '@/services/i18n'; + +const mocks = vi.hoisted(() => ({ users: [] as User[] })); + +vi.mock('@tanstack/react-router', async (importOriginal) => ({ + ...(await importOriginal()), + Link: ({ children }: PropsWithChildren) => {children}, + useNavigate: () => vi.fn() +})); +vi.mock('@/hooks/useUsersQuery', () => ({ + usersQueryOptions: vi.fn(), + useUsersQuery: () => ({ data: mocks.users }) +})); +vi.mock('@/hooks/useArchiveUserMutation', () => ({ useArchiveUserMutation: () => ({ mutate: vi.fn() }) })); +vi.mock('@/hooks/useUnarchiveUserMutation', () => ({ useUnarchiveUserMutation: () => ({ mutate: vi.fn() }) })); +vi.mock('@/store', () => ({ + useAppStore: (selector: (store: { currentUser: null }) => unknown) => selector({ currentUser: null }) +})); + +afterEach(cleanup); + +describe('admin users table', () => { + it.each([false, true, null, undefined])('should display disabled=%s independently of archive status', (disabled) => { + mocks.users = [ + { + additionalPermissions: [], + archivedAt: new Date('2026-10-01'), + basePermissionLevel: 'STANDARD', + createdAt: new Date('2026-01-01'), + disabled, + firstName: 'Jane', + groupIds: [], + id: 'user-1', + lastName: 'Doe', + updatedAt: new Date('2026-10-01'), + username: 'jane' + } + ]; + const Component = Route.options.component!; + render(); + expect(screen.getByTestId('user-login-status').textContent).toBe(disabled ? 'Disabled' : 'Enabled'); + expect(screen.getByTestId('user-status-archived')).toBeTruthy(); + const headers = within(screen.getByTestId('data-table-head')) + .getAllByRole('button', { hidden: true }) + .map((header) => header.textContent) + .filter(Boolean); + expect(headers.indexOf('Enabled / Disabled')).toBe(headers.indexOf('Status') - 1); + }); +}); diff --git a/apps/web/src/routes/_app/admin/users/index.tsx b/apps/web/src/routes/_app/admin/users/index.tsx index 5d0e908a7..3749dfd74 100644 --- a/apps/web/src/routes/_app/admin/users/index.tsx +++ b/apps/web/src/routes/_app/admin/users/index.tsx @@ -2,17 +2,35 @@ import React, { useState } from 'react'; import { snakeToCamelCase, toBasicISOString } from '@douglasneuroinformatics/libjs'; import { Button, DataTable, Dialog, Heading } from '@douglasneuroinformatics/libui/components'; +import type { TanstackTable } from '@douglasneuroinformatics/libui/components'; import { useTranslation } from '@douglasneuroinformatics/libui/hooks'; +import { cn } from '@douglasneuroinformatics/libui/utils'; import type { User } from '@opendatacapture/schemas/user'; import { createFileRoute, Link, useNavigate } from '@tanstack/react-router'; +import { ChevronDownIcon, ChevronsUpDownIcon, ChevronUpIcon } from 'lucide-react'; import { PageHeader } from '@/components/PageHeader'; import { useArchiveUserMutation } from '@/hooks/useArchiveUserMutation'; import { useUnarchiveUserMutation } from '@/hooks/useUnarchiveUserMutation'; -import { useAppStore } from '@/store'; import { usersQueryOptions, useUsersQuery } from '@/hooks/useUsersQuery'; +import { useAppStore } from '@/store'; + +type ArchiveAction = { kind: 'archive'; user: User }; -type ArchiveAction = { kind: 'archive'; user: User } | { kind: 'unarchive'; user: User }; +const SortableHeader = ({ column, label }: { column: TanstackTable.Column; label: string }) => { + const sorted = column.getIsSorted(); + const Icon = sorted === 'asc' ? ChevronUpIcon : sorted === 'desc' ? ChevronDownIcon : ChevronsUpDownIcon; + return ( + + ); +}; const RouteComponent = () => { const { t } = useTranslation(); @@ -52,7 +70,7 @@ const RouteComponent = () => { ); }, - header: t('common.username') + header: ({ column }) => }, { accessorKey: 'basePermissionLevel', @@ -66,19 +84,31 @@ const RouteComponent = () => { } return t(`common.${snakeToCamelCase(basePermissionLevel)}`); }, - header: t('common.basePermissionLevel') + header: ({ column }) => + }, + { + accessorFn: (user) => Boolean(user.disabled), + cell: ({ row }) => ( + + {row.original.disabled ? t({ en: 'Disabled', fr: 'Désactivé' }) : t({ en: 'Enabled', fr: 'Activé' })} + + ), + header: ({ column }) => ( + + ), + id: 'disabled' }, { - accessorKey: 'disabled', + accessorKey: 'archivedAt', cell: (ctx) => { const user = ctx.row.original; - if (user.disabled) { + if (user.archivedAt) { return ( {t({ - en: `Archived on ${toBasicISOString(user.updatedAt)}`, - es: `Archivado el ${toBasicISOString(user.updatedAt)}`, - fr: `Archivé le ${toBasicISOString(user.updatedAt)}` + en: `Archived on ${toBasicISOString(new Date(user.archivedAt))}`, + es: `Archivado el ${toBasicISOString(new Date(user.archivedAt))}`, + fr: `Archivé le ${toBasicISOString(new Date(user.archivedAt))}` })} ); @@ -89,7 +119,9 @@ const RouteComponent = () => { ); }, - header: t({ en: 'Status', es: 'Estado', fr: 'Statut' }), + header: ({ column }) => ( + + ), id: 'status' } ]} @@ -101,14 +133,14 @@ const RouteComponent = () => { onSelect: openUser }, { - disabled: (user) => user.username === currentUser?.username || Boolean(user.disabled), + disabled: (user) => user.username === currentUser?.username || Boolean(user.archivedAt), label: t({ en: 'Archive', es: 'Archivar', fr: 'Archiver' }), onSelect: (user) => setPendingAction({ kind: 'archive', user }) }, { - disabled: (user) => !user.disabled, + disabled: (user) => !user.archivedAt, label: t({ en: 'Unarchive', es: 'Desarchivar', fr: 'Désarchiver' }), - onSelect: (user) => setPendingAction({ kind: 'unarchive', user }) + onSelect: (user) => unarchiveUserMutation.mutate({ id: user.id }) } ]} togglesComponent={() => ( @@ -140,29 +172,22 @@ const RouteComponent = () => { })} - {pendingAction?.kind === 'archive' - ? t({ - en: 'This will archive the account and prevent the user from signing in.', - es: 'Esto archivará la cuenta e impedirá que el usuario inicie sesión.', - fr: "Cela archivera le compte et empêchera l'utilisateur de se connecter." - }) - : t({ - en: 'This will restore the account and allow the user to sign in again.', - es: 'Esto restaurará la cuenta y permitirá que el usuario inicie sesión de nuevo.', - fr: "Cela restaurera le compte et permettra à l'utilisateur de se reconnecter." - })} + {t({ + en: 'This will archive the account and prevent the user from signing in.', + es: 'Esto archivará la cuenta e impedirá que el usuario inicie sesión.', + fr: "Cela archivera le compte et empêchera l'utilisateur de se connecter." + })} @@ -227,58 +208,113 @@ export const UserPermissionsEditor = ({ groups, user }: UserPermissionsEditorPro ))} - {/* The add controls are the table's last row, under the headers that name them. */} - - - setAction($AppAction.parse(value))} - /> - - - setSubject($AppSubjectName.parse(value))} - /> - - - {isScopable && ( - - )} - - - - - + + [option, actionLabels[option]]) + )} + placeholder={placeholder} + value={action} + onValueChange={(value) => updateDraft(index, { action: $AppAction.parse(value) })} + /> + + + [option, subjectLabels[option]]) + )} + placeholder={placeholder} + value={subject} + onValueChange={(value) => updateDraft(index, { subject: $AppSubjectName.parse(value) })} + /> + + + {subject !== undefined && isGroupScopableSubject(subject) && ( + updateDraft(index, { scope })} + /> + )} + + + + + + + ); + })} + {drafts.length === 0 && ( + + + + + + )} + {highlightIncomplete && drafts.some(isIncompleteDraft) && ( +

+ {t({ + en: 'Complete or remove the highlighted permission rows before saving.', + fr: "Complétez ou retirez les lignes d'autorisation en surbrillance avant d'enregistrer." + })} +

+ )} {isManageAll && (
({ addNotification: vi.fn(), patch: vi.fn(), put: vi.fn() })); +vi.mock('axios', () => ({ default: { patch: mocks.patch, put: mocks.put } })); +vi.mock('@douglasneuroinformatics/libui/hooks', () => ({ + useNotificationsStore: (selector: (store: { addNotification: typeof mocks.addNotification }) => unknown) => + selector(mocks) +})); + +const permissions: Permissions = [{ action: 'read', groupId: 'group-1', subject: 'Subject' }]; + +function renderUpdateMutation() { + const queryClient = new QueryClient({ defaultOptions: { mutations: { retry: false } } }); + const wrapper = ({ children }: PropsWithChildren) => + createElement(QueryClientProvider, { children, client: queryClient }); + return { ...renderHook(() => useUpdateUserMutation(), { wrapper }), queryClient }; +} + +afterEach(cleanup); +beforeEach(() => { + vi.resetAllMocks(); + mocks.patch.mockResolvedValue({}); + mocks.put.mockResolvedValue({}); +}); + +describe('useUpdateUserMutation', () => { + it('should show one confirmation after both account and permissions save', async () => { + const { queryClient, result } = renderUpdateMutation(); + const invalidate = vi.spyOn(queryClient, 'invalidateQueries'); + await result.current.mutateAsync({ data: { email: 'jane@example.org' }, id: 'user-1', permissions }); + expect(mocks.patch).toHaveBeenCalledWith('/v1/users/user-1', { email: 'jane@example.org' }); + expect(mocks.put).toHaveBeenCalledWith('/v1/users/user-1/permissions', { permissions }); + expect(mocks.addNotification).toHaveBeenCalledExactlyOnceWith({ type: 'success' }); + expect(invalidate).toHaveBeenCalledWith({ queryKey: ['users'] }); + }); + + it('should save group membership before permissions that depend on it', async () => { + const accountSave = Promise.withResolvers(); + mocks.patch.mockReturnValueOnce(accountSave.promise); + const { result } = renderUpdateMutation(); + const save = result.current.mutateAsync({ data: { groupIds: ['group-1'] }, id: 'user-1', permissions }); + await vi.waitFor(() => expect(mocks.patch).toHaveBeenCalled()); + expect(mocks.put).not.toHaveBeenCalled(); + expect(mocks.addNotification).not.toHaveBeenCalled(); + accountSave.resolve({}); + await save; + expect(mocks.put).toHaveBeenCalledOnce(); + }); + + it('should save an account without replacing permissions when none were supplied', async () => { + const { result } = renderUpdateMutation(); + await result.current.mutateAsync({ data: { disabled: false }, id: 'user-1' }); + expect(mocks.put).not.toHaveBeenCalled(); + expect(mocks.addNotification).toHaveBeenCalledOnce(); + }); + + it('should clear permissions when the last row was removed', async () => { + const { result } = renderUpdateMutation(); + await result.current.mutateAsync({ data: {}, id: 'user-1', permissions: [] }); + expect(mocks.put).toHaveBeenCalledWith('/v1/users/user-1/permissions', { permissions: [] }); + }); + + it('should show no success confirmation when permissions fail to save', async () => { + mocks.put.mockRejectedValueOnce(new Error('permission save failed')); + const { result } = renderUpdateMutation(); + await expect(result.current.mutateAsync({ data: {}, id: 'user-1', permissions })).rejects.toThrow( + 'permission save failed' + ); + expect(mocks.addNotification).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/web/src/hooks/__tests__/useUpdateUserPermissionsMutation.test.ts b/apps/web/src/hooks/__tests__/useUpdateUserPermissionsMutation.test.ts deleted file mode 100644 index baae52b0e..000000000 --- a/apps/web/src/hooks/__tests__/useUpdateUserPermissionsMutation.test.ts +++ /dev/null @@ -1,60 +0,0 @@ -import type { PropsWithChildren } from 'react'; -import { createElement } from 'react'; - -import type { Permissions } from '@opendatacapture/schemas/core'; -import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; -import { renderHook, waitFor } from '@testing-library/react'; -import { beforeEach, describe, expect, it, vi } from 'vitest'; - -import { useUpdateUserPermissionsMutation } from '../useUpdateUserPermissionsMutation'; - -const mockAxios = vi.hoisted(() => ({ isAxiosError: vi.fn(() => false), put: vi.fn() })); - -vi.mock('axios', () => ({ default: mockAxios })); - -const permissions: Permissions = [{ action: 'read', groupId: 'group-1', subject: 'Subject' }]; - -const updatedUser = { - additionalPermissions: permissions, - basePermissionLevel: 'STANDARD', - createdAt: '2026-01-01T00:00:00.000Z', - firstName: 'Jane', - groupIds: ['group-1'], - id: 'user-1', - lastName: 'Doe', - updatedAt: '2026-01-01T00:00:00.000Z', - username: 'jdoe' -}; - -function renderUpdatePermissionsMutation() { - const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }); - const wrapper = ({ children }: PropsWithChildren) => - createElement(QueryClientProvider, { children, client: queryClient }); - return { ...renderHook(() => useUpdateUserPermissionsMutation(), { wrapper }), queryClient }; -} - -describe('useUpdateUserPermissionsMutation', () => { - beforeEach(() => { - vi.clearAllMocks(); - mockAxios.put.mockResolvedValue({ data: updatedUser }); - }); - - it('should put the complete set of permissions, since the route replaces what is stored', async () => { - const { result } = renderUpdatePermissionsMutation(); - await result.current.mutateAsync({ id: 'user-1', permissions }); - expect(mockAxios.put).toHaveBeenCalledWith('/v1/users/user-1/permissions', { permissions }); - }); - - it('should parse the updated user', async () => { - const { result } = renderUpdatePermissionsMutation(); - const response = await result.current.mutateAsync({ id: 'user-1', permissions }); - expect(response.additionalPermissions).toEqual(permissions); - }); - - it('should invalidate the users queries, so the page shows the new grant without a reload', async () => { - const { queryClient, result } = renderUpdatePermissionsMutation(); - const invalidate = vi.spyOn(queryClient, 'invalidateQueries'); - await result.current.mutateAsync({ id: 'user-1', permissions }); - await waitFor(() => expect(invalidate).toHaveBeenCalledWith({ queryKey: ['users'] })); - }); -}); diff --git a/apps/web/src/hooks/useUpdateUserMutation.ts b/apps/web/src/hooks/useUpdateUserMutation.ts index 7864d4ebd..12282b817 100644 --- a/apps/web/src/hooks/useUpdateUserMutation.ts +++ b/apps/web/src/hooks/useUpdateUserMutation.ts @@ -1,4 +1,5 @@ import { useNotificationsStore } from '@douglasneuroinformatics/libui/hooks'; +import type { Permissions } from '@opendatacapture/schemas/core'; import type { UpdateUserData } from '@opendatacapture/schemas/user'; import { useMutation, useQueryClient } from '@tanstack/react-query'; import axios from 'axios'; @@ -9,8 +10,11 @@ export function useUpdateUserMutation() { const queryClient = useQueryClient(); const addNotification = useNotificationsStore((store) => store.addNotification); return useMutation({ - mutationFn: async ({ data, id }: { data: UpdateUserData; id: string }) => { + mutationFn: async ({ data, id, permissions }: { data: UpdateUserData; id: string; permissions?: Permissions }) => { await axios.patch(`/v1/users/${id}`, data); + if (permissions !== undefined) { + await axios.put(`/v1/users/${id}/permissions`, { permissions }); + } }, onSuccess() { addNotification({ type: 'success' }); diff --git a/apps/web/src/hooks/useUpdateUserPermissionsMutation.ts b/apps/web/src/hooks/useUpdateUserPermissionsMutation.ts deleted file mode 100644 index e74eb7cc2..000000000 --- a/apps/web/src/hooks/useUpdateUserPermissionsMutation.ts +++ /dev/null @@ -1,24 +0,0 @@ -import { useNotificationsStore } from '@douglasneuroinformatics/libui/hooks'; -import type { Permissions } from '@opendatacapture/schemas/core'; -import { $User } from '@opendatacapture/schemas/user'; -import type { UpdateUserPermissionsData } from '@opendatacapture/schemas/user'; -import { useMutation, useQueryClient } from '@tanstack/react-query'; -import axios from 'axios'; - -import { USERS_QUERY_KEY } from './useUsersQuery'; - -export function useUpdateUserPermissionsMutation() { - const queryClient = useQueryClient(); - const addNotification = useNotificationsStore((store) => store.addNotification); - return useMutation({ - mutationFn: async ({ id, permissions }: { id: string; permissions: Permissions }) => { - const data: UpdateUserPermissionsData = { permissions }; - const response = await axios.put(`/v1/users/${id}/permissions`, data); - return $User.parse(response.data); - }, - onSuccess() { - addNotification({ type: 'success' }); - void queryClient.invalidateQueries({ queryKey: [USERS_QUERY_KEY] }); - } - }); -} diff --git a/apps/web/src/routes/_app/admin/users/$userId.tsx b/apps/web/src/routes/_app/admin/users/$userId.tsx index f5ef9b93a..0dae44feb 100644 --- a/apps/web/src/routes/_app/admin/users/$userId.tsx +++ b/apps/web/src/routes/_app/admin/users/$userId.tsx @@ -3,8 +3,10 @@ import { useMemo, useState } from 'react'; import { snakeToCamelCase } from '@douglasneuroinformatics/libjs'; import { Button, Card, Dialog, Heading } from '@douglasneuroinformatics/libui/components'; import { useTranslation } from '@douglasneuroinformatics/libui/hooks'; -import { ChevronLeftIcon } from '@heroicons/react/24/solid'; -import { createFileRoute, Link, useNavigate } from '@tanstack/react-router'; +import { ArchiveBoxArrowDownIcon, ArchiveBoxXMarkIcon, ChevronLeftIcon } from '@heroicons/react/24/solid'; +import { isGrantablePermission } from '@opendatacapture/schemas/core'; +import type { Permissions } from '@opendatacapture/schemas/core'; +import { createFileRoute, Link } from '@tanstack/react-router'; import { Chip } from '@/components/Chip'; import { PageHeader } from '@/components/PageHeader'; @@ -14,17 +16,17 @@ import { UserIcon } from '@/components/UserIcon'; import { UserPermissionsEditor } from '@/components/UserPermissionsEditor'; import { useArchiveUserMutation } from '@/hooks/useArchiveUserMutation'; import { useFindUserQuery, useFindUserQueryOptions } from '@/hooks/useFindUserQuery'; -import { useUnarchiveUserMutation } from '@/hooks/useUnarchiveUserMutation'; import { groupsQueryOptions, useGroupsQuery } from '@/hooks/useGroupsQuery'; +import { useUnarchiveUserMutation } from '@/hooks/useUnarchiveUserMutation'; import { useUpdateUserMutation } from '@/hooks/useUpdateUserMutation'; import { useAppStore } from '@/store'; +import { isIncompleteDraft, withPermissionDrafts } from '@/utils/permissions'; +import type { PermissionDraft } from '@/utils/permissions'; import { clearedIfBlank, omittedIfUnchanged, validationSummary } from '@/utils/validation'; -const RouteComponent = () => { - const { userId } = Route.useParams(); +const UserEditor = ({ userId }: { userId: string }) => { const currentUser = useAppStore((store) => store.currentUser); const { t } = useTranslation(); - const navigate = useNavigate(); const groupsQuery = useGroupsQuery(); const userQuery = useFindUserQuery(userId); const archiveUserMutation = useArchiveUserMutation(); @@ -32,14 +34,19 @@ const RouteComponent = () => { const updateUserMutation = useUpdateUserMutation(); const [submitErrorMessage, setSubmitErrorMessage] = useState(null); const [isConfirmOpen, setIsConfirmOpen] = useState(false); - // libui's `Form` clears its values after a successful submit, so the profile form is remounted - // from the saved user once a save lands. Keyed on this rather than on the query's refetch time so - // that saving a permission below does not discard edits typed here but not yet saved. + const [isSaving, setIsSaving] = useState(false); + const [highlightIncompleteDrafts, setHighlightIncompleteDrafts] = useState(false); const [savedProfileCount, setSavedProfileCount] = useState(0); const groups = groupsQuery.data; const user = userQuery.data; + const [localPermissions, setLocalPermissions] = useState(user.additionalPermissions); + + const [permissionDrafts, setPermissionDrafts] = useState([ + { scope: user.groupIds.length === 1 ? user.groupIds[0] : undefined } + ]); + const isCurrentUser = user.username === currentUser?.username; const userGroups = groups.filter((group) => user.groupIds.includes(group.id)); const roleLabel = user.basePermissionLevel @@ -69,36 +76,97 @@ const RouteComponent = () => {
- - - - - {t({ en: 'Return', fr: 'Retour' })} - -
- -
- - {user.username} - -

- {identityLine} -

- {userGroups.length > 0 && ( -
- {userGroups.map((group) => ( - {group.name} - ))} -
+
+ + + {t({ en: 'Return', fr: 'Retour' })} + + + +
+ +
+ + {user.username} + +

+ {identityLine} +

+ {userGroups.length > 0 && ( +
+ {userGroups.map((group) => ( + {group.name} + ))} +
+ )} +
+ {user.archivedAt ? ( + + ) : ( + + + + + + + + {t({ + en: 'Are you absolutely sure?', + es: '¿Está absolutamente seguro?', + fr: 'Êtes-vous absolument sûr ?' + })} + + + {t({ + en: 'This will archive the account and prevent the user from signing in.', + es: 'Esto archivará la cuenta e impedirá que el usuario inicie sesión.', + fr: "Cela archivera le compte et empêchera l'utilisateur de se connecter." + })} + + + + + + + + )}
-
- - + + +
{t({ en: 'Account', fr: 'Compte' })} @@ -110,8 +178,6 @@ const RouteComponent = () => { - {/* Above the form rather than beside the field: a rejected field can be several sections - away from the save button, and is then off-screen at the moment of the failure. */} {submitErrorMessage && (
{

{submitErrorMessage}

)} + setSubmitErrorMessage(validationSummary(error))} - onSubmit={({ confirmPassword: _, email, groupIds, phoneNumber, ...data }) => { + onSubmit={async ({ confirmPassword: _, email, groupIds, phoneNumber, ...data }) => { + if (isSaving) return undefined; + if (permissionDrafts.some(isIncompleteDraft)) { + setHighlightIncompleteDrafts(true); + const errorMessage = t({ + en: 'A permission row is incomplete. Choose its action, resource and scope, or remove it.', + fr: "Une ligne d'autorisation est incomplète. Choisissez son action, sa ressource et sa portée, ou retirez-la." + }); + setSubmitErrorMessage(errorMessage); + return { errorMessage, success: false }; + } + setHighlightIncompleteDrafts(false); + setIsSaving(true); setSubmitErrorMessage(null); - updateUserMutation.mutate( - { - data: { - ...data, - email: clearedIfBlank(email), - groupIds: Array.from(groupIds), - phoneNumber: omittedIfUnchanged(phoneNumber, user.phoneNumber) - }, - id: user.id - }, - { onSuccess: () => setSavedProfileCount((count) => count + 1) } - ); + const accountData = { + ...data, + email: clearedIfBlank(email), + groupIds: Array.from(groupIds), + phoneNumber: omittedIfUnchanged(phoneNumber, user.phoneNumber) + }; + try { + const permissions = withPermissionDrafts(localPermissions, permissionDrafts) + .filter(isGrantablePermission) + .filter((permission) => permission.groupId === null || groupIds.has(permission.groupId)); + await updateUserMutation.mutateAsync({ + data: accountData, + id: user.id, + permissions: user.basePermissionLevel === 'ADMIN' ? undefined : permissions + }); + await userQuery.refetch(); + setLocalPermissions(permissions); + setPermissionDrafts([{ scope: groupIds.size === 1 ? Array.from(groupIds)[0] : undefined }]); + setSavedProfileCount((count) => count + 1); + return undefined; + } catch { + const errorMessage = t({ + en: 'Could not save all changes. Reload the page to check the saved account and permissions.', + fr: 'Impossible d’enregistrer toutes les modifications. Rechargez la page pour vérifier le compte et les autorisations enregistrés.' + }); + setSubmitErrorMessage(errorMessage); + return { errorMessage, success: false }; + } finally { + setIsSaving(false); + } }} /> - - - - - {user.disabled - ? t({ en: 'Unarchive User', es: 'Desarchivar usuario', fr: "Désarchiver l'utilisateur" }) - : t({ en: 'Archive User', es: 'Archivar usuario', fr: "Archiver l'utilisateur" })} - - - {isCurrentUser - ? t({ - en: 'You cannot archive the account you are signed in with.', - es: 'No puede archivar la cuenta con la que ha iniciado sesión.', - fr: 'Vous ne pouvez pas archiver le compte avec lequel vous êtes connecté.' - }) - : user.disabled - ? t({ - en: 'Restores this account. The user will be able to sign in again.', - es: 'Restaura esta cuenta. El usuario podrá iniciar sesión de nuevo.', - fr: "Restaure ce compte. L'utilisateur pourra se reconnecter." - }) - : t({ - en: 'Disables this account. The user will no longer be able to sign in.', - es: 'Desactiva esta cuenta. El usuario ya no podrá iniciar sesión.', - fr: "Désactive ce compte. L'utilisateur ne pourra plus se connecter." - })} - - - - - - - - - - - {t({ - en: 'Are you absolutely sure?', - es: '¿Está absolutamente seguro?', - fr: 'Êtes-vous absolument sûr ?' - })} - - - {user.disabled - ? t({ - en: 'This will restore the account and allow the user to sign in again.', - es: 'Esto restaurará la cuenta y permitirá que el usuario inicie sesión de nuevo.', - fr: "Cela restaurera le compte et permettra à l'utilisateur de se reconnecter." - }) - : t({ - en: 'This will archive the account and prevent the user from signing in.', - es: 'Esto archivará la cuenta e impedirá que el usuario inicie sesión.', - fr: "Cela archivera le compte et empêchera l'utilisateur de se connecter." - })} - - - - - - - - - - + +
); }; +const RouteComponent = () => { + const { userId } = Route.useParams(); + return ; +}; + export const Route = createFileRoute('/_app/admin/users/$userId')({ component: RouteComponent, loader: async ({ context, params }) => { diff --git a/apps/web/src/utils/__tests__/permissions.test.ts b/apps/web/src/utils/__tests__/permissions.test.ts index f56322253..6dd734215 100644 --- a/apps/web/src/utils/__tests__/permissions.test.ts +++ b/apps/web/src/utils/__tests__/permissions.test.ts @@ -6,9 +6,11 @@ import { ALL_GROUPS, grantableActions, grantableSubjects, + isIncompleteDraft, toUserPermission, withoutPermission, - withPermission + withPermission, + withPermissionDrafts } from '../permissions'; describe('$AddPermissionFormData', () => { @@ -98,3 +100,51 @@ describe('withoutPermission', () => { expect(withoutPermission(permissions, 0)).toEqual([permissions[1]]); }); }); + +describe('isIncompleteDraft', () => { + it('should not flag a blank row, even with its scope preselected, so an untouched add row never blocks a save', () => { + expect(isIncompleteDraft({})).toBe(false); + expect(isIncompleteDraft({ scope: 'group-1' })).toBe(false); + }); + + it('should flag a started row missing its resource', () => { + expect(isIncompleteDraft({ action: 'read' })).toBe(true); + }); + + it('should flag a scopable grant with no scope chosen, so it is not silently dropped', () => { + expect(isIncompleteDraft({ action: 'read', subject: 'Subject' })).toBe(true); + }); + + it('should not flag a complete row', () => { + expect(isIncompleteDraft({ action: 'create', subject: 'Instrument' })).toBe(false); + expect(isIncompleteDraft({ action: 'read', scope: 'group-1', subject: 'Subject' })).toBe(false); + }); +}); + +describe('withPermissionDrafts', () => { + it('should include every complete row without requiring plus', () => { + expect( + withPermissionDrafts( + [], + [ + { action: 'read', scope: 'group-1', subject: 'User' }, + { action: 'create', subject: 'Instrument' } + ] + ) + ).toEqual([ + { action: 'read', groupId: 'group-1', subject: 'User' }, + { action: 'create', groupId: null, subject: 'Instrument' } + ]); + }); + + it('should ignore unfinished rows and avoid duplicate grants', () => { + const permissions: Permissions = [{ action: 'read', groupId: 'group-1', subject: 'User' }]; + expect( + withPermissionDrafts(permissions, [ + {}, + { action: 'read', subject: 'Subject' }, + { action: 'read', scope: 'group-1', subject: 'User' } + ]) + ).toEqual(permissions); + }); +}); diff --git a/apps/web/src/utils/permissions.ts b/apps/web/src/utils/permissions.ts index e76853c3b..47ec14fde 100644 --- a/apps/web/src/utils/permissions.ts +++ b/apps/web/src/utils/permissions.ts @@ -10,6 +10,8 @@ import { z } from 'zod/v4'; /** The scope option standing for every group. A group id is an ObjectId, so the two cannot collide. */ const ALL_GROUPS = '__all__'; +type PermissionDraft = Partial; + type AddPermissionFormData = z.infer; const $AddPermissionFormData = z .object({ @@ -58,6 +60,16 @@ const isSamePermission = (a: UserPermission, b: UserPermission): boolean => const withPermission = (permissions: Permissions, permission: UserPermission): Permissions => permissions.some((existing) => isSamePermission(existing, permission)) ? permissions : [...permissions, permission]; +/** A row with an action or resource chosen that is not yet a valid grant. The scope alone is preselected, so it does not count as started. */ +const isIncompleteDraft = (draft: PermissionDraft): boolean => + (draft.action !== undefined || draft.subject !== undefined) && !$AddPermissionFormData.safeParse(draft).success; + +const withPermissionDrafts = (permissions: Permissions, drafts: PermissionDraft[]): Permissions => + drafts.reduce((result, draft) => { + const parsed = $AddPermissionFormData.safeParse(draft); + return parsed.success ? withPermission(result, toUserPermission(parsed.data)) : result; + }, permissions); + const withoutPermission = (permissions: Permissions, index: number): Permissions => permissions.filter((_, i) => i !== index); @@ -66,8 +78,10 @@ export { ALL_GROUPS, grantableActions, grantableSubjects, + isIncompleteDraft, toUserPermission, withoutPermission, - withPermission + withPermission, + withPermissionDrafts }; -export type { AddPermissionFormData }; +export type { AddPermissionFormData, PermissionDraft }; diff --git a/testing/src/pages/_app/admin/users/$userId.page.ts b/testing/src/pages/_app/admin/users/$userId.page.ts index b32c29845..fcebdbcb9 100644 --- a/testing/src/pages/_app/admin/users/$userId.page.ts +++ b/testing/src/pages/_app/admin/users/$userId.page.ts @@ -1,4 +1,5 @@ import type { AppAction, AppSubjectName } from '@opendatacapture/schemas/core'; +import { expect } from '@playwright/test'; import type { Locator, Page } from '@playwright/test'; import { AppPage } from '../../route.page'; @@ -22,7 +23,7 @@ export class AdminUserPage extends AppPage { this.submitError = page.getByTestId('admin-user-edit-error'); this.permissionsTable = page.getByTestId('user-permissions-table'); this.permissionRows = page.getByTestId('user-permission-row'); - this.addPermissionRow = page.getByTestId('add-permission-row'); + this.addPermissionRow = page.getByTestId('add-permission-row').last(); this.adminNotice = page.getByTestId('user-permissions-admin-notice'); this.manageAllWarning = page.getByTestId('manage-all-warning'); } @@ -37,16 +38,14 @@ export class AdminUserPage extends AppPage { if (scope !== undefined) { await this.selectOption('scope', scope); } - await this.submitPermission(); } - async archiveUser() { - await this.$ref.getByRole('button', { name: 'Archive User' }).click(); - await this.$ref.getByRole('button', { name: 'Yes' }).click(); + async addPermissionRowAfterCurrent() { + await this.addPermissionRow.getByRole('button', { name: 'Add Permission' }).click(); } - async unarchiveUser() { - await this.$ref.getByRole('button', { name: 'Unarchive User' }).click(); + async archiveUser() { + await this.$ref.getByRole('button', { name: 'Archive' }).click(); await this.$ref.getByRole('button', { name: 'Yes' }).click(); } @@ -55,9 +54,8 @@ export class AdminUserPage extends AppPage { } async saveProfile() { - // The shared `Form` component's own submit button always has `aria-label="Submit"`, even though - // this form's visible label is "Save" -- see DouglasNeuroInformatics/libui#108. - await this.profileForm.getByRole('button', { name: 'Submit' }).click(); + await this.$ref.getByTestId('save-user-changes').click(); + await expect(this.$ref.getByTestId('save-user-changes')).toBeEnabled(); } /** Opens one of the add-permission selects; the items render in a portal outside the form. */ @@ -67,6 +65,10 @@ export class AdminUserPage extends AppPage { } async submitPermission() { - await this.addPermissionRow.getByRole('button', { name: 'Add Permission' }).click(); + await this.saveProfile(); + } + + async unarchiveUser() { + await this.$ref.getByRole('button', { name: 'Unarchive' }).click(); } } diff --git a/testing/src/specs/admin-management.spec.ts b/testing/src/specs/admin-management.spec.ts index 424849d6b..483d60be4 100644 --- a/testing/src/specs/admin-management.spec.ts +++ b/testing/src/specs/admin-management.spec.ts @@ -30,6 +30,28 @@ test.describe('admin management', () => { await expect(page.getByTestId('data-table-body')).toContainText('admin'); }); + test('should show enabled and disabled accounts immediately left of archive status', async ({ + api, + authenticateAs, + page + }) => { + const group = await api.createGroup(); + const { user: enabled } = await api.createUser({ groupIds: [group.id] }); + const { user: disabled } = await api.createUser({ disabled: true, groupIds: [group.id] }); + await authenticateAs('ADMIN'); + await page.goto('/admin/users'); + await expect(page.getByTestId('data-table-head').getByRole('button', { name: 'Status' })).toBeVisible(); + const headers = await page.getByTestId('data-table-head').getByRole('button').allTextContents(); + expect(headers.filter(Boolean).indexOf('Enabled / Disabled')).toBe(headers.filter(Boolean).indexOf('Status') - 1); + const search = page.getByTestId('data-table-search-bar').getByRole('searchbox'); + await search.fill(enabled.username); + await expect(page.getByTestId('user-login-status')).toHaveText('Enabled'); + await expect(page.getByTestId('user-status-active')).toBeVisible(); + await search.fill(disabled.username); + await expect(page.getByTestId('user-login-status')).toHaveText('Disabled'); + await expect(page.getByTestId('user-status-active')).toBeVisible(); + }); + test('should show a validation error when the group name is missing', async ({ authenticateAs, page }) => { await authenticateAs('ADMIN'); await page.goto('/admin/groups/create'); @@ -218,17 +240,16 @@ test.describe('admin management', () => { const profileForm = page.getByTestId('update-user-form'); await profileForm.getByLabel('Email').fill(`${user.username}@example.com`); - // The shared `Form` component's own submit button always has `aria-label="Submit"`, even though - // this form's visible label is "Save" -- see DouglasNeuroInformatics/libui#108. - await profileForm.getByRole('button', { name: 'Submit' }).click(); + await page.getByTestId('save-user-changes').click(); // The edit and archive toasts below can stack within the notification hub's shared 5s lifetime, // so `.last()` targets the most recently raised one rather than an ambiguous match on both. await expect(page.getByRole('heading', { name: 'Success' }).last()).toBeVisible(); - await page.getByRole('button', { name: 'Archive User' }).click(); + await page.getByRole('button', { exact: true, name: 'Archive' }).click(); await page.getByRole('button', { name: 'Yes' }).click(); - await expect(page).toHaveURL('/admin/users'); + await expect(page.getByRole('button', { exact: true, name: 'Unarchive' })).toBeVisible(); + expect((await api.findUserById(user.id)).archivedAt).toBeTruthy(); }); test('should say why a save failed when the rejected field is scrolled out of view', async ({ @@ -343,6 +364,116 @@ test.describe('admin management', () => { expect((await api.findUserById(user.id)).email).toBeNull(); }); + test('should save account and completed permission rows together without using plus to confirm them', async ({ + api, + getPageModel, + page, + uniqueId + }) => { + const group = await api.createGroup(); + const { user } = await api.createUser({ groupIds: [group.id] }); + const userPage = await getPageModel('/admin/users/$userId', { userId: user.id }); + const email = `shared-save-${uniqueId}@example.org`; + + await expect(userPage.profileForm.getByRole('button', { name: 'Submit' })).toBeHidden(); + const save = page.getByTestId('save-user-changes'); + const permissionsCard = page.getByTestId('user-permissions-card'); + expect((await save.boundingBox())!.y).toBeGreaterThan((await permissionsCard.boundingBox())!.y); + + await userPage.profileForm.getByLabel('Email').fill(email); + await userPage.addPermission({ action: 'read', subject: 'User' }); + await userPage.addPermissionRowAfterCurrent(); + await userPage.addPermission({ action: 'create', subject: 'Instrument' }); + const beforeSave = await api.findUserById(user.id); + expect(beforeSave.email).not.toBe(email); + expect(beforeSave.additionalPermissions).toEqual([]); + + await userPage.saveProfile(); + await expect(userPage.permissionRows).toHaveCount(2); + await expect(page.getByRole('heading', { exact: true, name: 'Success' })).toHaveCount(1); + await page.reload(); + await expect(userPage.profileForm.getByLabel('Email')).toHaveValue(email); + expect((await api.findUserById(user.id)).additionalPermissions).toEqual([ + { action: 'read', groupId: group.id, subject: 'User' }, + { action: 'create', groupId: null, subject: 'Instrument' } + ]); + }); + + test('should save no permissions when account validation fails', async ({ api, getPageModel }) => { + const group = await api.createGroup(); + const { user } = await api.createUser({ groupIds: [group.id] }); + const userPage = await getPageModel('/admin/users/$userId', { userId: user.id }); + await userPage.addPermission({ action: 'read', subject: 'User' }); + await userPage.profileForm.getByLabel('Email').fill('invalid-email'); + await userPage.saveProfile(); + await expect(userPage.submitError).toBeVisible(); + expect((await api.findUserById(user.id)).additionalPermissions).toEqual([]); + }); + + test('should offer Enabled then Disabled on one line under Status, with Enabled chosen for an enabled user', async ({ + api, + getPageModel + }) => { + const group = await api.createGroup(); + const { user } = await api.createUser({ groupIds: [group.id] }); + const userPage = await getPageModel('/admin/users/$userId', { userId: user.id }); + const enabled = userPage.profileForm.getByLabel('Enabled', { exact: true }); + const disabled = userPage.profileForm.getByLabel('Disabled', { exact: true }); + + await expect(enabled).toBeChecked(); + await expect(disabled).not.toBeChecked(); + const enabledBox = (await enabled.boundingBox())!; + const disabledBox = (await disabled.boundingBox())!; + expect(Math.abs(enabledBox.y - disabledBox.y)).toBeLessThan(2); + expect(disabledBox.x).toBeGreaterThan(enabledBox.x); + const statusLabel = userPage.profileForm.getByText('Status', { exact: true }); + expect(enabledBox.y).toBeGreaterThan((await statusLabel.boundingBox())!.y); + }); + + test('should refuse to save while a permission row is half filled, rather than silently dropping it', async ({ + api, + getPageModel, + page, + uniqueId + }) => { + const [group, otherGroup] = await Promise.all([api.createGroup(), api.createGroup()]); + const { user } = await api.createUser({ groupIds: [group.id, otherGroup.id] }); + const userPage = await getPageModel('/admin/users/$userId', { userId: user.id }); + await userPage.profileForm.getByLabel('Email').fill(`half-filled-${uniqueId}@example.org`); + // With two groups no scope is preselected, so this row cannot become a grant yet. + await userPage.addPermission({ action: 'read', subject: 'Subject' }); + + await page.getByTestId('save-user-changes').click(); + + await expect(page.getByTestId('permission-drafts-incomplete')).toBeVisible(); + await expect(userPage.addPermissionRow).toHaveAttribute('aria-invalid', 'true'); + await expect(page.getByRole('heading', { exact: true, name: 'Success' })).toHaveCount(0); + const stored = await api.findUserById(user.id); + expect(stored.email).not.toBe(`half-filled-${uniqueId}@example.org`); + expect(stored.additionalPermissions).toEqual([]); + + await userPage.selectOption('scope', otherGroup.id); + await userPage.saveProfile(); + await expect(page.getByTestId('permission-drafts-incomplete')).toBeHidden(); + await expect(userPage.permissionRows).toHaveCount(1); + expect((await api.findUserById(user.id)).additionalPermissions).toEqual([ + { action: 'read', groupId: otherGroup.id, subject: 'Subject' } + ]); + }); + + test('should discard unsaved permission additions and removals on reload', async ({ api, getPageModel, page }) => { + const group = await api.createGroup(); + const { user } = await api.createUser({ groupIds: [group.id] }); + await api.setUserPermissions(user.id, [{ action: 'create', groupId: null, subject: 'Instrument' }]); + const userPage = await getPageModel('/admin/users/$userId', { userId: user.id }); + await userPage.removePermission(0); + await userPage.addPermission({ action: 'read', subject: 'User' }); + await page.reload(); + await expect(userPage.permissionRows).toHaveCount(1); + await expect(userPage.permissionRows.first()).toContainText('Instrument'); + await expect(userPage.addPermissionRow.getByTestId('action-select-trigger')).toContainText('Choose'); + }); + test('should grant a permission confined to the one group a user belongs to from the user page @smoke', async ({ api, getPageModel, @@ -355,6 +486,8 @@ test.describe('admin management', () => { // The scope is left untouched: with a single group it is preselected, so the group is the // default rather than something the admin has to remember to choose. await userPage.addPermission({ action: 'read', subject: 'User' }); + expect((await api.findUserById(user.id)).additionalPermissions).toStrictEqual([]); + await userPage.saveProfile(); await expect(userPage.permissionRows).toHaveCount(1); await expect(userPage.permissionRows.first().getByTestId('user-permission-scope')).toContainText(group.name); @@ -414,6 +547,8 @@ test.describe('admin management', () => { const userPage = await getPageModel('/admin/users/$userId', { userId: user.id }); await expect(userPage.permissionRows).toHaveCount(2); await userPage.removePermission(0); + expect((await api.findUserById(user.id)).additionalPermissions).toHaveLength(2); + await userPage.saveProfile(); await expect(userPage.permissionRows).toHaveCount(1); expect((await api.findUserById(user.id)).additionalPermissions).toStrictEqual([