diff --git a/apps/api/src/subjects/__tests__/subjects.service.spec.ts b/apps/api/src/subjects/__tests__/subjects.service.spec.ts index ea12d89d3..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,8 @@ 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'; import { SubjectsService } from '../subjects.service'; @@ -30,7 +32,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 +83,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 +94,60 @@ 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'] }) + ); + }); 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, 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', 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[] } }]; + 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.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..58ee5aabc 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,36 @@ 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 { + // `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 []; + } + // 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: { + AND: [accessibleQuery(ability, 'read', 'InstrumentRecord'), groupId ? { groupId } : {}] + } }); - return records.map((r) => r.subjectId); + return groups.map((group) => group.subjectId); } } 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..a14255aee 100644 --- a/testing/src/specs/datahub.spec.ts +++ b/testing/src/specs/datahub.spec.ts @@ -1,9 +1,63 @@ +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'); 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. 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(3); + + await datahubPage.toggleWithRecordsOnly(); + + 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 + // `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..24554f6e9 100644 --- a/testing/src/support/api-client.ts +++ b/testing/src/support/api-client.ts @@ -1,5 +1,8 @@ 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'; import type { APIRequestContext } from '@playwright/test'; @@ -8,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; @@ -47,6 +52,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()}`; @@ -68,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 c9a0fd032..edc30fbf8 100644 --- a/testing/src/support/fixtures.ts +++ b/testing/src/support/fixtures.ts @@ -1,7 +1,8 @@ /* 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'; +import type { APIRequestContext, Page } from '@playwright/test'; import { SettingsPage } from '../pages/_app/admin/settings.page'; import { DashboardPage } from '../pages/_app/dashboard.page'; @@ -31,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 @@ -60,6 +71,15 @@ 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 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. */ uniqueId: string; }; @@ -90,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) => { @@ -111,6 +125,15 @@ 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 injectAuth(page, accessToken, appState); + return group; + }); + }, roleAccount: [ async ({ adminToken, api, apiRequestContext }, use) => { const cache = new Map([