-
Notifications
You must be signed in to change notification settings - Fork 914
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
Conversation
3e5ef7b
to
5d5d4a0
Compare
Codecov ReportAttention: Patch coverage is
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. |
57db1c2
to
fe81795
Compare
cf8fc82
to
927434f
Compare
cd07e60
to
a2b030a
Compare
@@ -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")) |
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.
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)
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 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?
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 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)], |
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.
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)
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.
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?
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.
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
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.
Okay, that makes sense since bedrock is the one not using the default.
parser=bool, | ||
) | ||
|
||
if WAGTAIL_ENABLE_ADMIN: |
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.
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", |
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.
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)), | ||
) |
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.
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] |
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.
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. |
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.
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
…m we load the right strings and get the lang picker
…e with the matching lang code in the CMS
…d not be cached downstream
…f accessed directly
Co-authored-by: Ryan Johnson <escattone@gmail.com>
…/won't need them and why
a0ce292
to
539d15c
Compare
Low benefit to us, because they need CSP tweaks and also would need scrubbing before the DB export.
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 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 :)
56bddf0
to
289341f
Compare
…owing i18n/routing refactor
289341f
to
07b9e59
Compare
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.
r+. Big change. I dropped in some questions / suggestions. But overall 👍
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)], |
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.
Okay, that makes sense since bedrock is the one not using the default.
…agtail tables get made ASAP, preventing 500s
… it'll boot. We won't be using wagtail for Pocket mode
e15de08
to
8f1a027
Compare
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
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.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.wagtail-localize
), with bothen-US
andfr
available for now (just to test). This means that you can create a Wagtail page inen-US
and also generate afr
equivalent page. Our language selector in the footer will respect any languages enabled viawagtail-localize
and the picker will be displayed if there is an alternative translation of the current page available.ftl()
, so if your CMS content is French, the nav, footer, etc are shown in French, too.bedrock.cms
app, containing aSimpleRichTextPage
model that we can use to render very simple proof-of-concept pages. There is also aStructuralPage
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.It does not do the following
What will come next
I'd particularly like feedback on
Testing
.env
file, locallymake preflight
./manage.py createsuperuser
Welcome to your new Wagtail site
page+ Add Child Page
and add a new Simple Rich Text Page with any content you want. In thePromote
tab enter the slugmy-test-page
. Publish the page.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 madeSettings
andLocales
, thenAdd Locale
to ensure that French is available...
Actions menu. SelectTranslate this page
and pick French.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.Live
French page, which will be at http://localhost:8000/fr/test/, confirming the page furniture around the content is also in FrenchWelcome 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 typeStructural Page
. Call thisFirefox
and give it a slug offirefox
. (This won't clash with the existing, hard-coded/firefox/
path, because Django will find and match on that original one first.)....
action menu to Move it to be the child of your new Firefox structural page. On theMove
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.