diff --git a/django/contrib/admin/filterspecs.py b/django/contrib/admin/filterspecs.py index 6f643ee102..eab5407cc7 100644 --- a/django/contrib/admin/filterspecs.py +++ b/django/contrib/admin/filterspecs.py @@ -11,22 +11,32 @@ from django.utils.encoding import smart_unicode, iri_to_uri from django.utils.translation import ugettext as _ from django.utils.html import escape from django.utils.safestring import mark_safe +from django.contrib.admin.util import get_model_from_relation, \ + reverse_field_path, get_limit_choices_to_from_path import datetime class FilterSpec(object): filter_specs = [] - def __init__(self, f, request, params, model, model_admin): + def __init__(self, f, request, params, model, model_admin, + field_path=None): self.field = f self.params = params + self.field_path = field_path + if field_path is None: + if isinstance(f, models.related.RelatedObject): + self.field_path = f.var_name + else: + self.field_path = f.name def register(cls, test, factory): cls.filter_specs.append((test, factory)) register = classmethod(register) - def create(cls, f, request, params, model, model_admin): + def create(cls, f, request, params, model, model_admin, field_path=None): for test, factory in cls.filter_specs: if test(f): - return factory(f, request, params, model, model_admin) + return factory(f, request, params, model, model_admin, + field_path=field_path) create = classmethod(create) def has_output(self): @@ -52,14 +62,20 @@ class FilterSpec(object): return mark_safe("".join(t)) class RelatedFilterSpec(FilterSpec): - def __init__(self, f, request, params, model, model_admin): - super(RelatedFilterSpec, self).__init__(f, request, params, model, model_admin) - if isinstance(f, models.ManyToManyField): - self.lookup_title = f.rel.to._meta.verbose_name + def __init__(self, f, request, params, model, model_admin, + field_path=None): + super(RelatedFilterSpec, self).__init__( + f, request, params, model, model_admin, field_path=field_path) + + other_model = get_model_from_relation(f) + if isinstance(f, (models.ManyToManyField, + models.related.RelatedObject)): + # no direct field on this model, get name from other model + self.lookup_title = other_model._meta.verbose_name else: - self.lookup_title = f.verbose_name - rel_name = f.rel.get_related_field().name - self.lookup_kwarg = '%s__%s__exact' % (f.name, rel_name) + 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_val = request.GET.get(self.lookup_kwarg, None) self.lookup_choices = f.get_choices(include_blank=False) @@ -78,12 +94,17 @@ class RelatedFilterSpec(FilterSpec): 'query_string': cl.get_query_string({self.lookup_kwarg: pk_val}), 'display': val} -FilterSpec.register(lambda f: bool(f.rel), RelatedFilterSpec) +FilterSpec.register(lambda f: ( + hasattr(f, 'rel') and bool(f.rel) or + isinstance(f, models.related.RelatedObject)), RelatedFilterSpec) class ChoicesFilterSpec(FilterSpec): - def __init__(self, f, request, params, model, model_admin): - super(ChoicesFilterSpec, self).__init__(f, request, params, model, model_admin) - self.lookup_kwarg = '%s__exact' % f.name + def __init__(self, f, request, params, model, model_admin, + field_path=None): + super(ChoicesFilterSpec, self).__init__(f, request, params, model, + model_admin, + field_path=field_path) + self.lookup_kwarg = '%s__exact' % self.field_path self.lookup_val = request.GET.get(self.lookup_kwarg, None) def choices(self, cl): @@ -98,10 +119,13 @@ class ChoicesFilterSpec(FilterSpec): FilterSpec.register(lambda f: bool(f.choices), ChoicesFilterSpec) class DateFieldFilterSpec(FilterSpec): - def __init__(self, f, request, params, model, model_admin): - super(DateFieldFilterSpec, self).__init__(f, request, params, model, model_admin) + def __init__(self, f, request, params, model, model_admin, + field_path=None): + super(DateFieldFilterSpec, self).__init__(f, request, params, model, + model_admin, + field_path=field_path) - self.field_generic = '%s__' % self.field.name + 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)]) @@ -111,14 +135,15 @@ class DateFieldFilterSpec(FilterSpec): self.links = ( (_('Any date'), {}), - (_('Today'), {'%s__year' % self.field.name: str(today.year), - '%s__month' % self.field.name: str(today.month), - '%s__day' % self.field.name: str(today.day)}), - (_('Past 7 days'), {'%s__gte' % self.field.name: one_week_ago.strftime('%Y-%m-%d'), - '%s__lte' % f.name: today_str}), - (_('This month'), {'%s__year' % self.field.name: str(today.year), - '%s__month' % f.name: str(today.month)}), - (_('This year'), {'%s__year' % self.field.name: str(today.year)}) + (_('Today'), {'%s__year' % self.field_path: str(today.year), + '%s__month' % self.field_path: str(today.month), + '%s__day' % self.field_path: str(today.day)}), + (_('Past 7 days'), {'%s__gte' % self.field_path: + one_week_ago.strftime('%Y-%m-%d'), + '%s__lte' % self.field_path: today_str}), + (_('This month'), {'%s__year' % self.field_path: str(today.year), + '%s__month' % self.field_path: str(today.month)}), + (_('This year'), {'%s__year' % self.field_path: str(today.year)}) ) def title(self): @@ -133,10 +158,13 @@ class DateFieldFilterSpec(FilterSpec): FilterSpec.register(lambda f: isinstance(f, models.DateField), DateFieldFilterSpec) class BooleanFieldFilterSpec(FilterSpec): - def __init__(self, f, request, params, model, model_admin): - super(BooleanFieldFilterSpec, self).__init__(f, request, params, model, model_admin) - self.lookup_kwarg = '%s__exact' % f.name - self.lookup_kwarg2 = '%s__isnull' % f.name + def __init__(self, f, request, params, model, model_admin, + field_path=None): + super(BooleanFieldFilterSpec, self).__init__(f, request, params, model, + model_admin, + field_path=field_path) + self.lookup_kwarg = '%s__exact' % self.field_path + self.lookup_kwarg2 = '%s__isnull' % self.field_path self.lookup_val = request.GET.get(self.lookup_kwarg, None) self.lookup_val2 = request.GET.get(self.lookup_kwarg2, None) @@ -159,21 +187,33 @@ FilterSpec.register(lambda f: isinstance(f, models.BooleanField) or isinstance(f # if a field is eligible to use the BooleanFieldFilterSpec, that'd be much # more appropriate, and the AllValuesFilterSpec won't get used for it. class AllValuesFilterSpec(FilterSpec): - def __init__(self, f, request, params, model, model_admin): - super(AllValuesFilterSpec, self).__init__(f, request, params, model, model_admin) - self.lookup_val = request.GET.get(f.name, None) - self.lookup_choices = model_admin.queryset(request).distinct().order_by(f.name).values(f.name) + def __init__(self, f, request, params, model, model_admin, + field_path=None): + 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) + queryset = parent_model._default_manager.all() + # optional feature: limit choices base on existing relationships + # queryset = queryset.complex_filter( + # {'%s__isnull' % reverse_path: False}) + limit_choices_to = get_limit_choices_to_from_path(model, field_path) + queryset = queryset.filter(limit_choices_to) + + self.lookup_choices = \ + queryset.distinct().order_by(f.name).values(f.name) 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.name]), + 'query_string': cl.get_query_string({}, [self.field_path]), 'display': _('All')} for val in self.lookup_choices: val = smart_unicode(val[self.field.name]) yield {'selected': self.lookup_val == val, - 'query_string': cl.get_query_string({self.field.name: val}), + 'query_string': cl.get_query_string({self.field_path: val}), 'display': val} FilterSpec.register(lambda f: True, AllValuesFilterSpec) diff --git a/django/contrib/admin/util.py b/django/contrib/admin/util.py index 3d076f7baa..c6411aed35 100644 --- a/django/contrib/admin/util.py +++ b/django/contrib/admin/util.py @@ -1,4 +1,5 @@ from django.db import models +from django.db.models.sql.constants import LOOKUP_SEP from django.db.models.deletion import Collector from django.db.models.related import RelatedObject from django.forms.forms import pretty_name @@ -280,3 +281,95 @@ def display_for_field(value, field): return formats.number_format(value) else: return smart_unicode(value) + + +class NotRelationField(Exception): + pass + + +def get_model_from_relation(field): + if isinstance(field, models.related.RelatedObject): + return field.model + elif getattr(field, 'rel'): # or isinstance? + return field.rel.to + else: + raise NotRelationField + + +def reverse_field_path(model, path): + """ Create a reversed field path. + + E.g. Given (Order, "user__groups"), + return (Group, "user__order"). + + Final field must be a related model, not a data field. + + """ + reversed_path = [] + parent = model + pieces = path.split(LOOKUP_SEP) + for piece in pieces: + field, model, direct, m2m = parent._meta.get_field_by_name(piece) + # skip trailing data field if extant: + if len(reversed_path) == len(pieces)-1: # final iteration + try: + get_model_from_relation(field) + except NotRelationField: + break + if direct: + related_name = field.related_query_name() + parent = field.rel.to + else: + related_name = field.field.name + parent = field.model + reversed_path.insert(0, related_name) + return (parent, LOOKUP_SEP.join(reversed_path)) + + +def get_fields_from_path(model, path): + """ Return list of Fields given path relative to model. + + e.g. (ModelX, "user__groups__name") -> [ + , + , + , + ] + """ + pieces = path.split(LOOKUP_SEP) + fields = [] + for piece in pieces: + if fields: + parent = get_model_from_relation(fields[-1]) + else: + parent = model + fields.append(parent._meta.get_field_by_name(piece)[0]) + return fields + + +def remove_trailing_data_field(fields): + """ Discard trailing non-relation field if extant. """ + try: + get_model_from_relation(fields[-1]) + except NotRelationField: + fields = fields[:-1] + return fields + + +def get_limit_choices_to_from_path(model, path): + """ Return Q object for limiting choices if applicable. + + If final model in path is linked via a ForeignKey or ManyToManyField which + has a `limit_choices_to` attribute, return it as a Q object. + """ + + fields = get_fields_from_path(model, path) + fields = remove_trailing_data_field(fields) + limit_choices_to = ( + fields and hasattr(fields[-1], 'rel') and + getattr(fields[-1].rel, 'limit_choices_to', None)) + if not limit_choices_to: + return models.Q() # empty Q + elif isinstance(limit_choices_to, models.Q): + return limit_choices_to # already a Q + else: + return models.Q(**limit_choices_to) # convert dict to Q diff --git a/django/contrib/admin/validation.py b/django/contrib/admin/validation.py index 0434fc64f7..027db6383e 100644 --- a/django/contrib/admin/validation.py +++ b/django/contrib/admin/validation.py @@ -1,7 +1,9 @@ from django.core.exceptions import ImproperlyConfigured from django.db import models +from django.db.models.fields import FieldDoesNotExist from django.forms.models import (BaseModelForm, BaseModelFormSet, fields_for_model, _get_foreign_key) +from django.contrib.admin.util import get_fields_from_path, NotRelationField from django.contrib.admin.options import flatten_fieldsets, BaseModelAdmin from django.contrib.admin.options import HORIZONTAL, VERTICAL @@ -53,8 +55,15 @@ def validate(cls, model): # list_filter if hasattr(cls, 'list_filter'): check_isseq(cls, 'list_filter', cls.list_filter) - for idx, field in enumerate(cls.list_filter): - get_field(cls, model, opts, 'list_filter[%d]' % idx, field) + for idx, fpath in enumerate(cls.list_filter): + try: + get_fields_from_path(model, fpath) + except (NotRelationField, FieldDoesNotExist), e: + raise ImproperlyConfigured( + "'%s.list_filter[%d]' refers to '%s' which does not refer to a Field." % ( + cls.__name__, idx, fpath + ) + ) # list_per_page = 100 if hasattr(cls, 'list_per_page') and not isinstance(cls.list_per_page, int): diff --git a/django/contrib/admin/views/main.py b/django/contrib/admin/views/main.py index e29d44a67c..0ebb7fd2bb 100644 --- a/django/contrib/admin/views/main.py +++ b/django/contrib/admin/views/main.py @@ -1,6 +1,6 @@ from django.contrib.admin.filterspecs import FilterSpec from django.contrib.admin.options import IncorrectLookupParameters -from django.contrib.admin.util import quote +from django.contrib.admin.util import quote, get_fields_from_path from django.core.paginator import Paginator, InvalidPage from django.db import models from django.utils.encoding import force_unicode, smart_str @@ -68,9 +68,11 @@ class ChangeList(object): def get_filters(self, request): filter_specs = [] if self.list_filter: - filter_fields = [self.lookup_opts.get_field(field_name) for field_name in self.list_filter] - for f in filter_fields: - spec = FilterSpec.create(f, request, self.params, self.model, self.model_admin) + for filter_name in self.list_filter: + field = get_fields_from_path(self.model, filter_name)[-1] + spec = FilterSpec.create(field, request, self.params, + self.model, self.model_admin, + field_path=filter_name) if spec and spec.has_output(): filter_specs.append(spec) return filter_specs, bool(filter_specs) diff --git a/django/db/models/related.py b/django/db/models/related.py index e4afd8a6f8..7734230ffa 100644 --- a/django/db/models/related.py +++ b/django/db/models/related.py @@ -1,3 +1,6 @@ +from django.utils.encoding import smart_unicode +from django.db.models.fields import BLANK_CHOICE_DASH + class BoundRelatedObject(object): def __init__(self, related_object, field_mapping, original): self.relation = related_object @@ -18,6 +21,22 @@ class RelatedObject(object): self.name = '%s:%s' % (self.opts.app_label, self.opts.module_name) self.var_name = self.opts.object_name.lower() + def get_choices(self, include_blank=True, blank_choice=BLANK_CHOICE_DASH, + limit_to_currently_related=False): + """Returns choices with a default blank choices included, for use + as SelectField choices for this field. + + Analogue of django.db.models.fields.Field.get_choices, provided + initially for utilisation by RelatedFilterSpec. + """ + first_choice = include_blank and blank_choice or [] + queryset = self.model._default_manager.all() + if limit_to_currently_related: + queryset = queryset.complex_filter( + {'%s__isnull' % self.parent_model._meta.module_name: False}) + lst = [(x._get_pk_val(), smart_unicode(x)) for x in queryset] + return first_choice + lst + def get_db_prep_lookup(self, lookup_type, value, connection, prepared=False): # Defer to the actual field definition for db prep return self.field.get_db_prep_lookup(lookup_type, value, diff --git a/docs/ref/contrib/admin/index.txt b/docs/ref/contrib/admin/index.txt index cc918be922..89478df65b 100644 --- a/docs/ref/contrib/admin/index.txt +++ b/docs/ref/contrib/admin/index.txt @@ -458,6 +458,11 @@ how both ``list_display`` and ``list_filter`` work:: list_display = ('username', 'email', 'first_name', 'last_name', 'is_staff') list_filter = ('is_staff', 'is_superuser') +Fields in ``list_filter`` can also span relations using the ``__`` lookup:: + + class UserAdminWithLookup(UserAdmin): + list_filter = ('groups__name') + The above code results in an admin change list page that looks like this: .. image:: _images/users_changelist.png diff --git a/docs/releases/1.3.txt b/docs/releases/1.3.txt index 24702111a2..fc482b27b4 100644 --- a/docs/releases/1.3.txt +++ b/docs/releases/1.3.txt @@ -159,6 +159,8 @@ requests. These include: ` and :ref:`transformation ` has been added to the cache API. + * Support for lookups spanning relations in admin's ``list_filter``. + .. _backwards-incompatible-changes-1.3: Backwards-incompatible changes in 1.3 diff --git a/tests/regressiontests/admin_views/customadmin.py b/tests/regressiontests/admin_views/customadmin.py index 34e39ef0ca..8cd262b860 100644 --- a/tests/regressiontests/admin_views/customadmin.py +++ b/tests/regressiontests/admin_views/customadmin.py @@ -32,3 +32,4 @@ site.register(models.Article, models.ArticleAdmin) site.register(models.Section, inlines=[models.ArticleInline]) site.register(models.Thing, models.ThingAdmin) site.register(models.Fabric, models.FabricAdmin) +site.register(models.ChapterXtra1, models.ChapterXtra1Admin) diff --git a/tests/regressiontests/admin_views/fixtures/admin-views-books.xml b/tests/regressiontests/admin_views/fixtures/admin-views-books.xml new file mode 100644 index 0000000000..37ded89900 --- /dev/null +++ b/tests/regressiontests/admin_views/fixtures/admin-views-books.xml @@ -0,0 +1,45 @@ + + + + Book 1 + + + Book 2 + + + Promo 1 + 1 + + + Promo 2 + 2 + + + Chapter 1 + [ insert contents here ] + 1 + + + Chapter 2 + [ insert contents here ] + 1 + + + Chapter 1 + [ insert contents here ] + 2 + + + Chapter 2 + [ insert contents here ] + 2 + + + ChapterXtra1 1 + 1 + + + ChapterXtra1 2 + 3 + + diff --git a/tests/regressiontests/admin_views/models.py b/tests/regressiontests/admin_views/models.py index ee0f5d51a1..61ca33904c 100644 --- a/tests/regressiontests/admin_views/models.py +++ b/tests/regressiontests/admin_views/models.py @@ -90,6 +90,14 @@ class ArticleInline(admin.TabularInline): class ChapterInline(admin.TabularInline): model = Chapter +class ChapterXtra1Admin(admin.ModelAdmin): + list_filter = ('chap', + 'chap__title', + 'chap__book', + 'chap__book__name', + 'chap__book__promo', + 'chap__book__promo__name',) + class ArticleAdmin(admin.ModelAdmin): list_display = ('content', 'date', callable_year, 'model_year', 'modeladmin_year') list_filter = ('date',) @@ -168,7 +176,7 @@ class Thing(models.Model): return self.title class ThingAdmin(admin.ModelAdmin): - list_filter = ('color',) + list_filter = ('color', 'color__warm', 'color__value') class Fabric(models.Model): NG_CHOICES = ( @@ -646,7 +654,7 @@ admin.site.register(CyclicTwo) # contrib.admin.util's get_deleted_objects function. admin.site.register(Book, inlines=[ChapterInline]) admin.site.register(Promo) -admin.site.register(ChapterXtra1) +admin.site.register(ChapterXtra1, ChapterXtra1Admin) admin.site.register(Pizza, PizzaAdmin) admin.site.register(Topping) admin.site.register(Album) diff --git a/tests/regressiontests/admin_views/tests.py b/tests/regressiontests/admin_views/tests.py index a3fe34f0bf..79a29d8ab0 100644 --- a/tests/regressiontests/admin_views/tests.py +++ b/tests/regressiontests/admin_views/tests.py @@ -19,6 +19,7 @@ from django.utils import formats from django.utils.cache import get_max_age from django.utils.encoding import iri_to_uri from django.utils.html import escape +from django.utils.http import urlencode from django.utils.translation import activate, deactivate from django.utils import unittest @@ -27,11 +28,12 @@ from models import Article, BarAccount, CustomArticle, EmptyModel, \ FooAccount, Gallery, ModelWithStringPrimaryKey, \ Person, Persona, Picture, Podcast, Section, Subscriber, Vodcast, \ Language, Collector, Widget, Grommet, DooHickey, FancyDoodad, Whatsit, \ - Category, Post, Plot, FunkyTag + Category, Post, Plot, FunkyTag, Chapter, Book, Promo class AdminViewBasicTest(TestCase): - fixtures = ['admin-views-users.xml', 'admin-views-colors.xml', 'admin-views-fabrics.xml'] + fixtures = ['admin-views-users.xml', 'admin-views-colors.xml', + 'admin-views-fabrics.xml', 'admin-views-books.xml'] # Store the bit of the URL where the admin is registered as a class # variable. That way we can test a second AdminSite just by subclassing @@ -204,7 +206,9 @@ class AdminViewBasicTest(TestCase): ) def testLimitedFilter(self): - """Ensure admin changelist filters do not contain objects excluded via limit_choices_to.""" + """Ensure admin changelist filters do not contain objects excluded via limit_choices_to. + This also tests relation-spanning filters (e.g. 'color__value'). + """ response = self.client.get('/test_admin/%s/admin_views/thing/' % self.urlbit) self.failUnlessEqual(response.status_code, 200) self.failUnless( @@ -216,6 +220,47 @@ class AdminViewBasicTest(TestCase): "Changelist filter not correctly limited by limit_choices_to." ) + def testRelationSpanningFilters(self): + response = self.client.get('/test_admin/%s/admin_views/chapterxtra1/' % + self.urlbit) + self.failUnlessEqual(response.status_code, 200) + self.assertContains(response, '
') + filters = { + 'chap__id__exact': dict( + values=[c.id for c in Chapter.objects.all()], + test=lambda obj, value: obj.chap.id == value), + 'chap__title': dict( + values=[c.title for c in Chapter.objects.all()], + test=lambda obj, value: obj.chap.title == value), + 'chap__book__id__exact': dict( + values=[b.id for b in Book.objects.all()], + test=lambda obj, value: obj.chap.book.id == value), + 'chap__book__name': dict( + values=[b.name for b in Book.objects.all()], + test=lambda obj, value: obj.chap.book.name == value), + 'chap__book__promo__id__exact': dict( + values=[p.id for p in Promo.objects.all()], + test=lambda obj, value: + obj.chap.book.promo_set.filter(id=value).exists()), + 'chap__book__promo__name': dict( + values=[p.name for p in Promo.objects.all()], + test=lambda obj, value: + obj.chap.book.promo_set.filter(name=value).exists()), + } + for filter_path, params in filters.items(): + for value in params['values']: + query_string = urlencode({filter_path: value}) + # ensure filter link exists + self.assertContains(response, '' % query_string) + # ensure link works + filtered_response = self.client.get( + '/test_admin/%s/admin_views/chapterxtra1/?%s' % ( + self.urlbit, query_string)) + self.failUnlessEqual(filtered_response.status_code, 200) + # ensure changelist contains only valid objects + for obj in filtered_response.context['cl'].query_set.all(): + self.assertTrue(params['test'](obj, value)) + def testIncorrectLookupParameters(self): """Ensure incorrect lookup parameters are handled gracefully.""" response = self.client.get('/test_admin/%s/admin_views/thing/' % self.urlbit, {'notarealfield': '5'}) diff --git a/tests/regressiontests/modeladmin/tests.py b/tests/regressiontests/modeladmin/tests.py index 762517a3ae..042c1b75ea 100644 --- a/tests/regressiontests/modeladmin/tests.py +++ b/tests/regressiontests/modeladmin/tests.py @@ -835,7 +835,7 @@ class ValidationTests(unittest.TestCase): self.assertRaisesRegexp( ImproperlyConfigured, - "'ValidationTestModelAdmin.list_filter\[0\]' refers to field 'non_existent_field' that is missing from model 'ValidationTestModel'.", + "'ValidationTestModelAdmin.list_filter\[0\]' refers to 'non_existent_field' which does not refer to a Field.", validate, ValidationTestModelAdmin, ValidationTestModel,