#3400 closed (fixed)
Support for lookup separator with list_filter admin option
Reported by: | Owned by: | Simon Meers | |
---|---|---|---|
Component: | contrib.admin | Version: | newforms-admin |
Severity: | Keywords: | edc nfa-someday list_filter FilterSpec nfa-changelist ep2008 | |
Cc: | Simon Litchfield, nick.lane.au@…, remco@…, andreas@…, richard@…, hanne.moa@…, jashugan@…, flosch@…, Gonzalo Saavedra, django@…, Aurelio Tinio, Odin Hørthe Omdal, andy@…, marcoberi@…, ramusus@…, alexey.rudy@…, tjurewicz@…, dan.fairs@…, semente@…, timothy.broder@…, Gabriel Hurley, Simon Meers, osirius@…, Robin Breathe, mmitar@…, cal@…, 3point2, Alexander Koshelev, Mark Jones | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
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)
Change History (85)
Changed 17 years ago by
Attachment: | list_filter.diff added |
---|
comment:1 Changed 17 years ago by
Keywords: | list_filter added |
---|---|
Triage Stage: | Unreviewed → Design decision needed |
comment:2 Changed 17 years ago by
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 17 years ago by
Patch needs improvement: | set |
---|---|
Triage Stage: | Design decision needed → 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:
- 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. - 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 17 years ago by
Cc: | nick.lane.au@… added |
---|
- Oops, sorry about the diff - yes that's the correct file.
FakeForeignKey
is a wrapper around theForeignKey
field so that theRelatedFilterSpec
will work across the foreign keys. Perhaps it could have a better name. It is only instantiated insideget_filters()
, butRelatedFilterSpec
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.
comment:6 Changed 17 years ago by
This is exactly what I need.
I'm going to apply this and try it out.
Changed 16 years ago by
Attachment: | django-list_filter.diff added |
---|
Updated diff for new management module
Changed 16 years ago by
Attachment: | ticket-3400-against-newforms-admin-6477.diff added |
---|
comment:7 Changed 16 years ago by
Keywords: | newforms-admin FilterSpec added |
---|---|
Version: | SVN → newforms-admin |
I attached an alternative patch for newforms-admin branch... It can deal with __
paths not just for RelatedField
s 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 16 years ago by
Also see #5833 for a way to enable users to register their own filter.
comment:9 Changed 16 years ago by
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 16 years ago by
Keywords: | nfa-changelist added |
---|
comment:11 Changed 15 years ago by
Cc: | remco@… added |
---|
Changed 15 years ago by
Attachment: | 3400-against-7875.patch added |
---|
comment:12 Changed 15 years ago by
Keywords: | ep2008 added |
---|
I checked and applied 3400-against-7875.patch, all tests passed.
comment:13 Changed 15 years ago by
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 15 years ago by
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 15 years ago by
Attachment: | 3400-against-8363.patch added |
---|
comment:15 Changed 15 years ago by
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 15 years ago by
Cc: | andreas@… added |
---|
comment:17 Changed 15 years ago by
Path 3400-against-8363.patch does not run against revision 9285.
Is there a way to use lookup separator without patch django?
comment:19 Changed 15 years ago by
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 15 years ago by
Cc: | hanne.moa@… added |
---|
comment:21 Changed 15 years ago by
Cc: | jashugan@… added |
---|
comment:22 Changed 15 years ago by
Cc: | flosch@… added |
---|
Changed 15 years ago by
Attachment: | 3400-against-9646.patch added |
---|
Quick update for trunk [9646]. Works wih 1.0.2 too
comment:23 Changed 15 years ago by
Any improvements needed for this patch?
Is there any chance this can make it in 1.1.?
comment:25 Changed 15 years ago by
Cc: | Gonzalo Saavedra added |
---|
Changed 15 years ago by
Attachment: | 3400-against-10680-with-tests-docs.patch added |
---|
patch against 10680 with tests and doc
comment:27 Changed 15 years ago by
I added patch against newest django trunk version.
main repo lives in http://github.com/HonzaKral/django/tree/ticket3400
comment:28 Changed 15 years ago by
Keywords: | edc added |
---|---|
Owner: | changed from nobody to jakub_vysoky |
Changed 15 years ago by
Attachment: | 3400-against-10706.diff added |
---|
new patch with init and some more data in tests
comment:29 Changed 14 years ago by
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 14 years ago by
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 14 years ago by
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 14 years ago by
Cc: | django@… added |
---|
comment:33 Changed 14 years ago by
Cc: | Aurelio Tinio added |
---|
comment:34 Changed 14 years ago by
Cc: | Odin Hørthe Omdal added |
---|
comment:35 Changed 14 years ago by
Cc: | andy@… added |
---|
comment:36 Changed 14 years ago by
Cc: | marcoberi@… added |
---|
comment:37 Changed 14 years ago by
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 14 years ago by
Cc: | ramusus@… added |
---|
comment:39 Changed 14 years ago by
Cc: | alexey.rudy@… added |
---|
comment:40 Changed 14 years ago by
Cc: | tjurewicz@… added |
---|
comment:41 Changed 14 years ago by
Cc: | dan.fairs@… added |
---|
comment:42 Changed 14 years ago by
Cc: | semente@… added |
---|
Changed 14 years ago by
Attachment: | 3400-against-10706.2.diff added |
---|
works & doesn't create intermediary FilterSpec while traversing model hierarchy
comment:43 Changed 14 years ago by
milestone: | 1.2 → 1.3 |
---|---|
Summary: | [patch] Support for lookup separator with list_filter admin option → Support for lookup separator with list_filter admin option |
Definitely isn't going to make it into 1.2.
comment:44 Changed 14 years ago by
@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 14 years ago by
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 14 years ago by
current patch does not respect limit_choices_to when specified for a ForeignKey.
comment:47 Changed 14 years ago by
Cc: | timothy.broder@… added |
---|
comment:48 Changed 14 years ago by
Cc: | Gabriel Hurley added |
---|
comment:49 Changed 14 years ago by
Cc: | Simon Meers added |
---|
comment:50 Changed 14 years ago by
Cc: | osirius@… added |
---|
comment:51 Changed 13 years ago by
Cc: | Robin Breathe added |
---|
comment:52 Changed 13 years ago by
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 13 years ago by
Cc: | mmitar@… added |
---|
comment:54 Changed 13 years ago by
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 13 years ago by
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 13 years ago by
Cc: | cal@… added |
---|
Hello,
Can DrMeers please upload his most recent patch please?
Cheers
Cal
comment:57 Changed 13 years ago by
Needs tests: | unset |
---|---|
Owner: | changed from jakub_vysoky to Simon Meers |
Patch needs improvement: | unset |
Status: | new → 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 13 years ago by
comment:58 Changed 13 years ago by
after testing this patch on my dev/test instance successfully, I'll deploy on my prod server and report
comment:59 Changed 13 years ago by
Cc: | 3point2 added |
---|
comment:60 Changed 13 years ago by
all good in prod for me so far... Will update if I notice anything out of the ordinary
comment:61 Changed 13 years ago by
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 13 years ago by
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 13 years ago by
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
- response = callback(request, *callback_args, callback_kwargs)
File "/home/textrecommend/local/lib/python2.6/site-packages/django/contrib/admin/options.py" in wrapper
- 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
- response = view_func(request, *args, kwargs)
File "/home/textrecommend/local/lib/python2.6/site-packages/django/views/decorators/cache.py" in _wrapped_view_func
- response = view_func(request, *args, kwargs)
File "/home/textrecommend/local/lib/python2.6/site-packages/django/contrib/admin/sites.py" in inner
- return view(request, *args, kwargs)
File "/home/textrecommend/local/lib/python2.6/site-packages/django/utils/decorators.py" in _wrapper
- return decorator(bound_func)(*args, kwargs)
File "/home/textrecommend/local/lib/python2.6/site-packages/django/utils/decorators.py" in _wrapped_view
- response = view_func(request, *args, kwargs)
File "/home/textrecommend/local/lib/python2.6/site-packages/django/utils/decorators.py" in bound_func
- return func(self, *args2, kwargs2)
File "/home/textrecommend/local/lib/python2.6/site-packages/django/contrib/admin/options.py" in changelist_view
- 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
- 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
- field_path=filter_name)
File "/home/textrecommend/local/lib/python2.6/site-packages/django/contrib/admin/filterspecs.py" in create
- field_path=field_path)
File "/home/textrecommend/local/lib/python2.6/site-packages/django/contrib/admin/filterspecs.py" in init
- 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
- 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 13 years ago by
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 13 years ago by
Attachment: | 3400.2.diff added |
---|
comment:66 Changed 13 years ago by
Cc: | Alexander Koshelev added |
---|
comment:67 Changed 13 years ago by
can anyone mail me about which file that needed to be change and full example for doing this code?
comment:68 Changed 13 years ago by
Cc: | Simon Litchfield added |
---|
comment:69 Changed 13 years ago by
Cc: | Mark Jones added |
---|
comment:70 Changed 13 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Patch for list_filter options