From 20ec1eea4dfb5c5658d7b739f2c7b06162f48f81 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sun, 3 May 2026 21:39:03 +0200 Subject: [PATCH 1/7] refactor(billing): slim BillingSubscription schema (delegate periodEnd to Stripe API) Drop currentPeriodEnd and cancelAtPeriodEnd from Mongoose model and Zod schema. Add stripeEventId field (tiebreaker for same-second event ordering, V5 P1 #2). Migration 20260503000200 unsets the removed fields from existing documents. --- ...00-slim-subscription-drop-period-fields.js | 31 ++++++++++++ .../billing.subscription.model.mongoose.js | 15 +++--- .../models/billing.subscription.schema.js | 4 +- .../services/billing.refund.service.js | 49 ------------------- 4 files changed, 40 insertions(+), 59 deletions(-) create mode 100644 modules/billing/migrations/20260503000200-slim-subscription-drop-period-fields.js delete mode 100644 modules/billing/services/billing.refund.service.js diff --git a/modules/billing/migrations/20260503000200-slim-subscription-drop-period-fields.js b/modules/billing/migrations/20260503000200-slim-subscription-drop-period-fields.js new file mode 100644 index 000000000..c62acfb69 --- /dev/null +++ b/modules/billing/migrations/20260503000200-slim-subscription-drop-period-fields.js @@ -0,0 +1,31 @@ +/** + * Migration: Slim BillingSubscription — drop currentPeriodEnd and cancelAtPeriodEnd. + * These fields are now fetched on-demand from the Stripe API via fetchSubscriptionDetails(). + * stripeEventId is added for same-second event tiebreaker ordering (V5 P1 #2) — no migration + * needed as it is optional and populated at the next webhook delivery. + * + * Safe to run multiple times (idempotent). + */ +export async function up() { + const { default: mongoose } = await import('mongoose'); + const db = mongoose.connection.db; + + await db.collection('subscriptions').updateMany( + {}, + { + $unset: { + currentPeriodEnd: '', + cancelAtPeriodEnd: '', + }, + }, + ); + + console.info('[migration] slim-subscription-drop-period-fields: dropped currentPeriodEnd + cancelAtPeriodEnd'); +} + +/** + * Down: no-op — dropped fields are re-populated by incoming webhooks. + */ +export function down() { + console.warn('[migration] slim-subscription-drop-period-fields DOWN: no-op; fields will be re-populated by webhooks'); +} diff --git a/modules/billing/models/billing.subscription.model.mongoose.js b/modules/billing/models/billing.subscription.model.mongoose.js index 6b622b44a..4176e0a2e 100644 --- a/modules/billing/models/billing.subscription.model.mongoose.js +++ b/modules/billing/models/billing.subscription.model.mongoose.js @@ -37,13 +37,6 @@ const SubscriptionMongoose = new Schema( enum: config.billing.statuses, default: 'active', }, - currentPeriodEnd: { - type: Date, - }, - cancelAtPeriodEnd: { - type: Boolean, - default: false, - }, // ── Meter fields (sparse — backward-compatible additions) ──────────────── @@ -87,6 +80,14 @@ const SubscriptionMongoose = new Schema( type: Number, default: null, }, + /** + * Stripe event.id of the last processed subscription event. + * Tiebreaker for same-second events (lex string ordering on evt_ IDs). + */ + stripeEventId: { + type: String, + default: null, + }, }, { timestamps: true, diff --git a/modules/billing/models/billing.subscription.schema.js b/modules/billing/models/billing.subscription.schema.js index 0fa35fc71..1c8d5587d 100644 --- a/modules/billing/models/billing.subscription.schema.js +++ b/modules/billing/models/billing.subscription.schema.js @@ -19,20 +19,19 @@ const baseShape = { organization: z.string().trim().regex(objectIdRegex, 'organization must be a valid ObjectId'), stripeCustomerId: optionalStripeId, stripeSubscriptionId: optionalStripeId, - currentPeriodEnd: z.coerce.date().nullable().optional(), // ── Meter fields (optional — backward-compatible) ──────────────────────── planVersion: z.string().trim().optional(), currentPeriodStart: z.coerce.date().nullable().optional(), pastDueSince: z.coerce.date().nullable().optional(), lastResetAt: z.coerce.date().nullable().optional(), stripeEventCreatedAt: z.number().int().nullable().optional(), + stripeEventId: z.string().nullable().optional(), }; const Subscription = z.object({ ...baseShape, plan: z.enum(config.billing.plans).default('free'), status: z.enum(config.billing.statuses).default('active'), - cancelAtPeriodEnd: z.boolean().default(false), }); /** @@ -42,7 +41,6 @@ const SubscriptionCore = z.object({ ...baseShape, plan: z.enum(config.billing.plans), status: z.enum(config.billing.statuses), - cancelAtPeriodEnd: z.boolean(), }); const SubscriptionUpdate = SubscriptionCore.partial(); diff --git a/modules/billing/services/billing.refund.service.js b/modules/billing/services/billing.refund.service.js deleted file mode 100644 index e757a8eb6..000000000 --- a/modules/billing/services/billing.refund.service.js +++ /dev/null @@ -1,49 +0,0 @@ -/** - * Module dependencies - */ -import getStripe from '../lib/stripe.js'; - -/** - * @function refundCharge - * @description Initiate a Stripe refund for a given charge. - * Wraps stripe.refunds.create with a deterministic idempotency key so - * retries on network failures never create duplicate refunds. - * The actual ledger debit happens via the `charge.refunded` webhook - * (single source of truth) — this service ONLY initiates the Stripe refund. - * - * @param {string} stripeChargeId - Stripe charge ID (ch_xxx). Must be non-empty. - * @param {number|undefined} [amountCents] - Amount to refund in cents. Omit for full refund. Must be > 0 if provided. - * @param {Object} [options={}] - Optional Stripe refund options. - * @param {string} [options.reason] - Optional Stripe refund reason. - * @returns {Promise} The Stripe refund object. - */ -// biome-ignore lint/correctness/useQwikValidLexicalScope: false positive — Node.js service, not Qwik -const refundCharge = async (stripeChargeId, amountCents, { reason } = {}) => { - if (typeof stripeChargeId !== 'string' || stripeChargeId.trim() === '') { - throw new Error('invalid argument: stripeChargeId must be a non-empty string'); - } - if (amountCents !== undefined) { - if (!Number.isInteger(amountCents) || amountCents <= 0) { - throw new Error('invalid argument: amountCents must be a positive integer'); - } - } - - const stripe = getStripe(); - if (!stripe) throw new Error('Stripe is not configured'); - - const idempotencyKey = `refund_${stripeChargeId}_${amountCents ?? 'full'}`; - - const params = { - charge: stripeChargeId, - reason: reason || 'requested_by_customer', - }; - if (amountCents !== undefined) { - params.amount = amountCents; - } - - return stripe.refunds.create(params, { idempotencyKey }); -}; - -export default { - refundCharge, -}; From 8f470d13ef4c4d7b0512bfbed5ac03589ecc39ad Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sun, 3 May 2026 21:39:11 +0200 Subject: [PATCH 2/7] refactor(billing): fetchSubscriptionDetails helper + getSubscription merge Stripe details New fetchSubscriptionDetails() fetches currentPeriodEnd, cancelAtPeriodEnd, status, nextRenewalDate from Stripe API on-demand (+50ms on GET /subscription, 0ms on quota gate). getSubscription() merges cached local fields with live Stripe details for UI consumers. Falls back gracefully when Stripe is not configured or subscription is free. --- modules/billing/services/billing.service.js | 40 +++++++++++++++++++-- 1 file changed, 37 insertions(+), 3 deletions(-) diff --git a/modules/billing/services/billing.service.js b/modules/billing/services/billing.service.js index e9cb98bdb..afabceebe 100644 --- a/modules/billing/services/billing.service.js +++ b/modules/billing/services/billing.service.js @@ -244,15 +244,49 @@ const createExtrasCheckout = async (organization, packId, successUrl, cancelUrl) }; /** - * @desc Get subscription for the given organization + * @desc Fetch live subscription details from Stripe API on-demand. + * Used by UI routes that need currentPeriodEnd, cancelAtPeriodEnd etc. + * +50ms latency is acceptable on infrequent "My Subscription" page loads. + * Not cached — callers can layer a short TTL if needed in the future. + * @param {String|null} stripeSubscriptionId - Stripe subscription ID (sub_xxx) + * @returns {Promise} Live fields or null when id is absent / Stripe not configured + */ +// biome-ignore lint/correctness/useQwikValidLexicalScope: false positive — Node.js service, not Qwik +const fetchSubscriptionDetails = async (stripeSubscriptionId) => { + if (!stripeSubscriptionId) return null; + const stripe = getStripe(); + if (!stripe) return null; + const stripeSub = await stripe.subscriptions.retrieve(stripeSubscriptionId); + const cancelAt = stripeSub.cancel_at ? new Date(stripeSub.cancel_at * 1000) : null; + return { + currentPeriodStart: new Date(stripeSub.current_period_start * 1000), + currentPeriodEnd: new Date(stripeSub.current_period_end * 1000), + cancelAtPeriodEnd: stripeSub.cancel_at_period_end, + status: stripeSub.status, + nextRenewalDate: cancelAt ?? new Date(stripeSub.current_period_end * 1000), + }; +}; + +/** + * @desc Get subscription for the given organization. + * Merges cached local fields with live Stripe details for UI consumers. + * Falls back gracefully when Stripe is not configured or the subscription is free. * @param {String} organizationId - The organization ID - * @returns {Promise} The subscription document or null + * @returns {Promise} Merged subscription object or null */ -const getSubscription = async (organizationId) => SubscriptionRepository.findByOrganization(organizationId); +// biome-ignore lint/correctness/useQwikValidLexicalScope: false positive — Node.js service, not Qwik +const getSubscription = async (organizationId) => { + const sub = await SubscriptionRepository.findByOrganization(organizationId); + if (!sub) return null; + const details = await fetchSubscriptionDetails(sub.stripeSubscriptionId).catch(() => null); + if (!details) return sub; + return { ...sub.toJSON?.() ?? sub, ...details }; +}; export default { createCheckout, createExtrasCheckout, createPortalSession, + fetchSubscriptionDetails, getSubscription, }; From 3774e29457c4947e1d370dd373fb9ae4d96044d2 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sun, 3 May 2026 21:39:17 +0200 Subject: [PATCH 3/7] =?UTF-8?q?refactor(billing):=20unified=20webhook=20ha?= =?UTF-8?q?ndlers=20=E2=80=94=20drop=20period=20fields,=20pass=20eventId?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit handleSubscriptionUpdated: drop currentPeriodEnd/cancelAtPeriodEnd from $set fields. All 4 handlers now call updateIfEventNewer(id, created, eventId, fields). handleInvoicePaymentSucceeded: add event param + use updateIfEventNewer (V5 P1 #1). handleSubscriptionDeleted: force meter rotate to free on cancel (V5 P1 #3). --- .../controllers/billing.webhook.controller.js | 2 +- .../services/billing.webhook.service.js | 37 +++++++++++++------ 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/modules/billing/controllers/billing.webhook.controller.js b/modules/billing/controllers/billing.webhook.controller.js index ef45dc5a0..afb9154cf 100644 --- a/modules/billing/controllers/billing.webhook.controller.js +++ b/modules/billing/controllers/billing.webhook.controller.js @@ -54,7 +54,7 @@ const handleWebhook = async (req, res) => { break; case 'invoice.payment_succeeded': await BillingWebhookService.withIdempotency(event, (e) => - BillingWebhookService.handleInvoicePaymentSucceeded(e.data.object), + BillingWebhookService.handleInvoicePaymentSucceeded(e.data.object, e), ); break; case 'charge.refunded': diff --git a/modules/billing/services/billing.webhook.service.js b/modules/billing/services/billing.webhook.service.js index 0d1298006..5a8438c98 100644 --- a/modules/billing/services/billing.webhook.service.js +++ b/modules/billing/services/billing.webhook.service.js @@ -212,12 +212,10 @@ const handleSubscriptionUpdated = async (subscription, event) => { const fields = { plan: newPlan, status: subscription.status, - currentPeriodEnd: new Date(subscription.current_period_end * 1000), - cancelAtPeriodEnd: subscription.cancel_at_period_end, }; if (newPeriodStart) fields.currentPeriodStart = newPeriodStart; - const updated = await SubscriptionRepository.updateIfEventNewer(String(existing._id), event.created, fields); + const updated = await SubscriptionRepository.updateIfEventNewer(String(existing._id), event.created, event.id, fields); if (!updated) { logger.info('[billing.webhook] skipped stale event', { eventId: event.id, type: event.type }); return; @@ -305,7 +303,7 @@ const handleSubscriptionDeleted = async (subscription, event) => { const existing = await SubscriptionRepository.findByStripeSubscriptionId(subscription.id); if (!existing) return; - const updated = await SubscriptionRepository.updateIfEventNewer(String(existing._id), event.created, { + const updated = await SubscriptionRepository.updateIfEventNewer(String(existing._id), event.created, event.id, { plan: 'free', status: 'canceled', }); @@ -314,7 +312,16 @@ const handleSubscriptionDeleted = async (subscription, event) => { return; } - await syncOrganizationPlan(existing.organization?._id || existing.organization, 'free'); + const organizationId = String(existing.organization?._id || existing.organization); + await syncOrganizationPlan(organizationId, 'free'); + + // Force reset meter to free quota — canceled sub must not retain paid-plan snapshot units. + try { + await BillingResetService.forceRotateForPlanChange(organizationId, { preserveUsage: false }); + } catch (err) { + // Log for monitoring — not thrown so webhook processing continues + console.error('[billing.webhook] forceRotateForPlanChange on cancel failed (non-fatal):', err?.message ?? err); + } }; /** @@ -340,7 +347,7 @@ const handleInvoicePaymentFailed = async (invoice, event) => { fields.pastDueSince = new Date(); } - const updated = await SubscriptionRepository.updateIfEventNewer(String(existing._id), event.created, fields); + const updated = await SubscriptionRepository.updateIfEventNewer(String(existing._id), event.created, event.id, fields); if (!updated) { logger.info('[billing.webhook] skipped stale event', { eventId: event.id, type: event.type }); return; @@ -359,11 +366,13 @@ const handleInvoicePaymentFailed = async (invoice, event) => { * @description Handle invoice.payment_succeeded event — clear degraded mode (pastDueSince). * When a past-due invoice is finally paid, remove the pastDueSince marker so * the subscription exits degraded mode on next request. + * Uses updateIfEventNewer to guard against out-of-order webhook delivery (V5 P1 #1). * @param {Object} invoice - Stripe invoice object + * @param {Object} event - Full Stripe event (with event.created and event.id for ordering) * @returns {Promise} */ // biome-ignore lint/correctness/useQwikValidLexicalScope: false positive — Node.js service, not Qwik -const handleInvoicePaymentSucceeded = async (invoice) => { +const handleInvoicePaymentSucceeded = async (invoice, event) => { const { subscription: stripeSubscriptionId } = invoice; if (!stripeSubscriptionId) return; @@ -372,11 +381,15 @@ const handleInvoicePaymentSucceeded = async (invoice) => { // Only clear if currently past_due (avoid unnecessary writes on routine invoices) if (existing.pastDueSince !== null && existing.pastDueSince !== undefined) { - await SubscriptionRepository.update({ - _id: existing._id, - pastDueSince: null, - status: 'active', - }); + const updated = await SubscriptionRepository.updateIfEventNewer( + String(existing._id), + event.created, + event.id, + { pastDueSince: null, status: 'active' }, + ); + if (!updated) { + logger.info('[billing.webhook] skipped stale event', { eventId: event.id, type: event.type }); + } } }; From 4f5f08b9f4bc27ac28801fac17a0729e192ae530 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sun, 3 May 2026 21:39:23 +0200 Subject: [PATCH 4/7] fix(billing): event tiebreaker via eventId for same-second deliveries (V5 P1 #2) updateIfEventNewer now takes eventId as 3rd param. Same-second events are ordered by lex string comparison of evt_ IDs, preventing the 2nd event from being silently skipped when both events have the same Unix second timestamp. Stores stripeEventId alongside stripeEventCreatedAt in the document. --- .../billing.subscription.repository.js | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/modules/billing/repositories/billing.subscription.repository.js b/modules/billing/repositories/billing.subscription.repository.js index 61bfc8461..22ac45c43 100644 --- a/modules/billing/repositories/billing.subscription.repository.js +++ b/modules/billing/repositories/billing.subscription.repository.js @@ -216,21 +216,36 @@ const markUnpaid = (id, threshold) => { * @description Atomically update a subscription only when the incoming Stripe event is newer * than the last processed event. Prevents out-of-order webhook delivery from * overwriting more-recent state. + * Same-second events are ordered by lex-string comparison of event IDs (evt_ prefix + * makes these globally monotonic within a second) — V5 P1 #2 tiebreaker. * Returns null when the guard prevents the write (stale event). * @param {string} id - The subscription ObjectId (string). - * @param {number} eventCreatedAt - Stripe event.created Unix timestamp. + * @param {number} eventCreatedAt - Stripe event.created Unix timestamp (seconds). + * @param {string} eventId - Stripe event.id (e.g. evt_xxx) — tiebreaker for same-second delivery. * @param {Object} fields - Fields to $set on the document. * @returns {Promise} Updated doc, or null if id invalid or event is stale. */ // biome-ignore lint/correctness/useQwikValidLexicalScope: false positive — Node.js repository, not Qwik -const updateIfEventNewer = (id, eventCreatedAt, fields) => { +const updateIfEventNewer = (id, eventCreatedAt, eventId, fields) => { if (!id || !mongoose.Types.ObjectId.isValid(id)) return null; return Subscription.findOneAndUpdate( { _id: id, - $or: [{ stripeEventCreatedAt: null }, { stripeEventCreatedAt: { $lt: eventCreatedAt } }], + $or: [ + { stripeEventCreatedAt: { $exists: false } }, + { stripeEventCreatedAt: null }, + { stripeEventCreatedAt: { $lt: eventCreatedAt } }, + { + stripeEventCreatedAt: eventCreatedAt, + $or: [ + { stripeEventId: { $exists: false } }, + { stripeEventId: null }, + { stripeEventId: { $lt: eventId } }, + ], + }, + ], }, - { $set: { ...fields, stripeEventCreatedAt: eventCreatedAt } }, + { $set: { ...fields, stripeEventCreatedAt: eventCreatedAt, stripeEventId: eventId } }, { returnDocument: 'after', runValidators: true }, ) .populate(defaultPopulate) From 5a280cad52176adf5ceef4396bb7545da25ecb43 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sun, 3 May 2026 21:39:29 +0200 Subject: [PATCH 5/7] =?UTF-8?q?refactor(billing):=20drop=20billing.refund.?= =?UTF-8?q?service=20=E2=80=94=20inline=20Stripe=20refund=20in=20admin=20c?= =?UTF-8?q?ontroller?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit billing.refund.service.js had a single function (refundCharge) used only by the admin controller. Inline it directly to eliminate an unnecessary service indirection layer. Idempotency key pattern and argument validation are preserved unchanged. --- .../controllers/billing.admin.controller.js | 34 +++++++++++++++++-- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/modules/billing/controllers/billing.admin.controller.js b/modules/billing/controllers/billing.admin.controller.js index 416e76c50..75a17860b 100644 --- a/modules/billing/controllers/billing.admin.controller.js +++ b/modules/billing/controllers/billing.admin.controller.js @@ -2,18 +2,46 @@ * Module dependencies */ import responses from '../../../lib/helpers/responses.js'; -import BillingRefundService from '../services/billing.refund.service.js'; +import getStripe from '../lib/stripe.js'; /** - * @desc Admin endpoint to trigger a Stripe refund for a charge + * @desc Admin endpoint to trigger a Stripe refund for a charge. + * Initiates the Stripe refund; actual ledger debit happens via the + * `charge.refunded` webhook (single source of truth). * @param {Object} req - Express request object * @param {Object} res - Express response object * @returns {Promise} */ +// biome-ignore lint/correctness/useQwikValidLexicalScope: false positive — Node.js controller, not Qwik const adminRefundCharge = async (req, res) => { try { const { chargeId, amountCents, reason } = req.body; - const refund = await BillingRefundService.refundCharge(chargeId, amountCents, { reason }); + + if (typeof chargeId !== 'string' || chargeId.trim() === '') { + return responses.error(res, 422, 'Unprocessable Entity', 'Failed to refund charge')( + new Error('invalid argument: chargeId must be a non-empty string'), + ); + } + if (amountCents !== undefined) { + if (!Number.isInteger(amountCents) || amountCents <= 0) { + return responses.error(res, 422, 'Unprocessable Entity', 'Failed to refund charge')( + new Error('invalid argument: amountCents must be a positive integer'), + ); + } + } + + const stripe = getStripe(); + if (!stripe) { + return responses.error(res, 502, 'Bad Gateway', 'Failed to refund charge')( + new Error('Stripe is not configured'), + ); + } + + const idempotencyKey = `refund_${chargeId}_${amountCents ?? 'full'}`; + const params = { charge: chargeId, reason: reason || 'requested_by_customer' }; + if (amountCents !== undefined) params.amount = amountCents; + + const refund = await stripe.refunds.create(params, { idempotencyKey }); responses.success(res, 'billing refund created')(refund); } catch (err) { const status = err.message?.startsWith('invalid argument') ? 422 : 502; From 4ab513d9924e70247ae36b0a71fdb8772fcaac22 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sun, 3 May 2026 21:39:39 +0200 Subject: [PATCH 6/7] =?UTF-8?q?test(billing):=20update=20tests=20for=20PR2?= =?UTF-8?q?=20=E2=80=94=20new=20signatures,=20V5=20P1=20coverage,=20inline?= =?UTF-8?q?=20refund?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - subscription.schema: drop cancelAtPeriodEnd/currentPeriodEnd tests, add stripeEventId - subscription.repository: updateIfEventNewer now 4-arg; add same-second tiebreaker test - webhook.integration: add eventId arg to all updateIfEventNewer assertions; V5 P1 #3 tests - webhook.subscription: updateIfEventNewer 4-arg; handleInvoicePaymentSucceeded with event - billing.service: updateIfEventNewer 4-arg; add forceRotateForPlanChange mock - billing.checkout: fetchSubscriptionDetails tests + updated getSubscription (Stripe merge) - billing.admin.integration: rewritten for inline Stripe (no refund service mock) - billing.refund.service: rewritten as admin controller refund tests --- .../tests/billing.admin.integration.tests.js | 54 ++-- .../tests/billing.checkout.unit.tests.js | 127 ++++++++- .../billing.refund.service.unit.tests.js | 243 +++++++++++------- .../tests/billing.service.unit.tests.js | 14 +- ...ling.subscription.repository.unit.tests.js | 40 ++- modules/billing/tests/billing.unit.tests.js | 22 +- .../billing.webhook.integration.tests.js | 77 +++++- ...billing.webhook.subscription.unit.tests.js | 51 +++- 8 files changed, 475 insertions(+), 153 deletions(-) diff --git a/modules/billing/tests/billing.admin.integration.tests.js b/modules/billing/tests/billing.admin.integration.tests.js index 5503e8a13..a967118f9 100644 --- a/modules/billing/tests/billing.admin.integration.tests.js +++ b/modules/billing/tests/billing.admin.integration.tests.js @@ -5,10 +5,12 @@ import { jest, describe, beforeEach, afterEach, test, expect } from '@jest/globa /** * Integration tests for billing admin endpoints. + * Tests the /api/admin/billing/refund endpoint with the inline refund logic + * (billing.refund.service.js was dropped in PR2; Stripe is called directly). */ describe('Billing admin integration tests:', () => { let billingAdminRoutes; - let mockBillingRefundService; + let mockStripeInstance; /** * Create a lightweight route registry compatible with app.route(). @@ -73,13 +75,15 @@ describe('Billing admin integration tests:', () => { const buildRoutes = async () => { jest.resetModules(); - mockBillingRefundService = { - refundCharge: jest.fn().mockResolvedValue({ - id: 're_test_123', - charge: 'ch_test_123', - amount: 2500, - status: 'succeeded', - }), + mockStripeInstance = { + refunds: { + create: jest.fn().mockResolvedValue({ + id: 're_test_123', + charge: 'ch_test_123', + amount: 2500, + status: 'succeeded', + }), + }, }; jest.unstable_mockModule('../../../config/index.js', () => ({ @@ -88,12 +92,19 @@ describe('Billing admin integration tests:', () => { plans: ['free', 'starter', 'pro'], statuses: ['active', 'canceled'], }, + stripe: { + secretKey: 'sk_test_fake', + }, validation: { supportedMethods: ['post', 'put', 'patch'], }, }, })); + jest.unstable_mockModule('stripe', () => ({ + default: jest.fn(() => mockStripeInstance), + })); + jest.unstable_mockModule('passport', () => ({ default: { authenticate: jest.fn(() => (req, _res, next) => { @@ -136,8 +147,13 @@ describe('Billing admin integration tests:', () => { }, })); - jest.unstable_mockModule('../services/billing.refund.service.js', () => ({ - default: mockBillingRefundService, + jest.unstable_mockModule('../../../lib/helpers/responses.js', () => ({ + default: { + // eslint-disable-next-line no-unused-vars + success: jest.fn((res, msg) => (data) => res.status(200).json({ type: 'success', data })), + // eslint-disable-next-line no-unused-vars + error: jest.fn((res, status, title, desc) => (err) => res.status(status).json({ type: 'error' })), + }, })); billingAdminRoutes = (await import('../routes/billing.admin.routes.js')).default; @@ -186,7 +202,10 @@ describe('Billing admin integration tests:', () => { res, ); - expect(mockBillingRefundService.refundCharge).toHaveBeenCalledWith('ch_test_123', 2500, { reason: 'duplicate' }); + expect(mockStripeInstance.refunds.create).toHaveBeenCalledWith( + { charge: 'ch_test_123', reason: 'duplicate', amount: 2500 }, + { idempotencyKey: 'refund_ch_test_123_2500' }, + ); expect(res.status).toHaveBeenCalledWith(200); }); @@ -238,11 +257,14 @@ describe('Billing admin integration tests:', () => { res, ); - expect(mockBillingRefundService.refundCharge).toHaveBeenCalledWith('ch_test_123', undefined, { reason: 'requested_by_customer' }); + expect(mockStripeInstance.refunds.create).toHaveBeenCalledWith( + { charge: 'ch_test_123', reason: 'requested_by_customer' }, + { idempotencyKey: 'refund_ch_test_123_full' }, + ); expect(res.status).toHaveBeenCalledWith(200); }); - test('refund service upstream error returns 502', async () => { + test('Stripe upstream error returns 502', async () => { const routes = await buildRoutes(); const refundRoute = routes.get('/api/admin/billing/refund'); const res = { @@ -250,7 +272,7 @@ describe('Billing admin integration tests:', () => { json: jest.fn().mockReturnThis(), }; - mockBillingRefundService.refundCharge.mockRejectedValueOnce(new Error('upstream error')); + mockStripeInstance.refunds.create.mockRejectedValueOnce(new Error('upstream error')); await runHandlers( [...refundRoute.all, ...refundRoute.post], @@ -261,7 +283,7 @@ describe('Billing admin integration tests:', () => { expect(res.status).toHaveBeenCalledWith(502); }); - test('refund service invalid argument error returns 422', async () => { + test('Stripe invalid argument error returns 422', async () => { const routes = await buildRoutes(); const refundRoute = routes.get('/api/admin/billing/refund'); const res = { @@ -269,7 +291,7 @@ describe('Billing admin integration tests:', () => { json: jest.fn().mockReturnThis(), }; - mockBillingRefundService.refundCharge.mockRejectedValueOnce(new Error('invalid argument: amountCents must be > 0')); + mockStripeInstance.refunds.create.mockRejectedValueOnce(new Error('invalid argument: amountCents must be > 0')); await runHandlers( [...refundRoute.all, ...refundRoute.post], diff --git a/modules/billing/tests/billing.checkout.unit.tests.js b/modules/billing/tests/billing.checkout.unit.tests.js index b235bbbbd..453733f0c 100644 --- a/modules/billing/tests/billing.checkout.unit.tests.js +++ b/modules/billing/tests/billing.checkout.unit.tests.js @@ -541,12 +541,43 @@ describe('Billing service unit tests:', () => { }); describe('getSubscription', () => { - test('should return subscription for organization', async () => { + test('should return subscription merged with Stripe details when stripeSubscriptionId exists', async () => { jest.unstable_mockModule('../../../config/index.js', () => ({ default: { stripe: { secretKey: 'sk_test_sub1' } }, })); - const mockSub = { organization: orgId, plan: 'pro' }; + const periodEnd = Math.floor(Date.now() / 1000) + 86400; + mockStripeInstance.subscriptions = { + retrieve: jest.fn().mockResolvedValue({ + current_period_start: periodEnd - 2592000, + current_period_end: periodEnd, + cancel_at_period_end: false, + status: 'active', + cancel_at: null, + }), + }; + + const mockSub = { organization: orgId, plan: 'pro', stripeSubscriptionId: 'sub_456', toJSON: () => ({ organization: orgId, plan: 'pro', stripeSubscriptionId: 'sub_456' }) }; + mockSubscriptionRepository.findByOrganization.mockResolvedValue(mockSub); + + const mod = await import('../services/billing.service.js'); + BillingService = mod.default; + + const result = await BillingService.getSubscription(orgId); + + expect(result).toMatchObject({ plan: 'pro', cancelAtPeriodEnd: false, status: 'active' }); + expect(result.currentPeriodEnd).toBeInstanceOf(Date); + expect(mockStripeInstance.subscriptions.retrieve).toHaveBeenCalledWith('sub_456'); + }); + + test('should return cached sub without Stripe fetch when no stripeSubscriptionId (free plan)', async () => { + jest.unstable_mockModule('../../../config/index.js', () => ({ + default: { stripe: { secretKey: 'sk_test_sub_free' } }, + })); + + mockStripeInstance.subscriptions = { retrieve: jest.fn() }; + + const mockSub = { organization: orgId, plan: 'free', stripeSubscriptionId: null }; mockSubscriptionRepository.findByOrganization.mockResolvedValue(mockSub); const mod = await import('../services/billing.service.js'); @@ -555,7 +586,7 @@ describe('Billing service unit tests:', () => { const result = await BillingService.getSubscription(orgId); expect(result).toEqual(mockSub); - expect(mockSubscriptionRepository.findByOrganization).toHaveBeenCalledWith(orgId); + expect(mockStripeInstance.subscriptions.retrieve).not.toHaveBeenCalled(); }); test('should return null when no subscription exists', async () => { @@ -572,5 +603,95 @@ describe('Billing service unit tests:', () => { expect(result).toBeNull(); }); + + test('should return cached sub when Stripe fetch throws (graceful fallback)', async () => { + jest.unstable_mockModule('../../../config/index.js', () => ({ + default: { stripe: { secretKey: 'sk_test_sub_fallback' } }, + })); + + mockStripeInstance.subscriptions = { + retrieve: jest.fn().mockRejectedValue(new Error('Stripe unreachable')), + }; + + const mockSub = { organization: orgId, plan: 'pro', stripeSubscriptionId: 'sub_789', toJSON: () => ({ organization: orgId, plan: 'pro' }) }; + mockSubscriptionRepository.findByOrganization.mockResolvedValue(mockSub); + + const mod = await import('../services/billing.service.js'); + BillingService = mod.default; + + const result = await BillingService.getSubscription(orgId); + + expect(result).toEqual(mockSub); + }); + }); + + describe('fetchSubscriptionDetails', () => { + test('should return mapped Stripe fields', async () => { + jest.unstable_mockModule('../../../config/index.js', () => ({ + default: { stripe: { secretKey: 'sk_test_fetch1' } }, + })); + + const periodEnd = Math.floor(Date.now() / 1000) + 86400; + mockStripeInstance.subscriptions = { + retrieve: jest.fn().mockResolvedValue({ + current_period_start: periodEnd - 2592000, + current_period_end: periodEnd, + cancel_at_period_end: true, + status: 'active', + cancel_at: null, + }), + }; + + const mod = await import('../services/billing.service.js'); + BillingService = mod.default; + + const result = await BillingService.fetchSubscriptionDetails('sub_test'); + + expect(result).toMatchObject({ + cancelAtPeriodEnd: true, + status: 'active', + }); + expect(result.currentPeriodEnd).toBeInstanceOf(Date); + expect(result.currentPeriodStart).toBeInstanceOf(Date); + expect(result.nextRenewalDate).toBeInstanceOf(Date); + }); + + test('should return null when stripeSubscriptionId is null', async () => { + jest.unstable_mockModule('../../../config/index.js', () => ({ + default: { stripe: { secretKey: 'sk_test_fetch2' } }, + })); + + const mod = await import('../services/billing.service.js'); + BillingService = mod.default; + + const result = await BillingService.fetchSubscriptionDetails(null); + + expect(result).toBeNull(); + }); + + test('should use cancel_at as nextRenewalDate when set', async () => { + jest.unstable_mockModule('../../../config/index.js', () => ({ + default: { stripe: { secretKey: 'sk_test_fetch3' } }, + })); + + const periodEnd = Math.floor(Date.now() / 1000) + 86400; + const cancelAt = periodEnd + 7200; + mockStripeInstance.subscriptions = { + retrieve: jest.fn().mockResolvedValue({ + current_period_start: periodEnd - 2592000, + current_period_end: periodEnd, + cancel_at_period_end: true, + status: 'active', + cancel_at: cancelAt, + }), + }; + + const mod = await import('../services/billing.service.js'); + BillingService = mod.default; + + const result = await BillingService.fetchSubscriptionDetails('sub_cancel'); + + expect(result.nextRenewalDate).toEqual(new Date(cancelAt * 1000)); + }); }); }); diff --git a/modules/billing/tests/billing.refund.service.unit.tests.js b/modules/billing/tests/billing.refund.service.unit.tests.js index 1b7194b13..2da5f8f5c 100644 --- a/modules/billing/tests/billing.refund.service.unit.tests.js +++ b/modules/billing/tests/billing.refund.service.unit.tests.js @@ -4,12 +4,22 @@ import { jest, describe, test, beforeEach, afterEach, expect } from '@jest/globals'; /** - * Unit tests for billing.refund.service.js + * Unit tests for admin refund logic (inlined in billing.admin.controller.js after + * billing.refund.service.js was dropped in PR2 simplification). + * Validates that the controller correctly calls stripe.refunds.create with the + * right params and idempotency key, and handles error cases properly. */ -describe('BillingRefundService unit tests:', () => { - let BillingRefundService; +describe('Admin refund controller unit tests:', () => { + let adminRefundCharge; let mockStripeInstance; let mockGetStripe; + let mockResponses; + + const makeRes = () => { + const res = { status: jest.fn(), json: jest.fn() }; + res.status.mockReturnValue(res); + return res; + }; beforeEach(async () => { jest.resetModules(); @@ -26,108 +36,149 @@ describe('BillingRefundService unit tests:', () => { mockGetStripe = jest.fn(() => mockStripeInstance); + mockResponses = { + success: jest.fn(() => jest.fn()), + error: jest.fn(() => jest.fn()), + }; + jest.unstable_mockModule('../lib/stripe.js', () => ({ default: mockGetStripe, })); - const mod = await import('../services/billing.refund.service.js'); - BillingRefundService = mod.default; + jest.unstable_mockModule('../../../lib/helpers/responses.js', () => ({ + default: mockResponses, + })); + + const mod = await import('../controllers/billing.admin.controller.js'); + adminRefundCharge = mod.default.adminRefundCharge; }); afterEach(() => { jest.restoreAllMocks(); }); - describe('refundCharge', () => { - test('should call stripe.refunds.create with charge and reason', async () => { - const result = await BillingRefundService.refundCharge('ch_test_xyz'); - - expect(mockStripeInstance.refunds.create).toHaveBeenCalledWith( - { charge: 'ch_test_xyz', reason: 'requested_by_customer' }, - { idempotencyKey: 'refund_ch_test_xyz_full' }, - ); - expect(result.id).toBe('re_test_abc'); - }); - - test('should include amount when amountCents is provided', async () => { - await BillingRefundService.refundCharge('ch_test_xyz', 2000); - - expect(mockStripeInstance.refunds.create).toHaveBeenCalledWith( - { charge: 'ch_test_xyz', reason: 'requested_by_customer', amount: 2000 }, - { idempotencyKey: 'refund_ch_test_xyz_2000' }, - ); - }); - - test('should forward an explicit reason when provided', async () => { - await BillingRefundService.refundCharge('ch_test_xyz', 2000, { reason: 'duplicate' }); - - expect(mockStripeInstance.refunds.create).toHaveBeenCalledWith( - { charge: 'ch_test_xyz', reason: 'duplicate', amount: 2000 }, - { idempotencyKey: 'refund_ch_test_xyz_2000' }, - ); - }); - - test('idempotency key is "refund_{chargeId}_full" for full refund', async () => { - await BillingRefundService.refundCharge('ch_abc'); - - const call = mockStripeInstance.refunds.create.mock.calls[0]; - expect(call[1].idempotencyKey).toBe('refund_ch_abc_full'); - }); - - test('idempotency key is "refund_{chargeId}_{amountCents}" for partial refund', async () => { - await BillingRefundService.refundCharge('ch_abc', 500); - - const call = mockStripeInstance.refunds.create.mock.calls[0]; - expect(call[1].idempotencyKey).toBe('refund_ch_abc_500'); - }); - - test('different amountCents produces different idempotency keys', async () => { - await BillingRefundService.refundCharge('ch_same', 1000); - await BillingRefundService.refundCharge('ch_same', 2000); - - const keys = mockStripeInstance.refunds.create.mock.calls.map((c) => c[1].idempotencyKey); - expect(keys[0]).toBe('refund_ch_same_1000'); - expect(keys[1]).toBe('refund_ch_same_2000'); - }); - - test('should throw when Stripe is not configured', async () => { - mockGetStripe.mockReturnValue(null); - - await expect(BillingRefundService.refundCharge('ch_test')).rejects.toThrow('Stripe is not configured'); - }); - - test('should throw for empty stripeChargeId', async () => { - await expect(BillingRefundService.refundCharge('')).rejects.toThrow( - 'invalid argument: stripeChargeId must be a non-empty string', - ); - }); - - test('should throw for non-string stripeChargeId', async () => { - await expect(BillingRefundService.refundCharge(null)).rejects.toThrow( - 'invalid argument: stripeChargeId must be a non-empty string', - ); - }); - - test('should throw for zero amountCents', async () => { - await expect(BillingRefundService.refundCharge('ch_test', 0)).rejects.toThrow( - 'invalid argument: amountCents must be a positive integer', - ); - }); - - test('should throw for negative amountCents', async () => { - await expect(BillingRefundService.refundCharge('ch_test', -100)).rejects.toThrow( - 'invalid argument: amountCents must be a positive integer', - ); - }); - - test('should throw for non-integer amountCents', async () => { - await expect(BillingRefundService.refundCharge('ch_test', 9.99)).rejects.toThrow( - 'invalid argument: amountCents must be a positive integer', - ); - }); - - test('should accept undefined amountCents (full refund)', async () => { - await expect(BillingRefundService.refundCharge('ch_test', undefined)).resolves.toBeDefined(); - }); + test('should call stripe.refunds.create with charge and default reason', async () => { + const req = { body: { chargeId: 'ch_test_xyz' } }; + const res = makeRes(); + + await adminRefundCharge(req, res); + + expect(mockStripeInstance.refunds.create).toHaveBeenCalledWith( + { charge: 'ch_test_xyz', reason: 'requested_by_customer' }, + { idempotencyKey: 'refund_ch_test_xyz_full' }, + ); + expect(mockResponses.success).toHaveBeenCalledWith(res, 'billing refund created'); + }); + + test('should include amount when amountCents is provided', async () => { + const req = { body: { chargeId: 'ch_test_xyz', amountCents: 2000 } }; + const res = makeRes(); + + await adminRefundCharge(req, res); + + expect(mockStripeInstance.refunds.create).toHaveBeenCalledWith( + { charge: 'ch_test_xyz', reason: 'requested_by_customer', amount: 2000 }, + { idempotencyKey: 'refund_ch_test_xyz_2000' }, + ); + }); + + test('should forward an explicit reason when provided', async () => { + const req = { body: { chargeId: 'ch_test_xyz', amountCents: 2000, reason: 'duplicate' } }; + const res = makeRes(); + + await adminRefundCharge(req, res); + + expect(mockStripeInstance.refunds.create).toHaveBeenCalledWith( + { charge: 'ch_test_xyz', reason: 'duplicate', amount: 2000 }, + { idempotencyKey: 'refund_ch_test_xyz_2000' }, + ); + }); + + test('idempotency key is "refund_{chargeId}_full" for full refund', async () => { + const req = { body: { chargeId: 'ch_abc' } }; + const res = makeRes(); + + await adminRefundCharge(req, res); + + const call = mockStripeInstance.refunds.create.mock.calls[0]; + expect(call[1].idempotencyKey).toBe('refund_ch_abc_full'); + }); + + test('idempotency key is "refund_{chargeId}_{amountCents}" for partial refund', async () => { + const req = { body: { chargeId: 'ch_abc', amountCents: 500 } }; + const res = makeRes(); + + await adminRefundCharge(req, res); + + const call = mockStripeInstance.refunds.create.mock.calls[0]; + expect(call[1].idempotencyKey).toBe('refund_ch_abc_500'); + }); + + test('should return 502 when Stripe is not configured', async () => { + mockGetStripe.mockReturnValue(null); + const req = { body: { chargeId: 'ch_test' } }; + const res = makeRes(); + + await adminRefundCharge(req, res); + + expect(mockResponses.error).toHaveBeenCalledWith(res, 502, 'Bad Gateway', 'Failed to refund charge'); + }); + + test('should return 422 for empty chargeId', async () => { + const req = { body: { chargeId: '' } }; + const res = makeRes(); + + await adminRefundCharge(req, res); + + expect(mockResponses.error).toHaveBeenCalledWith(res, 422, 'Unprocessable Entity', 'Failed to refund charge'); + expect(mockStripeInstance.refunds.create).not.toHaveBeenCalled(); + }); + + test('should return 422 for non-string chargeId', async () => { + const req = { body: { chargeId: null } }; + const res = makeRes(); + + await adminRefundCharge(req, res); + + expect(mockResponses.error).toHaveBeenCalledWith(res, 422, 'Unprocessable Entity', 'Failed to refund charge'); + expect(mockStripeInstance.refunds.create).not.toHaveBeenCalled(); + }); + + test('should return 422 for zero amountCents', async () => { + const req = { body: { chargeId: 'ch_test', amountCents: 0 } }; + const res = makeRes(); + + await adminRefundCharge(req, res); + + expect(mockResponses.error).toHaveBeenCalledWith(res, 422, 'Unprocessable Entity', 'Failed to refund charge'); + expect(mockStripeInstance.refunds.create).not.toHaveBeenCalled(); + }); + + test('should return 422 for negative amountCents', async () => { + const req = { body: { chargeId: 'ch_test', amountCents: -100 } }; + const res = makeRes(); + + await adminRefundCharge(req, res); + + expect(mockResponses.error).toHaveBeenCalledWith(res, 422, 'Unprocessable Entity', 'Failed to refund charge'); + }); + + test('should return 422 for non-integer amountCents', async () => { + const req = { body: { chargeId: 'ch_test', amountCents: 9.99 } }; + const res = makeRes(); + + await adminRefundCharge(req, res); + + expect(mockResponses.error).toHaveBeenCalledWith(res, 422, 'Unprocessable Entity', 'Failed to refund charge'); + }); + + test('should succeed with undefined amountCents (full refund)', async () => { + const req = { body: { chargeId: 'ch_test', amountCents: undefined } }; + const res = makeRes(); + + await adminRefundCharge(req, res); + + expect(mockStripeInstance.refunds.create).toHaveBeenCalled(); + expect(mockResponses.success).toHaveBeenCalled(); }); }); diff --git a/modules/billing/tests/billing.service.unit.tests.js b/modules/billing/tests/billing.service.unit.tests.js index d02d58a1a..d59c17dda 100644 --- a/modules/billing/tests/billing.service.unit.tests.js +++ b/modules/billing/tests/billing.service.unit.tests.js @@ -50,7 +50,7 @@ describe('Billing webhook service unit tests:', () => { })); jest.unstable_mockModule('../services/billing.reset.service.js', () => ({ - default: { resetWeek: jest.fn(), resetAllDue: jest.fn() }, + default: { resetWeek: jest.fn(), resetAllDue: jest.fn(), forceRotateForPlanChange: jest.fn().mockResolvedValue({}) }, })); jest.unstable_mockModule('../lib/events.js', () => ({ @@ -202,7 +202,8 @@ describe('Billing webhook service unit tests:', () => { expect(mockSubscriptionRepository.updateIfEventNewer).toHaveBeenCalledWith( subId, 1700000100, - expect.objectContaining({ plan: 'pro', status: 'active', cancelAtPeriodEnd: false }), + 'evt_1', + expect.objectContaining({ plan: 'pro', status: 'active' }), ); expect(mockOrganizationRepository.setPlan).toHaveBeenCalledWith(orgId, 'pro'); }); @@ -239,6 +240,7 @@ describe('Billing webhook service unit tests:', () => { expect(mockSubscriptionRepository.updateIfEventNewer).toHaveBeenCalledWith( subId, 1700000100, + 'evt_1', expect.objectContaining({ plan: 'starter' }), ); }); @@ -264,6 +266,7 @@ describe('Billing webhook service unit tests:', () => { expect(mockSubscriptionRepository.updateIfEventNewer).toHaveBeenCalledWith( subId, 1700000100, + 'evt_1', expect.objectContaining({ plan: 'free' }), ); }); @@ -272,21 +275,25 @@ describe('Billing webhook service unit tests:', () => { describe('handleSubscriptionDeleted', () => { const makeEvent = (overrides = {}) => ({ id: 'evt_del', created: 1700000200, ...overrides }); - test('should cancel subscription and reset to free', async () => { + test('should cancel subscription, reset to free, and rotate meter', async () => { const existing = { _id: subId, organization: { _id: orgId } }; mockSubscriptionRepository.findByStripeSubscriptionId.mockResolvedValue(existing); const mod = await import('../services/billing.webhook.service.js'); BillingWebhookService = mod.default; + const resetService = (await import('../services/billing.reset.service.js')).default; + await BillingWebhookService.handleSubscriptionDeleted({ id: 'sub_456' }, makeEvent()); expect(mockSubscriptionRepository.updateIfEventNewer).toHaveBeenCalledWith( subId, 1700000200, + 'evt_del', { plan: 'free', status: 'canceled' }, ); expect(mockOrganizationRepository.setPlan).toHaveBeenCalledWith(orgId, 'free'); + expect(resetService.forceRotateForPlanChange).toHaveBeenCalledWith(orgId, { preserveUsage: false }); }); test('should return early when subscription not found', async () => { @@ -316,6 +323,7 @@ describe('Billing webhook service unit tests:', () => { expect(mockSubscriptionRepository.updateIfEventNewer).toHaveBeenCalledWith( subId, 1700000300, + 'evt_fail', expect.objectContaining({ status: 'past_due', pastDueSince: expect.any(Date) }), ); }); diff --git a/modules/billing/tests/billing.subscription.repository.unit.tests.js b/modules/billing/tests/billing.subscription.repository.unit.tests.js index 7edf04e71..bb54c80c2 100644 --- a/modules/billing/tests/billing.subscription.repository.unit.tests.js +++ b/modules/billing/tests/billing.subscription.repository.unit.tests.js @@ -157,15 +157,30 @@ describe('BillingSubscriptionRepository unit tests:', () => { describe('updateIfEventNewer', () => { test('updates when stripeEventCreatedAt is null (first event)', async () => { - const updated = { _id: subId, status: 'canceled', stripeEventCreatedAt: 1000 }; + const updated = { _id: subId, status: 'canceled', stripeEventCreatedAt: 1000, stripeEventId: 'evt_abc' }; const populateMock = jest.fn().mockReturnValue({ exec: jest.fn().mockResolvedValue(updated) }); mockModel.findOneAndUpdate.mockReturnValue({ populate: populateMock }); - const result = await BillingSubscriptionRepository.updateIfEventNewer(subId, 1000, { status: 'canceled' }); + const result = await BillingSubscriptionRepository.updateIfEventNewer(subId, 1000, 'evt_abc', { status: 'canceled' }); expect(mockModel.findOneAndUpdate).toHaveBeenCalledWith( - { _id: subId, $or: [{ stripeEventCreatedAt: null }, { stripeEventCreatedAt: { $lt: 1000 } }] }, - { $set: { status: 'canceled', stripeEventCreatedAt: 1000 } }, + { + _id: subId, + $or: [ + { stripeEventCreatedAt: { $exists: false } }, + { stripeEventCreatedAt: null }, + { stripeEventCreatedAt: { $lt: 1000 } }, + { + stripeEventCreatedAt: 1000, + $or: [ + { stripeEventId: { $exists: false } }, + { stripeEventId: null }, + { stripeEventId: { $lt: 'evt_abc' } }, + ], + }, + ], + }, + { $set: { status: 'canceled', stripeEventCreatedAt: 1000, stripeEventId: 'evt_abc' } }, { returnDocument: 'after', runValidators: true }, ); expect(result).toEqual(updated); @@ -175,16 +190,29 @@ describe('BillingSubscriptionRepository unit tests:', () => { const populateMock = jest.fn().mockReturnValue({ exec: jest.fn().mockResolvedValue(null) }); mockModel.findOneAndUpdate.mockReturnValue({ populate: populateMock }); - const result = await BillingSubscriptionRepository.updateIfEventNewer(subId, 500, { status: 'canceled' }); + const result = await BillingSubscriptionRepository.updateIfEventNewer(subId, 500, 'evt_old', { status: 'canceled' }); expect(result).toBeNull(); }); test('returns null for invalid id', async () => { - const result = await BillingSubscriptionRepository.updateIfEventNewer('not-valid', 1000, {}); + const result = await BillingSubscriptionRepository.updateIfEventNewer('not-valid', 1000, 'evt_abc', {}); expect(result).toBeNull(); expect(mockModel.findOneAndUpdate).not.toHaveBeenCalled(); }); + + test('same-second events: lex-earlier eventId is rejected (tiebreaker guard)', async () => { + const populateMock = jest.fn().mockReturnValue({ exec: jest.fn().mockResolvedValue(null) }); + mockModel.findOneAndUpdate.mockReturnValue({ populate: populateMock }); + + const result = await BillingSubscriptionRepository.updateIfEventNewer(subId, 1000, 'evt_aaaa', { status: 'active' }); + + expect(result).toBeNull(); + const filter = mockModel.findOneAndUpdate.mock.calls[0][0]; + const sameSecondBranch = filter.$or[3]; + expect(sameSecondBranch.stripeEventCreatedAt).toBe(1000); + expect(sameSecondBranch.$or[2].stripeEventId.$lt).toBe('evt_aaaa'); + }); }); // ── findPlan ────────────────────────────────────────────────────────────── diff --git a/modules/billing/tests/billing.unit.tests.js b/modules/billing/tests/billing.unit.tests.js index c468436e0..d2c2d023d 100644 --- a/modules/billing/tests/billing.unit.tests.js +++ b/modules/billing/tests/billing.unit.tests.js @@ -38,13 +38,10 @@ describe('Billing unit tests:', () => { test('should be valid with all optional fields', (done) => { subscription.stripeCustomerId = 'cus_123'; subscription.stripeSubscriptionId = 'sub_456'; - subscription.currentPeriodEnd = '2026-12-31T00:00:00.000Z'; - subscription.cancelAtPeriodEnd = true; const result = schema.Subscription.safeParse(subscription); expect(typeof result).toBe('object'); expect(result.error).toBeFalsy(); - expect(result.data.cancelAtPeriodEnd).toBe(true); done(); }); @@ -100,10 +97,10 @@ describe('Billing unit tests:', () => { done(); }); - test('should default cancelAtPeriodEnd to false', (done) => { + test('should not expose cancelAtPeriodEnd (delegated to Stripe API)', (done) => { const result = schema.Subscription.safeParse(subscription); expect(result.error).toBeFalsy(); - expect(result.data.cancelAtPeriodEnd).toBe(false); + expect(result.data.cancelAtPeriodEnd).toBeUndefined(); done(); }); @@ -181,7 +178,6 @@ describe('Billing unit tests:', () => { expect(result.error).toBeFalsy(); expect(result.data.plan).toBe('pro'); expect(result.data.status).toBeUndefined(); - expect(result.data.cancelAtPeriodEnd).toBeUndefined(); done(); }); }); @@ -243,6 +239,20 @@ describe('Billing unit tests:', () => { expect(result.error).toBeFalsy(); expect(result.data.planVersion).toBe('v3'); }); + + test('should accept stripeEventId as optional string', () => { + subscription.stripeEventId = 'evt_1AbCdEf'; + const result = schema.Subscription.safeParse(subscription); + expect(result.error).toBeFalsy(); + expect(result.data.stripeEventId).toBe('evt_1AbCdEf'); + }); + + test('should accept stripeEventId as null', () => { + subscription.stripeEventId = null; + const result = schema.Subscription.safeParse(subscription); + expect(result.error).toBeFalsy(); + expect(result.data.stripeEventId).toBeNull(); + }); }); describe('ExtrasCheckoutRequest schema', () => { diff --git a/modules/billing/tests/billing.webhook.integration.tests.js b/modules/billing/tests/billing.webhook.integration.tests.js index c3cfe8451..d431ef2ef 100644 --- a/modules/billing/tests/billing.webhook.integration.tests.js +++ b/modules/billing/tests/billing.webhook.integration.tests.js @@ -10,6 +10,7 @@ describe('Billing webhook integration tests:', () => { let WebhookService; let mockSubscriptionRepository; let mockOrganizationRepository; + let mockResetService; const orgId = '507f1f77bcf86cd799439011'; const subId = '607f1f77bcf86cd799439022'; @@ -30,6 +31,11 @@ describe('Billing webhook integration tests:', () => { setPlan: jest.fn().mockResolvedValue({}), }; + mockResetService = { + resetWeek: jest.fn().mockResolvedValue({}), + forceRotateForPlanChange: jest.fn().mockResolvedValue({}), + }; + jest.unstable_mockModule('../repositories/billing.subscription.repository.js', () => ({ default: mockSubscriptionRepository, })); @@ -53,6 +59,14 @@ describe('Billing webhook integration tests:', () => { }, })); + jest.unstable_mockModule('../services/billing.reset.service.js', () => ({ + default: mockResetService, + })); + + jest.unstable_mockModule('../services/billing.extra.service.js', () => ({ + default: { creditPack: jest.fn(), refundPartial: jest.fn() }, + })); + jest.unstable_mockModule('../lib/events.js', () => ({ default: { emit: jest.fn() }, })); @@ -180,17 +194,15 @@ describe('Billing webhook integration tests:', () => { ...overrides, }); - test('should update plan, status, currentPeriodEnd, cancelAtPeriodEnd via updateIfEventNewer', async () => { + test('should update plan and status via updateIfEventNewer (period details fetched on-demand)', async () => { const existing = { _id: subId, organization: orgId }; mockSubscriptionRepository.findByStripeSubscriptionId.mockResolvedValue(existing); - const periodEnd = Math.floor(Date.now() / 1000) + 86400; - await WebhookService.handleSubscriptionUpdated( { id: 'sub_456', status: 'active', - current_period_end: periodEnd, + current_period_end: Math.floor(Date.now() / 1000) + 86400, cancel_at_period_end: true, items: { data: [{ price: { metadata: { planId: 'pro' } } }] }, }, @@ -200,12 +212,8 @@ describe('Billing webhook integration tests:', () => { expect(mockSubscriptionRepository.updateIfEventNewer).toHaveBeenCalledWith( subId, 1700000100, - expect.objectContaining({ - plan: 'pro', - status: 'active', - currentPeriodEnd: new Date(periodEnd * 1000), - cancelAtPeriodEnd: true, - }), + 'evt_updated', + expect.objectContaining({ plan: 'pro', status: 'active' }), ); expect(mockOrganizationRepository.setPlan).toHaveBeenCalledWith(orgId, 'pro'); }); @@ -258,6 +266,7 @@ describe('Billing webhook integration tests:', () => { expect(mockSubscriptionRepository.updateIfEventNewer).toHaveBeenCalledWith( subId, expect.any(Number), + expect.any(String), expect.objectContaining({ plan: 'free' }), ); }); @@ -275,6 +284,7 @@ describe('Billing webhook integration tests:', () => { expect(mockSubscriptionRepository.updateIfEventNewer).toHaveBeenCalledWith( subId, 1700000200, + 'evt_deleted', { plan: 'free', status: 'canceled' }, ); expect(mockOrganizationRepository.setPlan).toHaveBeenCalledWith(orgId, 'free'); @@ -297,6 +307,27 @@ describe('Billing webhook integration tests:', () => { expect(mockOrganizationRepository.setPlan).not.toHaveBeenCalled(); }); + + test('V5 P1 #3: should force rotate meter to free quota on cancellation', async () => { + const existing = { _id: subId, organization: orgId }; + mockSubscriptionRepository.findByStripeSubscriptionId.mockResolvedValue(existing); + + await WebhookService.handleSubscriptionDeleted({ id: 'sub_456' }, makeEvent()); + + expect(mockResetService.forceRotateForPlanChange).toHaveBeenCalledWith(orgId, { preserveUsage: false }); + }); + + test('V5 P1 #3: should not throw when forceRotateForPlanChange fails on cancel (non-fatal)', async () => { + const existing = { _id: subId, organization: orgId }; + mockSubscriptionRepository.findByStripeSubscriptionId.mockResolvedValue(existing); + mockResetService.forceRotateForPlanChange.mockRejectedValue(new Error('db error')); + + await expect( + WebhookService.handleSubscriptionDeleted({ id: 'sub_456' }, makeEvent()), + ).resolves.not.toThrow(); + + expect(mockOrganizationRepository.setPlan).toHaveBeenCalledWith(orgId, 'free'); + }); }); describe('handleInvoicePaymentFailed', () => { @@ -311,6 +342,7 @@ describe('Billing webhook integration tests:', () => { expect(mockSubscriptionRepository.updateIfEventNewer).toHaveBeenCalledWith( subId, 1700000300, + 'evt_failed', expect.objectContaining({ status: 'past_due' }), ); }); @@ -351,7 +383,7 @@ describe('Billing webhook integration tests:', () => { { id: 'sub_456', status: 'active', current_period_end: 9999999999, cancel_at_period_end: false, items: { data: [] } }, { id: 'evt_1', created: 10, data: {} }, ); - expect(mockSubscriptionRepository.updateIfEventNewer).toHaveBeenCalledWith(subId, 10, expect.any(Object)); + expect(mockSubscriptionRepository.updateIfEventNewer).toHaveBeenCalledWith(subId, 10, 'evt_1', expect.any(Object)); // Second call (t=5, older) — repository returns null (guard rejected) mockSubscriptionRepository.updateIfEventNewer.mockResolvedValueOnce(null); @@ -363,5 +395,28 @@ describe('Billing webhook integration tests:', () => { // org plan was synced only once (for the first event) expect(mockOrganizationRepository.setPlan).toHaveBeenCalledTimes(1); }); + + test('same-second events: lex-later event ID wins (V5 P1 #2 tiebreaker)', async () => { + const existing = { _id: subId, organization: orgId }; + mockSubscriptionRepository.findByStripeSubscriptionId.mockResolvedValue(existing); + + // First event at t=1000 succeeds + mockSubscriptionRepository.updateIfEventNewer.mockResolvedValueOnce({ _id: subId, stripeEventCreatedAt: 1000, stripeEventId: 'evt_aaaa' }); + await WebhookService.handleSubscriptionUpdated( + { id: 'sub_456', status: 'active', current_period_end: 9999999999, cancel_at_period_end: false, items: { data: [] } }, + { id: 'evt_aaaa', created: 1000, data: {} }, + ); + + // Second event at same t=1000 with lex-earlier ID — repository returns null (tiebreaker rejected) + mockSubscriptionRepository.updateIfEventNewer.mockResolvedValueOnce(null); + await WebhookService.handleSubscriptionUpdated( + { id: 'sub_456', status: 'past_due', current_period_end: 9999999999, cancel_at_period_end: false, items: { data: [] } }, + { id: 'evt_9999', created: 1000, data: {} }, + ); + + expect(mockSubscriptionRepository.updateIfEventNewer).toHaveBeenNthCalledWith(1, subId, 1000, 'evt_aaaa', expect.any(Object)); + expect(mockSubscriptionRepository.updateIfEventNewer).toHaveBeenNthCalledWith(2, subId, 1000, 'evt_9999', expect.any(Object)); + expect(mockOrganizationRepository.setPlan).toHaveBeenCalledTimes(1); + }); }); }); diff --git a/modules/billing/tests/billing.webhook.subscription.unit.tests.js b/modules/billing/tests/billing.webhook.subscription.unit.tests.js index 8c93c0839..5dff6575b 100644 --- a/modules/billing/tests/billing.webhook.subscription.unit.tests.js +++ b/modules/billing/tests/billing.webhook.subscription.unit.tests.js @@ -423,13 +423,16 @@ describe('Billing webhook subscription unit tests:', () => { expect(mockSubscriptionRepository.updateIfEventNewer).toHaveBeenCalledWith( subId, 1700000100, + 'evt_13', expect.objectContaining({ currentPeriodStart: new Date(newPeriodStart * 1000) }), ); }); }); describe('handleInvoicePaymentSucceeded', () => { - test('should clear pastDueSince and restore active status when subscription is past_due', async () => { + const makeEvent = (overrides = {}) => ({ id: 'evt_succeeded', created: 1700000400, ...overrides }); + + test('should clear pastDueSince and restore active status via updateIfEventNewer', async () => { const existing = { _id: subId, organization: orgId, @@ -437,12 +440,14 @@ describe('Billing webhook subscription unit tests:', () => { status: 'past_due', }; mockSubscriptionRepository.findByStripeSubscriptionId.mockResolvedValue(existing); - mockSubscriptionRepository.update.mockResolvedValue({}); - await BillingWebhookService.handleInvoicePaymentSucceeded({ subscription: 'sub_456' }); + await BillingWebhookService.handleInvoicePaymentSucceeded({ subscription: 'sub_456' }, makeEvent()); - expect(mockSubscriptionRepository.update).toHaveBeenCalledWith( - expect.objectContaining({ _id: subId, pastDueSince: null, status: 'active' }), + expect(mockSubscriptionRepository.updateIfEventNewer).toHaveBeenCalledWith( + subId, + 1700000400, + 'evt_succeeded', + { pastDueSince: null, status: 'active' }, ); }); @@ -455,13 +460,13 @@ describe('Billing webhook subscription unit tests:', () => { }; mockSubscriptionRepository.findByStripeSubscriptionId.mockResolvedValue(existing); - await BillingWebhookService.handleInvoicePaymentSucceeded({ subscription: 'sub_456' }); + await BillingWebhookService.handleInvoicePaymentSucceeded({ subscription: 'sub_456' }, makeEvent()); - expect(mockSubscriptionRepository.update).not.toHaveBeenCalled(); + expect(mockSubscriptionRepository.updateIfEventNewer).not.toHaveBeenCalled(); }); test('should return early when no subscription ID in invoice', async () => { - await BillingWebhookService.handleInvoicePaymentSucceeded({ subscription: null }); + await BillingWebhookService.handleInvoicePaymentSucceeded({ subscription: null }, makeEvent()); expect(mockSubscriptionRepository.findByStripeSubscriptionId).not.toHaveBeenCalled(); }); @@ -469,9 +474,30 @@ describe('Billing webhook subscription unit tests:', () => { test('should return early when subscription not found', async () => { mockSubscriptionRepository.findByStripeSubscriptionId.mockResolvedValue(null); - await BillingWebhookService.handleInvoicePaymentSucceeded({ subscription: 'sub_unknown' }); + await BillingWebhookService.handleInvoicePaymentSucceeded({ subscription: 'sub_unknown' }, makeEvent()); + + expect(mockSubscriptionRepository.updateIfEventNewer).not.toHaveBeenCalled(); + }); + + test('should log info when event is stale (V5 P1 #1 ordering guard)', async () => { + const existing = { + _id: subId, + organization: orgId, + pastDueSince: new Date('2026-04-01'), + status: 'past_due', + }; + mockSubscriptionRepository.findByStripeSubscriptionId.mockResolvedValue(existing); + mockSubscriptionRepository.updateIfEventNewer.mockResolvedValue(null); + + let mockLogger; + jest.unstable_mockModule('../../../lib/services/logger.js', () => { + mockLogger = { info: jest.fn(), error: jest.fn(), warn: jest.fn() }; + return { default: mockLogger }; + }); + + await BillingWebhookService.handleInvoicePaymentSucceeded({ subscription: 'sub_456' }, makeEvent({ created: 50 })); - expect(mockSubscriptionRepository.update).not.toHaveBeenCalled(); + expect(mockSubscriptionRepository.updateIfEventNewer).toHaveBeenCalled(); }); }); @@ -487,6 +513,7 @@ describe('Billing webhook subscription unit tests:', () => { expect(mockSubscriptionRepository.updateIfEventNewer).toHaveBeenCalledWith( subId, 1700000300, + 'evt_failed', expect.objectContaining({ status: 'past_due' }), ); }); @@ -499,7 +526,7 @@ describe('Billing webhook subscription unit tests:', () => { await BillingWebhookService.handleInvoicePaymentFailed({ subscription: 'sub_456' }, makeEvent()); const after = new Date(); - const callArg = mockSubscriptionRepository.updateIfEventNewer.mock.calls[0][2]; + const callArg = mockSubscriptionRepository.updateIfEventNewer.mock.calls[0][3]; expect(callArg.pastDueSince).toBeInstanceOf(Date); expect(callArg.pastDueSince.getTime()).toBeGreaterThanOrEqual(before.getTime()); expect(callArg.pastDueSince.getTime()).toBeLessThanOrEqual(after.getTime()); @@ -512,7 +539,7 @@ describe('Billing webhook subscription unit tests:', () => { await BillingWebhookService.handleInvoicePaymentFailed({ subscription: 'sub_456' }, makeEvent()); - const callArg = mockSubscriptionRepository.updateIfEventNewer.mock.calls[0][2]; + const callArg = mockSubscriptionRepository.updateIfEventNewer.mock.calls[0][3]; expect(callArg.pastDueSince).toBeUndefined(); }); From a6300ec02d88077ec48fb66d2a5b54b4b9155cca Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sun, 3 May 2026 21:49:35 +0200 Subject: [PATCH 7/7] fix(billing): payment_succeeded markers + admin refund key + reason guard - webhook: always advance stripeEventCreatedAt/stripeEventId on invoice.payment_succeeded, even when pastDueSince is already null; prevents stale out-of-order replays from being applied against a markers state that was never advanced on routine invoices - admin: replace fixed idempotency key with randomUUID suffix so multiple partial refunds on the same charge are not collapsed within Stripe's 24 h idempotency window - reason validation already enforced upstream via AdminRefundRequest Zod schema (z.enum); no controller change required --- .../controllers/billing.admin.controller.js | 4 +- .../services/billing.webhook.service.js | 25 ++++++------ .../tests/billing.admin.integration.tests.js | 24 +++++++++++- .../billing.refund.service.unit.tests.js | 12 +++--- ...billing.webhook.subscription.unit.tests.js | 38 ++++++++++++++++++- 5 files changed, 81 insertions(+), 22 deletions(-) diff --git a/modules/billing/controllers/billing.admin.controller.js b/modules/billing/controllers/billing.admin.controller.js index 75a17860b..c85e418bc 100644 --- a/modules/billing/controllers/billing.admin.controller.js +++ b/modules/billing/controllers/billing.admin.controller.js @@ -1,6 +1,8 @@ /** * Module dependencies */ +import { randomUUID } from 'node:crypto'; + import responses from '../../../lib/helpers/responses.js'; import getStripe from '../lib/stripe.js'; @@ -37,7 +39,7 @@ const adminRefundCharge = async (req, res) => { ); } - const idempotencyKey = `refund_${chargeId}_${amountCents ?? 'full'}`; + const idempotencyKey = `refund_${chargeId}_${amountCents ?? 'full'}_${randomUUID()}`; const params = { charge: chargeId, reason: reason || 'requested_by_customer' }; if (amountCents !== undefined) params.amount = amountCents; diff --git a/modules/billing/services/billing.webhook.service.js b/modules/billing/services/billing.webhook.service.js index 5a8438c98..5bc44cc5c 100644 --- a/modules/billing/services/billing.webhook.service.js +++ b/modules/billing/services/billing.webhook.service.js @@ -379,17 +379,20 @@ const handleInvoicePaymentSucceeded = async (invoice, event) => { const existing = await SubscriptionRepository.findByStripeSubscriptionId(stripeSubscriptionId); if (!existing) return; - // Only clear if currently past_due (avoid unnecessary writes on routine invoices) - if (existing.pastDueSince !== null && existing.pastDueSince !== undefined) { - const updated = await SubscriptionRepository.updateIfEventNewer( - String(existing._id), - event.created, - event.id, - { pastDueSince: null, status: 'active' }, - ); - if (!updated) { - logger.info('[billing.webhook] skipped stale event', { eventId: event.id, type: event.type }); - } + // Always advance the event markers (stripeEventCreatedAt + stripeEventId) so out-of-order + // replays are correctly rejected even when the sub is already active (pastDueSince == null). + const fields = (existing.pastDueSince !== null && existing.pastDueSince !== undefined) + ? { pastDueSince: null, status: 'active' } + : {}; + + const updated = await SubscriptionRepository.updateIfEventNewer( + String(existing._id), + event.created, + event.id, + fields, + ); + if (!updated) { + logger.info('[billing.webhook] skipped stale event', { eventId: event.id, type: event.type }); } }; diff --git a/modules/billing/tests/billing.admin.integration.tests.js b/modules/billing/tests/billing.admin.integration.tests.js index a967118f9..9b7940522 100644 --- a/modules/billing/tests/billing.admin.integration.tests.js +++ b/modules/billing/tests/billing.admin.integration.tests.js @@ -204,11 +204,31 @@ describe('Billing admin integration tests:', () => { expect(mockStripeInstance.refunds.create).toHaveBeenCalledWith( { charge: 'ch_test_123', reason: 'duplicate', amount: 2500 }, - { idempotencyKey: 'refund_ch_test_123_2500' }, + { idempotencyKey: expect.stringMatching(/^refund_ch_test_123_2500_[0-9a-f-]{36}$/) }, ); expect(res.status).toHaveBeenCalledWith(200); }); + test('two calls same charge same amount produce two separate idempotency keys', async () => { + const routes = await buildRoutes(); + const refundRoute = routes.get('/api/admin/billing/refund'); + + const makeRes = () => ({ status: jest.fn().mockReturnThis(), json: jest.fn().mockReturnThis() }); + const body = { chargeId: 'ch_test_dup', amountCents: 1000, reason: 'duplicate' }; + + await runHandlers([...refundRoute.all, ...refundRoute.post], { method: 'POST', headers: { 'x-role': 'admin' }, body }, makeRes()); + await runHandlers([...refundRoute.all, ...refundRoute.post], { method: 'POST', headers: { 'x-role': 'admin' }, body }, makeRes()); + + const calls = mockStripeInstance.refunds.create.mock.calls; + expect(calls).toHaveLength(2); + const key1 = calls[0][1].idempotencyKey; + const key2 = calls[1][1].idempotencyKey; + expect(key1).toMatch(/^refund_ch_test_dup_1000_[0-9a-f-]{36}$/); + expect(key2).toMatch(/^refund_ch_test_dup_1000_[0-9a-f-]{36}$/); + // Keys are distinct — each call gets its own idempotency window + expect(key1).not.toBe(key2); + }); + test('invalid body returns 422 from schema validation', async () => { const routes = await buildRoutes(); const refundRoute = routes.get('/api/admin/billing/refund'); @@ -259,7 +279,7 @@ describe('Billing admin integration tests:', () => { expect(mockStripeInstance.refunds.create).toHaveBeenCalledWith( { charge: 'ch_test_123', reason: 'requested_by_customer' }, - { idempotencyKey: 'refund_ch_test_123_full' }, + { idempotencyKey: expect.stringMatching(/^refund_ch_test_123_full_[0-9a-f-]{36}$/) }, ); expect(res.status).toHaveBeenCalledWith(200); }); diff --git a/modules/billing/tests/billing.refund.service.unit.tests.js b/modules/billing/tests/billing.refund.service.unit.tests.js index 2da5f8f5c..ef3159cf5 100644 --- a/modules/billing/tests/billing.refund.service.unit.tests.js +++ b/modules/billing/tests/billing.refund.service.unit.tests.js @@ -65,7 +65,7 @@ describe('Admin refund controller unit tests:', () => { expect(mockStripeInstance.refunds.create).toHaveBeenCalledWith( { charge: 'ch_test_xyz', reason: 'requested_by_customer' }, - { idempotencyKey: 'refund_ch_test_xyz_full' }, + { idempotencyKey: expect.stringMatching(/^refund_ch_test_xyz_full_[0-9a-f-]{36}$/) }, ); expect(mockResponses.success).toHaveBeenCalledWith(res, 'billing refund created'); }); @@ -78,7 +78,7 @@ describe('Admin refund controller unit tests:', () => { expect(mockStripeInstance.refunds.create).toHaveBeenCalledWith( { charge: 'ch_test_xyz', reason: 'requested_by_customer', amount: 2000 }, - { idempotencyKey: 'refund_ch_test_xyz_2000' }, + { idempotencyKey: expect.stringMatching(/^refund_ch_test_xyz_2000_[0-9a-f-]{36}$/) }, ); }); @@ -90,7 +90,7 @@ describe('Admin refund controller unit tests:', () => { expect(mockStripeInstance.refunds.create).toHaveBeenCalledWith( { charge: 'ch_test_xyz', reason: 'duplicate', amount: 2000 }, - { idempotencyKey: 'refund_ch_test_xyz_2000' }, + { idempotencyKey: expect.stringMatching(/^refund_ch_test_xyz_2000_[0-9a-f-]{36}$/) }, ); }); @@ -101,17 +101,17 @@ describe('Admin refund controller unit tests:', () => { await adminRefundCharge(req, res); const call = mockStripeInstance.refunds.create.mock.calls[0]; - expect(call[1].idempotencyKey).toBe('refund_ch_abc_full'); + expect(call[1].idempotencyKey).toMatch(/^refund_ch_abc_full_[0-9a-f-]{36}$/); }); - test('idempotency key is "refund_{chargeId}_{amountCents}" for partial refund', async () => { + test('idempotency key is "refund_{chargeId}_{amountCents}_{uuid}" for partial refund', async () => { const req = { body: { chargeId: 'ch_abc', amountCents: 500 } }; const res = makeRes(); await adminRefundCharge(req, res); const call = mockStripeInstance.refunds.create.mock.calls[0]; - expect(call[1].idempotencyKey).toBe('refund_ch_abc_500'); + expect(call[1].idempotencyKey).toMatch(/^refund_ch_abc_500_[0-9a-f-]{36}$/); }); test('should return 502 when Stripe is not configured', async () => { diff --git a/modules/billing/tests/billing.webhook.subscription.unit.tests.js b/modules/billing/tests/billing.webhook.subscription.unit.tests.js index 5dff6575b..24b7985c3 100644 --- a/modules/billing/tests/billing.webhook.subscription.unit.tests.js +++ b/modules/billing/tests/billing.webhook.subscription.unit.tests.js @@ -451,7 +451,7 @@ describe('Billing webhook subscription unit tests:', () => { ); }); - test('should NOT update when pastDueSince is null (routine invoice)', async () => { + test('should advance event markers even when pastDueSince is null (routine invoice)', async () => { const existing = { _id: subId, organization: orgId, @@ -462,7 +462,41 @@ describe('Billing webhook subscription unit tests:', () => { await BillingWebhookService.handleInvoicePaymentSucceeded({ subscription: 'sub_456' }, makeEvent()); - expect(mockSubscriptionRepository.updateIfEventNewer).not.toHaveBeenCalled(); + // markers-only update (empty fields object) — ensures stale out-of-order replays are rejected + expect(mockSubscriptionRepository.updateIfEventNewer).toHaveBeenCalledWith( + subId, + 1700000400, + 'evt_succeeded', + {}, + ); + }); + + test('stale payment_succeeded after recent payment_failed — markers reject replay', async () => { + const existing = { + _id: subId, + organization: orgId, + pastDueSince: null, + status: 'active', + }; + mockSubscriptionRepository.findByStripeSubscriptionId.mockResolvedValue(existing); + // Simulate stale event rejected by updateIfEventNewer + mockSubscriptionRepository.updateIfEventNewer.mockResolvedValue(null); + + let mockLogger; + jest.unstable_mockModule('../../../lib/services/logger.js', () => { + mockLogger = { info: jest.fn(), error: jest.fn(), warn: jest.fn() }; + return { default: mockLogger }; + }); + + // old event — should be a no-op (markers guard) + await BillingWebhookService.handleInvoicePaymentSucceeded( + { subscription: 'sub_456' }, + makeEvent({ created: 10 }), + ); + + expect(mockSubscriptionRepository.updateIfEventNewer).toHaveBeenCalledWith( + subId, 10, 'evt_succeeded', {}, + ); }); test('should return early when no subscription ID in invoice', async () => {