diff --git a/e2e/test/scenarios/filters/filter-types.cy.spec.js b/e2e/test/scenarios/filters/filter-types.cy.spec.js index e7e82f79e3dc36f8fbee3322f605e4de6572ee06..808b4c0eeb335d88c4ee6e2e88ac5b6c069fde79 100644 --- a/e2e/test/scenarios/filters/filter-types.cy.spec.js +++ b/e2e/test/scenarios/filters/filter-types.cy.spec.js @@ -43,6 +43,23 @@ const STRING_CASES = [ expectedDisplayName: "Title contains Al", expectedRowCount: 47, }, + { + title: "contains, multiple options", + columnName: "Title", + operator: "Contains", + values: ["Al", "Me"], + expectedDisplayName: "Title contains 2 selections", + expectedRowCount: 68, + }, + { + title: "contains, multiple options, case sensitive", + columnName: "Title", + operator: "Contains", + values: ["Al", "Me"], + options: ["Case sensitive"], + expectedDisplayName: "Title contains 2 selections", + expectedRowCount: 28, + }, { title: "contains, case sensitive", columnName: "Title", @@ -60,6 +77,23 @@ const STRING_CASES = [ expectedDisplayName: "Title does not contain Al", expectedRowCount: 153, }, + { + title: "does not contain, multiple options", + columnName: "Title", + operator: "Does not contain", + values: ["Al", "Me"], + expectedDisplayName: "Title does not contain 2 selections", + expectedRowCount: 132, + }, + { + title: "does not contain, multiple options, case sensitive", + columnName: "Title", + operator: "Does not contain", + values: ["Al", "Me"], + options: ["Case sensitive"], + expectedDisplayName: "Title does not contain 2 selections", + expectedRowCount: 172, + }, { title: "does not contain, case sensitive", columnName: "Title", @@ -77,14 +111,31 @@ const STRING_CASES = [ expectedDisplayName: "Title starts with sm", expectedRowCount: 11, }, + { + title: "starts with, multiple options", + columnName: "Title", + operator: "Starts with", + values: ["sm", "Me"], + expectedDisplayName: "Title starts with 2 selections", + expectedRowCount: 25, + }, + { + title: "starts with, multiple options, case sensitive", + columnName: "Title", + operator: "Starts with", + values: ["sm", "Me"], + options: ["Case sensitive"], + expectedDisplayName: "Title starts with 2 selections", + expectedRowCount: 14, + }, { title: "starts with, case sensitive", columnName: "Title", operator: "Starts with", - values: ["Sm"], + values: ["sm"], options: ["Case sensitive"], - expectedDisplayName: "Title starts with Sm", - expectedRowCount: 11, + expectedDisplayName: "Title starts with sm", + expectedRowCount: 0, }, { title: "ends with", @@ -94,6 +145,23 @@ const STRING_CASES = [ expectedDisplayName: "Title ends with At", expectedRowCount: 22, }, + { + title: "ends with, multiple options", + columnName: "Title", + operator: "Ends with", + values: ["At", "es"], + expectedDisplayName: "Title ends with 2 selections", + expectedRowCount: 38, + }, + { + title: "ends with, multiple options, case sensitive", + columnName: "Title", + operator: "Ends with", + values: ["At", "es"], + options: ["Case sensitive"], + expectedDisplayName: "Title ends with 2 selections", + expectedRowCount: 16, + }, { title: "ends with, case sensitive", columnName: "Title", @@ -416,8 +484,11 @@ describe("scenarios > filters > filter types", () => { popover().findByText(columnName).click(); selectFilterOperator(operator); popover().within(() => { - values.forEach((value, index) => { - cy.findByLabelText("Filter value").focus().type(value).blur(); + values.forEach(value => { + cy.findByLabelText("Filter value") + .focus() + .type(value) + .realPress("Tab"); }); options.forEach(option => cy.findByText(option).click()); cy.button("Add filter").click(); diff --git a/frontend/src/metabase/querying/components/FilterModal/StringFilterEditor/StringFilterEditor.tsx b/frontend/src/metabase/querying/components/FilterModal/StringFilterEditor/StringFilterEditor.tsx index 2aade6244f11aedc9200209f8b0d704d3f5ab3a2..a540229f100a59b9e59356d559515d0e22089218 100644 --- a/frontend/src/metabase/querying/components/FilterModal/StringFilterEditor/StringFilterEditor.tsx +++ b/frontend/src/metabase/querying/components/FilterModal/StringFilterEditor/StringFilterEditor.tsx @@ -2,8 +2,9 @@ import { useMemo, useState } from "react"; import { t } from "ttag"; import { getColumnIcon } from "metabase/common/utils/columns"; +import type { OperatorType } from "metabase/querying/hooks/use-string-filter"; import { useStringFilter } from "metabase/querying/hooks/use-string-filter"; -import { Grid, TextInput } from "metabase/ui"; +import { Grid, MultiAutocomplete } from "metabase/ui"; import type * as Lib from "metabase-lib"; import { StringFilterValuePicker } from "../../FilterValuePicker"; @@ -24,11 +25,10 @@ export function StringFilterEditor({ const [isFocused, setIsFocused] = useState(false); const { + type, operator, availableOptions, values, - valueCount, - hasMultipleValues, options, getDefaultValues, getFilterClause, @@ -90,8 +90,7 @@ export function StringFilterEditor({ stageIndex={stageIndex} column={column} values={values} - valueCount={valueCount} - hasMultipleValues={hasMultipleValues} + type={type} onChange={handleInputChange} onFocus={handleInputFocus} onBlur={handleInputBlur} @@ -107,8 +106,7 @@ interface StringValueInputProps { stageIndex: number; column: Lib.ColumnMetadata; values: string[]; - valueCount: number; - hasMultipleValues?: boolean; + type: OperatorType; onChange: (values: string[]) => void; onFocus: () => void; onBlur: () => void; @@ -119,13 +117,12 @@ function StringValueInput({ stageIndex, column, values, - valueCount, - hasMultipleValues, + type, onChange, onFocus, onBlur, }: StringValueInputProps) { - if (hasMultipleValues) { + if (type === "exact") { return ( <StringFilterValuePicker query={query} @@ -140,13 +137,14 @@ function StringValueInput({ ); } - if (valueCount === 1) { + if (type === "partial") { return ( - <TextInput - value={values[0]} + <MultiAutocomplete + data={[]} + value={values} placeholder={t`Enter some text`} aria-label={t`Filter value`} - onChange={event => onChange([event.target.value])} + onChange={onChange} onFocus={onFocus} onBlur={onBlur} /> diff --git a/frontend/src/metabase/querying/components/FilterModal/StringFilterEditor/StringFilterEditor.unit.spec.tsx b/frontend/src/metabase/querying/components/FilterModal/StringFilterEditor/StringFilterEditor.unit.spec.tsx index 77f718241225b9c9ceccfeddd96a09c6e0add42a..775fa534bcd7b716f709d9877a03ebd91a3239c4 100644 --- a/frontend/src/metabase/querying/components/FilterModal/StringFilterEditor/StringFilterEditor.unit.spec.tsx +++ b/frontend/src/metabase/querying/components/FilterModal/StringFilterEditor/StringFilterEditor.unit.spec.tsx @@ -277,11 +277,8 @@ describe("StringFilterEditor", () => { filter, }); - await userEvent.clear(screen.getByDisplayValue("Ga")); - await userEvent.type( - screen.getByPlaceholderText("Enter some text"), - "Wi", - ); + const input = screen.getByLabelText("Filter value"); + await userEvent.type(input, "{backspace}Wi"); await userEvent.tab(); expect(getNextFilterName()).toBe("Category starts with Wi"); diff --git a/frontend/src/metabase/querying/components/FilterPicker/StringFilterPicker/StringFilterPicker.tsx b/frontend/src/metabase/querying/components/FilterPicker/StringFilterPicker/StringFilterPicker.tsx index 0e00ae6ae71e381bebda09b063107ea747d2a574..607a27d3d2bd8ef2abd1a6469b6470d329ac9317 100644 --- a/frontend/src/metabase/querying/components/FilterPicker/StringFilterPicker/StringFilterPicker.tsx +++ b/frontend/src/metabase/querying/components/FilterPicker/StringFilterPicker/StringFilterPicker.tsx @@ -2,8 +2,9 @@ import type { FormEvent } from "react"; import { useMemo } from "react"; import { t } from "ttag"; +import type { OperatorType } from "metabase/querying/hooks/use-string-filter"; import { useStringFilter } from "metabase/querying/hooks/use-string-filter"; -import { Box, Checkbox, Flex, TextInput } from "metabase/ui"; +import { Box, Checkbox, Flex, MultiAutocomplete } from "metabase/ui"; import * as Lib from "metabase-lib"; import { StringFilterValuePicker } from "../../FilterValuePicker"; @@ -28,12 +29,10 @@ export function StringFilterPicker({ ); const { + type, operator, availableOptions, values, - valueCount, - hasMultipleValues, - hasCaseSensitiveOption, options, isValid, getDefaultValues, @@ -86,12 +85,11 @@ export function StringFilterPicker({ stageIndex={stageIndex} column={column} values={values} - valueCount={valueCount} - hasMultipleValues={hasMultipleValues} + type={type} onChange={setValues} /> <FilterPickerFooter isNew={isNew} canSubmit={isValid}> - {hasCaseSensitiveOption && ( + {type === "partial" && ( <CaseSensitiveOption value={options["case-sensitive"] ?? false} onChange={newValue => setOptions({ "case-sensitive": newValue })} @@ -108,8 +106,7 @@ interface StringValueInputProps { stageIndex: number; column: Lib.ColumnMetadata; values: string[]; - valueCount: number; - hasMultipleValues?: boolean; + type: OperatorType; onChange: (values: string[]) => void; } @@ -118,11 +115,10 @@ function StringValueInput({ stageIndex, column, values, - valueCount, - hasMultipleValues, + type, onChange, }: StringValueInputProps) { - if (hasMultipleValues) { + if (type === "exact") { return ( <Box p="md" mah="25vh" style={{ overflow: "auto" }}> <StringFilterValuePicker @@ -137,16 +133,17 @@ function StringValueInput({ ); } - if (valueCount === 1) { + if (type === "partial") { return ( <Flex p="md"> - <TextInput - value={values[0]} + <MultiAutocomplete + value={values} + data={[]} placeholder={t`Enter some text`} autoFocus w="100%" aria-label={t`Filter value`} - onChange={event => onChange([event.target.value])} + onChange={onChange} /> </Flex> ); diff --git a/frontend/src/metabase/querying/components/FilterPicker/StringFilterPicker/StringFilterPicker.unit.spec.tsx b/frontend/src/metabase/querying/components/FilterPicker/StringFilterPicker/StringFilterPicker.unit.spec.tsx index ccc1491784dccfc0c82f45d603a611095b2fd305..eeb7e796b07fdb14324b11f769b5043d4affd965 100644 --- a/frontend/src/metabase/querying/components/FilterPicker/StringFilterPicker/StringFilterPicker.unit.spec.tsx +++ b/frontend/src/metabase/querying/components/FilterPicker/StringFilterPicker/StringFilterPicker.unit.spec.tsx @@ -204,25 +204,6 @@ describe("StringFilterPicker", () => { expect(getNextFilterColumnName()).toBe("Product → Description"); }); - it("should add a filter with one value via keyboard", async () => { - const { onChange, getNextFilterParts, getNextFilterColumnName } = setup(); - - await setOperator("Contains"); - const input = screen.getByPlaceholderText("Enter some text"); - await userEvent.type(input, "{enter}"); - expect(onChange).not.toHaveBeenCalled(); - - await userEvent.type(input, "green{enter}"); - expect(onChange).toHaveBeenCalled(); - expect(getNextFilterParts()).toMatchObject({ - operator: "contains", - column: expect.anything(), - values: ["green"], - options: { "case-sensitive": false }, - }); - expect(getNextFilterColumnName()).toBe("Product → Description"); - }); - it("should add a case-sensitive filter", async () => { const { getNextFilterParts, getNextFilterColumnName } = setup(); @@ -391,8 +372,10 @@ describe("StringFilterPicker", () => { it("should update a filter", async () => { const { getNextFilterParts, getNextFilterColumnName } = setup(opts); - const input = screen.getByRole("textbox", { name: "Filter value" }); - await userEvent.clear(input); + const input = screen.getByLabelText("Filter value"); + expect(screen.getByDisplayValue("abc")).toBeInTheDocument(); + await userEvent.type(input, "{backspace}"); + expect(screen.queryByDisplayValue("abc")).not.toBeInTheDocument(); await userEvent.type(input, "foo"); await userEvent.click(screen.getByLabelText("Case sensitive")); @@ -539,8 +522,7 @@ describe("StringFilterPicker", () => { await setOperator("Contains"); - expect(screen.getByDisplayValue("Gadget")).toBeInTheDocument(); - expect(screen.queryByDisplayValue("Gizmo")).not.toBeInTheDocument(); + expect(screen.getByDisplayValue("Gadget,Gizmo")).toBeInTheDocument(); expect(updateButton).toBeEnabled(); await setOperator("Is empty"); diff --git a/frontend/src/metabase/querying/hooks/use-string-filter/constants.ts b/frontend/src/metabase/querying/hooks/use-string-filter/constants.ts index 67838f0717b9129ff9ae809e3de4d02305d036b5..803fe63d575e32d5551bce7ec8610fe1174007ab 100644 --- a/frontend/src/metabase/querying/hooks/use-string-filter/constants.ts +++ b/frontend/src/metabase/querying/hooks/use-string-filter/constants.ts @@ -3,40 +3,34 @@ import type { OperatorOption } from "./types"; export const OPERATOR_OPTIONS: Record<string, OperatorOption> = { "=": { operator: "=", - valueCount: 1, - hasMultipleValues: true, + type: "exact", }, "!=": { operator: "!=", - valueCount: 1, - hasMultipleValues: true, + type: "exact", }, contains: { operator: "contains", - valueCount: 1, - hasCaseSensitiveOption: true, + type: "partial", }, "does-not-contain": { operator: "does-not-contain", - valueCount: 1, - hasCaseSensitiveOption: true, + type: "partial", }, "starts-with": { operator: "starts-with", - valueCount: 1, - hasCaseSensitiveOption: true, + type: "partial", }, "ends-with": { operator: "ends-with", - valueCount: 1, - hasCaseSensitiveOption: true, + type: "partial", }, "is-empty": { operator: "is-empty", - valueCount: 0, + type: "empty", }, "not-empty": { operator: "not-empty", - valueCount: 0, + type: "empty", }, }; diff --git a/frontend/src/metabase/querying/hooks/use-string-filter/index.ts b/frontend/src/metabase/querying/hooks/use-string-filter/index.ts index 5b647ceac5f2021472997bb201bf01be4e4f4670..6991e18bc02e5247564298af7c4ae0017854b77a 100644 --- a/frontend/src/metabase/querying/hooks/use-string-filter/index.ts +++ b/frontend/src/metabase/querying/hooks/use-string-filter/index.ts @@ -1 +1,2 @@ export * from "./use-string-filter"; +export * from "./types"; diff --git a/frontend/src/metabase/querying/hooks/use-string-filter/types.ts b/frontend/src/metabase/querying/hooks/use-string-filter/types.ts index 257a135fdd1d1d1c21cef130297e6ac62eae1bb5..c9957bae905007508e0aa20a9822c828266c2e6a 100644 --- a/frontend/src/metabase/querying/hooks/use-string-filter/types.ts +++ b/frontend/src/metabase/querying/hooks/use-string-filter/types.ts @@ -1,9 +1,9 @@ import type { FilterOperatorOption } from "metabase/querying/utils/filters"; import type * as Lib from "metabase-lib"; +export type OperatorType = "exact" | "partial" | "empty"; + export interface OperatorOption extends FilterOperatorOption<Lib.StringFilterOperatorName> { - valueCount: number; - hasMultipleValues?: boolean; - hasCaseSensitiveOption?: boolean; + type: OperatorType; } diff --git a/frontend/src/metabase/querying/hooks/use-string-filter/use-string-filter.ts b/frontend/src/metabase/querying/hooks/use-string-filter/use-string-filter.ts index 645dc22602c76f7c4f914ece37a1a9225d4bba6d..fd3fadef382a5e55cd5744d486ccf8741e24db51 100644 --- a/frontend/src/metabase/querying/hooks/use-string-filter/use-string-filter.ts +++ b/frontend/src/metabase/querying/hooks/use-string-filter/use-string-filter.ts @@ -46,17 +46,14 @@ export function useStringFilter({ filterParts ? filterParts.options : {}, ); - const { valueCount, hasMultipleValues, hasCaseSensitiveOption } = - getOptionByOperator(operator); + const { type } = getOptionByOperator(operator); const isValid = isValidFilter(operator, column, values, options); return { + type, operator, availableOptions, values, - valueCount, - hasMultipleValues, - hasCaseSensitiveOption, options, isValid, getDefaultValues, diff --git a/frontend/src/metabase/querying/hooks/use-string-filter/use-string-filter.unit.spec.ts b/frontend/src/metabase/querying/hooks/use-string-filter/use-string-filter.unit.spec.ts index 3c305dc518d7334a093532b8dd0423bef6399156..79d8360a086252679cb083b3f6dd1f23e41c7cc1 100644 --- a/frontend/src/metabase/querying/hooks/use-string-filter/use-string-filter.unit.spec.ts +++ b/frontend/src/metabase/querying/hooks/use-string-filter/use-string-filter.unit.spec.ts @@ -198,11 +198,11 @@ describe("useStringFilter", () => { }, { operator: "starts-with", - values: [""], + values: [], }, { operator: "ends-with", - values: [""], + values: [], }, ])( 'should validate values for "$operator" operator', diff --git a/frontend/src/metabase/querying/hooks/use-string-filter/utils.ts b/frontend/src/metabase/querying/hooks/use-string-filter/utils.ts index 781119eb5da81f46910b2a2a1843cc7f8a05f108..72d78dd188797b92c33529336867c202a7d8cd59 100644 --- a/frontend/src/metabase/querying/hooks/use-string-filter/utils.ts +++ b/frontend/src/metabase/querying/hooks/use-string-filter/utils.ts @@ -42,14 +42,8 @@ export function getDefaultValues( operator: Lib.StringFilterOperatorName, values: string[], ): string[] { - const { valueCount, hasMultipleValues } = OPERATOR_OPTIONS[operator]; - if (hasMultipleValues) { - return values.filter(isNotEmpty); - } - - return Array(valueCount) - .fill("") - .map((value, index) => values[index] ?? value); + const { type } = OPERATOR_OPTIONS[operator]; + return type !== "empty" ? values.filter(isNotEmpty) : []; } export function isValidFilter( @@ -77,11 +71,8 @@ function getFilterParts( values: string[], options: Lib.StringFilterOptions, ): Lib.StringFilterParts | undefined { - const { valueCount, hasMultipleValues } = OPERATOR_OPTIONS[operator]; - if (!values.every(isNotEmpty)) { - return undefined; - } - if (hasMultipleValues ? values.length === 0 : values.length !== valueCount) { + const { type } = OPERATOR_OPTIONS[operator]; + if (values.length === 0 && type !== "empty") { return undefined; } diff --git a/frontend/src/metabase/ui/components/inputs/MultiAutocomplete/MultiAutocomplete.tsx b/frontend/src/metabase/ui/components/inputs/MultiAutocomplete/MultiAutocomplete.tsx index d3a385236b2645954c7ce4b4c8c2ef7ada4f551e..5849dc75004646153e5f8d9dae35325d7e55bbd3 100644 --- a/frontend/src/metabase/ui/components/inputs/MultiAutocomplete/MultiAutocomplete.tsx +++ b/frontend/src/metabase/ui/components/inputs/MultiAutocomplete/MultiAutocomplete.tsx @@ -4,6 +4,10 @@ import { useUncontrolled } from "@mantine/hooks"; import type { ClipboardEvent, FocusEvent } from "react"; import { useMemo, useState } from "react"; +type MultiAutocompleteProps = Omit<MultiSelectProps, "shouldCreate"> & { + shouldCreate?: (query: string, selectedValues: string[]) => boolean; +}; + export function MultiAutocomplete({ data, value: controlledValue, @@ -11,13 +15,13 @@ export function MultiAutocomplete({ searchValue: controlledSearchValue, placeholder, autoFocus, - shouldCreate, + shouldCreate = defaultShouldCreate, onChange, onSearchChange, onFocus, onBlur, ...props -}: MultiSelectProps) { +}: MultiAutocompleteProps) { const [selectedValues, setSelectedValues] = useUncontrolled({ value: controlledValue, defaultValue, @@ -59,7 +63,7 @@ export function MultiAutocomplete({ const handleSearchChange = (newSearchValue: string) => { setSearchValue(newSearchValue); - const isValid = shouldCreate?.(newSearchValue, []); + const isValid = shouldCreate?.(newSearchValue, selectedValues); if (isValid) { setSelectedValues([...lastSelectedValues, newSearchValue]); } else { @@ -73,7 +77,7 @@ export function MultiAutocomplete({ if (values.length > 1) { const validValues = [...new Set(values)] .map(value => value.trim()) - .filter(value => shouldCreate?.(value, [])); + .filter(value => shouldCreate?.(value, selectedValues)); if (validValues.length > 0) { event.preventDefault(); const newSelectedValues = [...lastSelectedValues, ...validValues]; @@ -127,3 +131,9 @@ function getAvailableSelectItems( return [...mapping.entries()].map(([value, label]) => ({ value, label })); } + +function defaultShouldCreate(query: string, selectedValues: string[]) { + return ( + query.trim().length > 0 && !selectedValues.some(value => value === query) + ); +}