diff --git a/.changeset/23978-approval-notification-subject-name.md b/.changeset/23978-approval-notification-subject-name.md new file mode 100644 index 00000000000..adad29dd302 --- /dev/null +++ b/.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 `/`, 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. diff --git a/packages/plugins/plugin-approvals/src/approval-notification-body.integration.test.ts b/packages/plugins/plugin-approvals/src/approval-notification-body.integration.test.ts index 5fa0a6c0df4..ac93c487ddc 100644 --- a/packages/plugins/plugin-approvals/src/approval-notification-body.integration.test.ts +++ b/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 `/`, 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 = {}; - const open = async (key: string, approvers: any[], extra: Record = {}) => { - 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 = {}, 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 = { + '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) { diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index f08a7e74dbf..c01b0f0749d 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/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', }, });