Skip to content

Commit a6d0715

Browse files
fix(plugin-approvals): approval.requested names the record by label and title; the empty-slot warning states the slot, not the request's fate (#23592)
Part of #23556 Clause-②: no Both texts the card names are server-authored, and both now say what is true. Scope is the card's two "expected" lines, per triage. ## 1. `approval.requested` names the record by its object label and id **Before:** `A decision on crm_opportunity/JCqLLy5dSug1MldB is waiting on you.` **After:** `A decision on Opportunity JCqLLy5dSug1MldB is waiting on you.` The object's API name appears only when its schema declares no label, as in the inbox. The body carries no title, from any source: not the live record's and not the submit snapshot's. It stays that way until #23594 narrows the inbox's subject-title read (`resolveSubjectDisplays`, the one fix point) by each reader's field-level security, because notification text is stored and may be e-mailed. `nameSubjectRecord` reads only the label, through `objectLabelOf`, which `resolveSubjectDisplays` also uses; `enrichRows` behaves exactly as before. ## 2. The per-slot warning states the slot, not the request's fate **Before:** `approver 'position:sales_manager' expanded to nobody — the slot routes to no one and the request cannot advance until someone is added or the approver is re-pointed`, followed at once by the node-level line that opens the request on the organization's administrators. **After:** `approver 'position:sales_manager' expanded to nobody — the slot names no one, so no one is asked through it. Staff it or re-point the approver; a slate left naming no one is decided by the node's onEmptyApprovers.` This is the card's second shape: the consequence is left to the node-level line. The resolver sees one approver entry, not the slate. Whether the request opens on the slate's other people, or the node's `onEmptyApprovers` decides an empty slate, is known only to `openNodeRequest`, which already logs that decision on every branch: `admin_rescue` with and without administrators, `fallback`, `auto_approve`, and `fail`, which throws. So the routing is stated once, by that line. The old ending was false on the default path. It was also false on a mixed `first_response` slate, where the request opens on the other people. ## Premise check, measured on `origin/main` at `fa6a6085c0` - Both producer lines were where the card says (`approval-service.ts:3598` and `:2216`-`:2219`). Each was pinned by the test the card names. - The dispatch assumed the list resolves `record_title` through a lookup that honours the target's declared exposure. **It does not.** The exposure check from `896a4342e6` (`servesPayloadDisplayTarget`) applies only to lookup targets named inside the payload snapshot (`payload_display`). The subject record's own title is read under the system context, with no exposure check and no field-level narrowing. The body matches that resolution exactly, so a recipient is shown nothing their own inbox row does not already show them. The gap in the list itself is in Acceptance notes. ## Pins (moved, not added beside) - `approval-requested-notification.integration.test.ts` (real `ObjectQL` over `SqlDriver`): - the moved body pin now reads `A decision on Deal "ladder" is waiting on you.`; - a parity check: the body equals the recipient's own `listRequests` row's `object_label` and `record_title`; - a new negative case: a live record with no title, whose submit snapshot still carries one, is named `Deal D_untitled` and the snapshot title is absent. - `approval-service.test.ts` (the moved WARN pin, four graph types): the line names the slot and `onEmptyApprovers`, and does not match `cannot advance`. - `admin-rescue-named-slate.integration.test.ts`: the card's acceptance line at its real scene, an `admin_rescue` node whose organization has two administrators. The per-slot line and the routing line each fire once, and no line matches `cannot advance`. This file is outside the claim's declared file surface. It is the only fixture with an organization administrator, and building one elsewhere would have duplicated it. ## Ablations (one-time, run at `452d891d16`, restore proven by `scripts/ablation-replace.mjs`: blob equal to HEAD, `git diff HEAD` empty) | ablation | expected | observed | |---|---|---| | the old WARN text put back | red on the WARN pins | 5 failed / 371 passed: the 4 graph-type cells, and the admin_rescue case at its `cannot advance` assertion, with both scene counts green | | the body built by running `enrichRows` over the unredacted row (the tempting shortcut) | the negative snapshot case red, the parity case green | 1 failed / 9 passed: the body read `Deal "Snapshot-only title"` | ## Tests and gates, final head `8e5919cb46` - `pnpm --filter @objectstack/plugin-approvals exec vitest run --maxWorkers=2`: 74 files, 1054 tests passed. - `pnpm --filter @objectstack/plugin-approvals typecheck`: exit 0, test layer included (`check:test-typecheck: OK`, debt held). - Derived local families: `dispatch-gates --commands` named 67 for this change set; all 67 ran with exit 0. `--ran` reconciliation: 67 derived, 67 run, 0 NOT-MEASURED, 0 UNRUN. On the first pass, `check:dual-build-cjs-loads` and `check:i18n` answered 3 (PREREQUISITE NOT MET, no dist); after a workspace build both answered 0. - `origin/main` was merged once before opening, at `c980d114d3`. It brought no change under `plugin-approvals`. The approvals closure was rebuilt, `spec check:generated` passed, and the suite and gates above were rerun after it. - The full farm (Test Core, Build Core, dogfood, type-check lanes, the artifact-roster families) is CI's, through this PR's check-runs. ## Changeset `.changeset/23556-approval-request-names-record.md`, `patch` on `@objectstack/plugin-approvals`. Both are server-authored texts in a released package. No schema, translation key, export or notification field moves. ## Acceptance notes - **Measured, out of scope here, handed to the seat:** the inbox's `record_title`, and with it the 920bc12 toasts, the one-tap action page, and now this body, can carry a field the reader's field-level security hides. Probe: an approver whose readable and queryable set is `id, amount` gets the payload served without `title`. The same approver's `getRequest` and `listRequests` rows still carry `record_title: "Synthetic secret title"`, because the subject read runs under the system context. The remedy belongs in `resolveSubjectDisplays`, the one function both readers now share; the notification would then pass each recipient. The stake for this PR is that notification text is stored, so titles already sent cannot be withdrawn. - **Same family, not changed here (scope):** nine sibling approval notification bodies still name the record as `object/id`: `approval.ooo_substituted`, `approval.ooo_skipped`, the reassignment, both reminder bodies, the escalation pair, the escalation-action line and the revision-limit rejection. Each is now a one-line change through `nameSubjectRecord`. - `#22824` is the routing this text now defers to. It is not changed here. ## Seat re-scope note (seat's append, patch round 2, head `7c8b072027`) Appended by `domain:services` seat 1 (`session_017WLmd5in9RXu7AjyHR8fKQ`); the dev writes this body once and never PATCHes it, per the dev contract. Seat decision `6183426689` on #23556: the notification carries no title until #23594 lands, so this PR is `Part of #23556`, and #23556 keeps the title half (`Blocked-by: #23594`). Section 1 above is replaced with the dev's round-2 text (report `6183760676`). Where the sections below still describe round 1, round 2 supersedes them: - **Premise check:** the last two sentences ("The body matches that resolution exactly…") describe round 1. The body no longer reads the subject record at all. - **Pins:** the moved body pin reads `A decision on Deal D_ladder is waiting on you.`, and the parity check against `record_title` is dropped. The negative case is "[#23594] a record whose title resolves is still named by label and id". There the live title and the snapshot title differ, and both are asserted absent; the inbox resolving the live title is the scene control. - **Ablations:** row 2 is superseded by round 2's ablation, run at `7c8b072027`. Letting the title through gives 2 failed / 8 passed, and the restore is proven. - **Acceptance notes:** in the first bullet, "and now this body" no longer holds. - **Tests and gates:** the final head is `7c8b072027`, with the same counts: 74 files / 1054 tests, and 67 of 67 gates. --- _Generated by [Claude Code](https://claude.ai/code/session_017WLmd5in9RXu7AjyHR8fKQ)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 56b2612 commit a6d0715

5 files changed

Lines changed: 153 additions & 25 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+
fix(plugin-approvals): the `approval.requested` notification names the record by its object label and id, and the empty-slot warning no longer says the request "cannot advance"
6+
7+
Clause-②: no
8+
9+
- **The notification names the record's object by its label.** The body of `approval.requested` used to read `A decision on crm_opportunity/JCqLLy5dSug1MldB is waiting on you.`, with the object's API name. It now reads `A decision on Opportunity JCqLLy5dSug1MldB is waiting on you.`, using the label the Approvals Inbox row shows for that object. The API name is used only when the object's schema declares no label, as in the inbox. The body carries the record's id, as before, and not its title, from any source. Title, topic, action link and recipients are unchanged.
10+
- **The empty-slot warning states the slot, not the request's fate.** An approver that expands to nobody used to log `the slot routes to no one and the request cannot advance until someone is added or the approver is re-pointed`. On the default `onEmptyApprovers: 'admin_rescue'` path the next line opened the request on the organization's administrators, who then decided it. The line now reads `the slot names no one, so no one is asked through it. Staff it or re-point the approver; a slate left naming no one is decided by the node's onEmptyApprovers.` What the node does with an empty slate is stated once, by the node-level line that already follows it.
11+
12+
A patch: both are server-authored texts. No schema, translation key, export or notification field moves.

‎packages/plugins/plugin-approvals/src/admin-rescue-named-slate.integration.test.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@
3131
* on the slate for being one; another organization's administrator is not
3232
* on it;
3333
* - the request says why: a timeline note and `__resolvedFrom`;
34+
* - [#23556] the log names the empty slot once and the routing once, and no
35+
* line says the request cannot advance;
3436
* - an administrator out of office is covered by their delegate;
3537
* - CONTROLS: a staffed slate is unchanged; `fallback`, `auto_approve` and
3638
* `fail` are unchanged; an organization with no administrator still gets
@@ -260,6 +262,18 @@ describe('an empty admin_rescue slate opens on the organization\'s administrator
260262
});
261263
});
262264

265+
it('[#23556] the log names the empty slot and the routing once — no line says the request cannot advance', async () => {
266+
const before = warnings.length;
267+
const row = await open('logged', UNSTAFFED);
268+
expect([...row.pending_approvers].sort()).toEqual(['u_admin', 'u_owner']);
269+
const lines = warnings.slice(before);
270+
// The scene: the per-slot line and the node-level routing line both fired,
271+
// so the absence below is not read off an opening that logged nothing.
272+
expect(lines.filter((w) => w.includes("approver 'position:sales_manager' expanded to nobody"))).toHaveLength(1);
273+
expect(lines.filter((w) => w.includes("opening on this organization's administrators"))).toHaveLength(1);
274+
expect(lines.filter((w) => /cannot advance/.test(w))).toEqual([]);
275+
});
276+
263277
it('a fallback that resolves to no one degrades to the administrators, and says so', async () => {
264278
const row = await open('fallback_empty', UNSTAFFED, {
265279
onEmptyApprovers: 'fallback', fallbackApprovers: [{ type: 'position', value: 'treasury' }],

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

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,9 @@
2121
* approver on the slate it opened on — a named user and every holder of a
2222
* staffed position alike — through the same `notify()` ingress and payload
2323
* shape as `approval.reminder`;
24+
* - [#23556] the body names the record by its object's label and its id, and
25+
* [#23594] carries no title, even for a record whose title resolves — not
26+
* the live record's and not the submit snapshot's;
2427
* - a person the slate lists twice is one person, and a `type:value` slot
2528
* literal (an unstaffed position) names no one;
2629
* - the control: a step whose approvers resolve to nobody notifies no one,
@@ -185,12 +188,13 @@ describe('opening an approval step tells each resolved approver, once (approval.
185188

186189
// One message per person, each addressed to that person alone, in the
187190
// reminder's shape: the request as its source, the inbox deep-linked to
188-
// it, the submitter as the actor.
191+
// it, the submitter as the actor. [#23556] The body names the record by
192+
// its object's label and its id, never `crm_deal/D_ladder`.
189193
for (const e of requested(row.id)) {
190194
expect(e.audience).toHaveLength(1);
191195
expect(e.payload).toEqual({
192196
title: 'Approval requested',
193-
body: `A decision on crm_deal/D_ladder is waiting on you.`,
197+
body: `A decision on Deal D_ladder is waiting on you.`,
194198
actionUrl: `/system/approvals?request=${encodeURIComponent(row.id)}`,
195199
});
196200
expect(e.source).toEqual({ object: 'sys_approval_request', id: row.id });
@@ -199,6 +203,32 @@ describe('opening an approval step tells each resolved approver, once (approval.
199203
expect(new Set(requested(row.id).map((e) => e.dedupKey)).size).toBe(3);
200204
});
201205

206+
it('[#23594] a record whose title resolves is still named by label and id — no title reaches the body', async () => {
207+
// The inbox's subject title is not yet narrowed by its reader's field-level
208+
// security (#23594), and notification text is stored and may be e-mailed,
209+
// so the body withholds the title from EVERY source: the live record's and
210+
// the submit snapshot's (which differ here, so either leak is told apart).
211+
const LIVE_TITLE = 'Acme renewal';
212+
const SNAPSHOT_TITLE = 'Snapshot-only title';
213+
await engine.insert('crm_deal', { id: 'D_titled', title: LIVE_TITLE }, { context: SYSTEM } as any);
214+
const row: any = await svc.openNodeRequest({
215+
object: 'crm_deal', recordId: 'D_titled', runId: 'run_titled', nodeId: 'review', flowName: 'deal_review',
216+
config: { approvers: [{ type: 'user', value: 'u_alice' }], behavior: 'first_response' } as any,
217+
submitterId: 'u_sub', record: { id: 'D_titled', title: SNAPSHOT_TITLE },
218+
}, asUser('u_sub'));
219+
// The scene: both titles really are there to leak — the inbox resolves the
220+
// live one for this very recipient, and the snapshot carries its own.
221+
const [listed] = await svc.listRequests({ approverId: 'u_alice', recordId: 'D_titled' }, asUser('u_alice'));
222+
expect(listed?.record_title).toBe(LIVE_TITLE);
223+
expect(row.payload).toMatchObject({ title: SNAPSHOT_TITLE });
224+
225+
const [message] = requested(row.id);
226+
expect(message?.audience).toEqual(['u_alice']);
227+
expect(message?.payload?.body).toBe('A decision on Deal D_titled is waiting on you.');
228+
expect(String(message?.payload?.body)).not.toContain(LIVE_TITLE);
229+
expect(String(message?.payload?.body)).not.toContain(SNAPSHOT_TITLE);
230+
});
231+
202232
it('a person the slate lists under two groups is told once', async () => {
203233
const row = await open('twice', [
204234
{ type: 'user', value: 'u_bob', group: 'finance' },

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3508,7 +3508,12 @@ describe('ApprovalService — a graph approver that expands to nobody warns (#38
35083508
expect(req.pending_approvers).toEqual([`${type}:${value}`]);
35093509
const hit = warnings.find(([msg]) => String(msg).includes('expanded to nobody'));
35103510
expect(hit, `no warning for ${type}`).toBeTruthy();
3511-
expect(String(hit[0])).toContain('cannot advance until someone is added or the approver is re-pointed');
3511+
// [#23556] The line names the slot and points at the node's policy; the
3512+
// request's fate is the node-level line's to state, so this one never says
3513+
// the request "cannot advance" — false on the default admin_rescue path.
3514+
expect(String(hit[0])).toContain(`'${type}:${value}'`);
3515+
expect(String(hit[0])).toContain('onEmptyApprovers');
3516+
expect(String(hit[0])).not.toMatch(/cannot advance/);
35123517
expect(hit[1]).toMatchObject({ type, value, organizationId: 't1' });
35133518
});
35143519

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

Lines changed: 89 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -2212,9 +2212,20 @@ export class ApprovalService implements IApprovalService {
22122212
// permanently stuck approval (#3424). The fallback itself stays (a
22132213
// literal keeps 15.x slots and substring fixtures working); it just
22142214
// stops being invisible.
2215+
//
2216+
// [#23556] The line states the SLOT, never the request's fate. This
2217+
// resolver sees one approver entry, not the slate: whether the request
2218+
// opens on the slate's other people, or the slate names no one and the
2219+
// node's `onEmptyApprovers` decides (by default `admin_rescue`, which
2220+
// opens on the organization's administrators, #22824), is only known
2221+
// to `openNodeRequest`, which logs that decision itself on the line that
2222+
// follows. The old ending ("the request cannot advance until someone is
2223+
// added") restated a fate this resolver cannot know, and was false on
2224+
// the default path.
22152225
this.logger?.warn?.(
2216-
`[approvals] approver '${type}:${a.value}' expanded to nobody — the slot routes to no one `
2217-
+ `and the request cannot advance until someone is added or the approver is re-pointed`,
2226+
`[approvals] approver '${type}:${a.value}' expanded to nobody — the slot names no one, so no one is `
2227+
+ `asked through it. Staff it or re-point the approver; a slate left naming no one is decided by the `
2228+
+ `node's onEmptyApprovers.`,
22182229
{ type, value: a.value, organizationId: organizationId ?? null },
22192230
);
22202231
}
@@ -3583,8 +3594,13 @@ export class ApprovalService implements IApprovalService {
35833594
// `approval.ooo_substituted` — one message per approver, not two.
35843595
// No one-tap decision links: ADR-0043 mints those on `remind()` only, and
35853596
// minting bearer tokens on every opening is a decision this does not make.
3597+
//
3598+
// [#23556] The body names the record by its object's label and its id —
3599+
// `Deal D_123`, never `crm_deal/D_123` — so a grouped bell entry tells its
3600+
// reader which kind of record is waiting. One name for every recipient.
35863601
const toldByOoo = new Set(substitutions.map((sub) => sub.to));
35873602
const openedOn = new Set(approvers.filter((a) => a && !a.includes(':') && !toldByOoo.has(a)));
3603+
const subject = this.nameSubjectRecord(input.object, input.recordId);
35883604
for (const approver of openedOn) {
35893605
await this.notify({
35903606
topic: 'approval.requested',
@@ -3595,7 +3611,7 @@ export class ApprovalService implements IApprovalService {
35953611
dedupKey: `approval-requested-${id}-${approver}`,
35963612
payload: {
35973613
title: 'Approval requested',
3598-
body: `A decision on ${input.object}/${input.recordId} is waiting on you.`,
3614+
body: `A decision on ${subject} is waiting on you.`,
35993615
actionUrl: '/system/approvals',
36003616
},
36013617
});
@@ -6680,6 +6696,75 @@ export class ApprovalService implements IApprovalService {
66806696
} catch { return {}; }
66816697
}
66826698

6699+
/**
6700+
* The subject records' titles and their objects' labels, keyed
6701+
* `"<object> <id>"` and by object name: the LIVE half of how the approvals
6702+
* list names a request's record ({@link ApprovalService.enrichRows} sets
6703+
* `record_title` and `object_label` from it, then falls back to the reader's
6704+
* redacted payload snapshot for a title this read did not find).
6705+
*
6706+
* Batched: one read per object. Best-effort — an unregistered object or a
6707+
* failed read leaves its entries unset.
6708+
*
6709+
* [#23594] The read runs under the system context, so no reader's
6710+
* field-level security narrows the title it finds. Narrowing it per reader
6711+
* is a change to THIS function — the one place the inbox's subject title is
6712+
* resolved. The `approval.requested` body ({@link ApprovalService.nameSubjectRecord})
6713+
* withholds the title until then, because notification text is stored and
6714+
* may be e-mailed, so a title sent cannot be withdrawn by a later fix.
6715+
*/
6716+
private async resolveSubjectDisplays(
6717+
byObject: ReadonlyMap<string, ReadonlySet<string>>,
6718+
): Promise<{ titles: Map<string, string>; objectLabels: Map<string, string> }> {
6719+
const titles = new Map<string, string>();
6720+
const objectLabels = new Map<string, string>();
6721+
for (const [object, idSet] of byObject) {
6722+
const label = this.objectLabelOf(object);
6723+
if (label) objectLabels.set(object, label);
6724+
const ids = Array.from(idSet);
6725+
const displayField = this.resolveDisplayField(object);
6726+
try {
6727+
const recs = await this.engine.find(object, {
6728+
where: { id: { $in: ids } }, limit: ids.length, context: SYSTEM_CTX,
6729+
});
6730+
for (const rec of (recs ?? []) as any[]) {
6731+
const title = ApprovalService.pickTitle(rec, displayField);
6732+
if (rec?.id && title) titles.set(`${object} ${rec.id}`, title);
6733+
}
6734+
} catch { /* object may be unregistered — the caller's fallback applies */ }
6735+
}
6736+
return { titles, objectLabels };
6737+
}
6738+
6739+
/**
6740+
* An object's schema label, or `undefined` — the one spelling of "the
6741+
* object's label" the inbox row ({@link ApprovalService.resolveSubjectDisplays})
6742+
* and the `approval.requested` body ({@link ApprovalService.nameSubjectRecord})
6743+
* share. A schema that cannot be read has no label (the label is optional).
6744+
*/
6745+
private objectLabelOf(object: string): string | undefined {
6746+
try {
6747+
const schema: any = (this.engine as any).getSchema?.(object);
6748+
return schema?.label ? String(schema.label) : undefined;
6749+
} catch { return undefined; }
6750+
}
6751+
6752+
/**
6753+
* [#23556] The record an approval notification is about: the object's label
6754+
* (its API name only when the schema has none, as the inbox falls back),
6755+
* then the record id — `Deal D_123`. The body carried the id before; what
6756+
* changed is the API name giving way to the label.
6757+
*
6758+
* ⛔ No title, from any source — neither the live record nor the payload
6759+
* snapshot (which the inbox serves only after redacting it for its reader,
6760+
* #10749). Notification text is stored and may be e-mailed, and it has no
6761+
* reader to narrow for.
6762+
*/
6763+
private nameSubjectRecord(object: string, recordId: string): string {
6764+
// [#23594] The title would go here; withheld until resolveSubjectDisplays narrows it by each reader's FLS.
6765+
return `${this.objectLabelOf(object) ?? object} ${recordId}`;
6766+
}
6767+
66836768
/**
66846769
* Attach inbox display fields to rows so clients never render a raw
66856770
* identifier: `record_title`, `submitter_name`, `object_label`,
@@ -6703,25 +6788,7 @@ export class ApprovalService implements IApprovalService {
67036788
if (!set) { set = new Set(); byObject.set(r.object_name, set); }
67046789
set.add(r.record_id);
67056790
}
6706-
const titles = new Map<string, string>();
6707-
const objectLabels = new Map<string, string>();
6708-
for (const [object, idSet] of byObject) {
6709-
try {
6710-
const schema: any = (this.engine as any).getSchema?.(object);
6711-
if (schema?.label) objectLabels.set(object, String(schema.label));
6712-
} catch { /* label optional */ }
6713-
const ids = Array.from(idSet);
6714-
const displayField = this.resolveDisplayField(object);
6715-
try {
6716-
const recs = await this.engine.find(object, {
6717-
where: { id: { $in: ids } }, limit: ids.length, context: SYSTEM_CTX,
6718-
});
6719-
for (const rec of (recs ?? []) as any[]) {
6720-
const title = ApprovalService.pickTitle(rec, displayField);
6721-
if (rec?.id && title) titles.set(`${object} ${rec.id}`, title);
6722-
}
6723-
} catch { /* object may be unregistered — payload fallback below */ }
6724-
}
6791+
const { titles, objectLabels } = await this.resolveSubjectDisplays(byObject);
67256792

67266793
// Lookup foreign keys inside payload snapshots → referenced record titles.
67276794
const lookupFieldsByObject = new Map<string, Array<{ key: string; reference: string }>>();

0 commit comments

Comments
 (0)