diff --git a/.changeset/approver-live-record-3447.md b/.changeset/approver-live-record-3447.md new file mode 100644 index 000000000..cdffed750 --- /dev/null +++ b/.changeset/approver-live-record-3447.md @@ -0,0 +1,7 @@ +--- +"@objectstack/plugin-approvals": minor +--- + +Approval nodes now resolve `field` / `manager` approvers against the record's **live** state at node entry, not the trigger snapshot the flow froze at submit time (#3447). An earlier step — or the approver of an earlier step — can now write the field that routes a later step's approvers, enabling dynamic routing / dynamic co-sign (e.g. a lead reviewer picking which departments co-review, then those departments resolving as parallel approvers). Graph approvers (team / position / department / tier) already resolved live; this brings the in-record types into line. + +Also fixes two latent defects on the same path: a multi-select user field now fans out into one approver slot per user (previously the array was stringified to a single bogus id), and out-of-office delegation is applied per fanned-out user (previously silently skipped for multi-value fields). When the record can't be re-read (hard-deleted mid-flow, or a backend that can't serve a point read), resolution falls back to the trigger snapshot and warns rather than wedging the flow. diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index 2153b589c..9ae1085cd 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -187,6 +187,65 @@ describe('ApprovalService (node era)', () => { expect(engine._tables['opportunity'][0].approval_status).toBe('pending'); }); + // ── approver expansion: field (live re-read + fan-out, #3447) ──── + // + // A `field` approver names WHO decides from a record field. It must bind to + // the record's LIVE value at node entry — an earlier step (or the approver of + // an earlier step) may have written it after submit — not the trigger snapshot + // the run froze into `$record`. A multi-select user field fans out into one + // slot per user. + + const fieldInput = (extra: Record = {}) => ({ + ...openInput([]), + config: { + approvers: [{ type: 'field' as const, value: 'approvers_dynamic' }], + behavior: 'unanimous' as const, + lockRecord: true, + }, + ...extra, + }); + + it('field approver: resolves against the LIVE record, not the trigger snapshot (#3447)', async () => { + // The minimal repro: submitted with the routing field empty; a prior step + // wrote the co-reviewers mid-flow. Resolution must see them. + engine._tables['opportunity'] = [{ id: 'opp1', amount: 100, approvers_dynamic: ['u2', 'u3'] }]; + const req = await svc.openNodeRequest( + fieldInput({ record: { id: 'opp1', amount: 100, approvers_dynamic: [] } }), CTX, + ); + expect(req.pending_approvers.sort()).toEqual(['u2', 'u3']); + }); + + it('field approver: the live value WINS over a stale snapshot value (#3447)', async () => { + // Snapshot named u1; the record now names u2. u2 decides. + engine._tables['opportunity'] = [{ id: 'opp1', approvers_dynamic: ['u2'] }]; + const req = await svc.openNodeRequest( + fieldInput({ record: { id: 'opp1', approvers_dynamic: ['u1'] } }), CTX, + ); + expect(req.pending_approvers).toEqual(['u2']); + }); + + it('field approver: fans a multi-select user field into one slot per user (#3447)', async () => { + engine._tables['opportunity'] = [{ id: 'opp1', approvers_dynamic: ['u2', 'u3', 'u4'] }]; + const req = await svc.openNodeRequest(fieldInput({ record: { id: 'opp1' } }), CTX); + expect(req.pending_approvers.sort()).toEqual(['u2', 'u3', 'u4']); + }); + + it('field approver: fans a legacy CSV string field into multiple slots (#3447)', async () => { + engine._tables['opportunity'] = [{ id: 'opp1', approvers_dynamic: 'u2,u3' }]; + const req = await svc.openNodeRequest(fieldInput({ record: { id: 'opp1' } }), CTX); + expect(req.pending_approvers.sort()).toEqual(['u2', 'u3']); + }); + + it('field approver: falls back to the trigger snapshot when the record is gone (#3447)', async () => { + // No `opportunity` row — hard-deleted between submit and node entry. The + // snapshot still carries a value, so the request opens against it (warn but + // proceed) rather than wedging the flow. + const req = await svc.openNodeRequest( + fieldInput({ record: { id: 'opp1', approvers_dynamic: ['u7'] } }), CTX, + ); + expect(req.pending_approvers).toEqual(['u7']); + }); + // ── approver expansion: position (ADR-0090 D3) ────────────────── const positionInput = (extra: Record = {}) => ({ @@ -1294,6 +1353,20 @@ describe('ApprovalService — out-of-office delegation (#1322)', () => { expect(req.pending_approvers).toEqual(['bob']); }); + it('type:field (multi-select) — reroutes each out-of-office user independently (#3447)', async () => { + // Pre-#3447 the array was stringified to one bogus id ('alice,dave'), which + // matched no delegation — OOO silently no-op'd. Fanned out, alice → bob + // applies and dave is left untouched. + seedDelegation([{ delegator_id: 'alice', delegate_id: 'bob' }]); + const input = { + ...openInput([]), + record: { id: 'opp1', reviewers: ['alice', 'dave'] }, + config: { approvers: [{ type: 'field', value: 'reviewers' }], behavior: 'unanimous', lockRecord: true }, + }; + const req = await svc.openNodeRequest(input as any, CTX); + expect(req.pending_approvers.sort()).toEqual(['bob', 'dave']); + }); + it('type:manager — reroutes when the resolved manager is out of office', async () => { engine._tables['sys_user'] = [{ id: 'carol', manager_id: 'alice' }]; seedDelegation([{ delegator_id: 'alice', delegate_id: 'bob' }]); diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 1df6b27fa..40c63af66 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -443,7 +443,15 @@ export class ApprovalService implements IApprovalService { return this.applyOooDelegation(String(a.value), now, organizationId, substitutions); } if (type === 'field' && record) { - return this.applyOooDelegation(String((record as any)[a.value] ?? ''), now, organizationId, substitutions); + // #3447: a record field can name MANY approvers — a multi-select user + // field arrives as an array (or a legacy CSV string). Fan each out into + // its own slot and OOO-substitute per person; collapsing to `String(...)` + // (→ `'u1,u2'`) would mint one bogus approver id and skip every delegate. + const out: string[] = []; + for (const id of csvSplit((record as any)[a.value])) { + out.push(...await this.applyOooDelegation(id, now, organizationId, substitutions)); + } + return out; } try { if (type === 'team') { @@ -662,6 +670,45 @@ export class ApprovalService implements IApprovalService { } } + /** + * Re-read a business record's CURRENT state by id so approver resolution binds + * to live data at node entry, not the trigger snapshot the flow froze into + * `$record` at submit time (#3447). A `field` / `manager` approver names *who* + * decides, and an earlier node — or the approver of an earlier step — may have + * written that routing field after submit (e.g. a lead reviewer picking which + * departments co-review). Graph approvers (team / position / …) already query + * live; this brings the in-record types into line. + * + * Read under system identity: approver routing is a platform concern and the + * record is the flow's own subject, so the submitter's RLS/FLS must not narrow + * it. Degrades to `fallback` (the snapshot) when the record can't be re-read — + * hard-deleted between submit and node entry, or an object whose backend can't + * serve a point read — warning rather than throwing so a transient miss can't + * wedge an approval. That "warn but proceed" stance matches the + * no-concrete-approver guard (#3424) and the "record is gone" enrichment path + * that already falls back to the payload snapshot. + */ + private async loadLiveRecord(object: string, recordId: string, fallback?: any): Promise { + try { + const rows = await this.engine.find(object, { + where: { id: recordId }, limit: 1, context: SYSTEM_CTX, + } as any); + const live = Array.isArray(rows) ? rows[0] : rows; + if (live) return live; + this.logger?.warn?.( + `[approvals] live record ${object}/${recordId} not found at node entry — ` + + 'resolving approvers against the trigger snapshot (#3447 fallback).', + { object, recordId }, + ); + } catch (err: any) { + this.logger?.warn?.( + `[approvals] live record re-read failed for ${object}/${recordId}: ${err?.message ?? err} — ` + + 'resolving approvers against the trigger snapshot (#3447 fallback).', + ); + } + return fallback ?? {}; + } + // ── ADR-0019: Approval-as-flow-node ────────────────────────── // // A flow's Approval node opens a request via `openNodeRequest` (carrying its @@ -715,8 +762,12 @@ export class ApprovalService implements IApprovalService { // per_group finalization is decided against the slate resolved at OPEN time // (OOO-substituted), not re-resolved live at each decision. const groups: Record = {}; + // #3447: resolve approvers against the record's LIVE state at node entry, not + // the trigger snapshot carried in `input.record`. This is the whole fix — an + // earlier step may have written the field this node routes on. + const liveRecord = await this.loadLiveRecord(input.object, input.recordId, input.record); const approvers = await this.expandApprovers( - { approvers: input.config.approvers }, input.record, ctxOrg, { now: nowDate.getTime(), substitutions, groups }, + { approvers: input.config.approvers }, liveRecord, ctxOrg, { now: nowDate.getTime(), substitutions, groups }, ); // #3424: an approval routed to a target with no holders (e.g. an unstaffed