Merge pull request #3623 from stormpython/fix/3560

Pie Charts should throw an error when only zero values

src/kibana/components/vislib/lib/data.js

@@ -512,30 +512,24 @@ define(function (require) {
     };
 
     /**
-     * Checks whether all pie slices have zero values.
-     * If so, an error is thrown.
+     * Removes zeros from pie chart data
+     * @param slices
+     * @returns {*}
      */
-    Data.prototype._validatePieData = function () {
-      var visData = this.getVisData();
+    Data.prototype._removeZeroSlices = function (slices) {
+      var self = this;
 
-      visData.forEach(function (chartData) {
-        chartData.slices = (function withoutZeroSlices(slices) {
-          if (!slices.children) return slices;
+      if (!slices.children) return slices;
 
-          slices = _.clone(slices);
-          slices.children = slices.children.reduce(function (children, child) {
-            if (child.size !== 0) {
-              children.push(withoutZeroSlices(child));
-            }
-            return children;
-          }, []);
-          return slices;
-        }(chartData.slices));
-
-        if (chartData.slices.children.length === 0) {
-          throw new errors.PieContainsAllZeros();
+      slices = _.clone(slices);
+      slices.children = slices.children.reduce(function (children, child) {
+        if (child.size !== 0) {
+          children.push(self._removeZeroSlices(child));
         }
-      });
+        return children;
+      }, []);
+
+      return slices;
     };
 
     /**
@@ -547,12 +541,12 @@ define(function (require) {
      */
     Data.prototype.pieNames = function () {
       var self = this;
+      var data = this.getVisData();
       var names = [];
 
-      this._validatePieData();
-
-      _.forEach(this.getVisData(), function (obj) {
+      _.forEach(data, function (obj) {
         var columns = obj.raw ? obj.raw.columns : undefined;
+        obj.slices = self._removeZeroSlices(obj.slices);
 
         _.forEach(self.getNames(obj, columns), function (name) {
           names.push(name);

src/kibana/components/vislib/visualizations/_chart.js

@@ -1,6 +1,7 @@
 define(function (require) {
   return function ChartBaseClass(d3, Private) {
     var _ = require('lodash');
+    var errors = require('errors');
 
     var Dispatch = Private(require('components/vislib/lib/dispatch'));
     var Tooltip = Private(require('components/vislib/components/tooltip/tooltip'));

src/kibana/components/vislib/visualizations/pie_chart.js

@@ -24,11 +24,26 @@ define(function (require) {
       }
       PieChart.Super.apply(this, arguments);
 
+      var charts = this.handler.data.getVisData();
+      this._validatePieData(charts);
+
       this._attr = _.defaults(handler._attr || {}, {
         isDonut: handler._attr.isDonut || false
       });
     }
 
+    /**
+     * Checks whether pie slices have all zero values.
+     * If so, an error is thrown.
+     */
+    PieChart.prototype._validatePieData = function (charts) {
+      var isAllZeros = charts.every(function (chart) {
+        return chart.slices.children.length === 0;
+      });
+
+      if (isAllZeros) { throw new errors.PieContainsAllZeros(); }
+    };
+
     /**
      * Adds Events to SVG paths
      *
@@ -150,6 +165,15 @@ define(function (require) {
       return path;
     };
 
+    PieChart.prototype._validateContainerSize = function (width, height) {
+      var minWidth = 20;
+      var minHeight = 20;
+
+      if (width <= minWidth || height <= minHeight) {
+        throw new errors.ContainerTooSmall();
+      }
+    };
+
     /**
      * Renders d3 visualization
      *
@@ -162,17 +186,15 @@ define(function (require) {
       return function (selection) {
         selection.each(function (data) {
           var slices = data.slices;
-          var el = this;
-          var div = d3.select(el);
-          var width = $(el).width();
-          var height = $(el).height();
-          var minWidth = 20;
-          var minHeight = 20;
+          var div = d3.select(this);
+          var width = $(this).width();
+          var height = $(this).height();
           var path;
 
-          if (width <= minWidth || height <= minHeight) {
-            throw new errors.ContainerTooSmall();
-          }
+          if (!slices.children.length) return;
+
+          self.convertToPercentage(slices);
+          self._validateContainerSize(width, height);
 
           var svg = div.append('svg')
           .attr('width', width)
@@ -180,7 +202,6 @@ define(function (require) {
           .append('g')
           .attr('transform', 'translate(' + width / 2 + ',' + height / 2 + ')');
 
-          self.convertToPercentage(slices);
           path = self.addPath(width, height, svg, slices);
           self.addPathEvents(path);
 

test/unit/specs/vislib/lib/data.js

@@ -205,6 +205,37 @@ define(function (require) {
 
     });
 
+    describe('_removeZeroSlices', function () {
+      var pieData = {
+        slices: {
+          children: [
+            {size: 30},
+            {size: 20},
+            {size: 0}
+          ]
+        }
+      };
+      var DataFactory;
+      var data;
+
+      beforeEach(function () {
+        module('DataFactory');
+      });
+
+      beforeEach(function () {
+        inject(function (Private) {
+          DataFactory = Private(require('components/vislib/lib/data'));
+          data = new DataFactory(pieData, {});
+          data._removeZeroSlices(pieData.slices);
+        });
+      });
+
+      it('should remove zero values', function () {
+        var slices = data.data.slices;
+        expect(slices.children.length).to.be(2);
+      });
+    });
+
     describe('Data.flatten', function () {
       var DataFactory;
       var serIn;

test/unit/specs/vislib/visualizations/pie_chart.js

@@ -118,6 +118,41 @@ define(function (require) {
       expect($(chart1.el).find('.y-axis-chart-title').length).to.be(1);
       expect($(chart2.el).find('.x-axis-chart-title').length).to.be(1);
     });
+
+    describe('_validatePieData method', function () {
+      var allZeros = [
+        { slices: { children: [] } },
+        { slices: { children: [] } },
+        { slices: { children: [] } }
+      ];
+
+      var someZeros = [
+        { slices: { children: [{}] } },
+        { slices: { children: [{}] } },
+        { slices: { children: [] } }
+      ];
+
+      var noZeros = [
+        { slices: { children: [{}] } },
+        { slices: { children: [{}] } },
+        { slices: { children: [{}] } }
+      ];
+
+      it('should throw an error when all charts contain zeros', function () {
+        expect(function () {
+          chart1.ChartClass.prototype._validatePieData(allZeros);
+        }).to.throwError();
+      });
+
+      it('should not throw an error when only some or no charts contain zeros', function () {
+        expect(function () {
+          chart1.ChartClass.prototype._validatePieData(someZeros);
+        }).to.not.throwError();
+        expect(function () {
+          chart1.ChartClass.prototype._validatePieData(noZeros);
+        }).to.not.throwError();
+      });
+    });
   });
 
   aggArray.forEach(function (dataAgg, i) {