fix(billing): address CodeRabbit review — error handling, schema, pagination, naming

- Use 500 instead of 422 for Stripe failures to avoid leaking internals
- Fix SubscriptionUpdate to not inject defaults on partial updates
- Remove create-on-missing-id fallback from repository update method
- Normalize stripeCustomerId before querying
- Add JSDoc to route registrar and rename to billing.routes.js (convention)
- Switch to Stripe autoPagingToArray for full pagination support
- Document single-price-per-interval expectation
- Add cache TTL refresh test

Author: PierreBrisorgueil

modules/billing/controllers/billing.plans.controller.js

@@ -16,7 +16,7 @@ const getPlans = async (req, res) => {
     const plans = await BillingPlansService.getPlans();
     responses.success(res, 'billing plans')(plans);
   } catch (err) {
-    responses.error(res, 422, 'Unprocessable Entity', errors.getMessage(err))(err);
+    responses.error(res, 500, 'Internal Server Error', 'Failed to retrieve billing plans')(err);
   }
 };
 

modules/billing/models/subscription.schema.js

@@ -29,7 +29,31 @@ const Subscription = z.object({
   cancelAtPeriodEnd: z.boolean().default(false),
 });
 
-const SubscriptionUpdate = Subscription.partial();
+/**
+ * Update schema without defaults to avoid populating unspecified fields during PATCH
+ */
+const SubscriptionCore = z.object({
+  organization: z
+    .string()
+    .trim()
+    .regex(objectIdRegex, 'organization must be a valid ObjectId'),
+  stripeCustomerId: z
+    .string()
+    .trim()
+    .optional()
+    .transform((val) => (val === '' ? undefined : val)),
+  stripeSubscriptionId: z
+    .string()
+    .trim()
+    .optional()
+    .transform((val) => (val === '' ? undefined : val)),
+  plan: z.enum(['free', 'starter', 'pro']),
+  status: z.enum(['active', 'past_due', 'canceled', 'trialing', 'incomplete']),
+  currentPeriodEnd: z.coerce.date().nullable().optional(),
+  cancelAtPeriodEnd: z.boolean(),
+});
+
+const SubscriptionUpdate = SubscriptionCore.partial();
 
 export default {
   Subscription,

modules/billing/repositories/subscription.repository.js

@@ -54,14 +54,12 @@ const get = (id) => {
  */
 const update = (subscription) => {
   const id = resolveId(subscription);
-  if (id && mongoose.Types.ObjectId.isValid(id)) {
-    // eslint-disable-next-line no-unused-vars
-    const { _id, id: _virtualId, ...payload } = subscription;
-    return Subscription.findByIdAndUpdate(id, payload, { returnDocument: 'after', runValidators: true })
-      .populate(defaultPopulate)
-      .exec();
-  }
-  return new Subscription(subscription).save().then((doc) => doc.populate(defaultPopulate));
+  if (!id || !mongoose.Types.ObjectId.isValid(id)) return null;
+  // eslint-disable-next-line no-unused-vars
+  const { _id, id: _virtualId, ...payload } = subscription;
+  return Subscription.findByIdAndUpdate(id, payload, { returnDocument: 'after', runValidators: true })
+    .populate(defaultPopulate)
+    .exec();
 };
 
 /**
@@ -94,8 +92,9 @@ const findByOrganization = (organizationId) => {
  * @returns {Promise<Object|null>} A promise resolving to the retrieved subscription or null.
  */
 const findByStripeCustomerId = (stripeCustomerId) => {
-  if (!stripeCustomerId || !stripeCustomerId.trim()) return null;
-  return Subscription.findOne({ stripeCustomerId }).populate(defaultPopulate).exec();
+  const normalized = stripeCustomerId?.trim();
+  if (!normalized) return null;
+  return Subscription.findOne({ stripeCustomerId: normalized }).populate(defaultPopulate).exec();
 };
 
 export default {

modules/billing/routes/billing.route.js modules/billing/routes/billing.routes.jsmodules/billing/routes/billing.route.js renamed to modules/billing/routes/billing.routes.js

@@ -4,7 +4,9 @@
 import billing from '../controllers/billing.plans.controller.js';
 
 /**
- * Routes
+ * @desc Register billing routes
+ * @param {Object} app - Express application instance
+ * @returns {void}
  */
 export default (app) => {
   // plans (public)

modules/billing/services/billing.plans.service.js

@@ -46,30 +46,26 @@ const getStripe = () => {
  * @returns {Promise<Array>} sorted array of plan objects
  */
 const fetchPlansFromStripe = async (stripe) => {
-  const products = await stripe.products.list({
-    active: true,
-    limit: 100,
-  });
-
-  const prices = await stripe.prices.list({
-    active: true,
-    limit: 100,
-  });
+  const [products, prices] = await Promise.all([
+    stripe.products.list({ active: true }).autoPagingToArray({ limit: 1000 }),
+    stripe.prices.list({ active: true }).autoPagingToArray({ limit: 1000 }),
+  ]);
 
   const pricesByProduct = {};
-  for (const price of prices.data) {
+  for (const price of prices) {
     if (!pricesByProduct[price.product]) pricesByProduct[price.product] = [];
     pricesByProduct[price.product].push(price);
   }
 
-  const plans = products.data.map((product) => {
+  const plans = products.map((product) => {
     const productPrices = pricesByProduct[product.id] || [];
 
     let monthlyPrice = 0;
     let annualPrice = 0;
     let stripePriceMonthly = null;
     let stripePriceAnnual = null;
 
+    // Expects one active price per interval per product; last match wins if duplicates exist
     for (const price of productPrices) {
       const amount = typeof price.unit_amount === 'number' ? price.unit_amount : 0;
       if (price.recurring?.interval === 'month') {

modules/billing/tests/billing.plans.unit.tests.js

@@ -3,6 +3,13 @@
  */
 import { jest, beforeEach, afterEach } from '@jest/globals';
 
+/**
+ * Helper to create a mock Stripe list result with autoPagingToArray
+ */
+const mockListResult = (data) => ({
+  autoPagingToArray: jest.fn().mockResolvedValue(data),
+});
+
 /**
  * Unit tests for billing plans service
  */
@@ -11,39 +18,27 @@ describe('Billing plans service unit tests:', () => {
   let mockConfig;
   let mockStripeInstance;
 
-  const mockProducts = {
-    data: [
-      {
-        id: 'prod_pro',
-        name: 'Pro',
-        metadata: { planId: 'pro' },
-      },
-      {
-        id: 'prod_starter',
-        name: 'Starter',
-        metadata: { planId: 'starter' },
-      },
-    ],
-  };
-
-  const mockAllPrices = {
-    data: [
-      { product: 'prod_starter', recurring: { interval: 'month' }, unit_amount: 900, id: 'price_starter_m' },
-      { product: 'prod_starter', recurring: { interval: 'year' }, unit_amount: 9000, id: 'price_starter_y' },
-      { product: 'prod_pro', recurring: { interval: 'month' }, unit_amount: 2900, id: 'price_pro_m' },
-      { product: 'prod_pro', recurring: { interval: 'year' }, unit_amount: 29000, id: 'price_pro_y' },
-    ],
-  };
+  const productsData = [
+    { id: 'prod_pro', name: 'Pro', metadata: { planId: 'pro' } },
+    { id: 'prod_starter', name: 'Starter', metadata: { planId: 'starter' } },
+  ];
+
+  const pricesData = [
+    { product: 'prod_starter', recurring: { interval: 'month' }, unit_amount: 900, id: 'price_starter_m' },
+    { product: 'prod_starter', recurring: { interval: 'year' }, unit_amount: 9000, id: 'price_starter_y' },
+    { product: 'prod_pro', recurring: { interval: 'month' }, unit_amount: 2900, id: 'price_pro_m' },
+    { product: 'prod_pro', recurring: { interval: 'year' }, unit_amount: 29000, id: 'price_pro_y' },
+  ];
 
   beforeEach(async () => {
     jest.resetModules();
 
     mockStripeInstance = {
       products: {
-        list: jest.fn().mockResolvedValue(mockProducts),
+        list: jest.fn().mockReturnValue(mockListResult(productsData)),
       },
       prices: {
-        list: jest.fn().mockResolvedValue(mockAllPrices),
+        list: jest.fn().mockReturnValue(mockListResult(pricesData)),
       },
     };
 
@@ -122,10 +117,8 @@ describe('Billing plans service unit tests:', () => {
   });
 
   test('should fall back to product id when metadata planId is missing', async () => {
-    mockStripeInstance.products.list.mockResolvedValue({
-      data: [{ id: 'prod_basic', name: 'Basic', metadata: {} }],
-    });
-    mockStripeInstance.prices.list.mockResolvedValue({ data: [] });
+    mockStripeInstance.products.list.mockReturnValue(mockListResult([{ id: 'prod_basic', name: 'Basic', metadata: {} }]));
+    mockStripeInstance.prices.list.mockReturnValue(mockListResult([]));
 
     const mod = await import('../services/billing.plans.service.js');
     BillingPlansService = mod.default;
@@ -145,12 +138,12 @@ describe('Billing plans service unit tests:', () => {
   });
 
   test('should handle prices without recurring interval', async () => {
-    mockStripeInstance.products.list.mockResolvedValue({
-      data: [{ id: 'prod_one', name: 'OneTime', metadata: { planId: 'one' } }],
-    });
-    mockStripeInstance.prices.list.mockResolvedValue({
-      data: [{ product: 'prod_one', unit_amount: 500, id: 'price_one' }],
-    });
+    mockStripeInstance.products.list.mockReturnValue(
+      mockListResult([{ id: 'prod_one', name: 'OneTime', metadata: { planId: 'one' } }]),
+    );
+    mockStripeInstance.prices.list.mockReturnValue(
+      mockListResult([{ product: 'prod_one', unit_amount: 500, id: 'price_one' }]),
+    );
 
     const mod = await import('../services/billing.plans.service.js');
     BillingPlansService = mod.default;
@@ -163,12 +156,12 @@ describe('Billing plans service unit tests:', () => {
   });
 
   test('should handle null unit_amount gracefully', async () => {
-    mockStripeInstance.products.list.mockResolvedValue({
-      data: [{ id: 'prod_metered', name: 'Metered', metadata: { planId: 'metered' } }],
-    });
-    mockStripeInstance.prices.list.mockResolvedValue({
-      data: [{ product: 'prod_metered', recurring: { interval: 'month' }, unit_amount: null, id: 'price_metered' }],
-    });
+    mockStripeInstance.products.list.mockReturnValue(
+      mockListResult([{ id: 'prod_metered', name: 'Metered', metadata: { planId: 'metered' } }]),
+    );
+    mockStripeInstance.prices.list.mockReturnValue(
+      mockListResult([{ product: 'prod_metered', recurring: { interval: 'month' }, unit_amount: null, id: 'price_metered' }]),
+    );
 
     const mod = await import('../services/billing.plans.service.js');
     BillingPlansService = mod.default;
@@ -178,14 +171,14 @@ describe('Billing plans service unit tests:', () => {
     expect(plans[0].stripePriceMonthly).toBe('price_metered');
   });
 
-  test('should fetch products and prices with limit 100', async () => {
+  test('should use autoPagingToArray for products and prices', async () => {
     const mod = await import('../services/billing.plans.service.js');
     BillingPlansService = mod.default;
 
     await BillingPlansService.getPlans();
 
-    expect(mockStripeInstance.products.list).toHaveBeenCalledWith({ active: true, limit: 100 });
-    expect(mockStripeInstance.prices.list).toHaveBeenCalledWith({ active: true, limit: 100 });
+    expect(mockStripeInstance.products.list).toHaveBeenCalledWith({ active: true });
+    expect(mockStripeInstance.prices.list).toHaveBeenCalledWith({ active: true });
   });
 
   test('should only make one prices.list call regardless of product count', async () => {
@@ -196,4 +189,21 @@ describe('Billing plans service unit tests:', () => {
 
     expect(mockStripeInstance.prices.list).toHaveBeenCalledTimes(1);
   });
+
+  test('should refresh cache after TTL expires', async () => {
+    const mod = await import('../services/billing.plans.service.js');
+    BillingPlansService = mod.default;
+
+    await BillingPlansService.getPlans();
+
+    // Advance time past TTL (1 hour)
+    const originalDateNow = Date.now;
+    Date.now = jest.fn().mockReturnValue(originalDateNow() + 61 * 60 * 1000);
+
+    await BillingPlansService.getPlans();
+
+    expect(mockStripeInstance.products.list).toHaveBeenCalledTimes(2);
+
+    Date.now = originalDateNow;
+  });
 });

modules/billing/tests/billing.unit.tests.js

@@ -173,5 +173,15 @@ describe('Billing unit tests:', () => {
       expect(result.data.stripeSubscriptionId).toBe('sub_xyz789');
       done();
     });
+
+    test('should not inject defaults in SubscriptionUpdate partial', (done) => {
+      const update = { plan: 'pro' };
+      const result = schema.SubscriptionUpdate.safeParse(update);
+      expect(result.error).toBeFalsy();
+      expect(result.data.plan).toBe('pro');
+      expect(result.data.status).toBeUndefined();
+      expect(result.data.cancelAtPeriodEnd).toBeUndefined();
+      done();
+    });
   });
 });