diff --git a/.changeset/2349-recall-no-rejected-notice.md b/.changeset/2349-recall-no-rejected-notice.md new file mode 100644 index 000000000..2f3bf5180 --- /dev/null +++ b/.changeset/2349-recall-no-rejected-notice.md @@ -0,0 +1,10 @@ +--- +"hotcrm": patch +--- + +Clause-②: no +Metadata-protocol: none + +**Recalling an approval request no longer tells you it was rejected (#2349).** When a rep recalled their own pending request (the Recall button on the request, or `POST /api/v1/approvals/requests/{id}/recall`), HotCRM sent them the flow's rejection notice, for example "Deal rejected: NAME — Your opportunity NAME was not approved", in the inbox and, for a deal, by email. Nobody had rejected anything. The platform resumes a recalled request down the approval's reject path, and three approval flows sent their notice there without looking at why. + +Large Deal Approval (at both the Sales Manager and the Sales Director step), Account Approval and Lead Conversion Approval now end quietly on a recall. A rejection by an approver still sends the notice exactly as before. Qualification Approval and Stage Change Approval never sent a notice on reject and are unchanged. diff --git a/src/sales/flows/account-approval.flow.ts b/src/sales/flows/account-approval.flow.ts index 758aeb01e..4d5be0718 100644 --- a/src/sales/flows/account-approval.flow.ts +++ b/src/sales/flows/account-approval.flow.ts @@ -192,7 +192,14 @@ export const AccountApprovalFlow: Flow = { // Approval-node branch labels — the engine resumes down the out-edge whose // `label` matches the recorded decision. { id: 'e3', source: 'account_review', target: 'notify_approved', type: 'default', label: 'approve' }, - { id: 'e4', source: 'account_review', target: 'notify_rejected', type: 'default', label: 'reject' }, + // A recall resumes down `reject` as well, with `decision: 'recall'`: it + // ends the run without the rejection's notice. The pair partitions, and an + // unreadable decision notifies (#2349, the full note on + // `opportunity_approval`'s `e4`). + { id: 'e4', source: 'account_review', target: 'notify_rejected', type: 'conditional', label: 'reject', + condition: P`!has(vars.account_review) || !has(vars.account_review.decision) || vars.account_review.decision != "recall"` }, + { id: 'e4_recall', source: 'account_review', target: 'end', type: 'conditional', label: 'reject', + condition: P`has(vars.account_review) && has(vars.account_review.decision) && vars.account_review.decision == "recall"` }, { id: 'e6', source: 'notify_approved', target: 'end', type: 'default' }, { id: 'e8', source: 'notify_rejected', target: 'end', type: 'default' }, ], diff --git a/src/sales/flows/lead-conversion-approval.flow.ts b/src/sales/flows/lead-conversion-approval.flow.ts index 02c82f69d..fd14f7b3d 100644 --- a/src/sales/flows/lead-conversion-approval.flow.ts +++ b/src/sales/flows/lead-conversion-approval.flow.ts @@ -164,7 +164,14 @@ export const LeadConversionApprovalFlow: Flow = { // Approval-node branch labels — the engine resumes down the out-edge whose // `label` matches the recorded decision. { id: 'e3', source: 'conversion_review', target: 'notify_approved', type: 'default', label: 'approve' }, - { id: 'e4', source: 'conversion_review', target: 'notify_rejected', type: 'default', label: 'reject' }, + // A recall resumes down `reject` as well, with `decision: 'recall'`: it + // ends the run without the rejection's notice. The pair partitions, and an + // unreadable decision notifies (#2349, the full note on + // `opportunity_approval`'s `e4`). + { id: 'e4', source: 'conversion_review', target: 'notify_rejected', type: 'conditional', label: 'reject', + condition: P`!has(vars.conversion_review) || !has(vars.conversion_review.decision) || vars.conversion_review.decision != "recall"` }, + { id: 'e4_recall', source: 'conversion_review', target: 'end', type: 'conditional', label: 'reject', + condition: P`has(vars.conversion_review) && has(vars.conversion_review.decision) && vars.conversion_review.decision == "recall"` }, { id: 'e6', source: 'notify_approved', target: 'end', type: 'default' }, { id: 'e8', source: 'notify_rejected', target: 'end', type: 'default' }, ], diff --git a/src/sales/flows/opportunity-approval.flow.ts b/src/sales/flows/opportunity-approval.flow.ts index 294bf13d2..8a7609529 100644 --- a/src/sales/flows/opportunity-approval.flow.ts +++ b/src/sales/flows/opportunity-approval.flow.ts @@ -10,7 +10,9 @@ type Flow = Automation.Flow; * * Expressed as **Approval nodes** (`type: 'approval'`, ADR-0019): the engine * opens an approval request on entry, suspends the run, and resumes down the - * out-edge whose `label` matches the decision (`approve` / `reject`). + * out-edge whose `label` matches the decision (`approve` / `reject`). A + * recall resumes down `reject` too, and ends the run there without the + * rejection's notice (#2349). * Notifications use the **`notify` node** (ADR-0012). ⛔ Neither a * `connector_action` nor a standalone `ApprovalProcess` is available — both * authoring surfaces were removed in ObjectStack 7.4. @@ -249,7 +251,22 @@ export const OpportunityApprovalFlow: Flow = { // Manager decision (approval-node branch labels) { id: 'e3', source: 'manager_review', target: 'check_high_value', type: 'default', label: 'approve' }, - { id: 'e4', source: 'manager_review', target: 'notify_rejected', type: 'default', label: 'reject' }, + + // ── A recall is not a rejection (#2349) ── + // The platform resumes a RECALLED request down this same `reject` edge + // (`ApprovalService.recall`, `@objectstack/plugin-approvals` 18.2.0), so + // the label alone sent the rep a "rejected" notice for a request nobody + // decided. What tells the two apart is the decision the resume lands as + // `vars..decision`: `'recall'` for a recall, `'reject'` for an + // approver's rejection or an auto-rejection. Both edges carry the label + // (the resume picks among them by it), and their conditions PARTITION in + // opposite polarity, as `e5`/`e6` do. A decision that cannot be read + // notifies: an unreadable decision is today's behaviour, not a silent + // loss of a real rejection's notice. + { id: 'e4', source: 'manager_review', target: 'notify_rejected', type: 'conditional', label: 'reject', + condition: P`!has(vars.manager_review) || !has(vars.manager_review.decision) || vars.manager_review.decision != "recall"` }, + { id: 'e4_recall', source: 'manager_review', target: 'end', type: 'conditional', label: 'reject', + condition: P`has(vars.manager_review) && has(vars.manager_review.decision) && vars.manager_review.decision == "recall"` }, // Tier gate (decision-node conditional branches). These EDGES are the live // sites — the engine never evaluates a `decision` node's singular @@ -278,9 +295,14 @@ export const OpportunityApprovalFlow: Flow = { { id: 'e6', source: 'check_high_value', target: 'mark_approved', type: 'conditional', condition: P`!has(vars.oppRecord) || !has(vars.oppRecord.amount) || vars.oppRecord.amount == null || vars.oppRecord.amount <= ${HIGH_VALUE_DEAL_AMOUNT}`, label: 'Standard (≤ $500K)' }, - // Director decision (approval-node branch labels) + // Director decision (approval-node branch labels). A recall here — the + // rep withdrawing a deal the manager already approved — ends the run the + // same way (see `e4`). { id: 'e7', source: 'director_signoff', target: 'mark_approved', type: 'default', label: 'approve' }, - { id: 'e8', source: 'director_signoff', target: 'notify_rejected', type: 'default', label: 'reject' }, + { id: 'e8', source: 'director_signoff', target: 'notify_rejected', type: 'conditional', label: 'reject', + condition: P`!has(vars.director_signoff) || !has(vars.director_signoff.decision) || vars.director_signoff.decision != "recall"` }, + { id: 'e8_recall', source: 'director_signoff', target: 'end', type: 'conditional', label: 'reject', + condition: P`has(vars.director_signoff) && has(vars.director_signoff.decision) && vars.director_signoff.decision == "recall"` }, // Terminal branches { id: 'e9', source: 'mark_approved', target: 'notify_approved', type: 'default' }, diff --git a/test/approval-decision-attribution.test.ts b/test/approval-decision-attribution.test.ts index fd0bb022a..974fe3c36 100644 --- a/test/approval-decision-attribution.test.ts +++ b/test/approval-decision-attribution.test.ts @@ -2,9 +2,8 @@ import { describe, it, expect, beforeAll } from 'vitest'; import { bootStack, type VerifyStack } from '@objectstack/verify'; -import artifact from '../objectstack.config'; import { - hotcrmStack, buildArtifactAgain, bootOptions, signUpPerson, systemUpdate, flowRuns, type Person, + hotcrmStack, buildArtifactAgain, armedLeadGate, bootOptions, signUpPerson, systemUpdate, flowRuns, type Person, } from './helpers/verify-stack'; type Rec = Record; @@ -149,33 +148,6 @@ describe('opportunity_qualification_approval: an approval is the approver\'s', ( }); }); -/** - * The lead gate ships OFF (REQ-0005), so it is measured on an install that has - * armed it, the one way an install does: `crm_lead.conversion_approval_status` - * defaults to `pending`. A second boot, from a fresh build of the app's - * configuration (`buildArtifactAgain`), because one configuration object holds - * one live kernel. - */ -const armedLeadGate = (base: typeof artifact): typeof artifact => { - const a = base as unknown as Rec; - return { - ...a, - packages: (a.packages as Rec[]).map((p) => ({ - ...p, - manifest: { - ...p.manifest, - objects: ((p.manifest.objects ?? []) as Rec[]).map((o) => (o.name !== 'crm_lead' ? o : { - ...o, - fields: { - ...o.fields, - conversion_approval_status: { ...o.fields.conversion_approval_status, defaultValue: 'pending' }, - }, - })), - }, - })), - } as typeof artifact; -}; - describe('lead_conversion_approval (gate armed): both decisions are the approver\'s', () => { let armed: VerifyStack; let who: { rep: Person; manager: Person }; diff --git a/test/approval-recall-no-rejected-notice.test.ts b/test/approval-recall-no-rejected-notice.test.ts new file mode 100644 index 000000000..23975504f --- /dev/null +++ b/test/approval-recall-no-rejected-notice.test.ts @@ -0,0 +1,204 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect, beforeAll, vi } from 'vitest'; +import { bootStack, type VerifyStack } from '@objectstack/verify'; +import { + hotcrmStack, buildArtifactAgain, armedLeadGate, bootOptions, signUpPerson, flowRuns, notificationsTo, + settleNotificationDeliveries, type Person, +} from './helpers/verify-stack'; + +type Rec = Record; + +/** + * A recall is not a rejection (#2349). + * + * `@objectstack/plugin-approvals` 18.2.0 `ApprovalService.recall` resumes the + * waiting run down the approval node's `reject` edge, with + * `output.decision = 'recall'` (a rejection resumes the same edge with + * `decision = 'reject'`). In `opportunity_approval`, `account_approval` and + * `lead_conversion_approval` that edge went straight to `notify_rejected`, so a + * rep who took their own request back was told an approver had turned it down, + * in the inbox and (for a deal) by email. Measured on 18.2.0 before this change: + * every recall below wrote the flow's "rejected" notice. + * + * The resume lands the decision as the flow variable `.decision`, which + * an edge reads as `vars..decision`. Each reject edge is now a + * partitioned pair: a recall ends the run, anything else notifies as before. + * The other two approval flows route reject to `clear_request` and notify on + * neither, so they are not here. + * + * Every case drives the real door — `POST /approvals/requests/{id}/recall` as + * the submitter, `/reject` as the approver — and reads the notifications off + * the messaging outbox (`notificationsTo`), which `notify` writes inside the + * resumed run. + */ + +const people = async (s: VerifyStack, tag: string) => ({ + rep: await signUpPerson(s, `rep-${tag}@approval-recall.test`, { + name: 'Recall Rep', positions: ['sales_rep'], permissionSets: ['sales_rep'], + }), + manager: await signUpPerson(s, `manager-${tag}@approval-recall.test`, { + name: 'Recall Manager', positions: ['sales_manager'], permissionSets: ['sales_manager'], + }), +}); + +type Who = Awaited>; +type Verb = 'recall' | 'reject' | 'approve'; + +/** The record's one pending request, acted on through the approvals door. */ +const act = async (s: VerifyStack, id: string, who: Person, verb: Verb): Promise => { + let open: Rec[] = []; + await vi.waitFor(async () => { + open = (await s.rows('sys_approval_request', { record_id: id })).filter((r) => r.status === 'pending'); + expect(open, 'the record has no single open approval request').toHaveLength(1); + }, { timeout: 15_000, interval: 100 }); + await settleNotificationDeliveries(s, async () => { + const res = await s.apiAs(who.token, 'POST', `/approvals/requests/${open[0]!.id}/${verb}`, { comment: `${verb} by test.` }); + expect(res.status, await res.clone().text()).toBe(200); + }); +}; + +/** The `topic` notices the record's owner was sent about this one record. */ +const noticesAbout = async (s: VerifyStack, owner: Person, topic: string, object: string, id: string): Promise => + (await notificationsTo(s, owner.id, topic)).filter((n) => n.payload?.actionUrl === `/${object}/${id}`); + +/** The run behind the record, finished, and whether its `notify_rejected` node ran. */ +const finishedRun = async (s: VerifyStack, flow: string, id: string): Promise<{ notifiedRejected: boolean }> => { + const runs = await flowRuns(s, flow, id); + expect(runs, `not one ${flow} run for the record`).toHaveLength(1); + expect(runs[0]!.status, String(runs[0]!.error ?? '')).toBe('completed'); + const nodes = (runs[0]!.summary?.nodes ?? []) as Rec[]; + return { notifiedRejected: nodes.some((n) => n.nodeId === 'notify_rejected' && n.runs > 0) }; +}; + +describe('opportunity_approval: a recall ends the run without the rejected notice', () => { + let verify: VerifyStack; + let who: Who; + let director: Person; + let accountId: string; + let k = 0; + beforeAll(async () => { + verify = await hotcrmStack(); + who = await people(verify, 'opp'); + // `director_signoff` routes here, so the second tier is a real approver's. + director = await signUpPerson(verify, 'director-opp@approval-recall.test', { + name: 'Recall Director', positions: ['sales_director'], permissionSets: ['sales_manager'], + }); + const [account] = await verify.seed('crm_account', [{ name: 'Recall Co', owner_id: who.rep.id, approval_status: 'approved' }]); + accountId = String(account!.id); + }, 180_000); + + /** A rep's deal raised across the line by the rep: the request is theirs, pending. */ + const dealInApproval = async (amount: number): Promise => { + const [deal] = await verify.seed('crm_opportunity', [{ + name: `Recall Deal ${++k}`, stage: 'proposal', amount: 42_000, close_date: '2030-06-30', + crm_account: accountId, owner_id: who.rep.id, + }]); + const id = String(deal!.id); + await verify.hooks.run('crm_opportunity', 'update', { id, amount }, { as: who.rep.token }); + return id; + }; + + it('recalling at Sales Manager review (e4) sends no notice', async () => { + const id = await dealInApproval(120_000); + await act(verify, id, who.rep, 'recall'); + expect((await verify.rows('sys_approval_request', { record_id: id })).map((r) => r.status)).toEqual(['recalled']); + expect(await noticesAbout(verify, who.rep, 'opportunity_rejected', 'crm_opportunity', id)).toEqual([]); + expect(await finishedRun(verify, 'opportunity_approval', id)).toEqual({ notifiedRejected: false }); + }); + + it('recalling at Sales Director sign-off (e8) sends no notice', async () => { + const id = await dealInApproval(600_000); + await act(verify, id, who.manager, 'approve'); + await act(verify, id, who.rep, 'recall'); + expect((await verify.rows('sys_approval_request', { record_id: id })).map((r) => r.status).sort()).toEqual(['approved', 'recalled']); + expect(await noticesAbout(verify, who.rep, 'opportunity_rejected', 'crm_opportunity', id)).toEqual([]); + expect(await finishedRun(verify, 'opportunity_approval', id)).toEqual({ notifiedRejected: false }); + }); + + it('a Sales Manager\'s rejection still notifies the owner, once', async () => { + const id = await dealInApproval(120_000); + await act(verify, id, who.manager, 'reject'); + expect(await noticesAbout(verify, who.rep, 'opportunity_rejected', 'crm_opportunity', id)).toHaveLength(1); + expect(await finishedRun(verify, 'opportunity_approval', id)).toEqual({ notifiedRejected: true }); + }); + + it('a Sales Director\'s rejection still notifies the owner, once', async () => { + const id = await dealInApproval(600_000); + await act(verify, id, who.manager, 'approve'); + await act(verify, id, director, 'reject'); + expect(await noticesAbout(verify, who.rep, 'opportunity_rejected', 'crm_opportunity', id)).toHaveLength(1); + expect(await finishedRun(verify, 'opportunity_approval', id)).toEqual({ notifiedRejected: true }); + }); +}); + +describe('account_approval: a recall ends the run without the rejected notice', () => { + let verify: VerifyStack; + let who: Who; + let k = 0; + beforeAll(async () => { + verify = await hotcrmStack(); + who = await people(verify, 'account'); + }, 180_000); + + /** An account a rep creates on the New form: born blank, so its request opens, submitted by the rep. */ + const accountInApproval = async (): Promise => { + const res = await verify.apiAs(who.rep.token, 'POST', '/data/crm_account', { name: `Recall Account ${++k} Co` }); + expect(res.status, await res.clone().text()).toBe(201); + const body: Rec = await res.json(); + return String((body.record ?? body.data ?? body).id); + }; + + it('recalling sends no notice', async () => { + const id = await accountInApproval(); + await act(verify, id, who.rep, 'recall'); + expect((await verify.rows('sys_approval_request', { record_id: id })).map((r) => r.status)).toEqual(['recalled']); + expect(await noticesAbout(verify, who.rep, 'account_rejected', 'crm_account', id)).toEqual([]); + expect(await finishedRun(verify, 'account_approval', id)).toEqual({ notifiedRejected: false }); + }); + + it('a Sales Manager\'s rejection still notifies the owner, once', async () => { + const id = await accountInApproval(); + await act(verify, id, who.manager, 'reject'); + expect(await noticesAbout(verify, who.rep, 'account_rejected', 'crm_account', id)).toHaveLength(1); + expect(await finishedRun(verify, 'account_approval', id)).toEqual({ notifiedRejected: true }); + }); +}); + +describe('lead_conversion_approval (gate armed): a recall ends the run without the rejected notice', () => { + let armed: VerifyStack; + let who: Who; + let k = 0; + beforeAll(async () => { + armed = await bootStack(armedLeadGate(await buildArtifactAgain()), bootOptions()); + who = await people(armed, 'lead'); + }, 180_000); + + it('this boot really ships the gate armed', () => { + expect(armed.metadata.object('crm_lead')?.fields?.conversion_approval_status?.defaultValue).toBe('pending'); + }); + + /** A lead the rep creates on the armed install: born `pending`, so its request opens, submitted by the rep. */ + const leadInApproval = async (): Promise => { + const lead = await armed.hooks.run('crm_lead', 'insert', { + company: `Recall Lead ${++k}`, email: `buyer${k}@recall-lead.example.com`, first_name: 'Joe', last_name: 'Green', + phone: '555', title: 'Buyer', lead_source: 'web', status: 'new', + }, { as: who.rep.token }); + return String(lead.id); + }; + + it('recalling sends no notice', async () => { + const id = await leadInApproval(); + await act(armed, id, who.rep, 'recall'); + expect((await armed.rows('sys_approval_request', { record_id: id })).map((r) => r.status)).toEqual(['recalled']); + expect(await noticesAbout(armed, who.rep, 'lead_conversion_rejected', 'crm_lead', id)).toEqual([]); + expect(await finishedRun(armed, 'lead_conversion_approval', id)).toEqual({ notifiedRejected: false }); + }); + + it('a Sales Manager\'s rejection still notifies the owner, once', async () => { + const id = await leadInApproval(); + await act(armed, id, who.manager, 'reject'); + expect(await noticesAbout(armed, who.rep, 'lead_conversion_rejected', 'crm_lead', id)).toHaveLength(1); + expect(await finishedRun(armed, 'lead_conversion_approval', id)).toEqual({ notifiedRejected: true }); + }); +}); diff --git a/test/flow-variable-conditions.test.ts b/test/flow-variable-conditions.test.ts index 6d8d9e940..e0f9935dc 100644 --- a/test/flow-variable-conditions.test.ts +++ b/test/flow-variable-conditions.test.ts @@ -368,6 +368,14 @@ function binds(node: AnyRec): string[] { if (typeof cfg.outputVariable === 'string' && cfg.outputVariable) out.push(cfg.outputVariable); if (typeof cfg.iteratorVariable === 'string' && cfg.iteratorVariable) out.push(cfg.iteratorVariable); if (typeof cfg.idVariable === 'string' && cfg.idVariable) out.push(cfg.idVariable); + // An `approval` node continues only through a resume, and every resume the + // approvals service sends carries `output.decision` (`approve`, `reject`, or + // `recall` from `ApprovalService.recall`, `@objectstack/plugin-approvals` + // 18.2.0). The engine lands each output key as `.` + // (`applyResumeSignal`), so its out-edges read `vars..decision`. + // Measured end-to-end, not assumed: `approval-recall-no-rejected-notice.test.ts` + // goes red if a recall's decision stops reaching the edges that read it (#2349). + if (node.type === 'approval' && typeof node.id === 'string' && node.id) out.push(node.id); const a = cfg.assignments; if (Array.isArray(a)) { for (const item of a) { diff --git a/test/helpers/verify-stack.ts b/test/helpers/verify-stack.ts index 3ecbaf9cd..e84fa8b01 100644 --- a/test/helpers/verify-stack.ts +++ b/test/helpers/verify-stack.ts @@ -127,6 +127,33 @@ export const buildArtifactAgain = async (): Promise => { return (await import('../../objectstack.config')).default; }; +/** + * The app with the lead conversion gate ARMED, the one way an install arms it: + * `crm_lead.conversion_approval_status` defaults to `pending`. The gate ships + * OFF (REQ-0005), so on the shipped default no lead is ever born `pending` and + * `lead_conversion_approval` never opens a request. Pass it a fresh build + * ({@link buildArtifactAgain}): one configuration object holds one live kernel. + */ +export const armedLeadGate = (base: typeof artifact): typeof artifact => { + const a = base as unknown as Row; + return { + ...a, + packages: (a.packages as Row[]).map((p) => ({ + ...p, + manifest: { + ...p.manifest, + objects: ((p.manifest.objects ?? []) as Row[]).map((o) => (o.name !== 'crm_lead' ? o : { + ...o, + fields: { + ...o.fields, + conversion_approval_status: { ...o.fields.conversion_approval_status, defaultValue: 'pending' }, + }, + })), + }, + })), + } as typeof artifact; +}; + /** * The names of every object the booted runtime registers that this app does * not author — the platform's objects, read from the registry rather than from