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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- For the processed report, we may need to add a new system test in case they are not tested yet
@@ -34,6 +38,8 @@ | |||
<sum_visit_length>1</sum_visit_length> | |||
<bounce_count>0</bounce_count> | |||
<nb_visits_converted>0</nb_visits_converted> | |||
<Referrers.Website>www.referrer2.com</Referrers.Website> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invalid XML
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually it's not invalid as .
is valid for xml tags as long as it doesn't begin with .
. But I'll replace that with _
412f707
to
d5e877e
Compare
@mattab how should processed reports be handled? |
True, we would need to make sure to manipulate the I totally see what you mean... I wonder if we can in $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 We would also need to add the information to 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 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 |
It would be good to keep BC. An idea:
<columnsLabel>
<browser>Browser name</browser>
<pageTitle>Page Title</pageTitle>
<dimension3>My Custom Dimension</dimension3>
</columnsLabel>
<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> |
Would be great to do it before in Processed Report output 👍 |
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). |
@sgiehl Any update on this one? it is an important one for Custom Reports users |
ccc1cf6
to
cf8e567
Compare
I've updated the PR. |
Looks good to me and should be safe and keep BC 👍 Feedback:
|
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 |
Good point. When there is no segment defined, maybe we could fallback to |
8e84778
to
18dc3b8
Compare
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... |
18dc3b8
to
6a4ac0a
Compare
…ested (#13796) Could fix an edge case where user is logged in, but hasn't confirmed the auth code (so the user is not actually logged in), and then an update appears.
* added fallback method for Alexa, fixes issue #13427 * do not use short array syntax for consistency with other methods * use mini link for Alexa, use DomXPath to filter out the global ranking instead of regex
* use db sessions by default, deprecate file session handler * trying to fix tests
…not printed twice (#13801)
* update submodule * Update screenshots and try to get test to pass.
fixes #13272 Haven't actually tested it but should fix the issue. If tests pass, the logic would be still the same. I don't have a PHP 7.2 running here otherwise at the moment
* send bulk requests in chunks * send requests correctly
It won't remember any hash as the hash won't be visible in the referrer etc but it would work for most other pages. To make it work for hash it would get likely way more complicated like we would need to persist it through JS, temporarily store it somewhere and redirect accordingly. It fixes the case mentioned in the issue. fix #13328
* Add option to opt in to use send beacon * Fix JS tracker test.
slightly updated the PR to fix the url fields |
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