From 8fc726db171f172bb1c2727647adc31854c7b636 Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Mon, 20 Jul 2026 15:18:10 -0400 Subject: [PATCH 1/5] perf(api): group the has-record subject ids in the database The "with records only" datahub filter resolved its subject list with findMany({ distinct: ['subjectId'], select: { subjectId: true }, where }) but prisma applies `distinct` in the query engine rather than pushing it into mongodb, so the driver receives one row per *record* and collapses them to one row per subject only after they have arrived. On a group holding 100k records that is 100k rows transferred to produce a list of at most a few thousand ids. `groupBy` asks mongodb for the distinct values directly. Measured over HTTP against a live instance with 200,300 records, median of 7: GET /v1/subjects?hasRecord=true 366ms -> 42ms GET /v1/subjects?groupId=&hasRecord=true 67ms -> 37ms The unscoped case is where the difference shows, and it widens with the size of the collection: `groupBy` returns one row per subject regardless of how many records back it. The record query also now carries the caller's ability, which it previously ignored. The outer subject query already constrained the result, so this was not a data leak, but the inner query read records the caller had no permission to read. Note for anyone revisiting this: the issue also floated expressing the filter as `instrumentRecords: { some: ... }` on Subject. Measured on the same dataset that takes 26,849ms -- prisma compiles a relation filter on mongodb into a $lookup over the whole record collection. It is 270x slower than the code being replaced here, and carries the 100 MiB per-document ceiling that made the same construct fail outright in #1416. Refs #1413 Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01Bfen9VQemZanJLXinJ6MMG --- .../__tests__/subjects.service.spec.ts | 44 +++++++++++++------ apps/api/src/subjects/subjects.service.ts | 28 +++++++++--- 2 files changed, 52 insertions(+), 20 deletions(-) diff --git a/apps/api/src/subjects/__tests__/subjects.service.spec.ts b/apps/api/src/subjects/__tests__/subjects.service.spec.ts index ea12d89d3..c5d50c1ba 100644 --- a/apps/api/src/subjects/__tests__/subjects.service.spec.ts +++ b/apps/api/src/subjects/__tests__/subjects.service.spec.ts @@ -7,6 +7,7 @@ import { Test } from '@nestjs/testing'; import { pick } from 'lodash-es'; import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { createAppAbility } from '@/auth/ability.utils'; import type { RuntimePrismaClient } from '@/core/prisma'; import { SubjectsService } from '../subjects.service'; @@ -30,7 +31,8 @@ describe('SubjectsService', () => { $transaction: vi.fn(), instrumentRecord: { deleteMany: vi.fn(), - findMany: vi.fn() + findMany: vi.fn(), + groupBy: vi.fn() }, session: { deleteMany: vi.fn() @@ -80,16 +82,9 @@ describe('SubjectsService', () => { await expect(subjectsService.find()).resolves.toMatchObject([{ id: '123' }]); }); it('should return the array of subjects with records', async () => { - prismaClient.instrumentRecord.findMany.mockResolvedValueOnce([{ subjectId: '123' }]); + prismaClient.instrumentRecord.groupBy.mockResolvedValueOnce([{ subjectId: '123' }]); subjectModel.findMany.mockResolvedValueOnce([{ id: '123' }]); await expect(subjectsService.find({ hasRecord: true })).resolves.toMatchObject([{ id: '123' }]); - expect(prismaClient.instrumentRecord.findMany).toHaveBeenCalledWith( - expect.objectContaining({ - distinct: ['subjectId'], - select: { subjectId: true }, - where: {} - }) - ); expect(subjectModel.findMany).toHaveBeenCalledWith( expect.objectContaining({ where: expect.objectContaining({ @@ -98,18 +93,41 @@ describe('SubjectsService', () => { }) ); }); + // `distinct` is applied by the prisma query engine, so it returns one row per record over the + // wire; grouping asks mongodb for one row per subject instead. + it('should group the ids in the database rather than deduplicating them after the fact', async () => { + prismaClient.instrumentRecord.groupBy.mockResolvedValueOnce([{ subjectId: '123' }]); + subjectModel.findMany.mockResolvedValueOnce([{ id: '123' }]); + await subjectsService.find({ hasRecord: true }); + expect(prismaClient.instrumentRecord.groupBy).toHaveBeenCalledWith( + expect.objectContaining({ by: ['subjectId'] }) + ); + expect(prismaClient.instrumentRecord.findMany).not.toHaveBeenCalled(); + }); it('should filter instrument records by groupId when provided', async () => { - prismaClient.instrumentRecord.findMany.mockResolvedValueOnce([{ subjectId: '123' }]); + prismaClient.instrumentRecord.groupBy.mockResolvedValueOnce([{ subjectId: '123' }]); subjectModel.findMany.mockResolvedValueOnce([{ id: '123' }]); await subjectsService.find({ groupId: 'group-1', hasRecord: true }); - expect(prismaClient.instrumentRecord.findMany).toHaveBeenCalledWith( + expect(prismaClient.instrumentRecord.groupBy).toHaveBeenCalledWith( expect.objectContaining({ - where: { groupId: 'group-1' } + where: { AND: [{}, { groupId: 'group-1' }] } }) ); }); + it('should constrain the record query to what the caller may read', async () => { + prismaClient.instrumentRecord.groupBy.mockResolvedValueOnce([]); + subjectModel.findMany.mockResolvedValueOnce([]); + const ability = createAppAbility([ + { action: 'read', subject: 'InstrumentRecord' }, + { action: 'read', subject: 'Subject' } + ]); + await subjectsService.find({ hasRecord: true }, { ability }); + const [call] = prismaClient.instrumentRecord.groupBy.mock.lastCall as [{ where: { AND: unknown[] } }]; + // the ability contributes the first clause; without it the query would be unconstrained + expect(call.where.AND[0]).not.toStrictEqual({}); + }); it('should pass all subject IDs returned by instrument records to the subject query', async () => { - prismaClient.instrumentRecord.findMany.mockResolvedValueOnce([{ subjectId: '123' }, { subjectId: '456' }]); + prismaClient.instrumentRecord.groupBy.mockResolvedValueOnce([{ subjectId: '123' }, { subjectId: '456' }]); subjectModel.findMany.mockResolvedValueOnce([{ id: '123' }, { id: '456' }]); await subjectsService.find({ hasRecord: true }); expect(subjectModel.findMany).toHaveBeenCalledWith( diff --git a/apps/api/src/subjects/subjects.service.ts b/apps/api/src/subjects/subjects.service.ts index bde50db32..7e6fc8dd8 100644 --- a/apps/api/src/subjects/subjects.service.ts +++ b/apps/api/src/subjects/subjects.service.ts @@ -4,6 +4,7 @@ import { ConflictException, Injectable, NotFoundException } from '@nestjs/common import type { Prisma } from '@prisma/client'; import { accessibleQuery } from '@/auth/ability.utils'; +import type { AppAbility } from '@/auth/auth.types'; import type { RuntimePrismaClient } from '@/core/prisma'; import type { EntityOperationOptions } from '@/core/types'; @@ -136,7 +137,7 @@ export class SubjectsService { AND: [ accessibleQuery(ability, 'read', 'Subject'), groupInput, - { id: { in: await this.querySubjectIdsWithRecords(groupId) } } + { id: { in: await this.querySubjectIdsWithRecords(groupId, ability) } } ] } }); @@ -158,12 +159,25 @@ export class SubjectsService { return subject; } - private async querySubjectIdsWithRecords(groupId?: string): Promise { - const records = await this.prismaClient.instrumentRecord.findMany({ - distinct: ['subjectId'], - select: { subjectId: true }, - where: groupId ? { groupId } : {} + /** + * The ids of subjects having at least one record, for the "with records only" filter. + * + * Grouped by the database rather than deduplicated after the fact: `distinct` is applied by the + * prisma query engine, so it returns one row per *record* over the wire and collapses them only + * once they have arrived. + * + * Deliberately not expressed as a `instrumentRecords: { some: ... }` relation filter on Subject. + * On mongodb prisma compiles that into a $lookup over the whole record collection, which measured + * far slower than either form here and carries the 100 MiB per-document ceiling that made the same + * construct fail outright elsewhere. + */ + private async querySubjectIdsWithRecords(groupId?: string, ability?: AppAbility): Promise { + const groups = await this.prismaClient.instrumentRecord.groupBy({ + by: ['subjectId'], + where: { + AND: [accessibleQuery(ability, 'read', 'InstrumentRecord'), groupId ? { groupId } : {}] + } }); - return records.map((r) => r.subjectId); + return groups.map((group) => group.subjectId); } } From 42d78b8bb1a2eb0731d0ebae35e32c4dbabd1e7a Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Mon, 27 Jul 2026 14:59:03 -0400 Subject: [PATCH 2/5] test(api): pin the has-record ability clause to accessibleQuery `not.toStrictEqual({})` did not prove the ability was applied: an unconditional read rule legitimately produces `{}`, so the assertion would have passed on a query that ignored the caller entirely. Constrain the ability with conditions and compare the clause against accessibleQuery directly. Verified by mutation: passing `undefined` in place of the ability now fails. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014ZEJLwa7jwD8sKzcE4PfSc --- .../src/subjects/__tests__/subjects.service.spec.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/apps/api/src/subjects/__tests__/subjects.service.spec.ts b/apps/api/src/subjects/__tests__/subjects.service.spec.ts index c5d50c1ba..c5a576124 100644 --- a/apps/api/src/subjects/__tests__/subjects.service.spec.ts +++ b/apps/api/src/subjects/__tests__/subjects.service.spec.ts @@ -7,7 +7,7 @@ import { Test } from '@nestjs/testing'; import { pick } from 'lodash-es'; import { beforeEach, describe, expect, it, vi } from 'vitest'; -import { createAppAbility } from '@/auth/ability.utils'; +import { accessibleQuery, createAppAbility } from '@/auth/ability.utils'; import type { RuntimePrismaClient } from '@/core/prisma'; import { SubjectsService } from '../subjects.service'; @@ -114,17 +114,18 @@ describe('SubjectsService', () => { }) ); }); - it('should constrain the record query to what the caller may read', async () => { + it('should constrain the record query to what the caller may read, so the filter cannot be resolved from other groups records', async () => { prismaClient.instrumentRecord.groupBy.mockResolvedValueOnce([]); subjectModel.findMany.mockResolvedValueOnce([]); + // Conditions are what make this meaningful: an unconditional read rule yields `{}`, which is + // indistinguishable from the ability never having been applied. const ability = createAppAbility([ - { action: 'read', subject: 'InstrumentRecord' }, - { action: 'read', subject: 'Subject' } + { action: 'read', conditions: { groupId: { in: ['group-1'] } }, subject: 'InstrumentRecord' }, + { action: 'read', conditions: { groupIds: { hasSome: ['group-1'] } }, subject: 'Subject' } ]); await subjectsService.find({ hasRecord: true }, { ability }); const [call] = prismaClient.instrumentRecord.groupBy.mock.lastCall as [{ where: { AND: unknown[] } }]; - // the ability contributes the first clause; without it the query would be unconstrained - expect(call.where.AND[0]).not.toStrictEqual({}); + expect(call.where.AND[0]).toStrictEqual(accessibleQuery(ability, 'read', 'InstrumentRecord')); }); it('should pass all subject IDs returned by instrument records to the subject query', async () => { prismaClient.instrumentRecord.groupBy.mockResolvedValueOnce([{ subjectId: '123' }, { subjectId: '456' }]); From f852d33634b1f536c3b745fc69c6b3b06d96e071 Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Tue, 28 Jul 2026 13:25:28 -0400 Subject: [PATCH 3/5] fix(api): answer the has-record filter for a caller who may read no records Applying the caller's ability to the record query introduced a new failure on this endpoint. `accessibleQuery` does not return an empty filter when the ability holds no rule for the subject at all -- only an `undefined` ability does that -- it throws a CASL ForbiddenError, which is not an HttpException, so libnest's filter renders it a 500. A STANDARD user holds `create` but not `read` on InstrumentRecord, and `GET /v1/subjects` is gated on `read Subject`, which they do hold. So `?hasRecord=true` returned an Internal Server Error where main returns 200. The web UI hides the datahub from those users, but the endpoint is documented and its guard admits them. Return an empty list for such a caller: the honest reading of "subjects whose records you can see". The scoping itself stays. Also from review: - a unit test for the no-rule caller, built through `AbilityFactory` with a STANDARD payload rather than by hand, since a hand-built ability tends to carry the very rule whose absence breaks this - e2e coverage in `datahub.spec.ts`, which had only asserted the page header: one case toggling "With records only" and asserting the table empties, one asserting the endpoint answers a standard user. Both verified by mutation - dropped `expect(findMany).not.toHaveBeenCalled()`, which pinned an implementation detail the `groupBy` assertion above it already covers The narrowing test needs to know exactly what its group holds, and the `roleAccount` group is shared by every spec in a worker, so this adds an `isolatedGroupManager` fixture that authenticates into a group of its own. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014ZEJLwa7jwD8sKzcE4PfSc --- .../__tests__/subjects.service.spec.ts | 23 ++++++++++- apps/api/src/subjects/subjects.service.ts | 7 ++++ apps/web/src/routes/_app/datahub/index.tsx | 7 +++- testing/AGENTS.md | 5 +++ testing/src/pages/_app/datahub/index.page.ts | 12 ++++++ testing/src/specs/datahub.spec.ts | 39 +++++++++++++++++++ testing/src/support/api-client.ts | 15 +++++++ testing/src/support/fixtures.ts | 24 ++++++++++++ 8 files changed, 129 insertions(+), 3 deletions(-) diff --git a/apps/api/src/subjects/__tests__/subjects.service.spec.ts b/apps/api/src/subjects/__tests__/subjects.service.spec.ts index c5a576124..ca55e576a 100644 --- a/apps/api/src/subjects/__tests__/subjects.service.spec.ts +++ b/apps/api/src/subjects/__tests__/subjects.service.spec.ts @@ -1,4 +1,4 @@ -import { CryptoService, getModelToken, PRISMA_CLIENT_TOKEN } from '@douglasneuroinformatics/libnest'; +import { CryptoService, getModelToken, LoggingService, PRISMA_CLIENT_TOKEN } from '@douglasneuroinformatics/libnest'; import type { Model } from '@douglasneuroinformatics/libnest'; import { MockFactory } from '@douglasneuroinformatics/libnest/testing'; import type { MockedInstance } from '@douglasneuroinformatics/libnest/testing'; @@ -7,6 +7,7 @@ import { Test } from '@nestjs/testing'; import { pick } from 'lodash-es'; import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { AbilityFactory } from '@/auth/ability.factory'; import { accessibleQuery, createAppAbility } from '@/auth/ability.utils'; import type { RuntimePrismaClient } from '@/core/prisma'; @@ -102,7 +103,6 @@ describe('SubjectsService', () => { expect(prismaClient.instrumentRecord.groupBy).toHaveBeenCalledWith( expect.objectContaining({ by: ['subjectId'] }) ); - expect(prismaClient.instrumentRecord.findMany).not.toHaveBeenCalled(); }); it('should filter instrument records by groupId when provided', async () => { prismaClient.instrumentRecord.groupBy.mockResolvedValueOnce([{ subjectId: '123' }]); @@ -127,6 +127,25 @@ describe('SubjectsService', () => { const [call] = prismaClient.instrumentRecord.groupBy.mock.lastCall as [{ where: { AND: unknown[] } }]; expect(call.where.AND[0]).toStrictEqual(accessibleQuery(ability, 'read', 'InstrumentRecord')); }); + + // A STANDARD user holds `create` but not `read` on InstrumentRecord, and this route's guard names + // `read Subject`, so they reach the service. `accessibleQuery` throws on an ability with no rule + // for the subject at all, which escapes as a 500 rather than the empty list they should see. + it('should return an empty list for a caller who may read no records, rather than throwing', async () => { + const abilityFactory = new AbilityFactory(MockFactory.createMock(LoggingService) as unknown as LoggingService); + const ability = abilityFactory.createForPayload({ + basePermissionLevel: 'STANDARD', + groups: [{ id: 'group-1' }], + id: 'user-1' + } as any); + subjectModel.findMany.mockResolvedValueOnce([]); + + await expect(subjectsService.find({ hasRecord: true }, { ability })).resolves.toStrictEqual([]); + + expect(prismaClient.instrumentRecord.groupBy).not.toHaveBeenCalled(); + const [call] = subjectModel.findMany.mock.lastCall as [{ where: { AND: unknown[] } }]; + expect(call.where.AND).toContainEqual({ id: { in: [] } }); + }); it('should pass all subject IDs returned by instrument records to the subject query', async () => { prismaClient.instrumentRecord.groupBy.mockResolvedValueOnce([{ subjectId: '123' }, { subjectId: '456' }]); subjectModel.findMany.mockResolvedValueOnce([{ id: '123' }, { id: '456' }]); diff --git a/apps/api/src/subjects/subjects.service.ts b/apps/api/src/subjects/subjects.service.ts index 7e6fc8dd8..87acd64c9 100644 --- a/apps/api/src/subjects/subjects.service.ts +++ b/apps/api/src/subjects/subjects.service.ts @@ -172,6 +172,13 @@ export class SubjectsService { * construct fail outright elsewhere. */ private async querySubjectIdsWithRecords(groupId?: string, ability?: AppAbility): Promise { + // `accessibleQuery` throws rather than returning a restrictive filter when the ability holds no + // rule for the subject at all, and a STANDARD user holds `create` but not `read` on + // InstrumentRecord. This route's guard names `read Subject`, so such a caller reaches here; no + // readable records means no subjects qualify. + if (ability && !ability.can('read', 'InstrumentRecord')) { + return []; + } const groups = await this.prismaClient.instrumentRecord.groupBy({ by: ['subjectId'], where: { diff --git a/apps/web/src/routes/_app/datahub/index.tsx b/apps/web/src/routes/_app/datahub/index.tsx index 22317a4a1..cf6b56515 100644 --- a/apps/web/src/routes/_app/datahub/index.tsx +++ b/apps/web/src/routes/_app/datahub/index.tsx @@ -58,7 +58,11 @@ const Filters: React.FC<{ return ( - @@ -167,6 +171,7 @@ const Filters: React.FC<{ e.preventDefault()} > diff --git a/testing/AGENTS.md b/testing/AGENTS.md index 6d353efb3..1cb49ce71 100644 --- a/testing/AGENTS.md +++ b/testing/AGENTS.md @@ -65,11 +65,16 @@ A page object is only reachable from a spec once it is registered in the `pageMo | `appState` | test option | localStorage first-run gating; both flags default to accepted/complete | | `uniqueId` | test | Short random suffix for seeded data | | `api` | worker | `ApiClient` as admin — `createGroup()` / `createUser()` 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 | 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 6f3bc1461..27258da85 100644 --- a/testing/src/pages/_app/datahub/index.page.ts +++ b/testing/src/pages/_app/datahub/index.page.ts @@ -3,11 +3,23 @@ import type { Locator, Page } from '@playwright/test'; import { AppPage } from '../route.page'; export class DatahubPage extends AppPage { + readonly filtersTrigger: Locator; + readonly hasRecordsFilter: Locator; readonly pageHeader: Locator; readonly rowActionsTrigger: Locator; + readonly rows: Locator; constructor(page: Page) { super(page); + this.filtersTrigger = page.getByTestId('datahub-filters-trigger'); + this.hasRecordsFilter = page.getByTestId('datahub-filter-has-records'); 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'); + } + + /** Opens the filter menu and toggles "With records only", which refetches with `hasRecord=true`. */ + async toggleWithRecordsOnly() { + await this.filtersTrigger.click(); + await this.hasRecordsFilter.click(); } } diff --git a/testing/src/specs/datahub.spec.ts b/testing/src/specs/datahub.spec.ts index f529a3b39..58f981924 100644 --- a/testing/src/specs/datahub.spec.ts +++ b/testing/src/specs/datahub.spec.ts @@ -1,3 +1,4 @@ +import { DatahubPage } from '../pages/_app/datahub/index.page'; import { expect, test } from '../support/fixtures'; test.describe('data hub', () => { @@ -6,4 +7,42 @@ test.describe('data hub', () => { await expect(datahubPage.pageHeader).toBeVisible(); await expect(datahubPage.pageHeader).toContainText('Data Hub'); }); + + test('should list only subjects holding records once "with records only" is applied', async ({ + api, + isolatedGroupManager, + page, + uniqueId + }) => { + // A group of its own, so the row count is exactly what this test seeds. Both subjects are created + // through a session and never given an instrument record, so the filter must empty the table. + const group = await isolatedGroupManager(); + for (const suffix of ['a', 'b']) { + await api.createSession(group.id, { id: `recordless-${uniqueId}-${suffix}` }); + } + + const datahubPage = new DatahubPage(page); + await datahubPage.goto('/datahub'); + await expect(datahubPage.rows).toHaveCount(2); + + await datahubPage.toggleWithRecordsOnly(); + + await expect(datahubPage.rows).toHaveCount(0); + }); + + // `GET /v1/subjects` is gated on `read Subject`, which a standard user holds, but resolving + // `hasRecord` reads instrument records, which they do not. The honest answer is an empty list. + test('should answer the with-records filter for a caller who may read no records', async ({ + apiRequestContext, + roleAccount + }) => { + const { accessToken } = await roleAccount('STANDARD'); + + const response = await apiRequestContext.get('/api/v1/subjects?hasRecord=true', { + headers: { Authorization: `Bearer ${accessToken}` } + }); + + expect(response.status()).toBe(200); + expect(await response.json()).toStrictEqual([]); + }); }); diff --git a/testing/src/support/api-client.ts b/testing/src/support/api-client.ts index 6b96e4761..045d3f292 100644 --- a/testing/src/support/api-client.ts +++ b/testing/src/support/api-client.ts @@ -1,5 +1,7 @@ import type { $LoginCredentials } from '@opendatacapture/schemas/auth'; import type { CreateGroupData, Group } from '@opendatacapture/schemas/group'; +import type { CreateSessionData, Session } from '@opendatacapture/schemas/session'; +import type { CreateSubjectData } from '@opendatacapture/schemas/subject'; import type { CreateUserData, User } from '@opendatacapture/schemas/user'; import type { APIRequestContext } from '@playwright/test'; @@ -47,6 +49,19 @@ export class ApiClient { return group; } + /** + * Creates a session, and with it the subject it names. A subject seeded this way holds no + * instrument records, which is what distinguishes it under the "with records only" filter. + */ + async createSession(groupId: null | string, subjectData: CreateSubjectData): Promise { + const data: CreateSessionData = { date: new Date(), groupId, subjectData, type: 'IN_PERSON' }; + return this.expectJson( + this.request.post(`${API}/sessions`, { data, headers: this.authHeaders }), + 201, + 'create session' + ); + } + /** Creates a user (GROUP_MANAGER by default) and returns the login credentials for it. */ async createUser(overrides: Partial = {}): Promise<{ credentials: $LoginCredentials; user: User }> { const username = overrides.username ?? `user_${randomId()}`; diff --git a/testing/src/support/fixtures.ts b/testing/src/support/fixtures.ts index c9a0fd032..2d38810fd 100644 --- a/testing/src/support/fixtures.ts +++ b/testing/src/support/fixtures.ts @@ -1,5 +1,6 @@ /* eslint-disable no-empty-pattern */ +import type { Group } from '@opendatacapture/schemas/group'; import { request as apiRequestFactory, test as base, expect } from '@playwright/test'; import type { APIRequestContext } from '@playwright/test'; @@ -60,6 +61,14 @@ type TestFixtures = { authenticateAs: (role: 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 counts what the group contains cannot use it. A group manager's `read Subject` rule is + * scoped to their own groups, so a fresh group means the data this test seeds is all it can see. + */ + isolatedGroupManager: () => Promise; /** Short run-unique suffix for naming seeded data in this test. */ uniqueId: string; }; @@ -111,6 +120,21 @@ export const test = base.extend({ } ); }, + isolatedGroupManager: async ({ api, apiRequestContext, appState, page }, use) => { + await use(async () => { + const group = await api.createGroup(); + const { credentials } = await api.createUser({ basePermissionLevel: 'GROUP_MANAGER', groupIds: [group.id] }); + const accessToken = await ApiClient.login(apiRequestContext, credentials); + await page.addInitScript( + (injected) => { + window.__PLAYWRIGHT_ACCESS_TOKEN__ = injected.accessToken; + localStorage.setItem('app', JSON.stringify({ state: injected.state, version: 1 })); + }, + { accessToken, state: appState } + ); + return group; + }); + }, roleAccount: [ async ({ adminToken, api, apiRequestContext }, use) => { const cache = new Map([ From bad29c18451d2eddbd0b9d1c486ce269ead0fe63 Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Tue, 28 Jul 2026 14:23:47 -0400 Subject: [PATCH 4/5] test(e2e): keep a record-holding subject visible under the with-records filter The toggle test seeded only recordless subjects, so a filter that hid every subject unconditionally would have passed it. A third subject now holds an uploaded record and must be the one row that survives the toggle, pinning the filter in both directions. The auth-injection init script repeated between the two authenticating fixtures is shared instead of restated. Co-Authored-By: Claude Fable 5 --- testing/src/specs/datahub.spec.ts | 23 +++++++++++++++++---- testing/src/support/api-client.ts | 31 +++++++++++++++++++++++++++++ testing/src/support/fixtures.ts | 33 +++++++++++++++---------------- 3 files changed, 66 insertions(+), 21 deletions(-) diff --git a/testing/src/specs/datahub.spec.ts b/testing/src/specs/datahub.spec.ts index 58f981924..a14255aee 100644 --- a/testing/src/specs/datahub.spec.ts +++ b/testing/src/specs/datahub.spec.ts @@ -1,6 +1,13 @@ import { DatahubPage } from '../pages/_app/datahub/index.page'; import { expect, test } from '../support/fixtures'; +/** A minimal payload satisfying the seeded happiness questionnaire's validation schema. */ +const HAPPINESS_RECORD = { + isSatisfiedOverall: true, + personalLifeSatisfaction: 8, + professionalLifeSatisfaction: 7 +}; + test.describe('data hub', () => { test('should display the data hub header', async ({ getPageModel }) => { const datahubPage = await getPageModel('/datahub'); @@ -14,20 +21,28 @@ test.describe('data hub', () => { page, uniqueId }) => { - // A group of its own, so the row count is exactly what this test seeds. Both subjects are created - // through a session and never given an instrument record, so the filter must empty the table. + // A group of its own, so the row count is exactly what this test seeds. Two subjects exist only + // through sessions while a third holds a record, so the filter must drop exactly the recordless + // pair — hiding every subject or filtering none would both fail. const group = await isolatedGroupManager(); + const withRecord = `hasrecord-${uniqueId}`; for (const suffix of ['a', 'b']) { await api.createSession(group.id, { id: `recordless-${uniqueId}-${suffix}` }); } + await api.uploadRecords(group.id, await api.findInstrumentIdByName('DNP_HAPPINESS_QUESTIONNAIRE'), [ + { data: HAPPINESS_RECORD, date: new Date(), subjectId: withRecord } + ]); const datahubPage = new DatahubPage(page); await datahubPage.goto('/datahub'); - await expect(datahubPage.rows).toHaveCount(2); + await expect(datahubPage.rows).toHaveCount(3); await datahubPage.toggleWithRecordsOnly(); - await expect(datahubPage.rows).toHaveCount(0); + await expect(datahubPage.rows).toHaveCount(1); + // The cell renders at most the id's first nine characters (the subjectIdDisplayLength default), + // so the assertion matches the visible prefix rather than the full seeded id. + await expect(datahubPage.rows).toContainText(withRecord.slice(0, 9)); }); // `GET /v1/subjects` is gated on `read Subject`, which a standard user holds, but resolving diff --git a/testing/src/support/api-client.ts b/testing/src/support/api-client.ts index 045d3f292..24554f6e9 100644 --- a/testing/src/support/api-client.ts +++ b/testing/src/support/api-client.ts @@ -1,5 +1,6 @@ import type { $LoginCredentials } from '@opendatacapture/schemas/auth'; import type { CreateGroupData, Group } from '@opendatacapture/schemas/group'; +import type { UploadInstrumentRecordsData } from '@opendatacapture/schemas/instrument-records'; import type { CreateSessionData, Session } from '@opendatacapture/schemas/session'; import type { CreateSubjectData } from '@opendatacapture/schemas/subject'; import type { CreateUserData, User } from '@opendatacapture/schemas/user'; @@ -10,6 +11,8 @@ import { randomId } from './unique'; const API = '/api/v1'; +type UploadRecord = UploadInstrumentRecordsData['records'][number]; + /** Typed helper for seeding preconditions (groups, users) and authenticating over the API. */ export class ApiClient { private readonly request: APIRequestContext; @@ -83,6 +86,34 @@ export class ApiClient { return { credentials: { password, username }, user }; } + /** The id of a seeded instrument, looked up by the internal name its source file declares. */ + async findInstrumentIdByName(name: string): Promise { + const instruments = await this.expectJson<{ id: string; internal?: { name: string } }[]>( + this.request.get(`${API}/instruments/info`, { headers: this.authHeaders }), + 200, + 'list instruments' + ); + const instrument = instruments.find((candidate) => candidate.internal?.name === name); + if (!instrument) { + throw new Error(`No instrument named '${name}' among ${instruments.length} returned`); + } + return instrument.id; + } + + /** + * Bulk-creates one record per entry, and with them the subjects and sessions they name. This is the + * cheapest way to give a subject an instrument record: the export only carries subjects that have + * one. + */ + async uploadRecords(groupId: string, instrumentId: string, records: UploadRecord[]): Promise { + const data: UploadInstrumentRecordsData = { groupId, instrumentId, records }; + await this.expectJson( + this.request.post(`${API}/instrument-records/upload`, { data, headers: this.authHeaders }), + 201, + 'upload instrument records' + ); + } + private async expectJson( pending: ReturnType, status: number, diff --git a/testing/src/support/fixtures.ts b/testing/src/support/fixtures.ts index 2d38810fd..edc30fbf8 100644 --- a/testing/src/support/fixtures.ts +++ b/testing/src/support/fixtures.ts @@ -2,7 +2,7 @@ import type { Group } from '@opendatacapture/schemas/group'; import { request as apiRequestFactory, test as base, expect } from '@playwright/test'; -import type { APIRequestContext } from '@playwright/test'; +import type { APIRequestContext, Page } from '@playwright/test'; import { SettingsPage } from '../pages/_app/admin/settings.page'; import { DashboardPage } from '../pages/_app/dashboard.page'; @@ -32,6 +32,16 @@ const pageModels = { type PageModels = typeof pageModels; +/** Injects the token and first-run state the web app reads on boot; must run before navigation. */ +const injectAuth = (page: Page, accessToken: string, state: AppState) => + page.addInitScript( + (injected) => { + window.__PLAYWRIGHT_ACCESS_TOKEN__ = injected.accessToken; + localStorage.setItem('app', JSON.stringify({ state: injected.state, version: 1 })); + }, + { accessToken, state } + ); + type GetPageModel = >( key: TKey, ...args: NavigateVariadicArgs @@ -65,8 +75,9 @@ type TestFixtures = { * 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 counts what the group contains cannot use it. A group manager's `read Subject` rule is - * scoped to their own groups, so a fresh group means the data this test seeds is all it can see. + * 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; /** Short run-unique suffix for naming seeded data in this test. */ @@ -99,13 +110,7 @@ export const test = base.extend({ authenticateAs: async ({ appState, page, roleAccount }, use) => { await use(async (role) => { const { accessToken } = await roleAccount(role); - await page.addInitScript( - (injected) => { - window.__PLAYWRIGHT_ACCESS_TOKEN__ = injected.accessToken; - localStorage.setItem('app', JSON.stringify({ state: injected.state, version: 1 })); - }, - { accessToken, state: appState } - ); + await injectAuth(page, accessToken, appState); }); }, getPageModel: async ({ actingRole, authenticateAs, page }, use) => { @@ -125,13 +130,7 @@ export const test = base.extend({ const group = await api.createGroup(); const { credentials } = await api.createUser({ basePermissionLevel: 'GROUP_MANAGER', groupIds: [group.id] }); const accessToken = await ApiClient.login(apiRequestContext, credentials); - await page.addInitScript( - (injected) => { - window.__PLAYWRIGHT_ACCESS_TOKEN__ = injected.accessToken; - localStorage.setItem('app', JSON.stringify({ state: injected.state, version: 1 })); - }, - { accessToken, state: appState } - ); + await injectAuth(page, accessToken, appState); return group; }); }, From 3a73274225383fbd9b2b95fdf1f21885966255b8 Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Tue, 28 Jul 2026 18:08:03 -0400 Subject: [PATCH 5/5] docs(api): record that the record scoping clause drops ungrouped records A group manager's rule compiles to `groupId: { in: [...] }` and prisma's `in` never matches null, so admin-created records do not make their subject count as "with records" for a manager. Intended, but invisible at the call site. Co-Authored-By: Claude Fable 5 --- apps/api/src/subjects/subjects.service.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/apps/api/src/subjects/subjects.service.ts b/apps/api/src/subjects/subjects.service.ts index 87acd64c9..58ee5aabc 100644 --- a/apps/api/src/subjects/subjects.service.ts +++ b/apps/api/src/subjects/subjects.service.ts @@ -179,6 +179,10 @@ export class SubjectsService { if (ability && !ability.can('read', 'InstrumentRecord')) { return []; } + // The accessibleQuery clause also drops records whose groupId is null: a group manager's rule is + // `groupId: { in: [...] }`, and prisma's `in` never matches null. That is intended — an + // admin-created, ungrouped record should not make a subject count as "with records" for a + // manager — but it is invisible at this call site, so it is recorded here. const groups = await this.prismaClient.instrumentRecord.groupBy({ by: ['subjectId'], where: {