From d6143ef2db3b50b86308cfdd58c9808d33c558e9 Mon Sep 17 00:00:00 2001 From: diosmosis Date: Sun, 22 Dec 2019 06:09:17 -0800 Subject: [PATCH 1/6] Compute row percentages in PHP before metrics are formatted to avoid percentage formatting WARNINGS. --- .../Visualizations/HtmlTable.php | 106 ++++++++++++++---- .../_dataTableViz_htmlTable_ratio.twig | 11 +- plugins/Goals/API.php | 2 - 3 files changed, 87 insertions(+), 32 deletions(-) diff --git a/plugins/CoreVisualizations/Visualizations/HtmlTable.php b/plugins/CoreVisualizations/Visualizations/HtmlTable.php index 36467fa04d1..cf926faf23d 100644 --- a/plugins/CoreVisualizations/Visualizations/HtmlTable.php +++ b/plugins/CoreVisualizations/Visualizations/HtmlTable.php @@ -14,7 +14,9 @@ use Piwik\DataTable\Row; use Piwik\Metrics; use Piwik\DataTable; +use Piwik\NumberFormatter; use Piwik\Period; +use Piwik\Piwik; use Piwik\Plugin\Visualization; /** @@ -81,31 +83,8 @@ public function beforeRender() } } - // we do not want to get a datatable\map - $period = Common::getRequestVar('period', 'day', 'string'); - if (Period\Range::parseDateRange($period)) { - $period = 'range'; - } - if ($this->dataTable->getRowsCount()) { - $request = new ApiRequest(array( - 'method' => 'API.get', - 'module' => 'API', - 'action' => 'get', - 'format' => 'original', - 'filter_limit' => '-1', - 'disable_generic_filters' => 1, - 'expanded' => 0, - 'flat' => 0, - 'filter_offset' => 0, - 'period' => $period, - 'showColumns' => implode(',', $this->config->columns_to_display), - 'columns' => implode(',', $this->config->columns_to_display), - 'pivotBy' => '' - )); - - $dataTable = $request->process(); - $this->assignTemplateVar('siteSummary', $dataTable); + $this->assignTemplateVar('siteTotalRow', $this->getSiteSummary()->getFirstRow()); } if ($this->isPivoted()) { @@ -222,6 +201,54 @@ public function beforeGenericFiltersAreAppliedToLoadedDataTable() } } + public function afterGenericFiltersAreAppliedToLoadedDataTable() + { + parent::afterGenericFiltersAreAppliedToLoadedDataTable(); + + $this->calculateTotalPercentages(); // this must be done before metrics are formatted + } + + private function calculateTotalPercentages() + { + if (empty($this->report)) { + return; + } + + $columnNamesToIndices = Metrics::getMappingFromNameToId(); + $formatter = NumberFormatter::getInstance(); + + $totals = $this->dataTable->getMetadata('totals'); + + $siteSummary = $this->getSiteSummary(); + $siteTotalRow = $siteSummary->getFirstRow(); + + foreach ($this->dataTable->getRows() as $row) { + foreach ($this->report->getMetrics() as $column => $translation) { + $indexColumn = $columnNamesToIndices[$column]; // TODO: This might fail for goal specific columns... + + $value = $row->getColumn($indexColumn) ?: $row->getColumn($column); + if ($column == 'label' + || $value === false + ) { + continue; + } + + $reportTotal = isset($totals[$column]) ? $totals[$column] : 0; + $siteTotal = $siteTotalRow->getColumn($column) ?: 0; + + $percentageColumnName = $column . '_row_percentage'; + $rowPercentage = $formatter->formatPercent(Piwik::getPercentageSafe($value, $reportTotal, $precision = 1)); + $row->setMetadata($percentageColumnName, $rowPercentage); + + $siteTotalPercentage = $column . '_site_total_percentage'; + if ($siteTotal && $siteTotal > $reportTotal) { + $rowPercentage = $formatter->formatPercent(Piwik::getPercentageSafe($value, $siteTotal, $precision = 1)); + $row->setMetadata($siteTotalPercentage, $rowPercentage); + } + } + } + } + protected function isPivoted() { return $this->requestConfig->pivotBy || Common::getRequestVar('pivotBy', ''); @@ -248,4 +275,35 @@ protected function isFlattened() { return $this->requestConfig->flat || Common::getRequestVar('flat', ''); } + + private function getSiteSummary() + { + if (empty($this->siteSummary)) { + // we do not want to get a datatable\map + $period = Common::getRequestVar('period', 'day', 'string'); + if (Period\Range::parseDateRange($period)) { + $period = 'range'; + } + + $request = new ApiRequest(array( + 'method' => 'API.get', + 'module' => 'API', + 'action' => 'get', + 'format' => 'original', + 'filter_limit' => '-1', + 'disable_generic_filters' => 1, + 'expanded' => 0, + 'flat' => 0, + 'filter_offset' => 0, + 'period' => $period, + 'showColumns' => implode(',', $this->config->columns_to_display), + 'columns' => implode(',', $this->config->columns_to_display), + 'pivotBy' => '' + )); + + $this->siteSummary = $request->process(); + } + + return $this->siteSummary; + } } diff --git a/plugins/CoreVisualizations/templates/_dataTableViz_htmlTable_ratio.twig b/plugins/CoreVisualizations/templates/_dataTableViz_htmlTable_ratio.twig index 8d14bb126d2..9014e2ddb08 100644 --- a/plugins/CoreVisualizations/templates/_dataTableViz_htmlTable_ratio.twig +++ b/plugins/CoreVisualizations/templates/_dataTableViz_htmlTable_ratio.twig @@ -1,20 +1,19 @@ {% if column in properties.report_ratio_columns and (column in totals|keys or forceZero|default) -%} {% set reportTotal = totals[column]|default(0) %} - {% if siteTotalRow|default is not empty %} + + {% if siteTotalRow is defined and siteTotalRow is not empty %} {% set siteTotal = siteTotalRow.getColumn(column) %} - {% elseif siteSummary is defined and siteSummary is not empty and siteSummary.getFirstRow %} - {% set siteTotal = siteSummary.getFirstRow.getColumn(column) %} {% else %} {% set siteTotal = 0 %} {% endif %} - {% set rowPercentage = row.getColumn(column)|percentage(reportTotal, 1) %} + {% set rowPercentage = row.getMetadata(column ~ '_row_percentage') %} {% set metricTitle = translations[column]|default(column) %} {% set reportRatioTooltip = 'General_ReportRatioTooltip'|translate(label, rowPercentage|e('html_attr'), reportTotal|e('html_attr'), metricTitle|e('html_attr'), '"' ~ segmentTitlePretty ~ '"', translations[labelColumn]|default(labelColumn)|e('html_attr')) %} - {% if siteTotal and siteTotal > reportTotal %} - {% set totalPercentage = row.getColumn(column)|percentage(siteTotal, 1) %} + {% if row.getMetadata(column ~ '_site_total_percentage') != false %} + {% set totalPercentage = row.getMetadata(column ~ '_site_total_percentage') %} {% set totalRatioTooltip = 'General_TotalRatioTooltip'|translate(totalPercentage, siteTotal|number(2,0), metricTitle, periodTitlePretty) %} {% else %} {% set totalRatioTooltip = '' %} diff --git a/plugins/Goals/API.php b/plugins/Goals/API.php index d8401250077..2ceaa83ebfc 100644 --- a/plugins/Goals/API.php +++ b/plugins/Goals/API.php @@ -15,10 +15,8 @@ use Piwik\Cache as PiwikCache; use Piwik\Common; use Piwik\DataTable; -use Piwik\Db; use Piwik\Metrics; use Piwik\Piwik; -use Piwik\Plugin\Report; use Piwik\Plugins\API\DataTable\MergeDataTables; use Piwik\Plugins\CoreHome\Columns\Metrics\ConversionRate; use Piwik\Plugins\Goals\Columns\Metrics\AverageOrderRevenue; From 79c84fe3e0fba346dd56775c379575d83a854602 Mon Sep 17 00:00:00 2001 From: diosmosis Date: Sun, 22 Dec 2019 07:42:17 -0800 Subject: [PATCH 2/6] do not fail if site summary is not available --- .../Visualizations/HtmlTable.php | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/plugins/CoreVisualizations/Visualizations/HtmlTable.php b/plugins/CoreVisualizations/Visualizations/HtmlTable.php index cf926faf23d..5dc6461c4f6 100644 --- a/plugins/CoreVisualizations/Visualizations/HtmlTable.php +++ b/plugins/CoreVisualizations/Visualizations/HtmlTable.php @@ -84,7 +84,8 @@ public function beforeRender() } if ($this->dataTable->getRowsCount()) { - $this->assignTemplateVar('siteTotalRow', $this->getSiteSummary()->getFirstRow()); + $siteTotalRow = $this->getSiteSummary() ? $this->getSiteSummary()->getFirstRow() : null; + $this->assignTemplateVar('siteTotalRow', $siteTotalRow); } if ($this->isPivoted()) { @@ -220,7 +221,7 @@ private function calculateTotalPercentages() $totals = $this->dataTable->getMetadata('totals'); $siteSummary = $this->getSiteSummary(); - $siteTotalRow = $siteSummary->getFirstRow(); + $siteTotalRow = $siteSummary ? $siteSummary->getFirstRow() : null; foreach ($this->dataTable->getRows() as $row) { foreach ($this->report->getMetrics() as $column => $translation) { @@ -234,16 +235,19 @@ private function calculateTotalPercentages() } $reportTotal = isset($totals[$column]) ? $totals[$column] : 0; - $siteTotal = $siteTotalRow->getColumn($column) ?: 0; $percentageColumnName = $column . '_row_percentage'; $rowPercentage = $formatter->formatPercent(Piwik::getPercentageSafe($value, $reportTotal, $precision = 1)); $row->setMetadata($percentageColumnName, $rowPercentage); - $siteTotalPercentage = $column . '_site_total_percentage'; - if ($siteTotal && $siteTotal > $reportTotal) { - $rowPercentage = $formatter->formatPercent(Piwik::getPercentageSafe($value, $siteTotal, $precision = 1)); - $row->setMetadata($siteTotalPercentage, $rowPercentage); + if ($siteTotalRow) { + $siteTotal = $siteTotalRow->getColumn($column) ?: 0; + + $siteTotalPercentage = $column . '_site_total_percentage'; + if ($siteTotal && $siteTotal > $reportTotal) { + $rowPercentage = $formatter->formatPercent(Piwik::getPercentageSafe($value, $siteTotal, $precision = 1)); + $row->setMetadata($siteTotalPercentage, $rowPercentage); + } } } } From 6b228c8f78acb708fa3bed29334e35bbcf3e17c7 Mon Sep 17 00:00:00 2001 From: diosmosis Date: Tue, 31 Dec 2019 09:46:42 -0800 Subject: [PATCH 3/6] Make sure siteSummary requests total data without segment. --- .../CoreVisualizations/Visualizations/HtmlTable.php | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/plugins/CoreVisualizations/Visualizations/HtmlTable.php b/plugins/CoreVisualizations/Visualizations/HtmlTable.php index 5dc6461c4f6..29d3a6375ec 100644 --- a/plugins/CoreVisualizations/Visualizations/HtmlTable.php +++ b/plugins/CoreVisualizations/Visualizations/HtmlTable.php @@ -289,23 +289,18 @@ private function getSiteSummary() $period = 'range'; } - $request = new ApiRequest(array( - 'method' => 'API.get', + $this->siteSummary = ApiRequest::processRequest('API.get', [ 'module' => 'API', 'action' => 'get', - 'format' => 'original', 'filter_limit' => '-1', 'disable_generic_filters' => 1, - 'expanded' => 0, - 'flat' => 0, 'filter_offset' => 0, + 'date' => Common::getRequestVar('date', null, 'string'), + 'idSite' => Common::getRequestVar('idSite', null, 'int'), 'period' => $period, 'showColumns' => implode(',', $this->config->columns_to_display), 'columns' => implode(',', $this->config->columns_to_display), - 'pivotBy' => '' - )); - - $this->siteSummary = $request->process(); + ], $default = []); } return $this->siteSummary; From 44d3444a768b7cb785a54431a09fc8316a0b12d9 Mon Sep 17 00:00:00 2001 From: diosmosis Date: Tue, 31 Dec 2019 16:14:19 -0800 Subject: [PATCH 4/6] Make sure proper precision is used. --- plugins/CoreVisualizations/Visualizations/HtmlTable.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/CoreVisualizations/Visualizations/HtmlTable.php b/plugins/CoreVisualizations/Visualizations/HtmlTable.php index 29d3a6375ec..72a7964f2b4 100644 --- a/plugins/CoreVisualizations/Visualizations/HtmlTable.php +++ b/plugins/CoreVisualizations/Visualizations/HtmlTable.php @@ -237,7 +237,7 @@ private function calculateTotalPercentages() $reportTotal = isset($totals[$column]) ? $totals[$column] : 0; $percentageColumnName = $column . '_row_percentage'; - $rowPercentage = $formatter->formatPercent(Piwik::getPercentageSafe($value, $reportTotal, $precision = 1)); + $rowPercentage = $formatter->formatPercent(Piwik::getPercentageSafe($value, $reportTotal, $precision = 1), $precision); $row->setMetadata($percentageColumnName, $rowPercentage); if ($siteTotalRow) { @@ -245,7 +245,7 @@ private function calculateTotalPercentages() $siteTotalPercentage = $column . '_site_total_percentage'; if ($siteTotal && $siteTotal > $reportTotal) { - $rowPercentage = $formatter->formatPercent(Piwik::getPercentageSafe($value, $siteTotal, $precision = 1)); + $rowPercentage = $formatter->formatPercent(Piwik::getPercentageSafe($value, $siteTotal, $precision = 1), $precision); $row->setMetadata($siteTotalPercentage, $rowPercentage); } } From 65348bec154dd811fbc2f99997ab37b5674f073b Mon Sep 17 00:00:00 2001 From: diosmosis Date: Wed, 1 Jan 2020 07:49:46 -0800 Subject: [PATCH 5/6] try to fix tests --- core/Twig.php | 12 ++++++++++ .../Visualizations/HtmlTable.php | 6 ++--- .../_dataTableViz_htmlTable_comparisons.twig | 1 - .../_dataTableViz_htmlTable_ratio.twig | 24 +++++++++++++++---- 4 files changed, 34 insertions(+), 9 deletions(-) diff --git a/core/Twig.php b/core/Twig.php index b8b92f283e4..afb6a1cdc27 100644 --- a/core/Twig.php +++ b/core/Twig.php @@ -198,6 +198,7 @@ public function __construct() $this->addTest_false(); $this->addTest_true(); $this->addTest_emptyString(); + $this->addTest_isNumeric(); $this->twig->addExtension(new PiwikTwigFilterExtension()); } @@ -575,4 +576,15 @@ private function addFilter_safelink() }); $this->twig->addFilter($safelink); } + + private function addTest_isNumeric() + { + $test = new Twig_SimpleTest( + 'numeric', + function ($value) { + return is_numeric($value); + } + ); + $this->twig->addTest($test); + } } diff --git a/plugins/CoreVisualizations/Visualizations/HtmlTable.php b/plugins/CoreVisualizations/Visualizations/HtmlTable.php index 72a7964f2b4..21116fefb92 100644 --- a/plugins/CoreVisualizations/Visualizations/HtmlTable.php +++ b/plugins/CoreVisualizations/Visualizations/HtmlTable.php @@ -227,10 +227,8 @@ private function calculateTotalPercentages() foreach ($this->report->getMetrics() as $column => $translation) { $indexColumn = $columnNamesToIndices[$column]; // TODO: This might fail for goal specific columns... - $value = $row->getColumn($indexColumn) ?: $row->getColumn($column); - if ($column == 'label' - || $value === false - ) { + $value = $row->getColumn($indexColumn) ?: $row->getColumn($column) ?: 0; + if ($column == 'label') { continue; } diff --git a/plugins/CoreVisualizations/templates/_dataTableViz_htmlTable_comparisons.twig b/plugins/CoreVisualizations/templates/_dataTableViz_htmlTable_comparisons.twig index 3ad571d9aae..b8f9356bc81 100644 --- a/plugins/CoreVisualizations/templates/_dataTableViz_htmlTable_comparisons.twig +++ b/plugins/CoreVisualizations/templates/_dataTableViz_htmlTable_comparisons.twig @@ -67,7 +67,6 @@ 'totals': rowComparisonTotals, 'label': comparedRow.getColumn('label'), 'labelColumn': properties.columns_to_display|first, - 'changePercantage': columnChange, 'forceZero': true, 'tooltipSuffix': comparisonTooltipSuffix, 'translations': properties.translations, diff --git a/plugins/CoreVisualizations/templates/_dataTableViz_htmlTable_ratio.twig b/plugins/CoreVisualizations/templates/_dataTableViz_htmlTable_ratio.twig index 9014e2ddb08..a478c23bf8b 100644 --- a/plugins/CoreVisualizations/templates/_dataTableViz_htmlTable_ratio.twig +++ b/plugins/CoreVisualizations/templates/_dataTableViz_htmlTable_ratio.twig @@ -7,13 +7,29 @@ {% set siteTotal = 0 %} {% endif %} - {% set rowPercentage = row.getMetadata(column ~ '_row_percentage') %} + {% if rowPercentage|default is empty %} + {% if row.getMetadata(column ~ '_row_percentage') != false %} + {% set rowPercentage = row.getMetadata(column ~ '_row_percentage') %} + {% elseif row.getColumn(column)|default(0) is numeric and reportTotal is numeric %} + {% set rowPercentage = row.getColumn(column)|percentage(reportTotal, 1) %} + {% else %} + {% set rowPercentage = '' %} {# should not happen #} + {% endif %} + {% endif %} + {% set metricTitle = translations[column]|default(column) %} {% set reportRatioTooltip = 'General_ReportRatioTooltip'|translate(label, rowPercentage|e('html_attr'), reportTotal|e('html_attr'), metricTitle|e('html_attr'), '"' ~ segmentTitlePretty ~ '"', translations[labelColumn]|default(labelColumn)|e('html_attr')) %} - {% if row.getMetadata(column ~ '_site_total_percentage') != false %} - {% set totalPercentage = row.getMetadata(column ~ '_site_total_percentage') %} + {% if totalPercentage|default is empty %} + {% if row.getMetadata(column ~ '_site_total_percentage') != false %} + {% set totalPercentage = row.getMetadata(column ~ '_site_total_percentage') %} + {% elseif row.getColumn(column)|default(0) is numeric and siteTotal is numeric %} + {% set totalPercentage = row.getColumn(column)|percentage(siteTotal, 1) %} + {% endif %} + {% endif %} + + {% if totalPercentage|default is not empty %} {% set totalRatioTooltip = 'General_TotalRatioTooltip'|translate(totalPercentage, siteTotal|number(2,0), metricTitle, periodTitlePretty) %} {% else %} {% set totalRatioTooltip = '' %} @@ -21,5 +37,5 @@  {{ rowPercentage }} {% if changePercantage|default is not empty %}({{ changePercentage }}){% endif %} + > {{ rowPercentage }} {% if changePercentage|default is not empty %}({{ changePercentage }}){% endif %} {%- endif %} From d79abec69e20a99fdd8c4fd785876cfc95f84f7e Mon Sep 17 00:00:00 2001 From: diosmosis Date: Thu, 2 Jan 2020 17:29:34 -0800 Subject: [PATCH 6/6] update expected screenshots --- .../GoalsTable_goals_table_abandoned_carts.png | 4 ++-- .../GoalsTable_goals_table_ecommerce_view.png | 4 ++-- .../UI/expected-screenshots/UIIntegrationTest_dashboard2.png | 4 ++-- .../UIIntegrationTest_ecommerce_products.png | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/UI/expected-screenshots/GoalsTable_goals_table_abandoned_carts.png b/tests/UI/expected-screenshots/GoalsTable_goals_table_abandoned_carts.png index e19c74f4d4b..63a16248027 100644 --- a/tests/UI/expected-screenshots/GoalsTable_goals_table_abandoned_carts.png +++ b/tests/UI/expected-screenshots/GoalsTable_goals_table_abandoned_carts.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:1c12da1e7834e68f8028aeb580adad02bc5beff0a91f0202aee84663aa647480 -size 41571 +oid sha256:ef2a411199babceda9806b51ed8610ef0cc4afa3516e7119e0f7212338ac77e7 +size 41568 diff --git a/tests/UI/expected-screenshots/GoalsTable_goals_table_ecommerce_view.png b/tests/UI/expected-screenshots/GoalsTable_goals_table_ecommerce_view.png index 9759f73c2f7..1ea2378daa2 100644 --- a/tests/UI/expected-screenshots/GoalsTable_goals_table_ecommerce_view.png +++ b/tests/UI/expected-screenshots/GoalsTable_goals_table_ecommerce_view.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:480395ff18829784978b26b41fed308c1a7ec797e8f68eff2ea3ab153a74fe9c -size 39432 +oid sha256:08ca07f6306ebee2f3b849068330cb5c43d7e72e6652aacd131a49fe1f839814 +size 39429 diff --git a/tests/UI/expected-screenshots/UIIntegrationTest_dashboard2.png b/tests/UI/expected-screenshots/UIIntegrationTest_dashboard2.png index e823805d279..aa4a4516874 100644 --- a/tests/UI/expected-screenshots/UIIntegrationTest_dashboard2.png +++ b/tests/UI/expected-screenshots/UIIntegrationTest_dashboard2.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:1bed9a3c9f2e5c124a220c3152ccbad1ef49c5093ed6a394aa56793c0bd768b7 -size 1566862 +oid sha256:940bd0c6303cb7be7a4ebb5a9c6eb5b1ad76f7665f8d944f6b043b8f25a87b4e +size 1565674 diff --git a/tests/UI/expected-screenshots/UIIntegrationTest_ecommerce_products.png b/tests/UI/expected-screenshots/UIIntegrationTest_ecommerce_products.png index ea244b88629..d3fb37652b1 100644 --- a/tests/UI/expected-screenshots/UIIntegrationTest_ecommerce_products.png +++ b/tests/UI/expected-screenshots/UIIntegrationTest_ecommerce_products.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:1f47207e2d7a3f179867438293837658249eea1f323c52bab13516e9e21aefd7 -size 49575 +oid sha256:e4ca9e56aa44279ea815897f9ee75f41156bfe7bc86b64c1cfd4f9ad44ca5fb7 +size 49716