From bd7512ac26529f8e1b138820cdae0acee3fe1d57 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sat, 3 Oct 2026 23:15:08 +0200 Subject: [PATCH 1/2] fix(billing): fall back to getBaseUrl() when config.app.url is unset The 3 billing email builders (quota, credit, payment-failed) rendered an empty link whenever config.app.url was unset, which no Devkit default config sets. Mirror getBrand()'s existing fallback so app.url still wins when a project sets it. Also fixes billing.email.js's JSDoc, which described a template override mechanism that was never implemented. Refs #4128 --- ERRORS.md | 1 + modules/billing/billing.email.js | 12 ++-- .../billing.init.email-alerts.unit.tests.js | 69 ++++++++++++++++++- 3 files changed, 75 insertions(+), 7 deletions(-) diff --git a/ERRORS.md b/ERRORS.md index c52d78c22..3fbb25c7e 100644 --- a/ERRORS.md +++ b/ERRORS.md @@ -52,3 +52,4 @@ Use this file as a compact memory of recurring AI mistakes. - [2026-09-28] billing/stripe: `checkout.session.completed`'s subscription-retrieve `catch` returned silently on failure -> the event was recorded as processed by the idempotency wrapper while the subscription row stayed unlinked on the free plan, with no retry; fix = throw instead of return, so the claim persists (attempts increments, the doc is never deleted on failure) and Stripe redelivers; see pierreb-devkit/Node#4151 - [2026-09-28] billing/meter: `incrementMeter` always billed against the subscription's own plan quota, while the admission gate (`billing.quota.service.js`) already treated a fail-closed status (`paused`/`unpaid`/`incomplete`/`incomplete_expired`/`canceled`) as the free plan -> a fail-closed org kept consuming its stale paid-plan quota on the meter even though the gate let it through on free-plan grounds; the fail-closed status list is now one shared constant imported by both the gate and the meter — a literal duplicated in two enforcement paths drifts by design, not by mistake; see pierreb-devkit/Node#4151 - [2026-09-28] auth: `verifyEmail` read the user by token (`getBrut`) then wrote it in a separate step (`update`) -> two concurrent requests for the same token both passed the read check before either write landed, so both provisioned an organization/grant for the same signup; fix = one atomic `findOneAndUpdate` (`UserService.consumeEmailVerificationToken`) that verifies and clears the token together, so only the first concurrent caller can match the still-unexpired token; see pierreb-devkit/Node#4151 +- [2026-10-03] billing/mailer: the 3 billing email builders (quota, credit, payment-failed) built their link from `config.app?.url ? ... : ''`, but no Devkit default config sets `app.url` -> every billing email rendered an empty href unless a downstream happened to set it itself (undetected because no default-config test asserted a non-empty link). Fix = `config.app?.url || getBaseUrl()`, the same fallback `getBrand()`'s `url` field already uses; `app.url` still wins when a project sets it. Also corrected `billing.email.js`'s JSDoc, which described a template "glob-merge" override mechanism that was never implemented — the real resolver is `config.mailer.templates`; see pierreb-devkit/Node#4128 diff --git a/modules/billing/billing.email.js b/modules/billing/billing.email.js index 0de37c825..b939ac245 100644 --- a/modules/billing/billing.email.js +++ b/modules/billing/billing.email.js @@ -5,6 +5,7 @@ import config from '../../config/index.js'; import logger from '../../lib/services/logger.js'; import billingEvents from './lib/events.js'; import mailer from '../../lib/helpers/mailer/index.js'; +import getBaseUrl from '../../lib/helpers/getBaseUrl.js'; import MembershipRepository from '../organizations/repositories/organizations.membership.repository.js'; import { MEMBERSHIP_ROLES, MEMBERSHIP_STATUSES } from '../organizations/lib/constants.js'; @@ -57,8 +58,9 @@ export const sendBillingEmail = (mailOpts, context) => { * - payment.failed — sends payment-failed email prompting card update * * Template resolution: devkit ships generic templates in config/templates/billing-*.html. - * Downstream projects override by placing same-named files in their own config/templates/ - * directory — those shadow devkit defaults via the template-resolution glob-merge in config. + * A downstream overrides one by adding a `config.mailer.templates` entry that maps the + * key to its own file; any other key keeps resolving to the devkit default + * (see `resolveTemplatePath` in lib/helpers/mailer/index.js). */ export const setupBillingEmails = () => { // ── meter.threshold_crossed — 80% / 100% quota emails ────────────────────── @@ -67,7 +69,7 @@ export const setupBillingEmails = () => { if (threshold !== 80 && threshold !== 100) return; const appName = config.app?.title ?? ''; - const billingUrl = config.app?.url ? `${config.app.url}/billing` : ''; + const billingUrl = `${config.app?.url || getBaseUrl()}/billing`; resolveOrgAdminEmails(organizationId).then((emails) => { if (!emails.length) return; @@ -105,7 +107,7 @@ export const setupBillingEmails = () => { if (threshold !== 80 && threshold !== 100) return; const appName = config.app?.title ?? ''; - const billingUrl = config.app?.url ? `${config.app.url}/billing` : ''; + const billingUrl = `${config.app?.url || getBaseUrl()}/billing`; const isWarning = threshold === 80; resolveOrgAdminEmails(organizationId).then((emails) => { @@ -135,7 +137,7 @@ export const setupBillingEmails = () => { billingEvents.on('payment.failed', ({ organizationId }) => { const appName = config.app?.title ?? ''; - const billingPortalUrl = config.app?.url ? `${config.app.url}/billing` : ''; + const billingPortalUrl = `${config.app?.url || getBaseUrl()}/billing`; resolveOrgAdminEmails(organizationId).then((emails) => { if (!emails.length) return; diff --git a/modules/billing/tests/billing.init.email-alerts.unit.tests.js b/modules/billing/tests/billing.init.email-alerts.unit.tests.js index b62f5f7b7..67c19caa8 100644 --- a/modules/billing/tests/billing.init.email-alerts.unit.tests.js +++ b/modules/billing/tests/billing.init.email-alerts.unit.tests.js @@ -14,6 +14,7 @@ describe('billing.email setupBillingEmails listeners:', () => { let mockBillingEvents; let mockConfig; let mockLogger; + let mockGetBaseUrl; const orgId = '507f1f77bcf86cd799439011'; @@ -52,6 +53,8 @@ describe('billing.email setupBillingEmails listeners:', () => { error: jest.fn(), }; + mockGetBaseUrl = jest.fn().mockReturnValue('https://frontend.example.com'); + // Capture registered listeners so we can invoke them directly mockBillingEvents = { on: jest.fn((event, handler) => { @@ -71,6 +74,10 @@ describe('billing.email setupBillingEmails listeners:', () => { default: mockLogger, })); + jest.unstable_mockModule('../../../lib/helpers/getBaseUrl.js', () => ({ + default: mockGetBaseUrl, + })); + jest.unstable_mockModule('../lib/events.js', () => ({ default: mockBillingEvents, })); @@ -297,6 +304,45 @@ describe('billing.email setupBillingEmails listeners:', () => { expect.objectContaining({ to: 'valid@test.com' }), ); }); + + test('includes billingUrl built from config.app.url', async () => { + listeners['meter.threshold_crossed']({ + organizationId: orgId, + threshold: 80, + meterUsed: 800, + meterQuota: 1000, + weekKey: '2026-W18', + }); + + await new Promise((r) => setImmediate(r)); + + expect(mockMailer.sendMail).toHaveBeenCalledWith( + expect.objectContaining({ + params: expect.objectContaining({ billingUrl: 'https://myapp.example.com/billing' }), + }), + ); + expect(mockGetBaseUrl).not.toHaveBeenCalled(); + }); + + test('falls back to getBaseUrl() for billingUrl when config.app.url is unset', async () => { + mockConfig.app = { title: 'MyApp', contact: 'support@myapp.example.com' }; + + listeners['meter.threshold_crossed']({ + organizationId: orgId, + threshold: 80, + meterUsed: 800, + meterQuota: 1000, + weekKey: '2026-W18', + }); + + await new Promise((r) => setImmediate(r)); + + expect(mockMailer.sendMail).toHaveBeenCalledWith( + expect.objectContaining({ + params: expect.objectContaining({ billingUrl: 'https://frontend.example.com/billing' }), + }), + ); + }); }); // ── billing.extras.balance_threshold_crossed ───────────────────────────── @@ -422,6 +468,25 @@ describe('billing.email setupBillingEmails listeners:', () => { ); }); + test('falls back to getBaseUrl() for billingUrl when config.app.url is unset', async () => { + mockConfig.app = { title: 'MyApp', contact: 'support@myapp.example.com' }; + + listeners['billing.extras.balance_threshold_crossed']({ + organizationId: orgId, + threshold: 80, + remaining: 100, + planId: 'free', + }); + + await new Promise((r) => setImmediate(r)); + + expect(mockMailer.sendMail).toHaveBeenCalledWith( + expect.objectContaining({ + params: expect.objectContaining({ billingUrl: 'https://frontend.example.com/billing' }), + }), + ); + }); + test('logs error but does not throw when sendMail rejects', async () => { mockMailer.sendMail.mockRejectedValue(new Error('SMTP error')); @@ -538,7 +603,7 @@ describe('billing.email setupBillingEmails listeners:', () => { ); }); - test('sets billingPortalUrl to empty string when config.app.url is missing', async () => { + test('falls back to getBaseUrl() for billingPortalUrl when config.app.url is unset', async () => { mockConfig.app = {}; listeners['payment.failed']({ organizationId: orgId }); @@ -547,7 +612,7 @@ describe('billing.email setupBillingEmails listeners:', () => { expect(mockMailer.sendMail).toHaveBeenCalledWith( expect.objectContaining({ - params: expect.objectContaining({ billingPortalUrl: '' }), + params: expect.objectContaining({ billingPortalUrl: 'https://frontend.example.com/billing' }), }), ); }); From a3d358fe832e5de27848ad19ff16b85cd4ed5e8b Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sat, 3 Oct 2026 23:27:36 +0200 Subject: [PATCH 2/2] refactor(billing): extract getBillingUrl() helper The 3 billing email builders repeated the same `config.app?.url || getBaseUrl()`/billing expression. Fold it into one local helper so the fallback rule lives in a single place. Refs #4128 --- modules/billing/billing.email.js | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/modules/billing/billing.email.js b/modules/billing/billing.email.js index b939ac245..8db55d227 100644 --- a/modules/billing/billing.email.js +++ b/modules/billing/billing.email.js @@ -32,6 +32,13 @@ export const resolveOrgAdminEmails = async (organizationId) => { } }; +/** + * Resolve the billing page URL. config.app.url wins when a project sets it; + * otherwise falls back to getBaseUrl() (mirrors getBrand()'s url fallback). + * @returns {string} Absolute URL to the billing page. + */ +const getBillingUrl = () => `${config.app?.url || getBaseUrl()}/billing`; + /** * Fire-and-forget email send. Logs mailer errors without re-throwing. * @param {Object} mailOpts - Options passed directly to mailer.sendMail @@ -69,7 +76,7 @@ export const setupBillingEmails = () => { if (threshold !== 80 && threshold !== 100) return; const appName = config.app?.title ?? ''; - const billingUrl = `${config.app?.url || getBaseUrl()}/billing`; + const billingUrl = getBillingUrl(); resolveOrgAdminEmails(organizationId).then((emails) => { if (!emails.length) return; @@ -107,7 +114,7 @@ export const setupBillingEmails = () => { if (threshold !== 80 && threshold !== 100) return; const appName = config.app?.title ?? ''; - const billingUrl = `${config.app?.url || getBaseUrl()}/billing`; + const billingUrl = getBillingUrl(); const isWarning = threshold === 80; resolveOrgAdminEmails(organizationId).then((emails) => { @@ -137,7 +144,7 @@ export const setupBillingEmails = () => { billingEvents.on('payment.failed', ({ organizationId }) => { const appName = config.app?.title ?? ''; - const billingPortalUrl = `${config.app?.url || getBaseUrl()}/billing`; + const billingPortalUrl = getBillingUrl(); resolveOrgAdminEmails(organizationId).then((emails) => { if (!emails.length) return;