#8528: Admin list_filter doesn't respect null=True
------------------------------------------------+---------------------------
Reporter: StevenPotter | Owner: julien
Status: new | Milestone: 1.3
Component: django.contrib.admin | Version: 1.3-alpha
Resolution: | Keywords:
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 |
------------------------------------------------+---------------------------
Changes (by julien):
* needs_better_patch: 1 => 0
Comment:
The genuine new patch is "8528_filterspec_null_v2.2.diff" above. Please
ignore "8528_filterspec_null_v2.diff".
Thanks for your great feedback, Simon.
Replying to [comment:21 DrMeers]:
> The `if hasattr(self.field, 'rel')` prevents dealing with a
`RelatedObject`, which is actually not a field but a reverse relationship.
I think it makes sense not to allow `isnull` filtering on these?
In fact, it does make sense to allow it to work with reverse
relationships. For example, in the User admin, you might want to filter
all users who are not authors of any books. I have changed the condition
to `if isinstance(self.field, models.related.RelatedObject) and
self.field.field.null or hasattr(self.field, 'rel') and self.field.null`.
All seems to work fine now.
> Any reason you've cast `self.lookup_val_isnull` to `bool` in
`RelatedFilterSpec` but not in `AllValuesFilterSpec`?
Yeah that was a bit silly. The casting should occur when yiedling the
resulting dictionary. Fixed in new patch.
> You need an `__init__.py` in `tests/regressiontests/admin_filterspecs/`.
Done.
> It might be a good idea to include a reverse relationship in the tests
(e.g. a `Chapter` model with a `ForeignKey` to `Book`, and then test a
filter in both directions)? As mentioned above, I'm not sure that an
`isnull` lookup on a reverse relationship makes sense, but it would be
nice to explicitly ensure that nothing breaks here.
Good idea, I've added such tests extending UserAdmin.
> In the tests, would `choices[-1]` be more appropriate than `choices[3]`
and `choices[4]` for testing the "last choice"?
Done. Good suggestion.
> Lines in `tests.py` and `filterspecs.py` exceed PEP8's 79-character
limit.
I've PEP8'ed it up ;)
Any further feedback welcome. Thanks! ;)
--
Ticket URL: <http://code.djangoproject.com/ticket/8528#comment:22>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to
django-updates+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/django-updates?hl=en.
