Implement full Square payment review fixes + frontend polish
Implement every finding from the deep payment review (P0-P2, minors, nitpicks), then close the post-implementation re-review items, then align card-form typography and roll out the Square trust badge. Backend - Square API alignment: - tip_settings.allow_tipping nested under device_options (was top-level: terminal tips were silently lost in prod) - CreateCardOnFile now accepts customerID and sends card.customer_id; saved-card (ccof:) charges forward square_customer_id as CustomerID - New SquareClient methods GetPayment, CreateCustomer, CancelCheckout - SCA verification_token accepted + forwarded in all charge paths - ExpMonth/ExpYear -> *int; URL-path id validation; CancelCheckout NOT_FOUND-only no-op (dropped unverified NOOP); exported ErrorCode/ ErrorDetail helpers; mock rejects raw PANs, RList locks, redacts emails, ForceRefundPending hook Backend - money safety: - sweepManualPendingSquareRefunds reconciles rows WITH square_refund_id instead of stranding them forever - SweepStalePendingPayments reconciles at Square before failing (tri-state: leave pending on transport error, rescue completed, fail definitively) - GetCheckoutStatus cancellation-recheck; terminal CANCELED resolution; SweepStaleTerminalCheckouts covers terminal_checkouts table - till gift-card clawback on definitive failure incl. retry path + INSUFFICIENT_FUNDS/ADDRESS_VERIFICATION_FAILURE/TRANSACTION_LIMIT - cross-user saved-card collision fixed (UNIQUE(user_id,square_card_id)) - customer provisioning (lazy, save-only); one-off/guest mint no customer - discount preview/apply unified in discounts.go (global-milestone visible in preview, N+1 eliminated, redemption counter preserved on failures) - webhook event_id dedup; refund loop dedup; stale comment fixes - test-isolation t.Cleanup on committed sweep tests Frontend: - SCA tokenizeWithVerification across all charge flows (amount as major-units decimal), 5-min token-expiry re-tokenize, verification_token in request bodies - PaymentModal synchronous double-click + zero/negative-amount guards - till online-card UI wired to /api/admin/till/sale - policyPopover generalised; new /privacy-policy route; consent checkbox copy + Square privacy link - Square card iframe styled to app typography (Inter 14px, oklch tokens); mock form md:text-sm parity - 'Secure payment powered by Square' badge on all 8 card-payment flows Schema/docs: terminal_checkouts + square_customer_id + per-user card constraint in init-script.sql; README migrations; P14 plan + backlog + Technical Manual updated. Includes 39 modified/new test files; full backend suite (25 pkgs), -race on payments+square, and frontend build are green.
This commit is contained in:
@@ -3,9 +3,13 @@
|
||||
package square
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"log"
|
||||
"os"
|
||||
"strings"
|
||||
"sync"
|
||||
"testing"
|
||||
"time"
|
||||
@@ -36,8 +40,10 @@ func TestDevClient_CreatePayment_ReturnsCompleted(t *testing.T) {
|
||||
assert.NotZero(t, result.Fees)
|
||||
|
||||
assert.NotEmpty(t, result.CardFingerprint)
|
||||
assert.Equal(t, 12, result.ExpMonth)
|
||||
assert.Equal(t, 2030, result.ExpYear)
|
||||
require.NotNil(t, result.ExpMonth)
|
||||
assert.Equal(t, 12, *result.ExpMonth)
|
||||
require.NotNil(t, result.ExpYear)
|
||||
assert.Equal(t, 2030, *result.ExpYear)
|
||||
assert.Equal(t, "KEYED", result.EntryMethod)
|
||||
assert.Equal(t, "CVV_ACCEPTED", result.CVVStatus)
|
||||
assert.Equal(t, "AVS_ACCEPTED", result.AVSStatus)
|
||||
@@ -55,7 +61,7 @@ func TestDevClient_CreateCheckout_PendingThenCompleted(t *testing.T) {
|
||||
Currency: "GBP",
|
||||
IdempotencyKey: "checkout-key-1",
|
||||
ReferenceID: "booking-456",
|
||||
TipEnabled: true,
|
||||
AllowTipping: true,
|
||||
}
|
||||
|
||||
result, err := client.CreateCheckout(ctx, req)
|
||||
@@ -91,7 +97,7 @@ func TestDevClient_CreateCheckout_NoTip(t *testing.T) {
|
||||
Currency: "GBP",
|
||||
IdempotencyKey: "checkout-key-notip",
|
||||
ReferenceID: "booking-789",
|
||||
TipEnabled: false,
|
||||
AllowTipping: false,
|
||||
}
|
||||
|
||||
result, err := client.CreateCheckout(ctx, req)
|
||||
@@ -157,7 +163,7 @@ func TestDevClient_CardOnFile_CreateAndGet(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
userID := "user-test-123"
|
||||
|
||||
card, err := client.CreateCardOnFile(ctx, userID, "cnon:test-token")
|
||||
card, err := client.CreateCardOnFile(ctx, userID, "cnon:test-token", "")
|
||||
require.NoError(t, err)
|
||||
|
||||
assert.NotEmpty(t, card.ID)
|
||||
@@ -182,10 +188,10 @@ func TestDevClient_CardOnFile_MultipleCards(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
userID := "user-test-multiple"
|
||||
|
||||
card1, err := client.CreateCardOnFile(ctx, userID, "cnon:token-1")
|
||||
card1, err := client.CreateCardOnFile(ctx, userID, "cnon:token-1", "")
|
||||
require.NoError(t, err)
|
||||
|
||||
card2, err := client.CreateCardOnFile(ctx, userID, "cnon:token-2")
|
||||
card2, err := client.CreateCardOnFile(ctx, userID, "cnon:token-2", "")
|
||||
require.NoError(t, err)
|
||||
|
||||
assert.True(t, card1.Enabled)
|
||||
@@ -207,7 +213,7 @@ func TestDevClient_CardOnFile_Delete(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
userID := "user-test-delete"
|
||||
|
||||
card, err := client.CreateCardOnFile(ctx, userID, "cnon:token-delete")
|
||||
card, err := client.CreateCardOnFile(ctx, userID, "cnon:token-delete", "")
|
||||
require.NoError(t, err)
|
||||
|
||||
err = client.DeleteCardOnFile(ctx, card.ID)
|
||||
@@ -257,7 +263,7 @@ func TestDevClient_CreateCardOnFile_RejectsRawPAN(t *testing.T) {
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
card, err := client.CreateCardOnFile(ctx, "user-raw-"+tt.name, tt.cardNumber)
|
||||
card, err := client.CreateCardOnFile(ctx, "user-raw-"+tt.name, tt.cardNumber, "")
|
||||
require.Error(t, err, "raw PAN must be rejected for production parity")
|
||||
assert.Nil(t, card)
|
||||
assert.Contains(t, err.Error(), "invalid source_id")
|
||||
@@ -535,14 +541,14 @@ func TestDevClient_CreateCardOnFile_WithNewFields(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
userID := "user-new-fields"
|
||||
|
||||
card, err := client.CreateCardOnFile(ctx, userID, "cnon:test-token")
|
||||
card, err := client.CreateCardOnFile(ctx, userID, "cnon:test-token", "")
|
||||
require.NoError(t, err)
|
||||
|
||||
assert.True(t, card.Enabled)
|
||||
assert.NotEmpty(t, card.CardholderName)
|
||||
// Local linkage goes in reference_id, NOT customer_id — the app has no
|
||||
// Square customer provisioning, and a local ID in customer_id would be
|
||||
// rejected by the real Cards API.
|
||||
// Local linkage goes in reference_id; the mock does not store the
|
||||
// customer_id (prod sends it on card creation when the app has provisioned
|
||||
// a Square customer for the user).
|
||||
assert.Equal(t, userID, card.ReferenceID)
|
||||
assert.Empty(t, card.CustomerID)
|
||||
assert.Greater(t, card.Version, int64(0))
|
||||
@@ -554,7 +560,7 @@ func TestDevClient_DeleteCardOnFile_SoftDelete(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
userID := "user-soft-delete"
|
||||
|
||||
card, err := client.CreateCardOnFile(ctx, userID, "cnon:token-soft")
|
||||
card, err := client.CreateCardOnFile(ctx, userID, "cnon:token-soft", "")
|
||||
require.NoError(t, err)
|
||||
|
||||
err = client.DeleteCardOnFile(ctx, card.ID)
|
||||
@@ -703,3 +709,290 @@ func TestDetectCardInfo_Variants(t *testing.T) {
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestDevClient_GetPayment_Found(t *testing.T) {
|
||||
client := NewDevClient().(*MockClient)
|
||||
ctx := context.Background()
|
||||
|
||||
created, err := client.CreatePayment(ctx, CreatePaymentReq{
|
||||
Amount: 5000,
|
||||
Currency: "GBP",
|
||||
SourceID: "cnon:test-card",
|
||||
IdempotencyKey: "payment-for-get",
|
||||
ReferenceID: "booking-get",
|
||||
})
|
||||
require.NoError(t, err)
|
||||
|
||||
got, err := client.GetPayment(ctx, created.ID)
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, created.ID, got.ID)
|
||||
assert.Equal(t, int64(5000), got.Amount)
|
||||
assert.Equal(t, "COMPLETED", got.Status)
|
||||
}
|
||||
|
||||
func TestDevClient_GetPayment_NotFound(t *testing.T) {
|
||||
client := NewDevClient().(*MockClient)
|
||||
ctx := context.Background()
|
||||
|
||||
_, err := client.GetPayment(ctx, "pay_does_not_exist")
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), "not found")
|
||||
}
|
||||
|
||||
func TestDevClient_CreateCustomer_Dedup(t *testing.T) {
|
||||
client := NewDevClient().(*MockClient)
|
||||
ctx := context.Background()
|
||||
|
||||
first, err := client.CreateCustomer(ctx, "Jane Doe", "jane@example.com")
|
||||
require.NoError(t, err)
|
||||
require.NotEmpty(t, first.ID)
|
||||
assert.Equal(t, "jane@example.com", first.Email)
|
||||
assert.NotEmpty(t, first.CreatedAt)
|
||||
assert.True(t, strings.HasPrefix(first.ID, "cus_mock_"))
|
||||
|
||||
// Same email → same deterministic customer (Square dedups on the
|
||||
// email-derived idempotency key; the mock dedups on email).
|
||||
second, err := client.CreateCustomer(ctx, "Jane Doe", "jane@example.com")
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, first.ID, second.ID, "same-email retry must return the original customer")
|
||||
|
||||
other, err := client.CreateCustomer(ctx, "John Doe", "john@example.com")
|
||||
require.NoError(t, err)
|
||||
assert.NotEqual(t, first.ID, other.ID)
|
||||
|
||||
client.mu.RLock()
|
||||
defer client.mu.RUnlock()
|
||||
assert.Len(t, client.customers, 2)
|
||||
}
|
||||
|
||||
func TestDevClient_CreateCustomer_EmptyEmail(t *testing.T) {
|
||||
client := NewDevClient().(*MockClient)
|
||||
ctx := context.Background()
|
||||
|
||||
_, err := client.CreateCustomer(ctx, "Jane Doe", "")
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), "email")
|
||||
}
|
||||
|
||||
func TestDevClient_CancelCheckout_CancelsPending(t *testing.T) {
|
||||
client := NewDevClient().(*MockClient)
|
||||
client.HoldCheckouts = true
|
||||
ctx := context.Background()
|
||||
|
||||
result, err := client.CreateCheckout(ctx, CreateCheckoutReq{
|
||||
Amount: 2500,
|
||||
Currency: "GBP",
|
||||
IdempotencyKey: "cancel-checkout",
|
||||
ReferenceID: "cancel-ref",
|
||||
})
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, "PENDING", result.Status)
|
||||
|
||||
err = client.CancelCheckout(ctx, result.ID)
|
||||
require.NoError(t, err)
|
||||
|
||||
client.mu.RLock()
|
||||
checkout := client.checkouts[result.ID]
|
||||
client.mu.RUnlock()
|
||||
require.NotNil(t, checkout)
|
||||
assert.Equal(t, "CANCELED", checkout.Status)
|
||||
}
|
||||
|
||||
func TestDevClient_CancelCheckout_UnknownIsNoOp(t *testing.T) {
|
||||
client := NewDevClient().(*MockClient)
|
||||
ctx := context.Background()
|
||||
|
||||
err := client.CancelCheckout(ctx, "chk_does_not_exist")
|
||||
require.NoError(t, err)
|
||||
}
|
||||
|
||||
func TestDevClient_CancelCheckout_CompletedIsNoOp(t *testing.T) {
|
||||
// Square documents that disabling an already-completed/cancelled checkout
|
||||
// has no effect, so the mock must return nil and leave the status alone.
|
||||
client := NewDevClient().(*MockClient)
|
||||
ctx := context.Background()
|
||||
|
||||
result, err := client.CreateCheckout(ctx, CreateCheckoutReq{
|
||||
Amount: 2500,
|
||||
Currency: "GBP",
|
||||
IdempotencyKey: "cancel-completed",
|
||||
ReferenceID: "cancel-comp-ref",
|
||||
})
|
||||
require.NoError(t, err)
|
||||
|
||||
assert.Eventually(t, func() bool {
|
||||
_, err := client.GetCheckout(ctx, result.ID)
|
||||
return err == nil
|
||||
}, 5*time.Second, 100*time.Millisecond, "expected checkout to complete")
|
||||
|
||||
err = client.CancelCheckout(ctx, result.ID)
|
||||
require.NoError(t, err)
|
||||
|
||||
client.mu.RLock()
|
||||
checkout := client.checkouts[result.ID]
|
||||
client.mu.RUnlock()
|
||||
require.NotNil(t, checkout)
|
||||
assert.Equal(t, "COMPLETED", checkout.Status, "cancelling an already-completed checkout must be a no-op")
|
||||
}
|
||||
|
||||
func TestDevClient_CreateCustomer_RedactsEmailInLogs(t *testing.T) {
|
||||
client := NewDevClient().(*MockClient)
|
||||
ctx := context.Background()
|
||||
|
||||
var buf bytes.Buffer
|
||||
log.SetOutput(&buf)
|
||||
defer log.SetOutput(os.Stderr)
|
||||
|
||||
email := "pii.marker@example.com"
|
||||
cust, err := client.CreateCustomer(ctx, "PII Marker", email)
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, email, cust.Email, "return value must keep the full email")
|
||||
|
||||
logs := buf.String()
|
||||
if strings.Contains(logs, email) {
|
||||
t.Errorf("full email %q leaked into mock logs: %q", email, logs)
|
||||
}
|
||||
if !strings.Contains(logs, "pi***@example.com") {
|
||||
t.Errorf("expected redacted email 'pi***@example.com' in logs, got %q", logs)
|
||||
}
|
||||
}
|
||||
|
||||
func TestDevClient_CreatePayment_RejectsRawPAN(t *testing.T) {
|
||||
// PCI-DSS parity: CreatePayment accepts only token-like source_ids
|
||||
// (cnon:xxx / ccof:xxx). Raw PANs are rejected exactly like real Square.
|
||||
client := NewDevClient().(*MockClient)
|
||||
ctx := context.Background()
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
pan string
|
||||
}{
|
||||
{"visa", "4111111111111111"},
|
||||
{"mastercard", "5555555555554444"},
|
||||
{"amex", "378282246310005"},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
result, err := client.CreatePayment(ctx, CreatePaymentReq{
|
||||
Amount: 5000,
|
||||
Currency: "GBP",
|
||||
SourceID: tt.pan,
|
||||
IdempotencyKey: "raw-pan-" + tt.name,
|
||||
ReferenceID: "booking-raw",
|
||||
})
|
||||
require.Error(t, err, "raw PAN must be rejected for production parity")
|
||||
assert.Nil(t, result)
|
||||
assert.Contains(t, err.Error(), "invalid source_id")
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestDevClient_RefundPayment_ForcePending(t *testing.T) {
|
||||
// ForceRefundPending exercises the prod-only PENDING refund branch that
|
||||
// is otherwise only reachable against the real Square API.
|
||||
client := NewDevClient().(*MockClient)
|
||||
client.ForceRefundPending = true
|
||||
ctx := context.Background()
|
||||
|
||||
paymentResult, err := client.CreatePayment(ctx, CreatePaymentReq{
|
||||
Amount: 10000,
|
||||
Currency: "GBP",
|
||||
SourceID: "cnon:test-card",
|
||||
IdempotencyKey: "payment-for-pending-refund",
|
||||
ReferenceID: "booking-pending-refund",
|
||||
})
|
||||
require.NoError(t, err)
|
||||
|
||||
refundResult, err := client.RefundPayment(ctx, RefundPaymentReq{
|
||||
PaymentID: paymentResult.ID,
|
||||
Amount: 5000,
|
||||
IdempotencyKey: "pending-refund-key",
|
||||
Reason: "customer request",
|
||||
})
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, "PENDING", refundResult.Status)
|
||||
assert.Equal(t, int64(5000), refundResult.Amount)
|
||||
assert.Equal(t, paymentResult.ID, refundResult.PaymentID)
|
||||
}
|
||||
|
||||
func TestDevClient_RefundPayment_ZeroAmountUnknownPayment(t *testing.T) {
|
||||
// A £0 refund resolves to a full refund only when the payment is known.
|
||||
// Against an unknown payment it must fail (the real DB has a CHECK
|
||||
// amount > 0) rather than silently record a £0 refund.
|
||||
client := NewDevClient().(*MockClient)
|
||||
ctx := context.Background()
|
||||
|
||||
result, err := client.RefundPayment(ctx, RefundPaymentReq{
|
||||
PaymentID: "pay_unknown_zero",
|
||||
Amount: 0,
|
||||
IdempotencyKey: "zero-refund-unknown",
|
||||
})
|
||||
require.Error(t, err)
|
||||
assert.Nil(t, result)
|
||||
assert.Contains(t, err.Error(), "amount must be positive")
|
||||
}
|
||||
|
||||
func TestDevClient_RefundPayment_ZeroAmountFullRefundWhenPaymentExists(t *testing.T) {
|
||||
client := NewDevClient().(*MockClient)
|
||||
ctx := context.Background()
|
||||
|
||||
paymentResult, err := client.CreatePayment(ctx, CreatePaymentReq{
|
||||
Amount: 10000,
|
||||
Currency: "GBP",
|
||||
SourceID: "cnon:test-card",
|
||||
IdempotencyKey: "payment-for-zero-refund",
|
||||
ReferenceID: "booking-zero-refund",
|
||||
})
|
||||
require.NoError(t, err)
|
||||
|
||||
refundResult, err := client.RefundPayment(ctx, RefundPaymentReq{
|
||||
PaymentID: paymentResult.ID,
|
||||
Amount: 0,
|
||||
IdempotencyKey: "zero-refund-known",
|
||||
})
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, int64(10000), refundResult.Amount, "amount 0 = full refund when the payment exists")
|
||||
}
|
||||
|
||||
func TestDevClient_ListPaymentRefunds_ConcurrentReads(t *testing.T) {
|
||||
// Exercises the RLock read path concurrently with writes (Lock) — would
|
||||
// deadlock or panic under -race if ListPaymentRefunds wrongly used a
|
||||
// write lock.
|
||||
client := NewDevClient().(*MockClient)
|
||||
ctx := context.Background()
|
||||
|
||||
paymentResult, err := client.CreatePayment(ctx, CreatePaymentReq{
|
||||
Amount: 10000,
|
||||
Currency: "GBP",
|
||||
SourceID: "cnon:test-card",
|
||||
IdempotencyKey: "payment-for-concurrent-list",
|
||||
ReferenceID: "booking-concurrent-list",
|
||||
})
|
||||
require.NoError(t, err)
|
||||
|
||||
var wg sync.WaitGroup
|
||||
for i := 0; i < 8; i++ {
|
||||
wg.Add(2)
|
||||
go func(idx int) {
|
||||
defer wg.Done()
|
||||
_, err := client.RefundPayment(ctx, RefundPaymentReq{
|
||||
PaymentID: paymentResult.ID,
|
||||
Amount: 100,
|
||||
IdempotencyKey: fmt.Sprintf("refund-concurrent-%d", idx),
|
||||
Reason: "concurrent",
|
||||
})
|
||||
assert.NoError(t, err)
|
||||
}(i)
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
_, err := client.ListPaymentRefunds(ctx, paymentResult.ID, time.Now().Add(-time.Hour))
|
||||
assert.NoError(t, err)
|
||||
}()
|
||||
}
|
||||
wg.Wait()
|
||||
|
||||
results, err := client.ListPaymentRefunds(ctx, paymentResult.ID, time.Now().Add(-time.Hour))
|
||||
require.NoError(t, err)
|
||||
assert.Len(t, results, 8)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user