Handle parse_accept_lang_header error #53

Merged
merged 1 commit into from Sep 10, 2013

2 participants

@jgmize
Mozilla member

Handle bug described in https://code.djangoproject.com/ticket/21078
The fix has been merged into the Django master branch, but won't be in
a Django release for some time.

Also added test coverage for funfactory.urlresolvers.Prefix class.

@jgmize
Mozilla member

@Osmose r?

@Osmose Osmose and 2 others commented on an outdated diff Sep 10, 2013
tests/test_urlresolvers.py
@@ -54,3 +57,102 @@ def test_unicode_url(self, get_url_prefix):
# Ensure that UTF-8 characters are escaped properly.
self.assertEqual(result, '/Fran%C3%A7oi/test/')
self.assertEqual(type(result), str)
+
+
+class TestPrefixer(TestCase):
+ def setUp(self):
+ self.factory = RequestFactory()
+
+ @override_settings(LANGUAGE_CODE='en-US')
+ def test_get_language_default_language_code(self):
+ """
+ Should return default set by settings.LANGUAGE_CODE
@Osmose
Mozilla member
Osmose added a line comment Sep 10, 2013

Nitpicking: The docstrings could be a bit more descriptive, something like "If there's no lang param or HTTP_ACCEPT_LANGUAGE header, return settings.LANGUAGE_CODE."

@andymckay
Mozilla member
andymckay added a line comment Sep 10, 2013

That's a really annoying side effect of nose tests. I would just turn it off by putting this in your test case base class:

https://github.com/mozilla/zamboni/blob/master/apps/amo/tests/__init__.py#L237-L239

@kumar303
Mozilla member
kumar303 added a line comment Sep 10, 2013

or you could run tests with nicedots https://github.com/kumar303/nose-nicedots

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose commented on an outdated diff Sep 10, 2013
tests/test_urlresolvers.py
+ Should return param value
+ """
+ request = self.factory.get('/?lang=de')
+ ok_('lang' in request.GET)
+ ok_('de' in settings.LANGUAGE_URL_MAP)
+ prefixer = Prefixer(request)
+ eq_(prefixer.get_language(), 'de')
+
+ @override_settings(LANGUAGE_CODE='en-US',
+ LANGUAGE_URL_MAP={'en-us': 'en-US'})
+ def test_get_language_invalid_lang_param(self):
+ """
+ Should return default set by settings.LANGUAGE_CODE
+ """
+ request = self.factory.get('/?lang=de')
+ ok_('lang' in request.GET)
@Osmose
Mozilla member
Osmose added a line comment Sep 10, 2013

Might be more useful to assert that lang is de, although the line above is pretty clear that that's the case, so the assertions might be excessive anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose
Mozilla member

r+, it's pretty much all nits. I really like the idea of asserting that the tests are set up correctly too.

Nice work! 🐤

@kumar303 kumar303 commented on the diff Sep 10, 2013
funfactory/urlresolvers.py
@@ -98,16 +98,18 @@ def get_best_language(self, accept_lang):
langs = dict(LUM)
langs.update((k.split('-')[0], v) for k, v in LUM.items() if
k.split('-')[0] not in langs)
- ranked = parse_accept_lang_header(accept_lang)
- for lang, _ in ranked:
- lang = lang.lower()
- if lang in langs:
- return langs[lang]
- pre = lang.split('-')[0]
- if pre in langs:
- return langs[pre]
- # Could not find an acceptable language.
- return False
@kumar303
Mozilla member
kumar303 added a line comment Sep 10, 2013

I'm not sure if anything was relying on return False but maybe you should add it back in just in case

@jgmize
Mozilla member
jgmize added a line comment Sep 10, 2013

The only thing I could find that used it was the get_language method in the same class, which was only dependent on it being falsey, and any other libraries that use get_best_language directly should still work just fine as long as they aren't using the "is False" antipattern.

@kumar303
Mozilla member
kumar303 added a line comment Sep 10, 2013

ok, None works for me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jgmize jgmize Handle parse_accept_lang_header error
Handle bug described in https://code.djangoproject.com/ticket/21078
The fix has been merged into the Django master branch, but won't be in
a Django release for some time.

Also added test coverage for funfactory.urlresolvers.Prefix class.
358a5d9
@jgmize
Mozilla member

Thanks for the feedback. I've amended the commit based on it.

@Osmose
Mozilla member

arrplus, merge at your leisure

@jgmize jgmize merged commit 1ad0eb2 into mozilla:master Sep 10, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment