From decc8b7d5733944e5e11e8822e88f9b6ccdd9a4c Mon Sep 17 00:00:00 2001
From: Alexander Polyankin <alexander.polyankin@metabase.com>
Date: Wed, 15 Mar 2023 18:51:08 +0200
Subject: [PATCH] Fix quick filter drill with nulls (#29190)
---
.../29082-quick-filter-null.cy.spec.js | 47 ++++++++
.../queries/drills/quick-filter-drill.js | 110 +++++++++---------
.../components/drill/QuickFilterDrill.tsx | 6 +-
.../drill/QuickFilterDrill.unit.spec.js | 84 +++++++++----
4 files changed, 169 insertions(+), 78 deletions(-)
create mode 100644 e2e/test/scenarios/question/reproductions/29082-quick-filter-null.cy.spec.js
diff --git a/e2e/test/scenarios/question/reproductions/29082-quick-filter-null.cy.spec.js b/e2e/test/scenarios/question/reproductions/29082-quick-filter-null.cy.spec.js
new file mode 100644
index 00000000000..2e0d8f89625
--- /dev/null
+++ b/e2e/test/scenarios/question/reproductions/29082-quick-filter-null.cy.spec.js
@@ -0,0 +1,47 @@
+import { popover, restore, visitQuestionAdhoc } from "e2e/support/helpers";
+import { SAMPLE_DB_ID } from "e2e/support/cypress_data";
+import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
+
+const { ORDERS_ID, ORDERS } = SAMPLE_DATABASE;
+
+const questionDetails = {
+ name: "22788",
+ dataset_query: {
+ type: "query",
+ database: SAMPLE_DB_ID,
+ query: {
+ "source-table": ORDERS_ID,
+ filter: ["=", ["field", ORDERS.USER_ID, null], 1],
+ },
+ },
+};
+
+describe("issue 29082", () => {
+ beforeEach(() => {
+ restore();
+ cy.signInAsNormalUser();
+ cy.intercept("POST", "/api/dataset").as("dataset");
+ });
+
+ it("should handle nulls in quick filters (metabase#29082)", () => {
+ visitQuestionAdhoc(questionDetails);
+ cy.wait("@dataset");
+ cy.findByText("Showing 11 rows").should("exist");
+
+ cy.get(".TableInteractive-emptyCell").first().click();
+ popover().within(() => cy.findByText("=").click());
+ cy.wait("@dataset");
+ cy.findByText("Showing 8 rows").should("exist");
+ cy.findByText("Discount is empty").should("exist");
+
+ cy.findByText("Discount is empty").within(() => cy.icon("close").click());
+ cy.wait("@dataset");
+ cy.findByText("Showing 11 rows").should("exist");
+
+ cy.get(".TableInteractive-emptyCell").first().click();
+ popover().within(() => cy.findByText("≠").click());
+ cy.wait("@dataset");
+ cy.findByText("Showing 3 rows").should("exist");
+ cy.findByText("Discount is not empty").should("exist");
+ });
+});
diff --git a/frontend/src/metabase-lib/queries/drills/quick-filter-drill.js b/frontend/src/metabase-lib/queries/drills/quick-filter-drill.js
index 4f02affe2ff..8505973cbbf 100644
--- a/frontend/src/metabase-lib/queries/drills/quick-filter-drill.js
+++ b/frontend/src/metabase-lib/queries/drills/quick-filter-drill.js
@@ -1,12 +1,13 @@
import {
isa,
- isTypeFK,
- isTypePK,
isDate,
isNumeric,
+ isTypeFK,
+ isTypePK,
} from "metabase-lib/types/utils/isa";
import { TYPE } from "metabase-lib/types/constants";
import { isLocalField } from "metabase-lib/queries/utils";
+import { fieldRefForColumn } from "metabase-lib/queries/utils/dataset";
const INVALID_TYPES = [TYPE.Structured];
@@ -22,73 +23,78 @@ export function quickFilterDrill({ question, clicked }) {
return null;
}
- const { column } = clicked;
+ const { column, value } = clicked;
if (isTypePK(column.semantic_type) || isTypeFK(column.semantic_type)) {
return null;
}
- const operators = getOperatorsForColumn(column);
+ const operators = getOperatorsForColumn(column, value);
return { operators };
}
-export function quickFilterDrillQuestion({ question, clicked, operator }) {
- const { column, value } = clicked;
+export function quickFilterDrillQuestion({ question, clicked, filter }) {
+ const { column } = clicked;
- if (isLocalField(column.field_ref)) {
- return question.filter(operator, column, value);
+ if (isLocalColumn(column)) {
+ return question.query().filter(filter).question();
+ } else {
+ /**
+ * For aggregated and custom columns
+ * with field refs like ["aggregation", 0],
+ * we need to nest the query as filters like ["=", ["aggregation", 0], value] won't work
+ *
+ * So the query like
+ * {
+ * aggregations: [["count"]]
+ * source-table: 2,
+ * }
+ *
+ * Becomes
+ * {
+ * source-query: {
+ * aggregations: [["count"]]
+ * source-table: 2,
+ * },
+ * filter: ["=", [ "field", "count", {"base-type": "type/BigInteger"} ], value]
+ * }
+ */
+ return question.query().nest().filter(filter).question();
}
-
- /**
- * For aggregated and custom columns
- * with field refs like ["aggregation", 0],
- * we need to nest the query as filters like ["=", ["aggregation", 0], value] won't work
- *
- * So the query like
- * {
- * aggregations: [["count"]]
- * source-table: 2,
- * }
- *
- * Becomes
- * {
- * source-query: {
- * aggregations: [["count"]]
- * source-table: 2,
- * },
- * filter: ["=", [ "field", "count", {"base-type": "type/BigInteger"} ], value]
- * }
- */
-
- const nestedQuestion = question.query().nest().question();
-
- return nestedQuestion.filter(
- operator,
- {
- ...column,
- field_ref: getFieldLiteralFromColumn(column),
- },
- value,
- );
}
-function getOperatorsForColumn(column) {
- if (isNumeric(column) || isDate(column)) {
+function getOperatorsForColumn(column, value) {
+ const fieldRef = getColumnFieldRef(column);
+
+ if (INVALID_TYPES.some(type => isa(column.base_type, type))) {
+ return [];
+ } else if (value == null) {
return [
- { name: "<", operator: "<" },
- { name: ">", operator: ">" },
- { name: "=", operator: "=" },
- { name: "≠", operator: "!=" },
+ { name: "=", filter: ["is-null", fieldRef] },
+ { name: "≠", filter: ["not-null", fieldRef] },
];
- } else if (!INVALID_TYPES.some(type => isa(column.base_type, type))) {
+ } else if (isNumeric(column) || isDate(column)) {
return [
- { name: "=", operator: "=" },
- { name: "≠", operator: "!=" },
+ { name: "<", filter: ["<", fieldRef, value] },
+ { name: ">", filter: [">", fieldRef, value] },
+ { name: "=", filter: ["=", fieldRef, value] },
+ { name: "≠", filter: ["!=", fieldRef, value] },
];
} else {
- return [];
+ return [
+ { name: "=", filter: ["=", fieldRef, value] },
+ { name: "≠", filter: ["!=", fieldRef, value] },
+ ];
}
}
-function getFieldLiteralFromColumn(column) {
- return ["field", column.name, { "base-type": column.base_type }];
+function isLocalColumn(column) {
+ return isLocalField(column.field_ref);
+}
+
+function getColumnFieldRef(column) {
+ if (isLocalColumn(column)) {
+ return fieldRefForColumn(column);
+ } else {
+ return ["field", column.name, { "base-type": column.base_type }];
+ }
}
diff --git a/frontend/src/metabase/modes/components/drill/QuickFilterDrill.tsx b/frontend/src/metabase/modes/components/drill/QuickFilterDrill.tsx
index 1f4053c17f4..2a04793081a 100644
--- a/frontend/src/metabase/modes/components/drill/QuickFilterDrill.tsx
+++ b/frontend/src/metabase/modes/components/drill/QuickFilterDrill.tsx
@@ -11,12 +11,12 @@ const QuickFilterDrill: Drill = ({ question, clicked }) => {
return [];
}
- return drill.operators.map(({ name, operator }) => ({
- name: operator,
+ return drill.operators.map(({ name, filter }) => ({
+ name,
title: <span className="h2">{name}</span>,
section: "filter",
buttonType: "token-filter",
- question: () => quickFilterDrillQuestion({ question, clicked, operator }),
+ question: () => quickFilterDrillQuestion({ question, clicked, filter }),
}));
};
diff --git a/frontend/test/metabase/modes/components/drill/QuickFilterDrill.unit.spec.js b/frontend/test/metabase/modes/components/drill/QuickFilterDrill.unit.spec.js
index 94a01e4db93..026c975c638 100644
--- a/frontend/test/metabase/modes/components/drill/QuickFilterDrill.unit.spec.js
+++ b/frontend/test/metabase/modes/components/drill/QuickFilterDrill.unit.spec.js
@@ -8,8 +8,22 @@ import {
} from "__support__/sample_database_fixture";
import Question from "metabase-lib/Question";
-const NUMBER_AND_DATE_FILTERS = ["<", ">", "=", "!="];
-const OTHER_FILTERS = ["=", "!="];
+const NUMBER_AND_DATE_FILTERS = [
+ { name: "<", operator: "<" },
+ { name: ">", operator: ">" },
+ { name: "=", operator: "=" },
+ { name: "≠", operator: "!=" },
+];
+
+const NULL_FILTERS = [
+ { name: "=", operator: "is-null" },
+ { name: "≠", operator: "not-null" },
+];
+
+const OTHER_FILTERS = [
+ { name: "=", operator: "=" },
+ { name: "≠", operator: "!=" },
+];
const DEFAULT_NUMERIC_CELL_VALUE = 42;
@@ -111,14 +125,14 @@ describe("QuickFilterDrill", () => {
const { actions } = setup({ column: clickedField.column() });
it("should return correct filters", () => {
- const filters = NUMBER_AND_DATE_FILTERS.map(operator => ({
- name: operator,
+ const filters = NUMBER_AND_DATE_FILTERS.map(({ name }) => ({
+ name,
}));
expect(actions).toMatchObject(filters);
});
actions.forEach((action, i) => {
- const operator = NUMBER_AND_DATE_FILTERS[i];
+ const { operator } = NUMBER_AND_DATE_FILTERS[i];
it(`should correctly apply "${operator}" filter`, () => {
const question = action.question();
expect(question.datasetQuery().query).toEqual({
@@ -141,14 +155,14 @@ describe("QuickFilterDrill", () => {
});
it("should return correct filters", () => {
- const filters = NUMBER_AND_DATE_FILTERS.map(operator => ({
- name: operator,
+ const filters = NUMBER_AND_DATE_FILTERS.map(({ name }) => ({
+ name,
}));
expect(actions).toMatchObject(filters);
});
actions.forEach((action, i) => {
- const operator = NUMBER_AND_DATE_FILTERS[i];
+ const { operator } = NUMBER_AND_DATE_FILTERS[i];
it(`should correctly apply "${operator}" filter`, () => {
const question = action.question();
expect(question.datasetQuery().query).toEqual({
@@ -173,14 +187,14 @@ describe("QuickFilterDrill", () => {
});
it("should return correct filters", () => {
- const filters = NUMBER_AND_DATE_FILTERS.map(operator => ({
- name: operator,
+ const filters = NUMBER_AND_DATE_FILTERS.map(({ name }) => ({
+ name,
}));
expect(actions).toMatchObject(filters);
});
actions.forEach((action, i) => {
- const operator = NUMBER_AND_DATE_FILTERS[i];
+ const { operator } = NUMBER_AND_DATE_FILTERS[i];
it(`should correctly apply "${operator}" filter`, () => {
const question = action.question();
expect(question.datasetQuery().query).toEqual({
@@ -213,14 +227,14 @@ describe("QuickFilterDrill", () => {
});
it("should return correct filters", () => {
- const filters = NUMBER_AND_DATE_FILTERS.map(operator => ({
- name: operator,
+ const filters = NUMBER_AND_DATE_FILTERS.map(({ name }) => ({
+ name,
}));
expect(actions).toMatchObject(filters);
});
actions.forEach((action, i) => {
- const operator = NUMBER_AND_DATE_FILTERS[i];
+ const { operator } = NUMBER_AND_DATE_FILTERS[i];
it(`should correctly apply "${operator}" filter`, () => {
const question = action.question();
expect(question.datasetQuery().query).toEqual({
@@ -232,6 +246,30 @@ describe("QuickFilterDrill", () => {
});
});
+ describe("numeric cells with null values", () => {
+ const clickedField = ORDERS.TOTAL;
+ const { actions } = setup({ column: clickedField.column(), value: null });
+
+ it("should return correct filters", () => {
+ const filters = NULL_FILTERS.map(({ name }) => ({
+ name,
+ }));
+ expect(actions).toMatchObject(filters);
+ });
+
+ actions.forEach((action, i) => {
+ const { operator } = NULL_FILTERS[i];
+ it(`should correctly apply "${operator}" filter`, () => {
+ const question = action.question();
+ expect(question.datasetQuery().query).toEqual({
+ "source-table": ORDERS.id,
+ filter: [operator, clickedField.reference()],
+ });
+ expect(question.display()).toBe("table");
+ });
+ });
+ });
+
describe("date-time cells", () => {
const CELL_VALUE = new Date().toISOString();
const { actions } = setup({
@@ -240,14 +278,14 @@ describe("QuickFilterDrill", () => {
});
it("should return correct filters", () => {
- const filters = NUMBER_AND_DATE_FILTERS.map(operator => ({
- name: operator,
+ const filters = NUMBER_AND_DATE_FILTERS.map(({ name }) => ({
+ name,
}));
expect(actions).toMatchObject(filters);
});
actions.forEach((action, i) => {
- const operator = NUMBER_AND_DATE_FILTERS[i];
+ const { operator } = NUMBER_AND_DATE_FILTERS[i];
it(`should correctly apply "${operator}" filter`, () => {
const question = action.question();
expect(question.datasetQuery().query).toEqual({
@@ -268,14 +306,14 @@ describe("QuickFilterDrill", () => {
});
it("should return correct filters", () => {
- const filters = OTHER_FILTERS.map(operator => ({
- name: operator,
+ const filters = OTHER_FILTERS.map(({ name }) => ({
+ name,
}));
expect(actions).toMatchObject(filters);
});
actions.forEach((action, i) => {
- const operator = OTHER_FILTERS[i];
+ const { operator } = OTHER_FILTERS[i];
it(`should correctly apply "${operator}" filter`, () => {
const question = action.question();
expect(question.datasetQuery().query).toEqual({
@@ -294,14 +332,14 @@ describe("QuickFilterDrill", () => {
});
it("should return correct filters", () => {
- const filters = OTHER_FILTERS.map(operator => ({
- name: operator,
+ const filters = OTHER_FILTERS.map(({ name }) => ({
+ name,
}));
expect(actions).toMatchObject(filters);
});
actions.forEach((action, i) => {
- const operator = OTHER_FILTERS[i];
+ const { operator } = OTHER_FILTERS[i];
it(`should correctly apply "${operator}" filter`, () => {
const question = action.question();
expect(question.datasetQuery().query).toEqual({
--
GitLab