From 4d70d48ecbfe2b972c55c3b4fa1237dfe54f5b6b Mon Sep 17 00:00:00 2001 From: Russell Keith-Magee Date: Fri, 25 Feb 2011 15:14:26 +0000 Subject: [PATCH] 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. git-svn-id: http://code.djangoproject.com/svn/django/trunk@15649 bcc190cf-cafb-0310-a4f2-bffc1f526a37 --- django/contrib/admin/filterspecs.py | 97 ++++++++-- .../admin_filterspecs/__init__.py | 0 .../admin_filterspecs/models.py | 11 ++ .../admin_filterspecs/tests.py | 171 ++++++++++++++++++ 4 files changed, 260 insertions(+), 19 deletions(-) create mode 100644 tests/regressiontests/admin_filterspecs/__init__.py create mode 100644 tests/regressiontests/admin_filterspecs/models.py create mode 100644 tests/regressiontests/admin_filterspecs/tests.py diff --git a/django/contrib/admin/filterspecs.py b/django/contrib/admin/filterspecs.py index eab5407cc7e..d887af2b124 100644 --- a/django/contrib/admin/filterspecs.py +++ b/django/contrib/admin/filterspecs.py @@ -76,23 +76,46 @@ class RelatedFilterSpec(FilterSpec): self.lookup_title = f.verbose_name # use field name rel_name = other_model._meta.pk.name self.lookup_kwarg = '%s__%s__exact' % (self.field_path, rel_name) + self.lookup_kwarg_isnull = '%s__isnull' % (self.field_path) self.lookup_val = request.GET.get(self.lookup_kwarg, None) + self.lookup_val_isnull = request.GET.get( + self.lookup_kwarg_isnull, None) self.lookup_choices = f.get_choices(include_blank=False) def has_output(self): - return len(self.lookup_choices) > 1 + if isinstance(self.field, models.related.RelatedObject) \ + and self.field.field.null or hasattr(self.field, 'rel') \ + and self.field.null: + extra = 1 + else: + extra = 0 + return len(self.lookup_choices) + extra > 1 def title(self): return self.lookup_title def choices(self, cl): - yield {'selected': self.lookup_val is None, - 'query_string': cl.get_query_string({}, [self.lookup_kwarg]), + from django.contrib.admin.views.main import EMPTY_CHANGELIST_VALUE + yield {'selected': self.lookup_val is None + and not self.lookup_val_isnull, + 'query_string': cl.get_query_string( + {}, + [self.lookup_kwarg, self.lookup_kwarg_isnull]), 'display': _('All')} for pk_val, val in self.lookup_choices: yield {'selected': self.lookup_val == smart_unicode(pk_val), - 'query_string': cl.get_query_string({self.lookup_kwarg: pk_val}), + 'query_string': cl.get_query_string( + {self.lookup_kwarg: pk_val}, + [self.lookup_kwarg_isnull]), 'display': val} + if isinstance(self.field, models.related.RelatedObject) \ + and self.field.field.null or hasattr(self.field, 'rel') \ + and self.field.null: + yield {'selected': bool(self.lookup_val_isnull), + 'query_string': cl.get_query_string( + {self.lookup_kwarg_isnull: 'True'}, + [self.lookup_kwarg]), + 'display': EMPTY_CHANGELIST_VALUE} FilterSpec.register(lambda f: ( hasattr(f, 'rel') and bool(f.rel) or @@ -113,7 +136,8 @@ class ChoicesFilterSpec(FilterSpec): 'display': _('All')} for k, v in self.field.flatchoices: yield {'selected': smart_unicode(k) == self.lookup_val, - 'query_string': cl.get_query_string({self.lookup_kwarg: k}), + 'query_string': cl.get_query_string( + {self.lookup_kwarg: k}), 'display': v} FilterSpec.register(lambda f: bool(f.choices), ChoicesFilterSpec) @@ -127,11 +151,14 @@ class DateFieldFilterSpec(FilterSpec): self.field_generic = '%s__' % self.field_path - self.date_params = dict([(k, v) for k, v in params.items() if k.startswith(self.field_generic)]) + self.date_params = dict([(k, v) for k, v in params.items() + if k.startswith(self.field_generic)]) today = datetime.date.today() one_week_ago = today - datetime.timedelta(days=7) - today_str = isinstance(self.field, models.DateTimeField) and today.strftime('%Y-%m-%d 23:59:59') or today.strftime('%Y-%m-%d') + today_str = isinstance(self.field, models.DateTimeField) \ + and today.strftime('%Y-%m-%d 23:59:59') \ + or today.strftime('%Y-%m-%d') self.links = ( (_('Any date'), {}), @@ -152,10 +179,13 @@ class DateFieldFilterSpec(FilterSpec): def choices(self, cl): for title, param_dict in self.links: yield {'selected': self.date_params == param_dict, - 'query_string': cl.get_query_string(param_dict, [self.field_generic]), + 'query_string': cl.get_query_string( + param_dict, + [self.field_generic]), 'display': title} -FilterSpec.register(lambda f: isinstance(f, models.DateField), DateFieldFilterSpec) +FilterSpec.register(lambda f: isinstance(f, models.DateField), + DateFieldFilterSpec) class BooleanFieldFilterSpec(FilterSpec): def __init__(self, f, request, params, model, model_admin, @@ -174,14 +204,20 @@ class BooleanFieldFilterSpec(FilterSpec): def choices(self, cl): for k, v in ((_('All'), None), (_('Yes'), '1'), (_('No'), '0')): yield {'selected': self.lookup_val == v and not self.lookup_val2, - 'query_string': cl.get_query_string({self.lookup_kwarg: v}, [self.lookup_kwarg2]), + 'query_string': cl.get_query_string( + {self.lookup_kwarg: v}, + [self.lookup_kwarg2]), 'display': k} if isinstance(self.field, models.NullBooleanField): yield {'selected': self.lookup_val2 == 'True', - 'query_string': cl.get_query_string({self.lookup_kwarg2: 'True'}, [self.lookup_kwarg]), + 'query_string': cl.get_query_string( + {self.lookup_kwarg2: 'True'}, + [self.lookup_kwarg]), 'display': _('Unknown')} -FilterSpec.register(lambda f: isinstance(f, models.BooleanField) or isinstance(f, models.NullBooleanField), BooleanFieldFilterSpec) +FilterSpec.register(lambda f: isinstance(f, models.BooleanField) + or isinstance(f, models.NullBooleanField), + BooleanFieldFilterSpec) # This should be registered last, because it's a last resort. For example, # if a field is eligible to use the BooleanFieldFilterSpec, that'd be much @@ -192,8 +228,12 @@ class AllValuesFilterSpec(FilterSpec): super(AllValuesFilterSpec, self).__init__(f, request, params, model, model_admin, field_path=field_path) - self.lookup_val = request.GET.get(self.field_path, None) - parent_model, reverse_path = reverse_field_path(model, field_path) + self.lookup_kwarg = self.field_path + self.lookup_kwarg_isnull = '%s__isnull' % self.field_path + self.lookup_val = request.GET.get(self.lookup_kwarg, None) + self.lookup_val_isnull = request.GET.get(self.lookup_kwarg_isnull, + None) + parent_model, reverse_path = reverse_field_path(model, self.field_path) queryset = parent_model._default_manager.all() # optional feature: limit choices base on existing relationships # queryset = queryset.complex_filter( @@ -202,18 +242,37 @@ class AllValuesFilterSpec(FilterSpec): queryset = queryset.filter(limit_choices_to) self.lookup_choices = \ - queryset.distinct().order_by(f.name).values(f.name) + queryset.distinct().order_by(f.name).values_list(f.name, flat=True) def title(self): return self.field.verbose_name def choices(self, cl): - yield {'selected': self.lookup_val is None, - 'query_string': cl.get_query_string({}, [self.field_path]), + from django.contrib.admin.views.main import EMPTY_CHANGELIST_VALUE + yield {'selected': self.lookup_val is None + and self.lookup_val_isnull is None, + 'query_string': cl.get_query_string( + {}, + [self.lookup_kwarg, self.lookup_kwarg_isnull]), 'display': _('All')} + include_none = False + for val in self.lookup_choices: - val = smart_unicode(val[self.field.name]) + if val is None: + include_none = True + continue + val = smart_unicode(val) + yield {'selected': self.lookup_val == val, - 'query_string': cl.get_query_string({self.field_path: val}), + 'query_string': cl.get_query_string( + {self.lookup_kwarg: val}, + [self.lookup_kwarg_isnull]), 'display': val} + if include_none: + yield {'selected': bool(self.lookup_val_isnull), + 'query_string': cl.get_query_string( + {self.lookup_kwarg_isnull: 'True'}, + [self.lookup_kwarg]), + 'display': EMPTY_CHANGELIST_VALUE} + FilterSpec.register(lambda f: True, AllValuesFilterSpec) diff --git a/tests/regressiontests/admin_filterspecs/__init__.py b/tests/regressiontests/admin_filterspecs/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/regressiontests/admin_filterspecs/models.py b/tests/regressiontests/admin_filterspecs/models.py new file mode 100644 index 00000000000..fe9fdfe1bdf --- /dev/null +++ b/tests/regressiontests/admin_filterspecs/models.py @@ -0,0 +1,11 @@ +from django.db import models +from django.contrib.auth.models import User + +class Book(models.Model): + title = models.CharField(max_length=25) + year = models.PositiveIntegerField(null=True, blank=True) + author = models.ForeignKey(User, related_name='books_authored', blank=True, null=True) + contributors = models.ManyToManyField(User, related_name='books_contributed', blank=True, null=True) + + def __unicode__(self): + return self.title diff --git a/tests/regressiontests/admin_filterspecs/tests.py b/tests/regressiontests/admin_filterspecs/tests.py new file mode 100644 index 00000000000..64a6ac700ed --- /dev/null +++ b/tests/regressiontests/admin_filterspecs/tests.py @@ -0,0 +1,171 @@ +from django.contrib.auth.admin import UserAdmin +from django.test import TestCase +from django.test.client import RequestFactory +from django.contrib.auth.models import User +from django.contrib import admin +from django.contrib.admin.views.main import ChangeList +from django.utils.encoding import force_unicode + +from models import Book + +class FilterSpecsTests(TestCase): + + def setUp(self): + # Users + alfred = User.objects.create_user('alfred', 'alfred@example.com') + bob = User.objects.create_user('bob', 'alfred@example.com') + lisa = User.objects.create_user('lisa', 'lisa@example.com') + + #Books + bio_book = Book.objects.create(title='Django: a biography', year=1999, author=alfred) + django_book = Book.objects.create(title='The Django Book', year=None, author=bob) + gipsy_book = Book.objects.create(title='Gipsy guitar for dummies', year=2002) + gipsy_book.contributors = [bob, lisa] + gipsy_book.save() + + self.request_factory = RequestFactory() + + + def get_changelist(self, request, model, modeladmin): + return ChangeList(request, model, modeladmin.list_display, modeladmin.list_display_links, + modeladmin.list_filter, modeladmin.date_hierarchy, modeladmin.search_fields, + modeladmin.list_select_related, modeladmin.list_per_page, modeladmin.list_editable, modeladmin) + + def test_AllValuesFilterSpec(self): + modeladmin = BookAdmin(Book, admin.site) + + request = self.request_factory.get('/', {'year__isnull': 'True'}) + changelist = self.get_changelist(request, Book, modeladmin) + + # Make sure changelist.get_query_set() does not raise IncorrectLookupParameters + queryset = changelist.get_query_set() + + # Make sure the last choice is None and is selected + filterspec = changelist.get_filters(request)[0][0] + self.assertEquals(force_unicode(filterspec.title()), u'year') + choices = list(filterspec.choices(changelist)) + self.assertEquals(choices[-1]['selected'], True) + self.assertEquals(choices[-1]['query_string'], '?year__isnull=True') + + request = self.request_factory.get('/', {'year': '2002'}) + changelist = self.get_changelist(request, Book, modeladmin) + + # Make sure the correct choice is selected + filterspec = changelist.get_filters(request)[0][0] + self.assertEquals(force_unicode(filterspec.title()), u'year') + choices = list(filterspec.choices(changelist)) + self.assertEquals(choices[2]['selected'], True) + self.assertEquals(choices[2]['query_string'], '?year=2002') + + def test_RelatedFilterSpec_ForeignKey(self): + modeladmin = BookAdmin(Book, admin.site) + + request = self.request_factory.get('/', {'author__isnull': 'True'}) + changelist = ChangeList(request, Book, modeladmin.list_display, modeladmin.list_display_links, + modeladmin.list_filter, modeladmin.date_hierarchy, modeladmin.search_fields, + modeladmin.list_select_related, modeladmin.list_per_page, modeladmin.list_editable, modeladmin) + + # Make sure changelist.get_query_set() does not raise IncorrectLookupParameters + queryset = changelist.get_query_set() + + # Make sure the last choice is None and is selected + filterspec = changelist.get_filters(request)[0][1] + self.assertEquals(force_unicode(filterspec.title()), u'author') + choices = list(filterspec.choices(changelist)) + self.assertEquals(choices[-1]['selected'], True) + self.assertEquals(choices[-1]['query_string'], '?author__isnull=True') + + request = self.request_factory.get('/', {'author__id__exact': '1'}) + changelist = self.get_changelist(request, Book, modeladmin) + + # Make sure the correct choice is selected + filterspec = changelist.get_filters(request)[0][1] + self.assertEquals(force_unicode(filterspec.title()), u'author') + choices = list(filterspec.choices(changelist)) + self.assertEquals(choices[1]['selected'], True) + self.assertEquals(choices[1]['query_string'], '?author__id__exact=1') + + def test_RelatedFilterSpec_ManyToMany(self): + modeladmin = BookAdmin(Book, admin.site) + + request = self.request_factory.get('/', {'contributors__isnull': 'True'}) + changelist = self.get_changelist(request, Book, modeladmin) + + # Make sure changelist.get_query_set() does not raise IncorrectLookupParameters + queryset = changelist.get_query_set() + + # Make sure the last choice is None and is selected + filterspec = changelist.get_filters(request)[0][2] + self.assertEquals(force_unicode(filterspec.title()), u'user') + choices = list(filterspec.choices(changelist)) + self.assertEquals(choices[-1]['selected'], True) + self.assertEquals(choices[-1]['query_string'], '?contributors__isnull=True') + + request = self.request_factory.get('/', {'contributors__id__exact': '2'}) + changelist = self.get_changelist(request, Book, modeladmin) + + # Make sure the correct choice is selected + filterspec = changelist.get_filters(request)[0][2] + self.assertEquals(force_unicode(filterspec.title()), u'user') + choices = list(filterspec.choices(changelist)) + self.assertEquals(choices[2]['selected'], True) + self.assertEquals(choices[2]['query_string'], '?contributors__id__exact=2') + + + def test_RelatedFilterSpec_reverse_relationships(self): + modeladmin = CustomUserAdmin(User, admin.site) + + # FK relationship ----- + request = self.request_factory.get('/', {'books_authored__isnull': 'True'}) + changelist = self.get_changelist(request, User, modeladmin) + + # Make sure changelist.get_query_set() does not raise IncorrectLookupParameters + queryset = changelist.get_query_set() + + # Make sure the last choice is None and is selected + filterspec = changelist.get_filters(request)[0][0] + self.assertEquals(force_unicode(filterspec.title()), u'book') + choices = list(filterspec.choices(changelist)) + self.assertEquals(choices[-1]['selected'], True) + self.assertEquals(choices[-1]['query_string'], '?books_authored__isnull=True') + + request = self.request_factory.get('/', {'books_authored__id__exact': '1'}) + changelist = self.get_changelist(request, User, modeladmin) + + # Make sure the correct choice is selected + filterspec = changelist.get_filters(request)[0][0] + self.assertEquals(force_unicode(filterspec.title()), u'book') + choices = list(filterspec.choices(changelist)) + self.assertEquals(choices[1]['selected'], True) + self.assertEquals(choices[1]['query_string'], '?books_authored__id__exact=1') + + # M2M relationship ----- + request = self.request_factory.get('/', {'books_contributed__isnull': 'True'}) + changelist = self.get_changelist(request, User, modeladmin) + + # Make sure changelist.get_query_set() does not raise IncorrectLookupParameters + queryset = changelist.get_query_set() + + # Make sure the last choice is None and is selected + filterspec = changelist.get_filters(request)[0][1] + self.assertEquals(force_unicode(filterspec.title()), u'book') + choices = list(filterspec.choices(changelist)) + self.assertEquals(choices[-1]['selected'], True) + self.assertEquals(choices[-1]['query_string'], '?books_contributed__isnull=True') + + request = self.request_factory.get('/', {'books_contributed__id__exact': '2'}) + changelist = self.get_changelist(request, User, modeladmin) + + # Make sure the correct choice is selected + filterspec = changelist.get_filters(request)[0][1] + self.assertEquals(force_unicode(filterspec.title()), u'book') + choices = list(filterspec.choices(changelist)) + self.assertEquals(choices[2]['selected'], True) + self.assertEquals(choices[2]['query_string'], '?books_contributed__id__exact=2') + +class CustomUserAdmin(UserAdmin): + list_filter = ('books_authored', 'books_contributed') + +class BookAdmin(admin.ModelAdmin): + list_filter = ('year', 'author', 'contributors') + order_by = '-id'