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

Disable visitorId segment when feature disabled #17508

Merged
merged 10 commits into from May 11, 2021

Conversation

flamisz
Copy link
Contributor

@flamisz flamisz commented Apr 29, 2021

Description:

fixes: #17488

Review

  • Functional review done
  • Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@flamisz flamisz self-assigned this Apr 29, 2021
@flamisz flamisz added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Apr 29, 2021
@flamisz flamisz added this to the 4.3.0 milestone Apr 29, 2021
@flamisz flamisz added the Needs Review PRs that need a code review label Apr 30, 2021
@flamisz flamisz marked this pull request as ready for review April 30, 2021 02:41
@tsteur
Copy link
Member

tsteur commented May 3, 2021

@flamisz could you maybe test below diff that might be an easier fix for it (dimensions usually create segments automatically)

diff --git a/plugins/CoreHome/Columns/VisitorId.php b/plugins/CoreHome/Columns/VisitorId.php
index 7c8fdec80e..fb4456c61e 100644
--- a/plugins/CoreHome/Columns/VisitorId.php
+++ b/plugins/CoreHome/Columns/VisitorId.php
@@ -9,11 +9,14 @@
 namespace Piwik\Plugins\CoreHome\Columns;
 
 use Piwik\Columns\DimensionMetricFactory;
+use Piwik\Columns\DimensionSegmentFactory;
 use Piwik\Columns\MetricsList;
 use Piwik\Metrics\Formatter;
 use Piwik\Piwik;
 use Piwik\Plugin\ArchivedMetric;
 use Piwik\Plugin\Dimension\VisitDimension;
+use Piwik\Plugins\Live\SystemSettings;
+use Piwik\Segment\SegmentsList;
 
 /**
  * Dimension for the log_visit.idvisitor column. This column is added in the CREATE TABLE
@@ -31,6 +34,17 @@ class VisitorId extends VisitDimension
     protected $sqlFilterValue = array('Piwik\Common', 'convertVisitorIdToBin');
     protected $type = self::TYPE_BINARY;
 
+    public function configureSegments(SegmentsList $segmentsList, DimensionSegmentFactory $dimensionSegmentFactory)
+    {
+        $systemSettings = new SystemSettings();
+        $visitorProfileEnabled = $systemSettings->disableVisitorProfile->getValue() === false
+            && $systemSettings->disableVisitorLog->getValue() === false;
+        if ($visitorProfileEnabled) {
+            parent::configureSegments($segmentsList, $dimensionSegmentFactory);
+        }
+
+    }
+
     public function configureMetrics(MetricsList $metricsList, DimensionMetricFactory $dimensionMetricFactory)
     {
         $metric = $dimensionMetricFactory->createMetric(ArchivedMetric::AGGREGATION_UNIQUE);

@flamisz
Copy link
Contributor Author

flamisz commented May 3, 2021

@flamisz could you maybe test below diff that might be an easier fix for it (dimensions usually create segments automatically)

Hi @tsteur this solves a part of the issue, because this way visitorId is not included in the list segment list, but still need some other changes to hide the segments that include it, etc.

I refactor the code using this part as base, thanks for pointing this out.

@flamisz
Copy link
Contributor Author

flamisz commented May 3, 2021

@tsteur I have a question:
anyhow we solve it, we have to deal with cli commands. I'm not sure which commands will be involved, but the core:archive definitely will be.
By default, if we not including visitorId in the segment list, the cli command will through an exception if there is any saved segments that has visitorId in the definition.
We still want to archive these segments, don't we? So for cli commands everything should work the same way whatever is that related live plugin setting?

@tsteur
Copy link
Member

tsteur commented May 3, 2021

@flamisz not sure with what you mean hide where it includes it?

By default, if we not including visitorId in the segment list, the cli command will through an exception if there is any saved segments that has visitorId in the definition.

That should be fine AFAIK as it's rarely used I would say and the LIVE setting is also new and rarely used yet so this combination should happen very rarely (or not at all) and we might want to rather go for the 80/20 solution. The alternative be to remove any such segments on update or when enabling this feature but I think it might not be needed.

@flamisz
Copy link
Contributor Author

flamisz commented May 3, 2021

@tsteur

@flamisz not sure with what you mean hide where it includes it?

In the saved segment list dropdown. Hide all segments that has any visitorId dimension, because if we don't hide those, choosing them will throw an error. The change you showed only hide the visitorid dimension from the action list when we create a new segment or editing one.

But we can keep them there and throw the error, as you said it's very rarely used settings, and we can expand the description for that setting to warn the admin.

So basically this issue about not using visitorid segment when this setting is turned on. What I did is:

  1. hide the visitorId dimension from the action list when creating or editing saved segments - this is what your code does too
  2. hide all saved segments from the dropdown that has visitorid dimension.
  3. and I handled the API errors with specific error message
  4. made the console commands working

Only the first point is enough? So after these modifications we will throw a Error: Segment 'visitorId' is not a supported segment when we want to use it but can't, like using the API or in a console command. This would be a very easy and small PR, only your code is enough for it.

Is this ok? Do we want to hide those saved segments? They can't use it anyway, we just throw the error then?

@flamisz
Copy link
Contributor Author

flamisz commented May 4, 2021

Just one more thing. If someone has this turned on already and has a cron job to run archiving after an update the job will fail if has any segment with visitorId. That's why I thought maybe we should handle that. But as you said, maybe there is no such a combination at all. I go with the bare minimum, update the pr, we can add more if we want.

@flamisz flamisz closed this May 4, 2021
@flamisz flamisz reopened this May 4, 2021
@tsteur
Copy link
Member

tsteur commented May 4, 2021

Just one more thing. If someone has this turned on already and has a cron job to run archiving after an update the job will fail if has any segment with visitorId. That's why I thought maybe we should handle that. But as you said, maybe there is no such a combination at all. I go with the bare minimum, update the pr, we can add more if we want.

Fully aware of this. Same problem is happening when you use segments defined by plugins and then disable the plugin. It's a problem for another day I would say :)

@tsteur tsteur merged commit 2eb0b40 into 4.x-dev May 11, 2021
@tsteur tsteur deleted the 17488-remove-visitorid-segment branch May 11, 2021 20:54
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.

Remove the "Visitor ID" segment when the "Visitor profile" feature has been disabled
2 participants