diff --git a/.changeset/lucky-buttons-clap.md b/.changeset/lucky-buttons-clap.md new file mode 100644 index 000000000..fb7e6239f --- /dev/null +++ b/.changeset/lucky-buttons-clap.md @@ -0,0 +1,12 @@ +--- +'@object-ui/console': patch +--- + +Approvals Inbox: stop offering a record link that dead-ends for the viewer it is +offered to. Approver routing goes by position while record visibility is a +separate gate, so an approver can be routed a request about a record they cannot +read — the row's record chip then landed on the record page's "Record not found". +The row (and the drawer's record title) now suppress the link for exactly those +targets, decided by one batched readability read per distinct object. Nothing +else changes: the title still shows, the approval decision path is untouched, and +the server's access semantics are neither read nor reported on. diff --git a/apps/console/src/pages/system/ApprovalsInboxPage.recordLink.test.tsx b/apps/console/src/pages/system/ApprovalsInboxPage.recordLink.test.tsx new file mode 100644 index 000000000..46912873a --- /dev/null +++ b/apps/console/src/pages/system/ApprovalsInboxPage.recordLink.test.tsx @@ -0,0 +1,255 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Approvals Inbox — the record link is offered only to a viewer who can open it + * (objectui#5211). + * + * ## The defect + * + * Approver routing goes by position; record visibility is a separate gate, and + * nothing reconciles the two. An approver routed a request about a record they + * cannot read was still offered the row's record chip, which landed on the + * record page's "Record not found — … does not exist or may have been deleted". + * Per the maintainer's ruling (2026-08-19) the link is suppressed for exactly + * those rows. + * + * ## What each case is measuring + * + * - the unreadable row renders **no link** (and still renders the title, which + * comes from the request's payload snapshot); + * - the readable row is untouched — link, and the same href as before; + * - the whole page costs **one** readability read for two rows of one object — + * the batching the ruling made the deciding criterion; + * - the **decision path is untouched**: one case pins inline approve WITHOUT + * involving the probe (so it holds on the pre-change tree too — that is what + * makes it a pin), and one shows that a row whose link was suppressed is still + * decidable, which is the thing that already worked and must keep working. + * + * ⛔ Nothing here asserts anything about the server answering `404` rather than + * `403` for the by-id read. That is the server's deliberate choice and is out of + * scope in both directions; the probe issues a list read, never a by-id one. + * + * No build artifact sits between the edit and this test: the root Vitest config + * aliases every `@object-ui` specifier at that package's `src` directory, and + * the page and probe under test are this app's own source. (Writing that path + * with a glob segment would close this comment — the sequence is a block-comment + * terminator, and the parse error it produces points at a line far below.) + */ + +import '@testing-library/jest-dom/vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, screen, cleanup, fireEvent, waitFor, within } from '@testing-library/react'; +import { MemoryRouter, Routes, Route } from 'react-router-dom'; + +const APP = 'com.objectstack.account'; + +const { adapterFind, approvalsApiStub, rows, ADAPTER, AUTH, I18N } = vi.hoisted(() => { + const rows = [ + { + id: 'req_readable', + process_name: 'invoice_approval', + process_label: 'Invoice Approval', + object_name: 'showcase_invoice', + object_label: 'Invoice', + record_id: 'inv_readable', + record_title: 'Readable Invoice', + status: 'pending', + pending_approvers: ['u_1'], + submitter_id: 'u_2', + submitter_name: 'Sam Submitter', + submitted_at: '2026-08-19T00:00:00.000Z', + }, + { + id: 'req_hidden', + process_name: 'invoice_approval', + process_label: 'Invoice Approval', + object_name: 'showcase_invoice', + object_label: 'Invoice', + record_id: 'inv_hidden', + record_title: 'Hidden Invoice', + status: 'pending', + pending_approvers: ['u_1'], + submitter_id: 'u_2', + submitter_name: 'Sam Submitter', + submitted_at: '2026-08-19T00:00:00.000Z', + }, + ]; + + /** + * The server's own behaviour, reduced: a record the principal's grant filters + * out never appears in the row set, so an `id in (…)` read simply comes back + * without it. `inv_hidden` is that record. + */ + const adapterFind = vi.fn(async (_object: string, params?: Record) => { + const ids = (params?.$filter as { id?: { $in?: string[] } } | undefined)?.id?.$in ?? []; + return { data: ids.filter((id) => id !== 'inv_hidden').map((id) => ({ id })) }; + }); + + const approvalsApiStub = { + listRequests: vi.fn(async () => ({ data: rows })), + getRequest: vi.fn(async (id: string) => ({ data: rows.find((r) => r.id === id) })), + listActions: vi.fn(async () => ({ data: [] })), + approve: vi.fn(async () => ({ data: rows[0], finalized: true })), + reject: vi.fn(async () => ({ data: rows[0], finalized: true })), + }; + + // STABLE singletons. A mocked hook that returns a fresh object per render + // gives every render a new `user` / `adapter` identity, which re-runs the + // page's load effect (and the probe's) forever — the page never leaves its + // loading skeleton and every query below times out on an empty table. + const ADAPTER = { find: adapterFind }; + const AUTH = { user: { id: 'u_1', email: 'approver@example.com' } }; + const I18N = { + t: (key: string, options?: Record) => String(options?.defaultValue ?? key), + language: 'en', + }; + + return { adapterFind, approvalsApiStub, rows, ADAPTER, AUTH, I18N }; +}); + +vi.mock('@object-ui/i18n', () => ({ useObjectTranslation: () => I18N })); + +vi.mock('@object-ui/auth', () => { + const authFetch = vi.fn(async () => new Response('{}', { status: 200 })); + return { + useAuth: () => AUTH, + createAuthenticatedFetch: () => authFetch, + TokenStorage: { get: () => null }, + }; +}); + +vi.mock('@object-ui/app-shell', () => ({ + useAdapter: () => ADAPTER, + DeclaredActionsBar: () => null, + isViaOverrideRow: () => false, +})); + +// `buildApproverIdentities` stays REAL — the "can this approver act" pin is only +// worth anything if the identity resolution under it is the shipped one. +vi.mock('../../services/approvalsApi', async (importOriginal) => ({ + ...(await importOriginal>()), + approvalsApi: approvalsApiStub, +})); + +// Imported after the mocks so the page picks them up. +import { ApprovalsInboxPage } from './ApprovalsInboxPage'; + +function renderInbox() { + return render( + + + } /> + + , + ); +} + +/** The desktop table row holding `title` (the mobile card renders no links). */ +function rowFor(title: string): HTMLElement { + const found = screen.getAllByRole('row').find((r) => within(r).queryByText(title)); + if (!found) throw new Error(`no row for ${title}`); + return found; +} + +/** + * Approve the row holding `title`, through the row button and its confirmation. + * + * Query and click in ONE synchronous step, with `fireEvent`: the page declares + * `RecordCell` / `InlineActions` INSIDE its component body, so every re-render + * is a fresh component type and React replaces those DOM nodes. A multi-tick + * `userEvent` interaction lets a re-render land between its pointerdown and its + * click, and the rest of the sequence goes to a detached node — the click then + * silently does nothing and the confirmation never opens. + */ +async function approveFromRow(title: string): Promise { + fireEvent.click(within(rowFor(title)).getByRole('button', { name: 'Approve' })); + const dialog = await screen.findByRole('alertdialog'); + fireEvent.click(within(dialog).getByRole('button', { name: 'Approve' })); +} + +beforeEach(() => { + adapterFind.mockClear(); + for (const fn of Object.values(approvalsApiStub)) fn.mockClear(); +}); +afterEach(cleanup); + +describe('Approvals Inbox — record link readability (objectui#5211)', () => { + it('suppresses the record link on a row whose target this viewer cannot read', async () => { + renderInbox(); + + // The readable row is untouched: still a link, still the same href. + const link = await screen.findByRole('link', { name: /Readable Invoice/ }); + expect(link).toHaveAttribute('href', `/apps/${APP}/showcase_invoice/record/inv_readable`); + + // The unreadable one loses the link once the probe has answered… + await waitFor(() => + expect(screen.queryByRole('link', { name: /Hidden Invoice/ })).not.toBeInTheDocument(), + ); + // …and only the link. The title still shows — it comes from the request's + // payload snapshot, which is what the approver decides from. + expect(screen.getAllByText('Hidden Invoice').length).toBeGreaterThan(0); + expect(within(rowFor('Hidden Invoice')).queryByRole('link')).not.toBeInTheDocument(); + }); + + it('costs ONE readability read for a page of rows sharing an object', async () => { + renderInbox(); + await screen.findByRole('link', { name: /Readable Invoice/ }); + await waitFor(() => expect(adapterFind).toHaveBeenCalled()); + + // Two rows, one object => one batched list read. Not one per row. + expect(adapterFind).toHaveBeenCalledTimes(1); + expect(adapterFind).toHaveBeenCalledWith('showcase_invoice', { + $filter: { id: { $in: ['inv_readable', 'inv_hidden'] } }, + $select: ['id'], + $top: 2, + }); + }); + + /** + * A PIN, deliberately independent of the readability probe: it must pass on + * the tree BEFORE this change as well as after, which is what makes it + * evidence that the decision path was left alone rather than evidence that + * the change works. (It is stated separately from the case below because a + * pin coupled to the new behaviour proves nothing about the old.) + */ + it('pins the inline decision path — approve still fires for the row it is clicked on', async () => { + renderInbox(); + // Rows are up (desktop table + mobile card each render one per request). + await screen.findAllByRole('button', { name: 'Approve' }); + + await approveFromRow('Readable Invoice'); + + await waitFor(() => + expect(approvalsApiStub.approve).toHaveBeenCalledWith('req_readable', { actor_id: 'u_1' }), + ); + }); + + it('still lets the approver decide a row whose link was suppressed', async () => { + renderInbox(); + // Wait for the rows FIRST. Waiting only for the link's absence passes + // vacuously while the page is still a loading skeleton — measured: on the + // pre-change tree this case went green for that reason alone. + await screen.findByRole('link', { name: /Readable Invoice/ }); + await waitFor(() => + expect(screen.queryByRole('link', { name: /Hidden Invoice/ })).not.toBeInTheDocument(), + ); + + await approveFromRow('Hidden Invoice'); + + await waitFor(() => + expect(approvalsApiStub.approve).toHaveBeenCalledWith('req_hidden', { actor_id: 'u_1' }), + ); + }); + + it('keeps every link when the probe cannot answer (fail open)', async () => { + adapterFind.mockImplementationOnce(async () => { throw new Error('NETWORK'); }); + renderInbox(); + + await screen.findByRole('link', { name: /Readable Invoice/ }); + await waitFor(() => expect(adapterFind).toHaveBeenCalled()); + // Unknown is not "unreadable": a failed probe must never withhold a link + // from someone who could have used it. + expect(await screen.findByRole('link', { name: /Hidden Invoice/ })).toBeInTheDocument(); + expect(rows).toHaveLength(2); + }); +}); diff --git a/apps/console/src/pages/system/ApprovalsInboxPage.tsx b/apps/console/src/pages/system/ApprovalsInboxPage.tsx index 129206205..41016f24f 100644 --- a/apps/console/src/pages/system/ApprovalsInboxPage.tsx +++ b/apps/console/src/pages/system/ApprovalsInboxPage.tsx @@ -103,6 +103,7 @@ import { type ApprovalActionRow, type ApprovalActionAttachment, } from '../../services/approvalsApi'; +import { useRecordReadability } from './recordReadability'; type TabKey = 'pending' | 'submitted' | 'all'; @@ -478,6 +479,25 @@ export function ApprovalsInboxPage() { const [selected, setSelected] = useState(null); const [actions, setActions] = useState([]); const [drawerLoading, setDrawerLoading] = useState(false); + + /** + * objectui#5211 — an approver can be routed a request about a record they + * cannot read (approver routing goes by position; record visibility is a + * separate gate). The link below then dead-ends on the record page's + * "may have been deleted", so it is suppressed for exactly those rows. + * + * One batched list read per distinct object covers the whole page — see + * `recordReadability.ts` for the cost model, the fail-open rule, and why + * nothing here says anything about WHY a target is unreadable. + * + * `selected` joins the loaded rows because the drawer can be deep-linked + * (`?request=`) to a request that is not in the current row set. + */ + const readabilityTargets = useMemo( + () => (selected ? [...rows, selected] : rows), + [rows, selected], + ); + const readability = useRecordReadability(readabilityTargets); // Approve/reject/reassign/send-back/… are server-declared actions rendered by // DeclaredActionsBar (objectui#2697 + framework#3300); their param dialog // collects the comment and — since the shared upload-widget renderer (#2700/ @@ -1080,17 +1100,25 @@ export function ApprovalsInboxPage() { // Surface the decision-relevant amount inline so a reviewer can triage the // queue without opening each request (#2762 P1-3). const amount = decisionAmountEntry(r); + // objectui#5211: no link into a record this viewer cannot open. The title + // still shows — it comes from the request's own payload snapshot, which the + // approver was already given — it just stops being an anchor. + const title = r.record_title || formatIdentity(r.record_id); return (
+ {readability.isUnreadable(r) ? ( +
{title}
+ ) : ( e.stopPropagation()} className="inline-flex items-center gap-1 text-sm hover:underline truncate max-w-full" title={r.record_id} > - {r.record_title || formatIdentity(r.record_id)} + {title} + )}
{objectDisplay(r)} {amount && ( @@ -1680,6 +1708,15 @@ export function ApprovalsInboxPage() {
+ {/* Same suppression as the row (objectui#5211) — the drawer + offers the same link to the same record for the same + viewer, so fixing only the row would move the dead end + one click deeper instead of removing it. */} + {readability.isUnreadable(selected) ? ( +
+ {selected.record_title || formatIdentity(selected.record_id)} +
+ ) : ( {selected.record_title || formatIdentity(selected.record_id)} + )}
{objectDisplay(selected)}
@@ -2064,6 +2102,12 @@ export function ApprovalsInboxPage() { {tr('returnedHint', 'An approver sent this back to you. The record is unlocked — fix the data, then resubmit to start a new approval round.')}
+ {/* NOT readability-suppressed (objectui#5211), deliberately: + this branch renders only for the SUBMITTER of a returned + request, whose whole job here is to edit that record — + a different persona from the approver the suppression is + for, and one who demonstrably reached the record to + submit it. */}