diff --git a/.changeset/approvals-expose-lock-record.md b/.changeset/approvals-expose-lock-record.md new file mode 100644 index 0000000000..7b7e6c4cbb --- /dev/null +++ b/.changeset/approvals-expose-lock-record.md @@ -0,0 +1,37 @@ +--- +"@objectstack/spec": minor +"@objectstack/plugin-approvals": minor +--- + +feat(approvals): expose the pending node's `lockRecord` policy on the request row (#3814, objectui#2902) + +An approval node declares `lockRecord` (default `true`), and the record-lock +`beforeUpdate` hook enforces exactly that: `lockRecord: false` and the record +stays writable for the whole time the node waits. The behavior was correct and +has been since Phase B — but it was **invisible to every client**. + +`rowFromRequest` parses `node_config_json` and projects a whitelist out of it +(`__flowLabel`, `__nodeLabel`, `__round`, `escalation.timeoutHours`, +`decisionOutputs`). `lockRecord` was never in that list, and no other field on +`ApprovalRequestRow` carried the lock either. So the strongest thing a console +could learn from `GET /approvals/requests` was *"a pending request exists"* — +from which it can only assume the record is locked. + +That assumption is wrong on every opted-out node, and a flow that chains nodes +with different policies makes it visibly wrong: the same UI state renders for +"you may edit this" and "the server will reject your save with `RECORD_LOCKED`". +The console has no third option — guessing the other way would offer an edit +that dies on save. + +`ApprovalRequestRow` now carries **`lock_record: boolean`**, read from the same +snapshot the hook reads, with the same `!== false` default. Present on every +service read (`openNodeRequest` / `getRequest` / `listRequests`), so the flag a +client renders and the rule the server applies cannot drift. + +Additive and backward compatible — nothing to migrate. A client that wants +node-accurate lock state reads `request.lock_record`; treat `undefined` (an +older backend) as locked, which is the pre-existing behavior. + +The showcase's `showcase_budget_approval` now declares `lockRecord: false` on +its single-approver Manager Review and keeps `true` on the multi-approver +Executive Review, so both policies are exercised in one flow. diff --git a/examples/app-showcase/src/automation/flows/index.ts b/examples/app-showcase/src/automation/flows/index.ts index 8a1fd07a38..6739049294 100644 --- a/examples/app-showcase/src/automation/flows/index.ts +++ b/examples/app-showcase/src/automation/flows/index.ts @@ -186,7 +186,15 @@ export const BudgetApprovalFlow = defineFlow({ config: { approvers: [{ type: 'position', value: 'manager' }], behavior: 'first_response', - lockRecord: true, + // Deliberately UNLOCKED, and the counterpart to `exec_review` below — + // together they dogfood both record-lock policies in one flow + // (objectui#2902). A single-approver step like this is the case the + // flag exists for: the manager is expected to correct the budget + // narrative in place rather than send the whole thing back. It also + // matches this node's own revise loop, which assumes the record is + // reworkable. The console must show "in approval · editable" here and + // keep inline editing live; on `exec_review` it must show the lock. + lockRecord: false, // ADR-0044: at most two send-backs; the third auto-rejects. maxRevisions: 2, }, @@ -212,6 +220,8 @@ export const BudgetApprovalFlow = defineFlow({ config: { approvers: [{ type: 'position', value: 'exec' }], behavior: 'unanimous', + // Locked, unlike `manager_review` — a multi-approver sign-off must + // decide on a stable record, so edits are refused until it completes. lockRecord: true, }, }, diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index a56a547a80..fc1fb46ea7 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -169,6 +169,40 @@ describe('ApprovalService (node era)', () => { expect(JSON.parse(raw.node_config_json)).toMatchObject({ behavior: 'first_response', lockRecord: true }); }); + // ── record-lock policy on the read row (objectui#2902) ────────── + // + // The lock is enforced in `lifecycle-hooks.ts` off `node_config_json`, but + // the row projection used to drop the flag entirely — so a client could see + // "a pending request exists" and nothing more, and had to assume every + // pending node locked the record. Chaining nodes with different policies + // made that visibly wrong. These pin the flag onto every read path. + + it('lock_record: true when the node locks (the schema default)', async () => { + const req = await svc.openNodeRequest(openInput(['u9']), CTX); + expect(req.lock_record).toBe(true); + const [listed] = await svc.listRequests({ object: 'opportunity', recordId: 'opp1' }, SYS); + expect(listed.lock_record).toBe(true); + expect((await svc.getRequest(req.id, SYS))!.lock_record).toBe(true); + }); + + it('lock_record: false when the node opts out — the same read the hook honors', async () => { + const req = await svc.openNodeRequest(openInput(['u9'], {}, { lockRecord: false }), CTX); + expect(req.lock_record).toBe(false); + const [listed] = await svc.listRequests({ object: 'opportunity', recordId: 'opp1' }, SYS); + expect(listed.lock_record).toBe(false); + expect((await svc.getRequest(req.id, SYS))!.lock_record).toBe(false); + }); + + it('lock_record: an unset lockRecord reads as locked, matching the hook default', async () => { + // The hook allows the write only on an explicit `=== false`; the flag must + // default the same way or the UI would offer an edit the server rejects. + const req = await svc.openNodeRequest( + { ...openInput(['u9']), config: { approvers: [{ type: 'user' as const, value: 'u9' }], behavior: 'first_response' as const } } as any, + CTX, + ); + expect(req.lock_record).toBe(true); + }); + it('openNodeRequest: deduplicates a pending request per (object, record)', async () => { await svc.openNodeRequest(openInput(['u9']), CTX); await expect(svc.openNodeRequest(openInput(['u9'], { runId: 'run_2' }), CTX)) diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 3732d5d3f8..ac428046ba 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -263,6 +263,14 @@ function rowFromRequest(row: any): ApprovalRequestRow { sla_due_at: slaDueAt(row.created_at, cfg), // ADR-0044 revision round (rides the config snapshot; absent ⇒ round 1). round: typeof cfg?.__round === 'number' ? cfg.__round : undefined, + // objectui#2902: the node's record-lock policy. The lock is enforced + // server-side in `lifecycle-hooks.ts` off THIS SAME snapshot with the + // same `!== false` default, so the flag a client renders and the rule the + // server applies can never drift. Without it a console can only see + // "a pending request exists" and has to assume the record is locked — + // which mislabels every `lockRecord: false` node as locked and hides an + // edit the server would have accepted. + lock_record: cfg?.lockRecord !== false, // #3447 P2: the node's author-declared decision outputs, surfaced so a // decision UI can render input fields for them and POST `outputs` on // approve/reject. Per-request (each node declares its own), which is why diff --git a/packages/spec/src/contracts/approval-service.ts b/packages/spec/src/contracts/approval-service.ts index 40670696ef..778e74fa7f 100644 --- a/packages/spec/src/contracts/approval-service.ts +++ b/packages/spec/src/contracts/approval-service.ts @@ -139,6 +139,24 @@ export interface ApprovalRequestRow { * `node_config_json` snapshot (`__round`), so no schema migration. */ round?: number; + /** + * Whether THIS node's pending request locks the target record from edits + * (objectui#2902). Mirrors the `lockRecord` policy the record-lock + * `beforeUpdate` hook enforces, read from the same `node_config_json` + * snapshot the hook reads — so a client never has to guess, and never + * disagrees with the server. + * + * `lockRecord` defaults to `true` (see `ApprovalNodeConfigSchema`), so this + * is `false` only when the node explicitly opted out. Always present on a + * service read; a client that gets `undefined` is talking to a pre-#3814 + * backend and should fail closed (assume locked) rather than offer an edit + * the server will reject with `RECORD_LOCKED`. + * + * Node-scoped, not request-scoped in spirit: a flow chaining several + * approval nodes with different policies produces one request per node, and + * each carries its own value. + */ + lock_record?: boolean; /** * Server-computed decision aggregation progress (#3266, single-request reads * of PENDING requests only). Present when the node's behavior aggregates