Provide a hybrid list of locales drawn from CMS-backed and Django-backed sources

[stevejalim]

One-line summary

This changeset ensures that the language picker (and canonical-url.html header partial) remains accurate when a page is partly in Wagtail CMS and partly rendered via Django views.

• I used an AI to write some of this code.

Significant changes and points to review

This piece of work was like pulling at a loose thread on a sweater. It may look like there's a lot going on here, so I'll add notes on the Github diff where I think they'll help.

The prefer_cms decorator is applied to a regular Django view and will try to load the decorated URL path from the CMS first, then fall back to the original Django view. Previously, when a page came from the CMS, we would set the translations context variable to be a list of locales available in the CMS only, which had the downside that the language picker in the footer didn't mention the locales that were still available via the regular Django view. (The inverse was also true: if you reached a Django-rendered page that was decorated with prefer_cms, it would not show the locales available in the CMS because it didn't have awareness of them.

I tried different ways to get around this, including patching translations in lib.l10n_utils.render (which ended very badly as translations is used for more than just the footer picker, such as the logic around deciding whether to redirect to the default en-US locale.)

The approach that worked better was to, via the prefer_cms decorator, annotate the request with two lists of locale language codes: the locales available from the CMS and those available from the Django view. Then, we use a new Jinja helper to pull together these two lists for use in the language picker (we also use the same helper to provide correct locale lists for the canonical URL links in the header).

(Note that this logic only runs on views decorated with prefer_cms - otherwise we still use the regular translations context var for such things.)

To make this work the prefer_cms decorator needs to be told what locales are available in the regular Django view - it was not viable to introspect the view to get hold of the relevant locales. There are three (!) ways to pass this information in:

1. Pass in the same Fluent files that are used in the view – this is good for views that are HTML templates and Fluent strings only. We work out the relevant locales based on the same get_active_locales()logic as our mainl10n_utils.render()` function.
2. Pass in a list of language codes for the locales - this is good for views that might have content from an external source (eg a git repo) but which we know will be consistentely and completely translated into all of those locales
3. Pass in a callable function or method that will return the appropriate locales for the Django-rendered view. This is best for views with a single URL featuring a slug variable, where some pages are widely localized, but not all of them are, so we can't reliably used a fixed list of locales. Things like the old VPN Resource Center articles are a good example of this, and *this changeset uses the callable pattern to make the language picker work for the VPN RC

Issue / Bugzilla link

Resolves #15193

Testing

• Use the VPN RC as the test for this. (See Testing section on Migrate VPN resource center to Wagtail (#14860) #15236 if you need to set one up locally)
• Add new CMS pages to the VPN RC using slugs that exist for the Django view in more than just en-US (e.g. what-is-a-vpn and the-difference-between-a-vpn-and-a-web-proxy) - they can just have lorem ipsum content locally - the URL loath is what we are really need here
• Publish the pages and view them.
• Edit bedrock/product.urls
• The footer's language picker should allow you to switch between any of the 10 languages available for those pages regardless of whether they are in the CMS or in Django, with no duplicated locales mentioned and never a 404 or redirect back to the en-US version
• The canonical url links in the head of the pages should also reflect the other locales available.
• Note that the the VPN RC index page in the CMS currently does not mix Django and CMS-based content - that change is out of scope of this fix, which is focused only on the footer lang switcher

[codecov]

Codecov Report

Attention: Patch coverage is 94.59459% with 4 lines in your changes missing coverage. Please review.

Project coverage is 78.78%. Comparing base (d71a2e3) to head (9397fb6).

Files with missing lines	Patch %	Lines
bedrock/cms/utils.py	87.09%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15475      +/-   ##
==========================================
+ Coverage   78.71%   78.78%   +0.06%     
==========================================
  Files         156      157       +1     
  Lines        8197     8252      +55     
==========================================
+ Hits         6452     6501      +49     
- Misses       1745     1751       +6     

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

[stevejalim]

note: We should keep an eye on the refresh stuff in general to ensure we don't get regressions

[stevejalim]

note: I poured a lot of time into trying to work out why the page tree did contain this grandchild-page, when it's clear it's there from the fixture and if you ask for it's url_path in the debugger. Talking with core Wagtail people too, didn't seem to reveal what was up. Tested manually, though, so I'm OK that this is a false negative. Will be nice to work out why, though. Hopefully it's not a blocker to an r+

[stevejalim]

note (non-blocking): The fallback_callable approach was a late addition in order to make the VPN RC (with its inconsistent Django-side db-backed localization) work -- it's quite an outlier of a view for Bedrock.

It may be that the fallback_callable and the fallback_ftl_files are actually the only two approaches we need and we ca drop fallback_lang_codes, because they can be generated from a callable - let's see as we use prefer_cms more

[stevejalim]

note: I know that annotating the request is not a super-clean way to do things, but I've tried to keep it sensible with "private" attributes.

There's a performance impact here, because we will hit the DB more for views decorated with prefer_cms, but by its nature a CMS-related view will hit the DB anyway. We could probably optmise with some prefetching if we need to, but I'm hopeful that we won't need to add to the complexity here even more.

Equally, if you can think of a simpler or cleaner approach, I'm definitely happy to hear it!

[alexgibson]

I tested this out locally and both the page language picker and canonical URLs all seemed to work as expected for the VPN resource center index page, and child pages when they exist in the CMS. Great work!

Whilst I'll leave the more in depth Python code review to Rob, the overall approach here seems well thought out, and should give us the flexibility we need to handle all the use cases I can think of for the future.

I noticed there are a couple of comments around one or two tests that don't quite pass as expected, so I do wonder if we should perhaps try and figure out the root cause for those still. But perhaps they are edge cases, that likely exist in Wagtail as opposed to the changes here?

[stevejalim]

@alexgibson Thanks for the r+

But perhaps they are edge cases, that likely exist in Wagtail as opposed to the changes here?

I'm confident that they're either a quirk in Wagtail (the same way that when we create translations sometimes we need to run fixtree) or - perhaps more likely - a niggle with the test data set up in the fixture.

[robhudson]

I'd be curious if there's a SQL way to do this? Looks like this might hit the db for the list comprehension separately. Which is probably fine... sometimes successive small simple SQL queries can be better than one larger complex SQL query.

[stevejalim]

Fair point about hitting the DB twice there. I'll refactor to use Subquery so we can keep it all ORM

[stevejalim]

@robhudson will 9397fb6 get me an r+? 🤞

[robhudson]

Oops, sorry, thought I approved with comments. I'll take a quick look over again and approve.

[robhudson]

tricky stuff, nice work