@peterhashair opened this Pull Request on November 29th 2021 Contributor

Description:

Fixes: #18088.
convert sparking API to one request - fixes archive
Copy the logic from here : https://github.com/matomo-org/matomo/blob/e3218471244d08039b1b84f1c907f1be0e018768/plugins/Ecommerce/Controller.php#L37

Review

@peterhashair commented on November 30th 2021 Contributor

@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 commented on November 30th 2021 Member

@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 commented on November 30th 2021 Contributor

@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 commented on December 1st 2021 Contributor

@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 commented on December 1st 2021 Member

Awesome, can you check if it loads faster now?

@peterhashair commented on December 1st 2021 Contributor

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

@sgiehl commented on December 10th 2021 Member

@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 commented on December 12th 2021 Contributor

@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 commented on December 13th 2021 Member

@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.

This Pull Request was closed on December 9th 2021
Powered by GitHub Issue Mirror