From bbfb2c3cc533aba743e6506e1c282a4648ae50dc Mon Sep 17 00:00:00 2001 From: Chris Kehayias Date: Sun, 13 Sep 2026 09:26:05 -0400 Subject: [PATCH] test: raise coverage 49.67% -> 98.84%, fix the config that hid the gap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reported coverage figure was wrong. `vitest.config.ts` set no `coverage.include`, so the v8 provider only instrumented files that some test had already imported — 131 of 176 source files were absent from the report entirely rather than counted as 0%. That reported 83.14% while true coverage was 48.41%, and it is what CI uploads to Codecov. It is why `src/services/familyService.ts` (756 lines) and `src/app/(web)/tools/layout.tsx` (the /tools authorization gate) could sit at zero tests without anyone seeing it. Coverage over authored code, measured on the same denominator throughout: Statements 49.67% -> 98.84% (3,610/3,652) Lines 50.20% -> 99.70% (3,363/3,373) Branches 37.51% -> 92.21% Functions 38.35% -> 98.81% Test files 50 -> 116 Test cases 812 -> 1,535 Scope excludes vendored shadcn/ui primitives, generated MP models, build-time scripts, loading.tsx skeletons, barrels and type-only modules. Config - `coverage.include: ['src/**/*.{ts,tsx}']` — the actual fix. Note `coverage.all` was REMOVED in Vitest 5: setting it is a type error and does nothing, so `include` is the only lever. - Directory exclusions now end in `**`. A bare `'src/components/ui/'` matches nothing, which had quietly kept those files in the denominator. - `thresholds: { statements: 97, lines: 98 }`, enforced by the existing test:coverage CI job. Branches and functions are left unenforced: a single defensive guard can trip them, and a threshold people learn to override is worse than none. Verified the gate fires by setting it to an impossible value and confirming exit 1. - Renamed `vitest.config.ts` -> `.mts`. As `.ts` it was loaded as CommonJS and Vite warned it breaks when `configLoader: 'native'` becomes the default. The rename exposed a second instance of the same problem: `__dirname` only worked because of the CJS load, and as real ESM it would have thrown at config load and stopped every test from starting. Now uses `import.meta.dirname`. Build was already broken on dev `tsconfig.json` includes `**/*.ts`, `next.config.ts` sets no `ignoreBuildErrors`, and CI never runs `next build` — so two committed test files failed type checking while CI stayed green. Their fixtures built `CommunicationInfo`/`MessageInfo`/`FileUploadParams` values out of raw MP column names (`Author_User_ID`, `From_Contact`) instead of the DTO fields the helper requires. The tests passed because the mocks never inspected the missing fields. Fixtures now carry explicit type annotations so the compiler enforces the shape; `npm run build` succeeds. Runner output: 1,507 lines -> 17, zero warnings - Barcode tests mocked react-pdf's `View` as the string 'View', which React read as a capitalised unknown tag: two warnings per element, 797 lines for one 65-bar barcode. Mocked as `rpdf-view` (the dash matters — any undashed unknown tag still draws "unrecognized in this browser"). - Added console spies to the 15 test files that deliberately drive error paths. The MP logger is unconditional by design, so these were expected logs plus stack traces. The spies use `mockImplementation`, so they still record and assertions on what was logged keep working. Source fix (14 lines) `Select` fields in the group wizard and Add/Edit Family passed `value={... : undefined}`, leaving them uncontrolled until first selection and logging a React controlled/uncontrolled warning. Silencing that in tests would have hidden a real bug: while a field is uncontrolled, React state is not the source of truth for it, so `form.reset()` between wizard runs is not guaranteed to clear the trigger. Changed to `""`, Radix's documented "no selection" value. Also adds @testing-library/user-event (the standard RTL companion, not previously installed). TODOs: 8 opened, 4 resolved. 9 open — 3 high (filter injection in FamilyService, field loss in removeGroup, address PII into logs from mergeTemplate), 4 medium, 2 low. Docs updated: CLAUDE.md, DECISIONS.md, testing references, and a new facts snapshot. Verified: test:run exit 0 (1,535 passing) · test:coverage exit 0 · tsc --noEmit 0 errors · eslint 0 problems · next build exit 0. Co-Authored-By: Claude Opus 5 (1M context) --- ...omponents-template-editor-missing-tests.md | 39 +- ...nents-select-uncontrolled-to-controlled.md | 71 ++ ...-13-dead-empty-fields-branch-handlenext.md | 66 ++ ...mergetemplate-logs-address-pii-on-error.md | 58 ++ .../2026-09-13-page-logs-raw-error-object.md | 62 ++ ...-09-13-removegroup-order-guard-mismatch.md | 89 ++ ...-09-13-search-empty-state-never-renders.md | 51 + ...g-coverage-report-masked-untested-files.md | 64 ++ ...6-09-13-testing-no-typecheck-gate-in-ci.md | 85 ++ ...-13-testing-vitest-config-loaded-as-cjs.md | 77 ++ ...nvalidated-envelope-donor-ids-in-filter.md | 77 ++ .claude/TODO/INDEX.md | 95 +- .claude/references/DECISIONS.md | 4 +- .claude/references/_meta/facts/2026-09-13.md | 74 ++ .claude/references/testing/README.md | 8 +- .claude/references/testing/inventory.md | 28 +- .claude/references/testing/setup.md | 65 +- CLAUDE.md | 5 +- package-lock.json | 15 + package.json | 1 + src/app/(web)/no-access/page.test.tsx | 36 + src/app/(web)/page.test.tsx | 47 + .../(web)/tools/addeditfamily/actions.test.ts | 384 ++++++++ .../addeditfamily/add-edit-family.test.tsx | 931 ++++++++++++++++++ .../tools/addeditfamily/add-edit-family.tsx | 2 +- .../(web)/tools/addeditfamily/page.test.tsx | 152 +++ .../addresslabels/address-labels.test.tsx | 357 +++++++ .../(web)/tools/addresslabels/page.test.tsx | 36 + .../fieldmanagement/field-management.test.tsx | 314 ++++++ .../(web)/tools/fieldmanagement/page.test.tsx | 47 + .../tools/groupwizard/group-wizard.test.tsx | 223 +++++ src/app/(web)/tools/groupwizard/page.test.tsx | 61 ++ src/app/(web)/tools/layout.test.tsx | 94 ++ src/app/(web)/tools/template/page.test.tsx | 47 + .../tools/template/template-tool.test.tsx | 139 +++ .../(web)/tools/templateeditor/page.test.tsx | 38 + .../templateeditor/template-editor.test.tsx | 86 ++ src/app/global-error.test.tsx | 73 ++ src/app/providers.test.tsx | 62 ++ src/app/session-error/page.test.tsx | 50 + src/app/signin/page.test.tsx | 149 +++ src/auth.test.ts | 155 +++ src/components/address-labels/actions.test.ts | 136 ++- .../address-labels/address-label.test.tsx | 109 ++ .../address-labels-form.test.tsx | 185 ++++ .../address-labels-summary.test.tsx | 77 ++ .../address-labels/imb-barcode.test.tsx | 65 ++ .../address-labels/label-document.test.tsx | 96 ++ .../address-labels/mail-merge-tab.test.tsx | 227 +++++ .../address-labels/postnet-barcode.test.tsx | 56 ++ .../address-labels/sample-template.test.ts | 60 ++ .../address-labels/word-document.test.ts | 2 +- src/components/dev-panel/dev-panel.test.tsx | 51 +- .../panels/contact-records-actions.test.ts | 60 ++ .../panels/contact-records-panel.test.tsx | 146 +++ .../panels/deploy-tool-panel.test.tsx | 445 +++++++++ .../dev-panel/panels/params-panel.test.tsx | 92 ++ .../panels/require-dev-session.test.ts | 88 ++ .../dev-panel/panels/selection-panel.test.tsx | 135 +++ .../panels/user-tools-panel.test.tsx | 128 +++ .../field-order-editor.test.tsx | 239 +++++ .../new-group-dialog.test.tsx | 118 +++ .../field-management/page-search.test.tsx | 145 +++ .../sortable-field-item.test.tsx | 311 ++++++ .../field-management/sortable-group.test.tsx | 296 ++++++ .../use-field-order-state.test.ts | 494 +++++++++- .../group-wizard/contact-search.test.tsx | 171 ++++ .../group-wizard/group-search.test.tsx | 194 ++++ .../group-wizard/step-attributes.test.tsx | 180 ++++ .../group-wizard/step-attributes.tsx | 8 +- .../group-wizard/step-identity.test.tsx | 189 ++++ src/components/group-wizard/step-identity.tsx | 4 +- .../group-wizard/step-meeting.test.tsx | 157 +++ src/components/group-wizard/step-meeting.tsx | 8 +- .../group-wizard/step-organization.test.tsx | 212 ++++ .../group-wizard/step-organization.tsx | 6 +- .../group-wizard/step-review.test.tsx | 247 +++++ .../group-wizard/step-settings.test.tsx | 190 ++++ .../group-wizard/wizard-navigation.test.tsx | 112 +++ .../group-wizard/wizard-stepper.test.tsx | 88 ++ src/components/shared-actions/domain.test.ts | 50 + .../template-editor/actions.test.ts | 112 +++ .../template-editor/editor-canvas.test.tsx | 140 +++ .../editor-code-dialog.test.tsx | 144 +++ .../editor-export-dialog.test.tsx | 153 +++ .../editor-import-dialog.test.tsx | 106 ++ .../template-editor/editor-toolbar.test.tsx | 327 ++++++ .../template-editor/grapes-config.test.ts | 51 + .../merge-field-picker.test.tsx | 136 +++ .../template-editor/merge-fields.test.ts | 60 ++ .../template-editor-form.test.tsx | 85 ++ src/components/tool/tool-framework.test.tsx | 160 +++ src/contexts/user-context.test.tsx | 90 ++ src/lib/auth-client.test.ts | 35 + src/lib/dto/address-label.dto.test.ts | 37 + src/lib/dto/family.test.ts | 107 ++ .../providers/google-places/provider.test.ts | 219 ++++ .../ministry-platform/client.test.ts | 12 + .../ministry-platform/helper.test.ts | 54 +- .../ministry-platform/provider.test.ts | 37 +- .../services/communication.service.test.ts | 12 + .../services/domain.service.test.ts | 12 + .../services/file.service.test.ts | 12 + .../services/metadata.service.test.ts | 12 + .../services/procedure.service.test.ts | 12 + .../services/table.service.test.ts | 12 + .../utils/http-client.test.ts | 12 + src/proxy.test.ts | 12 + src/services/domainTimezoneService.test.ts | 49 + src/services/familyService.test.ts | 806 +++++++++++++++ src/services/googlePlacesService.test.ts | 146 +++ vitest.config.mts | 71 ++ vitest.config.ts | 30 - 113 files changed, 13226 insertions(+), 131 deletions(-) create mode 100644 .claude/TODO/2026-09-13-components-select-uncontrolled-to-controlled.md create mode 100644 .claude/TODO/2026-09-13-dead-empty-fields-branch-handlenext.md create mode 100644 .claude/TODO/2026-09-13-mergetemplate-logs-address-pii-on-error.md create mode 100644 .claude/TODO/2026-09-13-page-logs-raw-error-object.md create mode 100644 .claude/TODO/2026-09-13-removegroup-order-guard-mismatch.md create mode 100644 .claude/TODO/2026-09-13-search-empty-state-never-renders.md create mode 100644 .claude/TODO/2026-09-13-testing-coverage-report-masked-untested-files.md create mode 100644 .claude/TODO/2026-09-13-testing-no-typecheck-gate-in-ci.md create mode 100644 .claude/TODO/2026-09-13-testing-vitest-config-loaded-as-cjs.md create mode 100644 .claude/TODO/2026-09-13-unvalidated-envelope-donor-ids-in-filter.md create mode 100644 .claude/references/_meta/facts/2026-09-13.md create mode 100644 src/app/(web)/no-access/page.test.tsx create mode 100644 src/app/(web)/page.test.tsx create mode 100644 src/app/(web)/tools/addeditfamily/actions.test.ts create mode 100644 src/app/(web)/tools/addeditfamily/add-edit-family.test.tsx create mode 100644 src/app/(web)/tools/addeditfamily/page.test.tsx create mode 100644 src/app/(web)/tools/addresslabels/address-labels.test.tsx create mode 100644 src/app/(web)/tools/addresslabels/page.test.tsx create mode 100644 src/app/(web)/tools/fieldmanagement/field-management.test.tsx create mode 100644 src/app/(web)/tools/fieldmanagement/page.test.tsx create mode 100644 src/app/(web)/tools/groupwizard/page.test.tsx create mode 100644 src/app/(web)/tools/layout.test.tsx create mode 100644 src/app/(web)/tools/template/page.test.tsx create mode 100644 src/app/(web)/tools/template/template-tool.test.tsx create mode 100644 src/app/(web)/tools/templateeditor/page.test.tsx create mode 100644 src/app/(web)/tools/templateeditor/template-editor.test.tsx create mode 100644 src/app/global-error.test.tsx create mode 100644 src/app/providers.test.tsx create mode 100644 src/app/session-error/page.test.tsx create mode 100644 src/components/address-labels/address-label.test.tsx create mode 100644 src/components/address-labels/address-labels-form.test.tsx create mode 100644 src/components/address-labels/address-labels-summary.test.tsx create mode 100644 src/components/address-labels/imb-barcode.test.tsx create mode 100644 src/components/address-labels/label-document.test.tsx create mode 100644 src/components/address-labels/mail-merge-tab.test.tsx create mode 100644 src/components/address-labels/postnet-barcode.test.tsx create mode 100644 src/components/address-labels/sample-template.test.ts create mode 100644 src/components/dev-panel/panels/contact-records-actions.test.ts create mode 100644 src/components/dev-panel/panels/contact-records-panel.test.tsx create mode 100644 src/components/dev-panel/panels/deploy-tool-panel.test.tsx create mode 100644 src/components/dev-panel/panels/params-panel.test.tsx create mode 100644 src/components/dev-panel/panels/require-dev-session.test.ts create mode 100644 src/components/dev-panel/panels/selection-panel.test.tsx create mode 100644 src/components/dev-panel/panels/user-tools-panel.test.tsx create mode 100644 src/components/field-management/field-order-editor.test.tsx create mode 100644 src/components/field-management/new-group-dialog.test.tsx create mode 100644 src/components/field-management/page-search.test.tsx create mode 100644 src/components/field-management/sortable-field-item.test.tsx create mode 100644 src/components/field-management/sortable-group.test.tsx create mode 100644 src/components/group-wizard/contact-search.test.tsx create mode 100644 src/components/group-wizard/group-search.test.tsx create mode 100644 src/components/group-wizard/step-attributes.test.tsx create mode 100644 src/components/group-wizard/step-identity.test.tsx create mode 100644 src/components/group-wizard/step-meeting.test.tsx create mode 100644 src/components/group-wizard/step-organization.test.tsx create mode 100644 src/components/group-wizard/step-review.test.tsx create mode 100644 src/components/group-wizard/step-settings.test.tsx create mode 100644 src/components/group-wizard/wizard-navigation.test.tsx create mode 100644 src/components/group-wizard/wizard-stepper.test.tsx create mode 100644 src/components/shared-actions/domain.test.ts create mode 100644 src/components/template-editor/actions.test.ts create mode 100644 src/components/template-editor/editor-canvas.test.tsx create mode 100644 src/components/template-editor/editor-code-dialog.test.tsx create mode 100644 src/components/template-editor/editor-export-dialog.test.tsx create mode 100644 src/components/template-editor/editor-import-dialog.test.tsx create mode 100644 src/components/template-editor/editor-toolbar.test.tsx create mode 100644 src/components/template-editor/grapes-config.test.ts create mode 100644 src/components/template-editor/merge-field-picker.test.tsx create mode 100644 src/components/template-editor/merge-fields.test.ts create mode 100644 src/components/template-editor/template-editor-form.test.tsx create mode 100644 src/components/tool/tool-framework.test.tsx create mode 100644 src/lib/auth-client.test.ts create mode 100644 src/lib/dto/address-label.dto.test.ts create mode 100644 src/lib/dto/family.test.ts create mode 100644 src/lib/providers/google-places/provider.test.ts create mode 100644 src/services/familyService.test.ts create mode 100644 src/services/googlePlacesService.test.ts create mode 100644 vitest.config.mts delete mode 100644 vitest.config.ts diff --git a/.claude/TODO/2026-04-17-components-template-editor-missing-tests.md b/.claude/TODO/2026-04-17-components-template-editor-missing-tests.md index 42f1872..5b184da 100644 --- a/.claude/TODO/2026-04-17-components-template-editor-missing-tests.md +++ b/.claude/TODO/2026-04-17-components-template-editor-missing-tests.md @@ -16,7 +16,7 @@ files: - src/components/template-editor/actions.ts discovered: 2026-04-17 discovered_by: components-template-editor -status: open +status: resolved --- ## Problem @@ -39,3 +39,40 @@ Add at minimum: - Regressions in `compileMjml`'s auth gate or size cap can ship unnoticed (security/DoS risk). - Merge-field category changes or taxonomy refactors will not surface breakage. - Phase 2 of the template editor (adding MP persistence, token resolution) will be harder to land safely without a baseline. + +--- + +## Resolution (2026-09-13) +Closed by the coverage push. Every file in `src/components/template-editor/` +now has a co-located test, plus the two route files: + +| File | Tests | Statements | Lines | +|---|---|---|---| +| `merge-fields.ts` | 5 | 100% | 100% | +| `grapes-config.ts` | 6 | 100% | 100% | +| `actions.ts` | 7 | 100% | 100% | +| `merge-field-picker.tsx` | 7 | 100% | 100% | +| `editor-import-dialog.tsx` | 8 | 95.65% | 100% | +| `editor-code-dialog.tsx` | 11 | 97.67% | 100% | +| `editor-export-dialog.tsx` | 11 | 98.08% | 100% | +| `editor-toolbar.tsx` | 21 | 100% | 100% | +| `editor-canvas.tsx` | 7 | 100% | 100% | +| `template-editor-form.tsx` | 3 | 100% | 100% | +| `templateeditor/template-editor.tsx` | 6 | 100% | 100% | +| `templateeditor/page.tsx` | 1 | 100% | 100% | + +Directory aggregate: 98.8% statements, 100% lines, across 90 tests. + +Two mocking notes worth keeping, both learned the hard way: +- `useEditor()` must be mocked with a **stable object reference** built once via + `vi.hoisted()`. Several components run `useEffect`s keyed on the editor + object's identity, so a `useEditor: () => ({...})` factory returns a new + object on every render and spins an infinite render loop that hangs the runner. +- `editor-canvas.tsx` imports real `.css`, which this repo's Vite/PostCSS + config cannot process under test — both stylesheet imports are mocked to + empty modules. + +The two sibling template-editor TODOs (`-no-mp-persistence`, +`-merge-token-resolver`) were re-verified against the code during this work and +remain **accurate and open**. The new tests document current behaviour; they do +not paper over either defect. diff --git a/.claude/TODO/2026-09-13-components-select-uncontrolled-to-controlled.md b/.claude/TODO/2026-09-13-components-select-uncontrolled-to-controlled.md new file mode 100644 index 0000000..e39eaba --- /dev/null +++ b/.claude/TODO/2026-09-13-components-select-uncontrolled-to-controlled.md @@ -0,0 +1,71 @@ +--- +title: Radix Select fields switched from uncontrolled to controlled on first selection +severity: medium +tags: [bug] +area: components +files: [src/components/group-wizard/step-attributes.tsx, src/components/group-wizard/step-identity.tsx, src/components/group-wizard/step-meeting.tsx, src/components/group-wizard/step-organization.tsx, src/app/(web)/tools/addeditfamily/add-edit-family.tsx] +discovered: 2026-09-13 +discovered_by: coverage-review-orchestrator +status: resolved +--- + +## Problem +Fourteen `Select` fields across the group wizard and Add/Edit Family passed +`undefined` as their value while nothing was selected: + +```tsx +value={field.value ? String(field.value) : undefined} +``` + +A Radix `Select` with `value={undefined}` is **uncontrolled**. As soon as the +user picked an option, `field.value` became truthy and the component received a +string — switching to **controlled** mid-life. React logs: + +``` +Select is changing from uncontrolled to controlled. Components should not +switch from controlled to uncontrolled (or vice versa). Decide between using +a controlled or uncontrolled value for the lifetime of the component. +``` + +Beyond the console noise, this is a real correctness problem: while the field +is uncontrolled, React state is not the source of truth for it. Programmatic +resets — `form.reset()` between wizard runs, or clearing a field back to its +empty value — are not guaranteed to be reflected in the rendered trigger, +because the component keeps its own internal value until it becomes controlled. + +The falsy check compounds it. `field.value` of `0` is a legitimate lookup id in +Ministry Platform, and it is falsy, so a genuinely-selected id of 0 would also +have rendered as "no selection". + +## Evidence +- `src/components/group-wizard/step-meeting.tsx:49,95,123,151` +- `src/components/group-wizard/step-attributes.tsx:70,98,126,154` +- `src/components/group-wizard/step-organization.tsx:59,87,162` +- `src/components/group-wizard/step-identity.tsx:64,128` +- `src/app/(web)/tools/addeditfamily/add-edit-family.tsx:1168` + (variant: `value={value > 0 ? String(value) : undefined}`) +- Surfaced by the new step tests — 7 warnings per `vitest run`, e.g. + `step-organization.test.tsx > StepOrganization > selects a congregation`. + +## Fix applied +All fourteen sites now pass `""` instead of `undefined`: + +```tsx +value={field.value ? String(field.value) : ""} +``` + +`""` is Radix's documented "no selection" value for a controlled `Select`: the +`SelectValue` placeholder still renders, and the component is controlled for +its entire lifetime. (`SelectItem` may not use `""` as its own value, which is +what makes it safe as the sentinel.) + +Verified: 1,535 tests pass, `npm run test:run` emits zero warnings, `tsc +--noEmit` is clean, and `npm run build` succeeds. + +## Follow-up worth considering +The `field.value ? ...` truthiness test is still wrong for a legitimate id of +`0`. No current lookup uses 0 as a real id, so this is latent rather than +live — but the correct predicate is `field.value != null`, and a future MP +lookup that does use 0 would fail silently. Left as-is here because changing +the predicate is a behavioural change beyond the warning fix, and it deserves +its own review against the actual lookup data. diff --git a/.claude/TODO/2026-09-13-dead-empty-fields-branch-handlenext.md b/.claude/TODO/2026-09-13-dead-empty-fields-branch-handlenext.md new file mode 100644 index 0000000..643b5f0 --- /dev/null +++ b/.claude/TODO/2026-09-13-dead-empty-fields-branch-handlenext.md @@ -0,0 +1,66 @@ +--- +title: Unreachable "empty STEP_FIELDS" branch in GroupWizard.handleNext +severity: low +tags: [refactor, missing-test] +area: components +files: [src/app/(web)/tools/groupwizard/group-wizard.tsx] +discovered: 2026-09-13 +discovered_by: coverage-agent-group-wizard +status: open +--- + +## Problem +`handleNext` in `group-wizard.tsx` (lines 120-133) special-cases a step whose +`STEP_FIELDS[currentStep]` entry is missing or empty by skipping validation +and advancing immediately: + +```ts +const fields = STEP_FIELDS[currentStep]; +if (!fields || fields.length === 0) { + setCompletedSteps((prev) => new Set(prev).add(currentStep)); + setCurrentStep((prev) => Math.min(prev + 1, WIZARD_STEPS.length - 1)); + return; +} +``` + +The only entry in `STEP_FIELDS` (see `schema.ts`) with an empty array is +index `5` (the Review step, `STEP_FIELDS[5] = []`). But `WizardNavigation` +never renders a "Next" button on the last step (`currentStep === totalSteps - 1`) +— it renders the Submit/"Save Changes" button instead, which calls +`handleSubmit`, not `handleNext`. `STEP_FIELDS` is otherwise fully populated +for indices 0-4, and `currentStep` never exceeds `WIZARD_STEPS.length - 1` +(5), so this branch is unreachable through the actual UI. + +This isn't a security or correctness bug (nothing lets a user skip real +validation — the branch simply can never execute), but it is 3 statements of +dead defensive code that both inflates the file's apparent branch count and +cannot be exercised by any user-facing test, which is why coverage tooling +flags `group-wizard.tsx` at 94.5% statements instead of ≥95% despite every +reachable branch being covered. + +## Evidence +- `src/app/(web)/tools/groupwizard/group-wizard.tsx:120-126` — the + `!fields || fields.length === 0` branch. +- `src/components/group-wizard/schema.ts:58-65` — `STEP_FIELDS`, only index + `5` is empty, and index 5 is the last step. +- `src/components/group-wizard/wizard-navigation.tsx:28,55-75` — `isReviewStep` + gates rendering Submit vs. Next; the last step never shows the Next button + that calls `handleNext`. +- `npx vitest run --coverage` on `group-wizard.tsx` reports lines 123-125 + uncovered even after `src/app/(web)/tools/groupwizard/group-wizard.test.tsx` + was extended with 12 additional cases covering every other branch/function + in the file (94.5% statements / 96.25% lines final). + +## Proposed fix +Either: +1. Remove the `!fields || fields.length === 0` special case entirely (the + `form.trigger([])` call with an empty array resolves `true` immediately + in react-hook-form, so simplifying to always call `form.trigger(fields)` + would behave identically and remove the dead branch), or +2. If the branch is meant as future-proofing for a step reachable via some + other entry point, add a code comment explaining that intent so the next + reader (and coverage tooling) doesn't flag it as untested dead code. + +## Impact if not fixed +None functionally — this is a coverage/clarity issue, not a behavior bug. +Low priority; safe to leave as documented, known-dead code. diff --git a/.claude/TODO/2026-09-13-mergetemplate-logs-address-pii-on-error.md b/.claude/TODO/2026-09-13-mergetemplate-logs-address-pii-on-error.md new file mode 100644 index 0000000..77ba706 --- /dev/null +++ b/.claude/TODO/2026-09-13-mergetemplate-logs-address-pii-on-error.md @@ -0,0 +1,58 @@ +--- +title: mergeTemplate (and sibling actions) console.error the raw docxtemplater error, which can carry household addresses +severity: high +tags: [security, bug] +area: components +files: [src/components/address-labels/actions.ts] +discovered: 2026-09-13 +discovered_by: coverage-agent-address-labels +status: open +--- + +## Problem + +CLAUDE.md rule 14 requires that errors log "identifiers and shape" only — never +record content — "including inside thrown error messages, which travel further +than logs do." `mergeTemplate` (and its siblings `generateLabelPdf` / +`generateLabelDocx`) violate this by passing the entire caught `error` object to +`console.error`, not just `error.message`. + +For `mergeTemplate` this is a concrete PII leak, not a theoretical one: +docxtemplater's scope-parser error path attaches the live merge scope object to +`err.properties.scope` (see `node_modules/docxtemplater/js/errors.js:593-600`, +function that throws `"scopeparser_execution_failed"`). In this feature, the +merge scope is exactly the `addresses` array built in `mergeTemplate` — each +entry carries `Name`, `AddressLine1`, `AddressLine2`, `City`, `State`, +`PostalCode` for a real household. A user-uploaded template with a malformed +tag (e.g. a token that calls a method that throws, or a bad expression) causes +docxtemplater to throw with that scope attached, and +`console.error('mergeTemplate error:', error)` writes the full object — +including the nested `.properties.scope` with every printable household's +name and address — to server logs. + +## Evidence + +- `src/components/address-labels/actions.ts:328` — `console.error('mergeTemplate error:', error);` logs the raw `error`, not `error.message`. +- `src/components/address-labels/actions.ts:189` and `:232` — same pattern for `generateLabelPdf` / `generateLabelDocx` (lower risk here, but same anti-pattern). +- `node_modules/docxtemplater/js/errors.js:593-601` — `err.properties.scope = scope;` attaches the full render scope (the caller's data) to certain template errors. +- `mergeTemplate`'s `addresses` array (`actions.ts:286-296`) contains `Name`, `AddressLine1`, `AddressLine2`, `City`, `State`, `PostalCode` — real household PII — which is exactly what would appear in that scope. + +## Proposed fix + +Log only identifiers/shape, per rule 14: e.g. +```ts +console.error('mergeTemplate error:', error instanceof Error ? error.message : 'non-Error thrown'); +``` +or, if the `docxtemplater` error's `.properties.id`/`.name` is useful for +diagnosing which failure mode occurred, log those specific fields explicitly +rather than the object itself — never `error` or `error.properties` in bulk. +Apply the same change to the `console.error` calls in `generateLabelPdf` and +`generateLabelDocx` for consistency and defense in depth. + +## Impact if not fixed + +Any user who uploads a mail-merge template with a tag that fails docxtemplater's +scope parser causes every printable household's name and mailing address for +that batch to be written to application/server logs (e.g. Vercel function +logs), which are typically retained, more widely readable, and less access +controlled than the MP database itself. diff --git a/.claude/TODO/2026-09-13-page-logs-raw-error-object.md b/.claude/TODO/2026-09-13-page-logs-raw-error-object.md new file mode 100644 index 0000000..14cf70b --- /dev/null +++ b/.claude/TODO/2026-09-13-page-logs-raw-error-object.md @@ -0,0 +1,62 @@ +--- +title: "AddEditFamilyPage logs the raw error object instead of an identifier" +severity: medium +tags: [security, drift] +area: components +files: [src/app/(web)/tools/addeditfamily/page.tsx] +discovered: 2026-09-13 +discovered_by: coverage-agent-addeditfamily +status: open +--- + +## Problem +`AddEditFamilyPage` swallows a failed `resolveContactIdFromPage` call with: + +```ts +console.warn("Failed to resolve Contact_ID from page record:", error); +``` + +CLAUDE.md rule 14 requires errors to log identifiers and shape only — never +record content, `$filter` strings, or request/response bodies, "including +inside thrown error messages, which travel further than logs do." Logging the +raw `error` object prints its `.message` (and, depending on the runtime, +`.stack`), which can carry the interpolated MP `$filter` string built in +`FamilyService.resolveContactIdFromPage` (`` `${primaryKey} = ${recordId}` ``) +or other request detail if the underlying `getTableRecords` call or +`validateColumnName`/`validatePositiveInt` throws with that detail embedded. + +The sibling file `src/lib/tool-params.server.ts` (same call-swallowing shape, +literally a few lines away in the import graph) already gets this right: + +```ts +// Identifier only. A caller without an MP security role also lands +// here — the tools layout redirects them to /no-access, and the page +// simply renders without page metadata in the meantime. +console.warn('tool_params.page_data_unavailable', { pageID: parsedPageID }); +``` + +`page.tsx` should follow the same pattern instead of passing `error` through. + +## Evidence +- `src/app/(web)/tools/addeditfamily/page.tsx:29` — `console.warn("Failed to resolve Contact_ID from page record:", error)` +- `src/lib/tool-params.server.ts:58-60` — the correct pattern, logging an identifier object with no error content +- `src/services/familyService.ts:140-165` — `resolveContactIdFromPage` builds a `$filter` string from `primaryKey`/`recordId` that could surface in a thrown error's `.message` + +## Proposed fix +Replace the `console.warn` call with an identifier-only log, e.g.: + +```ts +console.warn("addeditfamily.resolve_contact_id_failed", { + tableName: params.pageData.Table_Name, + recordID: params.recordID, +}); +``` + +and drop `error` from the log call entirely (or log only `error instanceof Error ? error.name : typeof error` if a coarse classification is wanted). + +## Impact if not fixed +Low likelihood, low blast radius — this only logs when the page-record +resolution has already failed, and only to server logs. But it is the second +place after `tool-params.server.ts`'s example that this exact swallow-and-warn +shape appears, and it is the one that gets it wrong; left uncorrected, it is +an easy pattern to copy into the next new page. diff --git a/.claude/TODO/2026-09-13-removegroup-order-guard-mismatch.md b/.claude/TODO/2026-09-13-removegroup-order-guard-mismatch.md new file mode 100644 index 0000000..0cab77d --- /dev/null +++ b/.claude/TODO/2026-09-13-removegroup-order-guard-mismatch.md @@ -0,0 +1,89 @@ +--- +title: removeGroup drops a non-empty group's fields from the save payload +severity: high +tags: [bug] +area: components +files: [src/components/field-management/use-field-order-state.ts] +discovered: 2026-09-13 +discovered_by: coverage-agent-field-management +status: open +--- + +## Problem + +`removeGroup(name)` in `use-field-order-state.ts` guards `groupedFields` against +removing a group that still has fields, but does not apply the same guard to +`groupOrder`. The two state updates run independently: + +```ts +const removeGroup = useCallback((name: string) => { + setGroupedFields((prev) => { + if ((prev[name] || []).length > 0) return prev; // <-- guarded: no-op if non-empty + const { [name]: _, ...rest } = prev; + return rest; + }); + setGroupOrder((prev) => prev.filter((g) => g !== name)); // <-- unconditional + setIsDirty(true); +}, []); +``` + +If a caller invokes `removeGroup` on a group that still has fields (the UI's +trash-can button is only rendered for empty groups, but `removeGroup` is a +plain callback with no such enforcement — any other call site, a future UI +change, or a test/automation calling the hook directly can hit this), the +group's fields survive in `groupedFields` but the group name is removed from +`groupOrder`. + +`buildSavePayload()` iterates `groupOrder` (not `Object.keys(groupedFields)`) +to build the save payload: + +```ts +for (const groupName of groupOrder) { + const fieldIds = groupedFields[groupName] || []; + ... +} +``` + +Because the group name is no longer in `groupOrder`, its fields are silently +skipped — they never appear in the payload sent to +`savePageFieldOrder`/`api_Tools_...` stored proc. Since this feature writes +live Ministry Platform page field configuration, this is a silent +configuration-loss bug: fields simply vanish from the page's field list on +next save, with no error or warning to the user. + +## Evidence + +- `src/components/field-management/use-field-order-state.ts:123-130` (removeGroup) +- `src/components/field-management/use-field-order-state.ts:222-223` (buildSavePayload + iterates `groupOrder`, using `groupedFields[groupName] || []` as a fallback for + names no longer present) +- Reproduced in + `src/components/field-management/use-field-order-state.test.ts`, describe + block `useFieldOrderState > removeGroup`, test `'is a no-op on groupedFields + when the group still has fields, but still removes it from groupOrder'`: + after calling `removeGroup('1 - First')` on a group with one field, the test + asserts `groupedFields['1 - First']` still equals `[1]` but + `buildSavePayload()`'s output has no entry for that field at all. + +## Proposed fix + +Make the two updates consistent — either: +1. Only remove from `groupOrder` when the group was actually empty (mirror the + same guard used for `groupedFields`), or +2. If `removeGroup` is meant to force-delete a non-empty group (moving its + fields elsewhere, e.g. into "99 - Other Fields"), do that explicitly instead + of silently orphaning the fields. + +Given the existing UI only calls `removeGroup` for empty groups (see +`sortable-group.tsx`'s trash button, rendered only when `fieldIds.length === +0`), option 1 (guard `groupOrder` the same way) is the minimal, safe fix and +matches the function's only current call site's expectations. + +## Impact if not fixed + +Any caller of `removeGroup` on a non-empty group — now or in a future UI +change — silently drops that group's fields from the next save, with no error +surfaced to the admin performing the edit. This directly corrupts live +Ministry Platform page field configuration (view order, group assignment, +required/hidden/filter settings all lost for the affected fields) until +someone notices fields missing from the page and manually re-adds them. diff --git a/.claude/TODO/2026-09-13-search-empty-state-never-renders.md b/.claude/TODO/2026-09-13-search-empty-state-never-renders.md new file mode 100644 index 0000000..6b6e3d7 --- /dev/null +++ b/.claude/TODO/2026-09-13-search-empty-state-never-renders.md @@ -0,0 +1,51 @@ +--- +title: "Add/Edit Family search: 'No contacts found' empty state never renders" +severity: low +tags: [bug] +area: components +files: [src/app/(web)/tools/addeditfamily/add-edit-family.tsx] +discovered: 2026-09-13 +discovered_by: coverage-agent-addeditfamily +status: open +--- + +## Problem +`FamilySearchBar` (in `add-edit-family.tsx`) renders a `No contacts +found.` whenever the query is at least 2 characters and +`searchContacts` returned zero results. In practice this text never appears in +the DOM, because `cmdk`'s `CommandEmpty` only renders when the command's +*global* registered-item count (`filtered.count`, tracked across the whole +`Command` tree, not just the group it lives in) is zero. The sibling "Or" +`CommandGroup` always registers a `+ New Family with last name "…"` item as +soon as the query is >= 2 characters, so that count is never zero, and +`CommandEmpty` unconditionally returns `null` regardless of our own +`results.length === 0` check. + +Net effect: a user who searches for a name with no matches sees only the +"+ New Family" suggestion, with no indication that the search itself came up +empty (they may not notice there's a difference between "no matches" and +"you haven't typed enough"). + +## Evidence +- `src/app/(web)/tools/addeditfamily/add-edit-family.tsx:457-459` — the + `CommandEmpty` block, gated on local `results`/`query` state. +- `node_modules/cmdk/dist/index.mjs` — `CommandEmpty` (`Ie`) renders based on + `useCommandState(v => v.filtered.count === 0)`, and `filtered.count` is + computed from `shouldFilter === false` as `u.current.size` (total item + registrations), not scoped to a single `CommandGroup`. +- `src/app/(web)/tools/addeditfamily/add-edit-family.test.tsx` — the test + `"finds no CommandEmpty text when the search returns nothing"` documents and + asserts this actual (broken) behavior instead of the intended message. + +## Proposed fix +Don't rely on `CommandEmpty`'s built-in visibility heuristic here. Replace it +with an explicit conditional `
` (matching the style already used for the +"Type at least 2 characters" and "Searching…" states a few lines above) that +renders whenever `!isSearching && query.trim().length >= 2 && results.length +=== 0`, instead of wrapping the message in ``. + +## Impact if not fixed +Cosmetic/UX only — the "+ New Family" path still works, so no data is lost or +corrupted. A user searching for an existing family that doesn't actually exist +gets a slightly less clear affordance (no explicit "not found" feedback), but +can still proceed by creating a new family. diff --git a/.claude/TODO/2026-09-13-testing-coverage-report-masked-untested-files.md b/.claude/TODO/2026-09-13-testing-coverage-report-masked-untested-files.md new file mode 100644 index 0000000..6e721d0 --- /dev/null +++ b/.claude/TODO/2026-09-13-testing-coverage-report-masked-untested-files.md @@ -0,0 +1,64 @@ +--- +title: Coverage config omitted `coverage.include`, hiding every untested file from the report +severity: high +tags: [bug, testing, drift] +area: testing +files: [vitest.config.mts, .github/workflows/test.yml] +discovered: 2026-09-13 +discovered_by: coverage-review-orchestrator +status: resolved +--- + +## Problem +`vitest.config.mts` (then `vitest.config.ts`) configured v8 coverage without a `coverage.include` glob. +The v8 provider only instruments files that were actually loaded during the +test run, so any source file that no test imported was absent from the report +entirely rather than counted as 0%. + +Note for anyone fixing this from memory or from an older guide: the flag that +used to do this was `coverage.all: true`, and it was **removed in Vitest 5**. +Setting it now is a TypeScript error (`No overload matches this call`) and has +no runtime effect — `coverage.include` is the only lever. This was verified +in-session: adding `all: true` alongside `include` changed nothing but the type +check, and removing it changed no coverage number. + +The reported figure was therefore a percentage of *the code tests already +touch*, not of the codebase. It read 83.14% statements while true coverage over +`src/**` was 48.41%. 131 of 176 source files never appeared in the report at +all, including `src/services/familyService.ts` (756 lines, zero tests) and +`src/app/(web)/tools/layout.tsx` (the `/tools` authorization gate, zero tests). + +CI uploads this report to Codecov, so the inflated number was the project's +published quality signal. + +## Evidence +- `vitest.config.ts:15-24` (pre-fix, before the rename to `.mts`) — `coverage` block had `provider`, + `reporter`, `exclude`, but no `all` and no `include`. +- `npx vitest run --coverage` before fix: `Statements : 83.14% ( 1845/2219 )` +- Same suite with `include: ['src/**/*.{ts,tsx}']`: + `Statements : 48.41% ( 1845/3811 )` — identical numerator, denominator up 72%. +- Files absent from the pre-fix report despite existing: `familyService.ts`, + `googlePlacesService.ts`, all of `src/components/template-editor/`, all of + `src/app/(web)/tools/*/page.tsx`. + +## Proposed fix +Applied. `vitest.config.mts` now sets `include: ['src/**/*.{ts,tsx}']`, +plus explicit exclusions for vendored +shadcn/ui primitives, generated MP models, build-time scripts, barrel files, +type-only modules, and declarative `loading.tsx` shells. + +A second, related defect was found while applying this: directory exclusions +were written with a bare trailing slash (`'src/components/ui/'`), which matches +nothing. Those files stayed in the denominator until the patterns were +rewritten as `'src/components/ui/**'`. Any directory exclusion added here must +end in `**`. + +Codecov will show a one-time drop from 83% to ~49% on the first push carrying +this change — before the new tests land on top of it. That is the correction, +not a regression. + +## Impact if not fixed +Every coverage claim in `CLAUDE.md`, `.claude/references/testing/`, and Codecov +was overstating the tested share of the codebase by ~35 points. Untested +files stayed invisible, so no one could see that a 756-line service and a +route authorization gate had no tests at all. diff --git a/.claude/TODO/2026-09-13-testing-no-typecheck-gate-in-ci.md b/.claude/TODO/2026-09-13-testing-no-typecheck-gate-in-ci.md new file mode 100644 index 0000000..e3cfd4f --- /dev/null +++ b/.claude/TODO/2026-09-13-testing-no-typecheck-gate-in-ci.md @@ -0,0 +1,85 @@ +--- +title: Add a type-check gate to CI (stale test fixtures had silently broken `npm run build`) +severity: medium +tags: [bug, testing, drift] +area: testing +files: [.github/workflows/test.yml, package.json] +discovered: 2026-09-13 +discovered_by: coverage-review-orchestrator +status: open +--- + +## Problem +Two test files that are committed and unmodified on `dev` fail type checking. +`tsconfig.json` includes `**/*.ts` and `**/*.tsx`, `next.config.ts` sets no +`typescript.ignoreBuildErrors`, and `npm run build` runs `next build` — which +type-checks the project. So `npm run build` fails on a clean `dev` checkout. + +This is invisible because, as `CLAUDE.md` already notes, CI runs only +`npm run test:coverage` and never `npm run build`. The tests themselves pass: +the arguments are wrong *types* but the mocked implementations never inspect +the missing fields, so the assertions still hold. Type safety is the only thing +catching it, and nothing automated runs type safety. + +The underlying defect is that these tests assert against object shapes the +production types do not accept — `helper.test.ts` builds `CommunicationInfo` +and `MessageInfo` values out of raw MP column names (`Author_User_ID`, +`From_Contact`, `To_Contact_List`) instead of the camelCase DTO fields the +helper actually requires (`AuthorUserId`, `FromContactId`, `ReplyToContactId`, +`CommunicationType`, ...). A test that type-checks would have caught the drift; +these ones silently encode the wrong contract. + +## Evidence +``` +$ git status --porcelain src/lib/providers/ministry-platform/helper.test.ts \ + src/components/address-labels/word-document.test.ts +(no output — both unmodified) + +$ npx tsc --noEmit +src/components/address-labels/word-document.test.ts(62,46): error TS1355: + A 'const' assertion can only be applied to references to enum members, or + string, number, boolean, array, or object literals. +src/lib/providers/ministry-platform/helper.test.ts(762,57): error TS2345: + ... is missing the following properties from type 'CommunicationInfo': + AuthorUserId, FromContactId, ReplyToContactId, CommunicationType, and 4 more. +src/lib/providers/ministry-platform/helper.test.ts(782,57): error TS2345: (same) +src/lib/providers/ministry-platform/helper.test.ts(822,49): error TS2345: + ... is missing the following properties from type 'MessageInfo': + FromAddress, ToAddresses +src/lib/providers/ministry-platform/helper.test.ts(841,49): error TS2345: (same) +src/lib/providers/ministry-platform/helper.test.ts(923,11): error TS2559: + Type '{ IsDefault: boolean; Description: string; }' has no properties in + common with type 'FileUploadParams'. +``` +- `tsconfig.json` — `"include": ["**/*.ts", "**/*.tsx", ...]`, so tests are in + the program. +- `next.config.ts` — no `typescript` block, so `ignoreBuildErrors` is false. +- `.github/workflows/test.yml` — runs `npm run test:coverage` only. + +## Fixed in this branch +The stale fixtures have been corrected. `helper.test.ts` and `provider.test.ts` +now build real `CommunicationInfo` / `MessageInfo` / `FileUploadParams` / +`FileUpdateParams` values with explicit type annotations, so the compiler +enforces the shape, and the `as const` misuse at `word-document.test.ts:62` is +gone. `npx tsc --noEmit` is clean and all 96 tests in those files still pass. + +## Remaining work +**Add a type-check gate to CI.** Add a `typecheck` script (`tsc --noEmit`) to +`package.json` and run it in `.github/workflows/test.yml` alongside +`test:coverage`. + +`CLAUDE.md` documents "CI gates tests only ... Type-check locally" as the +mitigation. This issue is the evidence that the honour system did not hold: two +committed test files sat on `dev` in a state that broke `npm run build`, and +nothing caught it because the tests themselves passed — the arguments were the +wrong *types*, but the mocks never inspected the missing fields. + +Note that the new `coverage.thresholds` in `vitest.config.mts` are enforced by +the existing `test:coverage` CI job, so coverage regressions now fail the build. +Type regressions still do not. + +## Impact if not fixed +The specific breakage is repaired, but the gap that allowed it is still open. A +type error reaching `dev` breaks `npm run build` and therefore a Vercel +production deploy, while CI stays green — so it is found at deploy time by +whoever is shipping, not at PR time by whoever wrote it. diff --git a/.claude/TODO/2026-09-13-testing-vitest-config-loaded-as-cjs.md b/.claude/TODO/2026-09-13-testing-vitest-config-loaded-as-cjs.md new file mode 100644 index 0000000..d2b2562 --- /dev/null +++ b/.claude/TODO/2026-09-13-testing-vitest-config-loaded-as-cjs.md @@ -0,0 +1,77 @@ +--- +title: `vitest.config.ts` was loaded as CommonJS, warned as unsupported by Vite's future default loader +severity: low +tags: [drift, testing] +area: testing +files: [vitest.config.mts] +discovered: 2026-09-13 +discovered_by: coverage-review-orchestrator +status: resolved +--- + +## Problem +Every Vitest invocation prints: + +``` +(!) Your Vite config uses features that are unsupported by `configLoader: 'native'`, +which is planned to become the default in a future major version of Vite: + - ESM syntax in a file loaded as CommonJS (vitest.config.ts:1:1). + Use a `.mjs` extension or set `"type": "module"` in the closest package.json +``` + +`vitest.config.ts` uses ESM syntax (`import`/`export default`) but `package.json` +has no `"type": "module"`, so Vite loads it through the CommonJS path. That path +still works today, but Vite plans to make `configLoader: 'native'` the default, +at which point the config stops loading and the whole suite fails to start. + +This is noise on every local run and every CI run today, and a hard break on a +future Vite major. + +## Evidence +- `vitest.config.ts:1` — `import { defineConfig } from 'vitest/config';` +- `package.json` — no `"type"` field. +- Warning reproduces on any `npx vitest run`. +- Vite 8 / Vitest 6 are the likely forcing versions; neither is adopted here yet. + +## Proposed fix +Lowest-risk option is to rename `vitest.config.ts` -> `vitest.config.mts`. Vitest +resolves the `.mts` extension automatically, so no script changes are needed, and +it does not touch how Next.js or the rest of the toolchain treat the package. + +Do NOT add `"type": "module"` to `package.json` as the fix — this project also +contains CommonJS tooling and Next.js build config, and flipping the package type +has a much wider blast radius than renaming one file. + +Verify with `npx vitest run` (warning gone, suite still runs) and +`npm run test:coverage` (Codecov path `coverage/coverage-final.json` still +produced). + +## Impact if not fixed +Cosmetic today. On the Vite major that flips `configLoader` to `'native'`, the +config fails to load and the entire test suite — and therefore the CI `test` +gate that protects `dev` and `main` — stops running. Cheap to fix now, and the +dependency-audit process should not have to rediscover it under time pressure. + +--- + +## Resolution (2026-09-13) +Applied the proposed fix: renamed `vitest.config.ts` -> `vitest.config.mts`. +Vitest resolves the `.mts` extension automatically, so no script or CI changes +were needed, and `package.json` was left alone (adding `"type": "module"` would +have had a far wider blast radius, as noted above). + +The rename surfaced a second, hidden instance of the same problem. With the +file loaded as real ESM, Vite immediately flagged: + +``` +- `__dirname` (vitest.config.mts:66:25). Use `import.meta.dirname` instead +``` + +`__dirname` is a CommonJS global and does not exist in ESM; it had been working +only because the file was being loaded as CJS. The `@` path alias is now +resolved with `import.meta.dirname`. Had the rename been done without running +the suite, the alias would have thrown at config load and every test would have +failed to start. + +Verified: `npm run test:run` produces 17 lines of output with zero warnings, +116 files / 1,535 tests passing, and `npx tsc --noEmit` is clean. diff --git a/.claude/TODO/2026-09-13-unvalidated-envelope-donor-ids-in-filter.md b/.claude/TODO/2026-09-13-unvalidated-envelope-donor-ids-in-filter.md new file mode 100644 index 0000000..a59cd7a --- /dev/null +++ b/.claude/TODO/2026-09-13-unvalidated-envelope-donor-ids-in-filter.md @@ -0,0 +1,77 @@ +--- +title: Unvalidated client-supplied numeric fields interpolated into MP $filter strings in FamilyService +severity: high +tags: [security, bug] +area: services +files: [src/services/familyService.ts, src/app/(web)/tools/addeditfamily/actions.ts, src/lib/dto/family.ts] +discovered: 2026-09-13 +discovered_by: coverage-agent-services +status: open +--- + +## Problem +`saveFamily` (the `"use server"` action in `addeditfamily/actions.ts`) accepts a +`Household` object straight from the client and passes it directly to +`FamilyService.saveHousehold()` — it is never parsed through the +`HouseholdSchema`/`FamilyMemberSchema` Zod schemas that already exist in +`src/lib/dto/family.ts` for exactly this purpose. TypeScript's compile-time +`number` type on `FamilyMember.envelopeNo` and `FamilyMember.donorId` is not +enforced at runtime — a server action is a public POST endpoint, and any JSON +body can be sent to it, including e.g. `envelopeNo: "1 OR 1=1"`. + +Inside `FamilyService.saveHousehold` → `upsertDonor` → `resolveUniqueEnvelopeNo`, +those two client-controlled values are interpolated directly into an MP +`$filter` string with no `validatePositiveInt` (or any other) check: + +```ts +private async resolveUniqueEnvelopeNo( + requested: number, + excludeDonorId: number | null, +): Promise<{ envelopeNo: number; bumped: boolean }> { + let candidate = requested; + ... + const filter = + excludeDonorId && excludeDonorId > 0 + ? `Envelope_No = ${candidate} AND Donor_ID <> ${excludeDonorId}` + : `Envelope_No = ${candidate}`; +``` + +Every other filter built in this file first validates its numeric inputs +(`validatePositiveInt(contactId)` in `getHousehold`, `validatePositiveInt(recordId)` +in `resolveContactIdFromPage`), so this is an inconsistency, not the intended +design. This is the same shape of hole CLAUDE.md rule 11 calls out for strings +("escape user input in filters") — here the payload is untyped/unvalidated +numeric input instead of a string, but it reaches a raw `$filter` string the +same way. + +## Evidence +- `src/services/familyService.ts:687-712` (`resolveUniqueEnvelopeNo`) — `candidate` + (from `member.envelopeNo`) and `excludeDonorId` (from `member.donorId`) are + interpolated into the filter with no type/range check. +- `src/app/(web)/tools/addeditfamily/actions.ts:137-154` (`saveFamily`) — takes + `household: Household` straight from the client with no `HouseholdSchema.parse()` + or `.safeParse()` call before it reaches the service. +- Contrast with `src/services/familyService.ts:150,175` where the same file + validates `recordId`/`contactId` via `validatePositiveInt` before use in a + filter. +- Confirmed via test: `src/services/familyService.test.ts` exercises + `resolveUniqueEnvelopeNo` only with well-formed numbers (current behavior is + documented, not exploited, per this task's instructions not to modify source). + +## Proposed fix +1. In `saveFamily` (`actions.ts`), parse the incoming `household` argument with + `HouseholdSchema.safeParse()` before calling `FamilyService.saveHousehold()`, + returning an `ActionError` on failure — mirroring the "Validate at API + boundaries" guidance in CLAUDE.md. +2. Defense in depth: in `resolveUniqueEnvelopeNo` and `upsertDonor`, call + `validatePositiveInt` (or an equivalent nullable-safe check) on `requested`/ + `envelopeNo` and `excludeDonorId`/`existingDonorId` before they are + interpolated into any `$filter` string. + +## Impact if not fixed +An authenticated user with write access to `Households` (the only gate this +action checks) can submit a crafted `envelopeNo` or `donorId` value that is +not a plain positive integer. Depending on how the MP OData-style `$filter` +parser handles the resulting string, this ranges from a confusing 400/500 +error to a filter-injection primitive against the `Donors` table — the same +class of risk rule 11 exists to close off for string fields. diff --git a/.claude/TODO/INDEX.md b/.claude/TODO/INDEX.md index 8881b44..adc8793 100644 --- a/.claude/TODO/INDEX.md +++ b/.claude/TODO/INDEX.md @@ -1,7 +1,7 @@ --- title: TODO Index type: index -last_updated: 2026-05-21 +last_updated: 2026-09-13 --- @@ -9,18 +9,27 @@ last_updated: 2026-05-21 + # TODO Index -All open TODOs dropped during the context-engineering review (2026-04-17) and any later additions. Severity tiers: +Open TODOs from the context-engineering review (2026-04-17) and later additions. +Severity tiers: - **critical**: security hole, data loss, auth bypass - **high**: broken behavior, convention violation causing bugs - **medium**: doc drift, missing test, refactor with real cost - **low**: nits, minor doc fixes, stylistic improvements -Total: **3 open TODOs**. +Total: **9 open TODOs**. + +> **2026-09-13 — unit-test coverage push.** Statement coverage over authored +> code went from 49.67% to 98.84% (3,610/3,652), lines to 99.70%, across 1,535 +> tests in 116 files. Most of the items below were opened during that work, +> found by reading code while writing tests for it. A follow-up pass cleaned +> the runner output from 1,507 lines to 17 with zero warnings. Details in each +> file. --- @@ -29,40 +38,62 @@ Total: **3 open TODOs**. ### Critical (0) _none open_ -### High -_none open_ +### High (3) +| Area | Tags | Title | File | +|---|---|---|---| +| services | security, bug | Unvalidated client-supplied numeric fields interpolated into MP `$filter` strings in FamilyService | [→](2026-09-13-unvalidated-envelope-donor-ids-in-filter.md) | +| components | bug | `removeGroup` drops a non-empty group's fields from the save payload | [→](2026-09-13-removegroup-order-guard-mismatch.md) | +| components | security, bug | `mergeTemplate` console.errors the raw docxtemplater error, which can carry household addresses | [→](2026-09-13-mergetemplate-logs-address-pii-on-error.md) | -### Medium (3) +### Medium (4) | Area | Tags | Title | File | |---|---|---|---| +| testing | bug, testing, drift | Add a type-check gate to CI (stale fixtures had silently broken `npm run build`) | [→](2026-09-13-testing-no-typecheck-gate-in-ci.md) | +| components | security, drift | `AddEditFamilyPage` logs the raw error object instead of an identifier | [→](2026-09-13-page-logs-raw-error-object.md) | | components | bug, drift | Template editor ignores pageID/recordID (no MP persistence) | [→](2026-04-17-components-template-editor-no-mp-persistence.md) | | components | bug, refactor | Merge tokens `{{Field_Name}}` have no resolver anywhere | [→](2026-04-17-components-template-editor-merge-token-resolver.md) | -| components | missing-test | No tests for `src/components/template-editor/` | [→](2026-04-17-components-template-editor-missing-tests.md) | -### Low (0) -_none open_ +### Low (2) +| Area | Tags | Title | File | +|---|---|---|---| +| components | bug | Add/Edit Family search: "No contacts found" empty state never renders | [→](2026-09-13-search-empty-state-never-renders.md) | +| components | refactor, missing-test | Unreachable "empty STEP_FIELDS" branch in `GroupWizard.handleNext` | [→](2026-09-13-dead-empty-fields-branch-handlenext.md) | --- ## By tag -### security (0) -_none open_ - -### bug (2) -_see severity sections above; tag appears on items involving a functional defect_ - -### drift (2) -_doc-to-code or doc-to-doc divergence; mostly resolved inline by Phase 4 verification_ +### security (3) +- unvalidated-envelope-donor-ids-in-filter — high +- mergetemplate-logs-address-pii-on-error — high +- page-logs-raw-error-object — medium + +### bug (6) +- unvalidated-envelope-donor-ids-in-filter — high +- removegroup-order-guard-mismatch — high +- mergetemplate-logs-address-pii-on-error — high +- testing-no-typecheck-gate-in-ci — medium +- components-template-editor-no-mp-persistence — medium +- components-template-editor-merge-token-resolver — medium +- search-empty-state-never-renders — low + +### drift (3) +- testing-no-typecheck-gate-in-ci — medium +- page-logs-raw-error-object — medium +- components-template-editor-no-mp-persistence — medium ### missing-test (1) -- components-template-editor-missing-tests — medium +- dead-empty-fields-branch-handlenext — low -### refactor (1) -_improvements with real value but no functional defect_ +### refactor (2) +- components-template-editor-merge-token-resolver — medium +- dead-empty-fields-branch-handlenext — low -### doc (1) -_documentation-only tasks, mostly retiring old flat files or updating CLAUDE.md / README.md_ +### testing (1) +- testing-no-typecheck-gate-in-ci — medium + +### doc (0) +_none open_ ### perf (0) _none open_ @@ -73,19 +104,31 @@ _none open_ | Area | Count | |---|---| -| components | 3 | +| components | 6 | +| testing | 1 | +| services | 1 | | auth | 0 | | mp-provider | 0 | -| services | 0 | | utils | 0 | | routing | 0 | | mp-schema | 0 | -| testing | 0 | | contexts | 0 | | dto-constants | 0 | | doc (cross-cutting) | 0 | --- +## Recently resolved + +| Date | Title | File | +|---|---|---| +| 2026-09-13 | Coverage config omitted `coverage.include`, hiding every untested file | [→](2026-09-13-testing-coverage-report-masked-untested-files.md) | +| 2026-09-13 | No tests for `src/components/template-editor/` | [→](2026-04-17-components-template-editor-missing-tests.md) | +| 2026-09-13 | `vitest.config.ts` loaded as CommonJS (renamed to `.mts`) | [→](2026-09-13-testing-vitest-config-loaded-as-cjs.md) | +| 2026-09-13 | Radix Select fields switched uncontrolled -> controlled | [→](2026-09-13-components-select-uncontrolled-to-controlled.md) | + +--- + ## Schema -Every TODO follows [`SCHEMA.md`](SCHEMA.md) with required `severity`, `tags`, `area`, `files`, `discovered`, `discovered_by`, `status` frontmatter. +Every TODO follows [`SCHEMA.md`](SCHEMA.md) with required `severity`, `tags`, +`area`, `files`, `discovered`, `discovered_by`, `status` frontmatter. diff --git a/.claude/references/DECISIONS.md b/.claude/references/DECISIONS.md index e98eb01..a3ea270 100644 --- a/.claude/references/DECISIONS.md +++ b/.claude/references/DECISIONS.md @@ -350,7 +350,7 @@ Architectural decisions captured by the context-engineering review at SHA `971c4 **Date:** 2026-04-17 **Status:** Accepted **Context:** The project needs a TypeScript-first test runner that supports `import.meta`, runs fast against ESM without a transform step, and shares the `@/` alias configuration with Vite/Turbopack. -**Decision:** Vitest 4.x with `@vitejs/plugin-react` (`vitest.config.ts`). `globals: true` so tests use bare `describe` / `it` / `expect`. The `@/` alias is resolved via `path.resolve(__dirname, './src')`. +**Decision:** Vitest 5.x with `@vitejs/plugin-react` (`vitest.config.mts`). `globals: true` so tests use bare `describe` / `it` / `expect`. The `@/` alias is resolved via `path.resolve(import.meta.dirname, './src')` — the config is ESM (`.mts`), so `__dirname` is not available. **Consequences:** All mock patterns assume `vi.mock()` hoisting (`vi.hoisted()` is required for any variable referenced inside a mock factory). No Jest-specific matchers — only `@testing-library/jest-dom` matchers. Shared resolver config with the bundler. **Alternatives considered:** - **Jest with `ts-jest` / `@swc/jest`** — transform overhead and a second resolver config to maintain. @@ -360,7 +360,7 @@ Architectural decisions captured by the context-engineering review at SHA `971c4 **Date:** 2026-04-17 **Status:** Accepted **Context:** React component tests need a DOM that supports `localStorage`, `window.location`, `Headers`, and the full `@testing-library` API surface. -**Decision:** `environment: 'jsdom'` in `vitest.config.ts` with `jsdom@^28.1.0`. +**Decision:** `environment: 'jsdom'` in `vitest.config.mts` with `jsdom@^30.0.1`. **Consequences:** Slightly slower than happy-dom on pure-DOM tests, but compatibility is higher for edge APIs (`Headers`, `Storage`, `fetch` mocks). **Alternatives considered:** - **happy-dom** — faster, but historically has gaps around `Headers`, `fetch`, and `Storage` mocks that affect MP client tests. diff --git a/.claude/references/_meta/facts/2026-09-13.md b/.claude/references/_meta/facts/2026-09-13.md new file mode 100644 index 0000000..88b1914 --- /dev/null +++ b/.claude/references/_meta/facts/2026-09-13.md @@ -0,0 +1,74 @@ +--- +title: Facts Snapshot 2026-09-13 +type: meta +git_sha: eb9d792059403f70fccd5af437d47ef5b601b288 +--- + +## Purpose +Authoritative counts after the unit-test coverage push. Supersedes the counts +in `2026-04-17.md` (that file remains the frozen baseline for the +context-engineering review and should not be edited). + +## Git baseline +- **SHA:** `eb9d792059403f70fccd5af437d47ef5b601b288` (parent of the coverage work) +- **Branch:** `dev` +- **Created:** 2026-09-13 + +## Counts (authoritative) + +- **Test files:** 116 +- **Test cases:** 1,535 (from `vitest run`) +- **Source files in the coverage denominator:** 143 + +### Coverage (from `npm run test:coverage`) + +| Metric | Before | After | +|---|---|---| +| Statements | 49.67% (1,814/3,652) | **98.84%** (3,610/3,652) | +| Lines | 50.20% | **99.70%** (3,363/3,373) | +| Branches | 37.51% | **92.21%** (2,228/2,416) | +| Functions | 38.35% | **98.81%** (915/926) | + +The "before" figures are measured on the same denominator as "after". The +number the config *reported* before this work was 83.14%, which was wrong — see +`.claude/TODO/2026-09-13-testing-coverage-report-masked-untested-files.md`. + +### Test files by area + +| Area | Files | +|---|---| +| `src/app/**` | 23 | +| `src/components/group-wizard` | 12 | +| `src/components/dev-panel` | 11 | +| `src/components/address-labels` | 10 | +| `src/components/template-editor` | 10 | +| `src/components/field-management` | 7 | +| `src/components/*` (layout, shared-actions, tool, user-menu) | 6 | +| `src/lib/providers/ministry-platform` | 12 | +| `src/lib/**` (excl. MP provider) | 12 | +| `src/services` | 9 | +| `src/contexts` | 2 | +| `src/` (proxy, auth) | 2 | + +## Coverage configuration contract + +`vitest.config.mts` sets `coverage.include: ['src/**/*.{ts,tsx}']`. Without it +the v8 provider reports only files some test imported, so untested files vanish +from the report instead of counting as 0%. + +- `coverage.all` was **removed in Vitest 5** — setting it is a type error and + does nothing. `include` is the only lever. +- Directory exclusions must end in `**`. A bare trailing slash + (`'src/components/ui/'`) matches nothing. +- Enforced thresholds: **statements 97, lines 98**. Branches and functions are + deliberately unenforced (noisier; a single defensive guard can trip them). + +### Excluded from the denominator +Vendored shadcn/ui primitives (`src/components/ui/**`), generated MP models and +build-time scripts, `**/loading.tsx`, the two font/metadata root layouts, +`**/index.ts` barrels, and `**/types.ts` type-only modules. + +## Services (9) +`addressLabelService`, `authorizationService`, `domainTimezoneService`, +`familyService`, `fieldManagementService`, `googlePlacesService`, +`groupService`, `toolService`, `userService` diff --git a/.claude/references/testing/README.md b/.claude/references/testing/README.md index 33493f4..0a92324 100644 --- a/.claude/references/testing/README.md +++ b/.claude/references/testing/README.md @@ -5,20 +5,20 @@ domain: testing --- ## What's in this domain -Vitest test runner config, global setup, mocking patterns, and inventory of 37 test files (507 cases) co-located next to source. +Vitest test runner config, global setup, mocking patterns, and inventory of 116 test files (1,535 cases) co-located next to source. Coverage over authored code is 98.84% statements / 99.70% lines, enforced by thresholds in `vitest.config.mts`. ## File map | File | Purpose | When to read | |------|---------|--------------| -| `setup.md` | `vitest.config.ts`, `src/test-setup.ts`, env vars, jsdom, v8 coverage | Adding Vitest config or adjusting global setup | +| `setup.md` | `vitest.config.mts`, `src/test-setup.ts`, env vars, jsdom, v8 coverage, the `coverage.include` contract and enforced thresholds | Adding Vitest config or adjusting global setup | | `mocks.md` | Mandatory mock patterns (`vi.hoisted`, MPHelper mock class, singleton reset, auth/headers, fake timers) | Writing any new test that mocks an import | | `cookbook.md` | Copy-paste recipes pulled from real tests (service, server action, component, OAuth) | Writing a new test in an existing category | -| `inventory.md` | All 37 test files grouped by area with one-liner coverage | Locating where a surface is tested | +| `inventory.md` | All 116 test files grouped by area with one-liner coverage | Locating where a surface is tested | ## Code surfaces | Path | Role | |------|------| -| `vitest.config.ts` | Runner config (jsdom, globals, v8, `@` alias) | +| `vitest.config.mts` | Runner config (jsdom, globals, v8 coverage + thresholds, `@` alias) | | `src/test-setup.ts` | Env stubs + `@testing-library/jest-dom` | | `src/**/*.test.ts` | Unit tests (services, lib, auth, proxy) | | `src/**/*.test.tsx` | React component/hook tests | diff --git a/.claude/references/testing/inventory.md b/.claude/references/testing/inventory.md index 148caf5..5797e91 100644 --- a/.claude/references/testing/inventory.md +++ b/.claude/references/testing/inventory.md @@ -5,11 +5,33 @@ type: reference applies_to: [src/**/*.test.ts, src/**/*.test.tsx] symbols: [] related: [setup.md, cookbook.md] -last_verified: 2026-04-17 +last_verified: 2026-09-13 --- ## Purpose -All 37 test files grouped by area. Totals from facts snapshot: **37 files / 507 test cases** (`vitest run` at SHA `971c40b1`). +Test files grouped by area. Totals from the current facts snapshot: +**116 files / 1,535 test cases** (`vitest run`, see +`../_meta/facts/2026-09-13.md`). + +> The per-file tables below were written at 37 files and have NOT been expanded +> to all 116. They remain accurate for the files they list. For anything not +> listed, the co-location rule is reliable: a source file `foo.ts` is tested by +> `foo.test.ts` beside it. Counts by area: +> +> | Area | Files | +> |---|---| +> | `src/app/**` | 23 | +> | `src/components/group-wizard` | 12 | +> | `src/lib/providers/ministry-platform` | 12 | +> | `src/lib/**` (excl. MP provider) | 12 | +> | `src/components/dev-panel` | 11 | +> | `src/components/address-labels` | 10 | +> | `src/components/template-editor` | 10 | +> | `src/services` | 9 | +> | `src/components/field-management` | 7 | +> | `src/components/*` (layout, shared-actions, tool, user-menu) | 6 | +> | `src/contexts` | 2 | +> | `src/` (proxy, auth) | 2 | ## Ministry Platform provider (12 files) @@ -78,7 +100,7 @@ All 37 test files grouped by area. Totals from facts snapshot: **37 files / 507 ## Known uncovered surfaces - `src/lib/providers/ministry-platform/scripts/` (`build-sql-install.ts`, `generate-types.ts`, `generate-storedprocs.ts`) — CLI build scripts exercised via `npm run mp:build:install` / `npm run mp:generate:models` -- Auto-generated model files under `src/lib/providers/ministry-platform/models/` — excluded from coverage via `vitest.config.ts` +- Auto-generated model files under `src/lib/providers/ministry-platform/models/` — excluded from coverage via `vitest.config.mts` ## Related docs - `setup.md` — runner config diff --git a/.claude/references/testing/setup.md b/.claude/references/testing/setup.md index 31a8991..1697354 100644 --- a/.claude/references/testing/setup.md +++ b/.claude/references/testing/setup.md @@ -2,21 +2,25 @@ title: Test Runner Setup domain: testing type: reference -applies_to: [vitest.config.ts, src/test-setup.ts, package.json] +applies_to: [vitest.config.mts, src/test-setup.ts, package.json] symbols: [defineConfig] related: [mocks.md, cookbook.md] -last_verified: 2026-04-17 +last_verified: 2026-09-13 --- ## Purpose Vitest runner config, global setup, and commands. Everything the harness needs before a single test runs. ## Files -- `vitest.config.ts` — runner config (environment, globals, coverage, alias) +- `vitest.config.mts` — runner config (environment, globals, coverage, alias). + The `.mts` extension is deliberate: as `.ts` the file was loaded as CommonJS + and Vite warned it would break when `configLoader: 'native'` becomes the + default. `.mts` is resolved automatically, so no script changes were needed. + Being real ESM, it must use `import.meta.dirname`, not `__dirname`. - `src/test-setup.ts` — env stubs + `@testing-library/jest-dom` import - `package.json` — `test`, `test:run`, `test:coverage` scripts -## Versions (from `.claude/references/_meta/facts/2026-04-17.md`) +## Versions (from `.claude/references/_meta/facts/2026-09-13.md`) | Package | Version | |---|---| | vitest | ^4.1.0 | @@ -33,7 +37,7 @@ Vitest runner config, global setup, and commands. Everything the harness needs b | `npm run test:run` | Single run (`vitest run`) | | `npm run test:coverage` | Single run with v8 coverage (`vitest run --coverage`) | -## `vitest.config.ts` (full source) +## `vitest.config.mts` (full source) ```typescript import { defineConfig } from 'vitest/config'; @@ -51,24 +55,67 @@ export default defineConfig({ coverage: { provider: 'v8', reporter: ['text', 'json', 'html'], + // `include` is load-bearing. Without it the v8 provider only reports + // files that some test imported, so an untested file is invisible + // rather than counted as 0%. That masked ~1,600 uncovered statements + // and inflated the reported number from 48% to 83%. + // + // Note: this is the Vitest 5 spelling. The old `coverage.all: true` + // flag was REMOVED in Vitest 5 — setting it is a type error and does + // nothing. `include` is now the only way to widen the denominator. + include: ['src/**/*.{ts,tsx}'], + /** + * Statements and lines only, by deliberate choice. + * + * Branch and function coverage are noisier — a single added guard + * clause or a defensive `?? []` can drop branch coverage below a bar + * that nothing is actually wrong with, and a threshold people learn to + * override is worse than no threshold. Statements and lines move + * predictably with real test work. + * + * Set below the achieved figures (98.84% statements / 99.70% lines) so + * ordinary work has room, but above the 95% target so the suite cannot + * quietly slide back under it. Raise these when coverage rises; never + * lower them to make a red build green. + */ + thresholds: { + statements: 97, + lines: 98, + }, + // NOTE: directory exclusions must end in `**`. A bare trailing slash + // (e.g. 'src/components/ui/') matches nothing, so the files stay in the + // denominator — which is how the shadcn primitives were silently being + // counted before. exclude: [ - 'node_modules/', - '.next/', + 'node_modules/**', + '.next/**', 'src/test-setup.ts', '**/*.d.ts', - 'src/lib/providers/ministry-platform/models/', // Auto-generated files + '**/*.{test,spec}.{ts,tsx}', + 'src/lib/providers/ministry-platform/models/**', // Auto-generated files + 'src/lib/providers/ministry-platform/scripts/**', // Build-time CLI scripts + 'src/components/ui/**', // Vendored shadcn/ui primitives + '**/loading.tsx', // Declarative skeleton markup + 'src/app/layout.tsx', // Root font/metadata shell + 'src/app/(web)/layout.tsx', // Font/metadata shell + '**/index.ts', // Barrel re-exports + '**/types.ts', // Type-only modules ], }, }, resolve: { alias: { - '@': path.resolve(__dirname, './src'), + // `import.meta.dirname`, not `__dirname`: this file is ESM (.mts), and + // Vite's native config loader has no CommonJS globals. + '@': path.resolve(import.meta.dirname, './src'), }, }, }); ``` ## Key settings +- **`coverage.include`** — REQUIRED. Without it the v8 provider only reports files a test imported, so untested files vanish from the report instead of counting as 0%. Note `coverage.all` was removed in Vitest 5 and directory exclusions must end in `**`. +- **`coverage.thresholds`** — statements 97 / lines 98, enforced by the `test:coverage` CI job. Branches and functions are deliberately unenforced. - **`environment: 'jsdom'`** — DOM available without a browser (see `../DECISIONS.md` for rationale vs happy-dom) - **`globals: true`** — `describe/it/expect/vi` available without imports (tests still import explicitly by convention) - **`plugins: [react()]`** — `@vitejs/plugin-react` required for `.tsx` files and JSX transform diff --git a/CLAUDE.md b/CLAUDE.md index 5e788b6..5ff425a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -228,8 +228,10 @@ await mp.createTableRecords('Contact_Log', records, { ## Testing -- **Framework**: Vitest with jsdom environment, `@testing-library/react` for hooks/components, v8 coverage +- **Framework**: Vitest with jsdom environment, `@testing-library/react` + `@testing-library/user-event` for hooks/components, v8 coverage - **Counts**: test/file totals live in `.claude/references/_meta/facts/` (bit-rots quickly — trust `vitest run` output over any doc claim) +- **Coverage is enforced**: `vitest.config.mts` sets `thresholds: { statements: 97, lines: 98 }`, checked by the existing `test:coverage` CI job. Branches/functions are deliberately unenforced. Raise the thresholds as coverage rises; never lower them to green a red build. +- **`coverage.include` is load-bearing**: without `include: ['src/**/*.{ts,tsx}']` the v8 provider reports only files a test imported, so untested files vanish instead of counting as 0%. Two traps: `coverage.all` was **removed in Vitest 5** (setting it is a type error and does nothing), and directory exclusions must end in `**` — a bare `'src/components/ui/'` matches nothing. - **Config**: `vitest.config.ts` (runner), `src/test-setup.ts` (env vars + jest-dom) - **jest-dom import**: `src/test-setup.ts` must import `@testing-library/jest-dom/vitest`, **not** the bare `@testing-library/jest-dom`. Since jest-dom v7 only the `/vitest` entry augments Vitest's `expect` types; the bare import registers matchers at runtime, so tests pass while `next build` fails with `Property 'toBeInTheDocument' does not exist`. - **Co-location**: Test files live next to source — `foo.ts` → `foo.test.ts` @@ -237,6 +239,7 @@ await mp.createTableRecords('Contact_Log', records, { - **MPHelper mock**: Use mock class (`MPHelper: class { method = mockFn; }`), not `vi.fn().mockImplementation()` - **Singleton reset**: Reset `(ServiceClass as any).instance = undefined` in `beforeEach` to prevent state leakage - **Server action tests**: Mock `@/lib/auth` (`auth.api.getSession`), `next/headers` (`headers()`), and service singletons +- **Type-check locally before pushing**: CI does not run `tsc`, and `tsconfig.json` includes `**/*.ts`/`**/*.tsx`, so a type error in a *test* file breaks `npm run build` while CI stays green. This has happened — see `.claude/TODO/2026-09-13-testing-no-typecheck-gate-in-ci.md`. - See **[Testing Reference](.claude/references/testing/README.md)** for all mock patterns, coverage data, and test inventory ## Dependencies diff --git a/package-lock.json b/package-lock.json index 5b8aee2..fde644f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -55,6 +55,7 @@ "@tailwindcss/postcss": "^4.3.3", "@testing-library/jest-dom": "^7.0.1", "@testing-library/react": "^16.3.3", + "@testing-library/user-event": "^14.6.7", "@types/mjml": "^5.0.0", "@types/node": "^24.13.4", "@types/react": "^19.3.0", @@ -4655,6 +4656,20 @@ } } }, + "node_modules/@testing-library/user-event": { + "version": "14.6.7", + "resolved": "https://registry.npmjs.org/@testing-library/user-event/-/user-event-14.6.7.tgz", + "integrity": "sha512-MPCpX8bxe8zS+JmmTwLp8jd0dy1rAm60Te/SL8JrQM3qvQJcBOs1d7IefJMyZzqM3EWBrDn/LWDt1BCGu4ASfg==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">=12", + "npm": ">=6" + }, + "peerDependencies": { + "@testing-library/dom": ">=7.21.4" + } + }, "node_modules/@tybys/wasm-util": { "version": "0.10.4", "resolved": "https://registry.npmjs.org/@tybys/wasm-util/-/wasm-util-0.10.4.tgz", diff --git a/package.json b/package.json index 2966cb7..3faf4a4 100644 --- a/package.json +++ b/package.json @@ -68,6 +68,7 @@ "@tailwindcss/postcss": "^4.3.3", "@testing-library/jest-dom": "^7.0.1", "@testing-library/react": "^16.3.3", + "@testing-library/user-event": "^14.6.7", "@types/mjml": "^5.0.0", "@types/node": "^24.13.4", "@types/react": "^19.3.0", diff --git a/src/app/(web)/no-access/page.test.tsx b/src/app/(web)/no-access/page.test.tsx new file mode 100644 index 0000000..ddb53f4 --- /dev/null +++ b/src/app/(web)/no-access/page.test.tsx @@ -0,0 +1,36 @@ +import { describe, it, expect, afterEach } from 'vitest'; +import { render, screen, cleanup } from '@testing-library/react'; + +import NoAccessPage from './page'; + +/** + * This page is the landing spot for a signed-in user with no MP security role. + * It lives inside the (web) route group specifically so the app shell — and + * therefore the sign-out control — stays reachable. These tests pin the copy + * that tells the user what to do, since that copy IS the feature. + */ + +afterEach(cleanup); + +describe('NoAccessPage', () => { + it('renders a heading explaining the user lacks access', () => { + render(); + + expect( + screen.getByRole('heading', { level: 1, name: /don't have access to this tool/i }), + ).toBeInTheDocument(); + }); + + it('tells the user sign-in succeeded but no role is assigned', () => { + render(); + + expect(screen.getByText(/signed in successfully/i)).toBeInTheDocument(); + expect(screen.getByText(/doesn't have a security role/i)).toBeInTheDocument(); + }); + + it('directs the user to their Ministry Platform administrator', () => { + render(); + + expect(screen.getByText(/ask your ministry platform administrator/i)).toBeInTheDocument(); + }); +}); diff --git a/src/app/(web)/page.test.tsx b/src/app/(web)/page.test.tsx new file mode 100644 index 0000000..226e64b --- /dev/null +++ b/src/app/(web)/page.test.tsx @@ -0,0 +1,47 @@ +import { describe, it, expect, afterEach } from 'vitest'; +import { render, screen, cleanup } from '@testing-library/react'; + +import Home from './page'; + +/** + * The home page is the tool index. Its job is to link to every tool, so the + * test that matters is that each tool has a card AND a working href — a card + * whose link rotted is the failure mode this catches. + */ + +afterEach(cleanup); + +const TOOLS = [ + { name: 'Template Tool', href: '/tools/template' }, + { name: 'Template Editor', href: '/tools/templateeditor' }, + { name: 'Address Labels', href: '/tools/addresslabels' }, + { name: 'Group Wizard', href: '/tools/groupwizard' }, + { name: 'Field Management', href: '/tools/fieldmanagement' }, + { name: 'Add/Edit Family', href: '/tools/addeditfamily' }, +]; + +describe('Home', () => { + it('renders the app title', () => { + render(); + + expect(screen.getByRole('heading', { level: 1, name: 'MPNext Tools' })).toBeInTheDocument(); + }); + + it.each(TOOLS)('lists the $name card', ({ name }) => { + render(); + + expect(screen.getByText(name)).toBeInTheDocument(); + }); + + it.each(TOOLS)('links $name to $href', ({ href }) => { + const { container } = render(); + + expect(container.querySelector(`a[href="${href}"]`)).toBeInTheDocument(); + }); + + it('renders exactly one link per tool and no others', () => { + const { container } = render(); + + expect(container.querySelectorAll('a')).toHaveLength(TOOLS.length); + }); +}); diff --git a/src/app/(web)/tools/addeditfamily/actions.test.ts b/src/app/(web)/tools/addeditfamily/actions.test.ts new file mode 100644 index 0000000..67a80b2 --- /dev/null +++ b/src/app/(web)/tools/addeditfamily/actions.test.ts @@ -0,0 +1,384 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import type { Household, SaveProgress } from "@/lib/dto/family"; + +const { mockRequireSecurityRole } = vi.hoisted(() => ({ + mockRequireSecurityRole: vi.fn(async () => 42), +})); + +vi.mock("@/services/authorizationService", () => ({ + AuthorizationService: { + getInstance: () => ({ + requireSecurityRole: mockRequireSecurityRole, + }), + }, +})); + +const { + mockSearchContacts, + mockGetLookups, + mockGetDefaults, + mockGetHousehold, + mockResolveContactIdFromPage, + mockGetNextEnvelopeNumber, + mockSaveHousehold, +} = vi.hoisted(() => ({ + mockSearchContacts: vi.fn(), + mockGetLookups: vi.fn(), + mockGetDefaults: vi.fn(), + mockGetHousehold: vi.fn(), + mockResolveContactIdFromPage: vi.fn(), + mockGetNextEnvelopeNumber: vi.fn(), + mockSaveHousehold: vi.fn(), +})); + +const FakePartialSaveError = vi.hoisted(() => { + return class FakePartialSaveError extends Error { + progress: SaveProgress; + underlying: unknown; + constructor(progress: SaveProgress, underlying: unknown) { + super(underlying instanceof Error ? underlying.message : String(underlying)); + this.name = "PartialSaveError"; + this.progress = progress; + this.underlying = underlying; + } + }; +}); + +vi.mock("@/services/familyService", () => ({ + FamilyService: { + getInstance: vi.fn(async () => ({ + searchContacts: mockSearchContacts, + getLookups: mockGetLookups, + getDefaults: mockGetDefaults, + getHousehold: mockGetHousehold, + resolveContactIdFromPage: mockResolveContactIdFromPage, + getNextEnvelopeNumber: mockGetNextEnvelopeNumber, + saveHousehold: mockSaveHousehold, + })), + }, + PartialSaveError: FakePartialSaveError, +})); + +const { mockIsEnabled, mockAutocomplete, mockGetPlaceDetails } = vi.hoisted(() => ({ + mockIsEnabled: vi.fn(), + mockAutocomplete: vi.fn(), + mockGetPlaceDetails: vi.fn(), +})); + +vi.mock("@/services/googlePlacesService", () => ({ + GooglePlacesService: { + getInstance: vi.fn(async () => ({ + isEnabled: mockIsEnabled, + autocomplete: mockAutocomplete, + getPlaceDetails: mockGetPlaceDetails, + })), + }, +})); + +import { + searchContacts, + fetchFamilyLookups, + fetchFamilyDefaults, + fetchHousehold, + resolveContactIdFromPage, + fetchNextEnvelopeNumber, + placesEnabled, + placeAutocomplete, + placeDetails, + saveFamily, +} from "./actions"; + +function makeHousehold(overrides: Partial = {}): Household { + return { + householdId: 1, + householdName: "Smith", + householdPhone: "", + congregationId: 1, + sourceId: 18, + address: { + addressId: 0, + addressLine1: null, + addressLine2: null, + city: null, + state: null, + region: null, + postalCode: "", + countryCode: null, + }, + alternateMailingAddress: { + addressId: 0, + addressLine1: null, + addressLine2: null, + city: null, + state: null, + region: null, + postalCode: "", + countryCode: null, + }, + seasonStart: null, + seasonEnd: null, + repeatsAnnually: false, + areHeadsMarried: false, + members: [], + ...overrides, + }; +} + +describe("addeditfamily actions", () => { + beforeEach(() => { + vi.clearAllMocks(); + mockRequireSecurityRole.mockResolvedValue(42); + }); + + describe("searchContacts", () => { + it("authorizes a read against Contacts and delegates to the service", async () => { + mockSearchContacts.mockResolvedValueOnce([ + { contactId: 1, displayName: "Smith, John", detail: "" }, + ]); + const result = await searchContacts("smith"); + expect(mockRequireSecurityRole).toHaveBeenCalledWith({ + table: "Contacts", + operation: "read", + }); + expect(mockSearchContacts).toHaveBeenCalledWith("smith"); + expect(result).toEqual([{ contactId: 1, displayName: "Smith, John", detail: "" }]); + }); + + it("propagates authorization failures", async () => { + mockRequireSecurityRole.mockRejectedValueOnce(new Error("Not authorized")); + await expect(searchContacts("smith")).rejects.toThrow("Not authorized"); + expect(mockSearchContacts).not.toHaveBeenCalled(); + }); + }); + + describe("fetchFamilyLookups", () => { + it("authorizes and returns lookups", async () => { + const lookups = { congregations: [] } as unknown as Awaited>; + mockGetLookups.mockResolvedValueOnce(lookups); + const result = await fetchFamilyLookups(); + expect(mockRequireSecurityRole).toHaveBeenCalledWith({ table: "Contacts", operation: "read" }); + expect(result).toBe(lookups); + }); + }); + + describe("fetchFamilyDefaults", () => { + it("authorizes and returns defaults", async () => { + const defaults = { congregationId: 1 } as unknown as Awaited>; + mockGetDefaults.mockResolvedValueOnce(defaults); + const result = await fetchFamilyDefaults(); + expect(mockRequireSecurityRole).toHaveBeenCalledWith({ table: "Contacts", operation: "read" }); + expect(result).toBe(defaults); + }); + }); + + describe("fetchHousehold", () => { + it("returns success with the household when found", async () => { + const household = makeHousehold(); + mockGetHousehold.mockResolvedValueOnce(household); + const result = await fetchHousehold(5); + expect(mockRequireSecurityRole).toHaveBeenCalledWith({ table: "Households", operation: "read" }); + expect(result).toEqual({ success: true, household }); + }); + + it("returns a not-found error when the service returns null", async () => { + mockGetHousehold.mockResolvedValueOnce(null); + const result = await fetchHousehold(5); + expect(result).toEqual({ success: false, error: "Household not found" }); + }); + + it("returns a generic error message for a non-Error throw", async () => { + mockGetHousehold.mockRejectedValueOnce("boom"); + const result = await fetchHousehold(5); + expect(result).toEqual({ success: false, error: "Failed to load household" }); + }); + + it("returns the Error message when the service throws an Error", async () => { + mockGetHousehold.mockRejectedValueOnce(new Error("db exploded")); + const result = await fetchHousehold(5); + expect(result).toEqual({ success: false, error: "db exploded" }); + }); + + it("surfaces authorization failures as ActionError", async () => { + mockRequireSecurityRole.mockRejectedValueOnce(new Error("Not authorized")); + const result = await fetchHousehold(5); + expect(result).toEqual({ success: false, error: "Not authorized" }); + }); + }); + + describe("resolveContactIdFromPage", () => { + const args = { + tableName: "Event_Participants", + primaryKey: "Event_Participant_ID", + recordId: 10, + contactIdField: "Participant_ID_TABLE_Contact_ID", + }; + + it("authorizes against the given table and returns the contactId", async () => { + mockResolveContactIdFromPage.mockResolvedValueOnce(99); + const result = await resolveContactIdFromPage(args); + expect(mockRequireSecurityRole).toHaveBeenCalledWith({ + table: "Event_Participants", + operation: "read", + }); + expect(mockResolveContactIdFromPage).toHaveBeenCalledWith( + args.tableName, + args.primaryKey, + args.recordId, + args.contactIdField, + ); + expect(result).toEqual({ success: true, contactId: 99 }); + }); + + it("returns null contactId when unresolved", async () => { + mockResolveContactIdFromPage.mockResolvedValueOnce(null); + const result = await resolveContactIdFromPage(args); + expect(result).toEqual({ success: true, contactId: null }); + }); + + it("returns a generic error message for a non-Error throw", async () => { + mockResolveContactIdFromPage.mockRejectedValueOnce("boom"); + const result = await resolveContactIdFromPage(args); + expect(result).toEqual({ success: false, error: "Failed to resolve contact" }); + }); + + it("returns the Error message when the service throws an Error", async () => { + mockResolveContactIdFromPage.mockRejectedValueOnce(new Error("invalid column")); + const result = await resolveContactIdFromPage(args); + expect(result).toEqual({ success: false, error: "invalid column" }); + }); + }); + + describe("fetchNextEnvelopeNumber", () => { + it("authorizes and returns the next envelope number", async () => { + mockGetNextEnvelopeNumber.mockResolvedValueOnce(1234); + const result = await fetchNextEnvelopeNumber(); + expect(mockRequireSecurityRole).toHaveBeenCalledWith({ table: "Contacts", operation: "read" }); + expect(result).toBe(1234); + }); + }); + + describe("placesEnabled", () => { + it("authorizes against Addresses and returns the enabled flag", async () => { + mockIsEnabled.mockResolvedValueOnce(true); + const result = await placesEnabled(); + expect(mockRequireSecurityRole).toHaveBeenCalledWith({ table: "Addresses", operation: "read" }); + expect(result).toBe(true); + }); + }); + + describe("placeAutocomplete", () => { + it("short-circuits to [] for inputs under 3 chars without calling the service", async () => { + const result = await placeAutocomplete(" a ", "token-1"); + expect(mockRequireSecurityRole).toHaveBeenCalledWith({ table: "Addresses", operation: "read" }); + expect(result).toEqual([]); + expect(mockIsEnabled).not.toHaveBeenCalled(); + }); + + it("returns [] when the feature is disabled", async () => { + mockIsEnabled.mockResolvedValueOnce(false); + const result = await placeAutocomplete("123 Main", "token-1"); + expect(result).toEqual([]); + expect(mockAutocomplete).not.toHaveBeenCalled(); + }); + + it("delegates to the provider when enabled and input is long enough", async () => { + mockIsEnabled.mockResolvedValueOnce(true); + mockAutocomplete.mockResolvedValueOnce([ + { placeId: "p1", primary: "123 Main St", secondary: "", full: "123 Main St" }, + ]); + const result = await placeAutocomplete("123 Main", "token-1"); + expect(mockAutocomplete).toHaveBeenCalledWith("123 Main", "token-1"); + expect(result).toEqual([ + { placeId: "p1", primary: "123 Main St", secondary: "", full: "123 Main St" }, + ]); + }); + }); + + describe("placeDetails", () => { + it("returns success with details", async () => { + const details = { + placeId: "p1", + formattedAddress: "123 Main St", + addressLine1: "123 Main St", + city: "Springfield", + state: "IL", + postalCode: "62701", + countryCode: "US", + }; + mockGetPlaceDetails.mockResolvedValueOnce(details); + const result = await placeDetails("p1", "token-1"); + expect(mockRequireSecurityRole).toHaveBeenCalledWith({ table: "Addresses", operation: "read" }); + expect(result).toEqual({ success: true, details }); + }); + + it("returns a generic error message for a non-Error throw", async () => { + mockGetPlaceDetails.mockRejectedValueOnce("boom"); + const result = await placeDetails("p1", "token-1"); + expect(result).toEqual({ success: false, error: "Failed to fetch place details" }); + }); + + it("returns the Error message when the service throws an Error", async () => { + mockGetPlaceDetails.mockRejectedValueOnce(new Error("upstream failure")); + const result = await placeDetails("p1", "token-1"); + expect(result).toEqual({ success: false, error: "upstream failure" }); + }); + }); + + describe("saveFamily", () => { + it("authorizes an update against Households and returns progress on success", async () => { + const household = makeHousehold(); + const progress: SaveProgress = { + mainAddressId: 1, + altAddressId: null, + householdId: 1, + members: [], + }; + mockSaveHousehold.mockResolvedValueOnce(progress); + const result = await saveFamily(household); + expect(mockRequireSecurityRole).toHaveBeenCalledWith({ table: "Households", operation: "update" }); + expect(mockSaveHousehold).toHaveBeenCalledWith(household); + expect(result).toEqual({ success: true, progress }); + }); + + it("returns partial progress and message when a PartialSaveError is thrown", async () => { + const household = makeHousehold(); + const progress: SaveProgress = { + mainAddressId: 1, + altAddressId: null, + householdId: null, + members: [], + }; + mockSaveHousehold.mockRejectedValueOnce( + new FakePartialSaveError(progress, new Error("household insert failed")), + ); + const result = await saveFamily(household); + expect(result).toEqual({ + success: false, + error: "household insert failed", + progress, + }); + }); + + it("returns a generic error message for a non-Error, non-PartialSaveError throw", async () => { + const household = makeHousehold(); + mockSaveHousehold.mockRejectedValueOnce("boom"); + const result = await saveFamily(household); + expect(result).toEqual({ success: false, error: "Failed to save family" }); + }); + + it("returns the Error message for a plain Error throw", async () => { + const household = makeHousehold(); + mockSaveHousehold.mockRejectedValueOnce(new Error("connection reset")); + const result = await saveFamily(household); + expect(result).toEqual({ success: false, error: "connection reset" }); + }); + + it("surfaces authorization failures as ActionError", async () => { + const household = makeHousehold(); + mockRequireSecurityRole.mockRejectedValueOnce(new Error("Not authorized")); + const result = await saveFamily(household); + expect(result).toEqual({ success: false, error: "Not authorized" }); + expect(mockSaveHousehold).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/src/app/(web)/tools/addeditfamily/add-edit-family.test.tsx b/src/app/(web)/tools/addeditfamily/add-edit-family.test.tsx new file mode 100644 index 0000000..c0a1897 --- /dev/null +++ b/src/app/(web)/tools/addeditfamily/add-edit-family.test.tsx @@ -0,0 +1,931 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, screen, cleanup, within, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import type { ToolParams } from "@/lib/tool-params"; +import type { + FamilyLookups, + FamilyDefaults, + Household, + FamilyMember, + ContactSearchResult, + SaveProgress, +} from "@/lib/dto/family"; +import { emptyAddress } from "@/lib/dto/family"; +import type { PlacePrediction, PlaceDetails } from "@/lib/providers/google-places"; + +/** + * AddEditFamily component tests. + * + * Covers: initial load (success/error), search bar (debounce, empty, error, + * new-family creation), household panel field editing (incl. address tabs, + * Google Places autocomplete on/off), member card editing (always-visible + + * expanded fields, donor toggle, envelope assignment), add member, save + * (success/bumped-envelope/failure/exception), and the dirty-close confirm + * dialog. + */ + +const { + mockSearchContacts, + mockFetchFamilyLookups, + mockFetchFamilyDefaults, + mockFetchHousehold, + mockFetchNextEnvelopeNumber, + mockSaveFamily, + mockPlacesEnabled, + mockPlaceAutocomplete, + mockPlaceDetails, + mockRouterBack, + mockToastSuccess, + mockToastError, + mockToastWarning, +} = vi.hoisted(() => ({ + mockSearchContacts: vi.fn(), + mockFetchFamilyLookups: vi.fn(), + mockFetchFamilyDefaults: vi.fn(), + mockFetchHousehold: vi.fn(), + mockFetchNextEnvelopeNumber: vi.fn(), + mockSaveFamily: vi.fn(), + mockPlacesEnabled: vi.fn(), + mockPlaceAutocomplete: vi.fn(), + mockPlaceDetails: vi.fn(), + mockRouterBack: vi.fn(), + mockToastSuccess: vi.fn(), + mockToastError: vi.fn(), + mockToastWarning: vi.fn(), +})); + +vi.mock("./actions", () => ({ + searchContacts: mockSearchContacts, + fetchFamilyLookups: mockFetchFamilyLookups, + fetchFamilyDefaults: mockFetchFamilyDefaults, + fetchHousehold: mockFetchHousehold, + fetchNextEnvelopeNumber: mockFetchNextEnvelopeNumber, + saveFamily: mockSaveFamily, + placesEnabled: mockPlacesEnabled, + placeAutocomplete: mockPlaceAutocomplete, + placeDetails: mockPlaceDetails, +})); + +vi.mock("next/navigation", () => ({ + useRouter: () => ({ + back: mockRouterBack, + push: vi.fn(), + replace: vi.fn(), + refresh: vi.fn(), + prefetch: vi.fn(), + forward: vi.fn(), + }), +})); + +vi.mock("@/components/dev-panel", () => ({ + DevPanel: () => null, +})); + +vi.mock("sonner", () => ({ + toast: { + success: mockToastSuccess, + error: mockToastError, + warning: mockToastWarning, + }, +})); + +import { AddEditFamily } from "./add-edit-family"; + +// Radix Select / cmdk need these present in jsdom. +beforeEach(() => { + Element.prototype.hasPointerCapture = vi.fn().mockReturnValue(false); + Element.prototype.releasePointerCapture = vi.fn(); + Element.prototype.scrollIntoView = vi.fn(); +}); + +const LOOKUPS: FamilyLookups = { + congregations: [{ id: 1, name: "Main Campus" }, { id: 2, name: "East Campus" }], + sources: [{ id: 1, name: "Walk-in" }, { id: 2, name: "Referral" }], + householdPositions: [{ id: 1, name: "Head" }, { id: 2, name: "Spouse" }], + participantTypes: [{ id: 1, name: "Adult" }, { id: 2, name: "Child" }], + maritalStatuses: [{ id: 1, name: "Single" }, { id: 2, name: "Married" }], + prefixes: [{ id: 1, name: "Mr." }], + suffixes: [{ id: 1, name: "Jr." }], + genders: [{ id: 1, name: "Male" }, { id: 2, name: "Female" }], + contactStatuses: [{ id: 1, name: "Active" }, { id: 2, name: "Inactive" }], + primaryLanguages: [{ id: 1, name: "English" }], + faithBackgrounds: [{ id: 1, name: "Christian" }], + states: [{ code: "CA", name: "California" }, { code: "NY", name: "New York" }], + countries: [{ code: "US", name: "United States" }, { code: "CA", name: "Canada" }], +}; + +const DEFAULTS: FamilyDefaults = { + congregationId: 1, + sourceId: 1, + countryCode: "US", + state: "CA", + householdPositionId: 1, + participantTypeId: 1, + showEnvelopeNumbers: true, +}; + +function makeMember(overrides: Partial = {}): FamilyMember { + return { + contactId: 201, + firstName: "John", + middleName: "", + maidenName: "", + lastName: "Smith", + nickname: "", + prefixId: 0, + suffixId: 0, + birthDate: null, + genderId: 0, + maritalStatusId: 0, + mobilePhone: "", + emailAddress: "", + bulkEmailOpt: false, + envelopeNo: null, + contactStatusId: 1, + primaryLanguageId: null, + faithBackgroundId: null, + householdPositionId: 1, + participant: { participantId: 1, participantTypeId: 1, notes: null }, + donorId: null, + isDonor: false, + ...overrides, + }; +} + +function makeHousehold(overrides: Partial = {}): Household { + return { + householdId: 500, + householdName: "Smith", + householdPhone: "555-1111", + congregationId: 1, + sourceId: 1, + address: { + ...emptyAddress(), + addressId: 10, + addressLine1: "1 Main St", + city: "Springfield", + state: "CA", + postalCode: "90001", + countryCode: "US", + }, + alternateMailingAddress: { ...emptyAddress(), countryCode: "US", state: "CA" }, + seasonStart: null, + seasonEnd: null, + repeatsAnnually: false, + areHeadsMarried: false, + members: [ + makeMember({ contactId: 201, firstName: "John" }), + makeMember({ contactId: 202, firstName: "Jane", householdPositionId: 2 }), + ], + ...overrides, + }; +} + +const params: ToolParams = { pageID: 292 }; + +function setup(overrides: { initialContactId?: number | null } = {}) { + return render(); +} + +async function selectByLabel(labelText: string, optionText: string, occurrence = 0) { + const labels = screen.getAllByText(labelText); + const container = labels[occurrence].parentElement as HTMLElement; + const trigger = within(container).getByRole("combobox"); + await userEvent.click(trigger); + const option = await screen.findByText(optionText, { selector: '[role="option"] *, [role="option"]' }); + await userEvent.click(option); +} + +// The `Field` wrapper does not associate its