fix(plugin-approvals): approval.requested names the record by label and title; the empty-slot warning states the slot, not the request's fate

.changeset/23556-approval-request-names-record.md

@@ -0,0 +1,12 @@
+---
+'@objectstack/plugin-approvals': patch
+---
+
+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"
+
+Clause-②: no
+
+- **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.
+- **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.
+
+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

@@ -31,6 +31,8 @@
  *   on the slate for being one; another organization's administrator is not
  *   on it;
  * - the request says why: a timeline note and `__resolvedFrom`;
+ * - [#23556] the log names the empty slot once and the routing once, and no
+ *   line says the request cannot advance;
  * - an administrator out of office is covered by their delegate;
  * - CONTROLS: a staffed slate is unchanged; `fallback`, `auto_approve` and
  *   `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
     });
   });
 
+  it('[#23556] the log names the empty slot and the routing once — no line says the request cannot advance', async () => {
+    const before = warnings.length;
+    const row = await open('logged', UNSTAFFED);
+    expect([...row.pending_approvers].sort()).toEqual(['u_admin', 'u_owner']);
+    const lines = warnings.slice(before);
+    // The scene: the per-slot line and the node-level routing line both fired,
+    // so the absence below is not read off an opening that logged nothing.
+    expect(lines.filter((w) => w.includes("approver 'position:sales_manager' expanded to nobody"))).toHaveLength(1);
+    expect(lines.filter((w) => w.includes("opening on this organization's administrators"))).toHaveLength(1);
+    expect(lines.filter((w) => /cannot advance/.test(w))).toEqual([]);
+  });
+
   it('a fallback that resolves to no one degrades to the administrators, and says so', async () => {
     const row = await open('fallback_empty', UNSTAFFED, {
       onEmptyApprovers: 'fallback', fallbackApprovers: [{ type: 'position', value: 'treasury' }],

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

@@ -21,6 +21,9 @@
  *   approver on the slate it opened on — a named user and every holder of a
  *   staffed position alike — through the same `notify()` ingress and payload
  *   shape as `approval.reminder`;
+ * - [#23556] the body names the record by its object's label and its id, and
+ *   [#23594] carries no title, even for a record whose title resolves — not
+ *   the live record's and not the submit snapshot's;
  * - a person the slate lists twice is one person, and a `type:value` slot
  *   literal (an unstaffed position) names no one;
  * - 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.
 
     // One message per person, each addressed to that person alone, in the
     // reminder's shape: the request as its source, the inbox deep-linked to
-    // it, the submitter as the actor.
+    // it, the submitter as the actor. [#23556] The body names the record by
+    // its object's label and its id, never `crm_deal/D_ladder`.
     for (const e of requested(row.id)) {
       expect(e.audience).toHaveLength(1);
       expect(e.payload).toEqual({
         title: 'Approval requested',
-        body: `A decision on crm_deal/D_ladder is waiting on you.`,
+        body: `A decision on Deal D_ladder is waiting on you.`,
         actionUrl: `/system/approvals?request=${encodeURIComponent(row.id)}`,
       });
       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.
     expect(new Set(requested(row.id).map((e) => e.dedupKey)).size).toBe(3);
   });
 
+  it('[#23594] a record whose title resolves is still named by label and id — no title reaches the body', async () => {
+    // The inbox's subject title is not yet narrowed by its reader's field-level
+    // security (#23594), and notification text is stored and may be e-mailed,
+    // so the body withholds the title from EVERY source: the live record's and
+    // the submit snapshot's (which differ here, so either leak is told apart).
+    const LIVE_TITLE = 'Acme renewal';
+    const SNAPSHOT_TITLE = 'Snapshot-only title';
+    await engine.insert('crm_deal', { id: 'D_titled', title: LIVE_TITLE }, { context: SYSTEM } as any);
+    const row: any = await svc.openNodeRequest({
+      object: 'crm_deal', recordId: 'D_titled', runId: 'run_titled', nodeId: 'review', flowName: 'deal_review',
+      config: { approvers: [{ type: 'user', value: 'u_alice' }], behavior: 'first_response' } as any,
+      submitterId: 'u_sub', record: { id: 'D_titled', title: SNAPSHOT_TITLE },
+    }, asUser('u_sub'));
+    // The scene: both titles really are there to leak — the inbox resolves the
+    // live one for this very recipient, and the snapshot carries its own.
+    const [listed] = await svc.listRequests({ approverId: 'u_alice', recordId: 'D_titled' }, asUser('u_alice'));
+    expect(listed?.record_title).toBe(LIVE_TITLE);
+    expect(row.payload).toMatchObject({ title: SNAPSHOT_TITLE });
+
+    const [message] = requested(row.id);
+    expect(message?.audience).toEqual(['u_alice']);
+    expect(message?.payload?.body).toBe('A decision on Deal D_titled is waiting on you.');
+    expect(String(message?.payload?.body)).not.toContain(LIVE_TITLE);
+    expect(String(message?.payload?.body)).not.toContain(SNAPSHOT_TITLE);
+  });
+
   it('a person the slate lists under two groups is told once', async () => {
     const row = await open('twice', [
       { type: 'user', value: 'u_bob', group: 'finance' },

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

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

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

@@ -2212,9 +2212,20 @@ export class ApprovalService implements IApprovalService {
       // permanently stuck approval (#3424). The fallback itself stays (a
       // literal keeps 15.x slots and substring fixtures working); it just
       // stops being invisible.
+      //
+      // [#23556] The line states the SLOT, never the request's fate. This
+      // resolver sees one approver entry, not the slate: whether the request
+      // opens on the slate's other people, or the slate names no one and the
+      // node's `onEmptyApprovers` decides (by default `admin_rescue`, which
+      // opens on the organization's administrators, #22824), is only known
+      // to `openNodeRequest`, which logs that decision itself on the line that
+      // follows. The old ending ("the request cannot advance until someone is
+      // added") restated a fate this resolver cannot know, and was false on
+      // the default path.
       this.logger?.warn?.(
-        `[approvals] approver '${type}:${a.value}' expanded to nobody — the slot routes to no one `
-        + `and the request cannot advance until someone is added or the approver is re-pointed`,
+        `[approvals] approver '${type}:${a.value}' 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.`,
         { type, value: a.value, organizationId: organizationId ?? null },
       );
     }
@@ -3583,8 +3594,13 @@ export class ApprovalService implements IApprovalService {
     //    `approval.ooo_substituted` — one message per approver, not two.
     // No one-tap decision links: ADR-0043 mints those on `remind()` only, and
     // minting bearer tokens on every opening is a decision this does not make.
+    //
+    // [#23556] The body names the record by its object's label and its id —
+    // `Deal D_123`, never `crm_deal/D_123` — so a grouped bell entry tells its
+    // reader which kind of record is waiting. One name for every recipient.
     const toldByOoo = new Set(substitutions.map((sub) => sub.to));
     const openedOn = new Set(approvers.filter((a) => a && !a.includes(':') && !toldByOoo.has(a)));
+    const subject = this.nameSubjectRecord(input.object, input.recordId);
     for (const approver of openedOn) {
       await this.notify({
         topic: 'approval.requested',
@@ -3595,7 +3611,7 @@ export class ApprovalService implements IApprovalService {
         dedupKey: `approval-requested-${id}-${approver}`,
         payload: {
           title: 'Approval requested',
-          body: `A decision on ${input.object}/${input.recordId} is waiting on you.`,
+          body: `A decision on ${subject} is waiting on you.`,
           actionUrl: '/system/approvals',
         },
       });
@@ -6680,6 +6696,75 @@ export class ApprovalService implements IApprovalService {
     } catch { return {}; }
   }
 
+  /**
+   * The subject records' titles and their objects' labels, keyed
+   * `"<object> <id>"` and by object name: the LIVE half of how the approvals
+   * list names a request's record ({@link ApprovalService.enrichRows} sets
+   * `record_title` and `object_label` from it, then falls back to the reader's
+   * redacted payload snapshot for a title this read did not find).
+   *
+   * Batched: one read per object. Best-effort — an unregistered object or a
+   * failed read leaves its entries unset.
+   *
+   * [#23594] The read runs under the system context, so no reader's
+   * field-level security narrows the title it finds. Narrowing it per reader
+   * is a change to THIS function — the one place the inbox's subject title is
+   * resolved. The `approval.requested` body ({@link ApprovalService.nameSubjectRecord})
+   * withholds the title until then, because notification text is stored and
+   * may be e-mailed, so a title sent cannot be withdrawn by a later fix.
+   */
+  private async resolveSubjectDisplays(
+    byObject: ReadonlyMap<string, ReadonlySet<string>>,
+  ): Promise<{ titles: Map<string, string>; objectLabels: Map<string, string> }> {
+    const titles = new Map<string, string>();
+    const objectLabels = new Map<string, string>();
+    for (const [object, idSet] of byObject) {
+      const label = this.objectLabelOf(object);
+      if (label) objectLabels.set(object, label);
+      const ids = Array.from(idSet);
+      const displayField = this.resolveDisplayField(object);
+      try {
+        const recs = await this.engine.find(object, {
+          where: { id: { $in: ids } }, limit: ids.length, context: SYSTEM_CTX,
+        });
+        for (const rec of (recs ?? []) as any[]) {
+          const title = ApprovalService.pickTitle(rec, displayField);
+          if (rec?.id && title) titles.set(`${object} ${rec.id}`, title);
+        }
+      } catch { /* object may be unregistered — the caller's fallback applies */ }
+    }
+    return { titles, objectLabels };
+  }
+
+  /**
+   * An object's schema label, or `undefined` — the one spelling of "the
+   * object's label" the inbox row ({@link ApprovalService.resolveSubjectDisplays})
+   * and the `approval.requested` body ({@link ApprovalService.nameSubjectRecord})
+   * share. A schema that cannot be read has no label (the label is optional).
+   */
+  private objectLabelOf(object: string): string | undefined {
+    try {
+      const schema: any = (this.engine as any).getSchema?.(object);
+      return schema?.label ? String(schema.label) : undefined;
+    } catch { return undefined; }
+  }
+
+  /**
+   * [#23556] The record an approval notification is about: the object's label
+   * (its API name only when the schema has none, as the inbox falls back),
+   * then the record id — `Deal D_123`. The body carried the id before; what
+   * changed is the API name giving way to the label.
+   *
+   * ⛔ No title, from any source — neither the live record nor the payload
+   * snapshot (which the inbox serves only after redacting it for its reader,
+   * #10749). Notification text is stored and may be e-mailed, and it has no
+   * reader to narrow for.
+   */
+  private nameSubjectRecord(object: string, recordId: string): string {
+    // [#23594] The title would go here; withheld until resolveSubjectDisplays narrows it by each reader's FLS.
+    return `${this.objectLabelOf(object) ?? object} ${recordId}`;
+  }
+
   /**
    * Attach inbox display fields to rows so clients never render a raw
    * identifier: `record_title`, `submitter_name`, `object_label`,
@@ -6703,25 +6788,7 @@ export class ApprovalService implements IApprovalService {
       if (!set) { set = new Set(); byObject.set(r.object_name, set); }
       set.add(r.record_id);
     }
-    const titles = new Map<string, string>();
-    const objectLabels = new Map<string, string>();
-    for (const [object, idSet] of byObject) {
-      try {
-        const schema: any = (this.engine as any).getSchema?.(object);
-        if (schema?.label) objectLabels.set(object, String(schema.label));
-      } catch { /* label optional */ }
-      const ids = Array.from(idSet);
-      const displayField = this.resolveDisplayField(object);
-      try {
-        const recs = await this.engine.find(object, {
-          where: { id: { $in: ids } }, limit: ids.length, context: SYSTEM_CTX,
-        });
-        for (const rec of (recs ?? []) as any[]) {
-          const title = ApprovalService.pickTitle(rec, displayField);
-          if (rec?.id && title) titles.set(`${object} ${rec.id}`, title);
-        }
-      } catch { /* object may be unregistered — payload fallback below */ }
-    }
+    const { titles, objectLabels } = await this.resolveSubjectDisplays(byObject);
 
     // Lookup foreign keys inside payload snapshots → referenced record titles.
     const lookupFieldsByObject = new Map<string, Array<{ key: string; reference: string }>>();