From e04fb67ce8bef010f1436c93d65a9a47b5e5071d Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Thu, 30 Apr 2026 09:50:44 +0200 Subject: [PATCH 1/2] =?UTF-8?q?fix(billing):=20apply=20cumulative=20review?= =?UTF-8?q?=20=E2=80=94=20refund=20correlation=20+=20cleanup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CRITICAL: backfill PaymentIntent metadata with real cs_* session ID after creditPack succeeds in handleCheckoutPaymentCompleted, so charge.refunded events can correlate the charge back to the correct ledger entry (the __pending__ placeholder was breaking refundPartial lookups). Add 3 unit tests covering PI update, absent PI, and non-fatal failure path. HIGH: replace stale TODO PR-N3 in refundPartial with accurate comment — heuristic is still intentionally needed since refundPartial does not receive packId from the webhook call-site. HIGH: add Mixed type caveat JSDoc to billing.usage.model (meterBreakdown and counters fields) matching the existing note in billing.extraBalance.model. MEDIUM: document BillingExtraBalance.organization vs BillingUsage.organizationId asymmetry in billing.controller.js. MEDIUM: add startup priceUsd validation warning in billing.init.js so misconfigured packs are caught before a refund event exposes the gap. --- modules/billing/billing.init.js | 10 +++ .../billing/controllers/billing.controller.js | 3 + .../models/billing.usage.model.mongoose.js | 6 ++ .../billing/services/billing.extra.service.js | 6 +- .../services/billing.webhook.service.js | 27 +++++++- .../billing.webhook.checkout.unit.tests.js | 64 +++++++++++++++++++ 6 files changed, 113 insertions(+), 3 deletions(-) diff --git a/modules/billing/billing.init.js b/modules/billing/billing.init.js index ac6b934d4..ea7c99870 100644 --- a/modules/billing/billing.init.js +++ b/modules/billing/billing.init.js @@ -1,6 +1,7 @@ /** * Module dependencies */ +import config from '../../config/index.js'; import AnalyticsService from '../../lib/services/analytics.js'; import billingEvents from './lib/events.js'; @@ -13,6 +14,15 @@ import billingEvents from './lib/events.js'; */ // eslint-disable-next-line no-unused-vars export default async (app) => { + // Warn at startup if any pack is missing a valid priceUsd — refundPartial fallback will be inaccurate + if (config.billing?.packs?.length) { + for (const pack of config.billing.packs) { + if (typeof pack.priceUsd !== 'number' || pack.priceUsd <= 0) { + console.warn(`[billing] pack '${pack.packId}' missing valid priceUsd; refundPartial fallback will be inaccurate`); + } + } + } + // Update analytics group properties when a subscription plan changes billingEvents.on('plan.changed', ({ organizationId, newPlan }) => { try { diff --git a/modules/billing/controllers/billing.controller.js b/modules/billing/controllers/billing.controller.js index 1f647df04..7828627c4 100644 --- a/modules/billing/controllers/billing.controller.js +++ b/modules/billing/controllers/billing.controller.js @@ -9,6 +9,9 @@ import BillingUsageService from '../services/billing.usage.service.js'; import BillingExtraService from '../services/billing.extra.service.js'; import BillingExtraBalanceRepository from '../repositories/billing.extraBalance.repository.js'; +// NOTE: BillingExtraBalance uses 'organization' (ObjectId ref); BillingUsage uses 'organizationId' (string field). +// These two collections have asymmetric field names for historical reasons — keep queries consistent with each model's own convention. + /** * @desc Endpoint to create a Stripe Checkout session * @param {Object} req - Express request object diff --git a/modules/billing/models/billing.usage.model.mongoose.js b/modules/billing/models/billing.usage.model.mongoose.js index e5b7c1926..8b25b2608 100644 --- a/modules/billing/models/billing.usage.model.mongoose.js +++ b/modules/billing/models/billing.usage.model.mongoose.js @@ -12,6 +12,12 @@ const Schema = mongoose.Schema; * Meter fields (weekKey, meterUsed, etc.) are sparse/optional — only * populated when config.billing.meterMode is true. This ensures full * backward compatibility for non-meter downstream projects. + * + * NOTE — Mixed type caveats (applies to 'counters' and 'meterBreakdown' fields): + * Mongoose validators are NOT executed for in-place mutations on Mixed fields + * (doc.field.x = y; doc.save() silently skips validators). + * Always use atomic MongoDB operators ($inc, $set via findOneAndUpdate) + * or Model.create() which runs validators on the full document. */ const UsageMongoose = new Schema( { diff --git a/modules/billing/services/billing.extra.service.js b/modules/billing/services/billing.extra.service.js index 6087e0bd7..b4395b2f8 100644 --- a/modules/billing/services/billing.extra.service.js +++ b/modules/billing/services/billing.extra.service.js @@ -87,10 +87,12 @@ const refundPartial = async (orgId, stripeSessionId, amountRefundedCents) => { return { doc, applied: false, refundUnits: 0 }; } - // Find the pack config to compute proportion. + // Find the pack config to compute proportion using the topup entry's meterUnits. // Ambiguity guard: if 0 or >1 packs share the same meterUnits, fall back to // applied=false rather than using a wrong priceUsd. - // TODO PR-N3: webhook handler will pass packId from session metadata, removing the need for this heuristic. + // NOTE: packId is not passed to refundPartial — charge.refunded carries it in + // charge.metadata but the webhook call-site only passes (orgId, stripeSessionId, amountCents). + // This heuristic is therefore intentionally retained; the ambiguity guard is the safety net. const packs = config?.billing?.packs ?? []; const matchingPacks = packs.filter( (p) => (p.meterUnits ?? p.computeUnits) === topupEntry.amount, diff --git a/modules/billing/services/billing.webhook.service.js b/modules/billing/services/billing.webhook.service.js index 4c69a8b6f..dc3eb0172 100644 --- a/modules/billing/services/billing.webhook.service.js +++ b/modules/billing/services/billing.webhook.service.js @@ -4,6 +4,7 @@ import mongoose from 'mongoose'; import config from '../../../config/index.js'; +import getStripe from '../lib/stripe.js'; import SubscriptionRepository from '../repositories/billing.subscription.repository.js'; import ProcessedStripeEventRepository from '../repositories/billing.processedStripeEvent.repository.js'; import OrganizationRepository from '../../organizations/repositories/organizations.repository.js'; @@ -152,7 +153,7 @@ const handleCheckoutCompleted = async (session) => { const handleCheckoutPaymentCompleted = async (session) => { if (session.payment_status !== 'paid') return; - const { metadata, id: stripeSessionId } = session; + const { metadata, id: stripeSessionId, payment_intent: paymentIntentId } = session; const { organizationId, packId, kind } = metadata ?? {}; if (kind !== 'extras') return; @@ -160,6 +161,30 @@ const handleCheckoutPaymentCompleted = async (session) => { if (!packId) return; await BillingExtraService.creditPack(organizationId, packId, stripeSessionId); + + // Backfill PaymentIntent metadata with the real session ID so that charge.refunded + // events can correlate the charge back to this ledger entry. + // At session.create time stripeSessionId was set to '__pending__' (Stripe forbids + // self-reference). Propagating the real cs_* ID here ensures charge.metadata carries + // it when a refund is issued later. + if (paymentIntentId) { + const stripe = getStripe(); + if (stripe) { + try { + await stripe.paymentIntents.update(paymentIntentId, { + metadata: { + organizationId, + packId, + kind: 'extras', + stripeSessionId, // real cs_* ID + }, + }); + } catch (err) { + // Log but don't fail — refund correlation may need fallback path + console.warn('[billing.webhook] PaymentIntent metadata update failed:', err.message); + } + } + } }; /** diff --git a/modules/billing/tests/billing.webhook.checkout.unit.tests.js b/modules/billing/tests/billing.webhook.checkout.unit.tests.js index 4e325b143..ee93426f5 100644 --- a/modules/billing/tests/billing.webhook.checkout.unit.tests.js +++ b/modules/billing/tests/billing.webhook.checkout.unit.tests.js @@ -14,6 +14,7 @@ describe('Billing webhook checkout unit tests:', () => { let mockSubscriptionRepository; let mockOrganizationRepository; let mockExtraService; + let mockStripeInstance; const orgId = '507f1f77bcf86cd799439011'; const subId = '607f1f77bcf86cd799439022'; @@ -39,6 +40,12 @@ describe('Billing webhook checkout unit tests:', () => { refundPartial: jest.fn(), }; + mockStripeInstance = { + paymentIntents: { + update: jest.fn().mockResolvedValue({}), + }, + }; + jest.unstable_mockModule('../repositories/billing.subscription.repository.js', () => ({ default: mockSubscriptionRepository, })); @@ -66,6 +73,10 @@ describe('Billing webhook checkout unit tests:', () => { default: { emit: jest.fn() }, })); + jest.unstable_mockModule('../lib/stripe.js', () => ({ + default: jest.fn(() => mockStripeInstance), + })); + jest.unstable_mockModule('../../../config/index.js', () => ({ default: { billing: { plans: ['free', 'starter', 'pro', 'enterprise'] }, @@ -220,6 +231,59 @@ describe('Billing webhook checkout unit tests:', () => { expect(mockExtraService.creditPack).not.toHaveBeenCalled(); }); + + test('should call stripe.paymentIntents.update with real sessionId after creditPack succeeds (CRITICAL: refund correlation)', async () => { + const paymentIntentId = 'pi_test_abc123'; + + await BillingWebhookService.handleCheckoutPaymentCompleted({ + id: stripeSessionId, + payment_status: 'paid', + payment_intent: paymentIntentId, + metadata: { organizationId: orgId, packId: 'pack_500k', kind: 'extras' }, + }); + + expect(mockExtraService.creditPack).toHaveBeenCalledWith(orgId, 'pack_500k', stripeSessionId); + expect(mockStripeInstance.paymentIntents.update).toHaveBeenCalledWith( + paymentIntentId, + { + metadata: { + organizationId: orgId, + packId: 'pack_500k', + kind: 'extras', + stripeSessionId, // real cs_* ID (not '__pending__') + }, + }, + ); + }); + + test('should skip paymentIntents.update when payment_intent is absent', async () => { + await BillingWebhookService.handleCheckoutPaymentCompleted({ + id: stripeSessionId, + payment_status: 'paid', + // payment_intent omitted — e.g. in test fixtures without PI + metadata: { organizationId: orgId, packId: 'pack_500k', kind: 'extras' }, + }); + + expect(mockExtraService.creditPack).toHaveBeenCalled(); + expect(mockStripeInstance.paymentIntents.update).not.toHaveBeenCalled(); + }); + + test('should not throw when paymentIntents.update fails (non-fatal fallback)', async () => { + const paymentIntentId = 'pi_test_failing'; + mockStripeInstance.paymentIntents.update.mockRejectedValue(new Error('Stripe API error')); + + await expect( + BillingWebhookService.handleCheckoutPaymentCompleted({ + id: stripeSessionId, + payment_status: 'paid', + payment_intent: paymentIntentId, + metadata: { organizationId: orgId, packId: 'pack_500k', kind: 'extras' }, + }), + ).resolves.toBeUndefined(); + + // creditPack should still have run despite the PI update failure + expect(mockExtraService.creditPack).toHaveBeenCalledWith(orgId, 'pack_500k', stripeSessionId); + }); }); describe('handleCheckoutCompleted (mode=subscription)', () => { From 7358df3985241bf13c120ef227ba2f8838948497 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Thu, 30 Apr 2026 09:55:25 +0200 Subject: [PATCH 2/2] fix(billing): correct organization field type comment (both ObjectId) --- modules/billing/controllers/billing.controller.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/billing/controllers/billing.controller.js b/modules/billing/controllers/billing.controller.js index 7828627c4..e2844e4f0 100644 --- a/modules/billing/controllers/billing.controller.js +++ b/modules/billing/controllers/billing.controller.js @@ -9,8 +9,8 @@ import BillingUsageService from '../services/billing.usage.service.js'; import BillingExtraService from '../services/billing.extra.service.js'; import BillingExtraBalanceRepository from '../repositories/billing.extraBalance.repository.js'; -// NOTE: BillingExtraBalance uses 'organization' (ObjectId ref); BillingUsage uses 'organizationId' (string field). -// These two collections have asymmetric field names for historical reasons — keep queries consistent with each model's own convention. +// NOTE: BillingExtraBalance uses field name 'organization', BillingUsage uses 'organizationId' — both are Schema.ObjectId refs to Organization. +// Only the field name differs (historical reasons) — keep queries consistent with each model's own convention. /** * @desc Endpoint to create a Stripe Checkout session