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
Conversation
reload table if the changes
@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
@bx80 yes, I add a flag in to the evolution, when the lastN is remember force the reload, otherwise, it won't reload. |
This reverts commit f3ffe62.
add cache load on rowEvolution
@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. |
revert changes
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>
//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); |
There was a problem hiding this comment.
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;
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