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.

Sign up for GitHub

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

Jump to bottom

Refactor image chooser pagination to check WAGTAILIMAGES_CHOOSER_PAGE_SIZE at runtime #11898

Closed
wants to merge 1 commit into from
Closed

Refactor image chooser pagination to check WAGTAILIMAGES_CHOOSER_PAGE_SIZE at runtime #11898

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions wagtail/admin/viewsets/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ def construct_view(self, view_class, **kwargs):
key: value
for key, value in self.get_common_view_kwargs().items()
if hasattr(view_class, key)
and not isinstance(getattr(view_class, key), property)
Copy link
Member

@laymonage laymonage May 3, 2024 •
edited

Choose a reason for hiding this comment

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

I'm not sure if special-casing property is a good idea here... Sometimes we use cached_property on the view, and if we exclude property here, should we do the same for cached_property too?

I'm leaning towards defining a sentinel value for "undefined" configuration in the viewset as you described in #11162 (comment), and excluding that sentinel here, instead of specifically checking for property/cached_property. It would allow us to explicitly say "use whatever the view class has" for any attribute, instead of doing something like the following:

wagtail/wagtail/admin/viewsets/model.py

Lines 469 to 500 in 538365f

@cached_property
def search_fields(self):
"""
The fields to use for the search in the index view.
If set to ``None`` and :attr:`search_backend_name` is set to use a Wagtail search backend,
the ``search_fields`` attribute of the model will be used instead.
"""
return self.index_view_class.search_fields
@cached_property
def search_backend_name(self):
"""
The name of the Wagtail search backend to use for the search in the index view.
If set to a falsy value, the search will fall back to use Django's QuerySet API.
"""
return self.index_view_class.search_backend_name
@cached_property
def list_export(self):
"""
A list or tuple, where each item is the name of a field, an attribute,
or a single-argument callable on the model to be exported.
"""
return self.index_view_class.list_export
@cached_property
def export_headings(self):
"""
A dictionary of export column heading overrides in the format
``{field_name: heading}``.
"""
return self.index_view_class.export_headings

which fails if the view defines those attributes as property/cached_property, as you'd be passing the unbound version of the view's property to the view instance during as_view(). For example, some views override columns as a cached_property to allow accessing the self view instance to call its methods.

}
filtered_kwargs.update(kwargs)
return view_class.as_view(**filtered_kwargs)
Expand Down
16 changes: 16 additions & 0 deletions wagtail/images/tests/test_admin_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1681,6 +1681,22 @@ def test_pagination(self):
response = self.get({"p": 9999})
self.assertEqual(response.status_code, 404)

@override_settings(WAGTAILIMAGES_CHOOSER_PAGE_SIZE=4)
def test_chooser_page_size(self):
images = [
Image(
title="Test image %i" % i,
file=get_test_image_file(size=(1, 1)),
)
for i in range(1, 12)
]
Image.objects.bulk_create(images)

response = self.get()

self.assertContains(response, "Page 1 of 3")
self.assertEqual(response.status_code, 200)

def test_filter_by_tag(self):
for i in range(0, 10):
image = Image.objects.create(
Expand Down
9 changes: 7 additions & 2 deletions wagtail/images/views/chooser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,16 @@ def get_creation_form_kwargs(self):
class BaseImageChooseView(BaseChooseView):
template_name = "wagtailimages/chooser/chooser.html"
results_template_name = "wagtailimages/chooser/results.html"
per_page = 12
ordering = "-created_at"
construct_queryset_hook_name = "construct_image_chooser_queryset"

@property
def per_page(self):
# Make per_page into a property so that we can read back WAGTAILIMAGES_CHOOSER_PAGE_SIZE
# at runtime. This means we can no longer set it as an attribute on the viewset, but that's
# fine.
return getattr(settings, "WAGTAILIMAGES_CHOOSER_PAGE_SIZE", 20)

def get_object_list(self):
return (
permission_policy.instances_user_has_any_permission_for(
Expand Down Expand Up @@ -309,7 +315,6 @@ class ImageChooserViewSet(ChooserViewSet):
preserve_url_parameters = ChooserViewSet.preserve_url_parameters + ["select_format"]

icon = "image"
per_page = getattr(settings, "WAGTAILIMAGES_CHOOSER_PAGE_SIZE", 10)
choose_one_text = _("Choose an image")
create_action_label = _("Upload")
create_action_clicked_label = _("Uploading…")
Expand Down