From 98ef1c6bae32fd4170b6432a448bac518fb94cb8 Mon Sep 17 00:00:00 2001 From: thomasbeaudry Date: Thu, 5 Nov 2026 09:48:41 -0500 Subject: [PATCH 1/3] feat(api): order audit and sync logs by any column Both endpoints sorted only by date, so a table could not offer any other column without sorting just the current page. Add a `sortBy` parameter, with a trailing id so tied rows cannot repeat or skip across pages. Co-Authored-By: Claude Opus 5.5 --- .../audit/__tests__/audit.controller.test.ts | 2 +- .../src/audit/__tests__/audit.service.test.ts | 28 +++++++++++++++++-- apps/api/src/audit/audit.service.ts | 22 +++++++++++++-- .../instrument-repos.controller.test.ts | 2 +- .../instrument-repos.service.test.ts | 27 ++++++++++++++++-- .../instrument-repos.service.ts | 24 ++++++++++++++-- .../schemas/src/audit/__tests__/audit.test.ts | 7 ++++- packages/schemas/src/audit/audit.ts | 4 +++ .../src/instrument-repo/instrument-repo.ts | 11 ++++++++ 9 files changed, 114 insertions(+), 13 deletions(-) diff --git a/apps/api/src/audit/__tests__/audit.controller.test.ts b/apps/api/src/audit/__tests__/audit.controller.test.ts index cc61634e8..57aadbe94 100644 --- a/apps/api/src/audit/__tests__/audit.controller.test.ts +++ b/apps/api/src/audit/__tests__/audit.controller.test.ts @@ -8,7 +8,7 @@ import { beforeEach, describe, expect, it } from 'vitest'; import { AuditController } from '../audit.controller'; import { AuditService } from '../audit.service'; -const query: $AuditLogsQueryParams = { limit: AUDIT_LOGS_PAGE_SIZE, page: 2, sortOrder: 'asc' }; +const query: $AuditLogsQueryParams = { limit: AUDIT_LOGS_PAGE_SIZE, page: 2, sortBy: 'timestamp', sortOrder: 'asc' }; describe('AuditController', () => { let auditController: AuditController; diff --git a/apps/api/src/audit/__tests__/audit.service.test.ts b/apps/api/src/audit/__tests__/audit.service.test.ts index 376fc731c..0187b8e17 100644 --- a/apps/api/src/audit/__tests__/audit.service.test.ts +++ b/apps/api/src/audit/__tests__/audit.service.test.ts @@ -9,7 +9,12 @@ import { beforeEach, describe, expect, it } from 'vitest'; import { AuditService } from '../audit.service'; -const defaults: $AuditLogsQueryParams = { limit: AUDIT_LOGS_PAGE_SIZE, page: 1, sortOrder: 'desc' }; +const defaults: $AuditLogsQueryParams = { + limit: AUDIT_LOGS_PAGE_SIZE, + page: 1, + sortBy: 'timestamp', + sortOrder: 'desc' +}; describe('AuditService', () => { let auditService: AuditService; @@ -41,11 +46,28 @@ describe('AuditService', () => { it('should sort in the database rather than leaving it to the client', async () => { await auditService.find(defaults); - expect(auditLogModel.findMany.mock.lastCall?.[0]).toMatchObject({ orderBy: { timestamp: 'desc' } }); + expect(auditLogModel.findMany.mock.lastCall?.[0]).toMatchObject({ + orderBy: [{ timestamp: 'desc' }, { id: 'desc' }] + }); await auditService.find({ ...defaults, sortOrder: 'asc' }); - expect(auditLogModel.findMany.mock.lastCall?.[0]).toMatchObject({ orderBy: { timestamp: 'asc' } }); + expect(auditLogModel.findMany.mock.lastCall?.[0]).toMatchObject({ + orderBy: [{ timestamp: 'asc' }, { id: 'desc' }] + }); }); + it.each([ + ['action', { action: 'asc' }], + ['entity', { entity: 'asc' }], + ['group', { group: { name: 'asc' } }], + ['user', { user: { username: 'asc' } }] + ] as const)( + 'should order by %s, breaking ties on the id so pages neither repeat nor skip', + async (sortBy, orderBy) => { + await auditService.find({ ...defaults, sortBy, sortOrder: 'asc' }); + expect(auditLogModel.findMany.mock.lastCall?.[0]).toMatchObject({ orderBy: [orderBy, { id: 'desc' }] }); + } + ); + it('should apply the filters to both the page and the count', async () => { await auditService.find({ ...defaults, action: 'LOGIN', groupId: 'group-1' }); const where = { action: 'LOGIN', entity: undefined, groupId: 'group-1', userId: undefined }; diff --git a/apps/api/src/audit/audit.service.ts b/apps/api/src/audit/audit.service.ts index 0160190d9..41cc0adf5 100644 --- a/apps/api/src/audit/audit.service.ts +++ b/apps/api/src/audit/audit.service.ts @@ -1,7 +1,24 @@ import { InjectModel } from '@douglasneuroinformatics/libnest'; import type { Model } from '@douglasneuroinformatics/libnest'; import { Injectable } from '@nestjs/common'; -import type { $AuditLogsPage, $AuditLogsQueryParams } from '@opendatacapture/schemas/audit'; +import type { $AuditLogsPage, $AuditLogsQueryParams, $AuditLogsSortBy } from '@opendatacapture/schemas/audit'; + +type SortOrder = $AuditLogsQueryParams['sortOrder']; + +/** + * The trailing id makes the order total, since Mongo may otherwise return tied rows in a different + * order per page, repeating some rows and skipping others. + */ +const getOrderBy = (sortBy: $AuditLogsSortBy, sortOrder: SortOrder) => { + const orderBy = { + action: { action: sortOrder }, + entity: { entity: sortOrder }, + group: { group: { name: sortOrder } }, + timestamp: { timestamp: sortOrder }, + user: { user: { username: sortOrder } } + }[sortBy]; + return [orderBy, { id: 'desc' as const }]; +}; @Injectable() export class AuditService { @@ -13,13 +30,14 @@ export class AuditService { groupId, limit, page, + sortBy, sortOrder, userId }: $AuditLogsQueryParams): Promise<$AuditLogsPage> { const where = { action, entity, groupId, userId }; const [data, total] = await Promise.all([ this.auditLogModel.findMany({ - orderBy: { timestamp: sortOrder }, + orderBy: getOrderBy(sortBy, sortOrder), select: { action: true, entity: true, diff --git a/apps/api/src/instrument-repos/__tests__/instrument-repos.controller.test.ts b/apps/api/src/instrument-repos/__tests__/instrument-repos.controller.test.ts index e133317ba..33ac7d889 100644 --- a/apps/api/src/instrument-repos/__tests__/instrument-repos.controller.test.ts +++ b/apps/api/src/instrument-repos/__tests__/instrument-repos.controller.test.ts @@ -58,7 +58,7 @@ describe('InstrumentReposController', () => { }); it("should scope the activity log to the current user's ability, and pass the page asked for", async () => { - const query = { limit: 10, page: 2, sortOrder: 'desc' } as const; + const query = { limit: 10, page: 2, sortBy: 'createdAt', sortOrder: 'desc' } as const; const result = { data: [], pageCount: 0, total: 0 }; service.findSyncLogs.mockResolvedValueOnce(result); await expect(controller.findSyncLogs(query, ability)).resolves.toBe(result); diff --git a/apps/api/src/instrument-repos/__tests__/instrument-repos.service.test.ts b/apps/api/src/instrument-repos/__tests__/instrument-repos.service.test.ts index 5339925bf..1d284beb6 100644 --- a/apps/api/src/instrument-repos/__tests__/instrument-repos.service.test.ts +++ b/apps/api/src/instrument-repos/__tests__/instrument-repos.service.test.ts @@ -398,7 +398,7 @@ describe('InstrumentReposService', () => { }); describe('findSyncLogs', () => { - const query = { limit: 10, page: 1, sortOrder: 'desc' } as const; + const query = { limit: 10, page: 1, sortBy: 'createdAt', sortOrder: 'desc' } as const; it('should return the requested page of the history, with how many pages there are', async () => { instrumentRepoSyncLogModel.findMany.mockResolvedValueOnce([{ id: 'log-1' }]); @@ -410,7 +410,7 @@ describe('InstrumentReposService', () => { total: 25 }); expect(instrumentRepoSyncLogModel.findMany).toHaveBeenCalledWith({ - orderBy: { createdAt: 'desc' }, + orderBy: [{ createdAt: 'desc' }, { id: 'desc' }], skip: 0, take: 10, where: { AND: [{}], repoId: undefined } @@ -442,9 +442,30 @@ describe('InstrumentReposService', () => { await service.findSyncLogs({ ...query, sortOrder: 'asc' }); expect(instrumentRepoSyncLogModel.findMany.mock.lastCall?.[0]).toMatchObject({ - orderBy: { createdAt: 'asc' } + orderBy: [{ createdAt: 'asc' }, { id: 'desc' }] }); }); + + it.each([ + ['action', [{ action: 'asc' }]], + ['failedCount', [{ failures: { _count: 'asc' } }]], + ['importedCount', [{ importedCount: 'asc' }]], + ['repoName', [{ repoName: 'asc' }]], + // A null message sorts lowest, so this runs success, partial, failed, matching the derived status. + ['status', [{ message: 'asc' }, { failures: { _count: 'asc' } }]] + ] as const)( + 'should order by %s, breaking ties on the id so pages neither repeat nor skip', + async (sortBy, orderBy) => { + instrumentRepoSyncLogModel.findMany.mockResolvedValueOnce([]); + instrumentRepoSyncLogModel.count.mockResolvedValueOnce(0); + + await service.findSyncLogs({ ...query, sortBy, sortOrder: 'asc' }); + + expect(instrumentRepoSyncLogModel.findMany.mock.lastCall?.[0]).toMatchObject({ + orderBy: [...orderBy, { id: 'desc' }] + }); + } + ); }); describe('findLatestSyncLogs', () => { diff --git a/apps/api/src/instrument-repos/instrument-repos.service.ts b/apps/api/src/instrument-repos/instrument-repos.service.ts index 1f1aa27a3..3636bd91b 100644 --- a/apps/api/src/instrument-repos/instrument-repos.service.ts +++ b/apps/api/src/instrument-repos/instrument-repos.service.ts @@ -44,6 +44,26 @@ type InstrumentRepoRecord = Awaited['findFirs * The detail Nest exceptions thrown by `InstrumentsService.create` carry beyond their message: a VM * evaluation error as `cause`, schema validation errors as `issues`. */ +/** + * The trailing id makes the order total, since Mongo may otherwise return tied rows in a different + * order per page, repeating some rows and skipping others. Status is derived, not stored: Mongo sorts + * a null `message` lowest, so ordering by it and then by failure count runs SUCCESS, PARTIAL, FAILED. + */ +const getSyncLogOrderBy = ( + sortBy: InstrumentRepoSyncLogsQueryParams['sortBy'], + sortOrder: InstrumentRepoSyncLogsQueryParams['sortOrder'] +) => { + const orderBy = { + action: [{ action: sortOrder }], + createdAt: [{ createdAt: sortOrder }], + failedCount: [{ failures: { _count: sortOrder } }], + importedCount: [{ importedCount: sortOrder }], + repoName: [{ repoName: sortOrder }], + status: [{ message: sortOrder }, { failures: { _count: sortOrder } }] + }[sortBy]; + return [...orderBy, { id: 'desc' as const }]; +}; + const $ExceptionDetails = z.object({ cause: z.object({ message: z.string() }).optional(), issues: z.array(z.object({ message: z.string(), path: z.array(z.union([z.string(), z.number()])) })).optional() @@ -199,13 +219,13 @@ export class InstrumentReposService implements OnModuleInit { * returned whole: the client asks for the slice it is showing. */ async findSyncLogs( - { limit, page, repoId, sortOrder }: InstrumentRepoSyncLogsQueryParams, + { limit, page, repoId, sortBy, sortOrder }: InstrumentRepoSyncLogsQueryParams, { ability }: EntityOperationOptions = {} ): Promise { const where = { AND: [accessibleQuery(ability, 'read', 'InstrumentRepoSyncLog')], repoId }; const [data, total] = await Promise.all([ this.instrumentRepoSyncLogModel.findMany({ - orderBy: { createdAt: sortOrder }, + orderBy: getSyncLogOrderBy(sortBy, sortOrder), skip: (page - 1) * limit, take: limit, where diff --git a/packages/schemas/src/audit/__tests__/audit.test.ts b/packages/schemas/src/audit/__tests__/audit.test.ts index b068881c4..1aeb6c94a 100644 --- a/packages/schemas/src/audit/__tests__/audit.test.ts +++ b/packages/schemas/src/audit/__tests__/audit.test.ts @@ -8,7 +8,12 @@ describe('$AuditLogsQueryParams', () => { }); it('should default to the first page of the standard size, newest first', () => { - expect($AuditLogsQueryParams.parse({})).toEqual({ limit: AUDIT_LOGS_PAGE_SIZE, page: 1, sortOrder: 'desc' }); + expect($AuditLogsQueryParams.parse({})).toEqual({ + limit: AUDIT_LOGS_PAGE_SIZE, + page: 1, + sortBy: 'timestamp', + sortOrder: 'desc' + }); }); it.each(['', 'ten', '0', '-1', '1.5', String(AUDIT_LOGS_MAX_PAGE_SIZE + 1)])( diff --git a/packages/schemas/src/audit/audit.ts b/packages/schemas/src/audit/audit.ts index fc3e9054e..a97b6ccbc 100644 --- a/packages/schemas/src/audit/audit.ts +++ b/packages/schemas/src/audit/audit.ts @@ -34,10 +34,14 @@ export const $AuditLogsQuerySearchParams = z.object({ userId: z.string().optional() }); +export type $AuditLogsSortBy = z.infer; +export const $AuditLogsSortBy = z.enum(['action', 'entity', 'group', 'timestamp', 'user']); + export type $AuditLogsQueryParams = z.infer; export const $AuditLogsQueryParams = $AuditLogsQuerySearchParams.extend({ limit: $NumberLike.pipe(z.number().int().positive().max(AUDIT_LOGS_MAX_PAGE_SIZE)).default(AUDIT_LOGS_PAGE_SIZE), page: $NumberLike.pipe(z.number().int().positive()).default(1), + sortBy: $AuditLogsSortBy.default('timestamp'), sortOrder: z.enum(['asc', 'desc']).default('desc') }); diff --git a/packages/schemas/src/instrument-repo/instrument-repo.ts b/packages/schemas/src/instrument-repo/instrument-repo.ts index 08f280a70..388afaea0 100644 --- a/packages/schemas/src/instrument-repo/instrument-repo.ts +++ b/packages/schemas/src/instrument-repo/instrument-repo.ts @@ -57,6 +57,16 @@ export const INSTRUMENT_REPO_SYNC_LOGS_PAGE_SIZE = 10; /** Upper bound on `limit`, so a single request can never load an unbounded slice of the collection. */ export const INSTRUMENT_REPO_SYNC_LOGS_MAX_PAGE_SIZE = 1000; +export type InstrumentRepoSyncLogsSortBy = z.infer; +export const $InstrumentRepoSyncLogsSortBy = z.enum([ + 'action', + 'createdAt', + 'failedCount', + 'importedCount', + 'repoName', + 'status' +]); + export type InstrumentRepoSyncLogsQueryParams = z.infer; export const $InstrumentRepoSyncLogsQueryParams = z.object({ limit: $NumberLike @@ -65,6 +75,7 @@ export const $InstrumentRepoSyncLogsQueryParams = z.object({ page: $NumberLike.pipe(z.number().int().positive()).default(1), /** Confines the page to one repository, which is what the per-repository dialog asks for. */ repoId: z.string().optional(), + sortBy: $InstrumentRepoSyncLogsSortBy.default('createdAt'), sortOrder: z.enum(['asc', 'desc']).default('desc') }); From 0148fe6bc1d4bbf8625e8321530599152b583a9c Mon Sep 17 00:00:00 2001 From: thomasbeaudry Date: Thu, 5 Nov 2026 09:48:42 -0500 Subject: [PATCH 2/3] feat(web): make every table column sortable Tables differed: some sorted every column, some one, some none. Use the shared SortableHeader on every data column, ask the server to order the audit and sync logs, and move subject assignments onto DataTable. Co-Authored-By: Claude Opus 5.5 --- .../InstrumentRepoSyncLogTable.tsx | 95 +++++++++++----- .../InstrumentRepoSyncLogTable.test.tsx | 64 +++++++---- .../SortableHeader/SortableHeader.tsx | 6 +- .../__tests__/SortableHeader.test.tsx | 5 + apps/web/src/hooks/useAuditLogsQuery.ts | 8 +- .../hooks/useInstrumentRepoSyncLogsQuery.ts | 2 + .../_app/admin/__tests__/instruments.test.tsx | 21 ++++ .../_app/admin/audit/__tests__/logs.test.tsx | 49 ++++++--- apps/web/src/routes/_app/admin/audit/logs.tsx | 74 +++++-------- .../admin/groups/__tests__/index.test.tsx | 16 +++ .../src/routes/_app/admin/groups/index.tsx | 5 +- .../instrument-repos/__tests__/index.test.tsx | 39 ++++++- .../_app/admin/instrument-repos/index.tsx | 104 +++++++++++------- .../web/src/routes/_app/admin/instruments.tsx | 44 ++++++-- .../permission-sets/__tests__/index.test.tsx | 20 ++++ .../_app/admin/permission-sets/index.tsx | 13 ++- .../_app/admin/users/__tests__/index.test.tsx | 2 +- .../web/src/routes/_app/admin/users/index.tsx | 21 +--- .../$subjectId/__tests__/assignments.test.tsx | 32 +++--- .../subjects/$subjectId/assignments.tsx | 86 ++++++++------- .../$subjectId/table/__tests__/index.test.tsx | 10 +- .../subjects/$subjectId/table/index.tsx | 5 +- .../src/utils/__tests__/data-table.test.ts | 16 +++ apps/web/src/utils/data-table.ts | 18 +++ .../subjects/$subjectId/assignments.page.ts | 2 +- .../src/specs/admin-instrument-repos.spec.ts | 38 +++++++ testing/src/specs/gateway-assignment.spec.ts | 2 +- testing/src/specs/remote-assignment.spec.ts | 6 +- testing/src/specs/table-sorting.spec.ts | 67 +++++++++++ 29 files changed, 624 insertions(+), 246 deletions(-) create mode 100644 apps/web/src/utils/__tests__/data-table.test.ts create mode 100644 testing/src/specs/table-sorting.spec.ts diff --git a/apps/web/src/components/InstrumentRepoSyncLogTable/InstrumentRepoSyncLogTable.tsx b/apps/web/src/components/InstrumentRepoSyncLogTable/InstrumentRepoSyncLogTable.tsx index ecbdc647e..9cf11621a 100644 --- a/apps/web/src/components/InstrumentRepoSyncLogTable/InstrumentRepoSyncLogTable.tsx +++ b/apps/web/src/components/InstrumentRepoSyncLogTable/InstrumentRepoSyncLogTable.tsx @@ -2,34 +2,47 @@ import { Fragment, useState } from 'react'; import { Badge, Button, DataTable, Dialog } from '@douglasneuroinformatics/libui/components'; import { useTheme, useTranslation } from '@douglasneuroinformatics/libui/hooks'; -import type { InstrumentRepoSyncLog, InstrumentRepoSyncStage } from '@opendatacapture/schemas/instrument-repo'; +import { $InstrumentRepoSyncLogsSortBy } from '@opendatacapture/schemas/instrument-repo'; +import type { + InstrumentRepoSyncLog, + InstrumentRepoSyncLogsSortBy, + InstrumentRepoSyncStage +} from '@opendatacapture/schemas/instrument-repo'; import { ColorTagCell } from '@/components/ColorTagCell'; import { SortableHeader } from '@/components/SortableHeader'; import { TruncatedCell } from '@/components/TruncatedCell'; -import { MIN_TABLE_BODY_HEIGHT } from '@/utils/data-table'; +import { MIN_TABLE_BODY_HEIGHT, readServerSort } from '@/utils/data-table'; import { getSyncStatus } from '@/utils/instrument-repo-sync'; import type { InstrumentRepoSyncStatus } from '@/utils/instrument-repo-sync'; import { getSyncStatusColor } from '@/utils/tag-colors'; +type InstrumentRepoSyncLogSort = { + sortBy: InstrumentRepoSyncLogsSortBy; + sortOrder: 'asc' | 'desc'; +}; + type InstrumentRepoSyncLogTableProps = { logs: InstrumentRepoSyncLog[]; onPageChange: (page: number) => void; - onSortOrderChange: (sortOrder: 'asc' | 'desc') => void; + onSortChange: (sort: InstrumentRepoSyncLogSort) => void; pageCount: number; - sortOrder: 'asc' | 'desc'; + sort: InstrumentRepoSyncLogSort; }; -export const InstrumentRepoSyncLogTable = ({ +const InstrumentRepoSyncLogTable = ({ logs, onPageChange, - onSortOrderChange, + onSortChange, pageCount, - sortOrder + sort }: InstrumentRepoSyncLogTableProps) => { const { t } = useTranslation(); const [theme] = useTheme(); const [logToView, setLogToView] = useState(null); + // In server mode the table mounts with no sorting state of its own, so the column it is ordered by + // is told its direction, or it would read as unsorted. + const sortOrderOf = (sortBy: InstrumentRepoSyncLogsSortBy) => (sort.sortBy === sortBy ? sort.sortOrder : undefined); const statusLabels = { FAILED: t({ en: 'Failed', es: 'Fallido', fr: 'Échec' }), @@ -74,6 +87,7 @@ export const InstrumentRepoSyncLogTable = ({ disableSearch columns={[ { + accessorKey: 'createdAt', cell: (ctx) => ( ), - enableSorting: true, header: ({ column }) => ( ), - id: 'date', size: 150, // The rows already arrive newest first, so the table's first toggle of its unsorted // column must go to ascending, or the first click would request what is on screen. @@ -106,20 +118,31 @@ export const InstrumentRepoSyncLogTable = ({ value={ctx.row.original.repoName} /> ), - enableSorting: false, - header: t({ en: 'Repository', es: 'Repositorio', fr: 'Dépôt' }) + header: ({ column }) => ( + + ) }, { + accessorKey: 'action', cell: (ctx) => { const label = actionLabels[ctx.row.original.action]; return ; }, - enableSorting: false, - header: t({ en: 'Action', es: 'Acción', fr: 'Action' }), - id: 'action', + header: ({ column }) => ( + + ), size: 95 }, { + accessorFn: getSyncStatus, cell: (ctx) => { const status = getSyncStatus(ctx.row.original); return ( @@ -130,8 +153,13 @@ export const InstrumentRepoSyncLogTable = ({ /> ); }, - enableSorting: false, - header: t({ en: 'Status', es: 'Estado', fr: 'Statut' }), + header: ({ column }) => ( + + ), id: 'status', size: 140 }, @@ -141,19 +169,30 @@ export const InstrumentRepoSyncLogTable = ({ const count = String(ctx.row.original.importedCount); return ; }, - enableSorting: false, - header: t({ en: 'Imported', es: 'Importados', fr: 'Importés' }), - size: 105 + header: ({ column }) => ( + + ), + size: 114 }, { + accessorFn: (log) => log.failures.length, cell: (ctx) => { const count = String(ctx.row.original.failures.length); return ; }, - enableSorting: false, - header: t({ en: 'Failed', es: 'Fallidos', fr: 'Échoués' }), + header: ({ column }) => ( + + ), id: 'failedCount', - size: 85 + size: 92 }, { cell: (ctx) => { @@ -185,13 +224,15 @@ export const InstrumentRepoSyncLogTable = ({ // rest, so pinning every column but the repository leaves the repository the room a long // owner/name needs. initialState={{ - columnPinning: { left: ['date'], right: ['action', 'status', 'importedCount', 'failedCount', 'messages'] } + columnPinning: { + left: ['createdAt'], + right: ['action', 'status', 'importedCount', 'failedCount', 'messages'] + } }} mode="server" pageCount={pageCount} onPaginationChange={({ pageIndex }) => onPageChange(pageIndex + 1)} - // Date is the only sortable column, so the state describes it or nothing. - onSortingChange={(state) => onSortOrderChange(state.some(({ desc }) => desc) ? 'desc' : 'asc')} + onSortingChange={(state) => onSortChange(readServerSort(state, $InstrumentRepoSyncLogsSortBy.options))} /> setLogToView(null)}> @@ -227,3 +268,5 @@ export const InstrumentRepoSyncLogTable = ({ ); }; + +export { type InstrumentRepoSyncLogSort, InstrumentRepoSyncLogTable }; diff --git a/apps/web/src/components/InstrumentRepoSyncLogTable/__tests__/InstrumentRepoSyncLogTable.test.tsx b/apps/web/src/components/InstrumentRepoSyncLogTable/__tests__/InstrumentRepoSyncLogTable.test.tsx index 22f915c85..62511fa2a 100644 --- a/apps/web/src/components/InstrumentRepoSyncLogTable/__tests__/InstrumentRepoSyncLogTable.test.tsx +++ b/apps/web/src/components/InstrumentRepoSyncLogTable/__tests__/InstrumentRepoSyncLogTable.test.tsx @@ -4,6 +4,8 @@ import { afterEach, describe, expect, it, vi } from 'vitest'; import { InstrumentRepoSyncLogTable } from '../InstrumentRepoSyncLogTable'; +import type { InstrumentRepoSyncLogSort } from '../InstrumentRepoSyncLogTable'; + import '@/services/i18n'; const theme = vi.hoisted(() => ({ current: 'light' })); @@ -30,17 +32,17 @@ const logFixture = (overrides: Partial = {}): InstrumentR type TableProps = { logs: InstrumentRepoSyncLog[]; onPageChange?: (page: number) => void; - onSortOrderChange?: (sortOrder: 'asc' | 'desc') => void; + onSortChange?: (sort: InstrumentRepoSyncLogSort) => void; pageCount?: number; - sortOrder?: 'asc' | 'desc'; + sort?: InstrumentRepoSyncLogSort; }; -const Table = ({ pageCount = 1, sortOrder = 'desc', ...props }: TableProps) => ( +const Table = ({ pageCount = 1, sort = { sortBy: 'createdAt', sortOrder: 'desc' }, ...props }: TableProps) => ( undefined} - onSortOrderChange={() => undefined} + onSortChange={() => undefined} {...props} /> ); @@ -165,33 +167,53 @@ describe('InstrumentRepoSyncLogTable', () => { expect(within(rows()[0]!).getByText('Newer')).toBeTruthy(); }); + const headerButton = (name: string) => + within(screen.getByTestId('data-table-head')).getByText(name).closest('button')!; + it('should ask for the other order when the date header is clicked, rather than sort the page', () => { - const onSortOrderChange = vi.fn(); - render(); + const onSortChange = vi.fn(); + render(
); - fireEvent.click(within(screen.getByTestId('data-table-head')).getByText('Date')); + fireEvent.click(headerButton('Date')); - expect(onSortOrderChange).toHaveBeenCalledWith('asc'); + expect(onSortChange).toHaveBeenCalledWith({ sortBy: 'createdAt', sortOrder: 'asc' }); }); it('should cycle the order it asks for, since the column clears back to ascending', () => { - const onSortOrderChange = vi.fn(); - render(
); - const date = within(screen.getByTestId('data-table-head')).getByText('Date'); + const onSortChange = vi.fn(); + render(
); - fireEvent.click(date); - fireEvent.click(date); - fireEvent.click(date); + fireEvent.click(headerButton('Date')); + fireEvent.click(headerButton('Date')); + fireEvent.click(headerButton('Date')); - expect(onSortOrderChange.mock.calls.flat()).toEqual(['asc', 'desc', 'asc']); + expect(onSortChange.mock.calls.flat().map(({ sortOrder }) => sortOrder)).toEqual(['asc', 'desc', 'asc']); }); - it('should offer no sorting on a column the server cannot order by, so no control can mislead', () => { + it.each([ + ['Repository', 'repoName'], + ['Action', 'action'], + ['Status', 'status'], + ['Imported', 'importedCount'], + ['Failed', 'failedCount'] + ])('should ask the server to order by %s, since it holds every page', (name, sortBy) => { + const onSortChange = vi.fn(); + render(
); + + fireEvent.click(headerButton(name)); + + expect(onSortChange.mock.lastCall?.[0]).toMatchObject({ sortBy }); + }); + + it('should show the direction only on the column the server ordered by', () => { + render(
); + expect(headerButton('Status').querySelector('.lucide-chevron-up')).toBeTruthy(); + expect(headerButton('Date').querySelector('.lucide-chevrons-up-down')).toBeTruthy(); + }); + + it('should offer no sorting on the messages column, which holds a button rather than a value', () => { render(
); - const head = within(screen.getByTestId('data-table-head')); - expect(head.getByText('Date').tagName).toBe('BUTTON'); - expect(head.getByText('Status').tagName).not.toBe('BUTTON'); - expect(head.getByText('Repository').tagName).not.toBe('BUTTON'); + expect(within(screen.getByTestId('data-table-head')).getByText('Messages').closest('button')).toBeNull(); }); it('should ask for the next page rather than hold the whole history', () => { diff --git a/apps/web/src/components/SortableHeader/SortableHeader.tsx b/apps/web/src/components/SortableHeader/SortableHeader.tsx index 822be864f..ed2763b01 100644 --- a/apps/web/src/components/SortableHeader/SortableHeader.tsx +++ b/apps/web/src/components/SortableHeader/SortableHeader.tsx @@ -23,11 +23,13 @@ export const SortableHeader = ({ column, label, sortOrder }: SortableHead const Icon = sorted ? SORT_ICONS[sorted] : ChevronsUpDownIcon; return ( ); diff --git a/apps/web/src/components/SortableHeader/__tests__/SortableHeader.test.tsx b/apps/web/src/components/SortableHeader/__tests__/SortableHeader.test.tsx index f8355aa1f..4b8754c62 100644 --- a/apps/web/src/components/SortableHeader/__tests__/SortableHeader.test.tsx +++ b/apps/web/src/components/SortableHeader/__tests__/SortableHeader.test.tsx @@ -15,6 +15,11 @@ describe('SortableHeader', () => { expect(screen.getByRole('button', { name: /Instrument/u })).toBeTruthy(); }); + it('should carry the full label as a hover title, since a narrow column clips it', () => { + render( column={columnOf(false)} label="Dernière synchronisation" />); + expect(screen.getByText('Dernière synchronisation').getAttribute('title')).toBe('Dernière synchronisation'); + }); + it('should ask the column to toggle its sort when clicked', () => { const toggleSorting = vi.fn(); render( column={columnOf(false, toggleSorting)} label="Records" />); diff --git a/apps/web/src/hooks/useAuditLogsQuery.ts b/apps/web/src/hooks/useAuditLogsQuery.ts index fa7fb9cb1..48e364e82 100644 --- a/apps/web/src/hooks/useAuditLogsQuery.ts +++ b/apps/web/src/hooks/useAuditLogsQuery.ts @@ -1,10 +1,16 @@ import { $AuditLogsPage, AUDIT_LOGS_MAX_PAGE_SIZE, AUDIT_LOGS_PAGE_SIZE } from '@opendatacapture/schemas/audit'; -import type { $AuditLog, $AuditLogsQueryParams, $AuditLogsQuerySearchParams } from '@opendatacapture/schemas/audit'; +import type { + $AuditLog, + $AuditLogsQueryParams, + $AuditLogsQuerySearchParams, + $AuditLogsSortBy +} from '@opendatacapture/schemas/audit'; import { keepPreviousData, queryOptions, useQuery } from '@tanstack/react-query'; import axios from 'axios'; type AuditLogsQueryParams = $AuditLogsQuerySearchParams & { page?: number; + sortBy?: $AuditLogsSortBy; sortOrder?: 'asc' | 'desc'; }; diff --git a/apps/web/src/hooks/useInstrumentRepoSyncLogsQuery.ts b/apps/web/src/hooks/useInstrumentRepoSyncLogsQuery.ts index 813348f17..4b943df85 100644 --- a/apps/web/src/hooks/useInstrumentRepoSyncLogsQuery.ts +++ b/apps/web/src/hooks/useInstrumentRepoSyncLogsQuery.ts @@ -2,12 +2,14 @@ import { $InstrumentRepoSyncLogsPage, INSTRUMENT_REPO_SYNC_LOGS_PAGE_SIZE } from '@opendatacapture/schemas/instrument-repo'; +import type { InstrumentRepoSyncLogsSortBy } from '@opendatacapture/schemas/instrument-repo'; import { keepPreviousData, queryOptions, useQuery } from '@tanstack/react-query'; import axios from 'axios'; type InstrumentRepoSyncLogsQueryParams = { page?: number; repoId?: string; + sortBy?: InstrumentRepoSyncLogsSortBy; sortOrder?: 'asc' | 'desc'; }; diff --git a/apps/web/src/routes/_app/admin/__tests__/instruments.test.tsx b/apps/web/src/routes/_app/admin/__tests__/instruments.test.tsx index 4d8b74b0c..5b1092cf8 100644 --- a/apps/web/src/routes/_app/admin/__tests__/instruments.test.tsx +++ b/apps/web/src/routes/_app/admin/__tests__/instruments.test.tsx @@ -129,6 +129,8 @@ const renderPage = (view?: 'forms' | 'series') => { const rows = () => screen.getAllByTestId('data-table-row'); +const headerButton = (name: string) => within(screen.getByTestId('data-table-head')).getByText(name).closest('button')!; + const cellTexts = (rowIndex = 0) => { return [...rows()[rowIndex]!.querySelectorAll('span[title]')].map((cell) => cell.getAttribute('title')); }; @@ -173,6 +175,17 @@ describe('Route', () => { ]); }); + it.each(['Name', 'Source', 'Groups Using It', 'Kind', 'Edition', 'Added'])( + 'should sort the forms by %s when its header is clicked', + (name) => { + mocks.instrumentInfo = [scalarFixture({ id: 'form-1', title: 'Form' })]; + renderPage(); + expect(headerButton(name).querySelector('.lucide-chevrons-up-down')).toBeTruthy(); + fireEvent.click(headerButton(name)); + expect(headerButton(name).querySelector('.lucide-chevrons-up-down')).toBeNull(); + } + ); + it('should open the forms view when no view is requested', () => { renderPage(); expect(screen.getByRole('heading', { name: 'Form & Interactive Instruments' })).toBeTruthy(); @@ -295,6 +308,14 @@ describe('Route', () => { }); describe('admin instruments series view', () => { + it.each(['Name', 'Group', 'Created', 'Status'])('should sort by %s when its header is clicked', (name) => { + mocks.seriesOverview = [overviewFixture()]; + renderPage('series'); + expect(headerButton(name).querySelector('.lucide-chevrons-up-down')).toBeTruthy(); + fireEvent.click(headerButton(name)); + expect(headerButton(name).querySelector('.lucide-chevrons-up-down')).toBeNull(); + }); + it('should describe an active series shared across every group', () => { mocks.seriesOverview = [overviewFixture()]; renderPage('series'); diff --git a/apps/web/src/routes/_app/admin/audit/__tests__/logs.test.tsx b/apps/web/src/routes/_app/admin/audit/__tests__/logs.test.tsx index 293be35ee..9e1604c69 100644 --- a/apps/web/src/routes/_app/admin/audit/__tests__/logs.test.tsx +++ b/apps/web/src/routes/_app/admin/audit/__tests__/logs.test.tsx @@ -2,7 +2,7 @@ import type { DataTableProps } from '@douglasneuroinformatics/libui/components'; import type { $AuditLog, $AuditLogsPage, $AuditLogsQuerySearchParams } from '@opendatacapture/schemas/audit'; import type { Group } from '@opendatacapture/schemas/group'; import type { User } from '@opendatacapture/schemas/user'; -import { act, cleanup, fireEvent, render, screen, within } from '@testing-library/react'; +import { cleanup, fireEvent, render, screen, within } from '@testing-library/react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { auditLogsQueryOptions } from '@/hooks/useAuditLogsQuery'; @@ -160,7 +160,9 @@ const navigatedSearch = (current: AuditLogsSearch = mocks.search) => { return typeof search === 'function' ? search(current) : search; }; -const timeHeaderButton = () => within(screen.getByTestId('data-table-head')).getByText('Time', { selector: 'button' }); +const headerButton = (name: string) => within(screen.getByTestId('data-table-head')).getByText(name).closest('button')!; + +const timeHeaderButton = () => headerButton('Time'); const goToPage = (page: number) => { screen @@ -200,7 +202,7 @@ describe('Route', () => { const { ensureQueryData, promise } = runLoader(search); await promise; expect(ensureQueryData.mock.calls.map(([options]) => options.queryKey)).toEqual([ - auditLogsQueryOptions({ params: { ...search, page: 1, sortOrder: 'desc' } }).queryKey, + auditLogsQueryOptions({ params: { ...search, page: 1, sortBy: 'timestamp', sortOrder: 'desc' } }).queryKey, groupsQueryOptions().queryKey, usersQueryOptions().queryKey ]); @@ -338,7 +340,7 @@ describe('Route', () => { it('should request the first page newest first, with the active filters', () => { mocks.search = { userId: 'user-1' }; renderPage(); - expect(lastQueryParams()).toEqual({ page: 1, sortOrder: 'desc', userId: 'user-1' }); + expect(lastQueryParams()).toEqual({ page: 1, sortBy: 'timestamp', sortOrder: 'desc', userId: 'user-1' }); }); it('should show who acted, in which group, and the translated action and entity', () => { @@ -411,36 +413,51 @@ describe('Route', () => { it('should mark the time column as descending before the user sorts, matching the server order', () => { renderPage(); - expect(timeHeaderButton().querySelector('.lucide-arrow-down')).toBeTruthy(); + expect(timeHeaderButton().querySelector('.lucide-chevron-down')).toBeTruthy(); }); it('should request the next page when the user pages forward', () => { renderPage(); goToPage(2); - expect(lastQueryParams()).toEqual({ page: 2, sortOrder: 'desc' }); + expect(lastQueryParams()).toEqual({ page: 2, sortBy: 'timestamp', sortOrder: 'desc' }); }); it('should sort oldest first on the first click of the time header, since the logs already arrive newest first', () => { renderPage(); goToPage(2); fireEvent.click(timeHeaderButton()); - expect(lastQueryParams()).toEqual({ page: 1, sortOrder: 'asc' }); - expect(timeHeaderButton().querySelector('.lucide-arrow-up')).toBeTruthy(); + expect(lastQueryParams()).toEqual({ page: 1, sortBy: 'timestamp', sortOrder: 'asc' }); + expect(timeHeaderButton().querySelector('.lucide-chevron-up')).toBeTruthy(); }); it('should return to newest first on the second click of the time header', () => { renderPage(); fireEvent.click(timeHeaderButton()); fireEvent.click(timeHeaderButton()); - expect(lastQueryParams()).toEqual({ page: 1, sortOrder: 'desc' }); - expect(timeHeaderButton().querySelector('.lucide-arrow-down')).toBeTruthy(); - }); + expect(lastQueryParams()).toEqual({ page: 1, sortBy: 'timestamp', sortOrder: 'desc' }); + expect(timeHeaderButton().querySelector('.lucide-chevron-down')).toBeTruthy(); + }); + + it.each([ + ['User', 'user'], + ['Active Group', 'group'], + ['Action', 'action'], + ['Entity', 'entity'] + ])( + 'should ask the server to order by %s and return to the first page, since it holds every page', + (name, sortBy) => { + renderPage(); + goToPage(2); + fireEvent.click(headerButton(name)); + expect(lastQueryParams()).toEqual({ page: 1, sortBy, sortOrder: 'asc' }); + } + ); - it('should ignore a sorting change that does not involve the time column', () => { + it('should dim the time column once another column orders the table', () => { renderPage(); - goToPage(2); - act(() => mocks.reportSorting!([])); - expect(lastQueryParams()).toEqual({ page: 2, sortOrder: 'desc' }); + fireEvent.click(headerButton('User')); + expect(timeHeaderButton().querySelector('.lucide-chevrons-up-down')).toBeTruthy(); + expect(headerButton('User').querySelector('.lucide-chevron-up')).toBeTruthy(); }); it('should return to the first page when the filters change', () => { @@ -448,7 +465,7 @@ describe('Route', () => { goToPage(2); mocks.search = { action: 'LOGIN' }; rerender(); - expect(lastQueryParams()).toEqual({ action: 'LOGIN', page: 1, sortOrder: 'desc' }); + expect(lastQueryParams()).toEqual({ action: 'LOGIN', page: 1, sortBy: 'timestamp', sortOrder: 'desc' }); }); }); }); diff --git a/apps/web/src/routes/_app/admin/audit/logs.tsx b/apps/web/src/routes/_app/admin/audit/logs.tsx index de78ff129..4c0243ab7 100644 --- a/apps/web/src/routes/_app/admin/audit/logs.tsx +++ b/apps/web/src/routes/_app/admin/audit/logs.tsx @@ -1,25 +1,26 @@ import React, { Fragment, useCallback, useMemo, useState } from 'react'; import { snakeToCamelCase, toLowerCase } from '@douglasneuroinformatics/libjs'; -import { Button, DataTable, DropdownMenu, Heading, TanstackTable } from '@douglasneuroinformatics/libui/components'; +import { Button, DataTable, DropdownMenu, Heading } from '@douglasneuroinformatics/libui/components'; import { useDownload, useTheme, useTranslation } from '@douglasneuroinformatics/libui/hooks'; -import { $AuditLogsQuerySearchParams } from '@opendatacapture/schemas/audit'; +import { $AuditLogsQuerySearchParams, $AuditLogsSortBy } from '@opendatacapture/schemas/audit'; import type { - $AuditLog, $AuditLogAction, $AuditLogEntity, $AuditLogsQuerySearchParams as AuditLogsSearchParams } from '@opendatacapture/schemas/audit'; import { createFileRoute } from '@tanstack/react-router'; -import { ArrowDownIcon, ArrowUpIcon, CalendarIcon, DownloadIcon, XIcon } from 'lucide-react'; +import { CalendarIcon, DownloadIcon, XIcon } from 'lucide-react'; import { ColorTagCell } from '@/components/ColorTagCell'; import { FilterMenu } from '@/components/FilterMenu'; import { PageHeader } from '@/components/PageHeader'; +import { SortableHeader } from '@/components/SortableHeader'; import { auditLogsQueryOptions, fetchAllAuditLogs, useAuditLogsQuery } from '@/hooks/useAuditLogsQuery'; import { groupsQueryOptions, useGroupsQuery } from '@/hooks/useGroupsQuery'; import { usersQueryOptions, useUsersQuery } from '@/hooks/useUsersQuery'; import { resolveTheme } from '@/utils/chart-theme'; +import { readServerSort } from '@/utils/data-table'; import { getAuditActionColor } from '@/utils/tag-colors'; type DateFormat = 'iso' | 'local'; @@ -28,7 +29,7 @@ type SortOrder = 'asc' | 'desc'; const INITIAL_PAGE = 1; -const INITIAL_SORT_ORDER: SortOrder = 'desc'; +const INITIAL_SORT: { sortBy: $AuditLogsSortBy; sortOrder: SortOrder } = { sortBy: 'timestamp', sortOrder: 'desc' }; const ACTIONS: $AuditLogAction[] = ['CREATE', 'DELETE', 'UPDATE', 'ARCHIVE', 'UNARCHIVE', 'LOGIN', 'SEND_EMAIL']; @@ -42,34 +43,6 @@ const ENTITIES: $AuditLogEntity[] = [ 'USER' ]; -// The sort direction is rendered from the value the rows were actually fetched with, rather than from -// `column.getIsSorted()`: in server mode the table mounts with no sorting state of its own, so the -// column would read as unsorted even though the server has already ordered by timestamp descending. -const SortableHeader = ({ - column, - label, - sortOrder -}: { - column: TanstackTable.Column<$AuditLog>; - label: string; - sortOrder: SortOrder; -}) => { - return ( - - ); -}; - const DateFormatMenu = ({ onChange, value }: { onChange: (value: DateFormat) => void; value: DateFormat }) => { const { t } = useTranslation(); const options: { label: string; value: DateFormat }[] = [ @@ -192,9 +165,12 @@ const AuditLogsToolbar = () => { const AuditLogsTable: React.FC<{ search: AuditLogsSearchParams }> = ({ search }) => { const [page, setPage] = useState(INITIAL_PAGE); - const [sortOrder, setSortOrder] = useState(INITIAL_SORT_ORDER); + const [sort, setSort] = useState(INITIAL_SORT); - const auditLogsQuery = useAuditLogsQuery({ params: { ...search, page, sortOrder } }); + const auditLogsQuery = useAuditLogsQuery({ params: { ...search, page, ...sort } }); + // In server mode the table mounts with no sorting state of its own, so the column it is ordered by + // is told its direction, or it would read as unsorted. + const sortOrderOf = (sortBy: $AuditLogsSortBy) => (sort.sortBy === sortBy ? sort.sortOrder : undefined); const { resolvedLanguage, t } = useTranslation(); const [theme] = useTheme(); @@ -226,10 +202,9 @@ const AuditLogsTable: React.FC<{ search: AuditLogsSearchParams }> = ({ search }) return formatTimestamp(value); }, enableResizing: false, - enableSorting: true, header: ({ column }) => (
- +
), @@ -244,7 +219,9 @@ const AuditLogsTable: React.FC<{ search: AuditLogsSearchParams }> = ({ search }) const value = ctx.getValue() as string; return {value}; }, - header: t('common.user'), + header: ({ column }) => ( + + ), id: 'user' }, { @@ -253,7 +230,9 @@ const AuditLogsTable: React.FC<{ search: AuditLogsSearchParams }> = ({ search }) const value = ctx.getValue() as string; return {value}; }, - header: t('common.activeGroup'), + header: ({ column }) => ( + + ), id: 'group' }, { @@ -268,7 +247,9 @@ const AuditLogsTable: React.FC<{ search: AuditLogsSearchParams }> = ({ search }) /> ); }, - header: t('common.action') + header: ({ column }) => ( + + ) }, { accessorKey: 'entity', @@ -280,7 +261,9 @@ const AuditLogsTable: React.FC<{ search: AuditLogsSearchParams }> = ({ search }) ); }, - header: t('common.entity') + header: ({ column }) => ( + + ) } ]} data={auditLogsQuery.data?.data ?? []} @@ -288,11 +271,8 @@ const AuditLogsTable: React.FC<{ search: AuditLogsSearchParams }> = ({ search }) pageCount={auditLogsQuery.data?.pageCount ?? 0} onPaginationChange={({ pageIndex }) => setPage(pageIndex + 1)} onSortingChange={(state) => { - const timestamp = state.find(({ id }) => id === 'timestamp'); - if (timestamp) { - setSortOrder(timestamp.desc ? 'desc' : 'asc'); - setPage(INITIAL_PAGE); - } + setSort(readServerSort(state, $AuditLogsSortBy.options)); + setPage(INITIAL_PAGE); }} /> ); @@ -324,7 +304,7 @@ export const Route = createFileRoute('/_app/admin/audit/logs')({ loader: async ({ context, deps }) => { await Promise.all([ context.queryClient.ensureQueryData( - auditLogsQueryOptions({ params: { ...deps.search, page: INITIAL_PAGE, sortOrder: INITIAL_SORT_ORDER } }) + auditLogsQueryOptions({ params: { ...deps.search, page: INITIAL_PAGE, ...INITIAL_SORT } }) ), context.queryClient.ensureQueryData(groupsQueryOptions()), context.queryClient.ensureQueryData(usersQueryOptions()) diff --git a/apps/web/src/routes/_app/admin/groups/__tests__/index.test.tsx b/apps/web/src/routes/_app/admin/groups/__tests__/index.test.tsx index ef333d0a3..45e9dc4c2 100644 --- a/apps/web/src/routes/_app/admin/groups/__tests__/index.test.tsx +++ b/apps/web/src/routes/_app/admin/groups/__tests__/index.test.tsx @@ -91,6 +91,8 @@ const renderPage = (queryClient = new QueryClient()) => { const rows = () => screen.getAllByTestId('data-table-row'); +const headerButton = (name: string) => within(screen.getByTestId('data-table-head')).getByText(name).closest('button')!; + const openSheet = (rowIndex = 0) => { fireEvent.doubleClick(rows()[rowIndex]!); return screen.getByRole('dialog'); @@ -126,6 +128,20 @@ describe('Route', () => { afterEach(cleanup); + it('should sort the groups by name, then by type, as each header is clicked', () => { + mocks.groups = [ + groupFixture({ id: 'b', name: 'Beta', type: 'RESEARCH' }), + groupFixture({ id: 'a', name: 'Alpha', type: 'CLINICAL' }) + ]; + renderPage(); + fireEvent.click(headerButton('Group Name')); + expect(rows()[0]!.textContent).toContain('Alpha'); + fireEvent.click(headerButton('Group Name')); + expect(rows()[0]!.textContent).toContain('Beta'); + fireEvent.click(headerButton('Group Type')); + expect(rows()[0]!.textContent).toContain('Alpha'); + }); + it('should prefetch the groups and the instrument repositories, so the sheet never suspends', async () => { const queryClient = new QueryClient(); const ensureQueryData = vi.spyOn(queryClient, 'ensureQueryData').mockResolvedValue([]); diff --git a/apps/web/src/routes/_app/admin/groups/index.tsx b/apps/web/src/routes/_app/admin/groups/index.tsx index 7f8fa33e3..99e4956e2 100644 --- a/apps/web/src/routes/_app/admin/groups/index.tsx +++ b/apps/web/src/routes/_app/admin/groups/index.tsx @@ -10,6 +10,7 @@ import axios from 'axios'; import { ColorTagCell } from '@/components/ColorTagCell'; import { PageHeader } from '@/components/PageHeader'; +import { SortableHeader } from '@/components/SortableHeader'; import { useDeleteGroupMutation } from '@/hooks/useDeleteGroupMutation'; import { GROUPS_QUERY_KEY, groupsQueryOptions, useGroupsQuery } from '@/hooks/useGroupsQuery'; import { @@ -119,7 +120,7 @@ const RouteComponent = () => { ); }, - header: t('common.groupName') + header: ({ column }) => }, { accessorKey: 'type', @@ -137,7 +138,7 @@ const RouteComponent = () => { /> ); }, - header: t('common.groupType') + header: ({ column }) => } ]} data={groupsQuery.data} diff --git a/apps/web/src/routes/_app/admin/instrument-repos/__tests__/index.test.tsx b/apps/web/src/routes/_app/admin/instrument-repos/__tests__/index.test.tsx index b025effbf..030e6b0fb 100644 --- a/apps/web/src/routes/_app/admin/instrument-repos/__tests__/index.test.tsx +++ b/apps/web/src/routes/_app/admin/instrument-repos/__tests__/index.test.tsx @@ -162,6 +162,8 @@ const renderPage = (repos: InstrumentRepo[] = [repoFixture()]) => { const rows = () => screen.getAllByTestId('data-table-row'); +const headerButton = (name: string) => within(screen.getByTestId('data-table-head')).getByText(name).closest('button')!; + // Radix tabs activate on mouse down, not click. const openActivityLog = () => { return fireEvent.mouseDown(screen.getByTestId('instrument-repos-section-ACTIVITY'), { button: 0 }); @@ -189,6 +191,41 @@ const runLoader = (queryClient: QueryClient) => { }; describe('Route', () => { + describe('repository table sorting', () => { + afterEach(cleanup); + + it('should order the last synced date as a date, since a formatted string sorts by its month or day', () => { + renderPage([ + repoFixture({ id: 'new', lastSyncedAt: new Date('2026-02-01'), name: 'New' }), + repoFixture({ id: 'old', lastSyncedAt: new Date('2025-12-15'), name: 'Old' }) + ]); + fireEvent.click(headerButton('Last Synced')); + expect(rows()[0]!.textContent).toContain('New'); + fireEvent.click(headerButton('Last Synced')); + expect(rows()[0]!.textContent).toContain('Old'); + }); + + it('should order by the failed count of the latest sync, keeping a never-synced repository last', () => { + mocks.latestSyncLogs = [ + syncLogFixture({ failures: [{ instrumentDir: 'A', message: 'x', stage: 'LOAD' }], repoId: 'one' }), + syncLogFixture({ failures: [], repoId: 'none' }) + ]; + renderPage([ + repoFixture({ id: 'never', name: 'Never' }), + repoFixture({ id: 'none', name: 'None' }), + repoFixture({ id: 'one', name: 'One' }) + ]); + fireEvent.click(headerButton('Failed')); + expect(rows().map((row) => /Never|None|One/u.exec(row.textContent)![0])).toEqual(['One', 'None', 'Never']); + }); + + it.each(['Repository Name', 'URL', 'Instruments'])('should sort by %s when its header is clicked', (name) => { + renderPage([repoFixture()]); + fireEvent.click(headerButton(name)); + expect(headerButton(name).querySelector('.lucide-chevrons-up-down')).toBeNull(); + }); + }); + beforeEach(() => { vi.clearAllMocks(); mocks.groups = []; @@ -324,7 +361,7 @@ describe('Route', () => { fireEvent.click(within(log.getByTestId('data-table-head')).getByText('Date')); - expect(mocks.syncLogsParams.at(-1)).toMatchObject({ page: 1, sortOrder: 'asc' }); + expect(mocks.syncLogsParams.at(-1)).toMatchObject({ page: 1, sortBy: 'createdAt', sortOrder: 'asc' }); }); it("should request one repository's own entries in its dialog, rather than filter the page on screen", () => { diff --git a/apps/web/src/routes/_app/admin/instrument-repos/index.tsx b/apps/web/src/routes/_app/admin/instrument-repos/index.tsx index 293b3c657..57c3cfece 100644 --- a/apps/web/src/routes/_app/admin/instrument-repos/index.tsx +++ b/apps/web/src/routes/_app/admin/instrument-repos/index.tsx @@ -21,8 +21,10 @@ import { createFileRoute } from '@tanstack/react-router'; import { AnimatePresence, motion } from 'motion/react'; import { InstrumentRepoSyncLogTable } from '@/components/InstrumentRepoSyncLogTable'; +import type { InstrumentRepoSyncLogSort } from '@/components/InstrumentRepoSyncLogTable'; import { PageHeader } from '@/components/PageHeader'; import { SectionTabs } from '@/components/SectionTabs'; +import { SortableHeader } from '@/components/SortableHeader'; import { TruncatedCell } from '@/components/TruncatedCell'; import { useClearInstrumentRepoSyncLogsMutation } from '@/hooks/useClearInstrumentRepoSyncLogsMutation'; import { useCreateInstrumentRepoMutation } from '@/hooks/useCreateInstrumentRepoMutation'; @@ -175,7 +177,7 @@ type InstrumentReposSection = 'ACTIVITY' | 'REPOSITORIES'; const INITIAL_PAGE = 1; -const INITIAL_SORT_ORDER = 'desc'; +const INITIAL_SORT: InstrumentRepoSyncLogSort = { sortBy: 'createdAt', sortOrder: 'desc' }; /** * One page of the activity log, for every repository or for one. It owns its own paging state, so @@ -183,16 +185,16 @@ const INITIAL_SORT_ORDER = 'desc'; */ const SyncLogTable = ({ repoId }: { repoId?: string }) => { const [page, setPage] = useState(INITIAL_PAGE); - const [sortOrder, setSortOrder] = useState<'asc' | 'desc'>(INITIAL_SORT_ORDER); - const syncLogsQuery = useInstrumentRepoSyncLogsQuery({ params: { page, repoId, sortOrder } }); + const [sort, setSort] = useState(INITIAL_SORT); + const syncLogsQuery = useInstrumentRepoSyncLogsQuery({ params: { page, repoId, ...sort } }); return ( { - setSortOrder(order); + onSortChange={(nextSort) => { + setSort(nextSort); setPage(INITIAL_PAGE); }} /> @@ -211,7 +213,7 @@ const RouteComponent = () => { // The same first page the loader prefetched and `SyncLogTable` mounts on, so reading the total // here costs no extra request. const syncLogsQuery = useInstrumentRepoSyncLogsQuery({ - params: { page: INITIAL_PAGE, sortOrder: INITIAL_SORT_ORDER } + params: { page: INITIAL_PAGE, ...INITIAL_SORT } }); const latestSyncLogsQuery = useLatestInstrumentRepoSyncLogsQuery(); const clearSyncLogsMutation = useClearInstrumentRepoSyncLogsMutation(); @@ -232,6 +234,15 @@ const RouteComponent = () => { // Compute which repos are currently assigned to at least one (live) group. const inUseRepoIds = new Set(groupsQuery.data.flatMap((group) => group.instrumentRepoIds)); const latestSyncLogByRepoId = syncLogsByRepoId(latestSyncLogsQuery.data); + // Tanstack caches a column's value on the row object, so a value that depends on a separately + // loaded query has to be in the row, or the sort would keep what it read before the query settled. + const repoRows = reposQuery.data.map((repo) => ({ + ...repo, + failedCount: latestSyncLogByRepoId.get(repo.id)?.failures.length, + instrumentCount: instrumentInfoQuery.data + ? currentRepoInstruments(instrumentInfoQuery.data, repo.id).length + : repo.instrumentIds.length + })); const sectionHeadings = { ACTIVITY: { @@ -332,11 +343,16 @@ const RouteComponent = () => { ); }, - header: t({ - en: 'Repository Name', - es: 'Nombre del repositorio', - fr: 'Nom du dépôt' - }) + header: ({ column }) => ( + + ) }, { accessorKey: 'url', @@ -347,24 +363,26 @@ const RouteComponent = () => { value={ctx.row.original.url} /> ), - header: 'URL', - size: 210 + header: ({ column }) => , + size: 165 }, { - accessorFn: (row: InstrumentRepo) => { - return instrumentInfoQuery.data - ? currentRepoInstruments(instrumentInfoQuery.data, row.id).length - : row.instrumentIds.length; - }, - header: t({ - en: 'Instruments', - es: 'Instrumentos', - fr: 'Instruments' - }), + accessorKey: 'instrumentCount', + header: ({ column }) => ( + + ), id: 'instrumentCount', - size: 118 + size: 135 }, { + accessorKey: 'failedCount', cell: (ctx) => { const log = latestSyncLogByRepoId.get(ctx.row.original.id); if (!log) { @@ -372,24 +390,36 @@ const RouteComponent = () => { } return {log.failures.length}; }, - header: t({ en: 'Failed', es: 'Fallidos', fr: 'Échoués' }), + header: ({ column }) => ( + + ), id: 'failedCount', - size: 88 + size: 100, + sortUndefined: 'last' }, { - accessorFn: (row: InstrumentRepo) => { - return row.lastSyncedAt ? new Date(row.lastSyncedAt).toLocaleDateString() : '-'; + accessorFn: (row: InstrumentRepo) => + row.lastSyncedAt ? new Date(row.lastSyncedAt).getTime() : undefined, + cell: (ctx) => { + const lastSyncedAt = ctx.getValue(); + return lastSyncedAt ? new Date(lastSyncedAt).toLocaleDateString() : '-'; }, - header: t({ - en: 'Last Synced', - es: 'Última sincronización', - fr: 'Dernière synchronisation' - }), + header: ({ column }) => ( + + ), id: 'lastSynced', - size: 112 + size: 130, + sortUndefined: 'last' } ]} - data={reposQuery.data} + data={repoRows} // libui shares the width evenly between unpinned columns and honours `size` only on pinned // ones, so every column but the name is pinned, leaving the name the room the table has left. initialState={{ columnPinning: { right: ['url', 'instrumentCount', 'failedCount', 'lastSynced'] } }} @@ -715,7 +745,7 @@ export const Route = createFileRoute('/_app/admin/instrument-repos/')({ context.queryClient.ensureQueryData(instrumentReposQueryOptions()), context.queryClient.ensureQueryData(groupsQueryOptions()), context.queryClient.ensureQueryData( - instrumentRepoSyncLogsQueryOptions({ params: { page: INITIAL_PAGE, sortOrder: INITIAL_SORT_ORDER } }) + instrumentRepoSyncLogsQueryOptions({ params: { page: INITIAL_PAGE, ...INITIAL_SORT } }) ), context.queryClient.ensureQueryData(latestInstrumentRepoSyncLogsQueryOptions()) ]); diff --git a/apps/web/src/routes/_app/admin/instruments.tsx b/apps/web/src/routes/_app/admin/instruments.tsx index 541a99881..3c42a56c3 100644 --- a/apps/web/src/routes/_app/admin/instruments.tsx +++ b/apps/web/src/routes/_app/admin/instruments.tsx @@ -13,6 +13,7 @@ import { InstrumentKindCell } from '@/components/InstrumentKindCell'; import { InstrumentPreviewDialog } from '@/components/InstrumentPreviewDialog'; import type { InstrumentPreviewItem, InstrumentSource } from '@/components/InstrumentPreviewDialog'; import { PageHeader } from '@/components/PageHeader'; +import { SortableHeader } from '@/components/SortableHeader'; import { groupsQueryOptions, useGroupsQuery } from '@/hooks/useGroupsQuery'; import { useInstrumentInfoQuery } from '@/hooks/useInstrumentInfoQuery'; import { useInstrumentKindLabels } from '@/hooks/useInstrumentKindLabels'; @@ -154,12 +155,16 @@ const FormInstrumentsView = ({ scalarInstruments }: InstrumentViewProps) => { { accessorKey: 'title', cell: (ctx) => ()} />, - header: t({ en: 'Name', es: 'Nombre', fr: 'Nom' }) + header: ({ column }) => ( + + ) }, { accessorKey: 'sourceLabel', cell: (ctx) => ()} />, - header: t({ en: 'Source', es: 'Origen', fr: 'Source' }) + header: ({ column }) => ( + + ) }, { accessorFn: (row: InstrumentRow) => { @@ -168,26 +173,37 @@ const FormInstrumentsView = ({ scalarInstruments }: InstrumentViewProps) => { : t({ en: 'None', es: 'Ninguno', fr: 'Aucun' }); }, cell: (ctx) => ()} />, - header: t({ en: 'Groups Using It', es: 'Grupos que lo usan', fr: 'Groupes qui l’utilisent' }), + header: ({ column }) => ( + + ), id: 'groups' }, { accessorFn: (row: InstrumentRow) => kindLabels[row.kind], cell: (ctx) => , - header: t({ en: 'Kind', es: 'Tipo', fr: 'Type' }), + header: ({ column }) => ( + + ), id: 'kind', size: 130 }, { accessorKey: 'edition', cell: (ctx) => ())} />, - header: t({ en: 'Edition', es: 'Edición', fr: 'Édition' }), + header: ({ column }) => ( + + ), size: 100 }, { accessorFn: (row: InstrumentRow) => (row.createdAt ? toBasicISOString(row.createdAt) : '-'), cell: (ctx) => ()} />, - header: t({ en: 'Added', es: 'Agregado', fr: 'Ajouté' }), + header: ({ column }) => ( + + ), id: 'createdAt', size: 130 } @@ -281,18 +297,24 @@ const SeriesInstrumentsView = ({ scalarInstruments }: InstrumentViewProps) => { { accessorKey: 'title', cell: (ctx) => ()} />, - header: t({ en: 'Name', es: 'Nombre', fr: 'Nom' }) + header: ({ column }) => ( + + ) }, { accessorFn: (row: SeriesRow) => row.groupName ?? allGroupsLabel, cell: (ctx) => ()} />, - header: t({ en: 'Group', es: 'Grupo', fr: 'Groupe' }), + header: ({ column }) => ( + + ), id: 'group' }, { accessorFn: (row: SeriesRow) => (row.createdAt ? toBasicISOString(row.createdAt) : '-'), cell: (ctx) => ()} />, - header: t({ en: 'Created', es: 'Creado', fr: 'Créé' }), + header: ({ column }) => ( + + ), id: 'createdAt', size: 130 }, @@ -309,7 +331,9 @@ const SeriesInstrumentsView = ({ scalarInstruments }: InstrumentViewProps) => { /> ); }, - header: t({ en: 'Status', es: 'Estado', fr: 'Statut' }), + header: ({ column }) => ( + + ), id: 'status', size: 200 } diff --git a/apps/web/src/routes/_app/admin/permission-sets/__tests__/index.test.tsx b/apps/web/src/routes/_app/admin/permission-sets/__tests__/index.test.tsx index ac409efb3..8f16bf171 100644 --- a/apps/web/src/routes/_app/admin/permission-sets/__tests__/index.test.tsx +++ b/apps/web/src/routes/_app/admin/permission-sets/__tests__/index.test.tsx @@ -59,6 +59,8 @@ const renderPage = (permissionSets: PermissionSetSummary[]) => { const rows = () => screen.getAllByTestId('data-table-row'); +const headerButton = (name: string) => within(screen.getByTestId('data-table-head')).getByText(name).closest('button')!; + const cellTexts = (row: HTMLElement) => Array.from(row.children, (cell) => cell.textContent); const runLoader = (queryClient: QueryClient) => { @@ -76,6 +78,24 @@ describe('Route', () => { afterEach(cleanup); + it('should sort by name and by holder count as each header is clicked', () => { + renderPage([ + permissionSetFixture({ holderCount: 2, id: 'b', name: 'B' }), + permissionSetFixture({ holderCount: 1, id: 'a', name: 'A' }) + ]); + const names = () => screen.getAllByTestId('permission-set-name').map((name) => name.textContent); + fireEvent.click(headerButton('Name')); + expect(names()).toEqual(['A', 'B']); + fireEvent.click(headerButton('Holders')); + expect(names()).toEqual(['B', 'A']); + }); + + it('should sort by rule count when its header is clicked', () => { + renderPage([permissionSetFixture()]); + fireEvent.click(headerButton('Rules')); + expect(headerButton('Rules').querySelector('.lucide-chevrons-up-down')).toBeNull(); + }); + it('should prefetch the permission sets in the loader, so the table renders without a waterfall', async () => { const queryClient = new QueryClient(); const ensureQueryData = vi.spyOn(queryClient, 'ensureQueryData').mockResolvedValue([]); diff --git a/apps/web/src/routes/_app/admin/permission-sets/index.tsx b/apps/web/src/routes/_app/admin/permission-sets/index.tsx index b1781556f..8e7553c6d 100644 --- a/apps/web/src/routes/_app/admin/permission-sets/index.tsx +++ b/apps/web/src/routes/_app/admin/permission-sets/index.tsx @@ -6,6 +6,7 @@ import type { PermissionSetSummary } from '@opendatacapture/schemas/permission-s import { createFileRoute, Link, useNavigate } from '@tanstack/react-router'; import { PageHeader } from '@/components/PageHeader'; +import { SortableHeader } from '@/components/SortableHeader'; import { TruncatedCell } from '@/components/TruncatedCell'; import { useConfirmPermissionSetDeletion } from '@/hooks/useConfirmPermissionSetDeletion'; import { permissionSetsQueryOptions, usePermissionSetsQuery } from '@/hooks/usePermissionSetsQuery'; @@ -53,17 +54,23 @@ const RouteComponent = () => { ); }, - header: t({ en: 'Name', es: 'Nombre', fr: 'Nom' }) + header: ({ column }) => ( + + ) }, { accessorFn: (permissionSet) => permissionSet.rules.length, - header: t({ en: 'Rules', es: 'Reglas', fr: 'Règles' }), + header: ({ column }) => ( + + ), id: 'ruleCount' }, { accessorKey: 'holderCount', cell: ({ row }) => {row.original.holderCount}, - header: t({ en: 'Holders', es: 'Titulares', fr: 'Titulaires' }) + header: ({ column }) => ( + + ) } ]} data={permissionSetsQuery.data} diff --git a/apps/web/src/routes/_app/admin/users/__tests__/index.test.tsx b/apps/web/src/routes/_app/admin/users/__tests__/index.test.tsx index b9e875051..70dd79f8b 100644 --- a/apps/web/src/routes/_app/admin/users/__tests__/index.test.tsx +++ b/apps/web/src/routes/_app/admin/users/__tests__/index.test.tsx @@ -62,7 +62,7 @@ const rows = () => screen.getAllByTestId('data-table-row'); const usernames = () => rows().map((row) => row.querySelector('[data-row-selected]')!.parentElement!.textContent); const header = (label: string) => { - return within(screen.getByTestId('data-table-head')).getByText(label, { selector: 'button' }); + return within(screen.getByTestId('data-table-head')).getByText(label).closest('button')!; }; const openRowActions = (rowIndex = 0) => { diff --git a/apps/web/src/routes/_app/admin/users/index.tsx b/apps/web/src/routes/_app/admin/users/index.tsx index df28fd62b..898c163d0 100644 --- a/apps/web/src/routes/_app/admin/users/index.tsx +++ b/apps/web/src/routes/_app/admin/users/index.tsx @@ -2,15 +2,13 @@ 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 { useTheme, 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 { ColorTagCell } from '@/components/ColorTagCell'; import { PageHeader } from '@/components/PageHeader'; +import { SortableHeader } from '@/components/SortableHeader'; import { useArchiveUserMutation } from '@/hooks/useArchiveUserMutation'; import { useUnarchiveUserMutation } from '@/hooks/useUnarchiveUserMutation'; import { usersQueryOptions, useUsersQuery } from '@/hooks/useUsersQuery'; @@ -20,23 +18,6 @@ import { getArchiveStatusColor } from '@/utils/tag-colors'; type ArchiveAction = { kind: 'archive'; user: User }; -const SORTED_ICONS = { asc: ChevronUpIcon, desc: ChevronDownIcon }; - -const SortableHeader = ({ column, label }: { column: TanstackTable.Column; label: string }) => { - const sorted = column.getIsSorted(); - const Icon = sorted ? SORTED_ICONS[sorted] : ChevronsUpDownIcon; - return ( - - ); -}; - const RouteComponent = () => { const { t } = useTranslation(); const [theme] = useTheme(); diff --git a/apps/web/src/routes/_app/datahub/subjects/$subjectId/__tests__/assignments.test.tsx b/apps/web/src/routes/_app/datahub/subjects/$subjectId/__tests__/assignments.test.tsx index b73d8b8bb..0febc5107 100644 --- a/apps/web/src/routes/_app/datahub/subjects/$subjectId/__tests__/assignments.test.tsx +++ b/apps/web/src/routes/_app/datahub/subjects/$subjectId/__tests__/assignments.test.tsx @@ -59,8 +59,12 @@ const renderAssignments = (assignments: Assignment[] | undefined) => { render(); }; +const cellTexts = () => { + return Array.from(screen.getByTestId('data-table-row').children).map((cell) => cell.textContent); +}; + const openAssignment = (index = 0) => { - fireEvent.click(screen.getAllByTestId('assignment-row')[index]!); + fireEvent.click(screen.getAllByTestId('data-table-row')[index]!); }; const runGuard = () => { @@ -116,24 +120,18 @@ describe('Route', () => { it('should render no rows while the assignments have not loaded', () => { renderAssignments(undefined); - expect(screen.queryAllByTestId('assignment-row')).toHaveLength(0); + expect(screen.queryAllByTestId('data-table-row')).toHaveLength(0); }); it('should show the instrument title and the assigned and expiry dates of each assignment', () => { renderAssignments([createAssignment()]); - const cells = within(screen.getByTestId('assignment-row')).getAllByRole('cell'); - expect(cells.map((cell) => cell.textContent)).toEqual([ - 'Happiness Questionnaire', - '2026-01-15', - '2026-07-01', - 'Outstanding' - ]); + expect(cellTexts()).toEqual(['Happiness Questionnaire', '2026-01-15', '2026-07-01', 'Outstanding']); }); it('should fall back to the instrument id when the instrument is not among the known instruments', () => { mocks.instrumentInfo = undefined; renderAssignments([createAssignment()]); - expect(within(screen.getByTestId('assignment-row')).getAllByRole('cell')[0]!.textContent).toBe('instrument-1'); + expect(cellTexts()[0]).toBe('instrument-1'); }); it.each<[AssignmentStatus, string]>([ @@ -143,14 +141,22 @@ describe('Route', () => { ['OUTSTANDING', 'Outstanding'] ])('should display the %s status as %s', (status, label) => { renderAssignments([createAssignment({ status })]); - expect(within(screen.getByTestId('assignment-row')).getAllByRole('cell')[3]!.textContent).toBe(label); + expect(cellTexts()[3]).toBe(label); }); it('should mark only the clicked row as selected', () => { renderAssignments([createAssignment(), createAssignment({ id: 'assignment-2' })]); openAssignment(1); - const rows = screen.getAllByTestId('assignment-row'); - expect(rows.map((row) => row.getAttribute('data-state'))).toEqual([null, 'selected']); + const selection = screen.getAllByTestId('assignment-row-selection'); + expect(selection.map((row) => row.getAttribute('data-row-selected'))).toEqual(['false', 'true']); + }); + + it.each(['Title', 'Assigned', 'Expires', 'Status'])('should sort by %s when its header is clicked', (name) => { + const header = () => within(screen.getByTestId('data-table-head')).getByText(name).closest('button')!; + renderAssignments([createAssignment(), createAssignment({ id: 'assignment-2' })]); + expect(header().querySelector('.lucide-chevrons-up-down')).toBeTruthy(); + fireEvent.click(header()); + expect(header().querySelector('.lucide-chevrons-up-down')).toBeNull(); }); }); diff --git a/apps/web/src/routes/_app/datahub/subjects/$subjectId/assignments.tsx b/apps/web/src/routes/_app/datahub/subjects/$subjectId/assignments.tsx index bbc26503c..de848f798 100644 --- a/apps/web/src/routes/_app/datahub/subjects/$subjectId/assignments.tsx +++ b/apps/web/src/routes/_app/datahub/subjects/$subjectId/assignments.tsx @@ -1,18 +1,18 @@ // Assignment creation has been moved to the dedicated remote assignment page (/session/remote-assignment). // This page now only displays existing assignments and allows viewing/canceling them. -import React, { useEffect, useState } from 'react'; +import React, { useState } from 'react'; import { toBasicISOString } from '@douglasneuroinformatics/libjs'; -import { Button, Heading, Input, Label, Sheet, Table } from '@douglasneuroinformatics/libui/components'; +import { Button, DataTable, Heading, Input, Label, Sheet } from '@douglasneuroinformatics/libui/components'; import { useTranslation } from '@douglasneuroinformatics/libui/hooks'; import { CopyButton } from '@opendatacapture/react-core'; import type { Assignment, AssignmentStatus } from '@opendatacapture/schemas/assignment'; -import type { UnilingualInstrumentInfo } from '@opendatacapture/schemas/instrument'; import { createFileRoute, redirect } from '@tanstack/react-router'; import { AssignmentEmailForm } from '@/components/AssignmentEmailForm'; import { QRCode } from '@/components/QRCode'; +import { SortableHeader } from '@/components/SortableHeader'; import { config } from '@/config'; import { useAssignmentsQuery } from '@/hooks/useAssignmentsQuery'; import { useInstrument } from '@/hooks/useInstrument'; @@ -89,15 +89,8 @@ const AssignmentsTable: React.FC<{ selectedId: null | string; }> = ({ assignments, onSelection, selectedId }) => { const { t } = useTranslation('datahub'); - const [instruments, setInstruments] = useState<{ [key: string]: UnilingualInstrumentInfo }>({}); - const instrumentInfoQuery = useInstrumentInfoQuery(); - - useEffect(() => { - setInstruments( - Object.fromEntries((instrumentInfoQuery.data ?? []).map((instrument) => [instrument.id, instrument])) - ); - }, [instrumentInfoQuery.data]); + const instruments = new Map((instrumentInfoQuery.data ?? []).map((instrument) => [instrument.id, instrument])); const formatStatus = (status: AssignmentStatus) => { switch (status) { @@ -112,35 +105,50 @@ const AssignmentsTable: React.FC<{ } }; + // Tanstack caches a column's value on the row object, so a value that depends on a separately + // loaded query has to be in the row, or the sort would keep what it read before the query settled. + const rows = assignments.map((assignment) => ({ + ...assignment, + instrumentTitle: instruments.get(assignment.instrumentId)?.details.title ?? assignment.instrumentId, + statusLabel: formatStatus(assignment.status) + })); + return ( -
-
- - - {t('assignments.title')} - {t('assignments.assignedAt')} - {t('assignments.expiresAt')} - {t('assignments.status')} - - - - {assignments.map((assignment) => ( - onSelection(assignment)} - > - {instruments[assignment.instrumentId]?.details.title ?? assignment.instrumentId} - {toBasicISOString(assignment.createdAt)} - {toBasicISOString(assignment.expiresAt)} - {formatStatus(assignment.status)} - - ))} - -
- + ( + + {ctx.getValue()} + + + ), + header: ({ column }) => + }, + { + accessorKey: 'createdAt', + cell: (ctx) => toBasicISOString(ctx.getValue()), + header: ({ column }) => + }, + { + accessorKey: 'expiresAt', + cell: (ctx) => toBasicISOString(ctx.getValue()), + header: ({ column }) => + }, + { + accessorKey: 'statusLabel', + header: ({ column }) => + } + ]} + data={rows} + onRowClick={onSelection} + /> ); }; diff --git a/apps/web/src/routes/_app/datahub/subjects/$subjectId/table/__tests__/index.test.tsx b/apps/web/src/routes/_app/datahub/subjects/$subjectId/table/__tests__/index.test.tsx index 6decdffd9..787bbfb9b 100644 --- a/apps/web/src/routes/_app/datahub/subjects/$subjectId/table/__tests__/index.test.tsx +++ b/apps/web/src/routes/_app/datahub/subjects/$subjectId/table/__tests__/index.test.tsx @@ -1,4 +1,4 @@ -import { cleanup, fireEvent, render, screen } from '@testing-library/react'; +import { cleanup, fireEvent, render, screen, within } from '@testing-library/react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { Route } from '@/routes/_app/datahub/subjects/$subjectId/table/index'; @@ -70,6 +70,14 @@ describe('Route', () => { expect(headers()).toEqual(['DATE_COLLECTED', 'COLLECTION_METHOD', 'SERIES', 'TOTAL_SCORE']); }); + it('should sort by a record field, since every column of the record table is sortable', () => { + renderTable([record({ __id__: 'high', totalScore: 9 }), record({ __id__: 'low', totalScore: 1 })]); + const button = within(screen.getByTestId('data-table-head')).getByText('TOTAL_SCORE').closest('button')!; + expect(button.querySelector('.lucide-chevrons-up-down')).toBeTruthy(); + fireEvent.click(button); + expect(button.querySelector('.lucide-chevrons-up-down')).toBeNull(); + }); + // The provenance columns describe where a record came from rather than what it measured, so they // are fixed rather than derived from the records — they stand even with nothing to describe. it('should show the date and provenance columns when there are no records', () => { diff --git a/apps/web/src/routes/_app/datahub/subjects/$subjectId/table/index.tsx b/apps/web/src/routes/_app/datahub/subjects/$subjectId/table/index.tsx index c68ccb2b2..559129a52 100644 --- a/apps/web/src/routes/_app/datahub/subjects/$subjectId/table/index.tsx +++ b/apps/web/src/routes/_app/datahub/subjects/$subjectId/table/index.tsx @@ -7,6 +7,7 @@ import { createFileRoute } from '@tanstack/react-router'; import { SelectEdition } from '@/components/SelectEdition'; import { SelectInstrument } from '@/components/SelectInstrument'; +import { SortableHeader } from '@/components/SortableHeader'; import { TimeDropdown } from '@/components/TimeDropdown'; import { TruncatedCell } from '@/components/TruncatedCell'; import { useInstrumentVisualization } from '@/hooks/useInstrumentVisualization'; @@ -36,7 +37,7 @@ const RouteComponent = () => { const value = formatRecordValue(ctx.getValue()); return ; }, - header: camelToSnakeCase(subItem).toUpperCase(), + header: ({ column }) => , id: subItem }); } @@ -81,7 +82,7 @@ const RouteComponent = () => { } return value; }, - header: 'DATE_COLLECTED' + header: ({ column }) => }, ...metadataColumns, ...columns diff --git a/apps/web/src/utils/__tests__/data-table.test.ts b/apps/web/src/utils/__tests__/data-table.test.ts new file mode 100644 index 000000000..bd101b60d --- /dev/null +++ b/apps/web/src/utils/__tests__/data-table.test.ts @@ -0,0 +1,16 @@ +import { describe, expect, it } from 'vitest'; + +import { readServerSort } from '../data-table'; + +const COLUMNS = ['name', 'date'] as const; + +describe('readServerSort', () => { + it('should read the column and direction a sort click named', () => { + expect(readServerSort([{ desc: true, id: 'date' }], COLUMNS)).toEqual({ sortBy: 'date', sortOrder: 'desc' }); + expect(readServerSort([{ desc: false, id: 'name' }], COLUMNS)).toEqual({ sortBy: 'name', sortOrder: 'asc' }); + }); + + it('should fail loudly on a column the endpoint cannot order by, rather than ignore the click', () => { + expect(() => readServerSort([{ desc: false, id: 'messages' }], COLUMNS)).toThrow('messages'); + }); +}); diff --git a/apps/web/src/utils/data-table.ts b/apps/web/src/utils/data-table.ts index fe0ef2832..bdb5d2382 100644 --- a/apps/web/src/utils/data-table.ts +++ b/apps/web/src/utils/data-table.ts @@ -4,3 +4,21 @@ * libui exposes no prop for this, so the class reaches its body through the `data-testid` it renders. */ export const MIN_TABLE_BODY_HEIGHT = '[&_[data-testid=data-table-body]]:min-h-[205px]'; + +/** + * The one column a server-mode table's sorting state names, narrowed to the columns the endpoint can + * order by. libui disables sort removal, so a change always names a column; any other state is a + * column declared sortable that the endpoint does not know. + */ +export function readServerSort( + state: { desc: boolean; id: string }[], + sortableColumns: readonly TSortBy[] +): { sortBy: TSortBy; sortOrder: 'asc' | 'desc' } { + for (const { desc, id } of state) { + const sortBy = sortableColumns.find((column) => column === id); + if (sortBy) { + return { sortBy, sortOrder: desc ? 'desc' : 'asc' }; + } + } + throw new Error(`Cannot sort server-mode table by: ${JSON.stringify(state)}`); +} diff --git a/testing/src/pages/_app/datahub/subjects/$subjectId/assignments.page.ts b/testing/src/pages/_app/datahub/subjects/$subjectId/assignments.page.ts index 6a66e84c4..cb8237e48 100644 --- a/testing/src/pages/_app/datahub/subjects/$subjectId/assignments.page.ts +++ b/testing/src/pages/_app/datahub/subjects/$subjectId/assignments.page.ts @@ -9,7 +9,7 @@ export class SubjectAssignmentsPage extends AppPage { constructor(page: Page) { super(page); - this.assignmentRows = page.getByTestId('assignment-row'); + this.assignmentRows = page.getByTestId('data-table-row'); this.assignmentLink = page.getByRole('link', { name: 'Link to Assignment' }); this.cancelButton = page.getByRole('button', { name: 'Cancel' }); } diff --git a/testing/src/specs/admin-instrument-repos.spec.ts b/testing/src/specs/admin-instrument-repos.spec.ts index 610aefe41..396b0dc08 100644 --- a/testing/src/specs/admin-instrument-repos.spec.ts +++ b/testing/src/specs/admin-instrument-repos.spec.ts @@ -193,6 +193,44 @@ test.describe('admin instrument repos activity log', () => { await expectInvisible(); }); + test('should offer a sort on every column of both tables, asking the server to order the activity log', async ({ + getPageModel, + page + }) => { + const reposPage = await getPageModel('/admin/instrument-repos'); + const repoHead = reposPage.dataTable.getByTestId('data-table-head'); + for (const name of ['Repository Name', 'URL', 'Instruments', 'Failed', 'Last Synced']) { + await expect(repoHead.getByRole('button', { name })).toBeVisible(); + } + + await reposPage.openActivityLog(); + const logHead = reposPage.syncLogTable.getByTestId('data-table-head'); + for (const name of ['Date', 'Repository', 'Action', 'Status', 'Imported', 'Failed']) { + await expect(logHead.getByRole('button', { name })).toBeVisible(); + } + const request = page.waitForRequest((req) => req.url().includes('sortBy=status')); + await logHead.getByRole('button', { name: 'Status' }).click(); + await request; + }); + + test('should leave room for every header beside its sort icon, so none is clipped', async ({ getPageModel }) => { + const reposPage = await getPageModel('/admin/instrument-repos'); + const clippedLabels = (table: typeof reposPage.dataTable) => { + return table + .getByTestId('data-table-head') + .locator('button span') + .evaluateAll((labels) => { + return labels.filter((label) => label.scrollWidth > label.clientWidth).map((label) => label.textContent); + }); + }; + + await expect(reposPage.dataTable.getByTestId('instrument-repo-url')).toBeVisible(); + expect(await clippedLabels(reposPage.dataTable)).toEqual([]); + await reposPage.openActivityLog(); + await expect(reposPage.syncLogTable.getByTestId('data-table-head')).toBeVisible(); + expect(await clippedLabels(reposPage.syncLogTable)).toEqual([]); + }); + test('should fit both tables to the page without a horizontal scroll bar', async ({ getPageModel }) => { const reposPage = await getPageModel('/admin/instrument-repos'); const overflows = (table: typeof reposPage.dataTable) => { diff --git a/testing/src/specs/gateway-assignment.spec.ts b/testing/src/specs/gateway-assignment.spec.ts index 3e5b39bfb..a41963f8d 100644 --- a/testing/src/specs/gateway-assignment.spec.ts +++ b/testing/src/specs/gateway-assignment.spec.ts @@ -140,7 +140,7 @@ test.describe('gateway remote assignment', () => { // the page is addressed by subject id, not the (now irrelevant) in-memory session. await expect(async () => { await page.reload(); - await expect(page.getByRole('table')).toContainText('Complete', { timeout: 5_000 }); + await expect(page.getByTestId('data-table-row').first()).toContainText('Complete', { timeout: 5_000 }); }).toPass({ timeout: 90_000 }); }); }); diff --git a/testing/src/specs/remote-assignment.spec.ts b/testing/src/specs/remote-assignment.spec.ts index d281652d9..73c48357c 100644 --- a/testing/src/specs/remote-assignment.spec.ts +++ b/testing/src/specs/remote-assignment.spec.ts @@ -213,9 +213,11 @@ test.describe('remote assignment', () => { await page.getByRole('link', { name: 'Assignments' }).click(); await page.waitForURL('**/datahub/subjects/**/assignments'); - await page.getByRole('row').filter({ hasText: 'Happiness Questionnaire' }).click(); + await page.getByTestId('data-table-row').filter({ hasText: 'Happiness Questionnaire' }).click(); await page.getByRole('button', { name: 'Cancel' }).click(); - await expect(page.getByRole('row').filter({ hasText: 'Happiness Questionnaire' })).toContainText('Canceled'); + await expect(page.getByTestId('data-table-row').filter({ hasText: 'Happiness Questionnaire' })).toContainText( + 'Canceled' + ); }); }); diff --git a/testing/src/specs/table-sorting.spec.ts b/testing/src/specs/table-sorting.spec.ts new file mode 100644 index 000000000..27ae0874f --- /dev/null +++ b/testing/src/specs/table-sorting.spec.ts @@ -0,0 +1,67 @@ +import { ApiClient } from '../support/api-client'; +import { ADMIN } from '../support/constants'; +import { expect, test } from '../support/fixtures'; + +test.describe('table sorting', () => { + test.use({ actingRole: 'ADMIN' }); + + test('should offer a working sort on every column of the audit log, ordered by the server', async ({ + getPageModel, + page + }) => { + const auditLogsPage = await getPageModel('/admin/audit/logs'); + const head = auditLogsPage.dataTable.getByTestId('data-table-head'); + + for (const [name, sortBy] of [ + ['User', 'user'], + ['Active Group', 'group'], + ['Action', 'action'], + ['Entity', 'entity'] + ] as const) { + const response = page.waitForResponse((res) => res.url().includes(`sortBy=${sortBy}`)); + await head.getByRole('button', { name }).click(); + expect((await response).status()).toBe(200); + await expect(auditLogsPage.dataTable.getByTestId('data-table-row').first()).toBeVisible(); + } + }); + + // Status is derived and failure count is an array length, so these reach Mongo as a composite + // ordering only a real database can prove accepted. + test('should accept every sync log ordering against the real database', async ({ apiRequestContext }) => { + const token = await ApiClient.login(apiRequestContext, { password: ADMIN.password, username: ADMIN.username }); + for (const sortBy of ['action', 'createdAt', 'failedCount', 'importedCount', 'repoName', 'status']) { + const response = await apiRequestContext.get('/api/v1/instrument-repos/sync/logs', { + headers: { Authorization: `Bearer ${token}` }, + params: { sortBy } + }); + expect(response.status()).toBe(200); + } + }); + + test('should reorder the users when a column header is clicked', async ({ authenticateAs, page }) => { + await authenticateAs('ADMIN'); + await page.goto('/admin/users'); + const head = page.getByTestId('data-table-head'); + const firstRow = page.getByTestId('data-table-row').first(); + await head.getByRole('button', { name: 'Username' }).click(); + const ascending = await firstRow.textContent(); + await head.getByRole('button', { name: 'Username' }).click(); + await expect(firstRow).not.toHaveText(ascending!); + }); + + test('should offer a sort on every column of the group table', async ({ authenticateAs, page }) => { + await authenticateAs('ADMIN'); + await page.goto('/admin/groups'); + const head = page.getByTestId('data-table-head'); + await expect(head.getByRole('button', { name: 'Group Name' })).toBeVisible(); + await expect(head.getByRole('button', { name: 'Group Type' })).toBeVisible(); + }); + + test('should offer a sort on every column of the instruments table', async ({ getPageModel, page }) => { + await getPageModel('/admin/instruments'); + const head = page.getByTestId('data-table-head'); + for (const name of ['Name', 'Source', 'Groups Using It', 'Kind', 'Edition', 'Added']) { + await expect(head.getByRole('button', { name })).toBeVisible(); + } + }); +}); From 5ad5a060a58d5cd684811fe8e036b933d664d65a Mon Sep 17 00:00:00 2001 From: thomasbeaudry Date: Mon, 9 Nov 2026 11:23:34 -0500 Subject: [PATCH 3/3] style(web): give two arrow functions the block bodies main's lint now requires Co-Authored-By: Claude Opus 5.5 --- .../__tests__/InstrumentRepoSyncLogTable.test.tsx | 5 +++-- apps/web/src/routes/_app/admin/instrument-repos/index.tsx | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/apps/web/src/components/InstrumentRepoSyncLogTable/__tests__/InstrumentRepoSyncLogTable.test.tsx b/apps/web/src/components/InstrumentRepoSyncLogTable/__tests__/InstrumentRepoSyncLogTable.test.tsx index 62511fa2a..09726b34f 100644 --- a/apps/web/src/components/InstrumentRepoSyncLogTable/__tests__/InstrumentRepoSyncLogTable.test.tsx +++ b/apps/web/src/components/InstrumentRepoSyncLogTable/__tests__/InstrumentRepoSyncLogTable.test.tsx @@ -167,8 +167,9 @@ describe('InstrumentRepoSyncLogTable', () => { expect(within(rows()[0]!).getByText('Newer')).toBeTruthy(); }); - const headerButton = (name: string) => - within(screen.getByTestId('data-table-head')).getByText(name).closest('button')!; + const headerButton = (name: string) => { + return within(screen.getByTestId('data-table-head')).getByText(name).closest('button')!; + }; it('should ask for the other order when the date header is clicked, rather than sort the page', () => { const onSortChange = vi.fn(); diff --git a/apps/web/src/routes/_app/admin/instrument-repos/index.tsx b/apps/web/src/routes/_app/admin/instrument-repos/index.tsx index 57c3cfece..7f68f7b9e 100644 --- a/apps/web/src/routes/_app/admin/instrument-repos/index.tsx +++ b/apps/web/src/routes/_app/admin/instrument-repos/index.tsx @@ -398,8 +398,9 @@ const RouteComponent = () => { sortUndefined: 'last' }, { - accessorFn: (row: InstrumentRepo) => - row.lastSyncedAt ? new Date(row.lastSyncedAt).getTime() : undefined, + accessorFn: (row: InstrumentRepo) => { + return row.lastSyncedAt ? new Date(row.lastSyncedAt).getTime() : undefined; + }, cell: (ctx) => { const lastSyncedAt = ctx.getValue(); return lastSyncedAt ? new Date(lastSyncedAt).toLocaleDateString() : '-';