Opened 8 years ago

Closed 6 years ago

Last modified 5 years ago

#8528 closed (fixed)

Admin list_filter doesn't respect null=True

Reported by: StevenPotter Owned by: Julien Phalip
Component: contrib.admin Version: 1.3-alpha
Severity: Keywords:
Cc: bthomas@…, dkg@…, marcoberi@…, Dan Fairs, drmeers@…, adam@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Maybe it is just me, but it seems if a models field has the properties null=True, blank=True and a list filter is applied to that field one of the filter options should be "Is Null"

Attachments (9)

nullfilter_9084.diff (3.5 KB) - added by Bob Thomas 8 years ago.
Add an "Is null" filter for foreign keys and value fields
nullfilter-11627.diff (3.5 KB) - added by Ben Davis 7 years ago.
ticket8528-nullfilter-1-1-1-final.diff (3.7 KB) - added by marcob 7 years ago.
ticket8528-1.2.diff (3.4 KB) - added by arnaudr 6 years ago.
Patches cleanly against 1.2
AllValuesFilterSpec_Null.diff (5.0 KB) - added by oyvind 6 years ago.
Patch from #14467 with tests
ticket8528-1.2.4.after.3400.1.2.4.diff (4.3 KB) - added by marcob 6 years ago.
A newer patch I apply after the #3400 one (don't know if works with trunk, give it a try)
8528_filterspec_null.diff (11.9 KB) - added by Julien Phalip 6 years ago.
8528_filterspec_null_v2.diff (18.4 KB) - added by Julien Phalip 6 years ago.
8528_filterspec_null_v2.2.diff (18.4 KB) - added by Julien Phalip 6 years ago.

Download all attachments as: .zip

Change History (35)

Changed 8 years ago by Bob Thomas

Attachment: nullfilter_9084.diff added

Add an "Is null" filter for foreign keys and value fields

comment:1 Changed 8 years ago by Bob Thomas

Cc: bthomas@… added
Has patch: set
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

Boolean filters already have an "Unknown" option if the model field is a NullBooleanField. I added null options for RelatedFilterSpec and AllValuesFilterSpec, though. Fixes my semi-duplicate ticket #9177

comment:2 Changed 8 years ago by ElliottM

Triage Stage: UnreviewedAccepted

comment:3 Changed 8 years ago by (none)

milestone: post-1.0

Milestone post-1.0 deleted

comment:4 Changed 7 years ago by Karen Tracey

#10665 was a dup.

comment:5 Changed 7 years ago by Daniel Kahn Gillmor

Cc: dkg@… added

comment:6 Changed 7 years ago by Ben Davis

I vote that instead of "Is null" the filter says "(None)". "(None)" would seem to make more sense to the lay-person, not just because "null" is sql jargon, but also because the admin already displays "(None)" in the changelist for null foreign keys.

Also, I've attached an updated patch for rev. 11627

Changed 7 years ago by Ben Davis

Attachment: nullfilter-11627.diff added

comment:7 Changed 7 years ago by marcob

Cc: marcoberi@… added

comment:8 Changed 7 years ago by marcob

Patch for Django 1.1.1 and that uses:

 "(%s)" % _("None") 

Changed 7 years ago by marcob

comment:9 Changed 7 years ago by Dan Fairs

Cc: Dan Fairs added

comment:10 Changed 7 years ago by Simon Meers

Cc: drmeers@… added

comment:11 Changed 7 years ago by adam@…

Cc: adam@… added

comment:12 Changed 7 years ago by Ben Davis

Just FYI: this patches cleanly against 1.2-beta.

Changed 6 years ago by arnaudr

Attachment: ticket8528-1.2.diff added

Patches cleanly against 1.2

comment:13 Changed 6 years ago by marcob

Trying tests I got:

   TemplateSyntaxError: Caught AttributeError while rendering: 'RelatedObject' object has no attribute 'null'

Are you sure about that self.field.null in filterspecs.py ?

comment:14 Changed 6 years ago by Simon Litchfield

Version: 1.0-beta-11.3-alpha

#5273 was another dupe. 4+ tickets all up. Latest patch looks good. Be great to get this simple fix in for 1.3.

comment:15 Changed 6 years ago by Simon Litchfield

See also #5833

Changed 6 years ago by oyvind

Patch from #14467 with tests

comment:16 Changed 6 years ago by oyvind

Owner: changed from nobody to oyvind
Status: newassigned

comment:17 Changed 6 years ago by oyvind

milestone: 1.3

comment:18 Changed 6 years ago by Julien Phalip

Pointing out that oyvind's latest patch isn't applying to trunk any more since r14674. Pinging DrMeers :)

Changed 6 years ago by marcob

A newer patch I apply after the #3400 one (don't know if works with trunk, give it a try)

comment:19 Changed 6 years ago by Julien Phalip

Owner: changed from oyvind to Julien Phalip
Status: assignednew

I'm working on a patch. Hopefully we can ship this one in 1.3.

Changed 6 years ago by Julien Phalip

Attachment: 8528_filterspec_null.diff added

comment:20 Changed 6 years ago by Julien Phalip

OK, I've written a patch based on marcob's, with some small tweaks. In particular I'm using EMPTY_CHANGELIST_VALUE (instead of _('None')) for consistency with the way NULL values are displayed in readonly fields and in the changelist, and also to dissociate it from the 'All' filter which has a different purpose.

The tests are based on oyvind's patch and I've also added tests for the FK and M2M filters.

Let me know what you think. I'm particularly wondering about the condition "if hasattr(self.field, 'rel')" which I had to add to avoid breaking the existing admin views tests.

comment:21 Changed 6 years ago by Simon Meers

Patch needs improvement: set

Looks good Julien. Only minor comments:

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?

Any reason you've cast self.lookup_val_isnull to bool in RelatedFilterSpec but not in AllValuesFilterSpec?

You need an __init__.py in tests/regressiontests/admin_filterspecs/.

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.

In the tests, would choices[-1] be more appropriate than choices[3] and choices[4] for testing the "last choice"?

Lines in tests.py and filterspecs.py exceed PEP8's 79-character limit.

Thanks for the great work, hopefully we can get this RFC today.

Changed 6 years ago by Julien Phalip

Changed 6 years ago by Julien Phalip

comment:22 Changed 6 years ago by Julien Phalip

Patch needs improvement: unset

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 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! ;)

comment:23 Changed 6 years ago by Simon Meers

Triage Stage: AcceptedReady for checkin

Great work, thanks Julien. The only thing that caught my eye was the implicit boolean logic grouping when checking for RelatedObjects, but I don't think this makes any functional difference. Also for some reason patch still isn't creating the admin_filterspecs/__init__.py file, but I can see it in your .diff, so I'll assume that's a problem at my end?

comment:24 Changed 6 years ago by Russell Keith-Magee

Two quick notes for the benefit of posterity:

  1. I've made one very small addition to this patch as provided -- I've modified the has_output() method on RelatedValueFilterSpec, to allow for the fact that "none" should be included in the count of available values if the related field allows nulls. This caused me a bit of confusion in my simple manual test case because I only defined a single related object, and the filters didn't display -- because according to the filterspec, there was only one valid value, and a filter isn't required if there's only one value.
  1. I'm not going to backport this to 1.2.X. The bug obviously exists in 1.2.X, but it's the new FilterSpec framework that gives us the flexibility to fix the bug easily; a backport would be a major PITA.

comment:25 Changed 6 years ago by Russell Keith-Magee

Resolution: fixed
Status: newclosed

In [15649]:

Fixed #8528 -- Ensure that null values are displayed as a filtering option in the admin if a field allows nulls. Thanks to StevenPotter for the report, and oyvind, marcob, Simon Meers and Julien Phalip for the patch.

comment:26 Changed 5 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

Note: See TracTickets for help on using tickets.
Back to Top