From 52b6d1d6df2930bb1b15e778a66bb49f55ce8d68 Mon Sep 17 00:00:00 2001 From: Chris Kehayias Date: Fri, 21 Aug 2026 07:09:52 -0400 Subject: [PATCH 1/2] test: raise non-UI unit coverage from 71.9% to 99.5% statements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Push unit test coverage for non-UI functional code past the 90% target, fix the measurement that was hiding the gap, and document (without fixing) the defects the old coverage was pointing away from. Coverage — non-UI functional code (all src/**/*.ts plus src/contexts/*.tsx, excluding generated models, codegen scripts, and tests; 760 statements): Statements 71.93% -> 99.47% (756/760) Branches 70.73% -> 95.49% (297/311) Functions 72.28% -> 98.20% (164/167) Lines 72.93% -> 99.72% (738/740) 279 tests / 21 files -> 419 tests / 30 files, still ~3s. tsc --noEmit and eslint . clean. New test files (9) file.service.test.ts 35 tests (+88 stmts) procedure.service.test.ts 16 tests (+27) communication.service.test.ts 13 tests (+25) metadata.service.test.ts 8 tests (+12) domain.service.test.ts 8 tests (+11) client-credentials.test.ts 5 tests (+7) lib/utils.test.ts 7 tests lib/auth-client.test.ts 4 tests shared-actions/domain.test.ts 3 tests The five untested MP sub-services were 163 of the 213 missing statements. All five share the ensureValidToken -> getHttpClient -> error-wrap shape that table.service.ts already had covered, so the harness was copy-adaptable. Extended (6): provider.test.ts 9 -> 24 (the CommunicationService and FileService pass-throughs were entirely untested; provider.ts 60% -> 100%), auth.test.ts 12 -> 25, contact-logs/actions.test.ts 19 -> 24, domainTimezoneService.test.ts 16 -> 18, http-client.test.ts 26 -> 28, plus branch fills in contact-lookup-details/actions, user-menu/actions, and user-context. Source change (one) src/lib/auth.ts: extract the customSession callback body to an exported enrichSessionUser(user, session). Behavior identical. The better-auth plugin closes over its callback and never exposes it, so this was the only way to unit test the logic short of driving a full getSession() through the whole auth stack. auth.ts 44% -> 96%. Removed 7 tautological tests The old auth.test.ts "Name Splitting" and "Session Structure" blocks re-implemented the transformation inside the test body and asserted against their own copy — they would have passed with the customSession callback deleted outright, which is why auth.ts reported 18.5% while the file held 12 tests. Rewritten to call the real export. Verified by mutation: replacing firstName with a constant fails 6 tests; the old versions failed none. Measurement vitest.config.ts gains an explicit coverage.include — without it, v8 reports only on files some test imported, so untested files silently leave the denominator (the repo read 71.6% while true statement coverage was 32.7%). Adds per-glob coverage.thresholds; a breach fails the run with exit 1, which was verified by deliberately breaching one rather than only confirming a pass. Excludes components/ui/ (thin Radix wrappers) and the codegen scripts (dev-only tooling). Feature components stay visible in the report but ungated. Two Vitest 4 notes for whoever edits this next: coverage.all no longer exists and setting it is a tsc error (include replaces it), and --reporter=basic was removed. Deferred to .claude/TODO — no behavior changes in this commit Eight files, one per issue. Six were already known and are still present: numeric IDs interpolated into MP filters unsanitized (confirmed exploitable, in a file at 100%/100% coverage), searchContacts and getCurrentUserProfile as 'use server' actions with no session check, contact-log actions that authenticate but never authorize, an N+1 lookup fetch, and client.ts discarding expires_in. Two found while writing these tests: - contact-logs/actions.ts re-implements the dp_Users User_ID lookup that SessionContextService exists to serve, and throws when it cannot resolve — contradicting that service's log-and-proceed policy for unattributed writes. - contact-logs.tsx: 602 lines at 0%, driving every MP write a user can reach. Out of scope for a non-UI target, but the highest-value gap left. Tests that pin behavior a TODO proposes changing carry a comment naming the TODO file, so the assertion reads as a snapshot rather than a specification. Docs .claude/references/testing.md: the 95.39% / 228 tests / 19 files claims were wrong on all three counts and not reproducible under any configuration. Rewritten against measured numbers, with the new mock patterns (MP sub-service harness, fetch stubbing, FormData assertions) and an explicit rule against asserting on a re-implementation of the subject. .claude/docs/TestCoverage.md: rewritten as current state, with each §5 finding linked to its TODO file. No Ministry Platform data was read or written. Every test mocks at a boundary above the network — which matters most for communication.service (sends real email/SMS in production), procedure.service (procs can mutate), and the file and table write paths. Co-Authored-By: Claude Opus 5 (1M context) --- ...-actions-authenticate-but-not-authorize.md | 49 ++ ...-actions-bypass-session-context-service.md | 79 +++ .../TODO/contact-logs-component-untested.md | 44 ++ ...lient-token-lifetime-ignores-expires-in.md | 54 ++ .../TODO/mp-filter-injection-numeric-ids.md | 68 +++ .../TODO/n-plus-1-contact-log-types-lookup.md | 56 ++ ...-action-search-contacts-unauthenticated.md | 54 ++ ...ver-action-user-profile-unauthenticated.md | 56 ++ .claude/docs/TestCoverage.md | 427 +++++++------- .claude/references/testing.md | 271 ++++++++- src/auth.test.ts | 301 +++++++--- src/components/contact-logs/actions.test.ts | 63 ++ .../contact-lookup-details/actions.test.ts | 22 + src/components/shared-actions/domain.test.ts | 63 ++ src/components/user-menu/actions.test.ts | 24 + src/contexts/user-context.test.tsx | 31 + src/lib/auth-client.test.ts | 35 ++ src/lib/auth.ts | 53 +- .../auth/client-credentials.test.ts | 112 ++++ .../ministry-platform/provider.test.ts | 199 ++++++- .../services/communication.service.test.ts | 255 +++++++++ .../services/domain.service.test.ts | 138 +++++ .../services/file.service.test.ts | 537 ++++++++++++++++++ .../services/metadata.service.test.ts | 124 ++++ .../services/procedure.service.test.ts | 223 ++++++++ .../utils/http-client.test.ts | 33 ++ src/lib/utils.test.ts | 44 ++ src/services/domainTimezoneService.test.ts | 30 + vitest.config.ts | 57 ++ 29 files changed, 3140 insertions(+), 362 deletions(-) create mode 100644 .claude/TODO/contact-log-actions-authenticate-but-not-authorize.md create mode 100644 .claude/TODO/contact-log-actions-bypass-session-context-service.md create mode 100644 .claude/TODO/contact-logs-component-untested.md create mode 100644 .claude/TODO/mp-client-token-lifetime-ignores-expires-in.md create mode 100644 .claude/TODO/mp-filter-injection-numeric-ids.md create mode 100644 .claude/TODO/n-plus-1-contact-log-types-lookup.md create mode 100644 .claude/TODO/server-action-search-contacts-unauthenticated.md create mode 100644 .claude/TODO/server-action-user-profile-unauthenticated.md create mode 100644 src/components/shared-actions/domain.test.ts create mode 100644 src/lib/auth-client.test.ts create mode 100644 src/lib/providers/ministry-platform/auth/client-credentials.test.ts create mode 100644 src/lib/providers/ministry-platform/services/communication.service.test.ts create mode 100644 src/lib/providers/ministry-platform/services/domain.service.test.ts create mode 100644 src/lib/providers/ministry-platform/services/file.service.test.ts create mode 100644 src/lib/providers/ministry-platform/services/metadata.service.test.ts create mode 100644 src/lib/providers/ministry-platform/services/procedure.service.test.ts create mode 100644 src/lib/utils.test.ts diff --git a/.claude/TODO/contact-log-actions-authenticate-but-not-authorize.md b/.claude/TODO/contact-log-actions-authenticate-but-not-authorize.md new file mode 100644 index 00000000..f7c41bba --- /dev/null +++ b/.claude/TODO/contact-log-actions-authenticate-but-not-authorize.md @@ -0,0 +1,49 @@ +# TODO: Contact-log actions authenticate but never authorize + +**Created:** 2026-08-21 +**Severity:** Medium-High — needs a policy decision before it can be called a bug or a feature. +**Status:** Open. Documented during the test-coverage push. Requires a product decision, not just code. + +## Symptom + +In `src/components/contact-logs/actions.ts`, `updateContactLog`, `deleteContactLog`, +`getContactLogById`, and `getContactLogsByContactId` all confirm that *a* valid session exists, then +act on whatever ID they are handed. Nothing verifies that: + +- the contact log belongs to the caller, or +- the caller is permitted to touch that contact at all. + +`deleteContactLog` is the sharpest edge. Unlike create/update it does not even resolve `userGuid`: + +```ts +const session = await auth.api.getSession({ headers: await headers() }); +if (!session?.user?.id) throw new Error("Authentication required"); +if (!contactLogId || contactLogId <= 0) throw new Error("Valid Contact Log ID is required"); +await contactLogService.deleteContactLog(contactLogId); +``` + +Any authenticated session can delete **any** contact log in the domain by ID. Given CLAUDE.md's +stance on MP write safety ("Ministry Platform is a shared production database containing real church +member data"), this needs an explicit decision rather than an implicit one. + +## The decision to make + +Either: + +**(a)** "Any authenticated staff user may read, edit, and delete any contact log" is the intended +policy — MP itself is a staff-facing system and this may well match how the church operates. If so, +document it in `.claude/references/auth.md` and add a test that *encodes* the decision, so a future +reader knows it was chosen rather than overlooked. + +**(b)** Ownership or role gating is required. Then `deleteContactLog` and `updateContactLog` should +load the log first, compare `Made_By` against the acting `User_ID`, and reject mismatches unless the +caller holds a supervisory role. + +Option (a) is plausible and cheap. What is not acceptable is leaving it ambiguous — the current tests +mirror the code's assumptions exactly (`deleteContactLog(42)` asserts the service was called with +`42`), so they keep passing under either policy and encode nothing. + +## Related + +- `.claude/TODO/mp-filter-injection-numeric-ids.md` — the same entry points, different defect +- `.claude/docs/TestCoverage.md` §7.5 diff --git a/.claude/TODO/contact-log-actions-bypass-session-context-service.md b/.claude/TODO/contact-log-actions-bypass-session-context-service.md new file mode 100644 index 00000000..5c845263 --- /dev/null +++ b/.claude/TODO/contact-log-actions-bypass-session-context-service.md @@ -0,0 +1,79 @@ +# TODO: Contact-log actions re-implement User_ID resolution instead of using `SessionContextService` + +**Created:** 2026-08-21 +**Severity:** Medium — redundant MP round-trips per write, duplicated logic, and it contradicts the +project's own decided policy on unattributed writes. +**Status:** Open. Refactor, not a bug — behavior is currently correct-ish but the wrong shape. + +## Symptom + +`src/components/contact-logs/actions.ts` resolves the acting user's MP `User_ID` inline, and does it +**twice** — once in `createContactLog` and again, byte-for-byte, in `updateContactLog`: + +```ts +const { MPHelper } = await import("@/lib/providers/ministry-platform"); +const mp = new MPHelper(); +const users = await mp.getTableRecords<{ User_ID: number }>({ + table: "dp_Users", + filter: `User_GUID = '${sanitizeGuid(userGuid)}'`, + select: "User_ID", + top: 1 +}); +if (!users || users.length === 0 || !users[0].User_ID) { + throw new Error("Unable to determine user User_ID"); +} +``` + +Three separate things are wrong with this: + +### 1. The work is already done + +`src/lib/auth.ts` has `resolveMpUserId`, which performs exactly this `dp_Users` lookup, caches it +process-wide by `User_GUID`, and bakes the result into the session as `session.user.userId` via +`customSession`. The actions ignore that and pay an uncached MP round-trip on **every single write**. + +### 2. `SessionContextService` exists for precisely this call site + +`src/services/sessionContextService.ts` is documented as the canonical way to get the acting user for +a write: + +> "Use this — not `getCurrentUserId` — at every MP write boundary." + +`getActingUserIdForWrite({ table, operation })` reads `session.user.userId` (free, already resolved) +and emits a structured `mp.write.non_user` warning when it comes back null. It is fully tested at +100%. Nothing in `contact-logs/actions.ts` calls it. + +### 3. Throwing on an unresolved user contradicts the decided policy + +The actions throw `"Unable to determine user User_ID"` and abandon the write. `SessionContextService` +was built on the opposite premise — anonymous writes are legitimate and should be *logged*, not +*blocked*, so the unattributed write is visible in production logs and can be investigated. Right now +a user whose `dp_Users` row is missing or whose lookup transiently fails simply cannot save a contact +log at all. + +## Fix + +Replace both inline blocks with: + +```ts +import { sessionContextService } from "@/services/sessionContextService"; +... +const userId = await sessionContextService.getActingUserIdForWrite({ + table: "Contact_Log", + operation: "create", // or "update" +}); +const logDataWithUser = { ...contactLogData, Made_By: userId }; +``` + +Then delete the now-unused `getUserGuid` helper and the `MPHelper`/`sanitizeGuid` imports if nothing +else in the file needs them. + +Confirm before doing this that `Made_By` accepts null on the MP side. If it does not, the graceful +path is to omit the field rather than to fail the write — decide and document which. + +## Test impact + +`src/components/contact-logs/actions.test.ts` mocks `MPHelper.getTableRecords` to satisfy the inline +lookup; those mocks get replaced with a `sessionContextService` mock. Add a case asserting that a +null acting user still performs the write and emits `mp.write.non_user`, which is the behavior change +this refactor is really about. diff --git a/.claude/TODO/contact-logs-component-untested.md b/.claude/TODO/contact-logs-component-untested.md new file mode 100644 index 00000000..899c568e --- /dev/null +++ b/.claude/TODO/contact-logs-component-untested.md @@ -0,0 +1,44 @@ +# TODO: `contact-logs.tsx` — 602 lines driving MP writes, 0% coverage + +**Created:** 2026-08-21 +**Severity:** Medium — the largest untested file in the app, on the app's only write path. +**Status:** Open. Explicitly **out of scope** for the >90% non-UI coverage target (this is a React +component), but it is the highest-value remaining test gap in the repo and should not get lost. + +## Symptom + +`src/components/contact-logs/contact-logs.tsx` is 602 lines at 0% coverage. It is the component that +drives contact-log **create / update / delete** against Ministry Platform. It owns: + +- form state and client-side validation +- the delete-confirmation gate +- optimistic updates and rollback +- error handling and user-facing error surfaces + +The server actions beneath it sit at 97.8% coverage — but the actions are the easy half. The form +logic, the confirmation gate, and the error handling are where a regression silently corrupts or +deletes real member data. + +## Why it matters more than the coverage number suggests + +CLAUDE.md is unambiguous about MP write safety. The only interactive path a user has to mutate MP +data in this app goes through code that no test has ever executed. A regression that, say, fires +delete before the confirmation resolves would not be caught by anything in the suite. + +## Suggested approach — targeted, not exhaustive + +`@testing-library/react` is already installed and `components/layout/auth-wrapper.test.tsx` proves +the harness works. Do not chase full render coverage. Three tests, in priority order: + +1. **The delete-confirmation gate.** Clicking delete does *not* call `deleteContactLog` until the + confirmation is accepted; cancelling calls nothing. +2. **Form validation before submit.** Missing `Contact_Date` or `Notes` does not reach + `createContactLog` (the action throws on these, but the component should never send them). +3. **Action failure surfaces to the user** and does not leave an optimistic row in place. + +Even these three beat zero by a wide margin. + +## Related + +- `.claude/docs/TestCoverage.md` §6.3 +- `.claude/TODO/contact-log-actions-authenticate-but-not-authorize.md` diff --git a/.claude/TODO/mp-client-token-lifetime-ignores-expires-in.md b/.claude/TODO/mp-client-token-lifetime-ignores-expires-in.md new file mode 100644 index 00000000..ddcfc8c9 --- /dev/null +++ b/.claude/TODO/mp-client-token-lifetime-ignores-expires-in.md @@ -0,0 +1,54 @@ +# TODO: `MinistryPlatformClient` discards `expires_in` and caps every token at 5 minutes + +**Created:** 2026-08-21 +**Severity:** Low — wasteful, and the code contradicts its own comment. +**Status:** Open. + +## Symptom + +`src/lib/providers/ministry-platform/client.ts`: + +```ts +// Token refresh interval - refresh 5 minutes before actual expiration for safety +const TOKEN_LIFE = 5 * 60 * 1000; // 5 minutes +... +const creds = await getClientCredentialsToken(); +this.token = creds.access_token; +// Set expiration time with safety buffer (TOKEN_LIFE before actual expiration) +this.expiresAt = new Date(Date.now() + TOKEN_LIFE); +``` + +The comments describe subtracting a safety buffer from the real expiry. The code instead sets every +token's usable life to exactly 5 minutes, discarding the `expires_in` value that MP returns in the +token response. + +MP client-credentials tokens are typically valid for an hour, so this means roughly 12x more token +requests than necessary. Behavior is correct — just wasteful, and the stated intent and the actual +behavior disagree, which is the kind of gap that bites whoever edits it next. + +## Fix + +```ts +const creds = await getClientCredentialsToken(); +this.token = creds.access_token; +const lifetimeMs = (Number(creds.expires_in) || 3600) * 1000; +const SAFETY_MARGIN = 5 * 60 * 1000; +this.expiresAt = new Date(Date.now() + Math.max(lifetimeMs - SAFETY_MARGIN, 30_000)); +``` + +Rename `TOKEN_LIFE` to `TOKEN_SAFETY_MARGIN` so the constant says what it is. The `max(..., 30s)` +floor keeps a pathologically short `expires_in` from causing a refresh storm. + +## Test to add alongside the fix + +- `expires_in: 3600` -> `expiresAt` is ~55 minutes out +- `expires_in` missing -> falls back to the 1-hour default +- `expires_in: 60` -> clamped to the 30s floor rather than going negative + +Note for whoever writes these: `client.test.ts` already exercises the refresh path, and the current +behavior is not pinned by any assertion on `expiresAt` — so the fix will not break existing tests, +which is precisely the problem. + +## Related + +- `.claude/docs/TestCoverage.md` §7.7 diff --git a/.claude/TODO/mp-filter-injection-numeric-ids.md b/.claude/TODO/mp-filter-injection-numeric-ids.md new file mode 100644 index 00000000..c70986e5 --- /dev/null +++ b/.claude/TODO/mp-filter-injection-numeric-ids.md @@ -0,0 +1,68 @@ +# TODO: Numeric IDs are interpolated into MP filters without sanitization + +**Created:** 2026-08-21 +**Severity:** High — confirmed filter injection reachable from a public server action. +**Status:** Open. Documented during the test-coverage push; deliberately **not** fixed, because the fix changes runtime behavior on read paths and deserves its own review. + +## Symptom + +`ContactLogService` interpolates caller-supplied IDs straight into the MP `$filter` string: + +- `src/services/contactLogService.ts:101` — `filter: \`Contact_Log_ID = ${contactLogId}\`` +- `src/services/contactLogService.ts:118` — `filter: \`Contact_ID = ${contactId}\`` +- `src/services/contactLogService.ts:83` — same shape via `searchContactLogs` +- `src/services/userService.ts:75` and `:80` — `User_ID = ${profile.User_ID}` (lower risk; the value originates from MP, not from a caller) + +The codebase has `sanitizeFilterValue`, `sanitizeLikeValue`, and `sanitizeGuid` in +`src/lib/providers/ministry-platform/utils/filter-sanitize.ts`, and applies them faithfully to every +**string** parameter. There is no equivalent for numeric IDs, and the TypeScript `number` annotation +is erased at runtime. + +## Why the action-level guard does not stop it + +`src/components/contact-logs/actions.ts` guards with `if (!contactLogId || contactLogId <= 0)`. +For `contactLogId = "1 OR 1=1"`: + +``` +!id -> false (a non-empty string is truthy) +id <= 0 -> false (string/number comparison does not reject it) +guard passes +``` + +Server actions compile to callable POST endpoints. A caller controls the payload *shape*, not just +its values, so a string arriving where the signature says `number` is entirely reachable. + +## Reproduction + +Verified empirically against the real service with a mocked `MPHelper` (probe test since removed): + +``` +getContactLogById("1 OR 1=1") -> filter: "Contact_Log_ID = 1 OR 1=1" +searchContactLogs("5; DROP") -> filter: "Contact_ID = 5; DROP" +``` + +Both reach the MP API. `Contact_Log_ID = 1 OR 1=1` widens a single-record read into a full-table read. + +## Proposed fix + +1. Add to `filter-sanitize.ts`: + +```ts +export function sanitizeNumericId(value: unknown): number { + const n = typeof value === 'number' ? value : Number(value); + if (!Number.isInteger(n) || n <= 0) { + throw new Error('Invalid numeric ID'); + } + return n; +} +``` + +2. Apply it at all five interpolation sites listed above. +3. Test the rejection set: `'1 OR 1=1'`, `'5; DROP'`, `NaN`, `Infinity`, `1.5`, `-1`, `0`, `null`, + `undefined`, `' 7 '` (decide whether whitespace-padded numerics are accepted or rejected). + +## Why the existing tests missed it + +`contactLogService.ts` is at 100% statement coverage. No test passes a non-numeric value to any of +these methods, so every line executes and the defect survives. This is the clearest example in the +repo of coverage measuring the wrong thing — see `.claude/docs/TestCoverage.md` §7. diff --git a/.claude/TODO/n-plus-1-contact-log-types-lookup.md b/.claude/TODO/n-plus-1-contact-log-types-lookup.md new file mode 100644 index 00000000..0cc8c61a --- /dev/null +++ b/.claude/TODO/n-plus-1-contact-log-types-lookup.md @@ -0,0 +1,56 @@ +# TODO: N+1 lookup fetch in `getContactLogsByContactId` + +**Created:** 2026-08-21 +**Severity:** Low — performance only, no correctness impact. +**Status:** Open. Trivially fixable; documented rather than fixed to keep the coverage work behavior-neutral. + +## Symptom + +`src/components/contact-lookup-details/actions.ts:49-64` calls +`contactLogService.getContactLogTypes()` **inside** the `logs.map()` callback: + +```ts +const logsWithTypes = await Promise.all( + logs.map(async (log) => { + let contactLogType: string | null = null; + if (log.Contact_Log_Type_ID) { + const types = await contactLogService.getContactLogTypes(); // <-- per log + const type = types.find(t => t.Contact_Log_Type_ID === log.Contact_Log_Type_ID); + contactLogType = type?.Contact_Log_Type || null; + } + return { ...log, Contact_Log_Type: contactLogType } as ContactLogDisplay; + }) +); +``` + +For a contact with 50 logs that have a type set, that is 50 identical fetches of the same small +lookup table on every page load. + +## Fix + +Hoist the call above the loop and build a `Map` once: + +```ts +const types = await contactLogService.getContactLogTypes(); +const typeById = new Map(types.map(t => [t.Contact_Log_Type_ID, t.Contact_Log_Type])); +const logsWithTypes = logs.map(log => ({ + ...log, + Contact_Log_Type: log.Contact_Log_Type_ID ? typeById.get(log.Contact_Log_Type_ID) ?? null : null, +})) as ContactLogDisplay[]; +``` + +Since the map becomes synchronous, `Promise.all` goes away too. + +Alternatively (or additionally) memoize `getContactLogTypes()` in `ContactLogService` — it is a +lookup table that changes rarely, and other callers would benefit. + +## Why the existing tests missed it + +`contact-lookup-details/actions.ts` is at 100% statements / 90.9% branches. The test mocks +`getContactLogTypes` and never asserts a call count, so the inefficiency is invisible to the suite. +When fixing, add `expect(getContactLogTypes).toHaveBeenCalledTimes(1)` with a multi-log fixture so it +cannot regress. + +## Related + +- `.claude/docs/TestCoverage.md` §7.6 diff --git a/.claude/TODO/server-action-search-contacts-unauthenticated.md b/.claude/TODO/server-action-search-contacts-unauthenticated.md new file mode 100644 index 00000000..bd9f7a0d --- /dev/null +++ b/.claude/TODO/server-action-search-contacts-unauthenticated.md @@ -0,0 +1,54 @@ +# TODO: `searchContacts` server action has no session check + +**Created:** 2026-08-21 +**Severity:** High — unauthenticated PII disclosure. +**Status:** Open. Documented during the test-coverage push; not fixed, because adding auth here is a +behavior change on a user-facing path. + +## Symptom + +`src/components/contact-lookup/actions.ts` is a `'use server'` action with **zero** `getSession` +calls. It searches `Contacts` across `First_Name`, `Last_Name`, `Nickname`, `Email_Address`, and +`Mobile_Phone`, returning up to 20 matching records **including email address and mobile phone**. + +```ts +export async function searchContacts(searchTerm: string): Promise { + if (!searchTerm || searchTerm.trim().length === 0) return []; + const contactService = await ContactService.getInstance(); + return await contactService.contactSearch(searchTerm.trim()); +} +``` + +## Why this is reachable + +Server actions compile to callable POST endpoints. `src/proxy.ts:8` explicitly allows all `/api` +paths through without a session. Every sibling action file (`contact-logs/actions.ts`, +`contact-lookup-details/actions.ts`) does check the session — so this reads as an oversight, not a +deliberate design decision. + +A one-character search term returns 20 church members with contact details, to any caller. + +## Proposed fix + +Match the sibling pattern exactly: + +```ts +const session = await auth.api.getSession({ headers: await headers() }); +if (!session?.user?.id) { + throw new Error('Authentication required'); +} +``` + +Then add a test asserting an unauthenticated caller is rejected. + +## Why the existing tests missed it + +`contact-lookup/actions.ts` is at 100% statement and branch coverage with 5 passing tests (empty +input, whitespace, trimming, service errors, pass-through). None of them asks the authorization +question, because nothing in the code answers it. Coverage cannot flag a check that was never +written. + +## Related + +- `.claude/TODO/server-action-user-profile-unauthenticated.md` — same class of defect +- `.claude/docs/TestCoverage.md` §7.3 diff --git a/.claude/TODO/server-action-user-profile-unauthenticated.md b/.claude/TODO/server-action-user-profile-unauthenticated.md new file mode 100644 index 00000000..6c2851c8 --- /dev/null +++ b/.claude/TODO/server-action-user-profile-unauthenticated.md @@ -0,0 +1,56 @@ +# TODO: `getCurrentUserProfile` has neither authentication nor an ownership check + +**Created:** 2026-08-21 +**Severity:** High — unauthenticated disclosure of arbitrary users' profiles, roles, and groups. +**Status:** Open. Documented during the test-coverage push; not fixed. + +## Symptom + +`src/components/shared-actions/user.ts`: + +```ts +export async function getCurrentUserProfile(id: string): Promise { + const userService = await UserService.getInstance(); + return await userService.getUserProfile(id); +} +``` + +Two problems, not one: + +1. **No session check.** Like `searchContacts`, this is a `'use server'` action reachable as a POST + endpoint with no authentication. +2. **No ownership check.** The name says "current user" but the function takes an arbitrary + `User_GUID` and returns whatever profile that GUID names. `src/services/userService.ts:72-89` + also loads that user's **roles and user groups** — i.e. it discloses the authorization model for + any user whose GUID is known. + +GUIDs are not usefully secret: `session.user.userGuid` is present in the client-side session, and MP +GUIDs appear in URLs elsewhere in the app (`/contactlookup/[guid]`). + +## Proposed fix + +```ts +const session = await auth.api.getSession({ headers: await headers() }); +if (!session?.user?.id) throw new Error('Authentication required'); + +const requested = id ?? session.user.userGuid; +if (requested !== session.user.userGuid) { + // Either reject, or gate on an explicit "may read other users" role. + throw new Error('Forbidden'); +} +``` + +If cross-user reads are genuinely needed by some feature, that is a separate authorized path and +should be a separate, role-gated function — not an unauthenticated one named `getCurrentUserProfile`. +Consider dropping the parameter entirely and reading the GUID from the session, which makes the +ownership question unaskable. + +## Why the existing tests missed it + +Both existing tests pass `'guid-123'` and assert pass-through. 100% coverage, 4/4 statements. The +authorization question is never asked. + +## Related + +- `.claude/TODO/server-action-search-contacts-unauthenticated.md` +- `.claude/docs/TestCoverage.md` §7.4 diff --git a/.claude/docs/TestCoverage.md b/.claude/docs/TestCoverage.md index f077f696..81c0c721 100644 --- a/.claude/docs/TestCoverage.md +++ b/.claude/docs/TestCoverage.md @@ -1,331 +1,302 @@ # MPNext — Application & Unit Test Coverage Review -**Date:** 2026-08-20 -**Reviewed commit:** `bb2cd19` (branch `main`, clean working tree) +**Date:** 2026-08-21 +**Reviewed commit:** `64f18f0` (branch `main`), plus the coverage work described in §2 **Scope:** whole application — `src/**` excluding generated MP models +**Supersedes:** the 2026-08-20 review of `bb2cd19`, whose gap analysis has now been acted on --- ## 1. Executive summary -The test suite is **healthy where it exists and honest in style** — 277 tests across 21 files, all passing in ~3s, with `tsc --noEmit` and `eslint .` both clean. The service and provider layers are genuinely well tested. +Non-UI functional code now sits at **99.47% statement coverage**, up from 71.93%. The suite grew from +279 tests in 21 files to **419 tests in 30 files**, still running in ~3s, with `tsc --noEmit` and +`eslint .` both clean. + +| | Before | After | +|---|---|---| +| Statements | 71.93% (546/759) | **99.47%** (756/760) | +| Branches | 70.73% (220/311) | **95.49%** (297/311) | +| Functions | 72.28% (120/166) | **98.20%** (164/167) | +| Lines | 72.93% (539/739) | **99.72%** (738/740) | Three things matter more than the headline number: -| Finding | Impact | +| Finding | Status | |---|---| -| **Reported coverage is inflated ~2.2×.** `coverage.all` is not enabled, so files no test imports are omitted from the denominator entirely. Real statement coverage is **32.7%**, not the 71.6% the tool prints. | Medium — measurement | -| **`.claude/references/testing.md` claims 95.39% coverage.** That figure is not reproducible under any configuration; it is stale by a wide margin. | Medium — documentation | -| **Two `'use server'` actions have no session check at all**, and both are at 100% line coverage. Coverage is measuring the wrong thing on the paths that matter most. | **High — security** | - -Additionally, one **confirmed filter-injection path** was found during review (§7.1) — reproduced empirically, not inferred. - -Bottom line: this is not a "write more tests" problem so much as a **"tests are pointed away from the risk"** problem. The MP provider plumbing is tested three layers deep; the authorization boundary and the 624-line UI component that performs the writes are untested. - ---- +| **Measurement was inflated ~2.2×.** With no explicit `coverage.include`, every file no test imported dropped out of the denominator. | **Fixed.** `vitest.config.ts` now sets an explicit `include`, plus per-glob `thresholds` that fail the run on regression. | +| **`testing.md` claimed 95.39% coverage** — not reproducible under any configuration. | **Fixed.** Rewritten against measured numbers, with the new mock patterns documented. | +| **Coverage was pointed away from the risk.** Two `'use server'` actions have no session check at all, and both sat at 100% line coverage. | **Documented, not fixed** — see §5. Each defect now has a file in `.claude/TODO/`. | -## 2. Reproducing these numbers - -```bash -npm run test:run # 277 passed (21 files), ~3s -npm run test:coverage # prints 71.58% — see §3 for why this is wrong -npx tsc --noEmit # clean -npx eslint . # clean -``` - -> **Note:** `npx vitest run --reporter=basic` fails on Vitest 4 (`Failed to load custom Reporter from basic`). The `basic` reporter was removed. Any CI script or doc still passing `--reporter=basic` needs updating to `--reporter=default` or `dot`. - -To get the true figure, run with `all: true` and an explicit `include`: - -```jsonc -// vitest.config.ts → test.coverage -{ - provider: 'v8', - all: true, // <-- the missing line - include: ['src/**/*.{ts,tsx}'], - exclude: [ - 'node_modules/', '.next/', 'src/test-setup.ts', '**/*.d.ts', - '**/*.test.{ts,tsx}', - 'src/lib/providers/ministry-platform/models/', - 'src/lib/providers/ministry-platform/scripts/', // dev-only codegen - ], -} -``` +The shape of the original problem is worth restating, because the new number does not make it go +away: **high coverage is not evidence of correctness.** The confirmed filter-injection path in §5.1 +lives in a file at 100% statement coverage, and it still does. --- -## 3. The headline number is wrong - -| Metric | As currently reported | Actual (`all: true`) | -|---|---|---| -| Statements | 71.58% (539/753) | **32.72%** (539/1647) | -| Branches | 70.22% (217/309) | **26.27%** (217/826) | -| Functions | 71.95% (118/164) | **29.50%** (118/400) | -| Lines | 72.57% (532/733) | **33.31%** (532/1597) | - -The covered count is identical (539) in both runs — only the denominator changes. Every file no test ever imports is currently invisible: all 19 UI primitives, all 10 app routes/pages, 9 of 10 feature components, both codegen scripts. +## 2. What changed -The v8 text reporter also **omits any file at 100% on every metric**, showing only the directory roll-up. So `src/services` printing one row is not a truncation bug — the other five files there really are at 100%. Useful to know when reading the raw output. +### New test files (10) ---- +| File | Tests | Statements gained | +|---|---:|---:| +| `services/file.service.test.ts` | 35 | +88 | +| `services/procedure.service.test.ts` | 16 | +27 | +| `services/communication.service.test.ts` | 13 | +25 | +| `services/metadata.service.test.ts` | 8 | +12 | +| `services/domain.service.test.ts` | 8 | +11 | +| `auth/client-credentials.test.ts` | 5 | +7 | +| `lib/utils.test.ts` | 7 | +1 | +| `lib/auth-client.test.ts` | 4 | +1 | +| `components/shared-actions/domain.test.ts` | 3 | +3 | -## 4. Coverage by layer (true figures) +The five MP sub-services were the bulk of the gap — 163 of the 213 missing statements. All five share +the `ensureValidToken` → `getHttpClient` → error-wrap shape that `table.service.ts` already had tested, +so the harness was copy-adaptable. -| Layer | Stmts | Files | Assessment | -|---|---|---|---| -| Business logic (services, provider, actions, utils, contexts) | **70.8%** (532/751) | 46 | Solid — the real strength of this suite | -| React feature components | **2.6%** (7/269) | 10 | Effectively untested (only `auth-wrapper.tsx`) | -| UI primitives (`components/ui/`) | **0%** (0/145) | 19 | Acceptable — thin shadcn/Radix wrappers | -| App routes & pages | **0%** (0/37) | 10 | Mostly thin shells; low value | -| Codegen scripts | **0%** (0/445) | 2 | Dev-only tooling; should be excluded, not tested | +### Extended test files (6) -**The meaningful number is 70.8%** — business logic, excluding UI primitives, pages, and dev tooling. That is a defensible figure and the one worth tracking in CI. It is *not* 95%, and it is not 32.7% either; quoting either extreme misleads. +- `provider.test.ts` — 9 → 24 tests. The pass-throughs to `CommunicationService` and `FileService` + were entirely untested; `provider.ts` went 60% → 100%. +- `auth.test.ts` — 12 → 25 tests. See §3. +- `contact-logs/actions.test.ts` — 19 → 24. Added the missing-`userGuid` guard, non-positive-ID + rejection, and unresolved-`User_ID` paths. +- `domainTimezoneService.test.ts` — 16 → 18. Added `clearCache` and the unparseable-with-zone-marker path. +- `http-client.test.ts` — 26 → 28. Added the `putFormData` non-OK and query-param paths. +- `contact-lookup-details/actions.test.ts`, `user-menu/actions.test.ts`, `user-context.test.tsx` — + non-Error rejection wrapping, the env-fallback chain, and the `isPending` / undefined-profile branches. ---- +### One source change -## 5. What is well covered +`src/lib/auth.ts` — the `customSession` callback body was extracted to an exported +`enrichSessionUser(user, session)`. Behavior is identical; the better-auth plugin closes over its +callback and never exposes it, so this was the only way to unit test the logic short of driving a full +`getSession()` request through the whole auth stack. `auth.ts` went 44% → 96%. -Genuinely good work here, worth preserving: +### Config -- **`src/services/`** — 97.6%. `contactLogService`, `contactService`, `userService`, `sessionContextService` all at 100%/100%. -- **`domainTimezoneService.ts`** — 94.7% with 16 tests. Given that CLAUDE.md makes this the mandatory MP datetime boundary, testing it this thoroughly is exactly right. DST transitions, Windows→IANA mapping, and round-tripping are all exercised. -- **`http-client.ts`** — 95.9% / 26 tests. Query-string building, array params, and per-verb error paths covered. -- **`filter-sanitize.ts`** — 100% / 20 tests. Quote doubling, LIKE wildcard escaping, and GUID rejection all asserted. -- **`proxy.ts`** — 100%. Public-path bypass, missing-cookie redirect, and the throwing branch are all covered. -- **`helper.ts` / `table.service.ts`** — 100% statements. Zod `partial: true/false` semantics and validation error messages are asserted. -- **Mock hygiene** — `vi.hoisted()` is used correctly throughout, MPHelper is mocked as a class (not `mockImplementation`), and singletons are reset in `beforeEach`. The patterns documented in `testing.md` are actually followed. +`vitest.config.ts` gained an explicit `coverage.include`, an exclude for `src/components/ui/`, and +per-glob `thresholds`. The threshold gate was verified to fail (exit 1) when breached, not just to +pass when satisfied. --- -## 6. Coverage gaps, ranked +## 3. Reproducing these numbers -### 6.1 `lib/auth.ts` — 18.5% (5/27 stmts) 🔴 - -The OAuth wiring is the app's front door and is almost entirely unexercised. Untested: `getUserInfo` (lines 93–121), `mapProfileToUser`, the `customSession` callback, and `resolveMpUserId` (35–57) including its unbounded process-wide `userIdCache`. - -`src/auth.test.ts` has 12 tests and does **not** raise this number — see §7.2 for why. - -Worth covering: `getUserInfo` returning `null` on a non-OK userinfo response; `mapProfileToUser` mapping `sub` → `userGuid`; `resolveMpUserId` cache-hit vs. cache-miss; and the `catch` that must **not** block session creation. - -### 6.2 MP sub-services — 0% 🔴 - -| File | LOC | Stmts | -|---|---|---| -| `file.service.ts` | 212 | 0/88 | -| `communication.service.ts` | 78 | 0/25 | -| `procedure.service.ts` | 72 | 0/27 | -| `metadata.service.ts` | 37 | 0/12 | -| `domain.service.ts` | 40 | 0/11 | - -`table.service.ts` is at 100% and its four siblings are at zero — the same `ensureValidToken` → `getHttpClient` → error-wrap shape, tested once and then not again. `file.service.ts` (multipart uploads, blob downloads) and `communication.service.ts` (**sends real email/SMS to real church members**) are the two that carry actual blast radius. - -`provider.ts` sits at 60% for the same reason: the pass-throughs to these five services are never called. - -### 6.3 `contact-logs.tsx` — 0%, 624 lines 🔴 - -The single largest untested file in the app, and it is the component that drives contact-log **create / update / delete**. It holds form state, validation, optimistic updates, and delete confirmation. Every write path a user can actually reach goes through code with no test coverage. - -The server actions beneath it are 97.8% covered — but the actions are the easy half. The form logic, the confirmation gate, and the error handling are where a regression silently corrupts or deletes member data. - -### 6.4 `client-credentials.ts` — 0% (7 stmts) 🟡 - -25 lines, zero tests, and it is the only thing standing between the app and every MP API call. Needs one happy-path test and one `!response.ok` test. Cheap to fix. +```bash +npm run test:run # 419 passed (30 files), ~3s +npm run test:coverage # whole-app figure, and the threshold gate +npx tsc --noEmit # clean +npx eslint . # clean +``` -### 6.5 Other component gaps 🟡 +`npm run test:coverage` prints the **whole-app** number — 71.45% statements (756/1058) — because +feature components and app pages are in the denominator but ungated. To reproduce the **non-UI +functional** figure quoted in §1: -`contact-lookup-details.tsx` (176 LOC), `contact-lookup-results.tsx` (129), `contact-lookup-search.tsx` (87), `contact-lookup.tsx` (73), `header.tsx` (93), `dynamic-breadcrumb.tsx` (70), `user-menu.tsx` (69), `sidebar.tsx` (54) — all 0%. `@testing-library/react` is already installed and `auth-wrapper.test.tsx` proves the harness works, so the cost here is low. +```bash +npx vitest run --coverage \ + --coverage.include='src/**/*.ts' \ + --coverage.include='src/contexts/*.tsx' \ + --coverage.exclude='**/*.test.*' \ + --coverage.exclude='src/test-setup.ts' \ + --coverage.exclude='src/lib/providers/ministry-platform/models/**' \ + --coverage.exclude='src/lib/providers/ministry-platform/scripts/**' +``` -### 6.6 No coverage thresholds 🟡 +Both numbers are honest; they differ only in denominator. Quote the one whose scope you mean. -`vitest.config.ts` sets no `coverage.thresholds`. Nothing fails when coverage drops, so there is no ratchet — this review's numbers can silently regress before the next one. +> Two Vitest 4 gotchas. `--reporter=basic` fails (`Failed to load custom Reporter from basic`) — the +> `basic` reporter was removed; use `default` or `dot`. And `coverage.all` no longer exists and is not +> in the `CoverageOptions` type — setting it is a `tsc` error. `coverage.include` replaces it. --- -## 7. Where coverage is actively misleading +## 4. Coverage by layer -This is the most important section. Each item below is **fully covered by a passing test** and still wrong. +| Layer | Stmts | Files | Assessment | +|---|---|---|---| +| Services (`src/services/`) | **100%** | 5 | Complete, branches 98.9% | +| MP provider + sub-services | **99.7%** | 13 | Only the `client.ts` token-getter closure remains | +| Server actions | **100%** | 5 | Branches 89–100% | +| Contexts | **100%** | 2 | | +| `lib/auth.ts` + proxy | **97.4%** | 3 | Only the one-line delegating arrow remains | +| React feature components | **0%** | 9 | Ungated by design — see §6 | +| UI primitives (`components/ui/`) | excluded | 19 | Thin shadcn/Radix wrappers | +| Codegen scripts | excluded | 2 | Dev tooling, run manually | + +Only **four statements** in non-UI code are uncovered, all deliberate: + +- `app/api/auth/[...all]/route.ts` — a one-line `toNextJsHandler(auth)` re-export +- `lib/auth.ts:198` — the arrow delegating to `enrichSessionUser` +- `client.ts` — the token-getter closure handed to `HttpClient` +- `http-client.ts:31` — one arm of the GET error-message builder + +Plus two branch gaps at `helper.ts:189,273` — the `String(validationError)` arm of a validation-error +message. Zod always throws an `Error`, so reaching it requires a fake schema object. Not worth the +contrivance. -### 7.1 Confirmed: numeric IDs are interpolated into MP filters unsanitized 🔴 +--- -`ContactLogService` interpolates IDs directly: +## 5. Where coverage is still actively misleading -- `contactLogService.ts:101` — `filter: \`Contact_Log_ID = ${contactLogId}\`` -- `contactLogService.ts:118` — `filter: \`Contact_ID = ${contactId}\`` -- `contactLogService.ts:78` — same, in `searchContactLogs` -- `userService.ts:75,80` — `User_ID = ${profile.User_ID}` (lower risk; value originates from MP) +**This is the most important section.** Each item below is fully covered by passing tests and is still +wrong. Per the scope of this work, these were **documented, not fixed** — one file per issue in +`.claude/TODO/`. New tests pin today's behavior, and any test asserting behavior a TODO proposes +changing carries a comment naming the TODO file. -The codebase has `sanitizeFilterValue`, `sanitizeLikeValue`, and `sanitizeGuid` — and applies them faithfully to every **string** parameter. There is **no equivalent for numeric IDs**, and the TypeScript `number` annotation is erased at runtime. Server actions are public HTTP endpoints; a caller controls the payload shape, not just its values. +### 5.1 Confirmed: numeric IDs are interpolated into MP filters unsanitized 🔴 -The action-level guard does not help. For `contactLogId = "1 OR 1=1"`: +→ `.claude/TODO/mp-filter-injection-numeric-ids.md` -``` -!id → false (non-empty string is truthy) -id <= 0 → false (string/number comparison is not a rejection) -guard passes → true -``` +`contactLogService.ts:101,118,83` and `userService.ts:75,80` interpolate IDs directly. The codebase +has `sanitizeFilterValue`, `sanitizeLikeValue`, and `sanitizeGuid`, applies them faithfully to every +**string** parameter, and has no equivalent for numeric IDs — while the TypeScript `number` annotation +is erased at runtime. -Verified empirically against the real service with a mocked MPHelper: +The action-level guard does not help. For `contactLogId = "1 OR 1=1"`, `!id` is false (non-empty +string is truthy) and `id <= 0` is false, so the guard passes. Verified empirically: ``` getContactLogById("1 OR 1=1") → filter: "Contact_Log_ID = 1 OR 1=1" searchContactLogs("5; DROP") → filter: "Contact_ID = 5; DROP" ``` -Both reach the MP API. `Contact_Log_ID = 1 OR 1=1` widens a single-record read to the whole table. - -**No test passes a non-numeric value to any of these methods**, which is precisely why 100% line coverage on `contactLogService.ts` did not catch it. +`contactLogService.ts` is at **100% statements and 100% branches**. No test passes a non-numeric value, +which is exactly why full coverage did not catch it. -*Fix:* add `sanitizeNumericId(value: unknown): number` to `filter-sanitize.ts` — `Number.isInteger` + positive check, throwing otherwise — and apply it at every numeric interpolation site. Then test it with `'1 OR 1=1'`, `'5; DROP'`, `NaN`, `Infinity`, `1.5`, `-1`, `null`. +### 5.2 `searchContacts` — no authentication 🔴 -### 7.2 `src/auth.test.ts` — 5 tests assert against a copy of the logic, not the logic 🔴 +→ `.claude/TODO/server-action-search-contacts-unauthenticated.md` -The "Name Splitting" block re-implements the transformation inside the test body: +A `'use server'` action with zero `getSession` calls, returning up to 20 contacts including email and +mobile phone. `proxy.ts:8` allows all `/api` paths without a session, and every sibling action file +does check. 100% statements, 100% branches, 5 passing tests, none of which asks the authorization +question — because nothing in the code answers it. -```ts -// src/auth.test.ts:28-31 — this is the test, not the subject -const enrichedUser = { - ...user, - firstName: user.name?.split(' ')[0] || '', - lastName: user.name?.split(' ').slice(1).join(' ') || '', -}; -expect(enrichedUser.firstName).toBe('John'); -``` +### 5.3 `getCurrentUserProfile` — no authentication, no ownership check 🔴 -This asserts that `String.prototype.split` works. It never imports or invokes the `customSession` callback in `lib/auth.ts:143-161`. **Delete that callback entirely and these five tests still pass** — which is exactly why `auth.ts` reports 18.5% despite `auth.test.ts` containing 12 tests. +→ `.claude/TODO/server-action-user-profile-unauthenticated.md` -The `userAdditionalFields` / `parseAdditionalUserInputFromProviderProfile` tests in the same file are the opposite — they import the real export and guard a genuine better-auth 1.6 regression. That is the pattern the rest of the file should follow. +Takes an arbitrary `User_GUID` and returns that user's profile **plus their roles and user groups**. +100% covered. Both tests assert pass-through. -### 7.3 `searchContacts` — no authentication, 100% covered 🔴 +### 5.4 Contact-log actions authenticate but never authorize 🟠 -`src/components/contact-lookup/actions.ts` is a `'use server'` action with **zero** `getSession` calls. It searches `Contacts` across `First_Name`, `Last_Name`, `Nickname`, `Email_Address`, and `Mobile_Phone`, returning up to 20 records including email and mobile phone. +→ `.claude/TODO/contact-log-actions-authenticate-but-not-authorize.md` -Server actions compile to callable POST endpoints. `src/proxy.ts:8` explicitly allows all `/api` paths through without a session, and every sibling action file checks the session — so this is an inconsistency, not a deliberate design. +`deleteContactLog` is the sharpest edge: any authenticated session can delete any contact log in the +domain by ID. This needs a policy decision, not just code — "any authenticated staff user may delete +any log" may well be correct, but it should be chosen and documented rather than left implicit. -Its 5 tests cover empty input, whitespace, trimming, and service errors. All 5 pass. None asserts that an unauthenticated caller is rejected, because nothing rejects one. +### 5.5 Contact-log actions bypass `SessionContextService` 🟠 -### 7.4 `getCurrentUserProfile` — no authentication, no ownership check, 100% covered 🔴 +→ `.claude/TODO/contact-log-actions-bypass-session-context-service.md` -`src/components/shared-actions/user.ts` takes an arbitrary `id` (a User_GUID) and returns that user's profile **plus their roles and user groups** (`userService.ts:72-89`). No session check, and no verification that the caller owns the requested GUID. +Found while writing tests. `createContactLog` and `updateContactLog` each re-implement the `dp_Users` +User_ID lookup inline — work `resolveMpUserId` already does and caches, and which +`SessionContextService.getActingUserIdForWrite()` exists specifically to serve. Worse, they **throw** +when the User_ID cannot be resolved, contradicting the policy that service was built around: log +`mp.write.non_user` and proceed rather than blocking the write. -Both its tests pass `'guid-123'` and assert pass-through. The authorization question is never asked. +### 5.6 N+1 query in `getContactLogsByContactId` 🟡 -### 7.5 Contact-log actions authenticate but never authorize 🟠 +→ `.claude/TODO/n-plus-1-contact-log-types-lookup.md` -`updateContactLog`, `deleteContactLog`, `getContactLogById`, and `getContactLogsByContactId` all confirm *a* valid session, then act on whatever ID they are handed. Nothing checks that the log belongs to the caller, or that the caller may touch that contact. +`getContactLogTypes()` is called inside `logs.map()`. 50 logs with a type set means 50 identical +fetches of the same lookup table. The file is at 100%/100%; the test mocks the call and never asserts +a count. -`deleteContactLog` is the sharpest edge: unlike create/update it does not even resolve `userGuid`, so any authenticated session can delete any contact log in the domain by ID. Given CLAUDE.md's stance on MP write safety, this deserves an explicit decision — either "any authenticated staff user may delete any log" is the intended policy and should be documented, or an ownership check is missing. +### 5.7 `client.ts` token lifetime ignores `expires_in` 🟡 -The tests mirror the code's assumptions exactly (`deleteContactLog(42)` → asserts the service was called with `42`), so they will keep passing either way. +→ `.claude/TODO/mp-client-token-lifetime-ignores-expires-in.md` -### 7.6 N+1 query in `getContactLogsByContactId` 🟡 +The comment says "refresh 5 minutes *before* actual expiration"; the code caps every token at 5 +minutes total, discarding `expires_in`. Roughly 12× more token requests than necessary. -`contact-lookup-details/actions.ts:49-64` calls `contactLogService.getContactLogTypes()` **inside** the `logs.map()`. For 50 logs with a type set, that is 50 identical fetches of the same small lookup table. Hoist the call above the loop. +### 5.8 Resolved: `auth.test.ts` asserted against a copy of the logic ✅ -The test mocks `getContactLogTypes` and never asserts a call count, so the inefficiency is invisible to the suite. +The old "Name Splitting" and "Session Structure" blocks (7 tests) re-implemented the transformation +inside the test body and never invoked `customSession` — they would have passed with the callback +deleted. That is why `auth.ts` reported 18.5% despite the file containing 12 tests. -### 7.7 `client.ts` token lifetime ignores `expires_in` 🟡 - -`client.ts:52` sets `expiresAt = Date.now() + TOKEN_LIFE` where `TOKEN_LIFE = 5 minutes`, discarding the `expires_in` from the token response. The comment says "refresh 5 minutes *before* actual expiration," but the code caps every token at 5 minutes total. Mostly harmless (extra refreshes), but the stated intent and the behavior disagree, and no test pins either. +Now rewritten to call the real `enrichSessionUser`. Verified by mutation: changing +`firstName: user.name?.split(" ")[0]` to a constant fails 6 tests. The old versions failed none. --- -## 8. Recommendations - -Ordered by risk reduction per unit of effort. - -### Priority 1 — security correctness (do these first; they are bugs, not gaps) +## 6. Remaining gaps -1. Add `sanitizeNumericId` to `filter-sanitize.ts` and apply it at all five numeric interpolation sites (§7.1). Add the rejection tests. -2. Add a session check to `searchContacts` (§7.3) and to `getCurrentUserProfile` (§7.4); for the latter, verify the requested GUID matches `session.user.userGuid` unless a role explicitly permits otherwise. -3. Decide and document the authorization policy for contact-log read/update/delete (§7.5). If ownership is required, enforce it; either way, add a test that encodes the decision. -4. Add a **negative-path test per server action**: unauthenticated → rejected. This is ~10 small tests and it is the single highest-value block of tests missing from the repo. +### `contact-logs.tsx` — 602 lines, 0% 🔴 -### Priority 2 — make measurement honest +→ `.claude/TODO/contact-logs-component-untested.md` -5. Set `coverage.all: true` with the `include`/`exclude` from §2. Expect the reported number to drop to ~33% — that is the correction, not a regression. -6. Rewrite `src/auth.test.ts`'s name-splitting tests to invoke the real `customSession` callback, or delete them (§7.2). Tautological tests are worse than absent ones: they buy false confidence. -7. Correct `.claude/references/testing.md` — the 95.39% / "228 tests, 19 files" figures are wrong on all three counts (actual: 277 tests, 21 files, 32.7% raw / 70.8% business logic). Fix the `--reporter=basic` reference too. -8. Add a threshold ratchet at current business-logic levels so this cannot silently slide: - ```jsonc - thresholds: { statements: 70, branches: 65, functions: 70, lines: 70 } - ``` +The largest untested file in the app, and the component that drives contact-log create / update / +delete. Out of scope for a non-UI coverage target, but it is the highest-value test gap left in the +repo: the server actions beneath it are at 100%, and the actions are the easy half. The form logic, +the delete-confirmation gate, and the error handling are where a regression silently corrupts or +deletes member data. Three targeted tests would beat zero by a wide margin. -### Priority 3 — close the real gaps +### Other component gaps 🟡 -9. `client-credentials.ts` — 2 tests (§6.4). Smallest effort, guards every API call. -10. The four untested MP sub-services (§6.2). Clone the `table.service.test.ts` shape; start with `communication.service.ts` (sends real messages) and `file.service.ts` (largest). -11. `lib/auth.ts` — `getUserInfo` non-OK → `null`, `mapProfileToUser`, `resolveMpUserId` cache hit/miss/throw (§6.1). -12. `contact-logs.tsx` (§6.3) — the delete-confirmation gate and form validation first, not full render coverage. This is 624 lines driving MP writes; even three targeted tests beat zero. +`contact-lookup-details.tsx` (172 LOC), `contact-lookup-results.tsx` (82), `header.tsx` (89), +`contact-lookup-search.tsx` (68), `dynamic-breadcrumb.tsx` (65), `user-menu.tsx` (59), +`contact-lookup.tsx` (53), `sidebar.tsx` (39) — all 0%. `@testing-library/react` is installed and +`auth-wrapper.test.tsx` proves the harness works, so the cost is low. ### Explicitly not worth doing -- Testing `components/ui/` primitives — thin Radix/shadcn wrappers; exclude them from the denominator instead. -- Testing the codegen scripts (`generate-types.ts`, `generate-storedprocs.ts`, 445 stmts) — dev tooling, run manually, failures are immediately visible. Excluding them raises the honest denominator by ~27%. +- **`components/ui/` primitives** — thin Radix/shadcn wrappers. Excluded from the denominator. +- **Codegen scripts** (`generate-types.ts`, `generate-storedprocs.ts`, 445 stmts) — dev tooling, run + manually, failures immediately visible. Excluding them keeps the denominator honest. +- **`helper.ts:189,273`** — unreachable without a fake schema object. --- -## 9. Appendix — business-logic coverage, per file +## 7. Appendix — non-UI coverage, per file + +760 statements total. 17 barrel / type-only files carry zero statements and are omitted. | Stmts | Branch | Covered | File | |---:|---:|---:|---| -| 0% | 0% | 0/88 | `lib/providers/.../services/file.service.ts` | -| 0% | 0% | 0/27 | `lib/providers/.../services/procedure.service.ts` | -| 0% | 0% | 0/25 | `lib/providers/.../services/communication.service.ts` | -| 18.51% | 16.66% | 5/27 | `lib/auth.ts` | -| 60% | 100% | 18/30 | `lib/providers/ministry-platform/provider.ts` | -| 0% | 0% | 0/12 | `lib/providers/.../services/metadata.service.ts` | -| 0% | 100% | 0/11 | `lib/providers/.../services/domain.service.ts` | -| 0% | 0% | 0/7 | `lib/providers/.../auth/client-credentials.ts` | -| 0% | 100% | 0/3 | `components/shared-actions/domain.ts` | -| 0% | 100% | 0/1 | `lib/auth-client.ts` | -| 0% | 100% | 0/1 | `lib/utils.ts` | -| 94.73% | 93.33% | 72/76 | `services/domainTimezoneService.ts` | +| 0% | 100% | 0/1 | `app/api/auth/[...all]/route.ts` | | 94.73% | 100% | 18/19 | `lib/providers/ministry-platform/client.ts` | -| 95.91% | 91.66% | 47/49 | `lib/providers/.../utils/http-client.ts` | -| 96.15% | 75% | 25/26 | `contexts/user-context.tsx` | -| 97.8% | 85.96% | 89/91 | `components/contact-logs/actions.ts` | +| 96.42% | 80% | 27/28 | `lib/auth.ts` | +| 97.95% | 95.83% | 48/49 | `lib/providers/.../utils/http-client.ts` | | 100% | 81.81% | 54/54 | `lib/providers/ministry-platform/helper.ts` | -| 100% | 90.9% | 33/33 | `components/contact-lookup-details/actions.ts` | -| 100% | 60% | 8/8 | `components/user-menu/actions.ts` | +| 100% | 89.47% | 91/91 | `components/contact-logs/actions.ts` | +| 100% | 97.77% | 76/76 | `services/domainTimezoneService.ts` | +| 100% | 100% | 88/88 | `lib/providers/.../services/file.service.ts` | | 100% | 100% | 47/47 | `services/contactLogService.ts` | | 100% | 100% | 33/33 | `lib/providers/.../services/table.service.ts` | +| 100% | 100% | 33/33 | `components/contact-lookup-details/actions.ts` | +| 100% | 100% | 30/30 | `lib/providers/ministry-platform/provider.ts` | +| 100% | 100% | 27/27 | `lib/providers/.../services/procedure.service.ts` | +| 100% | 100% | 26/26 | `contexts/user-context.tsx` | +| 100% | 100% | 25/25 | `lib/providers/.../services/communication.service.ts` | | 100% | 100% | 17/17 | `services/contactService.ts` | | 100% | 100% | 15/15 | `services/sessionContextService.ts` | | 100% | 100% | 15/15 | `services/userService.ts` | | 100% | 100% | 14/14 | `proxy.ts` | +| 100% | 100% | 12/12 | `lib/providers/.../services/metadata.service.ts` | +| 100% | 100% | 11/11 | `lib/providers/.../services/domain.service.ts` | | 100% | 100% | 9/9 | `components/contact-lookup/actions.ts` | +| 100% | 100% | 8/8 | `components/user-menu/actions.ts` | +| 100% | 100% | 7/7 | `lib/providers/.../auth/client-credentials.ts` | +| 100% | 100% | 7/7 | `components/layout/auth-wrapper.tsx` | | 100% | 100% | 6/6 | `lib/providers/.../utils/filter-sanitize.ts` | | 100% | 100% | 4/4 | `components/shared-actions/user.ts` | +| 100% | 100% | 3/3 | `components/shared-actions/domain.ts` | | 100% | 100% | 3/3 | `contexts/session-context.tsx` | -| — | — | 0/0 | 16 barrel / type-only files | - -### Test inventory - -| Test file | Tests | -|---|---:| -| `lib/providers/ministry-platform/helper.test.ts` | 54 | -| `lib/providers/.../utils/http-client.test.ts` | 26 | -| `lib/providers/.../services/table.service.test.ts` | 21 | -| `services/contactLogService.test.ts` | 21 | -| `lib/providers/.../utils/filter-sanitize.test.ts` | 20 | -| `components/contact-logs/actions.test.ts` | 19 | -| `services/domainTimezoneService.test.ts` | 16 | -| `auth.test.ts` | 12 *(5 tautological — §7.2)* | -| `lib/providers/ministry-platform/client.test.ts` | 12 | -| `services/contactService.test.ts` | 12 | -| `services/sessionContextService.test.ts` | 10 | -| `components/contact-lookup-details/actions.test.ts` | 9 | -| `lib/providers/ministry-platform/provider.test.ts` | 9 | -| `proxy.test.ts` | 8 | -| `contexts/user-context.test.tsx` | 6 | -| `services/userService.test.ts` | 6 | -| `components/contact-lookup/actions.test.ts` | 5 | -| `components/layout/auth-wrapper.test.tsx` | 4 | -| `components/user-menu/actions.test.ts` | 3 | -| `components/shared-actions/user.test.ts` | 2 | -| `contexts/session-context.test.tsx` | 2 | -| **Total** | **277** | +| 100% | 100% | 1/1 | `lib/auth-client.ts` | +| 100% | 100% | 1/1 | `lib/utils.ts` | + +The full test inventory (419 tests across 30 files, with per-file counts) lives in +`.claude/references/testing.md`. --- -*All findings verified against the working tree at `bb2cd19`. §7.1 was reproduced with a temporary probe test (since removed) against the real `ContactLogService` with a mocked `MPHelper`. No Ministry Platform data was read or written during this review.* +*All findings verified against the working tree. §5.1 was reproduced with a temporary probe test +(since removed) against the real `ContactLogService` with a mocked `MPHelper`. §5.8 was verified by +mutation. No Ministry Platform data was read or written during this review or by any test in the +suite — every test mocks at a boundary above the network.* diff --git a/.claude/references/testing.md b/.claude/references/testing.md index a5016d8e..5ff98c8c 100644 --- a/.claude/references/testing.md +++ b/.claude/references/testing.md @@ -154,6 +154,121 @@ mockUseSession.mockReturnValue({ }); ``` +### Mocking the MP sub-service harness (`client` + `HttpClient`) + +The six MP sub-services (`TableService`, `FileService`, `CommunicationService`, +`ProcedureService`, `MetadataService`, `DomainService`) all take a +`MinistryPlatformClient` and call `ensureValidToken()` then `getHttpClient()`. +Build both as plain objects - no `vi.mock()` needed, since the service takes the +client as a constructor argument: + +```typescript +let mockHttpClient: HttpClient; +let mockClient: MinistryPlatformClient; + +beforeEach(() => { + mockHttpClient = { + get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn(), + buildUrl: vi.fn(), postFormData: vi.fn(), putFormData: vi.fn(), + } as unknown as HttpClient; + + mockClient = { + ensureValidToken: vi.fn().mockResolvedValue(undefined), + getHttpClient: vi.fn().mockReturnValue(mockHttpClient), + } as unknown as MinistryPlatformClient; + + service = new FileService(mockClient); +}); +``` + +Always assert the token-failure path calls nothing: + +```typescript +it('should not call the API when the token refresh fails', async () => { + (mockClient.ensureValidToken as ReturnType) + .mockRejectedValueOnce(new Error('Token refresh failed')); + + await expect(service.getFileMetadata(1)).rejects.toThrow('Token refresh failed'); + expect(mockHttpClient.get).not.toHaveBeenCalled(); +}); +``` + +### Stubbing global `fetch` + +Two places bypass `HttpClient` and call `fetch` directly: +`getClientCredentialsToken()` and `FileService.getFileContentByUniqueId()` (a +deliberately unauthenticated endpoint). Use `vi.stubGlobal` and always undo it: + +```typescript +let fetchMock: ReturnType; + +beforeEach(() => { + fetchMock = vi.fn(); + vi.stubGlobal('fetch', fetchMock); +}); + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +it('should throw on a non-OK response', async () => { + fetchMock.mockResolvedValueOnce({ ok: false, status: 404, statusText: 'Not Found' }); + await expect(subject()).rejects.toThrow('404 Not Found'); +}); +``` + +Mock the response as a plain object with only the fields the code touches +(`ok`, `status`, `statusText`, `json`, `blob`) - not a real `Response`. + +### Asserting multipart `FormData` payloads + +File uploads and communications with attachments go through `postFormData` / +`putFormData`. Read the captured `FormData` off the mock rather than trying to +match it with `toHaveBeenCalledWith`: + +```typescript +const [endpoint, formData, queryParams] = ( + mockHttpClient.postFormData as ReturnType +).mock.calls[0]; + +expect(endpoint).toBe('/files/Contacts/42'); +expect((formData.get('file-0') as File).name).toBe('photo.jpg'); +expect(JSON.parse(formData.get('communication') as string)).toEqual(payload); +expect(queryParams).toEqual({ $default: 'true' }); +``` + +`formData.get()` returns `null` for an absent key - useful for asserting that a +falsy optional param was dropped rather than sent as `"0"`. + +### Do not assert against a re-implementation of the subject + +The single worst pattern to reintroduce. An earlier version of `auth.test.ts` +looked like this: + +```typescript +// WRONG - this tests String.prototype.split, not our code. +const enriched = { + ...user, + firstName: user.name?.split(' ')[0] || '', +}; +expect(enriched.firstName).toBe('John'); +``` + +Those five tests passed at 100% line coverage while `lib/auth.ts` sat at 18.5%, +and they would have kept passing if the `customSession` callback were deleted +outright. Import the real export and call it: + +```typescript +// CORRECT +import { enrichSessionUser } from '@/lib/auth'; +const result = await enrichSessionUser({ id: 'ba', name: 'John Doe' }, session); +expect(result.user.firstName).toBe('John'); +``` + +If a function is unreachable because it is closed over by a library (as the +`customSession` callback was), extract it to a named export rather than +simulating it in the test. + ## Singleton Reset Pattern Service classes use static singleton instances. Reset between tests to avoid state leakage: @@ -193,46 +308,140 @@ it('should load profile', async () => { ## Coverage -Coverage uses the **v8** provider. Auto-generated model files are excluded. +Coverage uses the **v8** provider. ```bash -# Run with coverage (also reports on failure) -npx vitest run --coverage --coverage.reportOnFailure +npm run test:coverage # text + json + html reporters +npx vitest run --coverage --coverage.reportOnFailure # also report when tests fail ``` -### Current Coverage (228 tests, 19 files) +> Use `--reporter=default` or `--reporter=dot`. The `basic` reporter was removed +> in Vitest 4 and `--reporter=basic` now fails with +> `Failed to load custom Reporter from basic`. + +### The `include` glob is load-bearing + +`vitest.config.ts` sets `coverage.include: ['src/**/*.{ts,tsx}']`. Without an +explicit `include`, v8 reports only on files that some test imported, so every +untested file drops out of the denominator - the repo once reported 71.6% while +true statement coverage was 32.7%. Do not remove it. + +(Vitest 3's `coverage.all` flag no longer exists in Vitest 4 and is not in the +`CoverageOptions` type; `include` replaces it.) + +### Excluded from the denominator + +| Path | Why | +|---|---| +| `src/lib/providers/ministry-platform/models/` | Auto-generated from the MP API | +| `src/lib/providers/ministry-platform/scripts/` | Dev-only codegen, run manually; failures are immediately visible | +| `src/components/ui/` | Thin shadcn/Radix wrappers - testing them asserts that Radix works | -| Layer | Stmts | Branch | Lines | -|-------|-------|--------|-------| -| Services | 97.29% | 86.48% | 97.27% | -| Server Actions | ~99% | ~90% | ~99% | -| Proxy | 100% | 100% | 100% | -| Contexts | 91.42% | 85.71% | 91.42% | -| MP Provider | 87.37% | 86.66% | 88.23% | -| **All files** | **95.39%** | **88.02%** | **95.74%** | +Feature components (`*.tsx`) and app routes are **not** excluded. They stay +visible in the report at their real (mostly 0%) numbers; they are simply not +gated by a threshold. + +### Thresholds + +`coverage.thresholds` gates non-UI functional code per glob - services, the MP +provider, server actions, contexts, and auth/proxy plumbing. A breach fails the +run with `ERROR: Coverage for statements (X%) does not meet "" threshold (Y%)` +and a non-zero exit code. + +| Glob | Stmts | Branch | Funcs | Lines | +|---|---|---|---|---| +| `src/services/**` | 95 | 90 | 95 | 95 | +| `src/lib/**/*.ts` | 95 | 85 | 90 | 95 | +| `src/components/**/actions.ts` | 95 | 85 | 95 | 95 | +| `src/contexts/**` | 95 | 85 | 95 | 95 | +| `src/proxy.ts` | 100 | 100 | 100 | 100 | + +### Current coverage (419 tests, 30 files) + +Non-UI functional code - every `src/**/*.ts` plus `src/contexts/*.tsx`, excluding +generated models, codegen scripts, and test files (760 statements): + +| Metric | Value | +|---|---| +| Statements | **99.47%** (756/760) | +| Branches | **95.49%** (297/311) | +| Functions | **98.20%** (164/167) | +| Lines | **99.72%** (738/740) | + +Whole-app figure as `npm run test:coverage` prints it (1058 statements, including +untested feature components and app pages): **71.45%** statements. Both numbers +are honest; they differ only in denominator. Quote the one whose scope you mean. + +Known remaining gaps in non-UI code, all deliberate: + +Only four statements remain uncovered: + +- `app/api/auth/[...all]/route.ts` - a one-line `toNextJsHandler(auth)` re-export +- `lib/auth.ts:198` - the one-line arrow delegating to `enrichSessionUser` +- `client.ts` - the token-getter closure passed into `HttpClient` +- `http-client.ts:31` - one arm of the GET error-message builder + +Plus two branch gaps that are unreachable without a fake schema: +`helper.ts:189,273`, the `String(validationError)` arm of a validation-error +message - Zod always throws an `Error`. ## Test File Inventory | Test File | Tests | What It Covers | |-----------|-------|----------------| -| `services/contactService.test.ts` | 10 | Contact search, getByGuid, updateContact | -| `services/contactLogService.test.ts` | 16 | Contact log CRUD, date conversion, Zod validation | -| `services/userService.test.ts` | 4 | User profile lookup | -| `components/contact-lookup/actions.test.ts` | 5 | Search contacts action | -| `components/contact-logs/actions.test.ts` | 19 | Contact log CRUD actions with auth | -| `components/contact-lookup-details/actions.test.ts` | 10 | Contact details + log type mapping | -| `components/user-menu/actions.test.ts` | 3 | Sign-out + OAuth end session redirect | -| `components/shared-actions/user.test.ts` | 2 | getCurrentUserProfile delegation | -| `proxy.test.ts` | 8 | Route protection (public paths, session, errors) | -| `lib/providers/ministry-platform/provider.test.ts` | 9 | Provider delegation to services | -| `contexts/user-context.test.tsx` | 6 | UserProvider + useUser hook lifecycle | -| `contexts/session-context.test.tsx` | 2 | useAppSession wrapper | -| `auth.test.ts` | 11 | Name splitting, session structure | | `lib/providers/ministry-platform/helper.test.ts` | 54 | MPHelper CRUD, validation, procedures, files | +| `lib/providers/ministry-platform/services/file.service.test.ts` | 35 | All 8 file endpoints, multipart bodies, unauthenticated blob fetch | +| `lib/providers/ministry-platform/utils/http-client.test.ts` | 28 | HTTP verbs, URL building, form data, error handling | +| `auth.test.ts` | 25 | `enrichSessionUser`, cached User_ID resolution, OAuth config guards | +| `components/contact-logs/actions.test.ts` | 24 | Contact log CRUD actions, auth and argument guards | +| `lib/providers/ministry-platform/provider.test.ts` | 24 | Provider delegation to all six sub-services | +| `lib/providers/ministry-platform/services/table.service.test.ts` | 21 | TableService CRUD | +| `services/contactLogService.test.ts` | 21 | Contact log CRUD, date conversion, Zod validation | +| `lib/providers/ministry-platform/utils/filter-sanitize.test.ts` | 20 | Quote doubling, LIKE escaping, GUID rejection | +| `services/domainTimezoneService.test.ts` | 18 | Windows-to-IANA mapping, DST, round-tripping, cache | +| `lib/providers/ministry-platform/services/procedure.service.test.ts` | 16 | Procedure listing and execution, name encoding | +| `lib/providers/ministry-platform/services/communication.service.test.ts` | 13 | Email/SMS JSON vs multipart paths | | `lib/providers/ministry-platform/client.test.ts` | 12 | OAuth token management | -| `lib/providers/ministry-platform/services/table.service.test.ts` | 20 | TableService CRUD | -| `lib/providers/ministry-platform/utils/http-client.test.ts` | 26 | HTTP methods, URL building, error handling | - -## Known Issues - -- **ContactLogService date conversion bug**: `createContactLog` and `updateContactLog` convert ISO dates to SQL Server format (`YYYY-MM-DD HH:MM:SS`) _before_ Zod validation, but `ContactLogSchema` uses `z.string().datetime()` which only accepts ISO format. The validation rejects the converted date. Tests document this behavior. +| `services/contactService.test.ts` | 12 | Contact search, getByGuid, updateContact | +| `components/contact-lookup-details/actions.test.ts` | 11 | Contact details + log type mapping | +| `services/sessionContextService.test.ts` | 10 | Acting-user resolution, `mp.write.non_user` warning | +| `contexts/user-context.test.tsx` | 8 | UserProvider + useUser lifecycle | +| `lib/providers/ministry-platform/services/domain.service.test.ts` | 8 | Domain info and global filters | +| `lib/providers/ministry-platform/services/metadata.service.test.ts` | 8 | Metadata refresh, table listing | +| `proxy.test.ts` | 8 | Route protection (public paths, session, errors) | +| `lib/utils.test.ts` | 7 | `cn()` Tailwind class merging | +| `services/userService.test.ts` | 6 | User profile lookup | +| `components/contact-lookup/actions.test.ts` | 5 | Search contacts action | +| `components/user-menu/actions.test.ts` | 5 | Sign-out + OAuth end session redirect | +| `lib/providers/ministry-platform/auth/client-credentials.test.ts` | 5 | Client-credentials token grant | +| `components/layout/auth-wrapper.test.tsx` | 4 | Auth gating wrapper | +| `lib/auth-client.test.ts` | 4 | Client plugin wiring (`customSessionClient`, `signIn.social`) | +| `components/shared-actions/domain.test.ts` | 3 | `getMpTimezone` delegation | +| `components/shared-actions/user.test.ts` | 2 | `getCurrentUserProfile` delegation | +| `contexts/session-context.test.tsx` | 2 | `useAppSession` wrapper | +| **Total** | **419** | | + +## Ministry Platform Safety in Tests + +Per CLAUDE.md, no test may reach a real MP instance. Every suite mocks at a +boundary above the network: + +- `HttpClient` is mocked for all sub-service tests +- `MPHelper` is mocked as a class for all service and action tests +- Direct `fetch` callers are covered by `vi.stubGlobal('fetch', ...)` + +This matters most for `communication.service.test.ts` (sends real email/SMS in +production), `procedure.service.test.ts` (stored procedures can mutate data), and +`file.service.test.ts` / `table.service.test.ts` (writes and deletes). + +## Deferred Issues + +Defects and refactors found while testing are documented one-per-file in +`.claude/TODO/`, not fixed silently. Several are cases where a fully covered file +is still wrong - most notably numeric IDs interpolated into MP filters without +sanitization, and two `'use server'` actions with no session check at all. See +`.claude/TODO/` and `.claude/docs/TestCoverage.md`. + +Tests that pin behavior a TODO proposes changing carry a comment naming the TODO +file, so the next person knows the assertion is a snapshot of today's behavior +rather than a specification. diff --git a/src/auth.test.ts b/src/auth.test.ts index 61f38a36..e8373b4a 100644 --- a/src/auth.test.ts +++ b/src/auth.test.ts @@ -5,134 +5,271 @@ import type { GenericOAuthOptions, } from 'better-auth/plugins'; import type { OAuth2Tokens } from '@better-auth/core/oauth2'; -import { auth, userAdditionalFields } from '@/lib/auth'; + +const { mockGetTableRecords } = vi.hoisted(() => ({ + mockGetTableRecords: vi.fn(), +})); + +// MPHelper is mocked as a class (not vi.fn().mockImplementation) so `new MPHelper()` +// inside resolveMpUserId picks up the stubbed method — see .claude/references/testing.md. +vi.mock('@/lib/providers/ministry-platform', () => ({ + MPHelper: class { + getTableRecords = mockGetTableRecords; + }, +})); + +import { auth, userAdditionalFields, enrichSessionUser } from '@/lib/auth'; /** * Auth Tests * * Tests for the Better Auth configuration in src/lib/auth.ts. - * - customSession: lightweight name splitting only (no API calls) + * - enrichSessionUser: the customSession callback body — name splitting plus the + * cached dp_Users User_ID lookup that backs MP write attribution * - getUserInfo: fetches the OIDC profile and returns `sub` (better-auth 1.7 * resolves the account subject from it for OIDC discovery providers) * - mapProfileToUser: stores the OAuth sub claim as userGuid (additionalField) * - User profile loading is handled client-side by UserProvider */ +/** + * These tests invoke the REAL `enrichSessionUser` exported from src/lib/auth.ts, + * which is the body of the `customSession` callback. An earlier version of this + * block re-implemented the name-splitting inside the test and asserted against + * its own copy, so it passed even if the callback were deleted outright. Do not + * reintroduce that pattern: assert against the imported function. + * + * `userIdCache` in auth.ts is module-level and persists for the lifetime of this + * test file, so each test that cares about lookup counts uses its own GUID. + */ +describe('Auth - enrichSessionUser', () => { + const session = { id: 'session-123', token: 'tok', userId: 'ba-internal-id' }; -describe('Auth - Custom Session Enrichment Logic', () => { beforeEach(() => { vi.clearAllMocks(); + vi.spyOn(console, 'error').mockImplementation(() => {}); + mockGetTableRecords.mockResolvedValue([{ User_ID: 4242 }]); }); afterEach(() => { vi.restoreAllMocks(); }); - describe('Name Splitting', () => { - it('should split full name into firstName and lastName', () => { - const user = { id: 'ba-internal-id', name: 'John Doe', email: 'john@example.com', userGuid: 'user-guid-123' }; + describe('Name splitting', () => { + it('should split a full name into firstName and lastName', async () => { + const result = await enrichSessionUser( + { id: 'ba-internal-id', name: 'John Doe', userGuid: 'ab12cd34-ef56-7890-abcd-ef1234501001' }, + session, + ); + + expect(result.user.firstName).toBe('John'); + expect(result.user.lastName).toBe('Doe'); + }); - const enrichedUser = { - ...user, - firstName: user.name?.split(' ')[0] || '', - lastName: user.name?.split(' ').slice(1).join(' ') || '', - }; + it('should keep multi-part last names intact', async () => { + const result = await enrichSessionUser( + { id: 'ba-internal-id', name: 'Mary Jane Van Der Berg', userGuid: 'ab12cd34-ef56-7890-abcd-ef1234501002' }, + session, + ); - expect(enrichedUser.firstName).toBe('John'); - expect(enrichedUser.lastName).toBe('Doe'); + expect(result.user.firstName).toBe('Mary'); + expect(result.user.lastName).toBe('Jane Van Der Berg'); }); - it('should handle multi-part last names', () => { - const user = { id: 'ba-internal-id', name: 'Mary Jane Watson', email: 'mary@example.com' }; + it('should return an empty lastName for a single-word name', async () => { + const result = await enrichSessionUser( + { id: 'ba-internal-id', name: 'Prince', userGuid: 'ab12cd34-ef56-7890-abcd-ef1234501003' }, + session, + ); - const enrichedUser = { - ...user, - firstName: user.name?.split(' ')[0] || '', - lastName: user.name?.split(' ').slice(1).join(' ') || '', - }; + expect(result.user.firstName).toBe('Prince'); + expect(result.user.lastName).toBe(''); + }); + + it('should handle an undefined name without throwing', async () => { + const result = await enrichSessionUser( + { id: 'ba-internal-id', name: undefined, userGuid: 'ab12cd34-ef56-7890-abcd-ef1234501004' }, + session, + ); - expect(enrichedUser.firstName).toBe('Mary'); - expect(enrichedUser.lastName).toBe('Jane Watson'); + expect(result.user.firstName).toBe(''); + expect(result.user.lastName).toBe(''); }); - it('should handle single name (no last name)', () => { - const user = { id: 'ba-internal-id', name: 'Madonna', email: 'madonna@example.com' }; + it('should handle an empty-string name', async () => { + const result = await enrichSessionUser( + { id: 'ba-internal-id', name: '', userGuid: 'ab12cd34-ef56-7890-abcd-ef1234501005' }, + session, + ); + + expect(result.user.firstName).toBe(''); + expect(result.user.lastName).toBe(''); + }); + }); - const enrichedUser = { - ...user, - firstName: user.name?.split(' ')[0] || '', - lastName: user.name?.split(' ').slice(1).join(' ') || '', - }; + describe('Session structure', () => { + it('should preserve user.id and userGuid as distinct values', async () => { + const result = await enrichSessionUser( + { + id: 'ba-internal-id', + name: 'John Doe', + email: 'john@example.com', + userGuid: 'ab12cd34-ef56-7890-abcd-ef1234501006', + }, + session, + ); - expect(enrichedUser.firstName).toBe('Madonna'); - expect(enrichedUser.lastName).toBe(''); + // user.id is Better Auth's internal ID, NOT the MP User_GUID. + expect(result.user.id).toBe('ba-internal-id'); + // userGuid is the MP User_GUID, stored via additionalFields + mapProfileToUser. + expect(result.user.userGuid).toBe('ab12cd34-ef56-7890-abcd-ef1234501006'); }); - it('should handle undefined name gracefully', () => { - const user = { id: 'ba-internal-id', name: undefined as string | undefined, email: 'user@example.com' }; + it('should pass the session object through by reference, unmodified', async () => { + const result = await enrichSessionUser( + { id: 'ba-internal-id', name: 'John Doe', userGuid: 'ab12cd34-ef56-7890-abcd-ef1234501007' }, + session, + ); + + expect(result.session).toBe(session); + }); - const enrichedUser = { - ...user, - firstName: user.name?.split(' ')[0] || '', - lastName: user.name?.split(' ').slice(1).join(' ') || '', - }; + it('should not add userProfile to the session', async () => { + // The MP profile is loaded client-side by UserProvider, not baked into the + // session — a stateless JWT cookie cache cannot carry it cheaply. + const result = await enrichSessionUser( + { id: 'ba-internal-id', name: 'John Doe', userGuid: 'ab12cd34-ef56-7890-abcd-ef1234501008' }, + session, + ); - expect(enrichedUser.firstName).toBe(''); - expect(enrichedUser.lastName).toBe(''); + expect(result.user).not.toHaveProperty('userProfile'); + expect(result.session).not.toHaveProperty('userProfile'); }); + }); - it('should handle empty string name', () => { - const user = { id: 'ba-internal-id', name: '', email: 'user@example.com' }; + describe('User_ID resolution', () => { + it('should resolve the MP User_ID from dp_Users and expose it as userId', async () => { + const userGuid = 'ab12cd34-ef56-7890-abcd-ef1234502001'; + mockGetTableRecords.mockResolvedValueOnce([{ User_ID: 4242 }]); - const enrichedUser = { - ...user, - firstName: user.name?.split(' ')[0] || '', - lastName: user.name?.split(' ').slice(1).join(' ') || '', - }; + const result = await enrichSessionUser({ id: 'ba', name: 'John Doe', userGuid }, session); - expect(enrichedUser.firstName).toBe(''); - expect(enrichedUser.lastName).toBe(''); + expect(result.user.userId).toBe(4242); + expect(mockGetTableRecords).toHaveBeenCalledWith({ + table: 'dp_Users', + filter: `User_GUID = '${userGuid}'`, + select: 'User_ID', + top: 1, + }); }); - }); - describe('Session Structure', () => { - it('should return enriched user with userGuid and unchanged session', () => { - const user = { id: 'ba-internal-id', name: 'John Doe', email: 'john@example.com', userGuid: 'ab12cd34-ef56-7890-abcd-ef1234567890' }; - const session = { id: 'session-123', expiresAt: new Date() }; - - // Simulate customSession logic (no API calls, just name splitting) - const result = { - user: { - ...user, - firstName: user.name?.split(' ')[0] || '', - lastName: user.name?.split(' ').slice(1).join(' ') || '', - }, + it('should cache the lookup so a repeat session costs no MP call', async () => { + const userGuid = 'ab12cd34-ef56-7890-abcd-ef1234502002'; + mockGetTableRecords.mockResolvedValue([{ User_ID: 99 }]); + + const first = await enrichSessionUser({ id: 'ba', name: 'John Doe', userGuid }, session); + const second = await enrichSessionUser({ id: 'ba', name: 'John Doe', userGuid }, session); + + expect(first.user.userId).toBe(99); + expect(second.user.userId).toBe(99); + expect(mockGetTableRecords).toHaveBeenCalledTimes(1); + }); + + it('should look up each distinct userGuid separately', async () => { + mockGetTableRecords + .mockResolvedValueOnce([{ User_ID: 1 }]) + .mockResolvedValueOnce([{ User_ID: 2 }]); + + const a = await enrichSessionUser( + { id: 'ba', name: 'A A', userGuid: 'ab12cd34-ef56-7890-abcd-ef1234502003' }, session, - }; + ); + const b = await enrichSessionUser( + { id: 'ba', name: 'B B', userGuid: 'ab12cd34-ef56-7890-abcd-ef1234502004' }, + session, + ); - // user.id is Better Auth's internal ID, NOT the MP User_GUID - expect(result.user.id).toBe('ba-internal-id'); - // userGuid is the MP User_GUID stored via additionalFields + mapProfileToUser - expect(result.user.userGuid).toBe('ab12cd34-ef56-7890-abcd-ef1234567890'); - expect(result.user.firstName).toBe('John'); - expect(result.user.lastName).toBe('Doe'); - expect(result.session).toBe(session); + expect(a.user.userId).toBe(1); + expect(b.user.userId).toBe(2); + expect(mockGetTableRecords).toHaveBeenCalledTimes(2); }); - it('should not include userProfile in session', () => { - const user = { id: 'ba-internal-id', name: 'John Doe', email: 'john@example.com' }; - const session = { id: 'session-123', expiresAt: new Date() }; + it('should skip the lookup entirely when the user has no userGuid', async () => { + const result = await enrichSessionUser({ id: 'ba', name: 'John Doe' }, session); - const result = { - user: { - ...user, - firstName: user.name?.split(' ')[0] || '', - lastName: user.name?.split(' ').slice(1).join(' ') || '', - }, + expect(result.user.userId).toBeNull(); + expect(mockGetTableRecords).not.toHaveBeenCalled(); + }); + + it('should treat an empty userGuid as no userGuid', async () => { + const result = await enrichSessionUser( + { id: 'ba', name: 'John Doe', userGuid: '' }, session, - }; + ); - // userProfile is NOT part of the session — it's loaded client-side by UserProvider - expect(result.session).not.toHaveProperty('userProfile'); + expect(result.user.userId).toBeNull(); + expect(mockGetTableRecords).not.toHaveBeenCalled(); + }); + + it('should return a null userId when dp_Users has no matching row', async () => { + mockGetTableRecords.mockResolvedValueOnce([]); + + const result = await enrichSessionUser( + { id: 'ba', name: 'John Doe', userGuid: 'ab12cd34-ef56-7890-abcd-ef1234502005' }, + session, + ); + + expect(result.user.userId).toBeNull(); + }); + + it('should return a null userId when the row has no User_ID', async () => { + mockGetTableRecords.mockResolvedValueOnce([{ User_ID: 0 }]); + + const result = await enrichSessionUser( + { id: 'ba', name: 'John Doe', userGuid: 'ab12cd34-ef56-7890-abcd-ef1234502006' }, + session, + ); + + expect(result.user.userId).toBeNull(); + }); + + it('should not cache a failed resolution', async () => { + const userGuid = 'ab12cd34-ef56-7890-abcd-ef1234502007'; + mockGetTableRecords.mockResolvedValueOnce([]).mockResolvedValueOnce([{ User_ID: 77 }]); + + const first = await enrichSessionUser({ id: 'ba', name: 'John Doe', userGuid }, session); + const second = await enrichSessionUser({ id: 'ba', name: 'John Doe', userGuid }, session); + + expect(first.user.userId).toBeNull(); + expect(second.user.userId).toBe(77); + expect(mockGetTableRecords).toHaveBeenCalledTimes(2); + }); + + it('should never block session creation when the MP lookup throws', async () => { + // A failed User_ID lookup must degrade to null, not reject — otherwise a + // transient MP outage logs every user out. The missing attribution surfaces + // later as the mp.write.non_user warning at write time. + mockGetTableRecords.mockRejectedValueOnce(new Error('MP unreachable')); + + const result = await enrichSessionUser( + { id: 'ba', name: 'John Doe', userGuid: 'ab12cd34-ef56-7890-abcd-ef1234502008' }, + session, + ); + + expect(result.user.userId).toBeNull(); + expect(result.user.firstName).toBe('John'); + expect(console.error).toHaveBeenCalled(); + }); + + it('should reject a malformed userGuid rather than interpolating it into the filter', async () => { + // resolveMpUserId runs the GUID through sanitizeGuid, which throws on a + // non-canonical value. The throw is caught, so the session still succeeds. + const result = await enrichSessionUser( + { id: 'ba', name: 'John Doe', userGuid: "' OR 1=1 --" }, + session, + ); + + expect(result.user.userId).toBeNull(); + expect(mockGetTableRecords).not.toHaveBeenCalled(); }); }); }); diff --git a/src/components/contact-logs/actions.test.ts b/src/components/contact-logs/actions.test.ts index 34b5881c..f7c8704d 100644 --- a/src/components/contact-logs/actions.test.ts +++ b/src/components/contact-logs/actions.test.ts @@ -268,4 +268,67 @@ describe('contact-logs actions', () => { expect(result).toBeNull(); }); }); + + describe('Session and argument guards', () => { + it('should reject createContactLog when the session carries no userGuid', async () => { + // getUserGuid throws before any MP call is attempted — a Better Auth session + // without userGuid means mapProfileToUser did not run (see auth.test.ts). + mockGetSession.mockResolvedValueOnce({ user: { id: 'ba-internal-id' } }); + + await expect( + createContactLog({ Contact_ID: 1, Contact_Date: '2026-08-21', Notes: 'x' } as never) + ).rejects.toThrow('User GUID not found in session'); + + expect(mockGetTableRecords).not.toHaveBeenCalled(); + expect(mockCreateContactLog).not.toHaveBeenCalled(); + }); + + it('should reject updateContactLog when the session carries no userGuid', async () => { + mockGetSession.mockResolvedValueOnce({ user: { id: 'ba-internal-id' } }); + + await expect(updateContactLog(1, { Notes: 'x' })).rejects.toThrow( + 'User GUID not found in session' + ); + + expect(mockUpdateContactLog).not.toHaveBeenCalled(); + }); + + it('should reject updateContactLog when the acting User_ID cannot be resolved', async () => { + // NOTE: this action throws rather than proceeding with a null Made_By. That + // contradicts the policy SessionContextService was built around (log + // mp.write.non_user, do not block the write). Tracked in + // .claude/TODO/contact-log-actions-bypass-session-context-service.md — this + // test pins today's behavior so the refactor is a deliberate change. + mockGetSession.mockResolvedValueOnce(mockAuthSession); + mockGetTableRecords.mockResolvedValueOnce([]); + + await expect(updateContactLog(1, { Notes: 'x' })).rejects.toThrow( + 'Unable to determine user User_ID' + ); + + expect(mockUpdateContactLog).not.toHaveBeenCalled(); + }); + + it('should reject updateContactLog for a non-positive contact log ID', async () => { + mockGetSession.mockResolvedValueOnce(mockAuthSession); + mockGetTableRecords.mockResolvedValueOnce([{ User_ID: 4242 }]); + + await expect(updateContactLog(0, { Notes: 'x' })).rejects.toThrow( + 'Valid Contact Log ID is required' + ); + + expect(mockUpdateContactLog).not.toHaveBeenCalled(); + }); + + it('should reject updateContactLog for a negative contact log ID', async () => { + mockGetSession.mockResolvedValueOnce(mockAuthSession); + mockGetTableRecords.mockResolvedValueOnce([{ User_ID: 4242 }]); + + await expect(updateContactLog(-5, { Notes: 'x' })).rejects.toThrow( + 'Valid Contact Log ID is required' + ); + + expect(mockUpdateContactLog).not.toHaveBeenCalled(); + }); + }); }); diff --git a/src/components/contact-lookup-details/actions.test.ts b/src/components/contact-lookup-details/actions.test.ts index 02fefa9b..55ec8ebb 100644 --- a/src/components/contact-lookup-details/actions.test.ts +++ b/src/components/contact-lookup-details/actions.test.ts @@ -138,4 +138,26 @@ describe('contact-lookup-details actions', () => { expect(result[0].Contact_Log_Type).toBeNull(); }); }); + + describe('Non-Error rejections', () => { + // Both actions end in `throw error instanceof Error ? error : new Error(...)`. + // A service that rejects with a non-Error (a string from a bare `throw`, or a + // rejected promise carrying a plain object) must still surface a real Error, + // otherwise the caller gets `undefined` for `error.message`. + it('should wrap a non-Error rejection from getContactDetails', async () => { + mockGetSession.mockResolvedValueOnce(mockAuthSession); + mockGetContactByGuid.mockRejectedValueOnce('mp connection reset'); + + await expect(getContactDetails('ab12cd34-ef56-7890-abcd-ef1234567890')).rejects.toThrow( + 'Failed to fetch contact details' + ); + }); + + it('should wrap a non-Error rejection from getContactLogsByContactId', async () => { + mockGetSession.mockResolvedValueOnce(mockAuthSession); + mockGetContactLogsByContactId.mockRejectedValueOnce({ status: 500 }); + + await expect(getContactLogsByContactId(42)).rejects.toThrow('Failed to fetch contact logs'); + }); + }); }); diff --git a/src/components/shared-actions/domain.test.ts b/src/components/shared-actions/domain.test.ts new file mode 100644 index 00000000..f129a9d1 --- /dev/null +++ b/src/components/shared-actions/domain.test.ts @@ -0,0 +1,63 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; + +const { mockGetMpTimezone, mockGetInstance } = vi.hoisted(() => { + const getMpTimezone = vi.fn(); + return { + mockGetMpTimezone: getMpTimezone, + mockGetInstance: vi.fn(() => ({ getMpTimezone })), + }; +}); + +vi.mock('@/services/domainTimezoneService', () => ({ + DomainTimezoneService: { + getInstance: mockGetInstance, + }, +})); + +import { getMpTimezone } from '@/components/shared-actions/domain'; + +/** + * getMpTimezone action Tests + * + * Thin server action over DomainTimezoneService. It exists so client components + * can drive Intl.DateTimeFormat with the MP domain zone rather than the browser + * zone - per CLAUDE.md, MP stores wall-clock values in the domain time zone, so + * rendering them in the viewer's zone shifts every displayed timestamp. + * + * Worth pinning: the action resolves the singleton per call (not at module load) + * and does not swallow failures into a silent fallback zone, which would render + * wrong times rather than surfacing the problem. + */ +describe('getMpTimezone action', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('should return the IANA zone resolved by DomainTimezoneService', async () => { + mockGetMpTimezone.mockResolvedValueOnce('America/New_York'); + + await expect(getMpTimezone()).resolves.toBe('America/New_York'); + + expect(mockGetInstance).toHaveBeenCalledTimes(1); + expect(mockGetMpTimezone).toHaveBeenCalledTimes(1); + }); + + it('should resolve the service on each call rather than caching a stale instance', async () => { + mockGetMpTimezone.mockResolvedValue('America/Chicago'); + + await getMpTimezone(); + await getMpTimezone(); + + expect(mockGetInstance).toHaveBeenCalledTimes(2); + }); + + it('should propagate service failures instead of falling back to a default zone', async () => { + mockGetMpTimezone.mockRejectedValueOnce(new Error('Failed to resolve MP time zone')); + + await expect(getMpTimezone()).rejects.toThrow('Failed to resolve MP time zone'); + }); +}); diff --git a/src/components/user-menu/actions.test.ts b/src/components/user-menu/actions.test.ts index c5334ecc..f0cc180d 100644 --- a/src/components/user-menu/actions.test.ts +++ b/src/components/user-menu/actions.test.ts @@ -66,4 +66,28 @@ describe('handleSignOut', () => { await expect(handleSignOut()).rejects.toThrow('MINISTRY_PLATFORM_BASE_URL is not configured'); }); + + it('should fall back to NEXTAUTH_URL when BETTER_AUTH_URL is unset', async () => { + delete process.env.BETTER_AUTH_URL; + process.env.NEXTAUTH_URL = 'https://legacy.example.com'; + mockSignOut.mockResolvedValueOnce(undefined); + + await handleSignOut(); + + expect(mockRedirect).toHaveBeenCalledWith( + expect.stringContaining('post_logout_redirect_uri=https%3A%2F%2Flegacy.example.com') + ); + }); + + it('should fall back to localhost when neither auth URL is configured', async () => { + delete process.env.BETTER_AUTH_URL; + delete process.env.NEXTAUTH_URL; + mockSignOut.mockResolvedValueOnce(undefined); + + await handleSignOut(); + + expect(mockRedirect).toHaveBeenCalledWith( + expect.stringContaining('post_logout_redirect_uri=http%3A%2F%2Flocalhost%3A3000') + ); + }); }); diff --git a/src/contexts/user-context.test.tsx b/src/contexts/user-context.test.tsx index 6e70a594..a3169653 100644 --- a/src/contexts/user-context.test.tsx +++ b/src/contexts/user-context.test.tsx @@ -184,4 +184,35 @@ describe('UserContext', () => { expect(mockGetCurrentUserProfile).toHaveBeenCalledTimes(2); }); }); + + it('should not fetch while the session is still pending', async () => { + // Fetching during isPending would fire with a userGuid that may still change, + // then race the real value. + mockUseSession.mockReturnValue({ + data: { user: { id: 'internal-id', userGuid: 'guid-123' } }, + isPending: true, + }); + + await renderWithProvider(); + + expect(mockGetCurrentUserProfile).not.toHaveBeenCalled(); + expect(screen.getByTestId('name')).toHaveTextContent('none'); + }); + + it('should normalize an undefined profile to null', async () => { + // getCurrentUserProfile returns MPUserProfile | undefined; the context + // coerces undefined to null so consumers only handle one empty value. + mockUseSession.mockReturnValue({ + data: { user: { id: 'internal-id', userGuid: 'guid-123' } }, + isPending: false, + }); + mockGetCurrentUserProfile.mockResolvedValueOnce(undefined); + + await renderWithProvider(); + + await waitFor(() => { + expect(screen.getByTestId('name')).toHaveTextContent('none'); + }); + expect(mockGetCurrentUserProfile).toHaveBeenCalledWith('guid-123'); + }); }); diff --git a/src/lib/auth-client.test.ts b/src/lib/auth-client.test.ts new file mode 100644 index 00000000..f934f5f0 --- /dev/null +++ b/src/lib/auth-client.test.ts @@ -0,0 +1,35 @@ +import { describe, it, expect } from 'vitest'; +import { authClient } from '@/lib/auth-client'; + +/** + * auth-client Tests + * + * The browser-side Better Auth client. There is no logic to test here beyond the + * plugin wiring, but two things are worth guarding: + * + * 1. `customSessionClient` must be registered, otherwise the client-side session + * type loses the fields customSession adds on the server (firstName, + * lastName, userId, userGuid) and every consumer silently sees undefined. + * 2. `signIn.social` must exist. better-auth 1.7 dropped `genericOAuthClient()` + * and moved generic OAuth providers onto the standard social API, so a + * regression here would break sign-in entirely. + */ +describe('authClient', () => { + it('should expose the session hook used by useAppSession', () => { + expect(typeof authClient.useSession).toBe('function'); + }); + + it('should expose signIn.social for the ministry-platform provider', () => { + // better-auth 1.7: generic OAuth providers are reached through signIn.social, + // not the removed genericOAuthClient() plugin. + expect(typeof authClient.signIn.social).toBe('function'); + }); + + it('should expose signOut', () => { + expect(typeof authClient.signOut).toBe('function'); + }); + + it('should expose getSession for non-hook callers', () => { + expect(typeof authClient.getSession).toBe('function'); + }); +}); diff --git a/src/lib/auth.ts b/src/lib/auth.ts index 3087040a..b65d73c6 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -56,6 +56,39 @@ async function resolveMpUserId(userGuid: string): Promise { } } +/** + * Builds the enriched session payload returned by `customSession` below. + * + * Extracted from the `customSession` callback so it can be unit tested: the + * better-auth plugin closes over its callback and never exposes it, so the only + * other way to exercise this logic would be to drive a full `getSession()` + * request through the whole auth stack. Behavior is identical to the inline + * version it replaced. + * + * Profile loading still happens client-side via UserProvider / + * getCurrentUserProfile(). The only server-side lookup here is User_ID, cached + * in-memory after the first resolution per process, so it costs at most one MP + * call per (user × container). + */ +export async function enrichSessionUser< + U extends { name?: string | null }, + S, +>(user: U, session: S) { + const userGuid = (user as { userGuid?: string | null }).userGuid; + const userId: number | null = userGuid + ? await resolveMpUserId(userGuid) + : null; + return { + user: { + ...user, + firstName: user.name?.split(" ")[0] || "", + lastName: user.name?.split(" ").slice(1).join(" ") || "", + userId, + }, + session, + }; +} + const options = { baseURL: process.env.BETTER_AUTH_URL || process.env.NEXTAUTH_URL, secret: process.env.BETTER_AUTH_SECRET || process.env.NEXTAUTH_SECRET, @@ -162,25 +195,7 @@ export const auth = betterAuth({ plugins: [ ...(options.plugins ?? []), customSession( - async ({ user, session }) => { - // Profile loading still happens client-side via UserProvider / - // getCurrentUserProfile(). The only server-side lookup we do here is - // User_ID, cached in-memory after the first resolution per process, - // so it costs at most one MP call per (user × container). - const userGuid = (user as { userGuid?: string | null }).userGuid; - const userId: number | null = userGuid - ? await resolveMpUserId(userGuid) - : null; - return { - user: { - ...user, - firstName: user.name?.split(" ")[0] || "", - lastName: user.name?.split(" ").slice(1).join(" ") || "", - userId, - }, - session, - }; - }, + async ({ user, session }) => enrichSessionUser(user, session), options, ), nextCookies(), diff --git a/src/lib/providers/ministry-platform/auth/client-credentials.test.ts b/src/lib/providers/ministry-platform/auth/client-credentials.test.ts new file mode 100644 index 00000000..18f3d234 --- /dev/null +++ b/src/lib/providers/ministry-platform/auth/client-credentials.test.ts @@ -0,0 +1,112 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { getClientCredentialsToken } from '@/lib/providers/ministry-platform/auth/client-credentials'; + +/** + * getClientCredentialsToken Tests + * + * This 25-line function is the only thing standing between the app and every + * Ministry Platform API call - MinistryPlatformClient.ensureValidToken() calls it + * for every token refresh. It had no tests at all. + * + * What is worth pinning here: + * - the OAuth2 client_credentials grant is form-encoded, not JSON + * - the MP-specific scope string is sent verbatim (MP rejects a wrong scope) + * - a non-OK response throws rather than returning a token-shaped object with + * undefined fields, which would otherwise surface later as a confusing 401 + */ +describe('getClientCredentialsToken', () => { + let fetchMock: ReturnType; + const originalEnv = { ...process.env }; + + beforeEach(() => { + vi.clearAllMocks(); + fetchMock = vi.fn(); + vi.stubGlobal('fetch', fetchMock); + + process.env.MINISTRY_PLATFORM_BASE_URL = 'https://mp.example.org/ministryplatformapi'; + process.env.MINISTRY_PLATFORM_CLIENT_ID = 'test-client-id'; + process.env.MINISTRY_PLATFORM_CLIENT_SECRET = 'test-client-secret'; + }); + + afterEach(() => { + vi.unstubAllGlobals(); + process.env = { ...originalEnv }; + }); + + it('should POST the client_credentials grant to the MP token endpoint', async () => { + fetchMock.mockResolvedValueOnce({ + ok: true, + status: 200, + json: vi.fn().mockResolvedValue({ + access_token: 'test-token', + token_type: 'Bearer', + expires_in: 3600, + }), + }); + + const result = await getClientCredentialsToken(); + + expect(fetchMock).toHaveBeenCalledTimes(1); + const [url, init] = fetchMock.mock.calls[0]; + + expect(url).toBe('https://mp.example.org/ministryplatformapi/oauth/connect/token'); + expect(init.method).toBe('POST'); + expect(init.headers['Content-Type']).toBe('application/x-www-form-urlencoded'); + + expect(result).toEqual({ + access_token: 'test-token', + token_type: 'Bearer', + expires_in: 3600, + }); + }); + + it('should send grant_type, credentials, and the MP scope in the form body', async () => { + fetchMock.mockResolvedValueOnce({ + ok: true, + json: vi.fn().mockResolvedValue({ access_token: 'test-token' }), + }); + + await getClientCredentialsToken(); + + const body = new URLSearchParams(fetchMock.mock.calls[0][1].body); + + expect(body.get('grant_type')).toBe('client_credentials'); + expect(body.get('client_id')).toBe('test-client-id'); + expect(body.get('client_secret')).toBe('test-client-secret'); + expect(body.get('scope')).toBe( + 'http://www.thinkministry.com/dataplatform/scopes/all' + ); + }); + + it('should throw with the status text when the token request is rejected', async () => { + fetchMock.mockResolvedValueOnce({ + ok: false, + status: 401, + statusText: 'Unauthorized', + json: vi.fn(), + }); + + await expect(getClientCredentialsToken()).rejects.toThrow( + 'Failed to get client credentials token: Unauthorized' + ); + }); + + it('should not attempt to parse the body of a failed response', async () => { + const json = vi.fn(); + fetchMock.mockResolvedValueOnce({ + ok: false, + status: 500, + statusText: 'Internal Server Error', + json, + }); + + await expect(getClientCredentialsToken()).rejects.toThrow('Internal Server Error'); + expect(json).not.toHaveBeenCalled(); + }); + + it('should propagate network failures unchanged', async () => { + fetchMock.mockRejectedValueOnce(new TypeError('fetch failed')); + + await expect(getClientCredentialsToken()).rejects.toThrow('fetch failed'); + }); +}); diff --git a/src/lib/providers/ministry-platform/provider.test.ts b/src/lib/providers/ministry-platform/provider.test.ts index d3334c94..bf3c7abe 100644 --- a/src/lib/providers/ministry-platform/provider.test.ts +++ b/src/lib/providers/ministry-platform/provider.test.ts @@ -12,6 +12,15 @@ const { mockGetProcedures, mockExecuteProcedure, mockExecuteProcedureWithBody, + mockCreateCommunication, + mockSendMessage, + mockGetFilesByRecord, + mockUploadFiles, + mockUpdateFile, + mockDeleteFile, + mockGetFileContentByUniqueId, + mockGetFileMetadata, + mockGetFileMetadataByUniqueId, } = vi.hoisted(() => ({ mockGetTableRecords: vi.fn(), mockCreateTableRecords: vi.fn(), @@ -24,6 +33,15 @@ const { mockGetProcedures: vi.fn(), mockExecuteProcedure: vi.fn(), mockExecuteProcedureWithBody: vi.fn(), + mockCreateCommunication: vi.fn(), + mockSendMessage: vi.fn(), + mockGetFilesByRecord: vi.fn(), + mockUploadFiles: vi.fn(), + mockUpdateFile: vi.fn(), + mockDeleteFile: vi.fn(), + mockGetFileContentByUniqueId: vi.fn(), + mockGetFileMetadata: vi.fn(), + mockGetFileMetadataByUniqueId: vi.fn(), })); vi.mock('./client', () => ({ @@ -44,7 +62,10 @@ vi.mock('./services', () => ({ executeProcedure = mockExecuteProcedure; executeProcedureWithBody = mockExecuteProcedureWithBody; }, - CommunicationService: class {}, + CommunicationService: class { + createCommunication = mockCreateCommunication; + sendMessage = mockSendMessage; + }, MetadataService: class { refreshMetadata = mockRefreshMetadata; getTables = mockGetTables; @@ -53,7 +74,15 @@ vi.mock('./services', () => ({ getDomainInfo = mockGetDomainInfo; getGlobalFilters = mockGetGlobalFilters; }, - FileService: class {}, + FileService: class { + getFilesByRecord = mockGetFilesByRecord; + uploadFiles = mockUploadFiles; + updateFile = mockUpdateFile; + deleteFile = mockDeleteFile; + getFileContentByUniqueId = mockGetFileContentByUniqueId; + getFileMetadata = mockGetFileMetadata; + getFileMetadataByUniqueId = mockGetFileMetadataByUniqueId; + }, })); import { MinistryPlatformProvider } from './provider'; @@ -159,5 +188,171 @@ describe('MinistryPlatformProvider', () => { expect(mockGetTables).toHaveBeenCalledWith('Contacts'); expect(result).toEqual(mockTables); }); + + it('should delegate refreshMetadata to MetadataService', async () => { + mockRefreshMetadata.mockResolvedValueOnce(undefined); + + const provider = MinistryPlatformProvider.getInstance(); + await provider.refreshMetadata(); + + expect(mockRefreshMetadata).toHaveBeenCalledTimes(1); + }); + + it('should forward an omitted search term as undefined', async () => { + mockGetTables.mockResolvedValueOnce([]); + + const provider = MinistryPlatformProvider.getInstance(); + await provider.getTables(); + + expect(mockGetTables).toHaveBeenCalledWith(undefined); + }); + }); + + describe('Communication operations', () => { + // These pass-throughs sit in front of the endpoints that send real email and + // SMS to real church members. The DomainService/FileService/CommunicationService + // classes are mocked at the module boundary above, so nothing here reaches MP. + it('should delegate createCommunication to CommunicationService', async () => { + const communication = { Subject: 'Sunday update' } as never; + mockCreateCommunication.mockResolvedValueOnce({ Communication_ID: 555 }); + + const provider = MinistryPlatformProvider.getInstance(); + const result = await provider.createCommunication(communication); + + expect(mockCreateCommunication).toHaveBeenCalledWith(communication, undefined); + expect(result).toEqual({ Communication_ID: 555 }); + }); + + it('should forward attachments to createCommunication', async () => { + const communication = { Subject: 'Sunday update' } as never; + const attachments = [new File(['bulletin'], 'bulletin.pdf')]; + mockCreateCommunication.mockResolvedValueOnce({ Communication_ID: 556 }); + + const provider = MinistryPlatformProvider.getInstance(); + await provider.createCommunication(communication, attachments); + + expect(mockCreateCommunication).toHaveBeenCalledWith(communication, attachments); + }); + + it('should delegate sendMessage to CommunicationService', async () => { + const message = { Subject: 'Welcome' } as never; + mockSendMessage.mockResolvedValueOnce({ Communication_ID: 557 }); + + const provider = MinistryPlatformProvider.getInstance(); + const result = await provider.sendMessage(message); + + expect(mockSendMessage).toHaveBeenCalledWith(message, undefined); + expect(result).toEqual({ Communication_ID: 557 }); + }); + + it('should forward attachments to sendMessage', async () => { + const message = { Subject: 'Welcome' } as never; + const attachments = [new File(['receipt'], 'receipt.pdf')]; + mockSendMessage.mockResolvedValueOnce({ Communication_ID: 558 }); + + const provider = MinistryPlatformProvider.getInstance(); + await provider.sendMessage(message, attachments); + + expect(mockSendMessage).toHaveBeenCalledWith(message, attachments); + }); + }); + + describe('Domain filter operations', () => { + it('should delegate getGlobalFilters to DomainService', async () => { + const filters = [{ Key: 1, Value: 'Main Campus' }]; + mockGetGlobalFilters.mockResolvedValueOnce(filters); + + const provider = MinistryPlatformProvider.getInstance(); + const result = await provider.getGlobalFilters({ $userId: 7 }); + + expect(mockGetGlobalFilters).toHaveBeenCalledWith({ $userId: 7 }); + expect(result).toEqual(filters); + }); + }); + + describe('Procedure listing', () => { + it('should delegate getProcedures to ProcedureService', async () => { + mockGetProcedures.mockResolvedValueOnce([{ Name: 'api_Custom', Parameters: [] }]); + + const provider = MinistryPlatformProvider.getInstance(); + const result = await provider.getProcedures('api_'); + + expect(mockGetProcedures).toHaveBeenCalledWith('api_'); + expect(result).toHaveLength(1); + }); + }); + + describe('File operations', () => { + it('should delegate getFilesByRecord to FileService', async () => { + mockGetFilesByRecord.mockResolvedValueOnce([{ FileId: 501 }]); + + const provider = MinistryPlatformProvider.getInstance(); + const result = await provider.getFilesByRecord('Contacts', 42, true); + + expect(mockGetFilesByRecord).toHaveBeenCalledWith('Contacts', 42, true); + expect(result).toEqual([{ FileId: 501 }]); + }); + + it('should delegate uploadFiles to FileService', async () => { + const files = [new File(['x'], 'x.jpg')]; + mockUploadFiles.mockResolvedValueOnce([{ FileId: 502 }]); + + const provider = MinistryPlatformProvider.getInstance(); + await provider.uploadFiles('Contacts', 42, files, { description: 'Photo' }); + + expect(mockUploadFiles).toHaveBeenCalledWith('Contacts', 42, files, { + description: 'Photo', + }); + }); + + it('should delegate updateFile to FileService', async () => { + const file = new File(['x'], 'x.jpg'); + mockUpdateFile.mockResolvedValueOnce({ FileId: 501 }); + + const provider = MinistryPlatformProvider.getInstance(); + await provider.updateFile(501, file, { fileName: 'renamed.jpg' }); + + expect(mockUpdateFile).toHaveBeenCalledWith(501, file, { fileName: 'renamed.jpg' }); + }); + + it('should delegate deleteFile to FileService with the audit userId', async () => { + mockDeleteFile.mockResolvedValueOnce(undefined); + + const provider = MinistryPlatformProvider.getInstance(); + await provider.deleteFile(501, 7); + + expect(mockDeleteFile).toHaveBeenCalledWith(501, 7); + }); + + it('should delegate getFileContentByUniqueId to FileService', async () => { + const blob = new Blob(['bytes']); + mockGetFileContentByUniqueId.mockResolvedValueOnce(blob); + + const provider = MinistryPlatformProvider.getInstance(); + const result = await provider.getFileContentByUniqueId('unique-id', true); + + expect(mockGetFileContentByUniqueId).toHaveBeenCalledWith('unique-id', true); + expect(result).toBe(blob); + }); + + it('should delegate getFileMetadata to FileService', async () => { + mockGetFileMetadata.mockResolvedValueOnce({ FileId: 501 }); + + const provider = MinistryPlatformProvider.getInstance(); + const result = await provider.getFileMetadata(501); + + expect(mockGetFileMetadata).toHaveBeenCalledWith(501); + expect(result).toEqual({ FileId: 501 }); + }); + + it('should delegate getFileMetadataByUniqueId to FileService', async () => { + mockGetFileMetadataByUniqueId.mockResolvedValueOnce({ FileId: 501 }); + + const provider = MinistryPlatformProvider.getInstance(); + const result = await provider.getFileMetadataByUniqueId('unique-id'); + + expect(mockGetFileMetadataByUniqueId).toHaveBeenCalledWith('unique-id'); + expect(result).toEqual({ FileId: 501 }); + }); }); }); diff --git a/src/lib/providers/ministry-platform/services/communication.service.test.ts b/src/lib/providers/ministry-platform/services/communication.service.test.ts new file mode 100644 index 00000000..9e72fc7b --- /dev/null +++ b/src/lib/providers/ministry-platform/services/communication.service.test.ts @@ -0,0 +1,255 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { CommunicationService } from '@/lib/providers/ministry-platform/services/communication.service'; +import type { MinistryPlatformClient } from '@/lib/providers/ministry-platform/client'; +import type { HttpClient } from '@/lib/providers/ministry-platform/utils/http-client'; +import type { + Communication, + CommunicationInfo, + MessageInfo, +} from '@/lib/providers/ministry-platform/types'; + +/** + * CommunicationService Tests + * + * Covers: + * - createCommunication -> POST /communications (JSON) or postFormData (attachments) + * - sendMessage -> POST /messages (JSON) or postFormData (attachments) + * + * This service sends real email and SMS to real church members in production, so + * every test here drives a fully mocked HttpClient. Nothing in this file makes a + * network call, and no MP instance is contacted. + * + * The branch that matters most is `attachments && attachments.length > 0`: an + * empty array must take the plain-JSON path, not the multipart one, because the + * two hit different MP endpoints with different payload shapes. + */ +describe('CommunicationService', () => { + let communicationService: CommunicationService; + let mockClient: MinistryPlatformClient; + let mockHttpClient: HttpClient; + + const communicationInfo: CommunicationInfo = { + AuthorUserId: 7, + Body: 'Service is cancelled this Sunday.', + FromContactId: 100, + ReplyToContactId: 100, + CommunicationType: 'Email', + Contacts: [1, 2, 3], + IsBulkEmail: true, + SendToContactParents: false, + Subject: 'Sunday update', + StartDate: '2026-08-21T09:00:00', + }; + + const messageInfo: MessageInfo = { + FromAddress: { DisplayName: 'Church Office', Address: 'office@example.org' }, + ToAddresses: [{ DisplayName: 'John Doe', Address: 'john@example.com' }], + Subject: 'Welcome', + Body: 'Glad to have you.', + }; + + const createdCommunication: Communication = { + Communication_ID: 555, + Author_User_ID: 7, + Subject: 'Sunday update', + Body: 'Service is cancelled this Sunday.', + Domain_ID: 1, + Start_Date: '2026-08-21T09:00:00', + Communication_Status_ID: 1, + From_Contact: 100, + Reply_to_Contact: 100, + Active: true, + }; + + beforeEach(() => { + vi.clearAllMocks(); + vi.spyOn(console, 'error').mockImplementation(() => {}); + + mockHttpClient = { + get: vi.fn(), + post: vi.fn(), + put: vi.fn(), + delete: vi.fn(), + buildUrl: vi.fn(), + postFormData: vi.fn(), + putFormData: vi.fn(), + } as unknown as HttpClient; + + mockClient = { + ensureValidToken: vi.fn().mockResolvedValue(undefined), + getHttpClient: vi.fn().mockReturnValue(mockHttpClient), + } as unknown as MinistryPlatformClient; + + communicationService = new CommunicationService(mockClient); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + describe('createCommunication', () => { + it('should POST JSON to /communications when there are no attachments', async () => { + (mockHttpClient.post as ReturnType).mockResolvedValueOnce(createdCommunication); + + const result = await communicationService.createCommunication(communicationInfo); + + expect(mockClient.ensureValidToken).toHaveBeenCalledTimes(1); + expect(mockHttpClient.post).toHaveBeenCalledWith('/communications', { + ...communicationInfo, + }); + expect(mockHttpClient.postFormData).not.toHaveBeenCalled(); + expect(result).toEqual(createdCommunication); + }); + + it('should spread the payload rather than pass the caller object by reference', async () => { + (mockHttpClient.post as ReturnType).mockResolvedValueOnce(createdCommunication); + + await communicationService.createCommunication(communicationInfo); + + const sent = (mockHttpClient.post as ReturnType).mock.calls[0][1]; + expect(sent).toEqual(communicationInfo); + expect(sent).not.toBe(communicationInfo); + }); + + it('should take the JSON path when attachments is an empty array', async () => { + (mockHttpClient.post as ReturnType).mockResolvedValueOnce(createdCommunication); + + await communicationService.createCommunication(communicationInfo, []); + + expect(mockHttpClient.post).toHaveBeenCalledTimes(1); + expect(mockHttpClient.postFormData).not.toHaveBeenCalled(); + }); + + it('should POST multipart form data when attachments are present', async () => { + (mockHttpClient.postFormData as ReturnType).mockResolvedValueOnce( + createdCommunication + ); + + const bulletin = new File(['bulletin'], 'bulletin.pdf', { type: 'application/pdf' }); + const flyer = new File(['flyer'], 'flyer.png', { type: 'image/png' }); + + const result = await communicationService.createCommunication(communicationInfo, [ + bulletin, + flyer, + ]); + + expect(mockHttpClient.post).not.toHaveBeenCalled(); + expect(mockHttpClient.postFormData).toHaveBeenCalledTimes(1); + + const [endpoint, formData] = ( + mockHttpClient.postFormData as ReturnType + ).mock.calls[0]; + expect(endpoint).toBe('/communications'); + expect(formData).toBeInstanceOf(FormData); + expect(JSON.parse(formData.get('communication') as string)).toEqual(communicationInfo); + expect((formData.get('file-0') as File).name).toBe('bulletin.pdf'); + expect((formData.get('file-1') as File).name).toBe('flyer.png'); + expect(result).toEqual(createdCommunication); + }); + + it('should re-throw HTTP errors from the JSON path', async () => { + (mockHttpClient.post as ReturnType).mockRejectedValueOnce( + new Error('POST /communications failed: 400 Bad Request') + ); + + await expect( + communicationService.createCommunication(communicationInfo) + ).rejects.toThrow('400 Bad Request'); + }); + + it('should re-throw HTTP errors from the multipart path', async () => { + (mockHttpClient.postFormData as ReturnType).mockRejectedValueOnce( + new Error('POST /communications failed: 413 Payload Too Large') + ); + + const big = new File(['x'], 'big.zip'); + + await expect( + communicationService.createCommunication(communicationInfo, [big]) + ).rejects.toThrow('413 Payload Too Large'); + }); + + it('should not send anything when the token refresh fails', async () => { + (mockClient.ensureValidToken as ReturnType).mockRejectedValueOnce( + new Error('Token refresh failed') + ); + + await expect( + communicationService.createCommunication(communicationInfo) + ).rejects.toThrow('Token refresh failed'); + expect(mockHttpClient.post).not.toHaveBeenCalled(); + expect(mockHttpClient.postFormData).not.toHaveBeenCalled(); + }); + }); + + describe('sendMessage', () => { + it('should POST JSON to /messages when there are no attachments', async () => { + (mockHttpClient.post as ReturnType).mockResolvedValueOnce(createdCommunication); + + const result = await communicationService.sendMessage(messageInfo); + + expect(mockClient.ensureValidToken).toHaveBeenCalledTimes(1); + expect(mockHttpClient.post).toHaveBeenCalledWith('/messages', { ...messageInfo }); + expect(mockHttpClient.postFormData).not.toHaveBeenCalled(); + expect(result).toEqual(createdCommunication); + }); + + it('should take the JSON path when attachments is an empty array', async () => { + (mockHttpClient.post as ReturnType).mockResolvedValueOnce(createdCommunication); + + await communicationService.sendMessage(messageInfo, []); + + expect(mockHttpClient.post).toHaveBeenCalledTimes(1); + expect(mockHttpClient.postFormData).not.toHaveBeenCalled(); + }); + + it('should POST multipart form data under the message key when attachments are present', async () => { + (mockHttpClient.postFormData as ReturnType).mockResolvedValueOnce( + createdCommunication + ); + + const receipt = new File(['receipt'], 'receipt.pdf', { type: 'application/pdf' }); + + await communicationService.sendMessage(messageInfo, [receipt]); + + const [endpoint, formData] = ( + mockHttpClient.postFormData as ReturnType + ).mock.calls[0]; + expect(endpoint).toBe('/messages'); + expect(JSON.parse(formData.get('message') as string)).toEqual(messageInfo); + expect((formData.get('file-0') as File).name).toBe('receipt.pdf'); + }); + + it('should re-throw HTTP errors from the JSON path', async () => { + (mockHttpClient.post as ReturnType).mockRejectedValueOnce( + new Error('POST /messages failed: 422 Unprocessable Entity') + ); + + await expect(communicationService.sendMessage(messageInfo)).rejects.toThrow( + '422 Unprocessable Entity' + ); + }); + + it('should re-throw HTTP errors from the multipart path', async () => { + (mockHttpClient.postFormData as ReturnType).mockRejectedValueOnce( + new Error('POST /messages failed: 500 Internal Server Error') + ); + + await expect( + communicationService.sendMessage(messageInfo, [new File(['x'], 'x.txt')]) + ).rejects.toThrow('500 Internal Server Error'); + }); + + it('should not send anything when the token refresh fails', async () => { + (mockClient.ensureValidToken as ReturnType).mockRejectedValueOnce( + new Error('Token refresh failed') + ); + + await expect(communicationService.sendMessage(messageInfo)).rejects.toThrow( + 'Token refresh failed' + ); + expect(mockHttpClient.post).not.toHaveBeenCalled(); + expect(mockHttpClient.postFormData).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/src/lib/providers/ministry-platform/services/domain.service.test.ts b/src/lib/providers/ministry-platform/services/domain.service.test.ts new file mode 100644 index 00000000..c2a7188a --- /dev/null +++ b/src/lib/providers/ministry-platform/services/domain.service.test.ts @@ -0,0 +1,138 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { DomainService } from '@/lib/providers/ministry-platform/services/domain.service'; +import type { MinistryPlatformClient } from '@/lib/providers/ministry-platform/client'; +import type { HttpClient } from '@/lib/providers/ministry-platform/utils/http-client'; +import type { DomainInfo, GlobalFilterItem } from '@/lib/providers/ministry-platform/types'; + +/** + * DomainService Tests + * + * Covers the two read-only domain endpoints: + * - getDomainInfo -> GET /domain + * - getGlobalFilters -> GET /domain/filters + * + * Both follow the provider-wide shape: ensureValidToken() first, then the HTTP + * call, with errors logged and re-thrown unchanged (never swallowed). + * + * DomainInfo.TimeZoneName is what DomainTimezoneService reads to drive every MP + * datetime conversion, so the pass-through here is load-bearing. + */ +describe('DomainService', () => { + let domainService: DomainService; + let mockClient: MinistryPlatformClient; + let mockHttpClient: HttpClient; + + const mockDomainInfo: DomainInfo = { + DisplayName: 'Test Church', + TimeZoneName: 'Eastern Standard Time', + CultureName: 'en-US', + IsSimpleSignOnEnabled: false, + IsUserTimeZoneEnabled: false, + IsSmsMfaEnabled: false, + }; + + beforeEach(() => { + vi.clearAllMocks(); + vi.spyOn(console, 'error').mockImplementation(() => {}); + + mockHttpClient = { + get: vi.fn(), + post: vi.fn(), + put: vi.fn(), + delete: vi.fn(), + buildUrl: vi.fn(), + postFormData: vi.fn(), + putFormData: vi.fn(), + } as unknown as HttpClient; + + mockClient = { + ensureValidToken: vi.fn().mockResolvedValue(undefined), + getHttpClient: vi.fn().mockReturnValue(mockHttpClient), + } as unknown as MinistryPlatformClient; + + domainService = new DomainService(mockClient); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + describe('getDomainInfo', () => { + it('should fetch domain info from /domain', async () => { + (mockHttpClient.get as ReturnType).mockResolvedValueOnce(mockDomainInfo); + + const result = await domainService.getDomainInfo(); + + expect(mockClient.ensureValidToken).toHaveBeenCalledTimes(1); + expect(mockHttpClient.get).toHaveBeenCalledWith('/domain'); + expect(result).toEqual(mockDomainInfo); + }); + + it('should expose TimeZoneName for DomainTimezoneService', async () => { + (mockHttpClient.get as ReturnType).mockResolvedValueOnce(mockDomainInfo); + + const result = await domainService.getDomainInfo(); + + expect(result.TimeZoneName).toBe('Eastern Standard Time'); + }); + + it('should re-throw HTTP errors unchanged', async () => { + (mockHttpClient.get as ReturnType).mockRejectedValueOnce( + new Error('GET /domain failed: 500 Internal Server Error') + ); + + await expect(domainService.getDomainInfo()).rejects.toThrow('500 Internal Server Error'); + }); + + it('should re-throw token refresh failures without calling the API', async () => { + (mockClient.ensureValidToken as ReturnType).mockRejectedValueOnce( + new Error('Token refresh failed') + ); + + await expect(domainService.getDomainInfo()).rejects.toThrow('Token refresh failed'); + expect(mockHttpClient.get).not.toHaveBeenCalled(); + }); + }); + + describe('getGlobalFilters', () => { + const mockFilters: GlobalFilterItem[] = [ + { Key: 0, Value: 'Not Assigned' }, + { Key: 1, Value: 'Main Campus' }, + ]; + + it('should fetch global filters with no params', async () => { + (mockHttpClient.get as ReturnType).mockResolvedValueOnce(mockFilters); + + const result = await domainService.getGlobalFilters(); + + expect(mockClient.ensureValidToken).toHaveBeenCalledTimes(1); + expect(mockHttpClient.get).toHaveBeenCalledWith('/domain/filters', undefined); + expect(result).toEqual(mockFilters); + }); + + it('should pass optional params through to the query string', async () => { + (mockHttpClient.get as ReturnType).mockResolvedValueOnce(mockFilters); + + await domainService.getGlobalFilters({ $ignorePermissions: true, $userId: 42 }); + + expect(mockHttpClient.get).toHaveBeenCalledWith('/domain/filters', { + $ignorePermissions: true, + $userId: 42, + }); + }); + + it('should return an empty array when the domain has no global filters', async () => { + (mockHttpClient.get as ReturnType).mockResolvedValueOnce([]); + + await expect(domainService.getGlobalFilters()).resolves.toEqual([]); + }); + + it('should re-throw HTTP errors unchanged', async () => { + (mockHttpClient.get as ReturnType).mockRejectedValueOnce( + new Error('GET /domain/filters failed: 403 Forbidden') + ); + + await expect(domainService.getGlobalFilters()).rejects.toThrow('403 Forbidden'); + }); + }); +}); diff --git a/src/lib/providers/ministry-platform/services/file.service.test.ts b/src/lib/providers/ministry-platform/services/file.service.test.ts new file mode 100644 index 00000000..42688d8f --- /dev/null +++ b/src/lib/providers/ministry-platform/services/file.service.test.ts @@ -0,0 +1,537 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { FileService } from '@/lib/providers/ministry-platform/services/file.service'; +import type { MinistryPlatformClient } from '@/lib/providers/ministry-platform/client'; +import type { HttpClient } from '@/lib/providers/ministry-platform/utils/http-client'; +import type { FileDescription } from '@/lib/providers/ministry-platform/types'; + +/** + * FileService Tests + * + * Covers all eight file endpoints: + * - getFilesByRecord -> GET /files/{table}/{recordId} + * - uploadFiles -> POST /files/{table}/{recordId} (multipart) + * - updateFile -> PUT /files/{fileId} (multipart) + * - deleteFile -> DELETE /files/{fileId} + * - getFileContentByUniqueId -> raw fetch, deliberately unauthenticated + * - getFileMetadata -> GET /files/{fileId}/metadata + * - getFileMetadataByUniqueId -> GET /files/{uniqueFileId}/metadata + * + * Two behaviors get extra attention because they are easy to break silently: + * + * 1. `!== undefined` vs truthiness. `defaultOnly` and `isDefaultImage` use + * `!== undefined`, so `false` must still be sent. `longestDimension` and + * `userId` use plain truthiness, so `0` is dropped. Both are asserted so the + * distinction is pinned rather than accidental. + * 2. `getFileContentByUniqueId` must NOT call ensureValidToken and must NOT send + * an Authorization header - that endpoint is public by design. + */ +describe('FileService', () => { + let fileService: FileService; + let mockClient: MinistryPlatformClient; + let mockHttpClient: HttpClient; + + const mockFileDescription: FileDescription = { + FileId: 501, + FileName: 'photo.jpg', + FileExtension: '.jpg', + FileSize: 20480, + IsImage: true, + IsDefaultImage: false, + TableName: 'Contacts', + RecordId: 42, + UniqueFileId: 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee', + LastUpdated: '2026-08-21T09:00:00', + InclusionType: 'Attachment', + }; + + beforeEach(() => { + vi.clearAllMocks(); + vi.spyOn(console, 'error').mockImplementation(() => {}); + + mockHttpClient = { + get: vi.fn(), + post: vi.fn(), + put: vi.fn(), + delete: vi.fn(), + buildUrl: vi.fn(), + postFormData: vi.fn(), + putFormData: vi.fn(), + } as unknown as HttpClient; + + mockClient = { + ensureValidToken: vi.fn().mockResolvedValue(undefined), + getHttpClient: vi.fn().mockReturnValue(mockHttpClient), + } as unknown as MinistryPlatformClient; + + fileService = new FileService(mockClient); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + describe('getFilesByRecord', () => { + it('should fetch file descriptions for a record', async () => { + (mockHttpClient.get as ReturnType).mockResolvedValueOnce([ + mockFileDescription, + ]); + + const result = await fileService.getFilesByRecord('Contacts', 42); + + expect(mockClient.ensureValidToken).toHaveBeenCalledTimes(1); + expect(mockHttpClient.get).toHaveBeenCalledWith('/files/Contacts/42', {}); + expect(result).toEqual([mockFileDescription]); + }); + + it('should send $default=true when defaultOnly is true', async () => { + (mockHttpClient.get as ReturnType).mockResolvedValueOnce([]); + + await fileService.getFilesByRecord('Contacts', 42, true); + + expect(mockHttpClient.get).toHaveBeenCalledWith('/files/Contacts/42', { + $default: 'true', + }); + }); + + it('should send $default=false when defaultOnly is explicitly false', async () => { + (mockHttpClient.get as ReturnType).mockResolvedValueOnce([]); + + await fileService.getFilesByRecord('Contacts', 42, false); + + expect(mockHttpClient.get).toHaveBeenCalledWith('/files/Contacts/42', { + $default: 'false', + }); + }); + + it('should return an empty array when the record has no files', async () => { + (mockHttpClient.get as ReturnType).mockResolvedValueOnce([]); + + await expect(fileService.getFilesByRecord('Contacts', 999)).resolves.toEqual([]); + }); + + it('should re-throw HTTP errors unchanged', async () => { + (mockHttpClient.get as ReturnType).mockRejectedValueOnce( + new Error('GET /files/Contacts/42 failed: 404 Not Found') + ); + + await expect(fileService.getFilesByRecord('Contacts', 42)).rejects.toThrow('404 Not Found'); + }); + + it('should re-throw token refresh failures without calling the API', async () => { + (mockClient.ensureValidToken as ReturnType).mockRejectedValueOnce( + new Error('Token refresh failed') + ); + + await expect(fileService.getFilesByRecord('Contacts', 42)).rejects.toThrow( + 'Token refresh failed' + ); + expect(mockHttpClient.get).not.toHaveBeenCalled(); + }); + }); + + describe('uploadFiles', () => { + it('should append each file as file-{index} and post multipart', async () => { + (mockHttpClient.postFormData as ReturnType).mockResolvedValueOnce([ + mockFileDescription, + ]); + + const first = new File(['one'], 'one.jpg', { type: 'image/jpeg' }); + const second = new File(['two'], 'two.png', { type: 'image/png' }); + + const result = await fileService.uploadFiles('Contacts', 42, [first, second]); + + expect(mockClient.ensureValidToken).toHaveBeenCalledTimes(1); + + const [endpoint, formData, queryParams] = ( + mockHttpClient.postFormData as ReturnType + ).mock.calls[0]; + expect(endpoint).toBe('/files/Contacts/42'); + expect(formData).toBeInstanceOf(FormData); + expect((formData.get('file-0') as File).name).toBe('one.jpg'); + expect((formData.get('file-1') as File).name).toBe('two.png'); + expect(queryParams).toEqual({}); + expect(result).toEqual([mockFileDescription]); + }); + + it('should accept an empty file list without appending any file entries', async () => { + (mockHttpClient.postFormData as ReturnType).mockResolvedValueOnce([]); + + await fileService.uploadFiles('Contacts', 42, []); + + const [, formData] = ( + mockHttpClient.postFormData as ReturnType + ).mock.calls[0]; + expect(formData.get('file-0')).toBeNull(); + }); + + it('should mirror every optional param into both the form body and the query string', async () => { + (mockHttpClient.postFormData as ReturnType).mockResolvedValueOnce([ + mockFileDescription, + ]); + + await fileService.uploadFiles('Contacts', 42, [new File(['x'], 'x.jpg')], { + description: 'Profile photo', + isDefaultImage: true, + longestDimension: 800, + userId: 7, + }); + + const [, formData, queryParams] = ( + mockHttpClient.postFormData as ReturnType + ).mock.calls[0]; + + expect(formData.get('description')).toBe('Profile photo'); + expect(formData.get('isDefaultImage')).toBe('true'); + expect(formData.get('longestDimension')).toBe('800'); + + // userId is a query-string-only parameter; it is never added to the body. + expect(formData.get('userId')).toBeNull(); + + expect(queryParams).toEqual({ + $description: 'Profile photo', + $default: 'true', + $longestDimension: '800', + $userId: '7', + }); + }); + + it('should send $default=false when isDefaultImage is explicitly false', async () => { + (mockHttpClient.postFormData as ReturnType).mockResolvedValueOnce([]); + + await fileService.uploadFiles('Contacts', 42, [new File(['x'], 'x.jpg')], { + isDefaultImage: false, + }); + + const [, formData, queryParams] = ( + mockHttpClient.postFormData as ReturnType + ).mock.calls[0]; + expect(formData.get('isDefaultImage')).toBe('false'); + expect(queryParams).toEqual({ $default: 'false' }); + }); + + it('should drop longestDimension: 0 because the check is truthiness, not undefined', async () => { + (mockHttpClient.postFormData as ReturnType).mockResolvedValueOnce([]); + + await fileService.uploadFiles('Contacts', 42, [new File(['x'], 'x.jpg')], { + longestDimension: 0, + userId: 0, + }); + + const [, formData, queryParams] = ( + mockHttpClient.postFormData as ReturnType + ).mock.calls[0]; + expect(formData.get('longestDimension')).toBeNull(); + expect(queryParams).toEqual({}); + }); + + it('should re-throw HTTP errors unchanged', async () => { + (mockHttpClient.postFormData as ReturnType).mockRejectedValueOnce( + new Error('POST /files/Contacts/42 failed: 413 Payload Too Large') + ); + + await expect( + fileService.uploadFiles('Contacts', 42, [new File(['x'], 'big.zip')]) + ).rejects.toThrow('413 Payload Too Large'); + }); + + it('should not upload anything when the token refresh fails', async () => { + (mockClient.ensureValidToken as ReturnType).mockRejectedValueOnce( + new Error('Token refresh failed') + ); + + await expect( + fileService.uploadFiles('Contacts', 42, [new File(['x'], 'x.jpg')]) + ).rejects.toThrow('Token refresh failed'); + expect(mockHttpClient.postFormData).not.toHaveBeenCalled(); + }); + }); + + describe('updateFile', () => { + it('should PUT multipart with the replacement file under the file key', async () => { + (mockHttpClient.putFormData as ReturnType).mockResolvedValueOnce( + mockFileDescription + ); + + const replacement = new File(['new'], 'replacement.jpg', { type: 'image/jpeg' }); + + const result = await fileService.updateFile(501, replacement); + + expect(mockClient.ensureValidToken).toHaveBeenCalledTimes(1); + + const [endpoint, formData, queryParams] = ( + mockHttpClient.putFormData as ReturnType + ).mock.calls[0]; + expect(endpoint).toBe('/files/501'); + expect((formData.get('file') as File).name).toBe('replacement.jpg'); + expect(queryParams).toEqual({}); + expect(result).toEqual(mockFileDescription); + }); + + it('should support a metadata-only update with no file', async () => { + (mockHttpClient.putFormData as ReturnType).mockResolvedValueOnce( + mockFileDescription + ); + + await fileService.updateFile(501, undefined, { description: 'Renamed only' }); + + const [, formData, queryParams] = ( + mockHttpClient.putFormData as ReturnType + ).mock.calls[0]; + expect(formData.get('file')).toBeNull(); + expect(formData.get('description')).toBe('Renamed only'); + expect(queryParams).toEqual({ $description: 'Renamed only' }); + }); + + it('should mirror every optional param into both the form body and the query string', async () => { + (mockHttpClient.putFormData as ReturnType).mockResolvedValueOnce( + mockFileDescription + ); + + await fileService.updateFile(501, undefined, { + fileName: 'renamed.jpg', + description: 'Updated caption', + isDefaultImage: true, + longestDimension: 1200, + userId: 7, + }); + + const [, formData, queryParams] = ( + mockHttpClient.putFormData as ReturnType + ).mock.calls[0]; + + expect(formData.get('fileName')).toBe('renamed.jpg'); + expect(formData.get('description')).toBe('Updated caption'); + expect(formData.get('isDefaultImage')).toBe('true'); + expect(formData.get('longestDimension')).toBe('1200'); + + expect(queryParams).toEqual({ + $fileName: 'renamed.jpg', + $description: 'Updated caption', + $default: 'true', + $longestDimension: '1200', + $userId: '7', + }); + }); + + it('should send $default=false when isDefaultImage is explicitly false', async () => { + (mockHttpClient.putFormData as ReturnType).mockResolvedValueOnce( + mockFileDescription + ); + + await fileService.updateFile(501, undefined, { isDefaultImage: false }); + + const [, formData, queryParams] = ( + mockHttpClient.putFormData as ReturnType + ).mock.calls[0]; + expect(formData.get('isDefaultImage')).toBe('false'); + expect(queryParams).toEqual({ $default: 'false' }); + }); + + it('should re-throw HTTP errors unchanged', async () => { + (mockHttpClient.putFormData as ReturnType).mockRejectedValueOnce( + new Error('PUT /files/501 failed: 404 Not Found') + ); + + await expect(fileService.updateFile(501)).rejects.toThrow('404 Not Found'); + }); + + it('should not update anything when the token refresh fails', async () => { + (mockClient.ensureValidToken as ReturnType).mockRejectedValueOnce( + new Error('Token refresh failed') + ); + + await expect(fileService.updateFile(501)).rejects.toThrow('Token refresh failed'); + expect(mockHttpClient.putFormData).not.toHaveBeenCalled(); + }); + }); + + describe('deleteFile', () => { + it('should DELETE the file with no query params when no userId is given', async () => { + (mockHttpClient.delete as ReturnType).mockResolvedValueOnce(undefined); + + await expect(fileService.deleteFile(501)).resolves.toBeUndefined(); + + expect(mockClient.ensureValidToken).toHaveBeenCalledTimes(1); + expect(mockHttpClient.delete).toHaveBeenCalledWith('/files/501', {}); + }); + + it('should pass $userId for audit attribution when given', async () => { + (mockHttpClient.delete as ReturnType).mockResolvedValueOnce(undefined); + + await fileService.deleteFile(501, 7); + + expect(mockHttpClient.delete).toHaveBeenCalledWith('/files/501', { $userId: '7' }); + }); + + it('should drop userId: 0 because the check is truthiness, not undefined', async () => { + (mockHttpClient.delete as ReturnType).mockResolvedValueOnce(undefined); + + await fileService.deleteFile(501, 0); + + expect(mockHttpClient.delete).toHaveBeenCalledWith('/files/501', {}); + }); + + it('should re-throw HTTP errors unchanged', async () => { + (mockHttpClient.delete as ReturnType).mockRejectedValueOnce( + new Error('DELETE /files/501 failed: 403 Forbidden') + ); + + await expect(fileService.deleteFile(501)).rejects.toThrow('403 Forbidden'); + }); + + it('should not delete anything when the token refresh fails', async () => { + (mockClient.ensureValidToken as ReturnType).mockRejectedValueOnce( + new Error('Token refresh failed') + ); + + await expect(fileService.deleteFile(501)).rejects.toThrow('Token refresh failed'); + expect(mockHttpClient.delete).not.toHaveBeenCalled(); + }); + }); + + describe('getFileContentByUniqueId', () => { + const uniqueId = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee'; + let fetchMock: ReturnType; + + beforeEach(() => { + fetchMock = vi.fn(); + vi.stubGlobal('fetch', fetchMock); + (mockHttpClient.buildUrl as ReturnType).mockImplementation( + (endpoint: string) => `https://mp.example.org/api${endpoint}` + ); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it('should fetch the blob without authenticating (public endpoint)', async () => { + const blob = new Blob(['image-bytes'], { type: 'image/jpeg' }); + fetchMock.mockResolvedValueOnce({ + ok: true, + status: 200, + statusText: 'OK', + blob: vi.fn().mockResolvedValue(blob), + }); + + const result = await fileService.getFileContentByUniqueId(uniqueId); + + // This endpoint is documented as requiring no authentication - assert that + // no token work happens and no Authorization header is attached. + expect(mockClient.ensureValidToken).not.toHaveBeenCalled(); + expect(mockHttpClient.buildUrl).toHaveBeenCalledWith(`/files/${uniqueId}`, {}); + expect(fetchMock).toHaveBeenCalledWith( + `https://mp.example.org/api/files/${uniqueId}`, + { method: 'GET' } + ); + expect(result).toBe(blob); + }); + + it('should request the thumbnail variant when thumbnail is true', async () => { + fetchMock.mockResolvedValueOnce({ + ok: true, + blob: vi.fn().mockResolvedValue(new Blob(['thumb'])), + }); + + await fileService.getFileContentByUniqueId(uniqueId, true); + + expect(mockHttpClient.buildUrl).toHaveBeenCalledWith(`/files/${uniqueId}`, { + $thumbnail: 'true', + }); + }); + + it('should send $thumbnail=false when thumbnail is explicitly false', async () => { + fetchMock.mockResolvedValueOnce({ + ok: true, + blob: vi.fn().mockResolvedValue(new Blob(['full'])), + }); + + await fileService.getFileContentByUniqueId(uniqueId, false); + + expect(mockHttpClient.buildUrl).toHaveBeenCalledWith(`/files/${uniqueId}`, { + $thumbnail: 'false', + }); + }); + + it('should throw with status and statusText on a non-OK response', async () => { + fetchMock.mockResolvedValueOnce({ + ok: false, + status: 404, + statusText: 'Not Found', + blob: vi.fn(), + }); + + await expect(fileService.getFileContentByUniqueId(uniqueId)).rejects.toThrow( + `GET /files/${uniqueId} failed: 404 Not Found` + ); + }); + + it('should re-throw network failures unchanged', async () => { + fetchMock.mockRejectedValueOnce(new TypeError('fetch failed')); + + await expect(fileService.getFileContentByUniqueId(uniqueId)).rejects.toThrow('fetch failed'); + }); + }); + + describe('getFileMetadata', () => { + it('should fetch metadata by numeric file id', async () => { + (mockHttpClient.get as ReturnType).mockResolvedValueOnce(mockFileDescription); + + const result = await fileService.getFileMetadata(501); + + expect(mockClient.ensureValidToken).toHaveBeenCalledTimes(1); + expect(mockHttpClient.get).toHaveBeenCalledWith('/files/501/metadata'); + expect(result).toEqual(mockFileDescription); + }); + + it('should re-throw HTTP errors unchanged', async () => { + (mockHttpClient.get as ReturnType).mockRejectedValueOnce( + new Error('GET /files/501/metadata failed: 404 Not Found') + ); + + await expect(fileService.getFileMetadata(501)).rejects.toThrow('404 Not Found'); + }); + + it('should re-throw token refresh failures without calling the API', async () => { + (mockClient.ensureValidToken as ReturnType).mockRejectedValueOnce( + new Error('Token refresh failed') + ); + + await expect(fileService.getFileMetadata(501)).rejects.toThrow('Token refresh failed'); + expect(mockHttpClient.get).not.toHaveBeenCalled(); + }); + }); + + describe('getFileMetadataByUniqueId', () => { + const uniqueId = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee'; + + it('should fetch metadata by unique file id', async () => { + (mockHttpClient.get as ReturnType).mockResolvedValueOnce(mockFileDescription); + + const result = await fileService.getFileMetadataByUniqueId(uniqueId); + + expect(mockClient.ensureValidToken).toHaveBeenCalledTimes(1); + expect(mockHttpClient.get).toHaveBeenCalledWith(`/files/${uniqueId}/metadata`); + expect(result).toEqual(mockFileDescription); + }); + + it('should re-throw HTTP errors unchanged', async () => { + (mockHttpClient.get as ReturnType).mockRejectedValueOnce( + new Error('GET /files/bad/metadata failed: 404 Not Found') + ); + + await expect(fileService.getFileMetadataByUniqueId('bad')).rejects.toThrow('404 Not Found'); + }); + + it('should re-throw token refresh failures without calling the API', async () => { + (mockClient.ensureValidToken as ReturnType).mockRejectedValueOnce( + new Error('Token refresh failed') + ); + + await expect(fileService.getFileMetadataByUniqueId(uniqueId)).rejects.toThrow( + 'Token refresh failed' + ); + expect(mockHttpClient.get).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/src/lib/providers/ministry-platform/services/metadata.service.test.ts b/src/lib/providers/ministry-platform/services/metadata.service.test.ts new file mode 100644 index 00000000..c60d37ec --- /dev/null +++ b/src/lib/providers/ministry-platform/services/metadata.service.test.ts @@ -0,0 +1,124 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { MetadataService } from '@/lib/providers/ministry-platform/services/metadata.service'; +import type { MinistryPlatformClient } from '@/lib/providers/ministry-platform/client'; +import type { HttpClient } from '@/lib/providers/ministry-platform/utils/http-client'; +import type { TableMetadata } from '@/lib/providers/ministry-platform/types'; + +/** + * MetadataService Tests + * + * Covers: + * - refreshMetadata -> GET /refreshMetadata (fire-and-forget, returns void) + * - getTables -> GET /tables, with the optional $search parameter + * + * The $search branch matters: `search ? { $search: search } : undefined` means + * an empty string is treated as "no search", which is asserted below so the + * behavior is pinned rather than incidental. + */ +describe('MetadataService', () => { + let metadataService: MetadataService; + let mockClient: MinistryPlatformClient; + let mockHttpClient: HttpClient; + + beforeEach(() => { + vi.clearAllMocks(); + vi.spyOn(console, 'error').mockImplementation(() => {}); + + mockHttpClient = { + get: vi.fn(), + post: vi.fn(), + put: vi.fn(), + delete: vi.fn(), + buildUrl: vi.fn(), + postFormData: vi.fn(), + putFormData: vi.fn(), + } as unknown as HttpClient; + + mockClient = { + ensureValidToken: vi.fn().mockResolvedValue(undefined), + getHttpClient: vi.fn().mockReturnValue(mockHttpClient), + } as unknown as MinistryPlatformClient; + + metadataService = new MetadataService(mockClient); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + describe('refreshMetadata', () => { + it('should trigger the metadata cache refresh', async () => { + (mockHttpClient.get as ReturnType).mockResolvedValueOnce(undefined); + + await expect(metadataService.refreshMetadata()).resolves.toBeUndefined(); + + expect(mockClient.ensureValidToken).toHaveBeenCalledTimes(1); + expect(mockHttpClient.get).toHaveBeenCalledWith('/refreshMetadata'); + }); + + it('should re-throw HTTP errors unchanged', async () => { + (mockHttpClient.get as ReturnType).mockRejectedValueOnce( + new Error('GET /refreshMetadata failed: 503 Service Unavailable') + ); + + await expect(metadataService.refreshMetadata()).rejects.toThrow('503 Service Unavailable'); + }); + + it('should re-throw token refresh failures without calling the API', async () => { + (mockClient.ensureValidToken as ReturnType).mockRejectedValueOnce( + new Error('Token refresh failed') + ); + + await expect(metadataService.refreshMetadata()).rejects.toThrow('Token refresh failed'); + expect(mockHttpClient.get).not.toHaveBeenCalled(); + }); + }); + + describe('getTables', () => { + const mockTables: TableMetadata[] = [ + { Table_ID: 1, Table_Name: 'Contacts', Display_Name: 'Contacts' }, + { Table_ID: 2, Table_Name: 'Contact_Log', Display_Name: 'Contact Log' }, + ]; + + it('should list all tables when no search term is given', async () => { + (mockHttpClient.get as ReturnType).mockResolvedValueOnce(mockTables); + + const result = await metadataService.getTables(); + + expect(mockClient.ensureValidToken).toHaveBeenCalledTimes(1); + expect(mockHttpClient.get).toHaveBeenCalledWith('/tables', undefined); + expect(result).toEqual(mockTables); + }); + + it('should pass $search when a search term is given', async () => { + (mockHttpClient.get as ReturnType).mockResolvedValueOnce([mockTables[0]]); + + const result = await metadataService.getTables('Contact'); + + expect(mockHttpClient.get).toHaveBeenCalledWith('/tables', { $search: 'Contact' }); + expect(result).toHaveLength(1); + }); + + it('should treat an empty search string as no search', async () => { + (mockHttpClient.get as ReturnType).mockResolvedValueOnce(mockTables); + + await metadataService.getTables(''); + + expect(mockHttpClient.get).toHaveBeenCalledWith('/tables', undefined); + }); + + it('should return an empty array when nothing matches', async () => { + (mockHttpClient.get as ReturnType).mockResolvedValueOnce([]); + + await expect(metadataService.getTables('NoSuchTable')).resolves.toEqual([]); + }); + + it('should re-throw HTTP errors unchanged', async () => { + (mockHttpClient.get as ReturnType).mockRejectedValueOnce( + new Error('GET /tables failed: 401 Unauthorized') + ); + + await expect(metadataService.getTables()).rejects.toThrow('401 Unauthorized'); + }); + }); +}); diff --git a/src/lib/providers/ministry-platform/services/procedure.service.test.ts b/src/lib/providers/ministry-platform/services/procedure.service.test.ts new file mode 100644 index 00000000..e5bda245 --- /dev/null +++ b/src/lib/providers/ministry-platform/services/procedure.service.test.ts @@ -0,0 +1,223 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { ProcedureService } from '@/lib/providers/ministry-platform/services/procedure.service'; +import type { MinistryPlatformClient } from '@/lib/providers/ministry-platform/client'; +import type { HttpClient } from '@/lib/providers/ministry-platform/utils/http-client'; +import type { ProcedureInfo } from '@/lib/providers/ministry-platform/types'; + +/** + * ProcedureService Tests + * + * Covers: + * - getProcedures -> GET /procs (optional $search) + * - executeProcedure -> GET /procs/{name}, params in the query string + * - executeProcedureWithBody -> POST /procs/{name}, params in the body + * + * The procedure name is the only caller-supplied value interpolated into the + * endpoint path, so the encodeURIComponent behavior is asserted explicitly for + * names containing spaces and slashes. + * + * Stored procedures can mutate MP data. Every call here goes to a mocked + * HttpClient; nothing reaches a real Ministry Platform instance. + */ +describe('ProcedureService', () => { + let procedureService: ProcedureService; + let mockClient: MinistryPlatformClient; + let mockHttpClient: HttpClient; + + beforeEach(() => { + vi.clearAllMocks(); + vi.spyOn(console, 'log').mockImplementation(() => {}); + vi.spyOn(console, 'error').mockImplementation(() => {}); + + mockHttpClient = { + get: vi.fn(), + post: vi.fn(), + put: vi.fn(), + delete: vi.fn(), + buildUrl: vi.fn(), + postFormData: vi.fn(), + putFormData: vi.fn(), + } as unknown as HttpClient; + + mockClient = { + ensureValidToken: vi.fn().mockResolvedValue(undefined), + getHttpClient: vi.fn().mockReturnValue(mockHttpClient), + } as unknown as MinistryPlatformClient; + + procedureService = new ProcedureService(mockClient); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + describe('getProcedures', () => { + const mockProcedures: ProcedureInfo[] = [ + { Name: 'api_Custom_Get_Contacts', Parameters: [] }, + ]; + + it('should list procedures when no search term is given', async () => { + (mockHttpClient.get as ReturnType).mockResolvedValueOnce(mockProcedures); + + const result = await procedureService.getProcedures(); + + expect(mockClient.ensureValidToken).toHaveBeenCalledTimes(1); + expect(mockHttpClient.get).toHaveBeenCalledWith('/procs', undefined); + expect(result).toEqual(mockProcedures); + }); + + it('should pass $search when a search term is given', async () => { + (mockHttpClient.get as ReturnType).mockResolvedValueOnce(mockProcedures); + + await procedureService.getProcedures('api_Custom'); + + expect(mockHttpClient.get).toHaveBeenCalledWith('/procs', { $search: 'api_Custom' }); + }); + + it('should treat an empty search string as no search', async () => { + (mockHttpClient.get as ReturnType).mockResolvedValueOnce(mockProcedures); + + await procedureService.getProcedures(''); + + expect(mockHttpClient.get).toHaveBeenCalledWith('/procs', undefined); + }); + + it('should re-throw HTTP errors unchanged', async () => { + (mockHttpClient.get as ReturnType).mockRejectedValueOnce( + new Error('GET /procs failed: 401 Unauthorized') + ); + + await expect(procedureService.getProcedures()).rejects.toThrow('401 Unauthorized'); + }); + }); + + describe('executeProcedure', () => { + // MP returns one array per result set + const mockResults = [[{ Contact_ID: 1, Display_Name: 'John Doe' }]]; + + it('should execute a procedure with no parameters', async () => { + (mockHttpClient.get as ReturnType).mockResolvedValueOnce(mockResults); + + const result = await procedureService.executeProcedure('api_Custom_Get_Contacts'); + + expect(mockClient.ensureValidToken).toHaveBeenCalledTimes(1); + expect(mockHttpClient.get).toHaveBeenCalledWith( + '/procs/api_Custom_Get_Contacts', + undefined + ); + expect(result).toEqual(mockResults); + }); + + it('should pass query parameters through', async () => { + (mockHttpClient.get as ReturnType).mockResolvedValueOnce(mockResults); + + await procedureService.executeProcedure('api_Custom_Get_Contacts', { + '@ContactID': 42, + '@IncludeInactive': false, + }); + + expect(mockHttpClient.get).toHaveBeenCalledWith('/procs/api_Custom_Get_Contacts', { + '@ContactID': 42, + '@IncludeInactive': false, + }); + }); + + it('should URL-encode a procedure name containing a space', async () => { + (mockHttpClient.get as ReturnType).mockResolvedValueOnce([]); + + await procedureService.executeProcedure('api Custom Proc'); + + expect(mockHttpClient.get).toHaveBeenCalledWith('/procs/api%20Custom%20Proc', undefined); + }); + + it('should URL-encode path separators in the procedure name', async () => { + (mockHttpClient.get as ReturnType).mockResolvedValueOnce([]); + + await procedureService.executeProcedure('evil/../admin'); + + expect(mockHttpClient.get).toHaveBeenCalledWith('/procs/evil%2F..%2Fadmin', undefined); + }); + + it('should return an empty result set unchanged', async () => { + (mockHttpClient.get as ReturnType).mockResolvedValueOnce([]); + + await expect(procedureService.executeProcedure('api_Empty')).resolves.toEqual([]); + }); + + it('should re-throw HTTP errors unchanged', async () => { + (mockHttpClient.get as ReturnType).mockRejectedValueOnce( + new Error('GET /procs/api_Bad failed: 400 Bad Request') + ); + + await expect(procedureService.executeProcedure('api_Bad')).rejects.toThrow('400 Bad Request'); + }); + + it('should re-throw token refresh failures without calling the API', async () => { + (mockClient.ensureValidToken as ReturnType).mockRejectedValueOnce( + new Error('Token refresh failed') + ); + + await expect(procedureService.executeProcedure('api_Any')).rejects.toThrow( + 'Token refresh failed' + ); + expect(mockHttpClient.get).not.toHaveBeenCalled(); + }); + }); + + describe('executeProcedureWithBody', () => { + const mockResults = [[{ Rows_Affected: 1 }]]; + + it('should POST parameters in the request body', async () => { + (mockHttpClient.post as ReturnType).mockResolvedValueOnce(mockResults); + + const result = await procedureService.executeProcedureWithBody('api_Custom_Update', { + '@ContactID': 42, + '@Notes': 'Updated', + }); + + expect(mockClient.ensureValidToken).toHaveBeenCalledTimes(1); + expect(mockHttpClient.post).toHaveBeenCalledWith('/procs/api_Custom_Update', { + '@ContactID': 42, + '@Notes': 'Updated', + }); + expect(result).toEqual(mockResults); + }); + + it('should accept an empty parameter object', async () => { + (mockHttpClient.post as ReturnType).mockResolvedValueOnce([]); + + await procedureService.executeProcedureWithBody('api_NoArgs', {}); + + expect(mockHttpClient.post).toHaveBeenCalledWith('/procs/api_NoArgs', {}); + }); + + it('should URL-encode the procedure name', async () => { + (mockHttpClient.post as ReturnType).mockResolvedValueOnce([]); + + await procedureService.executeProcedureWithBody('api Custom Proc', {}); + + expect(mockHttpClient.post).toHaveBeenCalledWith('/procs/api%20Custom%20Proc', {}); + }); + + it('should re-throw HTTP errors unchanged', async () => { + (mockHttpClient.post as ReturnType).mockRejectedValueOnce( + new Error('POST /procs/api_Bad failed: 500 Internal Server Error') + ); + + await expect( + procedureService.executeProcedureWithBody('api_Bad', {}) + ).rejects.toThrow('500 Internal Server Error'); + }); + + it('should re-throw token refresh failures without calling the API', async () => { + (mockClient.ensureValidToken as ReturnType).mockRejectedValueOnce( + new Error('Token refresh failed') + ); + + await expect( + procedureService.executeProcedureWithBody('api_Any', {}) + ).rejects.toThrow('Token refresh failed'); + expect(mockHttpClient.post).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/src/lib/providers/ministry-platform/utils/http-client.test.ts b/src/lib/providers/ministry-platform/utils/http-client.test.ts index d261a3b7..ebb1b09a 100644 --- a/src/lib/providers/ministry-platform/utils/http-client.test.ts +++ b/src/lib/providers/ministry-platform/utils/http-client.test.ts @@ -355,6 +355,39 @@ describe('HttpClient', () => { ); expect(result).toEqual({ FileId: 1, FileName: 'updated.txt' }); }); + + it('should throw on a non-OK PUT FormData response', async () => { + const formData = new FormData(); + formData.append('file', new Blob(['updated']), 'updated.txt'); + + mockFetch.mockResolvedValueOnce({ + ok: false, + status: 413, + statusText: 'Payload Too Large', + json: () => Promise.resolve({}), + }); + + await expect(httpClient.putFormData('/files/1', formData)).rejects.toThrow( + 'PUT /files/1 failed: 413 Payload Too Large' + ); + }); + + it('should append query params to a PUT FormData request', async () => { + const formData = new FormData(); + formData.append('file', new Blob(['updated']), 'updated.txt'); + + mockFetch.mockResolvedValueOnce({ + ok: true, + json: () => Promise.resolve({ FileId: 1 }), + }); + + await httpClient.putFormData('/files/1', formData, { $userId: 7 }); + + expect(mockFetch).toHaveBeenCalledWith( + 'https://api.ministryplatform.com/files/1?$userId=7', + expect.objectContaining({ method: 'PUT' }) + ); + }); }); describe('DELETE Requests', () => { diff --git a/src/lib/utils.test.ts b/src/lib/utils.test.ts new file mode 100644 index 00000000..41f4e636 --- /dev/null +++ b/src/lib/utils.test.ts @@ -0,0 +1,44 @@ +import { describe, it, expect } from 'vitest'; +import { cn } from '@/lib/utils'; + +/** + * cn() Tests + * + * The class-name merge helper used by every component in the app. It is a + * one-liner, but the twMerge half is what makes `cn(base, override)` actually + * override rather than emit both classes — worth pinning, since swapping it for + * a plain clsx call would leave both classes in the DOM and let CSS source + * order decide the winner. + */ +describe('cn', () => { + it('should join multiple class strings', () => { + expect(cn('px-2', 'py-1')).toBe('px-2 py-1'); + }); + + it('should let a later Tailwind class override an earlier conflicting one', () => { + expect(cn('px-2', 'px-4')).toBe('px-4'); + expect(cn('text-red-500', 'text-blue-500')).toBe('text-blue-500'); + }); + + it('should keep non-conflicting classes from the same family', () => { + expect(cn('px-2', 'py-4')).toBe('px-2 py-4'); + }); + + it('should drop falsy conditional values', () => { + expect(cn('base', false && 'hidden', null, undefined, '')).toBe('base'); + }); + + it('should include a class from a truthy condition', () => { + const isActive = true; + expect(cn('base', isActive && 'font-bold')).toBe('base font-bold'); + }); + + it('should flatten arrays and objects the way clsx does', () => { + expect(cn(['px-2', 'py-1'])).toBe('px-2 py-1'); + expect(cn({ 'text-sm': true, 'text-lg': false })).toBe('text-sm'); + }); + + it('should return an empty string with no arguments', () => { + expect(cn()).toBe(''); + }); +}); diff --git a/src/services/domainTimezoneService.test.ts b/src/services/domainTimezoneService.test.ts index 4d22bbb4..eb71d0e0 100644 --- a/src/services/domainTimezoneService.test.ts +++ b/src/services/domainTimezoneService.test.ts @@ -44,6 +44,8 @@ describe("resolveIanaTimezone", () => { it("throws for unknown identifiers rather than silently falling back", () => { expect(() => resolveIanaTimezone("Atlantis Standard Time")).toThrow(/Unknown time zone/); expect(() => resolveIanaTimezone("")).toThrow(); + // Whitespace-only trims to empty and takes the same required-identifier path. + expect(() => resolveIanaTimezone(" ")).toThrow(/Time zone identifier is required/); }); }); @@ -167,4 +169,32 @@ describe("DomainTimezoneService", () => { expect(instant.toISOString()).toBe("2026-05-17T03:33:00.000Z"); }); }); + + it("throws for a value that has a zone marker but is not a real date", async () => { + const svc = freshService(); + // Has an offset marker, so it skips the wall-clock path and goes straight to + // `new Date(value)` — which yields Invalid Date rather than throwing on its own. + await expect(svc.parseMpDatetime("not-a-date+05:00")).rejects.toThrow( + /unable to parse/ + ); + expect(mockGetDomainInfo).not.toHaveBeenCalled(); + }); + }); + + describe("clearCache", () => { + it("forces the next call to refetch the domain time zone", async () => { + mockGetDomainInfo + .mockResolvedValueOnce({ TimeZoneName: "America/New_York" }) + .mockResolvedValueOnce({ TimeZoneName: "America/Chicago" }); + const svc = freshService(); + + expect(await svc.getMpTimezone()).toBe("America/New_York"); + expect(await svc.getMpTimezone()).toBe("America/New_York"); + expect(mockGetDomainInfo).toHaveBeenCalledTimes(1); + + svc.clearCache(); + + expect(await svc.getMpTimezone()).toBe("America/Chicago"); + expect(mockGetDomainInfo).toHaveBeenCalledTimes(2); + }); }); diff --git a/vitest.config.ts b/vitest.config.ts index 52a02689..b228ec05 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -13,13 +13,70 @@ export default defineConfig({ coverage: { provider: 'v8', reporter: ['text', 'json', 'html'], + + // This explicit `include` is load-bearing. With no `include`, v8 reports + // only on files some test actually imported, so every untested file drops + // out of the denominator and the headline percentage is inflated — it read + // 71.6% while true statement coverage was 32.7%. Naming the globs puts the + // untested files back in the denominator. (Vitest 3's `coverage.all` flag + // is gone in Vitest 4; `include` replaces it.) + include: ['src/**/*.{ts,tsx}'], + exclude: [ 'node_modules/', '.next/', 'src/test-setup.ts', '**/*.d.ts', + '**/*.test.{ts,tsx}', 'src/lib/providers/ministry-platform/models/', // Auto-generated files + 'src/lib/providers/ministry-platform/scripts/', // Dev-only codegen, run manually + + // Thin shadcn/Radix wrappers — excluded from the denominator entirely. + // Testing them asserts that Radix works. Feature components (*.tsx) are + // NOT excluded: they stay visible in the report, just ungated. + 'src/components/ui/', ], + + // Ratchet for non-UI functional code: services, the MP provider, server + // actions, contexts, and auth/proxy plumbing. Set just under the achieved + // figures (98.8% stmts / 94.5% branch / 97.6% funcs / 99.0% lines) so an + // ordinary refactor has room but a real regression fails the run. + // + // React components and app routes are deliberately NOT gated: they are + // excluded from these globs rather than from the report, so `npm run + // test:coverage` still shows their (currently 0%) numbers. + thresholds: { + 'src/services/**': { + statements: 95, + branches: 90, + functions: 95, + lines: 95, + }, + 'src/lib/**/*.ts': { + statements: 95, + branches: 85, + functions: 90, + lines: 95, + }, + 'src/components/**/actions.ts': { + statements: 95, + branches: 85, + functions: 95, + lines: 95, + }, + 'src/contexts/**': { + statements: 95, + branches: 85, + functions: 95, + lines: 95, + }, + 'src/proxy.ts': { + statements: 100, + branches: 100, + functions: 100, + lines: 100, + }, + }, }, }, resolve: { From 7eced35ff5024b666f23cc1873e6cde448740887 Mon Sep 17 00:00:00 2001 From: Chris Kehayias Date: Fri, 21 Aug 2026 07:12:47 -0400 Subject: [PATCH 2/2] docs(todo): record the pre-existing ajv lockfile drift breaking CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI has been red on main since 64f18f0 ("Package Update Cleanup"): `npm ci` fails in ~8s on an ajv lockfile mismatch, before any test runs. Found while verifying this branch — the failure is byte-identical on main, and this branch touches neither package.json nor package-lock.json. Cause: eslint wants ajv@^6, @hookform/resolvers wants ajv@^8, and the lockfile carries only one ajv entry (6.15.0). The nested ajv@8.x subtree, plus fast-uri and json-schema-traverse@1.0.0, is missing. Same mechanism as investigate-emnapi-lockfile-drift.md — a Windows install pruning nested entries that npm ci on Linux then requires. The TODO notes that the regeneration must happen on Linux (or with --os=linux --cpu=x64), since fixing ajv on Windows risks reintroducing the emnapi drift in the same commit. Co-Authored-By: Claude Opus 5 (1M context) --- .claude/TODO/ci-broken-ajv-lockfile-drift.md | 81 ++++++++++++++++++++ 1 file changed, 81 insertions(+) create mode 100644 .claude/TODO/ci-broken-ajv-lockfile-drift.md diff --git a/.claude/TODO/ci-broken-ajv-lockfile-drift.md b/.claude/TODO/ci-broken-ajv-lockfile-drift.md new file mode 100644 index 00000000..fbd8737b --- /dev/null +++ b/.claude/TODO/ci-broken-ajv-lockfile-drift.md @@ -0,0 +1,81 @@ +# TODO: CI is red on `main` — `npm ci` fails on `ajv` lockfile drift + +**Created:** 2026-08-21 +**Severity:** High — **every** CI run on `main` and on every branch fails at the install step. No PR can be verified by CI until this is fixed. +**Status:** Open. Pre-existing; discovered while pushing unit-test coverage (PR #71), unrelated to that work. + +## Symptom + +`npm ci` fails in ~8s on GitHub Actions, before any test runs: + +``` +npm error `npm ci` can only install packages when your package.json and +npm error package-lock.json or npm-shrinkwrap.json are in sync. +npm error Invalid: lock file's ajv@6.15.0 does not satisfy ajv@8.20.0 +npm error Missing: ajv@6.15.0 from lock file +npm error Missing: fast-uri@3.1.5 from lock file +npm error Invalid: lock file's json-schema-traverse@0.4.1 does not satisfy json-schema-traverse@1.0.0 +npm error Missing: json-schema-traverse@0.4.1 from lock file +``` + +## When it started + +Introduced by `64f18f0` ("Package Update Cleanup"). The run immediately before it +(`32470332071`, merge of PR #70) was green; `32472743169` on `64f18f0` is red with this +error, and every run since has failed identically. + +Verified byte-identical between the `main` run and PR #71's run, and PR #71 touches neither +`package.json` nor `package-lock.json` — so this is not branch-specific. + +## Cause + +Two packages want different `ajv` majors: + +| Package | Requires | +|---|---| +| `eslint` | `ajv@^6.14.0` | +| `@hookform/resolvers` | `ajv@^8` | + +`package-lock.json` contains exactly **one** `node_modules/ajv` entry, pinned to `6.15.0` +(line ~5242). The nested `ajv@8.x` entry that `@hookform/resolvers` needs is absent, along +with its `fast-uri@3.1.5` and `json-schema-traverse@1.0.0` subtree. + +This is the same class of failure as `.claude/TODO/investigate-emnapi-lockfile-drift.md`: a +Windows `npm install` / `npm dedupe` pruned nested entries out of the lockfile, and `npm ci` +on Linux then refuses to proceed. `ajv` is a different victim, same mechanism. + +## Fix + +Regenerate the lockfile without touching `node_modules`, then confirm both `ajv` trees survive: + +```bash +npm install --package-lock-only +git diff package-lock.json # expect a nested ajv@8.x under @hookform/resolvers +``` + +**Do this in WSL, a Linux container, or with `--os=linux --cpu=x64`.** A bare +`npm install` on Windows is what caused this, and the emnapi TODO documents it re-pruning +Linux-only optional entries — fixing `ajv` on Windows risks reintroducing that drift in the +same commit. + +Then verify the way CI does, on Linux: + +```bash +rm -rf node_modules && npm ci && npm run test:run +``` + +## Worth doing alongside + +Both incidents share one root cause: lockfiles are generated on Windows and consumed on +Linux. Options in `.claude/TODO/investigate-emnapi-lockfile-drift.md` §"Things to try" apply +verbatim here — in particular a CI guard or pre-commit hook that runs `npm ci --dry-run` +before a lockfile change can reach `main`. That would have caught both incidents at the +commit that introduced them rather than one merge later. + +The `/audit-deps` skill is the natural home for this check. + +## Related + +- `.claude/TODO/investigate-emnapi-lockfile-drift.md` — same mechanism, different packages +- `64f18f0` — the commit that introduced it +- Failing run on `main`: https://github.com/MinistryPlatform-Community/MPNext/actions/runs/32472743169