diff --git a/apps/api/src/subjects/__tests__/subjects.service.spec.ts b/apps/api/src/subjects/__tests__/subjects.service.spec.ts index ea12d89d3..6968cf66c 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,64 @@ 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', + firstName: null, + groups: [], + id: 'user-1', + lastName: null, + mustResetPassword: false, + username: 'standard-user' + }); + 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..a5c738bc3 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 an `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 2a85fcc3f..34861ba44 100644 --- a/testing/AGENTS.md +++ b/testing/AGENTS.md @@ -59,20 +59,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()` 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()` 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..091fa2077 100644 --- a/testing/src/pages/_app/datahub/index.page.ts +++ b/testing/src/pages/_app/datahub/index.page.ts @@ -3,13 +3,25 @@ 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; readonly searchInput: 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'); this.searchInput = page.getByTestId('data-table-search-bar').getByRole('searchbox'); } + + /** 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 bed4ba2cb..e6fbd7188 100644 --- a/testing/src/specs/datahub.spec.ts +++ b/testing/src/specs/datahub.spec.ts @@ -1,5 +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'); @@ -91,4 +99,50 @@ test.describe('data hub', () => { await expect(page).toHaveURL(/\/datahub\/.+\/table$/); }); + + 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 ba40158c1..550e6648c 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, UpdateUserData, 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()}`; @@ -77,6 +95,20 @@ export class ApiClient { ); } + /** 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; + } + /** * Switch outgoing mail on or off instance-wide, (re)seeding {@link E2E_MAIL_CONFIG}. Every * caller must switch it back off — `isMailEnabled` is global, and leaving it on changes the UI @@ -104,6 +136,20 @@ export class ApiClient { return (await response.json()) as User; } + /** + * 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 98c6ab68c..c6f39e6ec 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'; @@ -94,6 +95,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 @@ -151,6 +161,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([