diff --git a/apps/web/src/routes/_app/datahub/index.tsx b/apps/web/src/routes/_app/datahub/index.tsx index 15eae68ef..8fdc2b0b6 100644 --- a/apps/web/src/routes/_app/datahub/index.tsx +++ b/apps/web/src/routes/_app/datahub/index.tsx @@ -26,6 +26,7 @@ import { SearchSubmitGuard } from '@/components/SearchSubmitGuard'; import { subjectsQueryOptions, useSubjectsQuery } from '@/hooks/useSubjectsQuery'; import { useAppStore } from '@/store'; import { downloadExcel } from '@/utils/excel'; +import { getListedSubjectIds } from '@/utils/table'; type DateFilter = { allowNull: boolean; @@ -243,11 +244,9 @@ const Toggles: React.FC<{ getExportRecords() .then((data): any => { - const listedSubjects = table - .getPrePaginationRowModel() - .rows.flatMap((row) => row.getVisibleCells().map((cell) => removeSubjectIdScope(cell.row.original.id))); + const listedSubjects = getListedSubjectIds(table); - const filteredData = data.filter((dataEntry) => listedSubjects.includes(dataEntry.subjectId)); + const filteredData = data.filter((dataEntry) => listedSubjects.has(dataEntry.subjectId)); if (filteredData.length < 1) { throw Error( diff --git a/apps/web/src/utils/__tests__/table.test.tsx b/apps/web/src/utils/__tests__/table.test.tsx new file mode 100644 index 000000000..ccfee4b71 --- /dev/null +++ b/apps/web/src/utils/__tests__/table.test.tsx @@ -0,0 +1,80 @@ +import React from 'react'; + +import { DataTable } from '@douglasneuroinformatics/libui/components'; +import type { TanstackTable } from '@douglasneuroinformatics/libui/components'; +import type { Subject } from '@opendatacapture/schemas/subject'; +import { render } from '@testing-library/react'; +import { describe, expect, it } from 'vitest'; + +import { getListedSubjectIds } from '@/utils/table'; + +// Initialises the shared libui translator, which the table's controls read on render. +import '@/services/i18n'; + +const noop = () => undefined; + +/** A complete `Subject`, so the fixture satisfies the type rather than being cast into it. */ +const subject = (id: string): Subject => ({ + createdAt: new Date(0), + dateOfBirth: null, + firstName: null, + groupIds: [], + id, + lastName: null, + sessionIds: [], + sex: null, + updatedAt: new Date(0) +}); + +/** + * Renders a table shaped like the datahub master table (several columns plus row actions) and hands + * back its tanstack instance, so the id extraction is exercised against a real row model rather than + * a hand-built stand-in. + */ +const renderMasterTableLike = (ids: string[]) => { + let table: TanstackTable.Table | undefined; + const Capture = (props: { table: TanstackTable.Table }) => { + table = props.table; + return null; + }; + render( + + columns={[ + { accessorFn: (subject) => subject.id, header: 'Subject', id: 'subjectId' }, + { accessorFn: () => null, header: 'DOB', id: 'dateOfBirth' }, + { accessorFn: () => null, header: 'Sex', id: 'sex' } + ]} + data={ids.map(subject)} + rowActions={[{ label: 'View', onSelect: noop }]} + togglesComponent={Capture} + /> + ); + if (!table) { + throw new Error('DataTable did not invoke togglesComponent, so no tanstack table was captured'); + } + return table; +}; + +describe('getListedSubjectIds', () => { + it('should yield one id per row, not the one-per-rendered-cell duplication the row model offers', () => { + const table = renderMasterTableLike(['subject-a', 'subject-b', 'subject-c']); + const rows = table.getPrePaginationRowModel().rows; + + // Asserts the duplication the previous implementation hit is real rather than assumed: + // `getVisibleCells()` repeats the row's id once per column, including the row-actions column. + const perCell = rows.flatMap((row) => row.getVisibleCells().map((cell) => cell.row.original.id)); + expect(perCell.length).toBeGreaterThan(rows.length); + + expect([...getListedSubjectIds(table)]).toStrictEqual(['subject-a', 'subject-b', 'subject-c']); + }); + + it('should strip the group scope, so the ids match the unscoped subject ids an export carries', () => { + const table = renderMasterTableLike(['Group_A$subject-a', 'Group_A$subject-b']); + + expect([...getListedSubjectIds(table)]).toStrictEqual(['subject-a', 'subject-b']); + }); + + it('should return an empty set when the table lists no subjects, so nothing is exported', () => { + expect(getListedSubjectIds(renderMasterTableLike([])).size).toBe(0); + }); +}); diff --git a/apps/web/src/utils/table.ts b/apps/web/src/utils/table.ts new file mode 100644 index 000000000..2065fdcbd --- /dev/null +++ b/apps/web/src/utils/table.ts @@ -0,0 +1,15 @@ +import type { TanstackTable } from '@douglasneuroinformatics/libui/components'; +import type { Subject } from '@opendatacapture/schemas/subject'; +import { removeSubjectIdScope } from '@opendatacapture/subject-utils'; + +/** + * The subject ids currently listed by the table, for filtering an export down to them. + * + * Read one per row: iterating `getVisibleCells()` yields the row's id once per rendered column + * (including the row-actions column), so the ids arrive duplicated as many times as there are + * columns. A set also keeps the membership test that consumes this constant time rather than a + * linear scan per exported row. + */ +export function getListedSubjectIds(table: TanstackTable.Table): Set { + return new Set(table.getPrePaginationRowModel().rows.map((row) => removeSubjectIdScope(row.original.id))); +} diff --git a/testing/AGENTS.md b/testing/AGENTS.md index 333fe5d8f..81ef1e05a 100644 --- a/testing/AGENTS.md +++ b/testing/AGENTS.md @@ -64,20 +64,25 @@ A page object is only reachable from a spec once it is registered in the `pageMo ## Fixtures -| Fixture | Scope | Notes | -| --------------------- | ----------- | -------------------------------------------------------------------------------------------------- | -| `getPageModel` | test | Authenticates as `actingRole`, navigates, returns the page object | -| `authenticateAs(who)` | test | Injects a token without navigating; takes a role or `$LoginCredentials` | -| `actingRole` | test option | Default `GROUP_MANAGER`; override with `test.use({ actingRole: 'ADMIN' })` | -| `appState` | test option | localStorage first-run gating, seeded for every test; defaults to accepted | -| `uniqueId` | test | Short random suffix for seeded data | -| `api` | worker | `ApiClient` as admin — `createGroup()` / `createUser()` / `setUserPermissions()` for preconditions | -| `roleAccount(role)` | worker | Seeds a group + user per role once, then caches its token and username | -| `apiRequestContext` | worker | Raw `APIRequestContext` on the web origin, for driving the API directly | +| Fixture | Scope | Notes | +| ---------------------- | ----------- | -------------------------------------------------------------------------------------------------- | +| `getPageModel` | test | Authenticates as `actingRole`, navigates, returns the page object | +| `authenticateAs(who)` | test | Injects a token without navigating; takes a role or `$LoginCredentials` | +| `actingRole` | test option | Default `GROUP_MANAGER`; override with `test.use({ actingRole: 'ADMIN' })` | +| `appState` | test option | localStorage first-run gating, seeded for every test; defaults to accepted | +| `uniqueId` | test | Short random suffix for seeded data | +| `api` | worker | `ApiClient` as admin — `createGroup()` / `createUser()` / `setUserPermissions()` for preconditions | +| `isolatedGroupManager` | test | Authenticates into a group created for this test alone; returns the group | +| `roleAccount(role)` | worker | Seeds a group + user per role once, then caches its token and username | +| `apiRequestContext` | worker | Raw `APIRequestContext` on the web origin, for driving the API directly | Set up preconditions over the API with the `api` fixture rather than by clicking through the UI; only drive the UI for the behaviour actually under test. +`roleAccount`'s group is cached per worker and shared by every spec running in it, so a test that +asserts on **how much** a group contains must use `isolatedGroupManager` instead. A group manager +reads only their own groups, so a fresh group bounds what the test can see to what it seeded. + Auth is injected as `window.__PLAYWRIGHT_ACCESS_TOKEN__`, which `apps/web`'s `src/store/slices/auth.slice.ts` reads on boot. It is memory-only and never persisted. diff --git a/testing/src/pages/_app/datahub/index.page.ts b/testing/src/pages/_app/datahub/index.page.ts index 37f0f580f..1a87a5de4 100644 --- a/testing/src/pages/_app/datahub/index.page.ts +++ b/testing/src/pages/_app/datahub/index.page.ts @@ -1,15 +1,31 @@ -import type { Locator, Page } from '@playwright/test'; +import type { Download, Locator, Page } from '@playwright/test'; import { AppPage } from '../route.page'; export class DatahubPage extends AppPage { + readonly exportDropdown: Locator; readonly pageHeader: Locator; readonly rowActionsTrigger: Locator; + readonly rows: Locator; readonly searchInput: Locator; constructor(page: Page) { super(page); + this.exportDropdown = page.getByTestId('datahub-export-dropdown'); this.pageHeader = page.getByTestId('page-header'); this.rowActionsTrigger = page.getByTestId('row-actions-trigger').first(); + this.rows = page.getByTestId('data-table-body').getByTestId('data-table-row'); this.searchInput = page.getByTestId('data-table-search-bar').getByRole('searchbox'); } + + /** + * Picks a format from the export menu and returns the file it produced. Matched by extension, + * because a CSV export downloads a `README.txt` before the `.csv` itself. + */ + async exportAs(format: 'CSV' | 'Excel' | 'JSON'): Promise { + const extension = { CSV: '.csv', Excel: '.xlsx', JSON: '.json' }[format]; + const started = this.$ref.waitForEvent('download', (download) => download.suggestedFilename().endsWith(extension)); + await this.exportDropdown.click(); + await this.$ref.getByRole('menuitem', { exact: true, name: format }).click(); + return started; + } } diff --git a/testing/src/specs/datahub.spec.ts b/testing/src/specs/datahub.spec.ts index b9adcd9f9..f39a3de68 100644 --- a/testing/src/specs/datahub.spec.ts +++ b/testing/src/specs/datahub.spec.ts @@ -1,5 +1,6 @@ import { readFile } from 'node:fs/promises'; +import { DatahubPage } from '../pages/_app/datahub/index.page'; import { HAPPINESS_RECORD } from '../support/constants'; import { expect, test } from '../support/fixtures'; @@ -146,4 +147,37 @@ test.describe('data hub', () => { await expect(page).toHaveURL(/\/datahub\/.+\/table$/); }); + + // The export endpoint returns every record in the group; which of them reach the file is decided + // client-side from the rows the table is currently listing. Nothing else covers that scoping, and + // getting it wrong hands the user another subject's data. + test('should export only the subjects the table is listing', async ({ + api, + isolatedGroupManager, + page, + uniqueId + }) => { + const group = await isolatedGroupManager(); + const instrumentId = await api.findInstrumentIdByName('DNP_HAPPINESS_QUESTIONNAIRE'); + const listed = `export-${uniqueId}-listed`; + const filteredOut = `export-${uniqueId}-filtered-out`; + await api.uploadRecords( + group.id, + instrumentId, + [listed, filteredOut].map((subjectId) => ({ data: HAPPINESS_RECORD, date: new Date(), subjectId })) + ); + + const datahubPage = new DatahubPage(page); + await datahubPage.goto('/datahub'); + await expect(datahubPage.rows).toHaveCount(2); + + await datahubPage.searchInput.fill(listed); + await expect(datahubPage.rows).toHaveCount(1); + + const download = await datahubPage.exportAs('JSON'); + const payload = JSON.parse(await readFile(await download.path(), 'utf8')) as { subjectId: string }[]; + + expect(payload.length).toBeGreaterThan(0); + expect([...new Set(payload.map((row) => row.subjectId))]).toStrictEqual([listed]); + }); }); diff --git a/testing/src/support/fixtures.ts b/testing/src/support/fixtures.ts index 9ad407a8b..dd6757fd3 100644 --- a/testing/src/support/fixtures.ts +++ b/testing/src/support/fixtures.ts @@ -1,6 +1,7 @@ /* eslint-disable no-empty-pattern */ import type { $LoginCredentials } from '@opendatacapture/schemas/auth'; +import type { Group } from '@opendatacapture/schemas/group'; import { request as apiRequestFactory, test as base, expect } from '@playwright/test'; import type { APIRequestContext } from '@playwright/test'; @@ -98,6 +99,15 @@ type TestFixtures = { authenticateAs: (roleOrCredentials: $LoginCredentials | Role) => Promise; /** Navigates to a route as `actingRole` and returns its page object. */ getPageModel: GetPageModel; + /** + * Authenticates as a group manager of a group created for this test alone, and returns that group. + * + * The `roleAccount` group is cached per worker and shared by every spec running in it, so a test + * that asserts on exactly what a group contains cannot use it. A group manager's `read Subject` + * rule is scoped to their own groups, so a fresh group bounds what this test can see to what it + * seeded. + */ + isolatedGroupManager: () => Promise; /** * Writes `appState` to localStorage on every navigation, for every test, whether or not it * authenticates through `authenticateAs`. Without it a spec that logs in through the real form @@ -155,6 +165,14 @@ export const test = base.extend({ } ); }, + isolatedGroupManager: async ({ api, authenticateAs }, use) => { + await use(async () => { + const group = await api.createGroup(); + const { credentials } = await api.createUser({ basePermissionLevel: 'GROUP_MANAGER', groupIds: [group.id] }); + await authenticateAs(credentials); + return group; + }); + }, roleAccount: [ async ({ adminToken, api, apiRequestContext }, use) => { const cache = new Map([