Support pivot w/ CustomDimensions #13429

diosmosis · 2018-09-14T03:06:27Z

More than a few changes to get it to work:

Use segment metadata when available to support complex segments like the one for UserCountry.City dimension.
Pass the pivot by dimension report's parameters when fetching so it will support pivoting by parameterized dimensions (like dimension for specific custom dimension or goal).
Allow Report to be passed by instance to PivotByDimension (not required but think it's helpful).
In Report:: getForDimension() use ReportsProvider.
Small tweak to exception message in PivotByDimension.

tsteur · 2018-09-16T21:34:05Z

FYI because I saw custom dimension changes in another PR... changing the ID of a custom dimension may break all existing custom reports that use such a dimension or a metric https://github.com/matomo-org/plugin-CustomDimensions/compare/pivot-by

… Report instance in PivotByDimension filter.

diosmosis · 2018-09-18T21:11:37Z

Ready for review.

tsteur · 2018-09-19T00:39:07Z

core/Plugin/ReportsProvider.php

+        $matchParamCount = -1;
+
+        foreach ($reports as $report) {
+            $isMatchedWithThisReport = self::isReportParamsMatchingCurrentParams($report, $requestParams);


From what I can see this may cause problems with various plugins that are based on entities and don't create a report instance for each entity, such as funnels. By default, eg GetFunnelFlow doesn't set parameters, but $report->parameters may be set when configureReportMetadata is called and I can imagine depending on the flow how methods are called that the logic might break some things.

Same is happening in GetDaysToConversion but they set $this->parameters= array() back in the init method which some other report classes don't do just yet. So there might be an easy fix for this by updating all our plugins...

Is this change needed for this PR?

It was needed at one point w/ some other changes, but looks like it's not needed currently. Will remove it.

tsteur · 2018-09-19T00:49:14Z

core/API/DataTablePostProcessor.php

            $pivotByColumn = Common::getRequestVar('pivotByColumn', false, 'string', $this->request);
            $pivotByColumnLimit = Common::getRequestVar('pivotByColumnLimit', false, 'int', $this->request);

+            $dataTable->filter('PivotByDimension', array($this->report, $pivotBy, $pivotByColumn, $pivotByColumnLimit,
+                PivotByDimension::isSegmentFetchingEnabledInConfig()));
            $dataTable->filter('ColumnCallbackDeleteMetadata', array('segmentValue'));


This one is already executed by convertSegmentValueToSegment ()

tsteur · 2018-09-19T00:49:48Z

core/API/DataTablePostProcessor.php

            $pivotByColumn = Common::getRequestVar('pivotByColumn', false, 'string', $this->request);
            $pivotByColumnLimit = Common::getRequestVar('pivotByColumnLimit', false, 'int', $this->request);

+            $dataTable->filter('PivotByDimension', array($this->report, $pivotBy, $pivotByColumn, $pivotByColumnLimit,
+                PivotByDimension::isSegmentFetchingEnabledInConfig()));
            $dataTable->filter('ColumnCallbackDeleteMetadata', array('segmentValue'));
            $dataTable->filter('ColumnCallbackDeleteMetadata', array('segment'));


Is there a point in calling convertSegmentValueToSegment() when we remove the segment afterwards?

Yes, the segment is used by PivotByDimension (useful for supporting UserCountry.City for example, whose segment involves country & region),.

tsteur · 2018-09-19T00:51:18Z

core/DataTable/Filter/PivotByDimension.php

@@ -267,8 +269,12 @@ private function getIntersectedTable(DataTable $table, Row $row)
        }

        if ($this->isFetchingBySegmentEnabled) {
-            $segmentValue = $row->getColumn('label');
-            return $this->fetchIntersectedWithThisBySegment($table, $segmentValue);
+            $segment = $row->getMetadata('segment');


does this fix a different bug? FYI: The segment metadata might not always be set for all reports etc...

I see in the description mentions something around fixing segments. Not sure what the problem is and if this fixes it.

Yes, saw it while I was working. There was already a skipped test for it. This is why we call the filter above.

I see... do we still need the fallback for $row->getColumn('label') as many reports won't set a segment metadata? or a dimension with a segment etc?

Also I think there were some places were the segment string was not 100% correct because it wasn't possible to do it any better but I don't remember it where...

Can't quite remember, but I think the label fallback is still there?

I think I misread your question, but the label fallback is still there (it's in a ?: expression), in case the segmentValue isn't there. I'll clear up the code since that wasn't obvious.

tsteur · 2018-09-19T01:23:17Z

core/DataTable/Filter/PivotByDimension.php

+            $segment = $row->getMetadata('segment');
+            if (empty($segment)) {
+                $segmentValue = $row->getMetadata('segmentValue') ?: $row->getColumn('label');
+                $segment = $this->thisReportDimensionSegment->getSegment() . "==" . urlencode($segmentValue);


Can we be certain a segment is returned here? $this->thisReportDimensionSegment->getSegment()?

It wouldn't be a valid Segment instance if it wasn't (I think), but it makes sense to avoid a fatal.

tsteur · 2018-09-19T01:25:22Z

Looks pretty much fine, mostly concerned with findReportMatchingParameters and making sure to not cause any problems with reports that cannot be found anymore. Eg around funnels, etc. As mentioned in the comment, not sure if that is needed?

diosmosis · 2018-09-19T02:42:04Z

Updated.

tsteur · 2018-09-19T03:51:59Z

Just tested it again and #13317 is fixed for me. 👍 (tested it with https://github.com/matomo-org/plugin-CustomDimensions/pull/100/files)
Before merging maybe also merge matomo-org/plugin-CustomDimensions#100 and then update the submodule and fix the tests?

* Supporting CustomDimensions in pivotBy. * Use segment metadata in pivotby to support more dimensions. * try to fix some tests * Update test. * In ReportsProvider::factory() match reports by parameters too + use a Report instance in PivotByDimension filter. * Fixing tests. * Fix a couple issues * Revert ReportsProvider changes. * Add git lfs pull to .travis.yml * Apply PR fixes. * Revert .travis.yml. * Update CustomDimensions submodule. * Revert tests/travis change.

diosmosis added 4 commits September 12, 2018 22:21

Supporting CustomDimensions in pivotBy.

15b861d

Use segment metadata in pivotby to support more dimensions.

a4dbded

try to fix some tests

57cbe29

Update test.

a7da7db

diosmosis added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Sep 14, 2018

diosmosis added this to the 3.6.1 milestone Sep 14, 2018

diosmosis added 3 commits September 16, 2018 18:11

In ReportsProvider::factory() match reports by parameters too + use a…

e8474e6

… Report instance in PivotByDimension filter.

Fixing tests.

1ecb2ae

Fix a couple issues

f7a90be

tsteur reviewed Sep 19, 2018

View reviewed changes

diosmosis added 3 commits September 18, 2018 18:58

Revert ReportsProvider changes.

ef7ed8c

Add git lfs pull to .travis.yml

9dfbe84

Apply PR fixes.

23c0115

diosmosis added 3 commits September 19, 2018 14:19

Revert .travis.yml.

c529bc9

Update CustomDimensions submodule.

2112b8e

Revert tests/travis change.

80ab944

diosmosis merged commit 40d68b9 into 3.x-dev Sep 20, 2018

diosmosis deleted the 13317-custom-dim-pivot-by-factory branch September 20, 2018 05:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support pivot w/ CustomDimensions #13429

Support pivot w/ CustomDimensions #13429

diosmosis commented Sep 14, 2018 •

edited

tsteur commented Sep 16, 2018

diosmosis commented Sep 18, 2018

tsteur Sep 19, 2018

diosmosis Sep 19, 2018

tsteur Sep 19, 2018

tsteur Sep 19, 2018

diosmosis Sep 19, 2018

tsteur Sep 19, 2018

tsteur Sep 19, 2018

diosmosis Sep 19, 2018

tsteur Sep 19, 2018

tsteur Sep 19, 2018

diosmosis Sep 19, 2018

diosmosis Sep 19, 2018

tsteur Sep 19, 2018

diosmosis Sep 19, 2018

tsteur commented Sep 19, 2018

diosmosis commented Sep 19, 2018

tsteur commented Sep 19, 2018

Support pivot w/ CustomDimensions #13429

Support pivot w/ CustomDimensions #13429

Conversation

diosmosis commented Sep 14, 2018 • edited

tsteur commented Sep 16, 2018

diosmosis commented Sep 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsteur commented Sep 19, 2018

diosmosis commented Sep 19, 2018

tsteur commented Sep 19, 2018

diosmosis commented Sep 14, 2018 •

edited