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 anonymize ip option for google analytics #8417

Merged
merged 3 commits into from
Feb 18, 2017

Conversation

thomas-villagers
Copy link
Contributor

@thomas-villagers thomas-villagers commented Feb 4, 2017

Hi,

I implemented an option for anonymized ips with google analytics.
The section "Google Analytics" in the backend (Stores > Configuration, Sales > Google API) has a new field "Anonymize IP".

According to the value in Anonymize IP, the ga('send' ...)-call in the generated html is either

ga('send', 'pageview', {'anonymizeIp' : false});

or

ga('send', 'pageview', {'anonymizeIp' : true});

(see https://developers.google.com/analytics/devguides/collection/analyticsjs/ip-anonymization)

this is my first try to extend magento and i'm open to suggestions.

  • thomas

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Feb 4, 2017

CLA assistant check
All committers have signed the CLA.

@vrann vrann self-requested a review February 6, 2017 22:56
@vrann vrann self-assigned this Feb 6, 2017
@vrann
Copy link
Contributor

vrann commented Feb 9, 2017

@thomas-villagers Thank you for contribution.
Does it makes sense to set ip anonymization globally (for all hits)?
I.e.:

ga('set', 'anonymizeIp', true);

@thomas-villagers
Copy link
Contributor Author

@vrann Yes, of course! Where would that be done? I could'nt find it in Ga.php.

return "\nga('create', '" . $this->escapeHtmlAttr($accountId, false)
. ", 'auto');\nga('send', 'pageview'{$optPageURL});\n";
. ", 'auto');\nga('send', 'pageview'{$optPageURL}{$anonymizeIp});\n";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you set it globally instead, here?

ga('set', 'anonymizeIp', true);

@vrann
Copy link
Contributor

vrann commented Feb 10, 2017

@thomas-villagers I've added the comment to the code, if you change it to set anonymization globally, it will be applied to other types of hits as well.

@okorshenko okorshenko added this to the February 2017 milestone Feb 11, 2017
@vrann
Copy link
Contributor

vrann commented Feb 15, 2017

@thomas-villagers looks perfect

@mmansoor-magento mmansoor-magento merged commit b1fb0dc into magento:develop Feb 18, 2017
mmansoor-magento pushed a commit that referenced this pull request Feb 18, 2017
 - Merge Pull Request #8434 from Crossmotion/magento2:patch-1
@ctadlock
Copy link

@vrann @thomas-villagers This code is broken, the negative case was never tested. The code is doing a string comparison of config.pageTrackingData.isAnonymizedIpActive. The string value "0" is true.

https://stackoverflow.com/questions/7615214/in-javascript-why-is-0-equal-to-false-but-when-tested-by-if-it-is-not-fals

image

@ctadlock
Copy link

Here is the correct fix. Someone should do a PR for this...

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants