Allow editing menu items from Wagtail (#795)

[blms]

Associated Issue(s): #795

Changes in this PR

• Use content pages' configured names and taglines from Wagtail in the menu
• Also allow users to configure Members and Books (Django views) titles and taglines from Wagtail, in the home page edit view

Notes

• I wasn't sure if we actually wanted "Members" and "Books" and their taglines to be edited from within Wagtail, so I did that work in a separate commit, to easily extract if need be.
  • On that note, not sure if the home page edit view is the best place to set them? Seemed like the simplest.
• The homepage tagline was already editable from Wagtail, it's the homepage -> Promote -> "Meta description" field.
• No clue why those two unit tests were suddenly failing, maybe because the templates have changed? Seems like they should have failed before since None should not have shown up in the templates. (they were failing on things like: work.year should be in the template when it's None)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.81%. Comparing base (6acf9e5) to head (318d2fe).
Report is 13 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #853      +/-   ##
===========================================
+ Coverage    97.43%   97.81%   +0.38%     
===========================================
  Files          236      225      -11     
  Lines        13475    13070     -405     
  Branches        79        0      -79     
===========================================
- Hits         13129    12785     -344     
+ Misses         346      285      -61     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[rlskoeser]

Looks reasonable to me. Thanks for setting the current values as defaults in the model and migration.

Wonder if it might be useful to have a comment or note in the templates, but probably readable enough.

[blms]

@rlskoeser Working on this, it doesn't quite seem right to configure these Django pages the same way as the Wagtail ones, but maybe I'm wrong. The existing configurable pages are both landing pages and groupings of Wagtail content. Thus, they are configured by editing the LandingPage models in Wagtail's Pages section:

But the two Django pages aren't containers of Wagtail content, and it seems a bit odd to put them in the same place with the content. Snippets does seem like the way to go for those two. Here's what it would be like with snippets now:

But maybe it would be better for users for us to make a new LandingPage subclass instead? I could see it being a bit confusing that it happens in two different places. Ultimately, they'll live side by side in menus:

[blms]

@rlskoeser Also: The thing I was talking about in standup is the dropdown in the LandingPageSetting wagtail form with "Members", the only other option is "Books" right now and they have a unique constraint. So there should be exactly one LandingPageSetting instance each.

The one danger in that approach is that they could theoretically be deleted, but it wouldn't be hard to recreate them. I think that would be a danger with LandingPage subclass models too, the only approach that would avoid this is using wagtail Settings, and/or setting fallbacks.

[rlskoeser]

@blms here's where we used the AbstractLinkPage from the wagtailmenus package in a previous version of the CDH website:

https://github.com/Princeton-CDH/cdh-web/blob/3.5.3/cdhweb/pages/models.py#L191C16-L205C1

It looks like wagtailmenus is still active, seems fine to add that as a dependency so we can use the abstract link page class.

[rlskoeser]

Documenting the change in approach that we've discussed and decided elsewhere:

• use link pages in wagtail menus to control taglines and menu placement/inclusion/order
• add taglines to landing pages

Additionally, I'm fairly certain you should be able to rely on the context wagtail provides rather than the context processor line you added. If not, it should be added in a wagtail context processor (or whatever the wagtail equivalent is) rather than the settings context.

[rlskoeser]

I don't think lookup by title is reliable, can we use slug at least if we have to do a lookup?

[rlskoeser]

This isn't the right place to pass this in. I'm pretty sure there's a more wagtail-centric way to access snippets and pages - you may already have access to some of this in templates already. If you have trouble finding this quickly, let me know and I'll see if I can find an example in one of other projects.

[blms]

@rlskoeser For migrating to the AbstractLinkPage:

• Should we allow picking internal Wagtail links too, or only allow entering a URL? My assumption is the latter but wanted to be sure.
• Do we need the "Promote --> Slug" logic from the CDH website implementation? I'm thinking we don't need it and can just use the existing URL field on the model. (in fact omitting all link fields on the model requires overriding validation)
  • Edit: actually, maybe we need the slug too in order to match the correct Page for inclusion on the landing page headers (i.e. not in menus)
• Should I override TItle to not be a required field and remove it from the form? I'm thinking it's fine to let people customize it since it's part of the abstract model already, but either way is fine. (though not sure how it would display in the wagtail cms for editing if it has no title 🤔, maybe a __str__ override?)

Here's how it might look if my thinking is correct:

[rlskoeser]

@rlskoeser For migrating to the AbstractLinkPage:

• Should we allow picking internal Wagtail links too, or only allow entering a URL? My assumption is the latter but wanted to be sure.

Let's limit to the urls since we really only want this to be used for books or members list pages.

• Do we need the "Promote --> Slug" logic from the CDH website implementation? I'm thinking we don't need it and can just use the existing URL field on the model. (in fact omitting all link fields on the model requires overriding validation)

  • Edit: actually, maybe we need the slug too in order to match the correct Page for inclusion on the landing page headers (i.e. not in menus)

I don't remember this level of detail - defer to you. Would be nice if we don't need it, and might be possible to match on the url, but it's fine if slug is easier.

• Should I override TItle to not be a required field and remove it from the form? I'm thinking it's fine to let people customize it since it's part of the abstract model already, but either way is fine. (though not sure how it would display in the wagtail cms for editing if it has no title 🤔, maybe a __str__ override?)

Probably need a title in menus and wagtail interface, so maybe this should be the canonical place for the page title — the default can be used as a fallback in templates if needed.

[blms]

@rlskoeser Looks like github actions still aren't running, let me know once the billing issues are resolved and I can re-run. All tests are passing locally. Github actions are working again now.

Notes:

• I ended up updating the MembersList and WorkList views' get_context_data to include the page title from Wagtail and handle the default fallback to the page_title property.
  • Rationale: I couldn't find a way to access a single Page object outside of a loop on a regular django non-Wagtail page's template. I tried a loop through menu items to find it, but that seems to prevent defining a fallback (since we can't easily write a conditional for the presence of the page in the loop). I could use the approach from my previous iteration where I include them as a dict or something (but in Wagtail template context instead of global template context), but this seems cleaner to me, since we're now only using each LinkPage directly once.
• Because these now have to be wagtail pages that are children of the root, it becomes a lot less clean to automatically include them in a migration, so I added deploy notes that they must be added to appear in menus (and added them to the setup_site_pages management command)

[rlskoeser]

This all looks great. I'm satisfied with your solution for including the link page in context, and it's fine if we can't prepopulate the link pages as a migration.

If it's possible to create a django fixture file so the two link pages can be easily loaded, that would be helpful so the production site can be updated quickly when the new version goes out. (But only if it's easy)

[blms]

@rlskoeser I was able to create and load a fixture locally, but I have a couple of concerns with it:

• It contains a path field, which is a representation of the tree structure, so I think it may be pretty dependent on the existing pages
• It includes pks which theoretically could collide, including revision pks which I think are fairly likely to collide
• The owner is my username, which may end up being different (and on that note, it reminds me that I'll need an account with CMS access on the S&Co staging site, and maybe prod too if I am doing this on prod)

Maybe more trouble than it's worth? I think it would take about as long to load fixtures as it would to create the link pages manually.

[rlskoeser]

@blms thanks for investigating, sounds like it isn't worth the trouble.