fix(plugin-approvals): name the record by object label and id in every approval notification body

.changeset/23978-approval-notification-subject-name.md

@@ -0,0 +1,11 @@
+---
+"@objectstack/plugin-approvals": patch
+---
+
+Clause-②: no
+
+fix(plugin-approvals): every approval notification body names its record by the object's label and the record id
+
+- **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`.
+- **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.
+- **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

@@ -37,6 +37,18 @@
  * `GET /api/v1/notifications` through the real messaging service — is the
  * dogfood pin `packages/qa/dogfood/test/approval-notification-body.dogfood.test.ts`.
  *
+ * ## And how it names the record (#23978)
+ *
+ * Ruling B of #23556 / #23594 names the record an approval notification is
+ * about by its object's label and its id (`Deal D_123`), with no title, through
+ * `nameSubjectRecord`. `approval.requested` used it; every other body that
+ * names the record built `<object_name>/<record_id>`, so an approver read the
+ * API name and a slash in the reminder, the SLA breach, the escalation, the
+ * hand-off, both send-back sentences and both out-of-office sentences. The same
+ * battery pins the record's name in each of those bodies, and a control: a
+ * record of an object whose schema has no label is named by the object's API
+ * name and the id, the helper's own fallback.
+ *
  * ## And whose notification it is (#23284)
  *
  * The same battery pins the organization every call site hands messaging: the
@@ -102,6 +114,14 @@ const deal = {
     title: { name: 'title', label: 'Title', type: 'text' as const },
   },
 };
+/** [#23978] The control: an object whose schema declares no label. */
+const memo = {
+  name: 'crm_memo',
+  fields: {
+    id: { name: 'id', label: 'Id', type: 'text' as const, primaryKey: true },
+    title: { name: 'title', label: 'Title', type: 'text' as const },
+  },
+};
 
 /** The flow the requests belong to: an approval node with a revise window. */
 const FLOW = {
@@ -142,10 +162,10 @@ describe('every approval notification carries its text in body', () => {
   const emits: Emitted[] = [];
   const ids: Record<string, string> = {};
 
-  const open = async (key: string, approvers: any[], extra: Record<string, unknown> = {}) => {
-    await engine.insert('crm_deal', { id: `D_${key}`, title: key, organization_id: orgOfDeal(key) }, { context: SYSTEM } as any);
+  const open = async (key: string, approvers: any[], extra: Record<string, unknown> = {}, object: string = deal.name) => {
+    await engine.insert(object, { id: `D_${key}`, title: key, organization_id: orgOfDeal(key) }, { context: SYSTEM } as any);
     const row: any = await svc.openNodeRequest({
-      object: 'crm_deal', recordId: `D_${key}`, runId: `run_${key}`, nodeId: 'review', flowName: FLOW.name,
+      object, recordId: `D_${key}`, runId: `run_${key}`, nodeId: 'review', flowName: FLOW.name,
       config: { approvers, behavior: 'first_response', ...extra } as any,
       submitterId: 'u_sub', record: { id: `D_${key}`, title: key },
     }, asUser('u_sub'));
@@ -167,7 +187,7 @@ describe('every approval notification carries its text in body', () => {
       client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true,
     }), true);
     await engine.init();
-    for (const def of [sysUser, deal, ...(await packageObjects())]) {
+    for (const def of [sysUser, deal, memo, ...(await packageObjects())]) {
       engine.registry.registerObject(def as any, 'approvals-test', 'approvals-test');
     }
     await engine.syncSchemas();
@@ -202,6 +222,13 @@ describe('every approval notification carries its text in body', () => {
     await svc.sendBack(ids.revise, { actorId: 'u_alice', comment: SEND_BACK } as any, asUser('u_alice'));
     await open('budget', [{ type: 'user', value: 'u_alice' }], { maxRevisions: 0 });
     await svc.sendBack(ids.budget, { actorId: 'u_alice' } as any, asUser('u_alice'));
+    // [#23978] Within budget and with no note, the body is the service's own sentence.
+    await open('rework', [{ type: 'user', value: 'u_alice' }]);
+    await svc.sendBack(ids.rework, { actorId: 'u_alice' } as any, asUser('u_alice'));
+
+    // ── [#23978] The control: a reminder about a record of an object with no label ──
+    await open('bare', [{ type: 'user', value: 'u_alice' }], {}, memo.name);
+    await svc.remind(ids.bare, { actorId: 'u_sub' }, asUser('u_sub'));
 
     // ── Out of office: the routing substitutes a delegate ──
     await engine.insert('sys_approval_delegation', {
@@ -235,12 +262,16 @@ describe('every approval notification carries its text in body', () => {
       'approval.requested@budget',
       'approval.requested@sla_notify',
       'approval.requested@sla_reassign',
+      'approval.requested@rework',
+      'approval.requested@bare',
       'approval.ooo_substituted@ooo',
       'approval.ooo_skipped@ooo',
       'approval.returned@budget',
       'approval.returned@revise',
+      'approval.returned@rework',
       'approval.reassigned@thread',
       'approval.reminder@thread+links',
+      'approval.reminder@bare+links',
       'approval.request_info@thread',
       'approval.comment@thread',
       'approval.escalated@sla_reassign',
@@ -263,6 +294,39 @@ describe('every approval notification carries its text in body', () => {
     expect([sentBack.audience, sentBack.payload?.body]).toEqual([['u_sub'], SEND_BACK]);
   });
 
+  it('[#23978] every body that names the record names it by its object label and its id, as approval.requested does', () => {
+    // Ruling B of #23556 / #23594: label and id, no title — one form for every
+    // body, the one `approval.requested` already sends. The pin is the record's
+    // name in each body, not the rest of the sentence around it.
+    const NAMED: Record<string, string> = {
+      'approval.requested@thread': 'Deal D_thread',
+      'approval.reminder@thread+links': 'Deal D_thread',
+      'approval.sla_breached@sla_notify': 'Deal D_sla_notify',
+      'approval.sla_breached@sla_notify>submitter': 'Deal D_sla_notify',
+      'approval.escalated@sla_reassign': 'Deal D_sla_reassign',
+      'approval.reassigned@thread': 'Deal D_thread',
+      'approval.returned@budget': 'Deal D_budget',
+      'approval.returned@rework': 'Deal D_rework',
+      'approval.ooo_substituted@ooo': 'Deal D_ooo',
+      'approval.ooo_skipped@ooo': 'Deal D_ooo',
+      // The control: the object's schema has no label, so the record is named
+      // by the object's API name and the id.
+      'approval.reminder@bare+links': 'crm_memo D_bare',
+    };
+    const offenders = Object.entries(NAMED).flatMap(([site, name]) => {
+      const bodies = emits.filter((e) => siteOf(e) === site).map((e) => String(e.payload?.body));
+      if (!bodies.length) return [`${site}: sent nothing`];
+      return bodies.filter((body) => !body.includes(name)).map((body) => `${site}: ${body}`);
+    });
+    expect(offenders).toEqual([]);
+
+    // And no body anywhere in the battery joins the API name and the id with a slash.
+    const slashed = emits
+      .map((e) => `${siteOf(e)}: ${String(e.payload?.body)}`)
+      .filter((line) => /\bcrm_(deal|memo)\/D_/.test(line));
+    expect(slashed).toEqual([]);
+  });
+
   it('every notification carries a non-empty body, and no field the payload does not declare', () => {
     const offenders: string[] = [];
     for (const e of emits) {

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

@@ -3603,7 +3603,8 @@ export class ApprovalService implements IApprovalService {
     // OOO substitution audit + notification (#1322 M4). Each hop that rerouted
     // an approver away from an out-of-office user is recorded on the request's
     // audit trail (a system action, no human actor) and notified to both the
-    // delegate — who now owns the slot — and the skipped approver.
+    // delegate — who now owns the slot — and the skipped approver. [#23978]
+    // Both bodies name the record the way every approval notification does.
     for (const sub of substitutions) {
       await this.engine.insert('sys_approval_action', await this.actionRow(row, {
         id: uid('aact'), request_id: id, organization_id: requestOrg,
@@ -3620,7 +3621,7 @@ export class ApprovalService implements IApprovalService {
         dedupKey: `approval-ooo-${id}-${sub.to}`,
         payload: {
           title: 'Approval routed to you (out-of-office cover)',
-          body: `You are covering an approval on ${input.object}/${input.recordId} while ${sub.from} is out of office.`,
+          body: `You are covering an approval on ${this.nameSubjectRecord(input.object, input.recordId)} while ${sub.from} is out of office.`,
           actionUrl: '/system/approvals',
         },
       });
@@ -3632,7 +3633,7 @@ export class ApprovalService implements IApprovalService {
         dedupKey: `approval-ooo-skip-${id}-${sub.from}`,
         payload: {
           title: 'Approval routed to your delegate',
-          body: `An approval on ${input.object}/${input.recordId} was routed to ${sub.to} while you are out of office.`,
+          body: `An approval on ${this.nameSubjectRecord(input.object, input.recordId)} was routed to ${sub.to} while you are out of office.`,
           actionUrl: '/system/approvals',
         },
       });
@@ -4747,6 +4748,8 @@ export class ApprovalService implements IApprovalService {
 
     const now = this.clock.now().toISOString();
     const maxRevisions = typeof (config as any).maxRevisions === 'number' ? (config as any).maxRevisions : 3;
+    // [#23978] Both sentences below name the record the way every approval notification does.
+    const subject = this.nameSubjectRecord(raw.object_name, raw.record_id);
     let priorSendBacks = 0;
     if (runId && nodeId) {
       const siblings = await this.engine.find('sys_approval_request', {
@@ -4806,7 +4809,7 @@ export class ApprovalService implements IApprovalService {
           source: { object: 'sys_approval_request', id: requestId },
           payload: {
             title: 'Approval auto-rejected',
-            body: `Your ${raw.object_name}/${raw.record_id} exceeded the revision limit (${maxRevisions}) and was rejected.`,
+            body: `Your ${subject} exceeded the revision limit (${maxRevisions}) and was rejected.`,
             actionUrl: '/system/approvals',
           },
         });
@@ -4850,7 +4853,7 @@ export class ApprovalService implements IApprovalService {
         source: { object: 'sys_approval_request', id: requestId },
         payload: {
           title: 'Sent back for revision',
-          body: input.comment?.trim() || `Your ${raw.object_name}/${raw.record_id} needs rework before it can be approved.`,
+          body: input.comment?.trim() || `Your ${subject} needs rework before it can be approved.`,
           actionUrl: '/system/approvals',
         },
       });
@@ -5112,7 +5115,7 @@ export class ApprovalService implements IApprovalService {
       dedupKey: `approval-reassign-${requestId}-${to}`,
       payload: {
         title: 'Approval handed to you',
-        body: `You are now an approver on ${raw.object_name}/${raw.record_id}.`,
+        body: `You are now an approver on ${this.nameSubjectRecord(raw.object_name, raw.record_id)}.`,
         actionUrl: '/system/approvals',
       },
     });
@@ -5157,6 +5160,8 @@ export class ApprovalService implements IApprovalService {
     // Per-approver fan-out: concrete identities (user ids / emails) each get
     // their OWN one-tap approve/reject links (ADR-0043); `position:*`-style
     // literals can't carry a personal token and fall back to a plain nudge.
+    // [#23978] One name for every recipient, the form `approval.requested` uses.
+    const subject = this.nameSubjectRecord(raw.object_name, raw.record_id);
     let notified = 0;
     const concrete = pending.filter(a => a && !a.includes(':'));
     const literals = pending.filter(a => a && a.includes(':'));
@@ -5172,7 +5177,7 @@ export class ApprovalService implements IApprovalService {
           dedupKey: `approval-remind-${requestId}-${nowIso}-${approver}`,
           payload: {
             title: 'Approval reminder',
-            body: `A decision on ${raw.object_name}/${raw.record_id} is still waiting on you.`,
+            body: `A decision on ${subject} is still waiting on you.`,
             actionUrl: '/system/approvals',
             actions: [
               { label: 'Approve', url: this.actionLinkUrl(tokens.approve) },
@@ -5196,7 +5201,7 @@ export class ApprovalService implements IApprovalService {
         dedupKey: `approval-remind-${requestId}-${nowIso}`,
         payload: {
           title: 'Approval reminder',
-          body: `A decision on ${raw.object_name}/${raw.record_id} is still waiting on you.`,
+          body: `A decision on ${subject} is still waiting on you.`,
           actionUrl: '/system/approvals',
         },
       });
@@ -6499,6 +6504,8 @@ export class ApprovalService implements IApprovalService {
       typeof esc.escalateTo === 'string' && esc.escalateTo.trim() ? esc.escalateTo.trim() : undefined;
     const now = this.clock.now().toISOString();
     const pending = csvSplit(raw.pending_approvers);
+    // [#23978] Every escalation sentence names the record the way every approval notification does.
+    const subject = this.nameSubjectRecord(raw.object_name, raw.record_id);
 
     // `escalateTo` is a position machine name or a user id (same contract as
     // the `position` ApproverType, ADR-0090 D3). Position holders win; an
@@ -6539,7 +6546,7 @@ export class ApprovalService implements IApprovalService {
         source: { object: 'sys_approval_request', id: raw.id },
         payload: {
           title: 'Approval escalated to you',
-          body: `An overdue approval on ${raw.object_name}/${raw.record_id} was escalated to you.`,
+          body: `An overdue approval on ${subject} was escalated to you.`,
           actionUrl: '/system/approvals',
         },
       });
@@ -6558,7 +6565,7 @@ export class ApprovalService implements IApprovalService {
         source: { object: 'sys_approval_request', id: raw.id },
         payload: {
           title: 'Approval SLA breached',
-          body: `A decision on ${raw.object_name}/${raw.record_id} is overdue.`,
+          body: `A decision on ${subject} is overdue.`,
           actionUrl: '/system/approvals',
         },
       });
@@ -6572,7 +6579,7 @@ export class ApprovalService implements IApprovalService {
         source: { object: 'sys_approval_request', id: raw.id },
         payload: {
           title: 'Your approval request breached its SLA',
-          body: `${raw.object_name}/${raw.record_id}: escalation action '${action}' was taken.`,
+          body: `${subject}: escalation action '${action}' was taken.`,
           actionUrl: '/system/approvals',
         },
       });