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

add enable browser detection and some tests #18633

Merged
merged 17 commits into from Feb 14, 2022

Conversation

peterhashair
Copy link
Contributor

@peterhashair peterhashair commented Jan 17, 2022

Description:

Fixes: #18448

add enable browser detection and some tests

develop document

FAQ draft

Review

Peter added 2 commits January 17, 2022 17:21
add enableBrowserFeatureDetection and simple tests
@peterhashair
Copy link
Contributor Author

build js

Copy link
Member

@sgiehl sgiehl left a 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?

@tsteur
Copy link
Member

tsteur commented Jan 17, 2022

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 detectBrowserFeatures might be less testable because they are partially including random uids etc.

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

@peterhashair peterhashair added this to the 4.8.0 milestone Jan 18, 2022
@peterhashair
Copy link
Contributor Author

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

    });

@tsteur
Copy link
Member

tsteur commented Jan 19, 2022

Above test looks good. You'll also want to check this for the other tracking parameters. And could also adjust the messages like Disable browser fingerprint' change it to When browser fingerprint is disabled the request should not include browser resolution'

Peter added 2 commits January 19, 2022 14:51
update tests
disable jslint for complie js
@peterhashair peterhashair added Needs Review PRs that need a code review c: Documentation For issues related to in-app product help messages, or to the Matomo knowledge base. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Jan 26, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2022

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Feb 3, 2022
# Conflicts:
#	js/piwik.min.js
#	matomo.js
#	piwik.js
@sgiehl
Copy link
Member

sgiehl commented Feb 3, 2022

@peterhashair There are still some JSLint errors to be fixed. If you need help with that, let me know.

@sgiehl sgiehl removed Needs Review PRs that need a code review Stale The label used by the Close Stale Issues action labels Feb 3, 2022
update jsLint error
@peterhashair
Copy link
Contributor Author

peterhashair commented Feb 3, 2022

@sgiehl found it, fixed it. But it seems like the release check failed because the file size increased?

Copy link
Member

@sgiehl sgiehl left a 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.

tests/javascript/index.php Outdated Show resolved Hide resolved
tests/javascript/index.php Outdated Show resolved Hide resolved
@peterhashair peterhashair added the Needs Review PRs that need a code review label Feb 13, 2022
@peterhashair peterhashair merged commit 48020c2 into 4.x-dev Feb 14, 2022
@peterhashair peterhashair deleted the m-18448-add-enable-fingerprint branch February 14, 2022 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Documentation For issues related to in-app product help messages, or to the Matomo knowledge base. Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an option to disable fingerprinting / config_id entirely
3 participants