Sixth fresh-eyes review pass (5 agents: goal, QA, code-quality, security, context-mining). QA FAILED the deposit-required new-card flow; the P0 root cause was backend + frontend, now fixed. All 20 packages green. P0 money-safety: - Deposit-required bookings now actually charge the deposit on new-card payment. Two-part fix: (1) CreateBookingHandler re-reads the trigger-maintained total_amount/total_duration_minutes from the DB after the booking_services insert (the INSERT..RETURNING row predates the recalc trigger, so TotalAmount serialized as 0 and DepositPaid computed TRUE on an unpaid booking — the frontend gate trusted deposit_paid:true, never charged, and confirmed the booking with zero payment rows); (2) BookingFlow.svelte gates the confirmation view on depositPaid and guards against re-creating a booking on retry. Regression test TestBookings_Create_DepositPaidFalseOnUnpaidBooking. Payments (idempotency + money): - deriveBookingPaymentIdempotencyKey: no-client-key fallback now advances a sequence for repeatable types (partial) and rotates past refunded completed rows, so refund-then-repay and equal-amount partials diverge onto distinct keys; an un-refunded completed row keeps its key (double-charge protection holds). Dedup hits on refunded rows now 409, never stale success. - chargeFailureStatus default is 503 (ambiguous), never 402; table test. - Flaky TestBookingPayment_FullPayment_SplitsIntoDepositAndBalance fixed (ORDER BY payment_type). - resolveChargeSource: orphaned card-on-file disabled via DeleteCardOnFile when SaveCardForUser fails (best-effort, redacted log); retry path preserved. Square client: - Dev builds HARD-FAIL (panic) on SQUARE_ENVIRONMENT=production without SQUARE_ALLOW_REAL_API=1; sandbox routes with a loud banner. - Mock fault-injection FailAfterCommit (commit-then-5xx) exercises the exact lost-response same-key retry; SimulateCardTokenUsed; 45-char idempotency-key cap parity; SquareEnvironment/SquareLocationID shared env helpers used by the sweep (env contract no longer comment-only). - listRefunds truncation now errors (money-sensitive reconcile retries instead of over-refunding); getCardsOnFile truncation loudly logged. Webhooks + 2FA: - square-environment header checked fail-closed (403) when configured env is production/sandbox; dispatch DB work bounded by 30s timeout contexts. - 2FA codes HMAC-SHA256 pepper'd (TWO_FACTOR_PEPPER) with legacy-hash migration + upgrade-on-verify; disable-flow mint cooldown (1/min, 429) caps the brute-force loop; in-lockout records never LRU-evicted. Repo hygiene: - env-docs CI gate green again (FRONTEND_ORIGIN + SQUARE_ALLOW_REAL_API + TWO_FACTOR_PEPPER documented; Vite DEV built-in allowlisted). - Dead square_deposits schema dropped; obsidian/README/legal-page drift fixed (consumeradvice.scot signposting, CORS allowlist, p11 R3/P13, T1). - 2FA disable residual documented; P6 email/SMS delivery and P12 sandbox smoke test remain the pre-go-live gates. Verification: go test -tags test,dev -count=1 -parallel 8 ./... (20/20 ok), go build ./... + -tags dev, go vet clean, svelte-check 0 errors, env-docs gate OK, live deposit-required flow re-verified end-to-end (deposit £11 charged, square_payment_id recorded).
135 lines
5.6 KiB
Go
135 lines
5.6 KiB
Go
//go:build test && dev
|
|
|
|
package payments
|
|
|
|
import (
|
|
"context"
|
|
"errors"
|
|
"net/http/httptest"
|
|
"strings"
|
|
"sync"
|
|
"testing"
|
|
|
|
"crussell/db"
|
|
"crussell/internal/square"
|
|
"crussell/testutils/fixtures"
|
|
|
|
"github.com/stretchr/testify/require"
|
|
)
|
|
|
|
// orphanCardClient wraps the dev Square client to record every DeleteCardOnFile
|
|
// call. Used to assert the save-card orphan cleanup in resolveChargeSource
|
|
// WITHOUT needing to reach the real Square API. failDelete simulates a Square
|
|
// disable failure so tests can verify the charge is not failed by cleanup.
|
|
type orphanCardClient struct {
|
|
square.SquareClient
|
|
mu sync.Mutex
|
|
deleted []string
|
|
failDelete bool
|
|
}
|
|
|
|
func (c *orphanCardClient) DeleteCardOnFile(ctx context.Context, cardID string) error {
|
|
c.mu.Lock()
|
|
c.deleted = append(c.deleted, cardID)
|
|
c.mu.Unlock()
|
|
if c.failDelete {
|
|
return errors.New("square: network error disabling card at Square")
|
|
}
|
|
return nil
|
|
}
|
|
|
|
func (c *orphanCardClient) deletedIDs() []string {
|
|
c.mu.Lock()
|
|
defer c.mu.Unlock()
|
|
return append([]string(nil), c.deleted...)
|
|
}
|
|
|
|
// TestResolveChargeSource_SaveCard_HappyPath guards the save-card branch: the
|
|
// Square card is created, persisted locally via SaveCardForUser, and NO
|
|
// DeleteCardOnFile cleanup is triggered (a saved card must never be deleted).
|
|
func TestResolveChargeSource_SaveCard_HappyPath(t *testing.T) {
|
|
ctx := context.Background()
|
|
userID, err := fixtures.CreateTestUser(db.Conn)
|
|
require.NoError(t, err)
|
|
defer func() {
|
|
InvalidateSquareCustomerCache(userID)
|
|
_, _ = db.Conn.Exec(ctx, `DELETE FROM user_saved_cards WHERE user_id = $1`, userID)
|
|
_, _ = db.Conn.Exec(ctx, `DELETE FROM users WHERE id = $1`, userID)
|
|
}()
|
|
|
|
origClient := SquareClient
|
|
rec := &orphanCardClient{SquareClient: square.NewDevClient()}
|
|
SquareClient = rec
|
|
defer func() { SquareClient = origClient }()
|
|
|
|
token := "cnon:test-save-happy"
|
|
sourceID, savedCardID, sqCustID, ok := resolveChargeSource(ctx, httptest.NewRecorder(), NewPaymentService(), userID, &token, nil, true, "")
|
|
require.True(t, ok, "save-card source resolution must succeed on the happy path")
|
|
require.True(t, strings.HasPrefix(sourceID, "ccof:"), "source must be the created card-on-file, got %q", sourceID)
|
|
require.NotNil(t, savedCardID, "a successful SaveCardForUser must return the local row id")
|
|
require.NotEmpty(t, sqCustID, "the provisioned Square customer id must be returned")
|
|
|
|
var rows int
|
|
require.NoError(t, db.Conn.QueryRow(ctx, `SELECT COUNT(*) FROM user_saved_cards WHERE user_id = $1 AND square_card_id = $2`, userID, sourceID).Scan(&rows))
|
|
require.Equal(t, 1, rows, "the card must be persisted as a user_saved_cards row")
|
|
|
|
require.Empty(t, rec.deletedIDs(), "a successfully saved card must never be disabled at Square")
|
|
}
|
|
|
|
// TestResolveChargeSource_SaveCard_OrphanCleanedUp verifies the orphan-card
|
|
// fix: when CreateCardOnFile succeeds but the local DB save fails, the
|
|
// just-created Square card is disabled (DeleteCardOnFile) so no card-on-file
|
|
// is left at Square without a DB row. The charge must still resolve ok=true.
|
|
func TestResolveChargeSource_SaveCard_OrphanCleanedUp(t *testing.T) {
|
|
ctx := context.Background()
|
|
// A non-existent user: EnsureSquareCustomer is bypassed via the cache, and
|
|
// SaveCardForUser's INSERT fails on the users(id) FK — a clean injection of
|
|
// the DB-save failure without touching other test state.
|
|
userID := "c_orphan_00"
|
|
squareCustomerCache.Store(userID, "cus_orphan")
|
|
defer InvalidateSquareCustomerCache(userID)
|
|
|
|
origClient := SquareClient
|
|
rec := &orphanCardClient{SquareClient: square.NewDevClient()}
|
|
SquareClient = rec
|
|
defer func() { SquareClient = origClient }()
|
|
|
|
token := "cnon:test-orphan-cleanup"
|
|
sourceID, savedCardID, sqCustID, ok := resolveChargeSource(ctx, httptest.NewRecorder(), NewPaymentService(), userID, &token, nil, true, "")
|
|
require.True(t, ok, "a local save failure must NOT fail the charge")
|
|
require.Nil(t, savedCardID, "no local saved-card row must exist after the failed save")
|
|
require.Equal(t, "cus_orphan", sqCustID)
|
|
require.True(t, strings.HasPrefix(sourceID, "ccof:"), "source must still be the created card-on-file, got %q", sourceID)
|
|
|
|
deletes := rec.deletedIDs()
|
|
require.Len(t, deletes, 1, "the just-created Square card must be disabled exactly once")
|
|
require.Equal(t, sourceID, deletes[0], "the disabled card must be the one this call just created")
|
|
|
|
var rows int
|
|
require.NoError(t, db.Conn.QueryRow(ctx, `SELECT COUNT(*) FROM user_saved_cards WHERE user_id = $1`, userID).Scan(&rows))
|
|
require.Zero(t, rows, "no orphaned saved-card row may exist")
|
|
}
|
|
|
|
// TestResolveChargeSource_SaveCard_CleanupFailureStillCharges verifies the
|
|
// best-effort contract: when the DB save fails AND the Square disable also
|
|
// fails, the charge must still resolve ok=true (the orphan is only logged for
|
|
// manual cleanup, never allowed to fail the request).
|
|
func TestResolveChargeSource_SaveCard_CleanupFailureStillCharges(t *testing.T) {
|
|
ctx := context.Background()
|
|
userID := "c_orphan_01"
|
|
squareCustomerCache.Store(userID, "cus_orphan")
|
|
defer InvalidateSquareCustomerCache(userID)
|
|
|
|
origClient := SquareClient
|
|
rec := &orphanCardClient{SquareClient: square.NewDevClient(), failDelete: true}
|
|
SquareClient = rec
|
|
defer func() { SquareClient = origClient }()
|
|
|
|
token := "cnon:test-orphan-cleanup-fail"
|
|
sourceID, savedCardID, _, ok := resolveChargeSource(ctx, httptest.NewRecorder(), NewPaymentService(), userID, &token, nil, true, "")
|
|
require.True(t, ok, "a failed Square disable must never fail the charge")
|
|
require.Nil(t, savedCardID)
|
|
require.True(t, strings.HasPrefix(sourceID, "ccof:"), "source must be the created card-on-file, got %q", sourceID)
|
|
require.Equal(t, []string{sourceID}, rec.deletedIDs(), "the disable must be attempted even when it will fail")
|
|
}
|