Changed period in row evolution breaks graph/metric association (#14423)

* Changed period in row evolution breaks graph/metric association

fix #14208

worked for me, also with multi row evolution. Only known issue I found is if requesting eg first yearly row evolution which may have the `users` metric hidden, then switching to `day` period, and then the `users` metric is maybe supposed to become available but didn't. That should be quite edge case though.

* add class whether row is hidden

* Fixing couple regressions visible in ui tests.

* fix build + update comment

* tweak

* test tweak

2 files changed, 141 insertions, 46 deletions

diff --git a/plugins/CoreVisualizations/Visualizations/Graph.php b/plugins/CoreVisualizations/Visualizations/Graph.php
index 1dbdfc824e..28ffab7768 100644
--- a/plugins/CoreVisualizations/Visualizations/Graph.php
+++ b/plugins/CoreVisualizations/Visualizations/Graph.php
@@ -201,7 +201,12 @@ abstract class Graph extends Visualization
         $columnsToDisplay = $this->removeLabelFromArray($columnsToDisplay);
 
         // Strip out any columns_to_display that are not in the dataset
-        $allColumns = $this->getDataTable()->getColumns();
+        $allColumns = [];
+        if ($this->report) {
+            $allColumns = $this->report->getAllMetrics();
+        }
+        $allColumns = array_merge($allColumns, $this->getDataTable()->getColumns());
+        $allColumns = array_unique($allColumns);
 
         // If the datatable has no data, use the default columns (there must be data for evolution graphs or else nothing displays)
         if (empty($allColumns)) {
diff --git a/plugins/CoreVisualizations/javascripts/jqplot.js b/plugins/CoreVisualizations/javascripts/jqplot.js
index 6e9d53e4d9..00d90b0858 100644
--- a/plugins/CoreVisualizations/javascripts/jqplot.js
+++ b/plugins/CoreVisualizations/javascripts/jqplot.js
@@ -8,6 +8,11 @@
  * @license http://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later
  */
 
+function rowEvolutionGetMetricNameFromRow(tr)
+{
+    return $(tr).find('td [data-name]').text().trim();
+}
+
 (function ($, require) {
 
     var exports = require('piwik/UI'),
@@ -20,7 +25,7 @@
 
             return window.piwik.jqplotLabelFont || 'Arial';
         }
-        ;
+    ;
 
     exports.getLabelFontFamily = getLabelFontFamily;
 
@@ -161,9 +166,9 @@
             target.on('jqplotDataHighlight', function (e, seriesIndex, valueIndex) {
                 self._showDataPointTooltip(this, seriesIndex, valueIndex);
             })
-            .on('jqplotDataUnhighlight', function () {
-                self._destroyDataPointTooltip($(this));
-            });
+                .on('jqplotDataUnhighlight', function () {
+                    self._destroyDataPointTooltip($(this));
+                });
 
             // handle window resize
             this._plotWidth = target.innerWidth();
@@ -585,9 +590,42 @@
             new seriesPickerClass(this.targetDivId, this, initiallyShowAll);
 
             if (!initiallyShowAll) {
-                // initially, show only the first series
-                this.data = [this.data[0]];
-                this.jqplotParams.series = [this.jqplotParams.series[0]];
+
+                var initialMetrics = 0;
+                var $rowEvolution = $('#'+this.targetDivId).closest('.rowevolution');
+
+                var newData = [];
+                var newSeries = [];
+                if ($rowEvolution.data('initialMetrics')) {
+                    initialMetrics = $rowEvolution.data('initialMetrics');
+
+                    if (angular.isArray(initialMetrics)) {
+                        for (var j = 0; j < initialMetrics.length; j++) {
+                            // find index of series and data
+                            for (var k = 0; k < this.jqplotParams.series.length; k++) {
+                                if (this.jqplotParams.series[k]
+                                    && this.jqplotParams.series[k].label
+                                    && this.jqplotParams.series[k].label === initialMetrics[j]) {
+
+                                    newData.push(this.data[k]);
+                                    newSeries.push(this.jqplotParams.series[k]);
+                                    break;
+                                }
+                            }
+                        }
+                    }
+                }
+
+                if (newData.length) {
+                    // restore original selection
+                    this.data = newData;
+                    this.jqplotParams.series = newSeries;
+                } else {
+                    // initially, show only the first series
+                    this.data = [this.data[0]];
+                    this.jqplotParams.series = [this.jqplotParams.series[0]];
+                }
+
                 this.setYTicks();
             }
         },
@@ -598,7 +636,7 @@
         _setColors: function () {
             var colorManager = piwik.ColorManager,
                 seriesColorNames = ['series1', 'series2', 'series3', 'series4', 'series5',
-                                    'series6', 'series7', 'series8', 'series9', 'series10'];
+                    'series6', 'series7', 'series8', 'series9', 'series10'];
 
             var viewDataTable = $('#' + this.workingDivId).data('uiControlObject').param['viewDataTable'];
 
@@ -659,26 +697,21 @@ JQPlotExternalSeriesToggle.prototype = {
 
     // show a single series
     showSeries: function (i) {
-        for (var j = 0; j < this.activated.length; j++) {
-            this.activated[j] = (i == j);
-        }
+        this.activated = [i];
         this.replot();
     },
 
     // toggle a series (make plotting multiple series possible)
     toggleSeries: function (i) {
-        var activatedCount = 0;
-        for (var k = 0; k < this.activated.length; k++) {
-            if (this.activated[k]) {
-                activatedCount++;
+        if (this.activated.indexOf(i) > -1) {
+            // need to remove the metric
+            if (this.activated.length > 1) {
+                // prevent removing the only visible metric
+                this.activated.splice(this.activated.indexOf(i), 1);
             }
+        } else {
+            this.activated.push(i);
         }
-        if (activatedCount == 1 && this.activated[i]) {
-            // prevent removing the only visible metric
-            return;
-        }
-
-        this.activated[i] = !this.activated[i];
         this.replot();
     },
 
@@ -692,17 +725,24 @@ JQPlotExternalSeriesToggle.prototype = {
         config.params.series = [];
         config.params.axes = {xaxis: this.originalAxes.xaxis};
         config.params.seriesColors = [];
+
         for (var j = 0; j < this.activated.length; j++) {
-            if (!this.activated[j]) {
-                continue;
-            }
-            config.data.push(this.originalData[j]);
-            config.params.seriesColors.push(this.originalSeriesColors[j]);
-            config.params.series.push($.extend(true, {}, this.originalSeries[j]));
-            // build array of used axes
-            var axis = this.originalSeries[j].yaxis;
-            if ($.inArray(axis, usedAxes) == -1) {
-                usedAxes.push(axis);
+            // find index of series and data
+            for (var k = 0; k < this.originalSeries.length; k++) {
+                if (this.originalSeries[k]
+                    && this.originalSeries[k].label
+                    && this.originalSeries[k].label === this.activated[j]) {
+
+                    config.data.push(this.originalData[k]);
+                    config.params.seriesColors.push(this.originalSeriesColors[k]);
+                    config.params.series.push($.extend(true, {}, this.originalSeries[k]));
+                    // build array of used axes
+                    var axis = this.originalSeries[k].yaxis;
+                    if ($.inArray(axis, usedAxes) == -1) {
+                        usedAxes.push(axis);
+                    }
+                    break;
+                }
             }
         }
 
@@ -742,30 +782,73 @@ RowEvolutionSeriesToggle.prototype = JQPlotExternalSeriesToggle.prototype;
 
 RowEvolutionSeriesToggle.prototype.attachEvents = function () {
     var self = this;
-    this.seriesPickers = this.target.closest('.rowevolution').find('table.metrics tr');
+
+    var $rowEvolution = this.target.closest('.rowevolution');
+    this.seriesPickers = $rowEvolution.find('table.metrics tr');
+
+    var initialMetrics = [];
+
+    if ($rowEvolution.data('initialMetrics')) {
+        initialMetrics = [];
+        var savedMetrics = $rowEvolution.data('initialMetrics');
+        var existingMetricsInSeries = [];
+        var m = 0;
+        for (m = 0; m < this.originalSeries.length; m++) {
+            existingMetricsInSeries.push(this.originalSeries[m].label);
+        }
+        for (m = 0; m < savedMetrics.length; m++) {
+            if (existingMetricsInSeries.indexOf(savedMetrics[m]) > -1) {
+                // only if it exists... for example unique visitors etc might not be available for some metrics,
+                // then we need to make sure to highlight the default first metric for example
+                initialMetrics.push(savedMetrics[m]);
+            }
+        }
+    }
 
     this.seriesPickers.each(function (i) {
         var el = $(this);
+
         el.off('click').on('click', function (e) {
+            var metricName = rowEvolutionGetMetricNameFromRow(this);
+            // we are storing this info on the element as the series picker and the jqplot object gets recreated whenever
+            // we change a period so we cannot persist the selection there.
             if (e.shiftKey) {
-                self.toggleSeries(i);
-
+                self.toggleSeries(metricName);
                 document.getSelection().removeAllRanges(); // make sure chrome doesn't select text
             } else {
-                self.showSeries(i);
+                self.showSeries(metricName);
             }
+            $rowEvolution.data('initialMetrics', self.activated);
             return false;
         });
 
-        if (i == 0 || self.initiallyShowAll) {
+        var label = rowEvolutionGetMetricNameFromRow(el);
+        var metricExists = false;
+        for (var k = 0; k < self.originalSeries.length; k++) {
+            if (self.originalSeries[k] && labelMatches(self.originalSeries[k].label, label)) {
+                metricExists = true;
+            }
+        }
+
+        if (!metricExists) {
+            el.hide();
+        } else if (
+            (initialMetrics.length === 0 && i == 0)
+            || (initialMetrics.length > 0 && initialMetrics.indexOf(label) > -1)
+            || self.initiallyShowAll) {
             // show the active series
             // if initiallyShowAll, all are active; otherwise only the first one
-            self.activated.push(true);
+            if (!el.hasClass('hiddenByDefault')) {
+                el.show();
+            }
             el.find('td').css('opacity', '');
+            self.activated.push(rowEvolutionGetMetricNameFromRow(el));
         } else {
+            if (!el.hasClass('hiddenByDefault')) {
+                el.show();
+            }
             // fade out the others
             el.find('td').css('opacity', .5);
-            self.activated.push(false);
         }
 
         // prevent selecting in ie & opera (they don't support doing this via css)
@@ -775,18 +858,25 @@ RowEvolutionSeriesToggle.prototype.attachEvents = function () {
         } else if ($.browser.opera) {
             $(this).attr('unselectable', 'on');
         }
+
+        // the API outputs the label double encoded when it shouldn't. so when looking for a matching label we have
+        // to check if one is double encoded.
+        function labelMatches(lhs, rhs) {
+            return lhs === rhs || piwikHelper.htmlDecode(lhs) === rhs || lhs === piwikHelper.htmlDecode(rhs);
+        }
     });
 };
 
 RowEvolutionSeriesToggle.prototype.beforeReplot = function () {
+    var self = this;
     // fade out if not activated
-    for (var i = 0; i < this.activated.length; i++) {
-        if (this.activated[i]) {
-            this.seriesPickers.eq(i).find('td').css('opacity', 1);
-        } else {
-            this.seriesPickers.eq(i).find('td').css('opacity', .5);
+    this.seriesPickers.find('td').css('opacity', .5);
+    this.seriesPickers.each(function (i) {
+        var name = rowEvolutionGetMetricNameFromRow(this);
+        if (self.activated.indexOf(name) > -1) {
+            $(this).find('td').css('opacity', 1);
         }
-    }
+    });
 };
 
 // ------------------------------------------------------------
@@ -1180,7 +1270,7 @@ RowEvolutionSeriesToggle.prototype.beforeReplot = function () {
             // move close labels
             if (lastY2 !== false && lastRight == right && (
                 (right && y2 - lastY2 < 13) ||
-                    (!right && lastY2 - y2 < 13))) {
+                (!right && lastY2 - y2 < 13))) {
 
                 if (x1 > center[0]) {
                     // move down if the label is in the right half of the graph