@peterhashair opened this Pull Request on January 17th 2022 Contributor

Description:

Fixes: #18448

add enable browser detection and some tests

develop document

FAQ draft

Review

@peterhashair commented on January 17th 2022 Contributor

build js

@tsteur commented on January 17th 2022 Member

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 commented on January 19th 2022 Contributor

@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 commented on January 19th 2022 Member

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'

@github-actions[bot] commented on February 3rd 2022 Contributor

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

@sgiehl commented on February 3rd 2022 Member

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

@peterhashair commented on February 3rd 2022 Contributor

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

This Pull Request was closed on February 14th 2022
Powered by GitHub Issue Mirror