diff --git a/.changeset/24065-ooo-notification-person-names.md b/.changeset/24065-ooo-notification-person-names.md new file mode 100644 index 0000000000..d3c24cfa6b --- /dev/null +++ b/.changeset/24065-ooo-notification-person-names.md @@ -0,0 +1,9 @@ +--- +'@objectstack/plugin-approvals': patch +--- + +Clause-②: no + +The two out-of-office approval notifications name people by their display name instead of their raw user id. The delegate's `approval.ooo_substituted` body used to read `You are covering an approval on Deal D_123 while u_ooo is out of office.`, and now names the absent approver: `… while Olive Away is out of office.` The absent approver's `approval.ooo_skipped` body used to read `… was routed to u_bob while you are out of office.`, and now names the delegate: `… was routed to Bob …`. + +The names come from the same user-name lookup the approvals inbox uses for pending approvers and the timeline, read once per opening for every person the out-of-office routing touched. An account row with no name is named by its email, as the inbox names it. A person with no readable name (no account row for the id, or a failed read) is still named by the user id, as before. The `ooo_substitute` entry on the request's audit trail is unchanged and still records the two user ids (`u_ooo → u_bob — reason`). 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 ac93c487dd..841a040314 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 @@ -49,6 +49,16 @@ * 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 whom the out-of-office bodies name (#24065) + * + * The two out-of-office bodies also name a person: the delegate is told whom + * they cover, and the absent approver is told who covers for them. Both used to + * interpolate the raw `sys_user` id (`… while u_ooo is out of office.`). They + * now name each person through `resolveUserNames`, the chain the approvals door + * names people with. The battery pins each person's name in each body, and a + * control: an approver id no account row answers to is named by that id, the + * way every reader of the chain renders an unresolved address. + * * ## And whose notification it is (#23284) * * The same battery pins the organization every call site hands messaging: the @@ -80,6 +90,13 @@ const USERS = [ { id: 'u_boss', name: 'Boss', email: 'boss@example.com' }, ]; +/** + * [#24065] An approver id no `sys_user` row answers to, such as an account + * removed after its out-of-office delegation was written. Deliberately not in + * {@link USERS}. + */ +const GONE = 'u_gone'; + /** The text each person writes — distinct, so no one can pass for another. */ const COMMENT = 'The signed contract is attached to the record now.'; const QUESTION = 'Which cost centre should this be booked against?'; @@ -238,6 +255,15 @@ describe('every approval notification carries its text in body', () => { }, { context: SYSTEM } as any); const ooo = await open('ooo', [{ type: 'user', value: 'u_ooo' }]); expect(ooo.pending_approvers).toEqual(['u_bob']); + // [#24065] The control: the absent approver names no account, so no name + // can be read for them; the delegate the same hop routes to does. + await engine.insert('sys_approval_delegation', { + id: 'dlg_2', delegator_id: GONE, delegate_id: 'u_alice', reason: 'leave', + valid_from: '2026-09-01T00:00:00.000Z', valid_until: '2026-12-31T00:00:00.000Z', + created_at: '2026-09-01T00:00:00.000Z', updated_at: '2026-09-01T00:00:00.000Z', + }, { context: SYSTEM } as any); + const oooGone = await open('ooo_gone', [{ type: 'user', value: GONE }]); + expect(oooGone.pending_approvers).toEqual(['u_alice']); // ── The SLA sweep: a notify (submitter told) and a reassign (submitter not) ── await open('sla_notify', [{ type: 'user', value: 'u_alice' }], @@ -255,8 +281,8 @@ describe('every approval notification carries its text in body', () => { it('the battery reaches every call site that can notify anyone — twelve of the thirteen', () => { expect(new Set(emits.map(siteOf))).toEqual(new Set([ // The opening tells each concrete approver on every request the battery - // opens — all but `ooo`, whose one approver is the delegate, told once - // by `approval.ooo_substituted` instead. + // opens — all but `ooo` and `ooo_gone`, whose one approver is the + // delegate, told once by `approval.ooo_substituted` instead. 'approval.requested@thread', 'approval.requested@revise', 'approval.requested@budget', @@ -266,6 +292,8 @@ describe('every approval notification carries its text in body', () => { 'approval.requested@bare', 'approval.ooo_substituted@ooo', 'approval.ooo_skipped@ooo', + 'approval.ooo_substituted@ooo_gone', + 'approval.ooo_skipped@ooo_gone', 'approval.returned@budget', 'approval.returned@revise', 'approval.returned@rework', @@ -327,6 +355,44 @@ describe('every approval notification carries its text in body', () => { expect(slashed).toEqual([]); }); + /** [#24065] Every body one call site sent. */ + const bodiesAt = (site: string): string[] => + emits.filter((e) => siteOf(e) === site).map((e) => String(e.payload?.body)); + + it('[#24065] both out-of-office bodies name the absent approver and the delegate by display name, never by user id', () => { + // The pin is each person's name in each body, not the sentence around it. + const NAMED: Record = { + // The delegate is told whom they cover … + 'approval.ooo_substituted@ooo': { name: 'Olive Away', id: 'u_ooo' }, + // … and the absent approver who covers for them. + 'approval.ooo_skipped@ooo': { name: 'Bob', id: 'u_bob' }, + // The control's resolvable half: the delegate on the same hop as an + // approver no account answers to. + 'approval.ooo_skipped@ooo_gone': { name: 'Alice', id: 'u_alice' }, + }; + const offenders = Object.entries(NAMED).flatMap(([site, { name, id }]) => { + const bodies = bodiesAt(site); + if (!bodies.length) return [`${site}: sent nothing`]; + return bodies.filter((body) => !body.includes(name) || body.includes(id)).map((body) => `${site}: ${body}`); + }); + expect(offenders).toEqual([]); + + // And no out-of-office body anywhere in the battery names one of its + // people by their user id. + const byId = emits + .filter((e) => e.topic === 'approval.ooo_substituted' || e.topic === 'approval.ooo_skipped') + .flatMap((e) => USERS.filter((u) => String(e.payload?.body).includes(u.id)) + .map((u) => `${siteOf(e)}: names ${u.id} in ${String(e.payload?.body)}`)); + expect(byId).toEqual([]); + }); + + it('[#24065] a person no name can be read for is named by their stored id, as every reader of the chain renders an unresolved address', () => { + // `u_gone` names no `sys_user` row, so the chain answers no name for it. + // The body still names the person, by the id the delegation stores: never + // an empty slot, never `undefined`. + expect(bodiesAt('approval.ooo_substituted@ooo_gone')).toEqual([expect.stringContaining(GONE)]); + }); + 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 c01b0f0749..05d5b39c94 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -3605,6 +3605,22 @@ export class ApprovalService implements IApprovalService { // audit trail (a system action, no human actor) and notified to both the // delegate — who now owns the slot — and the skipped approver. [#23978] // Both bodies name the record the way every approval notification does. + // + // [#24065] And each body names the person by display name, through the + // chain the approvals door names people with (`resolveUserNames`): + // both people of every hop in ONE read for the whole fan-out, and no read + // at all when nothing was substituted. A person the chain answers no name + // for (no account row, or the read failed) is named by the stored id, the + // way every reader of that chain renders an unresolved address. A body is + // a STORED copy of the name (messaging stores it, the inbox serves it), + // like the stored labels the `resolveUserNames` docblock lists. + // + // The audit `comment` keeps the ids by design: it is the request's audit + // trail of identities, served as written by the timeline and the action + // lists. Display names belong in read-time `*_name` companions (the + // `reassign` hand-off's shape), not frozen into an audit row. + const oooNames = await this.resolveUserNames(substitutions.flatMap((sub) => [sub.from, sub.to])); + const oooPerson = (userId: string): string => oooNames.get(userId) || userId; 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, @@ -3621,7 +3637,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 ${this.nameSubjectRecord(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 ${oooPerson(sub.from)} is out of office.`, actionUrl: '/system/approvals', }, }); @@ -3633,7 +3649,7 @@ export class ApprovalService implements IApprovalService { dedupKey: `approval-ooo-skip-${id}-${sub.from}`, payload: { title: 'Approval routed to your delegate', - body: `An approval on ${this.nameSubjectRecord(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 ${oooPerson(sub.to)} while you are out of office.`, actionUrl: '/system/approvals', }, });