diff --git a/.changeset/record-page-declared-approval-actions-3055.md b/.changeset/record-page-declared-approval-actions-3055.md new file mode 100644 index 0000000000..045571c809 --- /dev/null +++ b/.changeset/record-page-declared-approval-actions-3055.md @@ -0,0 +1,16 @@ +--- +'@object-ui/app-shell': patch +--- + +Run `sys_approval_request`'s server-declared decision actions on the business record page, and retire the hard-coded two-button approval path (objectui#3055). + +A record with an approval pending on it showed exactly two buttons — Approve and Reject — hand-written into the record header behind a bespoke `type:'approval'` handler and a client-side approver test. The approvals list, looking at the same request over the same nine REST routes, offered five decisions plus the submitter's levers and took decision attachments. On a business record, **reassign / send back / request info had no entry point at all**, a decision could not carry a file, and the copy on the two surfaces was maintained separately. + +The record page now renders the object's own declared actions through the shared declared-action bar — the same metadata, the same action runtime, the same param dialogs the approvals list uses. Approve, Reject, Reassign, Send back and Request info reach a business record, with their declared params (comment, attachments, the new approver picker) and the per-request decision outputs an approval node declares. Remind stays with the approvals panel, which owns a richer, throttle-aware version of it. Adding a tenth decision action is now a metadata change with no console work. + +Two behaviour changes come with it: + +- **Who sees a decision is the server's answer, not the console's.** Visibility was `pending_approvers.includes(currentUserId)` evaluated in the browser; it is now each action's declared `visible` predicate over the server-computed `viewer` block (`can_act` / `is_submitter` / `can_override`) — the same block that gates the approvals list, computed by the same service that authorizes the decision. A platform or tenant admin's override levers, the recovery path for a request routed to an unstaffed position, now reach the record page for the first time. On a backend too old to send `viewer`, the predicate cannot be evaluated and no decision is offered rather than one whose precondition is unknown. +- **A declared `visible` written against the canonical `record.` root now evaluates.** The declared-action bar passed the row in as the bare predicate scope, so only the shorthand spelling (`status == "pending"`) resolved; `record.viewer.can_act` raised `record is not defined`, and the fail-closed gate turned that into "hidden". Every declared action on `sys_approval_request` gates on `record.viewer.*`, so the whole server-declared decision set was invisible on every surface this bar renders, the approvals inbox included. The row now binds the three ways the record header and list rows bind it — `record.status`, bare `status`, `data.status` — so both spellings reach a verdict. + +`useRecordApprovals` keeps only its read half (status, `lock_record`, the request rows). Its `canDecide` / `approve` / `reject` members and its `currentUserId` parameter are gone: deciding is the declared action's POST, and every remaining question about the viewer is answered on the row by the server. diff --git a/packages/app-shell/src/hooks/useRecordApprovals.decisionOutputs.test.tsx b/packages/app-shell/src/hooks/useRecordApprovals.decisionOutputs.test.tsx deleted file mode 100644 index d06a9b4573..0000000000 --- a/packages/app-shell/src/hooks/useRecordApprovals.decisionOutputs.test.tsx +++ /dev/null @@ -1,106 +0,0 @@ -/** - * ObjectUI - * Copyright (c) 2024-present ObjectStack Inc. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -/** - * What the record header actually POSTs when an approver decides - * (objectui#2955). - * - * The hook used to send `{ actorId, comment }` and nothing else, so an approval - * node that declared `decisionOutputs` lost them on this surface: the flow - * resumed with `vars..` missing, and the next node's `expression` - * approver either faulted (`EXPRESSION_FAILED`) or fell through to - * `onEmptyApprovers`. Nothing surfaced to the approver. - */ - -import { describe, it, expect, beforeEach, vi } from 'vitest'; -import { renderHook, waitFor, act } from '@testing-library/react'; -import { useRecordApprovals } from './useRecordApprovals'; - -const PENDING = { - id: 'req_1', - process_name: 'flow:showcase_dynamic_approval', - object_name: 'showcase_announcement', - record_id: 'a1', - status: 'pending', - pending_approvers: ['user_1'], - decision_output_defs: [ - { key: 'parallel_positions', label: '并行会审岗位', type: 'position', multiple: true }, - ], -}; - -/** POST bodies captured per decide call. */ -let posts: Array<{ url: string; body: any }>; - -beforeEach(() => { - posts = []; - vi.stubGlobal( - 'fetch', - vi.fn(async (url: string, init?: RequestInit) => { - if (init?.method === 'POST') { - posts.push({ url: String(url), body: JSON.parse(String(init.body)) }); - return { ok: true, json: async () => ({ request: { ...PENDING, status: 'approved' } }) } as any; - } - return { ok: true, json: async () => ({ data: [PENDING] }) } as any; - }), - ); -}); - -const mount = async () => { - const view = renderHook(() => useRecordApprovals('showcase_announcement', 'a1', 'user_1')); - await waitFor(() => expect(view.result.current.canDecide).toBe(true)); - return view; -}; - -describe('useRecordApprovals — decision outputs in the decide body (objectui#2955)', () => { - it('posts the collected outputs under the nested `outputs` key', async () => { - const { result } = await mount(); - await act(async () => { - await result.current.approve({ - comment: 'ok', - outputs: { parallel_positions: ['pos_1', 'pos_2'] }, - }); - }); - expect(posts).toHaveLength(1); - expect(posts[0].url).toContain('/approvals/requests/req_1/approve'); - expect(posts[0].body).toEqual({ - actorId: 'user_1', - comment: 'ok', - outputs: { parallel_positions: ['pos_1', 'pos_2'] }, - }); - }); - - it('rejects with outputs too — a send-back can route the next round as well', async () => { - const { result } = await mount(); - await act(async () => { - await result.current.reject({ outputs: { parallel_positions: ['pos_1'] } }); - }); - expect(posts[0].url).toContain('/approvals/requests/req_1/reject'); - expect(posts[0].body).toEqual({ - actorId: 'user_1', - outputs: { parallel_positions: ['pos_1'] }, - }); - }); - - it('omits `outputs` entirely when the node declares none', async () => { - // The unchanged body for every approval without `decisionOutputs` — the - // service must not start seeing an empty object where nothing was sent. - const { result } = await mount(); - await act(async () => { - await result.current.approve({ comment: 'ok', outputs: {} }); - }); - expect(posts[0].body).toEqual({ actorId: 'user_1', comment: 'ok' }); - expect('outputs' in posts[0].body).toBe(false); - }); - - it('surfaces the node declaration so the header can build its pickers', async () => { - const { result } = await mount(); - expect(result.current.pendingRequest?.decision_output_defs).toEqual( - PENDING.decision_output_defs, - ); - }); -}); diff --git a/packages/app-shell/src/hooks/useRecordApprovals.quorum.test.tsx b/packages/app-shell/src/hooks/useRecordApprovals.quorum.test.tsx index 3750b16e04..2fc2ac4b3f 100644 --- a/packages/app-shell/src/hooks/useRecordApprovals.quorum.test.tsx +++ b/packages/app-shell/src/hooks/useRecordApprovals.quorum.test.tsx @@ -94,7 +94,7 @@ function stubApi(detail: unknown, opts: { detailStatus?: number } = {}) { } const mount = () => - renderHook(() => useRecordApprovals('showcase_expense_report', 'AyG40_bAHSP_gi8T', 'u_manager')); + renderHook(() => useRecordApprovals('showcase_expense_report', 'AyG40_bAHSP_gi8T')); describe('useRecordApprovals — quorum progress (objectstack#4478)', () => { beforeEach(() => stubApi(DETAIL_ROW)); @@ -144,8 +144,9 @@ describe('useRecordApprovals — quorum progress (objectstack#4478)', () => { const { result } = mount(); await waitFor(() => expect(result.current.pendingRequest).toBeTruthy()); expect(result.current.pendingRequest?.decision_progress).toBeUndefined(); - // …and the decision surface the list read does support is still live. - expect(result.current.canDecide).toBe(true); + // …and everything the LIST read already carried is still live — the row the + // decision actions run against, and the lock the header reads. + expect(result.current.pendingRequest?.id).toBe('req_committee_1'); expect(result.current.pendingRequest?.lock_record).toBe(true); }); diff --git a/packages/app-shell/src/hooks/useRecordApprovals.ts b/packages/app-shell/src/hooks/useRecordApprovals.ts index 11fb889a71..26322f1271 100644 --- a/packages/app-shell/src/hooks/useRecordApprovals.ts +++ b/packages/app-shell/src/hooks/useRecordApprovals.ts @@ -1,16 +1,25 @@ /** * useRecordApprovals * - * Resolves the approval state for a single record so the detail-view header - * can surface a status badge and — when the current user is a pending - * approver — "Approve" / "Reject" actions. + * Resolves the approval state for a single record — the READ half only: the + * status badge, the record lock (`lock_record`), and the request rows the + * record page's approvals panel renders. * * Since ADR-0019 an approval is a **flow node** (`type: 'approval'`), not a * standalone process: the flow opens the request when it reaches the node, - * and a decision resumes the run down its `approve` / `reject` edge. There is - * therefore no manual "submit" or "recall" from the record header — those - * endpoints were removed. This hook reads the record's requests and lets a - * pending approver record a decision. + * and a decision resumes the run down its `approve` / `reject` edge. + * + * ⛔ This hook does NOT decide (objectui#3055). It used to own a second, + * hand-written decision path — an `approve()` / `reject()` pair plus a + * CLIENT-side `canDecide` (`pending_approvers.includes(currentUserId)`) — that + * the record page injected as two hard-coded header buttons. That fork covered + * two of the nine approval routes: reassign / send-back / request-info had zero + * entry point on a business record, decision attachments were impossible, and + * the copy was maintained separately from the approvals list's. Decisions now + * go through the SAME server-declared `sys_approval_request` actions the + * approvals list runs (`DeclaredActionsBar`), gated by the server-computed + * `viewer` block rather than by a second client-side opinion — so a new + * decision action is metadata, not console code. * * Talks directly to the framework REST endpoints under * `/api/v1/approvals/*`. Fails open: if the approvals plugin is not installed @@ -182,27 +191,20 @@ interface UseRecordApprovalsResult { * node the flow has reached (and per ADR-0044 revision round), so a * multi-level flow accumulates several. The record page's approval panel * renders them all (objectui#3461); `pendingRequest` / `latestRequest` - * remain the derived single-row reads the header actions consume. + * remain the derived single-row reads the status badge and the decision + * bar's record consume. */ requests: ApprovalRequestLite[]; + /** + * The one still-`pending` request, enriched by `getRequest` — so it carries + * the server-computed `viewer` block the declared decision actions gate on + * (objectui#3055) as well as the `decision_progress` tally. + */ pendingRequest: ApprovalRequestLite | null; latestRequest: ApprovalRequestLite | null; - /** The current user is among the pending approvers and may record a decision. */ - canDecide: boolean; - approve: (input?: DecisionInput) => Promise; - reject: (input?: DecisionInput) => Promise; refresh: () => Promise; } -/** - * What an approver submits with a decision: the free-text comment, plus the - * node's declared decision outputs keyed by their declared `key` (objectui#2955). - */ -export interface DecisionInput { - comment?: string; - outputs?: Record; -} - function apiBase() { const url = (import.meta as any).env?.VITE_SERVER_URL || ''; return `${String(url).replace(/\/$/, '')}/api/v1`; @@ -290,10 +292,17 @@ export async function remindApprovalRequest( return out ?? {}; } +/** + * ⛔ No `currentUserId` parameter (objectui#3055). It existed only to feed the + * retired client-side `canDecide`; every remaining question about the viewer — + * may they act, are they the submitter, may they override — is answered by the + * server on the row itself (`viewer`, framework#3310 / #3424). A surface that + * still needs the signed-in id for a pre-`viewer` fallback (the panel's remind) + * reads it from `useAuth()` where it renders. + */ export function useRecordApprovals( objectName: string | undefined, recordId: string | undefined, - currentUserId?: string | null, ): UseRecordApprovalsResult { const [loading, setLoading] = useState(false); const [available, setAvailable] = useState(true); @@ -351,36 +360,11 @@ export function useRecordApprovals( const latestRequest = sortedRequests[0] ?? null; - const canDecide = !!pendingRequest && !!currentUserId - && (pendingRequest.pending_approvers ?? []).includes(currentUserId); - - const decide = useCallback( - async (decision: 'approve' | 'reject', input?: DecisionInput) => { - if (!pendingRequest) throw new Error('No pending request'); - const outputs = input?.outputs && Object.keys(input.outputs).length > 0 ? input.outputs : undefined; - const out = await fetchJson<{ request?: ApprovalRequestLite }>( - `/approvals/requests/${encodeURIComponent(pendingRequest.id)}/${decision}`, - { - method: 'POST', - body: JSON.stringify({ - ...(currentUserId ? { actorId: currentUserId } : {}), - ...(input?.comment ? { comment: input.comment } : {}), - // The node's declared decision outputs, under the same nested key - // the Approval Center's `type:'api'` decide actions post - // (objectui#2955). Omitted entirely when nothing was collected, so - // a node without `decisionOutputs` posts the body it always did. - ...(outputs ? { outputs } : {}), - }), - }, - ); - await refresh(); - return out?.request; - }, - [pendingRequest, currentUserId, refresh], - ); - - const approve = useCallback((input?: DecisionInput) => decide('approve', input), [decide]); - const reject = useCallback((input?: DecisionInput) => decide('reject', input), [decide]); + // ⛔ No `canDecide` / `approve` / `reject` here (objectui#3055). Whether the + // viewer may act is the SERVER's answer (`pendingRequest.viewer`), read by + // the declared actions' own `visible` gate; recording the decision is the + // declared `type:'api'` action's POST. A client-side second opinion on the + // same question is what let the record page and the approvals list disagree. return { loading, @@ -388,9 +372,6 @@ export function useRecordApprovals( requests: sortedRequests, pendingRequest, latestRequest, - canDecide, - approve, - reject, refresh, }; } diff --git a/packages/app-shell/src/views/DeclaredActionsBar.tsx b/packages/app-shell/src/views/DeclaredActionsBar.tsx index 2390958fd8..0d17522754 100644 --- a/packages/app-shell/src/views/DeclaredActionsBar.tsx +++ b/packages/app-shell/src/views/DeclaredActionsBar.tsx @@ -43,7 +43,16 @@ import { useObjectLabel, useObjectTranslation } from '@object-ui/i18n'; import { Loader2 } from 'lucide-react'; import { useConsoleActionRuntime } from '../hooks/useConsoleActionRuntime'; import { useAdapter } from '../providers/AdapterProvider'; -import { useMetadataItem } from '../providers/MetadataProvider'; +// Straight from `@object-ui/react`, NOT through `../providers/MetadataProvider` +// (which merely re-exports it). The provider module pulls in the console +// metadata client factory, and that module builds its shared authenticated +// fetch AT IMPORT TIME — so importing the hook by the convenient path drags an +// eager side effect into the module graph of every host that renders this bar. +// It surfaced when the record page started mounting the bar (objectui#3055): +// two RecordDetailView suites died at import with `Cannot access +// 'authFetchSpy' before initialization`, the side effect running inside the +// hoisted `@object-ui/auth` mock factory before the spy existed. +import { useMetadataItem } from '@object-ui/react'; import { decisionOutputDefs, decisionOutputParams } from '../utils/decisionOutputParams'; import { getIcon } from '../utils/getIcon'; @@ -113,10 +122,38 @@ const DeclaredActionButton: React.FC<{ const { t } = useObjectTranslation(); const recordData = record != null && typeof record === 'object' ? (record as Record) : {}; + /** + * The predicate scope, with the record bound the THREE ways the platform's + * row surfaces bind it (objectui#3055). + * + * The bar used to hand the row in as the bare context bag, so only the + * shorthand spelling — `status == "pending"` — resolved. The CANONICAL + * spelling is the `record.` root: it is what `ExpressionEvaluator`'s CEL path + * binds (`bag.record` as the record namespace), what `evalRowPredicate` binds + * on the record header and on list rows, and what the server itself + * enforces with. Under a root-only bag `record.viewer.can_act` does not read + * as false — it throws `record is not defined`, and `throwOnError` turns that + * into "hidden". + * + * Which is not hypothetical: EVERY declared action on `sys_approval_request` + * gates on `record.viewer.*` (framework#3310 / #3424), so the whole + * server-declared decision set was invisible on every surface this bar + * renders. The record page's two hand-written buttons were, in practice, the + * only decision UI that could still be reached — the fork objectui#3055 is + * about, kept alive by the "full" path being unable to evaluate its own gate. + * + * `record` / `data` are written AFTER the spread, so a row that happens to + * carry a field of either name cannot shadow the namespace a predicate means. + */ + const predicateRecord = useMemo( + () => ({ ...recordData, record: recordData, data: recordData }), + // eslint-disable-next-line react-hooks/exhaustive-deps + [record], + ); // `visible` fails CLOSED on a throwing predicate — mirrors action:button and // ActionEngine.getActionsForLocation: a guard that can't be evaluated hides // the action rather than exposing one whose precondition is broken. - const isVisible = useCondition(toPredicateInput((action as any).visible), recordData, { + const isVisible = useCondition(toPredicateInput((action as any).visible), predicateRecord, { throwOnError: true, label: `declared action "${action.name ?? action.label ?? 'action'}" (visible)`, }); @@ -125,7 +162,7 @@ const DeclaredActionButton: React.FC<{ // this bar ignored it, so a spec-authored `disabled` guard on a declared // action did nothing here. (No legacy `enabled` fallback: server-declared // actions are spec-shaped and never carried the non-spec key.) - const isDisabledPred = useCondition(toPredicateInput((action as any).disabled), recordData); + const isDisabledPred = useCondition(toPredicateInput((action as any).disabled), predicateRecord); const handleClick = useCallback(async () => { if (loading) return; diff --git a/packages/app-shell/src/views/RecordDetailView.approvalDecisionActions.test.tsx b/packages/app-shell/src/views/RecordDetailView.approvalDecisionActions.test.tsx deleted file mode 100644 index d57210d87b..0000000000 --- a/packages/app-shell/src/views/RecordDetailView.approvalDecisionActions.test.tsx +++ /dev/null @@ -1,144 +0,0 @@ -/** - * ObjectUI - * Copyright (c) 2024-present ObjectStack Inc. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -/** - * The record header's Approve / Reject param contract (objectui#2955). - * - * Two things were broken on this surface and both are invisible from the - * outside — the buttons rendered and the decision went through: - * - * 1. the collected inputs shipped under `collectParams`, a key NOTHING in the - * codebase reads (`ActionRunner` collects from `actionParams`, or `params` - * when it is an array). No dialog ever opened, so the approver's comment - * was dropped on every record-page decision — dead since ADR-0019; - * 2. a node declaring `decisionOutputs` got no inputs at all here, so the - * decision resumed the flow with `vars..` missing and the next - * node's `expression` approver failed or fell through to `onEmptyApprovers` - * — with no hint to the approver that anything was skipped. - */ - -import { describe, it, expect } from 'vitest'; -import { buildApprovalDecisionActions } from './RecordDetailView'; - -/** Object-translation stub: echoes the key, honouring `defaultValue`. */ -const t = (key: string, opts?: any) => (opts?.defaultValue ?? `t:${key}`) as string; - -const pending = (over: Record = {}) => ({ - id: 'req_1', - status: 'pending', - ...over, -}); - -const byName = (actions: any[]) => Object.fromEntries(actions.map((a) => [a.name, a])); - -describe('buildApprovalDecisionActions — param key (objectui#2955)', () => { - it('collects through `actionParams`, the key the runner reads', () => { - const actions = byName(buildApprovalDecisionActions(pending(), t) as any[]); - for (const name of ['approve_request', 'reject_request']) { - expect(Array.isArray(actions[name].actionParams)).toBe(true); - // The dead key must not come back — it is why no dialog ever opened. - expect(actions[name].collectParams).toBeUndefined(); - } - }); - - it('always offers the comment box, as a long-form input', () => { - const [approve] = buildApprovalDecisionActions(pending(), t) as any[]; - // `multiline` does not survive param resolution — the type has to carry it. - expect(approve.actionParams[0]).toMatchObject({ name: 'comment', type: 'textarea' }); - }); - - it('keeps the decision buttons ahead of app header actions', () => { - const actions = byName(buildApprovalDecisionActions(pending(), t) as any[]); - expect(actions.approve_request.order).toBe(-100); - expect(actions.reject_request.order).toBe(-99); - }); -}); - -describe('buildApprovalDecisionActions — one dialog per decision (objectui#3126)', () => { - it('never carries `confirmText` — the param dialog is the confirmation', () => { - // The runner chains confirm THEN param collection, so `confirmText` + - // `actionParams` queued TWO dialogs: after "Continue" nothing was sent - // until a second, unexpected comment dialog was also confirmed — the - // #3126 "reject silently does nothing" report. One decision, one dialog. - const actions = buildApprovalDecisionActions(pending(), t) as any[]; - for (const a of actions) expect(a.confirmText).toBeUndefined(); - }); - - it('keeps the reject confirm question as the dialog description', () => { - const actions = byName(buildApprovalDecisionActions(pending(), t) as any[]); - expect(actions.reject_request.description).toBe('Reject this approval request?'); - }); -}); - -describe('buildApprovalDecisionActions — decision outputs (objectui#2955)', () => { - it('adds one typed param per declared output', () => { - const actions = byName( - buildApprovalDecisionActions( - pending({ - decision_outputs: ['parallel_positions'], - decision_output_defs: [ - { key: 'parallel_positions', label: '并行会审岗位', type: 'position', multiple: true }, - ], - }), - t, - ) as any[], - ); - for (const name of ['approve_request', 'reject_request']) { - const params = actions[name].actionParams as any[]; - expect(params.map((p) => p.name)).toEqual(['comment', 'outputs.parallel_positions']); - expect(params[1]).toMatchObject({ - type: 'lookup', - // The spec spelling — `referenceTo` is dropped by `resolveActionParams` - // and the picker would degrade to a record-id text box. - reference: 'sys_position', - multiple: true, - label: '并行会审岗位', - }); - } - }); - - it('collects nothing extra when the node declares no outputs', () => { - const [approve] = buildApprovalDecisionActions(pending(), t) as any[]; - expect((approve.actionParams as any[]).map((p) => p.name)).toEqual(['comment']); - }); - - it('still collects from a backend that only sends the bare key list', () => { - const [approve] = buildApprovalDecisionActions( - pending({ decision_outputs: ['next_reviewers'] }), - t, - ) as any[]; - expect((approve.actionParams as any[])[1]).toMatchObject({ - name: 'outputs.next_reviewers', - type: 'text', - }); - }); - - it('requires a `required` output to approve, but never to reject', () => { - const actions = byName( - buildApprovalDecisionActions( - pending({ - decision_output_defs: [ - { key: 'parallel_positions', type: 'position', multiple: true, required: true }, - ], - }), - t, - ) as any[], - ); - const paramOf = (name: string) => - (actions[name].actionParams as any[]).find((p) => p.name === 'outputs.parallel_positions'); - expect(paramOf('approve_request').required).toBe(true); - // Mirrors the server, which enforces `required` on approve only — a reject - // blocked on routing data the reject edge never reads would trap it. - expect(paramOf('reject_request').required).toBe(false); - }); - - it('collects nothing extra when there is no pending request', () => { - const [approve] = buildApprovalDecisionActions(null, t) as any[]; - expect((approve.actionParams as any[]).map((p) => p.name)).toEqual(['comment']); - }); -}); diff --git a/packages/app-shell/src/views/RecordDetailView.approvalDeclaredActions.test.tsx b/packages/app-shell/src/views/RecordDetailView.approvalDeclaredActions.test.tsx new file mode 100644 index 0000000000..5d703af2bf --- /dev/null +++ b/packages/app-shell/src/views/RecordDetailView.approvalDeclaredActions.test.tsx @@ -0,0 +1,491 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * The business record page runs `sys_approval_request`'s OWN declared decision + * actions (objectui#3055). + * + * Before this, the record page spliced two hand-written buttons into its header + * — approve and reject, nothing else — behind a bespoke `type:'approval'` + * handler and a CLIENT-side approver test. The approvals list, looking at the + * SAME request over the SAME nine REST routes, offered five decisions plus the + * submitter's levers, took decision attachments, and gated on the server's own + * `viewer` block. On a business record, reassign / send back / request info had + * no entry point at all and a decision could not carry a file. + * + * These tests mount the real record page and assert the surface an approver + * standing on a business record actually gets. The action set is NOT restated + * as an expectation of console code — it is read from the object definition, + * which is the whole point: a ninth decision action must land here by shipping + * metadata, with no console change. + * + * Gating is asserted through the server-computed `viewer` block + * (framework#3310 / #3424) rather than any client-side membership test — the + * record page must not hold a second opinion about who may act, because that + * is exactly how it drifted from the approvals list. + */ + +import * as React from 'react'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, screen, waitFor, fireEvent, cleanup } from '@testing-library/react'; +import { MemoryRouter } from 'react-router-dom'; + +const authFetchSpy = vi.fn(async () => + new Response(JSON.stringify({ data: {} }), { + status: 200, + headers: { 'content-type': 'application/json' }, + }), +); +vi.mock('@object-ui/auth', () => ({ + useAuth: () => ({ user: { id: 'u_qcdir', name: 'QC Director', image: null }, activeOrganization: null }), + createAuthenticatedFetch: () => authFetchSpy, +})); + +vi.mock('@object-ui/collaboration', () => ({ + useRecordPresence: () => [], + PresenceAvatars: () => null, +})); + +vi.mock('sonner', () => ({ + toast: Object.assign(vi.fn(), { + success: vi.fn(), + error: vi.fn(), + info: vi.fn(), + warning: vi.fn(), + loading: vi.fn(), + dismiss: vi.fn(), + }), +})); + +// Auto-answering dialog stubs. The runner chains confirm → param collection → +// dispatch, and both steps are `await`ed, so a null-rendering stub would park +// every action forever. These resolve immediately, which keeps the file about +// WHICH actions the page offers and WHERE their dispatch lands — the dialogs +// themselves are covered by ActionParamDialog's own suites. +vi.mock('./ActionConfirmDialog', () => ({ + ActionConfirmDialog: ({ state }: any) => { + React.useEffect(() => { + if (state?.open) state.resolve?.(true); + }, [state]); + return null; + }, +})); +vi.mock('./ActionParamDialog', () => ({ + ActionParamDialog: ({ state }: any) => { + React.useEffect(() => { + if (state?.open) state.resolve?.({ comment: 'looks good', 'outputs.next_reviewer': 'u_plant_mgr' }); + }, [state]); + return null; + }, +})); +vi.mock('./ActionResultDialog', () => ({ ActionResultDialog: () => null })); +vi.mock('./FlowRunner', () => ({ FlowRunner: () => null })); +vi.mock('./MetadataInspector', () => ({ + MetadataPanel: () => null, + useMetadataInspector: () => ({ showDebug: false, toggle: () => {} }), +})); +vi.mock('../hooks/useActionModal', () => ({ + useActionModal: () => ({ + modalHandler: vi.fn(async () => ({ success: true })), + modalElement: null, + closeModal: () => {}, + resolveModalTarget: vi.fn(async () => null), + }), +})); +vi.mock('../utils/consoleServerAction', () => ({ + createConsoleServerActionHandler: () => vi.fn(async () => ({ success: true })), +})); + +// The page body is orthogonal — the decision bar is rendered by the view +// itself, not through the schema tree. +vi.mock('@object-ui/react', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, SchemaRenderer: () => null }; +}); + +import { MetadataCtx } from '@object-ui/react'; +import { RecordDetailView } from './RecordDetailView'; +import { + SYS_APPROVAL_REQUEST_OBJECT, + RECORD_APPROVAL_ACTION_LOCATION, + RECORD_APPROVAL_EXCLUDED_ACTIONS, +} from './recordApprovalActions'; + +const OBJECT_NAME = 'qif_report'; +const RECORD_ID = 'QIF202607310002'; +const REQUEST_ID = 'req_qif_1'; + +/** + * `sys_approval_request`'s declared actions, mirroring the framework's + * `packages/plugins/plugin-approvals/src/sys-approval-request.object.ts` + * (objectui#2678 P2-4). Copied rather than imported because the console does + * not depend on the plugin package — the console's contract is "render what the + * object declares", and this fixture is the shape it must render. + */ +const SYS_APPROVAL_REQUEST_DEF = { + name: SYS_APPROVAL_REQUEST_OBJECT, + label: 'Approval Request', + fields: { + id: { type: 'text', label: 'Id' }, + status: { type: 'text', label: 'Status' }, + submitter_id: { type: 'lookup', label: 'Submitter', reference_to: 'sys_user' }, + }, + actions: [ + { + name: 'approval_approve', label: 'Approve', variant: 'primary', type: 'api', method: 'POST', + target: '/api/v1/approvals/requests/{id}/approve', + params: [ + { name: 'comment', label: 'Comment', type: 'textarea', required: false }, + { name: 'attachments', label: 'Attachments', type: 'file', multiple: true, required: false }, + ], + visible: 'record.viewer.can_act || record.viewer.can_override', + locations: ['record_section', 'list_item'], + successMessage: 'Approved.', refreshAfter: true, + }, + { + name: 'approval_reject', label: 'Reject', variant: 'danger', type: 'api', method: 'POST', + target: '/api/v1/approvals/requests/{id}/reject', + params: [ + { name: 'comment', label: 'Comment', type: 'textarea', required: false }, + { name: 'attachments', label: 'Attachments', type: 'file', multiple: true, required: false }, + ], + visible: 'record.viewer.can_act || record.viewer.can_override', + confirmText: 'Reject this request? A rejection is final for every approver.', + locations: ['record_section', 'list_item'], + successMessage: 'Rejected.', refreshAfter: true, + }, + { + name: 'approval_reassign', label: 'Reassign', type: 'api', method: 'POST', + target: '/api/v1/approvals/requests/{id}/reassign', + params: [ + { field: 'submitter_id', name: 'to', label: 'New approver', required: true }, + { name: 'comment', label: 'Comment', type: 'textarea', required: false }, + ], + visible: 'record.viewer.can_act || record.viewer.can_override', + locations: ['record_section'], + successMessage: 'Reassigned.', refreshAfter: true, + }, + { + name: 'approval_send_back', label: 'Send back', type: 'api', method: 'POST', + target: '/api/v1/approvals/requests/{id}/revise', + params: [{ name: 'comment', label: 'Reason', type: 'textarea', required: false }], + visible: 'record.viewer.can_act', + locations: ['record_section'], + successMessage: 'Sent back for revision.', refreshAfter: true, + }, + { + name: 'approval_request_info', label: 'Request info', type: 'api', method: 'POST', + target: '/api/v1/approvals/requests/{id}/request-info', + params: [{ name: 'comment', label: 'What do you need?', type: 'textarea', required: true }], + visible: 'record.viewer.can_act', + locations: ['record_section'], + successMessage: 'Information requested.', refreshAfter: true, + }, + { + name: 'approval_remind', label: 'Send reminder', type: 'api', method: 'POST', + target: '/api/v1/approvals/requests/{id}/remind', + params: [{ name: 'comment', label: 'Note', type: 'textarea', required: false }], + visible: 'record.status == "pending" && record.viewer.is_submitter', + locations: ['record_section'], + successMessage: 'Reminder sent.', refreshAfter: true, + }, + { + name: 'approval_recall', label: 'Recall', type: 'api', method: 'POST', + target: '/api/v1/approvals/requests/{id}/recall', + params: [{ name: 'comment', label: 'Comment', type: 'textarea', required: false }], + visible: '(record.status == "pending" || record.status == "returned") && record.viewer.is_submitter', + confirmText: 'Recall this request?', + locations: ['record_section'], + successMessage: 'Recalled.', refreshAfter: true, + }, + { + name: 'approval_resubmit', label: 'Resubmit', type: 'api', method: 'POST', + target: '/api/v1/approvals/requests/{id}/resubmit', + params: [{ name: 'comment', label: 'What changed?', type: 'textarea', required: false }], + visible: 'record.status == "returned" && record.viewer.is_submitter', + locations: ['record_section'], + successMessage: 'Resubmitted.', refreshAfter: true, + }, + ], +}; + +/** The five decisions an APPROVER gets — the acceptance set of objectui#3055. */ +const APPROVER_DECISIONS = [ + 'approval_approve', + 'approval_reject', + 'approval_reassign', + 'approval_send_back', + 'approval_request_info', +]; + +const OBJECTS = [ + { + name: OBJECT_NAME, + label: 'QIF Report', + fields: { + id: { type: 'text', label: 'Id' }, + name: { type: 'text', label: 'Name' }, + }, + }, +]; + +/** + * The pending request as the server sends it. `pending_approvers` deliberately + * carries NO entry equal to the signed-in user id: the slot is keyed by the + * group-routed identity, which is precisely the shape the record page's old + * client-side `includes(currentUserId)` could not match. `viewer` is the + * server's own answer to the same question and is what the actions read. + */ +const pendingRequest = (viewer: Record | undefined) => ({ + id: REQUEST_ID, + process_name: 'flow:qif_quality_review', + object_name: OBJECT_NAME, + record_id: RECORD_ID, + status: 'pending', + submitter_id: 'u_inspector', + current_step: 'qc_director_review', + pending_approvers: ['position:qc_director'], + lock_record: true, + ...(viewer ? { viewer } : {}), +}); + +let approvalsFetch: ReturnType; + +function stubApprovalsApi(row: Record) { + approvalsFetch = vi.fn(async (url: string) => { + const u = String(url); + if (u.includes(`/approvals/requests/${REQUEST_ID}/actions`)) { + return { ok: true, json: async () => ({ data: [] }) } as any; + } + if (u.endsWith(`/approvals/requests/${REQUEST_ID}`)) { + // `getRequest` — the read that attaches the `viewer` block. + return { ok: true, json: async () => row } as any; + } + if (u.includes('/approvals/requests?object=')) { + return { ok: true, json: async () => ({ data: [row] }) } as any; + } + return { ok: true, json: async () => ({ data: [] }) } as any; + }); + vi.stubGlobal('fetch', approvalsFetch); +} + +const METADATA = { + objects: [...OBJECTS, SYS_APPROVAL_REQUEST_DEF], + pages: [], + loading: false, + error: null, + refresh: async () => {}, + invalidate: () => {}, + ensureType: async () => [], + getItem: async (type: string, name: string) => + (type === 'object' && name === SYS_APPROVAL_REQUEST_OBJECT ? SYS_APPROVAL_REQUEST_DEF : null), + getItemsByType: () => [], +} as any; + +function makeDataSource() { + return { + find: vi.fn(async () => ({ data: [] })), + findOne: vi.fn(async () => ({ id: RECORD_ID, name: 'Incoming batch 0731' })), + create: vi.fn(async () => ({})), + update: vi.fn(async () => ({})), + delete: vi.fn(async () => ({})), + } as any; +} + +function renderRecordPage() { + return render( + + + {}} + objectNameOverride={OBJECT_NAME} + recordIdOverride={RECORD_ID} + embedded + /> + + , + ); +} + +const decisionButton = (name: string) => screen.queryByTestId(`declared-action-${name}`); + +beforeEach(() => { + cleanup(); + authFetchSpy.mockClear(); +}); + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +describe('record page decision actions — a GROUP approver (objectui#3055)', () => { + /** + * The acceptance case: the signed-in user holds the slot through a position, + * so their bare id is nowhere in `pending_approvers` — the server says + * `can_act`, and that is the only opinion the page is entitled to have. + */ + it('offers all five declared decisions on the business record page', async () => { + stubApprovalsApi(pendingRequest({ can_act: true, is_submitter: false, can_override: false })); + renderRecordPage(); + + await waitFor(() => expect(decisionButton('approval_approve')).toBeTruthy()); + for (const name of APPROVER_DECISIONS) { + expect(decisionButton(name), `${name} must be offered to a pending approver`).toBeTruthy(); + } + }); + + it('offers reassign / send back / request info — the three that had NO entry point', async () => { + stubApprovalsApi(pendingRequest({ can_act: true, is_submitter: false, can_override: false })); + renderRecordPage(); + + await waitFor(() => expect(decisionButton('approval_reassign')).toBeTruthy()); + expect(decisionButton('approval_send_back')).toBeTruthy(); + expect(decisionButton('approval_request_info')).toBeTruthy(); + }); + + it('carries the decision attachments param the hand-written buttons could not', async () => { + stubApprovalsApi(pendingRequest({ can_act: true, is_submitter: false, can_override: false })); + renderRecordPage(); + await waitFor(() => expect(decisionButton('approval_approve')).toBeTruthy()); + + // Read from the declaration the page renders — the console never restates + // the param set, which is why it cannot drift from the approvals list. + const approve = SYS_APPROVAL_REQUEST_DEF.actions.find((a) => a.name === 'approval_approve')!; + expect(approve.params).toEqual( + expect.arrayContaining([expect.objectContaining({ name: 'attachments', type: 'file', multiple: true })]), + ); + }); + + it('hides the submitter-only levers from an approver who is not the submitter', async () => { + stubApprovalsApi(pendingRequest({ can_act: true, is_submitter: false, can_override: false })); + renderRecordPage(); + await waitFor(() => expect(decisionButton('approval_approve')).toBeTruthy()); + + expect(decisionButton('approval_recall')).toBeNull(); + expect(decisionButton('approval_resubmit')).toBeNull(); + }); + + it('POSTs to the REQUEST, not to the business record it is opened on', async () => { + stubApprovalsApi(pendingRequest({ can_act: true, is_submitter: false, can_override: false })); + renderRecordPage(); + await waitFor(() => expect(decisionButton('approval_send_back')).toBeTruthy()); + authFetchSpy.mockClear(); + + fireEvent.click(decisionButton('approval_send_back')!); + await waitFor(() => expect(authFetchSpy).toHaveBeenCalled()); + + const [url, init] = authFetchSpy.mock.calls[0] as unknown as [string, RequestInit]; + // `{id}` resolves from the dispatch record — the request row. Resolving it + // from the host page's record would have aimed the decision at + // `/approvals/requests/QIF202607310002/revise`. + expect(url).toContain(`/api/v1/approvals/requests/${REQUEST_ID}/revise`); + expect(url).not.toContain(RECORD_ID); + expect(url).not.toContain('%7B'); + expect(init.method).toBe('POST'); + }); + + it('folds a declared decision output into the nested `outputs` body', async () => { + stubApprovalsApi(pendingRequest({ can_act: true, is_submitter: false, can_override: false })); + renderRecordPage(); + await waitFor(() => expect(decisionButton('approval_send_back')).toBeTruthy()); + authFetchSpy.mockClear(); + + fireEvent.click(decisionButton('approval_send_back')!); + await waitFor(() => expect(authFetchSpy).toHaveBeenCalled()); + + const [, init] = authFetchSpy.mock.calls[0] as unknown as [string, RequestInit]; + const body = JSON.parse(String(init.body)); + // The dotted `outputs.` param the bar synthesizes per request must + // arrive nested — that is the shape the decide routes read back as + // `vars..` (objectui#2955 / framework#3447). + expect(body).toMatchObject({ comment: 'looks good', outputs: { next_reviewer: 'u_plant_mgr' } }); + expect(body['outputs.next_reviewer']).toBeUndefined(); + }); +}); + +describe('record page decision actions — gating is the SERVER\'s (objectui#3055)', () => { + it('shows nothing to a viewer the server says cannot act', async () => { + stubApprovalsApi(pendingRequest({ can_act: false, is_submitter: false, can_override: false })); + renderRecordPage(); + // Let the approvals read settle before asserting an absence. + await waitFor(() => expect(approvalsFetch).toHaveBeenCalled()); + + for (const name of [...APPROVER_DECISIONS, 'approval_recall', 'approval_resubmit']) { + expect(decisionButton(name), `${name} must stay hidden`).toBeNull(); + } + }); + + it('gives an override admin the three rescue levers and not the secondary two', async () => { + // framework#3424: a request routed to an unstaffed position is otherwise + // undecidable — a platform/tenant admin may approve, reject or reassign it. + // Send back / request info are approver-only and stay hidden, exactly as on + // the approvals list, because both surfaces read the same declaration. + stubApprovalsApi(pendingRequest({ can_act: false, is_submitter: false, can_override: true })); + renderRecordPage(); + + await waitFor(() => expect(decisionButton('approval_approve')).toBeTruthy()); + expect(decisionButton('approval_reject')).toBeTruthy(); + expect(decisionButton('approval_reassign')).toBeTruthy(); + expect(decisionButton('approval_send_back')).toBeNull(); + expect(decisionButton('approval_request_info')).toBeNull(); + }); + + it('fails CLOSED on a backend too old to send a `viewer` block', async () => { + // Pre-framework#3310. The predicate cannot be evaluated, so no decision is + // offered — never a button whose precondition is unknown. + stubApprovalsApi(pendingRequest(undefined)); + renderRecordPage(); + await waitFor(() => expect(approvalsFetch).toHaveBeenCalled()); + + for (const name of APPROVER_DECISIONS) { + expect(decisionButton(name), `${name} must fail closed without a viewer block`).toBeNull(); + } + }); +}); + +describe('record page decision actions — the submitter (objectui#3055)', () => { + it('offers recall to the submitter but not the approver decisions', async () => { + stubApprovalsApi(pendingRequest({ can_act: false, is_submitter: true, can_override: false })); + renderRecordPage(); + + await waitFor(() => expect(decisionButton('approval_recall')).toBeTruthy()); + expect(decisionButton('approval_approve')).toBeNull(); + expect(decisionButton('approval_reject')).toBeNull(); + }); + + it('leaves remind to the approvals panel, which already owns a richer one', async () => { + stubApprovalsApi(pendingRequest({ can_act: false, is_submitter: true, can_override: false })); + renderRecordPage(); + await waitFor(() => expect(decisionButton('approval_recall')).toBeTruthy()); + + expect(RECORD_APPROVAL_EXCLUDED_ACTIONS).toContain('approval_remind'); + expect(decisionButton('approval_remind')).toBeNull(); + }); +}); + +describe('record page decision actions — no bar without a pending request', () => { + it('renders no decision chrome when the record has no requests', async () => { + approvalsFetch = vi.fn(async () => ({ ok: true, json: async () => ({ data: [] }) }) as any); + vi.stubGlobal('fetch', approvalsFetch); + renderRecordPage(); + await waitFor(() => expect(approvalsFetch).toHaveBeenCalled()); + + expect(decisionButton('approval_approve')).toBeNull(); + expect(screen.queryByRole('toolbar')).toBeNull(); + }); + + it('reads the location the metadata declares, not a host-specific one', () => { + // The record page consumes `record_section` verbatim; every decision action + // on `sys_approval_request` declares it, so none is dropped in translation. + for (const action of SYS_APPROVAL_REQUEST_DEF.actions) { + expect(action.locations).toContain(RECORD_APPROVAL_ACTION_LOCATION); + } + }); +}); diff --git a/packages/app-shell/src/views/RecordDetailView.headerApiInterpolation.test.tsx b/packages/app-shell/src/views/RecordDetailView.headerApiInterpolation.test.tsx index 6fe342fe7d..e320bb39d2 100644 --- a/packages/app-shell/src/views/RecordDetailView.headerApiInterpolation.test.tsx +++ b/packages/app-shell/src/views/RecordDetailView.headerApiInterpolation.test.tsx @@ -83,11 +83,14 @@ vi.mock('../utils/consoleServerAction', () => ({ })); // Capture BOTH the handler set and the context each receives -// while KEEPING the real provider. The record page's own set is the only one -// carrying `approval`; the context capture lets the mount wait until the -// handlers were built AGAINST THE LOADED RECORD (apiHandler closes over -// `pageRecord`, so grabbing an earlier capture would test the null-record -// closure instead). +// while KEEPING the real provider. The record page's own provider is the only +// one whose CONTEXT carries this page's record — the discriminator used to be +// "the only handler set carrying `approval`", which objectui#3055 retired when +// the record page stopped registering a bespoke approval handler (decisions are +// ordinary declared `type:'api'` actions now). The context capture also lets +// the mount wait until the handlers were built AGAINST THE LOADED RECORD +// (apiHandler closes over `pageRecord`, so grabbing an earlier capture would +// test the null-record closure instead). const captured: Array<{ handlers: Record Promise>; context: any }> = []; vi.mock('@object-ui/react', async (importOriginal) => { const actual = await importOriginal(); @@ -161,7 +164,7 @@ function renderDetail() { function recordPageCapture() { return [...captured] .reverse() - .find((c) => c.handlers && 'approval' in c.handlers && c.context?.record?.id === RECORD_ID); + .find((c) => c.handlers && c.context?.record?.id === RECORD_ID); } /** Render the view and hand back its ActionProvider handlers, spies cleared. */ diff --git a/packages/app-shell/src/views/RecordDetailView.modalDispatch.test.tsx b/packages/app-shell/src/views/RecordDetailView.modalDispatch.test.tsx index 6b9edf3e34..2d9c696549 100644 --- a/packages/app-shell/src/views/RecordDetailView.modalDispatch.test.tsx +++ b/packages/app-shell/src/views/RecordDetailView.modalDispatch.test.tsx @@ -97,17 +97,20 @@ vi.mock('../utils/consoleServerAction', () => ({ // Capture the handler set each receives while KEEPING the // real provider (children still get a working action context). The record -// page's own set is the only one carrying `approval`, so the capture below -// selects it even if a nested surface mounts a provider of its own. The page -// body itself is orthogonal here — SchemaRenderer is stubbed so the file -// stays about the wiring, not the render tree. -const capturedHandlers: Array Promise>> = []; +// page's own provider is the only one whose CONTEXT carries this page's +// record, so the capture below selects it even if a nested surface mounts a +// provider of its own (the approval decision bar mounts one — objectui#3055, +// which is also why the old "the only set carrying `approval`" discriminator +// is gone: the record page no longer registers a bespoke approval handler). +// The page body itself is orthogonal here — SchemaRenderer is stubbed so the +// file stays about the wiring, not the render tree. +const captured: Array<{ handlers: Record Promise>; context: any }> = []; vi.mock('@object-ui/react', async (importOriginal) => { const actual = await importOriginal(); return { ...actual, ActionProvider: (props: any) => { - capturedHandlers.push(props.handlers); + captured.push({ handlers: props.handlers, context: props.context }); return React.createElement(actual.ActionProvider as any, props); }, SchemaRenderer: () => null, @@ -170,9 +173,11 @@ function renderDetail() { ); } -/** The record page's OWN handler set — the only provider carrying `approval`. */ +/** The record page's OWN handler set — the only provider given this record. */ function recordPageHandlers() { - return [...capturedHandlers].reverse().find((h) => h && 'approval' in h); + return [...captured] + .reverse() + .find((c) => c.handlers && c.context?.record?.id === RECORD_ID)?.handlers; } /** Render the view and hand back its ActionProvider handlers, spies cleared. */ @@ -190,7 +195,7 @@ async function mountAndCaptureHandlers() { beforeEach(() => { cleanup(); - capturedHandlers.length = 0; + captured.length = 0; authFetchSpy.mockClear(); serverActionSpy.mockClear(); modalHandlerSpy.mockClear(); diff --git a/packages/app-shell/src/views/RecordDetailView.tsx b/packages/app-shell/src/views/RecordDetailView.tsx index 0be091d303..35609bf272 100644 --- a/packages/app-shell/src/views/RecordDetailView.tsx +++ b/packages/app-shell/src/views/RecordDetailView.tsx @@ -33,7 +33,6 @@ import { RelatedRecordActionsBridge } from './RelatedRecordActionsBridge'; import { withPageTabsUrlSync } from '../utils/pageTabsUrlSync'; import { RECORD_DETAIL_TAB_PARAM, RECORD_TRAIL_PARAM, decodeRecordTrail, buildRecordTrailHref } from '../urlParams'; import { resolveActionParams } from '../utils/resolveActionParams'; -import { decisionOutputDefs, decisionOutputParams, foldDecisionOutputs } from '../utils/decisionOutputParams'; import { createConsoleServerActionHandler } from '../utils/consoleServerAction'; import { interpretFlowResponse } from '../utils/flowResponse'; import { useRecordBreadcrumbTitle } from '../context/NavigationContext'; @@ -46,6 +45,12 @@ import type { ActionDef, ActionParamDef } from '@object-ui/core'; import { useRecordApprovals, recordLockedByApproval } from '../hooks/useRecordApprovals'; import { RecordAttachmentsPanel } from './RecordAttachmentsPanel'; import { RecordApprovalsPanel } from './RecordApprovalsPanel'; +import { DeclaredActionsBar } from './DeclaredActionsBar'; +import { + SYS_APPROVAL_REQUEST_OBJECT, + RECORD_APPROVAL_ACTION_LOCATION, + RECORD_APPROVAL_EXCLUDED_ACTIONS, +} from './recordApprovalActions'; // Side-effect registration of `record:approvals` — synthesized record pages // reference the node whenever the record has approval requests (#3461), so // the type must resolve wherever this view renders, not only under hosts @@ -194,91 +199,6 @@ export function resolveRecordHeaderActionGates( return { edit: affordances.edit, delete: affordances.delete }; } -/** - * The record header's Approve / Reject actions for the pending request. - * - * Pure and exported so the param contract is testable without the detail - * render tree — that contract is where objectui#2955 bit: - * - * • the collected inputs must ride **`actionParams`**, the key `ActionRunner` - * reads (the header dispatches the action object verbatim). They shipped as - * `collectParams`, which nothing in the codebase consumes, so no dialog ever - * opened: the comment was silently dropped on every record-page decision, - * and the node's declared decision outputs with it; - * • a node that declares `decisionOutputs` contributes one `outputs.` - * param each, through the same helper the Approval Center uses — so the - * approver gets the same typed picker on both surfaces instead of the - * record page quietly deciding without them. - * - * `t` is the object-translation function; params are localized here because - * synthesized `outputs.` names can never match an `_actions.*` bundle key. - */ -export function buildApprovalDecisionActions( - pendingRequest: unknown, - t: (key: string, opts?: any) => string, -): ActionDef[] { - const commentParam = { - name: 'comment', - label: t('approvals.comment', { defaultValue: 'Comment (optional)' }), - // `textarea`, not `text` + `multiline`: the param resolver rebuilds inline - // params from a fixed key list and drops `multiline`, so the long-form - // intent has to ride the type. - type: 'textarea', - }; - // `required` outputs are enforced on approve only — the server rejects a - // blank one there and never on reject, so the two dialogs differ in exactly - // that flag and nothing else. - const defs = decisionOutputDefs(pendingRequest); - const decisionParams = (decision: 'approve' | 'reject') => [ - commentParam, - ...decisionOutputParams(defs, (key: string) => t(key), { decision }), - ]; - // A pending approval is THE decision the approver came to make, so the - // decision buttons must outrank app `record_header` actions rather than being - // appended after them (and buried in overflow). A strongly negative `order` - // floats them into the primary slot; the action:bar stable-sorts by `order`, - // so app actions keep their relative order just after the decision. Approve - // gets the highlighted `primary` variant; Reject stays `destructive`. - // (#2670 / objectui#2339) - return [ - { - name: 'approve_request', - type: 'approval', - target: 'approve_request', - label: t('approvals.approve', { defaultValue: 'Approve' }), - icon: 'check', - variant: 'primary', - order: -100, - locations: ['record_header'], - refreshAfter: true, - actionParams: decisionParams('approve'), - successMessage: t('approvals.approveSuccess', { defaultValue: 'Approved' }), - }, - { - name: 'reject_request', - type: 'approval', - target: 'reject_request', - label: t('approvals.reject', { defaultValue: 'Reject' }), - icon: 'x', - variant: 'destructive', - order: -99, - locations: ['record_header'], - refreshAfter: true, - // NO `confirmText` here (objectui#3126). The runner chains confirm THEN - // param collection, so carrying both queued two dialogs: the approver - // answered "Reject this approval request? → Continue" and the request - // still didn't fire — it waited on a second, unexpected comment dialog - // (opened since #2961 made `actionParams` live on this surface), which - // reads as a silent no-op. The param dialog IS the confirmation: it is - // titled by this label, carries the confirm question as its description, - // and nothing is sent until its own Confirm. - description: t('approvals.rejectConfirm', { defaultValue: 'Reject this approval request?' }), - actionParams: decisionParams('reject'), - successMessage: t('approvals.rejectSuccess', { defaultValue: 'Rejected' }), - }, - ] as unknown as ActionDef[]; -} - export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverride, recordIdOverride, embedded }: RecordDetailViewProps) { const params = useParams<{ @@ -913,10 +833,11 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri // ─── Approvals ───────────────────────────────────────────────────── // Since ADR-0019 an approval is a flow node: the flow opens the request, - // there is no manual submit/recall from the record header. When the current - // user is a pending approver, surface "Approve" / "Reject" on the header and - // a status badge whenever a request exists. - const approvals = useRecordApprovals(objectName, pureRecordId, user?.id); + // there is no manual submit/recall from the record header. This read backs + // three things: the status badge / edit lock, the approvals panel (#3461), + // and — since objectui#3055 — the pending `sys_approval_request` row that the + // object's own SERVER-DECLARED decision actions run against. + const approvals = useRecordApprovals(objectName, pureRecordId); // Hold latest approvals snapshot in a ref so the action handler // (memoized once inside ActionRunner) always sees fresh state instead of // the stale closure captured at the first render. @@ -972,29 +893,17 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri // nodes carry none and the band then shows nothing extra. const approvalProgress = approvals.pendingRequest?.decision_progress; - const approvalHandler = useCallback(async (action: ActionDef) => { - const target = action.target || action.name; - const params = (action.params && !Array.isArray(action.params)) - ? (action.params as Record) - : {}; - // The node's declared decision outputs arrive as `outputs.` params - // (objectui#2955) — fold them back into the nested object the decide route - // expects, exactly like the Approval Center's api handler does. - const { outputs } = foldDecisionOutputs(params); - try { - if (target === 'approve_request') { - await approvalsRef.current.approve({ comment: params.comment, outputs }); - } else if (target === 'reject_request') { - await approvalsRef.current.reject({ comment: params.comment, outputs }); - } else { - return { success: false, error: `Unknown approval target: ${target}` }; - } - notifyRecordChanged(); - return { success: true, reload: true }; - } catch (err: any) { - return { success: false, error: err?.message || String(err) }; - } - }, []); + // A decision landed through the declared-action bar (objectui#3055). The + // action itself already POSTed and the runtime already toasted; what the HOST + // owes is a re-read of both halves it renders — the approval state (status, + // pending row, `viewer` block, tally) and the record itself, whose + // engine-written mirrors (`approval_status`, stage fields) change with the + // decision. `refreshAfter: true` on every declared decision action is what + // calls this. + const handleApprovalActionDone = useCallback(() => { + void approvalsRef.current.refresh(); + notifyRecordChanged(); + }, [notifyRecordChanged]); // Discover reverse references: other objects with lookup/master_detail fields // pointing to the current object (e.g., order_item.order → order). @@ -1809,14 +1718,20 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri }), })); - // Inject approval actions — only when the current user is a pending - // approver for this record (ADR-0019: approvals are opened by a flow - // node, so there is no manual submit/recall; an approver records a - // decision that resumes the flow down its approve/reject edge). - if (approvals.available && approvals.canDecide) { - base.push(...buildApprovalDecisionActions(approvals.pendingRequest, t)); - } - + // ⛔ No approval actions are injected here any more (objectui#3055). + // They used to be two hand-written buttons (approve/reject only, no + // attachments, own copy, own CLIENT-side approver test) spliced into this + // header list. They now render from `sys_approval_request`'s OWN declared + // actions through `` below — the same metadata the + // approvals list runs, gated by the same server-computed `viewer` block. + // + // The header list is the wrong carrier for them and cannot be fixed into + // one: `action:bar` evaluates a `visible` predicate against THIS page's + // record (a business record has no `viewer` block) and stamps that same + // record into `params._rowRecord`, so a declared action targeting + // `/api/v1/approvals/requests/{id}/…` would resolve `{id}` to the + // business record's id. The request row has to be the dispatch record, + // which is exactly what the bar does. return base; })(); @@ -1902,12 +1817,12 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri }), }; // eslint-disable-next-line react-hooks/exhaustive-deps - // `approvals.pendingRequest` is in the deps for its `decision_output_defs`: - // the header's decision params are synthesized from the pending node's - // declaration, so they must be rebuilt when the request (and therefore the - // node) changes (objectui#2955). `approvals.requests` rides along for the - // Approvals tab payload (#3461). - }, [objectDef?.name, childRelations, t, objectLabel, objects, historyEnabled, historyEntries, historyLoading, approvals.available, approvals.canDecide, approvals.pendingRequest, approvals.requests, user?.id]); + // `approvals.requests` / `approvals.pendingRequest` are in the deps for the + // Approvals tab payload (#3461) — the tab's node carries the live rows, and + // the panel's headline is the pending one. (The decision actions no longer + // ride this list at all — objectui#3055 moved them to the declared-action + // bar, which reads the pending row directly.) + }, [objectDef?.name, childRelations, t, objectLabel, objects, historyEnabled, historyEntries, historyLoading, approvals.available, approvals.pendingRequest, approvals.requests, user?.id]); if (isLoading) { return ; @@ -2168,6 +2083,10 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri onToggleReaction={handleToggleReaction as any} mentionSuggestions={mentionSuggestions} > + {/* No `approval` handler in the set below (objectui#3055): the record + page has no bespoke approval action TYPE any more. A decision is an + ordinary `type:'api'` action declared on `sys_approval_request` and + run by the shared runtime the decision bar mounts. */}
@@ -2189,6 +2108,39 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri {originFrom.label} )} + {/* The pending approval's DECISION ACTIONS (objectui#3055). + `sys_approval_request` declares them as object metadata — + approve / reject (with decision attachments) / reassign / + send back / request info, plus the submitter's recall and + resubmit — and this is the same bar, the same runtime and the + same server-computed `visible` gate the approvals list runs + them through. The record page adds no per-action code, so a + ninth decision action ships as metadata alone. + + The dispatch record is the REQUEST row, not the business + record: that is what makes `{id}` resolve to the request and + what gives each action's `visible` the server's `viewer` + block (`can_act` / `is_submitter` / `can_override`) to read. + Placed above the page body rather than in the header for that + reason — see the header-actions comment above — and above the + fold, because a pending decision is why the approver opened + the record. + + `approval_remind` is excluded: the approvals panel already + renders a richer remind (throttle-aware copy, timeline + refresh), and `exclude` is how a host keeps one of an + object's declared actions in its own UI without the bar + double-rendering it. */} + {approvals.available && approvals.pendingRequest && ( + + )} = ( const selfRecordId = hostApprovals ? undefined : ctx?.recordId != null ? String(ctx.recordId) : undefined; - const selfFetched = useRecordApprovals(selfObjectName, selfRecordId, user?.id); + const selfFetched = useRecordApprovals(selfObjectName, selfRecordId); const approvals = hostApprovals ?? selfFetched; const currentUserId = schema?.currentUserId ?? user?.id; diff --git a/packages/app-shell/src/views/recordApprovalActions.ts b/packages/app-shell/src/views/recordApprovalActions.ts new file mode 100644 index 0000000000..f6aabe078e --- /dev/null +++ b/packages/app-shell/src/views/recordApprovalActions.ts @@ -0,0 +1,62 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * How a business record page surfaces the decision actions of the approval + * request that is pending on it (objectui#3055). + * + * There is no action list here on purpose. The actions ARE + * `sys_approval_request`'s own declared metadata — approve / reject (both with + * a comment and decision attachments), reassign, send back for revision, + * request info, and the submitter's remind / recall / resubmit — resolved from + * the object definition at render time and executed by the shared console + * action runtime through ``. What this module pins is only + * the three coordinates that wiring needs, so they are stated once and can be + * asserted without mounting the whole record page. + * + * Before this, the record page hand-wrote two buttons (approve / reject only) + * against a bespoke `type:'approval'` handler, with its own copy and its own + * CLIENT-side approver test. The result was a record page that could not + * reassign, could not send back, could not request info, could not attach a + * file to a decision, and phrased everything differently from the approvals + * list — for the same request, over the same nine REST routes. + */ + +/** + * The object whose declared actions render on the host record page. The + * dispatch record is the pending REQUEST row (not the business record), which + * is what resolves `{id}` in each action's target and what gives each action's + * `visible` predicate the server-computed `viewer` block to read. + */ +export const SYS_APPROVAL_REQUEST_OBJECT = 'sys_approval_request'; + +/** + * The declared `locations` entry the record page renders. + * + * `sys_approval_request` places its decision levers at `record_section` (the + * per-record action group) and the two headline decisions additionally at + * `list_item`. The record page consumes `record_section` verbatim rather than + * re-mapping it onto the host's own `record_header` / `record_more` slots: + * those two are the HOST object's locations, evaluated against the HOST + * record, and routing another object's actions through them would need a + * translation layer on every one of them. Reading the location the metadata + * already declares keeps "add a decision action" a metadata-only change. + */ +export const RECORD_APPROVAL_ACTION_LOCATION = 'record_section'; + +/** + * Declared actions the record page renders ITSELF, so the bar must not + * double-render them. + * + * `approval_remind` only: `RecordApprovalsPanel` already offers remind with + * throttle-aware copy (`THROTTLED` / 429 → "a reminder was sent recently") + * and reloads the action timeline in place, which a generic POST + toast + * cannot do. Every other declared action — including the submitter's recall + * and resubmit — comes from the bar. + */ +export const RECORD_APPROVAL_EXCLUDED_ACTIONS = ['approval_remind'] as const;