Skip to content

Commit 830ed58

Browse files
fix(app-shell,plugin-detail,react): offer approval recall to the submitter only (#6485)
The record page's approval band lit its Recall button for every reader of a pending record. Recall is authorized server-side on submitter identity and refused to everyone else, so a non-submitter was handed a lever whose click could only produce a failure toast — a writability-feedback mismatch, not a permission question. Record state was never at risk. The button's only gate was `dataSource.cancelPendingApproval`, which asks whether the ADAPTER can recall, not whether the VIEWER may. Identity now joins it, threaded the way the rest of the band's signals already travel: the host resolves it and passes `InlineEditProvider.approvalIsSubmitter`, so the renderer stays DataSource-agnostic. The signal is tri-state and the third state is load-bearing: `undefined` — a host that resolves no approval identity — renders exactly as before, so no host silently loses its submitter's only unlock lever. Withdrawn rather than disabled-with-reason, matching the sibling submitter levers (the approvals panel's Remind, the declared `approval_recall` action's `visible` predicate). `isSubmitterOf` is now the one derivation of who submitted — server-resolved `viewer.is_submitter` first, id comparison as the pre-framework#3310 fallback, joined with `??` so a server `false` is believed rather than re-litigated. The Remind gate reads the same answer; its behaviour is unchanged. No permission changes: `canEdit` and the approval lock are untouched, nothing downstream reads this as authorization, and there is deliberately no admin carve-out. The untranslated refusal text seen in the same report is tracked separately on objectstack#11993 and is out of scope here. Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q Co-authored-by: Claude <noreply@anthropic.com>
1 parent 7fdf74e commit 830ed58

10 files changed

Lines changed: 817 additions & 8 deletions
Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
---
2+
'@object-ui/react': minor
3+
'@object-ui/plugin-detail': patch
4+
'@object-ui/app-shell': patch
5+
---
6+
7+
The record page's approval band offers its **Recall** button to the approval's submitter
8+
only (objectui#6464).
9+
10+
Field report on `@objectstack/*@17.2.0`: user A submits a record into a 4-level approval;
11+
user B — not the submitter, read access, not an admin — opens the record and the band still
12+
lights a clickable recall button. The click cannot succeed. The recall endpoint authorizes
13+
on submitter identity and refuses everyone else, so the only outcome available to that
14+
button was a failure toast. Record state was never at risk; this was purely a
15+
writability-feedback mismatch, the same family as objectui#3794.
16+
17+
The button's only gate was `dataSource.cancelPendingApproval` — "can this adapter recall at
18+
all" — which is a question about the DataSource, not about the viewer. Identity now joins
19+
it, threaded the way every other signal on that band already travels: the HOST resolves it
20+
and passes it through `InlineEditProvider`, so the renderer stays DataSource-agnostic and
21+
never re-derives who submitted what.
22+
23+
- `@object-ui/react` — `InlineEditProvider` accepts `approvalIsSubmitter`, surfaced on
24+
`InlineEditContextValue`. Additive and optional; no existing prop changes.
25+
- `@object-ui/plugin-detail` — the band's recall button is withdrawn when that signal is a
26+
resolved `false`.
27+
- `@object-ui/app-shell` — `RecordDetailView` resolves the verdict from its existing
28+
approvals read and threads it.
29+
30+
**The signal is tri-state, and the third state is the load-bearing one.** `true` offers
31+
recall, `false` withdraws it, and **`undefined` — a host that resolves no approval identity
32+
— renders exactly as it did before this release.** Omission preserving prior behaviour
33+
mirrors how `approvalPending` falls back to `locked`. Defaulting the unknown case to "hide"
34+
would have traded a cosmetic defect for a functional loss: every host whose band runs off
35+
the record's `approval_status` mirror alone would silently lose its submitter's only way to
36+
unlock their own record.
37+
38+
**Withdrawn rather than disabled-with-reason.** The card offered either. For a
39+
non-submitter this control is never actionable on any pending record, so a permanently
40+
disabled button is standing clutter rather than a lesson; and the two sibling submitter
41+
levers already hide — the approvals panel's Remind button, and the declared
42+
`approval_recall` action's `visible` predicate. The band, its quorum tally and the
43+
approvals timeline still tell a non-submitter exactly what state the record is in. Only the
44+
lever they can never pull is gone.
45+
46+
**This changes no permission.** Nothing about what the server allows moves, `canEdit` and
47+
the approval lock are untouched, and nothing downstream reads `approvalIsSubmitter` as an
48+
authorization verdict — the recall endpoint remains the sole authority, and it refused
49+
these callers before this change and refuses them after. There is deliberately **no admin
50+
carve-out** (the reporter ruled that case out, cf. objectstack#9464).
51+
52+
The derivation itself is now one function, `isSubmitterOf` — server-resolved
53+
`viewer.is_submitter` first (framework#3310), an id comparison as the fallback for backends
54+
that predate it, joined with `??` so a server that resolved `false` is believed rather than
55+
re-litigated client-side. The approvals panel's Remind gate, which already carried that
56+
expression inline and whose behaviour is unchanged, now reads the same answer: two copies
57+
would have been two definitions of who submitted.
58+
59+
The **untranslated refusal text** the reporter also saw ("No pending approval request found
60+
for this record", concatenated after a localized prefix) is a separate defect and is not
61+
addressed here; it is tracked on objectstack#11993.
Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
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 { isSubmitterOf, type ApprovalRequestLite } from '../useRecordApprovals';
11+
12+
/**
13+
* `isSubmitterOf` — the ONE derivation of "did this viewer submit this
14+
* approval" (objectui#6464).
15+
*
16+
* Two surfaces gate on the answer: the approvals panel's Remind button and the
17+
* record band's Recall button. Both are server-authorized on submitter
18+
* identity, so two client-side copies of the derivation would be two
19+
* definitions of who submitted — the drift `utils/approverIdentity` already
20+
* exists to prevent on the display side.
21+
*
22+
* What the cells below actually protect is the SOURCE ORDER. The server's
23+
* `viewer.is_submitter` (framework#3310) is the resolution the endpoint will
24+
* enforce; the id comparison exists only for backends that predate the `viewer`
25+
* block. Joining them with `||` instead of `??` would look identical on every
26+
* agreeing row and quietly overturn the server on the one row where it says no.
27+
*/
28+
29+
const row = (over: Partial<ApprovalRequestLite> = {}): ApprovalRequestLite => ({
30+
id: 'req_1',
31+
process_name: 'flow:budget_review',
32+
object_name: 'budget',
33+
record_id: 'B1',
34+
status: 'pending',
35+
submitter_id: 'u_alice',
36+
...over,
37+
});
38+
39+
describe('isSubmitterOf — server-resolved first (objectui#6464)', () => {
40+
it('believes the server when it says the viewer IS the submitter', () => {
41+
expect(isSubmitterOf(row({ viewer: { can_act: false, is_submitter: true } }), 'u_bob')).toBe(true);
42+
});
43+
44+
it('believes the server when it says the viewer is NOT the submitter', () => {
45+
expect(isSubmitterOf(row({ viewer: { can_act: false, is_submitter: false } }), 'u_bob')).toBe(false);
46+
});
47+
48+
/**
49+
* The cell that separates `??` from `||`. The server resolved `false` while
50+
* the raw `submitter_id` matches the signed-in id — a real shape whenever the
51+
* server's answer accounts for something the bare column does not (a
52+
* delegated or system-rewritten submission). `||` would re-litigate the
53+
* server's refusal client-side and return `true`, lighting a lever the recall
54+
* endpoint then rejects: the very defect this gate exists to close.
55+
*/
56+
it('does not let a matching submitter_id overturn a server `false`', () => {
57+
expect(
58+
isSubmitterOf(row({ submitter_id: 'u_alice', viewer: { can_act: false, is_submitter: false } }), 'u_alice'),
59+
).toBe(false);
60+
});
61+
});
62+
63+
describe('isSubmitterOf — the id fallback for pre-`viewer` backends', () => {
64+
it('matches the submitter by id when the server sends no viewer block', () => {
65+
expect(isSubmitterOf(row({ submitter_id: 'u_alice' }), 'u_alice')).toBe(true);
66+
});
67+
68+
it('rejects a non-submitter by id when the server sends no viewer block', () => {
69+
expect(isSubmitterOf(row({ submitter_id: 'u_alice' }), 'u_bob')).toBe(false);
70+
});
71+
72+
/**
73+
* Fallback-also-absent: an older server AND no signed-in id to compare
74+
* against. This resolves to a definite `false`, NOT to "unknown" — the
75+
* authoritative row is in hand and nothing in it identifies this viewer as
76+
* the submitter. It is also the same answer the Remind gate has always given
77+
* in this shape, which is the point of there being one derivation.
78+
*/
79+
it('resolves to false when there is no id to compare against', () => {
80+
expect(isSubmitterOf(row({ submitter_id: 'u_alice' }), undefined)).toBe(false);
81+
expect(isSubmitterOf(row({ submitter_id: 'u_alice' }), null)).toBe(false);
82+
expect(isSubmitterOf(row({ submitter_id: 'u_alice' }), '')).toBe(false);
83+
});
84+
85+
it('resolves to false when the row itself names no submitter', () => {
86+
// Both sides absent must not collapse into `undefined === undefined`.
87+
expect(isSubmitterOf(row({ submitter_id: null }), undefined)).toBe(false);
88+
expect(isSubmitterOf(row({ submitter_id: undefined }), undefined)).toBe(false);
89+
});
90+
});
91+
92+
describe('isSubmitterOf — "unknown" is its own answer', () => {
93+
/**
94+
* No request to consult is NOT a denial. Callers feeding a feedback gate keep
95+
* their prior behaviour on `undefined`; collapsing it to `false` here would
96+
* hide the affordance on every backend that exposes no approvals API at all.
97+
* Asserted with `toBeUndefined`, since `toBeFalsy` passes for both.
98+
*/
99+
it('returns undefined — not false — with no request', () => {
100+
expect(isSubmitterOf(null, 'u_alice')).toBeUndefined();
101+
expect(isSubmitterOf(undefined, 'u_alice')).toBeUndefined();
102+
});
103+
});

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

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,43 @@ export function recordLockedByApproval(request: ApprovalRequestLite | null | und
199199
return request.lock_record !== false;
200200
}
201201

202+
/**
203+
* Did the current viewer submit this approval request?
204+
*
205+
* The submitter's levers — Remind on the approvals panel, Recall on the record
206+
* band (objectui#6464) — are the ones the server authorizes on submitter
207+
* identity and refuses to anyone else. This is the ONE place that answer is
208+
* derived, so the two surfaces cannot drift into disagreeing about who the
209+
* submitter is (the identical hazard `utils/approverIdentity` exists for on the
210+
* display side).
211+
*
212+
* Source order, and it matters: the server-resolved `viewer.is_submitter`
213+
* (framework#3310) wins, because the server already did the resolution the
214+
* recall endpoint will enforce. The id comparison is the FALLBACK for backends
215+
* that predate the `viewer` block — `??` and not `||`, so a server that
216+
* resolved `false` is believed rather than being re-litigated client-side.
217+
*
218+
* Returns `undefined` — "unknown", not "no" — when there is no request to
219+
* consult. A caller feeding a feedback gate must keep its prior behaviour on
220+
* `undefined` rather than reading it as a denial; a caller that needs a plain
221+
* boolean coerces explicitly. With a request in hand, an unresolvable viewer is
222+
* a definite `false`: the authoritative row is present and nothing in it
223+
* identifies this viewer as the submitter.
224+
*
225+
* ⚠️ FEEDBACK ONLY — this decides which affordances are SHOWN, never who MAY
226+
* act. Every lever it gates is authorized server-side independently.
227+
*/
228+
export function isSubmitterOf(
229+
request: ApprovalRequestLite | null | undefined,
230+
currentUserId: string | null | undefined,
231+
): boolean | undefined {
232+
if (!request) return undefined;
233+
return (
234+
request.viewer?.is_submitter
235+
?? (!!currentUserId && request.submitter_id === currentUserId)
236+
);
237+
}
238+
202239
interface UseRecordApprovalsResult {
203240
loading: boolean;
204241
available: boolean;

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

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import { toast } from 'sonner';
1313
import { createAuthenticatedFetch } from '@object-ui/auth';
1414
import { useObjectTranslation } from '@object-ui/react';
1515
import {
16+
isSubmitterOf,
1617
listApprovalActions,
1718
remindApprovalRequest,
1819
type ApprovalActionLite,
@@ -268,11 +269,14 @@ export const RecordApprovalsPanel: React.FC<RecordApprovalsPanelProps> = ({
268269
* Remind is the submitter's lever (server-authorized): prefer the
269270
* server-resolved `viewer.is_submitter` from the enriched pending row and
270271
* fall back to a plain id match for backends predating framework#3310.
272+
*
273+
* The derivation moved to `isSubmitterOf` unchanged (objectui#6464) so the
274+
* record band's Recall gate reads the SAME answer — two copies of it would be
275+
* two definitions of who submitted. `?? false` restates this call site's own
276+
* reading of "no pending request": there is nothing to remind about, so the
277+
* button is gone either way.
271278
*/
272-
const isSubmitter = pendingRequest
273-
? pendingRequest.viewer?.is_submitter
274-
?? (!!currentUserId && pendingRequest.submitter_id === currentUserId)
275-
: false;
279+
const isSubmitter = isSubmitterOf(pendingRequest, currentUserId) ?? false;
276280

277281
const handleRemind = React.useCallback(async () => {
278282
if (!pendingRequest) return;

0 commit comments

Comments
 (0)