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/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..09726b34f 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,54 @@ describe('InstrumentRepoSyncLogTable', () => { expect(within(rows()[0]!).getByText('Newer')).toBeTruthy(); }); + 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 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..7f68f7b9e 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,37 @@ 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() : '-'; + return row.lastSyncedAt ? new Date(row.lastSyncedAt).getTime() : undefined; }, - header: t({ - en: 'Last Synced', - es: 'Última sincronización', - fr: 'Dernière synchronisation' - }), + cell: (ctx) => { + const lastSyncedAt = ctx.getValue(); + return lastSyncedAt ? new Date(lastSyncedAt).toLocaleDateString() : '-'; + }, + 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 +746,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/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') }); 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 2f6e65d70..9ea498bbb 100644 --- a/testing/src/specs/gateway-assignment.spec.ts +++ b/testing/src/specs/gateway-assignment.spec.ts @@ -142,7 +142,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 f4f8af20f..2ae6c534c 100644 --- a/testing/src/specs/remote-assignment.spec.ts +++ b/testing/src/specs/remote-assignment.spec.ts @@ -243,9 +243,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(); + } + }); +});