diff --git a/.changeset/filter-builder-field-switch-retypes-value.md b/.changeset/filter-builder-field-switch-retypes-value.md new file mode 100644 index 0000000000..4f31f3f964 --- /dev/null +++ b/.changeset/filter-builder-field-switch-retypes-value.md @@ -0,0 +1,38 @@ +--- +'@object-ui/components': patch +--- + +`FilterBuilder` settles a row's **value** when its field changes, instead of leaving a value the new column's input cannot show. + +objectui#4768 / PR #4779 settled the row's operator on a field switch and +re-shaped the value only when the operator's family changed — scalar to scalar +has no shape question, so what the user typed was carried through on purpose. +But the field's **type** changed too, and the value input is redrawn from it: a +browser renders a non-numeric value in `` as **blank**. A +`text` row filtered `equals "acme"`, pointed at a number column, showed an empty +box while the row went on carrying `"acme"` — `foldFilterGroupToSpecRules` +persisted it and the live grid queried `amount equals "acme"`. The same +invisible-value shape as objectui#4768, one column over. + +Changing the field is now one edit with the operator, the value's shape **and** +the value's type. Convertible values are carried, the rest clear to the family's +empty shape (scalar `''`, list `[]`, range `[]`): + +- `"42"` on a number column becomes the number `42`; `"acme"`, `"42abc"` and + `"1,000"` clear. The reading is deliberately stricter than `parseFloat`, which + would turn `"acme"` into `0` — a filter the user never wrote; +- `"true"` / `"false"` convert on a boolean column, and a boolean becomes + `"true"` / `"false"` on a text column, so the round trip closes; `1` and + `"yes"` are conventions rather than readings, and clear; +- date-like columns take only what their own input can render, plus the one + truncation that loses nothing it could have shown (`"2024-03-05T14:30"` → + `"2024-03-05"` on a date column). A bare date does **not** gain a midnight to + fit a `datetime` column: `equals 2024-03-05T00:00` is a filter that looks + answered and matches almost nothing; +- a value the new column can already hold is left alone — switching between two + text columns, or two numeric ones, still keeps what the user typed, and an + unfilled row stays unfilled. + +The convertibility judgement is defined once, next to `reshapeFilterValue`, +and `getInputType` now reads the same family table it does — so the type a value +is converted **to** and the input it is edited **in** cannot drift apart. diff --git a/packages/components/src/__tests__/filter-builder-field-switch-operator.test.tsx b/packages/components/src/__tests__/filter-builder-field-switch-operator.test.tsx index 7c8c50381a..d5769e81e6 100644 --- a/packages/components/src/__tests__/filter-builder-field-switch-operator.test.tsx +++ b/packages/components/src/__tests__/filter-builder-field-switch-operator.test.tsx @@ -193,17 +193,32 @@ describe('the value is re-shaped for the family the operator lands in', () => { operator: 'between', value: ['2024-01-01', '2024-12-31'], }); - await pick(0, 'Amount'); + // Retargeted from `Amount` to `Title` by objectui#4781: the fact under test + // is the pair → scalar collapse, and a text column forces the same operator + // reset (its bucket has no `between`) while being able to HOLD the date + // string the collapse produces. On a number column the collapse still + // happens and #4781 then clears `"2024-01-01"`, which would have hidden + // this pin behind a value judgement that is not what it is here to state. + await pick(0, 'Title'); expect(lastRow(onChange).value).toBe('2024-01-01'); }); - it('a scalar value is carried through untouched — only the SHAPE is settled', async () => { - // Deliberately not blanked: the reset answers the operator's family, and - // scalar-to-scalar has no shape question to answer. What the user typed is - // theirs; a field switch is not a licence to discard it. + it('a scalar the new column can hold is carried through — only the SHAPE is settled', async () => { + // The reset answers the operator's family, and scalar-to-scalar has no + // shape question to answer. What the user typed is theirs; a field switch + // is not a licence to discard it. + // + // This case used to point at `Amount` and assert that `"acme"` survived + // onto a NUMBER column. That is the defect objectui#4781 reported — the + // number input renders `"acme"` blank while the row keeps filtering by it — + // and the maintainer ruled the value must clear there. The pin's own + // subject (the shape reset does not blank a value) is unchanged, so it is + // asserted on a target that can still hold the value; the number target now + // has its own, opposite pin in + // `filter-builder-field-switch-value.test.tsx`. const { onChange } = renderRow({ field: 'title', operator: 'contains', value: 'acme' }); - await pick(0, 'Amount'); + await pick(0, 'Stage'); expect(lastRow(onChange)).toMatchObject({ operator: 'equals', value: 'acme' }); }); diff --git a/packages/components/src/__tests__/filter-builder-field-switch-value.test.tsx b/packages/components/src/__tests__/filter-builder-field-switch-value.test.tsx new file mode 100644 index 0000000000..c57ecc561c --- /dev/null +++ b/packages/components/src/__tests__/filter-builder-field-switch-value.test.tsx @@ -0,0 +1,556 @@ +/** + * 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. + */ + +/** + * Changing a row's FIELD must leave the row's value inside what the new field's + * type can hold (objectui#4781). + * + * PR #4779 (objectui#4768) settled the row's OPERATOR on a field switch, and + * re-shaped the value only when the operator's family changed — scalar to + * scalar has no shape question, so `"acme"` was carried through deliberately. + * But the field's TYPE changed too, and the value input is redrawn from it: a + * browser renders a non-numeric value in `` as BLANK. So a + * `text` row filtered `equals "acme"`, pointed at a number column, showed an + * empty box while the row went on carrying `"acme"` — + * `foldFilterGroupToSpecRules` persisted it and the live grid queried + * `amount equals "acme"`. The same invisible-value shape as objectui#4768, one + * column over. + * + * Ruled on the card (2026-08-16, option B): convert when convertible, clear + * otherwise. `"42"` on a number column becomes `42`; `"acme"` clears to the + * family's empty shape (scalar `''`, list `[]`). + * + * DIRECTION, predicted before running: + * + * - every pin under "cleared when the new column cannot hold it" and + * "converted when the new column can" is RED on `origin/main`, where the + * scalar is carried through verbatim — `"acme"` stays `"acme"`, `"42"` + * stays the STRING `"42"`; + * - "a value the new column CAN already hold is left alone" is GREEN in both + * directions by design. It is the blast-radius guard: a fix that cleared on + * every field switch — option A, the one the maintainer ruled against — + * turns all of it red; + * - the `retypeFilterValue` matrix does not COMPILE on `main` (the helper + * does not exist there): those are pins on the new helper's contract, the + * same status PR #4779's `reconcileOperatorForField` block had; + * - the input-type derivation block is GREEN in both directions too. It + * guards a refactor that must not change behaviour: `getInputType` now + * reads the same family table the conversion does, so the type a value is + * converted TO and the input it is edited IN cannot drift apart. + */ +import { describe, it, expect, vi } from 'vitest'; +import React from 'react'; +import { render, screen, fireEvent, waitFor } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { FilterBuilder, retypeFilterValue } from '../custom/filter-builder'; + +/** + * One column per value family the builder can draw — and a SECOND text, numeric + * and date column, because "the value survives a switch WITHIN one family" is + * this fix's blast-radius guard and needs two columns of a family to be + * expressible at all. (A Select fires nothing when re-picking the value it + * already holds, so a same-column "switch" would assert on an event that never + * happened.) + */ +const FIELDS = [ + { value: 'title', label: 'Title', type: 'text' }, + { value: 'owner_note', label: 'Owner note', type: 'text' }, + { value: 'amount', label: 'Amount', type: 'number' }, + { value: 'quota', label: 'Quota', type: 'currency' }, + { value: 'closed_at', label: 'Closed at', type: 'date' }, + { value: 'due_on', label: 'Due on', type: 'date' }, + { value: 'created_at', label: 'Created at', type: 'datetime' }, + { value: 'call_at', label: 'Call at', type: 'time' }, + { value: 'is_won', label: 'Won', type: 'boolean' }, + { value: 'stage', label: 'Stage', type: 'select' }, +]; + +function renderRow(condition: Record) { + const onChange = vi.fn(); + // Hoisted OUT of the JSX, as in PR #4762's and PR #4779's suites: the sync + // effect re-seeds internal state whenever the `value` PROP's identity + // changes, which would undo the interaction under test on the next render. + const value = { id: 'root', logic: 'and', conditions: [{ id: 'c1', ...condition }] }; + const utils = render( + , + ); + return { ...utils, onChange }; +} + +/** The row the component handed back on its most recent `onChange`. */ +function lastRow(onChange: ReturnType) { + const calls = onChange.mock.calls; + expect(calls.length, 'the builder never called onChange').toBeGreaterThan(0); + return calls[calls.length - 1][0].conditions[0]; +} + +/** Drive the REAL field dropdown — index 0 of the row's comboboxes. */ +async function pickField(label: string) { + const triggers = screen.getAllByRole('combobox'); + fireEvent.keyDown(triggers[0], { key: 'ArrowDown' }); + const option = await waitFor(() => { + const found = screen.getAllByRole('option').find((o) => o.textContent === label); + expect(found, `no "${label}" option in the field dropdown`).toBeTruthy(); + return found!; + }); + fireEvent.click(option); +} + +/** What the single value input DISPLAYS — blank is the symptom this card is about. */ +function valueInput(): HTMLInputElement { + const input = document.querySelector('input[placeholder="Value"]'); + expect(input, 'the row drew no single-value input').toBeTruthy(); + return input as HTMLInputElement; +} + +/** + * Whether a browser's `` would SHOW `value` as the row stores it, + * rather than blanking it (the value sanitisation algorithm) or showing only + * part of it. + * + * The rule is asserted directly instead of by reading `input.value`, and that + * is not a shortcut — it is what this environment can honestly observe. jsdom + * blanks a non-numeric value on an input CREATED as `type="number"` (pinned + * just below), but it does not re-run the sanitisation when an existing input's + * `type` is flipped in place, which is exactly what a field switch does: after + * the switch it hands back the pre-switch string a browser would have cleared. + * Measured, not assumed — with this card's fix disconnected, an + * `expect(input.value).toBe(String(row.value))` written over the switch stayed + * GREEN while the row carried `"acme"` and a browser showed an empty box. A pin + * that cannot fail for the reason it was written is worse than no pin, so the + * criterion it was reaching for is stated here instead. + * + * `date` is included because the disagreement need not be a blank box: the + * builder formats a timestamp for a date input by cutting it at the `T`, so the + * box would show `2024-03-05` while the row stored `2024-03-05T14:30` — shown + * and stored still differ, which is the same defect one notch quieter. + */ +function inputShowsExactly(inputType: string, value: unknown): boolean { + const shown = String(value); + if (shown === '') return true; + switch (inputType) { + case 'number': + return shown.trim() !== '' && Number.isFinite(Number(shown)); + case 'date': + return /^\d{4}-\d{2}-\d{2}$/.test(shown); + case 'datetime-local': + return /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}(:\d{2})?$/.test(shown); + case 'time': + return /^\d{2}:\d{2}(:\d{2})?$/.test(shown); + default: + return true; + } +} + +describe('the reported repro: a text value pointed at a number column', () => { + it('clears the value the number input cannot show, instead of hiding it in the row', async () => { + const { onChange } = renderRow({ field: 'title', operator: 'equals', value: 'acme' }); + await pickField('Amount'); + + const row = lastRow(onChange); + expect(row.field).toBe('amount'); + // The row used to keep `"acme"` here — persisted by the fold, queried by + // the grid as `amount equals "acme"`, and invisible in the panel. + expect(row.value).toBe(''); + }); + + it('redraws the row with the number input, and shows nothing in it', async () => { + const { onChange } = renderRow({ field: 'title', operator: 'equals', value: 'acme' }); + await pickField('Amount'); + + const input = valueInput(); + expect(input.type).toBe('number'); + // Empty box, empty row — after the fix the two agree on nothing being + // there, which is the honest state. Before it, the box was equally empty + // and the row was not. + expect(input.value).toBe(''); + expect(lastRow(onChange).value).toBe(''); + }); +}); + +describe('what the row stores is what its input can show', () => { + it('a number column shows NOTHING for a non-numeric value — the symptom itself', () => { + // Green in both directions on purpose: it states the browser behaviour the + // card is about, not this fix. It is here because every clearing rule below + // rests on it, and it fails the day the value input stops being an + // `` — at which point the clearing rules need + // rethinking rather than silently guarding nothing. + const value = { + id: 'root', + logic: 'and', + conditions: [{ id: 'c1', field: 'amount', operator: 'equals', value: 'acme' }], + }; + render( + {}} + />, + ); + + expect(valueInput().type).toBe('number'); + expect(valueInput().value).toBe(''); + }); + + const SWITCHES: Array<[string, Record, string]> = [ + ['a word onto a number column', { field: 'title', value: 'acme' }, 'Amount'], + ['a word onto a date column', { field: 'title', value: 'acme' }, 'Closed at'], + ['a date onto a datetime column', { field: 'closed_at', value: '2024-03-05' }, 'Created at'], + [ + 'a timestamp onto a date column', + { field: 'created_at', value: '2024-03-05T14:30' }, + 'Closed at', + ], + // Already renderable before the switch, so it is the control: it stays + // green whatever `changeField` does, and is here to show the invariant is + // not passing merely because everything was cleared. + ['a numeric string onto a number column', { field: 'title', value: '42' }, 'Amount'], + ]; + + it.each(SWITCHES)('after moving %s, the row holds only what the input shows', async ( + _name, + condition, + target, + ) => { + const { onChange } = renderRow({ operator: 'equals', ...condition }); + await pickField(target); + + const row = lastRow(onChange); + expect( + inputShowsExactly(valueInput().type, row.value), + `${valueInput().type} input cannot show ${JSON.stringify(row.value)}`, + ).toBe(true); + }); +}); + +describe('converted when the new column can read it', () => { + it('a numeric string becomes a NUMBER on a number column', async () => { + const { onChange } = renderRow({ field: 'title', operator: 'equals', value: '42' }); + await pickField('Amount'); + + const row = lastRow(onChange); + expect(row.value).toBe(42); + // The type matters as much as the digits: the string survived on `main`, + // and a stored `"42"` is a different rule from a stored `42` to every + // consumer that compares strictly. + expect(typeof row.value).toBe('number'); + expect(valueInput().value).toBe('42'); + }); + + it('a number becomes its STRING on a text column', async () => { + const { onChange } = renderRow({ field: 'amount', operator: 'equals', value: 42 }); + await pickField('Title'); + + expect(lastRow(onChange).value).toBe('42'); + }); + + it('“true” becomes the BOOLEAN on a boolean column, and the picker shows it', async () => { + const { onChange } = renderRow({ field: 'title', operator: 'equals', value: 'true' }); + await pickField('Won'); + + expect(lastRow(onChange).value).toBe(true); + // Index 2 is the value control; a boolean column draws a two-item Select, + // which — like the operator trigger in objectui#4768 — renders BLANK for a + // value none of its items carry. + expect(screen.getAllByRole('combobox')[2].textContent).toBe('True'); + }); + + it('a boolean becomes “true” on a text column — the round trip closes', async () => { + const { onChange } = renderRow({ field: 'is_won', operator: 'equals', value: true }); + await pickField('Title'); + + expect(lastRow(onChange).value).toBe('true'); + }); + + it('a zoneless timestamp keeps its DATE on a date column', async () => { + const { onChange } = renderRow({ + field: 'created_at', + operator: 'equals', + value: '2024-03-05T14:30', + }); + await pickField('Closed at'); + + // The one truncation that loses nothing a `` could have + // shown: it already displayed `2024-03-05` while the row carried the time, + // so this is the same disagreement closed from the value's side. + expect(lastRow(onChange).value).toBe('2024-03-05'); + expect(valueInput().value).toBe('2024-03-05'); + }); +}); + +describe('cleared when the new column cannot hold it', () => { + it('clears a word on a date column', async () => { + const { onChange } = renderRow({ field: 'title', operator: 'equals', value: 'acme' }); + await pickField('Closed at'); + + expect(lastRow(onChange).value).toBe(''); + expect(valueInput().value).toBe(''); + }); + + it('clears a bare date on a DATETIME column rather than inventing midnight', async () => { + const { onChange } = renderRow({ + field: 'closed_at', + operator: 'equals', + value: '2024-03-05', + }); + await pickField('Created at'); + + // `equals 2024-03-05T00:00` would be a filter the user never wrote and one + // that matches almost nothing — worse than an empty input, because it + // looks answered. + expect(lastRow(onChange).value).toBe(''); + }); + + it('clears a word on a boolean column, leaving the picker at its placeholder', async () => { + const { onChange } = renderRow({ field: 'title', operator: 'equals', value: 'acme' }); + await pickField('Won'); + + expect(lastRow(onChange).value).toBe(''); + expect(screen.getAllByRole('combobox')[2].textContent).toBe('Select value'); + }); + + it('clears both bounds of a range the new column cannot read, back to `[]`', async () => { + // `between` is offered by every date-like column, so the OPERATOR survives + // this switch untouched and only the type judgement runs — the case + // PR #4779's shape-only reconcile could not reach at all. + const { onChange } = renderRow({ + field: 'closed_at', + operator: 'between', + value: ['2024-01-01', '2024-12-31'], + }); + await pickField('Created at'); + + const row = lastRow(onChange); + expect(row.operator).toBe('between'); + // `[]` and not `['', '']`: the "not filled in yet" shape + // `reshapeFilterValue` also produces, which the write path drops. + expect(row.value).toEqual([]); + }); +}); + +describe('a value the new column CAN already hold is left alone', () => { + // Green in both directions by design — the guard on this fix's blast radius. + // Option A (clear on every field switch) was ruled against precisely because + // it would turn this block red. + it('keeps a typed word across two text columns', async () => { + const { onChange } = renderRow({ field: 'title', operator: 'contains', value: 'acme' }); + await pickField('Owner note'); + + expect(lastRow(onChange)).toMatchObject({ field: 'owner_note', value: 'acme' }); + }); + + it('keeps a number across two numeric columns', async () => { + const { onChange } = renderRow({ field: 'amount', operator: 'greaterThan', value: 42 }); + await pickField('Quota'); + + expect(lastRow(onChange)).toMatchObject({ field: 'quota', operator: 'greaterThan', value: 42 }); + }); + + it('keeps a date string on a text column — text holds everything', async () => { + const { onChange } = renderRow({ field: 'closed_at', operator: 'equals', value: '2024-03-05' }); + await pickField('Title'); + + expect(lastRow(onChange).value).toBe('2024-03-05'); + }); + + it('keeps a range both date columns can read', async () => { + const { onChange } = renderRow({ + field: 'closed_at', + operator: 'between', + value: ['2024-01-01', '2024-12-31'], + }); + await pickField('Due on'); + + expect(lastRow(onChange).value).toEqual(['2024-01-01', '2024-12-31']); + }); + + it('leaves an unfilled row unfilled — clearing `""` is not a decision', async () => { + const { onChange } = renderRow({ field: 'title', operator: 'equals', value: '' }); + await pickField('Amount'); + + expect(lastRow(onChange)).toMatchObject({ field: 'amount', operator: 'equals', value: '' }); + }); + + it('leaves a value-less row alone', async () => { + const { onChange } = renderRow({ field: 'title', operator: 'isNull', value: '' }); + await pickField('Amount'); + + expect(lastRow(onChange)).toMatchObject({ operator: 'isNull', value: '' }); + }); +}); + +describe('`retypeFilterValue` — the convertibility judgement, one family at a time', () => { + // `type` here is the FIELD type, and the helper maps it to a family itself — + // the same mapping the value input is drawn from. + const scalar = (value: unknown, type: string) => + retypeFilterValue(value as never, type, 'equals'); + + it('number: a clean numeric reading, or nothing', () => { + expect(scalar('42', 'number')).toBe(42); + expect(scalar(' 42 ', 'number')).toBe(42); + expect(scalar('-3.5', 'number')).toBe(-3.5); + expect(scalar('1e3', 'number')).toBe(1000); + expect(scalar(7, 'currency')).toBe(7); + // Deliberately stricter than `parseFloat`, which reads all three as a + // number (`NaN`→0, 42, 1) and would write a filter the user never typed. + expect(scalar('acme', 'number')).toBe(''); + expect(scalar('42abc', 'number')).toBe(''); + expect(scalar('1,000', 'number')).toBe(''); + expect(scalar('Infinity', 'number')).toBe(''); + expect(scalar(true, 'number')).toBe(''); + }); + + it('boolean: the two words that round-trip, and nothing else', () => { + expect(scalar('true', 'boolean')).toBe(true); + expect(scalar('False', 'boolean')).toBe(false); + expect(scalar(false, 'boolean')).toBe(false); + // Conventions, not readings. + expect(scalar(1, 'boolean')).toBe(''); + expect(scalar('yes', 'boolean')).toBe(''); + expect(scalar('acme', 'boolean')).toBe(''); + }); + + it('date: what a date input can render, plus a zoneless timestamp’s date', () => { + expect(scalar('2024-03-05', 'date')).toBe('2024-03-05'); + expect(scalar('2024-03-05T14:30', 'date')).toBe('2024-03-05'); + expect(scalar('2024-03-05T14:30:59.500', 'date')).toBe('2024-03-05'); + // A date that does not exist renders blank in the input too. + expect(scalar('2024-02-31', 'date')).toBe(''); + expect(scalar('2024-13-01', 'date')).toBe(''); + expect(scalar('05/03/2024', 'date')).toBe(''); + // Zone-carrying: no input here renders it, and truncating by hand is the + // ambiguity the ruling says to refuse (the instant can fall on either day). + expect(scalar('2024-03-05T23:30:00Z', 'date')).toBe(''); + expect(scalar('2024-03-05T23:30:00+08:00', 'date')).toBe(''); + expect(scalar(20240305, 'date')).toBe(''); + }); + + it('datetime: a zoneless local timestamp only', () => { + expect(scalar('2024-03-05T14:30', 'datetime')).toBe('2024-03-05T14:30'); + expect(scalar('2024-03-05T14:30:15', 'datetime')).toBe('2024-03-05T14:30:15'); + expect(scalar('2024-03-05', 'datetime')).toBe(''); + expect(scalar('2024-03-05T25:00', 'datetime')).toBe(''); + expect(scalar('2024-03-05T14:30Z', 'datetime')).toBe(''); + }); + + it('time: a clock reading only', () => { + expect(scalar('14:30', 'time')).toBe('14:30'); + expect(scalar('14:30:15', 'time')).toBe('14:30:15'); + expect(scalar('29:71', 'time')).toBe(''); + // A time-of-day column asks a different question than an instant does, so + // the timestamp is not mined for its clock half. + expect(scalar('2024-03-05T14:30', 'time')).toBe(''); + }); + + it('text-shaped families hold everything, typed as text', () => { + for (const type of ['text', 'select', 'status', 'lookup', 'master_detail', 'user', 'owner']) { + expect(scalar('acme', type)).toBe('acme'); + expect(scalar(42, type)).toBe('42'); + expect(scalar(true, type)).toBe('true'); + } + // An unknown type, and no type at all, fall to text rather than clearing: + // a column this builder cannot classify must not eat the user's value. + expect(scalar('acme', 'something_new')).toBe('acme'); + expect(retypeFilterValue('acme', undefined, 'equals')).toBe('acme'); + }); + + it('the empty scalar is returned unchanged by every family', () => { + for (const type of ['text', 'number', 'boolean', 'date', 'datetime', 'time', 'select']) { + expect(scalar('', type), type).toBe(''); + } + }); +}); + +describe('`retypeFilterValue` — one shape at a time', () => { + it('a list converts entry by entry, keeping the ones that carry', () => { + // Pinned on the helper rather than through the dropdown, and honestly so: + // the only buckets offering `in`/`notIn` today are `select` and `lookup`, + // both text-family, so no field switch can currently drive a list into a + // number column. The helper answers for the family the operator lands in, + // not for today's buckets, and this is where that answer is fixed. + expect(retypeFilterValue(['42', 'acme', '7'], 'number', 'in')).toEqual([42, 7]); + expect(retypeFilterValue(['won', 'lost'], 'number', 'in')).toEqual([]); + expect(retypeFilterValue(['won', 'lost'], 'select', 'in')).toEqual(['won', 'lost']); + expect(retypeFilterValue([], 'number', 'notIn')).toEqual([]); + // A scalar reaching a list operator is normalised, not wrapped blindly. + expect(retypeFilterValue('42', 'number', 'in')).toEqual([42]); + expect(retypeFilterValue('', 'number', 'in')).toEqual([]); + }); + + it('a pair keeps its two slots, and collapses to `[]` only when both go', () => { + expect(retypeFilterValue(['1', '9'], 'number', 'between')).toEqual([1, 9]); + // One bound unreadable: the other still means something, and the range + // keeps the shape `between` requires rather than shrinking to one entry. + expect(retypeFilterValue(['1', 'acme'], 'number', 'between')).toEqual([1, '']); + expect(retypeFilterValue(['acme', 'globex'], 'number', 'between')).toEqual([]); + expect(retypeFilterValue([], 'number', 'between')).toEqual([]); + }); + + it('every family only ever yields its own type or its empty shape', () => { + // The invariant behind all of the above: whatever arrives, what comes out + // is something the new column's input can render. A conversion that + // "mostly" works — `parseFloat`'s partial reads, an invented midnight — + // fails this. + const CORPUS = [ + 'acme', '42', ' 42 ', '42abc', '', 'true', 'False', '2024-03-05', + '2024-03-05T14:30', '2024-03-05T14:30:00Z', '14:30', '2024-02-31', + 0, 42, -3.5, true, false, + ]; + const EXPECTED: Record boolean> = { + number: (v) => v === '' || typeof v === 'number', + boolean: (v) => v === '' || typeof v === 'boolean', + date: (v) => v === '' || /^\d{4}-\d{2}-\d{2}$/.test(String(v)), + datetime: (v) => v === '' || /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}(:\d{2})?$/.test(String(v)), + time: (v) => v === '' || /^\d{2}:\d{2}(:\d{2})?$/.test(String(v)), + text: (v) => typeof v === 'string', + }; + for (const [type, holds] of Object.entries(EXPECTED)) { + for (const value of CORPUS) { + const out = retypeFilterValue(value as never, type, 'equals'); + expect(holds(out), `${type} left ${JSON.stringify(value)} as ${JSON.stringify(out)}`).toBe( + true, + ); + } + } + }); +}); + +describe('the value input each field type draws is unchanged by the refactor', () => { + // `getInputType` now reads the family table instead of its own branch ladder. + // Behaviour-preserving, and pinned type by type so it stays that way. + const EXPECTED_INPUT_TYPE: Array<[string, string]> = [ + ['text', 'text'], + ['number', 'number'], + ['currency', 'number'], + ['percent', 'number'], + ['rating', 'number'], + ['date', 'date'], + ['datetime', 'datetime-local'], + ['time', 'time'], + ['select', 'text'], + ['lookup', 'text'], + ['something_new', 'text'], + ]; + + it.each(EXPECTED_INPUT_TYPE)('a %s column draws ', (type, inputType) => { + const value = { + id: 'root', + logic: 'and', + conditions: [{ id: 'c1', field: 'probe', operator: 'equals', value: '' }], + }; + const { unmount } = render( + {}} + />, + ); + expect(valueInput().type).toBe(inputType); + unmount(); + }); +}); diff --git a/packages/components/src/custom/filter-builder.tsx b/packages/components/src/custom/filter-builder.tsx index 5b1325a62d..04215f59ec 100644 --- a/packages/components/src/custom/filter-builder.tsx +++ b/packages/components/src/custom/filter-builder.tsx @@ -385,6 +385,245 @@ export function reconcileOperatorForField( return offeredOperators[0]?.value ?? operator } +/** + * The value FAMILY a field type edits in — the same six branches + * {@link getInputType} draws its `` from, named once so the type a + * value is CONVERTED to and the input it is EDITED in cannot disagree + * (objectui#4781). + * + * `boolean` is its own family even though it renders no `` at all (a + * two-item Select): what it can hold is a distinct question from what a text + * box can hold, and that is what this answers. + */ +type FilterValueFamily = "text" | "number" | "boolean" | "date" | "datetime" | "time" + +function valueFamilyForFieldType(fieldType: string | undefined): FilterValueFamily { + const type = fieldType || "text" + if (numberLikeTypes.includes(type)) return "number" + if (type === "boolean") return "boolean" + if (type === "date") return "date" + if (type === "datetime") return "datetime" + if (type === "time") return "time" + // select / status / lookup / master_detail / user / owner / text / unknown: + // all edited as free text or as a list of option ids, all string-shaped. + return "text" +} + +/** + * The `` each family is edited with. + * + * Module-private on purpose: what a column DRAWS is observable from the + * rendered input, and that is where the tests pin it, so publishing this table + * would widen the package's API for nothing. + */ +const FILTER_INPUT_TYPE_BY_FAMILY: Readonly> = { + text: "text", + number: "number", + date: "date", + datetime: "datetime-local", + time: "time", + // Never reached today — a boolean column's bucket offers only + // `equals`/`notEquals`, both of which take the two-item Select above. Mapped + // to the harmless default rather than left absent, so the record stays total. + boolean: "text", +} + +/** `YYYY-MM-DD`, the form `` both renders and emits. */ +const DATE_ONLY_PATTERN = /^(\d{4})-(\d{2})-(\d{2})$/ +/** `YYYY-MM-DDTHH:mm[:ss[.sss]]` with NO zone — what `datetime-local` emits. */ +const LOCAL_DATE_TIME_PATTERN = + /^(\d{4}-\d{2}-\d{2})T(\d{2}):(\d{2})(?::(\d{2})(?:\.\d{1,3})?)?$/ +/** `HH:mm[:ss]`, the form `` both renders and emits. */ +const CLOCK_TIME_PATTERN = /^(\d{2}):(\d{2})(?::(\d{2}))?$/ + +/** A date that exists — the pattern alone would accept `2024-02-31`. */ +function isRealCalendarDate(dateOnly: string): boolean { + const match = DATE_ONLY_PATTERN.exec(dateOnly) + if (!match) return false + const [year, month, day] = [Number(match[1]), Number(match[2]), Number(match[3])] + const probe = new Date(Date.UTC(year, month - 1, day)) + return ( + probe.getUTCFullYear() === year && + probe.getUTCMonth() === month - 1 && + probe.getUTCDate() === day + ) +} + +/** A clock reading that exists — the pattern alone would accept `29:71`. */ +function isRealClockTime(hours: string, minutes: string, seconds?: string): boolean { + return ( + Number(hours) <= 23 && + Number(minutes) <= 59 && + (seconds === undefined || Number(seconds) <= 59) + ) +} + +/** + * ONE scalar, converted into `family`, or `undefined` when that family cannot + * hold it — the single convertibility judgement this component makes + * (objectui#4781, ruled there as option B: convert when convertible, clear + * otherwise). + * + * The bar is a CLEAN, unambiguous reading, never a lenient one: + * + * - **number** — `Number()` over the trimmed string, kept only if finite. + * `"42"` → `42`; `"42abc"`, `"1,000"` and `"acme"` convert to nothing and + * the caller clears them. Deliberately stricter than the `parseFloat(x) || + * 0` this file's token input and range inputs use when the USER types into + * an ``: there the browser has already refused + * everything non-numeric, so leniency is unreachable; here the string + * arrives from a column that had no such input, and `parseFloat` would turn + * `"acme"` into `0` — a filter the user never wrote, which is precisely + * what objectui#4781 ruled against. + * - **boolean** — only the two words the row can round-trip back out of a + * boolean column (`"true"` / `"false"`, trimmed, case-insensitively). + * `1` / `0` / `"yes"` are conventions, not readings, so they clear. + * - **date / datetime / time** — only a value the target's own input can + * render as-is, plus the one truncation that loses nothing that input could + * have shown anyway (a zoneless timestamp → its date). Everything else + * clears, which is the ruling's default: + * - `date` ← `"2024-03-05"` as-is, `"2024-03-05T14:30"` → `"2024-03-05"`; + * - `datetime` ← a zoneless `YYYY-MM-DDTHH:mm[:ss]` as-is. A bare date + * does NOT convert: appending midnight would invent the half of the + * value the user never gave, and `equals 2024-03-05T00:00` is a filter + * that silently matches almost nothing — worse than an empty input; + * - `time` ← `HH:mm[:ss]` as-is. A timestamp does NOT convert: a + * time-of-day column asks a different question than an instant does. + * A zone-carrying string (`…Z`, `…+08:00`) clears everywhere: no input here + * can render it, and truncating it by hand is exactly the ambiguity the + * ruling says to refuse. + * - **text** — holds everything, so nothing clears; a non-string is written + * out with `String()` so the row's value is typed for the column it now + * filters (`42` → `"42"`, `true` → `"true"`). + * + * The empty scalar `""` is returned unchanged by every family: an unfilled row + * is already in the family's empty shape, and "clearing" it would be a no-op + * dressed as a decision. + */ +function convertScalarToFamily( + value: string | number | boolean, + family: FilterValueFamily, +): string | number | boolean | undefined { + if (value === "") return "" + + switch (family) { + case "text": + return typeof value === "string" ? value : String(value) + + case "number": { + if (typeof value === "number") return Number.isFinite(value) ? value : undefined + if (typeof value === "boolean") return undefined + const trimmed = value.trim() + if (trimmed === "") return "" + const parsed = Number(trimmed) + return Number.isFinite(parsed) ? parsed : undefined + } + + case "boolean": { + if (typeof value === "boolean") return value + if (typeof value === "number") return undefined + const token = value.trim().toLowerCase() + return token === "true" ? true : token === "false" ? false : undefined + } + + case "date": { + if (typeof value !== "string") return undefined + const trimmed = value.trim() + const local = LOCAL_DATE_TIME_PATTERN.exec(trimmed) + if (local) { + return isRealCalendarDate(local[1]) && isRealClockTime(local[2], local[3], local[4]) + ? local[1] + : undefined + } + return isRealCalendarDate(trimmed) ? trimmed : undefined + } + + case "datetime": { + if (typeof value !== "string") return undefined + const trimmed = value.trim() + const local = LOCAL_DATE_TIME_PATTERN.exec(trimmed) + if (!local) return undefined + return isRealCalendarDate(local[1]) && isRealClockTime(local[2], local[3], local[4]) + ? trimmed + : undefined + } + + case "time": { + if (typeof value !== "string") return undefined + const trimmed = value.trim() + const clock = CLOCK_TIME_PATTERN.exec(trimmed) + if (!clock) return undefined + return isRealClockTime(clock[1], clock[2], clock[3]) ? trimmed : undefined + } + } +} + +/** + * Re-TYPE a row's `value` for the field it is being changed TO — the type + * question, standing beside {@link reshapeFilterValue}'s shape question + * (objectui#4781). + * + * Changing a row's field used to carry the value through untouched whenever the + * shape did not have to change, which is right about the shape and wrong about + * the type: the value input is re-drawn from the NEW field's type, and an + * `` renders a non-numeric value as BLANK. So a `text` + * row filtered `equals "acme"`, pointed at a number column, showed an empty box + * while the row went on carrying `"acme"` — `foldFilterGroupToSpecRules` + * persisted it and the live grid queried `amount equals "acme"`. The same + * invisible-value shape objectui#4768 closed on the operator, one column over. + * + * Carry-if-possible, exactly as `reshapeFilterValue` carries what it can across + * a family change: `"42"` moved to a number column becomes `42` and survives; + * `"acme"` cannot be read as a number by any clean reading, so it clears to the + * family's empty shape — scalar `""`, list `[]`, pair `[]`. What a value can + * become is decided in one place, {@link convertScalarToFamily}. + * + * The arity is read from the operator through the same `filterValueArity` fold + * the rest of this component uses, because each shape clears differently: + * + * - `scalar` — converted, or `""`. + * - `list` — converted ENTRY BY ENTRY, keeping the ones that carry: + * `["42", "acme"]` → `[42]`, and `[]` when none do. Reachable today only + * between the two buckets that offer `in`/`notIn` (`select` ↔ `lookup`), + * which are both text-family, so the conversion is the identity there; it + * is written for the family the operator lands in rather than for today's + * buckets, and pinned directly on this helper. + * - `pair` — both bounds converted independently, an unconvertible bound + * becoming `""` so the range keeps its two slots; both empty collapses to + * `[]`, the "not filled in yet" shape `reshapeFilterValue` also produces + * and the write path drops. + * + * @internal exported for tests + */ +export function retypeFilterValue( + value: FilterBuilderCondition["value"], + fieldType: string | undefined, + operator: string, +): FilterBuilderCondition["value"] { + const family = valueFamilyForFieldType(fieldType) + + switch (filterValueArity(operator)) { + case "list": { + const carried: (string | number | boolean)[] = [] + for (const entry of normalizeToArray(value)) { + const converted = convertScalarToFamily(entry, family) + if (converted !== undefined && converted !== "") carried.push(converted) + } + return carried + } + case "pair": { + const [min = "", max = ""] = normalizeToArray(value) + const lower = convertScalarToFamily(min, family) ?? "" + const upper = convertScalarToFamily(max, family) ?? "" + return lower === "" && upper === "" ? [] : [lower, upper] + } + default: { + const scalar = Array.isArray(value) ? (value[0] ?? "") : value + return convertScalarToFamily(scalar, family) ?? "" + } + } +} + /** The two bounds a `pair` row edits, with the gaps filled in for rendering. */ function toPairBounds( value: FilterBuilderCondition["value"], @@ -626,20 +865,33 @@ function FilterBuilder({ * An operator the new bucket DOES offer is left alone — resetting it would * throw away a choice that is still valid (switching `contains` from one text * column to another must not silently become `equals`). + * + * The value is settled in the same edit, in two steps that answer two + * different questions and are both needed (objectui#4781): + * + * 1. `reshapeFilterValue` — the SHAPE the new OPERATOR takes, and only when + * the operator actually changed; + * 2. `retypeFilterValue` — the TYPE the new FIELD takes, always. A field + * switch redraws the value input from the new field's type, so a value + * that type cannot hold is a value the user can no longer see or edit + * while the row keeps filtering by it. Convertible values are carried + * (`"42"` → `42`); the rest clear. */ const changeField = (conditionId: string, nextField: string) => { const offered = getOperatorsForField(nextField) + const nextType = fields.find((f) => f.value === nextField)?.type handleChange({ ...filterGroup, conditions: filterGroup.conditions.map((c) => { if (c.id !== conditionId) return c const nextOperator = reconcileOperatorForField(c.operator, offered) - if (nextOperator === c.operator) return { ...c, field: nextField } + const reshaped = + nextOperator === c.operator ? c.value : reshapeFilterValue(c.value, nextOperator) return { ...c, field: nextField, operator: nextOperator, - value: reshapeFilterValue(c.value, nextOperator), + value: retypeFilterValue(reshaped, nextType, nextOperator), } }), }) @@ -652,15 +904,14 @@ function FilterBuilder({ return !VALUELESS_FILTER_BUILDER_OPERATORS.has(operator) } + // Derived from the value FAMILY rather than from a second branch ladder over + // the same type lists: the input a value is edited in and the type a value is + // converted to on a field switch are the same question, and answering it + // twice is how the two could come to disagree (objectui#4781) — the disagreement + // being exactly the defect, a value the row keeps and its input cannot show. const getInputType = (fieldValue: string) => { const field = fields.find((f) => f.value === fieldValue) - const fieldType = field?.type || "text" - - if (numberLikeTypes.includes(fieldType)) return "number" - if (fieldType === "date") return "date" - if (fieldType === "datetime") return "datetime-local" - if (fieldType === "time") return "time" - return "text" + return FILTER_INPUT_TYPE_BY_FAMILY[valueFamilyForFieldType(field?.type)] } const renderValueInput = (condition: FilterBuilderCondition) => {