Update auto-viz selection (#11368)

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?

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 });
   };
 };
 

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 });
                   }}

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,
       }),
     },
 

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`);

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 = [

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

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) {}
+}