Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Disable features that are inaccurate if visit data is not profilable #16773
base: 4.x-dev
Are you sure you want to change the base?
Disable features that are inaccurate if visit data is not profilable #16773
Changes from 43 commits
cb6daac
aa1d459
193c0e8
6bef1e6
203d9d7
60d6af3
d7345ed
bd15cda
939f6e3
22300fb
b3cd71c
8977dd4
ec7d33e
6df46dc
1a1b3b3
be876af
3f37ae3
3e603c8
0f54f90
9cc29a3
1833576
a077880
909f772
4d9d066
edf1b17
130419a
6843c1f
afa6503
ca91a39
de80d92
f7dda63
f044c4b
74d09ea
f9d3993
dd5a6bf
a9c4d22
d93b345
728c3d5
74f5db1
2c27a98
21de72d
02ca3d4
e45f361
7949371
7836482
c3f21ce
6555968
d3579bd
2e944ab
ecd256e
6d83b61
3f48e0c
cb630cb
2721ef9
2d4c7ee
ce6534f
2c84a17
3a84121
36bbccb
c0d2262
8fbcd7e
b8ff9d4
78093a6
79355ac
08c1608
ba5d50f
455685c
1713914
a686a65
e3942cc
d3875a9
d0528ff
7b8b8e8
5247254
48532ca
9213ad3
7420ec7
6920256
cc5f582
7418a98
122fb74
ab82c21
dc14820
26eb2a2
8757e25
4289b9d
4a4e571
69711ae
b6c7996
c140fd7
4179989
61a5f9f
84d3d8e
994da69
4295506
9e13c69
f1f6ddd
5a517c6
24efff8
675b382
986624d
6659c48
49587b1
3996bf6
3276bf1
7fe1b1d
173deaa
d345160
33f8625
524adb6
277018c
b372c39
0694bb4
07fe8fb
792d7aa
440bdb0
24e23c2
6636838
f439985
e4367a6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
fyi haven't looked too much through the code yet and not tested yet but I would assume this will be responsible to remove certain reports from the reporting UI page when the data is not profilable? I suppose when changing the date the reports won't change since we wouldn't reload the available reports. to be checked. just thought I leave the comment here while I have it on my mind.
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.
I'll test this, it's been a while so I can't remember if this is something I tested or not.
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.
Actually if we don't remove reports/segments but disable them some other way, this changes a lot, so I'll wait for an answer there.
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.
@mattab any thoughts? For reports I would say we always show them and simply show a message why a report doesn't show any data (because not profilable).
For segments I think @mattab wanted segments to be removed (or someone else). But they may still be needed for historical data etc. Or if sometimes you have profilable data and sometimes not (we used to have this case on matomo org but might be bit edge case)
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.
Segments are removed in the UI but nowhere else. We can also disable them and provide a tooltip saying why it doesn't make sense to use them on the current period. But when creating the segment, the period is arbitrary (eg, maybe they want it to be applied to a period w/ profilable data). Seems odd to see it greyed out and realize you have to change the period to be able to use it.
Perhaps it would be better to detect when a segment in use (ie, in the
segment=
query parameter) isn't going to provide correct data because the current period is not profilable, and display a notification.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.
@mattab can you provide an answer/opinion on ^?
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.
Yes, this would be the desired solution, it's less likely to break things and will also reduce any possible confusion.
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.
After doing more testing, i noticed sometimes they're removed and sometimes they have a footer message. I reckon that when >= 99% of all visits are not profilable, then we could directly hide all the reports. It makes the UI a lot cleaner. But if it causes difficulties, then it's fine to show all reports and have a visible warning notification in the footer?
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.
showing all reports but when there's no profilable data for the selected period, there is a footer message saying so