diff options
author | Stefan Giehl <stefan@matomo.org> | 2022-03-08 11:52:10 +0300 |
---|---|---|
committer | GitHub <noreply@github.com> | 2022-03-08 11:52:10 +0300 |
commit | a6a7cf70af86bda997b1faae1573e78db4e818a8 (patch) | |
tree | 609409b8d87f67d84855c9707a026d0debf201eb /plugins | |
parent | 4c04cd1ed3deb8228fcc9ff503ce013506f8d807 (diff) |
Fix comparison trends might be displayed incorrect for certain languages (#18832)
* Fix comparison trends might be displayed incorrect for certain languages
* update tests
* include trend values only when requested
* updates expected tests files
* add some code comments
* Adds ui tests for comparison sparklines in other language
* init sparklines after document fully loaded
Co-authored-by: diosmosis <diosmosis@users.noreply.github.com>
Diffstat (limited to 'plugins')
-rw-r--r-- | plugins/API/Filter/DataComparisonFilter.php | 35 | ||||
-rw-r--r-- | plugins/CoreHome/javascripts/sparkline.js | 38 | ||||
-rw-r--r-- | plugins/CoreVisualizations/Visualizations/Sparklines.php | 16 | ||||
-rw-r--r-- | plugins/CoreVisualizations/templates/macros.twig | 6 |
4 files changed, 64 insertions, 31 deletions
diff --git a/plugins/API/Filter/DataComparisonFilter.php b/plugins/API/Filter/DataComparisonFilter.php index c94df60b23..015bea7463 100644 --- a/plugins/API/Filter/DataComparisonFilter.php +++ b/plugins/API/Filter/DataComparisonFilter.php @@ -13,7 +13,6 @@ use Piwik\Common; use Piwik\Config; use Piwik\DataTable; use Piwik\DataTable\DataTableInterface; -use Piwik\DataTable\Simple; use Piwik\Http\BadRequestException; use Piwik\Metrics; use Piwik\Period; @@ -22,7 +21,6 @@ use Piwik\Piwik; use Piwik\Plugin\Report; use Piwik\Plugins\API\Filter\DataComparisonFilter\ComparisonRowGenerator; use Piwik\Segment; -use Piwik\Segment\SegmentExpression; use Piwik\Site; /** @@ -488,7 +486,7 @@ class DataComparisonFilter /** @var DataTable\Row[] $rows */ $rows = array_values($comparisons->getRows()); foreach ($rows as $index => $compareRow) { - list($periodIndex, $segmentIndex) = self::getIndividualComparisonRowIndices($table, $index, $segmentCount); + [$periodIndex, $segmentIndex] = self::getIndividualComparisonRowIndices($table, $index, $segmentCount); if (!$this->invertCompareChangeCompute && $index < $segmentCount) { continue; // do not calculate for first period @@ -506,11 +504,17 @@ class DataComparisonFilter } foreach ($compareRow->getColumns() as $name => $value) { - $changeTo = $this->computeChangePercent($otherPeriodRow, $compareRow, $name); + [$changeTo, $trendTo] = $this->computeChangePercent($otherPeriodRow, $compareRow, $name); $compareRow->addColumn($name . '_change', $changeTo); + if ($this->shouldIncludeTrendValues()) { + $compareRow->addColumn($name . '_trend', $trendTo); + } - $changeFrom = $this->computeChangePercent($compareRow, $otherPeriodRow, $name); + [$changeFrom, $trendFrom] = $this->computeChangePercent($compareRow, $otherPeriodRow, $name); $compareRow->addColumn($name . '_change_from', $changeFrom); + if ($this->shouldIncludeTrendValues()) { + $compareRow->addColumn($name . '_trend_from', $trendFrom); + } } } } @@ -526,8 +530,9 @@ class DataComparisonFilter $valueToCompare = $valueToCompare ?: 0; $change = DataTable\Filter\CalculateEvolutionFilter::calculate($value, $valueToCompare, $precision = 1, true, true); + $trend = $value - $valueToCompare < 0 ? -1 : ($value - $valueToCompare > 0 ? 1 : 0); - return $change; + return [$change, $trend]; } /** @@ -584,6 +589,20 @@ class DataComparisonFilter } /** + * Returns whether to include trend values for all evolution columns or not + * This is requested only for sparklines + * + * @see \Piwik\Plugins\CoreVisualizations\Visualizations\Sparklines::render() + * + * @return bool + * @throws \Exception + */ + private function shouldIncludeTrendValues(): bool + { + return (bool) Common::getRequestVar('include_trends', 0, 'int', $this->request); + } + + /** * Returns the pretty series label for a specific comparison based on the currently set comparison query parameters. * * @param int $labelSeriesIndex The index of the comparison. Comparison series order is determined by {@see self::getReportsToCompare()}. @@ -594,7 +613,7 @@ class DataComparisonFilter $comparePeriods = self::getComparePeriods(); $compareDates = self::getCompareDates(); - list($periodIndex, $segmentIndex) = self::getIndividualComparisonRowIndices(null, $labelSeriesIndex, count($compareSegments)); + [$periodIndex, $segmentIndex] = self::getIndividualComparisonRowIndices(null, $labelSeriesIndex, count($compareSegments)); $segmentObj = new Segment($compareSegments[$segmentIndex], []); $prettySegment = $segmentObj->getStoredSegmentName(false); @@ -614,6 +633,8 @@ class DataComparisonFilter $mappings[$index] = $name; $mappings[$index . '_change'] = $name . '_change'; $mappings[$index . '_change_from'] = $name . '_change_from'; + $mappings[$index . '_trend'] = $name . '_trend'; + $mappings[$index . '_trend_from'] = $name . '_trend_from'; } return $mappings; } diff --git a/plugins/CoreHome/javascripts/sparkline.js b/plugins/CoreHome/javascripts/sparkline.js index 5457f1fe2e..3aab2976c2 100644 --- a/plugins/CoreHome/javascripts/sparkline.js +++ b/plugins/CoreHome/javascripts/sparkline.js @@ -26,33 +26,35 @@ piwik.getSparklineColors = function () { // initializes each sparkline so they use colors defined in CSS piwik.initSparklines = function() { - $('.sparkline img').each(function () { - var $self = $(this); + $(function () { + $('.sparkline img').each(function () { + var $self = $(this); - if ($self.attr('src')) { + if ($self.attr('src')) { return; - } + } - var seriesIndices = $self.closest('.sparkline').data('series-indices'); - var sparklineColors = piwik.getSparklineColors(); + var seriesIndices = $self.closest('.sparkline').data('series-indices'); + var sparklineColors = piwik.getSparklineColors(); - if (seriesIndices && sparklineColors.lineColor instanceof Array) { + if (seriesIndices && sparklineColors.lineColor instanceof Array) { sparklineColors.lineColor = sparklineColors.lineColor.filter(function (c, index) { - return seriesIndices.indexOf(index) !== -1; + return seriesIndices.indexOf(index) !== -1; }); - } + } - var colors = JSON.stringify(sparklineColors); - var appendToSparklineUrl = '&colors=' + encodeURIComponent(colors); + var colors = JSON.stringify(sparklineColors); + var appendToSparklineUrl = '&colors=' + encodeURIComponent(colors); - // Append the token_auth to the URL if it was set (eg. embed dashboard) - var token_auth = broadcast.getValueFromUrl('token_auth'); - if (token_auth.length && piwik.shouldPropagateTokenAuth) { + // Append the token_auth to the URL if it was set (eg. embed dashboard) + var token_auth = broadcast.getValueFromUrl('token_auth'); + if (token_auth.length && piwik.shouldPropagateTokenAuth) { appendToSparklineUrl += '&token_auth=' + token_auth; - } - $self.attr('width', sparklineDisplayWidth); - $self.attr('height', sparklineDisplayHeight); - $self.attr('src', $self.attr('data-src') + appendToSparklineUrl); + } + $self.attr('width', sparklineDisplayWidth); + $self.attr('height', sparklineDisplayHeight); + $self.attr('src', $self.attr('data-src') + appendToSparklineUrl); + }); }); }; diff --git a/plugins/CoreVisualizations/Visualizations/Sparklines.php b/plugins/CoreVisualizations/Visualizations/Sparklines.php index ad9373f329..4a2b4a69ee 100644 --- a/plugins/CoreVisualizations/Visualizations/Sparklines.php +++ b/plugins/CoreVisualizations/Visualizations/Sparklines.php @@ -82,6 +82,15 @@ class Sparklines extends ViewDataTable $this->requestConfig->request_parameters_to_modify['columns'] = $columnsList; $this->requestConfig->request_parameters_to_modify['format_metrics'] = '1'; + /** + * This special request parameter is used to include trend indication columns for all evolution columns + * this is done to be able to determine safely in the view if an evolution is positive or negative, as this + * can't be done with formatted evolution values due to language specific signs being used. + * + * @see DataComparisonFilter::compareChangePercents + */ + $this->requestConfig->request_parameters_to_modify['include_trends'] = '1'; + $request = $this->getRequestArray(); if ($this->isComparing() && !empty($request['comparePeriods']) @@ -192,7 +201,7 @@ class Sparklines extends ViewDataTable $columnToUse = $this->removeUniqueVisitorsIfNotEnabledForPeriod($column, $period); - list($compareValues, $compareDescriptions, $evolutions) = $this->getValuesAndDescriptions($compareRow, $columnToUse, '_change'); + list($compareValues, $compareDescriptions, $evolutions) = $this->getValuesAndDescriptions($compareRow, $columnToUse, '_change', '_trend'); foreach ($compareValues as $i => $value) { $metricInfo = [ @@ -261,7 +270,7 @@ class Sparklines extends ViewDataTable $table->applyQueuedFilters(); } - private function getValuesAndDescriptions($firstRow, $columns, $evolutionColumnNameSuffix = null) + private function getValuesAndDescriptions($firstRow, $columns, $evolutionColumnNameSuffix = null, $trendColumnNameSuffix = null) { if (!is_array($columns)) { $columns = array($columns); @@ -285,8 +294,9 @@ class Sparklines extends ViewDataTable if ($evolutionColumnNameSuffix !== null) { $evolution = $firstRow->getColumn($col . $evolutionColumnNameSuffix); + $trend = $firstRow->getColumn($col . $trendColumnNameSuffix); if ($evolution !== false) { - $evolutions[] = ['percent' => ltrim($evolution, '+'), 'tooltip' => '']; + $evolutions[] = ['percent' => ltrim($evolution, '+'), 'trend' => $trend, 'tooltip' => '']; } } diff --git a/plugins/CoreVisualizations/templates/macros.twig b/plugins/CoreVisualizations/templates/macros.twig index d6c5ad9f56..8ded71264d 100644 --- a/plugins/CoreVisualizations/templates/macros.twig +++ b/plugins/CoreVisualizations/templates/macros.twig @@ -1,11 +1,11 @@ {% macro sparklineEvolution(evolution) %} {% set evolutionPretty = evolution.percent %} - - {% if evolution.percent < 0 %} + {% set compareValue = evolution.trend is defined ? evolution.trend : evolution.percent %} + {% if compareValue < 0 %} {% set evolutionClass = 'negative-evolution' %} {% set evolutionIcon = 'arrow_down.png' %} - {% elseif evolution.percent == 0 or evolution.percent == '0%' %} + {% elseif compareValue == 0 or compareValue == '0%' %} {% set evolutionClass = 'neutral-evolution' %} {% set evolutionIcon = 'stop.png' %} {% else %} |