Skip to content
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

Add PageListingViewSet for custom per-page-type page listings #11485

Merged
merged 19 commits into from
Mar 7, 2024

Conversation

gasman
Copy link
Collaborator

@gasman gasman commented Jan 19, 2024

Screenshot 2024-01-19 at 20 22 30

Introduce a new PageListingViewSet to provide filterable, customisable flat listings of a given page type. These share most of the functionality of the new universal listing implementation for pages, except for the ability to navigate through the tree (which is now shifted into a separate ExplorableIndexView subclass), and can potentially serve as a replacement for ModelAdmin's page model support. (It is currently missing the functionality for adding a new page, possibly choosing a parent page along the way.)

This can be tested against bakerydemo by adding the following to bakerydemo/base/wagtail_hooks.py:

from wagtail.admin.viewsets.pages import PageListingViewSet

class BreadPageFilterSet(PageListingViewSet.filterset_class):
    class Meta:
        model = BreadPage
        fields = ["bread_type"]


class BreadPageListingViewSet(PageListingViewSet):
    icon = "globe"
    menu_label = "Bread Pages"
    add_to_admin_menu = True
    model = BreadPage
    columns = PageListingViewSet.columns + [
        Column("bread_type", label="Bread Type", sort_key="bread_type"),
    ]
    filterset_class = BreadPageFilterSet


bread_page_listing_viewset = BreadPageListingViewSet('bread_pages')
@hooks.register('register_admin_viewset')
def register_bread_page_listing_viewset():
    return bread_page_listing_viewset

Please check the following:

  • Do the tests still pass?1
  • Does the code comply with the style guide?
    • Run make lint from the Wagtail root.
  • For Python changes: Have you added tests to cover the new/fixed behaviour?
  • For new features: Has the documentation been updated accordingly? (Not yet - will try to get this together during the 6.0 feature freeze...)

Footnotes

  1. Development Testing

@gasman gasman added this to the 6.0 milestone Jan 19, 2024
@gasman gasman requested a review from laymonage January 19, 2024 20:34
@gasman gasman self-assigned this Jan 19, 2024
Copy link

squash-labs bot commented Jan 19, 2024

Manage this branch in Squash

Test this branch here: https://gasmanfeaturepage-listing-view-rcsck.squash.io

@gasman
Copy link
Collaborator Author

gasman commented Jan 22, 2024

Rebased

@gasman gasman force-pushed the feature/page-listing-viewset branch 2 times, most recently from 62b55b9 to 0f710fa Compare January 22, 2024 18:30
@gasman
Copy link
Collaborator Author

gasman commented Jan 22, 2024

Re-rebased following #11483 :-)

@gasman gasman modified the milestones: 6.0, 6.1 Jan 24, 2024
Copy link
Member

@laymonage laymonage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and works well, thank you! One minor comment for a potential cleanup, but otherwise I think this just needs a rebase.

Comment on lines 316 to 331
columns = (
IndexView.columns[0:3]
+ [
Column(
"type",
label=_("Type"),
accessor="page_type_display_name",
sort_key="content_type",
width="12%",
),
]
+ IndexView.columns[3:]
+ [
NavigateToChildrenColumn("navigate", width="10%"),
]
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to use Django's @classproperty here? e.g.

Suggested change
columns = (
IndexView.columns[0:3]
+ [
Column(
"type",
label=_("Type"),
accessor="page_type_display_name",
sort_key="content_type",
width="12%",
),
]
+ IndexView.columns[3:]
+ [
NavigateToChildrenColumn("navigate", width="10%"),
]
)
@classproperty
def columns(cls):
columns = IndexView.columns.copy()
columns.insert(
3,
Column(
"type",
label=_("Type"),
accessor="page_type_display_name",
sort_key="content_type",
width="12%",
),
)
columns.append(NavigateToChildrenColumn("navigate", width="10%"))
return columns

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! Applied in 4340e21.

Comment on lines +104 to +110
@cached_property
def locale(self):
return self.get_locale()

@cached_property
def translations(self):
return self.get_translations() if self.locale else []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Been meaning to do this 😄

@gasman
Copy link
Collaborator Author

gasman commented Mar 1, 2024

Rebased - documentation still to come.

@gasman
Copy link
Collaborator Author

gasman commented Mar 4, 2024

Docs added - ready for re-review now.

@@ -0,0 +1,64 @@
# Custom page listings
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we have a corresponding entry for PageListingViewSet in https://github.com/wagtail/wagtail/blob/main/docs/reference/viewsets.md?plain=1, and reword one of the sentences here to explicitly mention PageListingViewSet with a link to the section in the API reference?

With enough docstrings in the source code, this should be just autodoc directives for the most part (if not all).

Then, add an explicit target so we can link to this from the reference docs.

Suggested change
# Custom page listings
(custom_page_listings)=
# Custom page listings

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @laymonage! Have added the reference docs in 2de9bd6.

Comment on lines 9 to 26
class PageListingViewSet(ViewSet):
#: The view class to use for the index view; must be a subclass of ``wagtail.admin.views.pages.listing.IndexView``.
index_view_class = IndexView
model = Page
columns = IndexView.columns
filterset_class = IndexView.filterset_class
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g.

Suggested change
class PageListingViewSet(ViewSet):
#: The view class to use for the index view; must be a subclass of ``wagtail.admin.views.pages.listing.IndexView``.
index_view_class = IndexView
model = Page
columns = IndexView.columns
filterset_class = IndexView.filterset_class
class PageListingViewSet(ViewSet):
"""
A viewset to present a flat listing of all pages of a specific type.
All attributes and methods from :class:`~wagtail.admin.viewsets.base.ViewSet`
are available.
For more information on how to use this class, see :ref:`custom_page_listings`.
"""
#: The view class to use for the index view; must be a subclass of ``wagtail.admin.views.pages.listing.IndexView``.
index_view_class = IndexView
#: Required; the page model class that this viewset will work with.
model = Page
#: A list of ``wagtail.admin.ui.tables.Column`` instances for the columns in the listing.
columns = IndexView.columns
#: A subclass of ``wagtail.admin.filters.WagtailFilterSet``, which is a
#: subclass of `django_filters.FilterSet <https://django-filter.readthedocs.io/en/stable/ref/filterset.html>`_.
#: This will be passed to the ``filterset_class`` attribute of the index view.
filterset_class = IndexView.filterset_class

docs/advanced_topics/customisation/custom_page_listings.md Outdated Show resolved Hide resolved
Copy link
Member

@laymonage laymonage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! As you said, the next step for ModelAdmin parity would be to add a ChooseParentView-equivalent. I'll create a separate issue for this 😃

@laymonage laymonage force-pushed the feature/page-listing-viewset branch from 2de9bd6 to c741351 Compare March 7, 2024 09:59
@laymonage laymonage merged commit c741351 into wagtail:main Mar 7, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:Page component:Universal listings Including page listings, model listings and filtering.
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants