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 #18493

Merged
merged 33 commits into from Dec 19, 2021

Conversation

peterhashair
Copy link
Contributor

Description:

Description:
Fixes: #18088.

Follow the fix on UI tests

Review

Peter Zhang and others added 25 commits November 29, 2021 14:35
update lock
update sparkline
update title link
add comments
update loader
update template
update to visualise
update controller
revert lock
update matrix
update config title
remove template
remove mutilple
us getMetrics
update tests
Co-authored-by: Ben Burgess <88810029+bx80@users.noreply.github.com>
Co-authored-by: Ben Burgess <88810029+bx80@users.noreply.github.com>
# Conflicts:
#	plugins/CoreHome/vue/dist/CoreHome.umd.min.js
update goal remove revenue sparkline
remove hide extra columns
update graphic
@peterhashair peterhashair marked this pull request as ready for review December 15, 2021 01:27
@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 15, 2021
@peterhashair
Copy link
Contributor Author

this is the follow-up on the UI failed fixes. Should pass the test now.

@bx80
Copy link
Contributor

bx80 commented Dec 15, 2021

@peterhashair The only remaining failing UI screenshot UIIntegrationTest_dashboard2.png is expecting the goals overview widget goal titles to be wrapped in single quotes, where as the processed screenshot has them without.
image
It seems like it might be related to this change?

Peter Zhang added 2 commits December 15, 2021 15:31
update remove goal in the settings
update screenshot
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.

UI tests are now all passing.

I noticed that because the GetMetrics report is a now a descendant of the Get report that the view is configured slightly differently with overview Goal Names shown as Goal 'Something' instead of Goal Something. This would seem to be more consistent, what do you think @sgiehl ?

Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Generally seems to work now. Left a couple of comments for code improvements

plugins/Goals/Pages.php Outdated Show resolved Hide resolved
plugins/Goals/Controller.php Outdated Show resolved Hide resolved
plugins/Goals/Controller.php Outdated Show resolved Hide resolved
plugins/Goals/Controller.php Outdated Show resolved Hide resolved
Peter Zhang and others added 4 commits December 17, 2021 12:13
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
remove check actions and remove foreach loop on goals
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Had another idea for further code improvement. Otherwise this should be good to merge, if tests are passing.

plugins/Goals/Controller.php Outdated Show resolved Hide resolved
@sgiehl sgiehl added this to the 4.7.0 milestone Dec 17, 2021
Co-authored-by: Stefan Giehl <stefan@matomo.org>
@peterhashair peterhashair merged commit 36f83c1 into 4.x-dev Dec 19, 2021
@peterhashair peterhashair deleted the m-18088-fixes-archive branch December 19, 2021 12:39
peterhashair pushed a commit that referenced this pull request Dec 20, 2021
peterhashair pushed a commit that referenced this pull request Dec 20, 2021
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
3 participants