Skip to content

Commit 6937572

Browse files
baozhoutaoclaude
andauthored
fix(approvals): decision outputs reach both decision surfaces (#2955) (#2961)
* fix(approvals): decision outputs reach both decision surfaces (#2955) An approval node's `decisionOutputs` (framework#3447 P2) never made it to the approver on either Console surface, though the server has shipped the typed declaration on the request row all along. The Approval Center DID synthesize a typed picker per declared output (#2831), but spelled the picker target `referenceTo`. Every collected param passes through `resolveActionParams()` first, and its inline branch rebuilds the param from a fixed key list, reading the target from `reference` — so the target was dropped and `paramToField()` degraded the targetless picker to a plain text input labelled "<label> 的记录 ID". `user` outputs were unaffected (that widget needs no target); `department`/`position`/`team` were all broken. The record header collected nothing at all: its Approve/Reject shipped their inputs under `collectParams`, a key nothing reads (ActionRunner collects from `actionParams`). No dialog had opened there since ADR-0019 — the comment was silently dropped on every record-page decision, and a node declaring `decisionOutputs` resumed the flow with `vars.<node>.<key>` missing, so the next node's `expression` approver faulted or fell through to `onEmptyApprovers` with no hint to anyone. The widget mapping now lives in `utils/decisionOutputParams` so the two surfaces cannot drift again; the header collects through `actionParams`, folds `outputs.<key>` into the decide body, and its comment box is a real textarea (the resolver drops `multiline`). Tests pin the round trip through param resolution — the stage that actually broke — not just the emitted shape. Verified end to end against the app-showcase runtime: an approval node declaring a `position`-typed multi-select renders a sys_position picker in both the Approval Center drawer and the record header, and deciding from the record page carries the outputs through to `vars.*` — the co-sign node's `expression` approver resolved to the user picked in the dialog. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(approvals): block a decision on a required output (#2955) The platform half of this issue gives `decisionOutputs` a `required` flag: the server rejects an approve that carries no value for one, before any write, so the run can never resume past the node with the key a downstream `expression` approver reads still missing. The console mirrors it where the approver actually is. A required output is marked required on the param it synthesizes, so the dialog flags the empty field and refuses Confirm instead of sending a decision the server will 400. APPROVE only, matching the server: a reject leaves down the reject edge where nothing reads the outputs, so the reject dialog offers the same fields without blocking on them — the two dialogs now differ in exactly that flag. Both decision surfaces get it from the one shared helper, and a backend that predates the field requires nothing, which is the behaviour of the previous commit unchanged. Verified against the showcase runtime (its dynamic-approval flow declares the flag): the record header's Approve refuses with "Next Reviewers 为必填项" and sends no request, its Reject offers the field unmarked, the Approval Center drawer agrees on both, and a filled approve routes the co-sign node to the picked user. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent e651c93 commit 6937572

9 files changed

Lines changed: 865 additions & 92 deletions
Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
---
2+
"@object-ui/app-shell": minor
3+
---
4+
5+
fix(approvals): decision outputs reach both decision surfaces (objectui#2955, framework#3447 P2)
6+
7+
An approval node can ask the approver for structured data with their decision
8+
(`decisionOutputs`) — typically to route the next node's approvers, which the
9+
flow then reads as `vars.<nodeId>.<key>`. The server has shipped this since
10+
framework#3447 P2 and surfaces the typed declaration on the request row
11+
(`decision_output_defs`), but neither Console decision surface actually
12+
delivered it.
13+
14+
**The Approval Center asked for a record id instead of showing a picker.** The
15+
typed pickers landed in objectui#2831 and the drawer really did synthesize a
16+
`lookup` param per declared output — but it spelled the picker target
17+
`referenceTo`, and `resolveActionParams()` (which every collected param passes
18+
through before the dialog renders it) rebuilds an inline param from a fixed key
19+
list, reading the target from `reference`. The target was dropped there, and
20+
`paramToField()` degrades a targetless picker to a plain text input — so a
21+
`position` output rendered as a box labelled "<label> 的记录 ID". The approver
22+
had to go find the record id somewhere else and paste it back. `user`-typed
23+
outputs were unaffected (that widget needs no target), which is why this
24+
survived: `department` / `position` / `team` were the broken three.
25+
26+
**The record header decided without collecting anything at all.** Approve /
27+
Reject on the detail page shipped their inputs under `collectParams` — a key
28+
nothing in the codebase reads (`ActionRunner` collects from `actionParams`).
29+
No dialog had opened on that surface since the ADR-0019 rework: the approver's
30+
comment was silently dropped on every record-page decision, and a node
31+
declaring `decisionOutputs` got no inputs either, so the flow resumed with
32+
`vars.<node>.<key>` missing — the next node's `expression` approver then failed
33+
with `EXPRESSION_FAILED`, or fell through to `onEmptyApprovers`, with nothing
34+
surfaced to the approver or the flow author. The header now collects through
35+
`actionParams`, renders the node's declared outputs with the same pickers the
36+
Approval Center uses, and posts them under `outputs` on the decide call. The
37+
comment box works again as a side effect, and it is a real textarea (the param
38+
resolver drops `multiline`, so the intent has to ride the type).
39+
40+
The widget mapping now lives in one place (`utils/decisionOutputParams`), so
41+
the two surfaces cannot drift apart again, and the round trip through param
42+
resolution — the stage that actually broke — is pinned by tests.
43+
44+
**And a `required` output is now enforced at the field.** The spec grew
45+
`decisionOutputs[].required` (the platform half of this issue, shipping in
46+
`@objectstack/spec` + `@objectstack/plugin-approvals`) — the server rejects an
47+
approve carrying no value for one, before any write. The dialog marks those params
48+
required, so the approver is stopped at the empty field with the Confirm button
49+
refusing rather than by a 400 after the round trip. Only on APPROVE: the server
50+
never requires them on a reject (the run leaves down the reject edge, where
51+
nothing reads the outputs), so the two dialogs differ in exactly that flag. On a
52+
backend that predates the field nothing is required, which is the behavior above
53+
unchanged.
Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
/**
2+
* ObjectUI
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* This source code is licensed under the MIT license found in the
6+
* LICENSE file in the root directory of this source tree.
7+
*/
8+
9+
/**
10+
* What the record header actually POSTs when an approver decides
11+
* (objectui#2955).
12+
*
13+
* The hook used to send `{ actorId, comment }` and nothing else, so an approval
14+
* node that declared `decisionOutputs` lost them on this surface: the flow
15+
* resumed with `vars.<node>.<key>` missing, and the next node's `expression`
16+
* approver either faulted (`EXPRESSION_FAILED`) or fell through to
17+
* `onEmptyApprovers`. Nothing surfaced to the approver.
18+
*/
19+
20+
import { describe, it, expect, beforeEach, vi } from 'vitest';
21+
import { renderHook, waitFor, act } from '@testing-library/react';
22+
import { useRecordApprovals } from './useRecordApprovals';
23+
24+
const PENDING = {
25+
id: 'req_1',
26+
process_name: 'flow:showcase_dynamic_approval',
27+
object_name: 'showcase_announcement',
28+
record_id: 'a1',
29+
status: 'pending',
30+
pending_approvers: ['user_1'],
31+
decision_output_defs: [
32+
{ key: 'parallel_positions', label: '并行会审岗位', type: 'position', multiple: true },
33+
],
34+
};
35+
36+
/** POST bodies captured per decide call. */
37+
let posts: Array<{ url: string; body: any }>;
38+
39+
beforeEach(() => {
40+
posts = [];
41+
vi.stubGlobal(
42+
'fetch',
43+
vi.fn(async (url: string, init?: RequestInit) => {
44+
if (init?.method === 'POST') {
45+
posts.push({ url: String(url), body: JSON.parse(String(init.body)) });
46+
return { ok: true, json: async () => ({ request: { ...PENDING, status: 'approved' } }) } as any;
47+
}
48+
return { ok: true, json: async () => ({ data: [PENDING] }) } as any;
49+
}),
50+
);
51+
});
52+
53+
const mount = async () => {
54+
const view = renderHook(() => useRecordApprovals('showcase_announcement', 'a1', 'user_1'));
55+
await waitFor(() => expect(view.result.current.canDecide).toBe(true));
56+
return view;
57+
};
58+
59+
describe('useRecordApprovals — decision outputs in the decide body (objectui#2955)', () => {
60+
it('posts the collected outputs under the nested `outputs` key', async () => {
61+
const { result } = await mount();
62+
await act(async () => {
63+
await result.current.approve({
64+
comment: 'ok',
65+
outputs: { parallel_positions: ['pos_1', 'pos_2'] },
66+
});
67+
});
68+
expect(posts).toHaveLength(1);
69+
expect(posts[0].url).toContain('/approvals/requests/req_1/approve');
70+
expect(posts[0].body).toEqual({
71+
actorId: 'user_1',
72+
comment: 'ok',
73+
outputs: { parallel_positions: ['pos_1', 'pos_2'] },
74+
});
75+
});
76+
77+
it('rejects with outputs too — a send-back can route the next round as well', async () => {
78+
const { result } = await mount();
79+
await act(async () => {
80+
await result.current.reject({ outputs: { parallel_positions: ['pos_1'] } });
81+
});
82+
expect(posts[0].url).toContain('/approvals/requests/req_1/reject');
83+
expect(posts[0].body).toEqual({
84+
actorId: 'user_1',
85+
outputs: { parallel_positions: ['pos_1'] },
86+
});
87+
});
88+
89+
it('omits `outputs` entirely when the node declares none', async () => {
90+
// The unchanged body for every approval without `decisionOutputs` — the
91+
// service must not start seeing an empty object where nothing was sent.
92+
const { result } = await mount();
93+
await act(async () => {
94+
await result.current.approve({ comment: 'ok', outputs: {} });
95+
});
96+
expect(posts[0].body).toEqual({ actorId: 'user_1', comment: 'ok' });
97+
expect('outputs' in posts[0].body).toBe(false);
98+
});
99+
100+
it('surfaces the node declaration so the header can build its pickers', async () => {
101+
const { result } = await mount();
102+
expect(result.current.pendingRequest?.decision_output_defs).toEqual(
103+
PENDING.decision_output_defs,
104+
);
105+
});
106+
});

packages/app-shell/src/hooks/useRecordApprovals.ts

Lines changed: 34 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
import { useCallback, useEffect, useMemo, useRef, useState } from 'react';
2222
import { bearerAuthHeaders } from '../utils/authToken';
23+
import type { DecisionOutputDef } from '../utils/decisionOutputParams';
2324

2425
export interface ApprovalRequestLite {
2526
id: string;
@@ -44,6 +45,19 @@ export interface ApprovalRequestLite {
4445
* have allowed. See {@link recordLockedByApproval}.
4546
*/
4647
lock_record?: boolean;
48+
/**
49+
* Structured data THIS pending node asks the approver to submit with their
50+
* decision (framework#3447 P2) — the flow reads it back as
51+
* `vars.<nodeId>.<key>`, typically to route the next node's approvers.
52+
*
53+
* `decision_output_defs` carries the typed declaration (`type` /`multiple`),
54+
* `decision_outputs` the bare key list a pre-typed backend sends; both are
55+
* absent on a backend that predates the feature. The record header collects
56+
* them exactly like the Approval Center does — a decision that silently
57+
* skipped them left the next node reading a missing key (objectui#2955).
58+
*/
59+
decision_outputs?: string[] | null;
60+
decision_output_defs?: DecisionOutputDef[] | null;
4761
}
4862

4963
/**
@@ -71,11 +85,20 @@ interface UseRecordApprovalsResult {
7185
latestRequest: ApprovalRequestLite | null;
7286
/** The current user is among the pending approvers and may record a decision. */
7387
canDecide: boolean;
74-
approve: (input?: { comment?: string }) => Promise<ApprovalRequestLite | undefined>;
75-
reject: (input?: { comment?: string }) => Promise<ApprovalRequestLite | undefined>;
88+
approve: (input?: DecisionInput) => Promise<ApprovalRequestLite | undefined>;
89+
reject: (input?: DecisionInput) => Promise<ApprovalRequestLite | undefined>;
7690
refresh: () => Promise<void>;
7791
}
7892

93+
/**
94+
* What an approver submits with a decision: the free-text comment, plus the
95+
* node's declared decision outputs keyed by their declared `key` (objectui#2955).
96+
*/
97+
export interface DecisionInput {
98+
comment?: string;
99+
outputs?: Record<string, any>;
100+
}
101+
79102
function apiBase() {
80103
const url = (import.meta as any).env?.VITE_SERVER_URL || '';
81104
return `${String(url).replace(/\/$/, '')}/api/v1`;
@@ -162,15 +185,21 @@ export function useRecordApprovals(
162185
&& (pendingRequest.pending_approvers ?? []).includes(currentUserId);
163186

164187
const decide = useCallback(
165-
async (decision: 'approve' | 'reject', input?: { comment?: string }) => {
188+
async (decision: 'approve' | 'reject', input?: DecisionInput) => {
166189
if (!pendingRequest) throw new Error('No pending request');
190+
const outputs = input?.outputs && Object.keys(input.outputs).length > 0 ? input.outputs : undefined;
167191
const out = await fetchJson<{ request?: ApprovalRequestLite }>(
168192
`/approvals/requests/${encodeURIComponent(pendingRequest.id)}/${decision}`,
169193
{
170194
method: 'POST',
171195
body: JSON.stringify({
172196
...(currentUserId ? { actorId: currentUserId } : {}),
173197
...(input?.comment ? { comment: input.comment } : {}),
198+
// The node's declared decision outputs, under the same nested key
199+
// the Approval Center's `type:'api'` decide actions post
200+
// (objectui#2955). Omitted entirely when nothing was collected, so
201+
// a node without `decisionOutputs` posts the body it always did.
202+
...(outputs ? { outputs } : {}),
174203
}),
175204
},
176205
);
@@ -180,8 +209,8 @@ export function useRecordApprovals(
180209
[pendingRequest, currentUserId, refresh],
181210
);
182211

183-
const approve = useCallback((input?: { comment?: string }) => decide('approve', input), [decide]);
184-
const reject = useCallback((input?: { comment?: string }) => decide('reject', input), [decide]);
212+
const approve = useCallback((input?: DecisionInput) => decide('approve', input), [decide]);
213+
const reject = useCallback((input?: DecisionInput) => decide('reject', input), [decide]);
185214

186215
return {
187216
loading,

0 commit comments

Comments
 (0)