diff --git a/.changeset/25934-reader-record-lock.md b/.changeset/25934-reader-record-lock.md new file mode 100644 index 00000000000..a91766728df --- /dev/null +++ b/.changeset/25934-reader-record-lock.md @@ -0,0 +1,14 @@ +--- +'@object-ui/app-shell': patch +--- + +fix(app-shell): a record locked by an approval is shown locked to a reader outside that approval + +Clause-②: no +Metadata-protocol: none + +A member who can read a record but is not on its approval (not an approver, not the submitter, no override) was offered inline edit while a `lockRecord: true` approval held the record, and the save was then refused with `409 RECORD_LOCKED`. The approvals list serves a request only to its participants, so the record page read that member's empty list as "no approval is running". + +When the list finds no pending request, the record page now also reads the record's own lock, `client.approvals.getRecordLock(object, recordId)`, which the server answers to every caller who can read the record with `{ locked }` and nothing else. A locked answer shows the record locked: Edit is disabled, inline edit is not offered, and the "Locked for approval" band is drawn without a recall button, because a reader who cannot see the request did not submit it. The reader still sees nothing of the request itself (approvers, comments, attachments). + +Unchanged: a pending request the viewer can see still decides with its own node's `lockRecord`, and a composition without the approvals service (the lock read answers `501`) still opens an answered empty record editable. diff --git a/ui/packages/app-shell/src/hooks/useRecordApprovals.ts b/ui/packages/app-shell/src/hooks/useRecordApprovals.ts index b65a91f4a9a..784f4d410b5 100644 --- a/ui/packages/app-shell/src/hooks/useRecordApprovals.ts +++ b/ui/packages/app-shell/src/hooks/useRecordApprovals.ts @@ -28,9 +28,18 @@ * approvals service still mounts the list route and answers it * `200 { data: [] }`: that is an answered read with no request, not an absent * route, and the result's `answered` says which one happened (objectui#12220). + * + * That list serves a request only to its participants (slate, submitter, + * override actor), so for anyone else an answered empty read cannot tell "no + * approval is running" from "one is running that you may not see". When the + * list finds no pending request, the hook also asks the server whether the + * record is held by a locking approval (`client.approvals.getRecordLock`, + * answered to every caller who can read the record — objectstack#25934 ruling + * A) and hands the answer back as `recordLock`. */ import { useCallback, useEffect, useMemo, useRef, useState, useSyncExternalStore } from 'react'; +import { useAdapter } from '../providers/AdapterProvider.js'; import { bearerAuthHeaders } from '../utils/authToken.js'; import type { DecisionOutputDef } from '../utils/decisionOutputParams.js'; @@ -278,6 +287,18 @@ interface UseRecordApprovalsResult { */ pendingRequest: ApprovalRequestLite | null; latestRequest: ApprovalRequestLite | null; + /** + * The server's record lock (`GET /approvals/records/:object/:recordId/lock`, + * objectstack#25934), read only when the list found no pending request: + * `{ locked: true }` means a pending approval whose node locks the record is + * running, though this viewer cannot see its request. `null` when there is + * no answer to give: a pending request was found (it decides, with its own + * `lock_record`), no adapter is mounted, or the read has never answered — a + * composition without the approvals service answers it 501, so there the + * record is not locked by this signal. A later failed read keeps the last + * answer, as the list read does. + */ + recordLock: { locked: boolean } | null; refresh: () => Promise; } @@ -415,6 +436,8 @@ interface ApprovalsSnapshot { answered: boolean; /** Rows as the server sent them, the pending one enriched. Unsorted. */ rows: ApprovalRequestLite[]; + /** The server's record lock, when the rows hold no pending request. */ + recordLock: { locked: boolean } | null; } const EMPTY_ROWS: ApprovalRequestLite[] = []; @@ -423,6 +446,7 @@ const INITIAL_SNAPSHOT: ApprovalsSnapshot = Object.freeze({ available: true, answered: false, rows: EMPTY_ROWS, + recordLock: null, }) as ApprovalsSnapshot; interface ApprovalsEntry { @@ -467,8 +491,16 @@ function holdEntry(key: string): () => void { }; } +/** Reads the server's record lock for the record a read is for. */ +type RecordLockReader = () => Promise<{ locked: boolean }>; + /** Send the record's read and publish its answer to every consumer of the key. */ -function startRead(key: string, objectName: string, recordId: string): Promise { +function startRead( + key: string, + objectName: string, + recordId: string, + readLock: RecordLockReader | null, +): Promise { const seq = ++readSeq; latestSeq.set(key, seq); const entry = entries.get(key); @@ -484,11 +516,26 @@ function startRead(key: string, objectName: string, recordId: string): Promise r.status === 'pending'); const full = pending ? await fetchProgressEnrichment(pending) : null; + // A pending row the viewer can see decides the lock with its own node's + // `lock_record`. Without one, ask the record itself (objectstack#25934): + // the list hides a request from a non-participant, the lock does not. + let recordLock: { locked: boolean } | null = null; + if (!pending && readLock) { + recordLock = entries.get(key)?.snapshot.recordLock ?? null; + try { + const lock = await readLock(); + recordLock = { locked: lock?.locked === true }; + } catch { + // A read that never answered (501 without the approvals service) + // leaves none; a failure after an answer keeps it, as the rows are. + } + } if (latestSeq.get(key) !== seq) return; publish(key, { rows: full ? rows.map((r) => (r === pending ? full : r)) : rows, available: true, answered: true, + recordLock, }); } catch (err: any) { if (latestSeq.get(key) !== seq) return; @@ -543,6 +590,14 @@ export function useRecordApprovals( // for the render, mirrored into a ref the mount effect and `refresh` read. const [unavailable, setUnavailable] = useState(false); const unavailableRef = useRef(false); + // The record-lock read goes through the SDK (`client.approvals.getRecordLock`), + // so it needs the adapter's client; with no adapter mounted it is skipped. + const adapter = useAdapter(); + const lockReader = useCallback( + (object: string, id: string): RecordLockReader | null => + adapter ? () => adapter.getClient().approvals.getRecordLock(object, id) : null, + [adapter], + ); const subscribe = useCallback((l: () => void) => subscribeKey(readKey, l), [readKey]); const getSnapshot = useCallback( @@ -565,19 +620,19 @@ export function useRecordApprovals( // read only when neither exists. The entry may be new while a read is in // flight (StrictMode's replayed mount): that read publishes into it. if (entry && !entry.read && !inFlight.has(readKey) && !unavailableRef.current) { - void startRead(readKey, objectName, recordId); + void startRead(readKey, objectName, recordId, lockReader(objectName, recordId)); } else if (entry && inFlight.has(readKey)) { entry.read = true; if (!entry.snapshot.loading) publish(readKey, { loading: true }); } return release; - }, [readKey, objectName, recordId]); + }, [readKey, objectName, recordId, lockReader]); const refresh = useCallback(async () => { if (!objectName || !recordId || !readKey) return; if (unavailableRef.current) return; - await startRead(readKey, objectName, recordId); - }, [objectName, recordId, readKey]); + await startRead(readKey, objectName, recordId, lockReader(objectName, recordId)); + }, [objectName, recordId, readKey, lockReader]); const requests = readKey === null ? EMPTY_ROWS : snapshot.rows; const loading = snapshot.loading; @@ -613,6 +668,7 @@ export function useRecordApprovals( requests: sortedRequests, pendingRequest, latestRequest, + recordLock: readKey === null ? null : snapshot.recordLock, refresh, }; } diff --git a/ui/packages/app-shell/src/views/RecordDetailView.approvalLock-25934.test.tsx b/ui/packages/app-shell/src/views/RecordDetailView.approvalLock-25934.test.tsx new file mode 100644 index 00000000000..2c361e93a1d --- /dev/null +++ b/ui/packages/app-shell/src/views/RecordDetailView.approvalLock-25934.test.tsx @@ -0,0 +1,278 @@ +/** + * 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. + */ + +/** + * A reader outside the approval is not offered an edit on a locked record + * (objectstack#25934, ruling A). + * + * The approvals list serves a request only to its participants, so a member + * who can read the record but is not on the slate, did not submit and holds no + * override reads `GET /approvals/requests?object=…&recordId=…` as + * `200 { data: [] }` while a `lockRecord: true` approval runs. The page took + * that for "no approval is running" and offered inline edit, and the save + * answered `409 RECORD_LOCKED`. The page now asks the record's own lock + * (`client.approvals.getRecordLock`, answered to every reader) when the list + * finds no pending request. + * + * Same probe as `RecordDetailView.approvalLock-12220`: it prints what the page + * threads into its ``. The record carries no + * `approval_status` mirror (showcase's shape), so no column fallback can stand + * in for the signal. + */ + +import * as React from 'react'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, screen, waitFor, 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', async (importOriginal) => ({ + ...(await importOriginal>()), + useAuth: () => ({ user: { id: 'u_owner', name: 'Account Owner', image: null }, activeOrganization: null }), + createAuthenticatedFetch: () => authFetchSpy, +})); + +vi.mock('@object-ui/collaboration', async (importOriginal) => ({ + ...(await importOriginal>()), + 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(), + }), +})); + +vi.mock('./ActionConfirmDialog', () => ({ ActionConfirmDialog: () => null })); +vi.mock('./ActionParamDialog', () => ({ ActionParamDialog: () => 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 })), +})); + +vi.mock('@object-ui/react', async (importOriginal) => { + const actual = await importOriginal(); + const Probe = () => { + const inline = actual.useInlineEdit(); + return ( +
+ ); + }; + return { ...actual, SchemaRenderer: Probe }; +}); + +import { AdapterCtx, MetadataCtx } from '@object-ui/react'; +import { RecordDetailView } from './RecordDetailView'; +import { __resetRecordApprovalsStore } from '../hooks/useRecordApprovals'; + +const OBJECT_NAME = 'account'; +const RECORD_ID = 'acc_northwind'; +const REQUEST_ID = 'req_account_1'; + +const OBJECTS = [ + { + name: OBJECT_NAME, + label: 'Account', + fields: { + id: { type: 'text', label: 'Id' }, + name: { type: 'text', label: 'Name' }, + }, + }, +]; + +const pendingRequest = (over: Record = {}) => ({ + id: REQUEST_ID, + process_name: 'flow:account_review', + object_name: OBJECT_NAME, + record_id: RECORD_ID, + status: 'pending', + submitter_id: 'u_submitter', + current_step: 'manager_review', + pending_approvers: ['u_manager'], + lock_record: true, + ...over, +}); + +let listGets: string[]; + +const jsonResponse = (status: number, body: unknown) => + new Response(JSON.stringify(body), { status, headers: { 'content-type': 'application/json' } }); + +/** + * What the approvals list route answers: `{ row }` is a 200, with `row: null` + * for the empty list a composition without the approvals service sends; + * `{ absent }` is an absent route answering that status. + */ +type ListAnswer = { row: Record | null } | { absent: number }; + +function stubApprovalsApi(answer: ListAnswer) { + listGets = []; + vi.stubGlobal( + 'fetch', + vi.fn(async (url: string) => { + const u = String(url); + if (u.includes('/approvals/requests?object=')) { + listGets.push(u); + if ('absent' in answer) return jsonResponse(answer.absent, { code: 'NOT_FOUND' }); + return jsonResponse(200, { data: answer.row ? [answer.row] : [] }); + } + if ('row' in answer && answer.row && u.endsWith(`/approvals/requests/${REQUEST_ID}`)) { + return jsonResponse(200, answer.row); + } + return jsonResponse(200, { data: [] }); + }), + ); +} + +const METADATA = { + objects: OBJECTS, + pages: [], + loading: false, + error: null, + refresh: async () => {}, + invalidate: () => {}, + ensureType: async () => [], + getItem: async () => null, + getItemsByType: () => [], +} as any; + +function makeDataSource() { + return { + find: vi.fn(async () => ({ data: [] })), + // Like showcase: no `approval_status` mirror for a fallback to read. + findOne: vi.fn(async () => ({ id: RECORD_ID, name: 'Northwind' })), + create: vi.fn(async () => ({})), + update: vi.fn(async () => ({})), + delete: vi.fn(async () => ({})), + cancelPendingApproval: vi.fn(async () => ({ requestId: REQUEST_ID, status: 'recalled' })), + } as any; +} + +/** + * The record-lock read as the SDK answers it: `{ locked }`, or a refusal + * carrying the route's status (501 when the approvals service is absent). + */ +type LockAnswer = { locked: boolean } | { refused: number }; +let getRecordLock: ReturnType; + +function makeAdapter(answer: LockAnswer) { + getRecordLock = vi.fn(async () => { + if ('refused' in answer) throw Object.assign(new Error(`HTTP ${answer.refused}`), { status: answer.refused }); + return { locked: answer.locked }; + }); + return { getClient: () => ({ approvals: { getRecordLock } }) } as any; +} + +function renderRecordPage(lock: LockAnswer) { + return render( + + + + {}} + objectNameOverride={OBJECT_NAME} + recordIdOverride={RECORD_ID} + embedded + /> + + + , + ); +} + +const attr = (probe: HTMLElement, name: string) => probe.getAttribute(`data-${name}`); + +beforeEach(() => { + cleanup(); + __resetRecordApprovalsStore(); + authFetchSpy.mockClear(); +}); + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +describe('record page lock for a reader outside the approval (objectstack#25934)', () => { + it('a non-participant on a record held by a locking approval is offered no inline edit', async () => { + stubApprovalsApi({ row: null }); + renderRecordPage({ locked: true }); + const probe = await screen.findByTestId('inline-probe'); + + await waitFor(() => expect(attr(probe, 'locked')).toBe('true')); + expect(getRecordLock).toHaveBeenCalledWith(OBJECT_NAME, RECORD_ID); + expect(attr(probe, 'approval-read-answered')).toBe('true'); + expect(attr(probe, 'approval-pending')).toBe('true'); + expect(attr(probe, 'can-edit')).toBe('false'); + // Their request is unreadable to them, so they did not submit it: no recall. + expect(attr(probe, 'approval-is-submitter')).toBe('false'); + }); + + it('a record the server reports unlocked opens editable', async () => { + stubApprovalsApi({ row: null }); + renderRecordPage({ locked: false }); + const probe = await screen.findByTestId('inline-probe'); + + await waitFor(() => expect(getRecordLock).toHaveBeenCalled()); + await waitFor(() => expect(attr(probe, 'approval-read-answered')).toBe('true')); + expect(attr(probe, 'locked')).toBe('false'); + expect(attr(probe, 'approval-pending')).toBe('false'); + await waitFor(() => expect(attr(probe, 'can-edit')).toBe('true')); + }); + + it('without the approvals service (lock read 501) nothing changes: an answered empty read is not locked', async () => { + stubApprovalsApi({ row: null }); + renderRecordPage({ refused: 501 }); + const probe = await screen.findByTestId('inline-probe'); + + await waitFor(() => expect(getRecordLock).toHaveBeenCalled()); + await waitFor(() => expect(attr(probe, 'approval-read-answered')).toBe('true')); + expect(attr(probe, 'locked')).toBe('false'); + expect(attr(probe, 'approval-pending')).toBe('false'); + await waitFor(() => expect(attr(probe, 'can-edit')).toBe('true')); + }); + + it('a participant\'s pending request decides, and the record lock is not read', async () => { + stubApprovalsApi({ row: pendingRequest({ lock_record: false }) }); + renderRecordPage({ locked: true }); + const probe = await screen.findByTestId('inline-probe'); + + await waitFor(() => expect(attr(probe, 'approval-pending')).toBe('true')); + expect(attr(probe, 'locked')).toBe('false'); + await waitFor(() => expect(attr(probe, 'can-edit')).toBe('true')); + expect(getRecordLock).not.toHaveBeenCalled(); + }); +}); diff --git a/ui/packages/app-shell/src/views/RecordDetailView.tsx b/ui/packages/app-shell/src/views/RecordDetailView.tsx index 5081fb31954..554e0b9ce08 100644 --- a/ui/packages/app-shell/src/views/RecordDetailView.tsx +++ b/ui/packages/app-shell/src/views/RecordDetailView.tsx @@ -1674,11 +1674,19 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri // record with nothing to recall. `resolveApprovalLock` is the one rule this // page and the DetailView band share; `approvalReadAnswered` below hands the // band the same `answered` this call reads. + // + // The list serves a request only to its participants, so a reader outside + // the slate reads it empty while a locking approval runs (objectstack#25934). + // For them the open approval is the server's record lock: `{ locked: true }` + // says a pending request from a locking node exists, which is rule 1's input + // exactly. No lock answer (no approvals service answers its route 501) is no + // open approval, so rule 2 is unchanged. + const lockedForReader = !approvals.pendingRequest && approvals.recordLock?.locked === true; const { pending: approvalPending, locked: approvalLocked } = resolveApprovalLock({ readAnswered: approvals.answered, openApproval: approvals.pendingRequest ? { locked: recordLockedByApproval(approvals.pendingRequest) } - : null, + : lockedForReader ? { locked: true } : null, recordStatus: (pageRecord as any)?.approval_status, }); // How far the pending node's tally has got (objectstack#4478). Multi-approver @@ -1701,7 +1709,13 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri // // This gates the AFFORDANCE only. `canEdit` / `approvalLocked` below are // untouched by it, and the recall endpoint authorizes the recall itself. - const approvalIsSubmitter = isSubmitterOf(approvals.pendingRequest, user?.id); + // + // A lock the viewer knows only from the record lock is one whose request they + // cannot read, and the submitter can always read their own: `false`, so the + // band offers this reader no recall. + const approvalIsSubmitter = lockedForReader + ? false + : isSubmitterOf(approvals.pendingRequest, user?.id); // Does this page mount the pending request's declared decision bar // (objectui#3055)? const approvalBarMounted = approvals.available && !!approvals.pendingRequest;