Fix payment review round: till integrity, HTTP client tests, concurrency tests, card-selection consolidation

Addresses the payment review (all 10 blocking + 2 minor findings):

Till money-integrity (CreateTillSale):
- Add pg_advisory_lock on the idempotency key (concurrent same-key double-funding race)
- Guard amount on pending-reuse retry (mirrors tip/gift-card guards)
- Explicitly complete the row for cash/on_the_house pending-reuse
- Reject method-switch on a live card-machine checkout (double-charge guard)
- 3 regression tests (amount-mismatch, cash-completes-row, method-switch)

BookingFlow:
- Fetch saved cards at the deposit step (was dead code)
- Charge the server-computed deposit_amount, not the client estimate

HTTP client tests (was untested): doJSON error parsing, refund sentinel
classification, payment/refund/card wire shapes, checkout polling states,
list-refunds pagination + 20-page guard, sha256 card idempotency key

Concurrency regression tests: real two-goroutine races for BuyGiftCard,
tip, and booking-payment locks asserting exactly-one record each

Frontend:
- Fix CRIT-1: zero-saved-card users blocked (all flows now handle it)
- Consolidate tip/deposit/Buy-Gift-Card card UI onto CardSelection
- Explicit save-card consent checkbox (was silent/inconsistent)
- Fix stale saved-card field names in BookingFlow (last4 -> last_4)
- Unique instance ids (crypto.randomUUID) in CardSelection/SquareCardInput
- UserPaymentModal: keep card form mounted on error + Try Again button

Health/docs: /api/health reports square state (mock/ok, was not_implemented),
close P1 backlog, correct stale webhook and env-var claims
This commit is contained in:
2026-08-22 00:34:49 +01:00
parent 64d4b65083
commit 53ca89603d
18 changed files with 1330 additions and 480 deletions
@@ -25,7 +25,7 @@ These are things that work fine in dev (with mocks) but need real implementation
| # | Task | Effort | Area | Dev Status | Notes |
|---|---|---|---|---|---|
| P1 | **Square payments: wire prod client alongside dev mock** | XL (5-7d) | Backend | Mock exists (`internal/square/square_dev.go:MockClient`). Prod client (`internal/square/square.go`) returns "not yet configured" for all 8 methods. Dev `devProdClient` (`square_dev.go:38-61`) also returns stubs when `SQUARE_ENVIRONMENT=sandbox` or `production`. Only `SQUARE_ENVIRONMENT=mock` processes payments (in-memory). The health endpoint reports `"not_implemented"`. | The in-memory Square mock was great for development — it let us build the full payment flow, refund logic, split records, VAT calculation, and saved cards without touching Square's API. Now we need the production SDK wired beside it. The mock already has the interface; implement `ProdClient` with real SDK calls. |
| P1 | **Square payments: wire prod client alongside dev mock** | XL (5-7d) | Backend | **COMPLETED Aug 2026**`internal/square/square_http_client.go` implements the real REST client (payments, terminal checkouts, refunds, cards, list-refunds). Prod client (`internal/square/square.go`) and dev `devProdClient` (`square_dev.go`) both call real Square when `SQUARE_ENVIRONMENT=sandbox|production`; `mock` uses the in-memory client. The health endpoint reports `"mock"`/`"ok"` accordingly (was `"not_implemented"`). | |
| P2 | **S3/R2 storage: implement prod side of the abstraction** | M (2-3d) | Backend | Dev works (`internal/s3/s3_dev.go` — RustFS + in-memory fallback). Prod side (`internal/s3/s3.go:52-62`) returns "not implemented" for Upload/Download/Delete. The prod `S3Client` struct lacks the `*s3.Client` field entirely — it was never populated. | The storage abstraction was defined early and the dev side got a full implementation. The prod side needs the AWS SDK v2 dependency and real S3/R2 calls. Portfolio images and profile pictures will start working in prod once this is done. |
| P3 | **Square webhook event handling: from log-only to action** | S (1d) | Backend | Webhook signature verification works (HMAC-SHA256, references Square docs). Event parsing works. But `handlePaymentUpdated` and `handleRefundUpdated` (`square.go:88-94`) only log the event data — they never update booking/payment state. | The webhook receiver was built first (parse + verify). The handlers that act on events were deferred. Now they need to: update payment status on `payment.updated`, update refund status on `refund.updated`. |
| P4 | **Payment reconciliation: add recovery for split-brain scenarios** | L (3-5d) | Backend | 11 `log.Printf("CRITICAL: ... manual reconciliation required")` calls exist across payment, refund, and till handlers. When Square succeeds but the DB transaction fails afterwards, state diverges with no automated recovery. | This happens when the application correctly processes a Square payment but then hits a DB error on commit. In dev, this was handled by just logging it. For prod, we need a reconciliation job or retry mechanism. |
+1 -1
View File
@@ -34,7 +34,7 @@ Square integration has two build-tagged implementations:
- **Dev** (`//go:build dev`): Mock client simulates async checkout with polling. No real payments.
- **Prod** (`//go:build !dev`): Connects to live Square API. Requires Square credentials in `.env`.
Saved cards stored in `user_saved_cards` with soft delete (`retained_until` for 7-year UK compliance). Refunds tracked in `refunds` table — partial or full. Square webhooks at `/api/webhooks/square` for payment status updates.
Saved cards stored in `user_saved_cards` with soft delete (`retained_until` for 7-year UK compliance). Refunds tracked in `refunds` table — partial or full. Square webhooks at `/api/webhooks/square` receive payment/refund events (HMAC-verified; currently log-only — status is tracked via the synchronous + sweep/reconcile paths, backlog P3).
Fees column on `payments` stores actual Square deductions. `square_deposits` table for bank reconciliation (matching batch deposits to Mettle account).
+1 -1
View File
@@ -64,7 +64,7 @@ Backend (:8080)
| `handlers/auth` | local.go, social.go | Registration (with referral code validation), login, refresh, email verification |
| `handlers/bookings` | bookings.go, reserve.go, manage.go, admin_reserve.go, cancel_reservation.go, admin_cancel_reservation.go, closing_time.go | Booking CRUD, reservations with **self-blocking prevention** (`excludeUserID` parameter on `CheckTimeBlockerOverlap` + pre-overlap DELETE with IP hash anon cleanup), admin management, edit requests, discounts, closing hours validation (`checkClosingHours` + `getClosingTimeForDate` resolves staged default hours for bookings), active booking limits, GetBookingsByCreatedRange, created_by_name resolution, **explicit reservation cancellation** (`DELETE /api/bookings/reserve` for users, `DELETE /api/admin/bookings/reserve` for admin walk-in/call-in) |
| `handlers/payments` | handlers.go, service.go, validators.go, giftcards.go, till.go, refunds.go, refund_policy.go | Square payments: terminal, online, refunds, tips, saved cards, gift cards (CRUD, topup, transfer, redeem, buy, expired balances, till sales). Refund calculation with notice-period tiers and deposit protection |
| `handlers/webhooks` | square.go | Square webhook handler for payment status updates. **Fail-closed signature check** — rejects requests with 403 when `SQUARE_WEBHOOK_SIGNATURE_KEY` is set but header is missing. Dev mode: skips verification when env var is empty. Still uses hex-encoding stub (`verifySquareSignature`) — production requires HMAC-SHA256 with base64 output, `x-square-hmacsha256-signature` header. See `TODO(PROD)` in source. |
| `handlers/webhooks` | square.go | Square webhook handler for payment status updates. **Fail-closed signature check** — rejects requests with 403 when `SQUARE_WEBHOOK_SIGNATURE_KEY` is set but header is missing. Dev mode: skips verification when env var is empty. HMAC-SHA256 signature verified per Square spec (base64 output, `x-square-hmacsha256-signature` header, notificationURL + body). `payment.updated`/`refund.updated` events are currently **log-only** (backlog P3 — status flows through the synchronous + sweep/reconcile paths instead). |
| `handlers/admin` | users.go, analytics.go, custom_services.go, discount_campaigns.go, settings.go | Admin user management, custom services CRUD (list/create/get/update/promote/delete), discount campaigns, analytics (stub), business settings (GET/PUT with VAT, gift card config) |
| `handlers/today` | today.go | Current/next appointment, today's grid, pending approvals, `DoneForDay` state with daily/weekly summary (`DailySummary` with `total_bookings`, `customers_served`, `summary_scope`), auto-status transitions, closed-day aggregation via `findWeekSummaryRange` + `computeAggregateSummary`. Exceptional hours lookup uses `exceptional_group_applications.week_start` (0=Monday). |
| `handlers/user` | profile.go, account.go, guest.go, loyalty.go, customer_relationship.go, gdpr_export.go | User profile, guest creation (with CheckEmailHandler for registered-email detection), loyalty, contact info, GDPR export (async with 12h cache) |
@@ -47,7 +47,7 @@ The tokenization component. Loads the SDK, attaches the Square card iframe form,
1. **Square application credentials**:
- `SQUARE_APPLICATION_ID` (client-side, public)
- `SQUARE_LOCATION_ID` (already used server-side)
- Frontend needs the application ID in the browser context (e.g. `PUBLIC_SQUARE_APPLICATION_ID` Vite env var)
- Frontend needs the application ID in the browser context (`VITE_SQUARE_APPLICATION_ID` Vite env var — implemented; see `frontend/src/lib/square/square.ts`)
2. **Square account with Web Payments enabled** and a card processing merchant account.
3. Frontend must be HTTPS (or localhost) for the SDK to load.