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

New browser engine reports based on new browser engine detection of DeviceDetector #6420

Merged
merged 48 commits into from Oct 24, 2014

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Oct 11, 2014

First step of removing old UserSettings stuff (refs #3962)

  • updates DeviceDetector to latest version (and adds missing browser/os icons)
  • old BrowserType reports (UserSettings plugin) has been removed / replaced with new browser engine reports in DeviceDetection plugin
  • those reports will be much more accurate as before the detection was only based on a browser name > browser engine mapping
  • browser eninge is now stored in a new db field
  • browser engine is also available for segmentation

sgiehl and others added 30 commits September 28, 2014 00:28
… 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
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
@sgiehl
Copy link
Member Author

sgiehl commented Oct 17, 2014

I discussed it with @tsteur. For now it's easier to create the column in the update script

@sgiehl
Copy link
Member Author

sgiehl commented Oct 19, 2014

@mattab Anything else? Otherwise I guess we can merge

@mattab
Copy link
Member

mattab commented Oct 19, 2014

@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

@sgiehl
Copy link
Member Author

sgiehl commented Oct 19, 2014

Ok. fine

parent::init();
$this->dimension = new BrowserEngine();
$this->name = Piwik::translate('DevicesDetection_BrowserEngines');
$this->documentation = ''; // TODO
Copy link
Member

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

Copy link
Member Author

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 ;)

@mattab mattab added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. labels Oct 22, 2014
@sgiehl
Copy link
Member Author

sgiehl commented Oct 22, 2014

@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>
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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?

Copy link
Member

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.

@sgiehl
Copy link
Member Author

sgiehl commented Oct 23, 2014

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

mattab pushed a commit that referenced this pull request Oct 24, 2014
New browser engine reports based on new browser engine detection of DeviceDetector
@mattab mattab merged commit fa5a3c1 into master Oct 24, 2014
@mattab
Copy link
Member

mattab commented Oct 24, 2014

Kuddos for the pull request and the quest to deprecate UserSettings plugin @sgiehl - keep it up!!

@sgiehl sgiehl removed the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Oct 24, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants