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

Sort segment static lists by usage #18321

Merged
merged 5 commits into from Nov 19, 2021

Conversation

bx80
Copy link
Contributor

@bx80 bx80 commented Nov 16, 2021

Description:

Fixes #15620

API calls to suggested values for segments that are static lists (eg. countries) will now be returned sorted by the most used values first. This should mitigate the issue where suggested value lists do not contain the most frequently used values.

For example, the getSuggestedValuesForSegment list for countryName will now return a list of country names with the countries that have the most visits at the top rather than an alphabetical list potentially containing countries that have no visits or missing countries with many visits but names towards the end of the alphabet.

This has been applied to segments: countryName, browserName and operatingSystemName

Review

@bx80 bx80 added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Nov 16, 2021
@bx80 bx80 added this to the 4.7.0 milestone Nov 16, 2021
@bx80 bx80 self-assigned this Nov 16, 2021
plugins/API/API.php Outdated Show resolved Hide resolved
core/Plugin/Dimension/VisitDimension.php Outdated Show resolved Hide resolved
@sgiehl
Copy link
Member

sgiehl commented Nov 18, 2021

Looks good and works as expected.
As you mentioned it might be better not to have a static list of segments that don't need the most frequent values. A possibility might be to change the Segment class. So it has a new property that defines if it requires most frequent values or not. And the method setSuggestedValuesCallback could have a second (optional) parameter that sets the property.

@bx80
Copy link
Contributor Author

bx80 commented Nov 19, 2021

I've added a property to the Segment class to indicate if it requires the frequent values method to be run. The static list of segments has been removed.

@bx80 bx80 added the Needs Review PRs that need a code review label Nov 19, 2021
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.

Looks good now 👍

@sgiehl sgiehl merged commit b426c7b into 4.x-dev Nov 19, 2021
@sgiehl sgiehl deleted the m-15620-segment-static-lists-by-usage branch November 19, 2021 10:39
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.

Specific countries appear in visit log but return no results when segmented
2 participants