☂️ Adopt generic class based views across all areas of admin (epic)

[lb-]

It> ☂️ This is an epic / umbrella issue

We will break down individual parts into sub-issues, please work on the sub-issues instead.

Description

• Wagtail 2.12 added generic class based views for use across Wagtail's admin, which was a progression of existing work to move to class based views where possible
• This provides the ability for common views to share common logic and behaviour
• This also provides a way for even more customisation over specific URLs by overriding a specific URL path with your own custom class based view that extends the generic ones
• This will replace the function views used in other parts of Wagtail currently, or ad-hoc class based views that either use Django's directly or do not
• The Wagtail generic class based views extend the Django class based views

Implementation (core) - high priority

• Snippets - adopt class based views #8367
• Adopt generic class based views for Home (aka dashboard) #8366
• Adopt generic class based views for User management #8622
• Adopt generic class based view for page listing (explorer) #8628
• Adopt generic class based view for user account editing #8629

Implementation (contrib) - medium priority

• Adopt generic class based views for Form builder wagtail/contrib/forms/views.py
  • we may want to pull up SafePaginateListView into a shared mixin
• Adopt generic class based views for Site Settings wagtail/contrib/settings/views.py
  • using function based views
• Adopt generic class based views for Search promotions wagtail/contrib/search_promotions/views.py
  • using function based views, no class based ones at all, may be suitable for viewset
  • Refactor searchpromotions index view to CBV #11661
• Adopt generic class based views for Redirects wagtail/contrib/redirects/views.py
  • using function based views, no class based ones at all, may be suitable for viewset
  • Refactor redirects index view to CBV #11762
• Simple translation wagtail/contrib/simple_translation/views.py
  • using Django's class based views

Implementation (contrib) - low priority

These areas may not be needed but adding here for completeness without checkboxes. We will likely either not do these ever or split these out to future issues.

• Adopt generic class based views for ModelAdmin wagtail/contrib/modeladmin/views.py
  • using Wagtail's generic views in most cases, may need to assess the other usage
  • migrating ModelAdmin views to this approach may end up being a large amount of extra overhead due to the different usage of search / filtering Params in the url
  • if we can do this in a way that preserves the existing API it may be ok but tread carefully.
• Styleguide wagtail/contrib/styleguide/views.py
  • using function view, not a huge priority though - maybe it's worth refactoring into using Template Components also but there is some long term plans that may involve removing this contrib module completely

Additional context

• PR to create generic views (2.12) Add shared class-based views for multiple image / document uploaders #6658
• Refactoring Page editor views into class based views Refactor page create/edit views into class-based views #6306
• Clean up and add common title/sub-titles to class based views Simplify generic view templates #7381
• Refactoring snippets into class based views Refactor create snippets view into class-based view #8332 Refactor Snippets list view to extend from generic IndexView #8335 Refactor Snippets edit view to extend from generic EditView #8344 Refactor Snippets delete view to extend from generic DeleteView #8346 Refactor Snippets usage view to extend from generic IndexView #8351 & Refactor Snippets index view to extend from TemplateView #8352
• An example of adopting the Model viewsets (new code) 7c86c4e

[lb-]

#8417

[lb-]

Created three new issues, these would be very helpful to streamline the efforts on UX Unification (GSoC) but not critical

• Adopt generic class based views for User management #8622
• Adopt generic class based view for page listing (explorer) #8628
• Adopt generic class based view for user account editing #8629

[kaedroho]

Thanks for this @lb-!!

I feel like we should drop the styleguide from this, since that view just needs to render a template and I don't think any of our generic views will help with this. Also, simple translation doesn't contain any index/create/edit/delete views so it might not make much sense to make this one generic either.

Modeladmin might be tricky since the way it implements search, filtering and customising the listing is very different to how the generic views currently/will work. If we need to change modeladmin's API in order to make it fit generic views better, I don't think this would be worth it. W plan to make model viewsets easier to use so developers could switch to that if they want this API.

[lb-]

PR for reports index that relates to this.
#8637

Thanks @laymonage !

[lb-]

@kaedroho I have descoped (no longer has checkboxes) the ModelAdmin and styleguide ones and re-grouped these into a low priority section.

Keeping them listed for completeness - hope that is ok.

Let me know if you are aware of any other views in Wagtail that should adopt the shared classes approach.

[lb-]

Note - there is now an RFC for further enhancements to the viewsets system.

wagtail/rfcs#87

Update. This RFC has been closed.

[Temidayo32]

@laymonage @lb-

using function based views, no class based ones at all, may be suitable for viewset

Can I clarify what this means? I'm not sure what is meant here.

[lb-]

@Temidayo32 at the time of creation of this issue, viewsets were still new-ish and didn't support a full range of views. The viewsets system is now quite capable of probably covering all these use cases.

It's also a public facing API. See https://docs.wagtail.org/en/stable/reference/viewsets.html

[Temidayo32]

@lb- Thanks for the response.

Adopt generic class based views for Redirects wagtail/contrib/redirects/views.py

I intend to work on this above. So if I understand you, wagtail/contrib/redirects/views.py should be refactored using the viewsets, right?

[lb-]

@Temidayo32 yep. I think that's what we would want to do now.

[Temidayo32]

@lb- Alright. Thanks for the clarification

[Chidera6]

Hi @thibaudcolas @laymonage , I intend working on this issue wagtail/contrib/search_promotions/views.py.

[Chiemezuo]

@Chidera6 incidentally, I'm trying to get started on the one for redirects wagtail/contrib/redirects/views.py
Is it okay for us to reach out to ourselves if we get stuck?

[Chidera6]

@Chidera6 incidentally, I'm trying to get started on the one for redirects wagtail/contrib/redirects/views.py Is it okay for us to reach out to ourselves if we get stuck?

That would be very nice @Chiemezuo but I think @Temidayo32 is working on that issue you selected.

[thibaudcolas]

@Temidayo32 @Chidera6 @Chiemezuo just a heads’up we consider this all to be relatively advanced as far as Wagtail contributions go. If you don’t have pre-existing experience with Django and with class-based views, I would recommend you focus on other possible contributions. If you do have this expertise… go for it!

As you approach this please take ample time reviewing other pull requests done in this space, so you get to see how the contributions that we accepted were made.

[Chiemezuo]

Thank you @lb-
Working on that right now.

[Temidayo32]

@Chidera6 Are you still working on wagtail/contrib/search_promotions/views.py?

[Temidayo32]

@thibaudcolas Is there anything left in this issue that I can contribute to? I know Django and want to help.

@tony-nyagah - there are lots of other function based views still needing work.

If you are keen, there is one I have started that may be good to get your help with, there are two FBVs in the images views that we are in the process of doing some clean up on. Some of this clean up would be better enabled by these moving to CBVs.

• url_generator - This serves a page that lets you create a URL to an image based on the size you want. Similar to how images are used in templates with a filter_spec.
• generate_url - This serves a JSON response with the generated URL data and is requested from the view above.

Both of these are at same URL, just the second one is based on a different path.

I started looking at using the generic.models.InspectView for this as it contains most of the basics we would need and avoids the added features of EditView.

Some notes.

• wagtail/images/templates/wagtailimages/images/url_generator.html would need to extend {% extends "wagtailadmin/generic/base.html" %}
• We can get rid of the title tag there and use the built in title generation from the class views.
• We would need to move the main content into {% block main_content %} not {% block content %}
• No need to use the header in that view as this will come from the base template {% include "wagtailadmin/shared/header.html"

Starting point - I think we would just make get_fields return an empty array as we would use the existing Form.

class GenerateURLView(generic.InspectView):
    any_permission_required = ["change"]
    model = get_image_model()
    pk_url_kwarg = "image_id"
    header_icon = "image"
    page_title = gettext_lazy("Generating URL")
    template_name = "wagtailimages/images/url_generator.html"

    def get_page_subtitle(self):
        return self.object.title

    def get_fields(self):
        return []

    def get_context_data(self, **kwargs):
        context = super().get_context_data(**kwargs)

        context["form"] = URLGeneratorForm(
            initial={
                "filter_method": "original",
                "width": self.object.width,
                "height": self.object.height,
            }
        )

        return context

This does not consider the other URL (returning JSON) we would want to build into this response, either with a request varies decorator or just logic in the response building.

To see this page in action you can go to edit an image, then click 'URL Generator' under the image preview.

For the other related work we are planning to do, see #11045 & #3683

If you are keen for this, let us know, I am happy to split this out to its own issue for further discussion.

I am interested @lb-

[Chidera6]

@Chidera6 Are you still working on wagtail/contrib/search_promotions/views.py?

Yes, I am still working on it @Temidayo32

[lb-]

@Temidayo32 - I think @tony-nyagah else has already started (image url generator views) so it's best to avoid conflicting PRs for now.

[sharma-ramya]

Hi @lb- I know Django and would like to contribute to this issue. Can you let me know what is available to avoid any conflicts with the current allocated tasks?
Can I get started on wagtail/contrib/forms/views.py?
Thanks in advance!

[Temidayo32]

This will replace the function views used in other parts of Wagtail currently, or ad-hoc class based views that either use Django's directly or do not

@lb- can you please clarify what is meant by ad-hoc class based views. Perhaps a link to one would be helpful. Thanks.

[lb-]

@Temidayo32 - please note that this issue was created well before the Outreachy program was scoped and before a lot of the recent (last six months) refactoring of Snippets and model viewsets was done.

However, generally, my initial specification for this issue was based on our introduction of more useful view classes that were used throughout Wagtail. This includes the Wagtail admin mixins, the model generic views and the viewsets system.

Some earlier class based views may have been ad-hoc, as in they did not use the shared generic views within Wagtail's code. Instead they would have been built upon Django's classed based views.

Migration needs to be assessed on a case by case basis though. Sometimes we do not need more than the Django base classes go do what's needed. But once we start dealing with models/objects - the Wagtail ones will replace a lot of the boilerplate needed for a consistent UI.

I hope that helps add some context, I do expect that once the Outreachy program starts that this issue will be closed / revised. Along with a new, clearer migration schedule based on a review of what's used throughout the admin currently.

The main goals here are to:

• A. Avoid duplication for common UI patterns (such as pagination, we have implemented various versions of this in different modules).
• B. Build on the patterns provided by Django and then use those to provide patterns for those building with Wagtail.
• C. Make parts of Wagtail easier to customise, maybe not in an officially supported way yet but still have some undocumented escape hatches that Classes provide.

[rohitsrma]

@thibaudcolas Is there anything left in this issue that I can contribute to? I know Django and want to help.

@tony-nyagah - there are lots of other function based views still needing work.

If you are keen, there is one I have started that may be good to get your help with, there are two FBVs in the images views that we are in the process of doing some clean up on. Some of this clean up would be better enabled by these moving to CBVs.

• url_generator - This serves a page that lets you create a URL to an image based on the size you want. Similar to how images are used in templates with a filter_spec.
• generate_url - This serves a JSON response with the generated URL data and is requested from the view above.

Both of these are at same URL, just the second one is based on a different path.

I started looking at using the generic.models.InspectView for this as it contains most of the basics we would need and avoids the added features of EditView.

Some notes.

• wagtail/images/templates/wagtailimages/images/url_generator.html would need to extend {% extends "wagtailadmin/generic/base.html" %}
• We can get rid of the title tag there and use the built in title generation from the class views.
• We would need to move the main content into {% block main_content %} not {% block content %}
• No need to use the header in that view as this will come from the base template {% include "wagtailadmin/shared/header.html"

Starting point - I think we would just make get_fields return an empty array as we would use the existing Form.

class GenerateURLView(generic.InspectView):
    any_permission_required = ["change"]
    model = get_image_model()
    pk_url_kwarg = "image_id"
    header_icon = "image"
    page_title = gettext_lazy("Generating URL")
    template_name = "wagtailimages/images/url_generator.html"

    def get_page_subtitle(self):
        return self.object.title

    def get_fields(self):
        return []

    def get_context_data(self, **kwargs):
        context = super().get_context_data(**kwargs)

        context["form"] = URLGeneratorForm(
            initial={
                "filter_method": "original",
                "width": self.object.width,
                "height": self.object.height,
            }
        )

        return context

This does not consider the other URL (returning JSON) we would want to build into this response, either with a request varies decorator or just logic in the response building.

To see this page in action you can go to edit an image, then click 'URL Generator' under the image preview.

For the other related work we are planning to do, see #11045 & #3683

If you are keen for this, let us know, I am happy to split this out to its own issue for further discussion.

Hello @lb- , May i work on this?

[lb-]

@rohitsrma - go for it. I don't think @Temidayo32 made progress on the Image url generator views and this is still up for grabs.

[rohitsrma]

Thanks @lb- , I've created a PR #11423 and would appreciate feedback on it.

[thibaudcolas]

Reviewing this today, I see the following three items would still need refactoring:

• Adopt generic class based views for Redirects wagtail/contrib/redirects/views.py
• Adopt generic class based views for Search promotions wagtail/contrib/search_promotions/views.py
• Simple translation wagtail/contrib/simple_translation/views.py

@laymonage I believe you might be working on refactoring all three areas as part of Universal listings continued – how do you think those CBV refactorings fit with those UX improvements? I imagine they go hand in hand?

[lb-]

Redirects has a PR up #11035

[rohitsrma]

@lb- , I'm working on refactoring search_promotions/views.py.

[zerolab]

@lb- it looks like the last remaining bit on this is the simple translation view (wagtail/contrib/simple_translation/views.py) and we can close this epic 🎉

Thank you for putting it together and guiding folks. And to everyone that contributed so far ⭐ 🌟

[laymonage]

Thanks all!

#11762 only covers the index view so there are still some other views in the redirects contrib module to be refactored. Same with the search promotions in #11661 (there is #11731).

Not sure about the exact scope of this issue but there are still some views that are function-based and could use some refactoring, although they are not as prominent so we might only consider them as nice-to-haves. Although, it would be cool to at least make them extend WagtailAdminTemplateMixin plus the wagtailadmin/generic/base.html template so we have the standardised breadcrumbs, header icon, etc.

• wagtail.admin.views.pages.preview.view_draft
  • Could be refactored to extend the generic preview view class, and extracted into a generic view so snippets can also have the "view latest draft" functionality instead of having to preview a specific revision.
• wagtail.admin.views.pages.create.add_subpage
  • This is the "choose page type" screen when creating new pages.
• wagtail.admin.views.pages.move.move_choose_destination
• wagtail.admin.views.pages.move.move_confirm
• wagtail.admin.views.pages.ordering.set_page_position
  • Not important as this only acts as some sort of API for the client-side code, but if we used CBV it could make it easier if we wanted to add the "custom ordering" feature to non-page models.
• wagtail.admin.views.pages.copy.copy
• wagtail.admin.views.pages.revisions.revisions_revert
  • This one has been a pain point whenever we update the page edit view, as this is implemented as an entirely separate view yet uses the same edit template. For snippets, this was done a bit more elegantly by reusing the EditView code combined with the RevisionsRevertMixin to ensure the two never go out-of-sync.
• wagtail.admin.views.collection_privacy.set_privacy
  • Probably not very important as it's mostly used to render modals.
• wagtail.admin.views.workflows.enable_workflow
  • Not very important as it's only used for the form action of enabling a disabled workflow, and only returns a redirect.
• wagtail.admin.views.workflows.usage
  • Would be nice to use the tables UI framework and maybe rework this as a variant of the page usage view.
• wagtail.admin.views.workflows.remove_workflow
  • Pretty sure we don't expose this at all, in favour of disabling workflows...
• wagtail.admin.views.workflows.select_task_type
  • Very similar to wagtail.admin.views.pages.create.add_subpage. Could probably share the same template.
• wagtail.admin.views.workflows.enable_task
  • Not very important as it's only used for the form action of enabling a disabled task, and only returns a redirect.
• wagtail.contrib.redirects.views.add
• wagtail.contrib.redirects.views.edit (Refactor redirects edit view to CBV #11860)
• wagtail.contrib.redirects.views.delete
• wagtail.contrib.redirects.views.start_import (not sure)
• wagtail.contrib.redirects.views.process_import (not sure)
• wagtail.images.views.images.add (unused in favour of the multiple add view)
• wagtail.images.views.images.edit
• wagtail.documents.views.documents.add (unused in favour of the multiple add view)
• wagtail.documents.views.documents.edit
• wagtail.contrib.search_promotions.views.add
• wagtail.contrib.search_promotions.views.edit
• wagtail.contrib.search_promotions.views.delete
• wagtail.contrib.search_promotions.views.chooser

Not included, mostly straightforward/intermediary views

• wagtail.embeds.views.chooser.chooser
• wagtail.embeds.views.chooser.chooser_upload
• wagtail.admin.views.bulk_action.dispatcher.index
• wagtail.admin.workflows.views.task_chosen
• wagtail.contrib.search_promotions.views.chooserresults
• wagtail.contrib.forms.views.get_submissions_list_view (form submissions listing entrypoint)
• wagtail.contrib.settings.views.redirect_to_relevant_instance (wagtailsettings entrypoint)
• wagtail.images.views.images.preview
• "serve" views (i.e. pages, documents, images)

I think that's it but I might've missed a few.

If anyone is keen to continue the work: if you're not familiar with the recent adoption of generic views (with breadcrumbs etc.), I highly recommend starting off with one view first. Iterate on a PR until you get a sense of how the generic views and templates work and how they're used throughout the admin. Once you get a PR in, and have a good sense of the views and templates, refactoring multiple related views in the same PR is fine. Recent examples:

• Refactor searchpromotions index view to CBV #11661
• Refactor redirects index view to CBV #11762
• https://github.com/wagtail/wagtail/pulls?q=is%3Apr+is%3Aclosed+sort%3Aupdated-desc+universal+listing

Also, some of the views that have been migrated to use class-based views may not already:

• extend the suitable generic views (from wagtail.admin.views.generic or at the very least, WagtailAdminTemplateMixin)
• extend the corresponding template (or at the very least, wagtailadmin/generic/base.html)

So there are still work to be done for such views.

To add, some of the views already extend the generic ones but haven't been wrapped in a viewset. These include:

• wagtail.admin.views.collections -> should have CollectionViewSet
• wagtail.admin.views.workflows -> should have WorkflowViewSet and TaskViewSet
• wagtail.contrib.redirects.views -> should have RedirectViewSet
• wagtail.contrib.search_promotions.views -> should have QueryViewSet
• wagtail.documents.views -> should have DocumentViewSet
• wagtail.images.views -> should have ImageViewSet

A recent example of wrapping such views in a viewset can be seen in:

• Add UserViewSet and implement universal listings UI for users views #11823