From 5893ecb27a0365a79ec04336c5a122b31d31db0e Mon Sep 17 00:00:00 2001 From: Luca <102960244+Luca-Timo@users.noreply.github.com> Date: Tue, 23 Jun 2026 23:35:56 +0200 Subject: [PATCH] fix(workflows): ship built-ins disabled for first beta + enabled-based mutex + admin sentinel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per review: the four cutover built-ins (dunning, gallery_expiring, gallery_expired, pre_event_email) now ship enabled:false. The mutual-exclusion guards revert to ENABLED-based (isBuiltinFlowActive, not existence) so the legacy paths keep running until the admin enables a built-in — enabling cuts over, disabling reverts to legacy (fixes concern #4's "disable = silent dark" foot-gun; no automation goes dark on upgrade). admin_toggled_at sentinel (migration 148) marks admin ownership; the boot re-seeder applies a shipped default-flip (enabled→disabled) only to never-touched built-ins and never overwrites an admin's enable/disable/edit (nit #1). SEED_VERSIONs bumped so the disabled default propagates. Nit: applyReminder unlinks the just-rendered Mahnung PDF if queueEmail throws (no orphan file). --- .../core/148_add_workflow_admin_toggled_at.js | 24 ++++++ backend/src/services/_workflowSeedBoot.js | 73 ++++++++++--------- backend/src/services/eventReminderService.js | 12 +-- backend/src/services/expirationChecker.js | 14 ++-- backend/src/services/invoiceService.js | 48 +++++++----- 5 files changed, 104 insertions(+), 67 deletions(-) create mode 100644 backend/migrations/core/148_add_workflow_admin_toggled_at.js diff --git a/backend/migrations/core/148_add_workflow_admin_toggled_at.js b/backend/migrations/core/148_add_workflow_admin_toggled_at.js new file mode 100644 index 00000000..1806229a --- /dev/null +++ b/backend/migrations/core/148_add_workflow_admin_toggled_at.js @@ -0,0 +1,24 @@ +/** + * Migration 148: mark when an admin has taken ownership of a (built-in) workflow. + * + * The boot seeder re-seeds a built-in on a SEED_VERSION bump and applies the new + * default `enabled` state. Without a sentinel that would re-flip a flow the + * admin had deliberately enabled/disabled. `admin_toggled_at` is stamped on any + * admin enable/disable or edit; the seeder then leaves that flow alone. Nullable + * → existing rows are treated as never-touched (seed defaults apply once). + */ +exports.up = async function (knex) { + if (!(await knex.schema.hasTable('workflows'))) return; + if (!(await knex.schema.hasColumn('workflows', 'admin_toggled_at'))) { + await knex.schema.alterTable('workflows', (t) => { + t.timestamp('admin_toggled_at'); + }); + } +}; + +exports.down = async function (knex) { + if (!(await knex.schema.hasTable('workflows'))) return; + if (await knex.schema.hasColumn('workflows', 'admin_toggled_at')) { + await knex.schema.alterTable('workflows', (t) => t.dropColumn('admin_toggled_at')); + } +}; diff --git a/backend/src/services/_workflowSeedBoot.js b/backend/src/services/_workflowSeedBoot.js index b847e72e..1732168f 100644 --- a/backend/src/services/_workflowSeedBoot.js +++ b/backend/src/services/_workflowSeedBoot.js @@ -206,16 +206,20 @@ function buildGalleryExpiredGraph() { // Built-in registry. `version` is the SEED_VERSION — bump when a graph changes // (or to re-assert the default `enabled` state) so a never-admin-touched copy is -// re-seeded on boot. `enabled` is the cutover default: the live automations -// (dunning, gallery expiry, pre-event) ship ENABLED and their legacy hardcoded -// paths stand down (isBuiltinFlowActive guards), so behaviour is preserved with -// zero double-send. Illustrative/stub flows (booking) ship disabled. -// invoice_dunning v5 = enabled-by-default cutover (was v4: collections handoff). +// re-seeded on boot. `enabled` is the seed default. +// +// FIRST-BETA POSTURE (review feedback): all built-ins ship DISABLED. The legacy +// hardcoded paths keep running by default (the mutual-exclusion guards are +// enabled-based, so they only stand down once the admin ENABLES the matching +// built-in — a deliberate, per-install cutover). Enabling reverts to legacy. +// Once the prefetch-safe approval interstitial has soaked in beta, flip the +// three notification built-ins back to enabled-by-default in a follow-up. +// invoice_dunning v6 = ship disabled (was v5 enabled-by-default). const BUILTINS = [ { key: DUNNING_KEY, - version: 5, - enabled: true, + version: 6, + enabled: false, name: 'Invoice dunning (built-in)', trigger_type: 'invoice.sent', trigger_config: {}, @@ -223,9 +227,9 @@ const BUILTINS = [ 'Drives overdue dunning through the engine: wait to the due date, then up to ' + 'three payment-check cycles. Each cycle fires the existing admin confirm-payment ' + 'email (the gate), which applies reminders + Mahngebühr via the proven payment-check ' - + 'flow; after the cycles exhaust it hands the case to collections. ENABLED by default; ' - + 'while it is enabled the hardcoded reminder ladder is skipped automatically, so the two ' - + 'never double-send. Reminder timing is now edited here (no longer in Settings → CRM).', + + 'flow; after the cycles exhaust it hands the case to collections. DISABLED by default — ' + + 'the hardcoded reminder ladder keeps running until you enable this; enabling cuts over to ' + + 'the engine (the ladder then stands down so the two never double-send), disabling reverts.', build: async () => { const firstDays = Number(await getAppSetting('crm_invoices_reminder_first_days')) || 14; const secondDays = Number(await getAppSetting('crm_invoices_reminder_second_days')) || 30; @@ -235,50 +239,49 @@ const BUILTINS = [ }, { key: 'gallery_expiring', - version: 1, - enabled: true, + version: 2, + enabled: false, name: 'Gallery expiring (built-in)', trigger_type: 'gallery.expiring', trigger_config: {}, description: 'When a gallery is approaching its expiry date, email the customer the expiration warning. ' - + 'ENABLED by default; it delegates to the same email the hourly expiration checker used to ' - + 'send, and that legacy email stands down while this flow is on (no double-send). Edit or ' - + 'extend it here (e.g. add a final-download nudge).', + + 'DISABLED by default; the hourly expiration checker keeps sending the warning until you ' + + 'enable this, which delegates to the identical email and stands the legacy send down. ' + + 'Edit or extend it here (e.g. add a final-download nudge).', build: async () => buildGalleryExpiringGraph(), }, { key: 'gallery_expired', - version: 1, - enabled: true, + version: 2, + enabled: false, name: 'Gallery expired (built-in)', trigger_type: 'gallery.expired', trigger_config: {}, description: 'When a gallery passes its expiry, email the customer (and admin) that it has expired. ' - + 'ENABLED by default; delegates to the same email the expiration checker used to send, ' - + 'and that legacy email stands down while this flow is on. The gallery is still archived ' - + 'automatically regardless of this flow.', + + 'DISABLED by default; the expiration checker keeps sending it until you enable this, which ' + + 'delegates to the identical email and stands the legacy send down. The gallery is still ' + + 'archived automatically regardless of this flow.', build: async () => buildGalleryExpiredGraph(), }, { key: 'pre_event_email', - version: 2, - enabled: true, + version: 3, + enabled: false, name: 'Pre-event reminder (built-in)', trigger_type: 'event.date_approaching', // daysBefore seeds the scheduler emitter from the current global setting so - // upgrades preserve timing; per-event offset overrides still win. This flow - // is now the source of truth for the lead time (was Settings → Reminder emails). + // enabling preserves timing; per-event offset overrides still win. trigger_config: async () => { const d = Number(await getAppSetting('crm_event_reminders_days_before')); return { daysBefore: Number.isFinite(d) && d >= 0 ? d : 2 }; }, description: - 'A few days before the event date, send the customer the pre-event reminder. ENABLED by ' - + 'default; the notify_pre_event action delegates to the proven reminder logic (per-type ' - + 'template, per-event override, send-once), and the legacy reminder pass stands down while ' - + 'this flow is on. Lead time = daysBefore in the trigger config (seeded from your old ' + 'A few days before the event date, send the customer the pre-event reminder. DISABLED by ' + + 'default; the legacy reminder pass keeps running until you enable this, which delegates to ' + + 'the proven reminder logic (per-type template, per-event override, send-once) and stands ' + + 'the legacy pass down. Lead time = daysBefore in the trigger config (seeded from your old ' + 'global setting); per-event overrides on the event page still apply.', build: async () => buildPreEventEmailGraph(), }, @@ -363,14 +366,14 @@ async function seedOneBuiltin(db, logger, def) { const existing = await db('workflows').where({ builtin_key: def.key }).first(); if (existing) { - // Re-seed only a never-admin-activated copy whose SEED_VERSION moved on. An - // already-ENABLED built-in is the admin's live (possibly customised) flow — - // never overwrite it. The version bump carries the cutover default (incl. - // flipping a still-disabled flow to enabled); the cutover targets flows that - // shipped disabled and were never touched, so this leaves admin choices alone. + // Never touch a built-in the admin has taken ownership of (enabled/disabled + // or edited it) — admin_toggled_at is the sentinel (migration 148). For a + // never-touched copy, re-seed on a SEED_VERSION bump and (re-)apply the seed + // default `enabled`, so a shipped default flip (e.g. enabled→disabled for + // first beta) propagates to installs the admin hasn't customised. const storedVersion = Number(parseSeedConfig(existing.trigger_config).seedVersion) || 0; - const isEnabled = existing.enabled === true || existing.enabled === 1; - if (isEnabled || storedVersion >= def.version) return; + const adminOwned = !!existing.admin_toggled_at; + if (adminOwned || storedVersion >= def.version) return; const newVersion = (existing.version || 1) + 1; await db.transaction(async (trx) => { diff --git a/backend/src/services/eventReminderService.js b/backend/src/services/eventReminderService.js index 07b92908..3c60f16b 100644 --- a/backend/src/services/eventReminderService.js +++ b/backend/src/services/eventReminderService.js @@ -128,13 +128,13 @@ async function runEventReminderPass() { return { scanned: 0, sent: 0, skipped: 0, disabled: true }; } - // Mutual exclusion with the workflow engine: once the pre_event_email built-in - // is seeded (flag on), the engine OWNS the reminder — it sends via the - // notify_pre_event action when the flow is enabled, or nothing when the admin - // disabled it. Either way the legacy pass stands down so the two never - // double-send. Fails closed → legacy pass keeps running if the subsystem is down. + // Mutual exclusion with the workflow engine: the legacy pass stands down only + // when the pre_event_email built-in is ENABLED (then the engine sends via the + // notify_pre_event action). If the flow is disabled, this legacy pass keeps + // running — so the built-ins can ship disabled without going dark, and + // disabling a built-in cleanly reverts to the legacy path. Fails closed. try { - if (await require('./workflows').isBuiltinFlowPresent('pre_event_email')) { + if (await require('./workflows').isBuiltinFlowActive('pre_event_email')) { return { scanned: 0, sent: 0, skipped: 0, byWorkflow: true }; } } catch (_) { /* workflow subsystem down → keep the legacy pass running */ } diff --git a/backend/src/services/expirationChecker.js b/backend/src/services/expirationChecker.js index f5db10d9..d9876383 100644 --- a/backend/src/services/expirationChecker.js +++ b/backend/src/services/expirationChecker.js @@ -26,12 +26,14 @@ async function checkExpirations() { // but skip the LEGACY email so the two never double-send. State transitions // (is_active=false, archive) always run regardless — they're the expiry // mechanic, not the notification. - // Existence-based: once the built-in is seeded the engine OWNS the email, so - // the legacy send stands down whether the flow is enabled (it sends) or - // disabled (admin turned it off). The trigger is still emitted regardless. - const { isBuiltinFlowPresent } = require('./workflows'); - const warningFlowOwns = await isBuiltinFlowPresent('gallery_expiring'); - const expiredFlowOwns = await isBuiltinFlowPresent('gallery_expired'); + // Enabled-based mutual exclusion: the legacy email stands down only when the + // matching built-in is ENABLED (then its action sends the identical mail). A + // disabled built-in leaves the legacy send running — so the flows can ship + // disabled without galleries going un-notified, and disabling a flow reverts + // to legacy. The trigger is still emitted regardless (for any custom flows). + const { isBuiltinFlowActive } = require('./workflows'); + const warningFlowOwns = await isBuiltinFlowActive('gallery_expiring'); + const expiredFlowOwns = await isBuiltinFlowActive('gallery_expired'); // Check for events needing warning emails // Skip events with null expires_at (they never expire) diff --git a/backend/src/services/invoiceService.js b/backend/src/services/invoiceService.js index 3847ca83..497ce2a6 100644 --- a/backend/src/services/invoiceService.js +++ b/backend/src/services/invoiceService.js @@ -2765,20 +2765,27 @@ async function applyReminder(invoice, lineItems, level, adminId) { attachments.push({ filename: `${fresh.invoice_number}_Mahnung.pdf`, contentPath: mahnungPath, contentType: 'application/pdf' }); const { to: reminderTo, cc: reminderCc } = resolveBillingRecipients(customer, invoice.cc_pdf_email); - await emailProcessor.queueEmail(invoice.event_id || null, reminderTo, templateKey, { - invoice_number: invoice.invoice_number, - customer_name: customer.display_name || customer.first_name || customer.email.split('@')[0], - total_amount: formatMajor(invoice.total_amount_minor, invoice.currency, locale), - new_total_amount: formatMajor(newTotal, invoice.currency, locale), - outstanding_amount: formatMajor(outstandingMinor, invoice.currency, locale), - paid_amount: formatMajor(invoice.paid_amount_minor, invoice.currency, locale), - late_fee_amount: formatMajor(lateFeeGross, invoice.currency, locale), - due_date: formatShortDate(invoice.due_date), - days_overdue: daysOverdue, - cc: reminderCc, - attachments, - // Dunning reminders are relationship mail — hold to business hours. - }, { respectBusinessHours: true }); + try { + await emailProcessor.queueEmail(invoice.event_id || null, reminderTo, templateKey, { + invoice_number: invoice.invoice_number, + customer_name: customer.display_name || customer.first_name || customer.email.split('@')[0], + total_amount: formatMajor(invoice.total_amount_minor, invoice.currency, locale), + new_total_amount: formatMajor(newTotal, invoice.currency, locale), + outstanding_amount: formatMajor(outstandingMinor, invoice.currency, locale), + paid_amount: formatMajor(invoice.paid_amount_minor, invoice.currency, locale), + late_fee_amount: formatMajor(lateFeeGross, invoice.currency, locale), + due_date: formatShortDate(invoice.due_date), + days_overdue: daysOverdue, + cc: reminderCc, + attachments, + // Dunning reminders are relationship mail — hold to business hours. + }, { respectBusinessHours: true }); + } catch (err) { + // Don't leave the just-rendered Mahnung PDF orphaned on disk if queueing the + // email failed — it would only be reachable via the next reminder anyway. + try { fs.unlinkSync(mahnungPath); } catch (_) { /* best-effort cleanup */ } + throw err; + } try { await logActivity('invoice_reminder_sent', { invoiceId: invoice.id, level, lateFeeMinor: lateFeeGross }, @@ -3447,14 +3454,15 @@ async function runScheduledTasks() { // Throttled to one email per 24h per invoice via // invoices.last_payment_check_at. const remindersEnabled = await getAppSetting('crm_invoices_reminders_enabled'); - // Mutual exclusion with the workflow engine: once the invoice_dunning built-in - // is seeded (flag on), the engine OWNS dunning and is the single switch — it - // fires the payment-check emails when the flow is enabled, or nothing when the - // admin disabled it. Either way the hardcoded ladder stands down so the two - // never double-send. Fails closed → ladder stays on if the subsystem is down. + // Mutual exclusion with the workflow engine: the hardcoded ladder stands down + // only when the invoice_dunning built-in is ENABLED (then the engine fires the + // payment-check emails). A disabled built-in leaves this ladder running — so + // the flow can ship disabled without dunning going dark, and disabling the + // flow reverts to the ladder. Fails closed → ladder stays on if the subsystem + // is down. let engineDrivesDunning = false; try { - engineDrivesDunning = await require('./workflows').isBuiltinFlowPresent('invoice_dunning'); + engineDrivesDunning = await require('./workflows').isBuiltinFlowActive('invoice_dunning'); } catch (_) { /* workflows tables absent / flag system down → ladder stays on */ } if (remindersEnabled !== false && !engineDrivesDunning) { const firstDays = ensureInt(await getAppSetting('crm_invoices_reminder_first_days')) || 14;