diff --git a/.changeset/10404-undo-absent-field-not-null.md b/.changeset/10404-undo-absent-field-not-null.md new file mode 100644 index 0000000000..a671ee66d9 --- /dev/null +++ b/.changeset/10404-undo-absent-field-not-null.md @@ -0,0 +1,35 @@ +--- +'@object-ui/core': minor +'@object-ui/app-shell': minor +--- + +fix(core): Undo of an `undoable` update no longer writes `null` over a field the row did not carry + +An `undoable` update records the prior value of every field it writes, read off +the row it ran on, so the success toast can offer Undo. On a list row projected +by `$select` (`ListView`, `ObjectGrid`, `RelatedList`), a written field that no +column shows was absent from the row. It was recorded as `null`, and Undo then +wrote `null` over the value it existed to restore: close a task from a grid that +shows only its name, press Undo, and its status became empty instead of `open`. + +Two changes: + +- The list harvest (`listViewPredicates`) now also names the fields an + `undoable` action writes: its `patch` keys, the key each param's value is + collected under (`name`, else `field`), and its `bodyExtra` keys. So a + projected row carries them, through the same gates as every harvested name: + a bare identifier, a field the object declares (or a platform column), and + field-level security once the permission answer has loaded. `recordId` is + not harvested; both writers strip it as the record's address. +- The capture never records an absent field as `null`, in `ActionRunner`'s + `operation: 'update'` path and in the console `api` handler's data-source + branch alike. When the row does not carry every written field (for example a + field the principal may write but not read), the action still runs, but it + offers no Undo: the success toast has no Undo button, nothing is pushed onto + the undo stack, and a console warning names the missing fields. A `null` the + row does carry is a real empty value and is still restored as `null`. + +Behaviour to know about: on a backend that leaves null-valued fields out of a +full record, an `undoable` update of such a field now offers no Undo instead of +one that restores it to empty. Absent and empty cannot be told apart there, and +no Undo is the side that cannot overwrite stored data. diff --git a/packages/app-shell/src/hooks/__tests__/useConsoleActionRuntime.undoAbsentField-10404.test.tsx b/packages/app-shell/src/hooks/__tests__/useConsoleActionRuntime.undoAbsentField-10404.test.tsx new file mode 100644 index 0000000000..cac89b85d8 --- /dev/null +++ b/packages/app-shell/src/hooks/__tests__/useConsoleActionRuntime.undoAbsentField-10404.test.tsx @@ -0,0 +1,128 @@ +/** + * 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. + */ + +/** + * objectui#10404 — the console `api` handler's generic (data-source) branch + * never records a field the row does not carry as `null` in its Undo snapshot. + * + * This branch writes `params` plus `bodyExtra` through `dataSource.update` and, + * for an `undoable` action, snapshots each written field's prior value off the + * row stashed under `params._rowRecord`. The snapshot used to be + * `rowRecord[k] ?? null`, so on a projected list row a written field no column + * shows was recorded as `null` and Undo overwrote the stored value with it — + * the same defect as `ActionRunner`'s `captureUpdateUndoData`, in the handler's + * own copy. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { renderHook, act } from '@testing-library/react'; + +vi.mock('react-router-dom', () => ({ useNavigate: () => vi.fn() })); + +vi.mock('@object-ui/auth', async (importOriginal) => ({ + ...(await importOriginal>()), + useAuth: () => ({ user: { id: 'u1', name: 'User', image: null }, activeOrganization: null }), + createAuthenticatedFetch: () => vi.fn(), +})); + +vi.mock('@object-ui/i18n', async (importOriginal) => ({ + ...(await importOriginal>()), + useObjectLabel: () => ({ + fieldLabel: (_o: unknown, _n: unknown, l: unknown) => l, + fieldOptionLabel: (_o: unknown, _f: unknown, _v: unknown, l: unknown) => l, + actionParamText: (_o: unknown, _a: unknown, _p: unknown, _attr: unknown, fallback: unknown) => fallback, + actionParamOptionLabel: (_o: unknown, _a: unknown, _p: unknown, _v: unknown, fallback: unknown) => fallback, + actionDescription: (_o: unknown, _a: unknown, fallback: unknown) => fallback, + }), + useObjectTranslation: () => ({ + t: (key: string, options?: { defaultValue?: string }) => String(options?.defaultValue ?? key), + }), +})); + +// Imported for real these drag in the modal/form graph; nothing here renders them. +vi.mock('../useActionModal', () => ({ + useActionModal: () => ({ + modalHandler: vi.fn(), + modalElement: null, + closeModal: () => {}, + resolveModalTarget: vi.fn(async () => null), + }), +})); +vi.mock('../../views/ActionConfirmDialog', () => ({ ActionConfirmDialog: () => null })); +vi.mock('../../views/ActionParamDialog', () => ({ ActionParamDialog: () => null })); +vi.mock('../../views/ActionResultDialog', () => ({ ActionResultDialog: () => null })); +vi.mock('../../views/FlowRunner', () => ({ FlowRunner: () => null })); +vi.mock('sonner', () => ({ toast: Object.assign(vi.fn(), { error: vi.fn(), success: vi.fn() }) })); + +import type { ActionDef, ActionResult } from '@object-ui/core'; +import { useConsoleActionRuntime } from '../useConsoleActionRuntime'; + +let warn: ReturnType; +beforeEach(() => { + warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); +}); +afterEach(() => vi.restoreAllMocks()); + +/** Run the card's action through the generic branch against `row`. */ +async function closeTask(row: Record, extra: Record = {}) { + const dataSource = { update: vi.fn(async () => ({})) }; + const { result } = renderHook(() => + useConsoleActionRuntime({ dataSource, objects: [], objectName: 'task' }), + ); + let res: ActionResult = { success: false }; + await act(async () => { + res = await result.current.apiHandler({ + type: 'api', + name: 'close_task', + label: 'Close', + target: 'close_task', + undoable: true, + bodyExtra: { status: 'closed' }, + params: { _rowRecord: row }, + ...extra, + } as ActionDef); + }); + return { res, dataSource }; +} + +describe('console `api` handler — the Undo snapshot never records an absent field as `null` (objectui#10404)', () => { + it('offers no Undo when the row lacks the written field, and still performs the write', async () => { + // A projected row: `status` was not selected. + const { res, dataSource } = await closeTask({ id: 't_1', name: 'Ada' }); + + expect(dataSource.update).toHaveBeenCalledWith('task', 't_1', { status: 'closed' }); + expect(res.success).toBe(true); + expect(res.undo).toBeUndefined(); + expect(warn.mock.calls[0][1]).toEqual({ action: 'close_task', missing: ['status'] }); + }); + + it('control: a row that carries the field snapshots its stored value', async () => { + const { res } = await closeTask({ id: 't_1', name: 'Ada', status: 'open' }); + + expect(res.undo?.undoData).toEqual({ status: 'open' }); + expect(res.undo?.redoData).toEqual({ status: 'closed' }); + expect(warn).not.toHaveBeenCalled(); + }); + + it('still snapshots a `null` the row carries — a real empty value', async () => { + const { res } = await closeTask({ id: 't_1', status: null }); + + expect(res.undo?.undoData).toEqual({ status: null }); + }); + + it('offers no partial Undo: a carried `bodyExtra` field beside an absent param field is no Undo at all', async () => { + const { res } = await closeTask( + { id: 't_1', status: 'open' }, + { params: { _rowRecord: { id: 't_1', status: 'open' }, note: 'typed' } }, + ); + + expect(res.success).toBe(true); + expect(res.undo).toBeUndefined(); + expect(warn.mock.calls[0][1]).toEqual({ action: 'close_task', missing: ['note'] }); + }); +}); diff --git a/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx b/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx index aab96e6eaa..f8dab33e82 100644 --- a/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx +++ b/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx @@ -522,21 +522,43 @@ export function useConsoleActionRuntime(opts: ConsoleActionRuntimeOptions): Cons // Undoable single-record update: capture the prior values of the changed // fields from the row record so the success toast can offer "Undo". + // + // ⛔ A field the row does not CARRY is never captured as `null` + // (objectui#10404) — the rule `captureUpdateUndoData` in + // `@object-ui/core`'s `ActionRunner` states, applied to this handler's + // written set (`params` plus `bodyExtra`). A list row is projected by + // `$select`, so a written field no column shows is absent while the + // server holds a real value; recording `null` made Undo overwrite it. A + // `null` the row carries is a real empty value and is captured as one. + // When any written field is not carried there is no Undo at all: the + // success toast then has no Undo button, and the warning names the cause. let undo: ActionResult['undo']; if (action.undoable && obj && recId && rowRecord && Object.keys(fields).length > 0 && typeof dataSource?.update === 'function') { - const undoData: Record = {}; - for (const k of Object.keys(fields)) undoData[k] = rowRecord[k] ?? null; - undo = { - id: `undo-${obj}-${recId}-${Date.now()}`, - type: 'update', - objectName: obj, - recordId: String(recId), - timestamp: Date.now(), - description: action.label || `Undo ${obj}`, - undoData, - redoData: { ...fields }, - }; + const written = Object.keys(fields); + const missing = written.filter( + (k) => !Object.prototype.hasOwnProperty.call(rowRecord, k) || rowRecord[k] === undefined, + ); + if (missing.length === 0) { + undo = { + id: `undo-${obj}-${recId}-${Date.now()}`, + type: 'update', + objectName: obj, + recordId: String(recId), + timestamp: Date.now(), + description: action.label || `Undo ${obj}`, + undoData: Object.fromEntries(written.map((k) => [k, rowRecord[k]])), + redoData: { ...fields }, + }; + } else { + console.warn( + '[useConsoleActionRuntime] `undoable` action succeeded but offers no Undo: the row it ran on ' + + 'does not carry every field it wrote, so their prior values are unknown and an Undo would ' + + 'overwrite stored data. A list row carries a written field when the object declares it and ' + + 'the principal may read it.', + { action: action.name, missing }, + ); + } } const shouldRefresh = action.refreshAfter !== false; diff --git a/packages/core/src/actions/ActionRunner.ts b/packages/core/src/actions/ActionRunner.ts index a9684f3a06..dc7bf353d5 100644 --- a/packages/core/src/actions/ActionRunner.ts +++ b/packages/core/src/actions/ActionRunner.ts @@ -883,16 +883,36 @@ function isUpdateOperationAction(action: ActionDef): boolean { * * Prior values come from the row record the invoking surface stashed under * `params._rowRecord` (the same client-side stash the record-id dance reads). - * A field absent from that row is captured as `null`, which is what clearing it - * back to empty means on the data plane. + * A field the row carries as `null` is captured as `null`: it was empty, and + * clearing it back to empty is what restoring it means on the data plane. + * + * ⛔ A field the row does not CARRY is never captured as `null` + * (objectui#10404). Absent is not empty: a list row is projected by `$select`, + * and a written field no column shows is missing from the row while the server + * holds a real value for it. Recording `null` there made Undo write `null` over + * the value it existed to restore. The list harvest (`listViewPredicates`) now + * asks for the written fields of an `undoable` action, so a projected row + * carries them; this is the backstop for the ones it cannot ask for (a field + * the object does not declare or the principal may not read, a row no harvest + * shaped). "Carries" means an own key whose value is not `undefined`: JSON + * cannot send `undefined`, so such a value says nothing about what is stored. + * + * Answers `undefined` when any written field is not carried, and the caller + * then offers no Undo at all. Capturing only the carried fields is not an + * option, for the reason above: a partial restore reported as a full one is + * worse than no Undo. */ function captureUpdateUndoData( writtenFields: readonly string[], rowRecord: Record, -): Record { - const undoData: Record = {}; - for (const field of writtenFields) undoData[field] = rowRecord[field] ?? null; - return undoData; +): Record | undefined { + if (!writtenFields.every((field) => rowCarries(rowRecord, field))) return undefined; + return Object.fromEntries(writtenFields.map((field) => [field, rowRecord[field]])); +} + +/** Whether the row CARRIES `field`: an own key whose value is not `undefined`. */ +function rowCarries(rowRecord: Record, field: string): boolean { + return Object.prototype.hasOwnProperty.call(rowRecord, field) && rowRecord[field] !== undefined; } /* @@ -1558,21 +1578,37 @@ export class ActionRunner { // Undo: prior values of exactly the fields this action wrote. Needs a row // record to read them from — without one there is nothing to restore, so - // the affordance is correctly not offered rather than offered empty. + // the affordance is correctly not offered rather than offered empty. The + // same holds when the row lacks a written field (objectui#10404): no Undo, + // never one that writes `null` over the stored value. if (action.undoable && rowRecord && writtenFields.length > 0) { const objectName = action.objectName || readContextObjectName(this.context); const recordId = collected.recordId ?? rowRecord.id; if (objectName && recordId != null) { - result.undo = { - id: `undo-${objectName}-${String(recordId)}-${Date.now()}`, - type: 'update', - objectName, - recordId: String(recordId), - timestamp: Date.now(), - description: action.label || `Undo ${objectName}`, - undoData: captureUpdateUndoData(writtenFields, rowRecord), - redoData: Object.fromEntries(writtenFields.map((k) => [k, params[k]])), - }; + const undoData = captureUpdateUndoData(writtenFields, rowRecord); + if (undoData) { + result.undo = { + id: `undo-${objectName}-${String(recordId)}-${Date.now()}`, + type: 'update', + objectName, + recordId: String(recordId), + timestamp: Date.now(), + description: action.label || `Undo ${objectName}`, + undoData, + redoData: Object.fromEntries(writtenFields.map((k) => [k, params[k]])), + }; + } else { + // The success toast then carries no Undo button, which is how the + // user learns this one cannot be undone; this names the cause for + // the author. + console.warn( + '[ActionRunner] `undoable` update succeeded but offers no Undo: the row it ran on does not ' + + 'carry every field it wrote, so their prior values are unknown and an Undo would overwrite ' + + 'stored data. A list row carries a written field when the object declares it and the ' + + 'principal may read it.', + { action: action.name, missing: writtenFields.filter((k) => !rowCarries(rowRecord, k)) }, + ); + } } } diff --git a/packages/core/src/actions/__tests__/ActionRunner.undoAbsentField-10404.test.ts b/packages/core/src/actions/__tests__/ActionRunner.undoAbsentField-10404.test.ts new file mode 100644 index 0000000000..9cc38984d4 --- /dev/null +++ b/packages/core/src/actions/__tests__/ActionRunner.undoAbsentField-10404.test.ts @@ -0,0 +1,126 @@ +/** + * 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. + */ + +/** + * objectui#10404 — an `undoable` update never records a field the row does not + * carry as `null`. + * + * The capture reads each written field's prior value off the row the surface + * stashed under `params._rowRecord`. A list row is projected by `$select`, so a + * written field no column shows is ABSENT from it while the server holds a real + * value. It used to be captured as `null`, and Undo then wrote `null` over the + * value it existed to restore (measured with a real `ObjectGrid`: + * `undoData` `{ status: null }` against a stored `'open'`). + * + * The list harvest now makes a projected row carry the written fields (see + * `predicate-fields.test.ts` and plugin-grid's `undoProjectedRow-10404`); this + * file pins the backstop for a row that still lacks one: no Undo at all, and a + * success toast without the Undo affordance. A `null` the row DOES carry is a + * real empty value and is still captured, so a full record is not regressed. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { ActionRunner, type ActionDef } from '../ActionRunner'; +import { globalUndoManager } from '../UndoManager'; + +afterEach(() => { + vi.restoreAllMocks(); + globalUndoManager.clear(); +}); + +/** + * A runner with a `script` dispatch that answers success, and a toast spy that + * records whether the success toast offered Undo. + */ +function wireRunner() { + const runner = new ActionRunner({ objectName: 'task' }); + const toasts: Array<{ type?: string; undo: boolean }> = []; + runner.setToastHandler((_message, opts) => { + toasts.push({ type: opts?.type, undo: opts?.undo !== undefined }); + }); + runner.registerHandler('script', vi.fn(async () => ({ success: true }))); + return { runner, toasts }; +} + +/** The card's action: an undoable update writing a field no column shows. */ +function closeTask(row: Record, extra: Partial = {}): ActionDef { + return { + type: 'script', + name: 'close_task', + label: 'Close', + operation: 'update', + undoable: true, + patch: { status: 'closed' }, + params: { _rowRecord: row }, + ...extra, + } as ActionDef; +} + +describe('undoable update — a field the row does not carry is never captured as `null` (objectui#10404)', () => { + it('offers no Undo when the row lacks the written field, and the toast carries no Undo button', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const push = vi.spyOn(globalUndoManager, 'push'); + const { runner, toasts } = wireRunner(); + + // A projected row: `status` was not selected, so the key is simply absent. + const result = await runner.execute(closeTask({ id: 't_1', name: 'Ada' })); + + expect(result.success).toBe(true); + expect(result.undo).toBeUndefined(); + expect(push).not.toHaveBeenCalled(); + expect(toasts).toEqual([{ type: 'success', undo: false }]); + // The author is told which field was missing. + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0][1]).toEqual({ action: 'close_task', missing: ['status'] }); + }); + + it('control: the same action on a row that carries the field captures its stored value', async () => { + const push = vi.spyOn(globalUndoManager, 'push'); + const { runner, toasts } = wireRunner(); + + const result = await runner.execute(closeTask({ id: 't_1', name: 'Ada', status: 'open' })); + + expect(result.undo?.undoData).toEqual({ status: 'open' }); + expect(result.undo?.redoData).toEqual({ status: 'closed' }); + expect(push).toHaveBeenCalledTimes(1); + expect(toasts).toEqual([{ type: 'success', undo: true }]); + }); + + it('still captures a `null` the row carries — an empty field restored to empty (a full record is not regressed)', async () => { + const { runner } = wireRunner(); + + const result = await runner.execute(closeTask({ id: 't_1', name: 'Ada', status: null })); + + expect(result.undo?.undoData).toEqual({ status: null }); + }); + + it('treats an own key holding `undefined` as not carried — JSON cannot send one, so it says nothing stored', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}); + const { runner } = wireRunner(); + + const result = await runner.execute(closeTask({ id: 't_1', status: undefined })); + + expect(result.success).toBe(true); + expect(result.undo).toBeUndefined(); + }); + + it('offers no partial Undo: one carried field and one absent field is no Undo at all', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const { runner } = wireRunner(); + + // `note` is a collected param the row does not carry; `status` is carried. + const result = await runner.execute(closeTask( + { id: 't_1', status: 'open' }, + { params: { _rowRecord: { id: 't_1', status: 'open' }, note: 'typed' } as never }, + )); + + expect(result.success).toBe(true); + expect(result.undo).toBeUndefined(); + expect(warn.mock.calls[0][1]).toEqual({ action: 'close_task', missing: ['note'] }); + }); +}); diff --git a/packages/core/src/utils/__tests__/column-identity.ratchet.test.ts b/packages/core/src/utils/__tests__/column-identity.ratchet.test.ts index fdddc02391..521c59750e 100644 --- a/packages/core/src/utils/__tests__/column-identity.ratchet.test.ts +++ b/packages/core/src/utils/__tests__/column-identity.ratchet.test.ts @@ -122,7 +122,7 @@ interface Entry { const INVENTORY: Record = { // ── the column-identity family: EMPTY (was 24 in PR1; 22 after re-triage) ── - // ── two layers, not two spellings (6) ──────────────────────────────────── + // ── two layers, not two spellings (7) ──────────────────────────────────── 'app-shell/src/utils/resolveActionParams.ts': { count: 2, verdict: 'two-layer', @@ -144,9 +144,9 @@ const INVENTORY: Record = { why: 'Action param name, same layering as resolveActionParams.', }, 'core/src/utils/predicate-fields.ts': { - count: 1, + count: 2, verdict: 'two-layer', - why: 'The ROW-key half of resolveActionParams\' pair, mirrored on purpose (objectui#10277): `defaultFromRowKey` names the row key a `defaultFromRow` param seeds from, so the `$select` harvest can ask for it. `field ?? name` because row data is keyed by object field, the precedence `rowValueKey` reads. Deliberately NOT `columnIdentity()`: that reader also accepts `fieldName` and treats an empty string as absent, so it would harvest keys the runtime never looks up.', + why: 'BOTH halves of resolveActionParams\' pair, each mirrored on purpose. `defaultFromRowKey` (objectui#10277) names the row key a `defaultFromRow` param seeds from, so the `$select` harvest can ask for it: `field ?? name`, because row data is keyed by object field, the precedence `rowValueKey` reads. `undoableWrittenKeys` (objectui#10404) names the key a param\'s collected value is WRITTEN under, whose prior value the Undo capture then looks up on the row: `name ?? field`, the precedence `paramName` reads. Two concepts, as in the file they mirror. Deliberately NOT `columnIdentity()`: that reader also accepts `fieldName` and treats an empty string as absent, so it would harvest keys the runtime never looks up.', }, // ── the form cluster — settled the other way (#3090) (3) ───────────────── @@ -266,8 +266,9 @@ describe('column identity dual-read ratchet (#3104)', () => { expect(sum((e) => e.verdict === 'column-identity')).toBe(0); // 12 at #3104 PR2; 11 since objectui#3174 routed `resolveActionParams`' // two orders through one named reader each; 12 again since objectui#10277 - // mirrored that file's row-key reader into the `$select` harvest. - expect(sum(() => true)).toBe(12); + // mirrored that file's row-key reader into the `$select` harvest; 13 since + // objectui#10404 mirrored its param-name reader there too. + expect(sum(() => true)).toBe(13); }); it('records a precedence for every read left in the family', () => { diff --git a/packages/core/src/utils/__tests__/predicate-fields.test.ts b/packages/core/src/utils/__tests__/predicate-fields.test.ts index 7a5cfee304..fd4cfbc1f1 100644 --- a/packages/core/src/utils/__tests__/predicate-fields.test.ts +++ b/packages/core/src/utils/__tests__/predicate-fields.test.ts @@ -266,6 +266,90 @@ describe('listViewPredicates — `defaultFromRow` params and `target` tokens (ob }); }); +describe('listViewPredicates — the fields an `undoable` action writes (objectui#10404)', () => { + /** + * The card's shape: an undoable declarative update whose written field is no + * column. The Undo capture reads `status`'s prior value off the row, so the + * projection owes it. + */ + const CLOSE = { + name: 'close_task', + operation: 'update', + undoable: true, + locations: ['list_item'], + patch: { status: 'closed' }, + }; + const refs = (view: Parameters[0]) => + collectPredicateFieldRefs(listViewPredicates(view)); + + it('harvests the `patch` keys of an undoable update', () => { + expect(refs({ rowActionDefs: [CLOSE] })).toEqual(['status']); + }); + + it('harvests nothing written when the action is not `undoable` — nothing reads those keys off the row', () => { + // `visible` is the same-fixture control: the harvest reached this def, so + // the missing `status` is the `undoable` narrowing, not a harvest that ran on nothing. + const { undoable: _undoable, ...plain } = CLOSE; + expect(refs({ rowActionDefs: [{ ...plain, visible: 'record.active' }] })).toEqual(['active']); + }); + + it('harvests each param under the key its collected value is WRITTEN under: `name`, else `field`', () => { + // The opposite precedence to a `defaultFromRow` seed (`field ?? name`): the + // capture looks up the WRITTEN key. `seed_from` shows both rules on one + // param, since it is also `defaultFromRow`. + expect( + refs({ + rowActionDefs: [{ + ...CLOSE, + params: [ + { name: 'note', field: 'note_field' }, + { field: 'due_date' }, + { name: 'owner_ref', field: 'seed_from', defaultFromRow: true }, + ], + }], + }), + ).toEqual(['seed_from', 'status', 'note', 'due_date', 'owner_ref']); + }); + + it('harvests the `bodyExtra` keys the console `api` handler writes', () => { + expect( + refs({ + objectActions: [{ + name: 'archive', type: 'api', target: 'archive', undoable: true, bodyExtra: { archived: true }, + }], + }), + ).toEqual(['archived']); + }); + + it('leaves out `recordId`, which both writers strip as the record address, and non-identifier keys', () => { + // `status` is the same-fixture control: the harvest ran on this def. + expect( + refs({ + rowActionDefs: [{ + ...CLOSE, + patch: { status: 'closed', 'not a field': 1, recordId: 'r_1' }, + params: [{ name: 'recordId' }, { name: 'outputs.comment' }], + }], + }), + ).toEqual(['status']); + }); + + it('reaches the written keys on all three action lists, and reads nothing out of a malformed bag', () => { + // `d` is the same-fixture control for the malformed half. + expect( + refs({ + rowActionDefs: [{ ...CLOSE, patch: { a_field: 1 } }], + bulkActionDefs: [{ ...CLOSE, patch: { b_field: 1 } }], + objectActions: [ + { ...CLOSE, patch: { c_field: 1 } }, + { name: 'd', undoable: true, patch: ['x'], bodyExtra: 'y', params: [null, 42, { name: 7 }] }, + { name: 'e', undoable: true, patch: { d_field: 1 } }, + ], + }), + ).toEqual(['a_field', 'b_field', 'c_field', 'd_field']); + }); +}); + describe('isProjectableField', () => { const declared = { title: { type: 'text' }, owner: { type: 'user' } }; diff --git a/packages/core/src/utils/predicate-fields.ts b/packages/core/src/utils/predicate-fields.ts index dab57595a6..b83a051014 100644 --- a/packages/core/src/utils/predicate-fields.ts +++ b/packages/core/src/utils/predicate-fields.ts @@ -33,11 +33,13 @@ * predicate.) * * Since objectstack#8018 the same reader also carries the non-predicate row keys - * a view's actions read: `recordIdField`, and since objectui#10277 the field a + * a view's actions read: `recordIdField`, since objectui#10277 the field a * `defaultFromRow` param seeds from and the `{field}` tokens of an action's - * `target`. The name kept the "predicate" spelling because the mechanism is - * identical (harvest a name, gate it against the declared fields, add it to - * `$select`); see {@link listViewPredicates} for what is in the set and why. + * `target`, and since objectui#10404 the fields an `undoable` action writes, + * whose prior values the Undo capture reads off the row. The name kept the + * "predicate" spelling because the mechanism is identical (harvest a name, gate + * it against the declared fields, add it to `$select`); see + * {@link listViewPredicates} for what is in the set and why. */ /** @@ -123,6 +125,46 @@ function defaultFromRowKey(param: unknown): unknown { return p.field ?? p.name; } +/** + * The keys an `undoable` action WRITES, which are the keys its Undo capture + * reads off the row (objectui#10404). Returned unfiltered; the caller applies + * the identifier gate. + * + * Each source is the one a writer actually merges into the written set: + * + * - `patch`: the static field values of `operation: 'update'`, merged under the + * collected params (`ActionRunner`'s `executeUpdateOperation`); + * - each param's COLLECTED key, `name ?? field`: the key the param dialog + * stores its answer under (`paramName` in `app-shell`'s + * `resolveActionParams`). This is the opposite precedence to + * {@link defaultFromRowKey} on purpose: that one asks which row key a value + * is READ from, this one asks which key a value is WRITTEN under, and the + * capture looks the written key up on the row; + * - `bodyExtra`: the static body the console's generic `api` handler merges + * over the params before it writes (`useConsoleActionRuntime`). + * + * `recordId` is left out: both writers read it as the record's address and + * strip it from the written set, so no capture ever looks it up. + * + * A key the harvest cannot make the row carry (undeclared, or a field the + * principal may not read) is not rescued here. The capture refuses to invent a + * prior value for it instead, and offers no Undo. + */ +function undoableWrittenKeys(def: Record): unknown[] { + const keys: unknown[] = []; + for (const bag of [def.patch, def.bodyExtra]) { + if (bag && typeof bag === 'object' && !Array.isArray(bag)) keys.push(...Object.keys(bag)); + } + if (Array.isArray(def.params)) { + for (const param of def.params) { + if (!param || typeof param !== 'object') continue; + const p = param as Record; + keys.push(p.name ?? p.field); + } + } + return keys.filter((key) => key !== 'recordId'); +} + /** Pull the CEL source out of any of the shapes a predicate is authored in. */ function predicateSource(pred: unknown): string | null { if (typeof pred === 'string') return pred.trim() || null; @@ -172,11 +214,12 @@ export function collectPredicateFieldRefs(predicates: readonly unknown[]): strin * kebab (custom defs AND the built-in Edit/Delete overrides), the selection bar, * and the object's declared actions. * - * Most entries are predicates. Four are not, and all are spelled as a synthetic + * Most entries are predicates. Five are not, and all are spelled as a synthetic * `record.` so the one harvester handles them: conditional formatting's * native `{ field, operator, value }` shape, an action's `recordIdField` - * (objectstack#8018), the field each `defaultFromRow` param seeds from, and each - * `{field}` token of the action's `target` (both objectui#10277). The + * (objectstack#8018), the field each `defaultFromRow` param seeds from, each + * `{field}` token of the action's `target` (both objectui#10277), and the fields + * an `undoable` action writes (objectui#10404). The * `recordIdField` case is the same *class* of bug the predicate harvest exists * to close, one surface over — the action runtime reads * `rowRecord[action.recordIdField]` to seed `recordIdParam`, so a key outside the @@ -195,7 +238,18 @@ export function collectPredicateFieldRefs(predicates: readonly unknown[]): strin * narrowed by the action's `type`, for the asymmetry stated below: an extra * column costs bytes. * - * A `recordIdField` or param key that is not a bare identifier is dropped here + * The written fields are the same class once more, and the worst of them, + * because what went wrong was a write. The Undo capture of an `undoable` update + * reads each written field's prior value off the row; on a projected row a + * written field no column shows was absent, the capture recorded `null`, and + * Undo then wrote `null` over the stored value it existed to restore. The + * written keys are `patch`, each param's collected key and `bodyExtra` (see + * `undoableWrittenKeys`). This one IS narrowed, to `undoable`, and not for + * cost: the capture is the only reader of a written field's prior value, and it + * runs only under `undoable`, so without it the row reads none of these keys. + * + * A `recordIdField`, param key or written key that is not a bare identifier is + * dropped here * (a `target` token is one by construction of its grammar), and a name the * object does not declare is dropped by the caller's `isProjectableField` * guard. Both drops are the safe direction: such a name is not a column @@ -260,6 +314,14 @@ export function listViewPredicates(view: { preds.push(`record.${m[1]}`); } } + // The fields an `undoable` action writes (objectui#10404): its Undo + // capture reads each one's prior value off the row. Same rule, same + // identifier gate. + if (d.undoable) { + for (const key of undoableWrittenKeys(d)) { + if (typeof key === 'string' && BARE_IDENTIFIER.test(key)) preds.push(`record.${key}`); + } + } } } for (const override of Object.values(view.userActions ?? {})) { diff --git a/packages/plugin-grid/src/__tests__/undoProjectedRow-10404.test.tsx b/packages/plugin-grid/src/__tests__/undoProjectedRow-10404.test.tsx new file mode 100644 index 0000000000..73a2329344 --- /dev/null +++ b/packages/plugin-grid/src/__tests__/undoProjectedRow-10404.test.tsx @@ -0,0 +1,210 @@ +/** + * 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. + */ + +/** + * objectui#10404 — Undo on a projected grid row restores the stored value, and + * never writes `null` over one. + * + * The grid projects what it displays plus what its actions read off the row. An + * `undoable` update reads the prior value of every field it writes off that row, + * so a written field no column shows was ABSENT from it. The runner captured it + * as `null`, and Undo wrote `null` over the stored value: measured on `main` + * before the fix with this fixture, `$select` was `["id","name"]`, `undoData` + * `{ status: null }`, and after Undo the stored `status` was `null`, not + * `'open'`. Control: with `status` as a column, `undoData` read `{ status: 'open' }`. + * + * Everything here is real except the network: the `ObjectGrid`, its kebab, the + * `ActionRunner` behind `ActionProvider`, and the Undo itself, which runs through + * the real `useGlobalUndo` executor. The `script` dispatch stands in for the + * platform action route and writes the params it is sent, and the data source + * honours `$select` the way a real backend does (an unselected key is absent, + * not `undefined`-valued). + * + * PIN 1 is the card's pin (the harvest): the row carries `status`, and Undo puts + * `'open'` back. PIN 2 is the backstop: a field the harvest cannot make the row + * carry (declared, writable, but not readable by the principal) gets no Undo at + * all, so nothing ever writes `null` over it. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, screen, waitFor, cleanup, act } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import '@testing-library/jest-dom'; +import React from 'react'; + +/** Field-level read policy, switched per test. Stable identity: `ObjectGrid` keys memos on `perms`. */ +const { permsStub, policy } = vi.hoisted(() => { + const policy: { readable: string[] } = { readable: [] }; + return { + policy, + permsStub: { + isLoaded: true, + checkField: (_object: string, field: string, action: string) => + action === 'read' ? policy.readable.includes(field) : true, + check: () => ({ allowed: true }), + getFieldPermissions: () => [], + getRowFilter: () => undefined, + getObjectApiOperations: () => undefined, + roles: [], + userId: null, + systemPermissions: undefined, + hasCapabilities: () => true, + can: () => true, + cannot: () => false, + }, + }; +}); + +vi.mock('@object-ui/permissions', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, usePermissions: () => permsStub as never }; +}); + +import { ObjectGrid } from '../ObjectGrid'; +import { registerAllFields } from '@object-ui/fields'; +import { ActionProvider, SchemaRendererProvider, useGlobalUndo, type UseGlobalUndoOptions } from '@object-ui/react'; +import { globalUndoManager } from '@object-ui/core'; + +registerAllFields(); + +const OBJECT = 'task'; +const OBJECT_FIELDS = { + name: { type: 'text', label: 'Name' }, + status: { type: 'text', label: 'Status' }, +}; + +/** The card's action: an undoable update whose written field is no column. */ +const CLOSE_TASK = { + name: 'close_task', + label: 'Close', + type: 'script', + operation: 'update', + undoable: true, + locations: ['list_item'], + patch: { status: 'closed' }, +}; + +function makeWorld() { + const stored: Record = { id: 't_1', name: 'Ada', status: 'open' }; + const dataSource = { + find: vi.fn(async (_object: string, params?: { $select?: string[] }) => { + const select = params?.$select; + const row = select + ? Object.fromEntries(Object.entries(stored).filter(([key]) => select.includes(key))) + : { ...stored }; + return { data: [row], total: 1 }; + }), + findOne: vi.fn(), + create: vi.fn(), + delete: vi.fn(), + update: vi.fn(async (_object: string, _id: string, data: Record) => { + Object.assign(stored, data); + return { ...stored }; + }), + getObjectSchema: vi.fn(async () => ({ name: OBJECT, fields: OBJECT_FIELDS })), + }; + // Stands in for the platform action route: writes what it is sent. + const script = vi.fn(async (action: { params?: Record }) => { + const { _rowRecord: _row, recordId: _id, ...fields } = action.params ?? {}; + Object.assign(stored, fields); + return { success: true }; + }); + return { stored, dataSource, script }; +} + +/** Exposes the real `useGlobalUndo` executor — the one the console's Undo button calls. */ +function UndoHandle({ dataSource, onReady }: { + dataSource: UseGlobalUndoOptions['dataSource']; + onReady: (undo: () => Promise) => void; +}) { + const { undo } = useGlobalUndo({ dataSource }); + onReady(undo); + return null; +} + +async function closeTheRow(readable: string[]) { + policy.readable = readable; + const world = makeWorld(); + const toasts: Array<{ type?: string; undo: boolean }> = []; + let undo: (() => Promise) | undefined; + render( + { toasts.push({ type: opts?.type, undo: opts?.undo !== undefined }); }} + > + + { undo = fn; }} /> + + + , + ); + await waitFor(() => expect(screen.getByText('Ada')).toBeInTheDocument()); + const select = (world.dataSource.find.mock.calls.at(-1)?.[1]?.$select ?? []) as string[]; + await userEvent.click(await screen.findByTestId('row-action-trigger')); + await userEvent.click(await screen.findByTestId('row-action-close_task')); + await waitFor(() => expect(toasts).toHaveLength(1)); + return { ...world, select, toasts, undo: () => act(async () => { await undo!(); }) }; +} + +beforeEach(() => { + vi.clearAllMocks(); + globalUndoManager.clear(); +}); +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); + globalUndoManager.clear(); +}); + +describe('ObjectGrid — Undo of an `undoable` update on a projected row (objectui#10404)', () => { + it('PIN 1: the row carries the written field, and Undo puts the stored value back', async () => { + const run = await closeTheRow(['name', 'status']); + const afterWrite = run.stored.status; + const captured = globalUndoManager.peekUndo()?.undoData; + + await run.undo(); + + // The card's outcome first, so a regression reads as what the user sees: + // before the fix this was `null`. + expect(run.stored.status).toBe('open'); + expect(run.dataSource.update).toHaveBeenCalledWith(OBJECT, 't_1', { status: 'open' }); + expect(captured).toEqual({ status: 'open' }); + // The write happened, and the toast offered Undo. + expect(afterWrite).toBe('closed'); + expect(run.toasts).toEqual([{ type: 'success', undo: true }]); + // Why the row carried it: the harvest asked for the written field though no + // column shows it. + expect(run.select).toEqual(expect.arrayContaining(['id', 'name', 'status'])); + }); + + it('PIN 2: a written field the row cannot carry gets no Undo, so nothing writes `null` over it', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + // `status` is writable but not readable: the harvest names it and the FLS + // gate drops it, exactly as it must (harvesting is not a read grant). + const run = await closeTheRow(['name']); + const pending = globalUndoManager.peekUndo(); + + // Ctrl+Z / the Undo executor finds nothing to run, so no `null` is written. + // Before the fix this wrote `{ status: null }`. + await run.undo(); + expect(run.stored.status).toBe('closed'); + expect(run.dataSource.update).not.toHaveBeenCalled(); + + // No Undo affordance on the toast, nothing on the Undo stack, and the + // author is told which field was missing. + expect(pending).toBeUndefined(); + expect(run.toasts).toEqual([{ type: 'success', undo: false }]); + expect(warn.mock.calls.some(([, detail]) => + JSON.stringify(detail) === JSON.stringify({ action: 'close_task', missing: ['status'] }))).toBe(true); + expect(run.select).not.toContain('status'); + }); +});