diff --git a/apps/api/src/subjects/__tests__/subjects.service.spec.ts b/apps/api/src/subjects/__tests__/subjects.service.spec.ts index df87ca6b4..5d6c7545c 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'; @@ -8,6 +8,7 @@ import type { Subject } from '@prisma/client'; 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'; @@ -32,7 +33,8 @@ describe('SubjectsService', () => { $transaction: vi.fn(), instrumentRecord: { deleteMany: vi.fn(), - findMany: vi.fn() + findMany: vi.fn(), + groupBy: vi.fn() }, session: { deleteMany: vi.fn() @@ -135,16 +137,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({ @@ -153,18 +148,65 @@ 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', + kind: 'login', + 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 8c8e1aed2..f4d50936c 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'; @@ -146,7 +147,7 @@ export class SubjectsService { AND: [ accessibleQuery(ability, 'read', 'Subject'), groupInput, - { id: { in: await this.querySubjectIdsWithRecords(groupId) } } + { id: { in: await this.querySubjectIdsWithRecords(groupId, ability) } } ] } }); @@ -190,12 +191,36 @@ export class SubjectsService { return subjects.map((subject) => subject.id); } - 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 8fdc2b0b6..4b2b785c1 100644 --- a/apps/web/src/routes/_app/datahub/index.tsx +++ b/apps/web/src/routes/_app/datahub/index.tsx @@ -60,7 +60,11 @@ const Filters: React.FC<{ return ( - @@ -170,6 +174,7 @@ const Filters: React.FC<{ e.preventDefault()} > diff --git a/testing/src/pages/_app/datahub/index.page.ts b/testing/src/pages/_app/datahub/index.page.ts index 1a87a5de4..e8e688c6d 100644 --- a/testing/src/pages/_app/datahub/index.page.ts +++ b/testing/src/pages/_app/datahub/index.page.ts @@ -4,6 +4,8 @@ import { AppPage } from '../route.page'; export class DatahubPage extends AppPage { readonly exportDropdown: Locator; + readonly filtersTrigger: Locator; + readonly hasRecordsFilter: Locator; readonly pageHeader: Locator; readonly rowActionsTrigger: Locator; readonly rows: Locator; @@ -11,6 +13,8 @@ export class DatahubPage extends AppPage { constructor(page: Page) { super(page); this.exportDropdown = page.getByTestId('datahub-export-dropdown'); + 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'); @@ -28,4 +32,10 @@ export class DatahubPage extends AppPage { await this.$ref.getByRole('menuitem', { exact: true, name: format }).click(); return started; } + + /** 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 f39a3de68..3deb89fe3 100644 --- a/testing/src/specs/datahub.spec.ts +++ b/testing/src/specs/datahub.spec.ts @@ -180,4 +180,50 @@ test.describe('data hub', () => { expect(payload.length).toBeGreaterThan(0); expect([...new Set(payload.map((row) => row.subjectId))]).toStrictEqual([listed]); }); + + 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 5ef6c67ab..69b567ce2 100644 --- a/testing/src/support/api-client.ts +++ b/testing/src/support/api-client.ts @@ -64,7 +64,10 @@ export class ApiClient { return group; } - /** Creates a session, and with it the subject it names. */ + /** + * 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(