Fix bugs with stacked charts and log scales (#10911)

* compute yExtents for stacked charts, display a better min for log bar charts

* remove unused const

frontend/src/metabase/visualizations/lib/LineAreaBarRenderer.js

@@ -233,20 +233,41 @@ function getDimensionsAndGroupsForOther({ series }, datas, warn) {
   return { dimension, groups };
 }
 
+function getYExtentsForGroups(groups) {
+  return groups.map(group => {
+    const sums = new Map();
+    for (const g of group) {
+      for (const { key, value } of g.all()) {
+        const prevValue = sums.get(key) || 0;
+        sums.set(key, prevValue + value);
+      }
+    }
+    return d3.extent(Array.from(sums.values()));
+  });
+}
+
 /// Return an object containing the `dimension` and `groups` for the chart(s).
 /// For normalized stacked charts, this also updates the dispaly names to add a percent in front of the name (e.g. 'Sum' becomes '% Sum')
-function getDimensionsAndGroupsAndUpdateSeriesDisplayNames(props, datas, warn) {
+/// This is only exported for testing.
+export function getDimensionsAndGroupsAndUpdateSeriesDisplayNames(
+  props,
+  datas,
+  warn,
+) {
   const { settings, chartType } = props;
 
-  return chartType === "scatter"
-    ? getDimensionsAndGroupsForScatterChart(datas)
-    : isStacked(settings, datas)
-    ? getDimensionsAndGroupsAndUpdateSeriesDisplayNamesForStackedChart(
-        props,
-        datas,
-        warn,
-      )
-    : getDimensionsAndGroupsForOther(props, datas, warn);
+  const { groups, dimension } =
+    chartType === "scatter"
+      ? getDimensionsAndGroupsForScatterChart(datas)
+      : isStacked(settings, datas)
+      ? getDimensionsAndGroupsAndUpdateSeriesDisplayNamesForStackedChart(
+          props,
+          datas,
+          warn,
+        )
+      : getDimensionsAndGroupsForOther(props, datas, warn);
+  const yExtents = getYExtentsForGroups(groups);
+  return { groups, dimension, yExtents };
 }
 
 ///------------------------------------------------------------ Y AXIS PROPS ------------------------------------------------------------///
@@ -308,8 +329,7 @@ function getIsSplitYAxis(left, right) {
   return right && right.series.length && (left && left.series.length > 0);
 }
 
-function getYAxisProps(props, groups, datas) {
-  const yExtents = groups.map(group => d3.extent(group[0].all(), d => d.value));
+function getYAxisProps(props, yExtents, datas) {
   const yAxisSplit = getYAxisSplit(props, datas, yExtents);
 
   const [yLeftSplit, yRightSplit] = getYAxisSplitLeftAndRight(
@@ -788,9 +808,10 @@ export default function lineAreaBar(
   const {
     dimension,
     groups,
+    yExtents,
   } = getDimensionsAndGroupsAndUpdateSeriesDisplayNames(props, datas, warn);
 
-  const yAxisProps = getYAxisProps(props, groups, datas);
+  const yAxisProps = getYAxisProps(props, yExtents, datas);
 
   // Don't apply to linear or timeseries X-axis since the points are always plotted in order
   if (!isTimeseries(settings) && !isQuantitative(settings)) {

frontend/src/metabase/visualizations/lib/apply_axis.js

@@ -401,15 +401,22 @@ export function applyChartYAxis(chart, series, yExtent, axisName) {
       // TODO: right axis?
       chart.elasticY(true);
     } else {
-      if (
-        !(
-          (yExtent[0] < 0 && yExtent[1] < 0) ||
-          (yExtent[0] > 0 && yExtent[1] > 0)
-        )
-      ) {
+      const [min, max] = yExtent;
+      if (!((min < 0 && max < 0) || (min > 0 && max > 0))) {
         throw "Y-axis must not cross 0 when using log scale.";
       }
-      scale.domain(yExtent);
+
+      // With chart.elasticY, the y axis adjusts to show the beginning of the
+      // bars. If there are any bar series, we try to do the same with the log
+      // scale. We start at ±1 because things get wacky in (0, ±1].
+      const noBarSeries = series.every(s => s.card.display !== "bar");
+      if (noBarSeries) {
+        scale.domain([min, max]);
+      } else if (min < 0) {
+        scale.domain([min, -1]);
+      } else {
+        scale.domain([1, max]);
+      }
     }
     axis.scale(scale);
   } else {

frontend/test/metabase/visualizations/components/LineAreaBarRenderer-bar.unit.spec.js

@@ -256,6 +256,22 @@ describe("LineAreaBarRenderer-bar", () => {
     const tick = element.querySelector(".axis.x .tick text");
     expect(tick.textContent).toEqual("(empty)");
   });
+
+  it(`should render a stacked chart on a logarithmic y scale`, async () => {
+    const settings = {
+      "stackable.stack_type": "stacked",
+      "graph.y_axis.scale": "log",
+    };
+    renderLineAreaBar(element, [
+      MainSeries("bar", settings, { value: 100 }),
+      ExtraSeries(1000),
+    ]);
+    const ticks = qsa(".axis.y .tick text").map(n => n.textContent);
+    const lastTickValue = parseInt(ticks[ticks.length - 1]);
+    // if there are no ticks above 500, we're incorrectly using only the
+    // first series to determine the y axis domain
+    expect(lastTickValue > 500).toBe(true);
+  });
 });
 
 function getDataKeyValues(hover) {

frontend/test/metabase/visualizations/components/LineAreaBarRenderer.unit.spec.js

@@ -14,7 +14,9 @@ import {
 } from "../__support__/visualizations";
 
 import { getComputedSettingsForSeries } from "metabase/visualizations/lib/settings/visualization";
-import lineAreaBarRenderer from "metabase/visualizations/lib/LineAreaBarRenderer";
+import lineAreaBarRenderer, {
+  getDimensionsAndGroupsAndUpdateSeriesDisplayNames,
+} from "metabase/visualizations/lib/LineAreaBarRenderer";
 
 const formatTz = offset =>
   (offset < 0 ? "-" : "+") + d3.format("02d")(Math.abs(offset)) + ":00";
@@ -314,6 +316,57 @@ describe("LineAreaBarRenderer", () => {
     });
   });
 
+  describe("getDimensionsAndGroupsAndUpdateSeriesDisplayNames", () => {
+    it("should group a single row", () => {
+      const props = { settings: {}, chartType: "bar" };
+      const data = [[["a", 1]]];
+      const warn = jest.fn();
+
+      const {
+        groups,
+        dimension,
+        yExtents,
+      } = getDimensionsAndGroupsAndUpdateSeriesDisplayNames(props, data, warn);
+
+      expect(warn).not.toBeCalled();
+      expect(groups[0][0].all()[0]).toEqual({ key: "a", value: 1 });
+      expect(dimension.top(1)).toEqual([["a", 1]]);
+      expect(yExtents).toEqual([[1, 1]]);
+    });
+
+    it("should group multiple series", () => {
+      const props = { settings: {}, chartType: "bar" };
+      const data = [[["a", 1], ["b", 2]], [["a", 2], ["b", 3]]];
+      const warn = jest.fn();
+
+      const {
+        groups,
+        yExtents,
+      } = getDimensionsAndGroupsAndUpdateSeriesDisplayNames(props, data, warn);
+
+      expect(warn).not.toBeCalled();
+      expect(groups.length).toEqual(2);
+      expect(yExtents).toEqual([[1, 2], [2, 3]]);
+    });
+
+    it("should group stacked series", () => {
+      const props = {
+        settings: { "stackable.stack_type": "stacked" },
+        chartType: "bar",
+      };
+      const data = [[["a", 1], ["b", 2]], [["a", 2], ["b", 3]]];
+      const warn = jest.fn();
+
+      const {
+        groups,
+        yExtents,
+      } = getDimensionsAndGroupsAndUpdateSeriesDisplayNames(props, data, warn);
+
+      expect(warn).not.toBeCalled();
+      expect(groups.length).toEqual(1);
+      expect(yExtents).toEqual([[3, 5]]);
+    });
+  });
   // querySelector shortcut
   const qs = selector => element.querySelector(selector);