diff --git a/.changeset/24753-approval-decision-notifies-submitter.md b/.changeset/24753-approval-decision-notifies-submitter.md new file mode 100644 index 00000000000..60668d45c2e --- /dev/null +++ b/.changeset/24753-approval-decision-notifies-submitter.md @@ -0,0 +1,13 @@ +--- +"@objectstack/plugin-approvals": minor +--- + +feat(plugin-approvals): the decision that ends an approval tells its submitter the outcome (`approval.approved` / `approval.rejected`) + +Clause-②: yes (widening) + +- **What was wrong.** Approving or rejecting a request notified nobody. A send-back (`approval.returned`) and a question (`approval.request_info`) reached the submitter, but the decision itself did not, so a submitter learned that their request was approved or rejected only by opening the Approvals Inbox. +- **What is new.** When a decision ends the approval, the submitter of the request it decides gets one notification on one of two new topics: `approval.approved` (title "Request approved") or `approval.rejected` (title "Request rejected"). The body names the record by its object's label and its id, as every approval notification does (`Your Deal D_123 was rejected.`), followed by the decider's comment as its own paragraph when they wrote one. It links to that request in the Approvals Inbox and, like the other approval notices, carries as its actor the person the decision's context vouches for. It goes through the installed `messaging` service like every other approval topic, so the same channels and delivery preferences apply; a preference row can mute one outcome and keep the other. +- **When "ends the approval" holds.** A co-sign vote that leaves a `unanimous`, `quorum` or `per_group` request pending tells no one. A decision whose flow moves on to another approval step tells no one either; the decision that ends the run is the one that notifies. That is read from the run after the decision resumes it, not from the flow's graph, so when a decision node routes around a later step, the first decision is the outcome. +- **Who is not told.** A submitter who decided their own request. And an SLA escalation's `auto_approve` or `auto_reject`: the escalation's own submitter notice (`approval.sla_breached`, naming the action taken) is the one message about it, so `escalation.notifySubmitter: false` still means the submitter hears nothing. A decision whose flow run cannot be resumed reports its failure to the decider and tells the submitter nothing, as a send-back in the same state already did. +- **What to change in your flows.** If a flow ends with a `notify` node whose only job is to tell the submitter the decision, remove it, or the submitter is told twice. `notify` nodes that address anyone else are unaffected. No flow in this repository's examples has that workaround: the three showcase flows whose `notify` node reports a decision address someone other than the submitter — `showcase_invoice_signoff` and `showcase_project_closure` the record's owner, `showcase_release_signoff` a fixed address — so they are unchanged, and only a record owner who is also the person who submitted now hears both. diff --git a/content/docs/automation/approvals.mdx b/content/docs/automation/approvals.mdx index 07906e21db6..be13f9ef400 100644 --- a/content/docs/automation/approvals.mdx +++ b/content/docs/automation/approvals.mdx @@ -536,8 +536,35 @@ second one for the opening. If a flow has a `notify` node beside its approval node only to tell the approvers that a request is waiting, remove that node: those approvers are now -told twice. Keep `notify` nodes that address someone else, such as the -submitter or a watcher, or that report the decision. +told twice. Keep `notify` nodes that address someone else, such as a watcher, +or that report the decision to anyone but the submitter. + + + +**The decision tells the submitter.** When a decision ends the approval, the +submitter of the request it decides gets one notification: `approval.approved` +("Request approved") or `approval.rejected` ("Request rejected"). Its body +names the record (`Deal D_123`) and the outcome, followed by the decider's +comment when they wrote one, and it links to that request in the Approvals +Inbox. There is one topic per outcome, so a delivery preference can mute +approvals and keep rejections. + +"Ends the approval" means the process, not the step. A co-sign vote that +leaves a `unanimous`, `quorum` or `per_group` request pending tells no one, +and neither does a decision whose flow moves on to another approval step: on +a two-step flow the first step's decision is silent and the decision that ends +the run is the one that notifies. Whether a later step opened is read from the +run, not from the flow's graph, so when a decision node routes around the +later step, the first decision is the outcome and notifies. + +Two cases tell nobody by design. A submitter who decided their own request +already knows. An SLA `auto_approve` or `auto_reject` is told by the +escalation's own submitter notice (`approval.sla_breached`, which names the +action taken), so `notifySubmitter: false` still means the submitter hears +nothing about it. + +If a flow ends with a `notify` node whose only job is to tell the submitter +the decision, remove that node: the submitter is now told twice. ### Mount the approvals inbox in your app diff --git a/packages/plugins/plugin-approvals/src/approval-decision-outcome-notification.integration.test.ts b/packages/plugins/plugin-approvals/src/approval-decision-outcome-notification.integration.test.ts new file mode 100644 index 00000000000..ee3523329da --- /dev/null +++ b/packages/plugins/plugin-approvals/src/approval-decision-outcome-notification.integration.test.ts @@ -0,0 +1,342 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The decision that ends an approval tells its submitter the outcome (#24753). + * + * ## The defect + * + * A send-back told the submitter (`approval.returned`), and so did a question + * (`approval.request_info`). The decision itself told no one: approving or + * rejecting a request emitted nothing to its `submitter_id`, so a submitter + * learned the outcome only by opening the approvals inbox — measured on the + * showcase, where a rejected expense report left its submitter's bell at + * "You're all caught up". + * + * ## What this pins + * + * Driven through the real `ApprovalService` on a real `ObjectQL` over + * `SqlDriver` (better-sqlite3, in-memory, real DDL), with the real + * `AutomationEngine` running the flows and the real approval node opening each + * step, and the messaging surface recording what reaches it: + * + * - an approval sends ONE `approval.approved` to the submitter, a rejection + * ONE `approval.rejected`, in the sibling notices' shape: the record named by + * its object's label and id, the outcome, the decider's comment when they + * wrote one, the inbox deep-linked to the request, the decider as the actor; + * - a submitter who decided their own request is told nothing; + * - "final" is the process, not the step: a decision whose resume opened the + * next approval step tells no one, and the decision that ends the run does — + * measured on the run, so a gateway that SKIPS the later step makes the first + * decision the final one. A co-sign vote that leaves the request pending + * tells no one either; + * - the SLA sweep's auto-decision is told by the escalation's own submitter + * notice, under its `notifySubmitter` switch, never a second time. + * + * Every negative case carries its positive control in the same test, so a + * build that never sends the notice cannot pass one. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { AutomationEngine, registerLogicNodes } from '@objectstack/service-automation'; +import type { EngineQueryOptions } from '@objectstack/spec/data'; +import { ApprovalService } from './approval-service.js'; +import { ApprovalsServicePlugin } from './approvals-plugin.js'; +import { registerApprovalNode } from './approval-node.js'; + +const SYSTEM = { isSystem: true, positions: [], permissions: [] } as any; +const asUser = (userId: string) => ({ userId, positions: [], permissions: [] }) as any; +const silent = { info: () => {}, warn: () => {}, error: () => {}, debug: () => {} } as any; + +const ORG = 'org_outcome'; +const OUTCOME_TOPICS = ['approval.approved', 'approval.rejected']; +const REJECT_NOTE = 'Over budget, resubmit next quarter.'; + +const USERS = [ + { id: 'u_sub', name: 'Sam Submitter', email: 'sam@example.com' }, + { id: 'u_alice', name: 'Alice', email: 'alice@example.com' }, + { id: 'u_bob', name: 'Bob', email: 'bob@example.com' }, + { id: 'u_mgr', name: 'Mona Manager', email: 'mona@example.com' }, + { id: 'u_exec', name: 'Eve Exec', email: 'eve@example.com' }, +]; + +const sysUser = { + name: 'sys_user', + label: 'User', + fields: { + id: { name: 'id', label: 'Id', type: 'text' as const, primaryKey: true }, + name: { name: 'name', label: 'Name', type: 'text' as const }, + email: { name: 'email', label: 'Email', type: 'text' as const }, + }, +}; +const deal = { + name: 'crm_deal', + label: 'Deal', + fields: { + id: { name: 'id', label: 'Id', type: 'text' as const, primaryKey: true }, + title: { name: 'title', label: 'Title', type: 'text' as const }, + amount: { name: 'amount', label: 'Amount', type: 'number' as const }, + }, +}; + +/** The plugin's own object definitions, as its manifest registers them. */ +async function packageObjects(): Promise { + let manifest: any; + const ctx: any = { + getService: (name: string) => { + if (name === 'manifest') return { register: (m: any) => { manifest = m; } }; + throw new Error(`no service '${name}'`); + }, + logger: silent, + }; + await new ApprovalsServicePlugin().init(ctx); + return manifest.objects; +} + +const user = (value: string) => ({ type: 'user', value }); + +/** One approval step between start and two ends. */ +function oneStep(name: string, config: Record): any { + return { + name, label: name, type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'review', type: 'approval', label: 'Review', config }, + { id: 'ok', type: 'end', label: 'Approved' }, + { id: 'no', type: 'end', label: 'Rejected' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'review' }, + { id: 'e2', source: 'review', target: 'ok', label: 'approve' }, + { id: 'e3', source: 'review', target: 'no', label: 'reject' }, + ], + }; +} + +/** + * Two approval steps in series, the showcase budget approval's shape: a + * manager, then — only above the threshold — an executive. Below it the + * gateway walks straight to the end, so the manager's decision is the last. + */ +const LADDER = { + name: 'deal_ladder', label: 'Deal Ladder', type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'mgr', type: 'approval', label: 'Manager Review', config: { approvers: [user('u_mgr')] } }, + { id: 'big', type: 'decision', label: 'Above 1000?' }, + { id: 'exec', type: 'approval', label: 'Executive Review', config: { approvers: [user('u_exec')] } }, + { id: 'ok', type: 'end', label: 'Approved' }, + { id: 'no', type: 'end', label: 'Rejected' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'mgr' }, + { id: 'e2', source: 'mgr', target: 'big', label: 'approve' }, + { id: 'e3', source: 'mgr', target: 'no', label: 'reject' }, + { id: 'e4', source: 'big', target: 'exec', label: 'true', condition: 'record.amount > 1000' }, + { id: 'e5', source: 'big', target: 'ok', label: 'false', condition: 'record.amount <= 1000' }, + { id: 'e6', source: 'exec', target: 'ok', label: 'approve' }, + { id: 'e7', source: 'exec', target: 'no', label: 'reject' }, + ], +}; + +interface Emitted { + topic: string; + audience: string[]; + source?: { object: string; id: string }; + payload?: Record; + actorId?: string; + organizationId?: string; +} + +describe('the decision that ends an approval tells its submitter (approval.approved / approval.rejected)', () => { + let engine: ObjectQL; + let automation: AutomationEngine; + let svc: ApprovalService; + let now = Date.parse('2026-10-20T09:00:00.000Z'); + const emits: Emitted[] = []; + + /** Start `flow` on a fresh deal as the submitter; answer the step it paused on. */ + const submit = async (flow: string, key: string, amount = 100): Promise => { + const record = { id: `D_${key}`, title: key, amount, organization_id: ORG }; + await engine.insert('crm_deal', record, { context: SYSTEM } as any); + const run = await automation.execute(flow, { object: 'crm_deal', record, userId: 'u_sub' } as any); + expect(run.status, `${flow} on D_${key} paused on its first step`).toBe('paused'); + return pendingOn(`D_${key}`); + }; + /** The one pending request on a deal, or `undefined`. */ + const pendingOn = async (recordId: string): Promise => { + const rows: any[] = await engine.find('sys_approval_request', { + where: { record_id: recordId, status: 'pending' }, context: SYSTEM, + } satisfies EngineQueryOptions); + expect(rows.length, `at most one pending request on ${recordId}`).toBeLessThanOrEqual(1); + return rows[0]; + }; + const stored = async (requestId: string): Promise => + engine.findOne('sys_approval_request', { where: { id: requestId } }, { context: SYSTEM }); + /** The outcome notices sent about one request. */ + const outcomesAbout = (requestId: string) => + emits.filter((e) => OUTCOME_TOPICS.includes(e.topic) && e.source?.id === requestId); + + beforeAll(async () => { + engine = new ObjectQL(); + engine.registerDriver(new SqlDriver({ + client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true, + }), true); + await engine.init(); + for (const def of [sysUser, deal, ...(await packageObjects())]) { + engine.registry.registerObject(def as any, 'approvals-test', 'approvals-test'); + } + await engine.syncSchemas(); + for (const u of USERS) await engine.insert('sys_user', u, { context: SYSTEM } as any); + + automation = new AutomationEngine(silent); + // The ladder's gateway is a `decision` node, one of the built-ins the + // automation plugin seeds; a bare engine starts without them. + registerLogicNodes(automation, { logger: silent } as any); + svc = new ApprovalService({ + engine: engine as any, + clock: { now: () => new Date(now += 1000) }, + logger: silent, + }); + svc.attachAutomation(automation as any); + svc.attachMessaging({ async emit(input) { emits.push(input as Emitted); } }); + registerApprovalNode(automation as any, svc, silent); + + automation.registerFlow('deal_review', oneStep('deal_review', { approvers: [user('u_alice')] })); + automation.registerFlow('own_review', oneStep('own_review', { approvers: [user('u_sub')] })); + automation.registerFlow('co_sign', oneStep('co_sign', { + approvers: [user('u_alice'), user('u_bob')], behavior: 'unanimous', + })); + automation.registerFlow('deal_ladder', LADDER as any); + for (const [name, notifySubmitter] of [['sla_told', true], ['sla_quiet', false]] as const) { + automation.registerFlow(name, oneStep(name, { + approvers: [user('u_alice')], + escalation: { enabled: true, timeoutHours: 1, action: 'auto_approve', notifySubmitter }, + })); + } + }); + + afterAll(async () => { + try { await engine?.destroy(); } catch { /* noop */ } + }); + + it('an approval sends the submitter one approval.approved: the record, the outcome, the link', async () => { + const req = await submit('deal_review', 'won'); + const out = await svc.decide(req.id, { decision: 'approve', actorId: 'u_alice' }, asUser('u_alice')); + expect([out.finalized, out.resumed, (await stored(req.id)).status]).toEqual([true, true, 'approved']); + + const told = outcomesAbout(req.id); + expect(told.map((e) => [e.topic, e.audience])).toEqual([['approval.approved', ['u_sub']]]); + expect(told[0].payload).toEqual({ + title: 'Request approved', + body: 'Your Deal D_won was approved.', + actionUrl: `/system/approvals?request=${encodeURIComponent(req.id)}`, + }); + expect(told[0].source).toEqual({ object: 'sys_approval_request', id: req.id }); + expect(told[0].actorId).toBe('u_alice'); + expect(told[0].organizationId).toBe(ORG); + expect((await stored(req.id)).organization_id).toBe(ORG); + }); + + it("a rejection sends the submitter one approval.rejected, carrying the decider's comment", async () => { + const req = await submit('deal_review', 'lost'); + await svc.decide(req.id, { decision: 'reject', actorId: 'u_alice', comment: ` ${REJECT_NOTE} ` }, asUser('u_alice')); + + const told = outcomesAbout(req.id); + expect(told.map((e) => [e.topic, e.audience])).toEqual([['approval.rejected', ['u_sub']]]); + expect(told[0].payload).toEqual({ + title: 'Request rejected', + body: `Your Deal D_lost was rejected.\n\n${REJECT_NOTE}`, + actionUrl: `/system/approvals?request=${encodeURIComponent(req.id)}`, + }); + expect(told[0].actorId).toBe('u_alice'); + }); + + it('a submitter who decides their own request is told nothing; another decider on the same flow is the control', async () => { + const own = await submit('own_review', 'self'); + await svc.decide(own.id, { decision: 'approve', actorId: 'u_sub' }, asUser('u_sub')); + // The scene: the request really finalized, so the silence is the rule, not a no-op. + expect((await stored(own.id)).status).toBe('approved'); + expect(outcomesAbout(own.id)).toEqual([]); + + // Control: the same decision by someone else does tell the submitter. + const other = await submit('deal_review', 'other'); + await svc.decide(other.id, { decision: 'approve', actorId: 'u_alice' }, asUser('u_alice')); + expect(outcomesAbout(other.id).map((e) => [e.topic, e.audience])).toEqual([['approval.approved', ['u_sub']]]); + }); + + it('a co-sign vote that leaves the request pending tells no one; the vote that finalizes it does', async () => { + const req = await submit('co_sign', 'cosign'); + const first = await svc.decide(req.id, { decision: 'approve', actorId: 'u_alice' }, asUser('u_alice')); + expect(first.finalized).toBe(false); + expect(outcomesAbout(req.id)).toEqual([]); + + const last = await svc.decide(req.id, { decision: 'approve', actorId: 'u_bob' }, asUser('u_bob')); + expect(last.finalized).toBe(true); + expect(outcomesAbout(req.id).map((e) => [e.topic, e.audience, e.actorId])).toEqual([ + ['approval.approved', ['u_sub'], 'u_bob'], + ]); + }); + + it('on a two-step process the first step tells no one; the step that ends the run tells its submitter', async () => { + const step1 = await submit('deal_ladder', 'ladder', 5000); + expect(step1.flow_node_id).toBe('mgr'); + await svc.decide(step1.id, { decision: 'approve', actorId: 'u_mgr' }, asUser('u_mgr')); + expect((await stored(step1.id)).status).toBe('approved'); + + // The run moved on to the executive step, so the manager's approval was a step, not the outcome. + const step2 = await pendingOn('D_ladder'); + expect([step2?.flow_node_id, step2?.flow_run_id]).toEqual(['exec', step1.flow_run_id]); + expect(emits.filter((e) => OUTCOME_TOPICS.includes(e.topic) && [step1.id, step2.id].includes(e.source?.id ?? ''))) + .toEqual([]); + + await svc.decide(step2.id, { decision: 'approve', actorId: 'u_exec' }, asUser('u_exec')); + expect(await pendingOn('D_ladder')).toBeUndefined(); + // The notice follows the terminal request's own `submitter_id`, the field + // every submitter-facing approval notice reads. + const submitter = String((await stored(step2.id)).submitter_id); + expect(outcomesAbout(step2.id).map((e) => [e.topic, e.audience])).toEqual([['approval.approved', [submitter]]]); + expect(outcomesAbout(step1.id)).toEqual([]); + }); + + it('a rejection at the first step ends the process, so it tells the submitter', async () => { + const step1 = await submit('deal_ladder', 'vetoed', 5000); + await svc.decide(step1.id, { decision: 'reject', actorId: 'u_mgr' }, asUser('u_mgr')); + expect(await pendingOn('D_vetoed')).toBeUndefined(); + expect(outcomesAbout(step1.id).map((e) => [e.topic, e.audience])).toEqual([['approval.rejected', ['u_sub']]]); + }); + + it('a gateway that skips the later step makes the first decision the outcome — measured on the run, not read off the graph', async () => { + // The graph still holds the executive step; this deal's amount routes around it. + const step1 = await submit('deal_ladder', 'small', 500); + await svc.decide(step1.id, { decision: 'approve', actorId: 'u_mgr' }, asUser('u_mgr')); + expect(await pendingOn('D_small')).toBeUndefined(); + expect(outcomesAbout(step1.id).map((e) => [e.topic, e.audience])).toEqual([['approval.approved', ['u_sub']]]); + }); + + it("the SLA sweep's auto-decision is told by the escalation's submitter notice under notifySubmitter, never a second time", async () => { + const told = await submit('sla_told', 'sla_told'); + const quiet = await submit('sla_quiet', 'sla_quiet'); + now += 2 * 3600_000; + expect(await svc.runEscalations()).toMatchObject({ escalated: 2 }); + for (const req of [told, quiet]) { + expect((await stored(req.id)).status, `${req.record_id} was auto-approved`).toBe('approved'); + expect(outcomesAbout(req.id)).toEqual([]); + } + const toSubmitter = (req: any) => emits + .filter((e) => e.source?.id === req.id && e.audience.includes('u_sub')) + .map((e) => e.topic); + // The one message the submitter gets names the action the sweep took … + expect(toSubmitter(told)).toEqual(['approval.sla_breached']); + expect(String(emits.find((e) => e.source?.id === told.id && e.audience.includes('u_sub'))?.payload?.body)) + .toContain("escalation action 'auto_approve' was taken"); + // … and `notifySubmitter: false` still means the submitter hears nothing. + expect(toSubmitter(quiet)).toEqual([]); + + // Control: a person deciding the same flow's request does tell the submitter. + const human = await submit('sla_told', 'sla_human'); + await svc.decide(human.id, { decision: 'approve', actorId: 'u_alice' }, asUser('u_alice')); + expect(outcomesAbout(human.id).map((e) => [e.topic, e.audience])).toEqual([['approval.approved', ['u_sub']]]); + }); +}); 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 da7f391f657..486625ca21c 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 @@ -25,10 +25,10 @@ * - a comment and a request-info question ARE the body their recipient gets; * - every notification carries a non-empty `body` and no field outside the * payload `notify()` declares — so a second spelling of the text is red; - * - the battery reaches every call site that can notify anyone — twelve of - * the thirteen — so neither pin passes over a site it never drove. + * - the battery reaches every call site that can notify anyone — thirteen of + * the fourteen — so neither pin passes over a site it never drove. * - * The thirteenth, the reminder to a slot literal (`position:

`), never reaches + * The fourteenth, the reminder to a slot literal (`position:

`), never reaches * messaging at all: `notify()` drops every audience entry that is a slot * address, and that reminder's audience is nothing else. Its payload is held * to the same declared shape by the compiler, not by this battery. @@ -101,6 +101,8 @@ const GONE = 'u_gone'; const COMMENT = 'The signed contract is attached to the record now.'; const QUESTION = 'Which cost centre should this be booked against?'; const SEND_BACK = 'Split the total by quarter, please.'; +/** [#24753] The rejecting approver's note, which the submitter's outcome notice carries. */ +const REJECTION = 'Not this quarter: the budget is spent.'; /** * [#23284] The two organizations the battery's deals live in. The SLA pair sits @@ -246,6 +248,12 @@ describe('every approval notification carries its text in body', () => { await open('rework', [{ type: 'user', value: 'u_alice' }]); await svc.sendBack(ids.rework, { actorId: 'u_alice' } as any, asUser('u_alice')); + // ── [#24753] The decision that ends a request tells its submitter the outcome ── + await open('approved', [{ type: 'user', value: 'u_alice' }]); + await svc.decide(ids.approved, { decision: 'approve', actorId: 'u_alice' }, asUser('u_alice')); + await open('rejected', [{ type: 'user', value: 'u_alice' }]); + await svc.decide(ids.rejected, { decision: 'reject', actorId: 'u_alice', comment: REJECTION }, 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')); @@ -281,7 +289,7 @@ describe('every approval notification carries its text in body', () => { try { await engine?.destroy(); } catch { /* noop */ } }); - it('the battery reaches every call site that can notify anyone — twelve of the thirteen', () => { + it('the battery reaches every call site that can notify anyone — thirteen of the fourteen', () => { expect(new Set(emits.map(siteOf))).toEqual(new Set([ // The opening tells each concrete approver on every request the battery // opens — all but `ooo` and `ooo_gone`, whose one approver is the @@ -293,6 +301,8 @@ describe('every approval notification carries its text in body', () => { 'approval.requested@sla_reassign', 'approval.requested@rework', 'approval.requested@bare', + 'approval.requested@approved', + 'approval.requested@rejected', 'approval.ooo_substituted@ooo', 'approval.ooo_skipped@ooo', 'approval.ooo_substituted@ooo_gone', @@ -300,6 +310,9 @@ describe('every approval notification carries its text in body', () => { 'approval.returned@budget', 'approval.returned@revise', 'approval.returned@rework', + // [#24753] One call site, one topic per outcome. + 'approval.approved@approved', + 'approval.rejected@rejected', 'approval.reassigned@thread', 'approval.reminder@thread', 'approval.reminder@bare', @@ -311,7 +324,7 @@ describe('every approval notification carries its text in body', () => { ])); }); - it("a comment, a request-info question and a send-back note are their recipient's body", () => { + it("a comment, a request-info question, a send-back note and a rejection note reach their recipient's body", () => { const one = (topic: string, key: string) => { const hits = emits.filter((e) => e.topic === topic && e.source?.id === ids[key]); expect(hits.length, `${topic} on ${key}`).toBe(1); @@ -323,6 +336,9 @@ describe('every approval notification carries its text in body', () => { expect([question.audience, question.payload?.body]).toEqual([['u_sub'], QUESTION]); const sentBack = one('approval.returned', 'revise'); expect([sentBack.audience, sentBack.payload?.body]).toEqual([['u_sub'], SEND_BACK]); + // [#24753] The rejection note follows the outcome sentence, which names the record. + const rejected = one('approval.rejected', 'rejected'); + expect([rejected.audience, rejected.payload?.body]).toEqual([['u_sub'], `Your Deal D_rejected was rejected.\n\n${REJECTION}`]); }); it('[#23978] every body that names the record names it by its object label and its id, as approval.requested does', () => { @@ -340,6 +356,8 @@ describe('every approval notification carries its text in body', () => { 'approval.returned@rework': 'Deal D_rework', 'approval.ooo_substituted@ooo': 'Deal D_ooo', 'approval.ooo_skipped@ooo': 'Deal D_ooo', + 'approval.approved@approved': 'Deal D_approved', + 'approval.rejected@rejected': 'Deal D_rejected', // 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': 'crm_memo D_bare', diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index b417ef74320..3382971460c 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -4477,11 +4477,35 @@ export class ApprovalService implements IApprovalService { * reached. `decideNode`'s pre-flight means the common case (the run died * before the decision) never gets this far; what survives here is a genuine * race, and it names the stranded run. + * + * [#24753] A decision that ends the approval process tells the submitter its + * outcome (`approval.approved` / `approval.rejected`, + * {@link ApprovalService.notifyDecisionOutcome}). */ async decide( requestId: string, input: ApprovalDecisionInput, context: ExecutionContext, + ): Promise { + return this.decideAndResume(requestId, input, context, 'outcome'); + } + + /** + * {@link ApprovalService.decide}'s body, with WHO TELLS THE SUBMITTER named by + * the caller (#24753). `'outcome'` — every human door: the decision tells the + * submitter itself, once the resume has shown whether it ended the process. + * `'escalation'` — the SLA sweep's auto-decision only: the escalation's own + * submitter notice tells them, under the node's declared + * `escalation.notifySubmitter` switch ({@link ApprovalService.escalateRequest}), + * so `notifySubmitter: false` keeps meaning that the submitter hears nothing + * about it. A parameter rather than a reading of the actor: the sweep's + * {@link SLA_ACTOR_ID} is an acting identity, never a discriminator. + */ + private async decideAndResume( + requestId: string, + input: ApprovalDecisionInput, + context: ExecutionContext, + submitterToldBy: 'outcome' | 'escalation', ): Promise { const result = await this.decideNode(requestId, input, context); @@ -4521,6 +4545,14 @@ export class ApprovalService implements IApprovalService { resumeFailure = outcome.resumeFailure; } + // [#24753] After the resume, never before it: only the continuation shows + // whether this decision ended the process or moved it to a further + // approval step. A resume that strands has thrown above, so — as on the + // send-back — a stranded decision tells no one. + if (result.finalized && submitterToldBy === 'outcome') { + await this.notifyDecisionOutcome(result.request, result.decision, input.comment, context); + } + return { request: result.request, finalized: result.finalized, @@ -4536,6 +4568,80 @@ export class ApprovalService implements IApprovalService { }; } + /** + * [#24753] Tell the submitter how their approval ended. A send-back + * (`approval.returned`) and a question (`approval.request_info`) already + * reached them; the decision itself reached no one, so a submitter learned + * their request was approved or rejected only by opening the inbox. + * + * ONE TOPIC PER OUTCOME — `approval.approved`, `approval.rejected`. Every + * topic in the `approval.*` vocabulary names what happened to the request + * (`requested`, `returned`, `reassigned`, `escalated`), and these two are the + * request's own terminal statuses, as `returned` is. So the payload stays the + * closed {@link ApprovalNotificationPayload} with no outcome field, and the + * preference matrix — matched on the exact topic or `*` — can mute one + * outcome and keep the other. + * + * The shape is the sibling notices': addressed to the request's own + * `submitter_id`, the record named by {@link ApprovalService.nameSubjectRecord} + * (#23556), a non-empty body (#21847), deep-linked to the request by + * `notify()`, the decider as the actor. The body is one English sentence, + * like every approval notification — none of them reads the caller's locale; + * the decider's comment, when they wrote one, follows it as its own paragraph. + * + * Told nothing: + * - a submitter who made the decision themselves — they already know; + * - anyone, when the decision ENDS A STEP and not the process: a resume that + * opened a further approval step on the same run (the next approval node + * down the edge this decision took) leaves the outcome to that step's + * decision. Measured after the resume, never read off the graph, because a + * decision gateway can skip a later step and only the run knows whether it + * did. A co-sign vote that leaves the request pending never gets here; + * - anyone, for the SLA sweep's auto-decision ({@link ApprovalService.decideAndResume}). + * + * Best-effort like every `notify()`: the decision is durable before this runs, + * so nothing here can fail it. + */ + private async notifyDecisionOutcome( + request: ApprovalRequestRow, + decision: 'approve' | 'reject', + comment: string | undefined, + context: ExecutionContext, + ): Promise { + const submitter = request.submitter_id != null ? String(request.submitter_id) : ''; + if (!submitter || recordedActor(context) === submitter) return; + if (request.flow_run_id) { + let nextStepOpened: boolean; + try { + const next = await this.engine.find('sys_approval_request', { + where: { flow_run_id: request.flow_run_id, status: 'pending' }, limit: 1, context: SYSTEM_CTX, + }); + nextStepOpened = Array.isArray(next) && next.length > 0; + } catch (err: any) { + this.logger?.warn?.('[approvals] could not tell whether a decision ended its approval process — its submitter is not told the outcome', { + request: request.id, run: request.flow_run_id, error: err?.message ?? String(err), + }); + return; + } + if (nextStepOpened) return; + } + const approved = decision === 'approve'; + const sentence = `Your ${this.nameSubjectRecord(request.object_name, request.record_id)} was ${approved ? 'approved' : 'rejected'}.`; + const note = comment?.trim(); + await this.notify({ + topic: approved ? 'approval.approved' : 'approval.rejected', + organizationId: request.organization_id, + audience: [submitter], + actorContext: context, + source: { object: 'sys_approval_request', id: request.id }, + payload: { + title: approved ? 'Request approved' : 'Request rejected', + body: note ? `${sentence}\n\n${note}` : sentence, + actionUrl: '/system/approvals', + }, + }); + } + /** * Withdraw an undecided request. Finalises the row as * `recalled`, releases the record lock (keyed on pending status), mirrors @@ -6770,11 +6876,14 @@ export class ApprovalService implements IApprovalService { }, }); } else if (action === 'auto_approve' || action === 'auto_reject') { - await this.decide(raw.id, { + // [#24753] The submitter hears this outcome from the submitter notice + // below (`notifySubmitter`), which names the action taken — not a second + // time from the decision's `approval.approved` / `approval.rejected`. + await this.decideAndResume(raw.id, { decision: action === 'auto_approve' ? 'approve' : 'reject', actorId: SLA_ACTOR_ID, comment: 'SLA escalation', - }, SYSTEM_CTX); + }, SYSTEM_CTX, 'escalation'); } else { // 'notify' (and the reassign-without-target fallback) await this.notify({