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

Keep flattened columns as extra columns #12199

Open
wants to merge 9 commits into
base: 3.x-dev
from

Conversation

Projects
None yet
4 participants
@sgiehl
Member

sgiehl commented Oct 16, 2017

Flattened columns will be added with their dimension id. If dimension id is the same for subtables (like in pages reports) the values will be concatenated.
If a table has no dimension a numerated label will be used instead. e.g. label1

fixes #12163

@mattab mattab added this to the 3.2.1 milestone Oct 16, 2017

@mattab
  • For the processed report, we may need to add a new system test in case they are not tested yet
@sgiehl

This comment has been minimized.

Show comment
Hide comment
@sgiehl

sgiehl Oct 17, 2017

Member

@mattab how should processed reports be handled?
Currently those additional columns aren't included as they are not defined in the reports metadata.
Tweaking the reports metadata by default is no option, as those columns aren't included in non-flat reports. So we would need to tweak the report results when the report is being processed, but I'm currently a bit lost how to do that proper.
Flat processed reports already have an incorrect label, as the dimension defined in metadata is used. Instead the label is combined with the one of the subtables. For example https://demo.piwik.org/index.php?module=API&method=API.getProcessedReport&apiModule=Referrers&apiAction=getSearchEngines&format=XML&idSite=3&period=month&date=2017-10-14&flat=1&filter_limit=25 uses Search Engine as label. Instead it would need to be Search Engine - Keyword. But actually there is no information that the keyword was merged in the report.

Member

sgiehl commented Oct 17, 2017

@mattab how should processed reports be handled?
Currently those additional columns aren't included as they are not defined in the reports metadata.
Tweaking the reports metadata by default is no option, as those columns aren't included in non-flat reports. So we would need to tweak the report results when the report is being processed, but I'm currently a bit lost how to do that proper.
Flat processed reports already have an incorrect label, as the dimension defined in metadata is used. Instead the label is combined with the one of the subtables. For example https://demo.piwik.org/index.php?module=API&method=API.getProcessedReport&apiModule=Referrers&apiAction=getSearchEngines&format=XML&idSite=3&period=month&date=2017-10-14&flat=1&filter_limit=25 uses Search Engine as label. Instead it would need to be Search Engine - Keyword. But actually there is no information that the keyword was merged in the report.

@sgiehl

This comment has been minimized.

Show comment
Hide comment
@sgiehl

sgiehl Oct 25, 2017

Member

@mattab @tsteur anyone able to give me a smart hint how to proceed?

Member

sgiehl commented Oct 25, 2017

@mattab @tsteur anyone able to give me a smart hint how to proceed?

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Oct 25, 2017

Member

True, we would need to make sure to manipulate the label as well when flatten is used. Might need to post an event like API.getProcessedReport.end in ProcessedReport::getProcessedReport or handle it directly in there (would need to forward flatten parameter). In Report::buildReportMetadata() we cannot really do it as we don't know there whether a report is flattened or not (and we cannot rely on $_GET/$_POST).

I totally see what you mean...

I wonder if we can in Report::buildReportMetadata() always return all labels of all possible dimensions like

        $report['dimensions']           = [$this->getDimension()->getId() => $this->getDimension()->getName(), $this->getSubtableDimension()->getId() => $this->getSubtableDimension()->getName()]; // note subtable dimensions might be not defined, and some reports have multiple dimensions so we would need a method like `$this->getDimensions()` or so.
        $report['metrics']              = $this->getMetrics();
        $report['metricsDocumentation'] = $this->getMetricsDocumentation();
        $report['processedMetrics']     = $this->getProcessedMetrics();

So we basically add this always to the report metadata output. And maybe then in ProcessedReport::getProcessedReport() we can do like an implode(' - ', $report['dimensions']) when flatten is used for the label? The -might be needed to be adjusted as a different string can be configured in the report class or so.

We would also need to add the information to columns => $columns in the processed report class.

I just checked Piwik Mobile roughly and it will likely not break it, but needs to be double checked. Actually, I just checked again and there is quite a chance it might break Piwik Mobile as it will think that any column specified in columns is a metric.

Likely this issue can break various implementations because. Adding something to columns that is not a metric can lead to breaking behaviour, and adding new row columns that are not metrics can break behaviour as well as so far it is assumed that numbers are returned in all fields except label which will be no longer the case. Wondering if better to do in Piwik 4?

Member

tsteur commented Oct 25, 2017

True, we would need to make sure to manipulate the label as well when flatten is used. Might need to post an event like API.getProcessedReport.end in ProcessedReport::getProcessedReport or handle it directly in there (would need to forward flatten parameter). In Report::buildReportMetadata() we cannot really do it as we don't know there whether a report is flattened or not (and we cannot rely on $_GET/$_POST).

I totally see what you mean...

I wonder if we can in Report::buildReportMetadata() always return all labels of all possible dimensions like

        $report['dimensions']           = [$this->getDimension()->getId() => $this->getDimension()->getName(), $this->getSubtableDimension()->getId() => $this->getSubtableDimension()->getName()]; // note subtable dimensions might be not defined, and some reports have multiple dimensions so we would need a method like `$this->getDimensions()` or so.
        $report['metrics']              = $this->getMetrics();
        $report['metricsDocumentation'] = $this->getMetricsDocumentation();
        $report['processedMetrics']     = $this->getProcessedMetrics();

So we basically add this always to the report metadata output. And maybe then in ProcessedReport::getProcessedReport() we can do like an implode(' - ', $report['dimensions']) when flatten is used for the label? The -might be needed to be adjusted as a different string can be configured in the report class or so.

We would also need to add the information to columns => $columns in the processed report class.

I just checked Piwik Mobile roughly and it will likely not break it, but needs to be double checked. Actually, I just checked again and there is quite a chance it might break Piwik Mobile as it will think that any column specified in columns is a metric.

Likely this issue can break various implementations because. Adding something to columns that is not a metric can lead to breaking behaviour, and adding new row columns that are not metrics can break behaviour as well as so far it is assumed that numbers are returned in all fields except label which will be no longer the case. Wondering if better to do in Piwik 4?

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Oct 26, 2017

Member

It would be good to keep BC.

An idea:

  • maybe the new columns can be added in a new <columnsLabel> eg.
<columnsLabel>
  <browser>Browser name</browser>
  <pageTitle>Page Title</pageTitle>
  <dimension3>My Custom Dimension</dimension3>
</columnsLabel>
  • and the value for these columnsLabel for each row could be added in the <reportMetadata> eg.
<reportMetadata>
<row>
  <browser>Firefox</browser>
  <pageTitle>Homepage</pageTitle>
  <dimension3>valueXYZ</dimension3>
  <url>http://piwik.org/faq/general/#faq_144</url>
  <logo>plugins/Morpheus/icons/dist/searchEngines/google.com.png</logo>
</row>
<row>
  <browser>Chome</browser>
  <pageTitle>Homepage</pageTitle>
  <dimension3>hello world</dimension3>
  <url>http://piwik.org/faq/general/#faq_144</url>
  <logo>plugins/Morpheus/icons/dist/searchEngines/bing.com.png</logo>
</row>
</reportMetadata>
Member

mattab commented Oct 26, 2017

It would be good to keep BC.

An idea:

  • maybe the new columns can be added in a new <columnsLabel> eg.
<columnsLabel>
  <browser>Browser name</browser>
  <pageTitle>Page Title</pageTitle>
  <dimension3>My Custom Dimension</dimension3>
</columnsLabel>
  • and the value for these columnsLabel for each row could be added in the <reportMetadata> eg.
<reportMetadata>
<row>
  <browser>Firefox</browser>
  <pageTitle>Homepage</pageTitle>
  <dimension3>valueXYZ</dimension3>
  <url>http://piwik.org/faq/general/#faq_144</url>
  <logo>plugins/Morpheus/icons/dist/searchEngines/google.com.png</logo>
</row>
<row>
  <browser>Chome</browser>
  <pageTitle>Homepage</pageTitle>
  <dimension3>hello world</dimension3>
  <url>http://piwik.org/faq/general/#faq_144</url>
  <logo>plugins/Morpheus/icons/dist/searchEngines/bing.com.png</logo>
</row>
</reportMetadata>
@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Oct 26, 2017

Member

Wondering if better to do in Piwik 4?

Would be great to do it before in Processed Report output 👍

Member

mattab commented Oct 26, 2017

Wondering if better to do in Piwik 4?

Would be great to do it before in Processed Report output 👍

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Oct 27, 2017

Member

In report metadata could work to not break API. In Piwik 4 we would then maybe actually put it into the row element (or maybe not, to be discussed).

Member

tsteur commented Oct 27, 2017

In report metadata could work to not break API. In Piwik 4 we would then maybe actually put it into the row element (or maybe not, to be discussed).

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Nov 19, 2017

Member

@sgiehl Any update on this one? it is an important one for Custom Reports users

Member

mattab commented Nov 19, 2017

@sgiehl Any update on this one? it is an important one for Custom Reports users

@mattab mattab modified the milestones: 3.2.1, 3.2.2 Nov 19, 2017

@sgiehl

This comment has been minimized.

Show comment
Hide comment
@sgiehl

sgiehl Nov 24, 2017

Member

I've updated the PR.
Everything is now added to the metadata.
If the report has more than one dimension a new metadata entry dimensions will be added, listing all dimensions. (Are report is considered as having more dimension if getDimensions returns an array with more than one entries, by default the array contains the reports dimension and if available the dimension of the subtable)
For flattened reports those dimensions will be additionally added to each rows metadata. This also applies for processed reports as row metadata is taken from all sub reports.

Member

sgiehl commented Nov 24, 2017

I've updated the PR.
Everything is now added to the metadata.
If the report has more than one dimension a new metadata entry dimensions will be added, listing all dimensions. (Are report is considered as having more dimension if getDimensions returns an array with more than one entries, by default the array contains the reports dimension and if available the dimension of the subtable)
For flattened reports those dimensions will be additionally added to each rows metadata. This also applies for processed reports as row metadata is taken from all sub reports.

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Dec 4, 2017

Member

Looks good to me and should be safe and keep BC 👍

Feedback:

  • for the dimension ID in the API output, could we use the Segment string eg. <downloadUrl>... instead of <Actions_DownloadUrl>..., eg. <dimension3> instead of <CustomDimensions_dimension3>
    • (to indicate the column is part of the label/main dimension, we could maybe even name label_downloadUrl or label_dimension3? not too sure if it's better)
  • as expected there are some tests failing in CustomDimensions and MarketingCampaignReporting plugins
Member

mattab commented Dec 4, 2017

Looks good to me and should be safe and keep BC 👍

Feedback:

  • for the dimension ID in the API output, could we use the Segment string eg. <downloadUrl>... instead of <Actions_DownloadUrl>..., eg. <dimension3> instead of <CustomDimensions_dimension3>
    • (to indicate the column is part of the label/main dimension, we could maybe even name label_downloadUrl or label_dimension3? not too sure if it's better)
  • as expected there are some tests failing in CustomDimensions and MarketingCampaignReporting plugins
@sgiehl

This comment has been minimized.

Show comment
Hide comment
@sgiehl

sgiehl Dec 4, 2017

Member

for the dimension ID in the API output, could we use the Segment string eg. ... instead of <Actions_DownloadUrl>..., eg. instead of <CustomDimensions_dimension3>
(to indicate the column is part of the label/main dimension, we could maybe even name label_downloadUrl or label_dimension3? not too sure if it's better)

Using the segment name can only work if a segment is defined. As this is a generic solution there might be plugins where no segment is defined for a subtable. In this case we would need to have a fallback

Member

sgiehl commented Dec 4, 2017

for the dimension ID in the API output, could we use the Segment string eg. ... instead of <Actions_DownloadUrl>..., eg. instead of <CustomDimensions_dimension3>
(to indicate the column is part of the label/main dimension, we could maybe even name label_downloadUrl or label_dimension3? not too sure if it's better)

Using the segment name can only work if a segment is defined. As this is a generic solution there might be plugins where no segment is defined for a subtable. In this case we would need to have a fallback

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Dec 4, 2017

Member

Good point. When there is no segment defined, maybe we could fallback to $Plugin_$Dimension? Btw, was this 'dimension key' already used somewhere else in Piwik?

Member

mattab commented Dec 4, 2017

Good point. When there is no segment defined, maybe we could fallback to $Plugin_$Dimension? Btw, was this 'dimension key' already used somewhere else in Piwik?

@sgiehl sgiehl added Needs Review and removed Pull Request WIP labels Dec 5, 2017

@sgiehl

This comment has been minimized.

Show comment
Hide comment
@sgiehl

sgiehl Dec 10, 2017

Member

It currently uses the dimension id, so probably something that might be used somewhere else in the future. So I wouldn't rename it to anything else. Also this label name needs to be used to add the translations...

Member

sgiehl commented Dec 10, 2017

It currently uses the dimension id, so probably something that might be used somewhere else in the future. So I wouldn't rename it to anything else. Also this label name needs to be used to add the translations...

$report = ReportsProvider::factory($this->apiModule, $this->getApiMethodForSubtable($this->request));
$subDimension = $report->getDimension();
$subDimensionName = $subDimension ? str_replace('.', '_', $subDimension->getId()) : 'label' . (substr_count($prefix, $this->recursiveLabelSeparator) + 1);

This comment has been minimized.

@diosmosis

diosmosis Jun 18, 2018

Member

Is it possible to have a test for when a report has no dimension or subdimension? To trigger the labelN logic.

@diosmosis

diosmosis Jun 18, 2018

Member

Is it possible to have a test for when a report has no dimension or subdimension? To trigger the labelN logic.

<Events_EventAction>Search</Events_EventAction>
<Events_EventName>Search query here</Events_EventName>
</row>
</result>

This comment has been minimized.

@diosmosis

diosmosis Jul 24, 2018

Member

I see now there will be rows w/ dimensions even if there is no data. Will this be a BC break? I wonder if the mobile app or other types of apps will experience different behavior because of this.

@diosmosis

diosmosis Jul 24, 2018

Member

I see now there will be rows w/ dimensions even if there is no data. Will this be a BC break? I wonder if the mobile app or other types of apps will experience different behavior because of this.

This comment has been minimized.

@diosmosis

diosmosis Jul 24, 2018

Member

cc @tsteur in case he has some insight

@diosmosis

diosmosis Jul 24, 2018

Member

cc @tsteur in case he has some insight

@diosmosis

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Jul 24, 2018

Member

Tested locally & looks good, noticed one possible BC issue (commented).

Also, I think the tests don't cover processed reports when there is data and they don't cover the labelN logic. I think when those are added would be good to merge.

Member

diosmosis commented Jul 24, 2018

Tested locally & looks good, noticed one possible BC issue (commented).

Also, I think the tests don't cover processed reports when there is data and they don't cover the labelN logic. I think when those are added would be good to merge.

@diosmosis

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Jul 25, 2018

Member

@sgiehl did some more testing & noticed this doesn't seem to work w/ CustomReports:

image

that's w/ these dimensions:

image

Member

diosmosis commented Jul 25, 2018

@sgiehl did some more testing & noticed this doesn't seem to work w/ CustomReports:

image

that's w/ these dimensions:

image

@mattab

@sgiehl can you please investigate Benaka's feedback

@mattab mattab modified the milestones: 3.6.0, 3.7.0 Aug 6, 2018

@sgiehl

This comment has been minimized.

Show comment
Hide comment
@sgiehl

sgiehl Aug 6, 2018

Member

test this is working for custom reports (which is where having multiple dimensions as columns will be awesome)

I've investigated the custom reports plugin together with this branch now for a while and figured out where the problem is. The Report classes for CustomReports are created dynamically the according dimension is detected by checking if it's a subtable report or not (check for the get parameter idSubtable). As this parameter isn't set when flattening the report (here) it always returns the dimension of the main report, which causes the flattening to simply concatenate the value instead of adding additional dimension entries. We could temporarily set the get parameter at that point or extend the ReportsProvider::factory to receive an additional subTableId or similar.

Member

sgiehl commented Aug 6, 2018

test this is working for custom reports (which is where having multiple dimensions as columns will be awesome)

I've investigated the custom reports plugin together with this branch now for a while and figured out where the problem is. The Report classes for CustomReports are created dynamically the according dimension is detected by checking if it's a subtable report or not (check for the get parameter idSubtable). As this parameter isn't set when flattening the report (here) it always returns the dimension of the main report, which causes the flattening to simply concatenate the value instead of adding additional dimension entries. We could temporarily set the get parameter at that point or extend the ReportsProvider::factory to receive an additional subTableId or similar.

@sgiehl

This comment has been minimized.

Show comment
Hide comment
@sgiehl

sgiehl Aug 8, 2018

Member

@diosmosis @mattab

tests don't cover processed reports when there is data and they don't cover the labelN logic. can you please add those?

Adjusted a test to cover that.

test this is working for custom reports (which is where having multiple dimensions as columns will be awesome)

With latest changes it works with Custom Reports, but only with reports having 2 Dimensions only. For supporting 3 dimensions we may need to adjust a bit more. See my comment before.

is it expected that this test output changes? Will this affect BC? https://github.com/matomo-org/matomo/pull/12199/files/fe2cb5a7f1b1ed404f86c391247982c3f18e99e9#diff-5c16d2fae8e978c9508dc0f4e4368a1b

The Link doesn't work anymore, but guess it's related to the added metadata. That is expected behaviour.

some other system and tests are still failing?

Remaining failing tests should be for CustomDimensions and MarketingCampaignsReporting plugin only and need to be adjusted when ready to merge...

Member

sgiehl commented Aug 8, 2018

@diosmosis @mattab

tests don't cover processed reports when there is data and they don't cover the labelN logic. can you please add those?

Adjusted a test to cover that.

test this is working for custom reports (which is where having multiple dimensions as columns will be awesome)

With latest changes it works with Custom Reports, but only with reports having 2 Dimensions only. For supporting 3 dimensions we may need to adjust a bit more. See my comment before.

is it expected that this test output changes? Will this affect BC? https://github.com/matomo-org/matomo/pull/12199/files/fe2cb5a7f1b1ed404f86c391247982c3f18e99e9#diff-5c16d2fae8e978c9508dc0f4e4368a1b

The Link doesn't work anymore, but guess it's related to the added metadata. That is expected behaviour.

some other system and tests are still failing?

Remaining failing tests should be for CustomDimensions and MarketingCampaignsReporting plugin only and need to be adjusted when ready to merge...

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Aug 9, 2018

Member

It is important to support custom reports with 3 dimensions and also have system tests for this. I'm not sure which solution is best though among the two you suggest in your comment..

Member

mattab commented Aug 9, 2018

It is important to support custom reports with 3 dimensions and also have system tests for this. I'm not sure which solution is best though among the two you suggest in your comment..

@sgiehl

This comment has been minimized.

Show comment
Hide comment
@sgiehl

sgiehl Aug 9, 2018

Member

@mattab After doing some more testing I guess both methods won't work for 3 Dimensions. To be honest I'm a bit stuck in finding a proper solution how to handle the dynamically added reports by CustomReports plugin without having to refactor the whole Report stuff in Matomo. Everything else that comes up to my mind would be bit too hackish...

Member

sgiehl commented Aug 9, 2018

@mattab After doing some more testing I guess both methods won't work for 3 Dimensions. To be honest I'm a bit stuck in finding a proper solution how to handle the dynamically added reports by CustomReports plugin without having to refactor the whole Report stuff in Matomo. Everything else that comes up to my mind would be bit too hackish...

@mattab mattab modified the milestones: 3.7.0, 3.6.1 Sep 1, 2018

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Oct 2, 2018

Member

Was the report class problem maybe fixed recently? I think the cache was made per idSite etc. Not sure if that helps?

Member

tsteur commented Oct 2, 2018

Was the report class problem maybe fixed recently? I think the cache was made per idSite etc. Not sure if that helps?

@sgiehl

This comment has been minimized.

Show comment
Hide comment
@sgiehl

sgiehl Oct 3, 2018

Member

@tsteur no the problem is, that there is no simple way to support reports of CustomDimensions plugin that have 3 dimensions. The report flattener fails to get the correct dimension for the subtables as the ReportFactory returns the main report.

Member

sgiehl commented Oct 3, 2018

@tsteur no the problem is, that there is no simple way to support reports of CustomDimensions plugin that have 3 dimensions. The report flattener fails to get the correct dimension for the subtables as the ReportFactory returns the main report.

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Oct 4, 2018

Member

@sgiehl I think I've managed to add the third dimension but not sure if there is any other problem? I noticed the first dimension wasn't really shown... not sure why... we could make the methods getThirdLeveltableDimension and even getSecondLeveltableDimension() API in Report class maybe as it is more clear than just getSubtableDimension()

diff --git a/core/API/DataTableManipulator/Flattener.php b/core/API/DataTableManipulator/Flattener.php
index e1bb7023ad..342480d054 100644
--- a/core/API/DataTableManipulator/Flattener.php
+++ b/core/API/DataTableManipulator/Flattener.php
@@ -75,7 +75,7 @@ class Flattener extends DataTableManipulator
 
         $dimensionName = !empty($dimension) ? str_replace('.', '_', $dimension->getId()) : 'label1';
 
-        $this->flattenDataTableInto($dataTable, $newDataTable, $dimensionName);
+        $this->flattenDataTableInto($dataTable, $newDataTable, $level = 1, $dimensionName);
 
         return $newDataTable;
     }
@@ -85,10 +85,10 @@ class Flattener extends DataTableManipulator
      * @param $newDataTable
      * @param $dimensionName
      */
-    protected function flattenDataTableInto($dataTable, $newDataTable, $dimensionName, $prefix = '', $logo = false)
+    protected function flattenDataTableInto($dataTable, $newDataTable, $level, $dimensionName, $prefix = '', $logo = false)
     {
         foreach ($dataTable->getRows() as $rowId => $row) {
-            $this->flattenRow($row, $rowId, $newDataTable, $dimensionName, $prefix, $logo);
+            $this->flattenRow($row, $rowId, $newDataTable, $level, $dimensionName, $prefix, $logo);
         }
     }
 
@@ -99,7 +99,8 @@ class Flattener extends DataTableManipulator
      * @param string $dimensionName
      * @param bool $parentLogo
      */
-    private function flattenRow(Row $row, $rowId, DataTable $dataTable, $dimensionName,
+    private function flattenRow
+    (Row $row, $rowId, DataTable $dataTable, $level, $dimensionName,
                                 $labelPrefix = '', $parentLogo = false)
     {
         $origLabel = $label = $row->getColumn('label');
@@ -162,6 +163,10 @@ class Flattener extends DataTableManipulator
                 $subDimension = $report->getSubtableDimension();
             }
 
+            if ($level === 2 && method_exists($report, 'getThirdLeveltableDimension')) {
+                $subDimension = $report->getThirdLeveltableDimension();
+            }
+
             if (empty($subDimension)) {
                 $report           = ReportsProvider::factory($this->apiModule, $this->getApiMethodForSubtable($this->request));
                 $subDimension     = $report->getDimension();
@@ -175,7 +180,7 @@ class Flattener extends DataTableManipulator
                 }
             }
 
-            $this->flattenDataTableInto($subTable, $dataTable, $subDimensionName, $prefix, $logo);
+            $this->flattenDataTableInto($subTable, $dataTable, $level + 1, $subDimensionName, $prefix, $logo);
         }
     }
Member

tsteur commented Oct 4, 2018

@sgiehl I think I've managed to add the third dimension but not sure if there is any other problem? I noticed the first dimension wasn't really shown... not sure why... we could make the methods getThirdLeveltableDimension and even getSecondLeveltableDimension() API in Report class maybe as it is more clear than just getSubtableDimension()

diff --git a/core/API/DataTableManipulator/Flattener.php b/core/API/DataTableManipulator/Flattener.php
index e1bb7023ad..342480d054 100644
--- a/core/API/DataTableManipulator/Flattener.php
+++ b/core/API/DataTableManipulator/Flattener.php
@@ -75,7 +75,7 @@ class Flattener extends DataTableManipulator
 
         $dimensionName = !empty($dimension) ? str_replace('.', '_', $dimension->getId()) : 'label1';
 
-        $this->flattenDataTableInto($dataTable, $newDataTable, $dimensionName);
+        $this->flattenDataTableInto($dataTable, $newDataTable, $level = 1, $dimensionName);
 
         return $newDataTable;
     }
@@ -85,10 +85,10 @@ class Flattener extends DataTableManipulator
      * @param $newDataTable
      * @param $dimensionName
      */
-    protected function flattenDataTableInto($dataTable, $newDataTable, $dimensionName, $prefix = '', $logo = false)
+    protected function flattenDataTableInto($dataTable, $newDataTable, $level, $dimensionName, $prefix = '', $logo = false)
     {
         foreach ($dataTable->getRows() as $rowId => $row) {
-            $this->flattenRow($row, $rowId, $newDataTable, $dimensionName, $prefix, $logo);
+            $this->flattenRow($row, $rowId, $newDataTable, $level, $dimensionName, $prefix, $logo);
         }
     }
 
@@ -99,7 +99,8 @@ class Flattener extends DataTableManipulator
      * @param string $dimensionName
      * @param bool $parentLogo
      */
-    private function flattenRow(Row $row, $rowId, DataTable $dataTable, $dimensionName,
+    private function flattenRow
+    (Row $row, $rowId, DataTable $dataTable, $level, $dimensionName,
                                 $labelPrefix = '', $parentLogo = false)
     {
         $origLabel = $label = $row->getColumn('label');
@@ -162,6 +163,10 @@ class Flattener extends DataTableManipulator
                 $subDimension = $report->getSubtableDimension();
             }
 
+            if ($level === 2 && method_exists($report, 'getThirdLeveltableDimension')) {
+                $subDimension = $report->getThirdLeveltableDimension();
+            }
+
             if (empty($subDimension)) {
                 $report           = ReportsProvider::factory($this->apiModule, $this->getApiMethodForSubtable($this->request));
                 $subDimension     = $report->getDimension();
@@ -175,7 +180,7 @@ class Flattener extends DataTableManipulator
                 }
             }
 
-            $this->flattenDataTableInto($subTable, $dataTable, $subDimensionName, $prefix, $logo);
+            $this->flattenDataTableInto($subTable, $dataTable, $level + 1, $subDimensionName, $prefix, $logo);
         }
     }
@sgiehl

This comment has been minimized.

Show comment
Hide comment
@sgiehl

sgiehl Oct 4, 2018

Member

@tsteur thanks, that helped. Needed to adjust some more stuff so all dimensions are added correctly. But now it looks better.
What about adding the new methods as API? Do you think it makes sense, or is the usage of custom reports plugin too "special"?

Member

sgiehl commented Oct 4, 2018

@tsteur thanks, that helped. Needed to adjust some more stuff so all dimensions are added correctly. But now it looks better.
What about adding the new methods as API? Do you think it makes sense, or is the usage of custom reports plugin too "special"?

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Oct 4, 2018

Member

I think it would make sense to add those methods to Report class as API especially since we might also add reports to core with 3 dimensions maybe.

Member

tsteur commented Oct 4, 2018

I think it would make sense to add those methods to Report class as API especially since we might also add reports to core with 3 dimensions maybe.

@mattab mattab modified the milestones: 3.6.1, 3.7.0 Oct 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment