diff --git a/frontend/src/metabase/query_builder/components/filters/modals/BulkFilterItem/BulkFilterItem.tsx b/frontend/src/metabase/query_builder/components/filters/modals/BulkFilterItem/BulkFilterItem.tsx
index 7123bde68419ab23964a84818fad909eebc81255..b86ce3e37d2309456d0fd35afa8060a2d8ca7041 100644
--- a/frontend/src/metabase/query_builder/components/filters/modals/BulkFilterItem/BulkFilterItem.tsx
+++ b/frontend/src/metabase/query_builder/components/filters/modals/BulkFilterItem/BulkFilterItem.tsx
@@ -3,7 +3,7 @@ import React, { useMemo, useCallback } from "react";
import Filter from "metabase-lib/lib/queries/structured/Filter";
import Dimension from "metabase-lib/lib/Dimension";
import StructuredQuery from "metabase-lib/lib/queries/StructuredQuery";
-import { isBoolean, isString } from "metabase/lib/schema_metadata";
+import { isBoolean, isString, isNumber } from "metabase/lib/schema_metadata";
import { BooleanPickerCheckbox } from "metabase/query_builder/components/filters/pickers/BooleanPicker";
import { BulkFilterSelect } from "../BulkFilterSelect";
@@ -85,6 +85,8 @@ export const BulkFilterItem = ({
case "type/PK":
case "type/FK":
case "type/Text":
+ case "type/Integer":
+ case "type/Float":
return (
<InlineValuePicker
filter={filter ?? newFilter}
@@ -129,6 +131,11 @@ const getNewFilter = (query: StructuredQuery, dimension: Dimension): Filter => {
useDefaultOperator: !isBooleanField,
});
+ const isNumericField = isNumber(field);
+ if (isNumericField) {
+ filter = filter.setOperator("between");
+ }
+
const isTextField = isString(field) && field.has_field_values !== "list";
if (isTextField) {
filter = filter.setOperator("contains");
diff --git a/frontend/src/metabase/query_builder/components/filters/modals/BulkFilterItem/BulkFilterItem.unit.spec.tsx b/frontend/src/metabase/query_builder/components/filters/modals/BulkFilterItem/BulkFilterItem.unit.spec.tsx
index e475220efb78dd5f8c744139d85ea9140ae63071..2a7b8483f98980183bf61acf889bcf1cd9e084eb 100644
--- a/frontend/src/metabase/query_builder/components/filters/modals/BulkFilterItem/BulkFilterItem.unit.spec.tsx
+++ b/frontend/src/metabase/query_builder/components/filters/modals/BulkFilterItem/BulkFilterItem.unit.spec.tsx
@@ -29,7 +29,7 @@ const booleanField = new Field({
const intField = new Field({
database_type: "test",
- semantic_type: "",
+ semantic_type: "type/Integer",
table_id: 8,
name: "int_num",
has_field_values: "none",
@@ -43,7 +43,7 @@ const intField = new Field({
const floatField = new Field({
database_type: "test",
- semantic_type: "",
+ semantic_type: "type/Integer",
table_id: 8,
name: "float_num",
has_field_values: "none",
@@ -145,6 +145,8 @@ const pkDimension = pkField.dimension();
const fkDimension = fkField.dimension();
const textDimension = textField.dimension();
+const store = getStore();
+
describe("BulkFilterItem", () => {
it("renders a boolean picker for a boolean filter", () => {
const testFilter = new Filter(
@@ -169,7 +171,7 @@ describe("BulkFilterItem", () => {
expect(screen.getByLabelText("false")).not.toBeChecked();
});
- it("renders a bulk filter select for integer field type", () => {
+ it("renders a value picker integer field type", () => {
const testFilter = new Filter(
["=", ["field", intField.id, null], 99],
null,
@@ -178,24 +180,23 @@ describe("BulkFilterItem", () => {
const changeSpy = jest.fn();
render(
- <BulkFilterItem
- query={query}
- filter={testFilter}
- dimension={intDimension}
- onAddFilter={changeSpy}
- onChangeFilter={changeSpy}
- onRemoveFilter={changeSpy}
- />,
+ <Provider store={store}>
+ <BulkFilterItem
+ query={query}
+ filter={testFilter}
+ dimension={intDimension}
+ onAddFilter={changeSpy}
+ onChangeFilter={changeSpy}
+ onRemoveFilter={changeSpy}
+ />
+ </Provider>,
);
- expect(screen.queryByText("true")).toBeNull();
- expect(screen.getByTestId("select-button")).toHaveAttribute(
- "aria-label",
- "int_num",
- );
+ screen.getByTestId("value-picker");
+ screen.getByLabelText(intField.name);
});
- it("renders a bulk filter select for float field type", () => {
+ it("renders a value picker for float field type", () => {
const testFilter = new Filter(
["=", ["field", floatField.id, null], 99],
null,
@@ -204,20 +205,39 @@ describe("BulkFilterItem", () => {
const changeSpy = jest.fn();
render(
- <BulkFilterItem
- query={query}
- filter={testFilter}
- dimension={floatDimension}
- onAddFilter={changeSpy}
- onChangeFilter={changeSpy}
- onRemoveFilter={changeSpy}
- />,
+ <Provider store={store}>
+ <BulkFilterItem
+ query={query}
+ filter={testFilter}
+ dimension={floatDimension}
+ onAddFilter={changeSpy}
+ onChangeFilter={changeSpy}
+ onRemoveFilter={changeSpy}
+ />
+ </Provider>,
);
- expect(screen.queryByText("true")).toBeNull();
- expect(screen.getByTestId("select-button")).toHaveAttribute(
- "aria-label",
- "float_num",
+ screen.getByTestId("value-picker");
+ screen.getByLabelText(floatField.name);
+ });
+
+ it("defaults to a between picker for float field type", () => {
+ const changeSpy = jest.fn();
+
+ render(
+ <Provider store={store}>
+ <BulkFilterItem
+ query={query}
+ filter={undefined}
+ dimension={floatDimension}
+ onAddFilter={changeSpy}
+ onChangeFilter={changeSpy}
+ onRemoveFilter={changeSpy}
+ />
+ </Provider>,
);
+ screen.getByTestId("value-picker");
+ screen.getByLabelText(floatField.name);
+ screen.getByText("Between");
});
it("renders a category picker for category type", () => {
@@ -227,7 +247,6 @@ describe("BulkFilterItem", () => {
query,
);
const changeSpy = jest.fn();
- const store = getStore();
render(
<Provider store={store}>
@@ -251,7 +270,6 @@ describe("BulkFilterItem", () => {
query,
);
const changeSpy = jest.fn();
- const store = getStore();
render(
<Provider store={store}>
@@ -276,7 +294,6 @@ describe("BulkFilterItem", () => {
query,
);
const changeSpy = jest.fn();
- const store = getStore();
render(
<Provider store={store}>
@@ -301,7 +318,6 @@ describe("BulkFilterItem", () => {
query,
);
const changeSpy = jest.fn();
- const store = getStore();
render(
<Provider store={store}>
@@ -322,7 +338,6 @@ describe("BulkFilterItem", () => {
it("defaults key filters to 'is' operator", () => {
const changeSpy = jest.fn();
- const store = getStore();
render(
<Provider store={store}>
@@ -341,7 +356,6 @@ describe("BulkFilterItem", () => {
it("defaults text filters to 'contains' operator", () => {
const changeSpy = jest.fn();
- const store = getStore();
render(
<Provider store={store}>
diff --git a/frontend/src/metabase/query_builder/components/filters/modals/BulkFilterItem/constants.ts b/frontend/src/metabase/query_builder/components/filters/modals/BulkFilterItem/constants.ts
index a193585019329d978af6944ad5cb1a464ac353f9..a2281df989659538b3f9fa03819068cbe84f0c45 100644
--- a/frontend/src/metabase/query_builder/components/filters/modals/BulkFilterItem/constants.ts
+++ b/frontend/src/metabase/query_builder/components/filters/modals/BulkFilterItem/constants.ts
@@ -4,8 +4,10 @@ export const FIELD_PRIORITY = [
"type/DateTimeWithLocalTZ",
"type/DateTimeWithZoneOffset",
"type/DateTimeWithZoneID",
- "type/Boolean",
"list",
+ "type/Integer",
+ "type/Float",
+ "type/Boolean",
"type/Category",
"type/PK",
"type/FK",
diff --git a/frontend/src/metabase/query_builder/components/filters/modals/BulkFilterModal/BulkFilterModal.tsx b/frontend/src/metabase/query_builder/components/filters/modals/BulkFilterModal/BulkFilterModal.tsx
index 357ba6a56f38db2e920c38af22e1e2f9a92605ec..035836e9ce617b184dd42efe499060c2a79b83d1 100644
--- a/frontend/src/metabase/query_builder/components/filters/modals/BulkFilterModal/BulkFilterModal.tsx
+++ b/frontend/src/metabase/query_builder/components/filters/modals/BulkFilterModal/BulkFilterModal.tsx
@@ -22,6 +22,8 @@ import {
ModalTitle,
} from "./BulkFilterModal.styled";
+import { fixBetweens } from "./utils";
+
export interface BulkFilterModalProps {
question: Question;
onClose?: () => void;
@@ -66,7 +68,8 @@ const BulkFilterModal = ({
}, [query]);
const handleApplyQuery = useCallback(() => {
- query.clean().update(undefined, { run: true });
+ const preCleanedQuery = fixBetweens(query);
+ preCleanedQuery.clean().update(undefined, { run: true });
onClose?.();
}, [query, onClose]);
diff --git a/frontend/src/metabase/query_builder/components/filters/modals/BulkFilterModal/utils.ts b/frontend/src/metabase/query_builder/components/filters/modals/BulkFilterModal/utils.ts
new file mode 100644
index 0000000000000000000000000000000000000000..0f956dd48707ea268c29a99bebe93c5e622042fe
--- /dev/null
+++ b/frontend/src/metabase/query_builder/components/filters/modals/BulkFilterModal/utils.ts
@@ -0,0 +1,64 @@
+import StructuredQuery from "metabase-lib/lib/queries/StructuredQuery";
+import Filter from "metabase-lib/lib/queries/structured/Filter";
+
+// fix between filters with missing or misordered arguments
+export function fixBetweens(query: StructuredQuery): StructuredQuery {
+ const betweenFilters = query
+ .filters()
+ .filter(filter => filter.operatorName() === "between");
+
+ // find the first invalid filter (if any), and fix it recursively
+ for (const filter of betweenFilters) {
+ const validArgumentsCount = countValidArumgents(filter);
+ if (validArgumentsCount === 1) {
+ return fixBetweens(handleEmptyBetween(filter));
+ } else if (validArgumentsCount === 2 && hasBackwardsArguments(filter)) {
+ return fixBetweens(swapFilterArguments(filter));
+ }
+ }
+
+ return query;
+}
+
+/**
+ * if a numeric field has an invalid between filter that has only one value
+ * we will intuit that the user wants a >= or <= filter
+ *
+ * @param filter a filter with a single valid argument
+ * @returns a new query with this filter's operator changed
+ */
+export function handleEmptyBetween(filter: Filter): StructuredQuery {
+ const [validArgument] = filter.arguments().filter(isValidArgument);
+
+ const newOperator = isValidArgument(filter.arguments()[0]) ? ">=" : "<=";
+
+ const newFilter = filter
+ .setOperator(newOperator)
+ .setArguments([validArgument]);
+
+ return filter.replace(newFilter);
+}
+
+/**
+ * swaps the filter's arguments
+ * @param filter a filter with backwards arguments
+ * @returns a new query with this filter's arguments swapped
+ */
+export function swapFilterArguments(filter: Filter): StructuredQuery {
+ const [lowerArgument, upperArgument] = filter.arguments();
+ const newFilter = filter.setArguments([upperArgument, lowerArgument]);
+ return filter.replace(newFilter);
+}
+
+const isValidArgument = (arg: number | null | undefined) =>
+ !(arg === null || arg === undefined);
+
+const countValidArumgents = (filter: Filter) =>
+ filter
+ .arguments()
+ .reduce((count, arg) => (isValidArgument(arg) ? count + 1 : count), 0);
+
+export const hasBackwardsArguments = (filter: Filter) => {
+ const [lowerArgument, upperArgument] = filter.arguments();
+ return lowerArgument > upperArgument;
+};
diff --git a/frontend/src/metabase/query_builder/components/filters/modals/BulkFilterModal/utils.unit.spec.ts b/frontend/src/metabase/query_builder/components/filters/modals/BulkFilterModal/utils.unit.spec.ts
new file mode 100644
index 0000000000000000000000000000000000000000..53ed7f2cf884c5ddd6fa2f0e62e691ad88e7b412
--- /dev/null
+++ b/frontend/src/metabase/query_builder/components/filters/modals/BulkFilterModal/utils.unit.spec.ts
@@ -0,0 +1,172 @@
+import { SAMPLE_DATABASE, ORDERS } from "__support__/sample_database_fixture";
+
+import StructuredQuery from "metabase-lib/lib/queries/StructuredQuery";
+import Filter from "metabase-lib/lib/queries/structured/Filter";
+
+import {
+ fixBetweens,
+ hasBackwardsArguments,
+ swapFilterArguments,
+ handleEmptyBetween,
+} from "./utils";
+
+const makeQuery = (query = {}): StructuredQuery => {
+ return new StructuredQuery(ORDERS.question(), {
+ type: "query",
+ database: SAMPLE_DATABASE?.id,
+ query: {
+ "source-table": ORDERS.id,
+ ...query,
+ },
+ });
+};
+
+const query = makeQuery();
+
+describe("BulkFilterModal utils", () => {
+ describe("hasBackwardsArguments", () => {
+ it("flags between filters with misordered arguments", () => {
+ const filter = new Filter([
+ "between",
+ ["field", ORDERS.fields[1].id, null],
+ 20,
+ 10,
+ ]);
+
+ const isBackwards = hasBackwardsArguments(filter);
+ expect(isBackwards).toBe(true);
+ });
+
+ it("hasBackwardsArguments identifies between filters with correctly ordered arguments", () => {
+ const filter = new Filter([
+ "between",
+ ["field", ORDERS.fields[1].id, null],
+ 20,
+ 100,
+ ]);
+
+ const isBackwards = hasBackwardsArguments(filter);
+ expect(isBackwards).toBe(false);
+ });
+ });
+
+ describe("swapFilterArguments", () => {
+ it("swaps arguments in a between filter", () => {
+ const filter = new Filter(
+ ["between", ["field", ORDERS.fields[1].id, null], 20, 10],
+ null,
+ query,
+ );
+ filter.add();
+
+ const newQuery = swapFilterArguments(filter);
+ const newFilter = newQuery.filters()[0];
+ expect(newFilter.arguments()).toEqual([10, 20]);
+ });
+ });
+
+ describe("handleEmptyBetween", () => {
+ it("replaces a between filter with an empty second argument with a >= filter", () => {
+ const filter = new Filter(
+ ["between", ["field", ORDERS.fields[1].id, null], 20, null],
+ null,
+ query,
+ );
+ const newQuery = handleEmptyBetween(filter);
+
+ const newFilter = newQuery.filters()[0];
+ expect(newFilter.arguments()).toEqual([20]);
+ expect(newFilter.operatorName()).toEqual(">=");
+ });
+
+ it("replaces a between filter with an empty first argument with a <= filter", () => {
+ const filter = new Filter(
+ ["between", ["field", ORDERS.fields[1].id, null], undefined, 30],
+ null,
+ query,
+ );
+ const newQuery = handleEmptyBetween(filter);
+
+ const newFilter = newQuery.filters()[0];
+ expect(newFilter.arguments()).toEqual([30]);
+ expect(newFilter.operatorName()).toEqual("<=");
+ });
+ });
+
+ describe("fixBetweens", () => {
+ it("handles empty between filters", () => {
+ const filter = new Filter(
+ ["between", ["field", ORDERS.fields[1].id, null], 30, null],
+ null,
+ query,
+ );
+ const newQuery = fixBetweens(filter.add());
+
+ const newFilter = newQuery.filters()[0];
+ expect(newFilter.arguments()).toEqual([30]);
+ expect(newFilter.operatorName()).toEqual(">=");
+ });
+
+ it("handles backwards between filters", () => {
+ const filter = new Filter(
+ ["between", ["field", ORDERS.fields[1].id, null], 30, 20],
+ null,
+ query,
+ );
+ const newQuery = fixBetweens(filter.add());
+
+ const newFilter = newQuery.filters()[0];
+ expect(newFilter.arguments()).toEqual([20, 30]);
+ expect(newFilter.operatorName()).toEqual("between");
+ });
+
+ it("ignores valid between filters", () => {
+ const filter = new Filter(
+ ["between", ["field", ORDERS.fields[1].id, null], 40, 50],
+ null,
+ query,
+ );
+ const newQuery = fixBetweens(filter.add());
+
+ const newFilter = newQuery.filters()[0];
+ expect(newFilter.arguments()).toEqual([40, 50]);
+ expect(newFilter.operatorName()).toEqual("between");
+ });
+
+ it("ignores non-betwee filters", () => {
+ const filter = new Filter(
+ ["=", ["field", ORDERS.fields[1].id, null], 40, 50],
+ null,
+ query,
+ );
+ const newQuery = fixBetweens(filter.add());
+
+ const newFilter = newQuery.filters()[0];
+ expect(newFilter.arguments()).toEqual([40, 50]);
+ expect(newFilter.operatorName()).toEqual("=");
+ });
+
+ it("handles multiple invalid between filters", () => {
+ const testQuery = makeQuery({
+ filter: [
+ "and",
+ ["between", ["field", ORDERS.fields[1].id, null], 80, 50],
+ ["between", ["field", ORDERS.fields[1].id, null], 30, null],
+ ["=", ["field", ORDERS.fields[1].id, null], 8, 9, 7],
+ ],
+ });
+
+ const fixedQuery = fixBetweens(testQuery);
+
+ const [newFilter1, newFilter2, newFilter3] = fixedQuery.filters();
+ expect(newFilter1.arguments()).toEqual([50, 80]);
+ expect(newFilter1.operatorName()).toEqual("between");
+
+ expect(newFilter2.arguments()).toEqual([30]);
+ expect(newFilter2.operatorName()).toEqual(">=");
+
+ expect(newFilter3.arguments()).toEqual([8, 9, 7]);
+ expect(newFilter3.operatorName()).toEqual("=");
+ });
+ });
+});
diff --git a/frontend/src/metabase/query_builder/components/filters/modals/InlineValuePicker/InlineValuePicker.styled.ts b/frontend/src/metabase/query_builder/components/filters/modals/InlineValuePicker/InlineValuePicker.styled.ts
index 835d858d9b9448e8a9c54f69d211522c38636245..b1ee1fcfb210fb1b576f990f1c7d11584978c2b6 100644
--- a/frontend/src/metabase/query_builder/components/filters/modals/InlineValuePicker/InlineValuePicker.styled.ts
+++ b/frontend/src/metabase/query_builder/components/filters/modals/InlineValuePicker/InlineValuePicker.styled.ts
@@ -4,7 +4,7 @@ import { color } from "metabase/lib/colors";
import OperatorSelectorComponent from "metabase/query_builder/components/filters/OperatorSelector";
import FieldValuesWidget from "metabase/components/FieldValuesWidget";
-import Input from "metabase/core/components/Input";
+import NumericInput from "metabase/core/components/NumericInput";
export const OperatorSelector = styled(OperatorSelectorComponent)`
margin-bottom: ${space(1)};
@@ -31,7 +31,7 @@ export const NumberSeparator = styled.span`
padding: 0 ${space(1)};
`;
-export const NumberInput = styled(Input)`
+export const NumberInput = styled(NumericInput)`
border-color: ${color("border")};
width: 10rem;
`;
diff --git a/frontend/src/metabase/query_builder/components/filters/modals/InlineValuePicker/InlineValuePicker.tsx b/frontend/src/metabase/query_builder/components/filters/modals/InlineValuePicker/InlineValuePicker.tsx
index 63e6371efa10bc6a71a7201d7ec7e4b5072902be..071ed3e123d2173bee9cf35c8f6ca6f0425bde01 100644
--- a/frontend/src/metabase/query_builder/components/filters/modals/InlineValuePicker/InlineValuePicker.tsx
+++ b/frontend/src/metabase/query_builder/components/filters/modals/InlineValuePicker/InlineValuePicker.tsx
@@ -100,14 +100,14 @@ function ValuesInput({
<NumberInput
placeholder={t`min`}
value={filterArguments[0] ?? ""}
- onChange={e => onChange([e.target.value, filterArguments[1]])}
+ onChange={val => onChange([val, filterArguments[1]])}
fullWidth
/>
<NumberSeparator>{t`and`}</NumberSeparator>
<NumberInput
placeholder={t`max`}
value={filterArguments[1] ?? ""}
- onChange={e => onChange([filterArguments[0], e.target.value])}
+ onChange={val => onChange([filterArguments[0], val])}
fullWidth
/>
</BetweenContainer>
diff --git a/frontend/test/metabase/scenarios/filters/filter-bulk.cy.spec.js b/frontend/test/metabase/scenarios/filters/filter-bulk.cy.spec.js
index fa37a669a798c4658ada8ba53af1f1df6ee4d6b0..473a61455eb178ad41ac7715245564295d177eb2 100644
--- a/frontend/test/metabase/scenarios/filters/filter-bulk.cy.spec.js
+++ b/frontend/test/metabase/scenarios/filters/filter-bulk.cy.spec.js
@@ -124,28 +124,14 @@ describe("scenarios > filters > bulk filtering", () => {
modal().within(() => {
cy.findByText("Summaries").click();
- cy.findByLabelText("Count").click();
- });
-
- popover().within(() => {
- cy.findByText("Equal to").click();
- });
- popover()
- .eq(1)
- .within(() => cy.findByText("Greater than").click());
+ cy.findByPlaceholderText("min").type("500");
- popover().within(() => {
- cy.findByPlaceholderText("Enter a number").type("500");
- cy.button("Add filter").click();
- });
-
- modal().within(() => {
cy.button("Apply").click();
cy.wait("@dataset");
});
- cy.findByText("Count is greater than 500").should("be.visible");
+ cy.findByText("Count is greater than or equal to 500").should("be.visible");
cy.findByText("Showing 21 rows").should("be.visible");
});
@@ -576,6 +562,58 @@ describe("scenarios > filters > bulk filtering", () => {
cy.findByText("Showing 2 rows").should("be.visible");
});
});
+
+ describe("number filters", () => {
+ beforeEach(() => {
+ visitQuestionAdhoc(productsQuestion);
+ openFilterModal();
+ });
+
+ it("applies a between filter", () => {
+ modal().within(() => {
+ cy.findByLabelText("Price").within(() => {
+ cy.findByPlaceholderText("min").type("50");
+ cy.findByPlaceholderText("max").type("80");
+ });
+ cy.button("Apply").click();
+ });
+ cy.findByText("Price between 50 80").should("be.visible");
+ cy.findByText("Showing 72 rows").should("be.visible");
+ });
+
+ it("applies a greater than filter", () => {
+ modal().within(() => {
+ cy.findByLabelText("Price").within(() => {
+ cy.findByTestId("select-button").click();
+ });
+ });
+
+ popover().within(() => {
+ cy.findByText("Greater than").click();
+ });
+
+ modal().within(() => {
+ cy.findByPlaceholderText("Enter a number").type("50");
+ cy.button("Apply").click();
+ });
+
+ cy.findByText("Price is greater than 50").should("be.visible");
+ cy.findByText("Showing 106 rows").should("be.visible");
+ });
+
+ it("infers a <= filter from an invalid between filter", () => {
+ modal().within(() => {
+ cy.findByLabelText("Price").within(() => {
+ cy.findByPlaceholderText("max").type("50");
+ });
+
+ cy.button("Apply").click();
+ });
+
+ cy.findByText("Price is less than or equal to 50").should("be.visible");
+ cy.findByText("Showing 94 rows").should("be.visible");
+ });
+ });
});
const modal = () => {