Skip to content

Commit 255a777

Browse files
fix(plugin-approvals): approval notifications reach their recipient with their text, sent as body, the field messaging reads (#21888)
Fixes #21847 Clause-②: no ## What changes Every notification the approvals service sends reached its recipient as a title over an empty body. That covers a comment, a request-info question, a send-back note, a reassignment, a reminder, an escalation, an SLA breach and an out-of-office substitution. The service put the text in `payload.message`. The messaging service builds the delivered notification from `payload.title` and `payload.body` only: the inline fan-out and the outbox snapshot in `messaging-service.ts`, and `processRow` in `dispatcher.ts`. So `GET /api/v1/notifications` answered `body: ""`, and the inbox row's `body_md` was empty. - `packages/plugins/plugin-approvals/src/approval-service.ts`: the twelve `notify()` call sites send their text as `body`. The texts are byte-identical: each removed `message:` line equals its added `body:` line once the key is stripped, indentation included (12 of 12). Who is notified, and when, is unchanged. - `notify()` now declares its payload as a local, non-exported type `ApprovalNotificationPayload` (`title`, `body`, `actionUrl`, and a reminder's `actions`). It used to take a free-form string-keyed record. A call site that spells the text any other way is now a compile error (Leg B below). No package entry gains an export. - No edit to `service-messaging` or `packages/spec`, and no `message` alias anywhere. There is one field, as `EmitInput` documents, per the triage direction on the card. - `.changeset/21847-approval-notification-body.md`: a `patch` for `@objectstack/plugin-approvals`. ## Measured before the change (`origin/main` e864db5) - **H1 holds.** All twelve call sites put the text in `payload.message` (`approval-service.ts` :3076 :3087 :4209 :4252 :4512 :4571 :4594 :4788 :4835 :5880 :5898 :5911). Messaging reads only `payload.body` (`messaging-service.ts` :1101 and :1221, `dispatcher.ts` :341). The red reading is under Tests. - **H2.** No call site bypasses `notify()`: the only `messaging.emit` in the package is the helper's own. The helper forwards the payload each call site builds, so the one fix inside the helper is the declared payload type. The renames happen at the call sites. - **H3. Nothing reads `payload.message` from an approval notification.** - This repo: `git grep` for `payload.message`, `payload?.message` and `{{ message }}` answered 5 hits. None reads a notification: an SMS transport's HTTP error, CLI and runtime test fixtures, and a PM script. - objectui at the `.objectui-sha` pin `0abd4f9f`: 7 hits, all HTTP error-response readers. For example, `apps/console/src/services/approvalsApi.ts:200` reads a failed request's JSON. Control term `/notifications` answered 58 lines at the same tree. - One possible reader stays unmeasured. `template-renderer.ts` spreads the payload into the template context, so a tenant-authored `sys_notification_template` row for an `approval.*` topic could have written `{{ message }}`. This repo seeds no such row. The changeset names the `{{ body }}` spelling. - **H4.** The texts name the object and record, and for an out-of-office substitution the two user ids. Every recipient is already a party to the request. The texts are unchanged. - No `content/docs/**` sentence is made false. The notification passages in `content/docs/automation/approvals.mdx` describe topics and deep links, not the field. ## Tests Both pins read plugin-approvals **source**. The unit file imports `./approval-service.js`. The dogfood `isolated` project aliases `@objectstack/plugin-approvals` to `src/index.ts`. Leg A proves this: `dist/` held the fix while the door pin went red. - `plugin-approvals/src/approval-notification-body.integration.test.ts` (new) drives the real `ApprovalService` on `ObjectQL` over `SqlDriver` (in-memory better-sqlite3). It pins three things: - the battery reaches every call site that can notify anyone (11 of 12); - a comment, a request-info question and a send-back note are their recipient's `body`; - every payload carries a non-empty `body` and no field outside the declared type. - `packages/qa/dogfood/test/approval-notification-body.dogfood.test.ts` (new) boots an app with real approvals, messaging and REST, and an email-authored user approver. A submitter's comment and an approver's request-info question are read back from each recipient's `GET /api/v1/notifications`. Each is the one row of its topic and has a title, so the old defect reads as a body that lost its text, not as a missing notification. **Red, at 68baad4** (the tests only, unfixed service): - unit: `Tests 2 failed | 1 passed (3)`. The comment read `expected [ [ 'u_alice' ], undefined ]`. There were 22 offenders, `body is undefined` plus `undeclared payload field 'message'`, at each of the 11 reached sites. The coverage pin was green. - door: `AssertionError: the comment is the notification's body: expected '' to be 'The signed contract is attached to th…'`. The row count of 1 and the non-empty title both held. **Green, at 7ca65ce:** - `pnpm --filter @objectstack/plugin-approvals typecheck`: exit 0. The test layer held 8 files, 324 errors and 27 pinned signatures, and the new file is clean. - `pnpm --filter @objectstack/plugin-approvals exec vitest run --maxWorkers=2`: `Test Files 61 passed (61)`, `Tests 898 passed (898)`. - `pnpm --filter @objectstack/dogfood exec vitest run --maxWorkers=2 --project isolated test/approval-notification-body.dogfood.test.ts`: `Tests 1 passed (1)`. - `pnpm --filter @objectstack/dogfood typecheck`: exit 0. - Build: `pnpm turbo run build --filter='@objectstack/dogfood^...' --filter='@objectstack/plugin-approvals^...' --concurrency=1` ran 63 of 63 tasks, 48 cached. plugin-approvals was rebuilt at 7ca65ce. ## Ablation, at 7ca65ce (`scripts/ablation-replace.mjs`, wrap mode, plus a trap restore) - **Leg A: `message` put back in the helper.** The anchor `severity: 'info', ...event, payload, audience,` went from 1 hit to 0. It was replaced by a call that sends `{ ...rest, message: body }`, and the replacement went from 0 hits to 1. The blob moved from 9a18228 to d066658afd62. - unit: exit 1, 2 failed (comment `body` undefined; 22 offenders). - door: exit 1, `expected '' to be 'The signed contract…'`. - Restore: blob 9a18228 equals HEAD, and `git diff HEAD` is empty. The trap's restore leg repeated that proof at exit. - **Leg B: one call site spells the text `message`.** At the request-info site, `body: input.comment.trim(),` became `message: input.comment.trim(),`. `tsc --noEmit` exited 1 with `src/approval-service.ts(4810,11): error TS2353: Object literal may only specify known properties, and 'message' does not exist in type 'ApprovalNotificationPayload'.` Restore: blob equals HEAD. ## Gates, at 7ca65ce - `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` (no paths) derived 70 commands, and all 70 ran. - On the first pass, 69 exited 0. `pnpm check:dual-build-cjs-loads` exited 3 (PREREQUISITE NOT MET: 8 packages had no `dist/`). - Those 8 were built (41 of 41 tasks cached) and the gate re-ran with exit 0: 106 entry points across 66 packages load. - `--ran` verdict: `70 derived, 70 run, 0 NOT-MEASURED, 0 UNRUN.` - Narrowed lint: `pnpm exec eslint --no-inline-config --format json` on the three touched `.ts` files. The JSON reports 3 files linted, with 0 errors and 0 warnings. - Population: `eslint.config.mjs`'s `packages/**/*.{ts,…}` blocks match all three, and none is ignored. - Invariance: the config never enables type-aware linting (no `parserOptions.project`), so this diff cannot change the verdict on any untouched file. - The full `pnpm lint`, the full dogfood suite and the full turbo test are CI's. ## Acceptance notes - **The reminder to a slot literal never reaches messaging.** That is the remind fan-out's branch for `position:P`-style pending entries. Its audience holds only slot literals, and `notify()` drops every audience entry containing a colon, so it returns 0 before emitting. Its payload is renamed with the rest and held by the compiler, and the producer battery covers the other eleven call sites. - Not filed: this run did not measure it at a public door. - Whether a position holder should receive that nudge is a different question from this card. --- _Generated by [Claude Code](https://claude.ai/code/session_011K3zqE8Pv1Evw5hc8tZCnN)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 67c544c commit 255a777

4 files changed

Lines changed: 489 additions & 14 deletions

File tree

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
'@objectstack/plugin-approvals': patch
3+
---
4+
5+
Approval notifications reach their recipient with their text (#21847). The approvals service put each notification's text in `payload.message`. The messaging service builds the delivered notification from `payload.title` and `payload.body`, the fields its `EmitInput` documents, and no channel reads `message`. So `GET /api/v1/notifications` served every approval notification as a title over an empty `body`, and the inbox row's `body_md` was empty too. The lost texts were comments, request-info questions, send-back notes, reassignments, reminders, escalations, SLA breaches and out-of-office substitutions. Every one of them now travels in `payload.body`.
6+
7+
Clause-②: no
8+
9+
- The texts are unchanged. Only the field they travel in moved. Who is notified, and when, is unchanged.
10+
- Approval notifications no longer carry `payload.message`. Nothing in the platform or the console read it. A tenant-authored `sys_notification_template` for an `approval.*` topic that wrote `{{ message }}` should write `{{ body }}`.
11+
- The service's notify helper now declares its payload: `title`, `body`, `actionUrl`, and a reminder's `actions`. A call site that spells the text any other way no longer compiles.
12+
- `@objectstack/service-messaging` is unchanged. There is one field, as documented, and no alias for the old one.
Lines changed: 251 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,251 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* Every approval notification carries its text in `body` — the one field the
5+
* `messaging` service's `EmitInput` documents for it, and the only one the
6+
* delivered notification is built from.
7+
*
8+
* ## The defect
9+
*
10+
* Every approval notification reached its recipient as a title over an empty
11+
* body. The service put its text in `payload.message`; messaging reads
12+
* `payload.title` and `payload.body` (the inline fan-out, the outbox snapshot
13+
* and the dispatcher alike), so the comment, the request-info question and
14+
* every escalation line were dropped on the way to the inbox. The flow
15+
* `notify` node and the @mention producer already send `body`.
16+
*
17+
* ## What this pins
18+
*
19+
* The fix is at the producer, with no alias in messaging. `notify()` types its
20+
* payload, so a call site that spells the text any other way does not compile;
21+
* this file pins the behaviour across EVERY call site the service has, driven
22+
* through the real `ApprovalService` on a real `ObjectQL` over `SqlDriver`
23+
* (better-sqlite3, in-memory, real DDL):
24+
*
25+
* - a comment and a request-info question ARE the body their recipient gets;
26+
* - every notification carries a non-empty `body` and no field outside the
27+
* payload `notify()` declares — so a second spelling of the text is red;
28+
* - the battery reaches every call site that can notify anyone — eleven of
29+
* the twelve — so neither pin passes over a site it never drove.
30+
*
31+
* The twelfth, the reminder to a slot literal (`position:<p>`), never reaches
32+
* messaging at all: `notify()` drops every audience entry that is a slot
33+
* address, and that reminder's audience is nothing else. Its payload is held
34+
* to the same declared shape by the compiler, not by this battery.
35+
*
36+
* The end-to-end half — the text read back from the recipient's
37+
* `GET /api/v1/notifications` through the real messaging service — is the
38+
* dogfood pin `packages/qa/dogfood/test/approval-notification-body.dogfood.test.ts`.
39+
*/
40+
41+
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
42+
import { ObjectQL } from '@objectstack/objectql';
43+
import { SqlDriver } from '@objectstack/driver-sql';
44+
import { APPROVAL_REVISE_NODE_TYPE } from '@objectstack/spec/automation';
45+
import { ApprovalService } from './approval-service.js';
46+
import { ApprovalsServicePlugin } from './approvals-plugin.js';
47+
48+
const SYSTEM = { isSystem: true, positions: [], permissions: [] } as any;
49+
const asUser = (userId: string, positions: string[] = []) =>
50+
({ userId, positions, permissions: [] }) as any;
51+
52+
const USERS = [
53+
{ id: 'u_sub', name: 'Sam Submitter', email: 'sam@example.com' },
54+
{ id: 'u_legal', name: 'Lee Legal', email: 'lee@example.com' },
55+
{ id: 'u_alice', name: 'Alice', email: 'alice@example.com' },
56+
{ id: 'u_bob', name: 'Bob', email: 'bob@example.com' },
57+
{ id: 'u_ooo', name: 'Olive Away', email: 'olive@example.com' },
58+
{ id: 'u_boss', name: 'Boss', email: 'boss@example.com' },
59+
];
60+
61+
/** The text each person writes — distinct, so no one can pass for another. */
62+
const COMMENT = 'The signed contract is attached to the record now.';
63+
const QUESTION = 'Which cost centre should this be booked against?';
64+
const SEND_BACK = 'Split the total by quarter, please.';
65+
66+
/** The fields `notify()` declares for a notification's payload. */
67+
const PAYLOAD_FIELDS = new Set(['title', 'body', 'actionUrl', 'actions']);
68+
69+
const sysUser = {
70+
name: 'sys_user',
71+
label: 'User',
72+
fields: {
73+
id: { name: 'id', label: 'Id', type: 'text' as const, primaryKey: true },
74+
name: { name: 'name', label: 'Name', type: 'text' as const },
75+
email: { name: 'email', label: 'Email', type: 'text' as const },
76+
},
77+
};
78+
const deal = {
79+
name: 'crm_deal',
80+
label: 'Deal',
81+
fields: {
82+
id: { name: 'id', label: 'Id', type: 'text' as const, primaryKey: true },
83+
title: { name: 'title', label: 'Title', type: 'text' as const },
84+
},
85+
};
86+
87+
/** The flow the requests belong to: an approval node with a revise window. */
88+
const FLOW = {
89+
name: 'deal_review',
90+
nodes: [
91+
{ id: 'review', type: 'approval' },
92+
{ id: 'rework', type: APPROVAL_REVISE_NODE_TYPE },
93+
],
94+
edges: [{ id: 'e_revise', source: 'review', target: 'rework', label: 'revise' }],
95+
};
96+
97+
/** The plugin's own object definitions, as its manifest registers them. */
98+
async function packageObjects(): Promise<any[]> {
99+
let manifest: any;
100+
const ctx: any = {
101+
getService: (name: string) => {
102+
if (name === 'manifest') return { register: (m: any) => { manifest = m; } };
103+
throw new Error(`no service '${name}'`);
104+
},
105+
logger: { info: () => {}, warn: () => {}, error: () => {}, debug: () => {} },
106+
};
107+
await new ApprovalsServicePlugin().init(ctx);
108+
return manifest.objects;
109+
}
110+
111+
interface Emitted {
112+
topic: string;
113+
audience: string[];
114+
source?: { object: string; id: string };
115+
payload?: Record<string, unknown>;
116+
}
117+
118+
describe('every approval notification carries its text in body', () => {
119+
let engine: ObjectQL;
120+
let svc: ApprovalService;
121+
let now = Date.parse('2026-10-01T09:00:00.000Z');
122+
const emits: Emitted[] = [];
123+
const ids: Record<string, string> = {};
124+
125+
const open = async (key: string, approvers: any[], extra: Record<string, unknown> = {}) => {
126+
await engine.insert('crm_deal', { id: `D_${key}`, title: key }, { context: SYSTEM } as any);
127+
const row: any = await svc.openNodeRequest({
128+
object: 'crm_deal', recordId: `D_${key}`, runId: `run_${key}`, nodeId: 'review', flowName: FLOW.name,
129+
config: { approvers, behavior: 'first_response', ...extra } as any,
130+
submitterId: 'u_sub', record: { id: `D_${key}`, title: key },
131+
}, asUser('u_sub'));
132+
ids[key] = row.id;
133+
return row;
134+
};
135+
136+
/** Which call site an emit came from: its topic, its request, and what tells two sites on one request apart. */
137+
const siteOf = (e: Emitted): string => {
138+
const key = Object.keys(ids).find((k) => ids[k] === e.source?.id) ?? '?';
139+
const links = Array.isArray(e.payload?.actions) ? '+links' : '';
140+
const toSubmitter = e.topic === 'approval.sla_breached' && e.audience.join() === 'u_sub' ? '>submitter' : '';
141+
return `${e.topic}@${key}${links}${toSubmitter}`;
142+
};
143+
144+
beforeAll(async () => {
145+
engine = new ObjectQL();
146+
engine.registerDriver(new SqlDriver({
147+
client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true,
148+
}), true);
149+
await engine.init();
150+
for (const def of [sysUser, deal, ...(await packageObjects())]) {
151+
engine.registry.registerObject(def as any, 'approvals-test', 'approvals-test');
152+
}
153+
await engine.syncSchemas();
154+
for (const u of USERS) await engine.insert('sys_user', u, { context: SYSTEM } as any);
155+
156+
svc = new ApprovalService({
157+
engine: engine as any,
158+
// Every reading moves a second on, so the action log has one order.
159+
clock: { now: () => new Date(now += 1000) },
160+
automation: {
161+
resume: async () => ({ status: 'completed' }),
162+
cancelRun: async () => undefined,
163+
getRun: async (runId: string) => ({ id: runId, status: 'paused' }),
164+
getFlow: async () => FLOW,
165+
} as any,
166+
logger: { warn: () => {}, info: () => {}, error: () => {}, debug: () => {} } as any,
167+
});
168+
svc.attachMessaging({ async emit(input) { emits.push(input as Emitted); } });
169+
170+
// ── The thread: a position slot (a literal no account holds) and a person ──
171+
const thread = await open('thread', [{ type: 'position', value: 'legal' }, { type: 'user', value: 'u_alice' }],
172+
{ behavior: 'unanimous' });
173+
expect(thread.pending_approvers).toEqual(['position:legal', 'u_alice']);
174+
await svc.comment(ids.thread, { actorId: 'u_sub', comment: COMMENT }, asUser('u_sub'));
175+
await svc.requestInfo(ids.thread, { actorId: 'u_alice', comment: QUESTION }, asUser('u_alice'));
176+
// A remind reaches the person with their own action links.
177+
await svc.remind(ids.thread, { actorId: 'u_sub' }, asUser('u_sub'));
178+
await svc.reassign(ids.thread, { actorId: 'u_alice', to: 'u_bob' }, asUser('u_alice'));
179+
180+
// ── Send back, within budget and past it ──
181+
await open('revise', [{ type: 'user', value: 'u_alice' }]);
182+
await svc.sendBack(ids.revise, { actorId: 'u_alice', comment: SEND_BACK } as any, asUser('u_alice'));
183+
await open('budget', [{ type: 'user', value: 'u_alice' }], { maxRevisions: 0 });
184+
await svc.sendBack(ids.budget, { actorId: 'u_alice' } as any, asUser('u_alice'));
185+
186+
// ── Out of office: the routing substitutes a delegate ──
187+
await engine.insert('sys_approval_delegation', {
188+
id: 'dlg_1', delegator_id: 'u_ooo', delegate_id: 'u_bob', reason: 'leave',
189+
valid_from: '2026-09-01T00:00:00.000Z', valid_until: '2026-12-31T00:00:00.000Z',
190+
created_at: '2026-09-01T00:00:00.000Z', updated_at: '2026-09-01T00:00:00.000Z',
191+
}, { context: SYSTEM } as any);
192+
const ooo = await open('ooo', [{ type: 'user', value: 'u_ooo' }]);
193+
expect(ooo.pending_approvers).toEqual(['u_bob']);
194+
195+
// ── The SLA sweep: a notify (submitter told) and a reassign (submitter not) ──
196+
await open('sla_notify', [{ type: 'user', value: 'u_alice' }],
197+
{ escalation: { timeoutHours: 1, action: 'notify', escalateTo: 'u_boss', notifySubmitter: true } });
198+
await open('sla_reassign', [{ type: 'user', value: 'u_alice' }],
199+
{ escalation: { timeoutHours: 1, action: 'reassign', escalateTo: 'u_boss', notifySubmitter: false } });
200+
now += 2 * 3600_000;
201+
expect(await svc.runEscalations()).toMatchObject({ escalated: 2 });
202+
});
203+
204+
afterAll(async () => {
205+
try { await engine?.destroy(); } catch { /* noop */ }
206+
});
207+
208+
it('the battery reaches every call site that can notify anyone — eleven of the twelve', () => {
209+
expect(new Set(emits.map(siteOf))).toEqual(new Set([
210+
'approval.ooo_substituted@ooo',
211+
'approval.ooo_skipped@ooo',
212+
'approval.returned@budget',
213+
'approval.returned@revise',
214+
'approval.reassigned@thread',
215+
'approval.reminder@thread+links',
216+
'approval.request_info@thread',
217+
'approval.comment@thread',
218+
'approval.escalated@sla_reassign',
219+
'approval.sla_breached@sla_notify',
220+
'approval.sla_breached@sla_notify>submitter',
221+
]));
222+
});
223+
224+
it("a comment, a request-info question and a send-back note are their recipient's body", () => {
225+
const one = (topic: string, key: string) => {
226+
const hits = emits.filter((e) => e.topic === topic && e.source?.id === ids[key]);
227+
expect(hits.length, `${topic} on ${key}`).toBe(1);
228+
return hits[0];
229+
};
230+
const comment = one('approval.comment', 'thread');
231+
expect([comment.audience, comment.payload?.body]).toEqual([['u_alice'], COMMENT]);
232+
const question = one('approval.request_info', 'thread');
233+
expect([question.audience, question.payload?.body]).toEqual([['u_sub'], QUESTION]);
234+
const sentBack = one('approval.returned', 'revise');
235+
expect([sentBack.audience, sentBack.payload?.body]).toEqual([['u_sub'], SEND_BACK]);
236+
});
237+
238+
it('every notification carries a non-empty body, and no field the payload does not declare', () => {
239+
const offenders: string[] = [];
240+
for (const e of emits) {
241+
const payload = e.payload ?? {};
242+
if (typeof payload.body !== 'string' || payload.body.trim() === '') {
243+
offenders.push(`${siteOf(e)}: body is ${JSON.stringify(payload.body)}`);
244+
}
245+
for (const field of Object.keys(payload)) {
246+
if (!PAYLOAD_FIELDS.has(field)) offenders.push(`${siteOf(e)}: undeclared payload field '${field}'`);
247+
}
248+
}
249+
expect(offenders).toEqual([]);
250+
});
251+
});

0 commit comments

Comments
 (0)