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

convert sparkline api to one request - fixes archive #18399

Merged
merged 21 commits into from Dec 9, 2021

Conversation

peterhashair
Copy link
Contributor

@peterhashair peterhashair commented Nov 29, 2021

Description:

Fixes: #18088.
convert sparking API to one request - fixes archive
Copy the logic from here :

public function getSparklines()

Review

Peter Zhang added 6 commits November 29, 2021 14:35
update lock
update sparkline
update title link
add comments
update loader
update template
@Findus23 Findus23 changed the title convert sparking api to one request - fixes archive convert sparkline api to one request - fixes archive Nov 29, 2021
@peterhashair
Copy link
Contributor Author

peterhashair commented Nov 30, 2021

@tsteur do you want to try that one, I think this is kind of working, reduce my CPU a little bit on my local on a one year period, still have failed some XML compare tests but should be fine, I will fix and tidy code more, once we confirm it reduce the load.

@tsteur
Copy link
Member

tsteur commented Nov 30, 2021

@peterhashair let's maybe keep the changes for the lock separate and only apply the UI changes here. Be good to check out my comment in chat to make use of Visualisations as much as possible.

@peterhashair
Copy link
Contributor Author

peterhashair commented Nov 30, 2021

@tsteur right, I was stuck on here, so go for the template option. Let me convert to Visualisations.

 $view = ViewDataTableFactory::build(Sparklines::ID, $apiMethod, 'Goals.' . __METHOD__, $forceDefault = true);

@peterhashair peterhashair self-assigned this Nov 30, 2021
Peter Zhang added 10 commits December 1, 2021 12:57
update to visualise
update controller
revert lock
update matrix
update config title
remove template
remove mutilple
us getMetrics
update tests
@peterhashair
Copy link
Contributor Author

peterhashair commented Dec 1, 2021

@tsteur as we discussed. I used the goal API getMetries method instead of get it should remove the for loop improve the performance a little bit.

@tsteur
Copy link
Member

tsteur commented Dec 1, 2021

Awesome, can you check if it loads faster now?

@peterhashair
Copy link
Contributor Author

peterhashair commented Dec 1, 2021

@tsteur yes, about 3-5 seconds faster compared to load period for 6 months on my local.

@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. labels Dec 1, 2021
@peterhashair peterhashair added this to the 4.7.0 milestone Dec 1, 2021
Copy link
Contributor

@bx80 bx80 left a comment

Choose a reason for hiding this comment

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

Works well for me and definitely a bit quicker. I've flagged a couple of very minor code tidies. 👍

plugins/Goals/Controller.php Outdated Show resolved Hide resolved
plugins/Goals/Pages.php Outdated Show resolved Hide resolved
Co-authored-by: Ben Burgess <88810029+bx80@users.noreply.github.com>
Peter Zhang and others added 4 commits December 10, 2021 09:59
Co-authored-by: Ben Burgess <88810029+bx80@users.noreply.github.com>
# Conflicts:
#	plugins/CoreHome/vue/dist/CoreHome.umd.min.js
@peterhashair peterhashair merged commit 35bcf9c into 4.x-dev Dec 9, 2021
@peterhashair peterhashair deleted the m-18088-fixes-archive branch December 9, 2021 23:36
@sgiehl
Copy link
Member

sgiehl commented Dec 10, 2021

@peterhashair @bx80 This actually kind of broke the goals overview page. If you would have had a look at the UI test failures you would have been able to see that. See https://builds-artifacts.matomo.org/matomo-org/matomo/m-18088-fixes-archive/52262/GoalsPages_overview.png
Please try to be a bit more accurate on that. It's not only part of the development to fix failing tests that are related, but also part of the review to check if tests are failing and if updates to any tests and expected UI files were actually valid.
I will now revert this PR. Feel free to recreate it, once it fully works.

@peterhashair
Copy link
Contributor Author

peterhashair commented Dec 12, 2021

@sgiehl right let me fix that it. But I got a little confused here, it seems like the preview Test hides the revenue doesn't matter on which conditions (I mean is that on purpose or a regression), the new approach has actually loaded the revenue when $isEcommerceEnabled. I add can hide revenue sparkline like it was or update the screenshot. which approach do you want me to?

@sgiehl
Copy link
Member

sgiehl commented Dec 13, 2021

@peterhashair Actually it doesn't really matter if that would have been a regression or not. It was not part of the issue to change the content of the reports in any way. And it's actually not only the additional sparkline, also the headline misses Goal and the other sparklines show differently. In general it would for sure be fine to fix regressions if there are any, but we should avoid doing that silently in PRs without mentioning it anywhere.

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.

Multiple requests may archive the same range request at the same time
4 participants