diff options
author | Thomas Steur <tsteur@users.noreply.github.com> | 2019-05-25 00:18:03 +0300 |
---|---|---|
committer | diosmosis <diosmosis@users.noreply.github.com> | 2019-05-25 00:18:03 +0300 |
commit | 871c31d0962a144b85ca2601a24b9d10ab707bfc (patch) | |
tree | fd5c680f7f480319ee521b54e604c351f11a7f55 | |
parent | b9df06dadfd8030401bb9ae1b8126e4cf13f33b5 (diff) |
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
-rw-r--r-- | plugins/CoreHome/templates/getMultiRowEvolutionPopover.twig | 2 | ||||
-rw-r--r-- | plugins/CoreVisualizations/Visualizations/Graph.php | 7 | ||||
-rw-r--r-- | plugins/CoreVisualizations/javascripts/jqplot.js | 180 | ||||
-rw-r--r-- | tests/UI/specs/RowEvolution_spec.js | 3 |
4 files changed, 143 insertions, 49 deletions
diff --git a/plugins/CoreHome/templates/getMultiRowEvolutionPopover.twig b/plugins/CoreHome/templates/getMultiRowEvolutionPopover.twig index ec6d8b82c5..90d0a9aaf9 100644 --- a/plugins/CoreHome/templates/getMultiRowEvolutionPopover.twig +++ b/plugins/CoreHome/templates/getMultiRowEvolutionPopover.twig @@ -8,7 +8,7 @@ <h2>{{ availableRecordsText|translate }}</h2> <table class="metrics" border="0" cellpadding="0" cellspacing="0"> {% for i, metric in metrics %} - <tr {% if metric.hide|default %}style="display:none"{% endif %}> + <tr {% if metric.hide|default %}class="hiddenByDefault" style="display:none"{% endif %}> <td class="sparkline"> {{ metric.sparkline|raw }} </td> 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 diff --git a/tests/UI/specs/RowEvolution_spec.js b/tests/UI/specs/RowEvolution_spec.js index fab3a148f6..d8cad654e0 100644 --- a/tests/UI/specs/RowEvolution_spec.js +++ b/tests/UI/specs/RowEvolution_spec.js @@ -8,8 +8,6 @@ */ describe("RowEvolution", function () { - this.timeout(0); - const viewDataTableUrl = "?module=Widgetize&action=iframe&moduleToWidgetize=Referrers&idSite=1&period=week&date=2012-02-09&" + "actionToWidgetize=getKeywords&viewDataTable=table&filter_limit=5"; @@ -62,6 +60,7 @@ describe("RowEvolution", function () { await page.waitForNetworkIdle(); const dialog = await page.$('.ui-dialog'); + await page.waitForNetworkIdle(); expect(await dialog.screenshot()).to.matchImage('multirow_evolution'); }); |