From 30adcc9d0ee8284a8275686da7e0f20edf90e6d5 Mon Sep 17 00:00:00 2001
From: Paul Rosenzweig <paulrosenzweig@users.noreply.github.com>
Date: Thu, 14 May 2020 11:14:33 -0400
Subject: [PATCH] Update visualization auto-selection logic (#12476)
---
frontend/src/metabase-lib/lib/Question.js | 42 +++++++---------
frontend/src/metabase/lib/card.js | 1 +
.../src/metabase/query_builder/actions.js | 48 ++++++++++++-------
.../view/sidebars/ChartTypeSidebar.jsx | 5 +-
.../src/metabase/query_builder/reducers.js | 2 -
frontend/src/metabase/visualizations/index.js | 6 ++-
.../metabase-lib/lib/Question.unit.spec.js | 15 +++++-
7 files changed, 68 insertions(+), 51 deletions(-)
diff --git a/frontend/src/metabase-lib/lib/Question.js b/frontend/src/metabase-lib/lib/Question.js
index cd32ea5bd8d..1a803c0df0f 100644
--- a/frontend/src/metabase-lib/lib/Question.js
+++ b/frontend/src/metabase-lib/lib/Question.js
@@ -277,40 +277,30 @@ 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;
+ // locking the display prevents auto-selection
+ lockDisplay(): Question {
+ return this.setDisplayIsLocked(true);
}
-
- // 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));
+ setDisplayIsLocked(locked: boolean): Question {
+ return this.setCard(assoc(this.card(), "displayIsLocked", locked));
}
- sensibleDisplays(): string[] {
- return (this._card && this._card.sensibleDisplays) || [];
+ displayIsLocked(): boolean {
+ return this._card && this._card.displayIsLocked;
}
- // 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());
+ // If we're locked to a display that is no longer "sensible", unlock it.
+ maybeUnlockDisplay(sensibleDisplays): Question {
+ const locked =
+ this.displayIsLocked() && sensibleDisplays.includes(this.display());
+ return this.setDisplayIsLocked(locked);
}
// 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 {
+ if (this.displayIsLocked()) {
+ return this;
+ }
const display = this.display();
const isScalar = ["scalar", "progress", "gauge"].includes(display);
const isOneByOne = rows.length === 1 && cols.length === 1;
@@ -329,7 +319,7 @@ export default class Question {
}
setDefaultDisplay(): Question {
- if (this.shouldNotSetDisplay()) {
+ if (this.displayIsLocked()) {
return this;
}
const query = this.query();
diff --git a/frontend/src/metabase/lib/card.js b/frontend/src/metabase/lib/card.js
index 39458e59fb0..e81ffb65206 100644
--- a/frontend/src/metabase/lib/card.js
+++ b/frontend/src/metabase/lib/card.js
@@ -80,6 +80,7 @@ export function serializeCardForUrl(card) {
description: card.description,
dataset_query: dataset_query,
display: card.display,
+ displayIsLocked: card.displayIsLocked,
parameters: card.parameters,
visualization_settings: card.visualization_settings,
original_card_id: card.original_card_id,
diff --git a/frontend/src/metabase/query_builder/actions.js b/frontend/src/metabase/query_builder/actions.js
index a47cd2e0b33..093d51dadb4 100644
--- a/frontend/src/metabase/query_builder/actions.js
+++ b/frontend/src/metabase/query_builder/actions.js
@@ -442,7 +442,7 @@ export const initializeQB = (location, params) => {
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());
+ question = question && question.lockDisplay();
}
card = question && question.card();
@@ -541,9 +541,11 @@ export const updateCardVisualizationSettings = settings => async (
) => {
const question = getQuestion(getState());
await dispatch(
- updateQuestion(question.updateSettings(settings), { run: "auto" }),
+ updateQuestion(question.updateSettings(settings), {
+ run: "auto",
+ shouldUpdateUrl: true,
+ }),
);
- dispatch(updateUrl(null, { dirty: true }));
};
export const replaceAllCardVisualizationSettings = settings => async (
@@ -552,9 +554,11 @@ export const replaceAllCardVisualizationSettings = settings => async (
) => {
const question = getQuestion(getState());
await dispatch(
- updateQuestion(question.setSettings(settings), { run: "auto" }),
+ updateQuestion(question.setSettings(settings), {
+ run: "auto",
+ shouldUpdateUrl: true,
+ }),
);
- dispatch(updateUrl(null, { dirty: true }));
};
export const UPDATE_TEMPLATE_TAG = "metabase/qb/UPDATE_TEMPLATE_TAG";
@@ -697,7 +701,7 @@ export const navigateToNewCardInsideQB = createThunkAction(
export const UPDATE_QUESTION = "metabase/qb/UPDATE_QUESTION";
export const updateQuestion = (
newQuestion,
- { doNotClearNameAndId = false, run = false } = {},
+ { doNotClearNameAndId = false, run = false, shouldUpdateUrl = false } = {},
) => {
return async (dispatch, getState) => {
const oldQuestion = getQuestion(getState());
@@ -726,6 +730,10 @@ export const updateQuestion = (
// Replace the current question with a new one
await dispatch.action(UPDATE_QUESTION, { card: newQuestion.card() });
+ if (shouldUpdateUrl) {
+ dispatch(updateUrl(null, { dirty: true }));
+ }
+
// See if the template tags editor should be shown/hidden
const oldTagCount = getTemplateTagCount(oldQuestion);
const newTagCount = getTemplateTagCount(newQuestion);
@@ -805,12 +813,8 @@ export const apiCreateQuestion = question => {
);
// 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();
+ // selected in the UI.
+ const card = createdQuestion.lockDisplay().card();
dispatch.action(API_CREATE_QUESTION, card);
};
@@ -936,12 +940,20 @@ export const QUERY_COMPLETED = "metabase/qb/QUERY_COMPLETED";
export const queryCompleted = (question, queryResults) => {
return async (dispatch, getState) => {
const [{ data }] = queryResults;
- const card = question
- .setSensibleDisplays(getSensibleDisplays(data))
- .setDefaultDisplay()
- .switchTableScalar(data)
- .card();
- dispatch.action(QUERY_COMPLETED, { card, queryResults });
+ const originalQuestion = getOriginalQuestion(getState());
+ const dirty =
+ !originalQuestion ||
+ (originalQuestion && question.isDirtyComparedTo(originalQuestion));
+ if (dirty) {
+ // Only update the display if the question is new or has been changed.
+ // Otherwise, trust that the question was saved with the correct display.
+ question = question
+ // if we are going to trigger autoselection logic, check if the locked display no longer is "sensible".
+ .maybeUnlockDisplay(getSensibleDisplays(data))
+ .setDefaultDisplay()
+ .switchTableScalar(data);
+ }
+ dispatch.action(QUERY_COMPLETED, { card: question.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 96915c8d6e7..9383a1f7141 100644
--- a/frontend/src/metabase/query_builder/components/view/sidebars/ChartTypeSidebar.jsx
+++ b/frontend/src/metabase/query_builder/components/view/sidebars/ChartTypeSidebar.jsx
@@ -61,8 +61,9 @@ const ChartTypeSidebar = ({
}
onClick={() => {
question
- .setSelectedDisplay(type)
- .update(null, { reload: false });
+ .setDisplay(type)
+ .lockDisplay(true) // prevent viz auto-selection
+ .update(null, { reload: false, shouldUpdateUrl: true });
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 3e9d03f019f..8a85aef1d80 100644
--- a/frontend/src/metabase/query_builder/reducers.js
+++ b/frontend/src/metabase/query_builder/reducers.js
@@ -199,8 +199,6 @@ export const card = handleActions(
[QUERY_COMPLETED]: {
next: (state, { payload }) => ({
...state,
- 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 22a70c30719..2ff36ac5b66 100644
--- a/frontend/src/metabase/visualizations/index.js
+++ b/frontend/src/metabase/visualizations/index.js
@@ -18,7 +18,11 @@ visualizations.get = function(key) {
export function getSensibleDisplays(data) {
return Array.from(visualizations)
- .filter(([, viz]) => viz.isSensible && viz.isSensible(data))
+ .filter(
+ ([, viz]) =>
+ // don't rule out displays if there's no data
+ data.rows.length <= 1 || (viz.isSensible && viz.isSensible(data)),
+ )
.map(([display]) => display);
}
diff --git a/frontend/test/metabase-lib/lib/Question.unit.spec.js b/frontend/test/metabase-lib/lib/Question.unit.spec.js
index 5ee8df44a0d..5a887937e23 100644
--- a/frontend/test/metabase-lib/lib/Question.unit.spec.js
+++ b/frontend/test/metabase-lib/lib/Question.unit.spec.js
@@ -279,12 +279,23 @@ describe("Question", () => {
it("should not set the display to scalar table was selected", () => {
const question = new Question(orders_count_card, metadata)
- .setSelectedDisplay("table")
- .setSensibleDisplays(["table", "scalar"])
+ .setDisplay("table")
+ .lockDisplay()
+ .maybeUnlockDisplay(["table", "scalar"])
.setDefaultDisplay();
expect(question.display()).toBe("table");
});
+
+ it("should set the display to scalar if funnel was selected", () => {
+ const question = new Question(orders_count_card, metadata)
+ .setDisplay("funnel")
+ .lockDisplay()
+ .maybeUnlockDisplay(["table", "scalar"])
+ .setDefaultDisplay();
+
+ expect(question.display()).toBe("scalar");
+ });
});
});
--
GitLab