From a2b2d3fb313f347bac505b5d1846dce2b679ce55 Mon Sep 17 00:00:00 2001
From: Luca <102960244+Luca-Timo@users.noreply.github.com>
Date: Sat, 6 Jun 2026 00:42:08 +0200
Subject: [PATCH] fix(crm): PR #603 review follow-ups + Outlook-proof email
 design
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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).
---
 backend/src/routes/adminCustomers.js   |  6 +--
 backend/src/routes/adminInvoices.js    | 21 +++++++-
 backend/src/services/emailProcessor.js | 75 ++++++++++++++++++++------
 backend/src/services/invoiceService.js | 16 ++++++
 4 files changed, 96 insertions(+), 22 deletions(-)

diff --git a/backend/src/routes/adminCustomers.js b/backend/src/routes/adminCustomers.js
index 0d3c81e7..cc03115c 100644
--- a/backend/src/routes/adminCustomers.js
+++ b/backend/src/routes/adminCustomers.js
@@ -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(),
diff --git a/backend/src/routes/adminInvoices.js b/backend/src/routes/adminInvoices.js
index b114bf44..23d8f2bf 100644
--- a/backend/src/routes/adminInvoices.js
+++ b/backend/src/routes/adminInvoices.js
@@ -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 }),
diff --git a/backend/src/services/emailProcessor.js b/backend/src/services/emailProcessor.js
index 0e9b2412..58462bfa 100644
--- a/backend/src/services/emailProcessor.js
+++ b/backend/src/services/emailProcessor.js
@@ -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 });
diff --git a/backend/src/services/invoiceService.js b/backend/src/services/invoiceService.js
index 91a92e68..4f708097 100644
--- a/backend/src/services/invoiceService.js
+++ b/backend/src/services/invoiceService.js
@@ -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