Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Do not fetch totals for insights #13891

Merged
merged 6 commits into from Dec 25, 2018
Merged

Do not fetch totals for insights #13891

merged 6 commits into from Dec 25, 2018

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Dec 20, 2018 •
edited

Got this error warning

WARNING: Trying to add two strings in DataTable\Row::sumRowArray: 35.8% + 48.6% for row # ['label' => -2, 'nb_visits' => 379, 'growth_percent' => '35.8%', 'growth_percent_numeric' => '35.8', 'grown' => 1, 'value_old' => 162, 'value_new' => 220, 'difference' => 58, 'importance' => 58, 'isDisappeared' => , 'isNew' => , 'isMover' => 1] [] [idsubtable = ]<br />

The totals row should be only needed for HTML table visualisations. Bar and Pie, etc should not need this totals row and therefore disabled it by default so other visualisations don't need to disable it. Makes viewing reports faster...

@tsteur
Do not fetch totals for insights
25e0afb 
Got this error warning

> WARNING: Trying to add two strings in DataTable\Row::sumRowArray: 35.8% + 48.6% for row # ['label' =&gt; -2, 'nb_visits' =&gt; 379, 'growth_percent' =&gt; '35.8%', 'growth_percent_numeric' =&gt; '35.8', 'grown' =&gt; 1, 'value_old' =&gt; 162, 'value_new' =&gt; 220, 'difference' =&gt; 58, 'importance' =&gt; 58, 'isDisappeared' =&gt; , 'isNew' =&gt; , 'isMover' =&gt; 1] [] [idsubtable = ]&lt;br /&gt;
@tsteur
also do not fetch totals for movers and shakers
8133869
@tsteur
Update Model.php
94e4b0d
@tsteur
by default fetch totals only when html tables are requested
7e05d97
@tsteur tsteur added this to the 3.8.0 milestone Dec 20, 2018
@tsteur 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 Dec 20, 2018
@tsteur
Copy link
Member Author

tsteur commented Dec 20, 2018

Checked on our instance and the notice don't seem to come up anymore.

@sgiehl
Copy link
Member

sgiehl commented Dec 21, 2018

Tried to have a closer look. But actually it's a bit confusing how that's currently working:

In visualizations (except of HtmlTable) totals is always set to false, but the calculation in PHP defaults to true. e.g.

matomo/core/API/DataTableManipulator/ReportTotalsCalculator.php

Line 83 in a860a9b

if (1 != Common::getRequestVar('totals', 1, 'integer', $this->request)) {
and

matomo/core/API/DataTablePostProcessor.php

Line 199 in 40d68b9

if (1 == Common::getRequestVar('totals', '1', 'integer', $this->request)) {

That means whenever the totals parameter is missing it will be generated, which is also the case when fetching Insights.getInsights, or viewing charts like Pie.

To fully disable total row calculation for Insights, you might need to add totals => 0 here:

matomo/plugins/Insights/Visualizations/Insight.php

Lines 48 to 59 in 7e05d97

$this->requestConfig->request_parameters_to_modify = array(
'reportUniqueId' => $report,
'minImpactPercent' => $this->requestConfig->min_impact_percent,
'minGrowthPercent' => $this->requestConfig->min_growth_percent,
'comparedToXPeriods' => $this->requestConfig->compared_to_x_periods_ago,
'orderBy' => $this->requestConfig->order_by,
'filterBy' => $this->requestConfig->filter_by,
'pivotBy' => false,
'pivotByColumn' => false,
'limitIncreaser' => $this->getLimitIncrease(),
'limitDecreaser' => $this->getLimitDecrease(),
);

Summarized it seems like totals is never set in any request, except of those where you now manually added the total => 0 to the request.

Btw. this variable naming is also bit confusing, or the comment is wrong:

matomo/plugins/CoreVisualizations/Visualizations/HtmlTable/Config.php

Lines 90 to 95 in 53c4d95

/**
* If true, the totals row will be hidden
*
* Default value: false
*/
public $show_totals_row = true;

@tsteur
Copy link
Member Author

tsteur commented Dec 23, 2018

cheers @sgiehl fixed it 👍 There was also an issue that when switching visualisation from HtmlTable to Insights it would still send totals=1 in the URL.

And the $totals = false didn't work cause here it would add the parameter only when the default is not false : https://github.com/matomo-org/matomo/blob/3.8.0-b5/core/ViewDataTable/Request.php#L82-L83 Set it to 0 instead.

@diosmosis
Copy link
Member

Code seems to work, looks ok and build passes, will merge. @sgiehl if you want to review more, feel free to do so, will create a new issue or just apply a fix if everyone's on vacation.

@diosmosis diosmosis merged commit 5f28571 into 3.x-dev Dec 25, 2018
@diosmosis diosmosis deleted the totalsinsight branch December 25, 2018 23:32
@GTVolk GTVolk mentioned this pull request Apr 22, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
3.8.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tsteur @sgiehl @diosmosis