#8528: Admin list_filter doesn't respect null=True
-----------------------------+--------------------------------------------- -
 Reporter:  StevenPotter     |       Owner:  nobody    
   Status:  new              |   Milestone:  post-1.0  
Component:  Admin interface  |     Version:  1.0-beta-1
 Keywords:                   |       Stage:  Unreviewed
Has_patch:  0                |  
-----------------------------+--------------------------------------------- -
 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"

--
Ticket URL: <http://code.djangoproject.com/ticket/8528>
Django Code <http://code.djangoproject.com/>
The web framework for perfectionists with deadlines

#8528: Admin list_filter doesn't respect null=True
-------------------------------------------+------------------------------- -
          Reporter:  StevenPotter          |         Owner:  nobody    
            Status:  new                   |     Milestone:  post-1.0  
         Component:  django.contrib.admin  |       Version:  1.0-beta-1
        Resolution:                        |      Keywords:            
             Stage:  Unreviewed            |     Has_patch:  1        
        Needs_docs:  0                     |   Needs_tests:  0        
Needs_better_patch:  0                     |  
-------------------------------------------+------------------------------- -
Changes (by bthomas):

 * cc: btho...@ncircle.com (added)
  * needs_better_patch:  => 0
  * has_patch:  0 => 1
  * needs_tests:  => 0
  * needs_docs:  => 0

Comment:

 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

--
Ticket URL: <http://code.djangoproject.com/ticket/8528#comment:1>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

#8528: Admin list_filter doesn't respect null=True
-------------------------------------------+------------------------------- -
          Reporter:  StevenPotter          |         Owner:  nobody    
            Status:  new                   |     Milestone:  post-1.0  
         Component:  django.contrib.admin  |       Version:  1.0-beta-1
        Resolution:                        |      Keywords:            
             Stage:  Accepted              |     Has_patch:  1        
        Needs_docs:  0                     |   Needs_tests:  0        
Needs_better_patch:  0                     |  
-------------------------------------------+------------------------------- -
Changes (by ElliottM):

  * stage:  Unreviewed => Accepted

--
Ticket URL: <http://code.djangoproject.com/ticket/8528#comment:2>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

#8528: Admin list_filter doesn't respect null=True
-------------------------------------------+------------------------------- -
          Reporter:  StevenPotter          |         Owner:  nobody    
            Status:  new                   |     Milestone:            
         Component:  django.contrib.admin  |       Version:  1.0-beta-1
        Resolution:                        |      Keywords:            
             Stage:  Accepted              |     Has_patch:  1        
        Needs_docs:  0                     |   Needs_tests:  0        
Needs_better_patch:  0                     |  
-------------------------------------------+------------------------------- -
Comment (by kmtracey):

 #10665 was a dup.

--
Ticket URL: <http://code.djangoproject.com/ticket/8528#comment:4>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

#8528: Admin list_filter doesn't respect null=True
-------------------------------------------+------------------------------- -
          Reporter:  StevenPotter          |         Owner:  nobody    
            Status:  new                   |     Milestone:            
         Component:  django.contrib.admin  |       Version:  1.0-beta-1
        Resolution:                        |      Keywords:            
             Stage:  Accepted              |     Has_patch:  1        
        Needs_docs:  0                     |   Needs_tests:  0        
Needs_better_patch:  0                     |  
-------------------------------------------+------------------------------- -
Changes (by dkg):

 * cc: d...@fifthhorseman.net (added)

--
Ticket URL: <http://code.djangoproject.com/ticket/8528#comment:5>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

#8528: Admin list_filter doesn't respect null=True
-------------------------------------------+------------------------------- -
          Reporter:  StevenPotter          |         Owner:  nobody    
            Status:  new                   |     Milestone:            
         Component:  django.contrib.admin  |       Version:  1.0-beta-1
        Resolution:                        |      Keywords:            
             Stage:  Accepted              |     Has_patch:  1        
        Needs_docs:  0                     |   Needs_tests:  0        
Needs_better_patch:  0                     |  
-------------------------------------------+------------------------------- -
Comment (by bendavis78):

 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

--
Ticket URL: <http://code.djangoproject.com/ticket/8528#comment:6>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

#8528: Admin list_filter doesn't respect null=True
-------------------------------------------+------------------------------- -
          Reporter:  StevenPotter          |         Owner:  nobody    
            Status:  new                   |     Milestone:            
         Component:  django.contrib.admin  |       Version:  1.0-beta-1
        Resolution:                        |      Keywords:            
             Stage:  Accepted              |     Has_patch:  1        
        Needs_docs:  0                     |   Needs_tests:  0        
Needs_better_patch:  0                     |  
-------------------------------------------+------------------------------- -
Changes (by marcob):

 * cc: marcob...@gmail.com (added)

--
Ticket URL: <http://code.djangoproject.com/ticket/8528#comment:7>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

#8528: Admin list_filter doesn't respect null=True
-------------------------------------------+------------------------------- -
          Reporter:  StevenPotter          |         Owner:  nobody    
            Status:  new                   |     Milestone:            
         Component:  django.contrib.admin  |       Version:  1.0-beta-1
        Resolution:                        |      Keywords:            
             Stage:  Accepted              |     Has_patch:  1        
        Needs_docs:  0                     |   Needs_tests:  0        
Needs_better_patch:  0                     |  
-------------------------------------------+------------------------------- -
Comment (by marcob):

 Patch for Django 1.1.1 and that uses:
 {{{
  "(%s)" % _("None")
 }}}

--
Ticket URL: <http://code.djangoproject.com/ticket/8528#comment:8>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

#8528: Admin list_filter doesn't respect null=True
-------------------------------------------+------------------------------- -
          Reporter:  StevenPotter          |         Owner:  nobody    
            Status:  new                   |     Milestone:            
         Component:  django.contrib.admin  |       Version:  1.0-beta-1
        Resolution:                        |      Keywords:            
             Stage:  Accepted              |     Has_patch:  1        
        Needs_docs:  0                     |   Needs_tests:  0        
Needs_better_patch:  0                     |  
-------------------------------------------+------------------------------- -
Changes (by danfairs):

 * cc: danfairs (added)

--
Ticket URL: <http://code.djangoproject.com/ticket/8528#comment:9>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

#8528: Admin list_filter doesn't respect null=True
-------------------------------------------+------------------------------- -
          Reporter:  StevenPotter          |         Owner:  nobody    
            Status:  new                   |     Milestone:            
         Component:  django.contrib.admin  |       Version:  1.0-beta-1
        Resolution:                        |      Keywords:            
             Stage:  Accepted              |     Has_patch:  1        
        Needs_docs:  0                     |   Needs_tests:  0        
Needs_better_patch:  0                     |  
-------------------------------------------+------------------------------- -
Changes (by DrMeers):

 * cc: drme...@gmail.com (added)

--
Ticket URL: <http://code.djangoproject.com/ticket/8528#comment:10>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

#8528: Admin list_filter doesn't respect null=True
-------------------------------------------+------------------------------- -
          Reporter:  StevenPotter          |         Owner:  nobody    
            Status:  new                   |     Milestone:            
         Component:  django.contrib.admin  |       Version:  1.0-beta-1
        Resolution:                        |      Keywords:            
             Stage:  Accepted              |     Has_patch:  1        
        Needs_docs:  0                     |   Needs_tests:  0        
Needs_better_patch:  0                     |  
-------------------------------------------+------------------------------- -
Changes (by a...@adamforster.org):

 * cc: a...@adamforster.org (added)

--
Ticket URL: <http://code.djangoproject.com/ticket/8528#comment:11>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

#8528: Admin list_filter doesn't respect null=True
-------------------------------------------+------------------------------- -
          Reporter:  StevenPotter          |         Owner:  nobody    
            Status:  new                   |     Milestone:            
         Component:  django.contrib.admin  |       Version:  1.0-beta-1
        Resolution:                        |      Keywords:            
             Stage:  Accepted              |     Has_patch:  1        
        Needs_docs:  0                     |   Needs_tests:  0        
Needs_better_patch:  0                     |  
-------------------------------------------+------------------------------- -
Comment (by bendavis78):

 Just FYI: this patches cleanly against 1.2-beta.

--
Ticket URL: <http://code.djangoproject.com/ticket/8528#comment:12>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

#8528: Admin list_filter doesn't respect null=True
-------------------------------------------+------------------------------- -
          Reporter:  StevenPotter          |         Owner:  nobody    
            Status:  new                   |     Milestone:            
         Component:  django.contrib.admin  |       Version:  1.0-beta-1
        Resolution:                        |      Keywords:            
             Stage:  Accepted              |     Has_patch:  1        
        Needs_docs:  0                     |   Needs_tests:  0        
Needs_better_patch:  0                     |  
-------------------------------------------+------------------------------- -
Comment (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 ?

--
Ticket URL: <http://code.djangoproject.com/ticket/8528#comment:13>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

#8528: Admin list_filter doesn't respect null=True
-------------------------------------------+------------------------------- -
          Reporter:  StevenPotter          |         Owner:  nobody  
            Status:  new                   |     Milestone:          
         Component:  django.contrib.admin  |       Version:  1.3-alpha
        Resolution:                        |      Keywords:          
             Stage:  Accepted              |     Has_patch:  1        
        Needs_docs:  0                     |   Needs_tests:  0        
Needs_better_patch:  0                     |  
-------------------------------------------+------------------------------- -
Changes (by simon29):

  * version:  1.0-beta-1 => 1.3-alpha

Comment:

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

--
Ticket URL: <http://code.djangoproject.com/ticket/8528#comment:14>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

#8528: Admin list_filter doesn't respect null=True
-------------------------------------------+------------------------------- -
          Reporter:  StevenPotter          |         Owner:  nobody  
            Status:  new                   |     Milestone:          
         Component:  django.contrib.admin  |       Version:  1.3-alpha
        Resolution:                        |      Keywords:          
             Stage:  Accepted              |     Has_patch:  1        
        Needs_docs:  0                     |   Needs_tests:  0        
Needs_better_patch:  0                     |  
-------------------------------------------+------------------------------- -
Comment (by simon29):

 See also #5833

--
Ticket URL: <http://code.djangoproject.com/ticket/8528#comment:15>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

#8528: Admin list_filter doesn't respect null=True
-------------------------------------------+------------------------------- -
          Reporter:  StevenPotter          |         Owner:  oyvind  
            Status:  assigned              |     Milestone:          
         Component:  django.contrib.admin  |       Version:  1.3-alpha
        Resolution:                        |      Keywords:          
             Stage:  Accepted              |     Has_patch:  1        
        Needs_docs:  0                     |   Needs_tests:  0        
Needs_better_patch:  0                     |  
-------------------------------------------+------------------------------- -
Changes (by oyvind):

  * owner:  nobody => oyvind
  * status:  new => assigned

--
Ticket URL: <http://code.djangoproject.com/ticket/8528#comment:16>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

#8528: Admin list_filter doesn't respect null=True
-------------------------------------------+------------------------------- -
          Reporter:  StevenPotter          |         Owner:  oyvind  
            Status:  assigned              |     Milestone:  1.3      
         Component:  django.contrib.admin  |       Version:  1.3-alpha
        Resolution:                        |      Keywords:          
             Stage:  Accepted              |     Has_patch:  1        
        Needs_docs:  0                     |   Needs_tests:  0        
Needs_better_patch:  0                     |  
-------------------------------------------+------------------------------- -
Changes (by oyvind):

  * milestone:  => 1.3

--
Ticket URL: <http://code.djangoproject.com/ticket/8528#comment:17>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

#8528: Admin list_filter doesn't respect null=True
-------------------------------------------+------------------------------- -
          Reporter:  StevenPotter          |         Owner:  oyvind  
            Status:  assigned              |     Milestone:  1.3      
         Component:  django.contrib.admin  |       Version:  1.3-alpha
        Resolution:                        |      Keywords:          
             Stage:  Accepted              |     Has_patch:  1        
        Needs_docs:  0                     |   Needs_tests:  0        
Needs_better_patch:  0                     |  
-------------------------------------------+------------------------------- -
Comment (by julien):

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

--
Ticket URL: <http://code.djangoproject.com/ticket/8528#comment:18>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

#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):

  * status:  assigned => new
  * owner:  oyvind => julien

Comment:

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

--
Ticket URL: <http://code.djangoproject.com/ticket/8528#comment:19>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

#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                     |  
------------------------------------------------+-------------------------- -

Comment (by julien):

 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.

--
Ticket URL: <http://code.djangoproject.com/ticket/8528#comment:20>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

#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:  1                     |  
------------------------------------------------+-------------------------- -
Changes (by DrMeers):

  * needs_better_patch:  0 => 1

Comment:

 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.

--
Ticket URL: <http://code.djangoproject.com/ticket/8528#comment:21>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

#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.

#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:  Ready for checkin     |     Has patch:  1        
    Needs documentation:  0                     |   Needs tests:  0        
Patch needs improvement:  0                     |  
------------------------------------------------+-------------------------- -
Changes (by DrMeers):

  * stage:  Accepted => Ready for checkin

Comment:

 Great work, thanks Julien. The only thing that caught my eye was the
 implicit boolean logic grouping when checking for `RelatedObject`s, 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?

--
Ticket URL: <http://code.djangoproject.com/ticket/8528#comment:23>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

#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:  Ready for checkin     |     Has patch:  1        
    Needs documentation:  0                     |   Needs tests:  0        
Patch needs improvement:  0                     |  
------------------------------------------------+-------------------------- -

Comment (by russellm):

 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.

  2. 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.

--
Ticket URL: <http://code.djangoproject.com/ticket/8528#comment:24>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.