Skip to content

Commit be14a9e

Browse files
fix(billing): address Copilot review — enum validation, accurate comments
- creditGrant now validates source via Zod (ExtraBalanceCreditGrant.parse) before write — findOneAndUpdate does not run validators so the explicit parse is necessary - Add test for Zod throw on invalid source enum value - Fix index comment: $ne predicate does not use tight index bounds (noted actual benefit) - Fix GrantSource/LedgerEntry comments: creditCompensation does NOT set source; source is exclusively set by creditGrant and future grant methods
1 parent d89eec9 commit be14a9e

4 files changed

Lines changed: 27 additions & 12 deletions

File tree

modules/billing/models/billing.extraBalance.model.mongoose.js

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -139,9 +139,12 @@ ExtraBalanceMongoose.index({ 'ledger.historyId': 1 }, { sparse: true });
139139
ExtraBalanceMongoose.index({ 'ledger.expiresAt': 1 }, { sparse: true });
140140

141141
/**
142-
* Index for grant idempotency — supports creditGrant duplicate-check via refId filter.
143-
* refId is the leading key so the `ledger.refId: {$ne: key}` query uses index bounds directly.
144-
* source is a trailing key for analytics queries filtering grant entries by source.
142+
* Index for grant analytics + idempotency support.
143+
* refId is the leading key for analytics and admin queries that filter grant entries by refId prefix.
144+
* source is a trailing key for filtering entries by grant type (e.g. all signup_grant entries).
145+
* Note: the creditGrant idempotency guard (`ledger.refId: {$ne: key}`) is an exclusion predicate
146+
* scoped by the unique `organization` field — it does not use tight index bounds, but the sparse
147+
* index still reduces the scan set to grant entries only.
145148
*/
146149
ExtraBalanceMongoose.index({ 'ledger.refId': 1, 'ledger.source': 1 }, { sparse: true });
147150

modules/billing/models/billing.extraBalance.schema.js

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,13 @@ const LedgerKind = z.enum(['topup', 'debit', 'refund', 'expiration', 'adjustment
1616

1717
/**
1818
* Allowed grant sources — mirrors the Mongoose enum on LedgerEntrySchema.source.
19-
* 'signup_grant' — one-shot free tier grant on org creation.
20-
* 'adjustment' — manual ops credit (non-Stripe, no Stripe session).
21-
* NOTE: 'adjustment' here is a source tag (who credited it), distinct from
22-
* LedgerKind 'adjustment' (how the balance was changed). They share the string
23-
* literal by convention: a manual adjustment uses kind='adjustment' + source='adjustment'.
24-
* Grant entries use kind='topup' + source='signup_grant'.
19+
* 'signup_grant' — one-shot free tier grant on org creation (kind='topup').
20+
* 'adjustment' — reserved for future non-Stripe manual credits.
21+
* NOTE: 'adjustment' here is a source tag (provenance), distinct from
22+
* LedgerKind 'adjustment' (balance mutation type). Existing creditCompensation()
23+
* writes kind='adjustment' entries WITHOUT setting source — source is only set by
24+
* creditGrant() and future grant methods. Do not assume kind='adjustment' implies
25+
* source='adjustment'.
2526
*/
2627
const GrantSource = z.enum(['signup_grant', 'adjustment']);
2728

@@ -51,7 +52,8 @@ const LedgerEntry = z
5152
.nullable(),
5253
refId: z.string().trim().optional().nullable(),
5354
/**
54-
* Credit source tag — set on grant/adjustment entries; absent on Stripe topup entries.
55+
* Credit source tag — set only by creditGrant() (and future grant methods).
56+
* Absent on Stripe topup entries (creditPack) and creditCompensation entries.
5557
* Mirrors LedgerEntrySchema.source in billing.extraBalance.model.mongoose.js.
5658
*/
5759
source: GrantSource.optional().nullable(),

modules/billing/repositories/billing.extraBalance.repository.js

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
*/
44
import mongoose from 'mongoose';
55
import AppError from '../../../lib/helpers/AppError.js';
6+
import BillingExtraBalanceSchema from '../models/billing.extraBalance.schema.js';
67

78
/**
89
* Validate that orgId is a syntactically valid MongoDB ObjectId.
@@ -183,12 +184,14 @@ const creditGrant = async (orgId, amount, source) => {
183184
if (!isValidOrgId(orgId)) return { doc: null, applied: false };
184185
if (!Number.isFinite(amount) || amount <= 0) throw new Error('invalid argument: amount must be a positive finite number');
185186
if (typeof source !== 'string' || source.trim() === '') throw new Error('invalid argument: source must be a non-empty string');
187+
// Validate source against the enum before writing — findOneAndUpdate does not run validators.
188+
BillingExtraBalanceSchema.ExtraBalanceCreditGrant.parse({ orgId, amount, source: source.trim() });
186189

187-
const idempotencyKey = `${source}-${orgId}`;
190+
const idempotencyKey = `${source.trim()}-${orgId}`;
188191
const entry = {
189192
kind: 'topup',
190193
amount,
191-
source,
194+
source: source.trim(),
192195
refId: idempotencyKey,
193196
at: new Date(),
194197
};

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -881,6 +881,13 @@ describe('BillingExtraBalance unit tests:', () => {
881881
).rejects.toThrow('invalid argument: source must be a non-empty string');
882882
});
883883

884+
test('should throw (Zod) on source value not in allowed enum', async () => {
885+
await expect(
886+
BillingExtraBalanceRepository.creditGrant(orgId, 500, 'freebie'),
887+
).rejects.toThrow();
888+
expect(mockModel.findOneAndUpdate).not.toHaveBeenCalled();
889+
});
890+
884891
test('should return null doc with applied:false for invalid orgId', async () => {
885892
const { default: mongoose } = await import('mongoose');
886893
mongoose.Types.ObjectId.isValid = jest.fn(() => false);

0 commit comments

Comments
 (0)