Skip to content
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

Merged
merged 150 commits into from Nov 27, 2014
Merged

Processed metrics metadata #6589

merged 150 commits into from Nov 27, 2014

Conversation

mattab
Copy link
Member

@mattab mattab commented Nov 4, 2014

No description provided.

@@ -227,6 +240,12 @@ private function handleDataTable(DataTableInterface $datatable)
$datatable = $filter->filter($label, $datatable, $addLabelIndex);
}

if (!($this->apiRenderer instanceof Original)
Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member

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:

http://demo2.piwik.org/index.php?module=API&method=VisitsSummary.get&idSite=1&period=day&date=yesterday&format=html&token_auth=637769d1f85f8bde0001943db1491a02

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.

Copy link
Member Author

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

diosmosis added 16 commits November 6, 2014 17:46
…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.
@mattab
Copy link
Member Author

mattab commented Nov 25, 2014

Entry pages has metrics documentation for exit_rate, is that what you
mean? It is not elsewhere in the report.

Yes that's what I mean, it would be more clear/consisten if metrics not
in the list of metrics would be removed from the metric documentation +1

@mattab
Copy link
Member Author

mattab commented Nov 25, 2014

Just remove from Processed Report

diosmosis added 21 commits November 25, 2014 15:10
…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
…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
diosmosis pushed a commit that referenced this pull request Nov 27, 2014
Major refactor, compute and format processed metrics through new ProcessedMetric metadata classes instead of using DataTable filters and column callback values.
@diosmosis diosmosis merged commit abf6f88 into master Nov 27, 2014
@diosmosis diosmosis deleted the processed_metrics_metadata branch November 27, 2014 04:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants