From 9ae5fd4c95decc4fc2bff76458ace98bed7f1249 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Wed, 5 Aug 2026 14:20:06 +0200 Subject: [PATCH 1/4] test(node-4020): config-agnostic assertions + generic org-creation seam isolation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Applies the epic's 4 homogeneity rules to the test files a consumer currently has to adapt: - billing quota: force an explicit dev-grade NODE_ENV (set/restore) instead of riding whatever ambient value a consumer's own test runner uses. - billing subscription schema: derive plan ids from the loaded config instead of a hardcoded devkit default; add a synthetic-catalogue block proving the schema is config-driven, not a re-labeled default. - organizations email-verification + silent-catch: mock the org-creation event emitter generically (the hook mechanism itself) instead of leaving it real, so no consumer-named listener is ever needed to isolate these tests; add coverage proving a generically-registered listener receives the documented payload. - invitations open-signup: the one assertion riding the ambient config.invitations.userFacing default now installs its own value (set/restore) — the true/false split is already covered elsewhere in the file. - new organizationAbilities unit suite: admin/user × owner/admin/member/ no-membership matrix over the CASL ability builder, generic policy coverage the stack lacked. Closes #4020 --- .../billing/tests/billing.quota.unit.tests.js | 11 ++ modules/billing/tests/billing.unit.tests.js | 72 ++++++++- .../tests/invitations.integration.tests.js | 36 +++-- .../organizations.abilities.unit.tests.js | 142 ++++++++++++++++++ ...anizations.emailVerification.unit.tests.js | 48 ++++++ ...zations.service.silent.catch.unit.tests.js | 9 ++ 6 files changed, 301 insertions(+), 17 deletions(-) create mode 100644 modules/organizations/tests/organizations.abilities.unit.tests.js diff --git a/modules/billing/tests/billing.quota.unit.tests.js b/modules/billing/tests/billing.quota.unit.tests.js index e62fb0b0d..6a1859bf7 100644 --- a/modules/billing/tests/billing.quota.unit.tests.js +++ b/modules/billing/tests/billing.quota.unit.tests.js @@ -17,8 +17,17 @@ describe('requireQuota middleware:', () => { let req; let res; let next; + let originalNodeEnv; beforeEach(async () => { + // rule 3 (Node#4020): lib/helpers/responses.js only serializes the raw + // `payload.error` blob in a dev-grade NODE_ENV (lib/helpers/config.js + // isProd()/DEV_ENVS). The assertions below read that field, so force an + // explicit dev-grade value here rather than depend on whichever NODE_ENV + // a consumer happens to run its own test suite under. + originalNodeEnv = process.env.NODE_ENV; + process.env.NODE_ENV = 'test'; + jest.resetModules(); mockBillingQuotaService = { @@ -47,6 +56,8 @@ describe('requireQuota middleware:', () => { afterEach(() => { jest.restoreAllMocks(); + if (originalNodeEnv === undefined) delete process.env.NODE_ENV; + else process.env.NODE_ENV = originalNodeEnv; }); // ── Organization guard ──────────────────────────────────────────────────── diff --git a/modules/billing/tests/billing.unit.tests.js b/modules/billing/tests/billing.unit.tests.js index d2075c8df..da640080d 100644 --- a/modules/billing/tests/billing.unit.tests.js +++ b/modules/billing/tests/billing.unit.tests.js @@ -1,6 +1,7 @@ /** * Module dependencies. */ +import { jest } from '@jest/globals'; import config from '../../../config/index.js'; import schema from '../models/billing.subscription.schema.js'; @@ -8,6 +9,11 @@ import schema from '../models/billing.subscription.schema.js'; * Unit tests */ describe('Billing unit tests:', () => { + // rule 1 (Node#4020): pick a plan from the LOADED config instead of hardcoding + // a devkit-default plan id ('pro'/'starter') — a consumer with a different plan + // catalogue must still pass these SubscriptionUpdate assertions. + const nonDefaultPlan = config.billing.plans.find((plan) => plan !== config.billing.defaultPlan) ?? config.billing.plans[0]; + describe('Subscription schema', () => { let subscription; @@ -105,7 +111,7 @@ describe('Billing unit tests:', () => { }); test('should strip unknown fields with SubscriptionUpdate', (done) => { - const update = { plan: 'pro', unknown: 'field' }; + const update = { plan: nonDefaultPlan, unknown: 'field' }; const result = schema.SubscriptionUpdate.safeParse(update); expect(result.error).toBeFalsy(); expect(result.data?.unknown).toBeUndefined(); @@ -113,10 +119,10 @@ describe('Billing unit tests:', () => { }); test('should allow partial updates with SubscriptionUpdate', (done) => { - const update = { plan: 'starter' }; + const update = { plan: nonDefaultPlan }; const result = schema.SubscriptionUpdate.safeParse(update); expect(result.error).toBeFalsy(); - expect(result.data.plan).toBe('starter'); + expect(result.data.plan).toBe(nonDefaultPlan); done(); }); @@ -173,10 +179,10 @@ describe('Billing unit tests:', () => { }); test('should not inject defaults in SubscriptionUpdate partial', (done) => { - const update = { plan: 'pro' }; + const update = { plan: nonDefaultPlan }; const result = schema.SubscriptionUpdate.safeParse(update); expect(result.error).toBeFalsy(); - expect(result.data.plan).toBe('pro'); + expect(result.data.plan).toBe(nonDefaultPlan); expect(result.data.status).toBeUndefined(); done(); }); @@ -330,4 +336,60 @@ describe('Billing unit tests:', () => { expect(result.error).toBeDefined(); }); }); + + // rule 1 (Node#4020): the two SubscriptionUpdate fixes above prove the tests no + // longer HARDCODE a devkit-default plan id, but they still run against the real + // devkit-default catalogue (free/starter/pro/enterprise). This block proves the + // schema itself is config-driven — not merely re-labeling the same default — + // by rebuilding it against a synthetic consumer profile with a totally different + // plan catalogue and confirming the enum follows the config, not a stack default. + describe('Subscription schema — synthetic consumer plan catalogue:', () => { + const syntheticPlans = ['launch', 'grow', 'scale']; + const syntheticDefaultPlan = 'launch'; + let SyntheticSchema; + + beforeAll(async () => { + jest.resetModules(); + jest.unstable_mockModule('../../../config/index.js', () => ({ + default: { + billing: { + plans: syntheticPlans, + defaultPlan: syntheticDefaultPlan, + statuses: ['active', 'canceled'], + }, + }, + })); + SyntheticSchema = (await import('../models/billing.subscription.schema.js')).default; + }); + + afterAll(() => { + jest.resetModules(); + }); + + test('accepts every plan from the synthetic catalogue (none of which exist in the devkit default)', () => { + for (const plan of syntheticPlans) { + const result = SyntheticSchema.Subscription.safeParse({ + organization: '507f1f77bcf86cd799439011', + plan, + status: 'active', + }); + expect(result.error).toBeFalsy(); + } + }); + + test('rejects a devkit-default plan id that is absent from the synthetic catalogue', () => { + const result = SyntheticSchema.Subscription.safeParse({ + organization: '507f1f77bcf86cd799439011', + plan: 'free', + status: 'active', + }); + expect(result.error).toBeDefined(); + }); + + test('SubscriptionUpdate round-trips a synthetic-catalogue plan id', () => { + const result = SyntheticSchema.SubscriptionUpdate.safeParse({ plan: syntheticPlans[1] }); + expect(result.error).toBeFalsy(); + expect(result.data.plan).toBe(syntheticPlans[1]); + }); + }); }); diff --git a/modules/invitations/tests/invitations.integration.tests.js b/modules/invitations/tests/invitations.integration.tests.js index c974542e6..1ef52df05 100644 --- a/modules/invitations/tests/invitations.integration.tests.js +++ b/modules/invitations/tests/invitations.integration.tests.js @@ -536,21 +536,33 @@ describe('Signup invitations:', () => { const created = await adminAgent.post('/api/invitations').send({ email }); const { token } = created.body.data; + // rules 1+3 (Node#4020): this P2 invariant holds specifically for + // userFacing:false (the byte-for-byte-unchanged baseline — the userFacing:true + // variant is covered separately by the dedicated "Open-signup userFacing" + // block below). Install that value explicitly rather than ride the config's + // ambient default, so this assertion never silently flips under a consumer + // whose default deployment config sets userFacing:true. + const originalUserFacing = config.invitations.userFacing; + config.invitations.userFacing = false; + // Public signup OPEN: a token may be presented but is not required. config.sign.up = true; config.sign.cap = null; - const res = await request(app) - .post(`/api/auth/signup?inviteToken=${token}`) - .send({ email, password: 'Sup3rStr0ng!' }); - expect(res.status).toBe(200); - - // The invite must remain VALID (presented, not consumed) — not stuck mid-claim. - const verify = await request(app).get(`/api/invitations/verify/${token}`); - expect(verify.body.data.valid).toBe(true); - try { - const u = await UserService.getBrut({ email }); - if (u) await UserService.remove(u); - } catch (_) { /* cleanup */ } + const res = await request(app) + .post(`/api/auth/signup?inviteToken=${token}`) + .send({ email, password: 'Sup3rStr0ng!' }); + expect(res.status).toBe(200); + + // The invite must remain VALID (presented, not consumed) — not stuck mid-claim. + const verify = await request(app).get(`/api/invitations/verify/${token}`); + expect(verify.body.data.valid).toBe(true); + } finally { + config.invitations.userFacing = originalUserFacing; + try { + const u = await UserService.getBrut({ email }); + if (u) await UserService.remove(u); + } catch (_) { /* cleanup */ } + } }); test('crash recovery: a stale claim (older than the window) is swept and the invite becomes reusable', async () => { diff --git a/modules/organizations/tests/organizations.abilities.unit.tests.js b/modules/organizations/tests/organizations.abilities.unit.tests.js new file mode 100644 index 000000000..8c17821d0 --- /dev/null +++ b/modules/organizations/tests/organizations.abilities.unit.tests.js @@ -0,0 +1,142 @@ +/** + * Unit tests for organizationAbilities — the CASL ability-definition function + * for the organizations module. Covers the full admin/user × owner/admin/member/ + * no-membership matrix (Node#4020, generic policy coverage the stack lacked). + * + * `can`/`cannot` are plain spies (no real CASL AbilityBuilder) — same convention + * as modules/invitations/tests/invitations.policy.unit.tests.js. + */ +import { describe, test, expect, jest } from '@jest/globals'; + +import { organizationAbilities } from '../policies/organizations.policy.js'; +import { MEMBERSHIP_ROLES } from '../lib/constants.js'; + +/** + * Build a membership fixture with a populated organizationId (the shape + * MembershipRepository.findOne's defaultPopulate returns in production). + * @param {string} role - one of MEMBERSHIP_ROLES + * @param {string} organizationId - hex-ish id string + * @returns {Object} membership fixture + */ +function membershipFixture(role, organizationId = '507f1f77bcf86cd799439011') { + return { role, organizationId: { _id: organizationId } }; +} + +describe('organizationAbilities:', () => { + describe('admin (roles includes "admin"):', () => { + test('grants manage all, regardless of membership (no membership)', () => { + const can = jest.fn(); + const cannot = jest.fn(); + organizationAbilities({ roles: ['admin'] }, null, { can, cannot }); + + expect(can).toHaveBeenCalledWith('manage', 'all'); + expect(can).toHaveBeenCalledTimes(1); + expect(cannot).not.toHaveBeenCalled(); + }); + + test('grants manage all and short-circuits even when a membership is present (owner)', () => { + const can = jest.fn(); + const cannot = jest.fn(); + organizationAbilities({ roles: ['admin'] }, membershipFixture(MEMBERSHIP_ROLES.OWNER), { can, cannot }); + + expect(can).toHaveBeenCalledWith('manage', 'all'); + // The admin branch returns immediately — never also grants the non-admin/ + // membership-scoped abilities below. + expect(can).toHaveBeenCalledTimes(1); + expect(cannot).not.toHaveBeenCalled(); + }); + }); + + describe('non-admin user — no membership:', () => { + test('grants only create Organization', () => { + const can = jest.fn(); + const cannot = jest.fn(); + organizationAbilities({ roles: ['user'] }, null, { can, cannot }); + + expect(can).toHaveBeenCalledWith('create', 'Organization'); + expect(can).toHaveBeenCalledTimes(1); + expect(cannot).not.toHaveBeenCalled(); + }); + + test('grants create Organization even when roles is absent (non-array falls through, not admin)', () => { + const can = jest.fn(); + organizationAbilities({}, null, { can, cannot: jest.fn() }); + + expect(can).toHaveBeenCalledWith('create', 'Organization'); + expect(can).toHaveBeenCalledTimes(1); + }); + }); + + describe('non-admin user — membership.role = owner:', () => { + test('grants create Organization + manage Organization/Membership scoped to the org', () => { + const can = jest.fn(); + const cannot = jest.fn(); + const membership = membershipFixture(MEMBERSHIP_ROLES.OWNER, 'org-owner-1'); + organizationAbilities({ roles: ['user'] }, membership, { can, cannot }); + + expect(can).toHaveBeenCalledWith('create', 'Organization'); + expect(can).toHaveBeenCalledWith('manage', 'Organization', { _id: 'org-owner-1' }); + expect(can).toHaveBeenCalledWith('manage', 'Membership', { organizationId: 'org-owner-1' }); + expect(can).toHaveBeenCalledTimes(3); + expect(cannot).not.toHaveBeenCalled(); + }); + }); + + describe('non-admin user — membership.role = admin:', () => { + test('grants create Organization + read/update Organization (not delete) + read/create/delete Membership', () => { + const can = jest.fn(); + const cannot = jest.fn(); + const membership = membershipFixture(MEMBERSHIP_ROLES.ADMIN, 'org-admin-1'); + organizationAbilities({ roles: ['user'] }, membership, { can, cannot }); + + expect(can).toHaveBeenCalledWith('create', 'Organization'); + expect(can).toHaveBeenCalledWith('read', 'Organization', { _id: 'org-admin-1' }); + expect(can).toHaveBeenCalledWith('update', 'Organization', { _id: 'org-admin-1' }); + expect(can).toHaveBeenCalledWith('read', 'Membership', { organizationId: 'org-admin-1' }); + expect(can).toHaveBeenCalledWith('create', 'Membership', { organizationId: 'org-admin-1' }); + expect(can).toHaveBeenCalledWith('delete', 'Membership', { organizationId: 'org-admin-1' }); + expect(can).toHaveBeenCalledTimes(6); + + expect(cannot).toHaveBeenCalledWith('delete', 'Organization'); + expect(cannot).toHaveBeenCalledTimes(1); + }); + }); + + describe('non-admin user — membership.role = member:', () => { + test('grants create Organization + read-only Organization/Membership, no write abilities', () => { + const can = jest.fn(); + const cannot = jest.fn(); + const membership = membershipFixture(MEMBERSHIP_ROLES.MEMBER, 'org-member-1'); + organizationAbilities({ roles: ['user'] }, membership, { can, cannot }); + + expect(can).toHaveBeenCalledWith('create', 'Organization'); + expect(can).toHaveBeenCalledWith('read', 'Organization', { _id: 'org-member-1' }); + expect(can).toHaveBeenCalledWith('read', 'Membership', { organizationId: 'org-member-1' }); + expect(can).toHaveBeenCalledTimes(3); + expect(cannot).not.toHaveBeenCalled(); + + expect(can).not.toHaveBeenCalledWith('manage', 'Organization', expect.anything()); + expect(can).not.toHaveBeenCalledWith('update', 'Organization', expect.anything()); + expect(can).not.toHaveBeenCalledWith('delete', expect.anything(), expect.anything()); + }); + }); + + describe('membership.organizationId shape — populated vs. raw id:', () => { + test('scopes abilities to the raw id when organizationId is NOT populated (no ._id)', () => { + const can = jest.fn(); + const membership = { role: MEMBERSHIP_ROLES.OWNER, organizationId: 'org-raw-1' }; + organizationAbilities({ roles: ['user'] }, membership, { can, cannot: jest.fn() }); + + expect(can).toHaveBeenCalledWith('manage', 'Organization', { _id: 'org-raw-1' }); + expect(can).toHaveBeenCalledWith('manage', 'Membership', { organizationId: 'org-raw-1' }); + }); + + test('scopes abilities to the populated ._id when organizationId IS populated', () => { + const can = jest.fn(); + const membership = membershipFixture(MEMBERSHIP_ROLES.OWNER, 'org-populated-1'); + organizationAbilities({ roles: ['user'] }, membership, { can, cannot: jest.fn() }); + + expect(can).toHaveBeenCalledWith('manage', 'Organization', { _id: 'org-populated-1' }); + }); + }); +}); diff --git a/modules/organizations/tests/organizations.emailVerification.unit.tests.js b/modules/organizations/tests/organizations.emailVerification.unit.tests.js index 9f3c20e27..2ad05ac10 100644 --- a/modules/organizations/tests/organizations.emailVerification.unit.tests.js +++ b/modules/organizations/tests/organizations.emailVerification.unit.tests.js @@ -2,10 +2,20 @@ * Unit tests for email verification gates on organization operations. */ import mongoose from 'mongoose'; +import { EventEmitter } from 'events'; import { jest, describe, test, expect, beforeEach } from '@jest/globals'; // --- Mocks --- +// rule 2 (Node#4020): isolate the org-creation seam GENERICALLY — mock the hook +// mechanism itself (lib/events.js, a plain EventEmitter registry any consumer +// module can subscribe to), never a specific consumer's own listener module. +// A real EventEmitter instance (not a bare jest.fn() double) so the "generic +// extension point" tests below can register/remove listeners on it exactly like +// a consumer would, without ever importing or naming a real consumer module. +const organizationEventsFixture = new EventEmitter(); +jest.unstable_mockModule('../lib/events.js', () => ({ default: organizationEventsFixture })); + const mockIsConfigured = jest.fn(); jest.unstable_mockModule('../../../lib/helpers/mailer/index.js', () => ({ default: { isConfigured: mockIsConfigured, sendMail: jest.fn() }, @@ -280,4 +290,42 @@ describe('Email verification gates:', () => { expect(res.status).toHaveBeenCalledWith(200); }); }); + + // --- organization-creation seam (generic extension point) --- + + describe('organization-creation seam (generic extension point):', () => { + test('a generically-registered listener on organizationEvents receives organization.created + organization.provisioned with their documented payload shape', async () => { + // Stand-in for "whatever a consumer registers on the seam" (e.g. billing's + // signupGrant listener, per lib/events.js's doc comment) — a plain function + // attached via the SAME .on() contract any consumer uses. Never references + // a real consumer module, so this proves the seam generically (rule 2). + const createdListener = jest.fn(); + const provisionedListener = jest.fn(); + organizationEventsFixture.on('organization.created', createdListener); + organizationEventsFixture.on('organization.provisioned', provisionedListener); + + try { + mockIsConfigured.mockReturnValue(false); + + const fakeOrg = { _id: new mongoose.Types.ObjectId(), name: 'Test', toJSON: () => ({ name: 'Test' }) }; + const fakeMembership = { _id: new mongoose.Types.ObjectId(), role: 'owner' }; + mockOrganizationsRepositoryCreate.mockResolvedValue(fakeOrg); + mockMembershipRepositoryCreate.mockResolvedValue(fakeMembership); + mockUpdateById.mockResolvedValue({}); + + const user = { id: fakeUserId.toString(), email: 'test@acme.com', firstName: 'Test', lastName: 'User', emailVerified: false }; + await OrganizationsService.handleSignupOrganization(user); + + expect(createdListener).toHaveBeenCalledWith({ orgId: fakeOrg._id.toString(), planId: 'free' }); + expect(provisionedListener).toHaveBeenCalledWith({ + userId: fakeUserId.toString(), + organizationId: fakeOrg._id.toString(), + }); + } finally { + // Never leak a listener into a sibling test — each test owns its own. + organizationEventsFixture.off('organization.created', createdListener); + organizationEventsFixture.off('organization.provisioned', provisionedListener); + } + }); + }); }); diff --git a/modules/organizations/tests/organizations.service.silent.catch.unit.tests.js b/modules/organizations/tests/organizations.service.silent.catch.unit.tests.js index 0c8a3c904..e1ac7a928 100644 --- a/modules/organizations/tests/organizations.service.silent.catch.unit.tests.js +++ b/modules/organizations/tests/organizations.service.silent.catch.unit.tests.js @@ -68,6 +68,15 @@ jest.unstable_mockModule('../helpers/organizations.slug.js', () => ({ generateOrganizationSlug: jest.fn().mockResolvedValue('test-org'), })); +// rule 2 (Node#4020): isolate the org-creation seam GENERICALLY — mock the hook +// mechanism itself (lib/events.js, a plain EventEmitter registry any consumer +// module can subscribe to), never a specific consumer's own listener module. +// This keeps the rollback-logging assertion below independent of whatever a +// consumer has registered on 'organization.created' / 'organization.provisioned'. +jest.unstable_mockModule('../lib/events.js', () => ({ + default: { emit: jest.fn(), on: jest.fn() }, +})); + const { default: OrgService } = await import('../services/organizations.service.js'); describe('organizations.service silent-catch error logging:', () => { From 2e4d76cfdc8e00e8344a16a32b13ca81e6c81f10 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Wed, 5 Aug 2026 14:31:18 +0200 Subject: [PATCH 2/4] refactor(simplify): dedup event-seam coverage, extract test helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Simplify pass over the Node#4020 test-homogeneity batch: - organizations.emailVerification: drop the new real-EventEmitter seam block and switch to the same bare-double lib/events.js mock already used elsewhere in the module — organizations.service.signup.unit.tests.js already covers the organization.created/organization.provisioned payload shape and ordering generically, so this file only needed isolation. - organizations.abilities: extract a callAbilities(user, membership) helper to remove the repeated can/cannot spy boilerplate across the matrix. - invitations.integration: fold the userFacing save/restore into the describe block's existing beforeEach/afterEach (same pattern already used for sign.up/sign.cap), dropping the test's bespoke try/finally. - billing.quota: hoist the NODE_ENV save/restore to beforeAll/afterAll — the value never changes per test. Re-verified: full test:coverage suite green, plus the config/NODE_ENV- independent files re-checked under a synthetic non-dev NODE_ENV. --- .../billing/tests/billing.quota.unit.tests.js | 16 ++++-- .../tests/invitations.integration.tests.js | 43 ++++++++------- .../organizations.abilities.unit.tests.js | 46 ++++++++-------- ...anizations.emailVerification.unit.tests.js | 53 ++++--------------- 4 files changed, 63 insertions(+), 95 deletions(-) diff --git a/modules/billing/tests/billing.quota.unit.tests.js b/modules/billing/tests/billing.quota.unit.tests.js index 6a1859bf7..0970bbab0 100644 --- a/modules/billing/tests/billing.quota.unit.tests.js +++ b/modules/billing/tests/billing.quota.unit.tests.js @@ -1,7 +1,7 @@ /** * Module dependencies. */ -import { jest, describe, test, beforeEach, afterEach, expect } from '@jest/globals'; +import { jest, describe, test, beforeAll, afterAll, beforeEach, afterEach, expect } from '@jest/globals'; import AppError from '../../../lib/helpers/AppError.js'; /** @@ -19,15 +19,23 @@ describe('requireQuota middleware:', () => { let next; let originalNodeEnv; - beforeEach(async () => { + beforeAll(() => { // rule 3 (Node#4020): lib/helpers/responses.js only serializes the raw // `payload.error` blob in a dev-grade NODE_ENV (lib/helpers/config.js // isProd()/DEV_ENVS). The assertions below read that field, so force an // explicit dev-grade value here rather than depend on whichever NODE_ENV - // a consumer happens to run its own test suite under. + // a consumer happens to run its own test suite under. Same value for + // every test in this file, so set once for the suite rather than per test. originalNodeEnv = process.env.NODE_ENV; process.env.NODE_ENV = 'test'; + }); + afterAll(() => { + if (originalNodeEnv === undefined) delete process.env.NODE_ENV; + else process.env.NODE_ENV = originalNodeEnv; + }); + + beforeEach(async () => { jest.resetModules(); mockBillingQuotaService = { @@ -56,8 +64,6 @@ describe('requireQuota middleware:', () => { afterEach(() => { jest.restoreAllMocks(); - if (originalNodeEnv === undefined) delete process.env.NODE_ENV; - else process.env.NODE_ENV = originalNodeEnv; }); // ── Organization guard ──────────────────────────────────────────────────── diff --git a/modules/invitations/tests/invitations.integration.tests.js b/modules/invitations/tests/invitations.integration.tests.js index 1ef52df05..f6cc04f1d 100644 --- a/modules/invitations/tests/invitations.integration.tests.js +++ b/modules/invitations/tests/invitations.integration.tests.js @@ -405,18 +405,25 @@ describe('Signup invitations:', () => { describe('E2 two-phase claim/finalize hardening', () => { let InvitationService; let InvitationRepository; - let originalUp; let originalCap; + let originalUp; let originalCap; let originalUserFacing; beforeAll(async () => { InvitationService = (await import(path.resolve('./modules/invitations/services/invitations.service.js'))).default; InvitationRepository = (await import(path.resolve('./modules/invitations/repositories/invitations.repository.js'))).default; }); - beforeEach(() => { originalUp = config.sign.up; originalCap = config.sign.cap; }); + beforeEach(() => { + originalUp = config.sign.up; + originalCap = config.sign.cap; + // rules 1+3 (Node#4020): saved/restored here even though only the + // OPEN-signup test below touches it — same block-level pattern as + // sign.up/sign.cap, so that test needs no bespoke try/finally of its own. + originalUserFacing = config.invitations.userFacing; + }); afterEach(async () => { - config.sign.up = originalUp; config.sign.cap = originalCap; + config.sign.up = originalUp; config.sign.cap = originalCap; config.invitations.userFacing = originalUserFacing; jest.restoreAllMocks(); - for (const email of ['e2-replay@example.com', 'e2-claimed@example.com', 'e2-stale@example.com', 'e2-createthrow@example.com', 'e2-concurrent@example.com', 'e2-finalize-throw@example.com', 'e2-oauth-finalize-throw@example.com', 'e2-release-throw@example.com']) { + for (const email of ['e2-replay@example.com', 'e2-claimed@example.com', 'e2-stale@example.com', 'e2-createthrow@example.com', 'e2-concurrent@example.com', 'e2-finalize-throw@example.com', 'e2-oauth-finalize-throw@example.com', 'e2-release-throw@example.com', 'e2-opensignup@example.com']) { try { const existing = await UserService.getBrut({ email }); if (existing) await UserService.remove(existing); @@ -541,28 +548,20 @@ describe('Signup invitations:', () => { // variant is covered separately by the dedicated "Open-signup userFacing" // block below). Install that value explicitly rather than ride the config's // ambient default, so this assertion never silently flips under a consumer - // whose default deployment config sets userFacing:true. - const originalUserFacing = config.invitations.userFacing; + // whose default deployment config sets userFacing:true. Restored by this + // describe block's own afterEach, same as sign.up/sign.cap below. config.invitations.userFacing = false; // Public signup OPEN: a token may be presented but is not required. config.sign.up = true; config.sign.cap = null; - try { - const res = await request(app) - .post(`/api/auth/signup?inviteToken=${token}`) - .send({ email, password: 'Sup3rStr0ng!' }); - expect(res.status).toBe(200); - - // The invite must remain VALID (presented, not consumed) — not stuck mid-claim. - const verify = await request(app).get(`/api/invitations/verify/${token}`); - expect(verify.body.data.valid).toBe(true); - } finally { - config.invitations.userFacing = originalUserFacing; - try { - const u = await UserService.getBrut({ email }); - if (u) await UserService.remove(u); - } catch (_) { /* cleanup */ } - } + const res = await request(app) + .post(`/api/auth/signup?inviteToken=${token}`) + .send({ email, password: 'Sup3rStr0ng!' }); + expect(res.status).toBe(200); + + // The invite must remain VALID (presented, not consumed) — not stuck mid-claim. + const verify = await request(app).get(`/api/invitations/verify/${token}`); + expect(verify.body.data.valid).toBe(true); }); test('crash recovery: a stale claim (older than the window) is swept and the invite becomes reusable', async () => { diff --git a/modules/organizations/tests/organizations.abilities.unit.tests.js b/modules/organizations/tests/organizations.abilities.unit.tests.js index 8c17821d0..e93f38ed9 100644 --- a/modules/organizations/tests/organizations.abilities.unit.tests.js +++ b/modules/organizations/tests/organizations.abilities.unit.tests.js @@ -22,12 +22,23 @@ function membershipFixture(role, organizationId = '507f1f77bcf86cd799439011') { return { role, organizationId: { _id: organizationId } }; } +/** + * Call organizationAbilities with fresh can/cannot spies and return them. + * @param {Object} user - user fixture + * @param {Object|null} membership - membership fixture + * @returns {{can: jest.Mock, cannot: jest.Mock}} the spies, populated by the call + */ +function callAbilities(user, membership) { + const can = jest.fn(); + const cannot = jest.fn(); + organizationAbilities(user, membership, { can, cannot }); + return { can, cannot }; +} + describe('organizationAbilities:', () => { describe('admin (roles includes "admin"):', () => { test('grants manage all, regardless of membership (no membership)', () => { - const can = jest.fn(); - const cannot = jest.fn(); - organizationAbilities({ roles: ['admin'] }, null, { can, cannot }); + const { can, cannot } = callAbilities({ roles: ['admin'] }, null); expect(can).toHaveBeenCalledWith('manage', 'all'); expect(can).toHaveBeenCalledTimes(1); @@ -35,9 +46,7 @@ describe('organizationAbilities:', () => { }); test('grants manage all and short-circuits even when a membership is present (owner)', () => { - const can = jest.fn(); - const cannot = jest.fn(); - organizationAbilities({ roles: ['admin'] }, membershipFixture(MEMBERSHIP_ROLES.OWNER), { can, cannot }); + const { can, cannot } = callAbilities({ roles: ['admin'] }, membershipFixture(MEMBERSHIP_ROLES.OWNER)); expect(can).toHaveBeenCalledWith('manage', 'all'); // The admin branch returns immediately — never also grants the non-admin/ @@ -49,9 +58,7 @@ describe('organizationAbilities:', () => { describe('non-admin user — no membership:', () => { test('grants only create Organization', () => { - const can = jest.fn(); - const cannot = jest.fn(); - organizationAbilities({ roles: ['user'] }, null, { can, cannot }); + const { can, cannot } = callAbilities({ roles: ['user'] }, null); expect(can).toHaveBeenCalledWith('create', 'Organization'); expect(can).toHaveBeenCalledTimes(1); @@ -59,8 +66,7 @@ describe('organizationAbilities:', () => { }); test('grants create Organization even when roles is absent (non-array falls through, not admin)', () => { - const can = jest.fn(); - organizationAbilities({}, null, { can, cannot: jest.fn() }); + const { can } = callAbilities({}, null); expect(can).toHaveBeenCalledWith('create', 'Organization'); expect(can).toHaveBeenCalledTimes(1); @@ -69,10 +75,8 @@ describe('organizationAbilities:', () => { describe('non-admin user — membership.role = owner:', () => { test('grants create Organization + manage Organization/Membership scoped to the org', () => { - const can = jest.fn(); - const cannot = jest.fn(); const membership = membershipFixture(MEMBERSHIP_ROLES.OWNER, 'org-owner-1'); - organizationAbilities({ roles: ['user'] }, membership, { can, cannot }); + const { can, cannot } = callAbilities({ roles: ['user'] }, membership); expect(can).toHaveBeenCalledWith('create', 'Organization'); expect(can).toHaveBeenCalledWith('manage', 'Organization', { _id: 'org-owner-1' }); @@ -84,10 +88,8 @@ describe('organizationAbilities:', () => { describe('non-admin user — membership.role = admin:', () => { test('grants create Organization + read/update Organization (not delete) + read/create/delete Membership', () => { - const can = jest.fn(); - const cannot = jest.fn(); const membership = membershipFixture(MEMBERSHIP_ROLES.ADMIN, 'org-admin-1'); - organizationAbilities({ roles: ['user'] }, membership, { can, cannot }); + const { can, cannot } = callAbilities({ roles: ['user'] }, membership); expect(can).toHaveBeenCalledWith('create', 'Organization'); expect(can).toHaveBeenCalledWith('read', 'Organization', { _id: 'org-admin-1' }); @@ -104,10 +106,8 @@ describe('organizationAbilities:', () => { describe('non-admin user — membership.role = member:', () => { test('grants create Organization + read-only Organization/Membership, no write abilities', () => { - const can = jest.fn(); - const cannot = jest.fn(); const membership = membershipFixture(MEMBERSHIP_ROLES.MEMBER, 'org-member-1'); - organizationAbilities({ roles: ['user'] }, membership, { can, cannot }); + const { can, cannot } = callAbilities({ roles: ['user'] }, membership); expect(can).toHaveBeenCalledWith('create', 'Organization'); expect(can).toHaveBeenCalledWith('read', 'Organization', { _id: 'org-member-1' }); @@ -123,18 +123,16 @@ describe('organizationAbilities:', () => { describe('membership.organizationId shape — populated vs. raw id:', () => { test('scopes abilities to the raw id when organizationId is NOT populated (no ._id)', () => { - const can = jest.fn(); const membership = { role: MEMBERSHIP_ROLES.OWNER, organizationId: 'org-raw-1' }; - organizationAbilities({ roles: ['user'] }, membership, { can, cannot: jest.fn() }); + const { can } = callAbilities({ roles: ['user'] }, membership); expect(can).toHaveBeenCalledWith('manage', 'Organization', { _id: 'org-raw-1' }); expect(can).toHaveBeenCalledWith('manage', 'Membership', { organizationId: 'org-raw-1' }); }); test('scopes abilities to the populated ._id when organizationId IS populated', () => { - const can = jest.fn(); const membership = membershipFixture(MEMBERSHIP_ROLES.OWNER, 'org-populated-1'); - organizationAbilities({ roles: ['user'] }, membership, { can, cannot: jest.fn() }); + const { can } = callAbilities({ roles: ['user'] }, membership); expect(can).toHaveBeenCalledWith('manage', 'Organization', { _id: 'org-populated-1' }); }); diff --git a/modules/organizations/tests/organizations.emailVerification.unit.tests.js b/modules/organizations/tests/organizations.emailVerification.unit.tests.js index 2ad05ac10..1eeb74665 100644 --- a/modules/organizations/tests/organizations.emailVerification.unit.tests.js +++ b/modules/organizations/tests/organizations.emailVerification.unit.tests.js @@ -2,7 +2,6 @@ * Unit tests for email verification gates on organization operations. */ import mongoose from 'mongoose'; -import { EventEmitter } from 'events'; import { jest, describe, test, expect, beforeEach } from '@jest/globals'; // --- Mocks --- @@ -10,11 +9,15 @@ import { jest, describe, test, expect, beforeEach } from '@jest/globals'; // rule 2 (Node#4020): isolate the org-creation seam GENERICALLY — mock the hook // mechanism itself (lib/events.js, a plain EventEmitter registry any consumer // module can subscribe to), never a specific consumer's own listener module. -// A real EventEmitter instance (not a bare jest.fn() double) so the "generic -// extension point" tests below can register/remove listeners on it exactly like -// a consumer would, without ever importing or naming a real consumer module. -const organizationEventsFixture = new EventEmitter(); -jest.unstable_mockModule('../lib/events.js', () => ({ default: organizationEventsFixture })); +// A bare double (not a real EventEmitter) — same convention already used by +// organizations.service.silent.catch.unit.tests.js and +// organizations.service.signup.unit.tests.js, which already covers the +// organization.created/organization.provisioned payload shape and ordering +// generically via this exact mock, so this file only needs isolation, not a +// second copy of that coverage. +jest.unstable_mockModule('../lib/events.js', () => ({ + default: { emit: jest.fn(), on: jest.fn() }, +})); const mockIsConfigured = jest.fn(); jest.unstable_mockModule('../../../lib/helpers/mailer/index.js', () => ({ @@ -290,42 +293,4 @@ describe('Email verification gates:', () => { expect(res.status).toHaveBeenCalledWith(200); }); }); - - // --- organization-creation seam (generic extension point) --- - - describe('organization-creation seam (generic extension point):', () => { - test('a generically-registered listener on organizationEvents receives organization.created + organization.provisioned with their documented payload shape', async () => { - // Stand-in for "whatever a consumer registers on the seam" (e.g. billing's - // signupGrant listener, per lib/events.js's doc comment) — a plain function - // attached via the SAME .on() contract any consumer uses. Never references - // a real consumer module, so this proves the seam generically (rule 2). - const createdListener = jest.fn(); - const provisionedListener = jest.fn(); - organizationEventsFixture.on('organization.created', createdListener); - organizationEventsFixture.on('organization.provisioned', provisionedListener); - - try { - mockIsConfigured.mockReturnValue(false); - - const fakeOrg = { _id: new mongoose.Types.ObjectId(), name: 'Test', toJSON: () => ({ name: 'Test' }) }; - const fakeMembership = { _id: new mongoose.Types.ObjectId(), role: 'owner' }; - mockOrganizationsRepositoryCreate.mockResolvedValue(fakeOrg); - mockMembershipRepositoryCreate.mockResolvedValue(fakeMembership); - mockUpdateById.mockResolvedValue({}); - - const user = { id: fakeUserId.toString(), email: 'test@acme.com', firstName: 'Test', lastName: 'User', emailVerified: false }; - await OrganizationsService.handleSignupOrganization(user); - - expect(createdListener).toHaveBeenCalledWith({ orgId: fakeOrg._id.toString(), planId: 'free' }); - expect(provisionedListener).toHaveBeenCalledWith({ - userId: fakeUserId.toString(), - organizationId: fakeOrg._id.toString(), - }); - } finally { - // Never leak a listener into a sibling test — each test owns its own. - organizationEventsFixture.off('organization.created', createdListener); - organizationEventsFixture.off('organization.provisioned', provisionedListener); - } - }); - }); }); From 736e325575db718ac6e66e3fec47635031ca31dd Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Wed, 5 Aug 2026 15:02:30 +0200 Subject: [PATCH 3/4] fix(billing): derive subscription test fixtures from loaded plan catalogue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-push panel P1: rule 1 was applied to the two SubscriptionUpdate tests only, but the 'Subscription schema' and 'Subscription schema — meter fields' blocks' shared beforeEach fixtures still hardcoded plan: 'free'. Since the schema builds its plan enum from config.billing.plans, every one of the ~15 tests riding that fixture would fail for a consumer whose catalogue lacks 'free'. Derive a fixturePlan from the loaded config instead (defaultPlan when it's actually a member of the catalogue, else plans[0]) and use it in both fixtures. 'should default plan to free' is untouched — it explicitly deletes subscription.plan before parsing, so it tests the schema's own Zod default (currently hardcoded 'free' in production code, unrelated to this fixture) rather than the fixture value. Extends the synthetic-consumer-catalogue block with a test that applies the same fixturePlan derivation to a catalogue containing no devkit-shipped plan id at all, proving the mechanism the fixtures now rely on survives it. --- modules/billing/tests/billing.unit.tests.js | 32 +++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/modules/billing/tests/billing.unit.tests.js b/modules/billing/tests/billing.unit.tests.js index da640080d..ae0041ef5 100644 --- a/modules/billing/tests/billing.unit.tests.js +++ b/modules/billing/tests/billing.unit.tests.js @@ -14,13 +14,22 @@ describe('Billing unit tests:', () => { // catalogue must still pass these SubscriptionUpdate assertions. const nonDefaultPlan = config.billing.plans.find((plan) => plan !== config.billing.defaultPlan) ?? config.billing.plans[0]; + // rule 1 (Node#4020): fixture baseline plan for tests that need "some valid + // plan" (not testing default-resolution itself) — derived from the loaded + // config so a consumer whose catalogue lacks the devkit-shipped 'free' id + // still gets a value schema.Subscription actually accepts. Prefers + // defaultPlan (readable, guaranteed-valid by convention) but falls back to + // plans[0] in case a consumer's defaultPlan is ever missing from its own + // plans array. + const fixturePlan = config.billing.plans.includes(config.billing.defaultPlan) ? config.billing.defaultPlan : config.billing.plans[0]; + describe('Subscription schema', () => { let subscription; beforeEach(() => { subscription = { organization: '507f1f77bcf86cd799439011', - plan: 'free', + plan: fixturePlan, status: 'active', }; }); @@ -194,7 +203,7 @@ describe('Billing unit tests:', () => { beforeEach(() => { subscription = { organization: '507f1f77bcf86cd799439011', - plan: 'free', + plan: fixturePlan, status: 'active', }; }); @@ -386,6 +395,25 @@ describe('Billing unit tests:', () => { expect(result.error).toBeDefined(); }); + // Pre-push panel finding (P1): the shared `fixturePlan` derivation above + // (`config.billing.plans.includes(config.billing.defaultPlan) ? ... : + // plans[0]`) is what the 'Subscription schema' + 'meter fields' blocks' + // fixtures now use in place of a hardcoded 'free'. Proves that SAME + // derivation, applied to THIS synthetic catalogue (no 'free', no + // devkit-shipped id at all), still resolves to a value the schema accepts + // — the mechanism those ~15 fixture-driven tests now rely on genuinely + // survives a catalogue that lacks every devkit-default plan id. + test('the fixturePlan derivation mechanism resolves to a value the synthetic schema accepts', () => { + const syntheticFixturePlan = syntheticPlans.includes(syntheticDefaultPlan) ? syntheticDefaultPlan : syntheticPlans[0]; + const result = SyntheticSchema.Subscription.safeParse({ + organization: '507f1f77bcf86cd799439011', + plan: syntheticFixturePlan, + status: 'active', + }); + expect(result.error).toBeFalsy(); + expect(result.data.plan).toBe(syntheticFixturePlan); + }); + test('SubscriptionUpdate round-trips a synthetic-catalogue plan id', () => { const result = SyntheticSchema.SubscriptionUpdate.safeParse({ plan: syntheticPlans[1] }); expect(result.error).toBeFalsy(); From 5b9004a68549d504c8d56a583319fd32b42c89ff Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Wed, 5 Aug 2026 15:21:42 +0200 Subject: [PATCH 4/4] refactor(billing): address fallback-reviewer nits on the plan-catalogue fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit was rate-limited on the P1 fix push, so the fallback Claude reviewer covered it (verdict: OK with nits). Two nits addressed: - Document the degenerate single-distinct-plan-catalogue case for nonDefaultPlan's `?? plans[0]` fallback — there's no code fix possible (no other plan exists to pick in that case), so this clarifies the round-trip guarantee still holds either way rather than adding branching that can't actually produce a better value. - Drop the synthetic-catalogue block's afterAll(jest.resetModules()) — dead cleanup, it's the last describe block in the file. --- modules/billing/tests/billing.unit.tests.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/modules/billing/tests/billing.unit.tests.js b/modules/billing/tests/billing.unit.tests.js index ae0041ef5..d7aa16bfd 100644 --- a/modules/billing/tests/billing.unit.tests.js +++ b/modules/billing/tests/billing.unit.tests.js @@ -11,7 +11,12 @@ import schema from '../models/billing.subscription.schema.js'; describe('Billing unit tests:', () => { // rule 1 (Node#4020): pick a plan from the LOADED config instead of hardcoding // a devkit-default plan id ('pro'/'starter') — a consumer with a different plan - // catalogue must still pass these SubscriptionUpdate assertions. + // catalogue must still pass these SubscriptionUpdate assertions. The `?? plans[0]` + // fallback only fires when EVERY plan in the catalogue equals defaultPlan (a + // single-distinct-plan catalogue) — there is no other plan to pick in that case, + // by construction. The round-trip these tests assert (partial update preserves + // the exact plan provided, no default silently injected) still holds either + // way; only the variable's "non-default" framing stops applying. const nonDefaultPlan = config.billing.plans.find((plan) => plan !== config.billing.defaultPlan) ?? config.billing.plans[0]; // rule 1 (Node#4020): fixture baseline plan for tests that need "some valid @@ -371,10 +376,6 @@ describe('Billing unit tests:', () => { SyntheticSchema = (await import('../models/billing.subscription.schema.js')).default; }); - afterAll(() => { - jest.resetModules(); - }); - test('accepts every plan from the synthetic catalogue (none of which exist in the devkit default)', () => { for (const plan of syntheticPlans) { const result = SyntheticSchema.Subscription.safeParse({