Merge pull request #3474 from metabase/fix-viz-error-logic

Fix viz error logic

frontend/src/metabase/dashboard/components/DashCard.jsx

@@ -2,7 +2,7 @@ import React, { Component, PropTypes } from "react";
 import ReactDOM from "react-dom";
 
 import visualizations, { getVisualizationRaw } from "metabase/visualizations";
-import Visualization from "metabase/visualizations/components/Visualization.jsx";
+import Visualization, { ERROR_MESSAGE_GENERIC, ERROR_MESSAGE_PERMISSION } from "metabase/visualizations/components/Visualization.jsx";
 
 import ModalWithTrigger from "metabase/components/ModalWithTrigger.jsx";
 import ChartSettings from "metabase/visualizations/components/ChartSettings.jsx";
@@ -15,9 +15,6 @@ import cx from "classnames";
 import _ from "underscore";
 import { getIn } from "icepick";
 
-const ERROR_MESSAGE_GENERIC = "There was a problem displaying this chart.";
-const ERROR_MESSAGE_PERMISSION = "Sorry, you don't have permission to see this card."
-
 export default class DashCard extends Component {
     constructor(props, context) {
         super(props, context);

frontend/src/metabase/visualizations/Funnel.jsx

@@ -5,7 +5,6 @@ import BarChart from "./BarChart.jsx";
 
 import { formatValue } from "metabase/lib/formatting";
 import { getSettings } from "metabase/lib/visualization_settings";
-import { isNumeric } from "metabase/lib/schema_metadata";
 import i from "icepick";
 
 export default class Funnel extends Component {
@@ -23,9 +22,8 @@ export default class Funnel extends Component {
     }
 
     static checkRenderable(cols, rows) {
-        if (!isNumeric(cols[0])) {
-            throw new Error("Funnel visualization requires a number.");
-        }
+        // leaving this blank for now since it should be difficult to get into an invalid state
+        // TODO: we should really change checkRenderable to take the entire `series` object
     }
 
     static transformSeries(series) {

frontend/src/metabase/visualizations/components/Visualization.jsx

+/* eslint "react/prop-types": "warn" */
+
 import React, { Component, PropTypes } from "react";
 
 import ExplicitSize from "metabase/components/ExplicitSize.jsx";
@@ -16,6 +18,9 @@ import { assoc, getIn } from "icepick";
 import _ from "underscore";
 import cx from "classnames";
 
+export const ERROR_MESSAGE_GENERIC = "There was a problem displaying this chart.";
+export const ERROR_MESSAGE_PERMISSION = "Sorry, you don't have permission to see this card."
+
 @ExplicitSize
 export default class Visualization extends Component {
     constructor(props, context) {
@@ -33,9 +38,31 @@ export default class Visualization extends Component {
     static propTypes = {
         series: PropTypes.array.isRequired,
 
+        className: PropTypes.string,
+
         isDashboard: PropTypes.bool,
         isEditing: PropTypes.bool,
 
+        actionButtons: PropTypes.node,
+
+        // errors
+        error: PropTypes.string,
+        errorIcon: PropTypes.string,
+
+        // slow card warnings
+        isSlow: PropTypes.bool,
+        expectedDuration: PropTypes.number,
+
+        // injected by ExplicitSize
+        width: PropTypes.number,
+        height: PropTypes.number,
+
+        // settings overrides from settings panel
+        settings: PropTypes.object,
+
+        // used for showing content in place of visualization, e.x. dashcard filter mapping
+        replacementContent: PropTypes.node,
+
         // used by TableInteractive
         setSortFn: PropTypes.func,
         cellIsClickableFn: PropTypes.func,
@@ -118,6 +145,12 @@ export default class Visualization extends Component {
                 }
             }
         }
+
+        // if on dashoard, and error didn't come from props replace it with the generic error message
+        if (isDashboard && error && this.props.error !== error) {
+            error = ERROR_MESSAGE_GENERIC;
+        }
+
         if (!error) {
             noResults = getIn(series, [0, "data", "rows", "length"]) === 0;
         }
@@ -136,7 +169,13 @@ export default class Visualization extends Component {
                 { isDashboard && (settings["card.title"] || extra) && (loading || error || !CardVisualization.noHeader) || replacementContent ?
                     <div className="p1 flex-no-shrink">
                         <LegendHeader
-                            series={[{ card: { name: settings["card.title"] }}]}
+                            series={
+                                settings["card.title"] ?
+                                    // if we have a card title set, use it
+                                    [{ card: { name: settings["card.title"] }}] :
+                                    // otherwise use the original series
+                                    series
+                            }
                             actionButtons={extra}
                             settings={settings}
                         />

frontend/test/karma.conf.js

@@ -2,13 +2,17 @@ var webpackConfig = require('../../webpack.config');
 webpackConfig.module.postLoaders = [
     { test: /\.js$/, exclude: /(\.spec\.js|vendor|node_modules)/, loader: 'istanbul-instrumenter' }
 ];
+webpackConfig.module.loaders.forEach(function(loader) {
+    loader.loader = loader.loader.replace(/^.*extract-text-webpack-plugin[^!]+!/, "");
+});
 
 module.exports = function(config) {
     config.set({
         basePath: '../',
         files: [
             'test/metabase-bootstrap.js',
-            'test/unit/**/*.spec.js'
+            // prevent tests from running twice: https://github.com/nikku/karma-browserify/issues/67#issuecomment-84448491
+            { pattern: 'test/unit/**/*.spec.js', watched: false, included: true, served: true }
         ],
         exclude: [
         ],
@@ -26,7 +30,11 @@ module.exports = function(config) {
         ],
         webpack: {
             resolve: webpackConfig.resolve,
-            module: webpackConfig.module
+            module: webpackConfig.module,
+            postcss: webpackConfig.postcss
+        },
+        webpackMiddleware: {
+            stats: "errors-only"
         },
         webpackMiddleware: {
             stats: "errors-only",

frontend/test/metabase-bootstrap.js

 import "metabase/vendor";
+import "metabase/css/index.css";
 
 window.MetabaseBootstrap = {
     timezones: [

frontend/test/unit/support/visualizations.js

+
+export function makeCard(card) {
+    return {
+        name: "card",
+        dataset_query: {},
+        visualization_settings: {},
+        display: "scalar",
+        ...card
+    }
+}
+
+export function makeData(cols, rows) {
+    return {
+        cols,
+        rows
+    };
+}
+
+export const Column = (col = {}) => ({
+    ...col,
+    name: col.name || "column_name",
+    display_name: col.display_name || col.name || "column_display_name"
+});
+
+export const DateTimeColumn = (col = {}) => Column({ "base_type" : "type/DateTime", "special_type" : null, ...col });
+export const NumberColumn   = (col = {}) => Column({ "base_type" : "type/Integer", "special_type" : "type/Number", ...col });
+export const StringColumn   = (col = {}) => Column({ "base_type" : "type/Text", "special_type" : null, ...col });
+
+export const Card = (name, ...overrides) => deepExtend({
+    card: {
+        name: name + "_name",
+        visualization_settings: {}
+    }
+}, ...overrides);
+
+export const ScalarCard = (name, ...overrides) =>
+    Card(name, {
+        card: {
+            display: "scalar",
+        },
+        data: {
+            cols: [NumberColumn({ name: name + "_col0" })],
+            rows: [[1]]
+        }
+    }, ...overrides);
+
+export const LineCard = (name, ...overrides) =>
+    Card(name, {
+        card: {
+            display: "line",
+        },
+        data: {
+            cols: [StringColumn({ name: name + "_col0" }), NumberColumn({ name: name + "_col1" })],
+            rows: [["a",0],["b",1]]
+        }
+    }, ...overrides);
+
+export const MultiseriesLineCard = (name, ...overrides) =>
+    Card(name, {
+        card: {
+            name: name + "_name",
+            display: "line",
+            visualization_settings: {}
+        },
+        data: {
+            cols: [StringColumn({ name: name + "_col0" }), StringColumn({ name: name + "_col1" }), NumberColumn({ name: name + "_col2" })],
+            rows: [
+                [name + "_cat1", "x", 0], [name + "_cat1", "y", 1], [name + "_cat1", "z", 1],
+                [name + "_cat2", "x", 2], [name + "_cat2", "y", 3], [name + "_cat2", "z", 4]
+            ]
+        }
+    }, ...overrides);
+
+function deepExtend(target, ...sources) {
+    for (const source of sources) {
+        for (const prop in source) {
+            if (source.hasOwnProperty(prop)) {
+                if (target[prop] && typeof target[prop] === 'object' && source[prop] && typeof source[prop] === 'object') {
+                    deepExtend(target[prop], source[prop]);
+                } else {
+                    target[prop] = source[prop];
+                }
+            }
+        }
+    }
+    return target;
+}

frontend/test/unit/visualizations/components/Visualization.spec.js

+
+import React from "react";
+import { renderIntoDocument, scryRenderedComponentsWithType as scryWithType } from "react-addons-test-utils";
+
+import Visualization from "metabase/visualizations/components/Visualization.jsx";
+
+import LegendHeader from "metabase/visualizations/components/LegendHeader.jsx";
+import LegendItem from "metabase/visualizations/components/LegendItem.jsx";
+
+import { ScalarCard, LineCard, MultiseriesLineCard } from "../../support/visualizations";
+
+describe("Visualization", () => {
+    describe("not in dashboard", () => {
+        describe("scalar card", () => {
+            it("should not render title", () => {
+                let viz = renderVisualization({ series: [ScalarCard("Foo")] });
+                expect(getTitles(viz)).toEqual([]);
+            });
+        });
+
+        describe("line card", () => {
+            it("should not render card title", () => {
+                let viz = renderVisualization({ series: [LineCard("Foo")] });
+                expect(getTitles(viz)).toEqual([]);
+            });
+            it("should not render setting title", () => {
+                let viz = renderVisualization({ series: [LineCard("Foo", { card: { visualization_settings: { "card.title": "Foo_title" }}})] });
+                expect(getTitles(viz)).toEqual([]);
+            });
+            it("should render breakout multiseries titles", () => {
+                let viz = renderVisualization({ series: [MultiseriesLineCard("Foo")] });
+                expect(getTitles(viz)).toEqual([
+                    ["Foo_cat1", "Foo_cat2"]
+                ]);
+            });
+        });
+    });
+
+    describe("in dashboard", () => {
+        describe("scalar card", () => {
+            it("should not render title", () => {
+                let viz = renderVisualization({ series: [ScalarCard("Foo")], isDashboard: true });
+                expect(getTitles(viz)).toEqual([]);
+            });
+            it("should render title when loading", () => {
+                let viz = renderVisualization({ series: [ScalarCard("Foo", { data: null })], isDashboard: true });
+                expect(getTitles(viz)).toEqual([
+                    ["Foo_name"]
+                ]);
+            });
+            it("should render title when there's an error", () => {
+                let viz = renderVisualization({ series: [ScalarCard("Foo")], isDashboard: true, error: "oops" });
+                expect(getTitles(viz)).toEqual([
+                    ["Foo_name"]
+                ]);
+            });
+            it("should not render scalar title", () => {
+                let viz = renderVisualization({ series: [ScalarCard("Foo")], isDashboard: true });
+                expect(getTitles(viz)).toEqual([]);
+            });
+            it("should render multi scalar titles", () => {
+                let viz = renderVisualization({ series: [ScalarCard("Foo"), ScalarCard("Bar")], isDashboard: true });
+                expect(getTitles(viz)).toEqual([
+                    ["Foo_name", "Bar_name"]
+                ]);
+            });
+        });
+
+        describe("line card", () => {
+            it("should render normal title", () => {
+                let viz = renderVisualization({ series: [LineCard("Foo")], isDashboard: true });
+                expect(getTitles(viz)).toEqual([
+                    ["Foo_name"]
+                ]);
+            });
+            it("should render normal title and breakout multiseries titles", () => {
+                let viz = renderVisualization({ series: [MultiseriesLineCard("Foo")], isDashboard: true });
+                expect(getTitles(viz)).toEqual([
+                    ["Foo_name"],
+                    ["Foo_cat1", "Foo_cat2"]
+                ]);
+            });
+            it("should render dashboard multiseries titles", () => {
+                let viz = renderVisualization({ series: [LineCard("Foo"), LineCard("Bar")], isDashboard: true });
+                expect(getTitles(viz)).toEqual([
+                    ["Foo_col1", "Bar_col1"]
+                ]);
+            });
+            it("should render dashboard multiseries titles and chart setting title", () => {
+                let viz = renderVisualization({ series: [
+                    LineCard("Foo", { card: { visualization_settings: { "card.title": "Foo_title" }}}),
+                    LineCard("Bar")
+                ], isDashboard: true });
+                expect(getTitles(viz)).toEqual([
+                    ["Foo_title"],
+                    ["Foo_col1", "Bar_col1"]
+                ]);
+            });
+            it("should render multiple breakout multiseries titles", () => {
+                let viz = renderVisualization({ series: [MultiseriesLineCard("Foo"), MultiseriesLineCard("Bar")], isDashboard: true });
+                expect(getTitles(viz)).toEqual([
+                    ["Foo_cat1", "Foo_cat2", "Bar_cat1", "Bar_cat2"]
+                ]);
+            });
+        });
+    });
+});
+
+function renderVisualization(props) {
+    return renderIntoDocument(<Visualization className="spread" {...props} />);
+}
+
+function getTitles(viz) {
+    return scryWithType(viz, LegendHeader).map(header =>
+        scryWithType(header, LegendItem).map(item =>
+            item.props.title
+        )
+    );
+}

frontend/test/unit/visualizations/lib/LineAreaBarRenderer.spec.js

@@ -4,10 +4,7 @@ import { formatValue } from "metabase/lib/formatting";
 
 import d3 from "d3";
 
-const Column = (col = {}) => ({ name: "x", display_name: "x", ...col })
-const DateTimeColumn = (col = {}) => Column({ "base_type" : "type/DateTime", "special_type" : null, ...col })
-const NumberColumn = (col = {}) => Column({ "base_type" : "type/Integer", "special_type" : "type/Number", ...col })
-
+import { DateTimeColumn, NumberColumn } from "../../support/visualizations";
 
 let formatTz = (offset) => (offset < 0 ? "-" : "+") + d3.format("02d")(Math.abs(offset)) + ":00"
 

package.json

@@ -115,6 +115,7 @@
     "postcss-import": "^8.0.2",
     "postcss-loader": "^0.8.1",
     "postcss-url": "^5.1.1",
+    "react-addons-test-utils": "^15.3.1",
     "react-hot-loader": "^1.3.0",
     "sauce-connect-launcher": "^0.15.1",
     "selenium-webdriver": "^2.53.3",