feat(documenso-audit-phase-1): persist documensoId early + preflight + state machine + reconciliation + tests
Phase 1 of the comprehensive Documenso upload audit per the 2026-05-26 locked-decisions block in docs/superpowers/audits/active-uat.md. P1.1 — persist documensoId immediately after create Was set only at the late `status: 'sent'` commit. Any throw between documensoCreate and the late update left an orphaned Documenso envelope the CRM had no link to. Now the UPDATE runs right after documensoCreate succeeds; rollback paths can find and void the envelope. P1.2 — pre-flight validation hard-blocks Submit UploadForSigningDialog computes a submissionErrors memo over recipients + fields. Submit button disabled when errors > 0. Inline amber summary lists every issue (missing email, invalid email, missing name, field assigned to non-existent recipient, no fields placed). Service layer mirrors the same email + name checks so direct API hits reject early. No override path per locked decision. P1.3 — cancel/delete affordance audit + sweep Document-list per-row Delete + Send for Signing actions now: - Wrapped in PermissionGate (documents.delete + send_for_signing). - Surface toast on success + toastError on failure (were silently swallowing errors). - Use a broader predicate-based query invalidation so every doc list across the app refreshes, not just the local key. EOI tab Regenerate + Cancel EOI buttons + reservation/contract tab Cancel buttons wrapped in PermissionGate (documents.edit, the cancel route's auth check). P1.4 — Documenso webhook URL auto-PATCH (env-gated) scripts/update-documenso-webhook.ts written. Reads DEV_AUTO_UPDATE_DOCUMENSO_WEBHOOK env flag (when 1, runs; otherwise no-op). Lists every webhook on the Documenso instance via v2 (with v1 fallback), identifies webhooks pointing at trycloudflare.com hosts OR /api/webhooks/documenso paths, PATCHes them to the new tunnel URL. scripts/tunnel-url.sh chains the script after the URL print so a re-tunnel auto-rotates the webhook (when flag set). P1.5 — state-machine refactor with rollbackTo() helper custom-document-upload.service.ts: - Single try around create → send → place steps. - state.step tracks which step is current; state.documensoDocId records the envelope id once we have it. - rollbackTo(reason) composes the recovery: status='cancelled' on the CRM row, documensoVoidSafe on the envelope when applicable. Idempotent — calling twice is safe. - Removes three independent try/catches. P1.6 — recipient ↔ Documenso identity reconciliation After documensoSend, validates every distinct email we sent appears in sentDoc.recipients. If Documenso silently dropped one, a ConflictError fires before field placement so the rollback path triggers. Explicit message names the missing emails for the rep. P1.7 — vitest extension + per-failure audit-log entries - 5 new vitest cases (blank email, whitespace email, malformed email, blank name, duplicate-emails-OK semantic). - rollbackTo writes a structured audit_log entry with failedStep, documensoEnvelopeId, errorClass, errorMessage. Post-mortem investigation has structured data instead of just logger lines. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -108,7 +108,7 @@
|
||||
|
||||
### Documenso upload — silent partial-state when field placement fails
|
||||
|
||||
- **`PARTIALLY SHIPPED (rollback gap fixed; comprehensive audit still queued)`**
|
||||
- **`SHIPPED locally (not yet committed) — comprehensive audit Phase 1 complete`**
|
||||
- **Files touched (this fix):** _src/lib/services/custom-document-upload.service.ts_ (~line 400, placeFields try/catch). _src/components/documents/upload-for-signing-dialog.tsx_ (recipient UI sibling fix shipped separately).
|
||||
- **Symptom:** rep uploads a PDF, places fields, hits Send. Error toast surfaces: `Documenso response missing recipientId for matt.ciaccio@gmail.com - cannot place fields`. Document appears in the CRM's signing UI AND in Documenso, recipients + roles are wired, but **all placed fields are missing**. The signing UI on the receiving end has no boxes to fill, which means a signer who receives the invite via email lands on a useless page.
|
||||
- **Root cause:** in `placeFieldsFromUpload`, the placements were built via `fields.map(f => { if (!recipientId) throw ConflictError(...) ...})` BEFORE the surrounding try/catch. The synchronous throw from `map()` bubbled past the catch-and-rollback block that wraps `placeFields()`, so when the recipient lookup missed:
|
||||
@@ -121,14 +121,14 @@
|
||||
1. The placements `map()` is now INSIDE the same try/catch that wraps `placeFields()`. Any throw — sync or async — triggers the rollback (Document row → cancelled, Documenso envelope → voided).
|
||||
2. Pre-throw `logger.error(...)` captures diagnostic state: the missed email, every email the Documenso response DID return. Future "why didn't this match" investigations have something to grep instead of guesswork.
|
||||
3. Comment block explaining the dedupe semantic (Documenso de-dupes by email at the envelope level, so duplicate emails across CRM recipient rows all map to the same Documenso recipientId — that's expected behaviour, not a bug).
|
||||
- **Still open — comprehensive audit:** user explicitly asked for a full pass on the upload-for-signing flow because "we need to do a comprehensive audit of the uploading to be sent through documenso to ensure all fields are wired up correctly and won't cause issues." The rollback gap is one example of multi-step orchestration that fails silently; the wider audit should cover:
|
||||
1. **Pre-flight validation BEFORE envelope creation.** Validate every recipient row has a usable email; validate every placed field's `recipientIndex` resolves to a recipient that has a non-empty email; validate at least one field exists when documentType requires one. Failing pre-flight returns a 400 with field-level errors, no Documenso side effects.
|
||||
2. **Per-step rollback hardening.** Currently rollback paths exist after `documensoCreate`, `documensoSend`, and `placeFields`, but they're independent try/catches. Refactor into a single sequenced state machine with an idempotent `rollbackTo(step)` helper so future inserts (e.g. metadata write between steps) inherit the rollback automatically.
|
||||
3. **Recipient → Documenso identity reconciliation.** Today the code assumes Documenso's response will echo every input email. When dedupe happens (same email → one recipient), the code WORKS correctly (mapping just collapses), but if Documenso silently DROPS a recipient (e.g. invalid email), the lookup throws and we get the silent-partial-state again. Add explicit reconciliation: after `sendDoc`, verify that every distinct email we sent IS present in `sentDoc.recipients`; raise a specific error type if not.
|
||||
4. **Idempotency on retry.** If the rep hits Send twice (network blip), do we double-create envelopes? Verify the document row's `documensoDocumentId` is checked before another `documensoCreate` call.
|
||||
5. **End-to-end tests covering every failure path.** vitest integration suite for: empty fields list, recipient with blank email, recipient with duplicate email, Documenso 4xx on create, Documenso 4xx on send, Documenso 4xx on place-fields, network timeout mid-flow.
|
||||
6. **Audit-log every milestone.** Currently we log on the final success / final failure. Per-step audit entries (`create_envelope`, `send_envelope`, `place_fields`) would make post-mortem investigation tractable.
|
||||
- **Effort for comprehensive audit:** ~6–10h. ~2h pre-flight validation, ~3h state-machine refactor + tests, ~1h reconciliation logic, ~1h idempotency, ~2h test coverage, ~1h audit-log richness. Bundle as one PR because the pieces interleave.
|
||||
- **Phase 1 audit shipped (5 sub-PRs delivered in this round):**
|
||||
1. **Persist `documensoId` immediately after `documensoCreate`** (P1.1). Was set only at the late success commit, leaving orphaned envelopes when any later step failed. Now the CRM row points at the envelope from the moment Documenso returns the id; rollback paths can find and void it. Catches future failures + future-proofs orphans.
|
||||
2. **Pre-flight validation hard-blocks Submit** (P1.2). UploadForSigningDialog computes a `submissionErrors` memo over recipients + fields. Submit button disabled when errors > 0. Inline amber summary lists every issue (missing email, invalid email, missing name, field assigned to non-existent recipient, no fields placed). Service layer also enforces the same checks (email regex + name presence) so direct API hits reject just as hard. No "I know there's a missing email" override.
|
||||
3. **State-machine refactor with `rollbackTo()` helper** (P1.5). Replaces three independent try/catches with one sequenced try around `create → send → place` and a single catch that calls `rollbackTo(reason)`. Tracks `state.step` + `state.documensoDocId` so future inserts (metadata writes between steps, etc.) inherit the rollback automatically. Idempotent — status flip is a no-op on a second call, voidDocument treats 404 as success.
|
||||
4. **Recipient ↔ Documenso identity reconciliation** (P1.6). After `documensoSend`, validates every distinct email we sent appears in `sentDoc.recipients`. If Documenso silently dropped one, a `ConflictError` fires before field placement so the rollback path triggers. Explicit error message names the missing email(s) for diagnosis.
|
||||
5. **End-to-end test coverage + per-failure audit-log entries** (P1.7). vitest suite extended with: blank email, whitespace-only email, malformed email, blank name, duplicate-emails-OK (Documenso dedupe semantic). `rollbackTo` writes a structured audit_log entry (`status=cancelled`, `failedStep`, `documensoEnvelopeId`, `errorClass`, `errorMessage`) so post-mortem investigation has structured data instead of pre-existing logger lines alone.
|
||||
- **Still open (acknowledged but lower priority):**
|
||||
- **Idempotency on retry** — if the rep hits Send twice, do we double-create envelopes? Today the dialog disables the button while `sendMutation.isPending` so it's mitigated at the UI; service-layer guard via checking `documents.documensoId` before another `documensoCreate` would be belt-and-braces. Queued for follow-up.
|
||||
- **Cross-refs:**
|
||||
- The `/documents/new` wizard refactor (Bucket 3 — wizard refactor finding) touches the same end-to-end flow — bundle the two so the same audit doesn't re-investigate the upload-for-signing service twice.
|
||||
- This is the SECOND time a multi-step Documenso flow has had a rollback gap — the first was the EOI auto-cancel/replace flow (fixed earlier in `65ff596`). Pattern: every multi-step orchestration that touches Documenso needs end-to-end rollback OR pre-flight validation. The audit doc's broader "activity feed comprehensive copy" finding mentioned a similar discipline gap; both should land before more multi-step features ship.
|
||||
|
||||
Reference in New Issue
Block a user