Fix up dimension/metric settings, implement for pie chart

frontend/src/metabase/lib/visualization_settings.js

@@ -17,25 +17,24 @@ import ChartSettingInput from "metabase/visualizations/components/settings/Chart
 import ChartSettingInputNumeric from "metabase/visualizations/components/settings/ChartSettingInputNumeric.jsx";
 import ChartSettingSelect from "metabase/visualizations/components/settings/ChartSettingSelect.jsx";
 import ChartSettingToggle from "metabase/visualizations/components/settings/ChartSettingToggle.jsx";
-import ChartSettingFieldPicker from "metabase/visualizations/components/settings/ChartSettingFieldPicker.jsx";
+import ChartSettingFieldsPicker from "metabase/visualizations/components/settings/ChartSettingFieldsPicker.jsx";
 import ChartSettingColorsPicker from "metabase/visualizations/components/settings/ChartSettingColorsPicker.jsx";
 import ChartSettingOrderedFields from "metabase/visualizations/components/settings/ChartSettingOrderedFields.jsx";
 
-function dimensionAndMetricsAreValid(dimension, metric, cols) {
+function columnsAreValid(colNames, data, filter = () => true) {
+    if (typeof colNames === "string") {
+        colNames = [colNames]
+    }
+    if (!data || !Array.isArray(colNames)) {
+        return false;
+    }
     const colsByName = {};
-    for (const col of cols) {
+    for (const col of data.cols) {
         colsByName[col.name] = col;
     }
-    return (
-        (dimension != null &&
-            dimension.reduce((acc, name) =>
-                acc && (name == undefined || (colsByName[name] && isDimension(colsByName[name])))
-            , true)) &&
-        (metric != null &&
-            metric.reduce((acc, name) =>
-                acc && (name == undefined || (colsByName[name] && isMetric(colsByName[name])))
-            , true))
-    );
+    return colNames.reduce((acc, name) =>
+        acc && (name == undefined || (colsByName[name] && filter(colsByName[name])))
+    , true);
 }
 
 function getSeriesTitles([{ data: { rows, cols } }], vizSettings) {
@@ -85,6 +84,21 @@ function getDefaultDimensionsAndMetrics([{ data: { cols, rows } }]) {
     }
 }
 
+function getDefaultDimensionAndMetric([{ data: { cols, rows } }]) {
+    const type = getChartTypeFromData(cols, rows, false);
+    if (type === DIMENSION_METRIC) {
+        return {
+            dimension: cols[0].name,
+            metric: cols[1].name
+        };
+    } else {
+        return {
+            dimension: null,
+            metric: null
+        };
+    }
+}
+
 function getOptionFromColumn(col) {
     return {
         name: getFriendlyName(col),
@@ -98,19 +112,12 @@ const SETTINGS = {
     "graph.dimensions": {
         section: "Data",
         title: "X-axis",
-        widget: ChartSettingFieldPicker,
-        getValue: (series, vizSettings) => {
-            const [{ card, data }] = series;
-            if (data && dimensionAndMetricsAreValid(
-                card.visualization_settings["graph.dimensions"],
-                card.visualization_settings["graph.metrics"],
-                data.cols
-            )) {
-                return card.visualization_settings["graph.dimensions"];
-            } else {
-                return getDefaultDimensionsAndMetrics(series).dimensions;
-            }
-        },
+        widget: ChartSettingFieldsPicker,
+        isValid: ([{ card, data }], vizSettings) =>
+            columnsAreValid(card.visualization_settings["graph.dimensions"], data, isDimension) &&
+            columnsAreValid(card.visualization_settings["graph.metrics"], data, isMetric),
+        getDefault: (series, vizSettings) =>
+            getDefaultDimensionsAndMetrics(series).dimensions,
         getProps: ([{ card, data }], vizSettings) => {
             const value = vizSettings["graph.dimensions"];
             const options = data.cols.filter(isDimension).map(getOptionFromColumn);
@@ -124,19 +131,12 @@ const SETTINGS = {
     "graph.metrics": {
         section: "Data",
         title: "Y-axis",
-        widget: ChartSettingFieldPicker,
-        getValue: (series, vizSettings) => {
-            const [{ card, data }] = series;
-            if (data && dimensionAndMetricsAreValid(
-                card.visualization_settings["graph.dimensions"],
-                card.visualization_settings["graph.metrics"],
-                data.cols
-            )) {
-                return card.visualization_settings["graph.metrics"];
-            } else {
-                return getDefaultDimensionsAndMetrics(series).metrics;
-            }
-        },
+        widget: ChartSettingFieldsPicker,
+        isValid: ([{ card, data }], vizSettings) =>
+            columnsAreValid(card.visualization_settings["graph.dimensions"], data, isDimension) &&
+            columnsAreValid(card.visualization_settings["graph.metrics"], data, isMetric),
+        getDefault: (series, vizSettings) =>
+            getDefaultDimensionsAndMetrics(series).metrics,
         getProps: ([{ card, data }], vizSettings) => {
             const value = vizSettings["graph.dimensions"];
             const options = data.cols.filter(isMetric).map(getOptionFromColumn);
@@ -275,12 +275,26 @@ const SETTINGS = {
     "pie.dimension": {
         section: "Data",
         title: "Measure",
-        widget: ChartSettingFieldPicker
+        widget: ChartSettingSelect,
+        isValid: ([{ card, data }], vizSettings) =>
+            columnsAreValid(card.visualization_settings["pie.dimension"], data, isDimension),
+        getDefault: (series, vizSettings) =>
+            getDefaultDimensionAndMetric(series).dimension,
+        getProps: ([{ card, data: { cols }}]) => ({
+            options: cols.filter(isDimension).map(getOptionFromColumn)
+        }),
     },
     "pie.metric": {
         section: "Data",
         title: "Slice by",
-        widget: ChartSettingFieldPicker
+        widget: ChartSettingSelect,
+        isValid: ([{ card, data }], vizSettings) =>
+            columnsAreValid(card.visualization_settings["pie.metric"], data, isMetric),
+        getDefault: (series, vizSettings) =>
+            getDefaultDimensionAndMetric(series).metric,
+        getProps: ([{ card, data: { cols }}]) => ({
+            options: cols.filter(isMetric).map(getOptionFromColumn)
+        }),
     },
     "pie.show_legend": {
         section: "Legend",
@@ -349,6 +363,9 @@ const SETTINGS = {
         title: "Fields to include",
         widget: ChartSettingOrderedFields,
         getHidden: (series, vizSettings) => vizSettings["table.pivot"],
+        isValid: ([{ card, data }]) =>
+            card.visualization_settings["table.fields"] &&
+            columnsAreValid(card.visualization_settings["table.fields"].map(x => x.name), data),
         getDefault: ([{ data: { cols }}]) => cols.map(col => ({
             name: col.name,
             enabled: true
@@ -425,8 +442,11 @@ const SETTINGS = {
     "map.metric": {
         title: "Metric field",
         widget: ChartSettingSelect,
-        getDefault: ([{ card, data: { cols }}]) =>
-            (_.find(cols, isMetric) || {}).name,
+        isValid: ([{ card, data }], vizSettings) =>
+            card.visualization_settings["map.metric"] &&
+            columnsAreValid(card.visualization_settings["map.metric"], data, isMetric),
+        getDefault: (series, vizSettings) =>
+            getDefaultDimensionAndMetric(series).metric,
         getProps: ([{ card, data: { cols }}]) => ({
             options: cols.filter(isMetric).map(getOptionFromColumn)
         }),
@@ -435,8 +455,11 @@ const SETTINGS = {
     "map.dimension": {
         title: "Region field",
         widget: ChartSettingSelect,
-        getDefault: ([{ card, data: { cols }}]) =>
-            (_.find(cols, isDimension) || {}).name,
+        isValid: ([{ card, data }], vizSettings) =>
+            card.visualization_settings["map.dimension"] &&
+            columnsAreValid(card.visualization_settings["map.dimension"], data, isDimension),
+        getDefault: (series, vizSettings) =>
+            getDefaultDimensionAndMetric(series).dimension,
         getProps: ([{ card, data: { cols }}]) => ({
             options: cols.filter(isDimension).map(getOptionFromColumn)
         }),
@@ -481,17 +504,28 @@ function getSetting(id, vizSettings, series) {
         getSetting(dependentId, vizSettings, series);
     }
 
-    if (settingDef.getValue) {
-        vizSettings[id] = settingDef.getValue(series, vizSettings);
-    } else if (card.visualization_settings[id] === undefined && settingDef.getDefault) {
-        vizSettings[id] = settingDef.getDefault(series, vizSettings);
-    } else if (card.visualization_settings[id] === undefined && "default" in settingDef) {
-        vizSettings[id] = settingDef.default;
-    } else if (card.visualization_settings[id] !== undefined) {
-        vizSettings[id] = card.visualization_settings[id];
-    }
+    try {
+        if (settingDef.getValue) {
+            return vizSettings[id] = settingDef.getValue(series, vizSettings);
+        }
 
-    return vizSettings[id];
+        if (card.visualization_settings[id] !== undefined) {
+            if (!settingDef.isValid || settingDef.isValid(series, vizSettings)) {
+                return vizSettings[id] = card.visualization_settings[id];
+            }
+        }
+
+        if (settingDef.getDefault) {
+            return vizSettings[id] = settingDef.getDefault(series, vizSettings);
+        }
+
+        if ("default" in settingDef) {
+            return vizSettings[id] = settingDef.default;
+        }
+    } catch (e) {
+        console.error("Error getting setting", id, e);
+    }
+    return vizSettings[id] = undefined;
 }
 
 function getSettingIdsForSeries(series) {

frontend/src/metabase/visualizations/PieChart.jsx

@@ -5,7 +5,7 @@ import styles from "./PieChart.css";
 import ChartTooltip from "./components/ChartTooltip.jsx";
 import ChartWithLegend from "./components/ChartWithLegend.jsx";
 
-import { MinColumnsError } from "metabase/visualizations/lib/errors";
+import { MinColumnsError, ChartSettingsError } from "metabase/visualizations/lib/errors";
 import { getFriendlyName } from "metabase/visualizations/lib/utils";
 
 import { formatValue } from "metabase/lib/formatting";
@@ -35,8 +35,10 @@ export default class PieChart extends Component {
         return cols.length === 2;
     }
 
-    static checkRenderable(cols, rows) {
-        if (cols.length < 2) { throw new MinColumnsError(2, cols.length); }
+    static checkRenderable(cols, rows, settings) {
+        if (!settings["pie.dimension"] || !settings["pie.metric"]) {
+            throw new ChartSettingsError("Please select columns in the chart settings.", "Data");
+        }
     }
 
     componentDidUpdate() {
@@ -51,22 +53,25 @@ export default class PieChart extends Component {
 
     render() {
         const { series, hovered, onHoverChange, className, gridSize, settings } = this.props;
-        const { data } = series[0];
 
-        const formatDimension = (dimension, jsx = true) => formatValue(dimension, { column: data.cols[0], jsx, majorWidth: 0 })
-        const formatMetric    =    (metric, jsx = true) => formatValue(metric, { column: data.cols[1], jsx, majorWidth: 0 })
+        const [{ data: { cols, rows }}] = series;
+        const dimensionIndex = _.findIndex(cols, (col) => col.name === settings["pie.dimension"]);
+        const metricIndex = _.findIndex(cols, (col) => col.name === settings["pie.metric"]);
+
+        const formatDimension = (dimension, jsx = true) => formatValue(dimension, { column: cols[dimensionIndex], jsx, majorWidth: 0 })
+        const formatMetric    =    (metric, jsx = true) => formatValue(metric, { column: cols[metricIndex], jsx, majorWidth: 0 })
         const formatPercent   =               (percent) => (100 * percent).toFixed(2) + "%"
 
-        let total = data.rows.reduce((sum, row) => sum + row[1], 0);
+        let total = rows.reduce((sum, row) => sum + row[metricIndex], 0);
 
         // use standard colors for up to 5 values otherwise use color harmony to help differentiate slices
-        let sliceColors = Object.values(data.rows.length > 5 ? colors.harmony : colors.normal);
+        let sliceColors = Object.values(rows.length > 5 ? colors.harmony : colors.normal);
 
-        let [slices, others] = _.chain(data.rows)
-            .map(([key, value], index) => ({
-                key,
-                value,
-                percentage: value / total,
+        let [slices, others] = _.chain(rows)
+            .map((row, index) => ({
+                key: row[dimensionIndex],
+                value: row[metricIndex],
+                percentage: row[metricIndex] / total,
                 color: sliceColors[index % sliceColors.length]
             }))
             .partition((d) => d.percentage > SLICE_THRESHOLD)
@@ -112,8 +117,8 @@ export default class PieChart extends Component {
                     value: formatMetric(o.value, false)
                 }))
             : [
-                { key: getFriendlyName(data.cols[0]), value: formatDimension(slices[index].key) },
-                { key: getFriendlyName(data.cols[1]), value: formatMetric(slices[index].value) },
+                { key: getFriendlyName(cols[dimensionIndex]), value: formatDimension(slices[index].key) },
+                { key: getFriendlyName(cols[metricIndex]), value: formatMetric(slices[index].value) },
                 { key: "Percentage", value: formatPercent(slices[index].percentage) }
             ]
         });

frontend/src/metabase/visualizations/components/settings/ChartSettingFieldPicker.jsx → frontend/src/metabase/visualizations/components/settings/ChartSettingFieldsPicker.jsx

@@ -5,9 +5,9 @@ import cx from "classnames";
 
 import ChartSettingSelect from "./ChartSettingSelect.jsx";
 
-const ChartSettingFieldPicker = ({ value = [], onChange, options, addAnother }) =>
+const ChartSettingFieldsPicker = ({ value = [], onChange, options, addAnother }) =>
     <div>
-        { value.map((v, index) =>
+        { Array.isArray(value) ? value.map((v, index) =>
             <div key={index} className="flex align-center">
                 <ChartSettingSelect
                     value={v}
@@ -34,7 +34,7 @@ const ChartSettingFieldPicker = ({ value = [], onChange, options, addAnother })
                     onClick={() => onChange([...value.slice(0, index), ...value.slice(index + 1)])}
                 />
             </div>
-        )}
+        ) : <span className="text-error">error</span>}
         { addAnother &&
             <div className="mt1">
                 <a onClick={() => {
@@ -53,4 +53,4 @@ const ChartSettingFieldPicker = ({ value = [], onChange, options, addAnother })
         }
     </div>
 
-export default ChartSettingFieldPicker;
+export default ChartSettingFieldsPicker;