Skip to content

Commit 7c7c0bf

Browse files
fix(billing): credit signupGrant on the generic org-creation path (#3949)
* fix(billing): credit signupGrant on the generic org-creation path createOrganizationForUser credited the one-shot signupGrant, but the generic POST /api/organizations path (organizations.crud.service.create) did not, so an org created there (e.g. via a manual "create workspace" screen) started at 0 balance on a plan that defines a grant. Call the existing idempotent BillingSignupGrantService.grantOnSignup from crud.create too, best-effort and outside the rollback try/catch. Add a config-driven backfill migration: for every plan in planDefinitions with a positive signupGrant, credit orgs on that plan that lack a signup_grant ledger entry. Generalises the 20260511 backfill (which hardcoded 500 and enumerated the subscriptions collection, missing orgs with no subscription document) by enumerating organizations and reading the configured amount. Idempotent via the signup_grant-<orgId> refId. Claude-Session: https://claude.ai/code/session_01S8zb8xNiyALVu366vzyi8x * fix(billing): route the signup-grant backfill through the runtime grant path The migration re-implemented the ledger write with the raw driver keyed on a STRING organization, but billingextrabalances.organization is Schema.ObjectId and every repository write casts to ObjectId — the string write would have created orphan, app-invisible ghost documents and silently credited no one while logging success. Delegate to BillingSignupGrantService.grantOnSignup per org so the migration reuses the exact runtime grant path: Mongoose ObjectId casting, the plan co-presence guard (getActivePlan), Zod amount validation (creditGrant), and refId idempotency. Add an integration test proving an ObjectId-keyed org is credited (no string ghost doc), idempotent re-runs, and already-granted orgs are skipped. Resolves the critical + two mediums raised in pre-merge review. Claude-Session: https://claude.ai/code/session_01S8zb8xNiyALVu366vzyi8x * test(billing): assert grant is skipped when membership creation fails Cover the rollback path: when MembershipRepository.create throws, create() re-throws before reaching the grant call, so grantOnSignup must never fire. Locks in the placement of the best-effort grant after the rollback try/catch. Claude-Session: https://claude.ai/code/session_01S8zb8xNiyALVu366vzyi8x
1 parent 997c51d commit 7c7c0bf

4 files changed

Lines changed: 307 additions & 0 deletions

File tree

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
/**
2+
* Migration: Backfill the one-shot signupGrant to plan orgs created without it.
3+
*
4+
* The signupGrant was historically credited only on the invite/verify
5+
* org-creation path (organizations.service.js::createOrganizationForUser). The
6+
* generic path behind POST /api/organizations (organizations.crud.service.js)
7+
* never credited it, so orgs created that way — e.g. via a manual "create
8+
* workspace" screen — started at 0 balance. The code gap is fixed alongside
9+
* this migration; this repairs already-affected orgs.
10+
*
11+
* Delegates to BillingSignupGrantService.grantOnSignup per org so it goes
12+
* through the EXACT runtime grant path rather than re-implementing the ledger
13+
* write. That reuse is deliberate — it inherits, for free and consistently:
14+
* - ObjectId casting: the repository writes via the Mongoose model, whose
15+
* `organization` field is Schema.ObjectId, so the string orgId is cast to a
16+
* real ObjectId (a raw-driver write keyed on a string would create orphan,
17+
* app-invisible documents that never match existing ones).
18+
* - the plan-definition co-presence guard (BillingPlanService.getActivePlan
19+
* refuses a signupGrant configured without oneShot).
20+
* - Zod amount validation (creditGrant → ExtraBalanceCreditGrant.parse).
21+
* - idempotency via the synthetic refId `signup_grant-<orgId>`, so an org
22+
* already credited (at signup, by the earlier 20260511 backfill, or by a
23+
* prior run of this one) is a no-op.
24+
*
25+
* Config-driven: only orgs on a plan that defines a positive signupGrant are
26+
* touched; a project that configures none is a no-op. This generalises the
27+
* earlier 20260511 backfill, which hardcoded 500 and enumerated the
28+
* `subscriptions` collection (missing free orgs with no subscription document);
29+
* this one enumerates the `organizations` collection.
30+
*
31+
* Safe to run while the app is live: each grant is a single-document atomic
32+
* ledger push and the migration runner serialises execution via a DB-level claim.
33+
*/
34+
import mongoose from 'mongoose';
35+
import config from '../../../config/index.js';
36+
import BillingSignupGrantService from '../services/billing.signupGrant.service.js';
37+
38+
/**
39+
* @returns {Promise<void>}
40+
*/
41+
export async function up() {
42+
// Plans that define a positive signupGrant — the only orgs worth scanning.
43+
const grantPlanIds = (config?.billing?.planDefinitions ?? [])
44+
.filter((def) => def?.planId && typeof def.signupGrant === 'number' && def.signupGrant > 0)
45+
.map((def) => def.planId);
46+
47+
if (grantPlanIds.length === 0) {
48+
console.info('[migration] backfill-signup-grant: no plan defines a signupGrant — nothing to do');
49+
return;
50+
}
51+
52+
const organizations = mongoose.connection.db.collection('organizations');
53+
const cursor = organizations.find(
54+
{ plan: { $in: grantPlanIds } },
55+
{ projection: { _id: 1, plan: 1 } },
56+
);
57+
58+
let granted = 0;
59+
let skipped = 0;
60+
let failed = 0;
61+
62+
for await (const org of cursor) {
63+
// grantOnSignup is idempotent (refId) and never throws — reuses the runtime
64+
// path so ObjectId casting + validation + co-presence guard all apply.
65+
const result = await BillingSignupGrantService.grantOnSignup({
66+
orgId: org._id.toString(),
67+
planId: org.plan,
68+
});
69+
70+
if (result == null) {
71+
// Plan has no exposed grant (co-presence guard) or a swallowed error — logged by the service.
72+
failed += 1;
73+
} else if (result.applied === false) {
74+
// Idempotent no-op — org already had a signup_grant entry.
75+
skipped += 1;
76+
} else {
77+
granted += 1;
78+
}
79+
}
80+
81+
console.info(`[migration] backfill-signup-grant: complete — granted=${granted} skipped=${skipped} failed=${failed}`);
82+
}
83+
84+
/**
85+
* Reverse: intentional no-op.
86+
*
87+
* This migration shares the `signup_grant` refId scheme with the app's runtime
88+
* grant and the earlier 20260511 backfill, so signup_grant ledger entries cannot
89+
* be attributed to this migration specifically. A blanket removal would revert
90+
* legitimately-earned grants too. Reverting is left to a manual, audited
91+
* operation if ever required.
92+
*
93+
* @returns {Promise<void>}
94+
*/
95+
export async function down() {
96+
console.warn('[migration] backfill-signup-grant DOWN: intentional no-op — signup_grant entries are not attributable to this migration; revert manually if required.');
97+
}
Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
/**
2+
* Module dependencies.
3+
*/
4+
import mongoose from 'mongoose';
5+
import { describe, beforeAll, afterEach, afterAll, test, expect } from '@jest/globals';
6+
7+
import mongooseService from '../../../lib/services/mongoose.js';
8+
import config from '../../../config/index.js';
9+
import { up as backfillSignupGrants } from '../migrations/20260707100000-backfill-missing-signup-grant-credits.js';
10+
11+
/**
12+
* Integration tests for the signup-grant backfill migration.
13+
*
14+
* Regression guard for the string-vs-ObjectId bug: billingextrabalances.organization
15+
* is Schema.ObjectId, so the migration MUST credit orgs through a path that casts to
16+
* ObjectId (it delegates to grantOnSignup → the Mongoose repository) — a raw-driver
17+
* write keyed on a string would create orphan, app-invisible ghost documents.
18+
*/
19+
describe('backfill-missing-signup-grant migration (integration):', () => {
20+
let organizations;
21+
let extraBalances;
22+
let grantPlanId;
23+
let grantAmount;
24+
let createdOrgIds = [];
25+
26+
/**
27+
* Seed an organization on the given plan and track it for cleanup.
28+
* @param {string} plan - The plan id to set on the org.
29+
* @returns {Promise<import('mongoose').Types.ObjectId>} The new org ObjectId.
30+
*/
31+
const seedOrg = async (plan) => {
32+
const _id = new mongoose.Types.ObjectId();
33+
await organizations.insertOne({ _id, name: `IT ${_id.toString()}`, plan });
34+
createdOrgIds.push(_id);
35+
return _id;
36+
};
37+
38+
beforeAll(async () => {
39+
await mongooseService.loadModels();
40+
await mongooseService.connect();
41+
organizations = mongoose.connection.db.collection('organizations');
42+
extraBalances = mongoose.connection.db.collection('billingextrabalances');
43+
const def = (config?.billing?.planDefinitions ?? []).find(
44+
(d) => typeof d.signupGrant === 'number' && d.signupGrant > 0,
45+
);
46+
grantPlanId = def.planId;
47+
grantAmount = def.signupGrant;
48+
});
49+
50+
afterEach(async () => {
51+
if (createdOrgIds.length) {
52+
await organizations.deleteMany({ _id: { $in: createdOrgIds } });
53+
await extraBalances.deleteMany({ organization: { $in: createdOrgIds } });
54+
createdOrgIds = [];
55+
}
56+
});
57+
58+
afterAll(async () => {
59+
await mongooseService.disconnect();
60+
});
61+
62+
test('credits the configured grant to an ObjectId-keyed org missing it, readable by ObjectId', async () => {
63+
const orgId = await seedOrg(grantPlanId);
64+
65+
await backfillSignupGrants();
66+
67+
// Stored + readable keyed on the ObjectId — the critical-fix assertion.
68+
const eb = await extraBalances.findOne({ organization: orgId });
69+
expect(eb).not.toBeNull();
70+
expect(eb.cachedBalance).toBe(grantAmount);
71+
const grant = (eb.ledger || []).find((e) => e.source === 'signup_grant');
72+
expect(grant).toBeDefined();
73+
expect(grant.amount).toBe(grantAmount);
74+
expect(grant.refId).toBe(`signup_grant-${orgId.toString()}`);
75+
// A string-keyed lookup must NOT find a duplicate ghost document.
76+
expect(await extraBalances.findOne({ organization: orgId.toString() })).toBeNull();
77+
});
78+
79+
test('is idempotent — a second run does not double-credit', async () => {
80+
const orgId = await seedOrg(grantPlanId);
81+
82+
await backfillSignupGrants();
83+
await backfillSignupGrants();
84+
85+
const docs = await extraBalances.find({ organization: orgId }).toArray();
86+
expect(docs).toHaveLength(1);
87+
expect(docs[0].cachedBalance).toBe(grantAmount);
88+
const grants = (docs[0].ledger || []).filter((e) => e.source === 'signup_grant');
89+
expect(grants).toHaveLength(1);
90+
});
91+
92+
test('skips an org that already holds a signup_grant entry', async () => {
93+
const orgId = await seedOrg(grantPlanId);
94+
const repo = (await import('../repositories/billing.extraBalance.repository.js')).default;
95+
await repo.creditGrant(orgId.toString(), grantAmount, 'signup_grant');
96+
97+
await backfillSignupGrants();
98+
99+
const docs = await extraBalances.find({ organization: orgId }).toArray();
100+
expect(docs).toHaveLength(1);
101+
const grants = (docs[0].ledger || []).filter((e) => e.source === 'signup_grant');
102+
expect(grants).toHaveLength(1);
103+
});
104+
});

modules/organizations/services/organizations.crud.service.js

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ const normalizeDomain = (value = '') => value.trim().toLowerCase();
2323
import OrganizationsRepository from '../repositories/organizations.repository.js';
2424
import MembershipRepository from '../repositories/organizations.membership.repository.js';
2525
import UserService from '../../users/services/users.service.js';
26+
import BillingSignupGrantService from '../../billing/services/billing.signupGrant.service.js';
2627
import { slugify } from '../helpers/organizations.slug.js';
2728
import { MEMBERSHIP_STATUSES, MEMBERSHIP_ROLES } from '../lib/constants.js';
2829
import { runOrganizationRemovedHandlers } from '../lib/orgRemoval.registry.js';
@@ -120,6 +121,14 @@ const create = async (body, user) => {
120121
throw err;
121122
}
122123

124+
// Best-effort signup grant — mirrors organizations.service.js::createOrganizationForUser.
125+
// Every org-creation path (including this generic one behind POST /api/organizations)
126+
// must credit the configured one-shot signupGrant, else a fresh org on a plan that
127+
// defines one starts at 0 balance. Called outside the rollback try/catch so a billing
128+
// failure never rolls back the org; grantOnSignup never throws and is idempotent
129+
// (refId signup_grant-<orgId>). No-op when the plan defines no signupGrant.
130+
await BillingSignupGrantService.grantOnSignup({ orgId: result._id.toString(), planId: result.plan || 'free' });
131+
123132
return result;
124133
};
125134

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
/**
2+
* Module dependencies.
3+
*/
4+
import { jest, describe, test, expect, beforeEach } from '@jest/globals';
5+
6+
/**
7+
* Unit tests — verify organizations.crud.service.create() credits the one-shot
8+
* signup grant on the generic POST /api/organizations path, mirroring the
9+
* invite/verify path (organizations.service.js::createOrganizationForUser). A
10+
* fresh org on a plan that defines a signupGrant must not start at 0 balance.
11+
*/
12+
13+
const mockGrantOnSignup = jest.fn().mockResolvedValue({ applied: true });
14+
jest.unstable_mockModule('../../billing/services/billing.signupGrant.service.js', () => ({
15+
default: { grantOnSignup: mockGrantOnSignup },
16+
}));
17+
18+
const mockOrgCreate = jest.fn().mockResolvedValue({ _id: 'org1', plan: 'free' });
19+
const mockOrgRemove = jest.fn().mockResolvedValue({});
20+
jest.unstable_mockModule('../repositories/organizations.repository.js', () => ({
21+
default: {
22+
create: mockOrgCreate,
23+
findOne: jest.fn().mockResolvedValue(null),
24+
remove: mockOrgRemove,
25+
list: jest.fn().mockResolvedValue([]),
26+
update: jest.fn(),
27+
get: jest.fn(),
28+
exists: jest.fn().mockResolvedValue(false),
29+
},
30+
}));
31+
32+
const mockMembershipCreate = jest.fn().mockResolvedValue({ _id: 'm1' });
33+
jest.unstable_mockModule('../repositories/organizations.membership.repository.js', () => ({
34+
default: {
35+
create: mockMembershipCreate,
36+
deleteMany: jest.fn(),
37+
list: jest.fn().mockResolvedValue([]),
38+
findOne: jest.fn().mockResolvedValue(null),
39+
update: jest.fn(),
40+
remove: jest.fn(),
41+
count: jest.fn(),
42+
},
43+
}));
44+
45+
jest.unstable_mockModule('../../users/services/users.service.js', () => ({
46+
default: {
47+
updateById: jest.fn().mockResolvedValue({}),
48+
findWithFilter: jest.fn().mockResolvedValue([]),
49+
getBrut: jest.fn(),
50+
},
51+
}));
52+
53+
jest.unstable_mockModule('../../../lib/helpers/emailVerification.js', () => ({
54+
assertEmailVerified: jest.fn(),
55+
}));
56+
57+
jest.unstable_mockModule('../../../lib/services/logger.js', () => ({
58+
default: { error: jest.fn(), warn: jest.fn(), info: jest.fn() },
59+
}));
60+
61+
jest.unstable_mockModule('../../../config/index.js', () => ({
62+
default: { organizations: { enabled: false } },
63+
}));
64+
65+
const { default: OrgCrudService } = await import('../services/organizations.crud.service.js');
66+
67+
describe('organizations.crud.service.create signup grant:', () => {
68+
beforeEach(() => {
69+
mockGrantOnSignup.mockClear();
70+
mockOrgCreate.mockResolvedValue({ _id: 'org1', plan: 'free' });
71+
});
72+
73+
test('credits the one-shot signup grant for the freshly created org', async () => {
74+
const user = { id: 'u1', _id: 'u1', email: 'a@b.com', emailVerified: true };
75+
const result = await OrgCrudService.create({ name: 'Test Org' }, user);
76+
77+
expect(result).toEqual({ _id: 'org1', plan: 'free' });
78+
expect(mockGrantOnSignup).toHaveBeenCalledTimes(1);
79+
expect(mockGrantOnSignup).toHaveBeenCalledWith({ orgId: 'org1', planId: 'free' });
80+
});
81+
82+
test('defaults planId to free when the created org has no plan field', async () => {
83+
mockOrgCreate.mockResolvedValueOnce({ _id: 'org2' });
84+
const user = { id: 'u1', _id: 'u1', email: 'a@b.com', emailVerified: true };
85+
await OrgCrudService.create({ name: 'No Plan Org' }, user);
86+
87+
expect(mockGrantOnSignup).toHaveBeenCalledWith({ orgId: 'org2', planId: 'free' });
88+
});
89+
90+
test('does not credit the grant when membership creation fails (rollback exits before the grant)', async () => {
91+
mockMembershipCreate.mockRejectedValueOnce(new Error('membership boom'));
92+
const user = { id: 'u1', _id: 'u1', email: 'a@b.com', emailVerified: true };
93+
94+
await expect(OrgCrudService.create({ name: 'Rollback Org' }, user)).rejects.toThrow();
95+
expect(mockGrantOnSignup).not.toHaveBeenCalled();
96+
});
97+
});

0 commit comments

Comments
 (0)