From b4a0d3cf8d916f5395b245e32c3edbb1e334d729 Mon Sep 17 00:00:00 2001
From: Alexander Polyankin <alexander.polyankin@metabase.com>
Date: Thu, 29 Sep 2022 11:10:51 +0300
Subject: [PATCH] Handle metric filters in drilldowns (#25573)
---
frontend/src/metabase-lib/lib/Question.ts | 45 ++++++++--
.../src/metabase-lib/lib/metadata/Metadata.ts | 3 +-
.../src/metabase-lib/lib/metadata/Metric.ts | 12 ++-
.../lib/queries/structured/Aggregation.ts | 59 +++++++++++-
.../drill/UnderlyingRecordsDrill.jsx | 23 +----
frontend/src/metabase/modes/lib/actions.js | 24 -----
.../__support__/sample_database_fixture.js | 1 +
.../__support__/sample_database_fixture.json | 31 +++++++
.../metabase-lib/lib/Question.unit.spec.js | 89 ++++++++++++++++---
.../6010-metric-filter-drill.cy.spec.js | 54 +++++++++++
10 files changed, 268 insertions(+), 73 deletions(-)
create mode 100644 frontend/test/metabase/scenarios/visualizations/reproductions/6010-metric-filter-drill.cy.spec.js
diff --git a/frontend/src/metabase-lib/lib/Question.ts b/frontend/src/metabase-lib/lib/Question.ts
index 7fedba9b2fe..dec1762ac7b 100644
--- a/frontend/src/metabase-lib/lib/Question.ts
+++ b/frontend/src/metabase-lib/lib/Question.ts
@@ -49,10 +49,9 @@ import {
aggregate,
breakout,
distribution,
- drillUnderlyingRecords,
+ drillFilter,
filter,
pivot,
- toUnderlyingRecords,
} from "metabase/modes/lib/actions";
import {
DashboardApi,
@@ -67,10 +66,13 @@ import {
} from "metabase-types/types/Parameter";
import { Card as CardObject, DatasetQuery } from "metabase-types/types/Card";
import { VisualizationSettings } from "metabase-types/api/card";
-import { Dataset, Value } from "metabase-types/types/Dataset";
+import { Column, Dataset, Value } from "metabase-types/types/Dataset";
import { TableId } from "metabase-types/types/Table";
import { DatabaseId } from "metabase-types/types/Database";
-import { ClickObject } from "metabase-types/types/Visualization";
+import {
+ ClickObject,
+ DimensionValue,
+} from "metabase-types/types/Visualization";
import { DependentMetadataItem } from "metabase-types/types/Query";
import { utf8_to_b64url } from "metabase/lib/encoding";
import { CollectionId } from "metabase-types/api";
@@ -536,12 +538,41 @@ class QuestionInner {
return pivot(this, breakouts, dimensions) || this;
}
- drillUnderlyingRecords(dimensions): Question {
- return drillUnderlyingRecords(this, dimensions) || this;
+ drillUnderlyingRecords(values: DimensionValue[], column?: Column): Question {
+ let query = this.query();
+ if (!(query instanceof StructuredQuery)) {
+ return this;
+ }
+
+ query = values.reduce(
+ (query, { value, column }) => drillFilter(query, value, column),
+ query,
+ );
+
+ const dimension = column && query.parseFieldReference(column.field_ref);
+ if (dimension instanceof AggregationDimension) {
+ const aggregation = dimension.aggregation();
+ const filters = aggregation ? aggregation.filters() : [];
+ query = filters.reduce((query, filter) => query.filter(filter), query);
+ }
+
+ return query.question().toUnderlyingRecords();
}
toUnderlyingRecords(): Question {
- return toUnderlyingRecords(this) || this;
+ const query = this.query();
+ if (!(query instanceof StructuredQuery)) {
+ return this;
+ }
+
+ return query
+ .clearAggregations()
+ .clearBreakouts()
+ .clearSort()
+ .clearLimit()
+ .clearFields()
+ .question()
+ .setDisplay("table");
}
toUnderlyingData(): Question {
diff --git a/frontend/src/metabase-lib/lib/metadata/Metadata.ts b/frontend/src/metabase-lib/lib/metadata/Metadata.ts
index 42290d1a26a..26c030dccf1 100644
--- a/frontend/src/metabase-lib/lib/metadata/Metadata.ts
+++ b/frontend/src/metabase-lib/lib/metadata/Metadata.ts
@@ -1,6 +1,7 @@
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-nocheck
import _ from "underscore";
+import Metric from "metabase-lib/lib/metadata/Metric";
import type Question from "../Question";
import Base from "./Base";
import type Database from "./Database";
@@ -75,7 +76,7 @@ export default class Metadata extends Base {
* @param {MetricId} metricId
* @returns {?Metric}
*/
- metric(metricId) {
+ metric(metricId): Metric | null {
return (metricId != null && this.metrics[metricId]) || null;
}
diff --git a/frontend/src/metabase-lib/lib/metadata/Metric.ts b/frontend/src/metabase-lib/lib/metadata/Metric.ts
index dec163eabf7..db44bd3810a 100644
--- a/frontend/src/metabase-lib/lib/metadata/Metric.ts
+++ b/frontend/src/metabase-lib/lib/metadata/Metric.ts
@@ -1,5 +1,6 @@
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-nocheck
+import Filter from "metabase-lib/lib/queries/structured/Filter";
import Base from "./Base";
/**
* @typedef { import("./metadata").Aggregation } Aggregation
@@ -28,13 +29,16 @@ export default class Metric extends Base {
: null;
}
+ /** Underlying filter clauses for this metric */
+ filters(): Filter[] {
+ const query = this.definitionQuery();
+ return query ? query.filters() : [];
+ }
+
/** Underlying aggregation clause for this metric */
aggregation() {
const query = this.definitionQuery();
-
- if (query) {
- return query.aggregations()[0];
- }
+ return query?.aggregations()[0];
}
/** Column name when this metric is used in a query */
diff --git a/frontend/src/metabase-lib/lib/queries/structured/Aggregation.ts b/frontend/src/metabase-lib/lib/queries/structured/Aggregation.ts
index ec8275a3846..5b367b89ac2 100644
--- a/frontend/src/metabase-lib/lib/queries/structured/Aggregation.ts
+++ b/frontend/src/metabase-lib/lib/queries/structured/Aggregation.ts
@@ -7,6 +7,8 @@ import { Aggregation as AggregationObject } from "metabase-types/types/Query";
import { AggregationOperator } from "metabase-types/types/Metadata";
import { MetricId } from "metabase-types/types/Metric";
import { FieldId } from "metabase-types/types/Field";
+import Filter from "metabase-lib/lib/queries/structured/Filter";
+import Metric from "metabase-lib/lib/metadata/Metric";
import StructuredQuery from "../StructuredQuery";
import Dimension, { AggregationDimension } from "../../Dimension";
import MBQLClause from "./MBQLClause";
@@ -206,10 +208,18 @@ export default class Aggregation extends MBQLClause {
* Get the operator from a standard aggregation clause
* Returns `null` if the clause isn't a "standard" metric
*/
- operatorName(): string | null | undefined {
+ operatorName(): string | null {
if (this.isStandard()) {
return this[0];
}
+
+ return null;
+ }
+
+ expressionName(): string | null {
+ if (this.isCustom()) {
+ return this[0];
+ }
}
/**
@@ -238,16 +248,20 @@ export default class Aggregation extends MBQLClause {
* Get metricId from a metric aggregation clause
* Returns `null` if the clause doesn't represent a metric
*/
- metricId(): MetricId | null | undefined {
+ metricId(): MetricId | null {
if (this.isMetric()) {
return this[1];
}
+
+ return null;
}
- metric() {
+ metric(): Metric | null {
if (this.isMetric()) {
return this.metadata().metric(this.metricId());
}
+
+ return null;
}
// OPTIONS
@@ -266,7 +280,7 @@ export default class Aggregation extends MBQLClause {
/**
* Returns the aggregation without "aggregation-options" clause, if any
*/
- aggregation() {
+ aggregation(): Aggregation {
if (this.hasOptions()) {
return new Aggregation(this[1], this._index, this._query);
} else {
@@ -274,6 +288,43 @@ export default class Aggregation extends MBQLClause {
}
}
+ filters(): Filter[] {
+ if (this.isCustom()) {
+ const filter = this.customFilter();
+ return filter ? [filter] : [];
+ }
+
+ if (this.isMetric()) {
+ const filters = this.metricFilters();
+ return filters ?? [];
+ }
+
+ return [];
+ }
+
+ customFilter(): Filter | null {
+ if (this.isCustom()) {
+ switch (this.expressionName()) {
+ case "share":
+ case "count-where":
+ return new Filter(this[1], null, this.query());
+ case "sum-where":
+ return new Filter(this[2], null, this.query());
+ }
+ }
+
+ return null;
+ }
+
+ metricFilters(): Filter[] | null {
+ if (this.isMetric()) {
+ const metric = this.metric();
+ return metric?.filters().map(filter => filter.setQuery(this.query()));
+ }
+
+ return null;
+ }
+
// MISC
aggregationDimension() {
return new AggregationDimension(
diff --git a/frontend/src/metabase/modes/components/drill/UnderlyingRecordsDrill.jsx b/frontend/src/metabase/modes/components/drill/UnderlyingRecordsDrill.jsx
index e43ccac17b1..675f264a6b0 100644
--- a/frontend/src/metabase/modes/components/drill/UnderlyingRecordsDrill.jsx
+++ b/frontend/src/metabase/modes/components/drill/UnderlyingRecordsDrill.jsx
@@ -1,8 +1,6 @@
import { ngettext, msgid } from "ttag";
import { inflect } from "metabase/lib/formatting";
-import { AggregationDimension } from "metabase-lib/lib/Dimension";
-
export default ({ question, clicked }) => {
// removes post-aggregation filter stage
clicked = clicked && question.topLevelClicked(clicked);
@@ -21,20 +19,6 @@ export default ({ question, clicked }) => {
// the metric value should be the number of rows that will be displayed
const count = typeof clicked.value === "number" ? clicked.value : 2;
- // special case for aggregations that include a filter, such as share, count-where, and sum-where
- let extraFilter = null;
- const dimension =
- clicked.column && query.parseFieldReference(clicked.column.field_ref);
- if (dimension instanceof AggregationDimension) {
- const aggregation = dimension.aggregation();
- extraFilter =
- aggregation[0] === "count-where" || aggregation[0] === "share"
- ? aggregation[1]
- : aggregation[0] === "sum-where"
- ? aggregation[2]
- : null;
- }
-
const recordName = query.table() && query.table().displayName();
const inflectedTableName = recordName
? inflect(recordName, count)
@@ -51,12 +35,7 @@ export default ({ question, clicked }) => {
count,
),
question: () => {
- const q = question.drillUnderlyingRecords(dimensions);
- if (extraFilter) {
- return q.query().filter(extraFilter).question();
- } else {
- return q;
- }
+ return question.drillUnderlyingRecords(dimensions, clicked.column);
},
},
];
diff --git a/frontend/src/metabase/modes/lib/actions.js b/frontend/src/metabase/modes/lib/actions.js
index 1d676256108..af276fe197a 100644
--- a/frontend/src/metabase/modes/lib/actions.js
+++ b/frontend/src/metabase/modes/lib/actions.js
@@ -83,30 +83,6 @@ export function distribution(question, column) {
}
}
-export function toUnderlyingRecords(question) {
- const query = question.query();
- if (query instanceof StructuredQuery) {
- return query
- .clearAggregations()
- .clearBreakouts()
- .clearSort()
- .clearLimit()
- .clearFields()
- .question()
- .setDisplay("table");
- }
-}
-
-export function drillUnderlyingRecords(question, dimensions) {
- let query = question.query();
- if (query instanceof StructuredQuery) {
- for (const dimension of dimensions) {
- query = drillFilter(query, dimension.value, dimension.column);
- }
- return toUnderlyingRecords(query.question());
- }
-}
-
// STRUCTURED QUERY UTILITIES
const fieldRefWithTemporalUnit = (mbqlClause, unit) => {
diff --git a/frontend/test/__support__/sample_database_fixture.js b/frontend/test/__support__/sample_database_fixture.js
index ff96c276983..442dc28fbdd 100644
--- a/frontend/test/__support__/sample_database_fixture.js
+++ b/frontend/test/__support__/sample_database_fixture.js
@@ -82,6 +82,7 @@ export function makeMetadata(metadata) {
},
metrics: {
1: { name: "metric" },
+ 2: { name: "metric" },
},
segments: {
1: { name: "segment" },
diff --git a/frontend/test/__support__/sample_database_fixture.json b/frontend/test/__support__/sample_database_fixture.json
index 60065705f31..b59d59aaf53 100644
--- a/frontend/test/__support__/sample_database_fixture.json
+++ b/frontend/test/__support__/sample_database_fixture.json
@@ -57,6 +57,37 @@
"how_is_this_calculated": null,
"created_at": "2017-06-14T23:32:12.267Z",
"points_of_interest": null
+ },
+ "2": {
+ "description": "Because we want to know the total I ugess",
+ "table_id": 1,
+ "definition": {
+ "filter": [">", 6, 20],
+ "aggregation": [["sum", ["field", 6, null]]],
+ "source-table": 1
+ },
+ "creator": {
+ "email": "sameer@metabase.com",
+ "first_name": "Sameer",
+ "last_login": "2017-06-14T23:23:59.582Z",
+ "is_qbnewb": true,
+ "is_superuser": true,
+ "id": 1,
+ "last_name": "Al-Sakran",
+ "date_joined": "2017-06-14T23:23:59.409Z",
+ "common_name": "Sameer Al-Sakran"
+ },
+ "database_id": 1,
+ "show_in_getting_started": false,
+ "name": "Total Order Value",
+ "archived": false,
+ "caveats": null,
+ "creator_id": 1,
+ "updated_at": "2017-06-14T23:32:12.266Z",
+ "id": 2,
+ "how_is_this_calculated": null,
+ "created_at": "2017-06-14T23:32:12.267Z",
+ "points_of_interest": null
}
},
"segments": {
diff --git a/frontend/test/metabase-lib/lib/Question.unit.spec.js b/frontend/test/metabase-lib/lib/Question.unit.spec.js
index f87a77d1c05..15e8b3c25fe 100644
--- a/frontend/test/metabase-lib/lib/Question.unit.spec.js
+++ b/frontend/test/metabase-lib/lib/Question.unit.spec.js
@@ -56,6 +56,36 @@ const orders_count_card = {
},
};
+const orders_count_where_card = {
+ id: 2,
+ name: "# orders data",
+ display: "table",
+ visualization_settings: {},
+ dataset_query: {
+ type: "query",
+ database: SAMPLE_DATABASE.id,
+ query: {
+ "source-table": ORDERS.id,
+ aggregation: [["count-where", [">", ORDERS.TOTAL.id, 50]]],
+ },
+ },
+};
+
+const orders_metric_filter_card = {
+ id: 2,
+ name: "# orders data",
+ display: "table",
+ visualization_settings: {},
+ dataset_query: {
+ type: "query",
+ database: SAMPLE_DATABASE.id,
+ query: {
+ "source-table": ORDERS.id,
+ aggregation: [["metric", 2]],
+ },
+ },
+};
+
const native_orders_count_card = {
id: 3,
name: "# orders data",
@@ -531,20 +561,13 @@ describe("Question", () => {
});
describe("drillUnderlyingRecords(...)", () => {
- const ordersCountQuestion = new Question(
- orders_count_by_id_card,
- metadata,
- );
-
- // ???
- it("applies a filter to a given filterspec", () => {
+ it("applies a filter to a given query", () => {
+ const question = new Question(orders_count_by_id_card, metadata);
const dimensions = [{ value: 1, column: ORDERS.ID.column() }];
- const drilledQuestion =
- ordersCountQuestion.drillUnderlyingRecords(dimensions);
- expect(drilledQuestion.canRun()).toBe(true);
+ const newQuestion = question.drillUnderlyingRecords(dimensions);
- expect(drilledQuestion._card.dataset_query).toEqual({
+ expect(newQuestion._card.dataset_query).toEqual({
type: "query",
database: SAMPLE_DATABASE.id,
query: {
@@ -553,6 +576,50 @@ describe("Question", () => {
},
});
});
+
+ it("applies a filter from an aggregation to a given query", () => {
+ const question = new Question(orders_count_where_card, metadata);
+ const dimensions = [{ value: 1, column: ORDERS.ID.column() }];
+ const column = { field_ref: ["aggregation", 0] };
+
+ const newQuestion = question.drillUnderlyingRecords(dimensions, column);
+
+ expect(newQuestion.canRun()).toBe(true);
+ expect(newQuestion._card.dataset_query).toEqual({
+ type: "query",
+ database: SAMPLE_DATABASE.id,
+ query: {
+ "source-table": ORDERS.id,
+ filter: [
+ "and",
+ ["=", ["field", ORDERS.ID.id, null], 1],
+ [">", ORDERS.TOTAL.id, 50],
+ ],
+ },
+ });
+ });
+
+ it("applies a filter from a metric to a given query", () => {
+ const question = new Question(orders_metric_filter_card, metadata);
+ const dimensions = [{ value: 1, column: ORDERS.ID.column() }];
+ const column = { field_ref: ["aggregation", 0] };
+
+ const newQuestion = question.drillUnderlyingRecords(dimensions, column);
+
+ expect(newQuestion.canRun()).toBe(true);
+ expect(newQuestion._card.dataset_query).toEqual({
+ type: "query",
+ database: SAMPLE_DATABASE.id,
+ query: {
+ "source-table": ORDERS.id,
+ filter: [
+ "and",
+ ["=", ["field", ORDERS.ID.id, null], 1],
+ [">", ORDERS.TOTAL.id, 20],
+ ],
+ },
+ });
+ });
});
describe("toUnderlyingRecords(...)", () => {
diff --git a/frontend/test/metabase/scenarios/visualizations/reproductions/6010-metric-filter-drill.cy.spec.js b/frontend/test/metabase/scenarios/visualizations/reproductions/6010-metric-filter-drill.cy.spec.js
new file mode 100644
index 00000000000..0a49feb184a
--- /dev/null
+++ b/frontend/test/metabase/scenarios/visualizations/reproductions/6010-metric-filter-drill.cy.spec.js
@@ -0,0 +1,54 @@
+import { restore, visitQuestion } from "__support__/e2e/helpers";
+import { SAMPLE_DATABASE } from "__support__/e2e/cypress_sample_database";
+
+const { ORDERS, ORDERS_ID } = SAMPLE_DATABASE;
+
+describe("issue 6010", () => {
+ beforeEach(() => {
+ restore();
+ cy.signInAsAdmin();
+ cy.intercept("POST", "/api/dataset").as("dataset");
+ });
+
+ it("should apply the filter from a metric when drilling through (metabase#6010)", () => {
+ createMetric()
+ .then(({ body: { id } }) => createQuestion(id))
+ .then(({ body: { id } }) => visitQuestion(id));
+
+ cy.get(".dot").eq(0).click({ force: true });
+ cy.findByText(/View these Orders/).click();
+ cy.wait("@dataset");
+
+ cy.findByText("Created At is January, 2018").should("be.visible");
+ cy.findByText("Total is greater than 150").should("be.visible");
+ });
+});
+
+const createMetric = () => {
+ return cy.request("POST", "/api/metric", {
+ name: "Metric",
+ description: "Metric with a filter",
+ table_id: ORDERS_ID,
+ definition: {
+ "source-table": ORDERS_ID,
+ filter: [">", ORDERS.TOTAL, 150],
+ aggregation: [["count"]],
+ },
+ });
+};
+
+const createQuestion = metric_id => {
+ return cy.createQuestion({
+ name: "Question",
+ display: "line",
+ query: {
+ "source-table": ORDERS_ID,
+ breakout: [["field", ORDERS.CREATED_AT, { "temporal-unit": "month" }]],
+ aggregation: [["metric", metric_id]],
+ },
+ visualization_settings: {
+ "graph.dimensions": ["CREATED_AT"],
+ "graph.metrics": ["count"],
+ },
+ });
+};
--
GitLab