Opened 8 years ago

Closed 5 years ago

Last modified 4 years ago

#3400 closed (fixed)

Support for lookup separator with list_filter admin option

Reported by: nick@… Owned by: DrMeers
Component: contrib.admin Version: newforms-admin
Severity: Keywords: edc nfa-someday list_filter FilterSpec nfa-changelist ep2008
Cc: simon29, nick.lane.au@…, remco@…, andreas@…, richard@…, hanne.moa@…, jashugan@…, flosch@…, gonz, django@…, aurelio, velmont, andy@…, marcoberi@…, ramusus@…, alexey.rudy@…, tjurewicz@…, dan.fairs@…, semente@…, timothy.broder@…, gabrielhurley, DrMeers, osirius@…, isometry, mmitar@…, cal@…, 3point2, alexkoshelev, mark0978 Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

This patch adds support for using the lookup separator in the list_filter option, for example:

class Town(models.Model):
    name = models.CharField()

class House(models.Model):
    town = models.ForeignKey(Town)

class Room(models.Model):
    house = models.ForeignKey(House)

class Booking(models.Model):
    room = models.ForeignKey(Room, raw_id_admin=True)
    
    class Admin:
        list_filter = ('room__house__town',)

Will add a filter "For town:" that spans multiple foreign keys.

Attachments (14)

list_filter.diff (2.1 KB) - added by nick@… 8 years ago.
Patch for list_filter options
list_filter.2.diff (2.0 KB) - added by nick@… 8 years ago.
Improved patch
list_filter.3.diff (4.4 KB) - added by nick.lane.au@… 8 years ago.
Improved patch per suggestions
django-list_filter.diff (4.3 KB) - added by lucas@… 8 years ago.
Updated diff for new management module
ticket-3400-against-newforms-admin-6477.diff (9.3 KB) - added by Honza_Kral 8 years ago.
3400-against-7875.patch (9.2 KB) - added by Honza_Kral 7 years ago.
3400-against-8363.patch (9.8 KB) - added by vitek_pliska 7 years ago.
3400-against-9646.patch (9.8 KB) - added by vitek_pliska 6 years ago.
Quick update for trunk [9646]. Works wih 1.0.2 too
3400-against-10680-with-tests-docs.patch (14.0 KB) - added by jakub_vysoky 6 years ago.
patch against 10680 with tests and doc
3400-against-10706.diff (14.0 KB) - added by Honza_Kral 6 years ago.
new patch with init and some more data in tests
3400-against-10706.2.diff (10.5 KB) - added by pavelszalbot 5 years ago.
works & doesn't create intermediary FilterSpec while traversing model hierarchy
3400.diff (26.3 KB) - added by DrMeers 5 years ago.
3400.2.diff (26.3 KB) - added by DrMeers 5 years ago.
3400.3.diff (27.2 KB) - added by vitek_pliska 5 years ago.
Patch update (against 14662)

Download all attachments as: .zip

Change History (85)

Changed 8 years ago by nick@…

Patch for list_filter options

Changed 8 years ago by nick@…

Improved patch

comment:1 Changed 8 years ago by Simon G. <dev@…>

  • Keywords list_filter added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

comment:2 Changed 8 years ago by nick@…

Hmm not sure how I didn't notice before, but this breaks the model validation in django.core.management:

for fn in opts.admin.list_display:
    try:
        f = opts.get_field(fn)
    except models.FieldDoesNotExist:
        if not hasattr(cls, fn):
            e.add(opts, '"admin.list_display" refers to %r, which isn\'t an attribute, method or property.' % fn)
    else:
        if isinstance(f, models.ManyToManyField):
            e.add(opts, '"admin.list_display" doesn\'t support ManyToManyFields (%r).' % fn)

Of course, it shouldn't be hard to fix - but is it worth it? This type of filtering is very useful for my particular apps but if it's not considered common enough for the core there's no point fixing the validation.

comment:3 Changed 8 years ago by mtredinnick

  • Patch needs improvement set
  • Triage Stage changed from Design decision needed to Accepted

I think it's worth getting this right. You probably aren't the only person wanting this functionality.

A couple of comments on the patch:

  1. please create the diff from the top of the Django source tree. I had to guess a lot to work out that it was to be applied to django/contrib/admin/views/main.py (which I think is right). Having the full path from the tree root to the patched file is helpful.
  2. If the FakeForeignKey class has any visibility outside of that function (which I'm not sure about), it shouldn't be declared inside the function. In fact, I'm not really thrilled with declaring it inside the function in the first place, so probably best to move it out to the top-level of the file. I realise you are trying to avoid namespace pollution, but it's not worth the sacrifice of clarity to me.

comment:4 Changed 8 years ago by nick.lane.au@…

  • Cc nick.lane.au@… added
  1. Oops, sorry about the diff - yes that's the correct file.
  2. FakeForeignKey is a wrapper around the ForeignKey field so that the RelatedFilterSpec will work across the foreign keys. Perhaps it could have a better name. It is only instantiated inside get_filters(), but RelatedFilterSpec will have access to it.. so I'll move it to the top-level of the file like you have suggested.

Will update the patch shortly when I have some time, Cheers.

Changed 8 years ago by nick.lane.au@…

Improved patch per suggestions

comment:5 Changed 8 years ago by Simon Litchfield <simon@…>

Works great thanks Nick. This was bugging me just the other day.

comment:6 Changed 8 years ago by cbrand@…

This is exactly what I need.
I'm going to apply this and try it out.

Changed 8 years ago by lucas@…

Updated diff for new management module

Changed 8 years ago by Honza_Kral

comment:7 Changed 8 years ago by Honza_Kral

  • Keywords newforms-admin FilterSpec added
  • Version changed from SVN to newforms-admin

I attached an alternative patch for newforms-admin branch... It can deal with __ paths not just for RelatedFields but for any fields. It will traverse the path, return the last field in the chain and supply additional parameter field_path that is used for filtering the results.

With this you can filter for example by a DateField stored on a related model.

comment:8 Changed 7 years ago by Honza_Kral

Also see #5833 for a way to enable users to register their own filter.

comment:9 Changed 7 years ago by brosner

  • Keywords nfa-someday added; newforms-admin removed

This is a feature enhancement and not critical to the merge of newforms-admin into trunk. Tagging with nfa-someday. I would like to see this functionality get in. After newforms-admin gets merged I will look into this more and examine the patches present.

comment:10 Changed 7 years ago by ales_zoulek

  • Keywords nfa-changelist added

comment:11 Changed 7 years ago by shanx

  • Cc remco@… added

Changed 7 years ago by Honza_Kral

comment:12 Changed 7 years ago by vitek_pliska

  • Keywords ep2008 added

I checked and applied 3400-against-7875.patch, all tests passed.

comment:13 Changed 7 years ago by anonymous

Just wrote small app for testing this. I don't want to write tests.
models.py:

from django.conf import settings
from django.db import models
from django.contrib import admin

class Town(models.Model):
    name = models.CharField(max_length=64)
    
    def __unicode__(self):
        return self.name

class House(models.Model):
    town = models.ForeignKey(Town)

    def __unicode__(self):
        return self.town.name

class Room(models.Model):
    house = models.ForeignKey(House)

    def __unicode__(self):
        return self.house.town.name

class Booking(models.Model):
    room = models.ForeignKey(Room)

    def __unicode__(self):
        return self.room.house.town.name
    
class BookingOpts(admin.ModelAdmin):
    list_filter = ('room__house__town',)
    raw_id_admin = ('room', )

admin.site.register(Town)
admin.site.register(House)
admin.site.register(Room)
admin.site.register(Booking, BookingOpts)

Works fine.

comment:14 Changed 7 years ago by anonymous

I've applied 3400-against-7875.patch to today svn django-trunk and I get the following error:

Error while importing URLconf 'myapp.urls': ProjectAdmin.list_filter[8] refers to field technology__programing_languages that is missing from model Project.

The models are:

class Project(models.Model):

technology = models.ForeignKey("ProjectTechnology", verbose_name=_('project technology'),unique=True, null=True, blank=True, editable=False, related_name="technology")
...

class ProjectTechnology(models.Model):

programing_languages = models.ManyToManyField(ProgramingLanguage, null=True, blank=True)
...

class ProgramingLanguage(models.Model):

name = models.CharField(_('name'), max_length=200)
...

I'm almost a newbie, what it could be?

Thanks.

Changed 7 years ago by vitek_pliska

comment:15 Changed 7 years ago by anonymous

Thanks, 3400-against-8363.patch solves the problem.

Should it be possible now to show attributes (or foreignkeys) of other model B that have a foreignkey to model A in model A list_filter?, how?

If I should make my question to other sites, don't complaint to tell me.

Thanks in advance.

comment:16 Changed 7 years ago by peritus

  • Cc andreas@… added

comment:17 Changed 7 years ago by anonymous

Path 3400-against-8363.patch does not run against revision 9285.

Is there a way to use lookup separator without patch django?

comment:18 Changed 7 years ago by cornbread

Does this patch include a way to add sort_by as well?

comment:19 Changed 7 years ago by cornbread

  • Cc richard@… added

This needs a sort_by option as well:

class Town(models.Model):
    name = models.CharField()

class House(models.Model):
    town = models.ForeignKey(Town)

class Room(models.Model):
    house = models.ForeignKey(House)

class Booking(models.Model):
    room = models.ForeignKey(Room, raw_id_admin=True)
    
    class Admin:
        sort_by = ('room__house__town',)

comment:20 Changed 7 years ago by HM

  • Cc hanne.moa@… added

comment:21 Changed 7 years ago by jashugan

  • Cc jashugan@… added

comment:22 Changed 6 years ago by flosch

  • Cc flosch@… added

Changed 6 years ago by vitek_pliska

Quick update for trunk [9646]. Works wih 1.0.2 too

comment:23 Changed 6 years ago by Almad

Any improvements needed for this patch?

Is there any chance this can make it in 1.1.?

comment:24 Changed 6 years ago by mrts

  • milestone set to 1.2

Tentatively targeting for 1.2.

comment:25 Changed 6 years ago by gonz

  • Cc gonz added

comment:26 Changed 6 years ago by mrts

See also #10743.

Changed 6 years ago by jakub_vysoky

patch against 10680 with tests and doc

comment:27 Changed 6 years ago by jakub_vysoky

I added patch against newest django trunk version.

main repo lives in http://github.com/HonzaKral/django/tree/ticket3400

comment:28 Changed 6 years ago by jakub_vysoky

  • Keywords edc added
  • Owner changed from nobody to jakub_vysoky

Changed 6 years ago by Honza_Kral

new patch with init and some more data in tests

comment:29 Changed 6 years ago by francois@…

  • Needs tests set

I feel stupid posting this, but are we sure it works with related_of_related as in the example above?

Using :

from django.db import models
class Town(models.Model):
    name = models.CharField(max_length=50)

class House(models.Model):
    town = models.ForeignKey(Town)

class Room(models.Model):
    house = models.ForeignKey(House)

class Booking(models.Model):
    room = models.ForeignKey(Room)

and

from django.contrib import  admin
from content.models import *

class BookingAdmin(admin.ModelAdmin):
    list_filter = ('room__house__town',)
    pass

admin.site.register(Booking,BookingAdmin)
admin.site.register(Town)
admin.site.register(Room)
admin.site.register(House)

I get a exception in /django/contrib/admin/views/main.py circa line 85


                if '__' in filter_name:
                    f = None
                    model = self.model
                    path = filter_name.split('__')
                    for field_name in path[:-1]:
                        f = model._meta.get_field(field_name)
                        model = f.rel.to
                        f = model._meta.get_field(path[-1]) # !!!! Exception
                        spec = FilterSpec.create(f, request, self.params, model, self.model_admin, field_path=filter_name)
                else:
                    f = lookup_opts.get_field(filter_name)
                    spec = FilterSpec.create(f, request, self.params, self.model, self.model_admin)

as it tries to get a field for path[2] (town) on model path[0] (room) on the first iteration.

I get that problem using the patch 3400-against-9646.patch, which patched all right against 1.0.2-final. That bit code looks identical on subsequent patches, though.

comment:30 Changed 6 years ago by francois@…

solved it by

                for field_name in path[:-1]:
                        f = model._meta.get_field(field_name)
                        model = f.rel.to
                        f = model._meta.get_field(path[path.index(field_name)+1])

doesn't look very robust but works so far.. Needs more testing, I guess

comment:31 Changed 6 years ago by pavelszalbot

Working and little bit better solution - not creating intermediary FilterSpec(s):

                    for field_index in xrange(len(path)-1):
                        f = model._meta.get_field(path[field_index])
                        model = f.rel.to
                        f = model._meta.get_field(path[field_index+1])
                    spec = FilterSpec.create(f, request, self.params, model, self.model_admin, field_path=filter_name)

comment:32 Changed 6 years ago by jedie

  • Cc django@… added

comment:33 Changed 6 years ago by aurelio

  • Cc aurelio added

comment:34 Changed 6 years ago by velmont

  • Cc velmont added

comment:35 Changed 6 years ago by andybak

  • Cc andy@… added

comment:36 Changed 6 years ago by marcob

  • Cc marcoberi@… added

comment:37 Changed 6 years ago by bendavis78

Patch currently does not work with reverse relationships. For example:

class Target(models.Model):
    name = CharField(max_length=200)

class Survey(models.Model):
    target = models.OneToOneField(Target)
    source = models.ForeignKey(SurveySource)

class TargetAdmin(admin.ModelAdmin):
    list_filter = ['survey__source']
    

My hunch is that this only breaks in the validation code -- I'm investigating right now if that will fix it. The only question is how to correctly traverse reverse relationships using ._meta

comment:38 Changed 6 years ago by anonymous

  • Cc ramusus@… added

comment:39 Changed 5 years ago by anonymous

  • Cc alexey.rudy@… added

comment:40 Changed 5 years ago by trent

  • Cc tjurewicz@… added

comment:41 Changed 5 years ago by danfairs

  • Cc dan.fairs@… added

comment:42 Changed 5 years ago by anonymous

  • Cc semente@… added

Changed 5 years ago by pavelszalbot

works & doesn't create intermediary FilterSpec while traversing model hierarchy

comment:43 Changed 5 years ago by SmileyChris

  • milestone changed from 1.2 to 1.3
  • Summary changed from [patch] Support for lookup separator with list_filter admin option to Support for lookup separator with list_filter admin option

Definitely isn't going to make it into 1.2.

comment:44 Changed 5 years ago by marcob

@SmileyChris: may We do something to make it into 1.2 ? I'm not lobbying :-) With something I meaned some real work

comment:45 Changed 5 years ago by SmileyChris

No chance. Since 1.2 beta has been released, we've hit a full feature freeze (and this is definitely a feature).

comment:46 Changed 5 years ago by brentp

current patch does not respect limit_choices_to when specified for a ForeignKey.

comment:47 Changed 5 years ago by broderboy

  • Cc timothy.broder@… added

comment:48 Changed 5 years ago by anonymous

  • Cc gabrielhurley added

comment:49 Changed 5 years ago by DrMeers

  • Cc DrMeers added

comment:50 Changed 5 years ago by SamBull

  • Cc osirius@… added

comment:51 Changed 5 years ago by isometry

  • Cc isometry added

comment:52 Changed 5 years ago by frans

good point by brentp. I've tried to have a crack at it but got lost in the the bowels of limit_choices_to. If anyone who knows the innards can sketch an approach, I don't mind tackling the actual patching/testing.

comment:53 Changed 5 years ago by mitar

  • Cc mmitar@… added

comment:54 Changed 5 years ago by mitar

Reverse relationship support would be really great as then it would be possible to extend list_filter for UserAdmin with fields from profile. Profile have OneToOneField or ForeignField field defined which point back to django.contrib.auth.models.User model and currently it is not possible to extend admin to filter by fields in the profile. (While it is possible to extend searchable fields.)

comment:55 Changed 5 years ago by DrMeers

I have written a patch which works for reverse relationships also; anything you can put in search__fields now works for list__filter also. Just tidying up implementation, limit_choices_to, etc, will upload shortly.

comment:56 Changed 5 years ago by foxwhisper

  • Cc cal@… added

Hello,

Can DrMeers please upload his most recent patch please?

Cheers

Cal

comment:57 Changed 5 years ago by DrMeers

  • Needs tests unset
  • Owner changed from jakub_vysoky to DrMeers
  • Patch needs improvement unset
  • Status changed from new to assigned

OK, here is my draft patch. I have not tested it thoroughly; there may still be a few issues to smooth out, so test-drivers would be appreciated.

A few notes:

  • Reverse relationships work
  • You can now use any field path that search_filters will accept
  • I have removed the strict validation in favour of letting setup_joins catch any issues, as search_fields does. We could perhaps look at adding admin validation for field paths for both instead.
  • I played around with combining multiple limit_choices_to clauses based on the models in the field path, however discovered that in practise it really doesn't make sense to apply any before the final model in the field path
  • limit_choices_to is respected for AllValuesFilterSpec, and a utility function is provided if other filterspecs wish to do likewise
  • There are some basic tests included in the patch

Changed 5 years ago by DrMeers

comment:58 Changed 5 years ago by frans

after testing this patch on my dev/test instance successfully, I'll deploy on my prod server and report

comment:59 Changed 5 years ago by 3point2

  • Cc 3point2 added

comment:60 Changed 5 years ago by frans

all good in prod for me so far... Will update if I notice anything out of the ordinary

comment:61 Changed 5 years ago by foxwhisper

  • Needs tests set

Deployed to dev and all looks fine so far.. will let you guys know if anything bad happens. Thanks dr meers!

comment:62 Changed 5 years ago by DrMeers

  • Needs tests unset

Did you not notice the included tests? If there is a particular aspect you thought was untested, please let me know and I'll add it.

comment:63 Changed 5 years ago by foxwhisper

Hi DrMeers,

I actually had problems merging the tests in, the patch file said the path specified for the tests was not valid, but it worked on the other files, so I marked it as "needs tests" but I prob should have marked it as patch needs improvement.

Also, I'm getting problems now in production, which I will attempt to fix later:

Environment:

Request Method: GET
Request URL: http://dev.app.textrecommend.co.uk/admin/mvoucher/vouchersms/
Django Version: 1.2 beta 1
Python Version: 2.6.5
Installed Applications:
['webapp.mvoucher',

'webapp.djangoextend',
'django.contrib.auth',
'django.contrib.contenttypes',
'django.contrib.sessions',
'django.contrib.sites',
'django.contrib.admin']

Installed Middleware:
('django.middleware.common.CommonMiddleware',

'django.contrib.sessions.middleware.SessionMiddleware',
'django.contrib.auth.middleware.AuthenticationMiddleware',
'webapp.djangoextend.djangologging.middleware.SuppressLoggingOnAjaxRequestsMiddleware',
'webapp.djangoextend.djangologging.middleware.LoggingMiddleware',
'webapp.djangoextend.core.GlobalRequestMiddleware',
'webapp.djangoextend.middleware.DjangoExtendMiddleware')

Traceback:
File "/home/textrecommend/local/lib/python2.6/site-packages/django/core/handlers/base.py" in get_response

  1. response = callback(request, *callback_args, callback_kwargs)

File "/home/textrecommend/local/lib/python2.6/site-packages/django/contrib/admin/options.py" in wrapper

  1. return self.admin_site.admin_view(view)(*args, kwargs)

File "/home/textrecommend/local/lib/python2.6/site-packages/django/utils/decorators.py" in _wrapped_view

  1. response = view_func(request, *args, kwargs)

File "/home/textrecommend/local/lib/python2.6/site-packages/django/views/decorators/cache.py" in _wrapped_view_func

  1. response = view_func(request, *args, kwargs)

File "/home/textrecommend/local/lib/python2.6/site-packages/django/contrib/admin/sites.py" in inner

  1. return view(request, *args, kwargs)

File "/home/textrecommend/local/lib/python2.6/site-packages/django/utils/decorators.py" in _wrapper

  1. return decorator(bound_func)(*args, kwargs)

File "/home/textrecommend/local/lib/python2.6/site-packages/django/utils/decorators.py" in _wrapped_view

  1. response = view_func(request, *args, kwargs)

File "/home/textrecommend/local/lib/python2.6/site-packages/django/utils/decorators.py" in bound_func

  1. return func(self, *args2, kwargs2)

File "/home/textrecommend/local/lib/python2.6/site-packages/django/contrib/admin/options.py" in changelist_view

  1. self.date_hierarchy, self.search_fields, self.list_select_related, self.list_per_page, self.list_editable, self)

File "/home/textrecommend/local/lib/python2.6/site-packages/django/contrib/admin/views/main.py" in init

  1. self.filter_specs, self.has_filters = self.get_filters(request)

File "/home/textrecommend/local/lib/python2.6/site-packages/django/contrib/admin/views/main.py" in get_filters

  1. field_path=filter_name)

File "/home/textrecommend/local/lib/python2.6/site-packages/django/contrib/admin/filterspecs.py" in create

  1. field_path=field_path)

File "/home/textrecommend/local/lib/python2.6/site-packages/django/contrib/admin/filterspecs.py" in init

  1. limit_choices_to = get_limit_choices_to_from_path(model, field_path)

File "/home/textrecommend/local/lib/python2.6/site-packages/django/contrib/admin/util.py" in get_limit_choices_to_from_path

  1. hasattr(fields[-1], 'rel') and

Exception Type: IndexError at /admin/mvoucher/vouchersms/
Exception Value: list index out of range

/home/textrecommend/local/lib/python2.6/site-packages/django/contrib/admin/util.py in get_limit_choices_to_from_path

hasattr(fields[-1], 'rel') and ...

▼ Local vars
Variable Value
fields
[]
model
<class 'webapp.mvoucher.models.VoucherSMS'>
path
'carrier_status_code'

# If the carrier returns a status code, this is placed into here.
carrier_status_code = models.CharField(max_length=64, blank=True, null=True)

comment:64 Changed 5 years ago by foxwhisper

  • Patch needs improvement set

I fixed this by changing:

django/contrib/admin/util.py

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)
+    if len(fields)==0:
+        return models.Q()
    limit_choices_to = (
        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

Changed 5 years ago by DrMeers

comment:65 Changed 5 years ago by DrMeers

  • Patch needs improvement unset

Thanks foxwhipser, patch updated.

comment:66 Changed 5 years ago by alexkoshelev

  • Cc alexkoshelev added

comment:67 Changed 5 years ago by m4rt0g1

can anyone mail me about which file that needed to be change and full example for doing this code?

comment:68 Changed 5 years ago by simon29

  • Cc simon29 added

comment:69 Changed 5 years ago by mark0978

  • Cc mark0978 added

Changed 5 years ago by vitek_pliska

Patch update (against 14662)

comment:70 Changed 5 years ago by Honza_Kral

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [14674]) Fixed #3400 -- Support for lookup separator with list_filter admin option. Thanks to DrMeers and vitek_pliska for the patch!

comment:71 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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