New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some work on report totals #13555

Open
wants to merge 14 commits into
base: 3.x-dev
from

Conversation

Projects
None yet
1 participant
@tsteur
Member

tsteur commented Oct 8, 2018

@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 added some commits Oct 8, 2018

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

This comment has been minimized.

@tsteur

tsteur Oct 8, 2018

Member

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 tsteur added this to the 3.7.0 milestone Oct 8, 2018

tsteur added some commits Oct 8, 2018

tsteur added some commits Oct 8, 2018

@tsteur

This comment has been minimized.

Member

tsteur commented Oct 9, 2018

Should work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment