Skip to content

Commit 53642d4

Browse files
baozhoutaoclaude
andauthored
fix(fields,core,detail): make the sharing-rule dialog usable — i18n, a picker that lists people, and permission-aware CTAs (#2920)
* fix(fields,core,detail): make the sharing-rule dialog usable — i18n, a picker that lists people, and permission-aware CTAs (objectstack#3821) The Console's "New sharing rule" dialog could not produce a working rule. **The recipient picker listed nothing, ever.** `QueryParams['$orderby']` was typed as `Record | string[] | SortObject[]`, so `queryParamsToRecord` fed any non-array value to `Object.entries`. Handed the clause string `'name asc'` — which callers do build by hand — it walked the string index by index and emitted `$orderby=0 n,1 a,2 m,…`. The server sorted by columns that don't exist and every row was filtered out, so `recipient_id` showed "No matches" for every recipient type. `ObjectGrid` builds the same shape from a schema-level `sort` in three places, so grids with a string sort silently showed an empty table. A string `$orderby` is now passed through verbatim; the type admits `string`, and `RecipientPickerField` switched to the structured form so it can't regress. **The three sharing-rule widgets never had translations.** `ObjectRefField`, `RecipientPickerField` and `FilterConditionField` hardcoded their copy, so a Chinese Console showed "Select an object", "Select a user", "No matches". They now go through `useFieldTranslation`, with keys in all ten locales. The recipient placeholder was the interesting one: it interpolated the enum value into an English sentence (`Select a ${type.replace(/_/g,' ')}`) — a shape no locale can translate — and is now a per-type key. **Editing a rule silently dropped its recipient.** The picker resets the stored id when `recipient_type` changes; it treated the edit form's `'' → 'user'` hydration as such a change, so opening a saved rule blanked the recipient and saving persisted the blank. Only a non-empty predecessor counts now. **Building a filter submitted the surrounding form.** No `FilterBuilder` control declared `type="button"`, and a bare <button> in a <form> defaults to submit — adding a condition fired validation mid-edit and, on a valid form, saved. **A rejected write showed the user raw server diagnostics.** The form rendered `error.message` verbatim: `FORBIDDEN: insufficient privileges to update showcase_private_note pi-TgoJ4_DM55Fqz` — untranslated, leaking the object's machine name and the record id. Permission failures now render localized copy; other failures keep the server's message, which is the useful part. **The detail header offered "Edit" on records the user may only read.** Object permissions can't say "this one record is read-only" — a read-only sharing grant is exactly that — so the CTA opened the form and the user only found out at save time. `RecordDetailView` and `DetailView` now also consult the explain engine's record-grained verdict (`POST /security/explain` with a `recordId`, ADR-0090 D6 / ADR-0095 C2 — the same pipeline the middleware runs). One cached request per record, skipped when the object-level check already says no, and fail-open on every uncertainty: a courtesy hint must never be why a permitted user can't act. The server stays the authority (ADR-0057 D10). Hardens `evaluatePermission` while there: a role config carrying only `fieldPermissions` (no `actions`) made `check()` throw out of the render. Browser-verified against the framework showcase Console in Chinese, end to end: copy is localized, the picker lists real users / business units / positions, a saved rule reopens intact, editing the filter no longer submits, and a read-only recipient gets no Edit button while the same recipient at `edit` level does — and can save. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(components,fields): a long combobox option truncates instead of running past the border (objectstack#3821) The sharing-rule object picker rendered "成员 (showcase_project_membership)" straight through the control's right edge and into the field beside it. Two causes, both mine from the picker work in this branch: - `Combobox`'s trigger keeps the component's `w-[200px]` default, so the object picker sat at a third of the form column while `名称` next to it ran full width — and it is the field holding the longest content on the form. - The selected label was a bare text child of a flex button. Flex items need `truncate` AND `min-w-0` to clip; it had neither, so the text simply drew outside the box instead of ellipsizing. The label now lives in a truncating span, the trigger may shrink, and the dropdown takes `--radix-popover-trigger-width` rather than its own hardcoded 200px — a combobox widened by its consumer used to clip its own options. The two sharing-rule pickers pass `w-full` so they line up with every other input; `w-[200px]` stays as the component default and consumers keep overriding it, so no other call site changes. Pinned by tests that assert the classes, since none of this is visible in an accessibility tree. Browser-measured both ways: at 1280px the trigger is 362px and the 247px label fits with room to spare; narrowed to 700px the label clips at 210px with `text-overflow: ellipsis` and stays inside the button. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 952b978 commit 53642d4

31 files changed

Lines changed: 1215 additions & 46 deletions
Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
---
2+
"@object-ui/core": patch
3+
"@object-ui/types": patch
4+
"@object-ui/fields": patch
5+
"@object-ui/i18n": patch
6+
"@object-ui/components": patch
7+
"@object-ui/plugin-detail": patch
8+
"@object-ui/permissions": patch
9+
---
10+
11+
fix(core,fields): a string `$orderby` is a clause, not a character array — and localize the sharing-rule widgets (objectstack#3821)
12+
13+
**The recipient picker listed nothing, ever.** `QueryParams['$orderby']` was
14+
typed as `Record | string[] | SortObject[]`, so `queryParamsToRecord` sent any
15+
non-array value through `Object.entries`. Handed the clause string `'name asc'`
16+
— which callers do build by hand — it walked the string index by index and
17+
emitted `$orderby=0 n,1 a,2 m,3 e,4 ,5 a,6 s,7 c`. The server sorted by columns
18+
that don't exist and every row was filtered out, so
19+
`sys_sharing_rule.recipient_id` rendered "No matches" for every recipient type
20+
and no sharing rule could be created from the Console. `ObjectGrid` builds the
21+
same shape from a schema-level `sort` in three places, so grids with a string
22+
sort silently showed an empty table.
23+
24+
A string `$orderby` is now passed through verbatim (the server's OData
25+
normalizer has always parsed `'name asc'`), and the type admits `string`.
26+
`RecipientPickerField` additionally switched to the structured
27+
`{ name: 'asc' }` form so it can't regress this way against any data source.
28+
29+
**The three sharing-rule authoring widgets never had translations.**
30+
`ObjectRefField`, `RecipientPickerField` and `FilterConditionField` hardcoded
31+
their English copy — a Chinese Console showed "Select an object", "Select a
32+
user", "Search…", "No matches", "Edit as JSON". They now go through
33+
`useFieldTranslation` like every other widget, with keys added under `fields.*`
34+
in all ten locales.
35+
36+
The recipient placeholder was the interesting one: it read
37+
`` `Select a ${recipientType.replace(/_/g,' ')}` ``, interpolating the enum
38+
value into an English sentence — a shape no locale can translate. It is now a
39+
per-type key (`fields.recipient.selectUser`, `…selectBusinessUnit`, …), so
40+
"选择业务单元" and "Select a business unit" no longer have to share a structure.
41+
42+
**Editing a rule silently dropped its recipient.** The picker resets the stored
43+
id when `recipient_type` changes, because an id valid for a user is meaningless
44+
for a team. It treated the edit form's `'' → 'user'` hydration as such a change:
45+
opening any saved rule blanked the recipient, and saving persisted the blank.
46+
Only a non-empty predecessor now counts as a type switch.
47+
48+
**Building a filter submitted the surrounding form.** None of `FilterBuilder`'s
49+
controls declared `type="button"`, and a bare `<button>` inside a `<form>`
50+
defaults to `type="submit"`. Adding, removing or clearing a condition therefore
51+
submitted the sharing-rule dialog — firing validation mid-edit, and on an
52+
already-valid form saving the record before the admin was done.
53+
54+
**A rejected write showed the user raw server diagnostics.** The form rendered
55+
`error.message` verbatim, so a sharing / RLS denial reached the dialog and the
56+
toast as `FORBIDDEN: insufficient privileges to update showcase_private_note
57+
pi-TgoJ4_DM55Fqz` — untranslated, and leaking the object's machine name and the
58+
record id to whoever hit it. Permission failures now render localized copy
59+
(`form.noPermissionToSave`, added in all ten locales), with the server text kept
60+
on the console for debugging; other failures still show the server's message,
61+
which is the useful part, and fall back to `form.submitFailed` when there is
62+
none — replacing the previously hardcoded English "An error occurred during
63+
submission".
64+
65+
**The detail header offered "Edit" on records the user may only read.** Object
66+
permissions can't express "this one record is read-only" — a read-only sharing
67+
grant sits inside an object the user may otherwise edit — so the header showed
68+
the primary Edit CTA, opened the form, and let the user retype a field before
69+
the server rejected the save. `DetailView` now gates Edit / Delete on the
70+
object-level check AND on the explain engine's record-grained verdict
71+
(`POST /api/v1/security/explain` with a `recordId`, ADR-0090 D6 / ADR-0095 C2 —
72+
the same pipeline the enforcement middleware runs, so button and server cannot
73+
disagree). Explaining oneself needs no special permission. The probe is one
74+
cached request per record, skipped entirely when the object-level check already
75+
says no, and **fails open** on every uncertainty — an unanswered hint must never
76+
be the reason a permitted user cannot act; the server stays the authority
77+
(ADR-0057 D10).
78+
79+
**A long option rendered straight past the combobox border.** `Combobox`'s
80+
trigger pinned itself to the component's `w-[200px]` default while the fields
81+
around it ran the full form column, and the selected label was a bare text child
82+
of a flex button — flex items need `truncate` AND `min-w-0` to clip, and it had
83+
neither. So "成员 (showcase_project_membership)" in the object picker overflowed
84+
the control and collided with the field beside it. The label now truncates, the
85+
trigger can shrink, the dropdown matches the trigger's width instead of a
86+
hardcoded 200px (a widened combobox used to clip its own options), and the two
87+
sharing-rule pickers ask for `w-full` so they line up with every other input.
88+
89+
Hardens `evaluatePermission` while there: a role config carrying only
90+
`fieldPermissions` (no `actions`) made `check()` throw a TypeError that
91+
propagated out of the render. A permission check must not be able to crash a
92+
view.
93+
94+
Browser-verified against the framework showcase Console in Chinese: object /
95+
criteria / recipient copy is fully localized, the recipient dropdown lists real
96+
users, business units and positions, a saved rule reopens with its recipient and
97+
criteria intact, editing the filter no longer submits, and a rule created
98+
end-to-end stores a real record id rather than free text. The criteria authored
99+
in the builder is honored by the evaluator: `{"pinned":true}` on an owner-private
100+
object granted the recipient exactly the matching records and nothing else.

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

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010

1111
import { useState, useEffect, useCallback, useMemo, useRef } from 'react';
1212
import { useParams, useNavigate, useLocation, useSearchParams, Link } from 'react-router-dom';
13-
import { RecordChatterPanel, InlineEditSaveBar, buildDefaultPageSchema, deriveFieldGroupDetailSections, extractMentions, resolveTitleField } from '@object-ui/plugin-detail';
13+
import { RecordChatterPanel, InlineEditSaveBar, buildDefaultPageSchema, deriveFieldGroupDetailSections, extractMentions, resolveTitleField, useRecordEditable } from '@object-ui/plugin-detail';
1414
import { Empty, EmptyTitle, EmptyDescription } from '@object-ui/components';
1515
import { useAuth, createAuthenticatedFetch } from '@object-ui/auth';
1616
import { usePermissions } from '@object-ui/permissions';
@@ -959,6 +959,26 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri
959959
[objectDef, objects, canOnObject, permissionsLoaded],
960960
);
961961

962+
// [objectstack#3821] RECORD-level write gate. Everything above is object
963+
// scoped — it cannot express "this one record is read-only", which is
964+
// exactly what a read-only sharing grant is. So the header offered Edit on
965+
// a record the user may only read: the form opened, the user retyped a
966+
// field, and the server rejected the save with a 403. Ask the explain
967+
// engine for the row-level verdict (fail-open; the server stays the
968+
// authority per ADR-0057 D10) and fold it into the same affordance gates.
969+
const recordWriteAllowed = useRecordEditable(
970+
objectDef?.name,
971+
pureRecordId,
972+
'update',
973+
!!objectDef?.name && !!pureRecordId,
974+
);
975+
const recordDeleteAllowed = useRecordEditable(
976+
objectDef?.name,
977+
pureRecordId,
978+
'delete',
979+
!!objectDef?.name && !!pureRecordId,
980+
);
981+
962982
// ── Audit history fetch ────────────────────────────────────────────
963983
// Loads recent sys_audit_log entries for this record so the record page can
964984
// render a read-only "History" tab (`record:history`). Gated on three preconditions to keep
@@ -1794,7 +1814,12 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri
17941814
// menu permanently — Delete must never surface as an inline red button
17951815
// just because an object has few actions.
17961816
const synthSystemActions: ActionDef[] = (() => {
1797-
const affordances = resolveRecordHeaderActionGates(objectDef, effectiveApiOperations);
1817+
const objectAffordances = resolveRecordHeaderActionGates(objectDef, effectiveApiOperations);
1818+
// Object-level gate AND the record-level verdict (objectstack#3821).
1819+
const affordances = {
1820+
edit: objectAffordances.edit && recordWriteAllowed,
1821+
delete: objectAffordances.delete && recordDeleteAllowed,
1822+
};
17981823
const items: ActionDef[] = [];
17991824
if (affordances.edit) {
18001825
// Single primary Edit CTA → opens the full record form. Inline editing
@@ -1942,7 +1967,7 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri
19421967
`lockRecord: false` still shows its band and recall button while
19431968
leaving the record editable (objectui#2902). */}
19441969
<InlineEditProvider
1945-
canEdit={resolveRecordHeaderActionGates(objectDef, effectiveApiOperations).edit && !approvalLocked}
1970+
canEdit={resolveRecordHeaderActionGates(objectDef, effectiveApiOperations).edit && recordWriteAllowed && !approvalLocked}
19461971
locked={approvalLocked}
19471972
approvalPending={approvalPending}
19481973
lockedReason={t('detail.lockedTooltip', {
Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
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+
/**
10+
* objectstack#3821 — a long option must ellipsize inside the trigger, not
11+
* render past its border.
12+
*
13+
* The selected label was a bare text child of a flex button, so
14+
* "成员 (showcase_project_membership)" in the sharing-rule object picker
15+
* overflowed the control and collided with the field beside it. Flex children
16+
* need `truncate` AND `min-w-0` to clip; either one alone does nothing.
17+
*/
18+
import { describe, it, expect, vi } from 'vitest';
19+
import React from 'react';
20+
import { render, screen } from '@testing-library/react';
21+
import '@testing-library/jest-dom';
22+
import { Combobox } from '../custom/combobox';
23+
24+
const LONG = '成员 (showcase_project_membership)';
25+
const OPTIONS = [{ value: 'showcase_project_membership', label: LONG }];
26+
27+
describe('Combobox trigger with a long label', () => {
28+
it('renders the label in a truncating element', () => {
29+
render(<Combobox options={OPTIONS} value={OPTIONS[0].value} onValueChange={vi.fn()} />);
30+
const label = screen.getByText(LONG);
31+
expect(label.className).toContain('truncate');
32+
});
33+
34+
it('lets the trigger shrink so truncation can engage', () => {
35+
render(<Combobox options={OPTIONS} value={OPTIONS[0].value} onValueChange={vi.fn()} />);
36+
// `truncate` is inert inside a flex item that refuses to shrink below its
37+
// content, which is what `min-w-0` on the trigger unlocks.
38+
expect(screen.getByRole('combobox').className).toContain('min-w-0');
39+
});
40+
41+
it('lets a consumer widen the trigger — the 200px default must not win', () => {
42+
render(
43+
<Combobox options={OPTIONS} value={OPTIONS[0].value} onValueChange={vi.fn()} className="w-full" />,
44+
);
45+
const cls = screen.getByRole('combobox').className;
46+
expect(cls).toContain('w-full');
47+
expect(cls).not.toContain('w-[200px]');
48+
});
49+
50+
it('truncates the placeholder too', () => {
51+
render(<Combobox options={OPTIONS} value="" onValueChange={vi.fn()} placeholder={LONG} />);
52+
expect(screen.getByText(LONG).className).toContain('truncate');
53+
});
54+
});
Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
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+
/**
10+
* FilterBuilder is embedded inside forms — notably the sharing-rule dialog's
11+
* `criteria_json` field. None of its controls declared `type="button"`, and a
12+
* bare <button> inside a <form> defaults to `type="submit"`, so building a
13+
* filter submitted the surrounding dialog: validation fired mid-edit, and on an
14+
* already-valid form the record saved before the admin was done
15+
* (objectstack#3821).
16+
*/
17+
import { describe, it, expect, vi } from 'vitest';
18+
import React from 'react';
19+
import { render, screen, fireEvent } from '@testing-library/react';
20+
import '@testing-library/jest-dom';
21+
import { FilterBuilder } from '../custom/filter-builder';
22+
23+
const FIELDS = [
24+
{ value: 'title', label: 'Title', type: 'text' },
25+
{ value: 'pinned', label: 'Pinned', type: 'boolean' },
26+
];
27+
28+
function renderInForm(onSubmit: () => void, value?: any) {
29+
return render(
30+
<form onSubmit={onSubmit}>
31+
<FilterBuilder
32+
fields={FIELDS as any}
33+
value={value}
34+
onChange={vi.fn()}
35+
showClearAll
36+
/>
37+
<button type="submit">save</button>
38+
</form>,
39+
);
40+
}
41+
42+
const CONDITION = {
43+
id: 'c1',
44+
logic: 'and',
45+
conditions: [{ id: 'c1a', field: 'title', operator: 'equals', value: 'x' }],
46+
};
47+
48+
describe('FilterBuilder never submits its surrounding form', () => {
49+
it('adding a condition does not submit', () => {
50+
const onSubmit = vi.fn((e: any) => e.preventDefault());
51+
renderInForm(onSubmit);
52+
fireEvent.click(screen.getByRole('button', { name: /add filter|/i }));
53+
expect(onSubmit).not.toHaveBeenCalled();
54+
});
55+
56+
it('removing a condition does not submit', () => {
57+
const onSubmit = vi.fn((e: any) => e.preventDefault());
58+
renderInForm(onSubmit, CONDITION);
59+
fireEvent.click(screen.getByRole('button', { name: /remove condition||/i }));
60+
expect(onSubmit).not.toHaveBeenCalled();
61+
});
62+
63+
it('clear-all does not submit', () => {
64+
const onSubmit = vi.fn((e: any) => e.preventDefault());
65+
renderInForm(onSubmit, CONDITION);
66+
fireEvent.click(screen.getByRole('button', { name: /clear all|/i }));
67+
expect(onSubmit).not.toHaveBeenCalled();
68+
});
69+
70+
it('every FilterBuilder control declares an explicit button type', () => {
71+
const { container } = renderInForm(vi.fn(), CONDITION);
72+
const implicitSubmit = Array.from(container.querySelectorAll('button')).filter(
73+
(b) => !b.getAttribute('type') && b.textContent !== 'save',
74+
);
75+
expect(implicitSubmit.map((b) => b.textContent)).toEqual([]);
76+
});
77+
});
Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
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+
/**
10+
* objectstack#3821 — a rejected write is user-facing copy, not server
11+
* diagnostics.
12+
*
13+
* The form used to render `error.message` verbatim, so a sharing/RLS denial
14+
* surfaced in the dialog and the toast as
15+
* `FORBIDDEN: insufficient privileges to update showcase_private_note
16+
* pi-TgoJ4_DM55Fqz` — untranslated, and leaking the object's machine name and
17+
* the record id to whoever hit it.
18+
*/
19+
import { describe, it, expect, vi, beforeEach, beforeAll } from 'vitest';
20+
import React from 'react';
21+
import { render, screen, fireEvent } from '@testing-library/react';
22+
import '@testing-library/jest-dom';
23+
import { ComponentRegistry } from '@object-ui/core';
24+
25+
beforeAll(async () => {
26+
await import('../renderers');
27+
}, 30000);
28+
29+
const SCHEMA = {
30+
type: 'form',
31+
fields: [{ name: 'title', label: 'Title', type: 'text' }],
32+
submitText: 'Save',
33+
} as any;
34+
35+
function forbiddenError() {
36+
const err: any = new Error(
37+
'FORBIDDEN: insufficient privileges to update showcase_private_note pi-TgoJ4_DM55Fqz',
38+
);
39+
err.code = 'FORBIDDEN';
40+
err.status = 403;
41+
return err;
42+
}
43+
44+
function renderForm(onAction: () => Promise<unknown>) {
45+
const Form = ComponentRegistry.get('form')!;
46+
// `onAction` is a PROP on the renderer, not part of the schema.
47+
return render(<Form schema={SCHEMA} onAction={onAction} />);
48+
}
49+
50+
describe('form write errors are user-facing copy', () => {
51+
beforeEach(() => {
52+
vi.spyOn(console, 'warn').mockImplementation(() => {});
53+
});
54+
55+
it('shows a permission message instead of the raw FORBIDDEN text', async () => {
56+
renderForm(async () => { throw forbiddenError(); });
57+
fireEvent.click(screen.getByRole("button", { name: /submit/i }));
58+
59+
const alert = await screen.findByText(/permission to save/i);
60+
expect(alert).toBeInTheDocument();
61+
});
62+
63+
it('never leaks the object machine name or the record id', async () => {
64+
const { container } = renderForm(async () => { throw forbiddenError(); });
65+
fireEvent.click(screen.getByRole("button", { name: /submit/i }));
66+
67+
await screen.findByText(/permission to save/i);
68+
expect(container.textContent).not.toMatch(/showcase_private_note/);
69+
expect(container.textContent).not.toMatch(/pi-TgoJ4_DM55Fqz/);
70+
expect(container.textContent).not.toMatch(/FORBIDDEN/);
71+
});
72+
73+
it('keeps the server text for non-permission failures — it is the useful part', async () => {
74+
renderForm(async () => { throw new Error('Title must be unique'); });
75+
fireEvent.click(screen.getByRole("button", { name: /submit/i }));
76+
77+
expect(await screen.findByText(/title must be unique/i)).toBeInTheDocument();
78+
});
79+
80+
it('falls back to a generic localized message when the error carries no text', async () => {
81+
renderForm(async () => { throw {}; });
82+
fireEvent.click(screen.getByRole("button", { name: /submit/i }));
83+
84+
expect(await screen.findByText(/could not save/i)).toBeInTheDocument();
85+
});
86+
});

0 commit comments

Comments
 (0)