Merge pull request #246 from RJK134/claude/phase-21a-teaching-assignment

Author: RJK134

CLAUDE.md

@@ -48,7 +48,7 @@ SJMS 2.5 is a **unified enterprise student records system** for UK higher educat
 
 ## Target Metrics
 
-196 Prisma models · 129 pages · 271 API endpoints (57 routers) · 36 roles · 15 n8n workflows
+196 Prisma models · 129 pages · 271 API endpoints (58 routers) · 36 roles · 15 n8n workflows
 Sub-2s page loads · Sub-500ms API (p95) · WCAG 2.1 AA · British English throughout
 
 ## Critical Rules for Claude

package-lock.json

@@ -67,7 +67,7 @@ if (modelCount !== expectedModels) {
 // router → 57. Update when *.router.ts files change.
 
 const routerFiles = listFiles('server/src/api', (p) => p.endsWith('.router.ts'));
-const expectedRouters = 57;
+const expectedRouters = 58;
 if (routerFiles.length !== expectedRouters) {
   fail(
     'Flat router count',

scripts/check-docs-truth.mjs

@@ -0,0 +1,156 @@
+import { describe, it, expect, vi, beforeEach } from 'vitest';
+import { ForbiddenError } from '../../utils/errors';
+
+// Phase 21A — coverage for the `staffId` branch of `scopeToUser` and its
+// integration with `resolveIdentity`'s new `staffId` projection. The
+// existing prisma mock surface is reused but extended to allow the
+// Person → Staff include to return a value.
+
+vi.mock('../../utils/prisma', () => ({
+  default: {
+    person: { findFirst: vi.fn() },
+  },
+}));
+
+import { scopeToUser } from '../../middleware/data-scope';
+import prisma from '../../utils/prisma';
+
+const mockedPersonFind = vi.mocked((prisma as any).person.findFirst);
+
+// Each test uses a unique sub because resolveIdentity caches by sub and
+// the cache is module-scoped — reusing a sub leaks the prior test's
+// resolved identity into the next test's lookup.
+let subCounter = 0;
+const baseUser = (overrides: any = {}) => ({
+  sub: `kc-sub-${++subCounter}`,
+  email: 'user@example.com',
+  realm_access: { roles: [] },
+  resource_access: {},
+  ...overrides,
+});
+
+const makeReq = (user: any): any => ({
+  user,
+  query: {},
+});
+
+const makeNext = () => vi.fn();
+
+beforeEach(() => {
+  vi.clearAllMocks();
+});
+
+describe('scopeToUser("staffId")', () => {
+  it('admin staff bypass — no scope applied', async () => {
+    const req = makeReq(baseUser({ realm_access: { roles: ['system_admin'] } }));
+    const next = makeNext();
+
+    await scopeToUser('staffId')(req, {} as any, next);
+
+    expect(next).toHaveBeenCalledWith();
+    expect(req.query.staffId).toBeUndefined();
+    expect(mockedPersonFind).not.toHaveBeenCalled();
+  });
+
+  it('teaching staff: injects resolved staffId into the query', async () => {
+    mockedPersonFind.mockResolvedValue({
+      id: 'per-1',
+      student: null,
+      staff: { id: 'sta-7' },
+    } as any);
+
+    const req = makeReq(baseUser({ realm_access: { roles: ['lecturer'] } }));
+    const next = makeNext();
+
+    await scopeToUser('staffId')(req, {} as any, next);
+
+    expect(req.query.staffId).toBe('sta-7');
+    expect(next).toHaveBeenCalledWith();
+  });
+
+  it('teaching staff WITHOUT a linked Staff record: denied with ForbiddenError', async () => {
+    mockedPersonFind.mockResolvedValue({
+      id: 'per-1',
+      student: null,
+      staff: null,
+    } as any);
+
+    const req = makeReq(baseUser({ realm_access: { roles: ['lecturer'] } }));
+    const next = makeNext();
+
+    await scopeToUser('staffId')(req, {} as any, next);
+
+    expect(req.query.staffId).toBeUndefined();
+    expect(next).toHaveBeenCalledWith(expect.any(ForbiddenError));
+  });
+
+  it('teaching staff on a studentId-filter route: bypasses (no staff scope applied)', async () => {
+    const req = makeReq(baseUser({ realm_access: { roles: ['lecturer'] } }));
+    const next = makeNext();
+
+    await scopeToUser('studentId')(req, {} as any, next);
+
+    expect(req.query.studentId).toBeUndefined();
+    expect(next).toHaveBeenCalledWith();
+    expect(mockedPersonFind).not.toHaveBeenCalled();
+  });
+
+  it('student hitting a staffId route: defensively denied with ForbiddenError', async () => {
+    mockedPersonFind.mockResolvedValue({
+      id: 'per-stu-1',
+      student: { id: 'stu-1' },
+      staff: null,
+    } as any);
+
+    const req = makeReq(baseUser({ realm_access: { roles: ['student'] } }));
+    const next = makeNext();
+
+    await scopeToUser('staffId')(req, {} as any, next);
+
+    expect(next).toHaveBeenCalledWith(expect.any(ForbiddenError));
+  });
+});
+
+describe('scopeToUser("studentId") — unchanged behaviour', () => {
+  it('students still get studentId injected', async () => {
+    mockedPersonFind.mockResolvedValue({
+      id: 'per-stu-1',
+      student: { id: 'stu-1' },
+      staff: null,
+    } as any);
+
+    const req = makeReq(baseUser({ realm_access: { roles: ['student'] } }));
+    const next = makeNext();
+
+    await scopeToUser('studentId')(req, {} as any, next);
+
+    expect(req.query.studentId).toBe('stu-1');
+    expect(next).toHaveBeenCalledWith();
+  });
+
+  it('applicants still get personId injected on personId routes', async () => {
+    mockedPersonFind.mockResolvedValue({
+      id: 'per-app-1',
+      student: null,
+      staff: null,
+    } as any);
+
+    const req = makeReq(baseUser({ realm_access: { roles: ['applicant'] } }));
+    const next = makeNext();
+
+    await scopeToUser('personId')(req, {} as any, next);
+
+    expect(req.query.personId).toBe('per-app-1');
+    expect(next).toHaveBeenCalledWith();
+  });
+
+  it('unauthenticated requests are passed through to next', async () => {
+    const req: any = { query: {} };
+    const next = makeNext();
+
+    await scopeToUser('staffId')(req, {} as any, next);
+
+    expect(next).toHaveBeenCalledWith();
+    expect(req.query.staffId).toBeUndefined();
+  });
+});

server/src/__tests__/unit/data-scope-staff.test.ts

@@ -0,0 +1,168 @@
+import { describe, it, expect, vi, beforeEach } from 'vitest';
+import { NotFoundError } from '../../utils/errors';
+
+// ── Mock dependencies before importing the service under test ──────────────
+vi.mock('../../repositories/moduleDelivery.repository', () => ({
+  list: vi.fn(),
+  getById: vi.fn(),
+  create: vi.fn(),
+  update: vi.fn(),
+  remove: vi.fn(),
+  findByModuleStaffYear: vi.fn(),
+  findModuleIdsByStaff: vi.fn(),
+}));
+vi.mock('../../utils/audit', () => ({ logAudit: vi.fn() }));
+vi.mock('../../utils/webhooks', () => ({ emitEvent: vi.fn() }));
+
+import * as service from '../../api/module-deliveries/module-deliveries.service';
+import * as repo from '../../repositories/moduleDelivery.repository';
+import { logAudit } from '../../utils/audit';
+import { emitEvent } from '../../utils/webhooks';
+
+const mockedRepo = vi.mocked(repo);
+const mockedLogAudit = vi.mocked(logAudit);
+const mockedEmitEvent = vi.mocked(emitEvent);
+
+const fakeReq = { ip: '127.0.0.1', user: {}, get: vi.fn() } as any;
+
+const fakeRow = {
+  id: 'mdl-1',
+  moduleId: 'mod-1',
+  staffId: 'sta-1',
+  academicYear: '2025/26',
+  capacity: 30,
+  notes: null,
+  createdAt: new Date(),
+  updatedAt: new Date(),
+};
+
+const findEvent = (eventName: string) =>
+  mockedEmitEvent.mock.calls
+    .map((c) => (typeof c[0] === 'object' ? c[0] : null))
+    .find((e) => e && (e as { event?: string }).event === eventName);
+
+beforeEach(() => {
+  vi.clearAllMocks();
+});
+
+describe('module-deliveries.service.list', () => {
+  it('forwards all filters to the repository unchanged', async () => {
+    mockedRepo.list.mockResolvedValue({ data: [], total: 0, hasMore: false, nextCursor: null } as any);
+
+    await service.list({
+      cursor: 'c1',
+      limit: 50,
+      sort: 'academicYear',
+      order: 'asc',
+      moduleId: 'mod-1',
+      staffId: 'sta-1',
+      academicYear: '2025/26',
+    });
+
+    expect(mockedRepo.list).toHaveBeenCalledWith(
+      { moduleId: 'mod-1', staffId: 'sta-1', academicYear: '2025/26' },
+      { cursor: 'c1', limit: 50, sort: 'academicYear', order: 'asc' },
+    );
+  });
+});
+
+describe('module-deliveries.service.getById', () => {
+  it('returns the row when present', async () => {
+    mockedRepo.getById.mockResolvedValue(fakeRow as any);
+    const result = await service.getById('mdl-1');
+    expect(result).toBe(fakeRow);
+  });
+
+  it('throws NotFoundError when the row is absent', async () => {
+    mockedRepo.getById.mockResolvedValue(null);
+    await expect(service.getById('missing')).rejects.toThrow(NotFoundError);
+  });
+});
+
+describe('module-deliveries.service.create', () => {
+  it('persists, audits CREATE, and emits module_delivery.created', async () => {
+    mockedRepo.create.mockResolvedValue(fakeRow as any);
+
+    const result = await service.create(
+      { moduleId: 'mod-1', staffId: 'sta-1', academicYear: '2025/26', capacity: 30 } as any,
+      'user-1',
+      fakeReq,
+    );
+
+    expect(result).toBe(fakeRow);
+    expect(mockedLogAudit).toHaveBeenCalledWith(
+      'ModuleDelivery',
+      'mdl-1',
+      'CREATE',
+      'user-1',
+      null,
+      fakeRow,
+      fakeReq,
+    );
+    const ev = findEvent('module_delivery.created') as any;
+    expect(ev).toBeTruthy();
+    expect(ev.entityType).toBe('ModuleDelivery');
+    expect(ev.entityId).toBe('mdl-1');
+    expect(ev.actorId).toBe('user-1');
+    expect(ev.data).toEqual({ moduleId: 'mod-1', staffId: 'sta-1', academicYear: '2025/26' });
+  });
+});
+
+describe('module-deliveries.service.update', () => {
+  it('reads previous, persists, audits UPDATE, and emits module_delivery.updated', async () => {
+    mockedRepo.getById.mockResolvedValue(fakeRow as any);
+    const next = { ...fakeRow, capacity: 40 };
+    mockedRepo.update.mockResolvedValue(next as any);
+
+    const result = await service.update('mdl-1', { capacity: 40 } as any, 'user-1', fakeReq);
+
+    expect(result).toEqual(next);
+    expect(mockedLogAudit).toHaveBeenCalledWith(
+      'ModuleDelivery',
+      'mdl-1',
+      'UPDATE',
+      'user-1',
+      fakeRow,
+      next,
+      fakeReq,
+    );
+    expect(findEvent('module_delivery.updated')).toBeTruthy();
+  });
+
+  it('throws NotFoundError when updating an unknown id', async () => {
+    mockedRepo.getById.mockResolvedValue(null);
+    await expect(
+      service.update('missing', { capacity: 40 } as any, 'user-1', fakeReq),
+    ).rejects.toThrow(NotFoundError);
+    expect(mockedRepo.update).not.toHaveBeenCalled();
+  });
+});
+
+describe('module-deliveries.service.remove', () => {
+  it('reads previous, hard-deletes, audits DELETE, and emits module_delivery.deleted', async () => {
+    mockedRepo.getById.mockResolvedValue(fakeRow as any);
+    mockedRepo.remove.mockResolvedValue(fakeRow as any);
+
+    await service.remove('mdl-1', 'user-1', fakeReq);
+
+    expect(mockedRepo.remove).toHaveBeenCalledWith('mdl-1');
+    expect(mockedLogAudit).toHaveBeenCalledWith(
+      'ModuleDelivery',
+      'mdl-1',
+      'DELETE',
+      'user-1',
+      fakeRow,
+      null,
+      fakeReq,
+    );
+    const ev = findEvent('module_delivery.deleted') as any;
+    expect(ev).toBeTruthy();
+    expect(ev.data).toEqual({ moduleId: 'mod-1', staffId: 'sta-1', academicYear: '2025/26' });
+  });
+
+  it('throws NotFoundError when removing an unknown id', async () => {
+    mockedRepo.getById.mockResolvedValue(null);
+    await expect(service.remove('missing', 'user-1', fakeReq)).rejects.toThrow(NotFoundError);
+    expect(mockedRepo.remove).not.toHaveBeenCalled();
+  });
+});