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

Hide segments containing expressions from disabled plugins or features #21521

Merged
merged 3 commits into from
Nov 16, 2023

Conversation

bx80
Copy link
Contributor

@bx80 bx80 commented Nov 13, 2023

Description:

refs #21339
Fixes #21318

When an attempt is made to use a segment which contains expressions referencing a disabled dimension then the exception Segment '$name' is not a supported segment. is thrown.

This can happen when a plugin is disabled or if certain features which provide segments are turned off, such as the visitor profile.

For the cron-based archiving process the exception is caught and a non-fatal info message is shown.

In the Matomo dashboard segments containing unavailable expressions can still be chosen in the segment selector and doing so will result in fatal errors as API calls to retrieve segment data will encounter the Segment '$name' is not a supported segment. error and return HTTP 500 codes.

How to recreate

To recreate this issue try the following steps:

  • Make sure browser_archiving_disabled_enforce = 1 is not set in config.ini.php
  • Create a new segment with the definition "visitorId==4b3d9389bae51466"
  • Disable visitor profiles in general settings (Live -> "Turn off visitor log and visitor profiles")
  • Switch to the new segment in the dashboard
  • View the goals overview page (or any other page loading widget data)
  • A front end exception will be shown and an error logged Error: Segment 'visitorId' is not a supported segment.

What this PR does

This PR filters the list of segments shown in the segment selector to remove any segments which contain unavailable expressions. If the disabled plugin or feature is re-enabled then the segments will immediately be available again.

Integration tests are included which check availability of test segments for disabled visitor id, disabled user id and a disabled plugin segment.

Review

@bx80 bx80 added Bug For errors / faults / flaws / inconsistencies etc. Regression Indicates a feature used to work in a certain way but it no longer does even though it should. labels Nov 13, 2023
@bx80 bx80 added this to the 5.0.0 milestone Nov 13, 2023
@bx80 bx80 self-assigned this Nov 13, 2023
@bx80 bx80 added the Needs Review PRs that need a code review label Nov 13, 2023
@bx80 bx80 requested review from a team and sgiehl November 13, 2023 01:23
Copy link
Contributor

@michalkleiner michalkleiner 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 to me, failing tests are unrelated.
Let's see what @sgiehl thinks.

@sgiehl
Copy link
Member

sgiehl commented Nov 15, 2023

I had some thoughts around this topic. Did you consider the following scenario when creating/testing this PR:
A segment can be create for a specific website, but also globally. In addition a segment can be valid for one website, while it can be invalid for another. e.g. Disabling the visits log is possible globally, but also in the settings for each website.
How does the UI and archiving behave for segments that are available globally and only invalid for a specific website?

@bx80
Copy link
Contributor Author

bx80 commented Nov 16, 2023

How does the UI and archiving behave for segments that are available globally and only invalid for a specific website?

Archiving will catch the exception thrown when trying to initialize the warning and then skip the segment, nothing has changed there.

For the UI, we're checking each segment against the current site before adding it to the segment selector, so if there is a globally available segment it will be checked for validity against the currently chosen site and only added to the site selector if it is valid. If the user then changes site the globally available segment will be checked again for the new site.

@sgiehl
Copy link
Member

sgiehl commented Nov 16, 2023

Ok. So for hiding unavailable segments from the UI the approach works nicely. In terms of archiving we might need to do some more testing. I'll merge this PR already, as we can implement further changes in a new PR if needed.

@sgiehl sgiehl merged commit a15cffe into 5.x-dev Nov 16, 2023
22 of 25 checks passed
@sgiehl sgiehl deleted the m21339-filter-unavailable-segments branch November 16, 2023 15:11
@sgiehl sgiehl added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc. 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. Regression Indicates a feature used to work in a certain way but it no longer does even though it should.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When disabling visitor profile feature, then also disable related segment to prevent archiving from failing
3 participants