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
Refactor image chooser pagination to check WAGTAILIMAGES_CHOOSER_PAGE_SIZE at runtime #11898
Conversation
Manage this branch in SquashTest this branch here: https://gasmanrohitsrma-fix-chooser-pa-9pr2k.squash.io |
@@ -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) |
There was a problem hiding this comment.
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.
Superseded by #11937 |
Reworking of #11884, to fix #11813.
The problem with setting
per_page
as an attribute on the viewset is that it requires reading the setting on module load, which means we can't useoverride_settings
to test it. Instead, make it a property onBaseImageChooseView
so that it gets read back at runtime. (Which in turn means that theconstruct_view
logic in the base ViewSet class needs to take care not to write to attributes on the view if they're actually properties.)We could also have done this by adding a
get_per_page
method on the generic chooser view (defaulting to just returningself.per_page
) and making it overrideable on the viewset usinginject_view_methods
- but we've pretty much settled on using properties rather thanget_foo
methods in our generic views now.I also took the liberty of increasing the default page size to 20, since this bug inadvertently created an ambiguity over whether the "correct" value is 10 (which works for rows of 5) or 12 (which works for rows of 4) - this way it works equally well for both :-)