Skip to content
Snippets Groups Projects
Unverified Commit decc8b7d authored by Alexander Polyankin's avatar Alexander Polyankin Committed by GitHub
Browse files

Fix quick filter drill with nulls (#29190)

parent 6f3c99e1
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
e2e/test/scenarios/question/reproductions/29082-quick-filter-null.cy.spec.js 0 → 100644
+ 47
− 0
View file @ decc8b7d
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");
});
});
frontend/src/metabase-lib/queries/drills/quick-filter-drill.js
+ 58
− 52
View file @ decc8b7d
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 }];
}
}
frontend/src/metabase/modes/components/drill/QuickFilterDrill.tsx
+ 3
− 3
View file @ decc8b7d
......@@ -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 }),
}));
};
......
frontend/test/metabase/modes/components/drill/QuickFilterDrill.unit.spec.js
+ 61
− 23
View file @ decc8b7d
......@@ -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({
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment