#20235 closed Cleanup/optimization (fixed)
MultipleObjectMixin requires object_list to be passed in rather than using self.object_list
Reported by: | Matthew Somerville | Owned by: | Markus Holtermann |
---|---|---|---|
Component: | Generic views | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | tinodb | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
Unlike SingleObjectMixin, which uses self.object in its get_context_data, MultipleObjectMixin requires that object_list is passed in to get_context_data as a kwarg. BaseListView does this, but using MultipleObjectMixin separately requires this knowledge, not documented on the mixin page at https://docs.djangoproject.com/en/dev/ref/class-based-views/mixins-multiple-object/#django.views.generic.list.MultipleObjectMixin . It is alluded to at https://docs.djangoproject.com/en/dev/ref/class-based-views/generic-display/#django.views.generic.list.BaseListView but I don't think this is clear, and I think having it in MultipleObjectMixin is a better location, as with SingleObjectMixin.
As I assume MultipleObjectMixin is in use already with its current behaviour, I believe the nicest fix is to add a default to the pop() so that MultipleObjectMixin can be used by setting self.object_list in your subclass, the same as with SingleObjectMixin and self.object.
I have a patch on a branch of my github fork: https://github.com/dracos/django/compare/multipleobjectfixin
Change History (8)
comment:1 by , 12 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 12 years ago
Cc: | added |
---|---|
Component: | Uncategorized → Generic views |
Type: | Uncategorized → Cleanup/optimization |
A question about the tests. Why are you testing that get_context_object_name
works, and not that the correct queryset is used? The first two assertions would also pass without your changes, right? Or do I miss something?
comment:3 by , 12 years ago
The second test (of object_list
) would not pass without the change, because get_context_data()
is called without arguments, and without the change that would throw an error as object_list
wouldn't be present for the pop
. The other tests were just to try and match the SingleObjectMixin test as much as possible.
comment:4 by , 12 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:5 by , 12 years ago
The test look fine to me. I rebased your patch onto the current master so that it merges cleanly and opened a pull request at https://github.com/django/django/pull/1083
Thanks for your work, dracos.
comment:6 by , 12 years ago
Patch needs improvement: | set |
---|
As discussed at the sprint, the I share some confusion about what the tests are doing. In particular the customer context_object_name
is not necessary, and also setting object_list
as a class level variable is ugly.
comment:7 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Was also bitten by this inconsistency. From a quick review the patch seems correct.