From 2a78383a2d832d46f9945110fb87ba35ee4acef4 Mon Sep 17 00:00:00 2001 From: Stephen Adamson Date: Mon, 3 Aug 2026 18:11:04 +0100 Subject: [PATCH] Expand payment remediation test coverage Adds tests for chargeFailureStatus retryable-vs-definitive classification (429/408/425 -> 503, 4xx declines -> 402), legacy NULL-key refund resume, tip/discount split-record math, and the shared charge helpers adopted by till and gift-card paths. --- backend/handlers/payments/errors_test.go | 161 ++++++++++++++ .../handlers/payments/payment_status_test.go | 48 +++++ .../payments/payments_r6_review_test.go | 66 ++++++ backend/handlers/payments/payments_test.go | 199 +++++++++++++++++- backend/handlers/payments/refunds_test.go | 98 +++++++++ 5 files changed, 563 insertions(+), 9 deletions(-) create mode 100644 backend/handlers/payments/errors_test.go diff --git a/backend/handlers/payments/errors_test.go b/backend/handlers/payments/errors_test.go new file mode 100644 index 0000000..2c471ad --- /dev/null +++ b/backend/handlers/payments/errors_test.go @@ -0,0 +1,161 @@ +//go:build test && dev + +package payments + +import ( + "context" + "errors" + "net/http" + "reflect" + "testing" + + "crussell/internal/square" + "crussell/testutils" +) + +// structuredSquareAPIError returns an error of the SAME concrete type the real +// Square client produces for structured API errors (the unexported +// *square.squareAPIError, re-stamped with the given HTTP status). The type is +// not nameable outside internal/square and there is no exported constructor, +// so the helper clones the dev mock's real structured 400 error (the only +// package-visible producer) via reflection and rewrites its status code. This +// mirrors the existing test's "build through the mock" style while covering +// status codes the mock cannot produce (429/408/425/422/500). +func structuredSquareAPIError(t *testing.T, status int) error { + t.Helper() + mc := square.NewDevClient().(*square.MockClient) + _, err := mc.CreatePayment(context.Background(), square.CreatePaymentReq{ + Amount: 1000, + Currency: "GBP", + SourceID: "ccof:card_1", + }) + if err == nil { + t.Fatal("expected the mock to reject a ccof charge without a customer") + } + if square.ErrorStatusCode(err) == 0 { + t.Fatal("expected the mock's ccof rejection to carry a structured status code") + } + v := reflect.ValueOf(err) + if v.Kind() != reflect.Ptr { + t.Fatalf("expected the structured error to be a pointer, got %v", v.Kind()) + } + clone := reflect.New(v.Elem().Type()) + clone.Elem().Set(v.Elem()) + clone.Elem().FieldByName("StatusCode").SetInt(int64(status)) + return clone.Interface().(error) +} + +// TestChargeFailureStatus classifies Square CreatePayment errors into +// 402 (definitive decline) vs 503 (ambiguous) so the charge handlers surface +// retryable failures as 503 (the pending record is resumed on a same-key +// retry) and only definitively-rejected charges as 402. +func TestChargeFailureStatus(t *testing.T) { + ctx := context.Background() + mc := square.NewDevClient().(*square.MockClient) + + // The dev mock produces a structured 4xx squareAPIError for a + // card-on-file charge missing its required customer (mirrors a real + // Square 400 INVALID_REQUEST_ERROR) — exercises the definitive-decline + // classification through the real error type. + _, structuredErr := mc.CreatePayment(ctx, square.CreatePaymentReq{ + Amount: 1000, + Currency: "GBP", + SourceID: "ccof:card_1", + }) + if structuredErr == nil { + t.Fatal("expected the mock to reject a ccof charge without a customer") + } + if square.ErrorStatusCode(structuredErr) == 0 { + t.Fatal("expected the mock's ccof rejection to carry a structured status code") + } + + tests := []struct { + name string + err error + want int + }{ + {"structured 4xx decline → 402", structuredErr, http.StatusPaymentRequired}, + {"plain mock failure (ambiguous) → 503", errors.New("mock: payment declined (simulated failure)"), http.StatusServiceUnavailable}, + {"context deadline → 503", context.DeadlineExceeded, http.StatusServiceUnavailable}, + {"context cancelled → 503", context.Canceled, http.StatusServiceUnavailable}, + {"nil (defensive) → 402", nil, http.StatusPaymentRequired}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := chargeFailureStatus(tt.err); got != tt.want { + t.Errorf("chargeFailureStatus(%v) = %d, want %d", tt.err, got, tt.want) + } + }) + } +} + +// TestChargeFailureStatus_RetryableCarveOuts locks the 429/408/425 carve-outs: +// those retryable/ambiguous 4xx statuses must classify as 503 (ambiguous — +// retry later), never as the 402 (definitive decline) that the generic 4xx +// branch would produce. True declines (400/422) and 5xx keep their existing +// classifications. +func TestChargeFailureStatus_RetryableCarveOuts(t *testing.T) { + tests := []struct { + name string + err error + want int + }{ + {"structured 429 rate limited (retryable) → 503", structuredSquareAPIError(t, http.StatusTooManyRequests), http.StatusServiceUnavailable}, + {"structured 408 request timeout (ambiguous) → 503", structuredSquareAPIError(t, http.StatusRequestTimeout), http.StatusServiceUnavailable}, + {"structured 425 too early (ambiguous) → 503", structuredSquareAPIError(t, http.StatusTooEarly), http.StatusServiceUnavailable}, + {"structured 422 unprocessable (definitive) → 402", structuredSquareAPIError(t, http.StatusUnprocessableEntity), http.StatusPaymentRequired}, + {"structured 400 bad request (definitive) → 402", structuredSquareAPIError(t, http.StatusBadRequest), http.StatusPaymentRequired}, + {"structured 500 server error (ambiguous) → 503", structuredSquareAPIError(t, http.StatusInternalServerError), http.StatusServiceUnavailable}, + {"plain error (ambiguous) → 503", errors.New("mock: payment declined (simulated failure)"), http.StatusServiceUnavailable}, + {"context deadline (ambiguous) → 503", context.DeadlineExceeded, http.StatusServiceUnavailable}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := chargeFailureStatus(tt.err); got != tt.want { + t.Errorf("chargeFailureStatus(%v) = %d, want %d", tt.err, got, tt.want) + } + }) + } +} + +// TestCreateBookingPayment_AmbiguousSquareFailure_Returns503 verifies the +// charge-failure classification end to end: the dev mock's simulated failure +// is a PLAIN error (no structured Square status), so the handler now returns +// 503 (ambiguous — the pending record stays pending for a same-key retry) +// instead of 402 (which implied a definitive decline). +func TestCreateBookingPayment_AmbiguousSquareFailure_Returns503(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + _, bookingID, userToken := setupPaymentStatusTest(t, ctx, tx, "confirmed") + + origClient := SquareClient + mc := square.NewDevClient().(*square.MockClient) + mc.ShouldFail = true + SquareClient = mc + defer func() { SquareClient = origClient }() + + cardToken := "cnon:test-card-nonce" + req := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "deposit", + NewCardToken: &cardToken, + IdempotencyKey: "ambiguous-503-" + bookingID, + } + + handler := CreateBookingPayment + w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + + if w.Code != http.StatusServiceUnavailable { + t.Fatalf("expected 503 for ambiguous mock Square failure, got %d: %s", w.Code, w.Body.String()) + } + + // The pending record must be left pending (not failed) so a same-key retry + // reuses it instead of creating a second Square charge. + var status string + err := tx.QueryRow(ctx, `SELECT status FROM payments WHERE booking_id = $1 AND idempotency_key = $2`, bookingID, req.IdempotencyKey).Scan(&status) + if err != nil { + t.Fatalf("failed to query payment status: %v", err) + } + if status != "pending" { + t.Errorf("expected payment status 'pending' after ambiguous failure, got %q", status) + } +} diff --git a/backend/handlers/payments/payment_status_test.go b/backend/handlers/payments/payment_status_test.go index a103eb4..e30a57c 100644 --- a/backend/handlers/payments/payment_status_test.go +++ b/backend/handlers/payments/payment_status_test.go @@ -822,6 +822,54 @@ func (c *mismatchedRefCheckoutClient) GetCheckout(ctx context.Context, checkoutI return c.result, nil } +func TestGetCheckoutStatus_EmptyReferenceID_Returns400(t *testing.T) { + // A checkout with an EMPTY reference_id was created outside this app (no + // booking was attached at creation time) — it must NOT be attachable to a + // booking via polling. Fail closed with 400, exactly like a mismatched + // reference, so a mis-scoped charge is never recorded. + ctx, tx := testutils.SetupTestTx(t) + _, bookingID, _ := setupTestData(t, ctx, tx) + + origClient := SquareClient + SquareClient = &mismatchedRefCheckoutClient{ + SquareClient: square.NewDevClient(), + result: &square.PaymentResult{ + ID: "pay_empty_ref", + Status: "COMPLETED", + Amount: 5000, + SquarePayID: "pay_empty_ref", + ReferenceID: "", // created outside this app — no booking reference + CreatedAt: "2026-07-31T00:00:00Z", + UpdatedAt: "2026-07-31T00:00:00Z", + }, + } + defer func() { SquareClient = origClient }() + + checkoutID := "abcd1234ef56" // 12 hex chars, passes the checkout-id validation + req := httptest.NewRequest("GET", "/api/admin/payments/"+checkoutID+"/status?booking_id="+bookingID, nil) + rctx := chi.NewRouteContext() + rctx.URLParams.Add("checkout_id", checkoutID) + reqCtx := context.WithValue(req.Context(), chi.RouteCtxKey, rctx) + reqCtx = context.WithValue(reqCtx, mw.UserIDKey, "000000000001") + reqCtx = context.WithValue(reqCtx, mw.UserRoleKey, "admin") + req = req.WithContext(reqCtx) + + w := httptest.NewRecorder() + GetCheckoutStatus(w, req) + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400 for empty reference_id, got %d: %s", w.Code, w.Body.String()) + } + + // No payment may be recorded for the unreferenced checkout. + var rowCount int + if err := tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE booking_id = $1", bookingID).Scan(&rowCount); err != nil { + t.Fatalf("failed to count payment rows: %v", err) + } + if rowCount != 0 { + t.Errorf("expected no payment rows after empty reference, got %d", rowCount) + } +} + func TestGetCheckoutStatus_ReferenceIDMismatch_Returns400(t *testing.T) { // The terminal checkout's reference_id must match the booking being // polled; a checkout that references a different booking is refused with diff --git a/backend/handlers/payments/payments_r6_review_test.go b/backend/handlers/payments/payments_r6_review_test.go index aba6812..24a4e61 100644 --- a/backend/handlers/payments/payments_r6_review_test.go +++ b/backend/handlers/payments/payments_r6_review_test.go @@ -3,10 +3,13 @@ package payments import ( + "bytes" "context" "fmt" + "log/slog" "net/http" "net/http/httptest" + "strings" "sync" "testing" @@ -345,3 +348,66 @@ type failingDeleteClient struct { func (c *failingDeleteClient) DeleteCardOnFile(ctx context.Context, cardID string) error { return fmt.Errorf("square: network error disabling card %s", cardID) } + +// syncBuffer is a mutex-guarded slog writer so log records can be read safely +// under -race. +type syncBuffer struct { + mu sync.Mutex + buf bytes.Buffer +} + +func (b *syncBuffer) Write(p []byte) (int, error) { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.Write(p) +} + +func (b *syncBuffer) String() string { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.String() +} + +// tokenSafeFailingDeleteClient fails with a token-free error so the +// log-redaction test isolates redaction of the square_card_id attribute rather +// than the error string. +type tokenSafeFailingDeleteClient struct { + square.SquareClient +} + +func (c *tokenSafeFailingDeleteClient) DeleteCardOnFile(ctx context.Context, cardID string) error { + return fmt.Errorf("square: network error disabling card at Square") +} + +// TestDeletePaymentMethod_LogsRedactCardToken verifies the local card-delete +// warning logs the redacted tokenPrefix form of the square_card_id (a ccof: +// token), never the full value (SECURITY: full ccof tokens must not reach +// server logs). +func TestDeletePaymentMethod_LogsRedactCardToken(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + fullToken := "ccof:secret_token_123456" + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, fullToken, "VISA", "9999") + require.NoError(t, err) + + origClient := SquareClient + SquareClient = &tokenSafeFailingDeleteClient{SquareClient: square.NewDevClient()} + defer func() { SquareClient = origClient }() + + var sb syncBuffer + origLogger := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&sb, nil))) + defer slog.SetDefault(origLogger) + + svc := NewPaymentService() + require.NoError(t, svc.DeletePaymentMethod(ctx, cardID, userID)) + + logs := sb.String() + if strings.Contains(logs, fullToken) { + t.Errorf("full ccof token %q leaked into logs: %q", fullToken, logs) + } + if !strings.Contains(logs, "ccof:sec...") { + t.Errorf("expected redacted token prefix in logs, got %q", logs) + } +} diff --git a/backend/handlers/payments/payments_test.go b/backend/handlers/payments/payments_test.go index 0cf8d78..243449f 100644 --- a/backend/handlers/payments/payments_test.go +++ b/backend/handlers/payments/payments_test.go @@ -5,9 +5,11 @@ package payments import ( "bytes" "context" + "crypto/sha256" "encoding/base64" "encoding/json" "fmt" + "math" "net/http" "net/http/httptest" "testing" @@ -607,8 +609,12 @@ func TestRefund_FullRefund(t *testing.T) { func TestRefund_SameKeyRetry_Dedups(t *testing.T) { // A retry of the same refund (network timeout, double-click) must not - // create a second Square refund or a second DB row — the idempotency key - // (paymentID + amount) dedups it. + // create a second Square refund or a second DB row. Retry dedup runs on + // the CLIENT-supplied idempotency key — a UUID generated per refund + // attempt and REUSED on retry (hashed+truncated into the stored key), so + // the same key dedups. A NO-key refund now carries a fresh random fallback + // key per attempt and never dedups against a prior no-key refund — that is + // deliberate (see TestRefund_TwoEqualPartialRefunds_NoClientKey_DoNotCollide). t.Parallel() ctx, tx := testutils.SetupTestTx(t) @@ -631,8 +637,9 @@ func TestRefund_SameKeyRetry_Dedups(t *testing.T) { } req := RefundRequest{ - Amount: 5000, - Reason: "customer request", + Amount: 5000, + Reason: "customer request", + IdempotencyKey: "refund-same-key-retry-uuid", } handler := RefundPayment @@ -642,7 +649,8 @@ func TestRefund_SameKeyRetry_Dedups(t *testing.T) { t.Fatalf("first refund: expected 200, got %d. body: %s", w1.Code, w1.Body.String()) } - // Same-key retry (identical amount, reason) — must dedup, not double-refund. + // Same-key retry (identical client key, amount, reason) — must dedup, not + // double-refund. w2 := makePaymentRequest(handler, "POST", "/api/admin/payments/"+paymentID+"/refund", req, adminToken, ctx) if w2.Code != http.StatusOK { t.Fatalf("retry refund: expected 200, got %d. body: %s", w2.Code, w2.Body.String()) @@ -659,6 +667,71 @@ func TestRefund_SameKeyRetry_Dedups(t *testing.T) { } } +// TestRefund_TwoEqualPartialRefunds_NoClientKey_DoNotCollide verifies the P2 +// security fix for the DEFAULT (no client key) path: the fallback idempotency +// key is now unique per refund attempt, so two DISTINCT partial refunds of the +// SAME amount against the SAME payment each get their own key and both +// complete. With the old amount-derived default key (paymentID + "-refund-" + +// amount) the second would collide with the first and be silently swallowed by +// the dedup lookup. +func TestRefund_TwoEqualPartialRefunds_NoClientKey_DoNotCollide(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + _, bookingID, _ := setupTestData(t, ctx, tx) + + adminID, err := fixtures.CreateTestAdminUser(tx) + if err != nil { + t.Fatalf("failed to create admin user: %v", err) + } + adminToken := jwt.GenerateTestToken(adminID, "admin") + + paymentID, err := fixtures.CreateTestPayment(tx, bookingID, 5000.00, "in_person_card", "full", "completed") + if err != nil { + t.Fatalf("failed to create payment: %v", err) + } + _, err = tx.Exec(ctx, "UPDATE payments SET square_payment_id = 'sqp_test_no_key_equal_refunds' WHERE id = $1", paymentID) + if err != nil { + t.Fatalf("failed to update payment: %v", err) + } + + handler := RefundPayment + // Two distinct £20 partial refunds of the same £50 payment, NO client + // idempotency key on either — the exact case that collided on the + // amount-derived default key before the fix. + refund1 := RefundRequest{Amount: 2000, Reason: "partial one"} + w1 := makePaymentRequest(handler, "POST", "/api/admin/payments/"+paymentID+"/refund", refund1, adminToken, ctx) + if w1.Code != http.StatusOK { + t.Fatalf("first refund: expected 200, got %d. body: %s", w1.Code, w1.Body.String()) + } + + refund2 := RefundRequest{Amount: 2000, Reason: "partial two"} + w2 := makePaymentRequest(handler, "POST", "/api/admin/payments/"+paymentID+"/refund", refund2, adminToken, ctx) + if w2.Code != http.StatusOK { + t.Fatalf("second equal partial refund: expected 200, got %d. body: %s", w2.Code, w2.Body.String()) + } + + // Both refunds must exist as separate completed rows with DIFFERENT + // fallback keys — the second was NOT swallowed by the first's dedup lookup. + var completedCount int + err = tx.QueryRow(ctx, `SELECT COUNT(*) FROM refunds WHERE payment_id = $1 AND status = 'completed'`, paymentID).Scan(&completedCount) + if err != nil { + t.Fatalf("failed to query refunds: %v", err) + } + if completedCount != 2 { + t.Errorf("expected 2 completed refund rows, got %d (second was swallowed!)", completedCount) + } + + var distinctKeys int + err = tx.QueryRow(ctx, `SELECT COUNT(DISTINCT idempotency_key) FROM refunds WHERE payment_id = $1`, paymentID).Scan(&distinctKeys) + if err != nil { + t.Fatalf("failed to count distinct refund keys: %v", err) + } + if distinctKeys != 2 { + t.Errorf("expected 2 DISTINCT fallback keys for the two same-amount refunds, got %d", distinctKeys) + } +} + // TestRefund_TwoEqualPartialRefunds_ClientKeyDisambiguates verifies the P2 fix: // two DISTINCT partial refunds of the same amount against the same payment // must both complete. With only the amount-derived key (paymentID-amount) the @@ -1012,8 +1085,12 @@ func TestRefund_PendingSameKeyRetry_Resumes(t *testing.T) { t.Fatalf("failed to update payment: %v", err) } - // Seed a pending refund with the same key the handler will compute. - key := paymentID + "-refund-2500" + // Seed a pending refund with the key the handler derives from the client + // key below (sha256-hashed + truncated to 24 hex chars). The retry sends + // the SAME client key, so the exact-key dedup resumes the row. + clientKey := "refund-pending-resume-uuid" + ikHash := sha256.Sum256([]byte(clientKey)) + key := paymentID + "-refund-" + fmt.Sprintf("%x", ikHash)[:24] _, err = tx.Exec(ctx, ` INSERT INTO refunds (payment_id, booking_id, amount, status, reason, idempotency_key, created_at) VALUES ($1, $2, 25, 'pending', 'customer request', $3, NOW()) @@ -1023,8 +1100,9 @@ func TestRefund_PendingSameKeyRetry_Resumes(t *testing.T) { } req := RefundRequest{ - Amount: 2500, - Reason: "customer request", + Amount: 2500, + Reason: "customer request", + IdempotencyKey: clientKey, } handler := RefundPayment @@ -2069,6 +2147,109 @@ func TestBuildSplitRecords_OverflowBeyondTotal_BecomesTip(t *testing.T) { } } +// TestBuildSplitRecords_DiscountBooking_TipOverflow_SumNeverExceedsCharge +// proves the money invariant documented on buildSplitRecords: the split +// records (deposit + balance + tip) ALWAYS partition the charged amount +// exactly, so their sum can never exceed what Square actually charged — even +// when a discount is present. A discount reduces what the customer owes but is +// booked as a SEPARATE ledger row (payment_method='discount') that +// GetBookingPaymentInfo excludes from TotalPaid, so buildSplitRecords runs +// against the full booking total and never inflates the split sum past the +// charged pence. +func TestBuildSplitRecords_DiscountBooking_TipOverflow_SumNeverExceedsCharge(t *testing.T) { + // Discounted booking: £100 total with a £10 discount (the discount row + // exists in the ledger but is excluded from TotalPaid). Tip overflow + // charges over and above the booking total. + record := makeTestRecord("b-disc-tip", "full", 120) + info := &BookingPaymentInfo{ + StartTime: clock.Now().Add(48 * time.Hour), + TotalAmount: 100, + TotalPaid: 0, // the £10 discount payment row is excluded here + } + records := buildSplitRecords(record, "full", info, 120) + + var sum float64 + for _, r := range records { + sum += r.Amount + } + // Partition invariant: the sum equals the charged amount exactly — it can + // never exceed it, no matter how the tip overflows. + if sum > 120 { + t.Errorf("split records sum to %.2f, exceeding the charged amount £120", sum) + } + if math.Abs(sum-120) > 0.005 { + t.Errorf("expected split records to partition the charged £120 exactly, got sum %.2f (records: %+v)", sum, records) + } + + // Same invariant with real money already paid toward the booking: the + // deposit room is consumed, but the sum still partitions the new charge. + record2 := makeTestRecord("b-disc-tip-2", "full", 70) + info2 := &BookingPaymentInfo{ + StartTime: clock.Now().Add(48 * time.Hour), + TotalAmount: 100, + TotalPaid: 60, + } + records2 := buildSplitRecords(record2, "full", info2, 70) + var sum2 float64 + for _, r := range records2 { + sum2 += r.Amount + } + if sum2 > 70 { + t.Errorf("split records sum to %.2f, exceeding the charged amount £70", sum2) + } + if math.Abs(sum2-70) > 0.005 { + t.Errorf("expected split records to partition the charged £70 exactly, got sum %.2f (records: %+v)", sum2, records2) + } +} + +// TestGetBookingPaymentInfo_ExcludesDiscountPayments proves the database half +// of the invariant: GetBookingPaymentInfo.TotalPaid excludes discount payment +// rows, so a discounted booking never inflates TotalPaid (which would shrink +// the tip carve-out / grow the balance allocation into an over-recorded total). +func TestGetBookingPaymentInfo_ExcludesDiscountPayments(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + serviceID, err := fixtures.CreateTestService(tx) + require.NoError(t, err) + bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + require.NoError(t, err) + + // £60 of real money plus a £10 discount payment row (payment_method='discount'). + _, err = fixtures.CreateTestPayment(tx, bookingID, 60.00, "online_square", "full", "completed") + require.NoError(t, err) + _, err = tx.Exec(ctx, ` + INSERT INTO payments (booking_id, payment_type, payment_method, amount, status, created_by) + VALUES ($1, 'partial', 'discount', 10.00, 'completed', $2) + `, bookingID, userID) + require.NoError(t, err) + + svc := NewPaymentService() + info, err := svc.GetBookingPaymentInfo(ctx, bookingID) + require.NoError(t, err) + + if info.TotalPaid != 60.00 { + t.Errorf("expected TotalPaid to exclude the £10 discount row, got %.2f", info.TotalPaid) + } + + // A £70 tip-overflow charge on top must partition exactly into £70 of split + // records — the discount can never push the recorded sum past the charge. + record := makeTestRecord(bookingID, "full", 70) + records := buildSplitRecords(record, "full", info, 70) + var sum float64 + for _, r := range records { + sum += r.Amount + } + if sum > 70 { + t.Errorf("split records sum to %.2f, exceeding the charged amount £70", sum) + } + if math.Abs(sum-70) > 0.005 { + t.Errorf("expected split records to partition the charged £70 exactly, got sum %.2f", sum) + } +} + // --------------------------------------------------------------------------- // Handler-level atomicity — verify the full handler succeeds with split. // --------------------------------------------------------------------------- diff --git a/backend/handlers/payments/refunds_test.go b/backend/handlers/payments/refunds_test.go index b3e78d1..2c0e501 100644 --- a/backend/handlers/payments/refunds_test.go +++ b/backend/handlers/payments/refunds_test.go @@ -6,6 +6,7 @@ import ( "context" "fmt" "net/http" + "strings" "sync" "testing" "time" @@ -2645,6 +2646,103 @@ func TestSweepPendingSquareRefunds_RetriesStaleManualRefund(t *testing.T) { } } +// ============================================================================= +// D2b — legacy pending refund rows with NULL idempotency_key resume with a key +// ============================================================================= + +// TestResumePendingRefund_LegacyNullKey_DerivesFreshKey verifies that resuming +// a pending refund whose row has a NULL idempotency_key (a legacy row created +// before keyed refunds) re-issues the Square refund with a NON-EMPTY derived +// key. Square's RefundPayment REQUIRES a non-empty idempotency key — re-issuing +// with "" would return a 400 INVALID_REQUEST_ERROR that classifies as ambiguous +// and leaves the refund pending forever. +func TestResumePendingRefund_LegacyNullKey_DerivesFreshKey(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + adminID, err := fixtures.CreateTestAdminUser(tx) + if err != nil { + t.Fatalf("failed to create admin user: %v", err) + } + serviceID, err := fixtures.CreateTestService(tx) + if err != nil { + t.Fatalf("failed to create service: %v", err) + } + bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, + time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + if err != nil { + t.Fatalf("failed to create booking: %v", err) + } + + paymentID, err := fixtures.CreateTestPayment(tx, bookingID, 50.00, "online_square", "full", "completed") + if err != nil { + t.Fatalf("failed to create card payment: %v", err) + } + _, err = tx.Exec(ctx, "UPDATE payments SET square_payment_id = 'sqp_legacy_null_key' WHERE id = $1", paymentID) + if err != nil { + t.Fatalf("failed to set square_payment_id: %v", err) + } + + // Seed a pending refund row with NO idempotency_key column value → NULL + // (legacy row). The (payment_id, amount) pending fallback finds it. + var refundID string + err = tx.QueryRow(ctx, ` + INSERT INTO refunds (payment_id, booking_id, amount, status, reason, origin, created_at) + VALUES ($1, $2, 50, 'pending', 'customer request', 'manual', NOW()) + RETURNING id + `, paymentID, bookingID).Scan(&refundID) + if err != nil { + t.Fatalf("failed to insert legacy pending refund with NULL idempotency_key: %v", err) + } + + origClient := SquareClient + counting := &countingRefundClient{SquareClient: square.NewDevClient()} + SquareClient = counting + defer func() { SquareClient = origClient }() + + // A NEW client-supplied key makes the exact-key dedup miss, so the pending + // (payment_id, amount) fallback resumes the legacy NULL-key row. + adminToken := jwt.GenerateTestToken(adminID, "admin") + req := RefundRequest{Amount: 5000, Reason: "customer request", IdempotencyKey: "fresh-client-key-legacy"} + rec := makePaymentRequest(RefundPayment, "POST", "/api/admin/payments/"+paymentID+"/refund", req, adminToken, ctx) + + if rec.Code != http.StatusOK { + t.Fatalf("expected 200 on resume, got %d: %s", rec.Code, rec.Body.String()) + } + + calls := counting.refundCalls() + if len(calls) != 1 { + t.Fatalf("expected exactly 1 Square refund call for the resumed legacy refund, got %d", len(calls)) + } + if calls[0].IdempotencyKey == "" { + t.Fatal("expected a NON-EMPTY idempotency key for the resumed legacy refund (Square RefundPayment requires one)") + } + if calls[0].IdempotencyKey == req.IdempotencyKey { + t.Errorf("expected a derived key, not the client's fresh key, got %q", calls[0].IdempotencyKey) + } + // Derived shape: -refund--<12 hex chars>. + prefix := paymentID + "-refund-5000-" + if !strings.HasPrefix(calls[0].IdempotencyKey, prefix) { + t.Errorf("expected derived key with prefix %q, got %q", prefix, calls[0].IdempotencyKey) + } + if len(calls[0].IdempotencyKey) > 45 { + t.Errorf("expected derived key within Square's 45-char limit, got %d chars: %q", len(calls[0].IdempotencyKey), calls[0].IdempotencyKey) + } + + // The row must resolve to completed (the mock returns COMPLETED). + var status string + err = db.Conn.QueryRow(ctx, `SELECT status FROM refunds WHERE id = $1`, refundID).Scan(&status) + if err != nil { + t.Fatalf("failed to query refund status: %v", err) + } + if status != "completed" { + t.Errorf("expected resumed refund status 'completed', got %q", status) + } +} + // ============================================================================= // D3 — the 23h age guard reconciles against Square before marking failed // =============================================================================