Support special-case showing of a locale list at / if no accept-lang …

…header is present

This changeset ensures we retain the existing behaviour where a robot/spider
going to / with no accept-language header gets a 200 OK response but with a
locale-picker page, NOT the homepage.

The fix here was to avoid running Django's LocaleMiddleware when (and only
when) we detect the special case of a request for the root path and no
Accept-Language header - which is basically a call for the root page with
no way to determine the client's locale. (We don't use sessions in Bedrock
so Django will never find a language code in a session anyway)

bedrock/base/middleware.py

@@ -33,6 +33,7 @@
     path_needs_lang_code,
     split_path_and_polish_lang,
 )
+from lib.l10n_utils import is_root_path_with_no_language_clues
 
 
 class BedrockLangCodeFixupMiddleware(MiddlewareMixin):
@@ -80,7 +81,7 @@ def process_request(self, request):
             request.GET = qs
             return self._redirect(request, cleaned_lang_via_querystring, subpath)
 
-        if not lang_code and path_needs_lang_code(request.path):
+        if not lang_code and path_needs_lang_code(request.path) and not is_root_path_with_no_language_clues(request):
             lang_code = get_language_from_headers(request)
             return self._redirect(request, lang_code, subpath)
 
@@ -91,10 +92,14 @@ def process_request(self, request):
 
 
 class BedrockLocaleMiddleware(DjangoLocaleMiddleware):
-    """Light middleware wrapped around Django's own i18n middleware
-    that ensures we normalize language codes - i..e. we ensure they are in
+    """Middleware that usually* wraps Django's own i18n middleware in order to
+    ensure we normalize language codes - i..e. we ensure they are in
     mixed case we use, rather than Django's internal all-lowercase codes.
 
+    *for one specific situation, though, we skip Django's LocaleMiddleware entirely:
+    we have a special SEO-helping page that gets returned to robots/spiders that
+    don't declare an accept-language header, showing a list of locales to pick.
+
     It needs to be kept super-light so that it doesn't diverge too far from
     the stock LocaleMiddleware, lest we wake up dragons when we use
     wagtail-localize, which squarely depends on django's LocaleMiddleware.
@@ -104,11 +109,22 @@ class BedrockLocaleMiddleware(DjangoLocaleMiddleware):
     """
 
     def process_request(self, request):
-        with normalized_get_language():
-            with simplified_check_for_language():
-                return super().process_request(request)
+        if is_root_path_with_no_language_clues(request):
+            # Skip using Django's LocaleMiddleware
+            metrics.incr(
+                "bedrock.localemiddleware.skipdjangolocalemiddleware",
+                tags=[f"path:{request.path}"],
+            )
+        else:
+            with normalized_get_language():
+                with simplified_check_for_language():
+                    return super().process_request(request)
 
     def process_response(self, request, response):
+        if is_root_path_with_no_language_clues(request):
+            # Skip using Django's LocaleMiddleware on the response cycle too
+            return response
+
         with normalized_get_language():
             with simplified_check_for_language():
                 return super().process_response(request, response)

bedrock/base/tests/test_middleware.py

@@ -3,6 +3,7 @@
 # file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
 from contextlib import suppress
+from unittest import mock
 
 from django.test import Client, TestCase
 from django.test.utils import override_settings
@@ -11,8 +12,12 @@
 import pytest
 from jinja2.exceptions import UndefinedError
 from markus.testing import MetricsMock
+from pytest_django.asserts import assertTemplateUsed
 
-from bedrock.base.middleware import BedrockLangCodeFixupMiddleware
+from bedrock.base.middleware import (
+    BedrockLangCodeFixupMiddleware,
+    BedrockLocaleMiddleware,
+)
 
 
 @override_settings(
@@ -167,7 +172,12 @@ def test_returns_500(self):
 @pytest.mark.parametrize(
     "request_path, expected_status_code, expected_dest, expected_request_locale",
     (
-        ("/", 302, "/en-US/", None),
+        (
+            "/",
+            302,
+            "/de/",  # because accept-language header has de as default lang,
+            None,
+        ),
         ("/en-us/", 302, "/en-US/", None),
         ("/en-US/", 200, None, "en-US"),
         ("/de/", 200, None, "de"),
@@ -210,7 +220,10 @@ def test_BedrockLangCodeFixupMiddleware(
     expected_request_locale,
     rf,
 ):
-    request = rf.get(request_path)
+    request = rf.get(
+        request_path,
+        HTTP_ACCEPT_LANGUAGE="de-DE,en-GB;q=0.4,en-US;q=0.2",
+    )
 
     middleware = BedrockLangCodeFixupMiddleware()
 
@@ -222,3 +235,33 @@ def test_BedrockLangCodeFixupMiddleware(
     else:
         # the request will have been annotated by the middleware
         assert request.locale == expected_request_locale
+
+
+@pytest.mark.django_db
+def test_BedrockLangCodeFixupMiddleware__no_lang_info_gets_locale_page__end_to_end(client):
+    """Quick end-to-end test confirming the custom 404-locale template is rendered
+    at the / path when there is no accept-language header"""
+
+    resp = client.get("/", follow=False)
+    assert "HTTP_ACCEPT_LANGUAGE" not in resp.request
+    assert resp.status_code == 200
+    # this template use actually happens in lib.l10n_utils.render
+    assertTemplateUsed(resp, "404-locale.html")
+
+
+@mock.patch("django.middleware.locale.LocaleMiddleware.process_request")
+@mock.patch("django.middleware.locale.LocaleMiddleware.process_response")
+def test_BedrockLocaleMiddleware_skips_super_call_if_path_is_for_root_and_has_no_lang_clues(
+    mock_django_localemiddleware_process_response,
+    mock_django_localemiddleware_process_request,
+    rf,
+):
+    fake_request = rf.get("/")
+    assert "HTTP_ACCEPT_LANGUAGE" not in fake_request
+    middleware = BedrockLocaleMiddleware(fake_request)
+    middleware.process_request(fake_request)
+    assert not mock_django_localemiddleware_process_request.called
+
+    fake_response = mock.Mock(name="fake response")
+    middleware.process_response(fake_request, fake_response)
+    assert not mock_django_localemiddleware_process_response.called

lib/l10n_utils/__init__.py

@@ -53,13 +53,13 @@ def render_to_string(template_name, context=None, request=None, using=None, ftl_
     return loader.render_to_string(template_name, context, request, using)
 
 
-def _is_root_path_with_no_language_clues(request):
+def is_root_path_with_no_language_clues(request):
     return request.path_info == "/" and not request.headers.get("Accept-Language")
 
 
 def redirect_to_best_locale(request, translations):
     # Strict only for the root URL when we have no language clues
-    strict = _is_root_path_with_no_language_clues(request)
+    strict = is_root_path_with_no_language_clues(request)
     # Note that translations is list of locale strings (eg ["en-GB", "ru", "fr"])
     locale = get_best_translation(translations, get_accept_languages(request), strict)
 
@@ -180,7 +180,7 @@ def render(request, template, context=None, ftl_files=None, activation_files=Non
 
         # Does that path's locale match the request's locale?
         # AND is it NOT for the root path with no discernable lang?
-        if locale in translations and not _is_root_path_with_no_language_clues(request):
+        if locale in translations and not is_root_path_with_no_language_clues(request):
             # Redirect to the locale if:
             # - The URL is the root path but is missing the trailing slash OR
             # - The locale isn't the current prefix in the URL