From c801f4be48986a68bed922a34723146120db021a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=8C=85=E5=91=A8=E6=B6=9B?= Date: Tue, 28 Jul 2026 02:17:52 -0700 Subject: [PATCH] fix(detail): the approval band honors the node's `lockRecord` instead of assuming every approval locks (#2902) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The detail page treated "a pending approval request exists" as "this record is locked". An approval node declares `lockRecord` (default `true`), and on `lockRecord: false` the server keeps accepting writes while that node waits — so the console asserted a lock the backend did not enforce. The label was the smaller half. The same conflated signal fed `canEdit`, so the record-level inline-edit session was suppressed too: no pencils, `enter()` a no-op. On a single-approver step — the case `lockRecord: false` exists for, where the approver is meant to fill in the missing detail before deciding — the capability was unreachable from the UI. And a flow chaining nodes with different policies drew one identical band for "edit freely" and "your save dies with RECORD_LOCKED", so the states were indistinguishable until Save failed. Approval state is now two signals: `approvalPending` (an approval is running — drives the band and recall, both meaningful either way) and `locked` (it also forbids edits, from the pending node's `lock_record`). The band renders amber lock + "Locked for approval" or sky clock + "In approval · editable", each with its own tooltip; recall left the locked branch, since an editable pending approval is just as recallable. `InlineEditProvider`'s new `approvalPending` prop defaults to `locked`, so a host threading only `locked` is unchanged. The `approval_status` fallback has no node granularity and still reads as locked, as does a pending request from a backend too old to report the policy — failing closed beats offering an edit the server rejects. Needs framework#3814 for `lock_record` on the request row. Closes #2902 Co-Authored-By: Claude Opus 5 --- .../approval-band-honors-lock-record.md | 45 +++++++++++ .../hooks/useRecordApprovals.locking.test.ts | 61 ++++++++++++++ .../app-shell/src/hooks/useRecordApprovals.ts | 30 +++++++ .../app-shell/src/views/RecordDetailView.tsx | 46 ++++++++--- packages/i18n/src/locales/ar.ts | 2 + packages/i18n/src/locales/de.ts | 2 + packages/i18n/src/locales/en.ts | 2 + packages/i18n/src/locales/es.ts | 2 + packages/i18n/src/locales/fr.ts | 2 + packages/i18n/src/locales/ja.ts | 2 + packages/i18n/src/locales/ko.ts | 2 + packages/i18n/src/locales/pt.ts | 2 + packages/i18n/src/locales/ru.ts | 2 + packages/i18n/src/locales/zh.ts | 2 + packages/plugin-detail/src/DetailView.tsx | 81 +++++++++++++------ .../DetailView.approvalBand.test.tsx | 46 ++++++++++- .../src/__tests__/InlineEditContext.test.tsx | 38 +++++++++ .../plugin-detail/src/useDetailTranslation.ts | 4 +- .../react/src/context/InlineEditContext.tsx | 29 ++++++- 19 files changed, 359 insertions(+), 41 deletions(-) create mode 100644 .changeset/approval-band-honors-lock-record.md create mode 100644 packages/app-shell/src/hooks/useRecordApprovals.locking.test.ts diff --git a/.changeset/approval-band-honors-lock-record.md b/.changeset/approval-band-honors-lock-record.md new file mode 100644 index 0000000000..9654610d73 --- /dev/null +++ b/.changeset/approval-band-honors-lock-record.md @@ -0,0 +1,45 @@ +--- +"@object-ui/react": minor +"@object-ui/plugin-detail": minor +"@object-ui/app-shell": minor +"@object-ui/i18n": minor +--- + +fix(detail): the approval band honors the node's `lockRecord` instead of assuming every approval locks (#2902) + +A record detail page treated "a pending approval request exists" as "this +record is locked". An approval node declares `lockRecord` (default `true`), and +on `lockRecord: false` the server keeps accepting writes for the whole time +that node waits — so the console was asserting a lock the backend did not +enforce. + +The label was the smaller half of it. The same conflated signal fed `canEdit`, +so the record-level inline-edit session was suppressed too: no pencils, +`enter()` a no-op. On a single-approver step — a department head or plant +manager, exactly the case `lockRecord: false` exists for, where the approver is +meant to fill in the missing detail before deciding — the capability was +unreachable from the UI. And a flow chaining nodes with different policies drew +one identical band for "edit freely" and "the server will reject your save with +`RECORD_LOCKED`", so the two states were indistinguishable until Save failed. + +Approval state is now two signals: + +- **`approvalPending`** — an approval is running. Drives the band and the recall + button, both meaningful whether or not the record is editable. +- **`locked`** — that approval also forbids edits, from the pending node's + `lock_record` (framework#3814, read off the same `node_config_json` snapshot + the server's record-lock hook reads). + +The band renders two states: amber lock + "Locked for approval", or sky clock + +"In approval · editable", each with its own tooltip. Recall moved out of the +locked branch — an editable pending approval is just as recallable. Inline +editing stays live in the editable state. + +`InlineEditProvider` takes a new optional `approvalPending` prop, defaulting to +`locked`, so a host that threads only `locked` renders exactly as before. The +record's `approval_status` field remains the fallback for backends with no +approvals API; it carries no node granularity, so it still reads as locked — as +does a pending request from a backend too old to report the policy. + +New `detail.approvalPendingEditable` / `detail.approvalPendingTooltip` keys are +translated in all ten locales. diff --git a/packages/app-shell/src/hooks/useRecordApprovals.locking.test.ts b/packages/app-shell/src/hooks/useRecordApprovals.locking.test.ts new file mode 100644 index 0000000000..675a5ebb13 --- /dev/null +++ b/packages/app-shell/src/hooks/useRecordApprovals.locking.test.ts @@ -0,0 +1,61 @@ +/** + * 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. + */ + +import { describe, it, expect } from 'vitest'; +import { recordLockedByApproval, type ApprovalRequestLite } from './useRecordApprovals'; + +/** + * Record-lock policy of a pending approval (objectui#2902). + * + * "A pending approval request exists" and "the record is locked" are different + * facts: an approval node declares `lockRecord`, and the server's record-lock + * hook lets writes through for the whole time a `lockRecord: false` node waits. + * The console used to equate them, which disabled inline editing and showed a + * "Locked for approval" badge on records the server would have let the user + * edit — exactly the single-approver steps the flag exists for. + */ + +const req = (over: Partial = {}): ApprovalRequestLite => ({ + id: 'r1', + process_name: 'flow:budget_approval', + object_name: 'showcase_project', + record_id: 'p1', + status: 'pending', + ...over, +}); + +describe('recordLockedByApproval (objectui#2902)', () => { + it('is not locked with no pending request', () => { + expect(recordLockedByApproval(null)).toBe(false); + expect(recordLockedByApproval(undefined)).toBe(false); + }); + + it('is locked when the pending node declares lockRecord', () => { + expect(recordLockedByApproval(req({ lock_record: true }))).toBe(true); + }); + + it('is NOT locked when the pending node opted out — the #2902 regression', () => { + expect(recordLockedByApproval(req({ lock_record: false }))).toBe(false); + }); + + it('fails closed when the backend does not report the policy', () => { + // Pre-framework#3814: the flag was never sent. Assuming "unlocked" there + // would offer an edit the server rejects with RECORD_LOCKED, so the + // unknown must read as locked. + expect(recordLockedByApproval(req())).toBe(true); + }); + + it('reads each node independently as a flow advances', () => { + // The issue's repro: node A unlocked, node B locked. One request per node, + // each carrying its own policy — the band must change between them. + const nodeA = req({ id: 'rA', current_step: 'manager_review', lock_record: false }); + const nodeB = req({ id: 'rB', current_step: 'exec_review', lock_record: true }); + expect(recordLockedByApproval(nodeA)).toBe(false); + expect(recordLockedByApproval(nodeB)).toBe(true); + }); +}); diff --git a/packages/app-shell/src/hooks/useRecordApprovals.ts b/packages/app-shell/src/hooks/useRecordApprovals.ts index 6a9611636f..51664e68c8 100644 --- a/packages/app-shell/src/hooks/useRecordApprovals.ts +++ b/packages/app-shell/src/hooks/useRecordApprovals.ts @@ -32,6 +32,36 @@ export interface ApprovalRequestLite { pending_approvers?: string[] | null; submitted_at?: string; completed_at?: string | null; + /** + * Whether THIS pending node locks the record from edits (objectui#2902). + * The approval node's `lockRecord` policy, surfaced by the server from the + * same `node_config_json` snapshot its record-lock `beforeUpdate` hook reads + * — so the badge we render and the rule the server applies agree. + * + * `undefined` on a pre-framework#3814 backend, which never sent the flag. + * Callers must fail CLOSED there (treat as locked): offering an edit the + * server then rejects with `RECORD_LOCKED` is worse than hiding one it would + * have allowed. See {@link recordLockedByApproval}. + */ + lock_record?: boolean; +} + +/** + * Does an open approval request lock its record from edits? + * + * A pending request is NOT the same thing as a locked record: an approval node + * may declare `lockRecord: false`, and the server then lets the record be + * edited while that node waits (a single-approver step where the approver is + * meant to fill in the missing detail is the motivating case). Treating "has a + * pending request" as "locked" mislabels those nodes and hides an edit the + * server would have accepted — objectui#2902. + * + * Fails closed on both unknowns: no request at all is not a lock, but a request + * from a backend too old to report the policy is. + */ +export function recordLockedByApproval(request: ApprovalRequestLite | null | undefined): boolean { + if (!request) return false; + return request.lock_record !== false; } interface UseRecordApprovalsResult { diff --git a/packages/app-shell/src/views/RecordDetailView.tsx b/packages/app-shell/src/views/RecordDetailView.tsx index 1a53f4ec3a..f0a6dc3847 100644 --- a/packages/app-shell/src/views/RecordDetailView.tsx +++ b/packages/app-shell/src/views/RecordDetailView.tsx @@ -36,7 +36,7 @@ import { resolveActionParams } from '../utils/resolveActionParams'; import { useRecordBreadcrumbTitle } from '../context/NavigationContext'; import type { FeedItem } from '@object-ui/types'; import type { ActionDef, ActionParamDef } from '@object-ui/core'; -import { useRecordApprovals } from '../hooks/useRecordApprovals'; +import { useRecordApprovals, recordLockedByApproval } from '../hooks/useRecordApprovals'; import { RecordAttachmentsPanel } from './RecordAttachmentsPanel'; import { RecordPermissionAssignmentsRenderer } from './metadata-admin/RecordPermissionAssignmentsRenderer'; import { getRecordDisplayName } from '../utils'; @@ -868,14 +868,32 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri void approvalsRef.current.refresh(); }, [recordInvalidationNonce]); - // The shared lock signal for the inline-edit session: the record's own - // `approval_status` (what the DetailView lock band keys off) OR an open - // pending request from the approvals API — the latter catches backends - // that lock via the request record without materializing the field. - const approvalLocked = + // Approval state is TWO signals, not one (objectui#2902). + // + // `approvalPending` — an approval is in flight on this record. Drives the + // status band and the recall affordance, which are meaningful whether or not + // the record is editable. + // + // `approvalLocked` — that approval also forbids edits. The approval node's + // `lockRecord` policy decides this, and the server enforces exactly that + // policy in its record-lock `beforeUpdate` hook. Conflating the two (which + // this used to do) made every `lockRecord: false` node claim the record was + // locked while the server happily accepted writes — and, worse than the + // wrong label, `canEdit` below then suppressed the inline-edit affordances + // entirely, so the "approver may fill in the missing detail" case the flag + // exists for was unreachable from the console. + // + // The record's own `approval_status` field stays a fallback for backends + // that mirror status onto the record but expose no approvals API. It carries + // no node granularity, so it can only mean "locked" — the conservative read, + // and the same one `recordLockedByApproval` applies to a pre-framework#3814 backend. + const approvalStatusPending = (pageRecord as any)?.approval_status === 'pending' || - (pageRecord as any)?.approval_status === 'in_approval' || - !!approvals.pendingRequest; + (pageRecord as any)?.approval_status === 'in_approval'; + const approvalPending = approvalStatusPending || !!approvals.pendingRequest; + const approvalLocked = approvals.pendingRequest + ? recordLockedByApproval(approvals.pendingRequest) + : approvalStatusPending; const approvalHandler = useCallback(async (action: ActionDef) => { const target = action.target || action.name; @@ -1916,13 +1934,17 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri hides the pencil affordances and no-ops `enter()`, so users can't type into a draft that Save would reject with RECORD_LOCKED. `locked` surfaces the approval lock as its own signal so the - DetailView "Locked for approval" band renders from the SAME - dual-source `approvalLocked` that gated `canEdit` — engaging even - on backends that track the lock via approval requests only and - never materialize an `approval_status` field (objectui#2618). */} + DetailView approval band renders from the SAME dual-source + `approvalLocked` that gated `canEdit` — engaging even on backends + that track the lock via approval requests only and never + materialize an `approval_status` field (objectui#2618). + `approvalPending` rides alongside it so a node that declares + `lockRecord: false` still shows its band and recall button while + leaving the record editable (objectui#2902). */} = ({ )} - {/* Approval-lock band — when the DetailView's own header is suppressed - (composed under a Lightning-style page:header), surface the approval - lock reason + recall affordance inline. The inline-edit Save / Cancel + {/* Approval band — when the DetailView's own header is suppressed + (composed under a Lightning-style page:header), surface the running + approval + recall affordance inline. The inline-edit Save / Cancel bar itself now lives in the record-level - (objectui#2407 P1); this band is lock-only. */} + (objectui#2407 P1). */} {inlineEdit && schema.showHeader === false && (() => { - // Detect approval lock. Prefer the host-supplied signal - // (`inline.locked`) — the record-level session computes it from the - // record's `approval_status` field OR an open approvals request - // (objectui#2618), so the band engages even on backends that track - // the lock via approval requests only and never materialize an - // `approval_status` field on the record. Fall back to the record's - // own field for bare/legacy DetailView usage without a host that - // threads the lock. Either way a locked record's writes are rejected - // with RECORD_LOCKED, so surface the badge instead of editing and - // failing on save. + // Two independent facts, not one (objectui#2902): + // + // isPending — an approval is running on this record. + // isLocked — that approval also forbids edits. + // + // They come apart because an approval node declares `lockRecord`, and + // on a `lockRecord: false` node the backend accepts writes for the + // whole time the node waits. Rendering one band for both states told + // the user "locked" on a record they could freely edit, and the + // approver — the very person the flag exists to let edit — never tried. + // + // Prefer the host-supplied signals: the record-level session resolves + // the pending node's policy from the approvals API (objectui#2618 for + // why the record field alone is not enough). The record's own + // `approval_status` remains the fallback for bare/legacy DetailView + // usage with no host threading the state; it carries no node + // granularity, so it can only mean "locked" — the safe read, since a + // wrongly-offered edit dies on the server with RECORD_LOCKED. const approvalStatus = data?.approval_status; - const isLocked = - (inline?.locked ?? false) || - approvalStatus === 'pending' || - approvalStatus === 'in_approval'; - // Nothing to surface (not locked, no approval-cancel error): no band. - if (!isLocked && !saveError) return null; + const statusPending = + approvalStatus === 'pending' || approvalStatus === 'in_approval'; + const isLocked = (inline?.locked ?? false) || statusPending; + const isPending = (inline?.approvalPending ?? false) || isLocked; + // Nothing to surface (no approval, no approval-cancel error): no band. + if (!isPending && !saveError) return null; return (
- {isLocked && ( + {isPending && (
- - {t('detail.lockedByApproval')} + {isLocked ? : } + + {isLocked + ? t('detail.lockedByApproval') + : t('detail.approvalPendingEditable')} + + {/* Recall belongs to the approval, not to the lock: an editable + pending approval is just as recallable as a locked one. */} {dataSource?.cancelPendingApproval && (