-
Notifications
You must be signed in to change notification settings - Fork 9
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
Migrate Edraak i18n platform #38
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
""" | ||
Edraak.org i18n additions. | ||
|
||
This module improves the edX platform in terms of Arabic support. | ||
The feature flag `EDRAAK_I18N_APP` enables this module on non-test environments. | ||
""" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
""" | ||
Helper functions to Edraak i18n module. | ||
""" | ||
|
||
|
||
def add_locale_middleware(middleware_classes): | ||
""" | ||
Adds the Edraak's DefaultLocaleMiddleware to the MIDDLEWARE_CLASSES tuple correctly. | ||
|
||
This function is meant to be used within the settings files like lms/envs/aws.py and others. | ||
|
||
Args: | ||
middleware_classes: The MIDDLEWARE_CLASSES tuple from the settings. | ||
|
||
Returns: | ||
The new MIDDLEWARE_CLASSES with the edraak_i18n middleware. | ||
""" | ||
|
||
edraak_middleware = 'edraak_i18n.middleware.DefaultLocaleMiddleware' | ||
|
||
other_locale_middlewares = ( | ||
'lang_pref.middleware.LanguagePreferenceMiddleware', | ||
'dark_lang.middleware.DarkLangMiddleware', | ||
'django.middleware.locale.LocaleMiddleware', | ||
) | ||
|
||
indexes = [ | ||
middleware_classes.index(class_name) for class_name in other_locale_middlewares | ||
if class_name in middleware_classes | ||
] | ||
|
||
first_index = min(indexes) | ||
|
||
# Insert the DefaultLocaleMiddleware before any other locale-related middleware in order for it to work | ||
return middleware_classes[:first_index] + (edraak_middleware,) + middleware_classes[first_index:] | ||
|
||
|
||
def is_api_request(request): | ||
""" | ||
Checks if the a request is targeting an API endpoint. | ||
|
||
Args: | ||
request: A django request. | ||
|
||
Returns: True if the request is an API request and False otherwise. | ||
""" | ||
if request.path.startswith('/api/'): | ||
return True | ||
elif request.path.startswith('/user_api/'): | ||
return True | ||
elif request.path.startswith('/notifier_api/'): | ||
return True | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can use regex here to reduce lines of code There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but I'd like to be very specific and it is more readable for me this way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. its not generic though, if more urls are introduced in the future that represent api requests, then the decision tree becomes hideous, no? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not generic, and we'll wait for it to break in the future, we still rely on manual QA for e2e tests. While the regexp is a valid suggestion, it's not guaranteed to work. For example edX could add any of these:
There's still some guessing and the code won't be future proof anyway, in this case I always fallback to the stupid solution i.e. simple if statement vs. a RegExp. |
||
|
||
return False |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
""" | ||
Edraak i18n Middleware | ||
""" | ||
|
||
from django.conf import settings | ||
from edraak_i18n.helpers import is_api_request | ||
|
||
|
||
class DefaultLocaleMiddleware(object): | ||
""" | ||
Changes the language to `settings.LANGUAGE_CODE` for all non-API requests. | ||
|
||
This will force the i18n machinery to always choose settings.LANGUAGE_CODE | ||
as the default initial language, unless another one is set via sessions or cookies. | ||
|
||
Should be installed *before* any middleware that checks request.META['HTTP_ACCEPT_LANGUAGE'], | ||
specifically django.middleware.locale.LocaleMiddleware | ||
|
||
Although the middleware is installed by default, it sill checks for the feature flag below in order to work. | ||
|
||
- `EDRAAK_I18N_LOCALE_MIDDLEWARE` | ||
""" | ||
|
||
def process_request(self, request): | ||
""" | ||
Changes request's `HTTP_ACCEPT_LANGUAGE` to `settings.LANGUAGE_CODE`. | ||
""" | ||
|
||
# Edraak (hack): The DefaultLocaleMiddleware is disabled by default during tests, which is not very accurate. | ||
if not settings.FEATURES.get('EDRAAK_I18N_LOCALE_MIDDLEWARE'): | ||
return | ||
|
||
if is_api_request(request): | ||
# This middleware is only needed for browser page, it's effect is breaking the behaviour on the mobile | ||
# apps. | ||
return | ||
|
||
if 'HTTP_ACCEPT_LANGUAGE' in request.META: | ||
# Preserve the original value just in case, | ||
# the underscore prefix means that you probably shouldn't be using it anyway | ||
request.META['_HTTP_ACCEPT_LANGUAGE'] = request.META['HTTP_ACCEPT_LANGUAGE'] | ||
|
||
# Make the accept language as same as the site original language regardless of the original value | ||
# Django will use this value in the LocaleMiddleware to display the correct translation | ||
request.META['HTTP_ACCEPT_LANGUAGE'] = settings.LANGUAGE_CODE |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,192 @@ | ||
# -*- coding: utf-8 -*- | ||
""" | ||
Tests for the Edraak i18n module. | ||
""" | ||
|
||
# The disable below because pylint is not recognizing request.META. | ||
# pylint: disable=no-member | ||
|
||
from django.test import TestCase, RequestFactory, override_settings | ||
from django.conf import settings | ||
|
||
from mock import patch | ||
import ddt | ||
|
||
from dark_lang.models import DarkLangConfig | ||
from student.tests.factories import UserFactory | ||
|
||
from edraak_i18n.middleware import DefaultLocaleMiddleware | ||
from edraak_i18n.helpers import is_api_request | ||
|
||
|
||
@ddt.ddt | ||
class SettingsTest(TestCase): | ||
""" | ||
Sanity checks for the settings related to the edraak_i18n module. | ||
""" | ||
def test_if_enabled(self): | ||
""" | ||
Ensures that the app is enabled. | ||
""" | ||
self.assertIn('edraak_i18n', settings.INSTALLED_APPS, 'The app should be enabled by default in test.') | ||
|
||
def test_middleware_existing_but_disabled(self): | ||
""" | ||
Ensures that the middleware is disabled by default. | ||
""" | ||
self.assertNotIn('EDRAAK_I18N_LOCALE_MIDDLEWARE', settings.FEATURES, 'The middleware should be disabled.') | ||
self.assertIn('edraak_i18n.middleware.DefaultLocaleMiddleware', settings.MIDDLEWARE_CLASSES) | ||
|
||
@ddt.data( | ||
'lang_pref.middleware.LanguagePreferenceMiddleware', | ||
'dark_lang.middleware.DarkLangMiddleware', | ||
'django.middleware.locale.LocaleMiddleware', | ||
) | ||
def test_middleware_order(self, other_middleware): | ||
""" | ||
Ensures that the middleware comes before any other locale-related middleware. | ||
""" | ||
edraak_middleware = 'edraak_i18n.middleware.DefaultLocaleMiddleware' | ||
|
||
self.assertLess( | ||
a=settings.MIDDLEWARE_CLASSES.index(edraak_middleware), | ||
b=settings.MIDDLEWARE_CLASSES.index(other_middleware), | ||
msg='Edraak DefaultLocaleMiddleware should come before any other locale-related middleware' | ||
) | ||
|
||
|
||
@ddt.ddt | ||
class DefaultLocaleMiddlewareTest(TestCase): | ||
""" | ||
Unit and integration tests for the DefaultLocaleMiddleware. | ||
""" | ||
def setUp(self): | ||
""" | ||
Set up the environment for the test case and ensures correct DarkLang configurations. | ||
""" | ||
super(DefaultLocaleMiddlewareTest, self).setUp() | ||
|
||
self.middleware = DefaultLocaleMiddleware() | ||
self.request_factory = RequestFactory() | ||
|
||
self.user = UserFactory() | ||
|
||
DarkLangConfig( | ||
released_languages='en,ar,eo', | ||
changed_by=self.user, | ||
enabled=True | ||
).save() | ||
|
||
@ddt.data('/dummy/', '/api/dummy') | ||
@patch.dict(settings.FEATURES, {'EDRAAK_I18N_LOCALE_MIDDLEWARE': False}) | ||
@override_settings(LANGUAGE_CODE='eo') | ||
def test_deactivated(self, api_url): | ||
""" | ||
Checks if the middleware behaves correctly when it is disabled using the feature flag. | ||
""" | ||
req = self.request_factory.get(api_url) | ||
req.META['HTTP_ACCEPT_LANGUAGE'] = 'en' | ||
meta_before = req.META.copy() | ||
self.middleware.process_request(req) | ||
|
||
self.assertEquals( | ||
req.META['HTTP_ACCEPT_LANGUAGE'], | ||
'en', | ||
'The feature flag is disabled, the middleware should pass the request as is.' | ||
) | ||
|
||
self.assertDictEqual( | ||
d1=meta_before, | ||
d2=req.META, | ||
msg='The feature flag is disabled, the middleware should not change the request META.', | ||
) | ||
|
||
@patch.dict(settings.FEATURES, {'EDRAAK_I18N_LOCALE_MIDDLEWARE': True}) | ||
@override_settings(LANGUAGE_CODE='eo') | ||
def test_activated_non_api(self): | ||
""" | ||
Test the activated middleware on non-API pages. | ||
""" | ||
req = self.request_factory.get('/dummy/') | ||
req.META['HTTP_ACCEPT_LANGUAGE'] = 'en' | ||
self.middleware.process_request(req) | ||
|
||
self.assertEquals( | ||
req.META['HTTP_ACCEPT_LANGUAGE'], | ||
'eo', | ||
'The feature flag is enabled, the middleware should change the language for non-API views.' | ||
) | ||
|
||
self.assertEquals( | ||
req.META['_HTTP_ACCEPT_LANGUAGE'], | ||
'en', | ||
'Should preserve the original language in another META variable.' | ||
) | ||
|
||
@ddt.data('/api/', '/user_api/') | ||
@patch.dict(settings.FEATURES, {'EDRAAK_I18N_LOCALE_MIDDLEWARE': True}) | ||
@override_settings(LANGUAGE_CODE='ar') | ||
def test_enabled_api(self, api_url): | ||
""" | ||
Ensures that the middleware doesn't change the non-API pages. | ||
""" | ||
req = self.request_factory.get(api_url) | ||
client_language = 'en' | ||
req.META['HTTP_ACCEPT_LANGUAGE'] = client_language | ||
self.middleware.process_request(req) | ||
|
||
self.assertEquals( | ||
req.META['HTTP_ACCEPT_LANGUAGE'], | ||
client_language, | ||
'The feature flag is enabled, the middleware should NOT change the language for API views.' | ||
) | ||
|
||
@ddt.unpack | ||
@ddt.data( | ||
{'settings_lang': 'en', 'req_lang': 'en', 'valid': 'Skip to', 'invalid': u'Skïp tö'}, | ||
{'settings_lang': 'en', 'req_lang': 'eo', 'valid': 'Skip to', 'invalid': u'Skïp tö'}, | ||
{'settings_lang': 'eo', 'req_lang': 'en', 'valid': u'Skïp tö', 'invalid': 'Skip to'}, | ||
{'settings_lang': 'eo', 'req_lang': 'eo', 'valid': u'Skïp tö', 'invalid': 'Skip to'}, | ||
) | ||
@patch.dict(settings.FEATURES, {'EDRAAK_I18N_LOCALE_MIDDLEWARE': True}) | ||
def test_enabled_middleware_in_request(self, settings_lang, req_lang, valid, invalid): | ||
""" | ||
Testing different combinations of LANGUAGE_CODE and Accept-Language. | ||
|
||
The response language should always respect the `settings_lang` and ignores the `request_lang`. | ||
""" | ||
with override_settings(LANGUAGE_CODE=settings_lang): | ||
headers = { | ||
'Accept-Language': req_lang, | ||
} | ||
|
||
res = self.client.get('/', **headers) | ||
self.assertContains(res, valid, msg_prefix='Incorrect language detected') | ||
self.assertNotContains(res, invalid, msg_prefix='Incorrect language detected') | ||
|
||
|
||
@ddt.ddt | ||
class HelpersTest(TestCase): | ||
""" | ||
Test cases for the helper functions of edraak_i18n module. | ||
""" | ||
def setUp(self): | ||
""" | ||
Initializes the request factory. | ||
""" | ||
super(HelpersTest, self).setUp() | ||
self.request_factory = RequestFactory() | ||
|
||
@ddt.unpack | ||
@ddt.data( | ||
{'path': '/api/', 'should_be_api': True}, | ||
{'path': '/dashboard', 'should_be_api': False}, | ||
{'path': '/', 'should_be_api': False}, | ||
{'path': '/user_api/', 'should_be_api': True}, | ||
{'path': '/notifier_api/', 'should_be_api': True} | ||
) | ||
def test_is_api_request_helper(self, path, should_be_api): | ||
""" | ||
Tests the `is_api_request` helper on different params. | ||
""" | ||
self.assertEquals(is_api_request(self.request_factory.get(path)), should_be_api) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad you do 👍