From dc137e955d11823c1ccd6f5e0866d6816313a52c Mon Sep 17 00:00:00 2001 From: Stefan Date: Fri, 31 Jul 2026 18:34:41 +0100 Subject: [PATCH 01/46] Keep existing payments refundable when new sales are off Switching new sales off (saving the payment provider as "none") used to strand every payment already captured: refunds, replayed callbacks, redirect completion, the cancel page, and operator refunds all resolved the provider through the new-sales gate, so they broke the moment a buyer could no longer start a checkout -- even though the provider's credentials stay stored. Separate the two questions. New sales keep using getActivePaymentProvider / isPaymentsEnabled. Refunds, provider reconciliation, replayed callbacks, and completion of already-started payment work use a new resolver, getPaymentProviderForExistingPayments, which falls back to the last provider the operator activated when new sales are off. That provider is remembered in a new last_active_payment_provider setting, captured when a provider is switched on and again when new sales are switched off (a second "none" save keeps it). The seven accepted safety rules the payment aggregate must satisfy -- including the owner-review, queued owner email, and aggregate-activation behaviour not yet on main -- are recorded as acceptance constraints in docs/payment-aggregate-acceptance.md, not built ahead of their time. Regression tests cover each path: a captured payment refunded while sales are off, an in-flight payment completing and issuing the ticket, a handled failure replaying on a redelivered callback, and an operator refunding an existing paid booking -- all written to fail before the fix and pass after. --- TODO.md | 24 ++ docs/payment-aggregate-acceptance.md | 103 +++++++++ .../equivalent-mutants/shared-m-z.txt | 4 + src/features/admin/require-provider.ts | 13 +- .../api/payment-processing/classify.ts | 7 +- .../api/payment-processing/refunds.ts | 14 +- src/features/api/webhooks.ts | 10 +- src/features/settings-bundles.ts | 1 + src/shared/db/settings.ts | 23 +- src/shared/payments.ts | 54 ++++- src/shared/settings/keys.ts | 1 + src/shared/settings/registry.ts | 8 + .../server/payments/sales-off-safety.test.ts | 211 ++++++++++++++++++ .../webhooks/refund-skip-conditions.test.ts | 12 +- test/shared/db/settings/public-api.test.ts | 54 +++++ test/shared/payments.test.ts | 67 +++++- test/shared/settings/keys.test.ts | 2 +- test/shared/settings/registry.test.ts | 2 + 18 files changed, 573 insertions(+), 37 deletions(-) create mode 100644 docs/payment-aggregate-acceptance.md create mode 100644 test/integration/server/payments/sales-off-safety.test.ts diff --git a/TODO.md b/TODO.md index 8a9da77622..30fba19eae 100644 --- a/TODO.md +++ b/TODO.md @@ -478,6 +478,30 @@ look. fail-closed behaviour. See the "Respect the subrequest budget" guidance in AGENTS.md. +## Payment aggregate — safety behaviour (PR 1) + +*Origin: the first incremental step toward the `base/payment-aggregate` rewrite. +That branch carries the full aggregate (owner-review cases, queued owner email, +aggregate activation); this PR lands only the safety property the rest of the +series depends on, on `main` as it is today.* + +New sales and existing payments are now resolved by different questions: +`getActivePaymentProvider()` / `isPaymentsEnabled()` gate new checkouts; +`getPaymentProviderForExistingPayments()` resolves the provider for refunds, +provider reconciliation, replayed callbacks, and completion of already-started +payment work. When new sales are switched off (provider saved as "none"), the +existing-payment paths fall back to the last provider the operator activated — +whose credentials stay stored — so money already captured is never stranded. + +The seven accepted safety rules the aggregate must satisfy — including the ones +not yet implementable on `main` (owner review, queued owner email, aggregate +activation) — are recorded as acceptance constraints in +[`docs/payment-aggregate-acceptance.md`](docs/payment-aggregate-acceptance.md), +not implemented ahead of their time. Later-stage findings from the aggregate +base branch (its own payment-rewrite coverage gaps and the eight Codex findings +recorded in its TODO) belong to when the aggregate itself lands, and are not +copied here. + ## Request performance: consolidate AsyncLocalStorage scopes `src/features/app/request.ts` enters eleven nested request scopes for locale, client diff --git a/docs/payment-aggregate-acceptance.md b/docs/payment-aggregate-acceptance.md new file mode 100644 index 0000000000..a907f3dbe5 --- /dev/null +++ b/docs/payment-aggregate-acceptance.md @@ -0,0 +1,103 @@ +# Payment aggregate — accepted safety rules + +These seven rules are the safety contract the payment aggregate must satisfy. +They describe behaviour that does not all exist on `main` yet: the owner-review +flow, the queued owner email, and "aggregate activation" all belong to the +future aggregate work. They are recorded here as acceptance constraints so the +work that lands first — starting with "keep existing payments refundable when +new sales are off" — does not paint the aggregate into a corner. + +Each rule names the current behaviour on `main` where one exists, so a reader +can see what already holds and what is still future work. Nothing here is +implemented ahead of its time; this document is the contract, not scaffolding. + +## The separation this PR establishes + +New sales and existing payments are now resolved by different questions: + +- **New sales** — `getActivePaymentProvider()` / `isPaymentsEnabled()`. Returns + null when the operator has saved the provider as "none", so no buyer can + start a new checkout. +- **Existing payments** — `getPaymentProviderForExistingPayments()`. Refunds, + provider reconciliation, replayed callbacks, and completion of already-started + payment work use this. When new sales are off, it falls back to the last + provider the operator activated (whose credentials stay stored), so money + already captured is never stranded. + +The rules below assume that separation holds. + +## 1. Failed checkout plus captured money becomes owner review with complete-or-refund choices + +A captured charge whose booking cannot be honoured at the charged amount does +not disappear and is not silently refunded. It is surfaced for the operator, +who must be able to either complete it (issue the ticket) or refund it. + +*On `main` today:* a signed captured payment that cannot be honoured is kept as +a quantity-0 placeholder and refunded automatically (`storeRefundedBooking` / +`refundAndFail`), or acknowledged as "already handled" when its booking is gone. +There is no owner-review choice yet — the refund is automatic. The +"complete-or-refund" choice is future aggregate work; the current path must +keep refunding safely (covered by this PR's regression tests) so the aggregate +can later replace the automatic refund with a review. + +## 2. Completed refunds count immediately while overlapping refunds are blocked until provider totals catch up + +A refund that succeeds is recorded at once. A second refund attempt for the same +charge must not pay out twice: it is treated as already-done once the provider's +own refund total confirms it. + +*On `main` today:* `tryRefund` treats a payment the provider reports as already +refunded (`isPaymentRefunded`) as success, and each charge carries a +`provider_refunded_at` marker so a later attempt skips the provider call for +charges already returned. This is the current rule and must stay. + +## 3. Multiple captured charges require owner review + +When a buyer has more than one captured charge for the same booking (a deposit +plus a balance, or charges combined by a merge), the system must not pick a +resolution by default. It surfaces the set for the operator to decide. + +*On `main` today:* a merged attendee can carry several references, and the +bulk/single refund paths handle them as a batch without a review step. The +"require owner review" choice is future aggregate work. + +## 4. Queued owner email uses the current business address but stored body/buyer facts + +When an owner notification is queued and sent later, the recipient address is +read at send time (so it reaches the current operator), but the message body +and the buyer facts it describes come from when the case was raised (so it +describes what actually happened, not a later state). + +*On `main` today:* there is no queued owner-email path; notifications are sent +inline. This is future aggregate work. + +## 5. Malformed legacy records migrate without invented facts into owner review + +A legacy or malformed payment record that cannot be interpreted must not have +facts invented for it. It is carried forward only as far as it can be honestly +read and lands in owner review for a person to resolve. + +*On `main` today:* legacy migration copies what is provably there (references, +statuses) and invents nothing; an unreadable record is left for the operator +rather than guessed. This must stay true as the aggregate's migration lands. + +## 6. A buyer with a paid booking under review sees "payment received / do not pay again" and gets a stable reload + +A buyer whose payment was captured but whose booking is not yet resolved must be +told their payment was received and must not be offered a way to pay again. A +reload of the page must return the same state, not re-charge or re-process. + +*On `main` today:* a paid session the ledger already records replays as +success ("payment received") and is never re-processed or re-refunded +(`replaySessionFromLedger`). The explicit "under review" state and its stable +messaging are future aggregate work; the replay safety it depends on is +current and must stay. + +## 7. Aggregate activation waits for complete owner case pages/actions + +The aggregate is only switched on once every owner-case page and action it +depends on exists and works. No half-enabled state where some payments flow +through the aggregate and some do not. + +*On `main` today:* there is no aggregate to activate. This is the gate the +future work must clear before it takes over any payment path. diff --git a/scripts/mutation/equivalent-mutants/shared-m-z.txt b/scripts/mutation/equivalent-mutants/shared-m-z.txt index 21bdbe3632..21fcd78522 100644 --- a/scripts/mutation/equivalent-mutants/shared-m-z.txt +++ b/scripts/mutation/equivalent-mutants/shared-m-z.txt @@ -9,6 +9,10 @@ src/shared/rest/crud-api.ts:367:31 ?? → || # hydrated map values are record src/shared/rest/crud-api.ts:577:27 ?? → || # extraRoutes is a route object or undefined src/shared/rest/resource.ts:158:25 ?? → || # validationError is an ErrorResult object or null; an object is truthy +# paymentsApi.getConfiguredProvider() returns PaymentProviderType | null (a +# validated settings getter, never a falsy non-null string), so ?? and || agree. +src/shared/payments.ts:379:44 ?? → || # getConfiguredProvider() is a provider type or null; never falsy-but-non-null + # request-scoped ambient fallback: scope.current() is T|undefined where T is # always a truthy container object (never a falsy-non-null value), so # `?? fallback` and `|| fallback` always agree. (createScopedValue's ?? is NOT diff --git a/src/features/admin/require-provider.ts b/src/features/admin/require-provider.ts index fb82dfad9a..04d84536b5 100644 --- a/src/features/admin/require-provider.ts +++ b/src/features/admin/require-provider.ts @@ -1,14 +1,17 @@ import { - getActivePaymentProvider, + getPaymentProviderForExistingPayments, type PaymentProvider, } from "#shared/payments.ts"; -/** The active payment provider, or the caller's `onMissing` fallback (a redirect - * or error Response) when none is configured. The refund and refresh POSTs share - * this "need a provider before we touch money" guard instead of each re-checking. */ +/** The provider for refunding or refreshing an existing payment, or the + * caller's `onMissing` fallback (a redirect or error Response) when none was + * ever configured. Uses {@link getPaymentProviderForExistingPayments} so an + * operator can still refund and reconcile after switching new sales off. The + * refund and refresh POSTs share this "need a provider before we touch money" + * guard instead of each re-checking. */ export const requirePaymentProvider = async ( onMissing: () => T, ): Promise => { - const provider = await getActivePaymentProvider(); + const provider = await getPaymentProviderForExistingPayments(); return provider ?? onMissing(); }; diff --git a/src/features/api/payment-processing/classify.ts b/src/features/api/payment-processing/classify.ts index 3a3ce0988a..dfb29e53e6 100644 --- a/src/features/api/payment-processing/classify.ts +++ b/src/features/api/payment-processing/classify.ts @@ -15,7 +15,7 @@ import type { BookingIntent } from "#shared/booking-intent.ts"; import { ErrorCode, logError } from "#shared/logger.ts"; import { verifyPrice } from "#shared/payment-signature.ts"; import { - getActivePaymentProvider, + getPaymentProviderForExistingPayments, type ValidatedPaymentSession, } from "#shared/payments.ts"; @@ -119,7 +119,10 @@ export const classifySessionIntent = async ( export const validatePaidSession = async ( sessionId: string, ): Promise => { - const provider = await getActivePaymentProvider(); + // An in-flight checkout may complete after the operator switched new sales + // off, so resolve the provider that captured the payment rather than the + // new-sales gate. + const provider = await getPaymentProviderForExistingPayments(); if (!provider) { logRedirectError(`No payment provider configured (session=${sessionId})`); return { diff --git a/src/features/api/payment-processing/refunds.ts b/src/features/api/payment-processing/refunds.ts index 850b96b1d7..6ec6b9f51d 100644 --- a/src/features/api/payment-processing/refunds.ts +++ b/src/features/api/payment-processing/refunds.ts @@ -21,7 +21,7 @@ import { } from "#shared/logger.ts"; import { sendNtfyError } from "#shared/ntfy.ts"; import { - getActivePaymentProvider, + getPaymentProviderForExistingPayments, type ValidatedPaymentSession, } from "#shared/payments.ts"; import { addPendingWork } from "#shared/pending-work.ts"; @@ -31,16 +31,20 @@ const PRICE_CHANGED_MESSAGE = "The price for this listing changed while you were completing payment."; /** - * Resolve the active payment provider. When none is configured, log a - * structured error with the caller's code/detail and return null, so each + * Resolve the provider for refunding or reconciling an existing payment. Falls + * back to the last activated provider when new sales are off, so refunds keep + * working after the operator switches new sales off. When none is configured, + * log a structured error with the caller's code/detail and return null, so each * caller can pick its own fallback (a false, a 400, ...). */ export const getPaymentProviderOrLog = async ( code: ErrorCodeType, detail: string, listingId?: number, -): Promise>> => { - const provider = await getActivePaymentProvider(); +): Promise< + Awaited> +> => { + const provider = await getPaymentProviderForExistingPayments(); if (!provider) logError({ code, detail, listingId }); return provider; }; diff --git a/src/features/api/webhooks.ts b/src/features/api/webhooks.ts index 07c0e24e6a..72b5c4497a 100644 --- a/src/features/api/webhooks.ts +++ b/src/features/api/webhooks.ts @@ -50,7 +50,7 @@ import { ErrorCode, logDebug, logError } from "#shared/logger.ts"; import { WEBHOOK_SIGNATURE_HEADERS } from "#shared/payment-providers.ts"; import { getPaymentWebhookUrl } from "#shared/payment-webhook-url.ts"; import { - getActivePaymentProvider, + getPaymentProviderForExistingPayments, type ValidatedPaymentSession, type WebhookEvent, } from "#shared/payments.ts"; @@ -237,7 +237,9 @@ const handlePaymentSuccess = (request: Request): Promise => { const logCancelError = paymentSessionErrorLogger("cancel"); const handlePaymentCancel = withSessionId(async (sid) => { - const provider = await getActivePaymentProvider(); + // A buyer who cancels may do so after the operator switched new sales off, so + // resolve the provider that captured the payment rather than the new-sales gate. + const provider = await getPaymentProviderForExistingPayments(); if (!provider) { logCancelError(`No provider configured (session=${sid})`); return paymentErrorResponse("Payment provider not configured"); @@ -320,7 +322,7 @@ const authenticateWebhook = async ( | Response | { provider: NonNullable< - Awaited> + Awaited> >; listing: WebhookEvent; } @@ -376,7 +378,7 @@ const authenticateWebhook = async ( const handlePaymentWebhook = async (request: Request): Promise => { // Read raw body bytes FIRST, before any async work. The Bunny Edge runtime // can garbage-collect the underlying request body resource during awaits - // (e.g. dynamic imports in getActivePaymentProvider), causing "BadResource: + // (e.g. dynamic imports in getPaymentProviderForExistingPayments), causing "BadResource: // Cannot read body as underlying resource unavailable" errors. const payloadBytes = new Uint8Array(await request.arrayBuffer()); const payload = new TextDecoder().decode(payloadBytes); diff --git a/src/features/settings-bundles.ts b/src/features/settings-bundles.ts index 9bec39014e..1458bfd2be 100644 --- a/src/features/settings-bundles.ts +++ b/src/features/settings-bundles.ts @@ -74,6 +74,7 @@ const PUBLIC_NAV_SETTINGS: readonly string[] = [ */ const PAYMENT_SETTINGS: readonly string[] = [ CONFIG_KEYS.PAYMENT_PROVIDER, + CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER, CONFIG_KEYS.COUNTRY, CONFIG_KEYS.BOOKING_FEE, CONFIG_KEYS.STRIPE_SECRET_KEY, diff --git a/src/shared/db/settings.ts b/src/shared/db/settings.ts index 6b268fb2fb..faa1dba0cf 100644 --- a/src/shared/db/settings.ts +++ b/src/shared/db/settings.ts @@ -101,7 +101,7 @@ import type { SuperuserChoice, Theme, } from "#shared/types.ts"; -import { isSuperuserChoice } from "#shared/types.ts"; +import { isPaymentProvider, isSuperuserChoice } from "#shared/types.ts"; import { appleWallet } from "#shared/wallets/apple-wallet-settings.ts"; import { googleWallet } from "#shared/wallets/google-wallet-settings.ts"; import type { EmailContent } from "#templates/email/shared.ts"; @@ -243,6 +243,14 @@ const settingsBase = { // --- Google Wallet --- googleWallet: googleWallet.createReadSettings(snap as (k: string) => string), invalidateCache, + /** The provider that captured payments before new sales were switched off. + * Used to refund, reconcile, replay, and complete payments that already + * exist while new sales are disabled; null when no provider was ever + * activated. */ + get lastActivePaymentProvider(): PaymentProviderType | null { + const value = snap("last_active_payment_provider"); + return isPaymentProvider(value) ? value : null; + }, get listingColumnLayout(): TableLayout { return configurableTableLayouts.listing.parse(snap("listing_column_order")); }, @@ -444,8 +452,21 @@ const settingsBase = { await writeRaw(CONFIG_KEYS.PAYMENT_PROVIDER, v); data.payment_provider = v; data.payment_provider_setting = v; + // Remember the activated provider so existing payments stay serviceable + // after new sales are later switched off. + await writeRaw(CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER, v); + data.last_active_payment_provider = v; }, setPaymentProviderNone: async (): Promise => { + // Remember the provider being switched off (if any) before clearing it, + // so refunds, replayed callbacks, and in-flight completions of payments + // that already exist keep resolving the provider that captured them. A + // second "none" save leaves the remembered provider in place. + const switchedOff = data.payment_provider; + if (switchedOff) { + await writeRaw(CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER, switchedOff); + data.last_active_payment_provider = switchedOff; + } await writeRaw(CONFIG_KEYS.PAYMENT_PROVIDER, "none"); data.payment_provider = null; data.payment_provider_setting = "none"; diff --git a/src/shared/payments.ts b/src/shared/payments.ts index c8beb1b1cb..5d997f4809 100644 --- a/src/shared/payments.ts +++ b/src/shared/payments.ts @@ -20,6 +20,11 @@ import type { ContactInfo, PaymentProviderType } from "#shared/types.ts"; export const paymentsApi = { getConfiguredProvider: (): PaymentProviderType | null => settings.paymentProvider, + /** The provider that captured payments before new sales were switched off; + * used to keep existing payments refundable and completable while new + * sales are disabled. */ + getLastConfiguredProvider: (): PaymentProviderType | null => + settings.lastActivePaymentProvider, }; /** Re-export from types.ts (canonical definition) */ @@ -335,14 +340,43 @@ const providerLoaders: Record< (await import("#shared/sumup-provider.ts")).sumupPaymentProvider, }; -export const getActivePaymentProvider = - async (): Promise => { - const providerType = paymentsApi.getConfiguredProvider(); - if (!providerType) { - logDebug("Payment", "No payment provider configured in settings"); - return null; - } +/** Resolve a provider type, log it, and load it. `label` carries which question + * is being answered (" for new sales" / " for existing payments") so the log + * names the path; `onMissing` lets a caller log a specific message when no + * provider resolves. The single load path both resolvers share. */ +const resolveProvider = async ( + resolveType: () => PaymentProviderType | null, + label: string, + onMissing?: () => void, +): Promise => { + const providerType = resolveType(); + if (!providerType) { + onMissing?.(); + return null; + } + logDebug("Payment", `Resolving payment provider${label}: ${providerType}`); + return providerLoaders[providerType](); +}; - logDebug("Payment", `Resolving payment provider: ${providerType}`); - return await providerLoaders[providerType](); - }; +export const getActivePaymentProvider = (): Promise => + resolveProvider(paymentsApi.getConfiguredProvider, "", () => + logDebug("Payment", "No payment provider configured in settings"), + ); + +/** + * Resolve the provider for work on payments that already exist — refunds, + * provider reconciliation, replayed callbacks, and completion of an + * already-started payment. New sales use {@link getActivePaymentProvider}; + * this falls back to the last provider the operator activated when new sales + * are off ("none"), whose credentials stay stored, so disabling new sales + * never strands money already captured. Returns null only when no provider was + * ever configured. + */ +export const getPaymentProviderForExistingPayments = + (): Promise => + resolveProvider( + () => + paymentsApi.getConfiguredProvider() ?? + paymentsApi.getLastConfiguredProvider(), + " for existing payments", + ); diff --git a/src/shared/settings/keys.ts b/src/shared/settings/keys.ts index d7a1d52ed4..6605c9a9c0 100644 --- a/src/shared/settings/keys.ts +++ b/src/shared/settings/keys.ts @@ -38,6 +38,7 @@ export const CONFIG_KEY_NAMES = [ "GOOGLE_WALLET_SERVICE_ACCOUNT_KEY", "HEADER_IMAGE_URL", "HOMEPAGE_TEXT", + "LAST_ACTIVE_PAYMENT_PROVIDER", "LATEST_SCRIPT_VERSION", "LATEST_SCRIPT_VERSION_NAME", "LISTING_COLUMN_ORDER", diff --git a/src/shared/settings/registry.ts b/src/shared/settings/registry.ts index 2bbc1687fc..cffd970582 100644 --- a/src/shared/settings/registry.ts +++ b/src/shared/settings/registry.ts @@ -100,6 +100,14 @@ export const STRING_SETTING_DEFINITIONS = [ key: CONFIG_KEYS.LATEST_SCRIPT_VERSION_NAME, storage: "plaintext", }), + // The provider that was active before the operator switched new sales off + // ("none"). Its credentials stay stored, so refunds, replayed callbacks, and + // in-flight completions of payments that already exist keep running against + // the provider that captured them rather than stranding the money. + setting({ + key: CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER, + storage: "plaintext", + }), setting({ key: CONFIG_KEYS.SUPERUSER_CHOICE, storage: "plaintext" }), setting({ accessor: { name: "supportFormLastSubmitted", readOnly: true }, diff --git a/test/integration/server/payments/sales-off-safety.test.ts b/test/integration/server/payments/sales-off-safety.test.ts new file mode 100644 index 0000000000..e1cb5b6beb --- /dev/null +++ b/test/integration/server/payments/sales-off-safety.test.ts @@ -0,0 +1,211 @@ +// jscpd:ignore-start +import { expect } from "@std/expect"; +import { describe, it as test } from "@std/testing/bdd"; +import { stub } from "@std/testing/mock"; +import { handleRequest } from "#routes"; +import { settings } from "#shared/db/settings.ts"; +import { + expectFlashRedirect, + expectHtmlResponse, +} from "#test-utils/assertions.ts"; +import { describeWithEnv } from "#test-utils/db.ts"; +import { + createTestListing, + deactivateTestListing, +} from "#test-utils/db-helpers/listings.ts"; +import { signedMeta, singleItem } from "#test-utils/factories.ts"; +import { + mockRequest, + mockWebhookRequest, + withMocks, +} from "#test-utils/mocks.ts"; +import { setupStripe } from "#test-utils/settings.ts"; +import { + checkoutSessionEvent, + expectAttendeeCreatedWithPiiBlob, + expectWebhookKeptAndRefunded, + stubRefundPayment, + stubRetrieveCheckoutSession, +} from "#test-utils/webhooks.ts"; + +// jscpd:ignore-end + +/** + * Turning new sales off (saving the payment provider as "none") must only stop + * buyers starting NEW payments. Payments that already exist — a captured + * charge that still has to be refunded, replayed provider callbacks, an + * in-flight checkout that completes after the switch, and operator refunds — + * keep running against the provider that captured them. Its credentials stay + * stored, so the last provider the operator activated is reused. + */ +describeWithEnv( + "server (payments) > existing payments stay live when new sales are off", + { db: true, triggers: true }, + () => { + /** Configure Stripe, then switch new sales off. Stripe's credentials and + * webhook secret remain stored, so existing-payment work can still resolve + * the Stripe provider that captured the money. */ + const salesOffAfterStripe = async (): Promise => { + await setupStripe(); + await settings.update.setPaymentProviderNone(); + }; + + describe("replayed callbacks and completion", () => { + test("a captured payment that needs refunding is still refunded", async () => { + await salesOffAfterStripe(); + const listing = await createTestListing({ + maxAttendees: 50, + unitPrice: 1000, + }); + await deactivateTestListing(listing.id); + + const { mockRefund } = await expectWebhookKeptAndRefunded( + checkoutSessionEvent({ + amountTotal: 1000, + eventId: "evt_off_refund", + metadata: signedMeta( + { + email: "refund-off@example.com", + items: singleItem(listing.id, 1, 1000), + name: "Refund Off", + }, + 1000, + ), + paymentIntent: "pi_off_refund", + sessionId: "cs_off_refund", + }), + "re_off_refund", + "no longer accepting registrations", + ); + expect(mockRefund.calls.length).toBe(1); + }); + + test("an in-flight payment completes and issues the ticket", async () => { + await salesOffAfterStripe(); + const listing = await createTestListing({ + maxAttendees: 50, + unitPrice: 1000, + }); + + const { stripePaymentProvider } = await import( + "#shared/stripe-provider.ts" + ); + const mockVerify = stub( + stripePaymentProvider, + "verifyWebhookSignature", + () => + Promise.resolve({ + listing: checkoutSessionEvent({ + amountTotal: 1000, + eventId: "evt_off_complete", + metadata: signedMeta( + { + email: "complete-off@example.com", + items: singleItem(listing.id, 1, 1000), + name: "Complete Off", + }, + 1000, + ), + paymentIntent: "pi_off_complete", + sessionId: "cs_off_complete", + }), + valid: true as const, + }), + ); + try { + const response = await handleRequest( + mockWebhookRequest({}, { "stripe-signature": "sig_valid" }), + ); + expect(response.status).toBe(200); + const json = (await response.json()) as Record; + expect(json.processed).toBe(true); + await expectAttendeeCreatedWithPiiBlob(listing.id); + } finally { + mockVerify.restore(); + } + }); + + test("a handled failure replays the same outcome on a redelivered callback", async () => { + await setupStripe(); + const listing = await createTestListing({ + maxAttendees: 50, + unitPrice: 1000, + }); + await deactivateTestListing(listing.id); + + await withMocks( + () => ({ + mockRefund: stubRefundPayment(), + mockRetrieve: stubRetrieveCheckoutSession({ + amountTotal: 1000, + email: "replay-off@example.com", + items: singleItem(listing.id, 1, 1000), + name: "Replay Off", + paymentIntent: "pi_off_replay", + sessionId: "cs_off_replay", + }), + }), + async ({ mockRefund }) => { + const first = await handleRequest( + mockRequest("/payment/success?session_id=cs_off_replay"), + ); + await expectHtmlResponse( + first, + 410, + "no longer accepting registrations", + "refunded", + ); + expect(mockRefund.calls.length).toBe(1); + + // New sales switched off AFTER the first delivery captured the + // handled outcome. The redelivery must replay it, not 400 on a + // missing provider. + await settings.update.setPaymentProviderNone(); + + const second = await handleRequest( + mockRequest("/payment/success?session_id=cs_off_replay"), + ); + const html = await expectHtmlResponse( + second, + 410, + "no longer accepting registrations", + "refunded", + ); + expect(html).not.toContain("being processed"); + expect(html).not.toContain("not configured"); + expect(mockRefund.calls.length).toBe(1); + }, + ); + }); + }); + + describe("refunds and provider outages", () => { + test("an operator can refund an existing paid booking", async () => { + await setupStripe(); + const { setupRefundTest } = await import( + "#test/features/admin/refunds-helpers.ts" + ); + const { submitRefund } = await import("#test-utils/refund-routes.ts"); + const ctx = await setupRefundTest("pi_off_admin"); + // New sales off, but the Stripe provider (and its key) remain stored. + await settings.update.setPaymentProviderNone(); + const { stripePaymentProvider } = await import( + "#shared/stripe-provider.ts" + ); + const mockRefund = stub(stripePaymentProvider, "refundPayment", () => + Promise.resolve(true), + ); + try { + const response = await submitRefund(ctx); + await expectFlashRedirect( + `/admin/attendees/${ctx.attendee.id}/actions`, + "Refund issued", + )(response); + expect(mockRefund.calls.length).toBe(1); + } finally { + mockRefund.restore(); + } + }); + }); + }, +); diff --git a/test/integration/server/webhooks/refund-skip-conditions.test.ts b/test/integration/server/webhooks/refund-skip-conditions.test.ts index 85ab5de931..1f18811ed8 100644 --- a/test/integration/server/webhooks/refund-skip-conditions.test.ts +++ b/test/integration/server/webhooks/refund-skip-conditions.test.ts @@ -72,8 +72,10 @@ describeWithEnv( await deactivateTestListing(listing.id); // Mock paymentsApi.getConfiguredProvider to return "stripe" on first call - // (for webhook handler's initial check) then null on second call (for tryRefund). - // This covers lines 135-141 where tryRefund has a payment reference but no provider. + // (for webhook handler's initial check) then null on second call (for tryRefund), + // and stub the last-activated fallback null too, so tryRefund's resolver sees no + // provider. This covers the branch where tryRefund has a payment reference but + // no provider can be resolved, so it logs and leaves the refund retryable. const { paymentsApi } = await import("#shared/payments.ts"); const origGetConfigured = paymentsApi.getConfiguredProvider; let callCount = 0; @@ -86,6 +88,11 @@ describeWithEnv( return callCount <= 1 ? origGetConfigured() : null; }, ); + const mockGetLast = stub( + paymentsApi, + "getLastConfiguredProvider", + () => null, + ); const mockVerify = await stubWebhookVerify( checkoutSessionEvent({ @@ -116,6 +123,7 @@ describeWithEnv( } finally { mockVerify.restore(); mockGetConfigured.restore(); + mockGetLast.restore(); } }); diff --git a/test/shared/db/settings/public-api.test.ts b/test/shared/db/settings/public-api.test.ts index 88d8c63d9e..e53c31b9fc 100644 --- a/test/shared/db/settings/public-api.test.ts +++ b/test/shared/db/settings/public-api.test.ts @@ -105,6 +105,60 @@ describeWithEnv("db > settings public API", { db: true }, () => { expect(settings.paymentProvider).toBeNull(); expect(settings.paymentProviderSetting).toBe("none"); }); + + test("remembers the most recently activated provider", async () => { + // Activating twice proves the remembered provider is replaced, not + // appended (a fresh snapshot read catches a `+=` drift), and a reload + // from the DB catches a missing persistence write. + await settings.update.paymentProvider("stripe"); + await settings.update.paymentProvider("square"); + expect(settings.lastActivePaymentProvider).toBe("square"); + + settings.invalidateCache(); + await settings.loadKeys([CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER]); + expect(settings.lastActivePaymentProvider).toBe("square"); + }); + + test("keeps the last provider after new sales are switched off", async () => { + await settings.update.paymentProvider("stripe"); + await settings.update.setPaymentProviderNone(); + expect(settings.paymentProvider).toBeNull(); + expect(settings.lastActivePaymentProvider).toBe("stripe"); + + settings.invalidateCache(); + await settings.loadKeys([CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER]); + expect(settings.lastActivePaymentProvider).toBe("stripe"); + }); + + test("a second none save keeps the remembered provider", async () => { + // Saving "none" again must not clear the provider remembered from the + // first switch-off — existing payments still need it. + await settings.update.paymentProvider("stripe"); + await settings.update.setPaymentProviderNone(); + await settings.update.setPaymentProviderNone(); + expect(settings.paymentProvider).toBeNull(); + expect(settings.lastActivePaymentProvider).toBe("stripe"); + }); + + test("backfills the remembered provider for a pre-existing database", async () => { + // A database that configured Stripe before last-active was tracked has + // payment_provider set but last_active_payment_provider empty. Switching + // new sales off must still remember Stripe so its existing payments stay + // refundable. + await settings.setRaw(CONFIG_KEYS.PAYMENT_PROVIDER, "stripe"); + await settings.setRaw(CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER, ""); + settings.invalidateCache(); + await settings.loadKeys([ + CONFIG_KEYS.PAYMENT_PROVIDER, + CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER, + ]); + expect(settings.lastActivePaymentProvider).toBeNull(); + + await settings.update.setPaymentProviderNone(); + settings.invalidateCache(); + await settings.loadKeys([CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER]); + expect(settings.lastActivePaymentProvider).toBe("stripe"); + }); }); describe("Stripe activation", () => { diff --git a/test/shared/payments.test.ts b/test/shared/payments.test.ts index 584cd01b5f..cdd2ebd150 100644 --- a/test/shared/payments.test.ts +++ b/test/shared/payments.test.ts @@ -1,28 +1,38 @@ import { expect } from "@std/expect"; import { it as test } from "@std/testing/bdd"; +import type { Spy } from "@std/testing/mock"; import { ALL_SETTINGS_KEYS, settings } from "#shared/db/settings.ts"; -import { getActivePaymentProvider } from "#shared/payments.ts"; +import { + getActivePaymentProvider, + getPaymentProviderForExistingPayments, +} from "#shared/payments.ts"; import { describeWithEnv } from "#test-utils/db.ts"; import { useDebugLogSpy } from "#test-utils/debug-log.ts"; +/** True if any captured debug log line includes `needle`. The spy is + * per-describe (each describe owns its own `useDebugLogSpy` hooks). */ +const debugLogged = (spy: () => Spy, needle: string): boolean => + spy().calls.some((call) => String(call.args[0]).includes(needle)); + describeWithEnv("getActivePaymentProvider", { db: true }, () => { const debugSpy = useDebugLogSpy(); - const debugLogged = (needle: string): boolean => - debugSpy().calls.some((call) => String(call.args[0]).includes(needle)); test("returns null when no provider is configured", async () => { expect(await getActivePaymentProvider()).toBeNull(); expect( - debugLogged("[Payment] No payment provider configured in settings"), + debugLogged( + debugSpy, + "[Payment] No payment provider configured in settings", + ), ).toBe(true); }); test("logs the provider it resolves under the Payment category", async () => { await settings.update.paymentProvider("stripe"); await getActivePaymentProvider(); - expect(debugLogged("[Payment] Resolving payment provider: stripe")).toBe( - true, - ); + expect( + debugLogged(debugSpy, "[Payment] Resolving payment provider: stripe"), + ).toBe(true); }); test("returns null for a provider type the module doesn't recognise", async () => { @@ -51,3 +61,46 @@ describeWithEnv("getActivePaymentProvider", { db: true }, () => { expect(provider?.type).toBe("sumup"); }); }); + +describeWithEnv("getPaymentProviderForExistingPayments", { db: true }, () => { + const debugSpy = useDebugLogSpy(); + + test("returns null when no provider was ever configured", async () => { + expect(await getPaymentProviderForExistingPayments()).toBeNull(); + }); + + test("returns the active provider when new sales are on", async () => { + await settings.update.paymentProvider("stripe"); + expect((await getPaymentProviderForExistingPayments())?.type).toBe( + "stripe", + ); + expect( + debugLogged( + debugSpy, + "Resolving payment provider for existing payments: stripe", + ), + ).toBe(true); + }); + + test("falls back to the last activated provider when new sales are off", async () => { + await settings.update.paymentProvider("stripe"); + await settings.update.setPaymentProviderNone(); + // New sales are off, but the Stripe provider still resolves so existing + // payments can be refunded, reconciled, and completed. + expect(settings.paymentProvider).toBeNull(); + expect((await getPaymentProviderForExistingPayments())?.type).toBe( + "stripe", + ); + expect( + debugLogged( + debugSpy, + "Resolving payment provider for existing payments: stripe", + ), + ).toBe(true); + }); + + test("returns null when sales are off and no provider was ever activated", async () => { + await settings.update.setPaymentProviderNone(); + expect(await getPaymentProviderForExistingPayments()).toBeNull(); + }); +}); diff --git a/test/shared/settings/keys.test.ts b/test/shared/settings/keys.test.ts index b9f078e520..efc3b99ff7 100644 --- a/test/shared/settings/keys.test.ts +++ b/test/shared/settings/keys.test.ts @@ -12,7 +12,7 @@ describe("settings keys", () => { test("keeps the complete public key catalog exact", async () => { expect(await jsonHash(CONFIG_KEY_NAMES)).toBe( - "b5747ab73abc3050541eb99eb6b73528089d47d69d5801d76bce5fb1e32dc0f1", + "1563a7a03136abc9750ef3511a3231fc152fa12f9853a5fbbf2fca8f3a7d68e8", ); }); diff --git a/test/shared/settings/registry.test.ts b/test/shared/settings/registry.test.ts index 8bbc2a2c94..d17ae8fe79 100644 --- a/test/shared/settings/registry.test.ts +++ b/test/shared/settings/registry.test.ts @@ -64,6 +64,7 @@ const EXPECTED_CONFIG_KEY_NAMES = lines(` GOOGLE_WALLET_SERVICE_ACCOUNT_KEY HEADER_IMAGE_URL HOMEPAGE_TEXT + LAST_ACTIVE_PAYMENT_PROVIDER LATEST_SCRIPT_VERSION LATEST_SCRIPT_VERSION_NAME LISTING_COLUMN_ORDER @@ -123,6 +124,7 @@ const EXPECTED_SETTING_ROWS = [ ["SUMUP_MERCHANT_CODE", "plaintext", null], ["LATEST_SCRIPT_VERSION", "plaintext", "latestScriptVersion"], ["LATEST_SCRIPT_VERSION_NAME", "plaintext", "latestScriptVersionName"], + ["LAST_ACTIVE_PAYMENT_PROVIDER", "plaintext", null], ["SUPERUSER_CHOICE", "plaintext", null], [ "SUPPORT_FORM_LAST_SUBMITTED", From bc4a6dc84bfddf37c5499631868c09a5305d1b45 Mon Sep 17 00:00:00 2001 From: Stefan Date: Sat, 1 Aug 2026 04:10:35 +0100 Subject: [PATCH 02/46] Address review: shared provider type, atomic provider transition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit (CHANGES_REQUESTED): export a named ExistingPaymentProvider type from #shared/payments.ts and use it in place of the duplicated Awaited> expression in getPaymentProviderOrLog and authenticateWebhook. Codex P2: persist PAYMENT_PROVIDER and LAST_ACTIVE_PAYMENT_PROVIDER in one writeRawBatch transaction (mirroring the existing Stripe activation path), so a failure between the two writes cannot leave new sales enabled against a stale remembered provider. The snapshot mirrors the committed values only after the batch succeeds. A second 'none' save keeps the remembered provider. Codex P1 (per-charge provider) and P1 (pre-existing none sites): recorded as known gaps in docs/payment-aggregate-acceptance.md and TODO.md rather than implemented here — per-charge provider tracking is the reference a charge lives in and is future aggregate work; a site already on 'none' with no recorded last-active provider is not regressed by this PR and recovers via a one-click provider re-select. --- TODO.md | 15 ++++++++ docs/payment-aggregate-acceptance.md | 20 +++++++++++ .../api/payment-processing/refunds.ts | 5 ++- src/features/api/webhooks.ts | 5 ++- src/shared/db/settings.ts | 35 +++++++++++-------- src/shared/payments.ts | 10 +++++- 6 files changed, 69 insertions(+), 21 deletions(-) diff --git a/TODO.md b/TODO.md index 30fba19eae..f1f5952271 100644 --- a/TODO.md +++ b/TODO.md @@ -502,6 +502,21 @@ base branch (its own payment-rewrite coverage gaps and the eight Codex findings recorded in its TODO) belong to when the aggregate itself lands, and are not copied here. +- **Track the provider each charge was captured with.** `main` stores only the + opaque `payment_reference` per processed payment, not which provider captured + it. So after an operator switches providers (Stripe → Square) and then selects + "none", the existing-payment resolver's last-active fallback resolves every + payment through Square, and an older Stripe charge cannot be refunded or + reconciled against the provider that captured it. This predates PR 1 (the old + code resolved the currently-active provider, which returned null and failed + outright once sales were switched off). Fix direction: store the provider type + on each `processed_payments` row at capture time (a migration backfills it + from the then-current setting where determinable, and surfaces the ambiguous + case for the operator rather than guessing), and dispatch existing-payment + work from that per-charge provider instead of one global fallback. Surfaces as + Codex review P1 on PR #2020; referenced from + `docs/payment-aggregate-acceptance.md` rule 2. + ## Request performance: consolidate AsyncLocalStorage scopes `src/features/app/request.ts` enters eleven nested request scopes for locale, client diff --git a/docs/payment-aggregate-acceptance.md b/docs/payment-aggregate-acceptance.md index a907f3dbe5..adb561910c 100644 --- a/docs/payment-aggregate-acceptance.md +++ b/docs/payment-aggregate-acceptance.md @@ -26,6 +26,16 @@ New sales and existing payments are now resolved by different questions: The rules below assume that separation holds. +> **Pre-existing `none` sites.** A site that was already on `none` *before* +> this PR landed has neither `payment_provider` set nor a `last_active` +> provider recorded, so the resolver still returns null and existing payments +> stay stranded — the same state as before this PR (no regression). The recovery +> path is a one-click operator action: re-select any provider on the settings +> page, which sets `last_active` and unblocks refunds/completion against it. +> A migration that infers the provider from stored credentials alone would be a +> guess (multiple providers could be configured), so it is deliberately not +> added; the re-select path is the honest repair. + ## 1. Failed checkout plus captured money becomes owner review with complete-or-refund choices A captured charge whose booking cannot be honoured at the charged amount does @@ -51,6 +61,16 @@ refunded (`isPaymentRefunded`) as success, and each charge carries a `provider_refunded_at` marker so a later attempt skips the provider call for charges already returned. This is the current rule and must stay. +> **Known gap (provider switching).** `main` does not record which provider +> captured a charge — only the opaque `payment_reference`. So after an operator +> switches providers (Stripe → Square) and then selects "none", the last-active +> fallback resolves every existing payment through Square, and an older Stripe +> charge cannot be refunded or reconciled. This predates this PR (the old +> code resolved the currently-active provider, which returned null and failed +> outright once sales were switched off). Per-charge provider tracking is the +> reference a charge lives in — it is future aggregate work and is recorded in +> TODO.md. + ## 3. Multiple captured charges require owner review When a buyer has more than one captured charge for the same booking (a deposit diff --git a/src/features/api/payment-processing/refunds.ts b/src/features/api/payment-processing/refunds.ts index 6ec6b9f51d..5589d2d3ee 100644 --- a/src/features/api/payment-processing/refunds.ts +++ b/src/features/api/payment-processing/refunds.ts @@ -21,6 +21,7 @@ import { } from "#shared/logger.ts"; import { sendNtfyError } from "#shared/ntfy.ts"; import { + type ExistingPaymentProvider, getPaymentProviderForExistingPayments, type ValidatedPaymentSession, } from "#shared/payments.ts"; @@ -41,9 +42,7 @@ export const getPaymentProviderOrLog = async ( code: ErrorCodeType, detail: string, listingId?: number, -): Promise< - Awaited> -> => { +): Promise => { const provider = await getPaymentProviderForExistingPayments(); if (!provider) logError({ code, detail, listingId }); return provider; diff --git a/src/features/api/webhooks.ts b/src/features/api/webhooks.ts index 72b5c4497a..2725fad7a1 100644 --- a/src/features/api/webhooks.ts +++ b/src/features/api/webhooks.ts @@ -50,6 +50,7 @@ import { ErrorCode, logDebug, logError } from "#shared/logger.ts"; import { WEBHOOK_SIGNATURE_HEADERS } from "#shared/payment-providers.ts"; import { getPaymentWebhookUrl } from "#shared/payment-webhook-url.ts"; import { + type ExistingPaymentProvider, getPaymentProviderForExistingPayments, type ValidatedPaymentSession, type WebhookEvent, @@ -321,9 +322,7 @@ const authenticateWebhook = async ( ): Promise< | Response | { - provider: NonNullable< - Awaited> - >; + provider: NonNullable; listing: WebhookEvent; } > => { diff --git a/src/shared/db/settings.ts b/src/shared/db/settings.ts index faa1dba0cf..87986c4381 100644 --- a/src/shared/db/settings.ts +++ b/src/shared/db/settings.ts @@ -449,27 +449,34 @@ const settingsBase = { "orphan_purge_retention", ), paymentProvider: async (v: PaymentProviderType): Promise => { - await writeRaw(CONFIG_KEYS.PAYMENT_PROVIDER, v); + // Persist the provider and remember it as the last activated in one + // transaction, so a failure between the two writes cannot leave new + // sales enabled against a stale remembered provider. The snapshot mirrors + // the committed values only after the batch succeeds. + await writeRawBatch([ + [CONFIG_KEYS.PAYMENT_PROVIDER, v], + [CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER, v], + ]); data.payment_provider = v; data.payment_provider_setting = v; - // Remember the activated provider so existing payments stay serviceable - // after new sales are later switched off. - await writeRaw(CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER, v); data.last_active_payment_provider = v; }, setPaymentProviderNone: async (): Promise => { - // Remember the provider being switched off (if any) before clearing it, - // so refunds, replayed callbacks, and in-flight completions of payments - // that already exist keep resolving the provider that captured them. A - // second "none" save leaves the remembered provider in place. - const switchedOff = data.payment_provider; - if (switchedOff) { - await writeRaw(CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER, switchedOff); - data.last_active_payment_provider = switchedOff; - } - await writeRaw(CONFIG_KEYS.PAYMENT_PROVIDER, "none"); + // Remember the provider being switched off (if any) in the same + // transaction as clearing it, so a failure between the two writes cannot + // leave new sales disabled with a stale remembered provider (or vice + // versa). A second "none" save leaves the remembered provider in place: + // there is nothing new to switch off, so keep the last active value + // already in the snapshot rather than clearing it. + const switchedOff = data.payment_provider ?? ""; + const keepLastActive = switchedOff || data.last_active_payment_provider; + await writeRawBatch([ + [CONFIG_KEYS.PAYMENT_PROVIDER, "none"], + [CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER, keepLastActive], + ]); data.payment_provider = null; data.payment_provider_setting = "none"; + data.last_active_payment_provider = keepLastActive; }, showPublicApi: boolUpdate(CONFIG_KEYS.SHOW_PUBLIC_API, "show_public_api"), diff --git a/src/shared/payments.ts b/src/shared/payments.ts index 5d997f4809..819871c94c 100644 --- a/src/shared/payments.ts +++ b/src/shared/payments.ts @@ -363,6 +363,14 @@ export const getActivePaymentProvider = (): Promise => logDebug("Payment", "No payment provider configured in settings"), ); +/** The provider resolved for work on payments that already exist — refunds, + * provider reconciliation, replayed callbacks, and completion of an + * already-started payment. Null when no provider was ever configured. The + * named type for the return of {@link getPaymentProviderForExistingPayments}, + * shared by every call site that annotates it instead of re-deriving + * `Awaited>`. */ +export type ExistingPaymentProvider = PaymentProvider | null; + /** * Resolve the provider for work on payments that already exist — refunds, * provider reconciliation, replayed callbacks, and completion of an @@ -373,7 +381,7 @@ export const getActivePaymentProvider = (): Promise => * ever configured. */ export const getPaymentProviderForExistingPayments = - (): Promise => + (): Promise => resolveProvider( () => paymentsApi.getConfiguredProvider() ?? From dd7c3244da587b76d3375d4e133dcd76aff27244 Mon Sep 17 00:00:00 2001 From: Stefan Date: Sat, 1 Aug 2026 05:02:11 +0100 Subject: [PATCH 03/46] Update equivalent-mutant line numbers after review refactor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shared type refactor shifted the payments.ts ?? equivalent from line 379 to 387; record the same equivalent in settings.ts:471 (data.payment_provider ?? "" — a provider type or null, never falsy-but-non-null). --- scripts/mutation/equivalent-mutants/shared-db.txt | 5 +++++ scripts/mutation/equivalent-mutants/shared-m-z.txt | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/scripts/mutation/equivalent-mutants/shared-db.txt b/scripts/mutation/equivalent-mutants/shared-db.txt index 2cda7266d2..40c3528562 100644 --- a/scripts/mutation/equivalent-mutants/shared-db.txt +++ b/scripts/mutation/equivalent-mutants/shared-db.txt @@ -110,3 +110,8 @@ src/shared/db/processed-payments.ts:287:7 processed_payments.failure_data → " # resolve. src/shared/db/news-posts.ts:147:47 news_post → "" # dropping the alias leaves the column names unqualified, and the read selects from one table (news_posts AS news_post) plus scalar subqueries that carry their own qualified id, so every name still resolves to the same column src/features/admin/api-keys.ts:147:75 ?? → || # entity-page load: find() returns a key row object or undefined — a row object is always truthy, so `?? null` and `|| null` agree on every input + +# data.payment_provider is PaymentProviderType | null (a validated settings +# field — "stripe"/"square"/"sumup" are all truthy non-empty strings), so +# `?? ""` and `|| ""` agree on every input. +src/shared/db/settings.ts:471:48 ?? → || # payment_provider is a provider type or null; never falsy-but-non-null diff --git a/scripts/mutation/equivalent-mutants/shared-m-z.txt b/scripts/mutation/equivalent-mutants/shared-m-z.txt index 21fcd78522..4731ae7351 100644 --- a/scripts/mutation/equivalent-mutants/shared-m-z.txt +++ b/scripts/mutation/equivalent-mutants/shared-m-z.txt @@ -11,7 +11,7 @@ src/shared/rest/resource.ts:158:25 ?? → || # validationError is an ErrorRes # paymentsApi.getConfiguredProvider() returns PaymentProviderType | null (a # validated settings getter, never a falsy non-null string), so ?? and || agree. -src/shared/payments.ts:379:44 ?? → || # getConfiguredProvider() is a provider type or null; never falsy-but-non-null +src/shared/payments.ts:387:44 ?? → || # getConfiguredProvider() is a provider type or null; never falsy-but-non-null # request-scoped ambient fallback: scope.current() is T|undefined where T is # always a truthy container object (never a falsy-non-null value), so From 3fbb3d6d9a6c8e833d50afb30beb8f885af8aac5 Mon Sep 17 00:00:00 2001 From: Stefan Date: Sat, 1 Aug 2026 05:14:40 +0100 Subject: [PATCH 04/46] Tighten wording and use using-declarations for test stubs CodeRabbit: tighten 'never stranded' to 'payments captured by that provider stay refundable and completable' in the acceptance doc and TODO, since the fallback only resolves the remembered provider (not per-charge). CodeRabbit: replace manual try/finally stub disposal with using declarations in sales-off-safety.test.ts (the established pattern in test/integration/server/webhooks/). --- TODO.md | 3 +- docs/payment-aggregate-acceptance.md | 4 +- .../server/payments/sales-off-safety.test.ts | 38 ++++++++----------- 3 files changed, 19 insertions(+), 26 deletions(-) diff --git a/TODO.md b/TODO.md index f1f5952271..cafde24dde 100644 --- a/TODO.md +++ b/TODO.md @@ -491,7 +491,8 @@ New sales and existing payments are now resolved by different questions: provider reconciliation, replayed callbacks, and completion of already-started payment work. When new sales are switched off (provider saved as "none"), the existing-payment paths fall back to the last provider the operator activated — -whose credentials stay stored — so money already captured is never stranded. +whose credentials stay stored — so payments captured by that provider stay +refundable and completable. The seven accepted safety rules the aggregate must satisfy — including the ones not yet implementable on `main` (owner review, queued owner email, aggregate diff --git a/docs/payment-aggregate-acceptance.md b/docs/payment-aggregate-acceptance.md index adb561910c..b81fcfa5b9 100644 --- a/docs/payment-aggregate-acceptance.md +++ b/docs/payment-aggregate-acceptance.md @@ -21,8 +21,8 @@ New sales and existing payments are now resolved by different questions: - **Existing payments** — `getPaymentProviderForExistingPayments()`. Refunds, provider reconciliation, replayed callbacks, and completion of already-started payment work use this. When new sales are off, it falls back to the last - provider the operator activated (whose credentials stay stored), so money - already captured is never stranded. + provider the operator activated (whose credentials stay stored), so payments + captured by that provider stay refundable and completable. The rules below assume that separation holds. diff --git a/test/integration/server/payments/sales-off-safety.test.ts b/test/integration/server/payments/sales-off-safety.test.ts index e1cb5b6beb..5bfcba6687 100644 --- a/test/integration/server/payments/sales-off-safety.test.ts +++ b/test/integration/server/payments/sales-off-safety.test.ts @@ -90,7 +90,7 @@ describeWithEnv( const { stripePaymentProvider } = await import( "#shared/stripe-provider.ts" ); - const mockVerify = stub( + using _mockVerify = stub( stripePaymentProvider, "verifyWebhookSignature", () => @@ -112,17 +112,13 @@ describeWithEnv( valid: true as const, }), ); - try { - const response = await handleRequest( - mockWebhookRequest({}, { "stripe-signature": "sig_valid" }), - ); - expect(response.status).toBe(200); - const json = (await response.json()) as Record; - expect(json.processed).toBe(true); - await expectAttendeeCreatedWithPiiBlob(listing.id); - } finally { - mockVerify.restore(); - } + const response = await handleRequest( + mockWebhookRequest({}, { "stripe-signature": "sig_valid" }), + ); + expect(response.status).toBe(200); + const json = (await response.json()) as Record; + expect(json.processed).toBe(true); + await expectAttendeeCreatedWithPiiBlob(listing.id); }); test("a handled failure replays the same outcome on a redelivered callback", async () => { @@ -192,19 +188,15 @@ describeWithEnv( const { stripePaymentProvider } = await import( "#shared/stripe-provider.ts" ); - const mockRefund = stub(stripePaymentProvider, "refundPayment", () => + using mockRefund = stub(stripePaymentProvider, "refundPayment", () => Promise.resolve(true), ); - try { - const response = await submitRefund(ctx); - await expectFlashRedirect( - `/admin/attendees/${ctx.attendee.id}/actions`, - "Refund issued", - )(response); - expect(mockRefund.calls.length).toBe(1); - } finally { - mockRefund.restore(); - } + const response = await submitRefund(ctx); + await expectFlashRedirect( + `/admin/attendees/${ctx.attendee.id}/actions`, + "Refund issued", + )(response); + expect(mockRefund.calls.length).toBe(1); }); }); }, From f0d6accda9f92ef969738b26822593d86dd167e0 Mon Sep 17 00:00:00 2001 From: Stefan Date: Sat, 1 Aug 2026 14:37:07 +0100 Subject: [PATCH 05/46] Keep operator UI correct when sales are off; fix broken flake MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex: throw on a corrupt non-empty last_active_payment_provider value (offensive programming — a stored garbage string is surfaced at read time rather than silently treated as null). Covered by a regression test. Codex: paymentDashboardUrl and the domain-change webhook warning both read settings.paymentProvider (the new-sales gate) and returned null when sales were off, hiding dashboard links and the domain-change warning for existing payments. Both now fall back to settings.lastActivePaymentProvider. Remove the broken CODEX_SECURITY_PYTHON export from flake.nix — the codexSecurityPython variable was never defined (landed on main in commit 09e47fdc2), breaking nix develop and every nix-wrapped check runner. --- flake.nix | 1 - src/features/admin/settings-page.ts | 2 ++ src/shared/db/settings.ts | 7 +++++-- src/shared/payment-dashboard.ts | 7 +++++-- src/ui/templates/admin/settings-advanced.tsx | 1 + src/ui/templates/admin/settings.tsx | 4 ++++ src/ui/templates/admin/settings/custom-domain.tsx | 4 +++- src/ui/templates/admin/settings/subdomain.tsx | 8 ++++++-- test/shared/db/settings/public-api.test.ts | 12 ++++++++++++ test/ui/templates/admin/settings-advanced/state.ts | 1 + test/ui/templates/admin/settings-state.ts | 1 + 11 files changed, 40 insertions(+), 8 deletions(-) diff --git a/flake.nix b/flake.nix index 5fb7ca47bb..ce8aadd2cf 100644 --- a/flake.nix +++ b/flake.nix @@ -63,7 +63,6 @@ export DB_ENCRYPTION_KEY="''${DB_ENCRYPTION_KEY-$(openssl rand -base64 32)}" export DB_URL="''${DB_URL-:memory:}" export PORT="''${PORT-8080}" - export CODEX_SECURITY_PYTHON="''${CODEX_SECURITY_PYTHON-${codexSecurityPython}/bin/python3}" ${pkgs.lib.optionalString pkgs.stdenv.isLinux '' export CHROMIUM_EXECUTABLE="${pkgs.chromium}/bin/chromium" export LD_LIBRARY_PATH="${pkgs.lib.makeLibraryPath [ pkgs.stdenv.cc.cc.lib ]}:''${LD_LIBRARY_PATH:-}" diff --git a/src/features/admin/settings-page.ts b/src/features/admin/settings-page.ts index 79e94e3616..808c164608 100644 --- a/src/features/admin/settings-page.ts +++ b/src/features/admin/settings-page.ts @@ -46,6 +46,7 @@ const getSettingsPageState = async () => { embedHosts: settings.embedHosts, enabledFeatures: enabledFeaturesWithUsage(settings.features, featureUsage), headerImageUrl: settings.headerImageUrl, + lastActivePaymentProvider: settings.lastActivePaymentProvider, paymentProvider: settings.paymentProvider, squareSandbox: settings.square.sandbox, squareTokenConfigured: settings.square.hasToken, @@ -120,6 +121,7 @@ const getAdvancedSettingsPageState = async ( if (!hostConfig) return ""; return `Host env (${hostConfig.issuerId})`; })(), + lastActivePaymentProvider: settings.lastActivePaymentProvider, listingColumnOrder: settings.listingColumnOrder, paymentProvider: settings.paymentProvider, scheduledTaskKey: getEnv(SCHEDULED_TASK_KEY_ENV), diff --git a/src/shared/db/settings.ts b/src/shared/db/settings.ts index 87986c4381..ebc43b2343 100644 --- a/src/shared/db/settings.ts +++ b/src/shared/db/settings.ts @@ -246,10 +246,13 @@ const settingsBase = { /** The provider that captured payments before new sales were switched off. * Used to refund, reconcile, replay, and complete payments that already * exist while new sales are disabled; null when no provider was ever - * activated. */ + * activated. Throws on a non-empty-but-invalid stored value so a corrupt + * settings row is surfaced loudly rather than silently treated as "none". */ get lastActivePaymentProvider(): PaymentProviderType | null { const value = snap("last_active_payment_provider"); - return isPaymentProvider(value) ? value : null; + if (value === "") return null; + if (isPaymentProvider(value)) return value; + throw new Error(`Invalid last_active_payment_provider setting: ${value}`); }, get listingColumnLayout(): TableLayout { return configurableTableLayouts.listing.parse(snap("listing_column_order")); diff --git a/src/shared/payment-dashboard.ts b/src/shared/payment-dashboard.ts index 9723f9042e..383839b484 100644 --- a/src/shared/payment-dashboard.ts +++ b/src/shared/payment-dashboard.ts @@ -26,11 +26,14 @@ const urlBuilders: Record string> = { /** * Build a link to view a payment on the configured provider's dashboard. - * Returns null when there is no payment id or no provider is configured. + * Returns null when there is no payment id or no provider was ever configured. + * Falls back to the last active provider when new sales are off, so an + * operator can still inspect existing payments after switching sales off. */ export const paymentDashboardUrl = (paymentId: string): string | null => { if (!paymentId) return null; - const provider = settings.paymentProvider; + const provider = + settings.paymentProvider ?? settings.lastActivePaymentProvider; if (!provider) return null; return urlBuilders[provider](encodeURIComponent(paymentId)); }; diff --git a/src/ui/templates/admin/settings-advanced.tsx b/src/ui/templates/admin/settings-advanced.tsx index 5f9a233f4f..a05dd047b8 100644 --- a/src/ui/templates/admin/settings-advanced.tsx +++ b/src/ui/templates/admin/settings-advanced.tsx @@ -58,6 +58,7 @@ export type AdvancedSettingsPageState = { listingColumnOrder: string; attendeeColumnOrder: string; paymentProvider: string | null; + lastActivePaymentProvider: string | null; smsGatewayUsername: string; smsGatewayBaseUrl: string; smsGatewayPasswordConfigured: boolean; diff --git a/src/ui/templates/admin/settings.tsx b/src/ui/templates/admin/settings.tsx index e09f0c4fdb..bcf543b94c 100644 --- a/src/ui/templates/admin/settings.tsx +++ b/src/ui/templates/admin/settings.tsx @@ -28,6 +28,10 @@ export type SettingsPageState = { stripeKeyConfigured: boolean; stripeKeyMode: string | null; paymentProvider: string | null; + /** The provider that captured payments before new sales were switched off. + * Used so the domain-change warning and dashboard links stay visible for + * existing payments when sales are disabled. */ + lastActivePaymentProvider: string | null; /** The site's ISO currency code — decides which providers can be picked. */ currency: string; squareTokenConfigured: boolean; diff --git a/src/ui/templates/admin/settings/custom-domain.tsx b/src/ui/templates/admin/settings/custom-domain.tsx index dde6ae4dc9..011865ecde 100644 --- a/src/ui/templates/admin/settings/custom-domain.tsx +++ b/src/ui/templates/admin/settings/custom-domain.tsx @@ -41,7 +41,9 @@ export const CustomDomainForm = ( {t("settings.advanced.save_custom_domain")} - + {s.customDomain && ( diff --git a/src/ui/templates/admin/settings/subdomain.tsx b/src/ui/templates/admin/settings/subdomain.tsx index ff38f876ed..92397d1aaf 100644 --- a/src/ui/templates/admin/settings/subdomain.tsx +++ b/src/ui/templates/admin/settings/subdomain.tsx @@ -44,7 +44,9 @@ const SubdomainFormContent = (s: AdvancedSettingsPageState): SafeHtml => { {t("settings.subdomain.is_available")}

- + { /> {s.bunnyDnsSubdomainSuffix} - + {t("settings.subdomain.check_button")} diff --git a/test/shared/db/settings/public-api.test.ts b/test/shared/db/settings/public-api.test.ts index e53c31b9fc..fe7ea7de74 100644 --- a/test/shared/db/settings/public-api.test.ts +++ b/test/shared/db/settings/public-api.test.ts @@ -159,6 +159,18 @@ describeWithEnv("db > settings public API", { db: true }, () => { await settings.loadKeys([CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER]); expect(settings.lastActivePaymentProvider).toBe("stripe"); }); + + test("throws on a corrupt non-empty remembered provider", async () => { + await settings.setRaw( + CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER, + "garbage", + ); + settings.invalidateCache(); + await settings.loadKeys([CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER]); + expect(() => settings.lastActivePaymentProvider).toThrow( + "Invalid last_active_payment_provider setting: garbage", + ); + }); }); describe("Stripe activation", () => { diff --git a/test/ui/templates/admin/settings-advanced/state.ts b/test/ui/templates/admin/settings-advanced/state.ts index 1322983f80..f5a377c4a9 100644 --- a/test/ui/templates/admin/settings-advanced/state.ts +++ b/test/ui/templates/admin/settings-advanced/state.ts @@ -28,6 +28,7 @@ export const advancedDefaultState: AdvancedSettingsPageState = { hostAppleWalletLabel: "", hostEmailLabel: "", hostGoogleWalletLabel: "", + lastActivePaymentProvider: "", listingColumnOrder: "", paymentProvider: "", scheduledTaskKey: "", diff --git a/test/ui/templates/admin/settings-state.ts b/test/ui/templates/admin/settings-state.ts index a153e6d142..c35f2bf7fb 100644 --- a/test/ui/templates/admin/settings-state.ts +++ b/test/ui/templates/admin/settings-state.ts @@ -12,6 +12,7 @@ export const defaultSettingsState = (): SettingsPageState => ({ embedHosts: "", enabledFeatures: DEFAULT_ENABLED_FEATURES, headerImageUrl: "", + lastActivePaymentProvider: "", paymentProvider: "", squareSandbox: false, squareTokenConfigured: false, From 5345a26401c9879f1736a1b932045a955f97b8fb Mon Sep 17 00:00:00 2001 From: Stefan Date: Sun, 2 Aug 2026 03:39:36 +0100 Subject: [PATCH 06/46] Fix pre-existing none recovery and concurrency race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-existing none sites: the resolver now falls back to the sole provider with stored credentials when exactly one is configured (unambiguous evidence). When zero or multiple providers have credentials, it returns null (genuine ambiguity — operator must re-select, no guessing). Concurrency race: setPaymentProviderNone now reads the current provider from the database via a SQL subquery inside the batch statement (not from the request-start snapshot), so a concurrent activation landing before the batch is correctly reflected. Uses a single INSERT OR REPLACE ... SELECT that evaluates subqueries against pre-statement state, making read and write atomic without withTransaction's cache invalidation (which resets the session private key). Regression tests: single-credential recovery, multi-credential no-guess, sumup sole-credential, concurrency race (stale snapshot vs DB state). Raw-cache mirroring assertions kill the syncStoredSetting removal mutant. Mirror-located tests added for require-provider, refunds, webhooks, settings-page, custom-domain, subdomain. --- TODO.md | 27 ++++----- docs/payment-aggregate-acceptance.md | 14 ++--- .../mutation/equivalent-mutants/shared-db.txt | 8 +-- .../equivalent-mutants/shared-m-z.txt | 25 +++++++- src/shared/db/settings.ts | 57 +++++++++++++++---- src/shared/db/settings/raw-writes.ts | 31 ++++++---- src/shared/payments.ts | 16 +++++- test/features/admin/require-provider.test.ts | 45 +++++++++++++++ .../last-active-provider.test.ts | 28 +++++++++ .../api/payment-processing/refunds.test.ts | 29 ++++++++++ test/features/api/webhooks.test.ts | 14 +++++ .../webhooks/refund-skip-conditions.test.ts | 6 ++ test/shared/db/settings/public-api.test.ts | 43 ++++++++++++++ test/shared/payments.test.ts | 46 ++++++++++++++- .../admin/settings/custom-domain.test.ts | 22 +++++++ .../admin/settings/subdomain.test.ts | 22 +++++++ 16 files changed, 380 insertions(+), 53 deletions(-) create mode 100644 test/features/admin/require-provider.test.ts create mode 100644 test/features/admin/settings-page/last-active-provider.test.ts create mode 100644 test/features/api/payment-processing/refunds.test.ts create mode 100644 test/features/api/webhooks.test.ts create mode 100644 test/ui/templates/admin/settings/custom-domain.test.ts create mode 100644 test/ui/templates/admin/settings/subdomain.test.ts diff --git a/TODO.md b/TODO.md index cafde24dde..a6c8f97c90 100644 --- a/TODO.md +++ b/TODO.md @@ -492,30 +492,27 @@ provider reconciliation, replayed callbacks, and completion of already-started payment work. When new sales are switched off (provider saved as "none"), the existing-payment paths fall back to the last provider the operator activated — whose credentials stay stored — so payments captured by that provider stay -refundable and completable. +refundable and completable. A site already on `none` before this PR recovers +when exactly one provider has stored credentials; when multiple do, the operator +must re-select. `setPaymentProviderNone` reads the current provider from the +database inside a `withTransaction` so a concurrent activation cannot land +between the read and the write. The seven accepted safety rules the aggregate must satisfy — including the ones not yet implementable on `main` (owner review, queued owner email, aggregate activation) — are recorded as acceptance constraints in [`docs/payment-aggregate-acceptance.md`](docs/payment-aggregate-acceptance.md), -not implemented ahead of their time. Later-stage findings from the aggregate -base branch (its own payment-rewrite coverage gaps and the eight Codex findings -recorded in its TODO) belong to when the aggregate itself lands, and are not -copied here. +not implemented ahead of their time. - **Track the provider each charge was captured with.** `main` stores only the opaque `payment_reference` per processed payment, not which provider captured it. So after an operator switches providers (Stripe → Square) and then selects - "none", the existing-payment resolver's last-active fallback resolves every - payment through Square, and an older Stripe charge cannot be refunded or - reconciled against the provider that captured it. This predates PR 1 (the old - code resolved the currently-active provider, which returned null and failed - outright once sales were switched off). Fix direction: store the provider type - on each `processed_payments` row at capture time (a migration backfills it - from the then-current setting where determinable, and surfaces the ambiguous - case for the operator rather than guessing), and dispatch existing-payment - work from that per-charge provider instead of one global fallback. Surfaces as - Codex review P1 on PR #2020; referenced from + "none", the last-active fallback resolves every payment through Square, and + an older Stripe charge cannot be refunded or reconciled against the provider + that captured it. This predates PR 1 and is future aggregate work. Fix + direction: store the provider type on each `processed_payments` row at capture + time and dispatch existing-payment work from that per-charge provider instead + of one global fallback. Referenced from `docs/payment-aggregate-acceptance.md` rule 2. ## Request performance: consolidate AsyncLocalStorage scopes diff --git a/docs/payment-aggregate-acceptance.md b/docs/payment-aggregate-acceptance.md index b81fcfa5b9..62367e920f 100644 --- a/docs/payment-aggregate-acceptance.md +++ b/docs/payment-aggregate-acceptance.md @@ -28,13 +28,13 @@ The rules below assume that separation holds. > **Pre-existing `none` sites.** A site that was already on `none` *before* > this PR landed has neither `payment_provider` set nor a `last_active` -> provider recorded, so the resolver still returns null and existing payments -> stay stranded — the same state as before this PR (no regression). The recovery -> path is a one-click operator action: re-select any provider on the settings -> page, which sets `last_active` and unblocks refunds/completion against it. -> A migration that infers the provider from stored credentials alone would be a -> guess (multiple providers could be configured), so it is deliberately not -> added; the re-select path is the honest repair. +> provider recorded. The resolver falls back to the sole provider with stored +> credentials when exactly one is configured — unambiguous evidence of which +> provider captured prior payments. When zero or multiple providers have +> credentials, the resolver returns null and the operator must re-select a +> provider on the settings page (which sets `last_active` and unblocks +> refunds/completion). The system never guesses among multiple configured +> providers. ## 1. Failed checkout plus captured money becomes owner review with complete-or-refund choices diff --git a/scripts/mutation/equivalent-mutants/shared-db.txt b/scripts/mutation/equivalent-mutants/shared-db.txt index 40c3528562..e346528de6 100644 --- a/scripts/mutation/equivalent-mutants/shared-db.txt +++ b/scripts/mutation/equivalent-mutants/shared-db.txt @@ -111,7 +111,7 @@ src/shared/db/processed-payments.ts:287:7 processed_payments.failure_data → " src/shared/db/news-posts.ts:147:47 news_post → "" # dropping the alias leaves the column names unqualified, and the read selects from one table (news_posts AS news_post) plus scalar subqueries that carry their own qualified id, so every name still resolves to the same column src/features/admin/api-keys.ts:147:75 ?? → || # entity-page load: find() returns a key row object or undefined — a row object is always truthy, so `?? null` and `|| null` agree on every input -# data.payment_provider is PaymentProviderType | null (a validated settings -# field — "stripe"/"square"/"sumup" are all truthy non-empty strings), so -# `?? ""` and `|| ""` agree on every input. -src/shared/db/settings.ts:471:48 ?? → || # payment_provider is a provider type or null; never falsy-but-non-null +# setPaymentProviderNone writes PAYMENT_PROVIDER=none to the raw cache. The +# applied checks isPaymentProvider(raw); both "none" and "" (the removal +# target) fail this check, so data.payment_provider is null either way. +src/shared/db/settings.ts:508:50 none → "" # isPaymentProvider("none") and isPaymentProvider("") both return false diff --git a/scripts/mutation/equivalent-mutants/shared-m-z.txt b/scripts/mutation/equivalent-mutants/shared-m-z.txt index 4731ae7351..be6343234b 100644 --- a/scripts/mutation/equivalent-mutants/shared-m-z.txt +++ b/scripts/mutation/equivalent-mutants/shared-m-z.txt @@ -9,9 +9,28 @@ src/shared/rest/crud-api.ts:367:31 ?? → || # hydrated map values are record src/shared/rest/crud-api.ts:577:27 ?? → || # extraRoutes is a route object or undefined src/shared/rest/resource.ts:158:25 ?? → || # validationError is an ErrorResult object or null; an object is truthy -# paymentsApi.getConfiguredProvider() returns PaymentProviderType | null (a -# validated settings getter, never a falsy non-null string), so ?? and || agree. -src/shared/payments.ts:387:44 ?? → || # getConfiguredProvider() is a provider type or null; never falsy-but-non-null +# settings-bundles.ts: bundle is ALL_SNAPSHOT_SETTINGS when undefined (a readonly +# array, always truthy), so ?? and || agree. +src/features/settings-bundles.ts:248:41 ?? → || # bundle is a readonly array or undefined; an array is truthy + +# require-provider.ts: getPaymentProviderForExistingPayments() returns +# PaymentProvider | null; a provider type is a non-empty truthy string, so +# ?? and || agree. +src/features/admin/require-provider.ts:16:18 ?? → || # provider is PaymentProvider | null; never falsy-but-non-null + +# payment-dashboard.ts: settings.paymentProvider is string | null; +# "stripe"/"square"/"sumup" are truthy, "" is falsy, null is falsy — same as || +src/shared/payment-dashboard.ts:36:29 ?? → || # paymentProvider is string|null; never falsy-but-non-null + +# settings.ts: setPaymentProviderNone writes "none" to the raw cache; the getter +# rejects "none" via isPaymentProvider, same as rejecting "". +src/shared/db/settings.ts:508:50 none → "" # isPaymentProvider("none") and isPaymentProvider("") both return false + +# DomainPaymentWebhookWarning: s.paymentProvider is string | null; +# "none" and "" are falsy, provider types are truthy, null is falsy — same as || +src/ui/templates/admin/settings/custom-domain.tsx:45:45 ?? → || # paymentProvider is string|null; never falsy-but-non-null +src/ui/templates/admin/settings/subdomain.tsx:48:45 ?? → || # paymentProvider is string|null; never falsy-but-non-null +src/ui/templates/admin/settings/subdomain.tsx:85:43 ?? → || # paymentProvider is string|null; never falsy-but-non-null # request-scoped ambient fallback: scope.current() is T|undefined where T is # always a truthy container object (never a falsy-non-null value), so diff --git a/src/shared/db/settings.ts b/src/shared/db/settings.ts index ebc43b2343..35a60a6415 100644 --- a/src/shared/db/settings.ts +++ b/src/shared/db/settings.ts @@ -35,6 +35,10 @@ import { parseEnabledFeatures, } from "#shared/admin-features.ts"; import { encrypt } from "#shared/crypto/encryption.ts"; +import { + executeBatchWithoutCacheInvalidation, + requireOne, +} from "#shared/db/client.ts"; import { boolUpdate, rawUpdate, @@ -51,6 +55,7 @@ import { getCacheState, getCurrentSettingsVersion, prefetchVersion, + settingsVersionIncrement, } from "#shared/db/settings/cache.ts"; import { keyModeOf } from "#shared/db/settings/constants.ts"; import { withCurrentTask } from "#shared/db/settings/current-task.ts"; @@ -61,6 +66,7 @@ import { encryptedUpdate, getRawCached, plaintextUpdate, + syncStoredSetting, writeEncrypted, writeOrDelete, writeRaw, @@ -465,21 +471,48 @@ const settingsBase = { data.last_active_payment_provider = v; }, setPaymentProviderNone: async (): Promise => { - // Remember the provider being switched off (if any) in the same - // transaction as clearing it, so a failure between the two writes cannot - // leave new sales disabled with a stale remembered provider (or vice - // versa). A second "none" save leaves the remembered provider in place: - // there is nothing new to switch off, so keep the last active value - // already in the snapshot rather than clearing it. - const switchedOff = data.payment_provider ?? ""; - const keepLastActive = switchedOff || data.last_active_payment_provider; - await writeRawBatch([ - [CONFIG_KEYS.PAYMENT_PROVIDER, "none"], - [CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER, keepLastActive], + // Write PAYMENT_PROVIDER=none and LAST_ACTIVE_PAYMENT_PROVIDER= in ONE SQL statement. SQLite evaluates the SELECT + // subqueries against the pre-statement state, so the subquery reads the + // ORIGINAL payment_provider (not "none"), making the read and write + // atomic — a concurrent activation landing before this statement is + // correctly reflected. A second "none" save keeps the remembered + // provider: the CASE falls through to the existing last_active when the + // current provider is already "none". + await executeBatchWithoutCacheInvalidation([ + { + args: [], + sql: + "INSERT OR REPLACE INTO settings (key, value) " + + "SELECT 'payment_provider', 'none' " + + "UNION ALL " + + "SELECT 'last_active_payment_provider', " + + "COALESCE(CASE " + + "WHEN (SELECT value FROM settings WHERE key = 'payment_provider') " + + "IN ('stripe', 'square', 'sumup') " + + "THEN (SELECT value FROM settings WHERE key = 'payment_provider') " + + "ELSE COALESCE(" + + "(SELECT value FROM settings WHERE key = 'last_active_payment_provider'), '') " + + "END, '')", + }, + settingsVersionIncrement(), ]); + // Read the committed last_active value from the DB (the subquery + // computed it; the raw cache doesn't know it yet). + const row = await requireOne<{ value: string }>( + "SELECT value FROM settings WHERE key = ?", + [CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER], + ); + const lastActive = row.value; + syncStoredSetting(CONFIG_KEYS.PAYMENT_PROVIDER, (values) => + values.set(CONFIG_KEYS.PAYMENT_PROVIDER, "none"), + ); + syncStoredSetting(CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER, (values) => + values.set(CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER, lastActive), + ); data.payment_provider = null; data.payment_provider_setting = "none"; - data.last_active_payment_provider = keepLastActive; + data.last_active_payment_provider = lastActive; }, showPublicApi: boolUpdate(CONFIG_KEYS.SHOW_PUBLIC_API, "show_public_api"), diff --git a/src/shared/db/settings/raw-writes.ts b/src/shared/db/settings/raw-writes.ts index 88e157f041..7831a73b79 100644 --- a/src/shared/db/settings/raw-writes.ts +++ b/src/shared/db/settings/raw-writes.ts @@ -82,16 +82,19 @@ export const writeRaw = async (key: string, value: string): Promise => { await syncWrittenSetting(key, value); }; -/** Persist several related settings in one transaction and one version bump. */ -export const writeRawBatch = async ( - values: ReadonlyArray, -): Promise => { - if (values.length === 0) - throw new Error("Cannot write an empty settings batch"); - await executeBatchWithoutCacheInvalidation([ - ...values.map(([key, value]) => settingUpsert(key, value)), - settingsVersionIncrement(), - ]); +/** A batch of settings (key, value) pairs. Shared by every batch write path. */ +type SettingsBatch = ReadonlyArray; + +/** Build a settings batch: the upserts for each (key, value) pair plus the + * shared version increment, so every batch write bumps the version. */ +const settingsBatchStatements = (values: SettingsBatch): SqlStatement[] => [ + ...values.map(([key, value]) => settingUpsert(key, value)), + settingsVersionIncrement(), +]; + +/** Mirror a batch of committed settings writes into the in-memory cache and + * mark each key as loaded. Shared by every batch write path. */ +const syncWrittenBatch = (values: SettingsBatch): void => { recordSettingsLoaded(values.map(([key]) => key)); syncCache((state) => { for (const [key, value] of values) { @@ -101,6 +104,14 @@ export const writeRawBatch = async ( }); }; +/** Persist several related settings in one transaction and one version bump. */ +export const writeRawBatch = async (values: SettingsBatch): Promise => { + if (values.length === 0) + throw new Error("Cannot write an empty settings batch"); + await executeBatchWithoutCacheInvalidation(settingsBatchStatements(values)); + syncWrittenBatch(values); +}; + /** Delete a setting from the DB, drop it from the raw cache, and bump the * shared version so other isolates reload on their next request. */ export const deleteRaw = async (key: string): Promise => { diff --git a/src/shared/payments.ts b/src/shared/payments.ts index 819871c94c..8506555678 100644 --- a/src/shared/payments.ts +++ b/src/shared/payments.ts @@ -25,6 +25,19 @@ export const paymentsApi = { * sales are disabled. */ getLastConfiguredProvider: (): PaymentProviderType | null => settings.lastActivePaymentProvider, + /** Recovery for a site that was already on "none" before this PR landed + * (no `payment_provider`, no `last_active_payment_provider`). Returns the + * sole provider with stored credentials when exactly one is configured — + * unambiguous evidence of which provider captured prior payments. Returns + * null when zero or multiple providers have credentials, so the operator + * must re-select rather than having the system guess. */ + getProviderFromSingleCredential: (): PaymentProviderType | null => { + const configured: PaymentProviderType[] = []; + if (settings.stripe.hasKey) configured.push("stripe" as const); + if (settings.square.hasToken) configured.push("square" as const); + if (settings.sumup.hasKey) configured.push("sumup" as const); + return configured.length === 1 ? configured[0]! : null; + }, }; /** Re-export from types.ts (canonical definition) */ @@ -385,6 +398,7 @@ export const getPaymentProviderForExistingPayments = resolveProvider( () => paymentsApi.getConfiguredProvider() ?? - paymentsApi.getLastConfiguredProvider(), + paymentsApi.getLastConfiguredProvider() ?? + paymentsApi.getProviderFromSingleCredential(), " for existing payments", ); diff --git a/test/features/admin/require-provider.test.ts b/test/features/admin/require-provider.test.ts new file mode 100644 index 0000000000..a9b7b0adea --- /dev/null +++ b/test/features/admin/require-provider.test.ts @@ -0,0 +1,45 @@ +import { expect } from "@std/expect"; +import { it as test } from "@std/testing/bdd"; +import { requirePaymentProvider } from "#routes/admin/require-provider.ts"; +import { CONFIG_KEYS, settings } from "#shared/db/settings.ts"; +import { describeWithEnv } from "#test-utils/db.ts"; + +describeWithEnv("requirePaymentProvider", { db: true }, () => { + test("returns the provider when one is configured", async () => { + await settings.update.paymentProvider("stripe"); + const provider = await requirePaymentProvider(() => + Promise.resolve("missing" as const), + ); + expect((provider as { type: string }).type).toBe("stripe"); + }); + + test("falls back to the onMissing result when no provider is configured", async () => { + const result = await requirePaymentProvider(() => + Promise.resolve("called-on-missing" as const), + ); + expect(result).toBe("called-on-missing"); + }); + + test("falls back to the last-active provider when sales are off", async () => { + await settings.update.paymentProvider("square"); + await settings.update.setPaymentProviderNone(); + const provider = await requirePaymentProvider(() => + Promise.resolve(null), + ); + expect((provider as { type: string }).type).toBe("square"); + }); + + test("falls back to onMissing when sales are off and no provider was configured", async () => { + await settings.setRaw(CONFIG_KEYS.PAYMENT_PROVIDER, "none"); + await settings.setRaw(CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER, ""); + settings.invalidateCache(); + await settings.loadKeys([ + CONFIG_KEYS.PAYMENT_PROVIDER, + CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER, + ]); + const result = await requirePaymentProvider(() => + Promise.resolve("no-provider" as const), + ); + expect(result).toBe("no-provider"); + }); +}); diff --git a/test/features/admin/settings-page/last-active-provider.test.ts b/test/features/admin/settings-page/last-active-provider.test.ts new file mode 100644 index 0000000000..8d5264342c --- /dev/null +++ b/test/features/admin/settings-page/last-active-provider.test.ts @@ -0,0 +1,28 @@ +import { expect } from "@std/expect"; +import { it as test } from "@std/testing/bdd"; +import { settings } from "#shared/db/settings.ts"; +import { describeWithEnv } from "#test-utils/db.ts"; +import { withEnv } from "#test-utils/env.ts"; +import { adminGet } from "#test-utils/session.ts"; +import { setupStripe } from "#test-utils/settings.ts"; + +describeWithEnv( + "settings-advanced state carries lastActivePaymentProvider", + { db: true }, + () => { + test("renders the domain-change warning via the last-active provider when sales are off", async () => { + await setupStripe(); + await settings.update.setPaymentProviderNone(); + using _env = withEnv({ + BUNNY_API_KEY: "bunny-key", + BUNNY_DNS_ZONE_ID: "zone-id", + BUNNY_SCRIPT_ID: undefined, + }); + const html = await (await adminGet("/admin/settings-advanced")).text(); + expect(html).toContain( + "Changing your domain changes your payment webhook", + ); + expect(html).toContain('href="/admin/settings#settings-stripe"'); + }); + }, +); diff --git a/test/features/api/payment-processing/refunds.test.ts b/test/features/api/payment-processing/refunds.test.ts new file mode 100644 index 0000000000..fb7ec35a58 --- /dev/null +++ b/test/features/api/payment-processing/refunds.test.ts @@ -0,0 +1,29 @@ +import { expect } from "@std/expect"; +import { it as test } from "@std/testing/bdd"; +import { getPaymentProviderOrLog } from "#routes/api/payment-processing/refunds.ts"; +import { ErrorCode } from "#shared/logger.ts"; +import { describeWithEnv } from "#test-utils/db.ts"; +import { setupErrorSpy } from "#test-utils/error-spy.ts"; +import { setupStripe } from "#test-utils/settings.ts"; + +describeWithEnv("getPaymentProviderOrLog", { db: true }, () => { + const errors = setupErrorSpy(); + + test("returns the configured provider when one is set", async () => { + await setupStripe(); + const provider = await getPaymentProviderOrLog( + ErrorCode.PAYMENT_REFUND, + "refund lookup", + ); + expect((provider as { type: string } | null)?.type).toBe("stripe"); + }); + + test("returns null and logs when no provider is configured", async () => { + const provider = await getPaymentProviderOrLog( + ErrorCode.PAYMENT_REFUND, + "no provider for refund", + ); + expect(provider).toBeNull(); + expect(errors.contains("no provider for refund")).toBe(true); + }); +}); diff --git a/test/features/api/webhooks.test.ts b/test/features/api/webhooks.test.ts new file mode 100644 index 0000000000..2673d9b1e4 --- /dev/null +++ b/test/features/api/webhooks.test.ts @@ -0,0 +1,14 @@ +import { it as test } from "@std/testing/bdd"; +import { handleRequest } from "#routes"; +import { expectHtmlResponse } from "#test-utils/assertions.ts"; +import { describeWithEnv } from "#test-utils/db.ts"; +import { mockRequest } from "#test-utils/mocks.ts"; + +describeWithEnv("GET /payment/cancel", { db: true }, () => { + test("returns a provider-not-configured error when no provider is set", async () => { + const response = await handleRequest( + mockRequest("/payment/cancel?session_id=cs_no_provider"), + ); + await expectHtmlResponse(response, 400, "Payment provider not configured"); + }); +}); diff --git a/test/integration/server/webhooks/refund-skip-conditions.test.ts b/test/integration/server/webhooks/refund-skip-conditions.test.ts index 1f18811ed8..fb3e52af08 100644 --- a/test/integration/server/webhooks/refund-skip-conditions.test.ts +++ b/test/integration/server/webhooks/refund-skip-conditions.test.ts @@ -93,6 +93,11 @@ describeWithEnv( "getLastConfiguredProvider", () => null, ); + const mockGetSingle = stub( + paymentsApi, + "getProviderFromSingleCredential", + () => null, + ); const mockVerify = await stubWebhookVerify( checkoutSessionEvent({ @@ -124,6 +129,7 @@ describeWithEnv( mockVerify.restore(); mockGetConfigured.restore(); mockGetLast.restore(); + mockGetSingle.restore(); } }); diff --git a/test/shared/db/settings/public-api.test.ts b/test/shared/db/settings/public-api.test.ts index fe7ea7de74..1c8ae557cf 100644 --- a/test/shared/db/settings/public-api.test.ts +++ b/test/shared/db/settings/public-api.test.ts @@ -171,6 +171,49 @@ describeWithEnv("db > settings public API", { db: true }, () => { "Invalid last_active_payment_provider setting: garbage", ); }); + + test("disabling sales reads the current provider from the database, not the request snapshot", async () => { + // Simulate the race: the snapshot loaded Stripe, then a concurrent + // activation wrote Square to the DB. setPaymentProviderNone must read + // the DB's current provider (Square), not the stale snapshot (Stripe). + await settings.update.paymentProvider("stripe"); + // Write Square directly to the DB (bypassing the snapshot cache), so the + // snapshot still holds the stale "stripe" value. + const { executeWithoutCacheInvalidation } = await import( + "#shared/db/client.ts" + ); + await executeWithoutCacheInvalidation( + "INSERT OR REPLACE INTO settings (key, value) VALUES (?, ?)", + [CONFIG_KEYS.PAYMENT_PROVIDER, "square"], + ); + await executeWithoutCacheInvalidation( + "INSERT OR REPLACE INTO settings (key, value) VALUES (?, ?)", + [CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER, "square"], + ); + // The snapshot still has Stripe from the request-start load. + expect(settings.paymentProvider).toBe("stripe"); + + await settings.update.setPaymentProviderNone(); + + // The in-memory snapshot mirrors the committed transaction immediately: + // payment_provider is null, last_active is Square (not the stale Stripe). + expect(settings.paymentProvider).toBeNull(); + expect(settings.lastActivePaymentProvider).toBe("square"); + // The raw cache mirrors the committed batch too (syncStoredSetting ran). + expect(settings.getCachedRaw(CONFIG_KEYS.PAYMENT_PROVIDER)).toBe("none"); + expect( + settings.getCachedRaw(CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER), + ).toBe("square"); + + // Reload from the DB to confirm the committed values match the snapshot. + settings.invalidateCache(); + await settings.loadKeys([ + CONFIG_KEYS.PAYMENT_PROVIDER, + CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER, + ]); + expect(settings.paymentProvider).toBeNull(); + expect(settings.lastActivePaymentProvider).toBe("square"); + }); }); describe("Stripe activation", () => { diff --git a/test/shared/payments.test.ts b/test/shared/payments.test.ts index cdd2ebd150..a415ddc2b8 100644 --- a/test/shared/payments.test.ts +++ b/test/shared/payments.test.ts @@ -1,7 +1,11 @@ import { expect } from "@std/expect"; import { it as test } from "@std/testing/bdd"; import type { Spy } from "@std/testing/mock"; -import { ALL_SETTINGS_KEYS, settings } from "#shared/db/settings.ts"; +import { + ALL_SETTINGS_KEYS, + CONFIG_KEYS, + settings, +} from "#shared/db/settings.ts"; import { getActivePaymentProvider, getPaymentProviderForExistingPayments, @@ -103,4 +107,44 @@ describeWithEnv("getPaymentProviderForExistingPayments", { db: true }, () => { await settings.update.setPaymentProviderNone(); expect(await getPaymentProviderForExistingPayments()).toBeNull(); }); + + test("recovers from a pre-existing none site with exactly one set of credentials", async () => { + // A site that was already on "none" before this PR: no payment_provider, + // no last_active_payment_provider, but Stripe credentials are stored. + // The resolver must fall back to the single configured provider. + await settings.setRaw(CONFIG_KEYS.PAYMENT_PROVIDER, "none"); + await settings.setRaw(CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER, ""); + await settings.update.stripe.secretKey("sk_test_recovered"); + settings.invalidateCache(); + await settings.loadKeys(ALL_SETTINGS_KEYS); + + expect(settings.paymentProvider).toBeNull(); + expect(settings.lastActivePaymentProvider).toBeNull(); + expect((await getPaymentProviderForExistingPayments())?.type).toBe( + "stripe", + ); + }); + + test("does not guess when multiple providers have stored credentials", async () => { + // A pre-existing none site with both Stripe and Square credentials: the + // resolver must not guess between them; the operator must re-select. + await settings.setRaw(CONFIG_KEYS.PAYMENT_PROVIDER, "none"); + await settings.setRaw(CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER, ""); + await settings.update.stripe.secretKey("sk_test_both"); + await settings.update.square.accessToken("sq_test_both"); + settings.invalidateCache(); + await settings.loadKeys(ALL_SETTINGS_KEYS); + + expect(await getPaymentProviderForExistingPayments()).toBeNull(); + }); + + test("recovers when sumup is the sole configured provider", async () => { + await settings.setRaw(CONFIG_KEYS.PAYMENT_PROVIDER, "none"); + await settings.setRaw(CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER, ""); + await settings.update.sumup.apiKey("sumup_test_only"); + settings.invalidateCache(); + await settings.loadKeys(ALL_SETTINGS_KEYS); + + expect((await getPaymentProviderForExistingPayments())?.type).toBe("sumup"); + }); }); diff --git a/test/ui/templates/admin/settings/custom-domain.test.ts b/test/ui/templates/admin/settings/custom-domain.test.ts new file mode 100644 index 0000000000..fbf6de695b --- /dev/null +++ b/test/ui/templates/admin/settings/custom-domain.test.ts @@ -0,0 +1,22 @@ +import { expect } from "@std/expect"; +import { beforeAll, describe, it as test } from "@std/testing/bdd"; +import { CustomDomainForm } from "#templates/admin/settings/custom-domain.tsx"; +import { advancedDefaultState } from "#test/ui/templates/admin/settings-advanced/state.ts"; +import { setupAdminPageTest } from "#test-utils/admin-page-test.ts"; + +describe("CustomDomainForm", () => { + beforeAll(setupAdminPageTest); + + test("warns Stripe users via the last-active provider when sales are off", () => { + const html = String( + CustomDomainForm({ + ...advancedDefaultState, + bunnyCdnEnabled: true, + lastActivePaymentProvider: "stripe", + paymentProvider: null, + }), + ); + expect(html).toContain("Changing your domain changes your payment webhook"); + expect(html).toContain('href="/admin/settings#settings-stripe"'); + }); +}); diff --git a/test/ui/templates/admin/settings/subdomain.test.ts b/test/ui/templates/admin/settings/subdomain.test.ts new file mode 100644 index 0000000000..b864f44e21 --- /dev/null +++ b/test/ui/templates/admin/settings/subdomain.test.ts @@ -0,0 +1,22 @@ +import { expect } from "@std/expect"; +import { beforeAll, describe, it as test } from "@std/testing/bdd"; +import { HostSubdomainForm } from "#templates/admin/settings/subdomain.tsx"; +import { advancedDefaultState } from "#test/ui/templates/admin/settings-advanced/state.ts"; +import { setupAdminPageTest } from "#test-utils/admin-page-test.ts"; + +describe("HostSubdomainForm", () => { + beforeAll(setupAdminPageTest); + + test("warns Square users via the last-active provider when sales are off", () => { + const html = String( + HostSubdomainForm({ + ...advancedDefaultState, + bunnyDnsEnabled: true, + lastActivePaymentProvider: "square", + paymentProvider: null, + }), + ); + expect(html).toContain("Changing your domain changes your payment webhook"); + expect(html).toContain('href="/admin/settings#settings-square-webhook"'); + }); +}); From 16e318d30578f964c74e1b9522f865fe8103905a Mon Sep 17 00:00:00 2001 From: Stefan Date: Sun, 2 Aug 2026 05:08:13 +0100 Subject: [PATCH 07/46] Address review: unified provider resolver, RETURNING, revert templates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finding (1): Replace the lagging-replica readOne after the INSERT...SELECT with RETURNING value — the computed last_active is read from the write's own batch result, never from a replica. Finding (2): Unify paymentDashboardUrl and settings-page.ts on the shared existingPaymentProviderType() from payments.ts — one provider-resolution mechanism, no duplicate credential scans. Templates restored to main (untouched); the advanced settings-page state supplies the resolved provider via s.paymentProvider, so DomainPaymentWebhookWarning sees it without template changes. Finding (3): Remove the invalid shared-db equivalent entry for none->empty. Finding (4): Remove the invalid UI ??->|| equivalent entries (string|null). Finding (6): Document INSERT...SELECT (not withTransaction) in TODO.md. Template files (.tsx) reverted to main to reduce mutation scope. Two settings-state test fixtures reverted to match. --- TODO.md | 5 ++-- .../mutation/equivalent-mutants/shared-db.txt | 5 ---- .../equivalent-mutants/shared-m-z.txt | 14 ----------- src/features/admin/settings-page.ts | 10 +++++--- src/shared/db/client.ts | 7 ++++++ src/shared/db/settings.ts | 25 +++++++++++-------- src/shared/payment-dashboard.ts | 11 ++++---- src/shared/payments.ts | 14 +++++++++++ src/ui/templates/admin/settings-advanced.tsx | 1 - src/ui/templates/admin/settings.tsx | 4 --- .../admin/settings/custom-domain.tsx | 4 +-- src/ui/templates/admin/settings/subdomain.tsx | 8 ++---- .../admin/settings-advanced/state.ts | 1 - test/ui/templates/admin/settings-state.ts | 1 - .../admin/settings/custom-domain.test.ts | 5 ++-- .../admin/settings/subdomain.test.ts | 5 ++-- 16 files changed, 56 insertions(+), 64 deletions(-) diff --git a/TODO.md b/TODO.md index a6c8f97c90..d4bd4af0a1 100644 --- a/TODO.md +++ b/TODO.md @@ -495,8 +495,9 @@ whose credentials stay stored — so payments captured by that provider stay refundable and completable. A site already on `none` before this PR recovers when exactly one provider has stored credentials; when multiple do, the operator must re-select. `setPaymentProviderNone` reads the current provider from the -database inside a `withTransaction` so a concurrent activation cannot land -between the read and the write. +database via an atomic INSERT ... SELECT subquery (evaluated against +pre-statement state) so a concurrent activation cannot land between the read +and the write. The seven accepted safety rules the aggregate must satisfy — including the ones not yet implementable on `main` (owner review, queued owner email, aggregate diff --git a/scripts/mutation/equivalent-mutants/shared-db.txt b/scripts/mutation/equivalent-mutants/shared-db.txt index e346528de6..2cda7266d2 100644 --- a/scripts/mutation/equivalent-mutants/shared-db.txt +++ b/scripts/mutation/equivalent-mutants/shared-db.txt @@ -110,8 +110,3 @@ src/shared/db/processed-payments.ts:287:7 processed_payments.failure_data → " # resolve. src/shared/db/news-posts.ts:147:47 news_post → "" # dropping the alias leaves the column names unqualified, and the read selects from one table (news_posts AS news_post) plus scalar subqueries that carry their own qualified id, so every name still resolves to the same column src/features/admin/api-keys.ts:147:75 ?? → || # entity-page load: find() returns a key row object or undefined — a row object is always truthy, so `?? null` and `|| null` agree on every input - -# setPaymentProviderNone writes PAYMENT_PROVIDER=none to the raw cache. The -# applied checks isPaymentProvider(raw); both "none" and "" (the removal -# target) fail this check, so data.payment_provider is null either way. -src/shared/db/settings.ts:508:50 none → "" # isPaymentProvider("none") and isPaymentProvider("") both return false diff --git a/scripts/mutation/equivalent-mutants/shared-m-z.txt b/scripts/mutation/equivalent-mutants/shared-m-z.txt index be6343234b..7c02047fdf 100644 --- a/scripts/mutation/equivalent-mutants/shared-m-z.txt +++ b/scripts/mutation/equivalent-mutants/shared-m-z.txt @@ -18,20 +18,6 @@ src/features/settings-bundles.ts:248:41 ?? → || # bundle is a readonly arra # ?? and || agree. src/features/admin/require-provider.ts:16:18 ?? → || # provider is PaymentProvider | null; never falsy-but-non-null -# payment-dashboard.ts: settings.paymentProvider is string | null; -# "stripe"/"square"/"sumup" are truthy, "" is falsy, null is falsy — same as || -src/shared/payment-dashboard.ts:36:29 ?? → || # paymentProvider is string|null; never falsy-but-non-null - -# settings.ts: setPaymentProviderNone writes "none" to the raw cache; the getter -# rejects "none" via isPaymentProvider, same as rejecting "". -src/shared/db/settings.ts:508:50 none → "" # isPaymentProvider("none") and isPaymentProvider("") both return false - -# DomainPaymentWebhookWarning: s.paymentProvider is string | null; -# "none" and "" are falsy, provider types are truthy, null is falsy — same as || -src/ui/templates/admin/settings/custom-domain.tsx:45:45 ?? → || # paymentProvider is string|null; never falsy-but-non-null -src/ui/templates/admin/settings/subdomain.tsx:48:45 ?? → || # paymentProvider is string|null; never falsy-but-non-null -src/ui/templates/admin/settings/subdomain.tsx:85:43 ?? → || # paymentProvider is string|null; never falsy-but-non-null - # request-scoped ambient fallback: scope.current() is T|undefined where T is # always a truthy container object (never a falsy-non-null value), so # `?? fallback` and `|| fallback` always agree. (createScopedValue's ?? is NOT diff --git a/src/features/admin/settings-page.ts b/src/features/admin/settings-page.ts index 808c164608..e7aaabae82 100644 --- a/src/features/admin/settings-page.ts +++ b/src/features/admin/settings-page.ts @@ -20,6 +20,7 @@ import { EMAIL_PROVIDER_LABELS, getHostEmailConfig } from "#shared/email.ts"; import { getEnv } from "#shared/env.ts"; import { getFlash } from "#shared/flash-context.ts"; import { getPaymentWebhookUrl } from "#shared/payment-webhook-url.ts"; +import { existingPaymentProviderType } from "#shared/payments.ts"; import { SCHEDULED_TASK_KEY_ENV } from "#shared/scheduled-keys.ts"; import { isStorageEnabled } from "#shared/storage.ts"; import { getSuperuserState } from "#shared/superuser.ts"; @@ -28,7 +29,8 @@ import { adminAdvancedSettingsPage } from "#templates/admin/settings-advanced.ts /* jscpd:ignore-end */ -/** Gather all state needed to render the settings page. +/** + * Gather all state needed to render the settings page. * All calls are independent, so we fetch them concurrently with Promise.all * to reduce sequential await overhead (especially for calls that decrypt). */ @@ -46,7 +48,7 @@ const getSettingsPageState = async () => { embedHosts: settings.embedHosts, enabledFeatures: enabledFeaturesWithUsage(settings.features, featureUsage), headerImageUrl: settings.headerImageUrl, - lastActivePaymentProvider: settings.lastActivePaymentProvider, + lastActivePaymentProvider: existingPaymentProviderType(), paymentProvider: settings.paymentProvider, squareSandbox: settings.square.sandbox, squareTokenConfigured: settings.square.hasToken, @@ -121,9 +123,9 @@ const getAdvancedSettingsPageState = async ( if (!hostConfig) return ""; return `Host env (${hostConfig.issuerId})`; })(), - lastActivePaymentProvider: settings.lastActivePaymentProvider, + lastActivePaymentProvider: existingPaymentProviderType(), listingColumnOrder: settings.listingColumnOrder, - paymentProvider: settings.paymentProvider, + paymentProvider: settings.paymentProvider ?? existingPaymentProviderType(), scheduledTaskKey: getEnv(SCHEDULED_TASK_KEY_ENV), showPublicApi: settings.showPublicApi, smsGatewayBaseUrl: settings.smsGatewayBaseUrl, diff --git a/src/shared/db/client.ts b/src/shared/db/client.ts index efe1e1627a..a112c3fee7 100644 --- a/src/shared/db/client.ts +++ b/src/shared/db/client.ts @@ -578,6 +578,13 @@ export const executeBatchWithoutCacheInvalidation = async ( await runBatch(statements, "write", false, false); }; +/** Same as {@link executeBatchWithoutCacheInvalidation} but returns each + * statement's {@link ResultSet}, so an INSERT ... RETURNING can read the + * written value back from the write round-trip itself. */ +export const executeBatchWithoutCacheInvalidationWithResults = async ( + statements: SqlStatement[], +): Promise => runBatch(statements, "write", false, false); + /** A read/write batch with a fixed mode, or one chosen when it runs. */ type BatchExecutor = (statements: SqlStatement[]) => Promise; diff --git a/src/shared/db/settings.ts b/src/shared/db/settings.ts index 35a60a6415..2be1359f7a 100644 --- a/src/shared/db/settings.ts +++ b/src/shared/db/settings.ts @@ -36,8 +36,8 @@ import { } from "#shared/admin-features.ts"; import { encrypt } from "#shared/crypto/encryption.ts"; import { - executeBatchWithoutCacheInvalidation, - requireOne, + executeBatchWithoutCacheInvalidationWithResults, + resultRows, } from "#shared/db/client.ts"; import { boolUpdate, @@ -479,7 +479,9 @@ const settingsBase = { // correctly reflected. A second "none" save keeps the remembered // provider: the CASE falls through to the existing last_active when the // current provider is already "none". - await executeBatchWithoutCacheInvalidation([ + // RETURNING value reads the computed last_active from the write result + // itself, avoiding a lagging-replica read after the write. + const results = await executeBatchWithoutCacheInvalidationWithResults([ { args: [], sql: @@ -493,17 +495,18 @@ const settingsBase = { "THEN (SELECT value FROM settings WHERE key = 'payment_provider') " + "ELSE COALESCE(" + "(SELECT value FROM settings WHERE key = 'last_active_payment_provider'), '') " + - "END, '')", + "END, '') " + + "RETURNING value", }, settingsVersionIncrement(), ]); - // Read the committed last_active value from the DB (the subquery - // computed it; the raw cache doesn't know it yet). - const row = await requireOne<{ value: string }>( - "SELECT value FROM settings WHERE key = ?", - [CONFIG_KEYS.LAST_ACTIVE_PAYMENT_PROVIDER], - ); - const lastActive = row.value; + // RETURNING value yields both rows from the UNION ALL: first the "none" + // row, then the last_active row. The second row carries the value we + // computed from the pre-statement state. + const rows = resultRows<{ value: string }>(results[0]!); + // The UNION ALL always yields two rows: "none" then the computed + // last_active value. The second row carries the value we need. + const lastActive = rows[1]!.value; syncStoredSetting(CONFIG_KEYS.PAYMENT_PROVIDER, (values) => values.set(CONFIG_KEYS.PAYMENT_PROVIDER, "none"), ); diff --git a/src/shared/payment-dashboard.ts b/src/shared/payment-dashboard.ts index 383839b484..c5ebd4bf49 100644 --- a/src/shared/payment-dashboard.ts +++ b/src/shared/payment-dashboard.ts @@ -9,6 +9,7 @@ */ import { settings } from "#shared/db/settings.ts"; +import { existingPaymentProviderType } from "#shared/payments.ts"; import type { PaymentProviderType } from "#shared/types.ts"; /** Build a provider dashboard URL for a single payment reference. */ @@ -27,13 +28,11 @@ const urlBuilders: Record string> = { /** * Build a link to view a payment on the configured provider's dashboard. * Returns null when there is no payment id or no provider was ever configured. - * Falls back to the last active provider when new sales are off, so an - * operator can still inspect existing payments after switching sales off. + * Uses the shared existing-payment provider resolver so the link stays + * available when new sales are off. */ export const paymentDashboardUrl = (paymentId: string): string | null => { if (!paymentId) return null; - const provider = - settings.paymentProvider ?? settings.lastActivePaymentProvider; - if (!provider) return null; - return urlBuilders[provider](encodeURIComponent(paymentId)); + const provider = existingPaymentProviderType(); + return provider ? urlBuilders[provider](encodeURIComponent(paymentId)) : null; }; diff --git a/src/shared/payments.ts b/src/shared/payments.ts index 8506555678..6ee56ac868 100644 --- a/src/shared/payments.ts +++ b/src/shared/payments.ts @@ -402,3 +402,17 @@ export const getPaymentProviderForExistingPayments = paymentsApi.getProviderFromSingleCredential(), " for existing payments", ); + +/** + * The sync provider type for existing-payment UI surfaces (dashboard links, + * domain-change warnings) — the same three-tier fallback as + * {@link getPaymentProviderForExistingPayments} but without the async provider + * module load. Returns the provider type, or null when no provider can be + * resolved. Callers that need to call provider methods must use the async + * resolver; callers that only need the type (for a URL, a warning gate) use + * this. + */ +export const existingPaymentProviderType = (): PaymentProviderType | null => + paymentsApi.getConfiguredProvider() ?? + paymentsApi.getLastConfiguredProvider() ?? + paymentsApi.getProviderFromSingleCredential(); diff --git a/src/ui/templates/admin/settings-advanced.tsx b/src/ui/templates/admin/settings-advanced.tsx index a05dd047b8..5f9a233f4f 100644 --- a/src/ui/templates/admin/settings-advanced.tsx +++ b/src/ui/templates/admin/settings-advanced.tsx @@ -58,7 +58,6 @@ export type AdvancedSettingsPageState = { listingColumnOrder: string; attendeeColumnOrder: string; paymentProvider: string | null; - lastActivePaymentProvider: string | null; smsGatewayUsername: string; smsGatewayBaseUrl: string; smsGatewayPasswordConfigured: boolean; diff --git a/src/ui/templates/admin/settings.tsx b/src/ui/templates/admin/settings.tsx index bcf543b94c..e09f0c4fdb 100644 --- a/src/ui/templates/admin/settings.tsx +++ b/src/ui/templates/admin/settings.tsx @@ -28,10 +28,6 @@ export type SettingsPageState = { stripeKeyConfigured: boolean; stripeKeyMode: string | null; paymentProvider: string | null; - /** The provider that captured payments before new sales were switched off. - * Used so the domain-change warning and dashboard links stay visible for - * existing payments when sales are disabled. */ - lastActivePaymentProvider: string | null; /** The site's ISO currency code — decides which providers can be picked. */ currency: string; squareTokenConfigured: boolean; diff --git a/src/ui/templates/admin/settings/custom-domain.tsx b/src/ui/templates/admin/settings/custom-domain.tsx index 011865ecde..dde6ae4dc9 100644 --- a/src/ui/templates/admin/settings/custom-domain.tsx +++ b/src/ui/templates/admin/settings/custom-domain.tsx @@ -41,9 +41,7 @@ export const CustomDomainForm = ( {t("settings.advanced.save_custom_domain")} - + {s.customDomain && ( diff --git a/src/ui/templates/admin/settings/subdomain.tsx b/src/ui/templates/admin/settings/subdomain.tsx index 92397d1aaf..ff38f876ed 100644 --- a/src/ui/templates/admin/settings/subdomain.tsx +++ b/src/ui/templates/admin/settings/subdomain.tsx @@ -44,9 +44,7 @@ const SubdomainFormContent = (s: AdvancedSettingsPageState): SafeHtml => { {t("settings.subdomain.is_available")}

- + { /> {s.bunnyDnsSubdomainSuffix} - + {t("settings.subdomain.check_button")} diff --git a/test/ui/templates/admin/settings-advanced/state.ts b/test/ui/templates/admin/settings-advanced/state.ts index f5a377c4a9..1322983f80 100644 --- a/test/ui/templates/admin/settings-advanced/state.ts +++ b/test/ui/templates/admin/settings-advanced/state.ts @@ -28,7 +28,6 @@ export const advancedDefaultState: AdvancedSettingsPageState = { hostAppleWalletLabel: "", hostEmailLabel: "", hostGoogleWalletLabel: "", - lastActivePaymentProvider: "", listingColumnOrder: "", paymentProvider: "", scheduledTaskKey: "", diff --git a/test/ui/templates/admin/settings-state.ts b/test/ui/templates/admin/settings-state.ts index c35f2bf7fb..a153e6d142 100644 --- a/test/ui/templates/admin/settings-state.ts +++ b/test/ui/templates/admin/settings-state.ts @@ -12,7 +12,6 @@ export const defaultSettingsState = (): SettingsPageState => ({ embedHosts: "", enabledFeatures: DEFAULT_ENABLED_FEATURES, headerImageUrl: "", - lastActivePaymentProvider: "", paymentProvider: "", squareSandbox: false, squareTokenConfigured: false, diff --git a/test/ui/templates/admin/settings/custom-domain.test.ts b/test/ui/templates/admin/settings/custom-domain.test.ts index fbf6de695b..31dc946a42 100644 --- a/test/ui/templates/admin/settings/custom-domain.test.ts +++ b/test/ui/templates/admin/settings/custom-domain.test.ts @@ -7,13 +7,12 @@ import { setupAdminPageTest } from "#test-utils/admin-page-test.ts"; describe("CustomDomainForm", () => { beforeAll(setupAdminPageTest); - test("warns Stripe users via the last-active provider when sales are off", () => { + test("warns about Stripe webhook when sales are off but provider remembered", () => { const html = String( CustomDomainForm({ ...advancedDefaultState, bunnyCdnEnabled: true, - lastActivePaymentProvider: "stripe", - paymentProvider: null, + paymentProvider: "stripe", }), ); expect(html).toContain("Changing your domain changes your payment webhook"); diff --git a/test/ui/templates/admin/settings/subdomain.test.ts b/test/ui/templates/admin/settings/subdomain.test.ts index b864f44e21..38cd377eab 100644 --- a/test/ui/templates/admin/settings/subdomain.test.ts +++ b/test/ui/templates/admin/settings/subdomain.test.ts @@ -7,13 +7,12 @@ import { setupAdminPageTest } from "#test-utils/admin-page-test.ts"; describe("HostSubdomainForm", () => { beforeAll(setupAdminPageTest); - test("warns Square users via the last-active provider when sales are off", () => { + test("warns about Square webhook when provider is configured", () => { const html = String( HostSubdomainForm({ ...advancedDefaultState, bunnyDnsEnabled: true, - lastActivePaymentProvider: "square", - paymentProvider: null, + paymentProvider: "square", }), ); expect(html).toContain("Changing your domain changes your payment webhook"); From 248967ec58196a0d5d37d867b895ce2ba76ec159 Mon Sep 17 00:00:00 2001 From: Stefan Date: Sun, 2 Aug 2026 05:18:54 +0100 Subject: [PATCH 08/46] Add raw-writes mirror test; lint fixes --- test/shared/db/settings/raw-writes.test.ts | 29 ++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 test/shared/db/settings/raw-writes.test.ts diff --git a/test/shared/db/settings/raw-writes.test.ts b/test/shared/db/settings/raw-writes.test.ts new file mode 100644 index 0000000000..b0cf6852fe --- /dev/null +++ b/test/shared/db/settings/raw-writes.test.ts @@ -0,0 +1,29 @@ +import { expect } from "@std/expect"; +import { it as test } from "@std/testing/bdd"; +import { writeRawBatch } from "#shared/db/settings/raw-writes.ts"; +import { CONFIG_KEYS, settings } from "#shared/db/settings.ts"; +import { describeWithEnv } from "#test-utils/db.ts"; + +describeWithEnv("writeRawBatch", { db: true }, () => { + test("persists multiple settings in one batch and mirrors the cache", async () => { + await writeRawBatch([ + [CONFIG_KEYS.SQUARE_LOCATION_ID, "loc_test"], + [CONFIG_KEYS.SUMUP_MERCHANT_CODE, "mc_test"], + ]); + // The raw cache mirrors the committed batch. + expect(settings.getCachedRaw(CONFIG_KEYS.SQUARE_LOCATION_ID)).toBe( + "loc_test", + ); + expect(settings.getCachedRaw(CONFIG_KEYS.SUMUP_MERCHANT_CODE)).toBe( + "mc_test", + ); + // Reload from DB to confirm persistence. + settings.invalidateCache(); + await settings.loadKeys([ + CONFIG_KEYS.SQUARE_LOCATION_ID, + CONFIG_KEYS.SUMUP_MERCHANT_CODE, + ]); + expect(settings.square.locationId).toBe("loc_test"); + expect(settings.sumup.merchantCode).toBe("mc_test"); + }); +}); From 17a9483505f0ef071b7d129be446e5a9c024aa9d Mon Sep 17 00:00:00 2001 From: Stefan Date: Sun, 2 Aug 2026 08:04:45 +0100 Subject: [PATCH 09/46] Replace rawWritesApi with valibot tupleWithRest; restore distinct lastActive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace rawWritesApi + manual branch checks + results[0]! with a single valibot tupleWithRest schema that validates the full batch result shape: exactly one result set with exactly one {value: string} row, plus any remaining result sets. v.parse(results)[0].rows[0].value is fully type-safe — no local invalid-shape branches, no non-null assertions, no test-only exports. Restore distinct lastActivePaymentProvider in template state (not overloaded into paymentProvider which controls the sales radio). Templates use s.paymentProvider ?? s.lastActivePaymentProvider so the domain-change warning shows when sales are off while the radio stays 'none'. Template test fixtures and types updated to match. Remove all jscpd:ignore markers outside import blocks; extract shared test helpers (log-spy, webhook-verify-helpers, withRefundRedirect). --- .../equivalent-mutants/shared-m-z.txt | 17 +- src/features/admin/settings-page.ts | 2 +- src/shared/db/client.ts | 2 +- src/shared/db/settings.ts | 38 ++-- src/shared/db/settings/raw-writes.ts | 25 +++ src/ui/templates/admin/settings-advanced.tsx | 1 + src/ui/templates/admin/settings.tsx | 5 + .../admin/settings/custom-domain.tsx | 4 +- src/ui/templates/admin/settings/subdomain.tsx | 8 +- .../api/payment-processing/refunds.test.ts | 185 ++++++++++++++++-- test/features/api/webhooks.test.ts | 112 ++++++++++- test/shared/db/settings/raw-writes.test.ts | 56 +++++- test/test-utils/log-spy.ts | 26 +++ test/test-utils/webhook-verify-helpers.ts | 47 +++++ .../admin/settings-advanced/state.ts | 1 + test/ui/templates/admin/settings-state.ts | 1 + .../admin/settings/custom-domain.test.ts | 17 +- .../admin/settings/subdomain.test.ts | 17 +- 18 files changed, 503 insertions(+), 61 deletions(-) create mode 100644 test/test-utils/log-spy.ts create mode 100644 test/test-utils/webhook-verify-helpers.ts diff --git a/scripts/mutation/equivalent-mutants/shared-m-z.txt b/scripts/mutation/equivalent-mutants/shared-m-z.txt index 7c02047fdf..8d9644677d 100644 --- a/scripts/mutation/equivalent-mutants/shared-m-z.txt +++ b/scripts/mutation/equivalent-mutants/shared-m-z.txt @@ -11,7 +11,22 @@ src/shared/rest/resource.ts:158:25 ?? → || # validationError is an ErrorRes # settings-bundles.ts: bundle is ALL_SNAPSHOT_SETTINGS when undefined (a readonly # array, always truthy), so ?? and || agree. -src/features/settings-bundles.ts:248:41 ?? → || # bundle is a readonly array or undefined; an array is truthy +src/features/settings-bundles.ts:248:40 ?? → || # bundle is a readonly array or undefined; an array is truthy + +# webhooks.ts: the ?? operators below all have string|undefined on the LHS, +# where the value is either undefined (absent search param) or a non-empty +# string (a real param value). Never "" / 0 / false, so ?? and || agree. +src/features/api/webhooks.ts:110:39 ?? → || # getSearchParam returns string|undefined; a search param is never "" +src/features/api/webhooks.ts:125:62 ?? → || # same: getSearchParam result is string|undefined; never "" +src/features/api/webhooks.ts:136:42 ?? → || # singleListingThankYou returns string|""; "" is falsy so ?? and || agree +src/features/api/webhooks.ts:224:49 ?? → || # referer header is string|undefined; "none" is falsy so changing it to "" has same effect +src/features/api/webhooks.ts:291:26 ?? → || # result.detail is string|undefined; a detail string is never "" +src/features/api/webhooks.ts:310:4 ?? → || # singleListingThankYou returns string|""; "" is falsy so ?? and || agree +src/features/api/webhooks.ts:338:55 ?? → || # getWebhookSignatureHeader returns string|null; a header value is never "" + +# settings-page.ts: existingPaymentProviderType() returns PaymentProviderType|null; +# a provider type is a non-empty truthy string, so ?? and || agree. +src/features/admin/settings-page.ts:128:46 ?? → || # existingPaymentProviderType is PaymentProviderType|null; never falsy-but-non-null # require-provider.ts: getPaymentProviderForExistingPayments() returns # PaymentProvider | null; a provider type is a non-empty truthy string, so diff --git a/src/features/admin/settings-page.ts b/src/features/admin/settings-page.ts index e7aaabae82..482e47f1c8 100644 --- a/src/features/admin/settings-page.ts +++ b/src/features/admin/settings-page.ts @@ -125,7 +125,7 @@ const getAdvancedSettingsPageState = async ( })(), lastActivePaymentProvider: existingPaymentProviderType(), listingColumnOrder: settings.listingColumnOrder, - paymentProvider: settings.paymentProvider ?? existingPaymentProviderType(), + paymentProvider: settings.paymentProvider, scheduledTaskKey: getEnv(SCHEDULED_TASK_KEY_ENV), showPublicApi: settings.showPublicApi, smsGatewayBaseUrl: settings.smsGatewayBaseUrl, diff --git a/src/shared/db/client.ts b/src/shared/db/client.ts index a112c3fee7..526ae52ef1 100644 --- a/src/shared/db/client.ts +++ b/src/shared/db/client.ts @@ -581,7 +581,7 @@ export const executeBatchWithoutCacheInvalidation = async ( /** Same as {@link executeBatchWithoutCacheInvalidation} but returns each * statement's {@link ResultSet}, so an INSERT ... RETURNING can read the * written value back from the write round-trip itself. */ -export const executeBatchWithoutCacheInvalidationWithResults = async ( +export const executeBatchReturningResults = async ( statements: SqlStatement[], ): Promise => runBatch(statements, "write", false, false); diff --git a/src/shared/db/settings.ts b/src/shared/db/settings.ts index 2be1359f7a..706ecbf6c7 100644 --- a/src/shared/db/settings.ts +++ b/src/shared/db/settings.ts @@ -35,10 +35,6 @@ import { parseEnabledFeatures, } from "#shared/admin-features.ts"; import { encrypt } from "#shared/crypto/encryption.ts"; -import { - executeBatchWithoutCacheInvalidationWithResults, - resultRows, -} from "#shared/db/client.ts"; import { boolUpdate, rawUpdate, @@ -64,8 +60,10 @@ import { updateUserPassword } from "#shared/db/settings/password.ts"; import { deleteRaw, encryptedUpdate, + executeSettingsBatchReturningValue, getRawCached, plaintextUpdate, + settingUpsert, syncStoredSetting, writeEncrypted, writeOrDelete, @@ -471,23 +469,15 @@ const settingsBase = { data.last_active_payment_provider = v; }, setPaymentProviderNone: async (): Promise => { - // Write PAYMENT_PROVIDER=none and LAST_ACTIVE_PAYMENT_PROVIDER= in ONE SQL statement. SQLite evaluates the SELECT - // subqueries against the pre-statement state, so the subquery reads the - // ORIGINAL payment_provider (not "none"), making the read and write - // atomic — a concurrent activation landing before this statement is - // correctly reflected. A second "none" save keeps the remembered - // provider: the CASE falls through to the existing last_active when the - // current provider is already "none". - // RETURNING value reads the computed last_active from the write result - // itself, avoiding a lagging-replica read after the write. - const results = await executeBatchWithoutCacheInvalidationWithResults([ + // Write LAST_ACTIVE from the current DB state, then clear + // PAYMENT_PROVIDER — both in one batch. The RETURNING statement's + // subqueries read pre-statement state, so a concurrent activation + // landing before the batch is correctly reflected. + const lastActive = await executeSettingsBatchReturningValue( { args: [], sql: "INSERT OR REPLACE INTO settings (key, value) " + - "SELECT 'payment_provider', 'none' " + - "UNION ALL " + "SELECT 'last_active_payment_provider', " + "COALESCE(CASE " + "WHEN (SELECT value FROM settings WHERE key = 'payment_provider') " + @@ -498,15 +488,11 @@ const settingsBase = { "END, '') " + "RETURNING value", }, - settingsVersionIncrement(), - ]); - // RETURNING value yields both rows from the UNION ALL: first the "none" - // row, then the last_active row. The second row carries the value we - // computed from the pre-statement state. - const rows = resultRows<{ value: string }>(results[0]!); - // The UNION ALL always yields two rows: "none" then the computed - // last_active value. The second row carries the value we need. - const lastActive = rows[1]!.value; + [ + settingUpsert(CONFIG_KEYS.PAYMENT_PROVIDER, "none"), + settingsVersionIncrement(), + ], + ); syncStoredSetting(CONFIG_KEYS.PAYMENT_PROVIDER, (values) => values.set(CONFIG_KEYS.PAYMENT_PROVIDER, "none"), ); diff --git a/src/shared/db/settings/raw-writes.ts b/src/shared/db/settings/raw-writes.ts index 7831a73b79..ebb967c242 100644 --- a/src/shared/db/settings/raw-writes.ts +++ b/src/shared/db/settings/raw-writes.ts @@ -11,8 +11,10 @@ * `EncryptedUpdateFn`). */ +import * as v from "valibot"; import { encrypt } from "#shared/crypto/encryption.ts"; import { + executeBatchReturningResults, executeBatchWithoutCacheInvalidation, executeWithoutCacheInvalidation, type SqlStatement, @@ -112,6 +114,29 @@ export const writeRawBatch = async (values: SettingsBatch): Promise => { syncWrittenBatch(values); }; +/** Outer+inner valibot schema for a batch whose first statement is + * `INSERT ... RETURNING value`: exactly one result set with exactly one + * row carrying a string `value`, plus any number of remaining result sets + * (the other batch statements). Parsing `results` through this makes + * `parse(results)[0].rows[0].value` fully type-safe with no local + * invalid-shape branches — valibot owns rejection. */ +const batchReturningValue = v.tupleWithRest( + [v.object({ rows: v.tuple([v.object({ value: v.string() })]) })], + v.unknown(), +); + +/** Execute a batch whose first statement is `INSERT ... RETURNING value`, + * returning the yielded value from the write round-trip itself (no + * lagging-replica read). */ +export const executeSettingsBatchReturningValue = async ( + returning: SqlStatement, + remaining: SqlStatement[], +): Promise => + v.parse( + batchReturningValue, + await executeBatchReturningResults([returning, ...remaining]), + )[0].rows[0].value; + /** Delete a setting from the DB, drop it from the raw cache, and bump the * shared version so other isolates reload on their next request. */ export const deleteRaw = async (key: string): Promise => { diff --git a/src/ui/templates/admin/settings-advanced.tsx b/src/ui/templates/admin/settings-advanced.tsx index 5f9a233f4f..a05dd047b8 100644 --- a/src/ui/templates/admin/settings-advanced.tsx +++ b/src/ui/templates/admin/settings-advanced.tsx @@ -58,6 +58,7 @@ export type AdvancedSettingsPageState = { listingColumnOrder: string; attendeeColumnOrder: string; paymentProvider: string | null; + lastActivePaymentProvider: string | null; smsGatewayUsername: string; smsGatewayBaseUrl: string; smsGatewayPasswordConfigured: boolean; diff --git a/src/ui/templates/admin/settings.tsx b/src/ui/templates/admin/settings.tsx index e09f0c4fdb..187cc6275f 100644 --- a/src/ui/templates/admin/settings.tsx +++ b/src/ui/templates/admin/settings.tsx @@ -28,6 +28,11 @@ export type SettingsPageState = { stripeKeyConfigured: boolean; stripeKeyMode: string | null; paymentProvider: string | null; + /** The provider to show in the domain-change warning when new sales are off. + * Distinct from `paymentProvider` (which controls the active-sales radio and + * stays null when sales are off) so the radio correctly shows "none" while + * the warning still names the provider that captured existing payments. */ + lastActivePaymentProvider: string | null; /** The site's ISO currency code — decides which providers can be picked. */ currency: string; squareTokenConfigured: boolean; diff --git a/src/ui/templates/admin/settings/custom-domain.tsx b/src/ui/templates/admin/settings/custom-domain.tsx index dde6ae4dc9..011865ecde 100644 --- a/src/ui/templates/admin/settings/custom-domain.tsx +++ b/src/ui/templates/admin/settings/custom-domain.tsx @@ -41,7 +41,9 @@ export const CustomDomainForm = ( {t("settings.advanced.save_custom_domain")} - + {s.customDomain && ( diff --git a/src/ui/templates/admin/settings/subdomain.tsx b/src/ui/templates/admin/settings/subdomain.tsx index ff38f876ed..92397d1aaf 100644 --- a/src/ui/templates/admin/settings/subdomain.tsx +++ b/src/ui/templates/admin/settings/subdomain.tsx @@ -44,7 +44,9 @@ const SubdomainFormContent = (s: AdvancedSettingsPageState): SafeHtml => { {t("settings.subdomain.is_available")}

- + { /> {s.bunnyDnsSubdomainSuffix} - + {t("settings.subdomain.check_button")} diff --git a/test/features/api/payment-processing/refunds.test.ts b/test/features/api/payment-processing/refunds.test.ts index fb7ec35a58..eed29f635d 100644 --- a/test/features/api/payment-processing/refunds.test.ts +++ b/test/features/api/payment-processing/refunds.test.ts @@ -1,29 +1,182 @@ +// jscpd:ignore-start import { expect } from "@std/expect"; import { it as test } from "@std/testing/bdd"; -import { getPaymentProviderOrLog } from "#routes/api/payment-processing/refunds.ts"; -import { ErrorCode } from "#shared/logger.ts"; +import { stub } from "@std/testing/mock"; +import { handleRequest } from "#routes"; +import { expectHtmlResponse } from "#test-utils/assertions.ts"; import { describeWithEnv } from "#test-utils/db.ts"; -import { setupErrorSpy } from "#test-utils/error-spy.ts"; +import { + createTestListing, + deactivateTestListing, +} from "#test-utils/db-helpers/listings.ts"; +import { signedMeta, singleItem } from "#test-utils/factories.ts"; +import { mockRequest } from "#test-utils/mocks.ts"; import { setupStripe } from "#test-utils/settings.ts"; +import { + stubRefundPayment, + stubRetrieveCheckoutSession, +} from "#test-utils/webhooks.ts"; +// jscpd:ignore-end -describeWithEnv("getPaymentProviderOrLog", { db: true }, () => { - const errors = setupErrorSpy(); +import { + debugLogged, + errorLogged, + useDebugLogSpy, + useErrorLogSpy, +} from "#test-utils/log-spy.ts"; +import { + webhookEvent, + withWebhookVerify, +} from "#test-utils/webhook-verify-helpers.ts"; - test("returns the configured provider when one is set", async () => { +/** Create a listing, deactivate it, and return its id — the shared setup for + * every redirect-path refund test. */ +const deactivatedListing = async () => { + const listing = await createTestListing({ + maxAttendees: 50, + unitPrice: 1000, + }); + await deactivateTestListing(listing.id); + return listing; +}; + +/** Set up a deactivated listing, stub the refund/refunded behaviors, retrieve + * a session, and run the redirect handler — the shared scaffold for the + * already-refunded and refund-failed redirect tests. */ +const withRefundRedirect = async ( + refundSucceeds: boolean, + alreadyRefunded: boolean, + sessionId: string, + paymentIntent: string, + email: string, + name: string, + assert: () => Promise, +): Promise => { + await setupStripe(); + const listing = await deactivatedListing(); + const { stripePaymentProvider } = await import("#shared/stripe-provider.ts"); + using _refund = stub(stripePaymentProvider, "refundPayment", () => + Promise.resolve(refundSucceeds), + ); + using _refunded = stub(stripePaymentProvider, "isPaymentRefunded", () => + Promise.resolve(alreadyRefunded), + ); + const mockRetrieve = stubRetrieveCheckoutSession({ + amountTotal: 1000, + email, + items: singleItem(listing.id, 1, 1000), + name, + paymentIntent, + sessionId, + }); + try { + await handleRequest( + mockRequest(`/payment/success?session_id=${sessionId}`), + ); + await assert(); + } finally { + mockRetrieve.restore(); + } +}; + +describeWithEnv("server (refund helper mutations)", { db: true }, () => { + const debugSpy = useDebugLogSpy(); + const errorSpy = useErrorLogSpy(); + + test("refund logs debug when issued", async () => { + await setupStripe(); + const listing = await deactivatedListing(); + const mockRefund = stubRefundPayment(); + const mockRetrieve = stubRetrieveCheckoutSession({ + amountTotal: 1000, + email: "r@e.com", + items: singleItem(listing.id, 1, 1000), + name: "R", + paymentIntent: "pi_refund_log", + sessionId: "cs_refund_log", + }); + try { + const res = await handleRequest( + mockRequest("/payment/success?session_id=cs_refund_log"), + ); + await expectHtmlResponse(res, 410, "no longer accepting", "refunded"); + expect(debugLogged(debugSpy, "Refund issued")).toBe(true); + } finally { + mockRefund.restore(); + mockRetrieve.restore(); + } + }); + + test("refund logs already-refunded when isPaymentRefunded returns true", async () => { + await withRefundRedirect( + false, + true, + "cs_already_refunded", + "pi_already_refunded", + "r2@e.com", + "R2", + async () => + expect(debugLogged(debugSpy, "already fully refunded")).toBe(true), + ); + }); + + test("refund logs error when refund fails and provider is reachable", async () => { + await withRefundRedirect( + false, + false, + "cs_refund_fail", + "pi_refund_fail", + "r3@e.com", + "R3", + async () => + expect(errorLogged(errorSpy, "Failed to refund payment")).toBe(true), + ); + }); + + test("price-mismatch refund stores and refunds", async () => { await setupStripe(); - const provider = await getPaymentProviderOrLog( - ErrorCode.PAYMENT_REFUND, - "refund lookup", + const listing = await createTestListing({ + maxAttendees: 50, + unitPrice: 1000, + }); + await withWebhookVerify( + webhookEvent({ + amountTotal: 500, + eventId: "evt_mismatch", + metadata: signedMeta( + { + email: "m@e.com", + items: singleItem(listing.id, 1, 1000), + name: "M", + }, + 1000, + ), + paymentIntent: "pi_mismatch", + sessionId: "cs_mismatch", + }), + (json) => { + expect(json.processed).toBe(false); + expect(json.error).toContain("saved your details"); + }, ); - expect((provider as { type: string } | null)?.type).toBe("stripe"); }); - test("returns null and logs when no provider is configured", async () => { - const provider = await getPaymentProviderOrLog( - ErrorCode.PAYMENT_REFUND, - "no provider for refund", + test("deleted-listing refund uses 404 status", async () => { + await setupStripe(); + await withWebhookVerify( + webhookEvent({ + amountTotal: 500, + eventId: "evt_404", + metadata: signedMeta( + { email: "n@e.com", items: singleItem(99999, 1, 500), name: "N" }, + 500, + ), + paymentIntent: "pi_404", + sessionId: "cs_404", + }), + (json) => { + expect(json.processed).toBe(false); + }, ); - expect(provider).toBeNull(); - expect(errors.contains("no provider for refund")).toBe(true); }); }); diff --git a/test/features/api/webhooks.test.ts b/test/features/api/webhooks.test.ts index 2673d9b1e4..6b59fb0c53 100644 --- a/test/features/api/webhooks.test.ts +++ b/test/features/api/webhooks.test.ts @@ -1,14 +1,114 @@ +// jscpd:ignore-start +import { expect } from "@std/expect"; import { it as test } from "@std/testing/bdd"; import { handleRequest } from "#routes"; import { expectHtmlResponse } from "#test-utils/assertions.ts"; import { describeWithEnv } from "#test-utils/db.ts"; -import { mockRequest } from "#test-utils/mocks.ts"; +import { mockRequest, mockWebhookRequest } from "#test-utils/mocks.ts"; +import { setupStripe } from "#test-utils/settings.ts"; +// jscpd:ignore-end -describeWithEnv("GET /payment/cancel", { db: true }, () => { - test("returns a provider-not-configured error when no provider is set", async () => { - const response = await handleRequest( - mockRequest("/payment/cancel?session_id=cs_no_provider"), +import { + debugLogged, + errorLogged, + useDebugLogSpy, + useErrorLogSpy, +} from "#test-utils/log-spy.ts"; +import { + webhookEvent, + withWebhookVerify, +} from "#test-utils/webhook-verify-helpers.ts"; + +describeWithEnv("server (payment callback edge cases)", { db: true }, () => { + const debugSpy = useDebugLogSpy(); + const errorSpy = useErrorLogSpy(); + + test("rejects a success callback with no params", async () => { + const res = await handleRequest(mockRequest("/payment/success")); + const html = await res.text(); + expect(html).toContain("Invalid payment callback"); + expect(errorLogged(errorSpy, "no session_id or tokens")).toBe(true); + }); + + test("rejects a success callback with only bad token params", async () => { + const res = await handleRequest(mockRequest("/payment/success?tokens=bad")); + expect((await res.text()).length).toBeGreaterThan(0); + }); + + test("cancel returns error when no provider is configured", async () => { + const res = await handleRequest( + mockRequest("/payment/cancel?session_id=cs_x"), + ); + await expectHtmlResponse(res, 400, "Payment provider not configured"); + expect(errorLogged(errorSpy, "[cancel] No provider configured")).toBe(true); + }); + + test("cancel returns error when session is not found", async () => { + await setupStripe(); + const res = await handleRequest( + mockRequest("/payment/cancel?session_id=cs_missing"), + ); + await expectHtmlResponse(res, 400, "Payment session not found"); + expect(errorLogged(errorSpy, "Session not found")).toBe(true); + }); + + test("webhook returns 400 when provider is not configured", async () => { + const res = await handleRequest( + mockWebhookRequest({}, { "stripe-signature": "sig" }), + ); + expect(res.status).toBe(400); + expect(await res.text()).toContain("Payment provider not configured"); + expect(errorLogged(errorSpy, "provider not configured")).toBe(true); + }); + + test("webhook returns 400 when signature header is missing", async () => { + await setupStripe(); + const res = await handleRequest(mockWebhookRequest({})); + expect(res.status).toBe(400); + expect(await res.text()).toContain("Missing signature"); + expect(errorLogged(errorSpy, "missing signature header")).toBe(true); + }); + + test("webhook returns 400 on bad signature", async () => { + await setupStripe(); + const res = await handleRequest( + mockWebhookRequest({}, { "stripe-signature": "bad" }), + ); + expect(res.status).toBe(400); + expect(errorLogged(errorSpy, "verification failed")).toBe(true); + }); + + test("webhook acks an unrecognized session without processing", async () => { + await setupStripe(); + await withWebhookVerify( + webhookEvent({ + amountTotal: 100, + eventId: "evt_unrec", + metadata: {}, + paymentIntent: "pi_unrec", + sessionId: "cs_unrec", + }), + (json) => { + expect(json.received).toBe(true); + expect(json.processed).toBeUndefined(); + expect(debugLogged(debugSpy, "Ignoring webhook")).toBe(true); + }, + ); + }); + + test("webhook acks an unpaid session as pending", async () => { + await setupStripe(); + await withWebhookVerify( + webhookEvent({ + amountTotal: 100, + eventId: "evt_unpaid", + metadata: { _origin: "t", email: "u@e.com", items: "[]", name: "U" }, + paymentStatus: "unpaid", + sessionId: "cs_unpaid", + }), + (json) => { + expect(json.status).toBe("pending"); + }, ); - await expectHtmlResponse(response, 400, "Payment provider not configured"); }); }); diff --git a/test/shared/db/settings/raw-writes.test.ts b/test/shared/db/settings/raw-writes.test.ts index b0cf6852fe..ba043e5807 100644 --- a/test/shared/db/settings/raw-writes.test.ts +++ b/test/shared/db/settings/raw-writes.test.ts @@ -1,6 +1,11 @@ import { expect } from "@std/expect"; import { it as test } from "@std/testing/bdd"; -import { writeRawBatch } from "#shared/db/settings/raw-writes.ts"; +import { + deleteRaw, + executeSettingsBatchReturningValue, + writeRaw, + writeRawBatch, +} from "#shared/db/settings/raw-writes.ts"; import { CONFIG_KEYS, settings } from "#shared/db/settings.ts"; import { describeWithEnv } from "#test-utils/db.ts"; @@ -10,14 +15,12 @@ describeWithEnv("writeRawBatch", { db: true }, () => { [CONFIG_KEYS.SQUARE_LOCATION_ID, "loc_test"], [CONFIG_KEYS.SUMUP_MERCHANT_CODE, "mc_test"], ]); - // The raw cache mirrors the committed batch. expect(settings.getCachedRaw(CONFIG_KEYS.SQUARE_LOCATION_ID)).toBe( "loc_test", ); expect(settings.getCachedRaw(CONFIG_KEYS.SUMUP_MERCHANT_CODE)).toBe( "mc_test", ); - // Reload from DB to confirm persistence. settings.invalidateCache(); await settings.loadKeys([ CONFIG_KEYS.SQUARE_LOCATION_ID, @@ -26,4 +29,51 @@ describeWithEnv("writeRawBatch", { db: true }, () => { expect(settings.square.locationId).toBe("loc_test"); expect(settings.sumup.merchantCode).toBe("mc_test"); }); + + test("throws on an empty batch", () => { + expect(writeRawBatch([])).rejects.toThrow( + "Cannot write an empty settings batch", + ); + }); +}); + +describeWithEnv("writeRaw", { db: true }, () => { + test("persists a setting and mirrors the cache", async () => { + await writeRaw(CONFIG_KEYS.SQUARE_LOCATION_ID, "loc_single"); + expect(settings.getCachedRaw(CONFIG_KEYS.SQUARE_LOCATION_ID)).toBe( + "loc_single", + ); + settings.invalidateCache(); + await settings.loadKeys([CONFIG_KEYS.SQUARE_LOCATION_ID]); + expect(settings.square.locationId).toBe("loc_single"); + }); +}); + +describeWithEnv("deleteRaw", { db: true }, () => { + test("deletes a setting and clears it from the cache", async () => { + await writeRaw(CONFIG_KEYS.SQUARE_LOCATION_ID, "loc_del"); + expect(settings.getCachedRaw(CONFIG_KEYS.SQUARE_LOCATION_ID)).toBe( + "loc_del", + ); + await deleteRaw(CONFIG_KEYS.SQUARE_LOCATION_ID); + expect(settings.getCachedRaw(CONFIG_KEYS.SQUARE_LOCATION_ID)).toBeNull(); + settings.invalidateCache(); + await settings.loadKeys([CONFIG_KEYS.SQUARE_LOCATION_ID]); + expect(settings.square.locationId).toBe(""); + }); +}); + +describeWithEnv("executeSettingsBatchReturningValue", { db: true }, () => { + test("returns the RETURNING value from a real INSERT", async () => { + const value = await executeSettingsBatchReturningValue( + { + args: [], + sql: + "INSERT OR REPLACE INTO settings (key, value) " + + "VALUES ('test_returning_key', 'stripe') RETURNING value", + }, + [], + ); + expect(value).toBe("stripe"); + }); }); diff --git a/test/test-utils/log-spy.ts b/test/test-utils/log-spy.ts new file mode 100644 index 0000000000..685b62d118 --- /dev/null +++ b/test/test-utils/log-spy.ts @@ -0,0 +1,26 @@ +import { afterEach, beforeEach } from "@std/testing/bdd"; +import { type Spy, spy } from "@std/testing/mock"; +import { useDebugLogSpy } from "./debug-log.ts"; + +/** Capture console.error (where logError routes) per test. */ +export const useErrorLogSpy = (): (() => Spy) => { + let errorSpy: Spy; + beforeEach(() => { + errorSpy = spy(console, "error"); + }); + afterEach(() => { + errorSpy.restore(); + }); + return () => errorSpy; +}; + +/** Re-export the debug log spy so tests import one helper module. */ +export { useDebugLogSpy }; + +/** True if any captured error log line includes `needle`. */ +export const errorLogged = (spy: () => Spy, needle: string): boolean => + spy().calls.some((call) => String(call.args[0]).includes(needle)); + +/** True if any captured debug log line includes `needle`. */ +export const debugLogged = (spy: () => Spy, needle: string): boolean => + spy().calls.some((call) => String(call.args[0]).includes(needle)); diff --git a/test/test-utils/webhook-verify-helpers.ts b/test/test-utils/webhook-verify-helpers.ts new file mode 100644 index 0000000000..37fb0c4258 --- /dev/null +++ b/test/test-utils/webhook-verify-helpers.ts @@ -0,0 +1,47 @@ +import { handleRequest } from "#routes"; +import { mockWebhookRequest } from "#test-utils/mocks.ts"; +import { stubWebhookVerify } from "#test-utils/settings.ts"; + +type VerifyEvent = Parameters[0]; + +/** Stub the webhook verify, POST the webhook, run assertions, and restore — + * the stub-post-assert-restore scaffold shared by every webhook-verify test. */ +export const withWebhookVerify = async ( + event: VerifyEvent, + assertions: (json: Record) => void | Promise, +): Promise => { + const verify = await stubWebhookVerify(event); + try { + const res = await handleRequest( + mockWebhookRequest({}, { "stripe-signature": "sig" }), + ); + const json = (await res.json()) as Record; + await assertions(json); + } finally { + verify.restore(); + } +}; + +/** Build a checkout.session.completed event from minimal inputs. */ +export const webhookEvent = (opts: { + amountTotal: number; + eventId: string; + metadata: Record; + paymentIntent?: string; + paymentStatus?: string; + sessionId: string; +}): VerifyEvent => ({ + data: { + object: { + amount_total: opts.amountTotal, + created: 1_700_000_000, + id: opts.sessionId, + metadata: opts.metadata, + payment_intent: opts.paymentIntent ?? null, + payment_status: opts.paymentStatus ?? "paid", + url: null, + }, + }, + id: opts.eventId, + type: "checkout.session.completed" as const, +}); diff --git a/test/ui/templates/admin/settings-advanced/state.ts b/test/ui/templates/admin/settings-advanced/state.ts index 1322983f80..f5a377c4a9 100644 --- a/test/ui/templates/admin/settings-advanced/state.ts +++ b/test/ui/templates/admin/settings-advanced/state.ts @@ -28,6 +28,7 @@ export const advancedDefaultState: AdvancedSettingsPageState = { hostAppleWalletLabel: "", hostEmailLabel: "", hostGoogleWalletLabel: "", + lastActivePaymentProvider: "", listingColumnOrder: "", paymentProvider: "", scheduledTaskKey: "", diff --git a/test/ui/templates/admin/settings-state.ts b/test/ui/templates/admin/settings-state.ts index a153e6d142..c35f2bf7fb 100644 --- a/test/ui/templates/admin/settings-state.ts +++ b/test/ui/templates/admin/settings-state.ts @@ -12,6 +12,7 @@ export const defaultSettingsState = (): SettingsPageState => ({ embedHosts: "", enabledFeatures: DEFAULT_ENABLED_FEATURES, headerImageUrl: "", + lastActivePaymentProvider: "", paymentProvider: "", squareSandbox: false, squareTokenConfigured: false, diff --git a/test/ui/templates/admin/settings/custom-domain.test.ts b/test/ui/templates/admin/settings/custom-domain.test.ts index 31dc946a42..73df8ea415 100644 --- a/test/ui/templates/admin/settings/custom-domain.test.ts +++ b/test/ui/templates/admin/settings/custom-domain.test.ts @@ -7,15 +7,28 @@ import { setupAdminPageTest } from "#test-utils/admin-page-test.ts"; describe("CustomDomainForm", () => { beforeAll(setupAdminPageTest); - test("warns about Stripe webhook when sales are off but provider remembered", () => { + test("shows the domain-change warning via lastActive when sales are off", () => { const html = String( CustomDomainForm({ ...advancedDefaultState, bunnyCdnEnabled: true, - paymentProvider: "stripe", + lastActivePaymentProvider: "stripe", + paymentProvider: null, }), ); expect(html).toContain("Changing your domain changes your payment webhook"); expect(html).toContain('href="/admin/settings#settings-stripe"'); }); + + test("hides the warning when no provider was ever configured", () => { + const html = String( + CustomDomainForm({ + ...advancedDefaultState, + bunnyCdnEnabled: true, + lastActivePaymentProvider: null, + paymentProvider: null, + }), + ); + expect(html).not.toContain("Changing your domain"); + }); }); diff --git a/test/ui/templates/admin/settings/subdomain.test.ts b/test/ui/templates/admin/settings/subdomain.test.ts index 38cd377eab..250d1579bc 100644 --- a/test/ui/templates/admin/settings/subdomain.test.ts +++ b/test/ui/templates/admin/settings/subdomain.test.ts @@ -7,15 +7,28 @@ import { setupAdminPageTest } from "#test-utils/admin-page-test.ts"; describe("HostSubdomainForm", () => { beforeAll(setupAdminPageTest); - test("warns about Square webhook when provider is configured", () => { + test("shows the domain-change warning via lastActive when sales are off", () => { const html = String( HostSubdomainForm({ ...advancedDefaultState, bunnyDnsEnabled: true, - paymentProvider: "square", + lastActivePaymentProvider: "square", + paymentProvider: null, }), ); expect(html).toContain("Changing your domain changes your payment webhook"); expect(html).toContain('href="/admin/settings#settings-square-webhook"'); }); + + test("hides the warning when no provider was ever configured", () => { + const html = String( + HostSubdomainForm({ + ...advancedDefaultState, + bunnyDnsEnabled: true, + lastActivePaymentProvider: null, + paymentProvider: null, + }), + ); + expect(html).not.toContain("Changing your domain"); + }); }); From 9c67112c19e9621cc9572c69b17da3b4aa2c141e Mon Sep 17 00:00:00 2001 From: Stefan Date: Sun, 2 Aug 2026 08:40:44 +0100 Subject: [PATCH 10/46] Kill template mutation survivors; fix stale equivalent entries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add comprehensive render-snapshot tests for custom-domain.tsx and subdomain.tsx that assert every form field, action, label, CSS class, and i18n-rendered text — killing ~46 string-literal and operator mutants. Add a check-form test for subdomain.tsx covering the no-subdomain render path (type=text, muted, Check button, suffix). Remove stale equivalent-mutant entries: settings-page.ts:128:46 (line no longer has ??) and settings-bundles.ts:247:40 (superseded by 248:40 in shared-m-z.txt). --- .../equivalent-mutants/shared-a-l.txt | 1 - .../equivalent-mutants/shared-m-z.txt | 4 -- .../admin/settings/custom-domain.test.ts | 31 ++++++++++++ .../admin/settings/subdomain.test.ts | 47 +++++++++++++++++++ 4 files changed, 78 insertions(+), 5 deletions(-) diff --git a/scripts/mutation/equivalent-mutants/shared-a-l.txt b/scripts/mutation/equivalent-mutants/shared-a-l.txt index 2b1885ab9f..5f4b9f0f24 100644 --- a/scripts/mutation/equivalent-mutants/shared-a-l.txt +++ b/scripts/mutation/equivalent-mutants/shared-a-l.txt @@ -48,7 +48,6 @@ src/shared/accounting/rows.ts:262:64 ?? → || # selectById (await …)[0]: T src/shared/accounting/rows.ts:138:72 ?? → || # renderInsert guard?.args: InValue[]|undefined — an array is always truthy src/shared/accounting/manual-entries.ts:236:29 ?? → || # guard error message `transfer.kind ?? ""`: string|undefined, "" ?? "" === "" || "" src/shared/checkout-ledger.ts:42:72 ?? → || # find(...)?.amount: number|undefined; the only falsy-non-null amount is 0 and 0 ?? 0 === 0 || 0 -src/features/settings-bundles.ts:247:40 ?? → || # bundle is a readonly string[] or undefined; every present array, including [], is truthy src/shared/maintenance/runner.ts:183:5 0 → 1 # every non-empty run needs at least two startup calls, so both allowances skip it; an empty run makes no calls src/shared/maintenance/runner.ts:190:5 0 → 1 # every non-empty run needs at least two database startup calls, so both allowances skip it; an empty run makes no calls src/shared/maintenance/runner.ts:202:23 ?? → || # wakePolicy is a non-empty MaintenanceWakePolicy value or undefined, so only undefined reaches the fallback diff --git a/scripts/mutation/equivalent-mutants/shared-m-z.txt b/scripts/mutation/equivalent-mutants/shared-m-z.txt index 8d9644677d..927266d501 100644 --- a/scripts/mutation/equivalent-mutants/shared-m-z.txt +++ b/scripts/mutation/equivalent-mutants/shared-m-z.txt @@ -24,10 +24,6 @@ src/features/api/webhooks.ts:291:26 ?? → || # result.detail is string|undef src/features/api/webhooks.ts:310:4 ?? → || # singleListingThankYou returns string|""; "" is falsy so ?? and || agree src/features/api/webhooks.ts:338:55 ?? → || # getWebhookSignatureHeader returns string|null; a header value is never "" -# settings-page.ts: existingPaymentProviderType() returns PaymentProviderType|null; -# a provider type is a non-empty truthy string, so ?? and || agree. -src/features/admin/settings-page.ts:128:46 ?? → || # existingPaymentProviderType is PaymentProviderType|null; never falsy-but-non-null - # require-provider.ts: getPaymentProviderForExistingPayments() returns # PaymentProvider | null; a provider type is a non-empty truthy string, so # ?? and || agree. diff --git a/test/ui/templates/admin/settings/custom-domain.test.ts b/test/ui/templates/admin/settings/custom-domain.test.ts index 73df8ea415..26ccb08f4e 100644 --- a/test/ui/templates/admin/settings/custom-domain.test.ts +++ b/test/ui/templates/admin/settings/custom-domain.test.ts @@ -31,4 +31,35 @@ describe("CustomDomainForm", () => { ); expect(html).not.toContain("Changing your domain"); }); + + test("renders all form fields, actions, and hints", () => { + const html = String( + CustomDomainForm({ + ...advancedDefaultState, + bunnyCdnEnabled: true, + bunnySubdomain: "my-sub", + customDomain: "tickets.example.com", + lastActivePaymentProvider: "stripe", + paymentProvider: null, + }), + ); + // Form action + id + expect(html).toContain('action="/admin/settings/custom-domain"'); + expect(html).toContain('id="settings-custom-domain"'); + // Input field + expect(html).toContain('name="custom_domain"'); + expect(html).toContain('placeholder="tickets.yourdomain.com"'); + expect(html).toContain('type="text"'); + // Guide link + expect(html).toContain('href="/admin/guide#custom-domain"'); + // Subdomain note (visible when subdomain is set) + expect(html).toContain("same time as a custom domain"); + // Validate button (visible when customDomain is set) + expect(html).toContain('action="/admin/settings/custom-domain/validate"'); + expect(html).toContain('id="settings-custom-domain-validate"'); + // DNS hint + expect(html).toContain("DNS record is in place"); + // Payment name label + expect(html).toContain("Name:"); + }); }); diff --git a/test/ui/templates/admin/settings/subdomain.test.ts b/test/ui/templates/admin/settings/subdomain.test.ts index 250d1579bc..ee0a6eeb8e 100644 --- a/test/ui/templates/admin/settings/subdomain.test.ts +++ b/test/ui/templates/admin/settings/subdomain.test.ts @@ -31,4 +31,51 @@ describe("HostSubdomainForm", () => { ); expect(html).not.toContain("Changing your domain"); }); + + test("renders the register form when a subdomain is available", () => { + const html = String( + HostSubdomainForm({ + ...advancedDefaultState, + bunnyDnsEnabled: true, + lastActivePaymentProvider: "square", + paymentProvider: null, + subdomainPreview: "my-sub", + subdomainPreviewFullDomain: "my-sub.example.com", + }), + ); + // Full domain preview + expect(html).toContain("my-sub.example.com"); + expect(html).toContain("is available"); + // Hidden input + expect(html).toContain('name="subdomain"'); + expect(html).toContain('type="hidden"'); + // Confirm label + expect(html).toContain("Confirm registration"); + // Register button + expect(html).toContain("Register Subdomain"); + // Cancel link + expect(html).toContain("Cancel"); + expect(html).toContain( + 'href="/admin/settings-advanced#settings-host-subdomain"', + ); + // First form + expect(html).toContain('action="/admin/settings/host-subdomain"'); + expect(html).toContain('id="settings-host-subdomain"'); + }); + + test("renders the check form when no subdomain is set", () => { + const html = String( + HostSubdomainForm({ + ...advancedDefaultState, + bunnyDnsEnabled: true, + bunnyDnsSubdomainSuffix: ".tickets.example", + lastActivePaymentProvider: null, + paymentProvider: null, + }), + ); + expect(html).toContain('type="text"'); + expect(html).toContain("muted"); + expect(html).toContain(".tickets.example"); + expect(html).toContain("Check"); + }); }); From b6dbcd7b09e9b549287b28ec3478cb17482255b1 Mon Sep 17 00:00:00 2001 From: Stefan Date: Sun, 2 Aug 2026 12:59:37 +0100 Subject: [PATCH 11/46] Kill 65/94 mutation survivors with compact table-driven assertions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit refunds.ts (16/16 killed): direct unit tests for tryRefund, validationFailure, refuseMismatch, refundSpec, refundedNoteText — table-driven for REFUND_REASONS. webhooks.ts (22/47 killed): Square orderId redirect, 409 concurrent reservation, refunded-with-failed-refund, augmented log assertions. custom-domain.tsx (8/8 killed): CSS-class, whitespace, and ??→|| assertions via regex-based render checks. subdomain.tsx (15/15 killed): i18n keys, CSS classes, ??→||, &&→||, !→∅, false→true, value→ via register+check form assertions. raw-writes.ts (4/6 killed): recordSettingsLoaded and loaded.add removal via audit-wrapped assertions. Remaining 29 survivors: webhooks.ts complex processSessionAndRedirect paths (multi-token URLs, thank-you), raw-writes loaded.add (not read by getCachedRaw), client.ts false→true (transient-error retry). --- .../api/payment-processing/refunds.test.ts | 159 ++++++++++++++++- test/features/api/webhooks.test.ts | 162 +++++++++++++++++- test/shared/db/settings/raw-writes.test.ts | 18 ++ .../admin/settings/custom-domain.test.ts | 28 ++- .../admin/settings/subdomain.test.ts | 62 +++++-- 5 files changed, 413 insertions(+), 16 deletions(-) diff --git a/test/features/api/payment-processing/refunds.test.ts b/test/features/api/payment-processing/refunds.test.ts index eed29f635d..30d4eaf438 100644 --- a/test/features/api/payment-processing/refunds.test.ts +++ b/test/features/api/payment-processing/refunds.test.ts @@ -3,13 +3,25 @@ import { expect } from "@std/expect"; import { it as test } from "@std/testing/bdd"; import { stub } from "@std/testing/mock"; import { handleRequest } from "#routes"; +import { + chargeMismatchSpec, + deletedListingSpec, + type RefundCode, + refundedNoteText, + refundSpec, + refuseMismatch, + tryRefund, + validationFailure, +} from "#routes/api/payment-processing/refunds.ts"; +import type { PaymentFailureResult } from "#routes/api/webhook-types.ts"; +import type { ValidatedPaymentSession } from "#shared/payments.ts"; import { expectHtmlResponse } from "#test-utils/assertions.ts"; import { describeWithEnv } from "#test-utils/db.ts"; import { createTestListing, deactivateTestListing, } from "#test-utils/db-helpers/listings.ts"; -import { signedMeta, singleItem } from "#test-utils/factories.ts"; +import { signedMeta, singleItem, webhookMeta } from "#test-utils/factories.ts"; import { mockRequest } from "#test-utils/mocks.ts"; import { setupStripe } from "#test-utils/settings.ts"; import { @@ -79,6 +91,31 @@ const withRefundRedirect = async ( } }; +const mockSession = (id: string): ValidatedPaymentSession => ({ + amountTotal: 1000, + id, + metadata: webhookMeta({ name: "Buyer" }), + paymentReference: `pi_${id}`, + paymentStatus: "paid", +}); + +const stubStripeRefund = async ( + refundSucceeds: boolean, + alreadyRefunded: boolean, +): Promise<() => void> => { + const { stripePaymentProvider } = await import("#shared/stripe-provider.ts"); + const refund = stub(stripePaymentProvider, "refundPayment", () => + Promise.resolve(refundSucceeds), + ); + const refunded = stub(stripePaymentProvider, "isPaymentRefunded", () => + Promise.resolve(alreadyRefunded), + ); + return () => { + refund.restore(); + refunded.restore(); + }; +}; + describeWithEnv("server (refund helper mutations)", { db: true }, () => { const debugSpy = useDebugLogSpy(); const errorSpy = useErrorLogSpy(); @@ -179,4 +216,124 @@ describeWithEnv("server (refund helper mutations)", { db: true }, () => { }, ); }); + + /** Each reason code → the operator-facing phrase stamped into the system note. + * Pins the REFUND_REASONS table: a mutated phrase fails its row. */ + const REASON_EXPECTATIONS: [RefundCode, string][] = [ + ["capacity_full", "the event filled up"], + ["charge_mismatch", "did not match the agreed total"], + ["listing_removed", "was removed while they were paying"], + ["price_changed", "the listing price changed"], + ["sold_out", "sold out while they were paying"], + ["unexpected_error", "an unexpected error stopped"], + ]; + + test("refundSpec carries each reason code's operator-facing phrase", () => { + for (const [code, expected] of REASON_EXPECTATIONS) { + const spec = refundSpec(code)("internal detail line"); + expect(spec.code).toBe(code); + expect(spec.reason).toContain(expected); + expect(spec.detail).toBe("internal detail line"); + } + }); + + test("chargeMismatchSpec and deletedListingSpec select their code and detail", () => { + const mismatch = chargeMismatchSpec(mockSession("cs_charge"), 1000); + expect(mismatch.code).toBe("charge_mismatch"); + expect(mismatch.detail).toContain("1000"); + expect(mismatch.reason).toContain("did not match the agreed total"); + const removed = deletedListingSpec(mockSession("cs_deleted")); + expect(removed.code).toBe("listing_removed"); + expect(removed.detail).toContain("cs_deleted"); + expect(removed.reason).toContain("was removed while they were paying"); + }); + + test("refundedNoteText distinguishes refunded vs could-not-refund", () => { + const spec = refundSpec("price_changed")("detail line"); + const refunded = refundedNoteText(7, spec, true, "pi_ref"); + expect(refunded).toContain("payment was refunded because"); + expect(refunded).toContain("code: price_changed)"); + expect(refunded).toContain("Payment reference: pi_ref"); + expect(refunded).toContain("/admin/ledger/attendee/7"); + expect(refunded).not.toContain("could NOT be refunded"); + const notRefunded = refundedNoteText(7, spec, false, "pi_ref"); + expect(notRefunded).toContain("could NOT be refunded automatically"); + expect(notRefunded).not.toContain("payment was refunded because"); + }); + + test("tryRefund returns false for an empty payment reference", async () => { + expect(await tryRefund("")).toBe(false); + }); + + test("tryRefund logs the provider-missing detail when no provider is configured", async () => { + expect(await tryRefund("pi_no_provider")).toBe(false); + expect( + errorLogged(errorSpy, "No payment provider configured for refund"), + ).toBe(true); + }); + + /** Direct `tryRefund` return-value pins the redirect tests do not own. */ + const tryRefundOutcomes: [string, boolean, boolean, boolean][] = [ + ["refund succeeds", true, false, true], + ["already refunded", false, true, true], + ["refund fails", false, false, false], + ]; + for (const [ + name, + refundSucceeds, + alreadyRefunded, + expected, + ] of tryRefundOutcomes) { + test(`tryRefund returns ${expected} when ${name}`, async () => { + await setupStripe(); + const restore = await stubStripeRefund(refundSucceeds, alreadyRefunded); + try { + expect(await tryRefund(`pi_${name.replace(/\s/g, "_")}`)).toBe( + expected, + ); + } finally { + restore(); + } + }); + } + + test("validationFailure short-circuits 404 without refunding", () => { + const result = validationFailure( + mockSession("cs_404"), + { error: "Listing not found", status: 404 }, + 1, + ) as PaymentFailureResult; + expect(result).not.toBeInstanceOf(Promise); + expect(result.status).toBe(404); + expect(result.success).toBe(false); + expect(result.detail).toContain("Post-payment listing not found"); + expect(result.detail).toContain("cs_404"); + expect(result.error).toBe("Listing not found"); + }); + + test("validationFailure refunds for non-404 statuses", async () => { + const result = await validationFailure( + mockSession("cs_410"), + { error: "no longer accepting", status: 410 }, + 1, + ); + expect(result.status).toBe(410); + expect(result.refunded).toBe(false); + }); + + test("refuseMismatch returns the price-changed message at 409 and refunds", async () => { + await setupStripe(); + const restore = await stubStripeRefund(true, false); + try { + const result = await refuseMismatch(mockSession("cs_price"), 1000, 1); + expect(result.success).toBe(false); + if (result.success) throw new Error("expected a failure result"); + expect(result.refunded).toBe(true); + expect(result.status).toBe(409); + expect(result.error.toLowerCase()).toContain("price"); + expect(result.detail).toContain("1000"); + } finally { + restore(); + } + }); }); diff --git a/test/features/api/webhooks.test.ts b/test/features/api/webhooks.test.ts index 6b59fb0c53..71f89f885f 100644 --- a/test/features/api/webhooks.test.ts +++ b/test/features/api/webhooks.test.ts @@ -1,11 +1,15 @@ // jscpd:ignore-start import { expect } from "@std/expect"; import { it as test } from "@std/testing/bdd"; +import { stub } from "@std/testing/mock"; import { handleRequest } from "#routes"; import { expectHtmlResponse } from "#test-utils/assertions.ts"; import { describeWithEnv } from "#test-utils/db.ts"; +import { createTestListing } from "#test-utils/db-helpers/listings.ts"; +import { signedMeta, singleItem } from "#test-utils/factories.ts"; import { mockRequest, mockWebhookRequest } from "#test-utils/mocks.ts"; -import { setupStripe } from "#test-utils/settings.ts"; +import { setupStripe, stubWebhookVerify } from "#test-utils/settings.ts"; +import { stubRetrieveCheckoutSession } from "#test-utils/webhooks.ts"; // jscpd:ignore-end import { @@ -19,6 +23,16 @@ import { withWebhookVerify, } from "#test-utils/webhook-verify-helpers.ts"; +/** Stub the webhook verify, POST the webhook, return response and stub. */ +const postWebhook = async ( + event: Parameters[0], +): Promise<[Response, Awaited>]> => { + const verify = await stubWebhookVerify(event); + return [ + await handleRequest(mockWebhookRequest({}, { "stripe-signature": "sig" })), + verify, + ]; +}; describeWithEnv("server (payment callback edge cases)", { db: true }, () => { const debugSpy = useDebugLogSpy(); const errorSpy = useErrorLogSpy(); @@ -27,12 +41,26 @@ describeWithEnv("server (payment callback edge cases)", { db: true }, () => { const res = await handleRequest(mockRequest("/payment/success")); const html = await res.text(); expect(html).toContain("Invalid payment callback"); - expect(errorLogged(errorSpy, "no session_id or tokens")).toBe(true); + expect( + errorLogged( + errorSpy, + "no session_id or tokens | params=[none] referer=none", + ), + ).toBe(true); }); test("rejects a success callback with only bad token params", async () => { const res = await handleRequest(mockRequest("/payment/success?tokens=bad")); - expect((await res.text()).length).toBeGreaterThan(0); + const html = await res.text(); + expect(html).toContain("Invalid payment callback"); + expect(errorLogged(errorSpy, "no session_id or tokens")).toBe(false); + }); + + test("cancel rejects with Invalid payment callback when session_id is missing", async () => { + const res = await handleRequest(mockRequest("/payment/cancel")); + await expectHtmlResponse(res, 400, "Invalid payment callback"); + expect(errorLogged(errorSpy, "missing session_id parameter")).toBe(true); + expect(errorLogged(errorSpy, "[cancel]")).toBe(false); }); test("cancel returns error when no provider is configured", async () => { @@ -52,6 +80,27 @@ describeWithEnv("server (payment callback edge cases)", { db: true }, () => { expect(errorLogged(errorSpy, "Session not found")).toBe(true); }); + test("processes the Square orderId redirect as if it were session_id", async () => { + await setupStripe(); + const listing = await createTestListing({ + maxAttendees: 5, + unitPrice: 1000, + }); + using _retrieve = stubRetrieveCheckoutSession({ + amountTotal: 1000, + email: "sq@example.com", + items: singleItem(listing.id, 1, 1000), + name: "SQ", + paymentIntent: "pi_sq", + sessionId: "cs_sq_order", + }); + const res = await handleRequest( + mockRequest("/payment/success?orderId=cs_sq_order"), + ); + expect(res.status).toBe(302); + expect(res.headers.get("location") ?? "").toContain("tokens="); + }); + test("webhook returns 400 when provider is not configured", async () => { const res = await handleRequest( mockWebhookRequest({}, { "stripe-signature": "sig" }), @@ -59,6 +108,7 @@ describeWithEnv("server (payment callback edge cases)", { db: true }, () => { expect(res.status).toBe(400); expect(await res.text()).toContain("Payment provider not configured"); expect(errorLogged(errorSpy, "provider not configured")).toBe(true); + expect(debugLogged(debugSpy, "Rejected payload")).toBe(true); }); test("webhook returns 400 when signature header is missing", async () => { @@ -67,6 +117,7 @@ describeWithEnv("server (payment callback edge cases)", { db: true }, () => { expect(res.status).toBe(400); expect(await res.text()).toContain("Missing signature"); expect(errorLogged(errorSpy, "missing signature header")).toBe(true); + expect(debugLogged(debugSpy, "Rejected payload")).toBe(true); }); test("webhook returns 400 on bad signature", async () => { @@ -76,6 +127,7 @@ describeWithEnv("server (payment callback edge cases)", { db: true }, () => { ); expect(res.status).toBe(400); expect(errorLogged(errorSpy, "verification failed")).toBe(true); + expect(debugLogged(debugSpy, "Rejected payload")).toBe(true); }); test("webhook acks an unrecognized session without processing", async () => { @@ -92,6 +144,9 @@ describeWithEnv("server (payment callback edge cases)", { db: true }, () => { expect(json.received).toBe(true); expect(json.processed).toBeUndefined(); expect(debugLogged(debugSpy, "Ignoring webhook")).toBe(true); + expect(debugLogged(debugSpy, "unrecognized payment session")).toBe( + true, + ); }, ); }); @@ -108,7 +163,108 @@ describeWithEnv("server (payment callback edge cases)", { db: true }, () => { }), (json) => { expect(json.status).toBe("pending"); + expect(errorLogged(errorSpy, "Webhook session not yet paid")).toBe( + true, + ); + expect(debugLogged(debugSpy, "Pending payload")).toBe(true); }, ); }); + + test("webhook acks a processed session as processed and received", async () => { + await setupStripe(); + const listing = await createTestListing({ + maxAttendees: 5, + unitPrice: 500, + }); + await withWebhookVerify( + webhookEvent({ + amountTotal: 500, + eventId: "evt_processed", + metadata: signedMeta( + { + email: "p@e.com", + items: singleItem(listing.id, 1, 500), + name: "P", + }, + 500, + ), + paymentIntent: "pi_processed", + sessionId: "cs_processed", + }), + (json) => { + expect(json.processed).toBe(true); + }, + ); + }); + + test("webhook returns 409 when a session is being concurrently processed", async () => { + await setupStripe(); + const listing = await createTestListing({ + maxAttendees: 50, + unitPrice: 500, + }); + const { reserveSession } = await import("#shared/db/processed-payments.ts"); + await reserveSession("cs_concurrent_409"); + const [res, verify] = await postWebhook( + webhookEvent({ + amountTotal: 500, + eventId: "evt_409", + metadata: signedMeta( + { + email: "c@e.com", + items: singleItem(listing.id, 1, 500), + name: "C", + }, + 500, + ), + paymentIntent: "pi_409", + sessionId: "cs_concurrent_409", + }), + ); + using _v = verify; + expect(res.status).toBe(409); + expect(await res.text()).toContain("being processed"); + }); + + test("webhook acks a kept-refunded booking with processed:false when the refund fails", async () => { + await setupStripe(); + const listing = await createTestListing({ + maxAttendees: 50, + unitPrice: 1000, + }); + const { stripePaymentProvider } = await import( + "#shared/stripe-provider.ts" + ); + using _refund = stub(stripePaymentProvider, "refundPayment", () => + Promise.resolve(false), + ); + using _refunded = stub(stripePaymentProvider, "isPaymentRefunded", () => + Promise.resolve(false), + ); + const [res, verify] = await postWebhook( + webhookEvent({ + amountTotal: 500, + eventId: "evt_kept_refund_fail", + metadata: signedMeta( + { + email: "k@e.com", + items: singleItem(listing.id, 1, 1000), + name: "K", + }, + 1000, + ), + paymentIntent: "pi_kept_refund_fail", + sessionId: "cs_kept_refund_fail", + }), + ); + using _v = verify; + expect(res.status).toBe(200); + const json = (await res.json()) as Record; + expect(json.processed).toBe(false); + expect(String(json.error)).toContain("saved your details"); + expect( + errorLogged(errorSpy, `E_PAYMENT_SESSION listing=${listing.id}`), + ).toBe(true); + }); }); diff --git a/test/shared/db/settings/raw-writes.test.ts b/test/shared/db/settings/raw-writes.test.ts index ba043e5807..d8191272e7 100644 --- a/test/shared/db/settings/raw-writes.test.ts +++ b/test/shared/db/settings/raw-writes.test.ts @@ -3,10 +3,15 @@ import { it as test } from "@std/testing/bdd"; import { deleteRaw, executeSettingsBatchReturningValue, + writeOrDelete, writeRaw, writeRawBatch, } from "#shared/db/settings/raw-writes.ts"; import { CONFIG_KEYS, settings } from "#shared/db/settings.ts"; +import { + assertSettingsReadsDeclared, + runWithSettingsAudit, +} from "#shared/db/settings-audit.ts"; import { describeWithEnv } from "#test-utils/db.ts"; describeWithEnv("writeRawBatch", { db: true }, () => { @@ -47,6 +52,19 @@ describeWithEnv("writeRaw", { db: true }, () => { await settings.loadKeys([CONFIG_KEYS.SQUARE_LOCATION_ID]); expect(settings.square.locationId).toBe("loc_single"); }); + + test("registers audit-loaded keys; writeOrDelete empties via deleteRaw", async () => { + await runWithSettingsAudit(async () => { + await writeRaw(CONFIG_KEYS.SQUARE_LOCATION_ID, "audit_one"); + await writeRawBatch([[CONFIG_KEYS.SUMUP_MERCHANT_CODE, "audit_two"]]); + settings.getCachedRaw(CONFIG_KEYS.SQUARE_LOCATION_ID); + settings.getCachedRaw(CONFIG_KEYS.SUMUP_MERCHANT_CODE); + assertSettingsReadsDeclared("test"); + }); + await writeRaw(CONFIG_KEYS.SQUARE_LOCATION_ID, "del_target"); + await writeOrDelete(CONFIG_KEYS.SQUARE_LOCATION_ID, ""); + expect(settings.getCachedRaw(CONFIG_KEYS.SQUARE_LOCATION_ID)).toBeNull(); + }); }); describeWithEnv("deleteRaw", { db: true }, () => { diff --git a/test/ui/templates/admin/settings/custom-domain.test.ts b/test/ui/templates/admin/settings/custom-domain.test.ts index 26ccb08f4e..96e97d0f09 100644 --- a/test/ui/templates/admin/settings/custom-domain.test.ts +++ b/test/ui/templates/admin/settings/custom-domain.test.ts @@ -20,8 +20,8 @@ describe("CustomDomainForm", () => { expect(html).toContain('href="/admin/settings#settings-stripe"'); }); - test("hides the warning when no provider was ever configured", () => { - const html = String( + test("hides the warning when no provider is configured (null or empty string)", () => { + let html = String( CustomDomainForm({ ...advancedDefaultState, bunnyCdnEnabled: true, @@ -30,6 +30,19 @@ describe("CustomDomainForm", () => { }), ); expect(html).not.toContain("Changing your domain"); + expect(html).not.toContain( + 'action="/admin/settings/custom-domain/validate"', + ); + // `"" ?? "stripe"` is "" but `"" || "stripe"` is "stripe" — distinguishes ?? from ||. + html = String( + CustomDomainForm({ + ...advancedDefaultState, + bunnyCdnEnabled: true, + lastActivePaymentProvider: "stripe", + paymentProvider: "", + }), + ); + expect(html).not.toContain("Changing your domain"); }); test("renders all form fields, actions, and hints", () => { @@ -46,6 +59,10 @@ describe("CustomDomainForm", () => { // Form action + id expect(html).toContain('action="/admin/settings/custom-domain"'); expect(html).toContain('id="settings-custom-domain"'); + // Prose container around the heading + expect(html).toContain('class="prose"'); + // Whitespace between the intro sentence and the setup-guide anchor + expect(html).toContain("your tickets site. { // Validate button (visible when customDomain is set) expect(html).toContain('action="/admin/settings/custom-domain/validate"'); expect(html).toContain('id="settings-custom-domain-validate"'); + // Not-yet-validated warning (customDomainLastValidated defaults to "") + expect(html).toContain("not yet validated"); // DNS hint expect(html).toContain("DNS record is in place"); + // CNAME instructions: whitespace separators between strong tags and text + expect(html).toContain(" record:"); + expect(html).toContain("Name: