Story 2096: Implement Wagtail Integration#2100
Conversation
jlchilders11
changed the title
gregjkal
left a comment
gregjkal
left a comment
There was a problem hiding this comment.
Per our conversation in standup: please test the homepage to make sure it won't break without manually migrating the existing testimonial and/or news items. If it does break, we need to either make these changes backwards-compatible (my strong preference!), with a follow-up change that cleans any bridge code up, or add a data migration so it can be fixed during the deploy instead of crashing until manual fixes.
julioest
changed the title
julhoang
left a comment
julhoang
left a comment
There was a problem hiding this comment.
Hi @jlchilders11 ! This is awesome work! I just left a few of clarifying questions below.
Besides them, it looks like the HTML templates are now outdated. Do you have a next-step recommendation for us to update those templates to V3 versions? Also, do we need to update the V3 Create Post page and various other existing queries to query from Wagtail's PostPage instead of Entry?
Thanks so much for your work here!
There was a problem hiding this comment.
Is it the template for http://localhost:8000/pages/ ? It seems like this page just stay blank – would you mind helping me understand why we need this?
There was a problem hiding this comment.
The current intent is that the "pages" home page is mostly a placeholder for organizational purposes. I believe this returns a blank page due to discussion when this was being made originally, where it was decided that a blank page looked "less wrong" than a 404.
| + static(settings.MEDIA_URL, document_root=settings.MEDIA_ROOT) | ||
| + [ | ||
| path("outreach/", include(wagtail_urls)), | ||
| path("pages/", include(wagtail_urls)), |
There was a problem hiding this comment.
Would you mind helping me understand why we need to route the /news path via /pages with Wagtail, instead of being able to navigate to it directly? It seems like we can navigate to http://localhost:8000/outreach/ directly, is it not possible to do the same with /news? Or is the idea here to have Wagtail lives on a separate prefix for development phase til we are ready to replace the legacy path? 🤔
There was a problem hiding this comment.
Back when this was being made, the idea was to let these exist in a parallel, and eventually move all Wagtail pages under pages for clarity sake. I'm not sure that that paradigm is still relevant, but I do think that future wagtail pages should live under the pages endpoint.
| class FlaggedMixin(Page): | ||
| def serve(self, request, *args, **kwargs): | ||
| if not waffle.flag_is_active(request, "v3"): | ||
| return HttpResponseForbidden("You do not have access to this page.") | ||
| return super().serve(request, *args, **kwargs) | ||
|
|
||
| class Meta: | ||
| abstract = True |
There was a problem hiding this comment.
Would it work if we use V3Mixin directly? 🤔
If not, can we return 404 when user doesn't have permission, so that they won't have a hint that this page exists?
There was a problem hiding this comment.
So this needs the serve method overwritten, but I agree that this logic can be centralized to the v3mixin.
jlchilders11
force-pushed
the
jc/wagtail-integration
branch
from
a984bd7 to
bd274ee
Compare
For updating these to the V3 and existing things, I believe that should be handled in the data integration phase of those pages, which I believe are still outstanding tickets. If not, I do think that that work should be handled as part of a separate ticket, as it somewhat escapes the "migration" scope of this ticket. |
herzog0
left a comment
herzog0
left a comment
There was a problem hiding this comment.
Hey @jlchilders11 this is looking great!
I just have a couple of questions that I'll write in a Slack thread to keep it as a better reference for the future
| page = PostPage( | ||
| title=entry.title, | ||
| first_published_at=entry.publish_at, | ||
| owner=entry.author, |
There was a problem hiding this comment.
Do we also have control over the Updated field?
I think it would be good to populate that as well, so we can order the posts in wagtail as they have been published in the past. I mean, it'd be nice if the order in Wagtail reflected the order in the news/ page.
5 participants
Implement Wagtail Integrations:
Mixins
TaggableMixin- Adds the ability to tag pages for categorizationRouting
RoutableHomePage- New home page for all wagtail interactions that allows for forwarding routing to child pages, e.g. if you get to wagtail through/outreach/, you will be routed through theOutreachHomePagePosts
This is the bulk of the work, and is a porting of the existing
EntryModel to WagtailPostIndexPage- Index Page for Posts, implements most of the logic in formatting found in thenews.views.EntryListView. NOTE: this uses queryparams for filtering the child pages, rather than seperate URLsPostPage- Functional translation ofEntry, but uses a stream field rather than having separate models for different types of content. These can be filtered by the content of the stream field blockNOTES:
DEPLOYMENT NOTES:
When deploying this, a
RoutableHomePageshould be created and the existingOutreachHomePageshould be moved under it to respect the new routing methods.