Skip to content

Commit 44fe210

Browse files
fix(plugin-approvals): name the record by object label and id in every approval notification body (#24064)
Fixes #23978 Clause-②: no ## What changed Every approval notification body that names its record now names it through `nameSubjectRecord`. The record is named by the object's label and the record id (`Deal D_123`), or by the object's API name and the id when the schema declares no label. No body carries a title, from any source. This is the form ruling B of #23556 / #23594 set, and the one `approval.requested` already sent. The rest of each sentence is unchanged. `packages/plugins/plugin-approvals/src/approval-service.ts`, line numbers at this head. Each method names the record once and every body in that method uses that name: - `sendBack`, `subject` at `:4752`: `approval.returned` revision-limit auto-reject (`Your Deal D_1 exceeded the revision limit …`), and the send-back with no note (`Your Deal D_1 needs rework …`). - `reassign`, inline at `:5118`: `approval.reassigned` (`You are now an approver on Deal D_1.`). - `remind`, `subject` at `:5164`: both `approval.reminder` bodies, the one with action links and the one to slot literals. - `escalateRequest`, `subject` at `:6508`: `approval.escalated`, plus `approval.sla_breached` to the approvers and to the submitter. Rendered before / after, for a `crm_deal` labelled `Deal`: `A decision on crm_deal/D_1 is overdue.` became `A decision on Deal D_1 is overdue.` ### Two sites beyond the claim's eight (a declared widening, its own commit `a42511d0ac`) `approval.ooo_substituted` (`:3624`) and `approval.ooo_skipped` (`:3636`) in `openNodeRequest` built `${input.object}/${input.recordId}`. That renders the same slash form under a different variable spelling, so the claim's search for the `raw.object_name` spelling missed it. The battery's new slash sweep found both bodies. I fixed them in place because all four bounded in-place conditions hold: 1. They have the same defect class as this card. 2. The fix is mechanical, and the same method already calls the same helper at `:3691`. 3. No other claim holds the file (the claim records no open PR touching it). 4. They use the same test battery, so there is no new verification surface. To drop the widening, revert `a42511d0ac`. The battery's sweep then goes red on those two bodies, which is the defect itself. ### Not changed (claim exclusions) - The `DUPLICATE_REQUEST` refusal text at `:4903`. - `nameSubjectRecord` and `objectLabelOf`. - `sys-approval-request.object.ts`. - Anything in `packages/spec`. ## Premise check (`origin/main` `5c2ac83a23`) All eight claimed sites built `${raw.object_name}/${raw.record_id}` into a notification `body`. Each `raw` is a `sys_approval_request` row, read by `loadPendingRow` (`sendBack`, `reassign`, `remind`) or by the `runEscalations` scan (`escalateRequest`). When the request opens, its `object_name` and `record_id` are written from `input.object` and `input.recordId` (`:3556-3557`), so they are the subject record. `nameSubjectRecord` answers the label and the id, or the object name and the id when there is no label. All eight are the same family. ## Pin `approval-notification-body.integration.test.ts` is the battery that already drives every notifying call site. It runs the real `ApprovalService` over a real `ObjectQL` on `SqlDriver` (better-sqlite3, in memory), with the messaging surface recording what reaches it. This PR adds two requests to it: - `rework`: a send-back with no note. - `bare`: a reminder about a `crm_memo` record, an object whose schema has no label (the control). It also adds one test. The test asserts the record's name in each body (not the sentence around it): the reminder, both SLA-breach bodies, the escalation, the hand-off, both send-back sentences and both out-of-office sentences. For the control it asserts `crm_memo D_bare`. It also sweeps every body the battery sent for the slash form. - **Red against the unfixed source** (test-only commit `a941d0711d`): seven of the claimed sites received `crm_deal/D_…`, including the reminder (`A decision on crm_deal/D_thread is still waiting on you.`) and the SLA breach (`A decision on crm_deal/D_sla_notify is overdue.`). The four existing tests stayed green with the two added requests. - **Green** at `a42511d0ac`: the battery passes 5/5, and the package passes 81 files / 1104 tests (`vitest run --maxWorkers=2`). `pnpm --filter @objectstack/plugin-approvals typecheck` exits 0, and `check:test-typecheck` holds the ledger unchanged (8 files / 324 errors). - **Ablation** (`scripts/ablation-replace.mjs`, WRAP mode): I put the SLA-breach body back to the slash form. The anchor went from 1 hit to 0 and the blob changed from `c01b0f0749dd` to `46ea05a5b390`. The run went red with exactly one offender: `approval.sla_breached@sla_notify: A decision on crm_deal/D_sla_notify is overdue.` The restore was proven: the blob is `c01b0f0749dd`, equal to HEAD, and `git diff HEAD` is empty. The test imports the subject relatively from `src/`, so the ablation has no `dist/` leg. - The slot-literal reminder body (`:5201`) never reaches messaging, because `notify()` drops every slot-address audience. The battery's docblock already records this, and the compiler holds its shape. ## Gates (head `54a1f98fe6`, after merging `origin/main` `905f57bc74`) - I ran all 67 commands from `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` and recorded each exit before any pipe. 65 exited 0. - 2 exited 3 (`PREREQUISITE NOT MET`), so they are NOT MEASURED. This is a declared narrowing; CI's `Lint & Repo Gates` runs both on a full build. - `pnpm check:dual-build-cjs-loads` needs every package's `dist/` (37 absent), which means a whole-workspace build. - `pnpm check:i18n` needs the CLI's build closure plus ten packages. This diff changes no translatable source: no object or field label, nothing under `translations/`. - `dispatch-gates --ran`: 67 derived, 65 run, 2 NOT-MEASURED (derived from the recorded exit 3), 0 UNRUN. It exits 0. - Changeset level axis, measured offline: `node scripts/check-changeset-no-major.mjs --base 5c2ac83 --event` with this body's first two lines exits 0 (`patch` under `Clause-②: no`). ## Acceptance notes - **Persons named by raw user id (not fixed here, reported to the seat).** The two out-of-office bodies name the people by raw user id, for example `You are covering an approval on Deal D_ooo while u_ooo is out of office.` and `… was routed to u_bob …`. This is a person-naming defect, not record naming, so it stays outside this card's family. - **Stale docblocks.** The `objectLabelOf` and `nameSubjectRecord` docblocks still describe the helper as shared by the `approval.requested` body only. The claim excluded both from this edit, so they are left as they stand. --- _Generated by [Claude Code](https://claude.ai/code/session_018VqJPGA1NDNSCZsnecsChP)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent c7168ac commit 44fe210

3 files changed

Lines changed: 97 additions & 15 deletions

File tree

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
---
2+
"@objectstack/plugin-approvals": patch
3+
---
4+
5+
Clause-②: no
6+
7+
fix(plugin-approvals): every approval notification body names its record by the object's label and the record id
8+
9+
- **What changed.** The reminder, SLA-breach (to approvers and to the submitter), escalation, hand-off, revision-limit, send-back (with no note) and both out-of-office notification bodies named the record as `<object_name>/<record_id>`, for example `crm_deal/D_123`. They now name it the way `approval.requested` already did: the object's label and the record id, for example `Deal D_123`. An object whose schema declares no label is named by its API name and the id, for example `crm_memo D_123`.
10+
- **No title.** No body carries the record's title, from the live record or from the submit snapshot. The body is stored and may be e-mailed, and it has no single reader whose field access it could be narrowed for.
11+
- **Unchanged.** Each sentence's other wording, every topic, title, `actionUrl`, action link and audience, and the `DUPLICATE_REQUEST` refusal text.

‎packages/plugins/plugin-approvals/src/approval-notification-body.integration.test.ts‎

Lines changed: 68 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,18 @@
3737
* `GET /api/v1/notifications` through the real messaging service — is the
3838
* dogfood pin `packages/qa/dogfood/test/approval-notification-body.dogfood.test.ts`.
3939
*
40+
* ## And how it names the record (#23978)
41+
*
42+
* Ruling B of #23556 / #23594 names the record an approval notification is
43+
* about by its object's label and its id (`Deal D_123`), with no title, through
44+
* `nameSubjectRecord`. `approval.requested` used it; every other body that
45+
* names the record built `<object_name>/<record_id>`, so an approver read the
46+
* API name and a slash in the reminder, the SLA breach, the escalation, the
47+
* hand-off, both send-back sentences and both out-of-office sentences. The same
48+
* battery pins the record's name in each of those bodies, and a control: a
49+
* record of an object whose schema has no label is named by the object's API
50+
* name and the id, the helper's own fallback.
51+
*
4052
* ## And whose notification it is (#23284)
4153
*
4254
* The same battery pins the organization every call site hands messaging: the
@@ -102,6 +114,14 @@ const deal = {
102114
title: { name: 'title', label: 'Title', type: 'text' as const },
103115
},
104116
};
117+
/** [#23978] The control: an object whose schema declares no label. */
118+
const memo = {
119+
name: 'crm_memo',
120+
fields: {
121+
id: { name: 'id', label: 'Id', type: 'text' as const, primaryKey: true },
122+
title: { name: 'title', label: 'Title', type: 'text' as const },
123+
},
124+
};
105125

106126
/** The flow the requests belong to: an approval node with a revise window. */
107127
const FLOW = {
@@ -142,10 +162,10 @@ describe('every approval notification carries its text in body', () => {
142162
const emits: Emitted[] = [];
143163
const ids: Record<string, string> = {};
144164

145-
const open = async (key: string, approvers: any[], extra: Record<string, unknown> = {}) => {
146-
await engine.insert('crm_deal', { id: `D_${key}`, title: key, organization_id: orgOfDeal(key) }, { context: SYSTEM } as any);
165+
const open = async (key: string, approvers: any[], extra: Record<string, unknown> = {}, object: string = deal.name) => {
166+
await engine.insert(object, { id: `D_${key}`, title: key, organization_id: orgOfDeal(key) }, { context: SYSTEM } as any);
147167
const row: any = await svc.openNodeRequest({
148-
object: 'crm_deal', recordId: `D_${key}`, runId: `run_${key}`, nodeId: 'review', flowName: FLOW.name,
168+
object, recordId: `D_${key}`, runId: `run_${key}`, nodeId: 'review', flowName: FLOW.name,
149169
config: { approvers, behavior: 'first_response', ...extra } as any,
150170
submitterId: 'u_sub', record: { id: `D_${key}`, title: key },
151171
}, asUser('u_sub'));
@@ -167,7 +187,7 @@ describe('every approval notification carries its text in body', () => {
167187
client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true,
168188
}), true);
169189
await engine.init();
170-
for (const def of [sysUser, deal, ...(await packageObjects())]) {
190+
for (const def of [sysUser, deal, memo, ...(await packageObjects())]) {
171191
engine.registry.registerObject(def as any, 'approvals-test', 'approvals-test');
172192
}
173193
await engine.syncSchemas();
@@ -202,6 +222,13 @@ describe('every approval notification carries its text in body', () => {
202222
await svc.sendBack(ids.revise, { actorId: 'u_alice', comment: SEND_BACK } as any, asUser('u_alice'));
203223
await open('budget', [{ type: 'user', value: 'u_alice' }], { maxRevisions: 0 });
204224
await svc.sendBack(ids.budget, { actorId: 'u_alice' } as any, asUser('u_alice'));
225+
// [#23978] Within budget and with no note, the body is the service's own sentence.
226+
await open('rework', [{ type: 'user', value: 'u_alice' }]);
227+
await svc.sendBack(ids.rework, { actorId: 'u_alice' } as any, asUser('u_alice'));
228+
229+
// ── [#23978] The control: a reminder about a record of an object with no label ──
230+
await open('bare', [{ type: 'user', value: 'u_alice' }], {}, memo.name);
231+
await svc.remind(ids.bare, { actorId: 'u_sub' }, asUser('u_sub'));
205232

206233
// ── Out of office: the routing substitutes a delegate ──
207234
await engine.insert('sys_approval_delegation', {
@@ -235,12 +262,16 @@ describe('every approval notification carries its text in body', () => {
235262
'approval.requested@budget',
236263
'approval.requested@sla_notify',
237264
'approval.requested@sla_reassign',
265+
'approval.requested@rework',
266+
'approval.requested@bare',
238267
'approval.ooo_substituted@ooo',
239268
'approval.ooo_skipped@ooo',
240269
'approval.returned@budget',
241270
'approval.returned@revise',
271+
'approval.returned@rework',
242272
'approval.reassigned@thread',
243273
'approval.reminder@thread+links',
274+
'approval.reminder@bare+links',
244275
'approval.request_info@thread',
245276
'approval.comment@thread',
246277
'approval.escalated@sla_reassign',
@@ -263,6 +294,39 @@ describe('every approval notification carries its text in body', () => {
263294
expect([sentBack.audience, sentBack.payload?.body]).toEqual([['u_sub'], SEND_BACK]);
264295
});
265296

297+
it('[#23978] every body that names the record names it by its object label and its id, as approval.requested does', () => {
298+
// Ruling B of #23556 / #23594: label and id, no title — one form for every
299+
// body, the one `approval.requested` already sends. The pin is the record's
300+
// name in each body, not the rest of the sentence around it.
301+
const NAMED: Record<string, string> = {
302+
'approval.requested@thread': 'Deal D_thread',
303+
'approval.reminder@thread+links': 'Deal D_thread',
304+
'approval.sla_breached@sla_notify': 'Deal D_sla_notify',
305+
'approval.sla_breached@sla_notify>submitter': 'Deal D_sla_notify',
306+
'approval.escalated@sla_reassign': 'Deal D_sla_reassign',
307+
'approval.reassigned@thread': 'Deal D_thread',
308+
'approval.returned@budget': 'Deal D_budget',
309+
'approval.returned@rework': 'Deal D_rework',
310+
'approval.ooo_substituted@ooo': 'Deal D_ooo',
311+
'approval.ooo_skipped@ooo': 'Deal D_ooo',
312+
// The control: the object's schema has no label, so the record is named
313+
// by the object's API name and the id.
314+
'approval.reminder@bare+links': 'crm_memo D_bare',
315+
};
316+
const offenders = Object.entries(NAMED).flatMap(([site, name]) => {
317+
const bodies = emits.filter((e) => siteOf(e) === site).map((e) => String(e.payload?.body));
318+
if (!bodies.length) return [`${site}: sent nothing`];
319+
return bodies.filter((body) => !body.includes(name)).map((body) => `${site}: ${body}`);
320+
});
321+
expect(offenders).toEqual([]);
322+
323+
// And no body anywhere in the battery joins the API name and the id with a slash.
324+
const slashed = emits
325+
.map((e) => `${siteOf(e)}: ${String(e.payload?.body)}`)
326+
.filter((line) => /\bcrm_(deal|memo)\/D_/.test(line));
327+
expect(slashed).toEqual([]);
328+
});
329+
266330
it('every notification carries a non-empty body, and no field the payload does not declare', () => {
267331
const offenders: string[] = [];
268332
for (const e of emits) {

‎packages/plugins/plugin-approvals/src/approval-service.ts‎

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3603,7 +3603,8 @@ export class ApprovalService implements IApprovalService {
36033603
// OOO substitution audit + notification (#1322 M4). Each hop that rerouted
36043604
// an approver away from an out-of-office user is recorded on the request's
36053605
// audit trail (a system action, no human actor) and notified to both the
3606-
// delegate — who now owns the slot — and the skipped approver.
3606+
// delegate — who now owns the slot — and the skipped approver. [#23978]
3607+
// Both bodies name the record the way every approval notification does.
36073608
for (const sub of substitutions) {
36083609
await this.engine.insert('sys_approval_action', await this.actionRow(row, {
36093610
id: uid('aact'), request_id: id, organization_id: requestOrg,
@@ -3620,7 +3621,7 @@ export class ApprovalService implements IApprovalService {
36203621
dedupKey: `approval-ooo-${id}-${sub.to}`,
36213622
payload: {
36223623
title: 'Approval routed to you (out-of-office cover)',
3623-
body: `You are covering an approval on ${input.object}/${input.recordId} while ${sub.from} is out of office.`,
3624+
body: `You are covering an approval on ${this.nameSubjectRecord(input.object, input.recordId)} while ${sub.from} is out of office.`,
36243625
actionUrl: '/system/approvals',
36253626
},
36263627
});
@@ -3632,7 +3633,7 @@ export class ApprovalService implements IApprovalService {
36323633
dedupKey: `approval-ooo-skip-${id}-${sub.from}`,
36333634
payload: {
36343635
title: 'Approval routed to your delegate',
3635-
body: `An approval on ${input.object}/${input.recordId} was routed to ${sub.to} while you are out of office.`,
3636+
body: `An approval on ${this.nameSubjectRecord(input.object, input.recordId)} was routed to ${sub.to} while you are out of office.`,
36363637
actionUrl: '/system/approvals',
36373638
},
36383639
});
@@ -4747,6 +4748,8 @@ export class ApprovalService implements IApprovalService {
47474748

47484749
const now = this.clock.now().toISOString();
47494750
const maxRevisions = typeof (config as any).maxRevisions === 'number' ? (config as any).maxRevisions : 3;
4751+
// [#23978] Both sentences below name the record the way every approval notification does.
4752+
const subject = this.nameSubjectRecord(raw.object_name, raw.record_id);
47504753
let priorSendBacks = 0;
47514754
if (runId && nodeId) {
47524755
const siblings = await this.engine.find('sys_approval_request', {
@@ -4806,7 +4809,7 @@ export class ApprovalService implements IApprovalService {
48064809
source: { object: 'sys_approval_request', id: requestId },
48074810
payload: {
48084811
title: 'Approval auto-rejected',
4809-
body: `Your ${raw.object_name}/${raw.record_id} exceeded the revision limit (${maxRevisions}) and was rejected.`,
4812+
body: `Your ${subject} exceeded the revision limit (${maxRevisions}) and was rejected.`,
48104813
actionUrl: '/system/approvals',
48114814
},
48124815
});
@@ -4850,7 +4853,7 @@ export class ApprovalService implements IApprovalService {
48504853
source: { object: 'sys_approval_request', id: requestId },
48514854
payload: {
48524855
title: 'Sent back for revision',
4853-
body: input.comment?.trim() || `Your ${raw.object_name}/${raw.record_id} needs rework before it can be approved.`,
4856+
body: input.comment?.trim() || `Your ${subject} needs rework before it can be approved.`,
48544857
actionUrl: '/system/approvals',
48554858
},
48564859
});
@@ -5112,7 +5115,7 @@ export class ApprovalService implements IApprovalService {
51125115
dedupKey: `approval-reassign-${requestId}-${to}`,
51135116
payload: {
51145117
title: 'Approval handed to you',
5115-
body: `You are now an approver on ${raw.object_name}/${raw.record_id}.`,
5118+
body: `You are now an approver on ${this.nameSubjectRecord(raw.object_name, raw.record_id)}.`,
51165119
actionUrl: '/system/approvals',
51175120
},
51185121
});
@@ -5157,6 +5160,8 @@ export class ApprovalService implements IApprovalService {
51575160
// Per-approver fan-out: concrete identities (user ids / emails) each get
51585161
// their OWN one-tap approve/reject links (ADR-0043); `position:*`-style
51595162
// literals can't carry a personal token and fall back to a plain nudge.
5163+
// [#23978] One name for every recipient, the form `approval.requested` uses.
5164+
const subject = this.nameSubjectRecord(raw.object_name, raw.record_id);
51605165
let notified = 0;
51615166
const concrete = pending.filter(a => a && !a.includes(':'));
51625167
const literals = pending.filter(a => a && a.includes(':'));
@@ -5172,7 +5177,7 @@ export class ApprovalService implements IApprovalService {
51725177
dedupKey: `approval-remind-${requestId}-${nowIso}-${approver}`,
51735178
payload: {
51745179
title: 'Approval reminder',
5175-
body: `A decision on ${raw.object_name}/${raw.record_id} is still waiting on you.`,
5180+
body: `A decision on ${subject} is still waiting on you.`,
51765181
actionUrl: '/system/approvals',
51775182
actions: [
51785183
{ label: 'Approve', url: this.actionLinkUrl(tokens.approve) },
@@ -5196,7 +5201,7 @@ export class ApprovalService implements IApprovalService {
51965201
dedupKey: `approval-remind-${requestId}-${nowIso}`,
51975202
payload: {
51985203
title: 'Approval reminder',
5199-
body: `A decision on ${raw.object_name}/${raw.record_id} is still waiting on you.`,
5204+
body: `A decision on ${subject} is still waiting on you.`,
52005205
actionUrl: '/system/approvals',
52015206
},
52025207
});
@@ -6499,6 +6504,8 @@ export class ApprovalService implements IApprovalService {
64996504
typeof esc.escalateTo === 'string' && esc.escalateTo.trim() ? esc.escalateTo.trim() : undefined;
65006505
const now = this.clock.now().toISOString();
65016506
const pending = csvSplit(raw.pending_approvers);
6507+
// [#23978] Every escalation sentence names the record the way every approval notification does.
6508+
const subject = this.nameSubjectRecord(raw.object_name, raw.record_id);
65026509

65036510
// `escalateTo` is a position machine name or a user id (same contract as
65046511
// the `position` ApproverType, ADR-0090 D3). Position holders win; an
@@ -6539,7 +6546,7 @@ export class ApprovalService implements IApprovalService {
65396546
source: { object: 'sys_approval_request', id: raw.id },
65406547
payload: {
65416548
title: 'Approval escalated to you',
6542-
body: `An overdue approval on ${raw.object_name}/${raw.record_id} was escalated to you.`,
6549+
body: `An overdue approval on ${subject} was escalated to you.`,
65436550
actionUrl: '/system/approvals',
65446551
},
65456552
});
@@ -6558,7 +6565,7 @@ export class ApprovalService implements IApprovalService {
65586565
source: { object: 'sys_approval_request', id: raw.id },
65596566
payload: {
65606567
title: 'Approval SLA breached',
6561-
body: `A decision on ${raw.object_name}/${raw.record_id} is overdue.`,
6568+
body: `A decision on ${subject} is overdue.`,
65626569
actionUrl: '/system/approvals',
65636570
},
65646571
});
@@ -6572,7 +6579,7 @@ export class ApprovalService implements IApprovalService {
65726579
source: { object: 'sys_approval_request', id: raw.id },
65736580
payload: {
65746581
title: 'Your approval request breached its SLA',
6575-
body: `${raw.object_name}/${raw.record_id}: escalation action '${action}' was taken.`,
6582+
body: `${subject}: escalation action '${action}' was taken.`,
65766583
actionUrl: '/system/approvals',
65776584
},
65786585
});

0 commit comments

Comments
 (0)