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 browser engine reports based on new browser engine detection of DeviceDetector #6420
Conversation
… reports from UserSettings plugin to DevicesDetection plugin. Added segment option for browser engines. Added update script to move old archives and to update old visits with engine data
…DevicesDetection)
Conflicts: composer.lock core/Version.php tests/PHPUnit/Integration/expected/test_AutoSuggestAPITest__Live.getLastVisitsDetails_range.xml tests/PHPUnit/Integration/expected/test_csvExport__Live.getLastVisitsDetails_day.csv
Conflicts: tests/PHPUnit/Integration/expected/test_csvExport__Live.getLastVisitsDetails_day.csv
Conflicts: core/Version.php plugins/CustomVariables/tests/processed/test_CustomVariablesIntegrationTest__Live.getLastVisitsDetails_day.xml tests/PHPUnit/Integration/expected/test_ImportLogs__DevicesDetection.getBrowserEngines_month.xml tests/PHPUnit/Integration/expected/test_ImportLogs__UserSettings.getBrowserType_month.xml tests/PHPUnit/Integration/expected/test_OneVisitorTwoVisits__DevicesDetection.getBrowserEngines_day.xml tests/PHPUnit/Integration/expected/test_OneVisitorTwoVisits__UserSettings.getBrowserType_day.xml tests/PHPUnit/Integration/expected/test_noVisit_PeriodIsLast__DevicesDetection.getBrowserEngines_day.xml tests/PHPUnit/Integration/expected/test_noVisit_PeriodIsLast__DevicesDetection.getBrowserEngines_week.xml tests/PHPUnit/Integration/expected/test_noVisit_PeriodIsLast__UserSettings.getBrowserType_day.xml tests/PHPUnit/Integration/expected/test_noVisit_PeriodIsLast__UserSettings.getBrowserType_week.xml tests/PHPUnit/Integration/expected/test_noVisit__DevicesDetection.getBrowserEngines_day.xml tests/PHPUnit/Integration/expected/test_noVisit__UserSettings.getBrowserType_day.xml tests/PHPUnit/System/expected/test_ImportLogs__UserSettings.getBrowserType_month.xml tests/PHPUnit/System/expected/test_OneVisitorTwoVisits__UserSettings.getBrowserType_day.xml tests/PHPUnit/System/expected/test_OneVisitorTwoVisits_withCookieSupport__UserSettings.getBrowserType_day.xml tests/PHPUnit/System/expected/test_OneVisitor_NoKeywordSpecified__Live.getLastVisitsDetails_day.xml tests/PHPUnit/System/expected/test_noVisit_PeriodIsLast__UserSettings.getBrowserType_day.xml tests/PHPUnit/System/expected/test_noVisit_PeriodIsLast__UserSettings.getBrowserType_week.xml tests/PHPUnit/System/expected/test_noVisit__UserSettings.getBrowserType_day.xml
I discussed it with @tsteur. For now it's easier to create the column in the update script |
@mattab Anything else? Otherwise I guess we can merge |
@sgiehl Kuddo for the PR it looks excellent. Goal is to merge this PR in the coming week right after 2.8.1 https://github.com/piwik/piwik/milestones/Piwik%202.8.1 |
Ok. fine |
Conflicts: core/Version.php
Conflicts: core/Version.php
parent::init(); | ||
$this->dimension = new BrowserEngine(); | ||
$this->name = Piwik::translate('DevicesDetection_BrowserEngines'); | ||
$this->documentation = ''; // TODO |
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.
FYI there's a TODO
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.
Not sure if we need a documentation for that row. I have no idea what it should be. I you haven't neither, I'll simply remove the TODO ;)
…p removing unused archives for now, as we will remove them later all in one
@mattab guess everything you requested should be changed now. So feel free to merge :) |
<?xml version="1.0" encoding="utf-8" ?> | ||
<result> | ||
<row> | ||
<label>Gecko (Firefox)</label> |
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.
we should keep one UserSettings.getBrowserType
system test to make sure we don't regress this API call in the future?
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.
I've marked that method as deprecated, so it's not used in the tests anymore. Is there a way to include it again, without removing the deprecation?
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.
in System Tests we define API to call at: https://github.com/piwik/piwik/blob/master/tests/PHPUnit/System/OneVisitorTwoVisitsWithCookieSupportTest.php#L36-36
Right now it's set to 'UserSettings' which will automatically test all APIs. But you can manually specify all the APIs to test eg (including the @deprecated ones). note that you have to manually specify each method. does it work?
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.
I've tried replacing 'UserSettings' with 'UserSettings.getBrowserType' to test it, but the method is still not called.
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.
I've thought if it would make sense to add an BC test at that point. If we are going to remove UserSettings completely and use the 'Api-renaming' to have the UserSettings-API still callable, I guess it won't make sense to add test for each method at that point. Maybe it would be enough to add a simple test, that just checks if the methods are still callable. Instead of testing the content of the old methods we should add tests for the new methods... what do you think?
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.
BC test was a suggestion, and you're right, that it would be enough to check whether all UserSettings APIs are still callable.
Conflicts: core/Version.php
Conflicts: core/Version.php
As there is currently no proper way to call deprecated API methods, I would suggest to merge the PR now. I'll add tests for all UserSettings API methods later |
New browser engine reports based on new browser engine detection of DeviceDetector
Kuddos for the pull request and the quest to deprecate UserSettings plugin @sgiehl - keep it up!! |
First step of removing old UserSettings stuff (refs #3962)