refinement phase 6
This commit is contained in:
hamid
@@ -0,0 +1,31 @@
|
||||
# After refinement-phase-6 — Money-path correctness completion
|
||||
|
||||
**Track:** backend (money path). Gate: `dotnet build` 0 new warnings · `dotnet test` 396 pass (+13). Migration
|
||||
`RefinementPhase6MoneyFks` scaffolded (additive: 3 FK sets + invoice index + delete of a dead config seed row).
|
||||
|
||||
## What's now live (new endpoints — admin)
|
||||
- **`POST api/v1/admin_refunds/{id}/confirm_settlement`** — settle a `processing` BNPL/manual refund
|
||||
(`processing → succeeded`, posts the deferred `refund_payable ↔ escrow_held` clearing). Idempotent.
|
||||
- **`POST api/v1/admin_refunds/{id}/mark_failed`** — fail a `processing` refund (no ledger). Body
|
||||
`{ "reason": "..." }` (optional). Idempotent.
|
||||
- Both return `RefundSettlement` `{ refundId, bookingId, status, completedAt }`. Contract:
|
||||
`dev/contracts/domains/refunds-invoices.md`; snapshot `swagger.v1.json` refreshed.
|
||||
|
||||
## What changed under the hood (no client-visible shape change)
|
||||
- The **BNPL provider cash-back callback** now auto-settles the matching `processing` refund (a
|
||||
`refund/revert completed|confirmed|settled` / `cashback` event) — so a real BNPL revert reaches `succeeded`
|
||||
without an admin click.
|
||||
- **Refund create** persists the row (`approved`) before the external channel call (crash-window fix) — an
|
||||
interrupted refund is now a reconcilable `approved` row, not a lost execution.
|
||||
- **Forward-dep FKs** added: `refunds.ticket_id`, `nurse_clawbacks.original_payout_id`/`recovered_in_payout_id`,
|
||||
`invoices.partner_center_id` (+ index). All nullable, `NO ACTION`. No behavior change; integrity backstop only.
|
||||
- **Audit:** `Refund`/`NurseClawback`/`NursePayout`/`NursePayoutBatch`/`NurseVerification` are now `IAuditable`
|
||||
→ admin decisions leave an `audit_logs` diff row (IBAN redacted).
|
||||
- **Retired** the orphaned `refund_ticket_required` config key (a refund ticket is always auto-opened).
|
||||
|
||||
## Frontend notes
|
||||
- The customer refund-status flow is **unchanged** — a BNPL refund still shows `processing` with the ETA, and now
|
||||
actually flips to `succeeded` once settled (via admin confirm or the provider callback). No new client work
|
||||
required for the customer side.
|
||||
- If/when an admin refund console surfaces settlement, the two new endpoints are the actions (staff-gated,
|
||||
`sensitive` rate policy).
|
||||
@@ -0,0 +1,95 @@
|
||||
# Refinement Phase 6 — Money-path correctness completion — Report (2026-07-13)
|
||||
|
||||
**Track:** backend (money path) · **Depends on:** nothing hard (do before real BNPL/manual refunds — phase 8)
|
||||
· **Gate:** `dotnet build` 0 new warnings · `dotnet test` 396 pass (383 prior + 13 new).
|
||||
|
||||
## The headline fix (6.1) — the unreachable BNPL/manual refund settlement is now wired
|
||||
|
||||
Before this phase, a card refund cleared its `refund_payable ↔ escrow_held` leg immediately, but a
|
||||
BNPL-revert / manual-bank refund was left in `processing` with the clearing "deferred to reconciliation" — and
|
||||
**no reconciliation path existed**: `Refund.MarkSucceededAsync` had zero callers, and nothing performed
|
||||
`processing → succeeded`. Every BNPL/manual refund permanently overstated `escrow_held` and stranded
|
||||
`refund_payable`; the ledger could never reconcile with the bank.
|
||||
|
||||
Now:
|
||||
- **`ConfirmRefundSettlementCommand`** (`Features/Refunds/Commands/ConfirmRefundSettlement/`) transitions
|
||||
`processing → succeeded`, stamps the settled instant, and posts `LedgerPosting.RefundPayableClearing` in the
|
||||
**same commit**. It runs under the same `booking:{id}:refund` lock as `CreateRefundCommand` and **re-reads the
|
||||
tracked refund inside the lock**, so a racing/replayed confirm sees committed truth and no-ops (never
|
||||
double-clears). Idempotent: an already-`succeeded` refund is a no-op success.
|
||||
- **`MarkRefundSettlementFailedCommand`** (`.../MarkRefundSettlementFailed/`) is the counterpart —
|
||||
`processing → failed`, no ledger moves.
|
||||
- **Admin surface:** `POST admin_refunds/{id}/confirm_settlement` + `.../mark_failed` on `AdminRefundsController`.
|
||||
- **BNPL callback branch:** `HandleBnplCallback` gained a `RefundConfirmed` action. A provider event whose type
|
||||
says the revert/refund cash-back **completed/confirmed/settled** (or "cashback") resolves the `processing`
|
||||
refund created against that order's `payment_transaction` (`GetProcessingRefundIdForTransactionAsync`) and
|
||||
dispatches `ConfirmRefundSettlementCommand`. `ResolveAction` checks this **before** the order-level `settle`
|
||||
branch so `revert_settled` doesn't fall through.
|
||||
- Domain: the misnamed `Refund.MarkSucceededAsync` (not async, uncalled) was renamed `MarkSucceededReconciled`.
|
||||
|
||||
**Proof:** `RefundSettlementTests` — a BNPL refund lands `processing` (3 reversal legs, no clearing) → confirm →
|
||||
`succeeded`, clearing posts, the ledger reconciles (Σdebit = Σcredit, `refund_payable` fully drained,
|
||||
`escrow_held` credited back); a replayed confirm stays at 5 legs (no double-clear); `mark_failed` leaves 3 legs
|
||||
and blocks a later confirm (409).
|
||||
|
||||
## 6.4 — crash-window closed
|
||||
|
||||
`CreateRefundCommand` now persists the refund row (`approved`) **and commits** *before* calling the external
|
||||
channel; then executes the channel against the persisted row and commits the outcome (succeeded/processing/failed
|
||||
+ ledger). Same claim-first / execute-second shape the webhook handler uses — a crash between provider success
|
||||
and our commit now leaves a reconcilable `approved` row instead of a silently-executed refund with no record.
|
||||
|
||||
## 6.2 — forward-dep FKs added (additive migration `RefinementPhase6MoneyFks`)
|
||||
|
||||
Real FKs (all nullable, `ON DELETE NO ACTION`) on the columns b11 shipped FK-less "until the target ships"
|
||||
(the targets all shipped in b13/b15): `refunds.ticket_id → messaging.Tickets`,
|
||||
`nurse_clawbacks.original_payout_id`/`recovered_in_payout_id → payouts.NursePayouts`,
|
||||
`invoices.partner_center_id → partner.PartnerCenters` (**+ index**). The three now-false config-doc comments were
|
||||
corrected. Referential integrity no longer rests on application discipline alone.
|
||||
|
||||
## 6.3 — IAuditable extended to the admin-decided money & trust entities
|
||||
|
||||
`Refund`, `NurseClawback`, `NursePayout`, `NursePayoutBatch`, `NurseVerification` are now `IAuditable`, so the
|
||||
`AuditFieldInterceptor` writes an append-only `audit_logs` diff row on create + every admin decision (approve /
|
||||
reject / process / settle, and the verification `is_verified` decision). `NursePayout.IbanSnapshot` (encrypted)
|
||||
carries `[AuditRedacted]` so the diff records a marker, never the plaintext IBAN.
|
||||
|
||||
## 6.6 — orphaned config key retired
|
||||
|
||||
`refund_ticket_required` (seed row id 19) had no consumer left (b15 unconditionally auto-opens a refund ticket).
|
||||
The seed row is deleted by the migration and the false description removed; the test-host stub was dropped.
|
||||
|
||||
## 6.5 — the previously-untested admin money paths now have tests (+13 tests)
|
||||
|
||||
- **`ClawbackWriteOffTests`** — write-off posts a balanced `DEBIT bad_debt / CREDIT nurse_clawback_receivable`
|
||||
group and resolves the clawback; 404 unknown; 409 second write-off. (Was zero-coverage.)
|
||||
- **`Racing_same_key_insert_is_caught_as_an_idempotent_no_op`** (added to `PaymentWebhookTests`) — a provider
|
||||
that omits `external_event_id` skips the read-dedup, so the `(provider_code, external_event_id)` UNIQUE is the
|
||||
sole backstop (the exact state a true concurrent insert reaches); a colliding insert hits `DbUpdateException`
|
||||
and is treated as an idempotent duplicate no-op (no confirm, no ledger).
|
||||
- **`MessagingInternalBoundaryTests`** (Foundation, handler-level) — the `is_internal` boundary: user thread view
|
||||
strips internal notes; admin view returns them; non-staff admin-view request is forbidden; non-staff can't
|
||||
post an internal note; a staff internal note never surfaces in the user view.
|
||||
- **`RefundSettlementTests`** — the 6.1 settlement (both channels) + idempotency + mark_failed.
|
||||
|
||||
## Test-infra note (why a refund test host changed)
|
||||
|
||||
Adding the `refunds.ticket_id` FK means SQLite (which EF enables FK enforcement on) rejects a refund whose
|
||||
`ticket_id` points at a non-existent ticket. `RefundsTestHost` now **seeds a real `Ticket`** and exposes
|
||||
`Senders()` whose `OpenTicket` hook returns that real id (replacing the `TestSenders.WithTicketHooks()` fake id 1
|
||||
in the refund tests). `PaymentsTestHost`/`PayoutsTestHost` were unaffected (they leave the new FK columns null).
|
||||
|
||||
## Contracts / docs updated (same change)
|
||||
|
||||
- `dev/contracts/domains/refunds-invoices.md` — the two new endpoints + `RefundSettlement` shape + changelog +
|
||||
corrected the `refund_ticket_required` note.
|
||||
- `dev/contracts/openapi/swagger.v1.json` — refreshed (additive: the two routes + `RefundSettlementResult`).
|
||||
- `server/CLAUDE.md` — refunds/payments section (settlement wiring + crash-window + FKs + retired config), the
|
||||
audit-interceptor note (expanded IAuditable set + `[AuditRedacted]`), and the feature map.
|
||||
|
||||
## Follow-ups (out of scope, for later phases)
|
||||
|
||||
- **A `mark_failed` after a successful provider revert** leaves the reversal ledger posted with no clearing (the
|
||||
money is genuinely in limbo — an ops reconciliation case). Deliberate: the reversal is not un-posted.
|
||||
- The BNPL/PSP mocks stay until phase 8 (external rails); the settlement path is now complete behind them.
|
||||
- `audit_logs` growth (2.3 grows it faster) — retention/archival is phase 9 (§7.4).
|
||||
Reference in New Issue
Block a user