From 8566fc80d197258078f3c051c8db7796aea5160f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 08:08:22 +0000 Subject: [PATCH] feat(plugin-kanban): a drop that makes fields required collects them instead of dead-ending (#4254) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dragging a card into a column whose value flips a field's `requiredWhen` predicate to TRUE PATCHed the column value alone. The engine refused the whole update — correctly; that is what the predicate declares — and the board had no way to finish the move, so closing a won deal meant leaving the board for the record form. The drop now pre-evaluates the target column's predicates against the record WITH the target value applied. Fields the move makes required while still empty are collected in a small dialog, and the column value plus everything collected go out as ONE PATCH: two writes would leave the record in the refused state if the second failed. A drop that triggers no predicate keeps its exact path, PATCH body included. One evaluator, not two: the verdict is `@object-ui/core`'s `resolveFieldRuleState`, the same function the record form, the wizard and the line-item grid already use, delegating to `@objectstack/formula`'s CEL engine — so the board's prompt and the server's enforcement cannot drift. Emptiness is core's `isMissingForRequired`, the presence contract the form and the server share. Every control is `@object-ui/fields`' `FieldEditWidget`, the widget the record form renders for that type, so no second set of field-rendering decisions is introduced. Four kinds of field are deliberately not collected and fall through to the unchanged PATCH, where the refusal is legible since objectstack#7525: one that already has a value, one `visibleWhen` hides, a readonly one, and a type with no edit widget. Cancel writes nothing and leaves the card in its source column; a combined PATCH refused for some other reason surfaces the refusal and rolls back exactly as a plain rejected move does (#4138), rather than looping the dialog on an arbitrary server error. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3 --- .changeset/kanban-requiredwhen-prompt-4254.md | 20 ++ packages/i18n/src/locales/ar.ts | 2 + packages/i18n/src/locales/de.ts | 2 + packages/i18n/src/locales/en.ts | 2 + packages/i18n/src/locales/es.ts | 2 + packages/i18n/src/locales/fr.ts | 2 + packages/i18n/src/locales/ja.ts | 2 + packages/i18n/src/locales/ko.ts | 2 + packages/i18n/src/locales/pt.ts | 2 + packages/i18n/src/locales/ru.ts | 2 + packages/i18n/src/locales/zh.ts | 2 + .../ObjectKanban.requiredWhenPrompt.test.tsx | 326 ++++++++++++++++++ packages/plugin-kanban/src/ObjectKanban.tsx | 141 +++++++- .../src/RequiredFieldsDialog.tsx | 154 +++++++++ .../src/requiredWhenPrompt.test.ts | 174 ++++++++++ .../plugin-kanban/src/requiredWhenPrompt.ts | 128 +++++++ 16 files changed, 954 insertions(+), 9 deletions(-) create mode 100644 .changeset/kanban-requiredwhen-prompt-4254.md create mode 100644 packages/plugin-kanban/src/ObjectKanban.requiredWhenPrompt.test.tsx create mode 100644 packages/plugin-kanban/src/RequiredFieldsDialog.tsx create mode 100644 packages/plugin-kanban/src/requiredWhenPrompt.test.ts create mode 100644 packages/plugin-kanban/src/requiredWhenPrompt.ts diff --git a/.changeset/kanban-requiredwhen-prompt-4254.md b/.changeset/kanban-requiredwhen-prompt-4254.md new file mode 100644 index 000000000..78ca21108 --- /dev/null +++ b/.changeset/kanban-requiredwhen-prompt-4254.md @@ -0,0 +1,20 @@ +--- +'@object-ui/plugin-kanban': minor +'@object-ui/i18n': patch +--- + +Kanban: a drop that makes fields required now collects them instead of dead-ending + +Dragging a card into a column whose value flips a field's `requiredWhen` predicate to TRUE used to PATCH the column value alone. The engine refused the whole update — correctly, that is what the predicate declares — and the board had no way to finish the move: the only path to closing a won deal was to abandon the board and open the record form. HotCRM's opportunity pipeline is the reported case (`win_reason` is required when `stage == "closed_won"`), but the dead end belonged to every board whose target column carries a conditional requirement. + +The board now evaluates the target column's predicates BEFORE writing anything. If the move would make fields required while they are still empty, it opens a small dialog collecting exactly those fields, then submits the column value and everything collected as ONE PATCH — never two writes, which would leave the record in the refused state if the second one failed. A drop that triggers no predicate is untouched, down to the PATCH body. + +The verdict comes from `@object-ui/core`'s `resolveFieldRuleState` — the same evaluator the record form, the wizard and the line-item grid already resolve `visibleWhen`/`readonlyWhen`/`requiredWhen` with, delegating to `@objectstack/formula`'s CEL engine. The board's prompt and the server's enforcement therefore reach the identical verdict rather than drifting through a second hand-rolled predicate evaluator. Emptiness is core's `isMissingForRequired`, the presence contract the form and the server share, so a `false` boolean and a `0` count as answers and are not re-asked. + +Every control in the dialog is `@object-ui/fields`' `FieldEditWidget`, the same widget the record form renders for that field type — a select edits as a select, a date as a date picker — so this adds no second set of field-rendering decisions. + +Four kinds of field are deliberately NOT collected, and each falls through to the unchanged PATCH where the server's refusal (legible since objectstack#7525) speaks for itself: one that already has a value, one `visibleWhen` hides, one that is readonly, and one whose type has no edit widget at all. A dialog row with no control would be a worse dead end than the one being fixed. + +Cancelling writes nothing and leaves the card in its original column; a combined PATCH that is still refused for some other reason surfaces the refusal and rolls back exactly as a plain rejected move does, rather than looping the dialog on an arbitrary server error. + +`@object-ui/i18n` carries two new `kanban.*` strings for the dialog, translated across all ten packs. Its public type surface is unchanged — the `.d.ts` was measured identical before and after — hence the patch bump. diff --git a/packages/i18n/src/locales/ar.ts b/packages/i18n/src/locales/ar.ts index d981fd3c5..9282c84a7 100644 --- a/packages/i18n/src/locales/ar.ts +++ b/packages/i18n/src/locales/ar.ts @@ -631,6 +631,8 @@ const ar = { deleteColumn: "حذف عمود", noCards: "لا بطاقات", cardTitlePlaceholder: "أدخل عنوان البطاقة…", + requiredFieldsTitle: "أكمل الحقول المطلوبة", + requiredFieldsDescription: "يجعل هذا النقل الحقول أدناه مطلوبة. أكملها للمتابعة.", }, timeline: { bucket: { diff --git a/packages/i18n/src/locales/de.ts b/packages/i18n/src/locales/de.ts index 5b4f89d59..70fdf871d 100644 --- a/packages/i18n/src/locales/de.ts +++ b/packages/i18n/src/locales/de.ts @@ -627,6 +627,8 @@ const de = { deleteColumn: "Spalte löschen", noCards: "Keine Karten", cardTitlePlaceholder: "Kartentitel eingeben…", + requiredFieldsTitle: "Erforderliche Felder ausfüllen", + requiredFieldsDescription: "Durch diese Verschiebung werden die folgenden Felder zu Pflichtfeldern. Füllen Sie sie aus, um fortzufahren.", }, timeline: { bucket: { diff --git a/packages/i18n/src/locales/en.ts b/packages/i18n/src/locales/en.ts index 5ced11f9c..f45902a93 100644 --- a/packages/i18n/src/locales/en.ts +++ b/packages/i18n/src/locales/en.ts @@ -676,6 +676,8 @@ const en = { cardTitlePlaceholder: 'Enter card title…', uncategorized: 'Uncategorized', columns: 'columns', + requiredFieldsTitle: 'Complete required fields', + requiredFieldsDescription: 'This move makes the fields below required. Fill them in to continue.', }, timeline: { bucket: { diff --git a/packages/i18n/src/locales/es.ts b/packages/i18n/src/locales/es.ts index ac17a150e..b6aa79ec1 100644 --- a/packages/i18n/src/locales/es.ts +++ b/packages/i18n/src/locales/es.ts @@ -631,6 +631,8 @@ const es = { deleteColumn: "Eliminar columna", noCards: "Sin tarjetas", cardTitlePlaceholder: "Escriba el título de la tarjeta…", + requiredFieldsTitle: "Complete los campos obligatorios", + requiredFieldsDescription: "Este movimiento hace que los campos siguientes sean obligatorios. Complételos para continuar.", }, timeline: { bucket: { diff --git a/packages/i18n/src/locales/fr.ts b/packages/i18n/src/locales/fr.ts index c5dc6f608..99b5ac3e5 100644 --- a/packages/i18n/src/locales/fr.ts +++ b/packages/i18n/src/locales/fr.ts @@ -627,6 +627,8 @@ const fr = { deleteColumn: "Supprimer la colonne", noCards: "Aucune carte", cardTitlePlaceholder: "Saisir le titre de la carte…", + requiredFieldsTitle: "Compléter les champs obligatoires", + requiredFieldsDescription: "Ce déplacement rend les champs ci-dessous obligatoires. Remplissez-les pour continuer.", }, timeline: { bucket: { diff --git a/packages/i18n/src/locales/ja.ts b/packages/i18n/src/locales/ja.ts index 607dfe800..50625d62d 100644 --- a/packages/i18n/src/locales/ja.ts +++ b/packages/i18n/src/locales/ja.ts @@ -627,6 +627,8 @@ const ja = { deleteColumn: "カラムを削除", noCards: "カードがありません", cardTitlePlaceholder: "カードのタイトルを入力…", + requiredFieldsTitle: "必須項目を入力してください", + requiredFieldsDescription: "この移動により以下の項目が必須になります。入力して続行してください。", }, timeline: { bucket: { diff --git a/packages/i18n/src/locales/ko.ts b/packages/i18n/src/locales/ko.ts index f34f332a5..c6e041189 100644 --- a/packages/i18n/src/locales/ko.ts +++ b/packages/i18n/src/locales/ko.ts @@ -627,6 +627,8 @@ const ko = { deleteColumn: "열 삭제", noCards: "카드 없음", cardTitlePlaceholder: "카드 제목 입력…", + requiredFieldsTitle: "필수 항목 입력", + requiredFieldsDescription: "이 이동으로 아래 항목이 필수가 됩니다. 입력한 후 계속하세요.", }, timeline: { bucket: { diff --git a/packages/i18n/src/locales/pt.ts b/packages/i18n/src/locales/pt.ts index d18848c6b..5b007d7c6 100644 --- a/packages/i18n/src/locales/pt.ts +++ b/packages/i18n/src/locales/pt.ts @@ -626,6 +626,8 @@ const pt = { deleteColumn: "Excluir coluna", noCards: "Sem cartões", cardTitlePlaceholder: "Digite o título do cartão…", + requiredFieldsTitle: "Preencha os campos obrigatórios", + requiredFieldsDescription: "Esta movimentação torna os campos abaixo obrigatórios. Preencha-os para continuar.", }, timeline: { bucket: { diff --git a/packages/i18n/src/locales/ru.ts b/packages/i18n/src/locales/ru.ts index 3835b56f5..9a70a5e2b 100644 --- a/packages/i18n/src/locales/ru.ts +++ b/packages/i18n/src/locales/ru.ts @@ -633,6 +633,8 @@ const ru = { deleteColumn: "Удалить колонку", noCards: "Нет карточек", cardTitlePlaceholder: "Введите название карточки…", + requiredFieldsTitle: "Заполните обязательные поля", + requiredFieldsDescription: "Это перемещение делает поля ниже обязательными. Заполните их, чтобы продолжить.", }, timeline: { bucket: { diff --git a/packages/i18n/src/locales/zh.ts b/packages/i18n/src/locales/zh.ts index 14ccc16da..6bcbf18e2 100644 --- a/packages/i18n/src/locales/zh.ts +++ b/packages/i18n/src/locales/zh.ts @@ -648,6 +648,8 @@ const zh = { cardTitlePlaceholder: '输入卡片标题…', uncategorized: '未分类', columns: '列', + requiredFieldsTitle: '填写必填字段', + requiredFieldsDescription: '此次移动会使以下字段成为必填项。填写后即可继续。', }, timeline: { bucket: { diff --git a/packages/plugin-kanban/src/ObjectKanban.requiredWhenPrompt.test.tsx b/packages/plugin-kanban/src/ObjectKanban.requiredWhenPrompt.test.tsx new file mode 100644 index 000000000..4ccf4aae0 --- /dev/null +++ b/packages/plugin-kanban/src/ObjectKanban.requiredWhenPrompt.test.tsx @@ -0,0 +1,326 @@ +/** + * 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. + */ + +/** + * Dropping a card into a column whose value makes fields REQUIRED + * (`requiredWhen`) collects them instead of dead-ending — objectui#4254. + * + * ── The card's repro, as a board ────────────────────────────────────────── + * HotCRM's opportunity pipeline declares + * + * win_reason: Field.select({ requiredWhen: P`has(record.stage) && record.stage == "closed_won"` }) + * + * and dragging a deal into **Closed Won** PATCHed `{stage}` ALONE. The engine + * refuses (`Win Reason 不能为空`) and the board had no way to finish the move: + * the only path to closing a deal was to abandon the board for the record form. + * + * The approved shape (maintainer, 2026-08-12) is pre-evaluation: BEFORE the + * PATCH, evaluate the target column's `requiredWhen` predicates against the + * record with the target value applied; if that makes fields required and they + * are empty, collect them in a small dialog and submit stage + collected + * fields in ONE PATCH. + * + * ── Direction of each case, predicted BEFORE running against unfixed code ─ + * 1. dialog appears on a triggering drop → RED pre-fix (no dialog exists) + * 2. no PATCH is issued while collecting → RED pre-fix (`{stage}` goes out + * immediately) + * 3. submit sends ONE combined PATCH → RED pre-fix (no dialog to submit) + * 4. cancel sends NO PATCH, card stays put → RED pre-fix (PATCH already went) + * 5. a NON-triggering drop is byte-identical → GREEN before and after. The pin + * that the feature is inert off + * its trigger. + * 6. a required field that already HAS a value → GREEN before and after. Empty + * is half the predicate; a filled field must not be re-asked. + * 7. a required-and-empty field with NO edit widget (`file`) falls through to + * today's PATCH → GREEN before and after. We collect only what we can + * honestly render; everything else keeps the server's legible refusal + * (objectstack#7525 is closed) rather than showing an empty dialog row. + * 8. the dialog renders the FORM's own widget for the field's type + * → RED pre-fix (no dialog) + * + * Cases 5-7 are the controls: they are the reason a red in 1-4 is the feature + * and not the harness mis-driving the board. + * + * ── Harness ─────────────────────────────────────────────────────────────── + * Same shape as `ObjectKanban.rejectedMoveRollback.test.tsx` (#4138): dnd-kit's + * pointer sensors need layout and pointer-capture jsdom does not provide, so + * the board's real `onDragEnd` is captured from a wrapping `DndContext` and + * called with a synthesized drop. Everything downstream — `KanbanImpl`'s local + * move, `ObjectKanban`'s pre-evaluation, the dialog, the PATCH — is production + * code. + * + * **No `I18nProvider` is mounted here, deliberately** (objectui#4514): + * `initReactI18next` registers its instance as react-i18next's module-global + * default and that registration outlives `cleanup()`, so one provider mount + * would silently re-point every later assertion in this file. The copy asserted + * below is therefore the English fallback path, which is also what a + * provider-less host (preview gallery, embedding app) reads. + */ + +import React from 'react'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, screen, within, act, cleanup, waitFor, fireEvent } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { registerAllFields } from '@object-ui/fields'; +import { toast } from '@object-ui/components'; +import type { DataSource } from '@object-ui/types'; +import { ObjectKanban } from './ObjectKanban'; + +// Pay the board's lazy chunk at import time rather than racing it against a +// `findBy` budget (AGENTS.md §测试纪律); specifier byte-identical to `./index`'s +// so the component's own `React.lazy` factory resolves from the ESM cache. +import './KanbanImpl'; + +const dnd = vi.hoisted(() => ({ + onDragEnd: undefined as undefined | ((event: unknown) => void), +})); + +vi.mock('@dnd-kit/core', async (importOriginal) => { + const actual = await importOriginal(); + const ReactMod = await import('react'); + const CapturingDndContext = (props: Record) => { + dnd.onDragEnd = props.onDragEnd as (event: unknown) => void; + return ReactMod.createElement(actual.DndContext, props as never); + }; + return { ...actual, DndContext: CapturingDndContext }; +}); + +registerAllFields(); + +/** The card's own predicate shape: `has()` + `==` + `&&`. */ +const WIN_REASON_REQUIRED = 'has(record.stage) && record.stage == "closed_won"'; + +const DEAL = 'Acme renewal'; + +function objectDef(winReason: Record) { + return { + name: 'opportunity', + fields: { + name: { type: 'text', label: 'Name' }, + stage: { + type: 'picklist', + label: 'Stage', + options: [ + { value: 'negotiation', label: 'Negotiation' }, + { value: 'closed_won', label: 'Closed Won' }, + ], + }, + win_reason: { label: 'Win Reason', ...winReason }, + }, + }; +} + +/** A plain text field — drivable in jsdom without a Radix portal dance. */ +const TEXT_WIN_REASON = { type: 'text', requiredWhen: WIN_REASON_REQUIRED }; + +const schema = { + type: 'object-kanban', + objectName: 'opportunity', + groupBy: 'stage', + cardTitle: 'name', + columns: [ + { id: 'negotiation', title: 'Negotiation' }, + { id: 'closed_won', title: 'Closed Won' }, + ], +} as never; + +const records = (winReason: unknown = null) => [ + { id: 'o1', name: DEAL, stage: 'negotiation', win_reason: winReason }, +]; + +function makeDataSource(def: unknown, update: DataSource['update']): DataSource { + return { + getObjectSchema: vi.fn(async () => def), + find: vi.fn(async () => ({ value: records() })), + update, + } as unknown as DataSource; +} + +function cardsIn(columnTitle: string): string[] { + const list = screen.getByRole('list', { name: `${columnTitle} cards` }); + return within(list) + .queryAllByRole('listitem') + .map((el) => el.getAttribute('aria-label') ?? ''); +} + +async function mountBoard( + dataSource: DataSource, + data = records(), + startColumn = 'Negotiation', +) { + render(); + expect(await screen.findByText(DEAL)).toBeInTheDocument(); + // Flush the object-def fetch — the pre-evaluation reads `objectDef.fields`, + // and a drop before it lands would measure the absence of metadata rather + // than the feature. + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + }); + // Settle on the column the record's own `stage` puts it in — not always the + // first one, since one case starts the card in Closed Won so it can drag the + // NON-triggering direction. + await waitFor(() => expect(cardsIn(startColumn)).toEqual([DEAL])); +} + +async function dropOn(columnId: string) { + expect(dnd.onDragEnd).toBeTypeOf('function'); + await act(async () => { + dnd.onDragEnd!({ active: { id: 'o1' }, over: { id: columnId } }); + }); +} + +beforeEach(() => { + dnd.onDragEnd = undefined; + vi.spyOn(toast, 'error').mockImplementation(() => 'toast-id' as never); +}); + +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); +}); + +describe('ObjectKanban — a drop that makes fields required collects them (#4254)', () => { + it('opens a dialog listing exactly the required-and-empty fields', async () => { + const update = vi.fn(async () => ({ id: 'o1' })); + await mountBoard(makeDataSource(objectDef(TEXT_WIN_REASON), update as never)); + + await dropOn('closed_won'); + + const dialog = await screen.findByRole('dialog'); + expect(within(dialog).getByText('Complete required fields')).toBeInTheDocument(); + expect(within(dialog).getByText('Win Reason')).toBeInTheDocument(); + // Only the field the move made required — `name` is empty of a predicate, + // `stage` is the thing being set. + expect(within(dialog).queryByText('Name')).not.toBeInTheDocument(); + expect(within(dialog).queryByText('Stage')).not.toBeInTheDocument(); + }); + + it('issues NO PATCH while the dialog is collecting', async () => { + const update = vi.fn(async () => ({ id: 'o1' })); + await mountBoard(makeDataSource(objectDef(TEXT_WIN_REASON), update as never)); + + await dropOn('closed_won'); + + // Asserted BEFORE the dialog is looked for, on purpose: this is the issue's + // actual defect, so its red must land here and not on a missing dialog. + // Pre-fix this reads `{ stage: 'closed_won' }` — the lone PATCH the engine + // refuses for the field nobody was ever asked about. + expect(update).not.toHaveBeenCalled(); + + // The whole point of pre-evaluation: the refusal never happens because the + // request that would be refused is never sent. + await screen.findByRole('dialog'); + expect(update).not.toHaveBeenCalled(); + }); + + it('submits stage + collected fields in ONE combined PATCH', async () => { + const update = vi.fn(async () => ({ id: 'o1' })); + await mountBoard(makeDataSource(objectDef(TEXT_WIN_REASON), update as never)); + + await dropOn('closed_won'); + const dialog = await screen.findByRole('dialog'); + + fireEvent.change(within(dialog).getByRole('textbox'), { + target: { value: 'Beat the incumbent on price' }, + }); + await act(async () => { + fireEvent.click(within(dialog).getByRole('button', { name: 'Move card' })); + }); + + await waitFor(() => expect(update).toHaveBeenCalledTimes(1)); + expect(update).toHaveBeenCalledWith('opportunity', 'o1', { + stage: 'closed_won', + win_reason: 'Beat the incumbent on price', + }); + }); + + it('cancel sends NO PATCH and leaves the card in its original column', async () => { + const update = vi.fn(async () => ({ id: 'o1' })); + await mountBoard(makeDataSource(objectDef(TEXT_WIN_REASON), update as never)); + + await dropOn('closed_won'); + const dialog = await screen.findByRole('dialog'); + + await act(async () => { + fireEvent.click(within(dialog).getByRole('button', { name: 'Cancel' })); + }); + + expect(update).not.toHaveBeenCalled(); + await waitFor(() => expect(cardsIn('Negotiation')).toEqual([DEAL])); + expect(cardsIn('Closed Won')).toEqual([]); + expect(screen.getAllByRole('listitem', { name: DEAL })).toHaveLength(1); + }); + + it('renders the FORM’s own widget for the field type (a select edits as a select)', async () => { + const update = vi.fn(async () => ({ id: 'o1' })); + const def = objectDef({ + type: 'select', + requiredWhen: WIN_REASON_REQUIRED, + options: [ + { value: 'price', label: 'Better price' }, + { value: 'features', label: 'Better features' }, + ], + }); + await mountBoard(makeDataSource(def, update as never)); + + await dropOn('closed_won'); + const dialog = await screen.findByRole('dialog'); + + // `SelectField`'s trigger — the same control the record form renders for a + // select, reached through `FieldEditWidget`, not a parallel mini-form. + expect(within(dialog).getByRole('combobox')).toBeInTheDocument(); + expect(within(dialog).queryByRole('textbox')).not.toBeInTheDocument(); + }); + + // ── Controls: green before AND after ──────────────────────────────────── + + it('PIN: a drop that triggers no predicate PATCHes {stage} alone, no dialog', async () => { + const update = vi.fn(async () => ({ id: 'o1' })); + await mountBoard( + makeDataSource(objectDef(TEXT_WIN_REASON), update as never), + [{ id: 'o1', name: DEAL, stage: 'closed_won', win_reason: 'Already won' }], + 'Closed Won', + ); + + await dropOn('negotiation'); + + await waitFor(() => expect(update).toHaveBeenCalledTimes(1)); + expect(update).toHaveBeenCalledWith('opportunity', 'o1', { stage: 'negotiation' }); + expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); + }); + + it('PIN: a required field that already HAS a value is not re-asked', async () => { + const update = vi.fn(async () => ({ id: 'o1' })); + await mountBoard( + makeDataSource(objectDef(TEXT_WIN_REASON), update as never), + records('Existing reason'), + ); + + await dropOn('closed_won'); + + await waitFor(() => expect(update).toHaveBeenCalledTimes(1)); + expect(update).toHaveBeenCalledWith('opportunity', 'o1', { stage: 'closed_won' }); + expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); + }); + + it('PIN: a required field with no edit widget falls through to today’s PATCH', async () => { + const update = vi.fn(async () => ({ id: 'o1' })); + // `file` is in `INLINE_EXCLUDED_FIELD_TYPES` — there is no honest control to + // put in the dialog, so the move keeps the old behaviour and the server's + // (now legible) refusal is what the user sees. + const def = objectDef({ type: 'file', requiredWhen: WIN_REASON_REQUIRED }); + await mountBoard(makeDataSource(def, update as never)); + + await dropOn('closed_won'); + + await waitFor(() => expect(update).toHaveBeenCalledTimes(1)); + expect(update).toHaveBeenCalledWith('opportunity', 'o1', { stage: 'closed_won' }); + expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); + }); +}); diff --git a/packages/plugin-kanban/src/ObjectKanban.tsx b/packages/plugin-kanban/src/ObjectKanban.tsx index 8177d3653..4ea1c7c7d 100644 --- a/packages/plugin-kanban/src/ObjectKanban.tsx +++ b/packages/plugin-kanban/src/ObjectKanban.tsx @@ -23,6 +23,11 @@ import { extractRecords, buildExpandFields, getRecordDisplayName } from '@object import { getBadgeColorClasses, getCellRenderer, resolveCellRendererType } from '@object-ui/fields'; import { KanbanRenderer, KANBAN_UNCOLUMNED_ID } from './index'; import { KanbanSchema } from './types'; +import { + collectRequiredWhenPromptFields, + type RequiredWhenPromptField, +} from './requiredWhenPrompt'; +import { RequiredFieldsDialog } from './RequiredFieldsDialog'; /** * English fallbacks for the record-detail drawer heading this board opens on @@ -566,22 +571,22 @@ export const ObjectKanban: React.FC = ({ // card stays in the target column even after KanbanImpl's reset effect // re-syncs from props; the backend update reconciles asynchronously and // is reverted with a warning if it fails. - const handleCardMove = React.useCallback( + // + // `extraValues` carries the fields a `requiredWhen` prompt collected + // (objectui#4254), so the column value and everything the move makes + // required go out as ONE PATCH — two writes would leave the record in the + // state the engine refuses if the second one failed. With no prompt the + // spread is empty and the body is exactly what it has always been. + const persistCardMove = React.useCallback( async ( cardId: string, fromColumnId: string, toColumnId: string, - _newIndex: number, + extraValues?: Record, ) => { - void _newIndex; const groupBy = schema.groupBy; const objectName = schema.objectName; - if (!groupBy || fromColumnId === toColumnId) return; - // #2792: the "Uncategorized" lane is a display bucket, not a real option. - // Dragging a card OUT of it into a real column repairs the record's - // status (handled below); dropping one IN would write the sentinel id as - // a bogus status, so refuse to persist that direction. - if (toColumnId === KANBAN_UNCOLUMNED_ID) return; + if (!groupBy) return; // Optimistic local update so the card visibly stays in the new column. // Skipped when data is owned by a parent (ListView): `fetchedData` is not @@ -604,7 +609,19 @@ export const ObjectKanban: React.FC = ({ try { await dataSource.update(objectName, String(cardId), { [groupBy]: toColumnId, + ...(extraValues ?? {}), }); + // Land the prompt-collected values locally too, so a card that shows + // one of them as a `cardFields` cell reflects what was just written + // instead of waiting for a refetch. Only ever runs on the prompt path + // — a normal move takes no extra keys and so takes no extra state. + if (extraValues && !hasExternalData) { + setFetchedData((prev) => + prev.map((r) => + String(r.id ?? r._id) === String(cardId) ? { ...r, ...extraValues } : r, + ), + ); + } } catch (err) { console.warn('[ObjectKanban] Failed to persist card move', err); // Surface the failure — never silently snap the card back. A row-level @@ -651,6 +668,84 @@ export const ObjectKanban: React.FC = ({ [schema.groupBy, schema.objectName, dataSource, hasExternalData, tt], ); + /** + * The drop that is waiting on required fields (objectui#4254). Non-null only + * while the collect dialog is open; the move has NOT been PATCHed yet. + */ + const [pendingMove, setPendingMove] = useState<{ + cardId: string; + fromColumnId: string; + toColumnId: string; + fields: RequiredWhenPromptField[]; + } | null>(null); + const [pendingSubmitting, setPendingSubmitting] = useState(false); + + // Pre-evaluate the target column's `requiredWhen` predicates and collect what + // the move makes required BEFORE writing anything (objectui#4254). The board + // used to PATCH the column value alone into a refusal the user could neither + // read nor act on; with no prompted field this is inert and the move takes + // the unchanged path below. + const handleCardMove = React.useCallback( + async ( + cardId: string, + fromColumnId: string, + toColumnId: string, + _newIndex: number, + ) => { + void _newIndex; + const groupBy = schema.groupBy; + if (!groupBy || fromColumnId === toColumnId) return; + // #2792: the "Uncategorized" lane is a display bucket, not a real option. + // Dragging a card OUT of it into a real column repairs the record's + // status (handled below); dropping one IN would write the sentinel id as + // a bogus status, so refuse to persist that direction. + if (toColumnId === KANBAN_UNCOLUMNED_ID) return; + + // The record as stored, not the card-shaped projection: `effectiveData` + // overlays a derived `title`/`badges`, and the predicates must see the + // record's own field values. + const record = (Array.isArray(rawData) ? rawData : []).find( + (r) => String(r?.id ?? r?._id) === String(cardId), + ); + const fields = collectRequiredWhenPromptFields( + objectDef?.fields, + record, + groupBy, + toColumnId, + ); + if (fields.length > 0) { + // No optimistic write and no PATCH: the card sits in its source column + // behind the modal until the user commits, so Cancel needs no rollback + // and a refusal that never happens cannot need one either. + setPendingMove({ cardId, fromColumnId, toColumnId, fields }); + return; + } + + await persistCardMove(cardId, fromColumnId, toColumnId); + }, + [schema.groupBy, rawData, objectDef, persistCardMove], + ); + + // Label + localized options for each prompted field, resolved with the same + // helpers the cards use so the dialog names a field exactly as the board does. + const pendingFields = useMemo(() => { + if (!pendingMove) return []; + const objectKey = objectDef?.name || schema.objectName || ''; + return pendingMove.fields.map((f) => { + const options = f.def.options; + const localized = + objectKey && Array.isArray(options) + ? translateOptions(objectKey, f.name, options) + : options; + const declaredLabel = typeof f.def.label === 'string' ? f.def.label : ''; + return { + ...f, + def: { ...f.def, ...(localized ? { options: localized } : {}) }, + label: fieldLabel(objectKey, f.name, declaredLabel || f.name), + }; + }); + }, [pendingMove, objectDef, schema.objectName, translateOptions, fieldLabel]); + // Error branch renders only after every hook above has run, so hook order // stays stable across renders (no early return before the hooks). if (error) { @@ -677,6 +772,34 @@ export const ObjectKanban: React.FC = ({ }, onCardMove: handleCardMove, }} /> + {pendingMove && ( + setPendingMove(null)} + onSubmit={async (values) => { + const move = pendingMove; + setPendingSubmitting(true); + try { + // ONE combined PATCH: the column value and everything the move + // made required. `persistCardMove` owns the optimistic write, + // the toast and the #4138 rollback, so a refusal of the combined + // body behaves exactly like a refusal of a plain move — and the + // dialog closes rather than looping on an arbitrary server error. + await persistCardMove( + move.cardId, + move.fromColumnId, + move.toColumnId, + values, + ); + } finally { + setPendingSubmitting(false); + setPendingMove(null); + } + }} + /> + )} {navigation.isOverlay && navigation.isOpen && navigation.selectedRecord && (() => { const objectName = schema.objectName; const rec = navigation.selectedRecord as Record; diff --git a/packages/plugin-kanban/src/RequiredFieldsDialog.tsx b/packages/plugin-kanban/src/RequiredFieldsDialog.tsx new file mode 100644 index 000000000..f8e68fbdd --- /dev/null +++ b/packages/plugin-kanban/src/RequiredFieldsDialog.tsx @@ -0,0 +1,154 @@ +/** + * 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. + */ + +/** + * The small collect-then-move dialog a kanban drop opens when the target + * column makes fields required — objectui#4254. + * + * ## Not a mini-form + * + * Every control here is `@object-ui/fields`' {@link FieldEditWidget}, i.e. the + * SAME widget the record form renders for that field type (its own header: + * "the SAME dedicated widgets the form renders"), which is also what the data + * grid's inline editor and the detail page's inline edit compose. So a select + * edits as a select, a date as a date picker, a boolean as a checkbox — + * without this package owning a single input implementation, and without a + * second set of field-rendering decisions that could drift from the form's. + * + * The validation is the same one definition too: `isMissingForRequired` from + * `@object-ui/core`, the presence contract the form and the server share, so + * this dialog cannot decide a value is "empty" in a way the server would + * disagree with (a `false` boolean and a `0` are values, not absences). + * + * ## Copy + * + * Through `useSafeTranslate` — the per-call `tt(key, fallback)` channel this + * package already uses — so a host with no `I18nProvider` mounted (a + * standalone board, this package's tests, the preview gallery) reads English + * instead of a raw key. The two new keys live in the `kanban` namespace of all + * ten locale packs; `common.cancel` and `kanban.moveCard` are reused rather + * than minted again, since one control's label should not get several + * translations that can drift apart. + */ + +import React from 'react'; +import { + Button, + Dialog, + DialogContent, + DialogDescription, + DialogFooter, + DialogHeader, + DialogTitle, +} from '@object-ui/components'; +import { useSafeTranslate } from '@object-ui/react'; +import { isMissingForRequired } from '@object-ui/core'; +import { FieldEditWidget } from '@object-ui/fields'; +import type { RequiredWhenPromptField } from './requiredWhenPrompt'; + +/** A prompt field with its display label already resolved by the board. */ +export interface RequiredFieldsDialogField extends RequiredWhenPromptField { + label: string; +} + +export interface RequiredFieldsDialogProps { + open: boolean; + /** The required-and-empty fields, in the object's declared order. */ + fields: RequiredFieldsDialogField[]; + /** Disables the controls while the combined PATCH is in flight. */ + submitting?: boolean; + onCancel: () => void; + onSubmit: (values: Record) => void; +} + +export function RequiredFieldsDialog({ + open, + fields, + submitting, + onCancel, + onSubmit, +}: RequiredFieldsDialogProps): React.ReactElement { + const tt = useSafeTranslate(); + // Each drop is its own collection, and that is enforced by MOUNTING rather + // than by a reset effect: the board renders this component only while a move + // is pending, so cancelling or submitting unmounts it and the next drop + // starts from empty state. A `useEffect` re-seed would be both redundant and + // a cascading-render hazard the lint rule correctly flags. + const [values, setValues] = React.useState>({}); + // True only after a submit attempt, so the dialog opens clean rather than + // shouting "Required" at fields the user has not reached yet. + const [showErrors, setShowErrors] = React.useState(false); + + const missing = fields.filter((f) => isMissingForRequired(values[f.name])); + + const handleSubmit = () => { + if (missing.length > 0) { + setShowErrors(true); + return; + } + onSubmit(values); + }; + + return ( + { if (!next) onCancel(); }}> + + + + {tt('kanban.requiredFieldsTitle', 'Complete required fields')} + + + {tt( + 'kanban.requiredFieldsDescription', + 'This move makes the fields below required. Fill them in to continue.', + )} + + + +
+ {fields.map((f) => { + const isMissing = isMissingForRequired(values[f.name]); + return ( + // A wrapping `label` gives the control its accessible name + // implicitly — `FieldEditWidget` renders the widget itself and + // takes no `id` to associate with, and widening its contract + // belongs to `@object-ui/fields`, not to a caller. + + ); + })} +
+ + + + + + + + ); +} diff --git a/packages/plugin-kanban/src/requiredWhenPrompt.test.ts b/packages/plugin-kanban/src/requiredWhenPrompt.test.ts new file mode 100644 index 000000000..64a4760b2 --- /dev/null +++ b/packages/plugin-kanban/src/requiredWhenPrompt.test.ts @@ -0,0 +1,174 @@ +/** + * 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. + */ + +/** + * The pre-evaluation half of objectui#4254, pinned directly. + * + * The board-level test (`ObjectKanban.requiredWhenPrompt.test.tsx`) drives the + * whole flow through a real drop; this file pins the SET the flow is built on, + * because several of its exclusions are decisions rather than mechanics and two + * of them (`visibleWhen` FALSE, `readonlyWhen` TRUE) cannot be told apart from + * "no dialog appeared" through the DOM. + * + * Every case here is GREEN only with the module present — the module is the + * change — so the direction that matters is the reverse verification recorded + * in the PR: deleting the `requiredWhen` limb turns the board test red. + */ + +import { describe, it, expect } from 'vitest'; +import { collectRequiredWhenPromptFields } from './requiredWhenPrompt'; + +/** The card's own predicate: `has()` + `==` + `&&`. */ +const WHEN_WON = 'has(record.stage) && record.stage == "closed_won"'; + +const record = { id: 'o1', stage: 'negotiation', win_reason: null }; + +function fields(winReason: Record) { + return { + stage: { type: 'picklist', label: 'Stage' }, + win_reason: { type: 'text', label: 'Win Reason', ...winReason }, + }; +} + +describe('collectRequiredWhenPromptFields (#4254)', () => { + it('collects a field the target value makes required while empty', () => { + const out = collectRequiredWhenPromptFields( + fields({ requiredWhen: WHEN_WON }), + record, + 'stage', + 'closed_won', + ); + expect(out.map((f) => f.name)).toEqual(['win_reason']); + // The definition travels with it so the dialog can render the right widget. + expect(out[0].def).toMatchObject({ name: 'win_reason', type: 'text' }); + }); + + it('collects nothing when the target value leaves the predicate FALSE', () => { + expect( + collectRequiredWhenPromptFields( + fields({ requiredWhen: WHEN_WON }), + { ...record, stage: 'closed_won' }, + 'stage', + 'negotiation', + ), + ).toEqual([]); + }); + + it('skips a required field that already has a value', () => { + expect( + collectRequiredWhenPromptFields( + fields({ requiredWhen: WHEN_WON }), + { ...record, win_reason: 'Better price' }, + 'stage', + 'closed_won', + ), + ).toEqual([]); + }); + + it('treats false and 0 as VALUES, not absences', () => { + // The presence contract core/objectql share — a required checkbox whose + // answer is "no" is answered, and zero hours logged is data. + for (const value of [false, 0]) { + expect( + collectRequiredWhenPromptFields( + { stage: {}, flag: { type: 'boolean', requiredWhen: WHEN_WON } }, + { stage: 'negotiation', flag: value }, + 'stage', + 'closed_won', + ), + ).toEqual([]); + } + }); + + it('skips a field the record form would not show (visibleWhen FALSE)', () => { + expect( + collectRequiredWhenPromptFields( + fields({ requiredWhen: WHEN_WON, visibleWhen: 'false' }), + record, + 'stage', + 'closed_won', + ), + ).toEqual([]); + }); + + it('skips a readonly field — there is nothing the user could author', () => { + expect( + collectRequiredWhenPromptFields( + fields({ requiredWhen: WHEN_WON, readonlyWhen: 'true' }), + record, + 'stage', + 'closed_won', + ), + ).toEqual([]); + }); + + it('skips a type with no edit widget, leaving the server refusal to speak', () => { + expect( + collectRequiredWhenPromptFields( + fields({ type: 'file', requiredWhen: WHEN_WON }), + record, + 'stage', + 'closed_won', + ), + ).toEqual([]); + }); + + it('never prompts for the groupBy field itself — the drop IS its value', () => { + expect( + collectRequiredWhenPromptFields( + { stage: { type: 'picklist', requiredWhen: WHEN_WON } }, + { stage: null }, + 'stage', + 'closed_won', + ), + ).toEqual([]); + }); + + it('ignores fields with no requiredWhen predicate at all', () => { + expect( + collectRequiredWhenPromptFields( + { stage: {}, note: { type: 'text', required: true } }, + { stage: 'negotiation', note: null }, + 'stage', + 'closed_won', + ), + ).toEqual([]); + }); + + it('returns [] rather than throwing on absent metadata or record', () => { + expect(collectRequiredWhenPromptFields(undefined, record, 'stage', 'x')).toEqual([]); + expect(collectRequiredWhenPromptFields(fields({}), undefined, 'stage', 'x')).toEqual([]); + }); + + it('keeps the object’s declared field order', () => { + const out = collectRequiredWhenPromptFields( + { + stage: {}, + b_field: { type: 'text', requiredWhen: WHEN_WON }, + a_field: { type: 'text', requiredWhen: WHEN_WON }, + }, + { stage: 'negotiation', a_field: null, b_field: null }, + 'stage', + 'closed_won', + ); + expect(out.map((f) => f.name)).toEqual(['b_field', 'a_field']); + }); + + it('fails OPEN on a broken predicate — a typo must not block the move', () => { + // Same direction as the record form: an unevaluable predicate resolves to + // the safe default (not required), so a metadata typo cannot wedge a board. + expect( + collectRequiredWhenPromptFields( + fields({ requiredWhen: 'this is not ( valid CEL' }), + record, + 'stage', + 'closed_won', + ), + ).toEqual([]); + }); +}); diff --git a/packages/plugin-kanban/src/requiredWhenPrompt.ts b/packages/plugin-kanban/src/requiredWhenPrompt.ts new file mode 100644 index 000000000..9ba675654 --- /dev/null +++ b/packages/plugin-kanban/src/requiredWhenPrompt.ts @@ -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. + */ + +/** + * Which fields a kanban drop is about to make REQUIRED and empty — the + * pre-evaluation half of objectui#4254. + * + * Dropping a card into a column writes one value (`groupBy` := the column id), + * and that write can flip a field's `requiredWhen` predicate to TRUE. The board + * used to PATCH the column value alone, the engine refused the whole update + * (`Win Reason 不能为空`), and there was no way to finish the move from the + * board at all. This module answers the question that makes the move + * completable instead: *given the record and the value the drop is about to + * write, which fields become required while still being empty?* + * + * ## One evaluator, not two + * + * The verdict comes from `@object-ui/core`'s {@link resolveFieldRuleState} — + * the SAME function the record form (`components/renderers/form/form.tsx`), + * the wizard (`plugin-form/WizardForm.tsx`) and the line-item grid + * (`fields/widgets/GridField.tsx`) resolve `visibleWhen`/`readonlyWhen`/ + * `requiredWhen` with, which in turn delegates to `@objectstack/formula`'s + * `ExpressionEngine`. That is deliberate and load-bearing: the board's prompt + * and the server's enforcement must reach the identical verdict, and a second + * hand-rolled predicate evaluator is exactly how the two ends drift. The + * card's own predicate shape — `has(record.stage) && record.stage == + * "closed_won"` — was measured against that engine before this was written. + * + * ## Why "required" alone is not the filter + * + * A field is only offered when the dialog can honestly render a control for + * it, so four further conditions narrow the set. Each exclusion is a case + * where prompting would be worse than not prompting: + * + * - **already has a value** — the move is not what is missing. `required` is a + * PRESENCE contract, so the emptiness test is core's `isMissingForRequired` + * (the one the form and the server agree on: `false` and `0` are values, + * an empty array is not) rather than a truthiness check that would re-ask + * for every unchecked box. + * - **not visible** (`visibleWhen` FALSE) — the record form would not show it, + * so neither does this. + * - **readonly** — there is nothing the user could author. + * - **no edit widget** (`file`, `richtext`, `formula`, … — the types + * `@object-ui/fields` deliberately excludes from in-place editing) — a + * dialog row with no control is a worse dead end than the one being fixed. + * + * The last three fall through to the unchanged PATCH, where the server's + * refusal is now legible (objectstack#7525 is closed) — a legible refusal + * beats a dialog that cannot collect anything. + * + * The `groupBy` field itself is skipped: the drop IS its value. + */ +import { resolveFieldRuleState, isMissingForRequired } from '@object-ui/core'; +import { hasFieldEditWidget } from '@object-ui/fields'; + +/** A field the drop makes required, paired with the definition to render it. */ +export interface RequiredWhenPromptField { + /** Field name — the key the combined PATCH will carry. */ + name: string; + /** The field definition, with `name` guaranteed present for the widget. */ + def: Record & { type?: string }; +} + +/** + * The fields a move to `toColumnId` would make required and that are still + * empty, in the object's declared field order. + * + * Returns `[]` whenever the drop needs no prompt — the caller's signal to keep + * today's single-key PATCH exactly as it was. + * + * @param objectFields `objectDef.fields`, as fetched by the board. + * @param record The record as it stands BEFORE the drop. + * @param groupBy The field the board groups by (the one the drop writes). + * @param toColumnId The value the drop is about to write. + */ +export function collectRequiredWhenPromptFields( + objectFields: Record | null | undefined, + record: Record | null | undefined, + groupBy: string, + toColumnId: unknown, +): RequiredWhenPromptField[] { + if (!objectFields || !record) return []; + + // The record as the server would see it after the drop — this is what the + // predicates are judged against, and the whole reason the prompt can happen + // BEFORE the write rather than in reaction to a refusal. + const next = { ...record, [groupBy]: toColumnId }; + + const prompts: RequiredWhenPromptField[] = []; + for (const [name, raw] of Object.entries(objectFields)) { + if (name === groupBy) continue; + if (!raw || typeof raw !== 'object') continue; + const def = raw as Record; + + // Scoped to conditional requirements on purpose. A statically-required + // empty field means the record was already invalid before anyone touched + // the board; turning a drag into a general-purpose validation form is a + // different feature from the one that was approved. + if (def.requiredWhen == null) continue; + if (!isMissingForRequired(record[name])) continue; + + const state = resolveFieldRuleState( + { + visibleWhen: def.visibleWhen as never, + readonlyWhen: def.readonlyWhen as never, + requiredWhen: def.requiredWhen as never, + }, + next, + { required: def.required === true, readonly: def.readonly === true }, + // The pre-drop record, so a predicate reading `previous.*` sees what the + // form would show it. + record, + undefined, + `field '${name}'`, + ); + + if (!state.required || !state.visible || state.readonly) continue; + if (!hasFieldEditWidget(def.type as string | undefined)) continue; + + prompts.push({ name, def: { name, ...def } }); + } + return prompts; +}