Skip to content

Commit ce4112b

Browse files
baozhoutaoclaude
andcommitted
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>
1 parent 38ca8be commit ce4112b

9 files changed

Lines changed: 761 additions & 91 deletions
Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
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+
Not fixed here: `DecisionOutputDef` has no `required`, so a flow author still
45+
cannot demand that an approver fill an output before approving. That needs the
46+
spec-side field first (framework), and `onEmptyApprovers` remains the only
47+
backstop until then.
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)