Skip to content
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

Wagtail in Bedrock: main groundwork #14250

Merged
merged 25 commits into from
May 20, 2024
Merged

Wagtail in Bedrock: main groundwork #14250

merged 25 commits into from
May 20, 2024

Conversation

stevejalim
Copy link
Collaborator

@stevejalim stevejalim commented Feb 23, 2024

This changeset is the next significant step towards adding Wagtail into Bedrock.

Its focus is getting Wagtail into the codebase and making it work with our jinja2 templating, as well as using Wagtail's own L10N mechanisms alongside our Fluent-based L10N.

Significant points to note

It does the following

  • Adds Wagtail to our codebase, with the admin at /cms-admin/ if the WAGTAIL_ENABLE_ADMIN env-var is set. Pages managed by Wagtail (ie, in the wagtail DB tables) will always be available, if they have been published.
  • Ensures we use django_jinia for template rendering of our templates (while also allowing the Wagtail UI pages to be rendered using Django Template Language). Any HTML templates created for Wagtail pages should be written as Jinja templates, just like regular Django-powered Bedrock templates, and they will work. All our custom helpers, will be available as normal.
  • Enables Wagtail's l10n framework (wagtail-localize), with both en-US and fr available for now (just to test). This means that you can create a Wagtail page in en-US and also generate a fr equivalent page. Our language selector in the footer will respect any languages enabled via wagtail-localize and the picker will be displayed if there is an alternative translation of the current page available.
  • Supports Fluent strings, too. Wagtail templates can, naturally, extend Bedrock's base templates, most of which contain Fluent strings. This is fine. Things are set up so that the appropriate lang is used in the call to ftl(), so if your CMS content is French, the nav, footer, etc are shown in French, too.
  • Adds a new bedrock.cms app, containing a SimpleRichTextPage model that we can use to render very simple proof-of-concept pages. There is also a StructuralPage model that we can use to make "folders" that pages can live in, so that we can build out a URL path to be pretty much anything we want. More on that will follow, and the Testing section below will help explain it more, too.
  • Supports image uploads to Google Cloud Storage, if you have the appropriate credentials set up. Also works for local dev fine.

It does not do the following

  • It will NOT be possible to add ANY content-managed Wagtail pages to production Bedrock yet. This is partly because we default to having the Wagtail Admin disabled, but mostly because we are not allowing editing access to the main/core sqlite database on the clock pod. This aspect will all be changed in the future - see Option 2 here
  • There is no connection with a translation vendor yet - that work will come separately, via a custom integration.

What will come next

  • An RRA for Wagtail-in-Bedrock, scheduled for early May.
  • CSP tightening across Bedrock incl Wagtail admin
  • Tune the l10n activation threshold
  • Ensure unauthenticated pages do not set session cookies (they don't need them and it'll break our CDN)
  • Integration with nav (TBC)
  • Documentation of how to drive this - TBD based on how we decide to use Wagtail in more detail.
  • Infra updates: setting up an editing node behind SSO, moving to shared Postgres for that editing node, shipping a sqlite DB populated from the Postgres DB

I'd particularly like feedback on

  • Security/hole-poking. The Wagtail admin is inaccessible/unreachable by default, but we still use the Wagtail default serving view as our last URLpath. I think this is fine, but I'd welcome challenges to this viewpoint.
  • CSP: The Wagtail Admin's internal pages need some CSP allowances to make some parts work (but this is getting better). For now we use Bedrock's default CPS rules, which happen to be relaxed enough to not bother Wagtail, but this may soon change as we tighten it up. We can run separate CSP configs for the public pages vis the Wagtail admin. Thoughts welcome on this.
  • The overall experience, after testing. How did it feel? What was lumpy?

Testing

  • pull the branch
  • Add WAGTAIL_ENABLE_ADMIN=True to your .env file, locally
  • make preflight
  • ./manage.py createsuperuser
  • Go to http://localhost:8000/cms-admin/ and sign in.
  • We're going to make a simple standalone page. Click on the Welcome to your new Wagtail site page
  • Click + Add Child Page and add a new Simple Rich Text Page with any content you want. In the Promote tab enter the slug my-test-page. Publish the page.
  • Hit the Live button in the Status column of page you see next - it should take you to http://localhost:8000/en-US/test/ and you'll see the page you just made
  • Back in the admin at http://localhost:8000/cms-admin/, in the sidebar go to Settings and Locales, then Add Locale to ensure that French is available
  • Browse using the page explorer (I'll leave you to discover that) to the test page you made. Edit it and seek the ... Actions menu. Select Translate this page and pick French.
  • Use the Translate buttons to reveal fields through which you can add a French translation (or just "FR" before a copy-paste of the en-US text). Publish the page. Don't worry if it says the page is not fully translated - the slug can stay the same as en-US and that's fine.
  • View the Live French page, which will be at http://localhost:8000/fr/test/, confirming the page furniture around the content is also in French
  • Confirm the language switcher shows English as an alternative option, and that it works
  • Now let's move that page to be inside a section of the site
  • Go back to the Welcome to your new Wagtail site page (we'll rename this root page for real use another time). You might notice a French translation of this, too. Add a new Child page of the type Structural Page. Call this Firefox and give it a slug of firefox. (This won't clash with the existing, hard-coded /firefox/ path, because Django will find and match on that original one first.).
  • Edit your test page and use the ... action menu to Move it to be the child of your new Firefox structural page. On the Move page, look again for the ... action menu to be able to select the new parent for your test page. After moving, confirm that it is live at http://localhost:8000/en-US/firefox/test/ - this is how we could mix CMS-enabled pages in with hard-coded existing pages.
  • Note that the French translation did not move in the same way - the page trees are not rigidly in sync. You could repeat the above to move the French version too.

@stevejalim stevejalim force-pushed the wagtail-in-bedrock-redux branch 2 times, most recently from 3e5ef7b to 5d5d4a0 Compare February 26, 2024 10:17
Copy link

codecov bot commented Feb 26, 2024

Codecov Report

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

Project coverage is 76.61%. Comparing base (b1d5f85) to head (8f1a027).
Report is 6 commits behind head on main.

Files Patch % Lines
bedrock/settings/base.py 81.57% 7 Missing ⚠️
bedrock/urls/mozorg_mode.py 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14250      +/-   ##
==========================================
+ Coverage   76.44%   76.61%   +0.16%     
==========================================
  Files         148      150       +2     
  Lines        7987     8079      +92     
==========================================
+ Hits         6106     6190      +84     
- Misses       1881     1889       +8     

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

@stevejalim stevejalim force-pushed the wagtail-in-bedrock-redux branch 10 times, most recently from cf8fc82 to 927434f Compare April 26, 2024 09:22
@stevejalim stevejalim force-pushed the wagtail-in-bedrock-redux branch 2 times, most recently from cd07e60 to a2b030a Compare April 29, 2024 19:16
@stevejalim stevejalim changed the title WIP: Wagtail in Bedrock Wagtail in Bedrock: groundwork Apr 29, 2024
@@ -537,11 +549,14 @@ def language_url_map_with_fallbacks():
}

MEDIA_URL = config("MEDIA_URL", default="/user-media/")
MEDIA_ROOT = config("MEDIA_ROOT", default=path("media"))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: is changing this going to break something else that we already use MEDIA_URL for which isn't apparent from the codebase? Grepping around, I don't see anything that uses MEDIA_URL in Bedrock code, but that doesn't mean Django is/was not using it.

(Wagtail needs it to be able to serve image-library images, and it does so in a pretty standard-Django way)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was only defined because it was required by Django at some point and would break unless it was a real path, though I could be remembering wrong. So is your plan to have admin-uploaded media in a separate bucket from our current static media bucket? Are we not able to use a specific path in our existing media buckets?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to shake that out with SRE, but we could do either: isolate user-uploaded media, or go all-in. with the current config, if we use the same bucket, then the Wagtail-managed images and docs would go into s3/gcs://bedrock-media-bucket/user-media/ anyway I think, so there's no clash with what we add in to the root path

TEMPLATES = [
{
"BACKEND": "django_jinja.jinja2.Jinja2",
"APP_DIRS": True,
"APP_DIRS": False,
"DIRS": [f"bedrock/{name.split('.')[1]}/templates" for name in INSTALLED_APPS if _is_bedrock_custom_app(name)],
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the trick that lets us use django-jinja alongside regular Django templates. By turning off autodiscovery of apps' templates via APP_DIRS and instead specifying exactly what apps we want to use with django-jinja, we can let Wagtail use Django Template Language (DTL) (which it needs to work) without having to use DTL for our main Bedrock templates. Pages rendered from the CMS use jinja. It's only the templates used to render the Wagtail Admin itself that need DTL.

Also, I did try Django's own jinja2 support, but it wasn't nearly broad enough compared to django-jinja. (Mat from AMO also confirmed this and they are still on django-jinja rather than shipped-with-Django jinja2 support)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kinda curious if we can flip it so we have the Django template backend with APP_DIRS=False and specify the wagtail templates. Then let Jinja be the default everywhere else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably could, but I went with this way partly to be explicit that it was bedrock that needs a non-default template engine. That aside, it's a bit six-of-one-and-half-a-dozen-of-the-other.

I guess, in the future, if we drop in a third-party app (I can't think of a realistic example right now - maybe something related to l10n like django-rosetta?), I'd expect that to use DTL, so it would 'just work' with the config this way around, but that's not really a big justification

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, that makes sense since bedrock is the one not using the default.

parser=bool,
)

if WAGTAIL_ENABLE_ADMIN:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While all Wagtail-managed pages will be in the DB and therefore accessible all the time, we are making the ability to expose the Wagtail editing UI configurable. This is part of the bigger plan to have a secured "editing node"

"blockquote",
"link",
"ol",
"ul",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can tune this to include images, etc

# Note that we're also using localised URLs here
urlpatterns += bedrock_i18n_patterns(
path("", include(wagtail_urls)),
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is important: Wagtail has one main view: wagtail.views.serve which must (via this wagtail_urls urls module) come at the end of all URLs, and does internal pattern matching based on the slug of a page (eg 'vpn-tips') combined with the slugs of any ancestors (eg 'vpn' <- 'products')

As such, wagtail_url will also raise a 404 if no match is found, so any custom 404 handling needs to bear this in mind. I had to minorly refactor how we do the special careers 404 page (which mentions if a job listing has disappeared) in light of this.

@@ -127,6 +130,9 @@ def render(request, template, context=None, ftl_files=None, activation_files=Non
name_prefix = request.path_info.split("/", 2)[1]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading the render() method in full might be easier than just looking at the changed lines here

@@ -147,9 +153,13 @@ def render(request, template, context=None, ftl_files=None, activation_files=Non
context["template"] = template
context["template_source_url"] = template_source_url(template)

# if it's a CMS page, draw the active locales from the Page data.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For localised CMS pages, we're considering 'active' to be anything that has a translation and not (yet) considering how much of a version has actually been translated. That is on the list to add later

@stevejalim stevejalim added WIP 🚧 Pull request is still work in progress and removed Do Not Merge ⚠️ labels Apr 29, 2024
@stevejalim stevejalim changed the title Wagtail in Bedrock: groundwork Wagtail in Bedrock: main groundwork Apr 29, 2024
Low benefit to us, because they need CSP tweaks and also would need scrubbing before the DB export.
Copy link
Member

@alexgibson alexgibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked through the code but I did take this for a test drive locally, and the everything worked well (thanks for the detailed instructions!). I did find moving the test page a little tricky since the /firefox/ page didn't appear in the list initially, but did when I searched for it. This is probably something specific to the wagtail UI and me being new to it. So r+ still :)

Copy link
Member

@robhudson robhudson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r+. Big change. I dropped in some questions / suggestions. But overall 👍

bedrock/cms/tests/test_models.py Show resolved Hide resolved
bedrock/cms/tests/test_rendering.py Show resolved Hide resolved
bedrock/products/tests/test_views.py Outdated Show resolved Hide resolved
TEMPLATES = [
{
"BACKEND": "django_jinja.jinja2.Jinja2",
"APP_DIRS": True,
"APP_DIRS": False,
"DIRS": [f"bedrock/{name.split('.')[1]}/templates" for name in INSTALLED_APPS if _is_bedrock_custom_app(name)],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, that makes sense since bedrock is the one not using the default.

@stevejalim stevejalim merged commit 920d418 into main May 20, 2024
13 checks passed
@stevejalim stevejalim deleted the wagtail-in-bedrock-redux branch May 20, 2024 09:55
@deployment-status-mozilla deployment-status-mozilla bot temporarily deployed to test May 21, 2024 19:50 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review: M Code review time: 1-2 hours
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants