Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions .changeset/6464-recall-submitter-gate.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
---
'@object-ui/react': minor
'@object-ui/plugin-detail': patch
'@object-ui/app-shell': patch
---

The record page's approval band offers its **Recall** button to the approval's submitter
only (objectui#6464).

Field report on `@objectstack/*@17.2.0`: user A submits a record into a 4-level approval;
user B — not the submitter, read access, not an admin — opens the record and the band still
lights a clickable recall button. The click cannot succeed. The recall endpoint authorizes
on submitter identity and refuses everyone else, so the only outcome available to that
button was a failure toast. Record state was never at risk; this was purely a
writability-feedback mismatch, the same family as objectui#3794.

The button's only gate was `dataSource.cancelPendingApproval` — "can this adapter recall at
all" — which is a question about the DataSource, not about the viewer. Identity now joins
it, threaded the way every other signal on that band already travels: the HOST resolves it
and passes it through `InlineEditProvider`, so the renderer stays DataSource-agnostic and
never re-derives who submitted what.

- `@object-ui/react` — `InlineEditProvider` accepts `approvalIsSubmitter`, surfaced on
`InlineEditContextValue`. Additive and optional; no existing prop changes.
- `@object-ui/plugin-detail` — the band's recall button is withdrawn when that signal is a
resolved `false`.
- `@object-ui/app-shell` — `RecordDetailView` resolves the verdict from its existing
approvals read and threads it.

**The signal is tri-state, and the third state is the load-bearing one.** `true` offers
recall, `false` withdraws it, and **`undefined` — a host that resolves no approval identity
— renders exactly as it did before this release.** Omission preserving prior behaviour
mirrors how `approvalPending` falls back to `locked`. Defaulting the unknown case to "hide"
would have traded a cosmetic defect for a functional loss: every host whose band runs off
the record's `approval_status` mirror alone would silently lose its submitter's only way to
unlock their own record.

**Withdrawn rather than disabled-with-reason.** The card offered either. For a
non-submitter this control is never actionable on any pending record, so a permanently
disabled button is standing clutter rather than a lesson; and the two sibling submitter
levers already hide — the approvals panel's Remind button, and the declared
`approval_recall` action's `visible` predicate. The band, its quorum tally and the
approvals timeline still tell a non-submitter exactly what state the record is in. Only the
lever they can never pull is gone.

**This changes no permission.** Nothing about what the server allows moves, `canEdit` and
the approval lock are untouched, and nothing downstream reads `approvalIsSubmitter` as an
authorization verdict — the recall endpoint remains the sole authority, and it refused
these callers before this change and refuses them after. There is deliberately **no admin
carve-out** (the reporter ruled that case out, cf. objectstack#9464).

The derivation itself is now one function, `isSubmitterOf` — server-resolved
`viewer.is_submitter` first (framework#3310), an id comparison as the fallback for backends
that predate it, joined with `??` so a server that resolved `false` is believed rather than
re-litigated client-side. The approvals panel's Remind gate, which already carried that
expression inline and whose behaviour is unchanged, now reads the same answer: two copies
would have been two definitions of who submitted.

The **untranslated refusal text** the reporter also saw ("No pending approval request found
for this record", concatenated after a localized prefix) is a separate defect and is not
addressed here; it is tracked on objectstack#11993.
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
/**
* 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 { isSubmitterOf, type ApprovalRequestLite } from '../useRecordApprovals';

/**
* `isSubmitterOf` — the ONE derivation of "did this viewer submit this
* approval" (objectui#6464).
*
* Two surfaces gate on the answer: the approvals panel's Remind button and the
* record band's Recall button. Both are server-authorized on submitter
* identity, so two client-side copies of the derivation would be two
* definitions of who submitted — the drift `utils/approverIdentity` already
* exists to prevent on the display side.
*
* What the cells below actually protect is the SOURCE ORDER. The server's
* `viewer.is_submitter` (framework#3310) is the resolution the endpoint will
* enforce; the id comparison exists only for backends that predate the `viewer`
* block. Joining them with `||` instead of `??` would look identical on every
* agreeing row and quietly overturn the server on the one row where it says no.
*/

const row = (over: Partial<ApprovalRequestLite> = {}): ApprovalRequestLite => ({
id: 'req_1',
process_name: 'flow:budget_review',
object_name: 'budget',
record_id: 'B1',
status: 'pending',
submitter_id: 'u_alice',
...over,
});

describe('isSubmitterOf — server-resolved first (objectui#6464)', () => {
it('believes the server when it says the viewer IS the submitter', () => {
expect(isSubmitterOf(row({ viewer: { can_act: false, is_submitter: true } }), 'u_bob')).toBe(true);
});

it('believes the server when it says the viewer is NOT the submitter', () => {
expect(isSubmitterOf(row({ viewer: { can_act: false, is_submitter: false } }), 'u_bob')).toBe(false);
});

/**
* The cell that separates `??` from `||`. The server resolved `false` while
* the raw `submitter_id` matches the signed-in id — a real shape whenever the
* server's answer accounts for something the bare column does not (a
* delegated or system-rewritten submission). `||` would re-litigate the
* server's refusal client-side and return `true`, lighting a lever the recall
* endpoint then rejects: the very defect this gate exists to close.
*/
it('does not let a matching submitter_id overturn a server `false`', () => {
expect(
isSubmitterOf(row({ submitter_id: 'u_alice', viewer: { can_act: false, is_submitter: false } }), 'u_alice'),
).toBe(false);
});
});

describe('isSubmitterOf — the id fallback for pre-`viewer` backends', () => {
it('matches the submitter by id when the server sends no viewer block', () => {
expect(isSubmitterOf(row({ submitter_id: 'u_alice' }), 'u_alice')).toBe(true);
});

it('rejects a non-submitter by id when the server sends no viewer block', () => {
expect(isSubmitterOf(row({ submitter_id: 'u_alice' }), 'u_bob')).toBe(false);
});

/**
* Fallback-also-absent: an older server AND no signed-in id to compare
* against. This resolves to a definite `false`, NOT to "unknown" — the
* authoritative row is in hand and nothing in it identifies this viewer as
* the submitter. It is also the same answer the Remind gate has always given
* in this shape, which is the point of there being one derivation.
*/
it('resolves to false when there is no id to compare against', () => {
expect(isSubmitterOf(row({ submitter_id: 'u_alice' }), undefined)).toBe(false);
expect(isSubmitterOf(row({ submitter_id: 'u_alice' }), null)).toBe(false);
expect(isSubmitterOf(row({ submitter_id: 'u_alice' }), '')).toBe(false);
});

it('resolves to false when the row itself names no submitter', () => {
// Both sides absent must not collapse into `undefined === undefined`.
expect(isSubmitterOf(row({ submitter_id: null }), undefined)).toBe(false);
expect(isSubmitterOf(row({ submitter_id: undefined }), undefined)).toBe(false);
});
});

describe('isSubmitterOf — "unknown" is its own answer', () => {
/**
* No request to consult is NOT a denial. Callers feeding a feedback gate keep
* their prior behaviour on `undefined`; collapsing it to `false` here would
* hide the affordance on every backend that exposes no approvals API at all.
* Asserted with `toBeUndefined`, since `toBeFalsy` passes for both.
*/
it('returns undefined — not false — with no request', () => {
expect(isSubmitterOf(null, 'u_alice')).toBeUndefined();
expect(isSubmitterOf(undefined, 'u_alice')).toBeUndefined();
});
});
37 changes: 37 additions & 0 deletions packages/app-shell/src/hooks/useRecordApprovals.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -199,6 +199,43 @@ export function recordLockedByApproval(request: ApprovalRequestLite | null | und
return request.lock_record !== false;
}

/**
* Did the current viewer submit this approval request?
*
* The submitter's levers — Remind on the approvals panel, Recall on the record
* band (objectui#6464) — are the ones the server authorizes on submitter
* identity and refuses to anyone else. This is the ONE place that answer is
* derived, so the two surfaces cannot drift into disagreeing about who the
* submitter is (the identical hazard `utils/approverIdentity` exists for on the
* display side).
*
* Source order, and it matters: the server-resolved `viewer.is_submitter`
* (framework#3310) wins, because the server already did the resolution the
* recall endpoint will enforce. The id comparison is the FALLBACK for backends
* that predate the `viewer` block — `??` and not `||`, so a server that
* resolved `false` is believed rather than being re-litigated client-side.
*
* Returns `undefined` — "unknown", not "no" — when there is no request to
* consult. A caller feeding a feedback gate must keep its prior behaviour on
* `undefined` rather than reading it as a denial; a caller that needs a plain
* boolean coerces explicitly. With a request in hand, an unresolvable viewer is
* a definite `false`: the authoritative row is present and nothing in it
* identifies this viewer as the submitter.
*
* ⚠️ FEEDBACK ONLY — this decides which affordances are SHOWN, never who MAY
* act. Every lever it gates is authorized server-side independently.
*/
export function isSubmitterOf(
request: ApprovalRequestLite | null | undefined,
currentUserId: string | null | undefined,
): boolean | undefined {
if (!request) return undefined;
return (
request.viewer?.is_submitter
?? (!!currentUserId && request.submitter_id === currentUserId)
);
}

interface UseRecordApprovalsResult {
loading: boolean;
available: boolean;
Expand Down
12 changes: 8 additions & 4 deletions packages/app-shell/src/views/RecordApprovalsPanel.tsx
Original file line numberDiff line numberDiff line change
Expand Up@@ -13,6 +13,7 @@ import { toast } from 'sonner';
import { createAuthenticatedFetch } from '@object-ui/auth';
import { useObjectTranslation } from '@object-ui/react';
import {
isSubmitterOf,
listApprovalActions,
remindApprovalRequest,
type ApprovalActionLite,
Expand DownExpand Up@@ -268,11 +269,14 @@ export const RecordApprovalsPanel: React.FC<RecordApprovalsPanelProps> = ({
* Remind is the submitter's lever (server-authorized): prefer the
* server-resolved `viewer.is_submitter` from the enriched pending row and
* fall back to a plain id match for backends predating framework#3310.
*
* The derivation moved to `isSubmitterOf` unchanged (objectui#6464) so the
* record band's Recall gate reads the SAME answer — two copies of it would be
* two definitions of who submitted. `?? false` restates this call site's own
* reading of "no pending request": there is nothing to remind about, so the
* button is gone either way.
*/
const isSubmitter = pendingRequest
? pendingRequest.viewer?.is_submitter
?? (!!currentUserId && pendingRequest.submitter_id === currentUserId)
: false;
const isSubmitter = isSubmitterOf(pendingRequest, currentUserId) ?? false;

const handleRemind = React.useCallback(async () => {
if (!pendingRequest) return;
Expand Down
Loading
Loading