fix(training): defer completion email and share canonical training IDs (#3529)

* fix(training): defer completion email and share canonical training IDs

The portal's complete-training route awaited the completion-certificate
email before responding. That API call resolves the member, renders a PDF
and calls the email provider, so every final-video completion made the
employee wait seconds for work whose outcome they never see — and Node's
fetch has no default request timeout, so an unreachable API could stall
the response for minutes even though the completion was already durable.

Defer it with `after` so it runs once the response is sent, and bound the
request with an AbortSignal timeout so a hung API can't pin the
invocation open. The email path performs no DB or storage writes, only an
in-memory PDF render and a send, so nothing the UI reads afterwards
depends on it having finished.

The route also restated the valid-video-ID list and the HIPAA eligibility
rule that apps/api owns, a fifth copy of constants already duplicated
across both apps and two API modules. Move them to @trycompai/company —
the existing shared-definitions package for api/app/portal — so the two
completion paths cannot disagree about which videos exist or which orgs
may complete HIPAA training.

The two apps' training data files link to the canonical IDs by type only.
They are imported by client components and by a Trigger.dev task, and
@trycompai/company requires @trycompai/db at runtime, so a value
re-export would have pulled Prisma into those bundles. The type-level tie
still fails the build on drift, in both directions: an unknown ID in a
rendered list, or a canonical ID that no longer matches.

Also fixes the pre-existing training-hipaa spec, whose @db mock was never
given frameworkInstance when the HIPAA gate was added, and adds coverage
for the gate's rejection path.

* fix(training): make the canonical/rendered video lists exhaustive both ways

`satisfies readonly CanonicalTrainingVideo[]` only proved that every
rendered video is canonical. The reverse — a canonical ID with no
rendered video — still compiled, and that is the dangerous direction:
completion requires a row for every canonical ID, so adding one without
a matching video would leave employees permanently unable to finish
training.

Key the data by the canonical union instead. `Record<GeneralTrainingVideoId,
...>` is exhaustive by construction, so both drifts now fail the build:
a missing key (TS2741) and an unknown key (TS2353). Verified by
introducing each drift and compiling.

The `Exclude<GeneralTrainingVideoId, (typeof trainingVideos)[number]['id']>`
form would not have worked here — the `readonly TrainingVideo[]`
annotation widens `id` to `string`, so the Exclude resolves to `never`
and the guard passes vacuously.

Author: tofikwest

apps/api/src/frameworks/frameworks-people-score.helper.ts

@@ -1,8 +1,10 @@
 import { BackgroundCheckStatus, db } from '@db';
+import {
+  GENERAL_TRAINING_VIDEO_IDS as GENERAL_TRAINING_IDS,
+  HIPAA_TRAINING_ID,
+} from '@trycompai/company';
 import { filterComplianceMembers } from '../utils/compliance-filters';
 
-const GENERAL_TRAINING_IDS = ['sat-1', 'sat-2', 'sat-3', 'sat-4', 'sat-5'];
-const HIPAA_TRAINING_ID = 'hipaa-sat-1';
 const COMPLETED_BACKGROUND_CHECK_STATUSES = [
   BackgroundCheckStatus.completed,
   BackgroundCheckStatus.completed_with_flags,

apps/api/src/training/training-hipaa.spec.ts

@@ -4,6 +4,9 @@ import { TrainingService } from './training.service';
 jest.mock('@db', () => ({
   db: {
     member: { findFirst: jest.fn(), findUnique: jest.fn() },
+    // markVideoComplete gates hipaa-sat-1 on the org having a HIPAA framework
+    // instance, so the HIPAA paths must be able to resolve one.
+    frameworkInstance: { findFirst: jest.fn() },
     employeeTrainingVideoCompletion: {
       findMany: jest.fn(),
       findFirst: jest.fn(),
@@ -36,6 +39,7 @@ describe('TrainingService — HIPAA training', () => {
 
   describe('markVideoComplete', () => {
     it('should accept hipaa-sat-1 as a valid training ID', async () => {
+      db.frameworkInstance.findFirst.mockResolvedValue({ id: 'frm_1' });
       db.member.findFirst.mockResolvedValue({
         id: 'mem_1',
         organizationId: 'org_1',
@@ -65,6 +69,19 @@ describe('TrainingService — HIPAA training', () => {
       });
     });
 
+    // Mirrors the portal's complete-training route: the two completion paths
+    // must agree on which orgs may complete HIPAA training, or one can create
+    // records — and certificate artifacts — the other would have rejected.
+    it('should reject hipaa-sat-1 when the org has no HIPAA framework', async () => {
+      db.frameworkInstance.findFirst.mockResolvedValue(null);
+
+      await expect(
+        service.markVideoComplete('mem_1', 'org_1', 'hipaa-sat-1'),
+      ).rejects.toThrow(BadRequestException);
+
+      expect(db.employeeTrainingVideoCompletion.create).not.toHaveBeenCalled();
+    });
+
     it('should reject unknown training IDs', async () => {
       await expect(
         service.markVideoComplete('mem_1', 'org_1', 'unknown-id'),

apps/api/src/training/training.service.ts

@@ -5,13 +5,16 @@ import {
   BadRequestException,
 } from '@nestjs/common';
 import { db } from '@db';
+import {
+  GENERAL_TRAINING_VIDEO_IDS as GENERAL_TRAINING_IDS,
+  HIPAA_TRAINING_ID,
+  HIPAA_TRAINING_UNAVAILABLE_MESSAGE,
+  hipaaFrameworkInstanceWhere,
+  isTrainingVideoId,
+} from '@trycompai/company';
 import { TrainingEmailService } from './training-email.service';
 import { TrainingCertificatePdfService } from './training-certificate-pdf.service';
 
-const GENERAL_TRAINING_IDS = ['sat-1', 'sat-2', 'sat-3', 'sat-4', 'sat-5'];
-const HIPAA_TRAINING_ID = 'hipaa-sat-1';
-const ALL_TRAINING_IDS = [...GENERAL_TRAINING_IDS, HIPAA_TRAINING_ID];
-
 @Injectable()
 export class TrainingService {
   private readonly logger = new Logger(TrainingService.name);
@@ -40,19 +43,17 @@ export class TrainingService {
     organizationId: string,
     videoId: string,
   ) {
-    if (!ALL_TRAINING_IDS.includes(videoId)) {
+    if (!isTrainingVideoId(videoId)) {
       throw new BadRequestException(`Invalid video ID: ${videoId}`);
     }
 
     if (videoId === HIPAA_TRAINING_ID) {
       const hipaaInstance = await db.frameworkInstance.findFirst({
-        where: { organizationId, framework: { name: 'HIPAA' } },
+        where: hipaaFrameworkInstanceWhere(organizationId),
         select: { id: true },
       });
       if (!hipaaInstance) {
-        throw new BadRequestException(
-          'HIPAA training is not available for this organization',
-        );
+        throw new BadRequestException(HIPAA_TRAINING_UNAVAILABLE_MESSAGE);
       }
     }
 
@@ -116,7 +117,7 @@ export class TrainingService {
     const completions = await db.employeeTrainingVideoCompletion.findMany({
       where: {
         memberId,
-        videoId: { in: GENERAL_TRAINING_IDS },
+        videoId: { in: [...GENERAL_TRAINING_IDS] },
         completedAt: { not: null },
       },
     });
@@ -140,7 +141,7 @@ export class TrainingService {
     const completions = await db.employeeTrainingVideoCompletion.findMany({
       where: {
         memberId,
-        videoId: { in: GENERAL_TRAINING_IDS },
+        videoId: { in: [...GENERAL_TRAINING_IDS] },
         completedAt: { not: null },
       },
       orderBy: { completedAt: 'desc' },

apps/app/src/lib/data/hipaa-training-content.ts

@@ -1 +1,8 @@
-export const HIPAA_TRAINING_ID = 'hipaa-sat-1';
+import type { TrainingVideoId } from '@trycompai/company';
+
+// Tied to the canonical ID that both training-completion paths validate
+// against. Deliberately a local literal with a type-only link rather than a
+// re-export: client components import this file, and @trycompai/company pulls
+// in @trycompai/db at runtime. The annotation still fails typecheck if the
+// canonical ID ever changes, so the two cannot drift.
+export const HIPAA_TRAINING_ID: Extract<TrainingVideoId, 'hipaa-sat-1'> = 'hipaa-sat-1';

apps/app/src/lib/data/training-videos.ts

@@ -1,3 +1,5 @@
+import type { GeneralTrainingVideoId } from '@trycompai/company';
+
 /**
  * Represents a training video resource that can be used
  * for user education and compliance training.
@@ -15,40 +17,53 @@ export interface TrainingVideo {
   url: string;
 }
 
-export const trainingVideos: readonly TrainingVideo[] = [
-  {
-    id: 'sat-1',
+/**
+ * Videos keyed by the canonical IDs that `@trycompai/company` owns, shared
+ * with the NestJS training service and the portal's complete-training route.
+ *
+ * Keying by the canonical union makes the lists exhaustive in BOTH directions:
+ * a canonical ID with no entry here fails to compile, and an entry that isn't
+ * canonical fails too. Both matter — completion requires a row for every
+ * canonical ID, so a video we never render would leave employees permanently
+ * unable to finish training, while a rendered video the API rejects would 400.
+ *
+ * The import is type-only on purpose: client components and a Trigger.dev task
+ * import this file, and `@trycompai/company` requires `@trycompai/db` at
+ * runtime.
+ */
+const trainingVideosById: Record<GeneralTrainingVideoId, Omit<TrainingVideo, 'id'>> = {
+  'sat-1': {
     title: 'Security Awareness Training - Part 1',
     description: 'Security Awareness Training - Part 1',
     youtubeId: 'N-sBS3uCWB4',
     url: 'https://www.youtube.com/watch?v=N-sBS3uCWB4',
   },
-  {
-    id: 'sat-2',
+  'sat-2': {
     title: 'Security Awareness Training - Part 2',
     description: 'Security Awareness Training - Part 2',
     youtubeId: 'JwQNwhDyXig',
     url: 'https://www.youtube.com/watch?v=JwQNwhDyXig',
   },
-  {
-    id: 'sat-3',
+  'sat-3': {
     title: 'Security Awareness Training - Part 3',
     description: 'Security Awareness Training - Part 3',
     youtubeId: 'fzMNw_-KEGE',
     url: 'https://www.youtube.com/watch?v=fzMNw_-KEGE',
   },
-  {
-    id: 'sat-4',
+  'sat-4': {
     title: 'Security Awareness Training - Part 4',
     description: 'Security Awareness Training - Part 4',
     youtubeId: 'WbpqjH9kI2Y',
     url: 'https://www.youtube.com/watch?v=WbpqjH9kI2Y',
   },
-  {
-    id: 'sat-5',
+  'sat-5': {
     title: 'Security Awareness Training - Part 5',
     description: 'Security Awareness Training - Part 5',
     youtubeId: 'Clvfkm6azDs',
     url: 'https://www.youtube.com/watch?v=Clvfkm6azDs',
   },
-] as const;
+};
+
+export const trainingVideos: readonly TrainingVideo[] = Object.entries(trainingVideosById).map(
+  ([id, video]) => ({ id, ...video }),
+);

apps/portal/src/app/api/portal/complete-training/route.test.ts

@@ -16,6 +16,7 @@ const mocks = vi.hoisted(() => ({
   completionFindMany: vi.fn(),
   logger: vi.fn(),
   fetch: vi.fn(),
+  afterTasks: [] as Array<() => unknown>,
   env: {
     SERVICE_TOKEN_PORTAL: undefined as string | undefined,
     NEXT_PUBLIC_API_URL: 'http://api.test',
@@ -45,6 +46,25 @@ vi.mock('@/env.mjs', () => ({ env: mocks.env }));
 vi.mock('@/utils/logger', () => ({ logger: mocks.logger }));
 vi.stubGlobal('fetch', mocks.fetch);
 
+// The route defers the completion email with `after` so it runs once the
+// response has been sent. Capture the deferred task instead of running it, so
+// tests can prove the response does NOT wait on the email and then drive the
+// email path explicitly.
+vi.mock('next/server', async (importOriginal) => {
+  const actual = await importOriginal<typeof import('next/server')>();
+  return {
+    ...actual,
+    after: (task: () => unknown) => {
+      mocks.afterTasks.push(task);
+    },
+  };
+});
+
+/** Runs whatever the route deferred, mimicking Next flushing `after` tasks. */
+async function flushAfterTasks(): Promise<void> {
+  await Promise.all(mocks.afterTasks.splice(0).map((task) => task()));
+}
+
 import { POST } from './route';
 
 function makeRequest(body: unknown): NextRequest {
@@ -67,6 +87,7 @@ function makeRawRequest(raw: string): NextRequest {
 describe('POST /api/portal/complete-training', () => {
   beforeEach(() => {
     vi.clearAllMocks();
+    mocks.afterTasks.length = 0;
     mocks.env.SERVICE_TOKEN_PORTAL = undefined;
   });
 
@@ -203,9 +224,7 @@ describe('POST /api/portal/complete-training', () => {
   it('rejects an unknown video ID', async () => {
     mocks.getSession.mockResolvedValue({ user: { id: 'user_1' } });
 
-    const res = await POST(
-      makeRequest({ videoId: 'not-a-real-video', organizationId: 'org_1' }),
-    );
+    const res = await POST(makeRequest({ videoId: 'not-a-real-video', organizationId: 'org_1' }));
 
     expect(res.status).toBe(400);
     expect(mocks.memberFindFirst).not.toHaveBeenCalled();
@@ -227,9 +246,7 @@ describe('POST /api/portal/complete-training', () => {
     // No HIPAA framework instance for this org.
     mocks.frameworkInstanceFindFirst.mockResolvedValue(null);
 
-    const res = await POST(
-      makeRequest({ videoId: 'hipaa-sat-1', organizationId: 'org_1' }),
-    );
+    const res = await POST(makeRequest({ videoId: 'hipaa-sat-1', organizationId: 'org_1' }));
 
     expect(res.status).toBe(400);
     expect(mocks.frameworkInstanceFindFirst).toHaveBeenCalledWith({
@@ -257,9 +274,7 @@ describe('POST /api/portal/complete-training', () => {
     };
     mocks.completionUpsert.mockResolvedValue(record);
 
-    const res = await POST(
-      makeRequest({ videoId: 'hipaa-sat-1', organizationId: 'org_1' }),
-    );
+    const res = await POST(makeRequest({ videoId: 'hipaa-sat-1', organizationId: 'org_1' }));
 
     expect(res.status).toBe(200);
     expect(mocks.completionUpsert).toHaveBeenCalledWith({
@@ -300,15 +315,24 @@ describe('POST /api/portal/complete-training', () => {
     // dev-only `logger` util would have swallowed it outside development).
     const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
 
-    const res = await POST(
-      makeRequest({ videoId: 'hipaa-sat-1', organizationId: 'org_1' }),
-    );
+    const res = await POST(makeRequest({ videoId: 'hipaa-sat-1', organizationId: 'org_1' }));
 
     // Completion is persisted regardless — the email is best-effort.
     expect(res.status).toBe(200);
+    // ...and the response did not wait on it: the API call that renders the
+    // certificate and sends the mail only happens once `after` is flushed.
+    expect(mocks.fetch).not.toHaveBeenCalled();
+
+    await flushAfterTasks();
+
     expect(mocks.fetch).toHaveBeenCalledWith(
       'http://api.test/v1/training/send-hipaa-completion-email',
-      expect.objectContaining({ method: 'POST' }),
+      // The request is bounded: Node's fetch otherwise waits ~5 minutes, which
+      // would keep the invocation alive long after the response was sent.
+      expect.objectContaining({
+        method: 'POST',
+        signal: expect.any(AbortSignal),
+      }),
     );
     // The HTTP failure must surface (in prod too), not be swallowed as success.
     expect(errorSpy).toHaveBeenCalledWith(