RFC 85: Snippets parity with ModelAdmin

[laymonage]

Rendered

Follow up to the Snippets vs ModelAdmin parity discussion, as I think using an RFC is easier for reviewing each individual customisation point described in the discussion.

Note that some of the items in the RFC have been implemented and merged into main (PRs are linked). However, it is likely that some of the customisations need a clearer implementation plan, hence the RFC. These features may not make it into the next release (5.0 at the time of writing), so this RFC can still be useful for future reference.

I highly recommend giving the code of the current SnippetViewSet class a read: https://github.com/wagtail/wagtail/blob/main/wagtail/snippets/views/snippets.py#L585

[laymonage]

Discuss

[laymonage]

Discuss, though I personally don't see much value for this

[ababic]

The menu_icon attribute in ModelAdmin serves the same purpose really (it isn't just for the 'menu' - it appears in headers and everywhere else icons are featured), so I would class this as done. Icons are all about improving recognisably, so it's difficult to imagine wanting a separate icon to appear in the menu.

[laymonage]

Do we want this? We're currently using the tables component from wagtail.admin.ui.tables, so adding these may be slightly tricky. I'd imagine we could add an additional keyword argument to the base Column class for the empty value display and pass the value from SnippetViewSet there. However, we have quite a lot of keyword arguments on the Column class already

[ababic]

To be honest, I added these to ModelAdmin for parity with Django, but I haven't ever used them in a project. These days, if I wanted to change the blank/null representation for a specific field, I'd be more likely to add a property method to the model class and use that instead.

[laymonage]

Can this be marked as done, or do we want to do something more?

[ababic]

I think there's definitely scope for something more convenient than the django-filter approach, but that would probably fair better as a separate RFC, as the aim should be for a consistant approach throughout.

[laymonage]

I personally think it is not worth it, but feel free to discuss

[ababic]

I don't think a like-for-like implementation would be desirable either. Though, having some control over which fields to search on when searching from the listing page could definitely be useful.

[laymonage]

Thank you! I noticed that Wagtail's generic IndexView already has search_fields in place that we could hook up from SnippetViewSet.

I also managed to add support to switch between Wagtail's search backend or Django's QuerySet API in a minimal way that doesn't need a separate search handler class. I believe the main use cases for ModelAdmin's search handler is to choose between Wagtail search and Django anyway, so I think this should be enough for our parity goal.

See:

• Allow customising the search_fields and search backend to use via SnippetViewSet wagtail#10290

[laymonage]

As the current templates live in templates/wagtailsnippets/snippets/, the likely discovery order would be:

1. templates/wagtailsnippets/snippets/app-name/model-name/
2. templates/wagtailsnippets/snippets/app-name/
3. templates/wagtailsnippets/snippets/

[ababic]

I just found myself wanting to override a template to get some custom JS into a chooser's 'create' view and this feature would definitely be useful. I don't suppose the paths matter too much (as long as they are documented), but I would have thought that wagtailsnippets/ instead of wagtailsnippets/snippets/ would be verbose enough to avoid clashes with anything else.

[laymonage]

Thank you, this was an old diff of the RFC and the feature was implemented in wagtail/wagtail#10271 (available in 5.0+).

wagtailsnippets/snippets/ was kept for backwards-compatibility, but it can be customised via SnippetViewSet.template_prefix. The RFC has been updated to reflect this.

[laymonage]

My personal opinion is to not implement these for snippets, as we can achieve the same things via other means that are more in line with how we do things in Wagtail

[ababic]

Fundamentally, I agree. Helper classes were just my way of encapsulating certain bits of logic in a way that made things portable/swappable, without complicating the ModelAdmin class too much (something I'd always felt Django's version could do better).

It feels as though Wagtail has naturally evolved to do much of this in different ways, which is fine. I think it's fair to say there are some rough edges as a result of that though, that will need addressing at some point (for example, the title column in snippet listings always adding an edit link, regardless of whether the user has that permission). The route to overriding permission checking logic via generic views in general feels like it needs some refinement, but that doesn't necessarily need to be part of this RFC.

[laymonage]

I agree, thanks, I'll try to revisit this in a future release.

[laymonage]

Discuss

As noted in wagtail/wagtail#10206 (reply in thread), I think one way we could allow disabling this is by overriding the {foo}_view_class (or the {foo}_view) properties on the SnippetViewSet to None. Then, in get_urlpatterns(), we only register the URLs if the corresponding view is not None.

As noted in that comment, though, the tricky part is to gracefully handle the UI when the expected URLs are not available.

[ababic]

For all it's sins, ModelAdmin does at least have this element down, but the recommended solution would be to use a custom PermissionHelper class to block things at the permission-checking level (e.g. By always returning False).

[laymonage]

Thanks, I think this would be possible to some extent via the permission_policy that is customisable on SnippetViewSet, but I haven't tested this. Will revisit in a future release.

[laymonage]

I'm somewhat in favour of allowing both register_snippet(SnippetModel) and register_snippet(SnippetViewSet), with the option to deprecate the viewset parameter in the future.

[ababic]

I agree, that flexibility is a nice improvement on what modeladmin has.

[laymonage]

Thanks, again this has been implemented in wagtail/wagtail@8df6e62 and available in Wagtail 5.0+.

[laymonage]

I should probably answer this myself, but I wonder what others might think about it

[ababic]

Personally, I don't think we should bother breaking modeladmin out into it's own package - the maintenence overhead wouldn't really justify the effort of removing it in the first place. I think it'd be quite reasonable to deprecate it without any fixed timeline for removal, encourage users to feed back on any pain points that come with migration, and just remove it once we feel ready.

[gasman]

I see the modeladmin package as serving a similar role to the hallo.js one - a courtesy for people who are "locked in" to modeladmin, perhaps because they've made low-level customisations or simply because they don't have a developer available for the task of figuring out the correspondences from their modeladmin code to snippets. Keeping it outside Wagtail core will communicate that new projects shouldn't use it (and third-party packages that depend on it should migrate away). We only need to maintain it to the extent that the demand from the community exists, and over time the need for that package will organically fade away.

[lb-]

Just wanted to say wow! Thanks @laymonage - this is an incredible write up and covers the discussion of every single feature I can think of so far.

I've had a quick read but will double back with comments when I have a bit more time.

One thing that could be clearer is that Snippets are intended to become the recommended way to manage non-Page models within Wagtail.

You have mentioned that there is a seperate RFC for managing other views of Pages which is good. However, I think this is something that will intentionally be different compared to ModelAdmin.

[thibaudcolas]

Since ModelAdmin has relatively unique API names, particularly for all the customisable attributes, I had a go at checking how many of the projects I have access to use those APIs. Here are the results: ModelAdmin API usage - RFC 85 ModelAdmin vs. Snippets feature parity.

This is for 70 "Torchbox" website/client projects, and 70 "Wagtail" packages (excluding Wagtail itself). Those projects aren’t particularly representative but I hope this nonetheless helps illustrate how much usage those APIs might get relative to one-another. Summary:

Things that show up a lot (some of which could be false positives): menu_icon, menu_label, menu_order, list_filter add_to_settings_menu, button_helper_class, InspectView

Things that have 0 use across all 140 projects:

add_to_admin_menu
base_url_path
choose_parent_template_name
choose_parent_view_class
delete_template_name
export_filename
get_empty_value_display
get_extra_attrs_for_row
menu_item_name
search_handler_class
thumb_classname
thumb_col_header_text
thumb_default
thumb_image_field_name
thumb_image_width
ThumbnailMixin

Things that see lots of usage in TBX sites/projects, very little in Wagtail packages:

list_export
permission_helper_class
inspect_view_fields
index_view_class
InspectView
admin_order_field

Things that see lots of usage in Wagtail packages, little on TBX projects:

form_view_extra_css
form_view_extra_js
index_view_extra_js

[laymonage]

Hi all, I've updated the RFC to reflect the current state of the parity and our plans for the deprecation timeline. Please leave a comment if you have any feedback, thanks!

[lb-]

It might be good to call out there are already issues to do similar sets of behaviour and it is best we solve this with either documented example Stimulus approaches or Wagtail wide solutions such as via Panels as mentioned.

• Conditional form fields in admin wagtail#2172
• Allow Page Title field to be dynamically generated or relabelled wagtail#161
• Allow filtered input on fieldpanels by supplying a regex code of allowed characters wagtail#7417

[laymonage]

Thank you, I've updated this paragraph to add this detail.

[lb-]

I’d like to see us move the documentation for a custom clean method to somewhere else if possible.

Either to a Snippets tips & tricks or somewhere else suitable, this is a common request and the functionality is actually something generic to all forms in Wagtail and is often something asked about on Slack.

https://docs.wagtail.org/en/stable/reference/contrib/modeladmin/tips_and_tricks/custom_clean.html

[laymonage]

Great idea, I'll try to do this during the 5.1 feature freeze. Thank you!

[lb-]

This is incredible @laymonage

I was quite skeptical when this was discussed over a year ago as ModelAdmin is one of my favourite features. However, I feel that the approach taken has been smooth, steady and offers a much better API surface area that also makes sense in the context of the rest of Wagtail.

I think we should do our best to ensure that the split out ModelAdmin repo has some nightly tests against Wagtail main, at least for one LTS release cycle. So that the core team / others can try to keep compatibility with this external package while the community migrates.

It may be worth also reviewing what other key community packages are reliant on ModelAdmin and alert them with an issue on their repo about this RFC once approved. This may already be done.

Happy to approve this in its current state and very glad to see this all worked through in such great detail.

[laymonage]

Thank you everyone for all the comments! As this has been approved for a while and we've made a lot of progress in the Wagtail repo, I'm going to merge this just in time for Wagtail 5.1 RC release.