-
Notifications
You must be signed in to change notification settings - Fork 26
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
Fix 262 tests #277
Open
allcaps
wants to merge
4
commits into
wagtail:main
Choose a base branch
from
allcaps:fix-262-tests
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Fix 262 tests #277
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,18 +1,29 @@ | ||
from django.http import Http404 | ||
from django.conf import settings | ||
from django.urls import LocalePrefixPattern | ||
from django.utils.translation import activate, get_language | ||
|
||
from apps.core.models import HomePage | ||
|
||
pattern = LocalePrefixPattern() | ||
|
||
|
||
class ValidateLocaleMiddleware: | ||
def __init__(self, get_response): | ||
self.get_response = get_response | ||
|
||
def __call__(self, request): | ||
try: | ||
HomePage.objects.get(locale__language_code=get_language(), live=True) | ||
except HomePage.DoesNotExist: | ||
activate("en-latest") | ||
raise Http404() | ||
response = self.get_response(request) | ||
if ( | ||
request.path == "/" | ||
or request.resolver_match | ||
and pattern.match(request.resolver_match.route) | ||
): | ||
try: | ||
HomePage.objects.get(locale__language_code=get_language(), live=True) | ||
response = self.get_response(request) | ||
except HomePage.DoesNotExist: | ||
# The requested language is not available, use the default | ||
activate(settings.LANGUAGE_CODE) | ||
response = self.get_response(request) | ||
else: | ||
response = self.get_response(request) | ||
return response |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,119 @@ | ||
from http import HTTPStatus | ||
|
||
from django.conf import settings | ||
from django.test import TestCase | ||
from django.urls import reverse | ||
|
||
from apps.core.factories import HomePageFactory, LocaleFactory | ||
|
||
|
||
class TestValidateLocaleMiddleware(TestCase): | ||
""" | ||
LocaleMiddleware tries to determine the user’s language preference by following | ||
this algorithm: | ||
|
||
1. First, it looks for the language prefix in the requested URL. | ||
2. Failing that, it looks for a cookie, named `django_language`. | ||
3. Failing that, it looks at the Accept-Language HTTP header. This header is sent by | ||
your browser and tells the server which language(s) you prefer, in order by | ||
priority. Django tries each language in the header until it finds one with | ||
available translations. | ||
4. Failing that, it uses the global LANGUAGE_CODE setting. | ||
|
||
Wagtail Guide has all languages in LANGUAGES setting, therefore LocaleMiddleware | ||
will redirect to any language prefix URL. Unfortunately, Wagtail might not have | ||
the corresponding homepage for that language published. If Wagtail has no | ||
homepage available for the requested language, it raises a 404. | ||
|
||
This behaviour is undesirable. As step 3 of the algoritm will end up in a 404. | ||
To resolve this issue, we introduce our ValidateLocaleMiddleware. | ||
|
||
ValidateLocaleMiddleware checks for a published homepage for the requested language. | ||
If it does not exist, it falls back to English. The default, and always published | ||
language/homepage. | ||
|
||
The ValidateLocaleMiddleware kicks in on `/` and any i18n pattern. | ||
Other URLs are ignored, and have the default LocaleMiddleware behaviour. | ||
""" | ||
|
||
def setUp(self): | ||
self.en = LocaleFactory(language_code="en-latest") | ||
self.home_en = HomePageFactory(locale=self.en) | ||
|
||
def test_middleware_settings(self): | ||
self.assertIn("django.middleware.locale.LocaleMiddleware", settings.MIDDLEWARE) | ||
self.assertIn( | ||
"apps.core.middleware.ValidateLocaleMiddleware", settings.MIDDLEWARE | ||
) | ||
self.assertGreater( | ||
settings.MIDDLEWARE.index("apps.core.middleware.ValidateLocaleMiddleware"), | ||
settings.MIDDLEWARE.index("django.middleware.locale.LocaleMiddleware"), | ||
) | ||
|
||
def test_request_root_redirects_to_language_code(self): | ||
self.assertEqual(settings.LANGUAGE_CODE, "en-latest") | ||
response = self.client.get("/") | ||
self.assertEqual(response.status_code, HTTPStatus.FOUND) | ||
self.assertEqual(response.url, "/en-latest/") | ||
self.assertEqual(self.client.get("/en-latest/").status_code, HTTPStatus.OK) | ||
|
||
def test_request_root_with_accept_language_header(self): | ||
# To English, if German doesn't exist | ||
response = self.client.get("/", HTTP_ACCEPT_LANGUAGE="de") | ||
self.assertEqual(response.status_code, HTTPStatus.FOUND) | ||
self.assertEqual(response.url, "/en-latest/") | ||
self.assertEqual(self.client.get("/en-latest/").status_code, HTTPStatus.OK) | ||
# To German, if German exists | ||
de = LocaleFactory(language_code="de-latest") | ||
self.home_de = self.home_en.copy_for_translation(locale=de) | ||
self.home_de.save_revision().publish() | ||
response = self.client.get("/", HTTP_ACCEPT_LANGUAGE="de") | ||
self.assertEqual(response.status_code, HTTPStatus.FOUND) | ||
self.assertEqual(response.url, "/de-latest/") | ||
self.assertEqual(self.client.get("/de-latest/").status_code, HTTPStatus.OK) | ||
|
||
def test_request_root_with_cookie(self): | ||
self.assertEqual(settings.LANGUAGE_COOKIE_NAME, "django_language") | ||
de = LocaleFactory(language_code="de-latest") | ||
self.home_de = self.home_en.copy_for_translation(locale=de) | ||
self.home_de.save_revision().publish() | ||
# The HTTP_ACCEPT_LANGUAGE is ignored, the cookie takes precedence | ||
self.client.cookies["django_language"] = "en-latest" | ||
response = self.client.get("/", HTTP_ACCEPT_LANGUAGE="de") | ||
self.assertEqual(response.status_code, HTTPStatus.FOUND) | ||
self.assertEqual(response.url, "/en-latest/") | ||
self.assertEqual(self.client.get("/en-latest/").status_code, HTTPStatus.OK) | ||
|
||
def test_request_specific_url(self): | ||
de = LocaleFactory(language_code="de-latest") | ||
self.home_de = self.home_en.copy_for_translation(locale=de) | ||
self.home_de.save_revision().publish() | ||
# The HTTP_ACCEPT_LANGUAGE is ignored | ||
self.client.cookies["django_language"] = "de" | ||
# The HTTP_ACCEPT_LANGUAGE is ignored | ||
response = self.client.get("/en-latest/", HTTP_ACCEPT_LANGUAGE="de") | ||
self.assertEqual(response.status_code, HTTPStatus.OK) | ||
|
||
def test_wagtail_admin_respects_accept_language(self): | ||
# Non i18n_patterns respect HTTP_ACCEPT_LANGUAGE | ||
url = reverse("wagtailadmin_login") | ||
response = self.client.get(url, HTTP_ACCEPT_LANGUAGE="nl") | ||
expected = "<h1>Inloggen in Wagtail</h1>" | ||
self.assertInHTML(expected, str(response.content)) | ||
|
||
def test_django_admin_respects_accept_language(self): | ||
# Non i18n_patterns respect HTTP_ACCEPT_LANGUAGE | ||
url = reverse("admin:login") | ||
response = self.client.get(url, HTTP_ACCEPT_LANGUAGE="nl") | ||
expected = '<h1 id="site-name"><a href="/django-admin/">Django-beheer</a></h1>' | ||
self.assertInHTML(expected, str(response.content)) | ||
|
||
def test_sitemap_xml(self): | ||
# Non i18n_patterns respect HTTP_ACCEPT_LANGUAGE | ||
url = "/sitemap.xml" | ||
response = self.client.get(url, HTTP_ACCEPT_LANGUAGE="nl") | ||
expected = ( | ||
b'<urlset xmlns="http://www.sitemaps.org/schemas/sitemap/0.9" ' | ||
b'xmlns:xhtml="http://www.w3.org/1999/xhtml">' | ||
) | ||
self.assertIn(expected, response.content) |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
If you call the resolver before
get_response
you will always have None, so this code defaults to the else statement every single time. I ran the tests with a print statement in the else section and it printed every time.I also tried to set my language to a language that had a matching homepage and it did not properly set my preferred language until I put the
response = self.get_response(request)
at the top.I see that you removed the 404 response when a language does not exist, was there a reason for that?
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'm not sure why the tests do not fail on the language preference, but at least it was not working in the browser for me
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 think that the tests that correctly pass when a user asks for "/" and is redirected to the preferred language (browser or cookie) because of LocaleMiddleware
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.
So, I guess we need to take a step back and actually think about what the requirements are