diff --git a/apps/api/src/instrument-records/__tests__/instrument-records.service.spec.ts b/apps/api/src/instrument-records/__tests__/instrument-records.service.spec.ts index 6c39ceebe..d69283b56 100644 --- a/apps/api/src/instrument-records/__tests__/instrument-records.service.spec.ts +++ b/apps/api/src/instrument-records/__tests__/instrument-records.service.spec.ts @@ -184,19 +184,60 @@ describe('InstrumentRecordsService', () => { beforeEach(() => { instrumentsService.findById.mockResolvedValue(mockInstrument as any); subjectsService.createMany.mockResolvedValue([] as any); - sessionsService.create.mockResolvedValue(mockSession as any); + sessionsService.createMany.mockResolvedValue([mockSession] as any); sessionsService.deleteByIds.mockResolvedValue(undefined as any); instrumentRecordModel.createMany.mockResolvedValue([] as any); instrumentRecordModel.findMany.mockResolvedValue([] as any); }); - it('should call sessionsService.create with the provided username', async () => { + it('should create the sessions in one batched call carrying the provided username', async () => { usersService.findByUsername.mockResolvedValueOnce({ groups: [{ id: 'group-1' }], username: 'validuser' } as any); await instrumentRecordsService.upload({ ...baseUploadData, groupId: 'group-1', username: 'validuser' }); expect(usersService.findByUsername).toHaveBeenCalledWith('validuser', undefined); - expect(sessionsService.create).toHaveBeenCalledWith(expect.objectContaining({ username: 'validuser' })); + expect(sessionsService.create).not.toHaveBeenCalled(); + expect(sessionsService.createMany).toHaveBeenCalledTimes(1); + expect(sessionsService.createMany).toHaveBeenCalledWith( + expect.objectContaining({ groupId: 'group-1', type: 'RETROSPECTIVE', username: 'validuser' }) + ); + }); + + it('should batch every record into a single session creation call', async () => { + const records = Array.from({ length: 25 }, (_, i) => ({ + data: { answer: i }, + date: new Date(), + subjectId: `subject-${i}` + })); + sessionsService.createMany.mockResolvedValueOnce( + records.map((_, i) => ({ ...mockSession, id: `session-${i}` })) as any + ); + + await instrumentRecordsService.upload({ ...baseUploadData, records }); + + expect(sessionsService.createMany).toHaveBeenCalledTimes(1); + const [call] = sessionsService.createMany.mock.lastCall as [{ entries: unknown[] }]; + expect(call.entries).toHaveLength(25); + }); + + it('should pair each record with the session created for it, by position', async () => { + const records = [ + { data: { answer: 1 }, date: new Date(), subjectId: 'subject-a' }, + { data: { answer: 2 }, date: new Date(), subjectId: 'subject-b' } + ]; + sessionsService.createMany.mockResolvedValueOnce([ + { ...mockSession, id: 'session-a' }, + { ...mockSession, id: 'session-b' } + ] as any); + + await instrumentRecordsService.upload({ ...baseUploadData, records }); + + expect(instrumentRecordModel.createMany.mock.lastCall?.[0]).toMatchObject({ + data: [ + { sessionId: 'session-a', subjectId: 'subject-a' }, + { sessionId: 'session-b', subjectId: 'subject-b' } + ] + }); }); it('should throw a ForbiddenException when a non-admin user uploads without a group', async () => { @@ -209,7 +250,7 @@ describe('InstrumentRecordsService', () => { instrumentRecordsService.upload({ ...baseUploadData, username: 'validuser' }) ).rejects.toBeInstanceOf(ForbiddenException); - expect(sessionsService.create).not.toHaveBeenCalled(); + expect(sessionsService.createMany).not.toHaveBeenCalled(); }); it('should throw a ForbiddenException when a user uploads to a group they are not a member of', async () => { @@ -222,7 +263,7 @@ describe('InstrumentRecordsService', () => { instrumentRecordsService.upload({ ...baseUploadData, groupId: 'group-1', username: 'validuser' }) ).rejects.toBeInstanceOf(ForbiddenException); - expect(sessionsService.create).not.toHaveBeenCalled(); + expect(sessionsService.createMany).not.toHaveBeenCalled(); }); it('should reject and not create any sessions when an unknown username is provided', async () => { @@ -234,18 +275,89 @@ describe('InstrumentRecordsService', () => { NotFoundException ); - expect(sessionsService.create).not.toHaveBeenCalled(); + expect(sessionsService.createMany).not.toHaveBeenCalled(); }); - it('should call sessionsService.create with username undefined when no username is provided', async () => { + it('should create the sessions with username undefined when no username is provided', async () => { await instrumentRecordsService.upload({ ...baseUploadData }); expect(usersService.findByUsername).not.toHaveBeenCalled(); - expect(sessionsService.create).toHaveBeenCalledWith(expect.objectContaining({ username: undefined })); + expect(sessionsService.createMany).toHaveBeenCalledWith(expect.objectContaining({ username: undefined })); + }); + + it('should return only the records this upload created, not every record in the group', async () => { + await instrumentRecordsService.upload({ ...baseUploadData, groupId: 'group-1' }); + + expect(instrumentRecordModel.findMany).toHaveBeenCalledWith({ + where: { sessionId: { in: ['session-1'] } } + }); + }); + + it('should reject an invalid record before creating any sessions', async () => { + instrumentsService.findById.mockResolvedValue({ + ...mockInstrument, + validationSchema: { safeParse: () => ({ error: { issues: [] }, success: false }) } + } as any); + + await expect(instrumentRecordsService.upload({ ...baseUploadData })).rejects.toBeInstanceOf( + UnprocessableEntityException + ); + + expect(sessionsService.createMany).not.toHaveBeenCalled(); + expect(instrumentRecordModel.createMany).not.toHaveBeenCalled(); + }); + + it('should report which record failed and why, so a rejected batch can be corrected', async () => { + const issues = [{ message: 'Required', path: ['answer'] }]; + instrumentsService.findById.mockResolvedValue({ + ...mockInstrument, + validationSchema: { + safeParse: (data: any) => (data.answer === 2 ? { error: { issues }, success: false } : { data, success: true }) + } + } as any); + + await expect( + instrumentRecordsService.upload({ + ...baseUploadData, + records: [ + { data: { answer: 1 }, date: new Date(), subjectId: 'subject-1' }, + { data: { answer: 2 }, date: new Date(), subjectId: 'subject-2' } + ] + }) + ).rejects.toMatchObject({ + response: { issues, message: expect.stringContaining('at index 1') } + }); + }); + + it('should roll back the sessions when the record insert fails, so none is left without records', async () => { + instrumentRecordModel.createMany.mockRejectedValueOnce(new Error('insert failed')); + + await expect(instrumentRecordsService.upload({ ...baseUploadData })).rejects.toThrow('insert failed'); + + expect(sessionsService.deleteByIds).toHaveBeenCalledWith(['session-1']); + }); + + it('should keep the sessions when only the read-back fails, since the records already reference them', async () => { + instrumentRecordModel.findMany.mockRejectedValueOnce(new Error('read-back failed')); + + await expect(instrumentRecordsService.upload({ ...baseUploadData })).rejects.toThrow('read-back failed'); + + expect(sessionsService.deleteByIds).not.toHaveBeenCalled(); + }); + + // The bulk payload cannot carry a file and this path never attaches one, so such a record could + // only ever be incomplete. Refusing it is the same call `create` makes for series instruments. + it('should reject a file instrument rather than write a record its files can never reach', async () => { + instrumentsService.findById.mockResolvedValue({ ...mockInstrument, kind: 'FILE' } as any); + + await expect(instrumentRecordsService.upload({ ...baseUploadData })).rejects.toBeInstanceOf( + UnprocessableEntityException + ); + + expect(sessionsService.createMany).not.toHaveBeenCalled(); + expect(instrumentRecordModel.createMany).not.toHaveBeenCalled(); }); - // `pending` is intentionally not written on create; the find-side OR filter treats missing and - // false `pending` alike (see the 'find' describe block), so records stay query-visible without it. it('should create records via createMany with the processed record data', async () => { await instrumentRecordsService.upload({ ...baseUploadData }); diff --git a/apps/api/src/instrument-records/instrument-records.service.ts b/apps/api/src/instrument-records/instrument-records.service.ts index 31b6a0597..f66a223b0 100644 --- a/apps/api/src/instrument-records/instrument-records.service.ts +++ b/apps/api/src/instrument-records/instrument-records.service.ts @@ -24,7 +24,7 @@ import type { UploadInstrumentRecordsData } from '@opendatacapture/schemas/instrument-records'; import { Prisma } from '@prisma/client'; -import type { InstrumentRecord as PrismaInstrumentRecord, Session } from '@prisma/client'; +import type { InstrumentRecord as PrismaInstrumentRecord } from '@prisma/client'; import { isNumber, mergeWith, pickBy } from 'lodash-es'; import { accessibleQuery } from '@/auth/ability.utils'; @@ -34,7 +34,6 @@ import { GroupsService } from '@/groups/groups.service'; import { InstrumentsService } from '@/instruments/instruments.service'; import { SessionsService } from '@/sessions/sessions.service'; import { StorageService } from '@/storage/storage.service'; -import { CreateSubjectDto } from '@/subjects/dto/create-subject.dto'; import { SubjectsService } from '@/subjects/subjects.service'; import { UsersService } from '@/users/users.service'; @@ -398,6 +397,14 @@ export class InstrumentRecordsService { `Cannot create instrument record for series instrument '${instrument.id}'` ); } + // A bulk-uploaded file record could never be completed: the payload schema has nowhere to carry a + // file, this path never attaches one, and the response ids the client would need to attach one + // afterwards are discarded. Refuse it rather than write a record that can only ever be pending. + if (instrument.kind === 'FILE') { + throw new UnprocessableEntityException( + `Cannot create instrument record for file instrument '${instrument.id}': files cannot be attached to a bulk upload` + ); + } if (username) { const user = await this.usersService.findByUsername(username, options); @@ -409,70 +416,60 @@ export class InstrumentRecordsService { } } - const createdSessionsArray: Session[] = []; - - try { - const subjectIdList = records.map(({ subjectId }) => { - const subjectToAdd: CreateSubjectDto = { id: subjectId }; - - return subjectToAdd; - }); - - await this.subjectsService.createMany(subjectIdList); - - const preProcessedRecords = await Promise.all( - records.map(async (record) => { - const { data: rawData, date, subjectId } = record; + // Every record is validated before anything is written, so a malformed record in the middle of a + // batch rejects the request without first creating sessions that then have to be rolled back. + const validatedRecords = records.map((record, index) => { + const parseResult = instrument.validationSchema.safeParse(this.parseJson(record.data)); + if (!parseResult.success) { + throw new UnprocessableEntityException({ + error: 'Unprocessable Entity', + issues: parseResult.error.issues, + message: `Data received for record at index ${index} does not pass validation schema of instrument '${instrument.id}'`, + statusCode: 422 + }); + } + return { data: parseResult.data, date: record.date, subjectId: record.subjectId }; + }); - // Validate data - const parseResult = instrument.validationSchema.safeParse(this.parseJson(rawData)); - if (!parseResult.success) { - console.error(parseResult.error.issues); - throw new UnprocessableEntityException( - `Data received for record does not pass validation schema of instrument '${instrument.id}'` - ); - } + // One batched call rather than one session creation per record, which cost several queries each. + // Returned in input order, so each record can be paired with its session by index. + const sessions = await this.sessionsService.createMany({ + entries: validatedRecords.map((record) => ({ + date: record.date, + subjectData: { id: record.subjectId } + })), + groupId: groupId ?? null, + type: 'RETROSPECTIVE', + username: username ?? undefined + }); - const session = await this.sessionsService.create({ - date: date, - groupId: groupId ?? null, - subjectData: { id: subjectId }, - type: 'RETROSPECTIVE', - username: username ?? undefined - }); - - createdSessionsArray.push(session); - - const computedMeasures = instrument.measures - ? this.instrumentMeasuresService.computeMeasures(instrument.measures, parseResult.data) - : null; - - return { - computedMeasures, - data: this.serializeData(parseResult.data), - date, - groupId, - instrumentId, - pending: false, - sessionId: session.id, - subjectId - }; - }) - ); + // Only the insert is rolled back on failure. Deleting the sessions after it has succeeded would + // strand the records that now reference them, so the read-back below sits outside the catch. + try { await this.instrumentRecordModel.createMany({ - data: preProcessedRecords - }); - - return this.instrumentRecordModel.findMany({ - where: { + data: validatedRecords.map((record, index) => ({ + computedMeasures: instrument.measures + ? this.instrumentMeasuresService.computeMeasures(instrument.measures, record.data) + : null, + data: this.serializeData(record.data), + date: record.date, groupId, - instrumentId - } + instrumentId, + pending: false, + sessionId: sessions[index]!.id, + subjectId: record.subjectId + })) }); } catch (err) { - await this.sessionsService.deleteByIds(createdSessionsArray.map((session) => session.id)); + await this.sessionsService.deleteByIds(sessions.map((session) => session.id)); throw err; } + + return this.instrumentRecordModel.findMany({ + where: { + sessionId: { in: sessions.map((session) => session.id) } + } + }); } private getInstrumentById(instrumentId: string) { diff --git a/apps/api/src/sessions/__tests__/sessions.service.spec.ts b/apps/api/src/sessions/__tests__/sessions.service.spec.ts new file mode 100644 index 000000000..d5221f0e1 --- /dev/null +++ b/apps/api/src/sessions/__tests__/sessions.service.spec.ts @@ -0,0 +1,174 @@ +import { 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'; +import { InternalServerErrorException } from '@nestjs/common'; +import { Test } from '@nestjs/testing'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import type { RuntimePrismaClient } from '@/core/prisma'; +import { GroupsService } from '@/groups/groups.service'; +import { SubjectsService } from '@/subjects/subjects.service'; + +import { SessionsService } from '../sessions.service'; + +describe('SessionsService', () => { + let sessionsService: SessionsService; + let sessionModel: MockedInstance>; + let groupsService: MockedInstance; + let subjectsService: MockedInstance; + let prismaClient: MockedInstance & { [key: string]: any }; + + const entry = (id: string) => ({ date: new Date(), subjectData: { id } }); + + beforeEach(async () => { + const moduleRef = await Test.createTestingModule({ + providers: [ + SessionsService, + MockFactory.createForModelToken(getModelToken('Session')), + MockFactory.createForService(GroupsService), + MockFactory.createForService(LoggingService), + MockFactory.createForService(SubjectsService), + { + provide: PRISMA_CLIENT_TOKEN, + useValue: { + user: { findFirst: vi.fn() } + } + } + ] + }).compile(); + + sessionsService = moduleRef.get(SessionsService); + sessionModel = moduleRef.get(getModelToken('Session')); + groupsService = moduleRef.get(GroupsService); + subjectsService = moduleRef.get(SubjectsService); + prismaClient = moduleRef.get(PRISMA_CLIENT_TOKEN); + + subjectsService.createMany.mockResolvedValue([] as any); + subjectsService.addGroupForSubjects.mockResolvedValue({ count: 0 } as any); + prismaClient.user.findFirst.mockResolvedValue(null); + sessionModel.createMany.mockResolvedValue({ count: 1 } as any); + sessionModel.findMany.mockImplementation(({ where }: any) => + Promise.resolve(where.id.in.map((id: string) => ({ id }))) + ); + }); + + describe('createMany', () => { + it('should not touch the database when there are no entries', async () => { + await expect(sessionsService.createMany({ entries: [], groupId: null, type: 'RETROSPECTIVE' })).resolves.toEqual( + [] + ); + expect(sessionModel.createMany).not.toHaveBeenCalled(); + }); + + // A session whose groupId is unset is invisible to a group manager, whose Session rule is + // { groupId: { in: [...] } }, and uncounted by every group-scoped query. + it('should set the groupId on a session for a subject that is already a member of the group', async () => { + groupsService.findById.mockResolvedValueOnce({ id: 'group-1' } as any); + + await sessionsService.createMany({ + entries: [entry('subject-1')], + groupId: 'group-1', + type: 'RETROSPECTIVE' + }); + + expect(sessionModel.createMany.mock.lastCall?.[0]).toMatchObject({ data: [{ groupId: 'group-1' }] }); + }); + + it('should associate the batch with the group in a single write', async () => { + groupsService.findById.mockResolvedValueOnce({ id: 'group-1' } as any); + + await sessionsService.createMany({ + entries: [entry('subject-1'), entry('subject-2')], + groupId: 'group-1', + type: 'RETROSPECTIVE' + }); + + // Every subject is handed over; which of them actually need the write is decided by the query + // `SubjectsService` builds, not by a membership list read here. + expect(subjectsService.addGroupForSubjects).toHaveBeenCalledExactlyOnceWith( + ['subject-1', 'subject-2'], + 'group-1' + ); + }); + + it('should not associate anything when no group was supplied', async () => { + await sessionsService.createMany({ entries: [entry('subject-1')], groupId: null, type: 'RETROSPECTIVE' }); + + expect(groupsService.findById).not.toHaveBeenCalled(); + expect(subjectsService.addGroupForSubjects).not.toHaveBeenCalled(); + }); + + it('should return the sessions in the order the entries were given, so callers can pair by index', async () => { + // The read-back is a findMany, which is free to return documents in any order. + sessionModel.findMany.mockImplementationOnce(({ where }: any) => + Promise.resolve([...where.id.in].reverse().map((id: string) => ({ id }))) + ); + + const sessions = await sessionsService.createMany({ + entries: [entry('subject-a'), entry('subject-b')], + groupId: null, + type: 'RETROSPECTIVE' + }); + + const [call] = sessionModel.createMany.mock.lastCall as [{ data: { id: string; subjectId: string }[] }]; + expect(sessions.map((session) => session.id)).toStrictEqual(call.data.map((session) => session.id)); + expect(call.data.map((session) => session.subjectId)).toStrictEqual(['subject-a', 'subject-b']); + }); + + it('should create every session in one call rather than one call per entry', async () => { + const entries = Array.from({ length: 20 }, (_, i) => entry(`subject-${i}`)); + + await sessionsService.createMany({ entries, groupId: null, type: 'RETROSPECTIVE' }); + + expect(sessionModel.createMany).toHaveBeenCalledOnce(); + expect(sessionModel.createMany.mock.lastCall?.[0].data).toHaveLength(20); + }); + + it('should resolve the user once for the whole batch and stamp it on every session', async () => { + const entries = [entry('subject-a'), entry('subject-b')]; + prismaClient.user.findFirst.mockResolvedValueOnce({ id: 'user-1', username: 'someone' }); + + await sessionsService.createMany({ entries, groupId: null, type: 'RETROSPECTIVE', username: 'someone' }); + + expect(prismaClient.user.findFirst).toHaveBeenCalledOnce(); + expect(sessionModel.createMany.mock.lastCall?.[0]).toMatchObject({ + data: [{ userId: 'user-1' }, { userId: 'user-1' }] + }); + }); + + // Without this, a session missing from the read-back would be handed to the caller as + // `undefined` typed `Session`, and `POST /sessions` would answer 201 with an empty body. + it('should throw rather than return a gap when the read-back misses a created session', async () => { + sessionModel.findMany.mockImplementationOnce(({ where }: any) => + Promise.resolve(where.id.in.slice(1).map((id: string) => ({ id }))) + ); + + await expect( + sessionsService.createMany({ + entries: [entry('subject-a'), entry('subject-b')], + groupId: null, + type: 'RETROSPECTIVE' + }) + ).rejects.toBeInstanceOf(InternalServerErrorException); + }); + }); + + describe('create', () => { + it('should return the single session created for the entry', async () => { + groupsService.findById.mockResolvedValueOnce({ id: 'group-1' } as any); + + const session = await sessionsService.create({ + date: new Date(), + groupId: 'group-1', + subjectData: { id: 'subject-1' }, + type: 'IN_PERSON' + }); + + expect(sessionModel.createMany.mock.lastCall?.[0]).toMatchObject({ + data: [{ groupId: 'group-1', subjectId: 'subject-1', type: 'IN_PERSON' }] + }); + expect(session.id).toBe(sessionModel.createMany.mock.lastCall?.[0].data[0].id); + }); + }); +}); diff --git a/apps/api/src/sessions/sessions.service.ts b/apps/api/src/sessions/sessions.service.ts index 2b0889424..62f42ec52 100644 --- a/apps/api/src/sessions/sessions.service.ts +++ b/apps/api/src/sessions/sessions.service.ts @@ -3,8 +3,8 @@ import type { Model } from '@douglasneuroinformatics/libnest'; import { Injectable, InternalServerErrorException, NotFoundException } from '@nestjs/common'; import type { Group } from '@opendatacapture/schemas/group'; import type { CreateSessionData } from '@opendatacapture/schemas/session'; -import type { CreateSubjectData } from '@opendatacapture/schemas/subject'; -import type { Prisma, Session, Subject, User } from '@prisma/client'; +import type { Prisma, Session, User } from '@prisma/client'; +import { ObjectId } from 'mongodb'; import { accessibleQuery } from '@/auth/ability.utils'; import type { RuntimePrismaClient } from '@/core/prisma'; @@ -12,6 +12,11 @@ import type { EntityOperationOptions } from '@/core/types'; import { GroupsService } from '@/groups/groups.service'; import { SubjectsService } from '@/subjects/subjects.service'; +/** The batched form of `CreateSessionData`: what varies per session, and what the batch shares. */ +type CreateManySessionsData = Pick & { + entries: Pick[]; +}; + @Injectable() export class SessionsService { constructor( @@ -29,54 +34,68 @@ export class SessionsService { } async create({ date, groupId, subjectData, type, username }: CreateSessionData): Promise { - this.loggingService.debug({ message: 'Attempting to create session' }); - const subject = await this.resolveSubject(subjectData); - - let user: null | Omit = null; + const [session] = await this.createMany({ + entries: [{ date, subjectData }], + groupId, + type, + username + }); + return session!; + } - if (username) { - user = await this.prismaClient.user.findFirst({ - where: { - username: username - } - }); + /** + * Create one session per entry, in a fixed number of queries rather than a fixed number per entry. + * + * Returned in the same order as `entries`, so a caller can pair each session with the input that + * produced it without a second lookup. + */ + async createMany({ entries, groupId, type, username }: CreateManySessionsData): Promise { + if (entries.length === 0) { + return []; } + this.loggingService.debug({ message: `Attempting to create ${entries.length} session(s)` }); - // If the subject is not yet associated with the group, check it exists then append it - let group: Group | null = null; - if (groupId && !subject.groupIds.includes(groupId)) { - group = await this.groupsService.findById(groupId); - if (group) { - await this.subjectsService.addGroupForSubject(subject.id, group.id); - } + await this.subjectsService.createMany(entries.map((entry) => entry.subjectData)); + + const user: null | Omit = username + ? await this.prismaClient.user.findFirst({ where: { username } }) + : null; + + const group: Group | null = groupId ? await this.groupsService.findById(groupId) : null; + if (group) { + await this.subjectsService.addGroupForSubjects( + Array.from(new Set(entries.map((entry) => entry.subjectData.id))), + group.id + ); } - const { id } = await this.sessionModel.create({ - data: { - date, - group: group - ? { - connect: { id: group.id } - } - : undefined, - subject: { - connect: { id: subject.id } - }, + // Generated up front so the sessions can be read back in the order they were requested; + // createMany does not return the documents it inserted. + const ids = entries.map(() => new ObjectId().toHexString()); + + await this.sessionModel.createMany({ + data: entries.map((entry, index) => ({ + date: entry.date, + groupId: group?.id ?? null, + id: ids[index]!, + subjectId: entry.subjectData.id, type, - user: user - ? { - connect: { id: user.id } - } - : undefined - } + userId: user?.id ?? null + })) }); - return (await this.sessionModel.findUnique({ - include: { - subject: true - }, - where: { id } - }))!; + const created = await this.sessionModel.findMany({ + include: { subject: true }, + where: { id: { in: ids } } + }); + const byId = new Map(created.map((session) => [session.id, session])); + return ids.map((id) => { + const session = byId.get(id); + if (!session) { + throw new InternalServerErrorException(`Failed to read back created session with id: ${id}`); + } + return session; + }); } async deleteById(id: string, { ability }: EntityOperationOptions = {}) { @@ -125,19 +144,4 @@ export class SessionsService { } return session; } - - /** Get the subject if they exist, otherwise create them */ - private async resolveSubject(subjectData: CreateSubjectData) { - this.loggingService.debug({ message: 'Attempting to resolve subject', subjectData }); - let subject: Subject; - try { - subject = await this.subjectsService.findById(subjectData.id); - } catch (err) { - if (!(err instanceof NotFoundException)) { - throw new InternalServerErrorException('Unexpected Error', { cause: err }); - } - subject = await this.subjectsService.create(subjectData); - } - return subject; - } } diff --git a/apps/api/src/subjects/__tests__/subjects.service.spec.ts b/apps/api/src/subjects/__tests__/subjects.service.spec.ts index ea12d89d3..058333570 100644 --- a/apps/api/src/subjects/__tests__/subjects.service.spec.ts +++ b/apps/api/src/subjects/__tests__/subjects.service.spec.ts @@ -47,6 +47,29 @@ describe('SubjectsService', () => { prismaClient = moduleRef.get(PRISMA_CLIENT_TOKEN); }); + describe('addGroupForSubjects', () => { + // The exclusion belongs in the query, not the caller: mongodb arrays admit duplicates, so a + // caller filtering against a list it read earlier would push the id twice under concurrency. + it('should skip subjects already in the group from within the query itself', async () => { + await subjectsService.addGroupForSubjects(['subject-1', 'subject-2'], 'group-1'); + + expect(subjectModel.updateMany.mock.lastCall?.[0]).toMatchObject({ + data: { groupIds: { push: 'group-1' } }, + where: { + id: { in: ['subject-1', 'subject-2'] }, + NOT: { groupIds: { has: 'group-1' } } + } + }); + }); + + it('should associate every subject in one write rather than one per subject', async () => { + await subjectsService.addGroupForSubjects(['subject-1', 'subject-2', 'subject-3'], 'group-1'); + + expect(subjectModel.updateMany).toHaveBeenCalledOnce(); + expect(subjectModel.update).not.toHaveBeenCalled(); + }); + }); + describe('create', () => { it('should call the subject model', async () => { const subject = { diff --git a/apps/api/src/subjects/subjects.service.ts b/apps/api/src/subjects/subjects.service.ts index bde50db32..9a9498137 100644 --- a/apps/api/src/subjects/subjects.service.ts +++ b/apps/api/src/subjects/subjects.service.ts @@ -16,14 +16,21 @@ export class SubjectsService { @InjectModel('Subject') private readonly subjectModel: Model<'Subject'> ) {} - async addGroupForSubject(subjectId: string, groupId: string, { ability }: EntityOperationOptions = {}) { - return this.subjectModel.update({ - data: { - groupIds: { - push: groupId - } - }, - where: { ...accessibleQuery(ability, 'update', 'Subject'), id: subjectId } + /** + * Associate each of the given subjects with a group, in one write rather than one per subject. + * + * The membership check is part of the query rather than done by the caller: mongodb arrays admit + * duplicates, so a caller filtering against a list it read earlier would push the same group id + * twice if a concurrent request associated the subject in between. + */ + async addGroupForSubjects(subjectIds: string[], groupId: string, { ability }: EntityOperationOptions = {}) { + return this.subjectModel.updateMany({ + data: { groupIds: { push: groupId } }, + where: { + AND: [accessibleQuery(ability, 'update', 'Subject')], + id: { in: subjectIds }, + NOT: { groupIds: { has: groupId } } + } }); } @@ -53,42 +60,45 @@ export class SubjectsService { } async createMany(data: CreateSubjectDto[], { ability }: EntityOperationOptions = {}) { - //filter out all duplicate ids that are planned to be created via a set - const noDuplicatesSet = new Set( - data.map((record) => { - return record.id; - }) - ); - - const subjectIds = Array.from(noDuplicatesSet); + // keyed by id so duplicates within the request collapse, keeping the first entry for each + const requested = new Map(); + for (const subject of data) { + if (!requested.has(subject.id)) { + requested.set(subject.id, subject); + } + } //find the list of subject ids that already exist const existingSubjects = await this.subjectModel.findMany({ select: { id: true }, where: { AND: [accessibleQuery(ability, 'read', 'Subject')], - id: { in: subjectIds } + id: { in: Array.from(requested.keys()) } } }); //create a set of existing ids in the database to filter our to-be-created ids with const existingIds = new Set(existingSubjects.map((subj) => subj.id)); - //Filter out records whose IDs already exist - const subjectsToCreateIds = subjectIds.filter((record) => !existingIds.has(record)); - - const subjectsToCreate: CreateSubjectDto[] = subjectsToCreateIds.map((record) => { - return { - id: record - }; - }); + // The whole entry is kept, not just the id: a subject identified by personal info carries + // demographics that would otherwise be dropped on creation. + const subjectsToCreate = Array.from(requested.values()).filter((subject) => !existingIds.has(subject.id)); //if there are none left to create do not follow through with the command if (subjectsToCreate.length < 1) { return subjectsToCreate; } return this.subjectModel.createMany({ - data: subjectsToCreate, + // Named explicitly rather than spread: callers hand this whole Prisma rows, and a spread would + // carry fields that are not the caller's to set. + data: subjectsToCreate.map(({ dateOfBirth, firstName, id, lastName, sex }) => ({ + dateOfBirth, + firstName, + groupIds: [], + id, + lastName, + sex + })), ...accessibleQuery(ability, 'create', 'Subject') }); } diff --git a/testing/src/specs/sessions.spec.ts b/testing/src/specs/sessions.spec.ts new file mode 100644 index 000000000..a9b88d9ca --- /dev/null +++ b/testing/src/specs/sessions.spec.ts @@ -0,0 +1,32 @@ +import { ApiClient } from '../support/api-client'; +import { expect, test } from '../support/fixtures'; + +test.describe('sessions', () => { + // A session was previously given its groupId only when the subject was not already a member of + // that group, so every visit after a subject's first was created without one. A group manager's + // Session rule is `{ groupId: { in: [...] } }`, which made those sessions invisible to them and + // uncounted by every group-scoped query, the dashboard trends included. + // + // Seeded into a group of its own, so the count is exactly what this test created. + test('should keep a returning subject later sessions visible to their group manager', async ({ + api, + apiRequestContext, + uniqueId + }) => { + const group = await api.createGroup(); + const { credentials } = await api.createUser({ basePermissionLevel: 'GROUP_MANAGER', groupIds: [group.id] }); + const accessToken = await ApiClient.login(apiRequestContext, credentials); + const subjectId = `revisit-${uniqueId}`; + + await api.createSession(group.id, { id: subjectId }); + await api.createSession(group.id, { id: subjectId }); + + const response = await apiRequestContext.get(`/api/v1/sessions?groupId=${group.id}`, { + headers: { Authorization: `Bearer ${accessToken}` } + }); + + expect(response.status()).toBe(200); + const sessions = (await response.json()) as { subjectId: string }[]; + expect(sessions.filter((session) => session.subjectId === subjectId)).toHaveLength(2); + }); +}); diff --git a/testing/src/specs/upload.spec.ts b/testing/src/specs/upload.spec.ts new file mode 100644 index 000000000..7df177e44 --- /dev/null +++ b/testing/src/specs/upload.spec.ts @@ -0,0 +1,31 @@ +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('instrument record upload', () => { + // The response was previously every record in the group for the instrument, so a second upload + // leaked the first one's records back to the caller. An earlier upload to the same group and + // instrument is what distinguishes the scoped response from the group-wide one. + test('should answer a batch upload with exactly the records it created', async ({ api, uniqueId }) => { + const group = await api.createGroup(); + const instrumentId = await api.findInstrumentIdByName('DNP_HAPPINESS_QUESTIONNAIRE'); + await api.uploadRecords(group.id, instrumentId, [ + { data: HAPPINESS_RECORD, date: new Date(), subjectId: `upload-${uniqueId}-earlier` } + ]); + + const batch = ['a', 'b', 'c'].map((suffix) => `upload-${uniqueId}-${suffix}`); + const records = await api.uploadRecords( + group.id, + instrumentId, + batch.map((subjectId) => ({ data: HAPPINESS_RECORD, date: new Date(), subjectId })) + ); + + expect(records).toHaveLength(3); + expect(new Set(records.map((record) => record.subjectId))).toStrictEqual(new Set(batch)); + }); +}); diff --git a/testing/src/support/api-client.ts b/testing/src/support/api-client.ts index 6b96e4761..156c881e3 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,16 @@ export class ApiClient { return group; } + /** Creates a session, and with it the subject it names. */ + 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 +83,37 @@ 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. Returns + * the records the api reports created, which the upload contract scopes to this request alone. + */ + async uploadRecords( + groupId: string, + instrumentId: string, + records: UploadRecord[] + ): Promise<{ subjectId: string }[]> { + const data: UploadInstrumentRecordsData = { groupId, instrumentId, records }; + return this.expectJson<{ subjectId: string }[]>( + this.request.post(`${API}/instrument-records/upload`, { data, headers: this.authHeaders }), + 201, + 'upload instrument records' + ); + } + private async expectJson( pending: ReturnType, status: number,