Both defects came out of Phase 13's acceptance walk, and neither was visible to any test. **1. The one-shot rule-group guard was not a guard.** `seedRuleGroup` and `coreRules.seedGroup` both read their settings stamp, inserted the whole group, and wrote the stamp AFTER the loop. Two instances booting in the same moment both read "absent" and both insert -- the walk ended up with 52 module rules where the module ships 26. `docker compose up --scale app=2` and a rolling restart both start two instances on purpose, so this is ordinary rather than exotic. `settings.db` gains `claim(key, value)`: the same `INSERT IGNORE` as `seedDefault`, reporting its own `affectedRows`, so exactly one caller can win a key. The atomicity is the PRIMARY KEY's -- no transaction, no lock, the same bargain `engagementWorker`'s row claim already makes. Both seeders claim before inserting. The trade the code already documented is unchanged, only its order: a process that dies mid-loop leaves the group stamped and partly seeded, and the missing rules are an operator's visit to the "new rule" form. A duplicate rule is two mails per event, for every rule in the group, which both functions' own comments already call the worse outcome. **Why no test caught it:** the stubs supplied `get` and `set` over a Map, which cannot race, so they agreed with the bug -- Phase 4a's `foundRows` finding in a different costume. The fake is now `claim`-shaped and decides without yielding, exactly as the table does, and both suites gained a test that runs two seeders with `Promise.all` and asserts one insert each. Verified by reverting the fix: the module test then reports every rule inserted twice. **2. A rule that names a `digest` body could not be saved.** `registries.js` `checkSeedRule` permits `digest` in as many words -- it is the body `teamDigestWorker` renders for a rule whose email channel an individual set to digest mode, so it never appears in `channels` and never could -- and MODULE_API.md 2.4 tells modules they may point `template_keys` at `notify.digest`. `engagementRules.model.js` then rejected any key that was not one of the rule's channels. So every rule shipping a digest body answered an operator who opened it and pressed Save with a 400 naming a key they had never typed: core's own Team and news rules, and sixteen of module-uo's. The only way to save was to delete the digest body, silently dropping digest support from that rule. The two validators now agree; anything that is neither a channel nor `digest` is still refused, with a test for each direction. No MODULE_API bump: no member is added, removed or changed, and the documented contract is what the code now honours rather than something new. Co-Authored-By: Claude <noreply@anthropic.com>
368 lines
14 KiB
JavaScript
368 lines
14 KiB
JavaScript
// ── registerEngagementSeeds + the module seeder ────────────────────────────
|
|
//
|
|
// ENGAGEMENT.md Phase 11b, decision 7. Two halves, tested apart because they
|
|
// fail differently: the REGISTRY refuses a bad declaration at boot with the key
|
|
// named, and the SEEDER decides what reaches the database and — much more
|
|
// importantly — what does not reach it a second time.
|
|
//
|
|
// The properties worth a test are the ones no hand run would catch:
|
|
//
|
|
// • a module cannot ship an ENABLED rule, or a `protected` template, or a body
|
|
// for someone else's trigger, or a rule pointing at a template that does not
|
|
// exist. Each of those is a shipped mistake that only shows up as mail.
|
|
// • templates are re-ensured and rules are NOT — the asymmetry the whole
|
|
// design rests on, and the one an implementer would most plausibly "tidy".
|
|
// • a disabled module is skipped, which is the operator's switch meaning what
|
|
// it says even for content that is only rows in a table.
|
|
|
|
process.env.DB_HOST = '127.0.0.1'
|
|
process.env.DB_PORT = '59999'
|
|
|
|
const { test, beforeEach, after } = require('node:test')
|
|
const assert = require('node:assert/strict')
|
|
|
|
const registries = require('../src/modules/registries')
|
|
const db = require('../src/utils/db')
|
|
|
|
after(() => db.close())
|
|
beforeEach(() => registries._reset())
|
|
|
|
const blocks = [{ id: 'p1', type: 'email.text', props: { text: 'Hail, {{siteName}}.' } }]
|
|
|
|
const tpl = (over = {}) => ({
|
|
key: 'demo.house.warning',
|
|
name: 'A warning',
|
|
channel: 'email',
|
|
subject: 'A warning',
|
|
seedVersion: 1,
|
|
blocks,
|
|
...over,
|
|
})
|
|
|
|
const rule = (over = {}) => ({
|
|
trigger_id: 'demo.house.warning',
|
|
name: 'House warning',
|
|
audience: 'owner',
|
|
channels: ['email'],
|
|
template_keys: { email: 'demo.house.warning' },
|
|
cooldown_seconds: 3600,
|
|
max_sends_per_hour: 200,
|
|
...over,
|
|
})
|
|
|
|
/** Register a seed batch as `owner`; returns the error message or null. */
|
|
function trySeeds(owner, seeds) {
|
|
const api = registries.stage(owner)
|
|
try {
|
|
api.registerEngagementSeeds(seeds)
|
|
registries.apply(api.staged)
|
|
return null
|
|
} catch (err) {
|
|
return err.message
|
|
}
|
|
}
|
|
|
|
// ── The registry: what a module may and may not ship ───────────────────────
|
|
|
|
/**
|
|
* The settings table, as far as these tests need it — and specifically its
|
|
* ATOMICITY. `claim` is the real `INSERT IGNORE`: it decides and writes without
|
|
* yielding, so exactly one caller can win a key, which is the property the guard
|
|
* depends on. A fake that read with `get` and wrote later with `set` could never
|
|
* express that, and agreed with the bug Phase 13's walk found.
|
|
*/
|
|
function fakeSettings(store = new Map()) {
|
|
return {
|
|
store,
|
|
get: async (k) => store.get(k) || null,
|
|
set: async (k, v) => { store.set(k, v) },
|
|
claim: async (k, v) => {
|
|
if (store.has(k)) return false
|
|
store.set(k, v)
|
|
return true
|
|
},
|
|
}
|
|
}
|
|
|
|
test('a well-formed batch registers and reads back under its owner', () => {
|
|
assert.equal(trySeeds('demo', {
|
|
templates: [tpl()],
|
|
ruleGroups: [{ key: 'v1', note: 'the first set', rules: [rule()] }],
|
|
}), null)
|
|
|
|
const all = registries.allEngagementSeeds()
|
|
assert.equal(all.length, 1)
|
|
assert.equal(all[0].owner, 'demo')
|
|
assert.equal(all[0].templates.length, 1)
|
|
assert.equal(all[0].ruleGroups[0].key, 'v1')
|
|
assert.deepEqual(registries.engagementSeedsFor('demo').templates[0].key, 'demo.house.warning')
|
|
assert.equal(registries.engagementSeedsFor('nobody'), null)
|
|
})
|
|
|
|
test('a seeded rule is always disabled, whatever the module said', () => {
|
|
// Q3's invariant, and the one place in the workstream where a module could
|
|
// have overridden it. `enabled: 1` is not refused — it is IGNORED — because
|
|
// refusing would let a typo take a deployment's whole module offline at boot.
|
|
assert.equal(trySeeds('demo', {
|
|
templates: [tpl()],
|
|
ruleGroups: [{ key: 'v1', rules: [rule({ enabled: 1 })] }],
|
|
}), null)
|
|
assert.equal(registries.engagementSeedsFor('demo').ruleGroups[0].rules[0].enabled, 0)
|
|
})
|
|
|
|
test('a template key must be namespaced to its owner', () => {
|
|
// `engagement_templates.key` is UNIQUE across the table, so an unprefixed
|
|
// `notify.event` from a module would collide with core's and win or lose on
|
|
// boot order.
|
|
const err = trySeeds('demo', { templates: [tpl({ key: 'notify.event' })] })
|
|
assert.match(err, /not namespaced "demo\."/)
|
|
})
|
|
|
|
test('a module may not ship a rule for a trigger it does not own', () => {
|
|
const err = trySeeds('demo', {
|
|
templates: [tpl()],
|
|
ruleGroups: [{ key: 'v1', rules: [rule({ trigger_id: 'news.post' })] }],
|
|
})
|
|
assert.match(err, /not namespaced "demo\."/)
|
|
})
|
|
|
|
test('a module may not mark a template protected', () => {
|
|
const err = trySeeds('demo', { templates: [tpl({ protected: true })] })
|
|
assert.match(err, /may not be protected/)
|
|
})
|
|
|
|
test('a rule must name a template that exists — its own or core\'s', () => {
|
|
const missing = trySeeds('demo', {
|
|
templates: [tpl()],
|
|
ruleGroups: [{ key: 'v1', rules: [rule({ template_keys: { email: 'demo.nope' } })] }],
|
|
})
|
|
assert.match(missing, /neither one of its own seeds nor core's/)
|
|
|
|
// Core's generic bodies ARE permitted — that is §4.6.1 property 1 in force,
|
|
// and the nine plain bodies of decision 9 are exactly this case.
|
|
registries._reset()
|
|
assert.equal(trySeeds('demo', {
|
|
ruleGroups: [{
|
|
key: 'v1',
|
|
rules: [rule({ template_keys: { email: 'notify.event', inapp: 'inapp.event', digest: 'notify.digest' } })],
|
|
}],
|
|
}), null)
|
|
})
|
|
|
|
test('an email body needs a subject and an in-app body may not have one', () => {
|
|
assert.match(trySeeds('demo', { templates: [tpl({ subject: null })] }), /no subject/)
|
|
registries._reset()
|
|
assert.match(
|
|
trySeeds('demo', { templates: [tpl({ channel: 'inapp' })] }),
|
|
/cannot carry a subject/,
|
|
)
|
|
registries._reset()
|
|
assert.equal(trySeeds('demo', { templates: [tpl({ channel: 'inapp', subject: null })] }), null)
|
|
})
|
|
|
|
test('a rule must carry a per-hour ceiling', () => {
|
|
// Q3: the module chooses the number and may not decline to have one.
|
|
const err = trySeeds('demo', {
|
|
templates: [tpl()],
|
|
ruleGroups: [{ key: 'v1', rules: [rule({ max_sends_per_hour: 0 })] }],
|
|
})
|
|
assert.match(err, /max_sends_per_hour/)
|
|
})
|
|
|
|
test('a normalized rule carries every column the insert reads', () => {
|
|
// Refusing a bad ceiling and then DROPPING a good one are different bugs, and
|
|
// the first test cannot see the second: `engagementRules.db.insert` binds a
|
|
// fixed column list, so a field validated and not carried through arrives as
|
|
// NULL and fails the whole group at boot — on a real database only. Asserted
|
|
// against the column list itself rather than one field, because the next
|
|
// field added to the declaration is the next one that can be forgotten here.
|
|
assert.equal(trySeeds('demo', {
|
|
templates: [tpl()],
|
|
ruleGroups: [{ key: 'v1', rules: [rule({ delay_seconds: 60, cancel_on: ['demo.house.refreshed'] })] }],
|
|
}), null)
|
|
|
|
const seeded = registries.engagementSeedsFor('demo').ruleGroups[0].rules[0]
|
|
for (const column of [
|
|
'trigger_id', 'name', 'enabled', 'audience', 'audience_segment_id', 'max_sends_per_hour',
|
|
'channels', 'template_keys', 'conditions', 'cooldown_seconds', 'delay_seconds', 'cancel_on',
|
|
'updated_by',
|
|
]) {
|
|
assert.ok(column in seeded, `normalized rule is missing "${column}"`)
|
|
assert.notEqual(seeded[column], undefined, `normalized rule leaves "${column}" undefined`)
|
|
}
|
|
assert.equal(seeded.max_sends_per_hour, 200)
|
|
assert.equal(seeded.delay_seconds, 60)
|
|
assert.deepEqual(seeded.cancel_on, ['demo.house.refreshed'])
|
|
})
|
|
|
|
test('registering twice is a collision, not an addition', () => {
|
|
assert.equal(trySeeds('demo', { templates: [tpl()] }), null)
|
|
assert.match(trySeeds('demo', { templates: [tpl({ key: 'demo.other' })] }), /already registered/)
|
|
})
|
|
|
|
test('a bad template leaves nothing behind — validate-then-commit', () => {
|
|
const err = trySeeds('demo', {
|
|
templates: [tpl(), tpl({ key: 'demo.bad', channel: 'sms' })],
|
|
ruleGroups: [{ key: 'v1', rules: [rule()] }],
|
|
})
|
|
assert.match(err, /unknown channel "sms"/)
|
|
assert.equal(registries.engagementSeedsFor('demo'), null)
|
|
assert.deepEqual(registries.allEngagementSeeds(), [])
|
|
})
|
|
|
|
// ── The seeder ─────────────────────────────────────────────────────────────
|
|
|
|
const moduleSeeds = require('../src/engagement/moduleSeeds')
|
|
|
|
/** A registered batch, shaped the way `allEngagementSeeds()` returns it. */
|
|
function registered(owner, seeds) {
|
|
assert.equal(trySeeds(owner, seeds), null)
|
|
return () => registries.allEngagementSeeds()
|
|
}
|
|
|
|
test('guardKey names both the owner and the group', () => {
|
|
// Two modules may use the same group name, and one module may add a second
|
|
// group later without disturbing the first.
|
|
assert.equal(moduleSeeds.guardKey('uo', 'triggers-v1'), 'engagement_module_rules_seeded:uo:triggers-v1')
|
|
assert.notEqual(moduleSeeds.guardKey('uo', 'a'), moduleSeeds.guardKey('other', 'a'))
|
|
})
|
|
|
|
test('templates are re-ensured every run and rule groups are seeded once', async () => {
|
|
// The asymmetry the design rests on. A second run must re-offer every template
|
|
// (so a bumped seedVersion reaches an existing deployment) and must offer no
|
|
// rule at all (so a rule an operator deleted stays deleted).
|
|
const seeds = registered('demo', {
|
|
templates: [tpl()],
|
|
ruleGroups: [{ key: 'v1', rules: [rule()] }],
|
|
})
|
|
|
|
const settings = new Map()
|
|
const seededTemplates = []
|
|
const insertedRules = []
|
|
const stub = {
|
|
templatesDb: {
|
|
seedOne: async (t) => { seededTemplates.push(t.key); return 'inserted' },
|
|
staleCustomized: async () => [],
|
|
},
|
|
rulesDb: { insert: async (r) => { insertedRules.push(r.trigger_id) } },
|
|
settingsDb: fakeSettings(settings),
|
|
}
|
|
|
|
await moduleSeeds.seedModuleEngagement({ seeds, ...stub })
|
|
await moduleSeeds.seedModuleEngagement({ seeds, ...stub })
|
|
|
|
assert.deepEqual(seededTemplates, ['demo.house.warning', 'demo.house.warning'])
|
|
assert.deepEqual(insertedRules, ['demo.house.warning'])
|
|
assert.ok(settings.has(moduleSeeds.guardKey('demo', 'v1')))
|
|
})
|
|
|
|
test('a partial rule group is still stamped', async () => {
|
|
// Re-running would duplicate the rules that DID insert, and a duplicate rule
|
|
// is two mails per event — worse than the one missing rule an operator can add
|
|
// from the Rules screen. `coreRules.seedGroup` made the same call.
|
|
const seeds = registered('demo', {
|
|
templates: [tpl()],
|
|
ruleGroups: [{ key: 'v1', rules: [rule(), rule({ trigger_id: 'demo.house.gone', name: 'Gone' })] }],
|
|
})
|
|
|
|
const settings = new Map()
|
|
let inserts = 0
|
|
await moduleSeeds.seedModuleEngagement({
|
|
seeds,
|
|
templatesDb: { seedOne: async () => 'inserted', staleCustomized: async () => [] },
|
|
rulesDb: {
|
|
insert: async () => {
|
|
inserts += 1
|
|
if (inserts === 2) throw new Error('duplicate')
|
|
},
|
|
},
|
|
settingsDb: fakeSettings(settings),
|
|
})
|
|
|
|
assert.equal(inserts, 2)
|
|
assert.ok(settings.has(moduleSeeds.guardKey('demo', 'v1')))
|
|
})
|
|
|
|
test('two instances booting together seed the group once, not twice', async () => {
|
|
// The defect Phase 13's acceptance walk found, and the reason the guard is a
|
|
// CLAIM rather than a read followed by a write. `docker compose up
|
|
// --scale app=2` and a rolling restart both start two instances on purpose;
|
|
// under the old ordering both read "not seeded" before either stamped, and
|
|
// both inserted the whole group. The walk ended up with 52 module rules where
|
|
// the module ships 26 — and a duplicate rule is two mails per event.
|
|
//
|
|
// The two calls are deliberately NOT awaited in turn: interleaving them is the
|
|
// whole test, and awaiting the first would pass against the bug.
|
|
const seeds = registered('demo', {
|
|
templates: [tpl()],
|
|
ruleGroups: [{ key: 'v1', rules: [rule(), rule({ trigger_id: 'demo.house.gone', name: 'Gone' })] }],
|
|
})
|
|
|
|
const settings = fakeSettings()
|
|
const inserted = []
|
|
const stub = {
|
|
seeds,
|
|
templatesDb: { seedOne: async () => 'inserted', staleCustomized: async () => [] },
|
|
rulesDb: { insert: async (r) => { inserted.push(r.trigger_id) } },
|
|
settingsDb: settings,
|
|
}
|
|
|
|
await Promise.all([
|
|
moduleSeeds.seedModuleEngagement({ ...stub }),
|
|
moduleSeeds.seedModuleEngagement({ ...stub }),
|
|
])
|
|
|
|
assert.deepEqual(inserted, ['demo.house.warning', 'demo.house.gone'])
|
|
assert.ok(settings.store.has(moduleSeeds.guardKey('demo', 'v1')))
|
|
})
|
|
|
|
test('a skipped owner is seeded not at all', async () => {
|
|
// The operator's switch means what it says even for content that is only rows.
|
|
const seeds = registered('demo', {
|
|
templates: [tpl()],
|
|
ruleGroups: [{ key: 'v1', rules: [rule()] }],
|
|
})
|
|
let touched = 0
|
|
await moduleSeeds.seedModuleEngagement({
|
|
seeds,
|
|
skip: new Set(['demo']),
|
|
templatesDb: { seedOne: async () => { touched += 1; return 'inserted' }, staleCustomized: async () => [] },
|
|
rulesDb: { insert: async () => { touched += 1 } },
|
|
settingsDb: fakeSettings(),
|
|
})
|
|
assert.equal(touched, 0)
|
|
})
|
|
|
|
test('a database failure is logged, never thrown — this is the boot path', async () => {
|
|
const seeds = registered('demo', {
|
|
templates: [tpl()],
|
|
ruleGroups: [{ key: 'v1', rules: [rule()] }],
|
|
})
|
|
await moduleSeeds.seedModuleEngagement({
|
|
seeds,
|
|
templatesDb: {
|
|
seedOne: async () => { throw new Error('table is gone') },
|
|
staleCustomized: async () => { throw new Error('also gone') },
|
|
},
|
|
rulesDb: { insert: async () => { throw new Error('gone too') } },
|
|
settingsDb: { claim: async () => { throw new Error('and gone') }, set: async () => {} },
|
|
})
|
|
})
|
|
|
|
test('an invalid block array is refused rather than stored', async () => {
|
|
// A shipped block array no renderer understands reads to an operator as their
|
|
// deployment being broken. Refusing leaves renderByKey's fallback in charge.
|
|
const seeds = registered('demo', {
|
|
templates: [tpl({ blocks: [{ id: 'x', type: 'email.nosuchblock', props: {} }] })],
|
|
})
|
|
let stored = 0
|
|
const totals = await moduleSeeds.seedModuleEngagement({
|
|
seeds,
|
|
templatesDb: { seedOne: async () => { stored += 1; return 'inserted' }, staleCustomized: async () => [] },
|
|
rulesDb: { insert: async () => {} },
|
|
settingsDb: fakeSettings(),
|
|
})
|
|
assert.equal(stored, 0)
|
|
assert.equal(totals.templates, 0)
|
|
})
|