Better Handle Filter Operators in FieldValuesPicker (#23520)

* correctly handle multi inputs

frontend/src/metabase/query_builder/components/filters/modals/BulkFilterList/BulkFilterList.styled.tsx

@@ -9,7 +9,7 @@ export const ListRoot = styled.div`
 
 export const ListRow = styled.div`
   display: grid;
-  grid-template-columns: 1fr 2fr;
+  grid-template-columns: minmax(0, 1fr) minmax(0, 2fr);
   padding: 0.375rem 0;
 `;
 

frontend/src/metabase/query_builder/components/filters/modals/InlineValuePicker/InlineValuePicker.styled.ts

 import styled from "@emotion/styled";
 import { space } from "metabase/styled-components/theme";
+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";
 
 export const OperatorSelector = styled(OperatorSelectorComponent)`
   margin-bottom: ${space(1)};
@@ -15,3 +17,21 @@ export const ArgumentSelector = styled(FieldValuesWidget)`
 export const ValuesPickerContainer = styled.div`
   grid-column: 2;
 `;
+
+export const BetweenContainer = styled.div`
+  display: flex;
+  width: 100%;
+  align-items: center;
+  justify-content: space-between;
+`;
+
+export const NumberSeparator = styled.span`
+  color: ${color("text-light")};
+  font-weight: bold;
+  padding: 0 ${space(1)};
+`;
+
+export const NumberInput = styled(Input)`
+  border-color: ${color("border")};
+  width: 10rem;
+`;

frontend/src/metabase/query_builder/components/filters/modals/InlineValuePicker/InlineValuePicker.tsx

 import React, { useCallback, useMemo } from "react";
 import Filter from "metabase-lib/lib/queries/structured/Filter";
 import Field from "metabase-lib/lib/metadata/Field";
+import { t } from "ttag";
 
 import {
   OperatorSelector,
   ArgumentSelector,
   ValuesPickerContainer,
+  BetweenContainer,
+  NumberInput,
+  NumberSeparator,
 } from "./InlineValuePicker.styled";
 
 interface InlineValuePickerProps {
@@ -27,7 +31,7 @@ export function InlineValuePicker({
   );
 
   const changeArguments = useCallback(
-    (newArguments: number[]) => {
+    (newArguments: (string | number)[]) => {
       handleChange(filter.setArguments(newArguments));
     },
     [filter, handleChange],
@@ -35,6 +39,13 @@ export function InlineValuePicker({
 
   const filterOperators = field.filterOperators(filter.operatorName());
 
+  const hideArgumentSelector = [
+    "is-null",
+    "not-null",
+    "is-empty",
+    "not-empty",
+  ].includes(filter.operatorName());
+
   return (
     <ValuesPickerContainer
       data-testid="value-picker"
@@ -45,16 +56,60 @@ export function InlineValuePicker({
         operators={filterOperators}
         onOperatorChange={changeOperator}
       />
+      {!hideArgumentSelector && (
+        <ValuesInput filter={filter} field={field} onChange={changeArguments} />
+      )}
+    </ValuesPickerContainer>
+  );
+}
+
+interface ValuesInputTypes {
+  onChange: (newArguments: (string | number)[]) => void;
+  filter: Filter;
+  field: Field;
+}
+
+function ValuesInput({
+  onChange,
+  field,
+  filter,
+}: ValuesInputTypes): JSX.Element {
+  const isBetween =
+    filter.operatorName() === "between" &&
+    filter?.operator()?.fields.length === 2;
+
+  const filterArguments = filter.arguments() ?? [];
+
+  if (!isBetween) {
+    return (
       <ArgumentSelector
         // eslint-disable-next-line @typescript-eslint/ban-ts-comment
         // @ts-ignore: this component doesn't have types or propTypes
-        value={filter.arguments()}
-        onChange={changeArguments}
+        value={filterArguments}
+        onChange={onChange}
         className="input"
         fields={[field]}
+        multi={!!filter?.operator()?.multi}
         showOptionsInPopover
-        multi
       />
-    </ValuesPickerContainer>
+    );
+  }
+
+  return (
+    <BetweenContainer>
+      <NumberInput
+        placeholder={t`min`}
+        value={filterArguments[0] ?? ""}
+        onChange={e => onChange([e.target.value, filterArguments[1]])}
+        fullWidth
+      />
+      <NumberSeparator>{t`and`}</NumberSeparator>
+      <NumberInput
+        placeholder={t`max`}
+        value={filterArguments[1] ?? ""}
+        onChange={e => onChange([filterArguments[0], e.target.value])}
+        fullWidth
+      />
+    </BetweenContainer>
   );
 }

frontend/src/metabase/query_builder/components/filters/modals/InlineValuePicker/InlineValuePicker.unit.spec.tsx

@@ -127,7 +127,7 @@ describe("InlineValuePicker", () => {
 
   it("loads an existing set of text filter values", async () => {
     const testFilter = new Filter(
-      ["contains", ["field", textField.id, null], "fooBarBaz", "BazBarFoo"],
+      ["!=", ["field", textField.id, null], "fooBarBaz", "BazBarFoo"],
       null,
       query,
     );
@@ -142,7 +142,7 @@ describe("InlineValuePicker", () => {
         />
       </Provider>,
     );
-    screen.getByText("Contains");
+    screen.getByText("Is not");
     screen.getByText("fooBarBaz");
     screen.getByText("BazBarFoo");
   });
@@ -223,4 +223,106 @@ describe("InlineValuePicker", () => {
     expect(changeSpy).toHaveBeenCalled();
     expect(changeSpy.mock.calls[0][0].arguments()).toEqual([888]);
   });
+
+  it("tokenizes inputs for multi-input operators", async () => {
+    let testFilter = new Filter(
+      ["=", ["field", textField.id, null]],
+      null,
+      query,
+    );
+    const changeSpy = jest.fn(newFilter => (testFilter = newFilter));
+
+    render(
+      <Provider store={store}>
+        <InlineValuePicker
+          filter={testFilter}
+          field={textField}
+          handleChange={changeSpy}
+        />
+      </Provider>,
+    );
+
+    const textInput = screen.getByPlaceholderText("Enter some text");
+    userEvent.type(textInput, "foo,bar,");
+    changeSpy.mock.calls.forEach(([[_, __, value]]) => {
+      // passed value will never contain a comma because the tokenizer will filter it out
+      expect(value).not.toContain(",");
+    });
+  });
+
+  it("does not tokenize input for single-input operators", async () => {
+    const testFilter = new Filter(
+      ["contains", ["field", textField.id, null]],
+      null,
+      query,
+    );
+    const changeSpy = jest.fn();
+
+    render(
+      <Provider store={store}>
+        <InlineValuePicker
+          filter={testFilter}
+          field={textField}
+          handleChange={changeSpy}
+        />
+      </Provider>,
+    );
+
+    const textInput = screen.getByPlaceholderText("Enter some text");
+    userEvent.type(textInput, "foo,bar,");
+
+    const lastCall = changeSpy.mock.calls[changeSpy.mock.calls.length - 1][0];
+    // reads commas as part of the input instead of token separators
+    expect(lastCall[2]).toEqual("foo,bar,");
+  });
+
+  it("shows multiple inputs for between filters", async () => {
+    const testFilter = new Filter(
+      ["between", ["field", pkField.id, null], 14, 74],
+      null,
+      query,
+    );
+    const changeSpy = jest.fn();
+
+    render(
+      <Provider store={store}>
+        <InlineValuePicker
+          filter={testFilter}
+          field={pkField}
+          handleChange={changeSpy}
+        />
+      </Provider>,
+    );
+
+    screen.getByText("Between");
+    screen.getByPlaceholderText("min");
+    screen.getByPlaceholderText("max");
+  });
+
+  const noValueOperators = ["is-null", "not-null", "is-empty", "not-empty"];
+
+  noValueOperators.forEach(op => {
+    it(`hides value input for ${op} empty operator`, () => {
+      const testFilter = new Filter(
+        [op, ["field", textField.id, null]],
+        null,
+        query,
+      );
+      const changeSpy = jest.fn();
+
+      render(
+        <Provider store={store}>
+          <InlineValuePicker
+            filter={testFilter}
+            field={textField}
+            handleChange={changeSpy}
+          />
+        </Provider>,
+      );
+
+      expect(
+        screen.queryByPlaceholderText("Enter some text"),
+      ).not.toBeInTheDocument();
+    });
+  });
 });

frontend/test/metabase/scenarios/filters/filter-bulk.cy.spec.js

@@ -547,6 +547,29 @@ describe("scenarios > filters > bulk filtering", () => {
       });
       cy.findByText("Showing 12 rows").should("be.visible");
     });
+
+    it("adds multiple is text filters", () => {
+      modal().within(() => {
+        cy.findByLabelText("Title").within(() => {
+          cy.findByText("Contains").click();
+        });
+      });
+
+      popover().within(() => {
+        cy.findByText("Is").click();
+      });
+
+      modal().within(() => {
+        cy.findByLabelText("Title").within(() => {
+          cy.findByPlaceholderText("Search by Title").type(
+            "Small Marble Shoes,Rustic Paper Wallet",
+          );
+        });
+        cy.button("Apply").click();
+      });
+      cy.findByText("Title is 2 selections").should("be.visible");
+      cy.findByText("Showing 2 rows").should("be.visible");
+    });
   });
 });