From affe204ae613c52057d21306bdf2f07aa50cfd38 Mon Sep 17 00:00:00 2001 From: BigSimmo Date: Sat, 17 Oct 2026 22:07:36 +0800 Subject: [PATCH 1/3] fix(teaching,cme): address CodeRabbit review on #3614 - Dual sign-off reads accept only rows marked ratings.kind = dual_sign_off, so other registrar_assessments rows are never read or rewritten as assessments. - Slot times must be ISO 8601 with an offset. - Role-less booking listings are filtered in the database (own slots, plus open slots for a named service) instead of scanning every booking. - Part-time EPA minimum rounds up to a whole number. --- .../teaching/assessments/bookings/route.ts | 4 +-- src/lib/cme/training-assessments.ts | 3 ++- src/lib/teaching/assessments/dual-sign-off.ts | 23 +++++++++++------ .../assessments/supervisor-bookings.ts | 11 ++++++++ tests/teaching-dual-sign-off-durable.test.ts | 25 +++++++++++++++++++ 5 files changed, 56 insertions(+), 10 deletions(-) diff --git a/src/app/api/teaching/assessments/bookings/route.ts b/src/app/api/teaching/assessments/bookings/route.ts index 320dd77bf1..0ef5f61b29 100644 --- a/src/app/api/teaching/assessments/bookings/route.ts +++ b/src/app/api/teaching/assessments/bookings/route.ts @@ -61,8 +61,8 @@ const bookingActionSchema = z.discriminatedUnion("action", [ supervisorId: z.string().uuid().optional(), traineeId: z.string().uuid().optional(), title: z.string().min(2).max(200).optional(), - startsAt: z.string().min(1), - endsAt: z.string().min(1), + startsAt: z.string().datetime({ offset: true }), + endsAt: z.string().datetime({ offset: true }), locationOrLink: z.string().min(1).max(500).optional(), assessmentType: assessmentTypeSchema.optional(), notes: z.string().max(2000).optional(), diff --git a/src/lib/cme/training-assessments.ts b/src/lib/cme/training-assessments.ts index e6045c3f77..2111970f17 100644 --- a/src/lib/cme/training-assessments.ts +++ b/src/lib/cme/training-assessments.ts @@ -327,7 +327,8 @@ export function buildOwnerTrainingAssessments( // The deadline is the current rotation's own end date; with none recorded the page shows no date // rather than borrowing an unrelated milestone's due date. const rotationEndsOn = currentRotation?.endsOn ?? null; - const minimumEpas = currentRotation ? 2 * currentRotation.fte : 2; + // Pro rata for part-time, rounded up so the page never asks for a fraction of an EPA. + const minimumEpas = currentRotation ? Math.max(1, Math.ceil(2 * currentRotation.fte)) : 2; const epas: TrainingEpa[] = epaRecords.map((record) => ({ id: record.id, diff --git a/src/lib/teaching/assessments/dual-sign-off.ts b/src/lib/teaching/assessments/dual-sign-off.ts index d8909fa510..82e9c0fd83 100644 --- a/src/lib/teaching/assessments/dual-sign-off.ts +++ b/src/lib/teaching/assessments/dual-sign-off.ts @@ -104,6 +104,8 @@ export type SupervisorSignOffInput = { }; export type RegistrarAssessmentRatingsPayload = { + /** Marks rows written by the dual sign-off flow; reads accept nothing else. */ + kind: typeof DUAL_SIGN_OFF_KIND; serviceId: string; clinicalSetting: string; traineeReflection: string; @@ -115,7 +117,6 @@ export type RegistrarAssessmentRatingsPayload = { supervisorSignedAt: string | null; revisionReason: string | null; cancelReason: string | null; - kind?: string; }; export type RegistrarAssessmentRow = { @@ -164,6 +165,8 @@ export type RegistrarAssessmentsTableClient = { from(table: "registrar_assessments"): RegistrarAssessmentsQueryBuilder; }; +export const DUAL_SIGN_OFF_KIND = "dual_sign_off" as const; + const ASSESSMENT_SELECT_COLUMNS = "id, registrar_id, assessor_id, category, title, ratings, feedback, status, created_at, updated_at"; @@ -231,6 +234,7 @@ export function assessmentRecordToRow( category: RegistrarAssessmentCategory = "wba", ): RegistrarAssessmentRow { const ratingsPayload: RegistrarAssessmentRatingsPayload = { + kind: DUAL_SIGN_OFF_KIND, serviceId: record.serviceId, clinicalSetting: record.clinicalSetting, traineeReflection: record.traineeReflection, @@ -297,12 +301,15 @@ export function rowToAssessmentRecord(row: RegistrarAssessmentRow): AssessmentRe }; } -function isSupervisorBookingRow(row: RegistrarAssessmentRow): boolean { +/** + * Only rows the dual sign-off flow wrote (`ratings.kind = "dual_sign_off"`) are assessments here. + * Bookings and any other registrar_assessments rows are never read as, or rewritten into, one. + */ +function isDualSignOffRow(row: RegistrarAssessmentRow): boolean { return Boolean( - row.category === "supervisor_review" && row.ratings && typeof row.ratings === "object" && - (row.ratings as Record).kind === "supervisor_booking", + (row.ratings as Record).kind === DUAL_SIGN_OFF_KIND, ); } @@ -753,7 +760,7 @@ export class DurableClinicalAssessmentRepository { throw assessmentDatabaseError(error); } - if (!data || isSupervisorBookingRow(data)) { + if (!data || !isDualSignOffRow(data)) { throw new AssessmentNotFoundError(id); } @@ -854,6 +861,7 @@ export class DurableClinicalAssessmentRepository { .from("registrar_assessments") .select(ASSESSMENT_SELECT_COLUMNS) .eq("assessor_id", supervisorId) + .eq("ratings->>kind", DUAL_SIGN_OFF_KIND) .order("created_at", { ascending: false }); if (error) { @@ -862,7 +870,7 @@ export class DurableClinicalAssessmentRepository { const results: SafeAssessmentView[] = []; for (const row of data ?? []) { - if (isSupervisorBookingRow(row)) continue; + if (!isDualSignOffRow(row)) continue; const record = rowToAssessmentRecord(row); if (!filterState || record.state === filterState) { results.push(projectAssessmentForActor(record, supervisorId)); @@ -876,6 +884,7 @@ export class DurableClinicalAssessmentRepository { .from("registrar_assessments") .select(ASSESSMENT_SELECT_COLUMNS) .eq("registrar_id", traineeId) + .eq("ratings->>kind", DUAL_SIGN_OFF_KIND) .order("created_at", { ascending: false }); if (error) { @@ -884,7 +893,7 @@ export class DurableClinicalAssessmentRepository { const results: SafeAssessmentView[] = []; for (const row of data ?? []) { - if (isSupervisorBookingRow(row)) continue; + if (!isDualSignOffRow(row)) continue; const record = rowToAssessmentRecord(row); results.push(projectAssessmentForActor(record, traineeId)); } diff --git a/src/lib/teaching/assessments/supervisor-bookings.ts b/src/lib/teaching/assessments/supervisor-bookings.ts index da17cffa6c..267d6eee35 100644 --- a/src/lib/teaching/assessments/supervisor-bookings.ts +++ b/src/lib/teaching/assessments/supervisor-bookings.ts @@ -570,6 +570,17 @@ export class DurableSupervisorBookingRepository { } async listBookingsForActor(filter: ListBookingsFilter): Promise { + if (!filter.role) { + // No role: the caller's own slots either side, plus open slots when a service is named. Each + // part is filtered in the database, so the listing never scans other people's bookings. + const parts = await Promise.all([ + this.listBookingsForActor({ ...filter, role: "supervisor" }), + this.listBookingsForActor({ ...filter, role: "trainee" }), + filter.serviceId ? this.listBookingsForActor({ ...filter, role: "open" }) : Promise.resolve([]), + ]); + const byId = new Map(parts.flat().map((slot) => [slot.id, slot])); + return [...byId.values()].sort((a, b) => Date.parse(a.startsAt) - Date.parse(b.startsAt)); + } const buildQuery = () => { let query = this.db .from("registrar_assessments") diff --git a/tests/teaching-dual-sign-off-durable.test.ts b/tests/teaching-dual-sign-off-durable.test.ts index 3cb430a902..08ebc425e6 100644 --- a/tests/teaching-dual-sign-off-durable.test.ts +++ b/tests/teaching-dual-sign-off-durable.test.ts @@ -414,6 +414,31 @@ describe("Durable Teaching Assessments & Supervisor Bookings (registrar_assessme expect(mockDb.rows.size).toBe(0); }); + it("never reads a registrar_assessments row the dual sign-off flow did not write", async () => { + vi.stubEnv("NODE_ENV", "production"); + const foreignId = "77777777-7777-4777-8777-777777777777"; + mockDb.rows.set(foreignId, { + id: foreignId, + registrar_id: TRAINEE_ID, + assessor_id: SUPERVISOR_ID, + category: "epa", + title: "Imported EPA record", + ratings: { state: "approved", somethingElse: true }, + feedback: null, + status: "draft", + created_at: "2026-10-01T00:00:00.000Z", + updated_at: "2026-10-01T00:00:00.000Z", + }); + + mocks.user = { id: TRAINEE_ID }; + expect((await getDualSignOff(makeDualSignOffGet(`id=${foreignId}`))).status).toBe(404); + const update = await postDualSignOff( + makeDualSignOffPost({ action: "update_draft", assessmentId: foreignId, title: "Overwrite" }), + ); + expect(update.status).toBe(404); + expect(mockDb.rows.get(foreignId)?.ratings).toEqual({ state: "approved", somethingElse: true }); + }); + it("fails closed with 503 assessment_store_unavailable in production when admin client lacks durable table access", async () => { vi.stubEnv("NODE_ENV", "production"); mocks.adminClient = {}; From 9b0d0286721fb0f8df7862386674c8ab282e64ed Mon Sep 17 00:00:00 2001 From: "qodo-code-review[bot]" <151058649+qodo-code-review[bot]@users.noreply.github.com> Date: Sat, 17 Oct 2026 15:41:36 +0000 Subject: [PATCH 2/3] fix: Preserve access to legacy dual sign-offs --- src/lib/teaching/assessments/dual-sign-off.ts | 35 ++++++++++++++----- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/src/lib/teaching/assessments/dual-sign-off.ts b/src/lib/teaching/assessments/dual-sign-off.ts index 82e9c0fd83..37ef85777e 100644 --- a/src/lib/teaching/assessments/dual-sign-off.ts +++ b/src/lib/teaching/assessments/dual-sign-off.ts @@ -302,17 +302,36 @@ export function rowToAssessmentRecord(row: RegistrarAssessmentRow): AssessmentRe } /** - * Only rows the dual sign-off flow wrote (`ratings.kind = "dual_sign_off"`) are assessments here. - * Bookings and any other registrar_assessments rows are never read as, or rewritten into, one. + * Rows written by the current dual sign-off flow carry an explicit discriminator. + * + * Before that discriminator was added, dual sign-off rows had the same payload + * below without `kind`. Keep accepting only that recognisable legacy shape so + * those assessments can still be read and updated; in particular, do not treat + * every row with a missing kind as a dual sign-off assessment. */ -function isDualSignOffRow(row: RegistrarAssessmentRow): boolean { - return Boolean( - row.ratings && - typeof row.ratings === "object" && - (row.ratings as Record).kind === DUAL_SIGN_OFF_KIND, +function isLegacyDualSignOffRow(row: RegistrarAssessmentRow): boolean { + if (!row.ratings || typeof row.ratings !== "object") return false; + const ratings = row.ratings as Record; + if ("kind" in ratings || typeof ratings.serviceId !== "string") return false; + + return ( + typeof ratings.clinicalSetting === "string" && + typeof ratings.traineeReflection === "string" && + ratings.traineeSelfRatings !== null && + typeof ratings.traineeSelfRatings === "object" && + typeof ratings.state === "string" && + VALID_LIFECYCLE_STATES.has(ratings.state as AssessmentLifecycleState) && + !("startsAt" in ratings) && + !("bookingState" in ratings) ); } +function isDualSignOffRow(row: RegistrarAssessmentRow): boolean { + if (!row.ratings || typeof row.ratings !== "object") return false; + const ratings = row.ratings as Record; + return ratings.kind === DUAL_SIGN_OFF_KIND || isLegacyDualSignOffRow(row); +} + function assessmentDatabaseError(error: DbErrorLike | null): PublicApiError { if (error?.code === "22P02") return new PublicApiError("Invalid assessment identifier.", 400, { code: "invalid_id" }); if (error?.code === "23503") @@ -861,7 +880,6 @@ export class DurableClinicalAssessmentRepository { .from("registrar_assessments") .select(ASSESSMENT_SELECT_COLUMNS) .eq("assessor_id", supervisorId) - .eq("ratings->>kind", DUAL_SIGN_OFF_KIND) .order("created_at", { ascending: false }); if (error) { @@ -884,7 +902,6 @@ export class DurableClinicalAssessmentRepository { .from("registrar_assessments") .select(ASSESSMENT_SELECT_COLUMNS) .eq("registrar_id", traineeId) - .eq("ratings->>kind", DUAL_SIGN_OFF_KIND) .order("created_at", { ascending: false }); if (error) { From b185ab53e963dda23de6e727792e61bf0958d989 Mon Sep 17 00:00:00 2001 From: BigSimmo Date: Sat, 17 Oct 2026 23:53:23 +0800 Subject: [PATCH 3/3] test(teaching): keep pre-marker dual sign-off rows readable, listable and updatable Covers Qodo's legacy-shape fallback for assessments written by #3614 before ratings.kind existed; an update re-writes them with the marker. --- tests/teaching-dual-sign-off-durable.test.ts | 39 ++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/tests/teaching-dual-sign-off-durable.test.ts b/tests/teaching-dual-sign-off-durable.test.ts index 08ebc425e6..d601a04650 100644 --- a/tests/teaching-dual-sign-off-durable.test.ts +++ b/tests/teaching-dual-sign-off-durable.test.ts @@ -439,6 +439,45 @@ describe("Durable Teaching Assessments & Supervisor Bookings (registrar_assessme expect(mockDb.rows.get(foreignId)?.ratings).toEqual({ state: "approved", somethingElse: true }); }); + it("keeps a pre-marker dual sign-off row (no ratings.kind) readable, listable and updatable", async () => { + vi.stubEnv("NODE_ENV", "production"); + const legacyId = "88888888-8888-4888-8888-888888888888"; + mockDb.rows.set(legacyId, { + id: legacyId, + registrar_id: TRAINEE_ID, + assessor_id: SUPERVISOR_ID, + category: "wba", + title: "Legacy assessment", + ratings: { + serviceId: SERVICE_ID, + clinicalSetting: "Adult Acute Inpatient Unit", + traineeReflection: "Reflection.", + traineeSelfRatings: { 1: 3 }, + traineeSignedAt: null, + state: "draft", + supervisorRatings: null, + globalRecommendation: null, + supervisorSignedAt: null, + revisionReason: null, + cancelReason: null, + }, + feedback: null, + status: "draft", + created_at: "2026-10-17T14:30:00.000Z", + updated_at: "2026-10-17T14:30:00.000Z", + }); + + mocks.user = { id: TRAINEE_ID }; + expect((await getDualSignOff(makeDualSignOffGet(`id=${legacyId}`))).status).toBe(200); + const list = await getDualSignOff(makeDualSignOffGet("role=trainee")); + expect(JSON.stringify(await list.json())).toContain(legacyId); + const update = await postDualSignOff( + makeDualSignOffPost({ action: "update_draft", assessmentId: legacyId, title: "Legacy assessment edited" }), + ); + expect(update.status).toBe(200); + expect((mockDb.rows.get(legacyId)?.ratings as Record).kind).toBe("dual_sign_off"); + }); + it("fails closed with 503 assessment_store_unavailable in production when admin client lacks durable table access", async () => { vi.stubEnv("NODE_ENV", "production"); mocks.adminClient = {};