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>

6 files changed, 81 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 %}
diff --git a/tests/UI/expected-screenshots/Comparison_visits_overview_widget_sv.png b/tests/UI/expected-screenshots/Comparison_visits_overview_widget_sv.png
new file mode 100644
index 0000000000..c020741684
--- /dev/null
+++ b/tests/UI/expected-screenshots/Comparison_visits_overview_widget_sv.png
@@ -0,0 +1,3 @@
+version https://git-lfs.github.com/spec/v1
+oid sha256:a170b350af9aec503f895c76483a0285b663d789cdeb445f1838e7b298eeecc2
+size 235286
diff --git a/tests/UI/specs/Comparison_spec.js b/tests/UI/specs/Comparison_spec.js
index 9e92e9c597..7b29345b4f 100644
--- a/tests/UI/specs/Comparison_spec.js
+++ b/tests/UI/specs/Comparison_spec.js
@@ -31,6 +31,9 @@ describe("Comparison", function () {
             + "actionToWidgetize=getSearchEngines&viewDataTable=table&filter_limit=5&isFooterExpandedInDashboard=1" + comparePeriod,
         visitOverviewWidget = "?module=Widgetize&action=iframe&containerId=VisitOverviewWithGraph&disableLink=0&widget=1&" +
             "moduleToWidgetize=CoreHome&actionToWidgetize=renderWidgetContainer&disableLink=1&widget=1&" + generalParams + "&" +
+            compareParams,
+        visitOverviewSparklines = "?module=Widgetize&action=iframe&disableLink=1&widget=1&" +
+            "moduleToWidgetize=VisitsSummary&actionToWidgetize=get&forceView=1&viewDataTable=sparklines&" + generalParams + "&" +
             compareParams
     ;
 
@@ -259,4 +262,15 @@ describe("Comparison", function () {
         await page.waitForNetworkIdle();
         expect(await page.screenshot({ fullPage: true })).to.matchImage('visits_overview_widget');
     });
+
+    it('should show evolution metrics correctly formatted in other language', async () => {
+        await page.goto(visitOverviewSparklines + '&language=sv');
+        await page.waitForNetworkIdle();
+        await page.evaluate(function(){
+            // replace all metric names with `metric name` to avoid test failures when metric translation changes
+            $('.sparkline-metrics').each(function(){ $(this).html($(this).find('strong').prop('outerHTML') + ' metric name') });
+        });
+
+        expect(await page.screenshot({ fullPage: true })).to.matchImage('visits_overview_widget_sv');
+    });
 });