diff --git a/.changeset/12286-master-detail-stored-order.md b/.changeset/12286-master-detail-stored-order.md new file mode 100644 index 0000000000..bee070168c --- /dev/null +++ b/.changeset/12286-master-detail-stored-order.md @@ -0,0 +1,25 @@ +--- +'@object-ui/plugin-form': patch +'@object-ui/fields': patch +'@object-ui/types': patch +--- + +Saving a master-detail edit form no longer rewrites the position of lines the +user did not touch (objectui#12286). + +The edit form read each collection's lines with no sort, so its line grid drew +them in whatever order the server returned, and the grid stamped every line's +index into the child's sort field (`position`, `sort_order`, …) on any change. +Editing one cell of one line therefore saved a new position onto its untouched +siblings, swapping their stored order and bumping their `updated_at`. + +- `MasterDetailForm` now reads the lines ascending by the child's derived sort + field, the record id breaking ties, so the grid opens in stored order. +- The line grid (`field:grid` with a `sortField`) renumbers only when a row is + added, removed, duplicated or moved. A cell edit leaves every row's position + as it was, so lines whose stored positions are tied, empty or gapped are not + renumbered by an edit either. + +Editing one line's field now sends exactly one update, and moving a line sends +only the lines whose position changed. The `GridFieldMetadata.sortField` +description says when the grid stamps the field. diff --git a/content/docs/fields/grid.mdx b/content/docs/fields/grid.mdx index 03ec5c33f2..c430b24c9c 100644 --- a/content/docs/fields/grid.mdx +++ b/content/docs/fields/grid.mdx @@ -69,9 +69,11 @@ The field-level keys, each read under exactly this one spelling: the save writes the sum to. No total shows when it is unset. - `addLabel` — the Add button's label. - `sortField` — the name of a field on each **row** that the grid stamps with the - row's index (0, 1, 2, …) on every change, so the order a drag-reorder leaves is - saved with the rows. It is not one of the columns: a column of that name would - have its typed value overwritten. Rows carry no position when it is unset. When + row's index (0, 1, 2, …) whenever a row is added, removed, duplicated or moved, + so the order a drag-reorder leaves is saved with the rows. Editing a cell moves + no row and leaves every row's position as it was. It is not one of the columns: + a column of that name would have its typed value overwritten. Rows carry no + position when it is unset. When a master-detail form derives a detail's grid from the child object (it does unless the detail names its relationship field and types every column), it sets this to the child's first field named `position`, `sort_order`, `sequence`, diff --git a/content/docs/plugins/plugin-form.mdx b/content/docs/plugins/plugin-form.mdx index dc7325d664..ec1e0b98cf 100644 --- a/content/docs/plugins/plugin-form.mdx +++ b/content/docs/plugins/plugin-form.mdx @@ -484,6 +484,13 @@ Remove asks the delete grant on an edit form only: a create form saves the paren and one create per line, so a line removed before that first save is a draft, never a server delete, and stays removable for a caller who may only create. +An edit `MasterDetailForm` reads each collection's lines in their stored order: +ascending by the child's sort field (the field its grid stamps with each line's +position), the record id breaking ties, so lines that share a position, or have +none, open in the same order every time. Its save sends a line only when one of +its values differs from that read: editing a cell sends that line alone, and +moving a line sends only the lines whose position changed. + ## Examples ### Form with Validation diff --git a/packages/fields/src/widgets/GridField.fieldKeys-11070.test.tsx b/packages/fields/src/widgets/GridField.fieldKeys-11070.test.tsx index fb41b307c9..b24d6444af 100644 --- a/packages/fields/src/widgets/GridField.fieldKeys-11070.test.tsx +++ b/packages/fields/src/widgets/GridField.fieldKeys-11070.test.tsx @@ -32,8 +32,9 @@ * `totalField` names); `amount_field` and `amountField` change nothing; * - `addLabel` — declared, and it labels the Add button; * - `sortField` — declared (round 9): it names the CHILD field each row is - * stamped with its index in, on every change, so a drag-reorder persists; - * with no key the rows carry no position; + * stamped with its index in, on every change that adds, removes or moves a + * row, so a drag-reorder persists; with no key the rows carry no position + * (a cell edit leaves positions alone: `GridField.sortFieldCellEdit-12286`); * - `allow_duplicate` and `show_line_numbers` — retired under ADR-0049 (no * producer in either repository wrote them): the widget keeps the * behaviour their defaults gave, a duplicate action whenever rows can be diff --git a/packages/fields/src/widgets/GridField.sortFieldCellEdit-12286.test.tsx b/packages/fields/src/widgets/GridField.sortFieldCellEdit-12286.test.tsx new file mode 100644 index 0000000000..729ca26b26 --- /dev/null +++ b/packages/fields/src/widgets/GridField.sortFieldCellEdit-12286.test.tsx @@ -0,0 +1,68 @@ +/** + * 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. + */ + +/** + * A cell edit leaves every row's `sortField` as it was (objectui#12286). + * + * The grid stamped `row[sortField] = index` on EVERY change, a plain cell edit + * included. Whenever the stored positions were not already 0, 1, 2, … in the + * order drawn (two rows tied on one position, rows with none, gaps), editing + * one cell gave every row a new position, and a master-detail save then wrote + * each of those rows although the user had touched one. A cell edit moves no + * row, so it renumbers nothing; adding, removing, duplicating and moving a row + * still renumber (the drag-reorder row is pinned in + * `GridField.fieldKeys-11070.test.tsx`). + * + * The pins read the rows the grid hands to `onChange`, which is everything a + * host persists. + */ + +import { describe, expect, it, vi } from 'vitest'; +import { fireEvent, render, screen } from '@testing-library/react'; +import React from 'react'; +import type { GridFieldMetadata } from '@object-ui/types'; +import { GridField } from './GridField'; + +const field: GridFieldMetadata = { + type: 'grid', + name: 'lines', + columns: [{ name: 'description', label: 'Description', type: 'text' }], + sortField: 'position', +}; + +type Row = Record; + +/** Render `rows`, type `value` into row `row`'s Description cell, and return what the grid handed back. */ +function typeInto(rows: Row[], row: number, value: string): Row[] { + const onChange = vi.fn(); + render(); + fireEvent.change(screen.getAllByLabelText('Description')[row], { target: { value } }); + expect(onChange).toHaveBeenCalledTimes(1); + return onChange.mock.calls[0][0]; +} + +describe('GridField: a cell edit renumbers no row (objectui#12286)', () => { + it.each([ + ['tied', [{ description: 'A', position: 0 }, { description: 'B', position: 0 }]], + ['gapped', [{ description: 'A', position: 10 }, { description: 'B', position: 20 }]], + ['empty', [{ description: 'A', position: null }, { description: 'B', position: null }]], + ['absent', [{ description: 'A' }, { description: 'B' }]], + ])('%s positions: editing row 2 changes its cell and every position stays as read', (_label, rows) => { + const next = typeInto(rows, 1, 'B2'); + expect(next).toEqual([rows[0], { ...rows[1], description: 'B2' }]); + }); + + it('CONTROL: typing into the trailing entry row ADDS a line, and an add renumbers every row', () => { + const next = typeInto([{ description: 'A', position: 0 }, { description: 'B', position: 0 }], 2, 'C'); + expect(next.map((r) => [r.description, r.position])).toEqual([ + ['A', 0], + ['B', 1], + ['C', 2], + ]); + }); +}); diff --git a/packages/fields/src/widgets/GridField.tsx b/packages/fields/src/widgets/GridField.tsx index b81c41b27b..8e7d43f95c 100644 --- a/packages/fields/src/widgets/GridField.tsx +++ b/packages/fields/src/widgets/GridField.tsx @@ -770,8 +770,13 @@ function GridFieldBody({ // census having found no producer of either (objectui#11070 round 8). const totalField: string | undefined = cfg.totalField; // When set, the row's order is persisted by stamping `row[sortField] = index` - // on every change — so drag-reorder survives a reload (the app adds a numeric - // position field and lists sort by it). Without it, reorder is order-of-entry. + // on every change that adds, removes or moves a row — so drag-reorder + // survives a reload (the app adds a numeric position field and lists sort by + // it). Without it, reorder is order-of-entry. A cell edit moves no row, so it + // leaves every row's position as it was read (objectui#12286): renumbering + // there wrote a position onto lines the user never touched whenever the + // stored positions were not already 0, 1, 2, … in the order drawn (tied, + // empty, or gapped). // Declared on `GridFieldMetadata`; its producer is `deriveDetail` // (`@object-ui/plugin-form`, through `MasterDetailForm`), which picks the // child object's `position` / `sort_order` / … field (objectui#11070 round 9). @@ -782,9 +787,10 @@ function GridFieldBody({ // (objectui#11070 round 8). const allowReorder = cfg.allowReorder !== false && !readonly && !disabled; + /** Hand the next rows out; `renumber: false` is a cell edit, which moves no row. */ const emit = useCallback( - (next: Row[]) => { - onChange?.(sortField ? next.map((r, i) => ({ ...r, [sortField]: i })) : next); + (next: Row[], renumber = true) => { + onChange?.(sortField && renumber ? next.map((r, i) => ({ ...r, [sortField]: i })) : next); }, [onChange, sortField], ); @@ -809,7 +815,7 @@ function GridFieldBody({ emit([...rows, computeRow(columns, { ...blankRow(), ...patch }, tenantCurrency)]); return; } - emit(rows.map((r, i) => (i === rowIdx ? computeRow(columns, { ...r, ...patch }, tenantCurrency) : r))); + emit(rows.map((r, i) => (i === rowIdx ? computeRow(columns, { ...r, ...patch }, tenantCurrency) : r)), false); }, [rows, columns, maxRows, blankRow, emit, tenantCurrency], ); diff --git a/packages/plugin-form/README.md b/packages/plugin-form/README.md index b41d8d9775..c47406663c 100644 --- a/packages/plugin-form/README.md +++ b/packages/plugin-form/README.md @@ -947,6 +947,13 @@ through the wizard, and the parent operation of a master-detail form, whose header is a simple or a `tabbed` form. A master-detail child row already worked this way (objectui#10108), and all of them use the same comparison. +A master-detail edit reads each collection's lines in their stored order: +ascending by the child's sort field (the one its line grid stamps with each +line's position), the record id breaking ties (objectui#12286). The grid +renumbers that field only when a line is added, removed, duplicated or moved; a +cell edit leaves every line's position as it was read. So editing one line +sends one update, and moving a line sends only the lines whose position changed. + Before that comparison, every layout strips what a form never writes: the server-owned columns (record identity, audit provenance, ownership and tenancy — the roster in `sanitize.ts`, plus any field the object marks `system`), diff --git a/packages/plugin-form/src/MasterDetailForm.storedLineOrder-12286.test.tsx b/packages/plugin-form/src/MasterDetailForm.storedLineOrder-12286.test.tsx new file mode 100644 index 0000000000..3acddfa38a --- /dev/null +++ b/packages/plugin-form/src/MasterDetailForm.storedLineOrder-12286.test.tsx @@ -0,0 +1,233 @@ +/** + * 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. + */ + +/** + * A master-detail edit opens its lines in STORED order, and its save writes + * only the lines that changed (objectui#12286). + * + * Two halves, and each alone leaves a case: + * + * - **Read.** The lines were read with no sort, so the grid drew the server's + * order. The grid stamps each line's index into the child's sort field, so + * the next renumber wrote that arbitrary order over the stored one: an + * untouched line was written, its position swapped. The read now asks for + * the lines ascending by the derived sort field, the record id breaking ties. + * - **Write.** The grid renumbered on every change, a cell edit included. On + * lines whose stored positions are tied or empty, even a read in stored order + * gives them new positions, so editing one cell wrote every line. A cell + * edit now renumbers nothing; adding, removing and moving a line still do, + * and the save (which already sent only the fields that differ from the read, + * objectui#10108) sends a line only when one of its values moved. + * + * Every row drives the real `MasterDetailForm` with the real line grid. The + * fixture is the showcase invoice line's shape (a `position` number field that + * is not a column, identity-only inline columns, as `attachInlineSubforms` + * builds them), and the data source is a fake SERVER: it returns the lines in + * its own order, which is not the stored order, unless the read asks for one, + * and it reads only the `$orderby` form the form sends, so a read in another + * shape fails loudly rather than passing unsorted. + */ +import { describe, it, expect, vi } from 'vitest'; +import { render, waitFor, fireEvent, screen, act } from '@testing-library/react'; +import React from 'react'; + +import { registerAllFields } from '@object-ui/fields'; +import { MasterDetailForm } from './MasterDetailForm'; + +registerAllFields(); + +const INV_SCHEMA = { name: 'inv', fields: { ref: { type: 'text', label: 'Ref' } } }; +const LINE_SCHEMA = { + name: 'inv_line', + fields: { + invoice: { type: 'master_detail', label: 'Invoice', reference: 'inv', inlineEdit: 'grid' }, + // Derived as the grid's sort field (its name is `position`); never a column. + position: { type: 'number', label: 'Position', defaultValue: 0 }, + description: { type: 'text', label: 'Description' }, + quantity: { type: 'number', label: 'Qty' }, + }, +}; +/** What `attachInlineSubforms` builds for a relationship field with identity-only `inlineColumns`. */ +const DETAIL = { + childObject: 'inv_line', + relationshipField: 'invoice', + inlineMode: 'grid', + title: 'Line Items', + columns: [{ name: 'description' }, { name: 'quantity' }], +}; + +type Row = Record; +type Op = { object: string; action?: string; id?: string; data?: Record }; +type Sort = Array<{ field: string; order?: 'asc' | 'desc' }>; + +/** Order rows by an `$orderby` array, as a server does; nulls sort first ascending (SQLite's order). */ +function serverOrder(rows: Row[], orderby: unknown): Row[] { + if (orderby === undefined) return rows; + if (!Array.isArray(orderby) || orderby.some((s) => typeof s !== 'object')) { + throw new Error(`fake server: unread $orderby shape ${JSON.stringify(orderby)}`); + } + return [...rows].sort((a, b) => { + for (const { field, order } of orderby as Sort) { + const av = a[field] as number | string | null | undefined; + const bv = b[field] as number | string | null | undefined; + if (av === bv || (av == null && bv == null)) continue; + const cmp = av == null ? -1 : bv == null ? 1 : av < bv ? -1 : 1; + return order === 'desc' ? -cmp : cmp; + } + return 0; + }); +} + +/** The two members of a read's query the fake server answers. */ +type ReadParams = { $filter?: { invoice?: unknown }; $orderby?: unknown } | undefined; + +/** Mount an edit form over `table`, the server's lines in the server's own order. */ +function renderForm(table: Row[]) { + const batch = vi.fn(async (ops: Op[]) => ({ + results: ops.map((op) => (op.action === 'delete' ? true : { id: op.id ?? 'new', ...op.data })), + })); + const ds = { + getObjectSchema: vi.fn(async (o: string) => (o === 'inv_line' ? LINE_SCHEMA : INV_SCHEMA)), + findOne: vi.fn().mockResolvedValue({ id: 'inv1', ref: 'INV-1' }), + find: vi.fn(async (o: string, params: ReadParams) => + o === 'inv_line' + ? { + data: serverOrder( + table.filter((r) => r.invoice === params?.$filter?.invoice), + params?.$orderby, + ).map((r) => ({ ...r })), + } + : { data: [] }, + ), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + batchTransaction: batch, + }; + const view = render( + , + ); + return { ...view, ds, batch }; +} + +type FakeServer = ReturnType['ds']; + +const descriptions = () => screen.getAllByLabelText('Description') as HTMLInputElement[]; +const qtys = () => screen.getAllByLabelText('Qty') as HTMLInputElement[]; +const saveButton = () => screen.getByTestId('md-form-submit') as HTMLButtonElement; +/** The query of each read of the lines, in the order they were sent. */ +const lineReads = (ds: FakeServer): ReadParams[] => + ds.find.mock.calls.filter(([o]) => o === 'inv_line').map(([, params]) => params); + +/** + * The parent record and `lines` stored lines have landed (plus the grid's + * trailing entry row), from the read sent once the detail RESOLVED. The lines + * are read twice: once on mount, before the child schema has loaded and the + * sort field is derived, and again once it has; the second read supersedes the + * first. Waiting for both keeps a pin from reading the first one's rows. + */ +async function ready(container: HTMLElement, ds: FakeServer, lines: number) { + await waitFor(() => { + const ref = container.querySelector('input[name="ref"]') as HTMLInputElement | null; + expect(ref?.value).toBe('INV-1'); + }); + await waitFor(() => expect(lineReads(ds).length).toBeGreaterThanOrEqual(2)); + await act(async () => {}); + await waitFor(() => expect(descriptions()).toHaveLength(lines + 1)); + await waitFor(() => expect(saveButton().disabled).toBe(false)); +} + +/** The order the grid draws: each stored line's description, the entry row left off. */ +const drawn = () => descriptions().slice(0, -1).map((el) => el.value); + +/** Save, and return the batch's child operations. */ +async function saveChildOps(batch: ReturnType['batch']): Promise { + await act(async () => { + fireEvent.click(saveButton()); + }); + await waitFor(() => expect(batch).toHaveBeenCalledTimes(1)); + return (batch.mock.calls[0][0] as Op[]).slice(1); +} + +/** The invoice's two lines; the server returns Widget first, and Consulting is stored first. */ +const TWO_LINES: Row[] = [ + { id: 'l2', invoice: 'inv1', description: 'Widget A units', quantity: 4, position: 1 }, + { id: 'l1', invoice: 'inv1', description: 'Consulting hours', quantity: 10, position: 0 }, +]; + +describe('a master-detail edit opens its lines in stored order and writes only what changed (objectui#12286)', () => { + it('the grid opens in stored order when the server returns another, and the read asks for it', async () => { + const { container, ds } = renderForm(TWO_LINES); + await ready(container, ds, 2); + expect(drawn()).toEqual(['Consulting hours', 'Widget A units']); + // The read whose rows the grid holds: ascending by the derived sort field, + // the record id breaking ties. + expect(lineReads(ds).at(-1)).toEqual({ + $filter: { invoice: 'inv1' }, + $orderby: [ + { field: 'position', order: 'asc' }, + { field: 'id', order: 'asc' }, + ], + $top: 500, + }); + }); + + it("editing one line's field sends exactly one update, and it carries no position", async () => { + const { container, ds, batch } = renderForm(TWO_LINES); + await ready(container, ds, 2); + await act(async () => { + fireEvent.change(qtys()[0], { target: { value: '11' } }); + }); + expect(await saveChildOps(batch)).toEqual([ + { object: 'inv_line', action: 'update', id: 'l1', data: { quantity: 11 } }, + ]); + }); + + it('moving one line sends only the lines whose position changed', async () => { + const { container, ds, batch } = renderForm([ + { id: 'c', invoice: 'inv1', description: 'C', quantity: 3, position: 2 }, + { id: 'a', invoice: 'inv1', description: 'A', quantity: 1, position: 0 }, + { id: 'b', invoice: 'inv1', description: 'B', quantity: 2, position: 1 }, + ]); + await ready(container, ds, 3); + expect(drawn()).toEqual(['A', 'B', 'C']); + // Drag line 3 (C) onto line 2 (B): A stays at 0, C and B swap. + const target = screen.getByTestId('line-items-drag-1').closest('tr')!; + await act(async () => { + fireEvent.dragStart(screen.getByTestId('line-items-drag-2')); + fireEvent.dragOver(target); + fireEvent.drop(target); + }); + expect(drawn()).toEqual(['A', 'C', 'B']); + expect(await saveChildOps(batch)).toEqual([ + { object: 'inv_line', action: 'update', id: 'c', data: { position: 1 } }, + { object: 'inv_line', action: 'update', id: 'b', data: { position: 2 } }, + ]); + }); + + it.each([ + ['tied', 0], + ['empty', null], + ])('%s positions: the lines open in id order, and editing one sends that line alone', async (_label, position) => { + const { container, ds, batch } = renderForm([ + { id: 'l2', invoice: 'inv1', description: 'Second', quantity: 2, position }, + { id: 'l1', invoice: 'inv1', description: 'First', quantity: 1, position }, + ]); + await ready(container, ds, 2); + expect(drawn()).toEqual(['First', 'Second']); + await act(async () => { + fireEvent.change(qtys()[1], { target: { value: '5' } }); + }); + expect(await saveChildOps(batch)).toEqual([ + { object: 'inv_line', action: 'update', id: 'l2', data: { quantity: 5 } }, + ]); + }); +}); diff --git a/packages/plugin-form/src/MasterDetailForm.tsx b/packages/plugin-form/src/MasterDetailForm.tsx index a74472c1c5..8dd7131a12 100644 --- a/packages/plugin-form/src/MasterDetailForm.tsx +++ b/packages/plugin-form/src/MasterDetailForm.tsx @@ -1396,6 +1396,14 @@ export const MasterDetailForm: React.FC = ({ try { const res = await dataSource.find(d.childObject, { $filter: { [d.relationshipField]: schema.recordId }, + // objectui#12286 — the lines open in their STORED order: ascending by + // the sort field the grid stamps, the record id breaking ties (two + // lines on one position, or none on any), so the same lines always + // open in the same order. Unsorted, the grid drew the server's order + // and its next renumber wrote that order over every line. + ...(entry.sortField + ? { $orderby: [{ field: entry.sortField, order: 'asc' as const }, { field: 'id', order: 'asc' as const }] } + : {}), $top: 500, }); rows = (res?.data ?? []) as Record[]; diff --git a/packages/types/src/field-types.ts b/packages/types/src/field-types.ts index e23321c27b..b8a99c8aa1 100644 --- a/packages/types/src/field-types.ts +++ b/packages/types/src/field-types.ts @@ -1258,10 +1258,12 @@ export interface GridFieldMetadata extends BaseFieldMetadata { addLabel?: string; /** * The CHILD field the grid stamps with each row's index (0, 1, 2, …) on - * every change, so the order a drag-reorder leaves survives a save and a - * reload. It names a field on the row object, not one of {@link columns}: - * a column that is also the sort field has its typed value overwritten on - * every change. Rows keep the order they were entered in when it is unset. + * every change that adds, removes or moves a row, so the order a + * drag-reorder leaves survives a save and a reload. A cell edit moves no + * row and leaves every row's value of it as it was. It names a field on the + * row object, not one of {@link columns}: a column that is also the sort + * field has its typed value overwritten by every such change. Rows keep the + * order they were entered in when it is unset. * * Its producer is `deriveDetail` in `@object-ui/plugin-form`, through * `MasterDetailForm`: the first of the child object's fields named