Calculate totals row after applying search #22895
Conversation
|
If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'. |
github-actions
bot
added
the
Stale
sgiehl
force-pushed
the
dev-17662
branch
6 times, most recently
from
c6b7a33 to
459adc1
Compare
| @@ -107,6 +108,10 @@ protected function manipulateDataTable($dataTable) | |||
| /** @var DataTable\Row $totalRow */ | |||
| $totalRow = null; | |||
| foreach ($firstLevelTable->getRows() as $row) { | |||
| if ($row->getMetadata('is_aggregate') == '1') { | |||
| continue; // skip aggregated row added by flattening | |||
There was a problem hiding this comment.
Note: Before the flattening was applied after the totals row, so this wasn't needed. Now the flattener is applied before, and if include_aggregate_rows is set it add additional aggregation rows. Those rows need to be skipped for summing up values.
| } | ||
| $clone->deleteColumn($metric); | ||
| } | ||
| } |
There was a problem hiding this comment.
By moving the totals row it might now be the case that processed metrics are already calculated before. That has the effect, that the totals row calculation would simply sum up the values. As the values are then already present in the totals row, the processing triggered below wouldn't do anything, as the columns are already present.
Note: I also tried setting skip as aggregation operation for the processed metrics, hoping that would cause the columns to be missing in the result, but it turned out they are still created, but with null values. Therefor this can't be used, as the processing would still be skipped, as the column technically exists.
core/DataTable/Row.php
Outdated
| if (isset($aggregationOperations[$columnNameOrId])) { | ||
| $operationName = $aggregationOperations[$columnNameOrId]; | ||
| } elseif (is_numeric($columnNameOrId)) { | ||
| // if the column was provided as index, check if a aggration is set for the real name | ||
| $metricsIdToName = Metrics::getMappingFromIdToName(); | ||
| if (isset($metricsIdToName[$columnNameOrId]) && isset($aggregationOperations[$metricsIdToName[$columnNameOrId]])) { | ||
| $operationName = $aggregationOperations[$metricsIdToName[$columnNameOrId]]; | ||
| } | ||
| } else { | ||
| // if the column was provided as string, check if a aggration is set for the index | ||
| $metricsNameToId = Metrics::getMappingFromNameToId(); | ||
| if (isset($metricsNameToId[$columnNameOrId]) && isset($aggregationOperations[$metricsNameToId[$columnNameOrId]])) { | ||
| $operationName = $aggregationOperations[$metricsNameToId[$columnNameOrId]]; | ||
| } | ||
| } |
There was a problem hiding this comment.
This fixes an issue that in parts already appear before. We are setting aggregation operations for certain columns like here:
Lines 159 to 164 in 5863dc9
In that specific example the operations are set together with the metric id. That requires the ReplaceColumnNames filter NOT to run before, as it otherwise can't find the correct operation.
The code above will automatically try to find the correct operation by mapping metric ids to their name or vice versa to see if a provided operation is found then.
plugins/API/ProcessedReport.php
Outdated
| // remove columns from totals row that are not in metadata | ||
| foreach ($metadataTotals as $metadataCol => $metadataValue) { | ||
| if ( | ||
| isset($metadateColumns[$metadataCol]) | ||
| || preg_match('/^goal_[0-9]+_/', $metadataCol) | ||
| ) { | ||
| $simpleTotals[$metadataCol] = $metadataValue; | ||
| } | ||
| } |
There was a problem hiding this comment.
This filtering is also done for metrics displayed in the processed report, so there is no value in displaying total values for not included metrics.
This change caused the most test updates, as it now hides useless columns from a lot test results.
| <nb_uniq_visitors>61</nb_uniq_visitors> | ||
| <nb_visits>64</nb_visits> | ||
| <nb_events>69</nb_events> | ||
| <nb_events>63</nb_events> |
There was a problem hiding this comment.
Summing up all <nb_events> in the contained reports results in 63, so this now seems to be correct.
| <goal_1_nb_conversions>3</goal_1_nb_conversions> | ||
| <goal_1_revenue_per_visit>7.5</goal_1_revenue_per_visit> | ||
| <goal_1_revenue>30</goal_1_revenue> | ||
| <goal_1_nb_conversions_entry>3</goal_1_nb_conversions_entry> | ||
| <goal_1_revenue_per_entry>7.5</goal_1_revenue_per_entry> | ||
| <goal_1_revenue_entry>30</goal_1_revenue_entry> | ||
| <goal_2_nb_conversions>2</goal_2_nb_conversions> | ||
| <goal_2_revenue_per_visit>5</goal_2_revenue_per_visit> | ||
| <goal_2_revenue>20</goal_2_revenue> | ||
| <goal_2_nb_conversions_entry>1</goal_2_nb_conversions_entry> | ||
| <goal_2_revenue_per_entry>5</goal_2_revenue_per_entry> | ||
| <goal_2_revenue_entry>10</goal_2_revenue_entry> |
There was a problem hiding this comment.
Just in case someone wonders why the goal metrics conversion_rate and nb_conversions_entry_rate are missing here. This is because they are removed from processed report totals here:
matomo/plugins/API/ProcessedReport.php
Lines 808 to 810 in 4a2025e
It should though be possible to calculate them using the processed metric class somehow, but that might imply some bigger refactoring. So out of scope here.
sgiehl
force-pushed
the
dev-17662
branch
2 times, most recently
from
3b1424a to
19a275e
Compare
Co-authored-by: Michal Kleiner <[email protected]>
Description:
This PR aims to calculate the total row after search patterns were applied. This means that the total row will should total values only for rows that matched the search, instead of total values without searching.
The implementation turned out to be a lot more complex that only moving the filters around. Due to various dependencies within the filters and the applied order some additional changes were required. I've tried to add comments to all places where a change might look unexpected, so it's clear why the changes were done.
At some point in the future it would though be beneficial to fully refactor/rebuilt how the filters are applied, so it's less complex to do further changes.
fixes #21814
Review