Skip to content

Commit 952b978

Browse files
baozhoutaoclaude
baozhoutao
and
claude
authored
fix(detail): the approval band honors the node's lockRecord instead of assuming every approval locks (#2902) (#2906)
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 <noreply@anthropic.com>
1 parent 32bb1d7 commit 952b978

19 files changed

Lines changed: 359 additions & 41 deletions

‎.changeset/approval-band-honors-lock-record.md‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
---
2+
"@object-ui/react": minor
3+
"@object-ui/plugin-detail": minor
4+
"@object-ui/app-shell": minor
5+
"@object-ui/i18n": minor
6+
---
7+
8+
fix(detail): the approval band honors the node's `lockRecord` instead of assuming every approval locks (#2902)
9+
10+
A record detail page treated "a pending approval request exists" as "this
11+
record is locked". An approval node declares `lockRecord` (default `true`), and
12+
on `lockRecord: false` the server keeps accepting writes for the whole time
13+
that node waits — so the console was asserting a lock the backend did not
14+
enforce.
15+
16+
The label was the smaller half of it. The same conflated signal fed `canEdit`,
17+
so the record-level inline-edit session was suppressed too: no pencils,
18+
`enter()` a no-op. On a single-approver step — a department head or plant
19+
manager, exactly the case `lockRecord: false` exists for, where the approver is
20+
meant to fill in the missing detail before deciding — the capability was
21+
unreachable from the UI. And a flow chaining nodes with different policies drew
22+
one identical band for "edit freely" and "the server will reject your save with
23+
`RECORD_LOCKED`", so the two states were indistinguishable until Save failed.
24+
25+
Approval state is now two signals:
26+
27+
- **`approvalPending`** — an approval is running. Drives the band and the recall
28+
button, both meaningful whether or not the record is editable.
29+
- **`locked`** — that approval also forbids edits, from the pending node's
30+
`lock_record` (framework#3814, read off the same `node_config_json` snapshot
31+
the server's record-lock hook reads).
32+
33+
The band renders two states: amber lock + "Locked for approval", or sky clock +
34+
"In approval · editable", each with its own tooltip. Recall moved out of the
35+
locked branch — an editable pending approval is just as recallable. Inline
36+
editing stays live in the editable state.
37+
38+
`InlineEditProvider` takes a new optional `approvalPending` prop, defaulting to
39+
`locked`, so a host that threads only `locked` renders exactly as before. The
40+
record's `approval_status` field remains the fallback for backends with no
41+
approvals API; it carries no node granularity, so it still reads as locked — as
42+
does a pending request from a backend too old to report the policy.
43+
44+
New `detail.approvalPendingEditable` / `detail.approvalPendingTooltip` keys are
45+
translated in all ten locales.

‎packages/app-shell/src/hooks/useRecordApprovals.locking.test.ts‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
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+
import { describe, it, expect } from 'vitest';
10+
import { recordLockedByApproval, type ApprovalRequestLite } from './useRecordApprovals';
11+
12+
/**
13+
* Record-lock policy of a pending approval (objectui#2902).
14+
*
15+
* "A pending approval request exists" and "the record is locked" are different
16+
* facts: an approval node declares `lockRecord`, and the server's record-lock
17+
* hook lets writes through for the whole time a `lockRecord: false` node waits.
18+
* The console used to equate them, which disabled inline editing and showed a
19+
* "Locked for approval" badge on records the server would have let the user
20+
* edit — exactly the single-approver steps the flag exists for.
21+
*/
22+
23+
const req = (over: Partial<ApprovalRequestLite> = {}): ApprovalRequestLite => ({
24+
id: 'r1',
25+
process_name: 'flow:budget_approval',
26+
object_name: 'showcase_project',
27+
record_id: 'p1',
28+
status: 'pending',
29+
...over,
30+
});
31+
32+
describe('recordLockedByApproval (objectui#2902)', () => {
33+
it('is not locked with no pending request', () => {
34+
expect(recordLockedByApproval(null)).toBe(false);
35+
expect(recordLockedByApproval(undefined)).toBe(false);
36+
});
37+
38+
it('is locked when the pending node declares lockRecord', () => {
39+
expect(recordLockedByApproval(req({ lock_record: true }))).toBe(true);
40+
});
41+
42+
it('is NOT locked when the pending node opted out — the #2902 regression', () => {
43+
expect(recordLockedByApproval(req({ lock_record: false }))).toBe(false);
44+
});
45+
46+
it('fails closed when the backend does not report the policy', () => {
47+
// Pre-framework#3814: the flag was never sent. Assuming "unlocked" there
48+
// would offer an edit the server rejects with RECORD_LOCKED, so the
49+
// unknown must read as locked.
50+
expect(recordLockedByApproval(req())).toBe(true);
51+
});
52+
53+
it('reads each node independently as a flow advances', () => {
54+
// The issue's repro: node A unlocked, node B locked. One request per node,
55+
// each carrying its own policy — the band must change between them.
56+
const nodeA = req({ id: 'rA', current_step: 'manager_review', lock_record: false });
57+
const nodeB = req({ id: 'rB', current_step: 'exec_review', lock_record: true });
58+
expect(recordLockedByApproval(nodeA)).toBe(false);
59+
expect(recordLockedByApproval(nodeB)).toBe(true);
60+
});
61+
});

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,36 @@ export interface ApprovalRequestLite {
3232
pending_approvers?: string[] | null;
3333
submitted_at?: string;
3434
completed_at?: string | null;
35+
/**
36+
* Whether THIS pending node locks the record from edits (objectui#2902).
37+
* The approval node's `lockRecord` policy, surfaced by the server from the
38+
* same `node_config_json` snapshot its record-lock `beforeUpdate` hook reads
39+
* — so the badge we render and the rule the server applies agree.
40+
*
41+
* `undefined` on a pre-framework#3814 backend, which never sent the flag.
42+
* Callers must fail CLOSED there (treat as locked): offering an edit the
43+
* server then rejects with `RECORD_LOCKED` is worse than hiding one it would
44+
* have allowed. See {@link recordLockedByApproval}.
45+
*/
46+
lock_record?: boolean;
47+
}
48+
49+
/**
50+
* Does an open approval request lock its record from edits?
51+
*
52+
* A pending request is NOT the same thing as a locked record: an approval node
53+
* may declare `lockRecord: false`, and the server then lets the record be
54+
* edited while that node waits (a single-approver step where the approver is
55+
* meant to fill in the missing detail is the motivating case). Treating "has a
56+
* pending request" as "locked" mislabels those nodes and hides an edit the
57+
* server would have accepted — objectui#2902.
58+
*
59+
* Fails closed on both unknowns: no request at all is not a lock, but a request
60+
* from a backend too old to report the policy is.
61+
*/
62+
export function recordLockedByApproval(request: ApprovalRequestLite | null | undefined): boolean {
63+
if (!request) return false;
64+
return request.lock_record !== false;
3565
}
3666

3767
interface UseRecordApprovalsResult {

‎packages/app-shell/src/views/RecordDetailView.tsx‎

Lines changed: 34 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ import { resolveActionParams } from '../utils/resolveActionParams';
3636
import { useRecordBreadcrumbTitle } from '../context/NavigationContext';
3737
import type { FeedItem } from '@object-ui/types';
3838
import type { ActionDef, ActionParamDef } from '@object-ui/core';
39-
import { useRecordApprovals } from '../hooks/useRecordApprovals';
39+
import { useRecordApprovals, recordLockedByApproval } from '../hooks/useRecordApprovals';
4040
import { RecordAttachmentsPanel } from './RecordAttachmentsPanel';
4141
import { RecordPermissionAssignmentsRenderer } from './metadata-admin/RecordPermissionAssignmentsRenderer';
4242
import { getRecordDisplayName } from '../utils';
@@ -868,14 +868,32 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri
868868
void approvalsRef.current.refresh();
869869
}, [recordInvalidationNonce]);
870870

871-
// The shared lock signal for the inline-edit session: the record's own
872-
// `approval_status` (what the DetailView lock band keys off) OR an open
873-
// pending request from the approvals API — the latter catches backends
874-
// that lock via the request record without materializing the field.
875-
const approvalLocked =
871+
// Approval state is TWO signals, not one (objectui#2902).
872+
//
873+
// `approvalPending` — an approval is in flight on this record. Drives the
874+
// status band and the recall affordance, which are meaningful whether or not
875+
// the record is editable.
876+
//
877+
// `approvalLocked` — that approval also forbids edits. The approval node's
878+
// `lockRecord` policy decides this, and the server enforces exactly that
879+
// policy in its record-lock `beforeUpdate` hook. Conflating the two (which
880+
// this used to do) made every `lockRecord: false` node claim the record was
881+
// locked while the server happily accepted writes — and, worse than the
882+
// wrong label, `canEdit` below then suppressed the inline-edit affordances
883+
// entirely, so the "approver may fill in the missing detail" case the flag
884+
// exists for was unreachable from the console.
885+
//
886+
// The record's own `approval_status` field stays a fallback for backends
887+
// that mirror status onto the record but expose no approvals API. It carries
888+
// no node granularity, so it can only mean "locked" — the conservative read,
889+
// and the same one `recordLockedByApproval` applies to a pre-framework#3814 backend.
890+
const approvalStatusPending =
876891
(pageRecord as any)?.approval_status === 'pending' ||
877-
(pageRecord as any)?.approval_status === 'in_approval' ||
878-
!!approvals.pendingRequest;
892+
(pageRecord as any)?.approval_status === 'in_approval';
893+
const approvalPending = approvalStatusPending || !!approvals.pendingRequest;
894+
const approvalLocked = approvals.pendingRequest
895+
? recordLockedByApproval(approvals.pendingRequest)
896+
: approvalStatusPending;
879897

880898
const approvalHandler = useCallback(async (action: ActionDef) => {
881899
const target = action.target || action.name;
@@ -1916,13 +1934,17 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri
19161934
hides the pencil affordances and no-ops `enter()`, so users can't
19171935
type into a draft that Save would reject with RECORD_LOCKED.
19181936
`locked` surfaces the approval lock as its own signal so the
1919-
DetailView "Locked for approval" band renders from the SAME
1920-
dual-source `approvalLocked` that gated `canEdit` — engaging even
1921-
on backends that track the lock via approval requests only and
1922-
never materialize an `approval_status` field (objectui#2618). */}
1937+
DetailView approval band renders from the SAME dual-source
1938+
`approvalLocked` that gated `canEdit` — engaging even on backends
1939+
that track the lock via approval requests only and never
1940+
materialize an `approval_status` field (objectui#2618).
1941+
`approvalPending` rides alongside it so a node that declares
1942+
`lockRecord: false` still shows its band and recall button while
1943+
leaving the record editable (objectui#2902). */}
19231944
<InlineEditProvider
19241945
canEdit={resolveRecordHeaderActionGates(objectDef, effectiveApiOperations).edit && !approvalLocked}
19251946
locked={approvalLocked}
1947+
approvalPending={approvalPending}
19261948
lockedReason={t('detail.lockedTooltip', {
19271949
defaultValue: 'This record has a pending approval request; editing is locked',
19281950
})}

‎packages/i18n/src/locales/ar.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -764,6 +764,8 @@ const ar = {
764764
saving: "جارٍ الحفظ…",
765765
lockedByApproval: "مقفل للموافقة",
766766
lockedTooltip: "يحتوي هذا السجل على طلب موافقة معلق؛ التعديل مقفل",
767+
approvalPendingEditable: "قيد الموافقة · قابل للتعديل",
768+
approvalPendingTooltip: "يحتوي هذا السجل على طلب موافقة معلق، لكن هذه الخطوة لا تزال تسمح بالتعديل",
767769
cancelApproval: "إلغاء الموافقة",
768770
cancelApprovalInFlight: "جارٍ الإلغاء…",
769771
cancelApprovalTooltip: "إلغاء طلب الموافقة المعلق لفتح قفل السجل",

‎packages/i18n/src/locales/de.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -762,6 +762,8 @@ const de = {
762762
saving: "Speichern…",
763763
lockedByApproval: "Zur Genehmigung gesperrt",
764764
lockedTooltip: "Dieser Datensatz hat eine ausstehende Genehmigungsanfrage; die Bearbeitung ist gesperrt",
765+
approvalPendingEditable: "In Genehmigung · bearbeitbar",
766+
approvalPendingTooltip: "Dieser Datensatz hat eine ausstehende Genehmigungsanfrage; dieser Schritt erlaubt weiterhin die Bearbeitung",
765767
cancelApproval: "Genehmigung zurückziehen",
766768
cancelApprovalInFlight: "Zurückziehen…",
767769
cancelApprovalTooltip: "Ausstehende Genehmigungsanfrage zurückziehen, um den Datensatz zu entsperren",

‎packages/i18n/src/locales/en.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -680,6 +680,8 @@ const en = {
680680
editInlineHint: 'Double-click to edit',
681681
lockedByApproval: 'Locked for approval',
682682
lockedTooltip: 'This record has a pending approval request; editing is locked',
683+
approvalPendingEditable: 'In approval · editable',
684+
approvalPendingTooltip: 'This record has a pending approval request; this step still allows editing',
683685
cancelApproval: 'Recall approval',
684686
cancelApprovalInFlight: 'Recalling…',
685687
cancelApprovalTooltip: 'Recall the pending approval request to unlock this record',

‎packages/i18n/src/locales/es.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -762,6 +762,8 @@ const es = {
762762
saving: "Guardando…",
763763
lockedByApproval: "Bloqueado para aprobación",
764764
lockedTooltip: "Este registro tiene una solicitud de aprobación pendiente; la edición está bloqueada",
765+
approvalPendingEditable: "En aprobación · editable",
766+
approvalPendingTooltip: "Este registro tiene una solicitud de aprobación pendiente; este paso todavía permite la edición",
765767
cancelApproval: "Cancelar aprobación",
766768
cancelApprovalInFlight: "Cancelando…",
767769
cancelApprovalTooltip: "Cancelar la solicitud de aprobación pendiente para desbloquear el registro",

‎packages/i18n/src/locales/fr.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -764,6 +764,8 @@ const fr = {
764764
saving: "Enregistrement…",
765765
lockedByApproval: "Verrouillé pour approbation",
766766
lockedTooltip: "Cet enregistrement a une demande d'approbation en attente ; la modification est verrouillée",
767+
approvalPendingEditable: "En approbation · modifiable",
768+
approvalPendingTooltip: "Cet enregistrement a une demande d'approbation en attente ; cette étape autorise encore la modification",
767769
cancelApproval: "Annuler l'approbation",
768770
cancelApprovalInFlight: "Annulation…",
769771
cancelApprovalTooltip: "Annuler la demande d'approbation en attente pour déverrouiller l'enregistrement",

‎packages/i18n/src/locales/ja.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -773,6 +773,8 @@ const ja = {
773773
saving: "保存中…",
774774
lockedByApproval: "承認のためロック中",
775775
lockedTooltip: "このレコードには承認待ちのリクエストがあります。編集はロックされています",
776+
approvalPendingEditable: "承認中 · 編集可能",
777+
approvalPendingTooltip: "このレコードには承認待ちのリクエストがありますが、このステップでは編集できます",
776778
cancelApproval: "承認を取り消す",
777779
cancelApprovalInFlight: "取り消し中…",
778780
cancelApprovalTooltip: "承認待ちリクエストを取り消してレコードのロックを解除する",

0 commit comments

Comments
 (0)