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.

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

[Graphic]reload table if the default date changes #19349

Merged
merged 7 commits into from Jun 15, 2022
Merged

Conversation

peterhashair
Copy link
Contributor

@peterhashair peterhashair commented Jun 13, 2022

Description:

Fixes: #18781
reload table if the default date changes. that may cause performance issue

Please include a description of this change and which issue it fixes. If no issue exists yet please include context and what problem it solves.

Review

reload table if the changes
@bx80
Copy link
Contributor

bx80 commented Jun 13, 2022

@peterhashair I think the desired behavior should be to use the configured defaultLastN period unless there is a remembered lastN from a previous selection.

It seems this is almost happening except that the lastN dropdown value is initialized as 90 days but the data table is loading data for 30 days. I think the initial data table load should match the value of the lastN dropdown (90 days in this case).

Forcing a reload of the data table when it already exists is going probably to be bad for performance as this will apply to every visualization in every circumstance so big data tables may be reloaded multiple times when they don't need to be.

It might be worth checking where both the evolution lastN dropdown and data table load are getting their initial values. Whatever logic is setting the lastN dropdown to 90days should also be instructing the data table initial load to be 90days too. It would be better to get the data table to load the correct lastN first time rather than having it load the wrong values and then do a second load for the correct values. Hope that makes some sense! 🙂

add reload flag
@peterhashair
Copy link
Contributor Author

@bx80 yes, I add a flag in to the evolution, when the lastN is remember force the reload, otherwise, it won't reload.

@peterhashair peterhashair marked this pull request as ready for review June 14, 2022 00:39
@peterhashair peterhashair added this to the 4.12.0 milestone Jun 14, 2022
@peterhashair peterhashair added 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. and removed Needs Review PRs that need a code review labels Jun 14, 2022
Peter added 2 commits June 14, 2022 15:42
This reverts commit f3ffe62.
add cache load on rowEvolution
@peterhashair
Copy link
Contributor Author

@bx80 fixed it, found how to load the data, and put it onto the first API loads. but it seems like the PHPCS started to fail on all my PRs.

@peterhashair peterhashair added the Needs Review PRs that need a code review label Jun 14, 2022
@peterhashair peterhashair requested a review from bx80 June 14, 2022 23:34
@bx80
Copy link
Contributor

bx80 commented Jun 15, 2022

This is works well for me, nice to catch the cached parameters and get the table loading the correct data first time 👍

Other than the array key validation tweak, it should be good to go 🙂

Co-authored-by: Ben Burgess <88810029+bx80@users.noreply.github.com>
@peterhashair peterhashair merged commit 494533f into 4.x-dev Jun 15, 2022
@peterhashair peterhashair deleted the m18781 branch June 15, 2022 05:58
//handle cache if exist
$cache = ViewDataTableManager::getViewDataTableParameters(Piwik::getCurrentUserLogin(),
'CoreHome.getRowEvolutionGraph');
$lastDay = (isset($cache['evolution_' . $this->period . '_last_n']) ? $cache['evolution_' . $this->period . '_last_n'] : null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small note: This could have been simplified to:

$lastDay = $cache['evolution_' . $this->period . '_last_n']) ?? null;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

Show correct "Rows to display" on Evolution graph
3 participants