-
-
Notifications
You must be signed in to change notification settings - Fork 10
feat(billing): crons + dunning grace 7d (PR-N5) #3539
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
00ea2d4
17c2662
97e9662
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,8 +19,11 @@ import responses from '../../../lib/helpers/responses.js'; | |
| * When no quota is configured or limit is Infinity, the request is allowed. | ||
| * | ||
| * - When `config.billing.meterMode === true`: meter quota gate. | ||
| * Computes `(meterQuota - meterUsed) + extrasBalance`. Returns 402 when <= 0, | ||
| * including pack purchase info for the client. Falls through to next() otherwise. | ||
| * First checks for past_due degraded mode: | ||
| * - past_due + pastDueSince set + within 7-day grace: sets res.locals.billingDegraded = true | ||
| * and falls through to the meter check (may still block on exhaustion). | ||
| * - past_due + pastDueSince set + grace elapsed (>=7d): returns 402 PAYMENT_PAST_DUE. | ||
| * Then computes `(meterQuota - meterUsed) + extrasBalance`. Returns 402 METER_EXHAUSTED when <= 0. | ||
| * | ||
| * Expects `req.organization` to be set by resolveOrganization upstream. | ||
| * | ||
|
|
@@ -45,6 +48,23 @@ function requireQuota(resource, action) { | |
| // ── Meter mode (meterMode: true) ────────────────────────────────────── | ||
| if (config.billing?.meterMode === true) { | ||
| const orgId = req.organization._id.toString(); | ||
|
|
||
| // ── Degraded-mode gate (past_due grace period) ───────────────────── | ||
| const subscription = await SubscriptionRepository.findByOrganization(req.organization._id); | ||
| if (subscription?.status === 'past_due' && subscription.pastDueSince != null) { | ||
| const gracePeriodMs = 7 * 24 * 60 * 60 * 1000; | ||
| const elapsed = Date.now() - new Date(subscription.pastDueSince).getTime(); | ||
|
Comment on lines
+53
to
+56
|
||
| if (elapsed >= gracePeriodMs) { | ||
| return responses.error(res, 402, 'Payment Required', 'Subscription past due, please update payment')({ | ||
| type: 'PAYMENT_PAST_DUE', | ||
| message: 'Subscription past due, please update payment', | ||
| subscriptionStatus: 'past_due', | ||
| }); | ||
| } | ||
| // Within grace period — mark degraded for downstream awareness but allow through | ||
| res.locals.billingDegraded = true; | ||
| } | ||
|
|
||
| const usage = await BillingUsageService.getMeter(orgId); | ||
| const extrasBalance = await BillingExtraBalanceRepository.getBalance(orgId); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -243,11 +243,56 @@ const getBalance = async (orgId) => { | |||||||||||||||||||||||||||||||||||||||
| return doc ? doc.cachedBalance : 0; | ||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||
| * @function findOrgsWithExpiringTopups | ||||||||||||||||||||||||||||||||||||||||
| * @description Return the distinct organizationIds that have at least one topup ledger entry | ||||||||||||||||||||||||||||||||||||||||
| * with `expiresAt < now` for which no matching expiration entry (`kind: 'expiration'` | ||||||||||||||||||||||||||||||||||||||||
| * with `refId: 'expire-<entryId>'`) has been recorded yet. | ||||||||||||||||||||||||||||||||||||||||
| * Used by the billing.extrasExpiration cron to build the sweep target list. | ||||||||||||||||||||||||||||||||||||||||
| * @param {Date} now - Cutoff timestamp. Topups with expiresAt strictly before this are candidates. | ||||||||||||||||||||||||||||||||||||||||
| * @returns {Promise<string[]>} Array of distinct organizationId strings. | ||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||
| // biome-ignore lint/correctness/useQwikValidLexicalScope: false positive — Node.js repository, not Qwik | ||||||||||||||||||||||||||||||||||||||||
| const findOrgsWithExpiringTopups = async (now) => { | ||||||||||||||||||||||||||||||||||||||||
| if (!(now instanceof Date)) throw new TypeError('now must be a Date instance'); | ||||||||||||||||||||||||||||||||||||||||
| // Pull only the ledger field (projection) to keep the payload small. | ||||||||||||||||||||||||||||||||||||||||
| // Note: the MongoDB pre-filter `ledger.expiresAt: { $lt: now }` is a coarse pre-filter — | ||||||||||||||||||||||||||||||||||||||||
| // some returned docs may have no unhandled expirations (already recorded expiration entries); | ||||||||||||||||||||||||||||||||||||||||
| // the in-memory loop below performs the precise check. This is intentional for simplicity. | ||||||||||||||||||||||||||||||||||||||||
| const docs = await BillingExtraBalance() | ||||||||||||||||||||||||||||||||||||||||
| .find( | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+262
to
+263
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 MEDIUM RISK The query for expired topups is imprecise and memory-intensive. Without using Refactor this to use a MongoDB aggregation pipeline that filters the ledger array on the database side and returns only distinct organization IDs.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 17c2662: findOrgsWithExpiringTopups projects only ledger+organization (not full doc). Pre-filter behavior is intentional and documented in JSDoc — full aggregation pipeline deferred. |
||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||
| 'ledger.kind': 'topup', | ||||||||||||||||||||||||||||||||||||||||
| 'ledger.expiresAt': { $lt: now }, | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+259
to
+266
|
||||||||||||||||||||||||||||||||||||||||
| // Note: the MongoDB pre-filter `ledger.expiresAt: { $lt: now }` is a coarse pre-filter — | |
| // some returned docs may have no unhandled expirations (already recorded expiration entries); | |
| // the in-memory loop below performs the precise check. This is intentional for simplicity. | |
| const docs = await BillingExtraBalance() | |
| .find( | |
| { | |
| 'ledger.kind': 'topup', | |
| 'ledger.expiresAt': { $lt: now }, | |
| // Use $elemMatch so `kind` and `expiresAt` are evaluated on the same ledger entry. | |
| // The in-memory loop below still performs the precise check for missing expiration entries. | |
| const docs = await BillingExtraBalance() | |
| .find( | |
| { | |
| ledger: { | |
| $elemMatch: { | |
| kind: 'topup', | |
| expiresAt: { $lt: now }, | |
| }, | |
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as #3164066073 — the MongoDB pre-filter is coarse by design (documented in JSDoc). The JS-side loop performs the precise per-entry check. Ledger arrays are bounded per org in practice; aggregation pipeline deferred.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,141 @@ | ||
| /** | ||
| * Module dependencies. | ||
| */ | ||
| import { jest, describe, test, beforeEach, afterEach, expect } from '@jest/globals'; | ||
|
|
||
| /** | ||
| * Unit tests for BillingExtraBalanceRepository.findOrgsWithExpiringTopups (PR-N5) | ||
| */ | ||
| describe('BillingExtraBalanceRepository.findOrgsWithExpiringTopups:', () => { | ||
| let BillingExtraBalanceRepository; | ||
| let mockModel; | ||
|
|
||
| const orgId1 = '507f1f77bcf86cd799439011'; | ||
| const orgId2 = '507f1f77bcf86cd799439022'; | ||
|
|
||
| /** | ||
| * @param {string} topupId - Fake ObjectId for the topup entry. | ||
| * @param {Date} expiresAt - Expiry date for the topup. | ||
| * @param {boolean} [withExpiration=false] - Whether to include a matching expiration entry. | ||
| * @returns {Array} Ledger array. | ||
| */ | ||
| const makeLedger = (topupId, expiresAt, withExpiration = false) => { | ||
|
Check warning on line 22 in modules/billing/tests/billing.extraBalance.findOrgsWithExpiringTopups.unit.tests.js
|
||
| const ledger = [ | ||
| { _id: topupId, kind: 'topup', amount: 1000, expiresAt }, | ||
| ]; | ||
| if (withExpiration) { | ||
| ledger.push({ kind: 'expiration', amount: -1000, refId: `expire-${topupId}` }); | ||
| } | ||
| return ledger; | ||
| }; | ||
|
|
||
| beforeEach(async () => { | ||
| jest.resetModules(); | ||
|
|
||
| mockModel = { | ||
| find: jest.fn(), | ||
| }; | ||
|
|
||
| jest.unstable_mockModule('mongoose', () => ({ | ||
| default: { | ||
| Types: { ObjectId: { isValid: (id) => /^[a-f\d]{24}$/i.test(id) } }, | ||
| model: jest.fn(() => mockModel), | ||
| }, | ||
| })); | ||
|
|
||
| const mod = await import('../repositories/billing.extraBalance.repository.js'); | ||
| BillingExtraBalanceRepository = mod.default; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| jest.restoreAllMocks(); | ||
| }); | ||
|
|
||
| test('returns empty array when no docs match', async () => { | ||
| mockModel.find.mockReturnValue({ lean: jest.fn().mockResolvedValue([]) }); | ||
|
|
||
| const result = await BillingExtraBalanceRepository.findOrgsWithExpiringTopups(new Date()); | ||
| expect(result).toEqual([]); | ||
| }); | ||
|
|
||
| test('throws TypeError when now is not a Date', async () => { | ||
| await expect(BillingExtraBalanceRepository.findOrgsWithExpiringTopups('2026-01-01')).rejects.toThrow(TypeError); | ||
| await expect(BillingExtraBalanceRepository.findOrgsWithExpiringTopups(null)).rejects.toThrow(TypeError); | ||
| await expect(BillingExtraBalanceRepository.findOrgsWithExpiringTopups(undefined)).rejects.toThrow(TypeError); | ||
| await expect(BillingExtraBalanceRepository.findOrgsWithExpiringTopups(Date.now())).rejects.toThrow(TypeError); | ||
| }); | ||
|
|
||
| test('returns orgId when unhandled expired topup exists', async () => { | ||
| const now = new Date(); | ||
| const pastDate = new Date(now.getTime() - 1000); | ||
| const topupId = 'aaaaaaaaaaaaaaaaaaaaaaaa'; | ||
| const docs = [ | ||
| { organization: orgId1, ledger: makeLedger(topupId, pastDate, false) }, | ||
| ]; | ||
| mockModel.find.mockReturnValue({ lean: jest.fn().mockResolvedValue(docs) }); | ||
|
|
||
| const result = await BillingExtraBalanceRepository.findOrgsWithExpiringTopups(now); | ||
| expect(result).toContain(orgId1); | ||
| }); | ||
|
|
||
| test('excludes org when all expired topups already have expiration entries', async () => { | ||
| const now = new Date(); | ||
| const pastDate = new Date(now.getTime() - 1000); | ||
| const topupId = 'aaaaaaaaaaaaaaaaaaaaaaaa'; | ||
| const docs = [ | ||
| { organization: orgId1, ledger: makeLedger(topupId, pastDate, true) }, | ||
| ]; | ||
| mockModel.find.mockReturnValue({ lean: jest.fn().mockResolvedValue(docs) }); | ||
|
|
||
| const result = await BillingExtraBalanceRepository.findOrgsWithExpiringTopups(now); | ||
| expect(result).not.toContain(orgId1); | ||
| }); | ||
|
|
||
| test('excludes org when topup is not yet expired (expiresAt in the future)', async () => { | ||
| const now = new Date(); | ||
| const futureDate = new Date(now.getTime() + 10000); | ||
| const topupId = 'aaaaaaaaaaaaaaaaaaaaaaaa'; | ||
| const docs = [ | ||
| { organization: orgId1, ledger: makeLedger(topupId, futureDate, false) }, | ||
| ]; | ||
| mockModel.find.mockReturnValue({ lean: jest.fn().mockResolvedValue(docs) }); | ||
|
|
||
| const result = await BillingExtraBalanceRepository.findOrgsWithExpiringTopups(now); | ||
| expect(result).not.toContain(orgId1); | ||
| }); | ||
|
|
||
| test('returns multiple orgIds when multiple orgs have unhandled expirations', async () => { | ||
| const now = new Date(); | ||
| const pastDate = new Date(now.getTime() - 1000); | ||
| const topupId1 = 'aaaaaaaaaaaaaaaaaaaaaaaa'; | ||
| const topupId2 = 'bbbbbbbbbbbbbbbbbbbbbbbb'; | ||
| const docs = [ | ||
| { organization: orgId1, ledger: makeLedger(topupId1, pastDate, false) }, | ||
| { organization: orgId2, ledger: makeLedger(topupId2, pastDate, false) }, | ||
| ]; | ||
| mockModel.find.mockReturnValue({ lean: jest.fn().mockResolvedValue(docs) }); | ||
|
|
||
| const result = await BillingExtraBalanceRepository.findOrgsWithExpiringTopups(now); | ||
| expect(result).toHaveLength(2); | ||
| expect(result).toContain(orgId1); | ||
| expect(result).toContain(orgId2); | ||
| }); | ||
|
|
||
| test('handles org with mixed expired (handled) and unhandled topups — returns org', async () => { | ||
| const now = new Date(); | ||
| const pastDate = new Date(now.getTime() - 1000); | ||
| const topupId1 = 'aaaaaaaaaaaaaaaaaaaaaaaa'; | ||
| const topupId2 = 'bbbbbbbbbbbbbbbbbbbbbbbb'; | ||
| // topupId1 already has expiration, topupId2 does not | ||
| const ledger = [ | ||
| { _id: topupId1, kind: 'topup', amount: 1000, expiresAt: pastDate }, | ||
| { kind: 'expiration', amount: -1000, refId: `expire-${topupId1}` }, | ||
| { _id: topupId2, kind: 'topup', amount: 500, expiresAt: pastDate }, | ||
| ]; | ||
| const docs = [{ organization: orgId1, ledger }]; | ||
| mockModel.find.mockReturnValue({ lean: jest.fn().mockResolvedValue(docs) }); | ||
|
|
||
| const result = await BillingExtraBalanceRepository.findOrgsWithExpiringTopups(now); | ||
| expect(result).toContain(orgId1); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Executing a database lookup for the subscription on every request gated by this middleware can significantly impact throughput. Since
req.organizationis resolved upstream, consider pre-populating the subscription status there or using a local cache to avoid a per-request DB hit in this hot path.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted for future optimization. The subscription lookup in meterMode is necessary for the grace-period gate (pastDueSince check). Caching at the resolveOrganization middleware level is tracked as a follow-up perf item.