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
New API parameter: hideColumns= to specify a list of columns not to return #3300
Comments
What about a parameter that lists columns to show (ie, filter_show_columns)? This way new columns won't result in performance degradations. For metric documentation, since it's always the same, perhaps it could be moved to another API function 'getMetricDocumentation'? Doesn't make sense to return it w/ every report. What do you think of these ideas? |
That could be a simpler solution indeed, to return metrics documentation in a different API... listing columns to show might be more tricky. OK to move the <metricsDocumentation> to a separate API call :) I'll ask Thomas if that'd work |
I've been thinking about this some more, and I've revised my solution. Using one getMetricsDocumentation function won't work since the docs can differ based on plugin (for example, the Actions plugin defines a different documentation for nb_visits than the default docs). Instead I think we should do the following:
Do you think this will work? |
Actually, just came up w/ a better idea, I'll upload a patch in a bit. |
I think it's simpler to simply have a hideMetricsDoc=1 parameter? |
Yes that was my idea :) |
(In [6682]) Refs #3300, added hideMetricsDoc parameter to metadata API actions that can return metrics documentation (such as getProcessedReport, getReportMetadata & getMetadata). |
(In [6684]) Refs #3300, test hideMetricsDoc w/ getMetadata instead of getReportMetadata. |
Attachment: Patch for this issue (implements filter_hide_columns/filter_show_columns). |
I've put up a patch that implements the other part of this issue. It will allow you to use two new filter parameters: filter_hide_columns/filter_show_columns that will remove columns from the API result. One thing to note: This patch allows filter_hide_columns or filter_show_columns to be specified. The way I've implemented this in the patch will result in the RemoveColumns filter running even if the parameters aren't supplied. It shouldn't result in any added overhead though, since nothing will happen when there are no parameters. But if it should be changed, I can fix it by creating two small sub-classes (RemoveColumnsWhitelist/RemoveColumnsBlacklist) in RemoveColumns.php. Let me know what you think. |
Cool feature.
|
Attachment: New patch for hide/show columns. |
I uploaded a new patch. Replying to matt:
This didn't work too well w/ showColumns so I took a different approach to make sure subtables got filtered.
I got this to work, but I had to add a bit of a hack to API/API.php.
Added :). Let me know what you think of this patch. One other thing to note: Right now, this filter won't work w/ some columns, since they are added via DataTable::queueFilter. At the moment, queued filters are run after generic filters, so it won't affect them. Should this change? |
Thanks for the new patch, the code is much better indeed :-) Review:
|
Attachment: Another patch. |
Replying to matt:
If hideColumns specifies every column that is to be returned, an empty result is returned. If showColumns is empty, nothing happens. Do you think these are appropriate behaviors?
This is what I ended up doing. It won't work for ViewDataTable, but I'm not sure that's an issue.
I've added a new patch w/ these tests. Let me know if it's ok to commit. |
Feedback:
Looks good to commit! |
(In [6836]) Fixes #3300, added hideColumns/showColumns query parameters to customize API output & modified ColumnDelete filter to be more robust. |
Replying to matt:
I already do this (see 'nb_hits' in hide/showColumns in the VisitsSummary.get tests).
I noticed that there was already a ColumnDelete filter, so I removed my RemoveColumns filter and augmented that one before I committed. |
(In [6837]) Refs #3300, tweak to code comment |
In:
http://demo.piwik.org/?module=API&method=API.getReportMetadata&idSites=&period=day&date=today&format=xml&token_auth=anonymous&hideColumns=metricsDocumentation
or http://demo.piwik.org/?module=API&method=API.getProcessedReport&idSite=7&period=day&date=today&apiModule=UserCountry&apiAction=getCountry&showTimer=1&format=xml&token_auth=anonymous
It would be very useful to be able to NOT return the <metricsDocumentation> field since it takes much space. For Piwik Mobile transfert speed is key and extra data such as this makes the app much slower than necessary, esp. since Mobile phone internet trasnafer speed is often slow.
The new parameter would at least work for these API above, but it would be nice to have a generic parameter hideColumns that would work in all APIs automatically.
The text was updated successfully, but these errors were encountered: