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

[Bug 1058671] Cache geoip results. #2091

Closed
wants to merge 3 commits into from

Conversation

mythmon
Copy link
Contributor

@mythmon mythmon commented Aug 29, 2014

Add cookies with the geoip results. If these cookies are present, the geoip script will not be included. These cookies expire after 30 days.

r?

Add cookies with the geoip results. If these cookies are present, the
geoip script will not be included. These cookies expire after 30 days.
@@ -234,7 +234,10 @@
{% endif %}
{# End Optimizely #}

<script src="//geo.mozilla.org/country.js" type="text/javascript"></script>
{% if include_geoip %}
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume we'll add a and request.locale == 'en-US' here for that other request Kadir had.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I think we should add that in the context manager (less logic in templates and all that), but I also am not super excited about that particular change. I guess we should probably do it though.

@mythmon
Copy link
Contributor Author

mythmon commented Aug 29, 2014

Fixed the console.log stuff, added a 30 day expiry (instead of a session expiry) and only show it to en-US users. ^

@mythmon
Copy link
Contributor Author

mythmon commented Aug 29, 2014

Oh, and I added the tests I forgot to commit earlier, plus an extra pair for en-US/de checking.

@rlr
Copy link
Contributor

rlr commented Sep 2, 2014

nice! r+

@mythmon
Copy link
Contributor Author

mythmon commented Sep 2, 2014

35f80a7

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

Successfully merging this pull request may close these issues.

None yet

2 participants