diff --git a/.changeset/10120-form-omits-fls-denied-fields.md b/.changeset/10120-form-omits-fls-denied-fields.md index 2387455ae6..ae166c7dba 100644 --- a/.changeset/10120-form-omits-fls-denied-fields.md +++ b/.changeset/10120-form-omits-fls-denied-fields.md @@ -13,7 +13,7 @@ The verdict that answers 「may this caller edit this field」 already existed a **What changed, in observable terms.** -- The field-level verdict now arrives at the ONE outbound filter as a predicate (`sanitizeFormData`'s `canEdit`), instead of as a strip loop written out after each container's call. ⚠️ `DrawerForm` previously sent every displayed field regardless of the caller's field permissions; it no longer does. `ObjectForm` and `ModalForm` produce the same payloads they did before — their loops were correct, they were just copies. +- The field-level verdict now arrives at the ONE outbound filter as a predicate (`sanitizeFormData`'s `canEdit`), instead of as a strip loop written out after each container's call. ⚠️ `DrawerForm` previously sent every displayed field regardless of the caller's field permissions; it no longer does. `ObjectForm` and `ModalForm` already withheld the refused field, and still do — their loops were correct, they were just copies. - The render pass is likewise one function for all three containers. ⚠️ `DrawerForm` previously drew a field the caller may read but not edit as a live input; it now draws it read-only and disabled, exactly as the other two already did. A field the caller may not READ is dropped, also as before. - Both halves stay fail-open with no `PermissionProvider` / `MePermissionsProvider` mounted, unchanged: a standalone form, a designer preview and a guest surface have no resolvable principal, and the server still enforces. - A lookup's selected chip no longer offers its remove ✕ when the field is disabled. ⚠️ This is how BOTH refusals reach the widget — a field the object declares `readonly` is folded into `disabled` by the form's section builder, and a field the permission set refuses is marked disabled by the pass above — so a reporter could previously clear a master-detail parent the server would then refuse to unset. The trigger and the browse button were already disabled; the chip's ✕ was the one control the gate had missed. The chips themselves stay: the value is readable, only the affordance goes. diff --git a/.changeset/10156-edit-form-writes-only-changed-fields.md b/.changeset/10156-edit-form-writes-only-changed-fields.md new file mode 100644 index 0000000000..2ce615e0a7 --- /dev/null +++ b/.changeset/10156-edit-form-writes-only-changed-fields.md @@ -0,0 +1,23 @@ +--- +'@object-ui/plugin-form': patch +'@object-ui/types': patch +--- + +An edit form now writes only the fields that changed (objectui#10156). + +**Clause-②: no.** No exported symbol, type or prop changes. `@object-ui/plugin-form` publishes `.` only, from `index.tsx`, and `index.tsx` re-exports neither `sanitize` nor `masterDetailTx`. After a build, `dist/index.d.ts` names none of the new helpers. The change is in what the client sends, and ⚠️ in what a host `submitHandler` receives in edit mode (see below). + +**Before.** A master-detail child row already sent only the cells that differed from its loaded snapshot (objectui#10108). The other two edit payloads did not. The plain record edit `PATCH` and the parent operation of a master-detail batch sent every sanitized field on every save, including fields the user never touched. With the concurrency guard, a `409` followed by **Overwrite** therefore rewrote every field, not only the ones this user changed. + +**What changed, in observable terms.** + +- In edit mode, `ObjectForm`, `ModalForm` and `DrawerForm` compare their save with the record they read through `findOne`, and write only the fields that differ. The parent operation of a master-detail batch follows, because its header is a simple `ObjectForm`. +- There is one comparison, shared with the master-detail child rows. It sends anything it cannot prove unchanged. `null` and `undefined` count as the same value. `null` and `''` are different. So are `5` and `'5'`, a lookup id and its expanded object, a `Date` and a date string, and two objects whose keys come in a different order. A field the form changed by itself after the read, such as a cascade clear, is sent. +- A save with nothing changed still sends the full sanitized payload. It stays a real request, with the same concurrency guard and a real server record for `onSuccess`. +- A form that did not read the record itself still sends every field. That covers a create, a record supplied as `initialData`, and inline `customFields`. +- After a successful save, the form counts the fields it just wrote as saved. A form that stays open compares its next save with the record as it is now. Changing a field back to its first-read value is therefore still sent. +- The concurrency guard is unchanged. The update still carries `ifMatch` = the `updated_at` the form read, and a `409` still offers **Keep editing** or **Overwrite**. **Overwrite** now resends only the changed fields. +- ⚠️ A host `submitHandler` on an edit form receives the payload the form would have written. That is the changed fields, or the full sanitized payload when nothing changed. A host that needs the whole record must read it itself. In this repository, only `MasterDetailForm` passes a `submitHandler` to an edit form. Its header form receives the changed fields. Its row editor has no `recordId`, so it still receives every value. +- The JSDoc of `ObjectFormSchema.submitHandler` in `@object-ui/types`, and its copies on `ModalFormSchema` and `DrawerFormSchema`, now say what an edit-mode handler receives. + +**Not covered.** The `tabbed`, `wizard` and `split` variants have save paths of their own and still send every value they hold. That includes a master-detail header laid out `tabbed`, and a simple form whose mobile `stepper` option shows it one step at a time through the wizard. diff --git a/content/docs/plugins/plugin-form.mdx b/content/docs/plugins/plugin-form.mdx index c6082d1a22..5ca704878c 100644 --- a/content/docs/plugins/plugin-form.mdx +++ b/content/docs/plugins/plugin-form.mdx @@ -413,6 +413,20 @@ The components are on the package's export surface — `ObjectForm`, (`TabbedFormSchema`, `WizardFormSchema`, `ModalFormSchema`, …). There is no aggregate map among them. +## What an edit save writes + +An `object-form` in `mode: 'edit'` reads its record with `findOne`, and its save +writes only the fields that differ from that read. That covers the simple form, +the `modal` and `drawer` variants, and the parent operation of a master-detail +form. The `tabbed`, `wizard` and `split` variants still send every value the form +holds, and so does a simple form whose mobile `stepper` option routes it through +the wizard. A field whose sameness cannot be proven is sent: `5` and `'5'`, `null` and +`''`, and a lookup id and its expanded object all count as different. A save with +nothing changed still sends the full payload, as it always has. The `ifMatch` +concurrency guard is unchanged, and its **Overwrite** choice now resends only the +changed fields. A host `submitHandler` receives the same payload in edit mode. +The package README has the full rule, under "What an edit save writes". + ## Examples ### Form with Validation diff --git a/packages/plugin-form/README.md b/packages/plugin-form/README.md index dcb4565aac..863341b198 100644 --- a/packages/plugin-form/README.md +++ b/packages/plugin-form/README.md @@ -869,6 +869,57 @@ returning, which reported a save that never happened and, through (objectui#6300). A declared `submitHandler` is consulted first, so a host that said it owns the write is never bypassed for want of an adapter it never needed. +## What an edit save writes + +An `object-form` in `mode: 'edit'` with a `recordId` reads the record with +`dataSource.findOne`, and its save writes **only the fields that differ from +that read** (objectui#10156). The simple form, the `modal` and the `drawer` +variants do this, and so does the parent operation of a master-detail form, +whose header is a simple form. A master-detail child row already worked this +way (objectui#10108), and all of them use the same comparison. The `tabbed`, +`wizard` and `split` variants are not covered. That includes a master-detail +header laid out `tabbed`, and a simple form whose mobile `stepper` option shows it +one step at a time through the wizard. They still send every value the form +holds. + +The comparison sends every field it cannot prove unchanged, because a field +wrongly judged unchanged would lose the user's edit while the server still +answers 200: + +- `null` and `undefined` count as the same value. `''` is not a blank here, so + `null` and `''` are different. +- A number and a numeric string are different (`5` and `'5'`). +- A lookup id and the expanded lookup object are different. +- A `Date` and a date string are different. So are two date strings written in + different formats. +- Objects and arrays are equal only when they serialize identically. + Reordering their keys or elements makes them different. + +A field the form changed by itself after the read is a change, so it is sent. +That covers a cascade clear or a value cleared when its field was hidden. + +Some saves still send the full payload: + +- **A save with nothing changed.** It sends every field, as it always has. + That keeps it a real request, with the same concurrency guard and a real + server record for `onSuccess`. +- **A form with no record read of its own.** This includes a create, a record + given as `initialData` or through inline `customFields`, and a save made + while a new record is still loading. Each of these sends every field. + +After a successful save, the form treats the fields it just wrote as saved. A +form that stays open therefore compares its next save with the record as it +stands now, not as it was first read. + +The concurrency guard is unchanged. The update still carries +`ifMatch` = the `updated_at` the form read, and a `409` still offers +**Keep editing** or **Overwrite**. **Overwrite** now resends only the changed +fields, so it no longer rewrites fields this user never touched. + +⚠️ A host `submitHandler` gets the same payload in edit mode. Normally that is +the changed fields; after a save with nothing changed, it is the full payload. +A host that needs the whole record must read it itself. + ## Integration with Data Sources **The adapter is not a schema key.** A schema is a serialisable document; a live diff --git a/packages/plugin-form/src/DrawerForm.tsx b/packages/plugin-form/src/DrawerForm.tsx index a50c51fecc..b00d12e62b 100644 --- a/packages/plugin-form/src/DrawerForm.tsx +++ b/packages/plugin-form/src/DrawerForm.tsx @@ -50,7 +50,13 @@ import { CONTAINER_GRID_COLS, } from './autoLayout'; import { deriveFieldGroupSections, projectSectionDivider, resolveSectionCollapse } from './fieldGroups'; -import { sanitizeFormData } from './sanitize'; +import { + sanitizeFormData, + dirtyEditPayload, + snapshotLoadedRecord, + advanceLoadedRecord, + type LoadedRecordSnapshot, +} from './sanitize'; import { applyFieldPermissions, fieldWriteGate } from './fieldWriteGate'; import { seedCreateValues, omitServerResolvedDefaults } from './schemaDefaults'; import { resolveInitialRecord } from './initialRecord'; @@ -158,6 +164,10 @@ export interface DrawerFormSchema { * When supplied, the form validates and hands the collected values * to this handler INSTEAD of calling `dataSource.create` / * `dataSource.update`; the returned record is passed on to `onSuccess`. + * In `edit` mode, for a record this form read itself, it hands over what it + * would have written: the fields that differ from that read, or the full + * sanitized payload when nothing changed (objectui#10156; the whole rule is + * on `ObjectFormSchema['submitHandler']`). * * `MasterDetailForm` supplies it to route the parent AND its child * collections through one atomic `batchTransaction` (#2679 / ADR-0034 @@ -273,6 +283,12 @@ export const DrawerForm: React.FC = ({ // `initialData`/`initialValues` are objects callers commonly rebuild every // render, and flashing the loading state for those would thrash. const loadedRecordIdRef = useRef(undefined); + // The record itself as read — the baseline an edit save diffs against, so + // only the fields that changed are written (objectui#10156). Kept apart from + // `formData`, which seeds the form and supplies the OCC token: advancing it + // after a save would reseed the one and move the other. Set by the `findOne` + // below and nowhere else, so a caller-supplied record is never a baseline. + const loadedRecordRef = useRef(null); // Fetch initial data useEffect(() => { @@ -288,6 +304,8 @@ export const DrawerForm: React.FC = ({ let cancelled = false; const fetchData = async () => { if (schema.mode === 'create' || !schema.recordId) { + // Seeded from something other than a read: no baseline to diff against. + loadedRecordRef.current = null; // Declared static defaults are this form's opening values (#4047) — // see `schemaDefaults` for the create-only boundary and for why // runtime defaults are left to the server. @@ -297,6 +315,7 @@ export const DrawerForm: React.FC = ({ } if (!dataSource) { + loadedRecordRef.current = null; setFormData(resolveInitialRecord(schema)); setLoading(false); return; @@ -316,6 +335,7 @@ export const DrawerForm: React.FC = ({ const data = await dataSource.findOne(schema.objectName, schema.recordId); if (cancelled) return; loadedRecordIdRef.current = schema.recordId; + loadedRecordRef.current = snapshotLoadedRecord(schema, data); setFormData(data || {}); } catch (err) { if (cancelled) return; @@ -471,11 +491,14 @@ export const DrawerForm: React.FC = ({ // Omit the fields the producer owns (#4069) — see // `omitServerResolvedDefaults` for why an empty key is not the same as // no key at insert time. Create only: on an edit form a cleared column is - // a real removal. Computed ONCE so every persistence route below — the - // host-owned seam included — writes the identical payload. + // a real removal. An EDIT writes only the fields that differ from the + // record this form read (objectui#10156; `dirtyEditPayload` holds the + // rule, and sends whatever it cannot settle). Computed ONCE so every + // persistence route below — the host-owned seam included — writes the + // identical payload. const writePayload = schema.mode === 'create' ? omitServerResolvedDefaults(payload, objectSchema) - : payload; + : dirtyEditPayload(payload, loadedRecordRef.current, schema); if (schema.submitHandler) { // The host owns persistence (e.g. MasterDetailForm batching the parent @@ -498,12 +521,15 @@ export const DrawerForm: React.FC = ({ dataSource, objectName: schema.objectName, recordId: schema.recordId, - payload, + payload: writePayload, baseRecord: formData, }); if (outcome.status === 'cancelled') return; result = outcome.result; } + // The write landed: a save from this still-open drawer diffs against the + // record as it now stands, not as first read. + loadedRecordRef.current = advanceLoadedRecord(loadedRecordRef.current, schema, writePayload); if (schema.onSuccess) { await schema.onSuccess(result); } diff --git a/packages/plugin-form/src/ModalForm.tsx b/packages/plugin-form/src/ModalForm.tsx index d264f61a8e..d892881742 100644 --- a/packages/plugin-form/src/ModalForm.tsx +++ b/packages/plugin-form/src/ModalForm.tsx @@ -50,7 +50,13 @@ import { CONTAINER_GRID_COLS, } from './autoLayout'; import { deriveFieldGroupSections, projectSectionDivider, resolveSectionCollapse } from './fieldGroups'; -import { sanitizeFormData } from './sanitize'; +import { + sanitizeFormData, + dirtyEditPayload, + snapshotLoadedRecord, + advanceLoadedRecord, + type LoadedRecordSnapshot, +} from './sanitize'; import { applyFieldPermissions, fieldWriteGate } from './fieldWriteGate'; import { seedCreateValues, omitServerResolvedDefaults } from './schemaDefaults'; import { resolveInitialRecord } from './initialRecord'; @@ -176,6 +182,10 @@ export interface ModalFormSchema { * When supplied, the form validates and hands the collected values * to this handler INSTEAD of calling `dataSource.create` / * `dataSource.update`; the returned record is passed on to `onSuccess`. + * In `edit` mode, for a record this form read itself, it hands over what it + * would have written: the fields that differ from that read, or the full + * sanitized payload when nothing changed (objectui#10156; the whole rule is + * on `ObjectFormSchema['submitHandler']`). * * `MasterDetailForm` supplies it to route the parent AND its child * collections through one atomic `batchTransaction` (#2679 / ADR-0034 @@ -349,6 +359,12 @@ export const ModalForm: React.FC = ({ // `initialData`/`initialValues` are objects callers commonly rebuild every // render, and flashing the loading state for those would thrash. const loadedRecordIdRef = useRef(undefined); + // The record itself as read — the baseline an edit save diffs against, so + // only the fields that changed are written (objectui#10156). Kept apart from + // `formData`, which seeds the form and supplies the OCC token: advancing it + // after a save would reseed the one and move the other. Set by the `findOne` + // below and nowhere else, so a caller-supplied record is never a baseline. + const loadedRecordRef = useRef(null); // Fetch initial data useEffect(() => { @@ -364,6 +380,8 @@ export const ModalForm: React.FC = ({ let cancelled = false; const fetchData = async () => { if (schema.mode === 'create' || !schema.recordId) { + // Seeded from something other than a read: no baseline to diff against. + loadedRecordRef.current = null; // No persisted record to show, so the object's declared static // `defaultValue`s are the form's opening values (#4047) — caller- // supplied initial values still win. See `schemaDefaults` for why @@ -375,6 +393,7 @@ export const ModalForm: React.FC = ({ } if (!dataSource) { + loadedRecordRef.current = null; setFormData(resolveInitialRecord(schema)); setLoading(false); return; @@ -394,6 +413,7 @@ export const ModalForm: React.FC = ({ const data = await dataSource.findOne(schema.objectName, schema.recordId); if (cancelled) return; loadedRecordIdRef.current = schema.recordId; + loadedRecordRef.current = snapshotLoadedRecord(schema, data); setFormData(data || {}); } catch (err) { if (cancelled) return; @@ -501,12 +521,14 @@ export const ModalForm: React.FC = ({ // Omit the fields the producer owns (#4069) — see // `omitServerResolvedDefaults` for why an empty key is not the same as // no key at insert time. Create only: on an edit form a cleared column is - // a real removal. Computed ONCE (after the FLS strip above) so every - // persistence route below — the host-owned seam included — writes the - // identical payload. + // a real removal. An EDIT writes only the fields that differ from the + // record this form read (objectui#10156; `dirtyEditPayload` holds the + // rule, and sends whatever it cannot settle). Computed ONCE (after the + // FLS strip above) so every persistence route below — the host-owned + // seam included — writes the identical payload. const writePayload = schema.mode === 'create' ? omitServerResolvedDefaults(payload, objectSchema) - : payload; + : dirtyEditPayload(payload, loadedRecordRef.current, schema); if (schema.submitHandler) { // The host owns persistence (e.g. MasterDetailForm batching the parent @@ -529,12 +551,15 @@ export const ModalForm: React.FC = ({ dataSource, objectName: schema.objectName, recordId: schema.recordId, - payload, + payload: writePayload, baseRecord: formData, }); if (outcome.status === 'cancelled') return; result = outcome.result; } + // The write landed: a save from this still-open modal diffs against the + // record as it now stands, not as first read. + loadedRecordRef.current = advanceLoadedRecord(loadedRecordRef.current, schema, writePayload); if (schema.onSuccess) { await schema.onSuccess(result); } diff --git a/packages/plugin-form/src/ObjectForm.test.tsx b/packages/plugin-form/src/ObjectForm.test.tsx index 3a3319a43c..8ec5b623bb 100644 --- a/packages/plugin-form/src/ObjectForm.test.tsx +++ b/packages/plugin-form/src/ObjectForm.test.tsx @@ -274,13 +274,19 @@ describe('ObjectForm Integration', () => { ); // Wait for the record read to seed the form. - const nameInput = await waitFor(() => { + await waitFor(() => { const el = container.querySelector('input[name="name"]') as HTMLInputElement | null; if (!el || el.value !== 'Website') throw new Error('form not seeded yet'); return el; }); - fireEvent.change(nameInput, { target: { value: 'Website v2' } }); + // Submitted with NOTHING changed, on purpose. An edit writes only the + // fields that differ from the record it read (objectui#10156), so after + // a real edit an unchanged computed column is kept off the wire by the + // dirty diff alone and this row could no longer fail for the sanitizer. + // A save with nothing changed sends the full sanitized payload — the + // one edit route where the sanitizer is the only filter between these + // round-tripped columns and the server. const form = container.querySelector('form') as HTMLFormElement; fireEvent.submit(form); @@ -290,8 +296,8 @@ describe('ObjectForm Integration', () => { const [obj, id, payload] = ds.update.mock.calls[0]; expect(obj).toBe('test_project'); expect(id).toBe('p1'); - // Edited writable field is sent... - expect(payload).toMatchObject({ name: 'Website v2', budget: 150000 }); + // The writable fields are sent... + expect(payload).toMatchObject({ name: 'Website', budget: 150000 }); // ...but the computed and server-managed keys are stripped. expect(payload).not.toHaveProperty('budget_remaining'); expect(payload).not.toHaveProperty('task_count'); diff --git a/packages/plugin-form/src/ObjectForm.tsx b/packages/plugin-form/src/ObjectForm.tsx index 703e053271..46c9dde229 100644 --- a/packages/plugin-form/src/ObjectForm.tsx +++ b/packages/plugin-form/src/ObjectForm.tsx @@ -45,7 +45,13 @@ import { import { deriveFieldGroupSections, projectSectionDivider, resolveSectionCollapse } from './fieldGroups'; import { mergeCustomFields } from './customFieldsMerge'; import { hasSectionGroupReference, resolveSectionGroupReferences } from './sectionGroups'; -import { sanitizeFormData } from './sanitize'; +import { + sanitizeFormData, + dirtyEditPayload, + snapshotLoadedRecord, + advanceLoadedRecord, + type LoadedRecordSnapshot, +} from './sanitize'; import { applyFieldPermissions, fieldWriteGate } from './fieldWriteGate'; import { resolveInitialRecord } from './initialRecord'; import { noSubmitTargetError } from './submitTarget'; @@ -623,6 +629,16 @@ const SimpleObjectForm: React.FC = ({ // OCC-guarded edit save + its conflict dialog (see occSave.tsx). const { saveWithOcc, conflictDialog } = useOccSave(); + // The record this form READ for the record it edits — the baseline an edit + // save diffs against, so only the fields that changed are written + // (objectui#10156). A ref, not state: only the save path reads it, and + // nothing renders from it. Set by the `findOne` below and nowhere else, so a + // caller-supplied record (inline fields, `initialData`) never becomes a + // baseline and its save keeps sending every field. `initialData` itself is + // left alone: it seeds the form and supplies the OCC token, and advancing it + // after a save would reseed the one and move the other. + const loadedRecordRef = React.useRef(null); + // Check if using inline fields (fields defined as objects, not just names) const hasInlineFields = schema.customFields && schema.customFields.length > 0; @@ -698,6 +714,8 @@ const SimpleObjectForm: React.FC = ({ useEffect(() => { const fetchInitialData = async () => { if (!schema.recordId || schema.mode === 'create') { + // Seeded from something other than a read: no baseline to diff against. + loadedRecordRef.current = null; setInitialData(resolveInitialRecord(schema)); setLoading(false); return; @@ -717,6 +735,9 @@ const SimpleObjectForm: React.FC = ({ setLoading(true); try { const data = await dataSource.findOne(schema.objectName, schema.recordId); + // Tagged with the object and record it was read for, so a save that + // runs against a different one finds no baseline and sends everything. + loadedRecordRef.current = snapshotLoadedRecord(schema, data); setInitialData(data); } catch (err) { console.error('Failed to fetch record:', err); @@ -1103,6 +1124,15 @@ const SimpleObjectForm: React.FC = ({ if (isCreateFormMode(schema)) { payload = omitServerResolvedDefaults(payload, hasInlineFields ? null : objectSchema); } + // An EDIT writes only the fields that differ from the record this form + // read (objectui#10156) — on BOTH write routes below: the host-owned seam, + // which is how a master-detail form's parent operation is built, and the + // plain OCC-guarded update. Anything that cannot be settled is sent; see + // `dirtyEditPayload` for the whole rule, including why an empty diff sends + // the full payload. Every other mode gets `payload` back unchanged. The + // full `payload` stays the submit-redirect scope below: it is the record as + // the form now holds it, whether or not a field was written. + const writePayload = dirtyEditPayload(payload, loadedRecordRef.current, schema); try { let result; @@ -1111,7 +1141,7 @@ const SimpleObjectForm: React.FC = ({ // The host owns persistence (e.g. MasterDetailForm batching the parent // + children into one atomic transaction). The form just validates and // hands over the values; it does NOT create/update itself. - result = await schema.submitHandler(payload); + result = await schema.submitHandler(writePayload); } else if (!dataSource) { // No route left: no host seam and no adapter. Refuse instead of // reporting success — the `catch` below hands this to `schema.onError` @@ -1128,7 +1158,7 @@ const SimpleObjectForm: React.FC = ({ dataSource, objectName: schema.objectName, recordId: schema.recordId, - payload, + payload: writePayload, baseRecord: initialData, }); if (outcome.status === 'cancelled') return; @@ -1136,6 +1166,9 @@ const SimpleObjectForm: React.FC = ({ } else { throw new Error('Invalid form mode or missing record ID'); } + // The write landed: the next save from this still-mounted form diffs + // against the record as it now stands, not as first read. + loadedRecordRef.current = advanceLoadedRecord(loadedRecordRef.current, schema, writePayload); // Call success callback if provided, else give default feedback. Skip the // default when a `submitHandler` owns persistence (e.g. MasterDetailForm diff --git a/packages/plugin-form/src/__tests__/drawerFirstLoadWindow-10190.test.tsx b/packages/plugin-form/src/__tests__/drawerFirstLoadWindow-10190.test.tsx index 390975e879..b9550c1d7e 100644 --- a/packages/plugin-form/src/__tests__/drawerFirstLoadWindow-10190.test.tsx +++ b/packages/plugin-form/src/__tests__/drawerFirstLoadWindow-10190.test.tsx @@ -134,7 +134,11 @@ describe.each(SHAPES)('DrawerForm first load (%s) — objectui#10190', (_label, fireEvent.submit(form); await waitFor(() => expect(update).toHaveBeenCalled()); - expect(update.mock.calls[0][2]).toMatchObject({ title: 'typed', note: 'kept' }); + // An edit writes only the fields that differ from the record it read + // (objectui#10156). So `note` is absent precisely because the form still + // holds the 'kept' the record landed with: an emptied `note` would differ + // from the read and be written. + expect(update.mock.calls[0][2]).toEqual({ title: 'typed' }); }); }); diff --git a/packages/plugin-form/src/cascadePruneWire-10291.test.tsx b/packages/plugin-form/src/cascadePruneWire-10291.test.tsx index 3a3ec7f300..f928a0f45d 100644 --- a/packages/plugin-form/src/cascadePruneWire-10291.test.tsx +++ b/packages/plugin-form/src/cascadePruneWire-10291.test.tsx @@ -159,9 +159,10 @@ describe('objectui#10291 — form: a cascade-cleared scalar reaches the wire as // (c) positive control: the multi-value prune still writes a real array. expect(body).toHaveProperty('tags', []); // (d) negative control: a select no parent governs is not nulled. An edit - // form sends its whole value set today (objectui#10156 tracks "only - // dirty"), so the untouched field rides along with its stored value. - expect(body).toHaveProperty('tier_any', 'gold'); + // form writes only the fields that differ from the record it read + // (objectui#10156), so the untouched select is not on the wire at all — + // while a wrongful null WOULD be, because it differs from the stored 'gold'. + expect(body).not.toHaveProperty('tier_any'); }); it('EDIT, no widget mounted: a dependent select `visibleWhen` keeps hidden is cleared by the FORM HOST alone — and that clear reaches the wire as null too', async () => { @@ -202,7 +203,8 @@ describe('objectui#10291 — form: a cascade-cleared scalar reaches the wire as const body = JSON.parse(bodies[0].body); expect(body).toHaveProperty('region', 'apac'); expect(body).toHaveProperty('tier', null); - expect(body).toHaveProperty('tier_any', 'gold'); + // Untouched, so not written (objectui#10156); a wrongful null would be. + expect(body).not.toHaveProperty('tier_any'); }); it('CREATE: the pruned scalar is null on the POST body, and an untouched empty select is absent — never a spurious null', async () => { diff --git a/packages/plugin-form/src/editDirtyPayload-10156.test.tsx b/packages/plugin-form/src/editDirtyPayload-10156.test.tsx new file mode 100644 index 0000000000..de80d6c918 --- /dev/null +++ b/packages/plugin-form/src/editDirtyPayload-10156.test.tsx @@ -0,0 +1,443 @@ +/** + * 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. + */ + +/** + * An edit save writes only the fields that differ from the record the form + * read (objectui#10156). + * + * ## The risk these rows are shaped around + * + * This is the most-used write path in the product, and the defect a dirty diff + * can introduce is silent: a field wrongly judged CLEAN is not sent, the server + * answers 200, and the user's edit is gone. So the rows come in two kinds, and + * they fail for opposite reasons: + * + * - "only that field" rows assert the EXACT payload. They go red if the diff + * stops filtering. + * - "is sent" rows (type drift, a field the form moved itself) assert only + * that a field IS on the payload. They go red if the diff drops something it + * could not prove unchanged, and they stay green when the diff sends too much. + * + * Every component row drives a real renderer with real widgets, so the drift + * is the drift a user produces: retyping `5` into a number input whose stored + * value arrived as the string `'5'`, or typing into a textarea whose stored + * value is `null` and clearing it again. + */ +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 { ObjectForm } from './ObjectForm'; +import { MasterDetailForm } from './MasterDetailForm'; +import { + dirtyEditPayload, + isSameStoredValue, + snapshotLoadedRecord, + advanceLoadedRecord, +} from './sanitize'; + +registerAllFields(); + +const VERSION = '2026-09-25 00:00:00.000'; +const NEWER_VERSION = '2026-09-25 00:05:00.000'; + +const DEAL_SCHEMA = { + name: 'deal', + fields: { + name: { type: 'text', label: 'Name' }, + qty: { type: 'number', label: 'Qty' }, + remark: { type: 'textarea', label: 'Remark' }, + stage: { type: 'text', label: 'Stage' }, + amount: { type: 'currency', label: 'Amount' }, + owner_id: { type: 'lookup', label: 'Owner', system: true }, + updated_at: { type: 'datetime', label: 'Updated', system: true }, + }, +}; + +/** + * The record as the server returns it. `qty` arrives as a numeric STRING, the + * way a decimal column often does, and `remark` as `null`. + */ +const STORED = { + id: 'd1', + name: 'Mine', + qty: '5', + remark: null, + stage: 'open', + amount: 10.5, + owner_id: 'u9', + updated_at: VERSION, +}; + +/** What `sanitizeFormData` keeps of {@link STORED}: every business column. */ +const STORED_BUSINESS = { name: 'Mine', qty: '5', remark: null, stage: 'open', amount: 10.5 }; + +const conflictError = () => + Object.assign(new Error('Record was modified by another user'), { + code: 'CONCURRENT_UPDATE', + httpStatus: 409, + currentVersion: NEWER_VERSION, + }); + +function makeDS(update?: any) { + return { + getObjectSchema: vi.fn().mockResolvedValue(DEAL_SCHEMA), + findOne: vi.fn().mockResolvedValue({ ...STORED }), + find: vi.fn().mockResolvedValue({ data: [] }), + create: vi.fn(async (_o: string, d: any) => ({ id: 'new1', ...d })), + update: + update ?? + vi.fn(async (_o: string, _id: string, d: any, _opts?: { ifMatch?: string }) => ({ + ...STORED, + ...d, + })), + }; +} + +const fieldEl = (root: HTMLElement, tag: 'input' | 'textarea', name: string) => + waitFor(() => { + const el = root.querySelector(`${tag}[name="${name}"]`) as HTMLInputElement | null; + if (!el) throw new Error(`${name} not ready`); + return el; + }); + +const change = async (el: HTMLElement, value: string) => { + await act(async () => { + fireEvent.change(el, { target: { value } }); + }); +}; + +const submit = async (root: HTMLElement) => { + await act(async () => { + fireEvent.submit(root.querySelector('form') as HTMLFormElement); + }); +}; + +const editSchema = (extra: Record = {}) => + ({ type: 'object-form', objectName: 'deal', mode: 'edit', recordId: 'd1', ...extra }) as any; + +describe('the one comparison — every pair the rule states', () => { + const t = Date.UTC(2026, 0, 2); + const rows: Array<{ label: string; a: unknown; b: unknown; same: boolean }> = [ + { label: 'the same string', a: 'x', b: 'x', same: true }, + { label: 'null and undefined (one blank)', a: null, b: undefined, same: true }, + { label: 'null and an empty string', a: null, b: '', same: false }, + { label: 'undefined and an empty string', a: undefined, b: '', same: false }, + { label: 'a number and its numeric string', a: 1, b: '1', same: false }, + { label: 'a lookup id and its expanded object', a: 'a1', b: { id: 'a1', name: 'Acme' }, same: false }, + { label: 'two identical expanded lookups', a: { id: 'a1' }, b: { id: 'a1' }, same: true }, + { label: 'an object with its keys reordered', a: { a: 1, b: 2 }, b: { b: 2, a: 1 }, same: false }, + { label: 'an array reordered', a: ['x', 'y'], b: ['y', 'x'], same: false }, + { label: 'two identical arrays', a: ['x'], b: ['x'], same: true }, + { label: 'two Dates holding the same time', a: new Date(t), b: new Date(t), same: true }, + { label: 'a Date and its ISO string', a: new Date(t), b: new Date(t).toISOString(), same: false }, + { label: 'two date strings in different formats', a: '2026-01-02', b: '2026-01-02T00:00:00Z', same: false }, + { label: 'NaN against itself', a: NaN, b: NaN, same: false }, + ]; + + it.each(rows)('$label → same: $same', ({ a, b, same }) => { + expect(isSameStoredValue(a, b)).toBe(same); + expect(isSameStoredValue(b, a)).toBe(same); + }); +}); + +describe('dirtyEditPayload — what an edit writes', () => { + const target = { mode: 'edit', objectName: 'deal', recordId: 'd1' }; + const snapshot = snapshotLoadedRecord(target, STORED); + + it('writes the changed fields only', () => { + const payload = { ...STORED_BUSINESS, name: 'Mine v2' }; + expect(dirtyEditPayload(payload, snapshot, target)).toEqual({ name: 'Mine v2' }); + }); + + it('sends every field it cannot prove unchanged: 5 over "5", "" over null, an id over an expanded lookup', () => { + const snap = snapshotLoadedRecord(target, { ...STORED, account: { id: 'a1', name: 'Acme' } }); + const payload = { ...STORED_BUSINESS, qty: 5, remark: '', account: 'a1' }; + expect(dirtyEditPayload(payload, snap, target)).toEqual({ qty: 5, remark: '', account: 'a1' }); + }); + + it('an empty diff sends the payload it was given, unchanged', () => { + const payload = { ...STORED_BUSINESS }; + expect(dirtyEditPayload(payload, snapshot, target)).toBe(payload); + }); + + it('no snapshot, or one read for another record or object, sends everything', () => { + const payload = { ...STORED_BUSINESS, name: 'Mine v2' }; + expect(dirtyEditPayload(payload, null, target)).toBe(payload); + expect(dirtyEditPayload(payload, snapshot, { ...target, recordId: 'd2' })).toBe(payload); + expect(dirtyEditPayload(payload, snapshot, { ...target, objectName: 'lead' })).toBe(payload); + }); + + it('a create never diffs, even against a matching snapshot', () => { + const payload = { ...STORED_BUSINESS }; + expect(dirtyEditPayload(payload, snapshot, { ...target, mode: 'create' })).toBe(payload); + }); + + it('after a save the baseline carries what was written, and only for its own record', () => { + const advanced = advanceLoadedRecord(snapshot, target, { name: 'Mine v2' }); + expect(advanced?.record).toMatchObject({ name: 'Mine v2', stage: 'open' }); + expect(advanceLoadedRecord(snapshot, { ...target, recordId: 'd2' }, { name: 'x' })).toBe(snapshot); + }); +}); + +describe('ObjectForm edit — the plain record PATCH', () => { + it('changing one field sends only that field — nothing server-managed — with the OCC token it read', async () => { + const ds = makeDS(); + const { container } = render(); + + await change(await fieldEl(container, 'input', 'name'), 'Mine v2'); + await submit(container); + + await waitFor(() => expect(ds.update).toHaveBeenCalledTimes(1)); + expect(ds.update).toHaveBeenCalledWith('deal', 'd1', { name: 'Mine v2' }, { ifMatch: VERSION }); + }); + + it('type drift is sent: a retyped 5 over the stored "5", and "" over the stored null', async () => { + const ds = makeDS(); + const { container } = render(); + + await change(await fieldEl(container, 'input', 'name'), 'Mine v2'); + const qty = await fieldEl(container, 'input', 'qty'); + await change(qty, '6'); + await change(qty, '5'); + const remark = await fieldEl(container, 'textarea', 'remark'); + await change(remark, 'x'); + await change(remark, ''); + await submit(container); + + await waitFor(() => expect(ds.update).toHaveBeenCalledTimes(1)); + const payload = ds.update.mock.calls[0][2]; + // Preconditions that make these rows a TYPE drift, not a value change. + expect(STORED.qty).toBe('5'); + expect(STORED.remark).toBeNull(); + expect(payload).toHaveProperty('qty', 5); + expect(payload).toHaveProperty('remark', ''); + expect(payload).toHaveProperty('name', 'Mine v2'); + }); + + it('a save with nothing changed is the request it has always been: the full sanitized payload', async () => { + const onSuccess = vi.fn(); + const ds = makeDS(); + const { container } = render(); + + await fieldEl(container, 'input', 'name'); + await submit(container); + + await waitFor(() => expect(ds.update).toHaveBeenCalledTimes(1)); + expect(ds.update).toHaveBeenCalledWith('deal', 'd1', STORED_BUSINESS, { ifMatch: VERSION }); + // Success is the server's answer to a request that was made. + await waitFor(() => expect(onSuccess).toHaveBeenCalledWith(expect.objectContaining({ id: 'd1' }))); + }); + + it('a second save from the same mounted form diffs against what the first save wrote', async () => { + const ds = makeDS(); + const { container } = render(); + + const name = await fieldEl(container, 'input', 'name'); + await change(name, 'Mine v2'); + await submit(container); + await waitFor(() => expect(ds.update).toHaveBeenCalledTimes(1)); + expect(ds.update.mock.calls[0][2]).toEqual({ name: 'Mine v2' }); + + // Back to the value FIRST read, plus a second change. Against the first + // read, `name` would compare clean and be dropped while the server holds + // 'Mine v2'. + await change(await fieldEl(container, 'input', 'name'), 'Mine'); + await change(await fieldEl(container, 'input', 'stage'), 'won'); + await submit(container); + await waitFor(() => expect(ds.update).toHaveBeenCalledTimes(2)); + expect(ds.update.mock.calls[1][2]).toEqual({ name: 'Mine', stage: 'won' }); + }); + + it('"Overwrite" after a 409 re-sends the changed fields only, re-keyed to the server version', async () => { + const update = vi + .fn() + .mockRejectedValueOnce(conflictError()) + .mockImplementation(async (_o: string, _id: string, d: any) => ({ ...STORED, ...d })); + const ds = makeDS(update); + const { container } = render(); + + await change(await fieldEl(container, 'input', 'name'), 'Mine v2'); + await submit(container); + fireEvent.click(await screen.findByText('Overwrite')); + + await waitFor(() => expect(update).toHaveBeenCalledTimes(2)); + expect(update).toHaveBeenNthCalledWith(1, 'deal', 'd1', { name: 'Mine v2' }, { ifMatch: VERSION }); + expect(update).toHaveBeenNthCalledWith(2, 'deal', 'd1', { name: 'Mine v2' }, { ifMatch: NEWER_VERSION }); + }); + + it('CREATE is unchanged (control): a seeded value the user never touched is still posted', async () => { + const ds = makeDS(); + const { container } = render( + , + ); + + await change(await fieldEl(container, 'input', 'name'), 'New deal'); + await submit(container); + + await waitFor(() => expect(ds.create).toHaveBeenCalledTimes(1)); + expect(ds.update).not.toHaveBeenCalled(); + expect(ds.findOne).not.toHaveBeenCalled(); + expect(ds.create.mock.calls[0][1]).toMatchObject({ name: 'New deal', stage: 'open' }); + }); +}); + +describe.each([ + ['modal', 'ModalForm'], + ['drawer', 'DrawerForm'], +])('ObjectForm formType %s (%s) edit', (formType) => { + it('changing one field sends only that field, with the OCC token it read', async () => { + const ds = makeDS(); + const { baseElement } = render( + , + ); + + await change(await fieldEl(baseElement, 'input', 'name'), 'Mine v2'); + await submit(baseElement); + + await waitFor(() => expect(ds.update).toHaveBeenCalledTimes(1)); + expect(ds.update).toHaveBeenCalledWith('deal', 'd1', { name: 'Mine v2' }, { ifMatch: VERSION }); + }); + + it('type drift is sent: a retyped 5 over the stored "5"', async () => { + const ds = makeDS(); + const { baseElement } = render( + , + ); + + await change(await fieldEl(baseElement, 'input', 'name'), 'Mine v2'); + const qty = await fieldEl(baseElement, 'input', 'qty'); + await change(qty, '6'); + await change(qty, '5'); + await submit(baseElement); + + await waitFor(() => expect(ds.update).toHaveBeenCalledTimes(1)); + expect(ds.update.mock.calls[0][2]).toHaveProperty('qty', 5); + }); +}); + +describe('master-detail — the parent operation', () => { + const PO_SCHEMA = { + name: 'po', + fields: { + ref: { type: 'text', label: 'Ref' }, + status: { type: 'text', label: 'Status' }, + note: { type: 'text', label: 'Note' }, + owner_id: { type: 'lookup', label: 'Owner', system: true }, + updated_at: { type: 'datetime', label: 'Updated', system: true }, + }, + }; + const PO = { id: 'po1', ref: 'PO-1', status: 'draft', note: 'n', owner_id: 'u9', updated_at: VERSION }; + + const renderEdit = () => { + const batchTransaction = vi.fn().mockResolvedValue({ results: [{ id: 'po1' }] }); + const ds: any = { + getObjectSchema: vi.fn().mockResolvedValue(PO_SCHEMA), + findOne: vi.fn().mockResolvedValue({ ...PO }), + find: vi.fn().mockResolvedValue({ data: [] }), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + batchTransaction, + }; + const view = render( + , + ); + return { ...view, ds, batchTransaction }; + }; + + it('changing one parent field sends only that field in the parent operation', async () => { + const { container, batchTransaction } = renderEdit(); + + await change(await fieldEl(container, 'input', 'ref'), 'PO-2'); + fireEvent.click(screen.getByRole('button', { name: /^save$/i })); + + await waitFor(() => expect(batchTransaction).toHaveBeenCalledTimes(1)); + const ops = batchTransaction.mock.calls[0][0]; + expect(ops[0]).toEqual({ object: 'po', action: 'update', id: 'po1', data: { ref: 'PO-2' } }); + }); + + it('a save with nothing changed keeps the parent operation as the full sanitized payload', async () => { + const { container, batchTransaction } = renderEdit(); + + await fieldEl(container, 'input', 'ref'); + fireEvent.click(screen.getByRole('button', { name: /^save$/i })); + + await waitFor(() => expect(batchTransaction).toHaveBeenCalledTimes(1)); + const ops = batchTransaction.mock.calls[0][0]; + expect(ops[0]).toEqual({ + object: 'po', + action: 'update', + id: 'po1', + data: { ref: 'PO-1', status: 'draft', note: 'n' }, + }); + }); +}); + +describe('a field the FORM moved after the load is a change, and is sent', () => { + /** `gold` is offered only under `emea`, `silver` only under `apac`. */ + const REGIONAL_OPTIONS = [ + { label: 'Gold', value: 'gold', visibleWhen: "record.region == 'emea'" }, + { label: 'Silver', value: 'silver', visibleWhen: "record.region == 'apac'" }, + ]; + const TASK_SCHEMA = { + name: 'task', + fields: { + region: { type: 'text', label: 'Region' }, + tier: { type: 'select', label: 'Tier', dependsOn: ['region'], options: REGIONAL_OPTIONS }, + }, + }; + + it('a cascade clear: the user moves `region`, the form empties `tier`, and `tier: null` is written', async () => { + const ds: any = { + getObjectSchema: vi.fn().mockResolvedValue(TASK_SCHEMA), + findOne: vi.fn().mockResolvedValue({ id: 't1', region: 'emea', tier: 'gold', updated_at: VERSION }), + create: vi.fn(), + update: vi.fn(async (_o: string, _id: string, d: any) => ({ id: 't1', ...d })), + }; + const { container } = render( + , + ); + const region = await fieldEl(container, 'input', 'region'); + await waitFor(() => { + expect(container.querySelector('[data-testid="select-trigger-tier"]')).toBeTruthy(); + }); + + await change(region, 'apac'); + await submit(container); + + await waitFor(() => expect(ds.update).toHaveBeenCalledTimes(1)); + const payload = ds.update.mock.calls[0][2]; + // The user touched `region` only; `tier` moved because the form moved it. + expect(payload).toHaveProperty('tier', null); + expect(payload).toHaveProperty('region', 'apac'); + }); +}); diff --git a/packages/plugin-form/src/fieldSecurityPayload.test.tsx b/packages/plugin-form/src/fieldSecurityPayload.test.tsx index a23927bbed..8387faf896 100644 --- a/packages/plugin-form/src/fieldSecurityPayload.test.tsx +++ b/packages/plugin-form/src/fieldSecurityPayload.test.tsx @@ -24,6 +24,17 @@ * that lit control in the SAME payload: the unchanged, un-denied columns are * still on the wire. * + * ## Why the filter rows submit with nothing changed (objectui#10156) + * + * An edit now writes only the fields that differ from the record the form + * read. After a real edit, an unchanged `score` is kept off the wire by that + * dirty diff alone — so a row that edited `actual_value` and then asserted + * `score` absent would stay green with the field-level filter deleted. The + * rows that read the FILTER therefore submit with nothing changed: a save with + * no changes sends the full sanitized payload, the one edit route where the + * filter is the only thing between `score` and the server, and where the lit + * control above is still on the wire to be read. + * * ## Why all three containers, and why they are pinned in one file * * `ObjectForm`, `ModalForm` and `DrawerForm` are one family reached from one @@ -173,6 +184,24 @@ async function editAllowedFieldAndSubmit(root: HTMLElement, update: any) { return update.mock.calls[0][2] as Record; } +/** + * Submit with NOTHING changed, once the record is on screen, and return the + * payload: the full sanitized set, which is what the filter rows read (see the + * header for why an edited submit cannot carry them any more). + */ +async function submitUnchanged(root: HTMLElement, update: any) { + await waitFor(() => { + const el = root.querySelector('input[name="actual_value"]') as HTMLInputElement | null; + if (!el) throw new Error('actual_value not rendered'); + if (el.value !== String(RECORD.actual_value)) throw new Error('record not on screen yet'); + }); + const form = root.querySelector('form'); + if (!form) throw new Error('no form element'); + fireEvent.submit(form); + await waitFor(() => expect(update).toHaveBeenCalled()); + return update.mock.calls[0][2] as Record; +} + /** The three containers, each mounted under the same principal. */ const CONTAINERS: Array<{ name: string; @@ -206,16 +235,21 @@ const CONTAINERS: Array<{ describe('field-level security — the form neither sends nor offers a refused field (objectui#10120)', () => { describe.each(CONTAINERS)('$name', ({ mount }) => { - it('omits the FLS-refused field from the PATCH while the edited field and every un-denied one still go', async () => { + it('omits the FLS-refused field from the PATCH while every un-denied one still goes', async () => { const { ds, update } = makeDataSource(); - const payload = await editAllowedFieldAndSubmit(mount(REPORTER, ds), update); + const payload = await submitUnchanged(mount(REPORTER, ds), update); for (const key of DENIED) expect(payload).not.toHaveProperty(key); - // The edit the user actually made reached the wire … - expect(payload).toMatchObject({ actual_value: 5000 }); - // … and so did the card's lit control: unchanged columns with no deny on - // them are NOT what makes the save fail, so the fix is not "send less". - expect(payload).toMatchObject(UNCHANGED_ALLOWED); + // The card's lit control reached the wire in the same payload: unchanged + // columns with no deny on them are NOT what makes the save fail, so the + // fix is not "send less". + expect(payload).toMatchObject({ actual_value: RECORD.actual_value, ...UNCHANGED_ALLOWED }); + }); + + it('an edit writes the edited field alone — never the FLS-refused one', async () => { + const { ds, update } = makeDataSource(); + const payload = await editAllowedFieldAndSubmit(mount(REPORTER, ds), update); + expect(payload).toEqual({ actual_value: 5000 }); }); it('renders the FLS-refused field non-editable, so the refusal is never invited', async () => { @@ -241,9 +275,10 @@ describe('field-level security — the form neither sends nor offers a refused f return el; }); expect(score.disabled).toBe(false); - const payload = await editAllowedFieldAndSubmit(root, update); + // The same route as the filter row above; only the principal differs. + const payload = await submitUnchanged(root, update); expect(payload).toHaveProperty('score'); - expect(payload).toMatchObject({ actual_value: 5000, ...UNCHANGED_ALLOWED }); + expect(payload).toMatchObject({ actual_value: RECORD.actual_value, ...UNCHANGED_ALLOWED }); // `adjusted_score` and `sheet` stay absent for EVERY caller: they are // `readonly` on the field definition, which this card does not move. expect(payload).not.toHaveProperty('adjusted_score'); diff --git a/packages/plugin-form/src/masterDetailTx.ts b/packages/plugin-form/src/masterDetailTx.ts index faa37e406e..6e692f556a 100644 --- a/packages/plugin-form/src/masterDetailTx.ts +++ b/packages/plugin-form/src/masterDetailTx.ts @@ -16,7 +16,9 @@ */ import type { BatchTransactionOperation } from '@object-ui/types'; -import { sanitizeFormData } from './sanitize'; +// `changedFields` is the ONE dirty-field comparison, shared with the edit +// form's own record diff (objectui#10156) — see `isSameStoredValue` there. +import { sanitizeFormData, changedFields } from './sanitize'; export const idOf = (rec: any): string | undefined => rec == null ? undefined : (rec.id ?? rec._id ?? rec.recordId); @@ -54,58 +56,6 @@ const toWritable = (data: Record, childSchema: ChildSchema): Record const parentWritable = (data: Record): Record => sanitizeFormData(data, null); -/** - * Whether two payload values are the SAME stored value, for the dirty-field - * diff below. - * - * ⚠️ The comparison is deliberately asymmetric in its failure direction. Saying - * "changed" about an equal pair costs one redundant column on the wire; saying - * "unchanged" about a changed pair DISCARDS the user's edit, silently, with a - * 200 back. So every case this cannot settle confidently reads as changed: - * `1000` and `'1000'` are different, two objects are the same only when they - * serialize identically (a reordered key reads as changed, which is the safe - * side), and only the two blanks a form round-trip actually interchanges — - * `null` and `undefined` — are treated as one value. - */ -function isSameStoredValue(a: unknown, b: unknown): boolean { - if (a === b) return true; - if (a == null && b == null) return true; // null/undefined are one blank - if (a == null || b == null) return false; - if (a instanceof Date || b instanceof Date) { - const ta = a instanceof Date ? a.getTime() : NaN; - const tb = b instanceof Date ? b.getTime() : NaN; - return Number.isFinite(ta) && ta === tb; - } - if (typeof a === 'object' && typeof b === 'object') { - try { return JSON.stringify(a) === JSON.stringify(b); } catch { return false; } - } - return false; -} - -/** - * The subset of `next` that differs from the loaded snapshot `prev` — the - * DIRTY fields, and only those. - * - * An update operation that carries an unchanged column is not free: the - * platform refuses a write to a system-managed ownership column unless the - * caller holds the transfer grant, and it cannot tell a round-trip of the value - * it just served from an attempted transfer. A master-detail save commits as - * ONE atomic batch, so one such column on any row refuses every row - * (objectui#10108). `sanitizeFormData` already refuses the columns the server - * owns by name; sending only what the user actually changed is the half that - * does not depend on a roster being complete. - */ -function changedFields( - next: Record, - prev: Record, -): Record { - const out: Record = {}; - for (const [k, v] of Object.entries(next)) { - if (!isSameStoredValue(v, prev[k])) out[k] = v; - } - return out; -} - export interface RowDiff { toCreate: Record[]; toUpdate: Record[]; diff --git a/packages/plugin-form/src/sanitize.ts b/packages/plugin-form/src/sanitize.ts index 68722d00f6..9c115af88c 100644 --- a/packages/plugin-form/src/sanitize.ts +++ b/packages/plugin-form/src/sanitize.ts @@ -160,3 +160,175 @@ export function sanitizeFormData( return out; } + +/** + * Whether two payload values are the SAME stored value — the one comparison + * every dirty-field diff in this package uses: the master-detail child rows in + * `masterDetailTx.ts` (objectui#10108) and the edit form's own record + * (objectui#10156). ⛔ Do not write a second equality rule beside it; two rules + * would drift, and the one that drifted towards "equal" would drop edits. + * + * ⚠️ The comparison is deliberately asymmetric in its failure direction. Saying + * "changed" about an equal pair costs one redundant column on the wire; saying + * "unchanged" about a changed pair DISCARDS the user's edit, silently, with a + * 200 back. So every case this cannot settle confidently reads as changed. + * + * The rule, pair by pair: + * + * - The same value (`===`) is equal. + * - `null` and `undefined` are one blank — the only two blanks a form + * round-trip actually interchanges. `''` is NOT a blank here: `null` and + * `''` read as different, and so do `undefined` and `''`. + * - Numbers never equal strings: `1000` and `'1000'` read as different. + * - Two `Date`s are equal only when both hold the same finite time. A `Date` + * and a date STRING read as different, and so do two date strings in + * different formats (`'2026-01-02'` and `'2026-01-02T00:00:00Z'`). + * - Two objects or arrays are equal only when they serialize identically. A + * reordered key or array element reads as changed, which is the safe side. + * - A lookup's id and its expanded object (`'a1'` and `{ id: 'a1', … }`) read + * as different: one is a string and the other an object. + * - Anything else is different, including `NaN` against itself. + */ +export function isSameStoredValue(a: unknown, b: unknown): boolean { + if (a === b) return true; + if (a == null && b == null) return true; // null/undefined are one blank + if (a == null || b == null) return false; + if (a instanceof Date || b instanceof Date) { + const ta = a instanceof Date ? a.getTime() : NaN; + const tb = b instanceof Date ? b.getTime() : NaN; + return Number.isFinite(ta) && ta === tb; + } + if (typeof a === 'object' && typeof b === 'object') { + try { return JSON.stringify(a) === JSON.stringify(b); } catch { return false; } + } + return false; +} + +/** + * The subset of `next` that differs from the loaded snapshot `prev` — the + * DIRTY fields, and only those, judged by {@link isSameStoredValue}. + * + * An update that carries an unchanged column is not free: the platform refuses + * a write to a system-managed ownership column unless the caller holds the + * transfer grant, and it cannot tell a round-trip of the value it just served + * from an attempted transfer. A master-detail save commits as ONE atomic batch, + * so one such column on any row refuses every row (objectui#10108). + * `sanitizeFormData` already refuses the columns the server owns by name; + * sending only what the user actually changed is the half that does not depend + * on a roster being complete. + */ +export function changedFields( + next: Record, + prev: Record, +): Record { + const out: Record = {}; + for (const [k, v] of Object.entries(next)) { + if (!isSameStoredValue(v, prev[k])) out[k] = v; + } + return out; +} + +/** + * The record an edit form READ from its data source, tagged with the object + * and record it was read for (objectui#10156). + * + * It is the baseline an edit save diffs against, and it is held by the form + * component that performed the read — never passed in by a host. A record a + * caller supplied (`initialData`, inline `customFields`) is a prefill, not the + * row as stored, so no snapshot is taken for it and its save sends every field. + */ +export interface LoadedRecordSnapshot { + objectName: string; + recordId: string; + record: Record; +} + +/** The three facts of a form schema that decide whether a snapshot applies. */ +export interface EditSaveTarget { + mode?: string; + objectName: string; + recordId?: string | number | null; +} + +/** + * Take the snapshot for a record just read with `findOne`, or `null` when the + * read returned nothing usable. + */ +export function snapshotLoadedRecord( + target: EditSaveTarget, + data: unknown, +): LoadedRecordSnapshot | null { + if (target.recordId == null || target.recordId === '') return null; + if (!data || typeof data !== 'object' || Array.isArray(data)) return null; + return { + objectName: target.objectName, + recordId: String(target.recordId), + record: { ...(data as Record) }, + }; +} + +/** The snapshot's record when it was read for THIS edit, else `null`. */ +function loadedRecordFor( + snapshot: LoadedRecordSnapshot | null | undefined, + target: EditSaveTarget, +): Record | null { + if (!snapshot || target.mode !== 'edit') return null; + if (target.recordId == null || target.recordId === '') return null; + if (snapshot.objectName !== target.objectName) return null; + if (snapshot.recordId !== String(target.recordId)) return null; + return snapshot.record; +} + +/** + * The payload an EDIT save writes: only the fields that differ from the record + * the form loaded (objectui#10156). + * + * `payload` is the output of {@link sanitizeFormData}. Every case that cannot + * be settled resolves towards SENDING, because a false "clean" drops the user's + * edit and the server still answers 200: + * + * - Not an edit, or no snapshot was read for this object and record (a create, + * a caller-supplied record, a record swap still in flight) → `payload` + * unchanged, every field sent. + * - A snapshot applies → the fields {@link changedFields} reports. Fields the + * form itself moved after the load — a cascade clear, a clear-on-hide, a + * value the form computed — differ from the loaded record, so they are sent. + * - The diff is EMPTY → `payload` unchanged. A save with nothing changed stays + * the request it has always been: the same write, the same OCC guard and a + * real server record for `onSuccess`. Emitting no request instead would + * report success for a save no server saw, and that is the one outcome a + * wrong baseline must never be able to produce. + */ +export function dirtyEditPayload( + payload: Record, + snapshot: LoadedRecordSnapshot | null | undefined, + target: EditSaveTarget, +): Record { + if (!payload || typeof payload !== 'object') return payload; + const loaded = loadedRecordFor(snapshot, target); + if (!loaded) return payload; + const changed = changedFields(payload, loaded); + return Object.keys(changed).length > 0 ? changed : payload; +} + +/** + * The snapshot after an edit save SUCCEEDED: the fields just written, laid over + * the record as read. + * + * A form that stays mounted after a save must not diff its next save against + * the row as FIRST read. Changing a field and then changing it back to the + * value first read would compare equal to that stale baseline and be dropped, + * while the server still holds the first save's value. Advancing the baseline + * by what the server accepted closes that. A snapshot read for another record + * is returned untouched. + */ +export function advanceLoadedRecord( + snapshot: LoadedRecordSnapshot | null | undefined, + target: EditSaveTarget, + written: Record, +): LoadedRecordSnapshot | null { + const loaded = loadedRecordFor(snapshot, target); + if (!snapshot || !loaded) return snapshot ?? null; + if (!written || typeof written !== 'object') return snapshot; + return { ...snapshot, record: { ...loaded, ...written } }; +} diff --git a/packages/plugin-form/src/systemManagedPayload.test.tsx b/packages/plugin-form/src/systemManagedPayload.test.tsx index 9a2b14920a..998a1e65bf 100644 --- a/packages/plugin-form/src/systemManagedPayload.test.tsx +++ b/packages/plugin-form/src/systemManagedPayload.test.tsx @@ -224,19 +224,23 @@ describe('form write payloads — server-owned columns (objectui#10108)', () => />, ); - const input = await waitFor(() => { + await waitFor(() => { const el = container.querySelector('input[name="actual_value"]') as HTMLInputElement | null; - if (!el) throw new Error('actual_value not ready'); - return el; + if (!el || el.value !== '92') throw new Error('record not on screen yet'); }); - fireEvent.change(input, { target: { value: '95' } }); + // Submitted with NOTHING changed, on purpose. An edit writes only the + // fields that differ from the record it read (objectui#10156), so after a + // real edit an unchanged `owner_id` is kept off the wire by the dirty diff + // alone, and this row would stay green with the roster emptied. A save + // with nothing changed sends the full sanitized payload — the one edit + // route where the roster and the `system` flag are the only filter. fireEvent.submit(container.querySelector('form')!); await waitFor(() => expect(update).toHaveBeenCalled()); const payload = update.mock.calls[0][2]; for (const key of REFUSED) expect(payload).not.toHaveProperty(key); - // Control: the edit the user actually made is still on the wire. - expect(payload).toMatchObject({ actual_value: 95 }); + // Control: the business columns the record carries are still on the wire. + expect(payload).toMatchObject({ plan_indicator: 'IND1', actual_value: 92, remark: null }); }); }); }); diff --git a/packages/types/src/objectql.ts b/packages/types/src/objectql.ts index 1db02f5e6d..0247f7db4f 100644 --- a/packages/types/src/objectql.ts +++ b/packages/types/src/objectql.ts @@ -1633,6 +1633,16 @@ export interface ObjectFormSchema extends BaseSchema { * dataSource.update — the host owns the write (e.g. MasterDetailForm batching * the parent + child line items into one atomic server transaction). The * returned record is passed on to `onSuccess`. + * + * In `edit` mode, for a record the form read itself, the simple, `modal` and + * `drawer` layouts hand over what the form would have written + * (objectui#10156): the fields that differ from the record it read, or the + * full sanitized payload when nothing changed — every value except the ones + * the form never writes (server-owned, computed, read-only, refused by + * field-level security, or unknown to the object). A field whose sameness + * cannot be settled counts as changed, so it is handed over. The `tabbed`, + * `wizard` and `split` layouts, and a simple form that the mobile `stepper` + * option routes through the wizard, still hand over every collected value. */ submitHandler?: (values: Record) => any | Promise;