Switch Bedrock to using Django's own i18n mechanism #14327

stevejalim · 2024-03-18T21:21:17Z

Please do not merge - let @stevejalim do it at the ideal point

One-line summary

Along with SUMO and other sites, Bedrock has historically used an in-house internationalization (i18n) mechanism to detect/select the appropriate language for a user and to 'activate' the relevant language for translation, so that the localization (l10n) logic can pick the appropriate Fluent files, etc, to render a page in the language expected by the user.

This has worked fine for a long time, but the time has come to change Bedrock to instead use Django's core i18n machinery. Why? We'll soon be using Wagtail CMS within Bedrock and Wagtail's own l10n system (wagtail-localize) is build to need Django's i18n machinery. (We've tried it with ours and it just doesn't work.) Also, the closer Bedrock is to 'vanilla Django' the better (though even after this PR lands, there's plenty that's special about Bedrock, such as our Fluent l10n mechanism, which is not removed or changed as part of this work).

Significant changes and points to review

There's a lot changed here. We've dropped our custom prefixer and the urlresolver that used it, switching to core django behaviour using i18n_patterns to generate language-code-prefixed URL routes, combined with some custom middleware that does things like ensuring we retain the mixed-case langauge codes of our original system, as well as handle some corner cases such as our special locale-selection root page that only robots see.

Please don't hold back with questions when reviewing this code - I've learned a lot about our and Django's internals along the way, and it may be that there are better ways to do some things.

I'm adding a QA checklist below, which I'd value each reviewer ticking the relevant parts of as they test things out.

Issue / Bugzilla link

Resolves #14256

Testing

In a nutshell, nothing should be operationally different compared to production Bedrock. Please take some time to explore both the Mozilla and Pocket sites on the URLs below, switching between languages and poking at areas that you understand well - eg attribution, analytics, locale-selection, downloads, etc

Also, please can y'all double-check these, which I've tested as part of this work, but can't be too careful. Please tick off any you test

codecov · 2024-03-18T21:46:10Z

Codecov Report

Attention: Patch coverage is 97.26027% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 76.45%. Comparing base (b8ee881) to head (1d3b288).

❗ Current head 1d3b288 differs from pull request most recent head f968cb9. Consider uploading reports for the commit f968cb9 to get more accurate results

Files	Patch %	Lines
bedrock/settings/__init__.py	0.00%	2 Missing ⚠️
bedrock/urls/mozorg_mode.py	60.00%	2 Missing ⚠️
bedrock/urls/pocket_mode.py	75.00%	1 Missing ⚠️
lib/l10n_utils/translation.py	94.11%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #14327      +/-   ##
==========================================
+ Coverage   75.72%   76.45%   +0.72%     
==========================================
  Files         145      147       +2     
  Lines        7878     7971      +93     
==========================================
+ Hits         5966     6094     +128     
+ Misses       1912     1877      -35

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

stevejalim · 2024-03-25T18:03:18Z

lib/l10n_utils/translation.py

-        return locale
-    else:
-        return f"{parts[0]}-{parts[1].upper()}"
+def _fix_case(lang_code):


note: This could maybe be merged with bedrock.base.i18n.normalize_langauge (which does more than this function) as long as we add a flag/option to limit behaviour to be the same as _fix_case.

I'm in support of that. The less code we have that does similar things the easier to understand.

stevejalim · 2024-03-25T18:04:35Z

tests/redirects/map_globalconf.py

@@ -187,7 +187,7 @@
        url_test("/{m,{firefox/,}mobile}/faq/", "https://support.mozilla.org/products/mobile"),
        # bug 885799, 952429
        url_test("/projects/calendar/holidays.html", "https://www.thunderbird.net/calendar/holidays/"),
-        url_test("/en-US/projects/calendar/random/stuff/", "https://www.thunderbird.net/calendar/"),
+        url_test("/projects/calendar/random/stuff/", "https://www.thunderbird.net/calendar/"),


note: This is the only test I had to hack to make it fit. I think it's OK, particularly given the lower proportion of visits it probably gets

stevejalim · 2024-03-25T18:07:18Z

bedrock/base/i18n.py

+    if path.lstrip("/").split("/")[0] in settings.SUPPORTED_NONLOCALES:
+        return False
+
+    return True


note: It would be nice to make this dynamically check the mozorg nonlocale_urls.py (and to move any other similar routes into a relevant nonlocale_urls.py file.

Note: this comment will make more sense after you've seen the changes to urls/mozorg_mode.py

stevejalim · 2024-03-25T18:07:36Z

bedrock/base/i18n.py

+            LocalePrefixPattern(prefix_default_language=prefix_default_language),
+            list(urls),
+        )
+    ]


note: This is also how SUMO does it

stevejalim · 2024-03-25T18:09:49Z

bedrock/base/i18n.py

+    # them to be for our lookup
+    lang, _partition, classifier = language.partition("-")
+    lang = lang.lower()  # this part is _always_ lowercase
+


note: What follows could be refactored into something more easily readable, but waiting on feedback. Supporting the special cases could at least be moved into a small helper func

stevejalim · 2024-03-25T18:11:12Z

bedrock/base/i18n.py

+    Return a locale code we support on the site using the
+    user's Accept-Language header to determine which is best. This
+    mostly follows the RFCs but read bug 439568 for details.
+    """


note: (to self): reading this now, maybe we can just use Django's own helpers for this

stevejalim · 2024-03-25T18:12:47Z

bedrock/base/middleware.py

+    2) If no redirect is needed, sets request.locale to be the normalized
+    lang code we've got from the URL
+
+    Querystrings are preserved in GET redirects.


nitpick (non-blocking): This docstring should probably also mention the support for querystring-based language switching

stevejalim · 2024-03-25T18:13:55Z

bedrock/base/middleware.py

+        with normalized_get_language():
+            with simplified_check_for_language():
+                return super().process_response(request, response)
+


praise: Credit for the patcing approach goes to @escattone, who worked out how to do it for SUMO

bedrock/base/middleware.py

alexgibson · 2024-03-26T13:33:55Z

robhudson

Lots of good stuff here. I want to re-read when I have a chance and now drowning in Braze work. I should have more time tomorrow.

bedrock/base/middleware.py

bedrock/mozorg/tests/__init__.py

robhudson · 2024-03-28T00:26:31Z

lib/l10n_utils/translation.py

-        return locale
-    else:
-        return f"{parts[0]}-{parts[1].upper()}"
+def _fix_case(lang_code):


I'm in support of that. The less code we have that does similar things the easier to understand.

robhudson

Nice work on a difficult change.

bedrock/base/i18n.py

robhudson · 2024-04-09T21:47:37Z

bedrock/base/middleware.py

+        # Handle the non-JS language selection as a priority. Once we're switched
+        # the subsequent checks will clean up things more, if needed.
+        # Fixes https://github.com/mozilla/bedrock/issues/5931
+        lang_via_querystring = request.GET.get("lang", None)


Would None be the default here, assuming request.GET quacks like a dict?

Interestingly, it doesn't quite quack like that. Querydict.get works the same way as QueryDict.__getitem__ which will blow up if no default is passed (docs)

bedrock/base/middleware.py

bedrock/base/urlresolvers.py

bin/run-tests.sh

stevejalim · 2024-04-11T15:03:01Z

Thank you @robhudson and @alexgibson for the reviews. I've rebased on recent mains, including the change to Django 4.2 and we're still looking good, so I'm hoping to merge this as soon as we're back after the weekend

a-kyne · 2024-04-16T17:25:46Z

I was able to check a few things and did not spot any problems:
Mozorg mode on https://www-demo7.allizom.org/
- [X] Check the redirect from all lower case country codes to uppercase (example: en-us will redirect automatically to en-US. Ideally check for all settings.PROD_LANGUAGES
- [ ] Check that explicit fallbacks/canonical languages are still in place (example: ja-JP-mac -> 'jp'; sv -> sv-SE; zh-HK -> zh-TW
- [X] Check that the language-switcher in the footer works, with and without JS
- [X] Check that the Accept-Language header value (ie Browser language) is successfully respected and Bedrock uses the given lang code if there is none in the path
- [X] Also check that picking a specific lang via the path (eg /de/about/) will get a German page, even if the Browser default language is set to something other than German
- [ ] Confirm that three-letter lang codes (eg sco) work fine
[X] Check special 404 Locale picker page is served at / when there is no accept-language header - @robhudson

  [X] Check Contentful-sourced pages are OK - /products/vpn/resource-center/
    Check WP-blog-sourced pages are OK - eg downpage on /careers there should be some blog posts pulled in

 [x] Check sitemaps are happy at /sitemap.xml and /{lang-code}/sitemap.xml

…ted lang-code lookups

…zorg.urls This includes splitting mozorg.urls into two groups so we only use i18n_patterns with paths that need it. We've kept the main, localed, group's name to be urls.py to reduce the chance of merge conflicts, as this piece of work is likely to need rebasing against main quite a lot until it's ready. The decision about which get prefixed and which do not is based on the settings SUPPORTED_NONLOCALES and SUPPORTED_LOCALE_IGNORE that were used in our former i18n machinery. At the time of committing, they still exist, but both of should be able to be removed once this work is done (or at least moved to power some tests)

…ostic

…gnostic

… tweak _one_ test related to this

…ector - we don't need it unless we explicitly ask for it

* Contentful-related tests needed the dev-urls config wrapping in URLpatterns to make it work as per the real site; if we just specced dev_urls they did not work because they were not i18n-patterned

…ow up and the former matches original behaviour better

Fixes #5931 See #5931 for context

…ng place for us By default check_for_language() seeks gettext-compatible files in locale/ directories in the project, but we don't have those because our l10n is handled using Fluent strings in the www-l10n-team directory, cloned from their git repo. So, instead, we just perform a light check to see whether a given lang is among the langs that shold be available. The actual decision about whether we have have (enough) Fluent strings available for that locale happens in lib.l10n_utils.render()

…n't to _too_ much

…ning

…nt route to making check_for_language work; this was the wrong direction

…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)

… split_path_and_polish_lang

stevejalim added the Do Not Merge ⚠️ label Mar 18, 2024

stevejalim force-pushed the 14256-refactor-i18n-mechanism branch from f42c61f to 1d68364 Compare March 18, 2024 21:38

stevejalim force-pushed the 14256-refactor-i18n-mechanism branch 7 times, most recently from 7617bf1 to 8a19328 Compare March 24, 2024 12:50

stevejalim added the Review: M Code review time: 1-2 hours label Mar 25, 2024

stevejalim changed the title ~~WIP: Refactor i18n mechanism~~ Switch Bedrock to using Django's own i18n mechanism, not our own one Mar 25, 2024

stevejalim changed the title ~~Switch Bedrock to using Django's own i18n mechanism, not our own one~~ Switch Bedrock to using Django's own i18n mechanism Mar 25, 2024

stevejalim commented Mar 25, 2024

View reviewed changes

stevejalim force-pushed the 14256-refactor-i18n-mechanism branch from 8a19328 to 72c281e Compare March 25, 2024 18:26

stevejalim marked this pull request as ready for review March 25, 2024 19:18

stevejalim requested review from robhudson and pmac March 25, 2024 19:18

robhudson reviewed Mar 28, 2024

View reviewed changes

stevejalim force-pushed the 14256-refactor-i18n-mechanism branch from 72c281e to 95c5451 Compare April 3, 2024 14:01

robhudson approved these changes Apr 9, 2024

View reviewed changes

stevejalim force-pushed the 14256-refactor-i18n-mechanism branch from 95c5451 to 342335f Compare April 10, 2024 20:14

stevejalim force-pushed the 14256-refactor-i18n-mechanism branch 2 times, most recently from 2883809 to 1d3b288 Compare April 16, 2024 09:08

stevejalim added 3 commits April 17, 2024 14:08

[Breaking] Drop our custom locale middleware and switch to Django's

590bb46

[Breaking] Allow mixed-case lang codes as settings.LANGUAGES and rela…

1c2d4f5

…ted lang-code lookups

stevejalim added 25 commits April 17, 2024 14:08

Add tests for i18n helpers

e972c9c

Improve language normalization + tests

8736ecc

Move careers redirects to own file, so that they remain lang-code agn…

02f17e6

…ostic

Move foundation redirect out of urls.py so that it remains language a…

5765ec8

…gnostic

Move redirects out of urls.py so that they remain language agnostic -…

c3c4ed0

… tweak _one_ test related to this

Avoid unnecessarily adding lang code to a reversed() url in our redir…

3baf110

…ector - we don't need it unless we explicitly ask for it

Fix lang codes for CANONICAL_LANGS for mozorg mode

8f19837

Tidy up tests

e7773e6

Fixing tests with settings=DEV

8b3771d

* Contentful-related tests needed the dev-urls config wrapping in URLpatterns to make it work as per the real site; if we just specced dev_urls they did not work because they were not i18n-patterned

Default a locale to '' not None - the latter causes Locale page to bl…

3be47ce

…ow up and the former matches original behaviour better

Ensure no-JS language changing works

e26dc61

Fixes #5931 See #5931 for context

Formatting fix after rebasing on main

fa03b0b

Rename our get_language helper to make it clearer what it does

f057e62

Expand test coverage of our custom prefixer

0fe343d

Bring bedrock.base.i18n up to 100% coverage

429a9fa

Add tests for language-code fixup + slight refactor to ensure it does…

609eae3

…n't to _too_ much

Fix failing startup check for Pocket mode that blocked everything run…

1c2f119

…ning

Drop unnecessary-after-all patching of LANG_INFO - we found a differe…

646bfbf

…nt route to making check_for_language work; this was the wrong direction

Add a little extra logging to the language-fixup redirects we're doing

8ea97e3

Minor test polishing

85c2694

Improve naming to avoid confusion between polish (verb) and Polish in…

53f94e2

… split_path_and_polish_lang

Improve in-code docs for BedrockLangCodeFixupMiddleware

19b5aad

Test tweak after rebasing on main (which now has Django 4.2)

f968cb9

stevejalim force-pushed the 14256-refactor-i18n-mechanism branch from 1d3b288 to f968cb9 Compare April 17, 2024 13:08

stevejalim removed the Do Not Merge ⚠️ label Apr 17, 2024

stevejalim merged commit 66ce747 into main Apr 17, 2024
9 checks passed

stevejalim deleted the 14256-refactor-i18n-mechanism branch April 17, 2024 15:16

janbrasna mentioned this pull request Apr 22, 2024

Drop already removed nonlocales #14487

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch Bedrock to using Django's own i18n mechanism #14327

Switch Bedrock to using Django's own i18n mechanism #14327

stevejalim commented Mar 18, 2024 •

edited

codecov bot commented Mar 18, 2024 •

edited

stevejalim Mar 25, 2024

robhudson Mar 28, 2024

stevejalim Mar 25, 2024

stevejalim Mar 25, 2024

stevejalim Mar 25, 2024

stevejalim Mar 25, 2024

stevejalim Mar 25, 2024

stevejalim Mar 25, 2024

stevejalim Mar 25, 2024

alexgibson commented Mar 26, 2024 •

edited

robhudson left a comment

robhudson Mar 28, 2024

robhudson left a comment

robhudson Apr 9, 2024

stevejalim Apr 11, 2024

stevejalim commented Apr 11, 2024

a-kyne commented Apr 16, 2024

Switch Bedrock to using Django's own i18n mechanism #14327

Switch Bedrock to using Django's own i18n mechanism #14327

Conversation

stevejalim commented Mar 18, 2024 • edited

One-line summary

Significant changes and points to review

Issue / Bugzilla link

Testing

codecov bot commented Mar 18, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexgibson commented Mar 26, 2024 • edited

robhudson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robhudson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevejalim commented Apr 11, 2024

a-kyne commented Apr 16, 2024

stevejalim commented Mar 18, 2024 •

edited

codecov bot commented Mar 18, 2024 •

edited

alexgibson commented Mar 26, 2024 •

edited