cleanup phases 6

2026-08-02 18:48:32 +03:30
parent e2db97392a
commit 51e86a1e5f
239 changed files with 118 additions and 70 deletions
@@ -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).