@sgiehl opened this Pull Request on October 16th 2017 Member

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

@sgiehl commented on October 17th 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.

@sgiehl commented on October 25th 2017 Member

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

@tsteur commented on October 25th 2017 Owner

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 commented on October 26th 2017 Owner

It would be good to keep BC.

An idea:

  • maybe the new columns can be added in a new <columnsLabel> eg.

    <browser>Browser name</browser>
    <pageTitle>Page Title</pageTitle>
    <dimension3>My Custom Dimension</dimension3>
  • and the value for these columnsLabel for each row could be added in the <reportMetadata> eg.
    <dimension3>hello world</dimension3>
@mattab commented on October 26th 2017 Owner

Wondering if better to do in Piwik 4?

Would be great to do it before in Processed Report output :+1:

@tsteur commented on October 27th 2017 Owner

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 commented on November 19th 2017 Owner

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

@sgiehl commented on November 24th 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.

@mattab commented on December 4th 2017 Owner

Looks good to me and should be safe and keep BC :+1:


  • 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 commented on December 4th 2017 Member

for the dimension ID in the API output, could we use the Segment string eg. ... instead of ..., eg. instead of
(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 commented on December 4th 2017 Owner

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 commented on December 10th 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...

Powered by GitHub Issue Mirror