diff --git a/.changeset/relatedlist-one-sort-semantics.md b/.changeset/relatedlist-one-sort-semantics.md new file mode 100644 index 000000000..c6b606ff2 --- /dev/null +++ b/.changeset/relatedlist-one-sort-semantics.md @@ -0,0 +1,32 @@ +--- +"@object-ui/plugin-detail": minor +--- + +fix(detail): a related list has one sorting semantics instead of two — #3106 + +A related list carried two. Its own sort-button row (opt-in via `sortable`) went +out as a server `$orderby` over the whole child collection; the `data-table` it +embeds took `sortable`'s default of `true` and sorted the rows it was holding — +which, in windowed mode, is **one page**. + +Turning `sortable` on put both in the same card, with nothing saying they meant +different things. Leaving it off — the default — was worse: the page-local sort +was then the *only* one the user could reach, and it looked exactly like the +list being sorted. + +The table's column headers now drive this list's sort in both modes, so there is +one order behind them: + +- **Windowed**: the header sort becomes the server `$orderby` and resets to page + one, the same path the buttons took. +- **Client mode**: this list keeps sorting in memory, where its key is the label + resolved through its own id → name map (#3096) — a key the embedded table + cannot see, so its sort was the worse of the two even when both were possible. + +The button row survives only where there are no headers to click: a `list` +(`data-list`) related list, or a caller-supplied `schema` whose contents we +cannot assume. `sortable`'s documentation now says that is what it controls. + +Relational columns keep #3096's rule, moved to the header: no sort affordance +while the sort is a server `$orderby` (the key would be the stored foreign-key +id while the cell shows a name), live in client mode where the key is the label. diff --git a/packages/plugin-detail/src/RelatedList.tsx b/packages/plugin-detail/src/RelatedList.tsx index 380f838e1..52c947fd5 100644 --- a/packages/plugin-detail/src/RelatedList.tsx +++ b/packages/plugin-detail/src/RelatedList.tsx @@ -111,7 +111,17 @@ export interface RelatedListProps { * `$orderby`, keeping page windows deterministic. */ defaultSort?: string | Array<{ field: string; order: 'asc' | 'desc' }>; - /** Enable column sorting */ + /** + * Render the standalone row of sort buttons above the list. + * + * Only meaningful for a `list` (`data-list`) related list, which has no + * column headers to click. A `grid`/`table` one sorts through its table + * headers, which are live regardless of this flag — they always were, and + * since objectui#3106 they sort the collection rather than the page, so the + * button row above them would be a second control over the same order. + * + * @default false + */ sortable?: boolean; /** Enable text filtering */ filterable?: boolean; @@ -591,6 +601,38 @@ export const RelatedList: React.FC = ({ } }, [sortField]); + /** + * The order the embedded table's headers display — this list's own sort, so + * the arrow on a column header and the rows underneath it come from the same + * place (objectui#3106). + * + * Before any click that is the declared `defaultSort`, which is what the + * windowed fetch above sends. A header showing nothing while the server was + * asked for `created_at desc` would make the first click on that column + * request `asc` on a list that is already `desc`. + */ + const activeSort = React.useMemo( + () => (sortField ? [{ field: sortField, order: sortDirection }] : defaultSortSpec), + [sortField, sortDirection, defaultSortSpec], + ); + + /** + * A header click from the embedded table. It arrives with the direction + * already resolved against {@link activeSort}, so this assigns rather than + * toggling — running it back through `handleSort`'s own toggle would undo it + * whenever the two disagreed about the current state. + */ + const handleTableSort = React.useCallback( + (next: Array<{ field: string; order: 'asc' | 'desc' }>) => { + const first = next[0]; + if (!first) return; + setCurrentPage(0); + setSortField(first.field); + setSortDirection(first.order); + }, + [], + ); + // Confirm-delete dialog state. Replaces window.confirm() so the related // list matches the rest of the app's Shadcn AlertDialog UX. const [deleteTarget, setDeleteTarget] = React.useState(null); @@ -836,6 +878,36 @@ export const RelatedList: React.FC = ({ return pruned.slice(0, Math.max(1, maxColumns)); }, [columns, objectSchema, objectName, api, resolveFieldLabel, referenceField, relatedData, maxColumns, lookupLabels, perms]); + /** + * The same columns, with the sort affordance withheld from relational ones + * while the sort is the server's. + * + * A windowed sort goes out as `$orderby` on the flat field name, and a + * relational column stores a foreign-key id — so "sort by Owner" would order + * the collection by `rec_7f3…` while the cells show names (objectui#3096; + * objectstack#4256 settled that no relation join is coming). This is the rule + * the sort-button row already applied; the headers inherit it rather than + * re-opening the same door. + * + * In client mode the sort keys off the resolved label, so those headers stay + * live — the same split `sortedData` makes. + */ + const sortableColumns = React.useMemo(() => { + if (!windowed) return effectiveColumns; + return effectiveColumns.map((col: any) => { + const field = col.accessorKey || col.field || col.name; + return field && isExpandableFieldType((objectSchema?.fields as any)?.[field]) + ? { ...col, sortable: false } + : col; + }); + }, [effectiveColumns, windowed, objectSchema]); + + // A `grid`/`table` list renders a real table, whose column headers carry the + // sort. `list` renders `data-list`, which has none — so it keeps the button + // row as its only sort control. A caller-supplied `schema` renders whatever + // it says, so we cannot claim headers for it either. + const hasSortableHeaders = !schema && (type === 'grid' || type === 'table'); + const hasCustomRowActions = Array.isArray(rowActions) && rowActions.length > 0; const hasRowActions = !!onRowEdit || !!onRowDelete || hasCustomRowActions; const isMobile = useIsMobile(); @@ -883,11 +955,24 @@ export const RelatedList: React.FC = ({ return { type: 'data-table', data: paginatedData, - columns: effectiveColumns, + columns: sortableColumns, pagination: false, // We handle pagination ourselves pageSize: effectivePageSize || 10, searchable: false, exportable: false, + // Sorting is THIS list's, in both modes (objectui#3106). + // + // Windowed: `data` is one page, so a table-local sort would order + // that page and call it the list — the defect. Client mode: this + // list's own sort resolves a relational column through the label map + // it built (`lookupLabels`), which the table cannot see, so its sort + // is the better one even when both are possible. + // + // Either way there is now ONE sort behind the headers instead of a + // table-local order sitting on top of a server order. + manualSorting: true, + sort: activeSort, + onSortChange: handleTableSort, rowActions: hasRowActions, onRowEdit, onRowDelete: onRowDelete ? handleDeleteRow : undefined, @@ -907,7 +992,7 @@ export const RelatedList: React.FC = ({ default: return { type: 'div', children: 'No view configured' }; } - }, [type, paginatedData, effectiveColumns, schema, effectivePageSize, hasRowActions, hasCustomRowActions, rowActions, onRowAction, onRowEdit, onRowDelete, handleDeleteRow, onRowClick, isMobile, api, objectSchema]); + }, [type, paginatedData, sortableColumns, effectiveColumns, schema, effectivePageSize, hasRowActions, hasCustomRowActions, rowActions, onRowAction, onRowEdit, onRowDelete, handleDeleteRow, onRowClick, isMobile, api, objectSchema, activeSort, handleTableSort]); const headerClassName = collapsible ? 'cursor-pointer select-none' : undefined; const handleHeaderClick = collapsible ? () => setCollapsed((c) => !c) : undefined; @@ -1008,8 +1093,15 @@ export const RelatedList: React.FC = ({ )} - {/* Sortable column headers */} - {sortable && effectiveColumns && effectiveColumns.length > 0 && relatedData.length > 0 && ( + {/* Sort buttons — only where there are no column headers to click. + A `grid`/`table` related list renders a real table whose headers now + drive this same sort (objectui#3106), so a second row of buttons + above it would be two controls over one order — and, before the + headers were wired up, two DIFFERENT orders: the buttons sorted the + collection through the server while the headers sorted the page in + the browser, in one card, with nothing saying so. `data-list` has no + headers, so there the buttons stay the only way to sort. */} + {sortable && !hasSortableHeaders && effectiveColumns && effectiveColumns.length > 0 && relatedData.length > 0 && (
{effectiveColumns.map((col: any) => { const field = col.accessorKey || col.field || col.name; diff --git a/packages/plugin-detail/src/__tests__/RelatedList.headerSort.test.tsx b/packages/plugin-detail/src/__tests__/RelatedList.headerSort.test.tsx new file mode 100644 index 000000000..940b2a465 --- /dev/null +++ b/packages/plugin-detail/src/__tests__/RelatedList.headerSort.test.tsx @@ -0,0 +1,233 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * One sorting semantics per related list (objectui#3106). + * + * A related list used to carry two. Its own sort-button row (opt-in via + * `sortable`) went out as a server `$orderby` over the whole child collection; + * the table it embeds took `sortable`'s default of `true` and sorted the rows + * it held — which, in windowed mode, is one page. Turning `sortable` on put + * both in the same card, with nothing saying they meant different things; and + * leaving it off — the default — made the page-local one the *only* sort the + * user could reach. + * + * Now the table's headers drive this list's sort in both modes, and the button + * row survives only where there are no headers to click (`data-list`). + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, waitFor, screen } from '@testing-library/react'; +import * as React from 'react'; +import { RelatedList } from '../RelatedList'; + +// Capture the schema RelatedList hands to SchemaRenderer (the data-table). +const h = vi.hoisted(() => ({ schema: null as any })); +vi.mock('@object-ui/react', async (importOriginal) => { + const actual = await importOriginal>(); + return { + ...actual, + SchemaRenderer: (props: any) => { + h.schema = props.schema; + return null; + }, + }; +}); + +const rows = (start: number, count: number) => + Array.from({ length: count }, (_, i) => ({ + id: `c${start + i}`, + name: `Row ${start + i}`, + owner: `usr_${start + i}`, + })); + +const columns = [ + { accessorKey: 'name', header: 'Name' }, + { accessorKey: 'owner', header: 'Owner' }, +]; + +const makeWindowedDS = (totalRecords = 12) => ({ + find: vi.fn(async (_api: string, params: any) => { + const skip = params?.$skip ?? 0; + const top = params?.$top ?? totalRecords; + return { + data: rows(skip, Math.max(0, Math.min(top, totalRecords - skip))), + total: totalRecords, + }; + }), + getObjectSchema: vi.fn(async () => ({ + name: 'contact', + fields: { + name: { type: 'text' }, + owner: { type: 'lookup', reference_to: 'user' }, + account: { type: 'lookup', reference_to: 'account' }, + }, + })), +}); + +/** + * The most recent fetch OF THE CHILD COLLECTION. + * + * Not simply the last `find` call: a `lookup` column makes this list resolve + * the related records' labels through the same dataSource, so the last call is + * usually a label lookup against `user` with no `$orderby` at all. + */ +const lastFindParams = (ds: any) => { + const calls = ds.find.mock.calls.filter((c: any[]) => c[0] === 'contact'); + return calls[calls.length - 1]?.[1]; +}; + +function renderWindowed(ds: any, props?: Record) { + return render( + , + ); +} + +beforeEach(() => { + h.schema = null; +}); + +describe('RelatedList — the embedded table sorts the collection, not the window', () => { + it('hands the table controlled sorting instead of letting it sort locally', async () => { + const ds = makeWindowedDS(); + renderWindowed(ds); + await waitFor(() => expect(h.schema?.type).toBe('data-table')); + + expect(h.schema.manualSorting).toBe(true); + expect(Array.isArray(h.schema.sort)).toBe(true); + expect(typeof h.schema.onSortChange).toBe('function'); + }); + + it('turns a header sort into a server $orderby over the whole collection', async () => { + const ds = makeWindowedDS(); + renderWindowed(ds); + await waitFor(() => expect(typeof h.schema?.onSortChange).toBe('function')); + + await React.act(async () => { + h.schema.onSortChange([{ field: 'name', order: 'desc' }]); + }); + + await waitFor(() => { + expect(lastFindParams(ds).$orderby).toEqual([{ field: 'name', order: 'desc' }]); + }); + }); + + it('assigns the direction the header resolved rather than re-toggling it', async () => { + // The table sends the direction it computed against `sort`. Running that + // back through the button row's own toggle would invert it. + const ds = makeWindowedDS(); + renderWindowed(ds); + await waitFor(() => expect(typeof h.schema?.onSortChange).toBe('function')); + + await React.act(async () => { + h.schema.onSortChange([{ field: 'name', order: 'desc' }]); + }); + await waitFor(() => expect(lastFindParams(ds).$orderby).toEqual([{ field: 'name', order: 'desc' }])); + + await React.act(async () => { + h.schema.onSortChange([{ field: 'name', order: 'desc' }]); + }); + await waitFor(() => expect(lastFindParams(ds).$orderby).toEqual([{ field: 'name', order: 'desc' }])); + }); + + it('returns to the first page when the order changes', async () => { + const ds = makeWindowedDS(); + renderWindowed(ds); + await waitFor(() => expect(typeof h.schema?.onSortChange).toBe('function')); + + await React.act(async () => { + h.schema.onSortChange([{ field: 'name', order: 'asc' }]); + }); + + await waitFor(() => { + const p = lastFindParams(ds); + expect(p.$orderby).toEqual([{ field: 'name', order: 'asc' }]); + expect(p.$skip ?? 0).toBe(0); + }); + }); + + it('shows the declared defaultSort before anyone clicks — the same one it fetched with', async () => { + const ds = makeWindowedDS(); + renderWindowed(ds, { defaultSort: '-name' }); + await waitFor(() => expect(h.schema?.type).toBe('data-table')); + + expect(h.schema.sort).toEqual([{ field: 'name', order: 'desc' }]); + expect(lastFindParams(ds).$orderby).toEqual([{ field: 'name', order: 'desc' }]); + }); + + it('withholds the header from a relational column while windowed (#3096)', async () => { + // `$orderby` on `owner` orders by the stored id while the cell shows a + // name. This is the rule the sort-button row already applied. + const ds = makeWindowedDS(); + renderWindowed(ds); + await waitFor(() => expect(h.schema?.columns?.length).toBeGreaterThan(0)); + + const owner = h.schema.columns.find((c: any) => c.accessorKey === 'owner'); + const name = h.schema.columns.find((c: any) => c.accessorKey === 'name'); + expect(owner.sortable).toBe(false); + expect(name.sortable).not.toBe(false); + }); + + it('keeps a relational header live in client mode, where the key is the label', async () => { + // Not windowed (caller supplied `data`), so this list sorts in memory + // through its resolved label map — an honest key, unlike the stored id. + render( + , + ); + await waitFor(() => expect(h.schema?.columns?.length).toBeGreaterThan(0)); + + const owner = h.schema.columns.find((c: any) => c.accessorKey === 'owner'); + expect(owner.sortable).not.toBe(false); + // Still controlled: this list owns the order in both modes, so there is + // never a table-local sort sitting on top of it. + expect(h.schema.manualSorting).toBe(true); + }); +}); + +describe('RelatedList — one sort control per card', () => { + it('renders no sort-button row for a table list, even with sortable on', async () => { + // The headers carry the sort. A button row above them would be a second + // control over one order — and, before #3106, a second ORDER. + const ds = makeWindowedDS(); + renderWindowed(ds, { sortable: true }); + await waitFor(() => expect(h.schema?.type).toBe('data-table')); + + expect(screen.queryByRole('button', { name: /^Name/ })).toBeNull(); + expect(screen.queryByRole('button', { name: /^Owner/ })).toBeNull(); + }); + + it('keeps the button row for a `list` related list, which has no headers', async () => { + render( + , + ); + await waitFor(() => expect(h.schema?.type).toBe('data-list')); + + expect(screen.getByRole('button', { name: /Name/ })).toBeTruthy(); + }); +}); diff --git a/packages/plugin-detail/src/__tests__/RelatedList.relationalSort.test.tsx b/packages/plugin-detail/src/__tests__/RelatedList.relationalSort.test.tsx index 91182214d..a21432c46 100644 --- a/packages/plugin-detail/src/__tests__/RelatedList.relationalSort.test.tsx +++ b/packages/plugin-detail/src/__tests__/RelatedList.relationalSort.test.tsx @@ -13,10 +13,17 @@ * * - windowed (server `$orderby` over the whole child collection): the key is * the stored foreign-key id, and no join is available to reach the related - * record's name (objectstack#4256). The button is withheld. + * record's name (objectstack#4256). The affordance is withheld. * - client mode (sorting the rows already in memory): the key can be — and * now is — the label the cell shows, resolved from this list's own - * id → name map. The button stays, and it sorts by the name. + * id → name map. The affordance stays, and it sorts by the name. + * + * The guarantee is unchanged since #3106 moved the affordance; what carries it + * is now the embedded table's column headers rather than a separate row of sort + * buttons, so these assertions read the column's `sortable` flag and drive the + * table's `onSortChange` instead of clicking buttons that no longer exist for a + * `table` list. (The button row survives for `data-list`, which has no headers + * — covered in `RelatedList.headerSort.test.tsx`.) */ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { render, waitFor, fireEvent, screen } from '@testing-library/react'; @@ -72,14 +79,18 @@ const makeDataSource = () => ({ }), }); -const sortButton = (name: RegExp) => screen.queryByRole('button', { name }); +/** Whether the embedded table offers a sort on this column. */ +const columnSortable = (accessorKey: string) => { + const col = h.schema?.columns?.find((c: any) => c.accessorKey === accessorKey); + return col ? col.sortable !== false : undefined; +}; beforeEach(() => { h.schema = null; }); describe('RelatedList sort entry points — relational columns (objectui#3096)', () => { - it('withholds the relational sort button when the sort is a server $orderby', async () => { + it('withholds the relational column header when the sort is a server $orderby', async () => { const dataSource = makeDataSource(); render( , ); - // A plain column still offers its button — proving the row rendered at all. - await waitFor(() => expect(sortButton(/^name/i)).not.toBeNull()); - expect(sortButton(/^owner/i)).toBeNull(); + // A plain column stays sortable — proving the table rendered at all. + await waitFor(() => expect(columnSortable('name')).toBe(true)); + expect(columnSortable('owner')).toBe(false); }); it('keeps it in client mode and orders by the resolved label, not the id', async () => { @@ -119,9 +130,10 @@ describe('RelatedList sort entry points — relational columns (objectui#3096)', // Wait for the id → name map to resolve; until then the cells show ids. await waitFor(() => expect(dataSource.find).toHaveBeenCalledWith('sys_user', expect.anything())); - const owner = sortButton(/^owner/i); - expect(owner).not.toBeNull(); - fireEvent.click(owner!); + expect(columnSortable('owner')).toBe(true); + await React.act(async () => { + h.schema.onSortChange([{ field: 'owner', order: 'asc' }]); + }); await waitFor(() => expect(h.schema.data.map((row: any) => owners[row.owner])).toEqual([ diff --git a/packages/plugin-detail/src/__tests__/RelatedList.serverpagination.test.tsx b/packages/plugin-detail/src/__tests__/RelatedList.serverpagination.test.tsx index d5b3516fc..a3b7762a6 100644 --- a/packages/plugin-detail/src/__tests__/RelatedList.serverpagination.test.tsx +++ b/packages/plugin-detail/src/__tests__/RelatedList.serverpagination.test.tsx @@ -196,7 +196,12 @@ describe('RelatedList — server-windowed pagination (#2711)', () => { fireEvent.click(nextButton()); await screen.findByText('Page 2 of 3'); - fireEvent.click(screen.getByRole('button', { name: /name/i })); + // The sort now arrives from the embedded table's column headers rather + // than a separate row of sort buttons (objectui#3106) — same server + // `$orderby`, same page reset, one control instead of two. + await React.act(async () => { + h.schema.onSortChange([{ field: 'name', order: 'asc' }]); + }); await waitFor(() => { expect(ds.find).toHaveBeenCalledWith('contact', { $filter: { account: 'ACC-1' }, diff --git a/packages/plugin-detail/src/__tests__/RelatedList.systemcolumns.test.tsx b/packages/plugin-detail/src/__tests__/RelatedList.systemcolumns.test.tsx index d59092c2a..7d783bf3b 100644 --- a/packages/plugin-detail/src/__tests__/RelatedList.systemcolumns.test.tsx +++ b/packages/plugin-detail/src/__tests__/RelatedList.systemcolumns.test.tsx @@ -9,10 +9,26 @@ * column cap. System audit fields are now sorted last. */ import { describe, it, expect, vi } from 'vitest'; -import { render, screen, waitFor } from '@testing-library/react'; +import { render, waitFor } from '@testing-library/react'; import React from 'react'; import { RelatedList } from '../RelatedList'; +// Read the column order straight off the schema handed to the renderer. This +// used to infer it from the row of sort buttons, which a `table` list no longer +// renders now that its column headers carry the sort (objectui#3106) — and +// which was always a proxy for the thing under test anyway. +const h = vi.hoisted(() => ({ schema: null as any })); +vi.mock('@object-ui/react', async (importOriginal) => { + const actual = await importOriginal>(); + return { + ...actual, + SchemaRenderer: (props: any) => { + h.schema = props.schema; + return null; + }, + }; +}); + // Declare the system audit fields FIRST to reproduce the pre-fix ordering. const fields = { created_at: { type: 'datetime', label: 'Created At' }, @@ -49,13 +65,12 @@ describe('RelatedList — system audit columns are deprioritized', () => { />, ); - // `sortable` renders one button per effective column, in column order. const labels = await waitFor(() => { - const texts = screen.getAllByRole('button').map((b) => (b.textContent || '').trim()); - if (!texts.some((t) => t.includes('Product'))) throw new Error('headers not ready'); + const texts = (h.schema?.columns ?? []).map((c: any) => String(c.header ?? '').trim()); + if (!texts.some((t: string) => t.includes('Product'))) throw new Error('columns not ready'); return texts; }); - const idx = (s: string) => labels.findIndex((t) => t.includes(s)); + const idx = (s: string) => labels.findIndex((t: string) => t.includes(s)); expect(idx('Product')).toBeGreaterThanOrEqual(0); // A business field must lead; any shown system audit column comes after it.