Some work on report totals #13555

tsteur · 2018-10-08T04:02:13Z

@mattab I would need someone to check all reports and various features to check if the totals make sense and when it breaks and when it works etc.

A bonus is that it shows on hover percentage values for some % rate columns. The shown percentage here is basically compared to the average (would need to somehow adjust hover text). Doesn't work for time metrics though because then other percentages break.

While this implementation could work in some cases, I have no idea if that always works or shows correct data.

If this implementation doesn't work as expected, we may need to schedule this for 3.8 or 3.9 if it is important as it would be a lot of work.

refs #5267

tsteur · 2018-10-08T04:07:02Z

core/API/DataTableManipulator/ReportTotalsCalculator.php

+            if ($row->getColumn('label') === 'Total12345') {
+
+                $this->totals = $row->getColumns();
+                $dataTable->setMetadata('totals', $this->totals);


We could likely also set here something like $dataTable->setTotalsRow() so plugins can at least have the chance to hook onto it...

It's not that easy though. Tried eg this:

diff --git a/core/API/DataTableManipulator/ReportTotalsCalculator.php b/core/API/DataTableManipulator/ReportTotalsCalculator.php index 4baac2f939..b1439f61ae 100644 --- a/core/API/DataTableManipulator/ReportTotalsCalculator.php +++ b/core/API/DataTableManipulator/ReportTotalsCalculator.php @@ -127,6 +127,8 @@ class ReportTotalsCalculator extends DataTableManipulator if ($row->getColumn('label') === 'Total12345') { $this->totals = $row->getColumns(); + $row->setColumn('label', 'Totals'); + $dataTable->setTotalsRow($totalRow); $dataTable->setMetadata('totals', $this->totals); break; } diff --git a/core/DataTable.php b/core/DataTable.php index 22b09b97ee..9189170dac 100644 --- a/core/DataTable.php +++ b/core/DataTable.php @@ -301,6 +301,11 @@ class DataTable implements DataTableInterface, \IteratorAggregate, \ArrayAccess */ protected $summaryRow = null; + /** + * @var \Piwik\DataTable\Row + */ + protected $totalsRow = null; + /** * Table metadata. Read [this](#class-desc-the-basics) to learn more. * @@ -402,6 +407,16 @@ class DataTable implements DataTableInterface, \IteratorAggregate, \ArrayAccess } } + public function setTotalsRow(Row $totals) + { + $this->totalsRow = $totals; + } + + public function getTotalsRow() + { + return $this->totalsRow; + } + /** * Returns the name of the column this table was sorted by (if any). * diff --git a/core/Plugin/Visualization.php b/core/Plugin/Visualization.php index 614aefcffa..82129b8f8b 100644 --- a/core/Plugin/Visualization.php +++ b/core/Plugin/Visualization.php @@ -209,9 +209,8 @@ class Visualization extends ViewDataTable } if ($this->dataTable && $this->dataTable instanceof DataTable) { - $totals = $this->dataTable->getMetadata('totals'); + $totals = $this->dataTable->getTotalsRow(); if (!empty($totals)) { - $totals['label'] = 'Totals'; $this->dataTable->addRow(new DataTable\Row(array(DataTable\Row::COLUMNS => $totals))); } }

but breaks eg totals feature when flattening as it seems to create an empty clone currently and not copy totals row etc (to be implemented)

tsteur · 2018-10-09T21:53:34Z

Should work

diosmosis · 2018-12-04T05:03:30Z

core/API/DataTableManipulator/ReportTotalsCalculator.php

+                $row->setColumn('label', Piwik::translate('General_Totals'));
+                $dataTable->setTotalsRow($row);
+            }
+        }


I guess this code is what you are referring to in the ticket description? It definitely seems complicated... If the main issue for not doing it the simple way is BC (ie, it might break some reports), maybe we can whitelist which reports support it?

Eg, in the Report class there could be a supportsTotalsRow() method that defaults to false, then we could make sure core reports support it even if we have to tweak the API code (eg, changing MultiSites to ignore the totals row). What do you think?

Yes, I think now it works quite well though, and might actually even work better than before but not sure if I am missing something.

I was thinking about having to clone the datatable and make accomadations for specific filters/processed metrics/etc. What do you think about the idea w/ supportsTotalsRow()?

Sure could add it, but I think it's not really needed. Can't hurt though maybe to add it and we just set it to true by default so a report can opt out. Before it was basically supported on pretty much all reports so be great to still have it on those reports.
It should support pretty much all reports as it is now using

$tableMeta = $firstLevelTable->getMetadata(DataTable::COLUMN_AGGREGATION_OPS_METADATA_NAME); $totalRow->sumRow($row, $copyMetadata = false, $tableMeta)

instead of doing the sum in the code directly where I noticed even some problems but forgot which ones. There's some "hack" for some reports that don't add the processed metrics in the report directly $clone->filter('AddColumnsProcessedMetrics', array($deleteRowsWithNoVisit = false)); but otherwise it should be fine.

diosmosis · 2018-12-05T05:31:55Z

Would it help to make the totals row bold to differentiate it from other rows (since this row will stay even while paginating)? Eg:

diosmosis · 2018-12-05T05:32:26Z

I suppose the percent doesn't need to be shown here:

diosmosis · 2018-12-05T05:34:04Z

Doesn't seem to work w/ goals columns:

diosmosis · 2018-12-05T05:42:22Z

don't think it's a big deal, but I noticed the total row option will display, even if there's no data in the report:

I'm sure most will understand, but it seems a bit odd.

tsteur · 2018-12-06T03:26:54Z

@diosmosis for now added the supportsTotalsRow method... the totals row should be bold. Because the change is in a _dataTable.less starting with underscore you might need to clear your tmp/assets cache.

diosmosis · 2018-12-06T05:04:30Z

Still need support for goal columns if possible. Unless it's hard to do I guess?

tsteur · 2018-12-06T18:50:52Z

@diosmosis I just remember a report can already disable totals row through viewDataTable config show_totals_row and can also set a request parameter totals=0 so I'm thinking it is actually better to remove the supportsTotalsRow method again which only applies to one visualisation kind of anyway (and the API but there it shouldn't cause a problem, it's more visualisation specific). Keeps the report class more simple.

tsteur · 2018-12-06T20:29:19Z

@diosmosis just pushed another commit with various fixes/changes

It wasn't showing the totals row when it was saved from previous usage (saved viewDataTable params)
No longer show it when report is empty
Disabled it for goals visualisation (still works for all columns visualisation in the goals report etc). Tried to make this work for over an hour but gave up after a while. There's so much special code around it and don't understand everything. It looked like this what I had but didn't quite produce correct numbers... I think there might not be much to do to make it work but couldn't quite figure out where things are replaced in what way...

diff --git a/plugins/Goals/Visualizations/Goals.php b/plugins/Goals/Visualizations/Goals.php
index 4453b2f352..2283b84dba 100644
--- a/plugins/Goals/Visualizations/Goals.php
+++ b/plugins/Goals/Visualizations/Goals.php
@@ -8,8 +8,10 @@
 
 namespace Piwik\Plugins\Goals\Visualizations;
 
+use Piwik\API\DataTablePostProcessor;
 use Piwik\API\Request;
 use Piwik\Common;
+use Piwik\DataTable;
 use Piwik\DataTable\Filter\AddColumnsProcessedMetricsGoal;
 use Piwik\Piwik;
 use Piwik\Plugins\CoreVisualizations\Visualizations\HtmlTable;
@@ -41,6 +43,16 @@ class Goals extends HtmlTable
 
     public function beforeRender()
     {
+        if ($this->dataTable->getTotalsRow()){
+            $dataTable = new DataTable();
+            $dataTable->addRow($this->dataTable->getTotalsRow());
+            $this->setShowGoalsColumnsProperties($dataTable);
+            $processor = new DataTablePostProcessor($this->requestConfig->getApiModuleToRequest(), $this->requestConfig->getApiMethodToRequest(), $this->request->getRequestArray());
+            $processor->applyComputeProcessedMetrics($dataTable);
+            $dataTable = $processor->applyMetricsFormatting($dataTable);
+            $this->dataTable->setTotalsRow($dataTable->getFirstRow());
+        }
+
         $this->config->show_goals = true;
         $this->config->show_goals_columns  = true;
         $this->config->datatable_css_class = 'dataTableVizGoals';
@@ -57,7 +69,7 @@ class Goals extends HtmlTable
         parent::beforeRender();
     }
 
-    private function setShowGoalsColumnsProperties()
+    private function setShowGoalsColumnsProperties($dataTable = null)
     {
         // set view properties based on goal requested
         $idSite = Common::getRequestVar('idSite', null, 'int');
@@ -83,7 +95,11 @@ class Goals extends HtmlTable
         }
 
         // add goals columns
-        $this->config->filters[] = array('AddColumnsProcessedMetricsGoal', array($enable = true, $idGoal, $goalsToProcess), $priority = true);
+        if (!empty($dataTable)) {
+            $dataTable->filter('AddColumnsProcessedMetricsGoal', array($enable = true, $idGoal, $goalsToProcess));
+        } else {
+            $this->config->filters[] = array('AddColumnsProcessedMetricsGoal', array($enable = true, $idGoal, $goalsToProcess), $priority = true);
+        }
     }
 
     private function setPropertiesForEcommerceView()

tsteur added 2 commits October 8, 2018 16:53

some work on totals

c4f889d

fix graphs

da314e9

tsteur added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Oct 8, 2018

tsteur commented Oct 8, 2018

View reviewed changes

tsteur added this to the 3.7.0 milestone Oct 8, 2018

tsteur added 4 commits October 8, 2018 18:52

tweak rendering of totals row

1971619

disable evolution for totals row

3ac8055

let plugins configure totals row visibility

e4ca6f9

remove metadata

30960ca

tsteur mentioned this pull request Oct 8, 2018

Display a new row "Total" showing the sum of values for all DataTable columns #5267

Closed

tsteur added 8 commits October 9, 2018 09:10

more tweaks and make it configurable

f253f90

improve code and performance

28783b4

fix some tests

96d5eca

lots of improvements

feddae0

support bandwidth and other plugins

bc48c9b

fix tests

26e90ae

add screenshot test

757ab86

also fix existing test

8edddd5

diosmosis reviewed Dec 4, 2018

View reviewed changes

add new method supportsTotalRow

053b6d5

Merge branch '3.x-dev' into 5267

99ae2f6

various fixes

5a54eaa

Updating some expected test files.

2725fba

diosmosis merged commit 53c4d95 into 3.x-dev Dec 7, 2018

diosmosis deleted the 5267 branch December 7, 2018 02:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some work on report totals #13555

Some work on report totals #13555

tsteur commented Oct 8, 2018 •

edited

tsteur Oct 8, 2018

tsteur commented Oct 9, 2018

diosmosis Dec 4, 2018

tsteur Dec 4, 2018

diosmosis Dec 4, 2018

tsteur Dec 4, 2018

diosmosis commented Dec 5, 2018

diosmosis commented Dec 5, 2018

diosmosis commented Dec 5, 2018

diosmosis commented Dec 5, 2018 •

edited

tsteur commented Dec 6, 2018

diosmosis commented Dec 6, 2018

tsteur commented Dec 6, 2018

tsteur commented Dec 6, 2018

Some work on report totals #13555

Some work on report totals #13555

Conversation

tsteur commented Oct 8, 2018 • edited

tsteur Oct 8, 2018

Choose a reason for hiding this comment

tsteur commented Oct 9, 2018

diosmosis Dec 4, 2018

Choose a reason for hiding this comment

tsteur Dec 4, 2018

Choose a reason for hiding this comment

diosmosis Dec 4, 2018

Choose a reason for hiding this comment

tsteur Dec 4, 2018

Choose a reason for hiding this comment

diosmosis commented Dec 5, 2018

diosmosis commented Dec 5, 2018

diosmosis commented Dec 5, 2018

diosmosis commented Dec 5, 2018 • edited

tsteur commented Dec 6, 2018

diosmosis commented Dec 6, 2018

tsteur commented Dec 6, 2018

tsteur commented Dec 6, 2018

tsteur commented Oct 8, 2018 •

edited

diosmosis commented Dec 5, 2018 •

edited