From d32039aca96f4929ceb77b6ca8ca47ab05557d1e Mon Sep 17 00:00:00 2001
From: Tom Robinson <tlrobinson@gmail.com>
Date: Wed, 28 Aug 2019 18:18:17 -0700
Subject: [PATCH] Fix drill through to question user doesn't have all data
permissions for (#10655)
* Fix drill through to question user doesn't have all data permissions for. Resolves #7928
* Fix flow
* Fix #10656
* Tests
* Fix lint + add a couple cute aliases
* fix object detail logic + add test
---
frontend/src/metabase-lib/lib/Mode.js | 4 +-
.../src/metabase-lib/lib/metadata/Field.js | 4 +
.../lib/queries/StructuredQuery.js | 26 ++++
.../lib/queries/structured/Breakout.js | 14 +-
frontend/src/metabase/app-main.js | 1 +
frontend/src/metabase/modes/lib/modes.js | 71 ++++-----
.../src/metabase/query_builder/selectors.js | 23 +--
.../__support__/sample_dataset_fixture.js | 1 +
.../test/metabase-lib/lib/Mode.unit.spec.js | 139 +++++++++++++++---
9 files changed, 202 insertions(+), 81 deletions(-)
diff --git a/frontend/src/metabase-lib/lib/Mode.js b/frontend/src/metabase-lib/lib/Mode.js
index c91589e9f17..b3d4668e2ef 100644
--- a/frontend/src/metabase-lib/lib/Mode.js
+++ b/frontend/src/metabase-lib/lib/Mode.js
@@ -20,9 +20,7 @@ export default class Mode {
static forQuestion(question: Question): ?Mode {
// TODO Atte Keinänen 6/22/17: Move getMode here and refactor it after writing tests
- const card = question.card();
- const tableMetadata = question.tableMetadata();
- const queryMode = getMode(card, tableMetadata);
+ const queryMode = getMode(question);
if (queryMode) {
return new Mode(question, queryMode);
diff --git a/frontend/src/metabase-lib/lib/metadata/Field.js b/frontend/src/metabase-lib/lib/metadata/Field.js
index d374572bc68..39316284294 100644
--- a/frontend/src/metabase-lib/lib/metadata/Field.js
+++ b/frontend/src/metabase-lib/lib/metadata/Field.js
@@ -19,6 +19,7 @@ import {
isString,
isSummable,
isCategory,
+ isAddress,
isState,
isCountry,
isCoordinate,
@@ -95,6 +96,9 @@ export default class Field extends Base {
isString() {
return isString(this);
}
+ isAddress() {
+ return isAddress(this);
+ }
isState() {
return isState(this);
}
diff --git a/frontend/src/metabase-lib/lib/queries/StructuredQuery.js b/frontend/src/metabase-lib/lib/queries/StructuredQuery.js
index 347978750e2..3b18a7e5843 100644
--- a/frontend/src/metabase-lib/lib/queries/StructuredQuery.js
+++ b/frontend/src/metabase-lib/lib/queries/StructuredQuery.js
@@ -500,6 +500,32 @@ export default class StructuredQuery extends AtomicQuery {
return this.fields().length > 0;
}
+ // ALIASES: allows
+
+ /**
+ * @returns alias for addAggregation
+ */
+ aggregate(aggregation: Aggregation): StructuredQuery {
+ return this.addAggregation(aggregation);
+ }
+
+ /**
+ * @returns alias for addBreakout
+ */
+ breakout(breakout: Breakout | Dimension): StructuredQuery {
+ if (breakout instanceof Dimension) {
+ breakout = breakout.mbql();
+ }
+ return this.addBreakout(breakout);
+ }
+
+ /**
+ * @returns alias for addFilter
+ */
+ filter(filter: Filter | FilterWrapper) {
+ return this.addFilter(filter);
+ }
+
// JOINS
/**
diff --git a/frontend/src/metabase-lib/lib/queries/structured/Breakout.js b/frontend/src/metabase-lib/lib/queries/structured/Breakout.js
index 47150d8b0db..a532e23b4c8 100644
--- a/frontend/src/metabase-lib/lib/queries/structured/Breakout.js
+++ b/frontend/src/metabase-lib/lib/queries/structured/Breakout.js
@@ -3,8 +3,9 @@
import MBQLClause from "./MBQLClause";
import type { Breakout as BreakoutObject } from "metabase/meta/types/Query";
-import type StructuredQuery from "../StructuredQuery";
-import type Dimension from "../../Dimension";
+import type StructuredQuery from "metabase-lib/lib/queries/StructuredQuery";
+import type Dimension from "metabase-lib/lib/Dimension";
+import type Field from "metabase-lib/lib/metadata/Field";
export default class Breakout extends MBQLClause {
/**
@@ -50,9 +51,16 @@ export default class Breakout extends MBQLClause {
}
/**
- * Returns the breakout's dimension
+ * Returns the breakout's Dimension
*/
dimension(): Dimension {
return this._query.parseFieldReference(this);
}
+
+ /**
+ * Returns the breakout's Field
+ */
+ field(): Field {
+ return this.dimension().field();
+ }
}
diff --git a/frontend/src/metabase/app-main.js b/frontend/src/metabase/app-main.js
index 5dba41cdfe6..162592ede4f 100644
--- a/frontend/src/metabase/app-main.js
+++ b/frontend/src/metabase/app-main.js
@@ -17,6 +17,7 @@ const WHITELIST_FORBIDDEN_URLS = [
// we should gracefully handle cases where we don't have access to metadata
/api\/database\/\d+\/metadata$/,
/api\/database\/\d+\/fields/,
+ /api\/field\/\d+/,
/api\/field\/\d+\/values/,
/api\/table\/\d+\/query_metadata$/,
/api\/table\/\d+\/fks$/,
diff --git a/frontend/src/metabase/modes/lib/modes.js b/frontend/src/metabase/modes/lib/modes.js
index c443fc61f87..ca861d0d5c4 100644
--- a/frontend/src/metabase/modes/lib/modes.js
+++ b/frontend/src/metabase/modes/lib/modes.js
@@ -1,15 +1,5 @@
/* @flow weak */
-import * as Q_DEPRECATED from "metabase/lib/query"; // legacy query lib
-import {
- isDate,
- isAddress,
- isCategory,
- isPK,
-} from "metabase/lib/schema_metadata";
-import * as Query from "metabase/lib/query/query";
-import * as Card from "metabase/meta/Card";
-
import ObjectMode from "../components/modes/ObjectMode";
import SegmentMode from "../components/modes/SegmentMode";
import MetricMode from "../components/modes/MetricMode";
@@ -19,51 +9,45 @@ import PivotMode from "../components/modes/PivotMode";
import NativeMode from "../components/modes/NativeMode";
import DefaultMode from "../components/modes/DefaultMode";
-import type { Card as CardObject } from "metabase/meta/types/Card";
-import type { TableMetadata } from "metabase/meta/types/Metadata";
import type { QueryMode } from "metabase/meta/types/Visualization";
-import _ from "underscore";
+import type Question from "metabase-lib/lib/Question";
+
+import StructuredQuery from "metabase-lib/lib/queries/StructuredQuery";
+import NativeQuery from "metabase-lib/lib/queries/NativeQuery";
-export function getMode(
- card: CardObject,
- tableMetadata: ?TableMetadata,
-): ?QueryMode {
- if (!card) {
+export function getMode(question: ?Question): ?QueryMode {
+ if (!question) {
return null;
}
- if (Card.isNative(card)) {
+ const query = question.query();
+
+ if (query instanceof NativeQuery) {
return NativeMode;
}
- const query = Card.getQuery(card);
- if (Card.isStructured(card) && query) {
- if (!tableMetadata) {
- return null;
- }
-
- const aggregations = Query.getAggregations(query);
- const breakouts = Query.getBreakouts(query);
- const filters = Query.getFilters(query);
+ if (query instanceof StructuredQuery) {
+ const aggregations = query.aggregations();
+ const breakouts = query.breakouts();
+ const filters = query.filters();
if (aggregations.length === 0 && breakouts.length === 0) {
const isPKFilter = filter => {
- if (tableMetadata && Array.isArray(filter) && filter[0] === "=") {
- const fieldId = Q_DEPRECATED.getFieldTargetId(filter[1]);
- const field = tableMetadata.fields_lookup[fieldId];
+ if (filter.isFieldFilter()) {
+ const field = filter.field();
if (
field &&
+ field.isPK() &&
field.table &&
- field.table.id === query["source-table"] &&
- isPK(field)
+ field.table.id === query.sourceTableId()
) {
return true;
}
}
return false;
};
- if (_.any(filters, isPKFilter)) {
+ if (filters.some(isPKFilter)) {
return ObjectMode;
} else {
return SegmentMode;
@@ -73,26 +57,23 @@ export function getMode(
return MetricMode;
}
if (aggregations.length > 0 && breakouts.length > 0) {
- const breakoutFields = breakouts.map(
- breakout =>
- (Q_DEPRECATED.getFieldTarget(breakout, tableMetadata) || {}).field,
- );
+ const breakoutFields = breakouts.map(b => b.field());
if (
- (breakoutFields.length === 1 && isDate(breakoutFields[0])) ||
+ (breakoutFields.length === 1 && breakoutFields[0].isDate()) ||
(breakoutFields.length === 2 &&
- isDate(breakoutFields[0]) &&
- isCategory(breakoutFields[1]))
+ breakoutFields[0].isDate() &&
+ breakoutFields[1].isCategory())
) {
return TimeseriesMode;
}
- if (breakoutFields.length === 1 && isAddress(breakoutFields[0])) {
+ if (breakoutFields.length === 1 && breakoutFields[0].isAddress()) {
return GeoMode;
}
if (
- (breakoutFields.length === 1 && isCategory(breakoutFields[0])) ||
+ (breakoutFields.length === 1 && breakoutFields[0].isCategory()) ||
(breakoutFields.length === 2 &&
- isCategory(breakoutFields[0]) &&
- isCategory(breakoutFields[1]))
+ breakoutFields[0].isCategory() &&
+ breakoutFields[1].isCategory())
) {
return PivotMode;
}
diff --git a/frontend/src/metabase/query_builder/selectors.js b/frontend/src/metabase/query_builder/selectors.js
index 8bae3caf327..27a2de61419 100644
--- a/frontend/src/metabase/query_builder/selectors.js
+++ b/frontend/src/metabase/query_builder/selectors.js
@@ -1,3 +1,5 @@
+/*eslint no-use-before-define: "error"*/
+
import { createSelector } from "reselect";
import _ from "underscore";
import { getIn } from "icepick";
@@ -6,7 +8,6 @@ import { getIn } from "icepick";
// eslint-disable-next-line no-unused-vars
import Visualization from "metabase/visualizations/components/Visualization";
-import { getMode as getMode_ } from "metabase/modes/lib/modes";
import {
extractRemappings,
getVisualizationTransformed,
@@ -110,16 +111,6 @@ export const getDatabaseFields = createSelector(
(databaseId, databaseFields) => [], // FIXME!
);
-export const getMode = createSelector(
- [getLastRunCard, getTableMetadata],
- (card, tableMetadata) => getMode_(card, tableMetadata),
-);
-
-export const getIsObjectDetail = createSelector(
- [getMode],
- mode => mode && mode.name === "object",
-);
-
export const getParameters = createSelector(
[getCard, getParameterValues],
(card, parameterValues) => getParametersWithExtras(card, parameterValues),
@@ -193,6 +184,16 @@ export const getOriginalQuestion = createSelector(
},
);
+export const getMode = createSelector(
+ [getLastRunQuestion],
+ question => question && question.mode(),
+);
+
+export const getIsObjectDetail = createSelector(
+ [getMode],
+ mode => mode && mode.name() === "object",
+);
+
export const getIsDirty = createSelector(
[getQuestion, getOriginalQuestion],
(question, originalQuestion) => {
diff --git a/frontend/test/__support__/sample_dataset_fixture.js b/frontend/test/__support__/sample_dataset_fixture.js
index 9bbfbe1977d..20b2cf13184 100644
--- a/frontend/test/__support__/sample_dataset_fixture.js
+++ b/frontend/test/__support__/sample_dataset_fixture.js
@@ -24,6 +24,7 @@ export const ORDERS_PRODUCT_FK_FIELD_ID = 3;
export const ORDERS_SUBTOTAL_FIELD_ID = 4;
export const ORDERS_TAX_FIELD_ID = 5;
export const ORDERS_TOTAL_FIELD_ID = 6;
+export const ORDERS_USER_FK_FIELD_ID = 7;
export const MAIN_METRIC_ID = 1;
diff --git a/frontend/test/metabase-lib/lib/Mode.unit.spec.js b/frontend/test/metabase-lib/lib/Mode.unit.spec.js
index acd81bccaa6..8b2c91553ee 100644
--- a/frontend/test/metabase-lib/lib/Mode.unit.spec.js
+++ b/frontend/test/metabase-lib/lib/Mode.unit.spec.js
@@ -2,41 +2,130 @@ import {
metadata,
DATABASE_ID,
ORDERS_TABLE_ID,
+ ORDERS_PK_FIELD_ID,
+ ORDERS_CREATED_DATE_FIELD_ID,
+ ORDERS_PRODUCT_FK_FIELD_ID,
+ ORDERS_USER_FK_FIELD_ID,
+ PRODUCT_CATEGORY_FIELD_ID,
+ PEOPLE_STATE_FIELD_ID,
orders_raw_card,
} from "__support__/sample_dataset_fixture";
import Question from "metabase-lib/lib/Question";
describe("Mode", () => {
- const rawDataQuestionMode = new Question(metadata, orders_raw_card).mode();
- const timeBreakoutQuestionMode = Question.create({
- databaseId: DATABASE_ID,
- tableId: ORDERS_TABLE_ID,
- metadata,
- })
- .query()
- .addAggregation(["count"])
- .addBreakout(["datetime-field", ["field-id", 1], "day"])
- .question()
- .setDisplay("table")
- .mode();
+ const rawDataQuestion = new Question(metadata, orders_raw_card);
+ const rawDataQuery = rawDataQuestion.query();
+ const rawDataQuestionMode = rawDataQuestion.mode();
describe("forQuestion(question)", () => {
describe("with structured query question", () => {
// testbed for generative testing? see http://leebyron.com/testcheck-js
- it("returns `segment` mode with raw data", () => {});
+ it("returns `segment` mode with raw data", () => {
+ const mode = rawDataQuery.question().mode();
+ expect(mode && mode.name()).toEqual("segment");
+ });
+
+ it("returns `metric` mode with >= 1 aggregations", () => {
+ const mode = rawDataQuery
+ .aggregate(["count"])
+ .question()
+ .mode();
+ expect(mode && mode.name()).toEqual("metric");
+ });
- it("returns `metric` mode with >= 1 aggregations", () => {});
+ it("returns `timeseries` mode with >=1 aggregations and date breakout", () => {
+ const mode = rawDataQuery
+ .aggregate(["count"])
+ .breakout([
+ "datetime-field",
+ ["field-id", ORDERS_CREATED_DATE_FIELD_ID],
+ "day",
+ ])
+ .question()
+ .mode();
+ expect(mode && mode.name()).toEqual("timeseries");
+ });
+ it("returns `timeseries` mode with >=1 aggregations and date + category breakout", () => {
+ const mode = rawDataQuery
+ .aggregate(["count"])
+ .breakout([
+ "datetime-field",
+ ["field-id", ORDERS_CREATED_DATE_FIELD_ID],
+ "day",
+ ])
+ .breakout([
+ "fk->",
+ ["field-id", ORDERS_PRODUCT_FK_FIELD_ID],
+ ["field-id", PRODUCT_CATEGORY_FIELD_ID],
+ ])
+ .question()
+ .mode();
+ expect(mode && mode.name()).toEqual("timeseries");
+ });
- it("returns `timeseries` mode with >=1 aggregations and date breakout", () => {});
- it("returns `timeseries` mode with >=1 aggregations and date + category breakout", () => {});
+ it("returns `geo` mode with >=1 aggregations and an address breakout", () => {
+ const mode = rawDataQuery
+ .aggregate(["count"])
+ .breakout([
+ "fk->",
+ ["field-id", ORDERS_USER_FK_FIELD_ID],
+ ["field-id", PEOPLE_STATE_FIELD_ID],
+ ])
+ .question()
+ .mode();
+ expect(mode && mode.name()).toEqual("geo");
+ });
- it("returns `geo` mode with >=1 aggregations and an address breakout", () => {});
+ it("returns `pivot` mode with >=1 aggregations and 1-2 category breakouts", () => {
+ const mode = rawDataQuery
+ .aggregate(["count"])
+ .breakout([
+ "fk->",
+ ["field-id", ORDERS_PRODUCT_FK_FIELD_ID],
+ ["field-id", PRODUCT_CATEGORY_FIELD_ID],
+ ])
+ .breakout([
+ "fk->",
+ ["field-id", ORDERS_USER_FK_FIELD_ID],
+ ["field-id", PEOPLE_STATE_FIELD_ID],
+ ])
+ .question()
+ .mode();
+ expect(mode && mode.name()).toEqual("pivot");
+ });
- it("returns `pivot` mode with >=1 aggregations and 1-2 category breakouts", () => {});
+ it("returns `object` mode with pk filter", () => {
+ const mode = rawDataQuery
+ .filter(["=", ["field-id", ORDERS_PK_FIELD_ID], 42])
+ .question()
+ .mode();
+ expect(mode && mode.name()).toEqual("object");
+ });
- it("returns `default` mode with >=0 aggregations and >=3 breakouts", () => {});
+ it("returns `default` mode with >=0 aggregations and >=3 breakouts", () => {
+ const mode = rawDataQuery
+ .aggregate(["count"])
+ .breakout([
+ "datetime-field",
+ ["field-id", ORDERS_CREATED_DATE_FIELD_ID],
+ "day",
+ ])
+ .breakout([
+ "fk->",
+ ["field-id", ORDERS_PRODUCT_FK_FIELD_ID],
+ ["field-id", PRODUCT_CATEGORY_FIELD_ID],
+ ])
+ .breakout([
+ "fk->",
+ ["field-id", ORDERS_USER_FK_FIELD_ID],
+ ["field-id", PEOPLE_STATE_FIELD_ID],
+ ])
+ .question()
+ .mode();
+ expect(mode && mode.name()).toEqual("default");
+ });
it("returns `default` mode with >=1 aggregations and >=1 breakouts when first neither date or category", () => {});
});
describe("with native query question", () => {
@@ -83,6 +172,18 @@ describe("Mode", () => {
});
describe("for a question with an aggregation and a time breakout", () => {
+ const timeBreakoutQuestionMode = Question.create({
+ databaseId: DATABASE_ID,
+ tableId: ORDERS_TABLE_ID,
+ metadata,
+ })
+ .query()
+ .aggregate(["count"])
+ .breakout(["datetime-field", ["field-id", 1], "day"])
+ .question()
+ .setDisplay("table")
+ .mode();
+
it("has pivot as mode actions 1 and 2", () => {
expect(timeBreakoutQuestionMode.actions()[0].name).toBe(
"pivot-by-category",
--
GitLab