From 4495d93f77ee9b83546fd0130ef364257546dab9 Mon Sep 17 00:00:00 2001
From: Paul Rosenzweig <paulrosenzweig@users.noreply.github.com>
Date: Mon, 25 Nov 2019 09:42:49 -0500
Subject: [PATCH] Update auto-viz selection (#11368)
---
frontend/src/metabase-lib/lib/Question.js | 54 +++++++++++++
.../src/metabase/query_builder/actions.js | 75 +++++++------------
.../view/sidebars/ChartTypeSidebar.jsx | 4 +-
.../src/metabase/query_builder/reducers.js | 4 +-
frontend/src/metabase/visualizations/index.js | 6 ++
.../visualizations/visualizations/Map.jsx | 5 +-
.../metabase-lib/lib/Question.unit.spec.js | 19 +++++
.../scenarios/query_builder.e2e.spec.js | 23 +++---
8 files changed, 130 insertions(+), 60 deletions(-)
diff --git a/frontend/src/metabase-lib/lib/Question.js b/frontend/src/metabase-lib/lib/Question.js
index 26c04217026..e1c10c4f6d4 100644
--- a/frontend/src/metabase-lib/lib/Question.js
+++ b/frontend/src/metabase-lib/lib/Question.js
@@ -276,7 +276,61 @@ export default class Question {
return this.setCard(assoc(this.card(), "display", display));
}
+ // The selected display is set when the user explicitly chooses a
+ // visualization type. Having it set prevents auto selecting a new type,
+ // unless the selected type isn't sensible.
+ setSelectedDisplay(display): Question {
+ return this.setCard(
+ assoc(this.card(), "selectedDisplay", display),
+ ).setDisplay(display);
+ }
+ selectedDisplay(): string {
+ return this._card && this._card.selectedDisplay;
+ }
+
+ // This feels a bit hacky because it stores result-dependent info on card. We
+ // use the list of sensible displays to override a user-selected display if it
+ // no longer makes sense for the data.
+ setSensibleDisplays(displays): Question {
+ return this.setCard(assoc(this.card(), "sensibleDisplays", displays));
+ }
+ sensibleDisplays(): string[] {
+ return (this._card && this._card.sensibleDisplays) || [];
+ }
+
+ // This determines whether `setDefaultDisplay` should replace the current
+ // display. If we have a list of sensibleDisplays and the user-selected
+ // display is one of them, we won't overwrite it in `setDefaultDisplay`. If
+ // the user hasn't selected a display or `sensibleDisplays` hasn't been set,
+ // we can let `setDefaultDisplay` choose a display type.
+ shouldNotSetDisplay(): boolean {
+ return this.sensibleDisplays().includes(this.selectedDisplay());
+ }
+
+ // Switches display based on data shape. For 1x1 data, we show a scalar. If
+ // our display was a 1x1 type, but the data isn't 1x1, we show a table.
+ switchTableScalar({ rows = [], cols }): Question {
+ const display = this.display();
+ const isScalar = ["scalar", "progress", "gauge"].includes(display);
+ const isOneByOne = rows.length === 1 && cols.length === 1;
+
+ const newDisplay =
+ !isScalar && isOneByOne
+ ? // if we have a 1x1 data result then this should always be viewed as a scalar
+ "scalar"
+ : isScalar && !isOneByOne
+ ? // any time we were a scalar and now have more than 1x1 data switch to table view
+ "table"
+ : // otherwise leave the display unchanged
+ display;
+
+ return this.setDisplay(newDisplay);
+ }
+
setDefaultDisplay(): Question {
+ if (this.shouldNotSetDisplay()) {
+ return this;
+ }
const query = this.query();
if (query instanceof StructuredQuery) {
// TODO: move to StructuredQuery?
diff --git a/frontend/src/metabase/query_builder/actions.js b/frontend/src/metabase/query_builder/actions.js
index 824f7053a9b..3ec457cc102 100644
--- a/frontend/src/metabase/query_builder/actions.js
+++ b/frontend/src/metabase/query_builder/actions.js
@@ -53,6 +53,7 @@ import querystring from "querystring";
import StructuredQuery from "metabase-lib/lib/queries/StructuredQuery";
import NativeQuery from "metabase-lib/lib/queries/NativeQuery";
+import { getSensibleDisplays } from "metabase/visualizations";
import { getCardAfterVisualizationClick } from "metabase/visualizations/lib/utils";
import { getPersistableDefaultSettingsForSeries } from "metabase/visualizations/lib/settings/visualization";
@@ -450,6 +451,14 @@ export const initializeQB = (location, params) => {
await dispatch(loadMetadataForCard(card));
}
+ let question = card && new Question(card, getMetadata(getState()));
+ if (params.cardId) {
+ // loading a saved question prevents auto-viz selection
+ question = question && question.setSelectedDisplay(question.display());
+ }
+
+ card = question && question.card();
+
// Update the question to Redux state together with the initial state of UI controls
dispatch.action(INITIALIZE_QB, {
card,
@@ -457,8 +466,6 @@ export const initializeQB = (location, params) => {
uiControls,
});
- const question = card && new Question(card, getMetadata(getState()));
-
// if we have loaded up a card that we can run then lets kick that off as well
// but don't bother for "notebook" mode
if (question && uiControls.queryBuilderMode !== "notebook") {
@@ -829,7 +836,15 @@ export const apiCreateQuestion = question => {
createdQuestion.query().datasetQuery().type,
);
- dispatch.action(API_CREATE_QUESTION, createdQuestion.card());
+ // Saving a card, locks in the current display as though it had been
+ // selected in the UI. We also copy over `sensibleDisplays` since those were
+ // not persisted onto `createdQuestion`.
+ const card = createdQuestion
+ .setSensibleDisplays(question.sensibleDisplays())
+ .setSelectedDisplay(question.display())
+ .card();
+
+ dispatch.action(API_CREATE_QUESTION, card);
};
};
@@ -921,9 +936,7 @@ export const runQuestionQuery = ({
ignoreCache: ignoreCache,
isDirty: cardIsDirty,
})
- .then(queryResults =>
- dispatch(queryCompleted(question.card(), queryResults)),
- )
+ .then(queryResults => dispatch(queryCompleted(question, queryResults)))
.catch(error => dispatch(queryErrored(startTime, error)));
MetabaseAnalytics.trackEvent(
@@ -945,50 +958,16 @@ export const runQuestionQuery = ({
export const CLEAR_QUERY_RESULT = "metabase/query_builder/CLEAR_QUERY_RESULT";
export const clearQueryResult = createAction(CLEAR_QUERY_RESULT);
-const getDisplayTypeForCard = (card, queryResults) => {
- // TODO Atte Keinänen 6/1/17: Make a holistic decision based on all queryResults, not just one
- // This method seems to has been a candidate for a rewrite anyway
- const queryResult = queryResults[0];
-
- let cardDisplay = card.display;
-
- // try a little logic to pick a smart display for the data
- // TODO: less hard-coded rules for picking chart type
- const isScalarVisualization =
- card.display === "scalar" ||
- card.display === "progress" ||
- card.display === "gauge";
- if (
- !isScalarVisualization &&
- queryResult.data.rows &&
- queryResult.data.rows.length === 1 &&
- queryResult.data.cols.length === 1
- ) {
- // if we have a 1x1 data result then this should always be viewed as a scalar
- cardDisplay = "scalar";
- } else if (
- isScalarVisualization &&
- queryResult.data.rows &&
- (queryResult.data.rows.length > 1 || queryResult.data.cols.length > 1)
- ) {
- // any time we were a scalar and now have more than 1x1 data switch to table view
- cardDisplay = "table";
- } else if (!card.display) {
- // if our query aggregation is "rows" then ALWAYS set the display to "table"
- cardDisplay = "table";
- }
-
- return cardDisplay;
-};
-
export const QUERY_COMPLETED = "metabase/qb/QUERY_COMPLETED";
-export const queryCompleted = (card, queryResults) => {
+export const queryCompleted = (question, queryResults) => {
return async (dispatch, getState) => {
- dispatch.action(QUERY_COMPLETED, {
- card,
- cardDisplay: getDisplayTypeForCard(card, queryResults),
- queryResults,
- });
+ const [{ data }] = queryResults;
+ const card = question
+ .setSensibleDisplays(getSensibleDisplays(data))
+ .setDefaultDisplay()
+ .switchTableScalar(data)
+ .card();
+ dispatch.action(QUERY_COMPLETED, { card, queryResults });
};
};
diff --git a/frontend/src/metabase/query_builder/components/view/sidebars/ChartTypeSidebar.jsx b/frontend/src/metabase/query_builder/components/view/sidebars/ChartTypeSidebar.jsx
index 73d5b9ef2f4..96915c8d6e7 100644
--- a/frontend/src/metabase/query_builder/components/view/sidebars/ChartTypeSidebar.jsx
+++ b/frontend/src/metabase/query_builder/components/view/sidebars/ChartTypeSidebar.jsx
@@ -60,7 +60,9 @@ const ChartTypeSidebar = ({
visualization.isSensible(result.data)
}
onClick={() => {
- question.setDisplay(type).update(null, { reload: false });
+ question
+ .setSelectedDisplay(type)
+ .update(null, { reload: false });
onOpenChartSettings({ section: t`Data` });
setUIControls({ isShowingRawTable: false });
}}
diff --git a/frontend/src/metabase/query_builder/reducers.js b/frontend/src/metabase/query_builder/reducers.js
index fd807908bae..884e4872ee8 100644
--- a/frontend/src/metabase/query_builder/reducers.js
+++ b/frontend/src/metabase/query_builder/reducers.js
@@ -199,7 +199,9 @@ export const card = handleActions(
[QUERY_COMPLETED]: {
next: (state, { payload }) => ({
...state,
- display: payload.cardDisplay,
+ sensibleDisplays: payload.card.sensibleDisplays,
+ selectedDisplay: payload.card.selectedDisplay,
+ display: payload.card.display,
}),
},
diff --git a/frontend/src/metabase/visualizations/index.js b/frontend/src/metabase/visualizations/index.js
index 01f32feedae..e48272017c1 100644
--- a/frontend/src/metabase/visualizations/index.js
+++ b/frontend/src/metabase/visualizations/index.js
@@ -28,6 +28,12 @@ visualizations.get = function(key) {
return Map.prototype.get.call(this, key) || aliases.get(key) || Table;
};
+export function getSensibleDisplays(data) {
+ return Array.from(visualizations)
+ .filter(([, viz]) => viz.isSensible && viz.isSensible(data))
+ .map(([display]) => display);
+}
+
export function registerVisualization(visualization) {
if (visualization == null) {
throw new Error(t`Visualization is null`);
diff --git a/frontend/src/metabase/visualizations/visualizations/Map.jsx b/frontend/src/metabase/visualizations/visualizations/Map.jsx
index 44782555bc9..e37b6476df9 100644
--- a/frontend/src/metabase/visualizations/visualizations/Map.jsx
+++ b/frontend/src/metabase/visualizations/visualizations/Map.jsx
@@ -45,7 +45,10 @@ export default class Map extends Component {
static minSize = { width: 4, height: 4 };
static isSensible({ cols, rows }) {
- return true;
+ return (
+ PinMap.isSensible({ cols, rows }) ||
+ ChoroplethMap.isSensible({ cols, rows })
+ );
}
static placeholderSeries = [
diff --git a/frontend/test/metabase-lib/lib/Question.unit.spec.js b/frontend/test/metabase-lib/lib/Question.unit.spec.js
index 774b5580847..cf3c1578c6d 100644
--- a/frontend/test/metabase-lib/lib/Question.unit.spec.js
+++ b/frontend/test/metabase-lib/lib/Question.unit.spec.js
@@ -267,6 +267,25 @@ describe("Question", () => {
expect(scalarQuestion.display()).toBe("scalar");
});
});
+ describe("setDefaultDisplay", () => {
+ it("sets display to 'scalar' for order count", () => {
+ const question = new Question(
+ orders_count_card,
+ metadata,
+ ).setDefaultDisplay();
+
+ expect(question.display()).toBe("scalar");
+ });
+
+ it("should not set the display to scalar table was selected", () => {
+ const question = new Question(orders_count_card, metadata)
+ .setSelectedDisplay("table")
+ .setSensibleDisplays(["table", "scalar"])
+ .setDefaultDisplay();
+
+ expect(question.display()).toBe("table");
+ });
+ });
});
// TODO: These are mode-dependent and should probably be tied to modes
diff --git a/frontend/test/metabase/scenarios/query_builder.e2e.spec.js b/frontend/test/metabase/scenarios/query_builder.e2e.spec.js
index 28273b2f99b..5341043dd1f 100644
--- a/frontend/test/metabase/scenarios/query_builder.e2e.spec.js
+++ b/frontend/test/metabase/scenarios/query_builder.e2e.spec.js
@@ -31,9 +31,7 @@ describe("query builder", () => {
await app.async.findByText("Simple question").click();
await waitForAllRequestsToComplete(); // race condition in DataSelector with loading metadata
- try {
- await app.async.findByText("Sample Dataset").click(); // not needed if there's only one database
- } catch (e) {}
+ await maybeClickSampleDataset(app);
await app.async.findByText("Orders").click();
await app.async.findByText("37.65");
});
@@ -47,9 +45,7 @@ describe("query builder", () => {
await app.async.findByText("Custom question").click();
await waitForAllRequestsToComplete(); // race condition in DataSelector with loading metadata
- try {
- await app.async.findByText("Sample Dataset").click(); // not needed if there's only one database
- } catch (e) {}
+ await maybeClickSampleDataset(app);
await app.async.findByText("Orders").click();
await app.async.findByText("Visualize").click();
await app.async.findByText("37.65");
@@ -62,9 +58,7 @@ describe("query builder", () => {
await app.async.findByText("Custom question").click();
await waitForAllRequestsToComplete(); // race condition in DataSelector with loading metadata
- try {
- await app.async.findByText("Sample Dataset").click(); // not needed if there's only one database
- } catch (e) {}
+ await maybeClickSampleDataset(app);
await app.async.findByText("Orders").click();
await app.async.findByText("Pick the metric you want to see").click();
await app.async.findByText("Count of rows").click();
@@ -103,3 +97,14 @@ describe("query builder", () => {
});
});
});
+
+// This isn't needed if there's only one db. In that case, clicking "Sample
+// Dataset" will actually take you back to select a db again.
+async function maybeClickSampleDataset(app) {
+ try {
+ const sampleDataset = await app.async.findByText("Sample Dataset");
+ if (sampleDataset.hasClass("List-section-title")) {
+ sampleDataset.click();
+ }
+ } catch (e) {}
+}
--
GitLab