@flamisz opened this Pull Request on April 29th 2021 Contributor

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
@tsteur commented on May 3rd 2021 Member

@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 commented on May 3rd 2021 Contributor

@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 commented on May 3rd 2021 Contributor

@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 commented on May 3rd 2021 Member

@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 commented on May 3rd 2021 Contributor

@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 commented on May 4th 2021 Contributor

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.

@tsteur commented on May 4th 2021 Member

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 :)

This Pull Request was closed on May 11th 2021
Powered by GitHub Issue Mirror