@denisbabineau opened this Issue on March 21st 2016

I'm using piwik in a cordova application; I want to be able to enable/disable piwik tracking "at runtime" as per user request (thru UI opt-out). I cannot use setDoNotTrack since I can't override the navigator's properties and can't find other means to disable once initialized so I'm using setCustomRequestProcessing to return an undefined (empty) request when I don't want tracking to be sent. This works well for single requests going thru sendRequest but sendBulkRequest doesn't employ the same checks and causes invalid tracking information to be sent (you'll see things like [?], [?], ... in your browser requests to piwik endpoint):

            function sendRequest(request, delay, callback) {
                if (!configDoNotTrack && request) {

vs:

            function canSendBulkRequest(requests)
            {
                if (configDoNotTrack) {
                    return false;
                }

                return (requests && requests.length);
            }

The later only checks for empty arrays, not arrays with empty request. Recommend adding check before adding to array in buildContentImpressionsRequests:

                    request = getRequest(
                        content.buildImpressionRequestParams(contents[index].name, contents[index].piece, contents[index].target),
                        undefined,
                        'contentImpressions'
                    );

                    if (request) {  // NEW CHECK HERE
                        requests.push(request);
                    }
@tsteur commented on March 29th 2016 Owner

I will merge suggested fix soon but I will still leave the issue open. I'm merging it as it makes sense in general to check whether there is actually a request URL. This is a workaround though and won't be documented or officially supported. To fix the issue we should provide a method to actually disable any tracking. I'm kind of surprised we do not have such method yet :)

setDoNotTrack() method is a bit misleading as I would expect to set the value without being dependent on navigator. Instead I would have expected that current method setDoNotTrack is named enableDoNotTrack or similar. Then setDoNotTrack would have actually accepted a boolean and only set the variable without checking navigator.

Goal of this issue would be to introduce a new method that let's a user disable tracking. We could maybe have a method disableTracking or doNotSendTrackingRequests or ... It could set the already existing variable configDoNotTrack to true or it could make use of a new variable since doNotTrack !== doNotSendTrackingRequests in case someone calls enableTracking afterwards the configDoNotTrack should keep it's meaning.

@tsteur commented on March 29th 2016 Owner

FYI: I updated the title

@denisbabineau commented on March 30th 2016

@tsteur thanks for adding the check and I look forward to the proposed enhancements, should make things easier/simpler!

@tsteur commented on March 30th 2016 Owner

BTW: I merged the other pull request so in next version it should at least work as expected for you

@mattab commented on March 31st 2016 Owner

Hi @denisbabineau a Pull request would be welcome to implement the new JS function :+1:

@denisbabineau commented on March 31st 2016

@mattab I was going to create a PR for my original request (the simple check) but the YUICompressor version at the time was no longer available (I see you updated to 2.4.7 just today, so that's good), I didn't want to provide a possibly incompatible minified file. Ideally minified/binaries wouldn't be stored in source control with every commits but I'm sure the piwik project has a reason to be doing so, to each his own :-)

@mattab commented on March 31st 2016 Owner

I'm sure the piwik project has a reason to be doing so, to each is own :-)

Fyi: we do this because some users use Piwik from git: http://piwik.org/faq/how-to-install/faq_18271/ so it's good to have the minified piwik.js ready to use.

Powered by GitHub Issue Mirror