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
add enable browser detection and some tests #18633
Conversation
add enableBrowserFeatureDetection and simple tests
update tests
build js |
This reverts commit 21de4c7.
…g/matomo into m-18448-add-enable-fingerprint
add tests
update wrong function
update tests
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.
@peterhashair seems there are some JSLint errors, that need to be fixed. Otherwise looks good.
@tsteur would that be something that should be included in 4.7.0 release?
Very low risk of breaking something so that could be included. Be also good to add more tests. We could make sure no browser resolution and detected plugins information is in the tracking request when disabled. Seems the other usages of We'd also need to update the previously FAQ to mention the new method. We also need to update https://developer.matomo.org/api-reference/tracking-javascript to mention the new methods. BTW are these new methods mentioned in the developer changelog? This is documented in https://developer.matomo.org/guides/jstracker-core#adding-a-new-public-js-tracker-method |
@tsteur regarding the tests, I was thinking this test include the browser resolution check. Should I do a fake tracking in Integration tests? test("Browser detector feature Disable and enable", function() {
var pattern = /res/;
var tracker = Piwik.getTracker();
tracker.disableBrowserFeatureDetection();
var request = tracker.getRequest('hello=world');
equal(pattern.test(request), false, 'Disable browser fingerprint');
tracker.enableBrowserFeatureDetection();
var request = tracker.getRequest('hello=world');
equal(pattern.test(request), true, 'Enable browser fingerprint set');
}); |
Above test looks good. You'll also want to check this for the other tracking parameters. And could also adjust the messages like |
update tests
disable jslint for complie js
This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers |
# Conflicts: # js/piwik.min.js # matomo.js # piwik.js
@peterhashair There are still some JSLint errors to be fixed. If you need help with that, let me know. |
update jsLint error
@sgiehl found it, fixed it. But it seems like the release check failed because the file size increased? |
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.
Left some minor comments. Otherwise looks good.
remove space
update wording
Description:
Fixes: #18448
add enable browser detection and some tests
develop document
FAQ draft
Review