|
| 1 | +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. |
| 2 | + |
| 3 | +/** |
| 4 | + * [#7258] `OpportunityStageHook` pins the probability — on the PERSISTED row. |
| 5 | + * |
| 6 | + * This file exists because a careful reader got it wrong. #7225 read the three |
| 7 | + * shipped example hooks against `HookContextSchema.input`'s contract table |
| 8 | + * (`{ data, options }` — the record lives at `ctx.input.data`), concluded that |
| 9 | + * this hook's `input.stage` is always `undefined` and that it silently does |
| 10 | + * nothing, and prescribed re-spelling every one of them to `ctx.input.data`. |
| 11 | + * Measurement answered the other way: all three work, and the prescribed fix |
| 12 | + * would have turned the showcase's public web-to-lead insert into a hard |
| 13 | + * refusal. One full dispatch was spent on the false alarm. |
| 14 | + * |
| 15 | + * THE MISSING LINK, and why the contract table did not lie: `bindHooks` wraps |
| 16 | + * every DECLARATIVE hook in `wrapDeclarativeHook`, which calls `installFlatInput` |
| 17 | + * (`packages/objectql/src/hook-wrappers.ts:446`, helper at `:502`). That swaps |
| 18 | + * `ctx.input` for a Proxy presenting a FLAT RECORD VIEW over the envelope — |
| 19 | + * reads of a non-wrapper key resolve against `data`, and writes always land in |
| 20 | + * `data`, "so the engine's downstream `input.data` read picks up mutations made |
| 21 | + * by user code as `input.field = value`". The contract table describes the |
| 22 | + * RAW-ENGINE surface; an authored hook only ever meets the declarative one. |
| 23 | + * (The table's silence about that is filed separately as #7254.) |
| 24 | + * |
| 25 | + * WHY THE ASSERTIONS ARE ON PERSISTED ROWS. The defect #7225 believed in was |
| 26 | + * "the handler runs and has no effect" — a shape that a handler-side spy reads |
| 27 | + * as healthy. Only the row that came back out of the database can tell |
| 28 | + * "mutated the envelope nobody reads" from "mutated the record". So the harness |
| 29 | + * is the real `ObjectQL` engine over a real `SqlDriver` (better-sqlite3), the |
| 30 | + * app's REAL `crm_opportunity` object and the app's REAL hook, bound the way |
| 31 | + * `AppPlugin` binds it from `defineStack({ hooks })` |
| 32 | + * (`packages/runtime/src/app-plugin.ts`, `ql.bindHooks(hooks, { packageId })`). |
| 33 | + * No double anywhere in the chain. |
| 34 | + * |
| 35 | + * WHAT THIS FILE DOES AND DOES NOT CATCH — measured, both directions, rather |
| 36 | + * than assumed. Unregistering the hook turns four of the cases below red; |
| 37 | + * misspelling its `closed_won` predicate turns exactly the two closed_won cases |
| 38 | + * red and leaves closed_lost green. But re-spelling the handler to |
| 39 | + * `ctx.input.data` — the change #7225 prescribed — keeps all seven GREEN, and |
| 40 | + * that is correct rather than a hole: on the code-handler path `data` is a |
| 41 | + * wrapper key the proxy passes straight through, so both spellings really do |
| 42 | + * work here. The spelling is only fatal inside a sandboxed `body`, where |
| 43 | + * `ctx.input.data` is `undefined`; the pin that bites on it is therefore the |
| 44 | + * showcase's (`examples/app-showcase/test/hook-body-persisted-writes.test.ts`), |
| 45 | + * not this one. Read the two files as one pair. |
| 46 | + * |
| 47 | + * Test-only pin: nothing in `src/` changes. If this file ever goes red, the |
| 48 | + * behaviour it describes regressed — the hook is not to be "fixed" toward the |
| 49 | + * raw-engine spelling on the strength of the contract table alone. |
| 50 | + */ |
| 51 | + |
| 52 | +import { describe, it, expect, afterEach } from 'vitest'; |
| 53 | +import { ObjectQL } from '@objectstack/objectql'; |
| 54 | +import { SqlDriver } from '@objectstack/driver-sql'; |
| 55 | + |
| 56 | +import { Account, Opportunity, OpportunityLineItem } from '../src/objects/index.js'; |
| 57 | +import { allHooks } from '../src/hooks/index.js'; |
| 58 | +import { OpportunityStageHook } from '../src/hooks/opportunity.hook.js'; |
| 59 | + |
| 60 | +/** The app id `AppPlugin` derives its `packageId` from. */ |
| 61 | +const PACKAGE_ID = 'app:com.example.crm'; |
| 62 | + |
| 63 | +/** Engines opened by a test, destroyed when that test ends. */ |
| 64 | +const openEngines: ObjectQL[] = []; |
| 65 | +afterEach(async () => { |
| 66 | + while (openEngines.length) { |
| 67 | + try { await openEngines.pop()?.destroy(); } catch { /* noop */ } |
| 68 | + } |
| 69 | +}); |
| 70 | + |
| 71 | +/** |
| 72 | + * A real kernel-free engine carrying the app's real CRM objects. |
| 73 | + * |
| 74 | + * `hooks` is a parameter so the reverse check below can withhold exactly one |
| 75 | + * thing — the binding — from an otherwise identical engine. |
| 76 | + */ |
| 77 | +async function bootCrm(hooks: unknown[] = allHooks): Promise<ObjectQL> { |
| 78 | + const driver = new SqlDriver({ |
| 79 | + client: 'better-sqlite3', |
| 80 | + connection: { filename: ':memory:' }, |
| 81 | + useNullAsDefault: true, |
| 82 | + }); |
| 83 | + await driver.connect(); |
| 84 | + |
| 85 | + const engine = new ObjectQL(); |
| 86 | + openEngines.push(engine); |
| 87 | + engine.registerDriver(driver as never, true); |
| 88 | + await engine.init(); |
| 89 | + |
| 90 | + // The app's real objects, not a reduction of them: `crm_opportunity` carries |
| 91 | + // a required `account` lookup, a `line_total` summary over the line items, a |
| 92 | + // state-machine rule on `stage` and a formula reading `probability`. Any of |
| 93 | + // those could plausibly be what actually moves the column, so all of them are |
| 94 | + // in the picture. |
| 95 | + for (const def of [Account, Opportunity, OpportunityLineItem]) { |
| 96 | + engine.registry.registerObject(def as never, PACKAGE_ID, 'crm'); |
| 97 | + } |
| 98 | + await engine.syncSchemas(); |
| 99 | + |
| 100 | + // Exactly the AppPlugin call, minus the sandbox body runner this app has no |
| 101 | + // use for (its single hook is an inline code handler). |
| 102 | + engine.bindHooks(hooks as never[], { packageId: PACKAGE_ID }); |
| 103 | + return engine; |
| 104 | +} |
| 105 | + |
| 106 | +const ctx = { context: { userId: 'u_crm', isSystem: true } }; |
| 107 | + |
| 108 | +describe('#7258 — app-crm `OpportunityStageHook` moves the persisted probability', () => { |
| 109 | + /** An account to hang the opportunities off — `account` is a required lookup. */ |
| 110 | + async function seedAccount(engine: ObjectQL): Promise<string> { |
| 111 | + const account: any = await engine.insert('crm_account', { name: 'Acme', industry: 'technology' }, ctx as never); |
| 112 | + return String(account.id); |
| 113 | + } |
| 114 | + |
| 115 | + const readBack = async (engine: ObjectQL, id: string) => |
| 116 | + (await engine.find('crm_opportunity', { where: { id } }, ctx as never))[0] as any; |
| 117 | + |
| 118 | + it('INSERT closed_won: the stored row has probability 100, not the authored 50', async () => { |
| 119 | + const engine = await bootCrm(); |
| 120 | + const accountId = await seedAccount(engine); |
| 121 | + |
| 122 | + const created: any = await engine.insert( |
| 123 | + 'crm_opportunity', |
| 124 | + { name: 'Won deal', account: accountId, stage: 'closed_won', probability: 50 }, |
| 125 | + ctx as never, |
| 126 | + ); |
| 127 | + |
| 128 | + // The row as the DATABASE holds it. A hook that mutated only the envelope |
| 129 | + // would leave the authored 50 here. |
| 130 | + const stored = await readBack(engine, String(created.id)); |
| 131 | + expect(stored.stage).toBe('closed_won'); |
| 132 | + expect(stored.probability).toBe(100); |
| 133 | + }, 30000); |
| 134 | + |
| 135 | + it('INSERT closed_lost: the stored row has probability 0 — the falsy end of the pin', async () => { |
| 136 | + // `0` matters on its own: it is the value a `??`/`||`-shaped repair would |
| 137 | + // silently drop, so pinning only the 100 case would leave half the hook |
| 138 | + // unwitnessed. |
| 139 | + const engine = await bootCrm(); |
| 140 | + const accountId = await seedAccount(engine); |
| 141 | + |
| 142 | + const created: any = await engine.insert( |
| 143 | + 'crm_opportunity', |
| 144 | + { name: 'Lost deal', account: accountId, stage: 'closed_lost', probability: 90 }, |
| 145 | + ctx as never, |
| 146 | + ); |
| 147 | + |
| 148 | + const stored = await readBack(engine, String(created.id)); |
| 149 | + expect(stored.stage).toBe('closed_lost'); |
| 150 | + expect(stored.probability).toBe(0); |
| 151 | + }, 30000); |
| 152 | + |
| 153 | + it('UPDATE into closed_won: the hook fires on beforeUpdate too', async () => { |
| 154 | + // `events` lists both, and the update path binds `input` differently from |
| 155 | + // insert (the envelope carries `id` as well), so this is a distinct seam |
| 156 | + // rather than a repetition of the insert case. |
| 157 | + const engine = await bootCrm(); |
| 158 | + const accountId = await seedAccount(engine); |
| 159 | + |
| 160 | + const created: any = await engine.insert( |
| 161 | + 'crm_opportunity', |
| 162 | + { name: 'Advancing deal', account: accountId, stage: 'proposal', probability: 40 }, |
| 163 | + ctx as never, |
| 164 | + ); |
| 165 | + const id = String(created.id); |
| 166 | + expect((await readBack(engine, id)).probability).toBe(40); |
| 167 | + |
| 168 | + await engine.update('crm_opportunity', { id, stage: 'closed_won' }, ctx as never); |
| 169 | + |
| 170 | + const stored = await readBack(engine, id); |
| 171 | + expect(stored.stage).toBe('closed_won'); |
| 172 | + expect(stored.probability).toBe(100); |
| 173 | + }, 30000); |
| 174 | + |
| 175 | + it('CONTROL: a non-closed stage is left exactly as authored', async () => { |
| 176 | + // The half that makes the three cases above mean something. A hook that |
| 177 | + // stamped unconditionally — or an engine that recomputed the column on |
| 178 | + // every write — would also satisfy them. |
| 179 | + const engine = await bootCrm(); |
| 180 | + const accountId = await seedAccount(engine); |
| 181 | + |
| 182 | + const created: any = await engine.insert( |
| 183 | + 'crm_opportunity', |
| 184 | + { name: 'Open deal', account: accountId, stage: 'proposal', probability: 40 }, |
| 185 | + ctx as never, |
| 186 | + ); |
| 187 | + |
| 188 | + const stored = await readBack(engine, String(created.id)); |
| 189 | + expect(stored.stage).toBe('proposal'); |
| 190 | + expect(stored.probability).toBe(40); |
| 191 | + }, 30000); |
| 192 | + |
| 193 | + it('THE #7225 QUESTION: the handler sees the record FLAT on `ctx.input`', async () => { |
| 194 | + // The documented-surface witness for the code-handler half (the showcase |
| 195 | + // file carries the sandboxed-body twin). #7225's whole case was that |
| 196 | + // `input.stage` is `undefined` here; it is bound, and `input.data` — the |
| 197 | + // spelling the contract table teaches — is a passthrough on this path |
| 198 | + // rather than the only one that works. |
| 199 | + const engine = await bootCrm(); |
| 200 | + const accountId = await seedAccount(engine); |
| 201 | + |
| 202 | + const seen: Array<Record<string, unknown>> = []; |
| 203 | + engine.bindHooks( |
| 204 | + [{ |
| 205 | + name: 'probe_flat_input', |
| 206 | + object: 'crm_opportunity', |
| 207 | + events: ['beforeInsert'], |
| 208 | + // Lower than the real hook's 100 is irrelevant to what it observes: it |
| 209 | + // reads the caller's payload, which no ordering changes. |
| 210 | + priority: 10, |
| 211 | + handler: async (hookCtx: any) => { |
| 212 | + const input = hookCtx.input as Record<string, unknown>; |
| 213 | + seen.push({ |
| 214 | + keys: Object.keys(input), |
| 215 | + stage: input.stage, |
| 216 | + typeofData: typeof (input as { data?: unknown }).data, |
| 217 | + }); |
| 218 | + }, |
| 219 | + }] as never[], |
| 220 | + { packageId: 'probe' }, |
| 221 | + ); |
| 222 | + |
| 223 | + await engine.insert( |
| 224 | + 'crm_opportunity', |
| 225 | + { name: 'Probe deal', account: accountId, stage: 'closed_won', probability: 50 }, |
| 226 | + ctx as never, |
| 227 | + ); |
| 228 | + |
| 229 | + expect(seen).toHaveLength(1); |
| 230 | + // The flat record's own fields are what `Object.keys` enumerates — the |
| 231 | + // proxy's `ownKeys` trap, which is also why a sandboxed body receives the |
| 232 | + // record rather than the envelope. |
| 233 | + expect(seen[0].keys).toContain('stage'); |
| 234 | + expect(seen[0].keys).toContain('probability'); |
| 235 | + expect(seen[0].stage).toBe('closed_won'); |
| 236 | + }, 30000); |
| 237 | + |
| 238 | + it('REVERSE: the same writes with the hook unbound leave the authored values', async () => { |
| 239 | + // The pins above must be able to FAIL. This engine is identical except that |
| 240 | + // `bindHooks` is handed nothing — the #4984 phantom check, run as a |
| 241 | + // fixture rather than as a promise: if the assertions here also read 100/0, |
| 242 | + // then something other than the hook is moving the column and every |
| 243 | + // expectation above is vacuous. |
| 244 | + const engine = await bootCrm([]); |
| 245 | + const accountId = await seedAccount(engine); |
| 246 | + |
| 247 | + const won: any = await engine.insert( |
| 248 | + 'crm_opportunity', |
| 249 | + { name: 'Won deal, hook unbound', account: accountId, stage: 'closed_won', probability: 50 }, |
| 250 | + ctx as never, |
| 251 | + ); |
| 252 | + const lost: any = await engine.insert( |
| 253 | + 'crm_opportunity', |
| 254 | + { name: 'Lost deal, hook unbound', account: accountId, stage: 'closed_lost', probability: 90 }, |
| 255 | + ctx as never, |
| 256 | + ); |
| 257 | + |
| 258 | + expect((await readBack(engine, String(won.id))).probability).toBe(50); |
| 259 | + expect((await readBack(engine, String(lost.id))).probability).toBe(90); |
| 260 | + }, 30000); |
| 261 | + |
| 262 | + it('the hook is registered on the app bundle — an unregistered hook never runs', async () => { |
| 263 | + // The behavioural cases bind the hook themselves (this file assembles its |
| 264 | + // own engine). This one pins the wiring the RUNTIME reads: `AppPlugin` |
| 265 | + // walks `defineStack({ hooks })` via `collectBundleHooks` and nothing else, |
| 266 | + // so a hook absent from that array is dead metadata however correct its |
| 267 | + // file is. That is exactly what #7036 found in app-todo. |
| 268 | + const stack = (await import('../objectstack.config.js')).default as { |
| 269 | + hooks?: Array<{ name?: string; object?: string; events?: string[] }>; |
| 270 | + }; |
| 271 | + const registered = (stack.hooks ?? []).find((h) => h.name === OpportunityStageHook.name); |
| 272 | + expect(registered, '`opportunity_stage_probability` must be in defineStack({ hooks })').toBeDefined(); |
| 273 | + expect(registered!.object).toBe('crm_opportunity'); |
| 274 | + expect(registered!.events).toEqual(expect.arrayContaining(['beforeInsert', 'beforeUpdate'])); |
| 275 | + }); |
| 276 | +}); |
0 commit comments