Allow multiple values for string operators in the dashboard (#42013)

* Expand the single or multi-selectable string filter choices * Mark operators as `multi` * Wrap an existing test in a separate `describe` block * Test multiple values settings * Expand repro to include multiple dashboard filter values * Ease the load by limiting the card results * Switch to the more flexible data structure for the dashboard filters * Expand the assertions * Fix typo Co-authored-by: Kamil Mielnik <kamil@kamilmielnik.com> --------- Co-authored-by: Kamil Mielnik <kamil@kamilmielnik.com>
metabase · May 14, 2024 · 1a44a83 · 1a44a83
1 parent 50e64ab
commit 1a44a83
Show file tree

Hide file tree

Showing 5 changed files with 174 additions and 54 deletions.
diff --git a/e2e/test/scenarios/dashboard-filters/dashboard-filters-text-category.cy.spec.js b/e2e/test/scenarios/dashboard-filters/dashboard-filters-text-category.cy.spec.js
@@ -1,7 +1,4 @@
-import {
-  ORDERS_DASHBOARD_ID,
-  ORDERS_DASHBOARD_DASHCARD_ID,
-} from "e2e/support/cypress_sample_instance_data";
+import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
 import {
   restore,
   popover,
@@ -27,39 +24,67 @@ import {
 
 import { DASHBOARD_TEXT_FILTERS } from "./shared/dashboard-filters-text-category";
 
+const { ORDERS_ID } = SAMPLE_DATABASE;
+
 describe("scenarios > dashboard > filters > text/category", () => {
   beforeEach(() => {
     restore();
     cy.signInAsAdmin();
 
-    visitDashboard(ORDERS_DASHBOARD_ID);
-
-    editDashboard();
+    cy.createQuestionAndDashboard({
+      questionDetails: {
+        query: { "source-table": ORDERS_ID, limit: 5 },
+      },
+      cardDetails: {
+        size_x: 24,
+        size_y: 8,
+      },
+    }).then(({ body: { id, dashboard_id } }) => {
+      cy.wrap(id).as("dashCardId");
+      visitDashboard(dashboard_id);
+      editDashboard();
+    });
   });
 
   it("should work when set through the filter widget", () => {
-    Object.entries(DASHBOARD_TEXT_FILTERS).forEach(([filter]) => {
-      cy.log(`Make sure we can connect ${filter} filter`);
-      setFilter("Text or Category", filter);
+    DASHBOARD_TEXT_FILTERS.forEach(({ operator, single }) => {
+      cy.log(`Make sure we can connect ${operator} filter`);
+      setFilter("Text or Category", operator);
+      cy.findAllByRole("radio", { name: "Multiple values" }).should(
+        "be.checked",
+      );
+
+      if (single) {
+        cy.findAllByRole("radio", { name: "A single value" })
+          .click()
+          .should("be.checked");
+      }
 
       cy.findByText("Select…").click();
       popover().contains("Source").click();
     });
-
     saveDashboard();
+    waitDashboardCardQuery();
 
-    Object.entries(DASHBOARD_TEXT_FILTERS).forEach(
-      ([filter, { value, representativeResult }], index) => {
+    DASHBOARD_TEXT_FILTERS.forEach(
+      (
+        { operator, value, representativeResult, single, negativeAssertion },
+        index,
+      ) => {
         filterWidget().eq(index).click();
-        applyFilterByType(filter, value);
+        applyFilterByType(operator, value);
+        waitDashboardCardQuery();
+        filterWidget()
+          .eq(index)
+          .contains(single ? value : /\d selections/);
 
-        cy.log(`Make sure ${filter} filter returns correct result`);
-        cy.findByTestId("dashcard").within(() => {
-          cy.contains(representativeResult);
-        });
+        cy.log(`Make sure ${operator} filter returns correct result`);
+        cy.findByTestId("dashcard")
+          .should("contain", representativeResult)
+          .and("not.contain", negativeAssertion);
 
         clearFilterWidget(index);
-        cy.wait(`@dashcardQuery${ORDERS_DASHBOARD_DASHCARD_ID}`);
+        waitDashboardCardQuery();
       },
     );
   });
@@ -75,16 +100,19 @@ describe("scenarios > dashboard > filters > text/category", () => {
     popover().contains("Source").click();
 
     saveDashboard();
-    filterWidget().click();
+    waitDashboardCardQuery();
 
+    filterWidget().click();
     applyFilterByType(filterType, filterValue);
+    waitDashboardCardQuery();
 
     filterWidget().click();
     cy.log("uncheck all values");
 
     popover().within(() => {
       cy.findByText(filterValue).click();
       cy.button("Update filter").click();
+      waitDashboardCardQuery();
     });
 
     filterWidget().within(() => {
@@ -111,29 +139,27 @@ describe("scenarios > dashboard > filters > text/category", () => {
     popover().contains("User ID").click();
 
     saveDashboard();
-    cy.wait(`@dashcardQuery${ORDERS_DASHBOARD_DASHCARD_ID}`);
+    waitDashboardCardQuery();
 
     cy.location("search").should("eq", "?text=Organic&id=");
-    cy.findByTestId("dashcard").within(() => {
-      cy.contains("39.58");
-    });
+    cy.findByTestId("dashcard").contains("39.58");
 
     // This part reproduces metabase#13960
     // Remove default filter (category)
     cy.get("fieldset .Icon-close").click();
-    cy.wait(`@dashcardQuery${ORDERS_DASHBOARD_DASHCARD_ID}`);
+    waitDashboardCardQuery();
 
     cy.location("search").should("eq", "?text=&id=");
 
     filterWidget().contains("ID").click();
     cy.findByPlaceholderText("Enter an ID").type("4{enter}").blur();
     cy.button("Add filter").click();
-    cy.wait(`@dashcardQuery${ORDERS_DASHBOARD_DASHCARD_ID}`);
+    waitDashboardCardQuery();
 
     cy.location("search").should("eq", "?text=&id=4");
 
     cy.reload();
-    cy.wait(`@dashcardQuery${ORDERS_DASHBOARD_DASHCARD_ID}`);
+    waitDashboardCardQuery();
 
     cy.location("search").should("eq", "?text=&id=4");
     filterWidget().contains("Text");
@@ -164,13 +190,16 @@ describe("scenarios > dashboard > filters > text/category", () => {
     // Updates the filter value
     selectDefaultValueFromPopover("Twitter", { buttonLabel: "Update filter" });
     saveDashboard();
+    waitDashboardCardQuery();
     ensureDashboardCardHasText("37.65");
 
     // Resets the value back by clicking widget icon
     toggleFilterWidgetValues(["Google", "Organic"], {
       buttonLabel: "Update filter",
     });
+    waitDashboardCardQuery();
     resetFilterWidgetToDefault();
+    waitDashboardCardQuery();
     filterWidget().findByText("Twitter");
 
     // Removing value resets back to default
@@ -180,3 +209,9 @@ describe("scenarios > dashboard > filters > text/category", () => {
     filterWidget().findByText("Twitter");
   });
 });
+
+function waitDashboardCardQuery() {
+  cy.get("@dashCardId").then(id => {
+    cy.wait(`@dashcardQuery${id}`);
+  });
+}
diff --git a/e2e/test/scenarios/dashboard-filters/shared/dashboard-filters-text-category.js b/e2e/test/scenarios/dashboard-filters/shared/dashboard-filters-text-category.js
@@ -1,26 +1,67 @@
-export const DASHBOARD_TEXT_FILTERS = {
-  Is: {
+export const DASHBOARD_TEXT_FILTERS = [
+  {
+    operator: "Is",
+    single: true,
     value: "Organic",
     representativeResult: "39.58",
   },
-  "Is not": {
+  {
+    operator: "Is not",
+    single: true,
     value: "Organic",
     representativeResult: "37.65",
   },
-  Contains: {
-    value: "oo",
+  // It is important to keep multiple values as a single string in the value field.
+  {
+    operator: "Contains",
+    value: "oo,aa",
     representativeResult: "148.23",
+    negativeAssertion: "37.65",
   },
-  "Does not contain": {
-    value: "oo",
+  {
+    operator: "Contains",
+    single: true,
+    value: "oo,aa",
+    representativeResult: "No results!",
+    negativeAssertion: "148.23",
+  },
+  {
+    operator: "Does not contain",
+    value: "oo,tt",
+    representativeResult: "39.58",
+    negativeAssertion: "37.65",
+  },
+  {
+    operator: "Does not contain",
+    single: true,
+    value: "oo,tt",
     representativeResult: "37.65",
+    negativeASsertion: "39.58",
   },
-  "Starts with": {
-    value: "A",
+  {
+    operator: "Starts with",
+    value: "A,b",
     representativeResult: "85.72",
+    negativeASsertion: "70.15",
   },
-  "Ends with": {
-    value: "e",
+  {
+    operator: "Starts with",
+    single: true,
+    value: "A,b",
+    representativeResult: "No results!",
+    negativeASsertion: "85.72",
+  },
+  {
+    operator: "Ends with",
+    value: "e,s",
     representativeResult: "47.68",
+    negativeASsertion: "127.88",
+  },
+  {
+    operator: "Ends with",
+    single: true,
+    value: "e,s",
+    representativeResult: "No results!",
+    negativeASsertion: "47.68",
   },
-};
+];
diff --git a/frontend/src/metabase-lib/v1/operators/constants.js b/frontend/src/metabase-lib/v1/operators/constants.js
@@ -169,21 +169,25 @@ export const FIELD_FILTER_OPERATORS = {
     validArgumentsFilters: [comparableArgument, comparableArgument],
   },
   "starts-with": {
+    multi: true,
     validArgumentsFilters: [freeformArgument],
     options: CASE_SENSITIVE_OPTION,
     optionsDefaults: { "case-sensitive": false },
   },
   "ends-with": {
+    multi: true,
     validArgumentsFilters: [freeformArgument],
     options: CASE_SENSITIVE_OPTION,
     optionsDefaults: { "case-sensitive": false },
   },
   contains: {
+    multi: true,
     validArgumentsFilters: [freeformArgument],
     options: CASE_SENSITIVE_OPTION,
     optionsDefaults: { "case-sensitive": false },
   },
   "does-not-contain": {
+    multi: true,
     validArgumentsFilters: [freeformArgument],
     options: CASE_SENSITIVE_OPTION,
     optionsDefaults: { "case-sensitive": false },

diff --git a/frontend/src/metabase-lib/v1/parameters/constants.ts b/frontend/src/metabase-lib/v1/parameters/constants.ts
@@ -138,7 +138,14 @@ export const SINGLE_OR_MULTI_SELECTABLE_TYPES: Record<
   string,
   string | string[]
 > = {
-  string: ["=", "!="],
+  string: [
+    "=",
+    "!=",
+    "contains",
+    "does-not-contain",
+    "starts-with",
+    "ends-with",
+  ],
   category: "any",
   id: "any",
   location: ["=", "!="],

diff --git a/...tend/src/metabase/parameters/components/ParameterSettings/ParameterSettings.unit.spec.tsx b/...tend/src/metabase/parameters/components/ParameterSettings/ParameterSettings.unit.spec.tsx
@@ -137,22 +137,54 @@ describe("ParameterSidebar", () => {
   });
 
   describe("string", () => {
-    beforeEach(() => {
-      setup({
-        parameter: createMockUiParameter({
-          type: "string/=",
-          sectionId: "string",
-        }),
+    describe("smoke test", () => {
+      beforeEach(() => {
+        setup({
+          parameter: createMockUiParameter({
+            type: "string/=",
+            sectionId: "string",
+          }),
+        });
       });
-    });
 
-    it("should render type", () => {
-      expect(screen.getByDisplayValue("Text or Category")).toBeInTheDocument();
-    });
+      it("should render type", () => {
+        expect(
+          screen.getByDisplayValue("Text or Category"),
+        ).toBeInTheDocument();
+      });
 
-    it("should render operator", () => {
-      expect(screen.getByDisplayValue("Is")).toBeInTheDocument();
+      it("should render operator", () => {
+        expect(screen.getByDisplayValue("Is")).toBeInTheDocument();
+      });
     });
+
+    it.each([
+      "string/=",
+      "string/!=",
+      "string/contains",
+      "string/does-not-contain",
+      "string/starts-with",
+      "string/ends-with",
+    ])(
+      "should be able to toggle multiple values settings for `%s` operator",
+      async type => {
+        const { onChangeIsMultiSelect } = setup({
+          parameter: createMockUiParameter({
+            type,
+            sectionId: "string",
+          }),
+        });
+
+        expect(screen.getByText("People can pick")).toBeInTheDocument();
+        expect(
+          screen.getByRole("radio", { name: "Multiple values" }),
+        ).toBeChecked();
+        await userEvent.click(
+          screen.getByRole("radio", { name: "A single value" }),
+        );
+        expect(onChangeIsMultiSelect).toHaveBeenCalledWith(false);
+      },
+    );
   });
 
   describe("date", () => {
@@ -197,6 +229,7 @@ describe("ParameterSidebar", () => {
 const setup = ({ parameter = createMockUiParameter() }: SetupOpts = {}) => {
   const onChangeQueryType = jest.fn();
   const onChangeName = jest.fn();
+  const onChangeIsMultiSelect = jest.fn();
 
   renderWithProviders(
     <ParameterSettings
@@ -206,7 +239,7 @@ const setup = ({ parameter = createMockUiParameter() }: SetupOpts = {}) => {
       onChangeName={onChangeName}
       onChangeType={jest.fn()}
       onChangeDefaultValue={jest.fn()}
-      onChangeIsMultiSelect={jest.fn()}
+      onChangeIsMultiSelect={onChangeIsMultiSelect}
       onChangeQueryType={onChangeQueryType}
       onChangeSourceType={jest.fn()}
       onChangeSourceConfig={jest.fn()}
@@ -215,5 +248,5 @@ const setup = ({ parameter = createMockUiParameter() }: SetupOpts = {}) => {
     />,
   );
 
-  return { onChangeQueryType, onChangeName };
+  return { onChangeQueryType, onChangeName, onChangeIsMultiSelect };
 };