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
Processed metrics metadata #6589
Conversation
@@ -227,6 +240,12 @@ private function handleDataTable(DataTableInterface $datatable) | |||
$datatable = $filter->filter($label, $datatable, $addLabelIndex); | |||
} | |||
|
|||
if (!($this->apiRenderer instanceof Original) |
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.
why !($this->apiRenderer instanceof Original)
here? without thinking about it, I'd expect original renderer to also apply the FormatProcessedMetrics
filter?
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.
When 'original' is used, it's PHP code that's calling the API method. The caller will probably want to do extra processing, such as calling filters. If we format the columns here (ie, turn them into strings), users will run into the sumRowArray string + string error.
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.
this looks like a potential design concern that Original is handled separately. IMO it should be handled like other formats when it comes to FormatProcessedMetric filter. could we change this? btw API http requests can also specify format=original
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.
This is what happens when API format is original in the browser:
http://demo2.piwik.org/index.php?module=API&method=VisitsSummary.get&idSite=1&period=day&date=yesterday&format=original&token_auth=637769d1f85f8bde0001943db1491a02
And this when format=html
:
The latter is just the unescaped text of the former. There is no reason to use format=original in an HTTP request.
If you format values all the time, this change will not fix the sumRowArray bug.
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.
you pasted the token here as well :( Please be careful with links to demo2 - I've changed password
…d new filters & implemented as methods in Report to avoid confusion regarding re-use, allow adding processed metrics as DataTable metadata and use this to rewrite AddProcessedMetrics filter, correct name of Metrics::getMappingFromIdToName function, added placeholder AggregatedMetric class for future, revise Metric/ProcessedMetric hierarchy (add methods for translation/etc.), allow .get API methods to use metadata to automatically figure out which columns to select, get EcommerceOrderWithItemsTest to pass.
…s DataTable metadata.
…et and fixing some tests.
ffa12a8
to
7f94d08
Compare
…eneric filters disabled, and do not apply queued filters in datatablemanipulators when fetching subtables.
Yes that's what I mean, it would be more clear/consisten if metrics not |
Just remove from Processed Report |
…play certain processed metrics in reports).
Conflicts: tests/PHPUnit/System/expected/test_TwoVisitors_twoWebsites_differentDays_scheduled_report_in_pdf_tables_only__ScheduledReports.generateReport_month.original.pdf tests/PHPUnit/System/expected/test_csvExport__Live.getLastVisitsDetails_day.csv tests/PHPUnit/System/expected/test_ecommerceOrderWithItems_scheduled_report_in_pdf_tables_only__ScheduledReports.generateReport_week.original.pdf
…ill be removed if not explicitly requested.
…olution and ecommerce_revenue_evolution are not present if ecommerce is not enabled for the site.
Conflicts: tests/PHPUnit/System/expected/test_TwoVisitors_twoWebsites_differentDays_scheduled_report_in_csv__ScheduledReports.generateReport_month.original.csv tests/PHPUnit/System/expected/test_TwoVisitors_twoWebsites_differentDays_scheduled_report_in_html_tables_only__ScheduledReports.generateReport_month.original.html tests/PHPUnit/System/expected/test_apiGetReportMetadata__API.getReportMetadata_day.xml tests/PHPUnit/System/expected/test_ecommerceOrderWithItems_scheduled_report_in_html_tables_only__ScheduledReports.generateReport_week.original.html
Conflicts: plugins/Actions/Reports/GetSiteSearchKeywords.php tests/PHPUnit/System/expected/test_ArchiveCronTest_archive_php_cron_output.txt
Major refactor, compute and format processed metrics through new ProcessedMetric metadata classes instead of using DataTable filters and column callback values.
No description provided.