fix(crm): PR #603 review follow-ups + Outlook-proof email design
Addresses the maintainer's non-blocking review items + the Outlook email bug:
- invoice create: verify the chosen event belongs to the customer (only when
the event has assignments; legacy unassigned events pass through).
- mark-paid + import: bound paidAt to [2000-01-01, now+30d] so a typo'd year
can't silently drop a payment out of every cash-basis revenue window.
- customer routes: country_code now {min:2,max:2}+isAlpha+uppercase-normalize
(was isString/max:2 — allowed '', '1', '!@'), matching the business-profile
route.
- email transporter: close the previous instance before re-init (leak guard
for a future pooled transport).
- scheduled-email tz: warn loudly when business_hours is set but the profile
timezone is blank (was silently using the server/UTC tz).
- wrapEmailHtml: rebuild the chrome as inline-styled tables + bgcolor and
inline the themed CTA button, so the design survives Outlook/Apple Mail
stripping the head <style> (kept the <style> as progressive enhancement).
This commit is contained in:
Luca
@@ -165,7 +165,7 @@ router.post('/invite', [
|
||||
body('prefill.postal_code').optional({ nullable: true }).isString().isLength({ max: 20 }),
|
||||
body('prefill.city').optional({ nullable: true }).isString().isLength({ max: 120 }),
|
||||
body('prefill.state').optional({ nullable: true }).isString().isLength({ max: 120 }),
|
||||
body('prefill.country_code').optional({ nullable: true }).isString().isLength({ max: 2 }),
|
||||
body('prefill.country_code').optional({ values: 'falsy' }).isLength({ min: 2, max: 2 }).isAlpha().withMessage('country_code must be a 2-letter ISO code').customSanitizer((v) => (v || '').toUpperCase()),
|
||||
// Per-customer preferred language. Drives portal UI + quote/invoice
|
||||
// PDF locale. Defaults at insert time to the business profile's
|
||||
// default_locale when the admin doesn't supply one (see
|
||||
@@ -246,7 +246,7 @@ router.post('/', [
|
||||
body('prefill.postal_code').optional({ nullable: true }).isString().isLength({ max: 20 }),
|
||||
body('prefill.city').optional({ nullable: true }).isString().isLength({ max: 120 }),
|
||||
body('prefill.state').optional({ nullable: true }).isString().isLength({ max: 120 }),
|
||||
body('prefill.country_code').optional({ nullable: true }).isString().isLength({ max: 2 }),
|
||||
body('prefill.country_code').optional({ values: 'falsy' }).isLength({ min: 2, max: 2 }).isAlpha().withMessage('country_code must be a 2-letter ISO code').customSanitizer((v) => (v || '').toUpperCase()),
|
||||
body('prefill.country_name').optional({ nullable: true }).isString().isLength({ max: 120 }),
|
||||
body('prefill.preferred_language').optional({ nullable: true }).isString().isLength({ min: 2, max: 8 }),
|
||||
// At least one human-readable identifier so the record isn't a
|
||||
@@ -379,7 +379,7 @@ router.put('/:id', [
|
||||
body('postal_code').optional({ nullable: true }).isString().isLength({ max: 20 }),
|
||||
body('city').optional({ nullable: true }).isString().isLength({ max: 120 }),
|
||||
body('state').optional({ nullable: true }).isString().isLength({ max: 120 }),
|
||||
body('country_code').optional({ nullable: true }).isString().isLength({ max: 2 }),
|
||||
body('country_code').optional({ values: 'falsy' }).isLength({ min: 2, max: 2 }).isAlpha().withMessage('country_code must be a 2-letter ISO code').customSanitizer((v) => (v || '').toUpperCase()),
|
||||
body('country_name').optional({ nullable: true }).isString().isLength({ max: 120 }),
|
||||
body('preferred_language').optional({ nullable: true }).isString().isLength({ max: 8 }),
|
||||
body('notes').optional({ nullable: true }).isString(),
|
||||
|
||||
@@ -32,6 +32,23 @@ const { db } = require('../database/db');
|
||||
|
||||
const router = express.Router();
|
||||
|
||||
// PR #603 review follow-up #2 — bound payment dates. `isISO8601()` alone
|
||||
// accepts year 1900/9999; cash-basis revenue keys on paid_at, so a typo
|
||||
// (2026→2226) would silently push a payment out of every dashboard window
|
||||
// forever. Reject anything before 2000-01-01 or more than 30 days in the
|
||||
// future (small future window covers value-date lag without allowing fat-
|
||||
// finger years). Use as `.custom(isReasonablePaidAt)` after `.isISO8601()`.
|
||||
function isReasonablePaidAt(value) {
|
||||
const d = new Date(value);
|
||||
if (Number.isNaN(d.getTime())) throw new Error('Invalid payment date');
|
||||
const min = new Date('2000-01-01T00:00:00Z');
|
||||
const max = new Date(Date.now() + 30 * 24 * 60 * 60 * 1000);
|
||||
if (d < min || d > max) {
|
||||
throw new Error('Payment date must be between 2000-01-01 and 30 days from now');
|
||||
}
|
||||
return true;
|
||||
}
|
||||
|
||||
// Multer config for "import historical invoice" PDF uploads. Stored
|
||||
// under storage/business-docs/invoice-imports/<year>/<filename> so
|
||||
// imported files don't collide with the renderer's own output under
|
||||
@@ -438,7 +455,7 @@ router.post(
|
||||
body('currency').optional({ values: 'falsy' }).isString().isLength({ min: 3, max: 3 }),
|
||||
body('status').optional({ values: 'falsy' }).isIn(['sent', 'paid', 'overdue']),
|
||||
body('paidAmountMinor').optional({ values: 'falsy' }).isInt({ min: 0 }),
|
||||
body('paidAt').optional({ values: 'falsy' }).isISO8601(),
|
||||
body('paidAt').optional({ values: 'falsy' }).isISO8601().custom(isReasonablePaidAt),
|
||||
body('language').optional({ values: 'falsy' }).isString().isLength({ max: 8 }),
|
||||
],
|
||||
handleAsync(async (req, res) => {
|
||||
@@ -745,7 +762,7 @@ router.post(
|
||||
[
|
||||
param('id').isInt({ min: 1 }),
|
||||
body('amountMinor').isInt({ min: 1 }),
|
||||
body('paidAt').optional({ values: 'falsy' }).isISO8601(),
|
||||
body('paidAt').optional({ values: 'falsy' }).isISO8601().custom(isReasonablePaidAt),
|
||||
body('paymentMethod').optional({ values: 'falsy' }).isString().isLength({ max: 64 }),
|
||||
body('reference').optional({ values: 'falsy' }).isString().isLength({ max: 128 }),
|
||||
body('notes').optional({ values: 'falsy' }).isString().isLength({ max: 5000 }),
|
||||
|
||||
@@ -38,6 +38,13 @@ async function initializeTransporter(forceReinit = false) {
|
||||
// Configuration has changed or first initialization
|
||||
logger.info('Initializing email transporter' + (lastConfigHash && currentConfigHash !== lastConfigHash ? ' (configuration changed)' : ''));
|
||||
|
||||
// PR #603 review follow-up #3 — release the previous transporter before
|
||||
// swapping it. Harmless today (no connection pool), but prevents a
|
||||
// socket/connection leak if `pool: true` is ever enabled on the transport.
|
||||
if (transporter && typeof transporter.close === 'function') {
|
||||
try { transporter.close(); } catch (_) { /* best-effort */ }
|
||||
}
|
||||
|
||||
transporter = nodemailer.createTransport({
|
||||
host: config.smtp_host,
|
||||
port: config.smtp_port,
|
||||
@@ -252,6 +259,19 @@ async function wrapEmailHtml(htmlBody, subject, language = 'en') {
|
||||
const logoFullUrl = `${frontendUrl}${logoPath.startsWith('/') ? '' : '/'}${logoPath}`;
|
||||
logger.debug('Email logo URL:', { frontendUrl, logoPath, logoFullUrl });
|
||||
|
||||
const year = new Date().getFullYear();
|
||||
// PR review follow-up — Outlook (Word engine) and Apple Mail under some
|
||||
// configs STRIP the <head><style>, so any element styled only by a class
|
||||
// loses its design (the CTA rendered as a plain link, the header card +
|
||||
// button vanished). Fix: inline the CTA button style (themed with the
|
||||
// admin's primary colour) on every `class="button"` anchor, keeping the
|
||||
// class so style-capable clients still get :hover. The wrapper chrome
|
||||
// below is rebuilt as inline-styled tables with bgcolor attrs for the same
|
||||
// reason. The <style> block stays as progressive enhancement.
|
||||
const buttonInlineStyle = `background-color:${primaryColor};color:${buttonTextColor};display:inline-block;padding:12px 30px;text-decoration:none;border-radius:5px;font-weight:500;`;
|
||||
const inlinedBody = (typeof htmlBody === 'string' ? htmlBody : '')
|
||||
.replace(/class="button"/g, `class="button" style="${buttonInlineStyle}"`);
|
||||
|
||||
return `
|
||||
<!DOCTYPE html>
|
||||
<html lang="${language}">
|
||||
@@ -367,22 +387,32 @@ async function wrapEmailHtml(htmlBody, subject, language = 'en') {
|
||||
}
|
||||
</style>
|
||||
</head>
|
||||
<body>
|
||||
<div class="email-wrapper">
|
||||
<div class="email-container">
|
||||
<div class="email-header">
|
||||
<img src="${logoFullUrl}" alt="${companyName}" class="logo">
|
||||
</div>
|
||||
<div class="email-content">
|
||||
${htmlBody}
|
||||
</div>
|
||||
<div class="email-footer">
|
||||
<img src="${logoFullUrl}" alt="${companyName}">
|
||||
<p>${companyName}</p>
|
||||
<p style="font-size: 12px; color: #999;">© ${new Date().getFullYear()} ${companyName}. All rights reserved.</p>
|
||||
</div>
|
||||
</div>
|
||||
</div>
|
||||
<body style="margin:0;padding:0;background-color:${bodyBgColor};color:${bodyTextColor};font-family:-apple-system,BlinkMacSystemFont,'Segoe UI',Roboto,'Helvetica Neue',Arial,sans-serif;">
|
||||
<table role="presentation" width="100%" cellpadding="0" cellspacing="0" border="0" bgcolor="${bodyBgColor}" style="background-color:${bodyBgColor};" class="email-wrapper">
|
||||
<tr>
|
||||
<td align="center" style="padding:40px 20px;">
|
||||
<table role="presentation" width="600" cellpadding="0" cellspacing="0" border="0" class="email-container" style="width:100%;max-width:600px;background-color:${containerBgColor};border-radius:8px;overflow:hidden;">
|
||||
<tr>
|
||||
<td align="center" bgcolor="${primaryColor}" class="email-header" style="background-color:${primaryColor};padding:30px;text-align:center;">
|
||||
<img src="${logoFullUrl}" alt="${companyName}" width="180" class="logo" style="max-width:180px;height:auto;display:inline-block;border:0;">
|
||||
</td>
|
||||
</tr>
|
||||
<tr>
|
||||
<td class="email-content" style="padding:40px 30px;">
|
||||
${inlinedBody}
|
||||
</td>
|
||||
</tr>
|
||||
<tr>
|
||||
<td align="center" bgcolor="${secondaryColor}" class="email-footer" style="background-color:${secondaryColor};padding:30px;text-align:center;border-top:1px solid #eeeeee;">
|
||||
<img src="${logoFullUrl}" alt="${companyName}" width="120" style="max-width:120px;height:auto;opacity:0.8;margin-bottom:15px;border:0;">
|
||||
<p style="color:${mutedTextColor};font-size:14px;margin:5px 0;">${companyName}</p>
|
||||
<p style="font-size:12px;color:#999999;margin:5px 0;">© ${year} ${companyName}. All rights reserved.</p>
|
||||
</td>
|
||||
</tr>
|
||||
</table>
|
||||
</td>
|
||||
</tr>
|
||||
</table>
|
||||
</body>
|
||||
</html>`;
|
||||
}
|
||||
@@ -886,7 +916,18 @@ async function getScheduledEmailConfig() {
|
||||
const schedule = normaliseSchedule(profile.business_hours);
|
||||
|
||||
let timezone = (profile.timezone || '').trim();
|
||||
if (!timezone) timezone = Intl.DateTimeFormat().resolvedOptions().timeZone || 'UTC';
|
||||
if (!timezone) {
|
||||
// PR #603 review follow-up #4 — business hours are configured but the
|
||||
// profile timezone is blank, so we fall back to the SERVER's tz (usually
|
||||
// UTC on a Docker host). That silently shifts every business-hours
|
||||
// calculation. Warn loudly so the admin sets business_profile.timezone.
|
||||
timezone = Intl.DateTimeFormat().resolvedOptions().timeZone || 'UTC';
|
||||
logger.warn(
|
||||
'Scheduled-email business hours are set but business_profile.timezone is blank — '
|
||||
+ `falling back to the server timezone (${timezone}). Set the profile timezone `
|
||||
+ 'so business-hours snapping uses your local time, not the server\'s.',
|
||||
);
|
||||
}
|
||||
// Reject a bogus tz before it reaches Intl in the snap helper.
|
||||
try {
|
||||
new Intl.DateTimeFormat('en-US', { timeZone: timezone });
|
||||
|
||||
@@ -694,6 +694,22 @@ async function createInvoice(payload, adminId, trx = db) {
|
||||
const customer = await trx('customer_accounts').where({ id: payload.customerAccountId }).first();
|
||||
ensureCustomerCanBill(customer);
|
||||
|
||||
// PR #603 review follow-up #1 — when an invoice is attached to an event,
|
||||
// make sure that event actually belongs to the chosen customer. Without
|
||||
// this, a typo'd/copy-pasted eventId silently links the invoice to an
|
||||
// unrelated event, producing misleading reporting links. Only enforced
|
||||
// when the event HAS customer assignments (an event with none — e.g. a
|
||||
// legacy import — is allowed through, since we can't prove a mismatch).
|
||||
if (payload.eventId && await trx.schema.hasTable('event_customer_assignments')) {
|
||||
const assignments = await trx('event_customer_assignments')
|
||||
.where({ event_id: payload.eventId })
|
||||
.select('customer_account_id');
|
||||
if (assignments.length > 0 &&
|
||||
!assignments.some(a => a.customer_account_id === payload.customerAccountId)) {
|
||||
throw new AppError('The selected event is not assigned to this customer', 422, 'EVENT_CUSTOMER_MISMATCH');
|
||||
}
|
||||
}
|
||||
|
||||
// Accumulator intercept (migration 128). For customers in
|
||||
// billing_cadence='monthly' OR 'manual' mode every createInvoice call
|
||||
// APPENDS line items onto a single running draft instead of minting a
|
||||
|
||||
Reference in New Issue
Block a user