Opened 16 years ago

Closed 12 years ago

#5833 closed Bug (fixed)

Custom FilterSpecs

Reported by: Honza Král Owned by: jkocherhans
Component: contrib.admin Version: dev
Severity: Normal Keywords: nfa-someday list_filter filterspec nfa-changelist ep2008
Cc: Simon Litchfield, eallik@…, semente@…, andreas@…, mbencun@…, chenyuejie@…, djangoproject@…, jan.rzepecki@…, kmike84@…, eandre@…, Gonzalo Saavedra, ciantic@…, rvdrijst, eppsilon, ramusus@…, peimei@…, marcoberi@…, david@…, rmanocha@…, sciyoshi@…, Odin Hørthe Omdal, Sean Legassick, Alexander Koshelev, Marinho Brandão, Dan Fairs, timothy.broder@…, Michael Manfre, internet@…, Robin Breathe, bendavis78@…, 3point2, SafPlusPlus, krasniyrus@…, subsume@…, dirleyrls@…, Vitek Pliska, Salva Pinyol, Simon Charette, remco@…, rohit.aggarwal@…, guandalino Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by korpios)

One should be able to create custom FilterSpecs for the admin's list_filter interface.

Honza_Kral: I modified the filterspec definition to allow for users to register their own filters in admin. The mechanism is simple - I just reverted the order of the registry so that newly registered specs will come first. That way if you register your own filter via FilterSpec.register, it will be used before the default one.

Another approach by korpios is described in the comments.

Attachments (22)

ticket-5833-against-newforms-admin-6477.diff (3.6 KB ) - added by Honza Král 16 years ago.
custom_filterspecs_plus_fieldless.patch (10.1 KB ) - added by korpios 16 years ago.
Custom FilterSpecs, also allowing fieldless FilterSpecs
5833-against-7875.patch (3.7 KB ) - added by Honza Král 16 years ago.
updated version of my simple patch,
5833-against-7875.2.patch (3.6 KB ) - added by Honza Král 16 years ago.
5833-against-7875.3.patch (3.7 KB ) - added by Honza Král 16 years ago.
filterspec_with_custom_queryset_against_1_0.diff (11.3 KB ) - added by fas 16 years ago.
Custom Filtersets (fieldless) with custom query set manipulation.
filterspec_with_custom_queryset_against_1_0_2.diff (11.4 KB ) - added by gerdemb 15 years ago.
5833-against-9820.patch (11.4 KB ) - added by gerdemb 15 years ago.
Patch updated to latest HEAD passing all tests
5833-against-9836-new-proper.patch (14.3 KB ) - added by eandre@… 15 years ago.
New patch against r9836
5833.patch (14.6 KB ) - added by Samuel Cormier-Iijima 15 years ago.
New patch against 1.1/SVN
5833-custom-filter-spec-1.2.1.diff (14.7 KB ) - added by Ben Davis 14 years ago.
Fixed non-field filterspec support, updated for django 1.2.1 (fixed)
5833-metapatch.patch (2.6 KB ) - added by Yeago 14 years ago.
An operator agnostic version of FilterSpec
5833-custom-filter-spec-1.3.diff (15.6 KB ) - added by Ben Davis 13 years ago.
Patch for 1.3, updated w/ apply_filter_specs() method
5833.custom-filterspecs.diff (15.5 KB ) - added by Julien Phalip 13 years ago.
Works with Django 1.3 (still no tests or doc)
5833.custom-filterspecs.2.diff (34.6 KB ) - added by Julien Phalip 13 years ago.
Introducing SimpleFilterSpecs?, with tests.
5833.custom-filterspecs.3.diff (49.1 KB ) - added by Julien Phalip 13 years ago.
Patch + tests + doc
5833.custom-filterspecs.4.diff (50.0 KB ) - added by Julien Phalip 13 years ago.
Cleaner API based on Jacob's feedback so far
5833.custom-filterspecs.5.diff (53.3 KB ) - added by Julien Phalip 13 years ago.
Now get_query_set() receives the request as parameter
5833.custom-filterspecs.6.diff (67.1 KB ) - added by Julien Phalip 13 years ago.
5833.custom-filterspecs.7.diff (69.9 KB ) - added by Julien Phalip 13 years ago.
5833.custom-filterspecs.8.diff (75.9 KB ) - added by Jannis Leidel 13 years ago.
Extended patch with further API refinements based on thorough patch review
5833.custom-filterspecs.9.diff (81.4 KB ) - added by Julien Phalip 13 years ago.

Download all attachments as: .zip

Change History (155)

comment:1 by jkocherhans, 16 years ago

Owner: changed from nobody to jkocherhans
Status: newassigned

I really like the idea behind this, but I'd need to review the code closer and won't take the time to do it right now since it's a new feature. I'd rather focus on bugs preventing newforms-admin from being used.

comment:2 by Honza Král, 16 years ago

Also see #3400 for another ideas about filters which could benefit from this scheme...

comment:3 by Brian Rosner, 16 years ago

Keywords: nfa-someday added; newforms admin removed

This ticket isn't critical to merge newforms-admin into trunk. Tagging with nfa-someday.

comment:4 by Yuri Baburov, 16 years ago

How about FilterSpec.insert method to do prepend and FilterSpec.register to do append? discussed somewhere, no?

comment:5 by korpios, 16 years ago

I'm attaching my crack at this issue; in particular, I wanted to allow custom FilterSpecs that aren't associated with a field. This way, you can throw together a filter for fairly arbitrary queries.

Much of the field-specific code from FilterSpec has been moved out to a subclass, FieldFilterSpec; that should be used as the superclass for custom field-based filterspecs, while FilterSpec can be used for non-field-based ones.

The list_filter syntax gains two further options besides field names: a FilterSpec subclass, or a tuple of ('field_name', FieldFilterSpecSubclass).

An example combining all three:

list_filter = (
    'field1',
    ('field2', SomeFieldFilterSpec),
    SomeFilterSpec,
)

by korpios, 16 years ago

Custom FilterSpecs, also allowing fieldless FilterSpecs

comment:6 by korpios, 16 years ago

Cc: korpios@… added

comment:7 by korpios, 16 years ago

Description: modified (diff)
Summary: newforms-admin enable registering custom FilterSpecs[newforms-admin] Custom FilterSpecs

Tweaked the summary and description.

comment:8 by anonymous, 16 years ago

Triage Stage: UnreviewedDesign decision needed

comment:9 by Jacob, 16 years ago

that was me; sorry.

comment:10 by Ales Zoulek, 16 years ago

Keywords: nfa-changelist added

comment:11 by Erik Allik, 16 years ago

Cc: eallik@… added

by Honza Král, 16 years ago

Attachment: 5833-against-7875.patch added

updated version of my simple patch,

comment:12 by Honza Král, 16 years ago

Keywords: ep2008 added

by Honza Král, 16 years ago

Attachment: 5833-against-7875.2.patch added

by Honza Král, 16 years ago

Attachment: 5833-against-7875.3.patch added

comment:13 by Alex Gaynor, 16 years ago

Version: newforms-adminSVN

comment:14 by elwaywitvac <elwaywitvac@…>, 16 years ago

Patch needs improvement: set
Honza_Kral: I modified the filterspec definition to allow for users to register their own filters in admin. The mechanism is simple - I just reverted the order of the registry so that newly registered specs will come first. That way if you register your own filter via FilterSpec.register, it will be used before the default one.

I'm not sure having a reversed registry is intuitive. It certainly makes overriding FilterSpecs simple. Except if I were to register two FilterSpecs I would expect them to be tested the order I add them, the same way it is done in the rest of Django. Maybe the FilterSpec's should be in a Priority Queue instead of a Queue?

I had created a solution which doesn't reverse the registry ticket:8330. But I'm realizing now that this wouldn't allow for overriding other filters like Boolean. I'll keep playing.

comment:15 by elwaywitvac <elwaywitvac@…>, 16 years ago

Suggestion: make FieldFilterSpec's similar to how widgets work in newforms admin.

comment:16 by Guilherme M. Gondim <semente@…>, 16 years ago

Cc: semente@… added

comment:17 by korpios, 16 years ago

Cc: korpios@… removed

comment:18 by anonymous, 16 years ago

Cc: andreas@… added

comment:19 by fas, 16 years ago

Cc: mbencun@… added

in reply to:  14 comment:20 by fas, 16 years ago

Based on korpios' idea I made a patch that takes the idea further. With korpios' current solution you can add custom filters but not play with the queryset. Filters only based on field lookups is too little for complex administrations.

I provide a patch which lets you add, like korpios' suggested, custom filters like this:

list_filter = (
    'field1',
    ('field2', SomeFieldFilterSpec),
    SomeFilterSpec,
)

Furthermore, every FilterSpec can implement get_query_set:

def SomeFilterSpec:
    def get_query_set(self, cl, qs):
        if self.params.has_key("custom_get_parameter"):
            return qs.filter(somefield__startswith = self.params["custom_get_parameter"])

Note that this is a trivial example that can also be achieved with field lookup parameters. But there are no limits of how to filter the result set.

by fas, 16 years ago

Custom Filtersets (fieldless) with custom query set manipulation.

comment:21 by yuejie, 15 years ago

Cc: chenyuejie@… added

comment:22 by anonymous, 15 years ago

Cc: bpederse@… added

comment:23 by anonymous, 15 years ago

line 103 of filterspec_with_custom_queryset_against_1_0.diff
should read:

+        super(RelatedFilterSpec, self).__init__(request, params, model, model_admin, f)

comment:24 by gerdemb, 15 years ago

Cc: djangoproject@… added

I believe there is a small bug with this patch. Line 31 of filterspec_with_custom_queryset_against_1_0.diff should be indented once more so that get_field() is only called when a field is defined in the list_filter. In the case that the list_filter item is a single callable item, the field variable is not set and get_field() should not be called for verification.

Also, any word on if this feature will make it into 1.1? I notice that there is no implemented listed on the status page. I'm using this feature frequently in my application, and would prefer to run a released version of Django instead of patching. :)

comment:25 by Niels Sandholt Busch, 15 years ago

Cc: niels.busch@… added

comment:26 by xtrqt, 15 years ago

Cc: jan.rzepecki@… added

comment:27 by gerdemb, 15 years ago

New path is diff versus v1.0.2 of django and includes fixes mentioned in the following comments:

10/08/08 20:08:44 changed by anonymous ¶
12/12/08 09:58:24 changed by gerdemb ¶

comment:28 by Mikhail Korobov, 15 years ago

Cc: kmike84@… added

by gerdemb, 15 years ago

Attachment: 5833-against-9820.patch added

Patch updated to latest HEAD passing all tests

comment:29 by gerdemb, 15 years ago

I updated the patch to the latest HEAD and fixed the TODO comments. In order to pass the existing tests, I had to disable the use of custom GET params. In the original patch, the filterspec could consume a custom param and then return an arbitrary queryset based on this parameter, but to do this it silently removed the parameters that don't match a field name from the request. The problem is that there is a test to insure that any parameters that do not match field names are handled by forwarding to ?e=1 (testIncorrectLookupParameters) which failed when the previous patch silently removed them. Additionally, the code in the previous patch didn't handle filters on M2M relationships through an intermediate table because it assumed the first part of the search param would be a field name.

comment:31 by eandre@…, 15 years ago

Cc: eandre@… added
Needs tests: set
Patch needs improvement: unset
Triage Stage: Design decision neededAccepted

I am attaching a new patch against the latest HEAD. It's based on gerdemb's patch but now allows for consuming of GET parameters to allow for custom, non-field based querystring keys. The patch also implements a priority queue in a sense; FieldFilterSpec.register() allows you to pass in high_priority=True in order to register the FieldFilterSpec as high priority. This argument is True by default but passed in as False for all default filter specs. High priority specs are inserted before normal priority specs, but order is preserved amongst the two separate groups. Let me know what you think of that solution.

I'm also moving this to Accepted since it's on the 1.1 features list (forgive me if this is an incorrect decision). Also flagging it as Needs tests.

by eandre@…, 15 years ago

New patch against r9836

comment:32 by eandre@…, 15 years ago

Please see 5833-against-9836-new-proper.patch and not 5833-against-9836-new.patch; I accidently diffed some additional, unrelated changes, and couldn't get trac to replace the old attachment.

comment:33 by Karen Tracey, 15 years ago

I deleted the 5833-against-9836-new.patch to avoid confusion.

comment:34 by Gonzalo Saavedra, 15 years ago

Cc: Gonzalo Saavedra added

comment:35 by Niels Sandholt Busch, 15 years ago

Cc: niels.busch@… removed

comment:36 by Jacob, 15 years ago

milestone: 1.1 beta

comment:37 by anonymous, 15 years ago

Cc: ciantic@… added

comment:38 by rvdrijst, 15 years ago

Cc: rvdrijst added

comment:39 by Karen Tracey, 15 years ago

Needs documentation: set
Summary: [newforms-admin] Custom FilterSpecsCustom FilterSpecs

I don't see any documentation for this enhancement in any of the patches? Makes it hard to gets started on reviewing the code. In the absence of doc, tests would also at least show how the new function is intended to be used, but those too are still missing. If anyone who has worked on the patches here or has an interest in seeing this done could provide some of these missing pieces, that would help move this one along.

comment:40 by Jacob, 15 years ago

milestone: 1.1 beta

Without docs and tests, and with 1.1 feature freeze coming up soon, this isn't going to make 1.1.

in reply to:  40 comment:41 by Honza Král, 15 years ago

Replying to jacob:

Without docs and tests, and with 1.1 feature freeze coming up soon, this isn't going to make 1.1.

I would like to get this committed, we can work on it during the PyCON sprint. It is really important for us that this ticket or #3400 gets into 1.1 and we are ready to put some work into it.

comment:42 by Brian Rosner, 15 years ago

The feature freeze is tomorrow. Only bug fixes will be happening during the sprints at PyCon. I'm afraid this is just going to have to wait until 1.2.

comment:43 by mrts, 15 years ago

milestone: 1.2

Re-targeting for 1.2.

comment:44 by eppsilon, 15 years ago

Cc: eppsilon added

comment:45 by anonymous, 15 years ago

Cc: ramusus@… added

comment:46 by anonymous, 15 years ago

Cc: peimei@… added

comment:47 by marcob, 15 years ago

Cc: marcoberi@… added

comment:48 by anonymous, 15 years ago

Cc: david@… added

comment:49 by anonymous, 15 years ago

Cc: sciyoshi@… added

comment:50 by Samuel Cormier-Iijima, 15 years ago

The validation in the latest patch is broken. Attaching a new version against 1.1/SVN which also fixes #11771.

by Samuel Cormier-Iijima, 15 years ago

Attachment: 5833.patch added

New patch against 1.1/SVN

comment:51 by marcob, 15 years ago

When you delete a record, querystring looses return_to_ parameter.

(BTW last patch seems not working for me)

comment:52 by anonymous, 15 years ago

Another place where it would be likeable to keep the filters are with admin actions

comment:53 by Odin Hørthe Omdal, 14 years ago

Cc: Odin Hørthe Omdal added

comment:54 by anonymous, 14 years ago

Cc: Sean Legassick added

comment:55 by anonymous, 14 years ago

comment:56 by Alexander Koshelev, 14 years ago

Cc: Alexander Koshelev added

comment:57 by Marinho Brandão, 14 years ago

Cc: Marinho Brandão added

comment:58 by Dan Fairs, 14 years ago

Cc: Dan Fairs added

comment:59 by vrehak, 14 years ago

Just a small note: the patch 5883.patch should be modified in django/contrib/admin/views/main.py, line 200.

Currently it reads:

if new_qs: 

which fails if new_qs is empty QuerySet (i.e. the filters output is empty). It works for me if I change it to

if new_qs is not False: 

comment:60 by Russell Keith-Magee, 14 years ago

#12173 raised the issue of ORs in filterspecs.

comment:61 by anonymous, 14 years ago

Cc: rmanocha@… added

comment:62 by James Bennett, 14 years ago

milestone: 1.2

1.2 is feature-frozen, moving this feature request off the milestone.

comment:63 by broderboy, 14 years ago

Cc: timothy.broder@… added

comment:64 by Michael Manfre, 14 years ago

Cc: Michael Manfre added

comment:65 by Dominic Rodger, 14 years ago

Cc: internet@… added

comment:66 by Robin Breathe, 14 years ago

Cc: Robin Breathe added

comment:67 by Ben Davis, 14 years ago

Cc: bendavis78@… added

comment:68 by Ben Davis, 14 years ago

It seems that the latest patch did not support using a "field-less" filterspec. The validation was failing because it was still expecting a field. I've attached a patch against 1.2.1 that works for me. If anyone involved could please test this out, that would be great.

We need some docs and tests for this -- it'd be nice to see this added to 1.3. Although, it would be good to get some feedback from the core devs to make sure this is going in the right direction.

In the meantime, here's a silly little example of how a custom non-field-specific filterspec can be used (in case anyone wants to play around with it).

from testapp import models
from django.contrib.admin.filterspecs import FilterSpec
from django.contrib import admin
from django.utils.datastructures import SortedDict

class HowTastyFilterSpec(FilterSpec):
    def __init__(self, request, params, model, model_admin):
        super(HowTastyFilterSpec, self).__init__(request, params, model, model_admin)
        self.links = SortedDict((
            ('All', 'all'),
            ('Super good', 'super_good'),
            ('Pretty good', 'good'),
            ('Not great', 'not_great'),
            ('Gross', 'gross'),
        ))
        
    def consumed_params(self):
        return self.links.values()
    
    def choices(self, cl):
        selected = [v for v in self.links.values() if self.params.has_key(v)]
        for title, key in self.links.items():
            yield {'selected': self.params.has_key(key),
                   'query_string': cl.get_query_string({key:1}, selected),
                   'display': title}
    
    def get_query_set(self, cls, qs):
        if self.params.has_key('super_good'):
            return qs.filter(name__icontains='bacon')
        if self.params.has_key('good'):
            return qs.filter(name__icontains='burger')
        if self.params.has_key('not_great'):
            return qs.filter(name__icontains='haggas')
        if self.params.has_key('gross'):
            return qs.filter(name__icontains='guano')
        
        return qs
    
    def title(self):
        return u'How tasty?'

class FoodAdmin(admin.ModelAdmin):
    list_filter = [HowTastyFilterSpec]

admin.site.register(models.Food, FoodAdmin)
Last edited 13 years ago by Ben Davis (previous) (diff)

comment:69 by Ben Davis, 14 years ago

There was an issue when get_query_set returned an empty queryset, fixed in latest patch.

by Ben Davis, 14 years ago

Fixed non-field filterspec support, updated for django 1.2.1 (fixed)

comment:70 by 3point2, 14 years ago

Cc: 3point2 added

comment:71 by 3point2, 14 years ago

just wanted to +1 this, I think it would be a powerful feature (and I have a current need for it)

comment:72 by anonymous, 14 years ago

Cc: SafPlusPlus added

in reply to:  description comment:73 by anonymous, 14 years ago

Cc: krasniyrus@… added

Replying to Honza_Kral:

One should be able to create custom FilterSpecs for the admin's list_filter interface.

Honza_Kral: I modified the filterspec definition to allow for users to register their own filters in admin. The mechanism is simple - I just reverted the order of the registry so that newly registered specs will come first. That way if you register your own filter via FilterSpec.register, it will be used before the default one.

Another approach by korpios is described in the comments.

comment:74 by Yeago, 14 years ago

Cc: subsume@… added

One problem I have with this--and this may be completely irrelevant, is the get_query_set method.

The problem I see is that it accepts and returns a filtered queryset and these FilterSpecs chain like so:

Model.objects.filter(x=y).filter(z=y).filter(a=y)

When it comes to related objects, this is a problem because:

Model.objects.filter(xz=y).filter(xy=z).filter(yx=a)

...is not the same as:

Model.objects.filter(Q(xz=y)&Q(xy=z)&Q(yx=a))

One solution may be to allow get_query_set to return a Q() object and then string them together as above. Sure, you're still locked into a chain of &-joined Q objects but you already are and it could be the stuff of future tickets.

If there's a better place to have this discussion please forward me along. =). Thanks.

comment:75 by fas, 14 years ago

Can you please give a concrete example where multiple filter spec are used in such a way that it causes a problem?

comment:76 by Yeago, 14 years ago

Multiple filter specs won't become a problem until this feature is implemented. Right now, list_filter only accepts fields on the model in question. With this proposal FilterSpecs will now be opened up to conduct querying on related fields.

Concrete example: Take a situation where you have Person and related to it is a StaffContact. You have Person registered in Admin and you create two FilterSpecs, one called 'Contacted by' (which looks at the user field of StaffContact) and another called 'Contact range' (which looks at the field of StaffContact). Using these two fields together based on this implementation, chained fields can't give you all Person objects who were contacted by BOTH a particular staff person AND a particular date range, which might very well be preferable. Two JOINs will be created by this proposal, whereas you may have wanted one.

by Yeago, 14 years ago

Attachment: 5833-metapatch.patch added

An operator agnostic version of FilterSpec

comment:77 by Yeago, 13 years ago

Triage Stage: AcceptedDesign decision needed

switched back to design decision needed.

comment:78 by Yeago, 13 years ago

Triage Stage: Design decision neededAccepted

switched back to accepted.

comment:79 by Simon Litchfield, 13 years ago

Cc: Simon Litchfield added

comment:80 by dirleyrls, 13 years ago

Cc: dirleyrls@… added

comment:81 by Simon Litchfield, 13 years ago

See also #8528, which still makes sense even if this patch goes in. It provides a sensible default for null=True/blank=True fields, by giving the None option; where this ticket gives full control.

comment:82 by Vitek Pliska, 13 years ago

Cc: Vitek Pliska added

comment:83 by brentp, 13 years ago

Cc: bpederse@… removed

comment:84 by jovanb, 13 years ago

Replying to bendavis78:

Custom FilterSpec on a Model level instead of field level is frequently needed feature (ast least in my case), so thumbs-up for this patch.

comment:85 by Ben Davis, 13 years ago

This patch needs a major overhaul for 1.3. A lot of changes happened with the fix for #3400, see changeset r14674 for more info.

comment:86 by Ben Davis, 13 years ago

I took a stab at a patch for 1.3 -- approch w/ caution, YMMV, etc.. The validation logic seemed somewhat flawed, so I cleaned that up a bit. I wasn't too sure about whether or not to include subsume's changes. I think that still needs a little more discussion.

We still need TESTS and DOCUMENTATION on this.

comment:87 by Yeago, 13 years ago

I can only have so much discussion with myself. You either agree I have raised a valid limitation or you don't. You agree that my solution is a comprimising approach to open up more functionality, or you don't. There's a buzzword for this patch.... stopgap. Nobody has run into this problem filtering relations? Really? I was asked for examples and I gave them, then I wrote a patch. I think my changes deserve ample consideration.

comment:88 by Ben Davis, 13 years ago

I never said they didn't deserve consideration. I guess i'm still not sure I understand the problem your patch addresses (your example wasn't very clear to me). It seems to me that returning a queryset is the cleanest way. I may be wrong, I'm just not convinced otherwise. I'll look over it some more today.

comment:89 by Ben Davis, 13 years ago

@subsume, would you be able to provide a more concrete example than the one you gave earlier (e.g., models and example filterspecs)? It seems to me that most people would expect multiple filter specs to act like multiple .filter() calls. Each chain in the queryset further refines the previous one. Plus, if you take away the ability to modify the queryset, you're taking away a lot of flexibility (eg, the use of .exclude(), .extra(), etc...).

comment:90 by Yeago, 13 years ago

I can try and say it again plainly: filtering on fields of a related table will be done in separate JOINs using this method, whereas you may (and probably did...) only want one. If you try and filter for Companys that have a Location that is 1) open_saturday=True and 2) zip_code = 90210, the current approach leaves you in the cold because of how chained filtering works.

The current syntax is very seductive because of its simplicity but what is the point of a beautiful API that doesn't give you what you want?

You are not losing any flexibility--its wrong to say you can't use exclude(), extra(), etc--you most certainly can, its just a matter of knowing how Q and Q-like objects work (For example: "exclude" is simply ~Q(somefilter=True). Not difficult at all). From the perspective of filtering records, Q objects and their like are a more mature way to go about it because of their great mutability.

comment:91 by Ben Davis, 13 years ago

again, it would really help if you could give a test case (models.py and admin.py) so we can get a good idea of the problem you're encountering. For me, chaining the filters together produces the same effect as a &'d Q objects. I'm still not sure I understand what the actual undesired outcome is, which is why some concrete examples would help. Sorry, not trying to be a pain -- just having trouble grasping the issue you're raising.

comment:92 by Ben Davis, 13 years ago

subsume, I went ahead and created a test case for what I _think_ you're claiming is a problem, and I can't find the problem. You can get the code here: https://github.com/bendavis78/filter-test . Each of the filterspecs seems to behave as one would expect. Let me know what I'm missing.

comment:93 by Yeago, 13 years ago

Excellent work. I added a test to that github and sent you a pull request to review.

comment:94 by Ben Davis, 13 years ago

@subsume, I see what you're getting at. I still, however, think that yours is a rare use-case. In the case of the Comapnies/Locations example, I think it would make more sense to just have a LocationAdmin to filter on locations. While I still think most developers would want the default behavior to work like a normal queryset chain, I can see the benefit of having the option open, so as a compromise I propose we move the querset processing code out into a different function so that the default behavior can be overidden:

(from django/contrib/admin/views/main.py, near line 178)

    def apply_filter_specs(self, qs, lookup_params):
        # Let every filter spec modify the qs and params to its liking
        for filter_spec in self.filter_specs:
            new_qs = filter_spec.get_query_set(self, qs)
            if new_qs is not None and new_qs is not False:
                qs = new_qs
                # Only consume params if we got a new queryset
                for param in filter_spec.consumed_params():
                    try:
                        del lookup_params[param]
                    except KeyError:
                        pass
        return qs

    def get_query_set(self):
        qs = self.root_query_set
        lookup_params = self.params.copy() # a dictionary of the query string
        for i in (ALL_VAR, ORDER_VAR, ORDER_TYPE_VAR, SEARCH_VAR, IS_POPUP_VAR):
            if i in lookup_params:
                del lookup_params[i]
        key = ''
        for key, value in lookup_params.items():
            if not isinstance(key, str):
                # 'key' will be used as a keyword argument later, so Python
                # requires it to be a string.
                del lookup_params[key]
                lookup_params[smart_str(key)] = value

            # if key ends with __in, split parameter into separate values
            if key.endswith('__in'):
                lookup_params[key] = value.split(',')

            # if key ends with __isnull, special case '' and false
            if key.endswith('__isnull'):
                if value.lower() in ('', 'false'):
                    lookup_params[key] = False
                else:
                    lookup_params[key] = True
        
        qs = self.apply_filter_specs(qs, lookup_params)

        # Apply lookup parameters from the query string.

In your case, you would be able to override apply_filter_specs in to change that behavior. By no means am i the authority on this ticket, so if anyone else can chime with their opinion please do.

comment:95 by Yeago, 13 years ago

Definitely happy with this. Thanks.

by Ben Davis, 13 years ago

Patch for 1.3, updated w/ apply_filter_specs() method

comment:96 by Salva Pinyol, 13 years ago

Cc: Salva Pinyol added

comment:97 by Simon Charette, 13 years ago

Cc: Simon Charette added

comment:98 by Remco Wendt, 13 years ago

Cc: remco@… added

comment:99 by Simon Charette, 13 years ago

Is this ticket only missing docs and tests?

comment:100 by anonymous, 13 years ago

Cc: rohit.aggarwal@… added

comment:101 by anonymous, 13 years ago

milestone: 1.3

comment:102 by Russell Keith-Magee, 13 years ago

milestone: 1.3

1.3 feature deadline has passed.

comment:103 by Jeffrey Gelens, 13 years ago

Cc: jeffrey@… added

comment:104 by Ben Davis, 13 years ago

Patch won't apply to 1.3-rc1. Working on a new patch.

comment:105 by Ramiro Morales, 13 years ago

#15615 (closed as duplicate) asks for the same feature.

comment:106 by Julien Phalip, 13 years ago

milestone: 1.4

Putting this on the radar for 1.4. Want! ;)

comment:107 by Luke Plant, 13 years ago

milestone: 1.4

Sorry, but:

  • no docs - it doesn't exist
  • no tests - it doesn't work

This is a feature, not a bug, so until docs and tests appear, this cannot be on the radar screen, and it's not fair to put it in front of tickets which have these things. I'm going to remove the milestone again, just to make it clear we're absolutely serious about that, and to say that unless anyone is willing to come up with those things, there is no point suggesting it is a priority for any of the core developers.

(No offence meant to you, Julien, and all the great triaging work you are doing, BTW - thanks so much for that).

comment:108 by Yeago, 13 years ago

...but being on the 1.3 was fine and dandy? Ok.

comment:109 by Russell Keith-Magee, 13 years ago

@subsume: The point here is that putting a ticket on the milestone doesn't actually make anything happen. It just raises the false expectation that the feature will magically appear on it's own. We weren't very good about managing this expectation during the 1.3 cycle, so now that we have a clean slate, we have an opportunity to be more vigilant about this.

comment:110 by Karen Tracey, 13 years ago

Replying to subsume:

...but being on the 1.3 was fine and dandy? Ok.

No, looks to me like it was bumped out of 1.3 milestone minutes after being put in there, since it was put there after the feature deadline was passed. It's been put in just about every milestone since 1.1 beta, but no docs or tests have ever been included in any of the patches. Putting it in a milestone won't make these things appear, so perhaps the technique of not allowing it to be in the milestone until these these appear will help move it along. Doc/tests were asked for as long as 2 years ago...

(This is actually something I'd like to have in one of my personal projects, but it's pretty low priority for me and not worth figuring out how to use in the absence of any docs, so I've not ever looked in detail at it. I would be motivated to look at it if it had some doc.)

comment:111 by Julien Phalip, 13 years ago

Sorry all for the confusion I've been stirring here. Thanks Luke, Russ and Karen -- point taken. I've started a thread on django-dev about this so we don't pollute this ticket with too many meta conversations: http://groups.google.com/group/django-developers/browse_thread/thread/bf7da09f1f5881dc

comment:112 by Gabriel Hurley, 13 years ago

Severity: Normal
Type: New feature

by Julien Phalip, 13 years ago

Works with Django 1.3 (still no tests or doc)

comment:113 by Julien Phalip, 13 years ago

I've used bendavis78's patch as a baseline and made a few small fixes to make the existing test suite pass with current trunk (it should also work with 1.3). I thought I'd put the patch out there if someone wants to tinker with it. I'm now moving on to writing some tests for the new functionality -- hopefully that won't reveal too many annoying snags :-)

comment:114 by Julien Phalip, 13 years ago

OK, so I've tweaked the patch some more and tried to come up with an API that's (hopefully) simple and easy to use, introducing a new SimpleFilterSpec class. Here's an example:

# The model
class Person(models.Model):
    name = models.CharField(max_length=100)
    age = models.PositiveIntegerField()


from django.contrib.admin.filterspecs import SimpleFilterSpec

# The custom filter spec
class AgeCategoryFilterSpec(SimpleFilterSpec):
    
    def get_title(self):
        return u'age category'
    
    def get_choices(self, request):
        return (
            (u'0-19', u'0 to 19'),
            (u'20-39', u'20 to 39'),
            (u'40-over', u'40 and over'),
        )
    
    def get_query_set(self, cls, qs):
        age_category = self.get_value()
        if age_category == u'0-19':
            return qs.filter(age__gte=0, age__lte=19)
        if age_category == u'20-39':
            return qs.filter(age__gte=20, age__lte=39)
        if age_category == u'40-over':
            return qs.filter(age__gte=40)
        return qs

# The admin
class PersonAdmin(admin.ModelAdmin):
    list_display = ('name', 'age')
    list_filter = ('age', AgeCategoryFilterSpec,)

SimpleFilterSpec does all the boring work for you, i.e. yielding the iterator items when rendering the template, managing the query string both in and out, and fetching the requested value. All you need to provide is the title and the choices, and then you can filter the queryset based on the requested value. The lookup parameter defaults to the title slugified (i.e. "age-category" with the example above), although you can customise it by overriding the get_lookup_parameter() hook.

I think this API would cover the majority of use cases. If you want something more eccentric then you can always directly extend the FilterSpec class.

It's all implemented in the attached patch, including tests for this new feature.

Also, here are some notable changes I've made:

  • I've renamed the choices() method to _choices() as I think it feels more like internal API, and also to avoid users mistakingly overriding it instead of the proposed new official get_choices() method.
  • The name of the title() method didn't feel right so I've renamed it get_title(). Hopefully this is isn't considered backwards-incompatible (it's easily reversible if so).

So that's it. At this stage I'd really like to get some feedback both on the API and on the implementation approach, so please let me know what you think. If people like it then I'll write more tests and get started with the doc.

by Julien Phalip, 13 years ago

Introducing SimpleFilterSpecs?, with tests.

comment:115 by Julien Phalip, 13 years ago

One more thing, SimpleFilterSpec also automatically inserts "All" as the first, default option for the custom filter.

Last edited 13 years ago by Julien Phalip (previous) (diff)

by Julien Phalip, 13 years ago

Patch + tests + doc

comment:116 by Julien Phalip, 13 years ago

Needs documentation: unset
Needs tests: unset

Hi guys, so I've just posted a patch with some comprehensive tests and doc. As it is, I think the patch is pretty robust and works as advertised... that is, if we agree on the various sub-features and on the API. So I really need to get some feedback now. If you're interested in getting this feature shipped in core, please come join us in this django-dev thread: http://groups.google.com/group/django-developers/browse_thread/thread/ec7c4124e7317145

Cheers! ;)

by Julien Phalip, 13 years ago

Cleaner API based on Jacob's feedback so far

by Julien Phalip, 13 years ago

Now get_query_set() receives the request as parameter

comment:117 by guandalino, 13 years ago

Cc: guandalino added

cc add

comment:118 by Karen Tracey, 13 years ago

I experimented with this a bit today, and was able to use the doc to implement my particular need without too much difficulty so that is good.

One thing I thought was odd though was that my filter's get_query_set is called even if its query_parameter_name is not in request.GET, and as a result my get_query_set must be coded to handle that case properly (return the queryset unchanged). If it is not coded properly the result is that my filter limits the queryset used even when its "All" choice is the selected choice for it. I think it would be preferable to avoid calling the get_query_set for filters whose query parameters aren't in request.GET.

There is no mention anywhere I could see of why request is passed into get_choices and get_queryset, nor is it used that I can see anywhere in the samples. For what kinds of filters might this be used?

My particular use case is a slight modification of an existing admin filter: I want to filter by a related field, but I only want a subset of the existing values to appear in the choices. It was not hard to implement as a ListFilter but after getting get_query_set wrong the first time it occurred to me it would be better if I could just override the get_choices method for the existing admin filter that already filtered the way I wanted. Unfortunately that existing admin filter isn't implemented as a ListFilter so I can't do that, apparently I'd have to choose the scary-sounding alternate option of providing a tuple of field name and class inheriting from FieldListFilter (which is noted as being internal and prone to refactoring). Is it not possible to unify things so that it is easy to both write completely new-and-different filters and ones that are minor tweaks to what admin already provides?

in reply to:  118 comment:119 by Julien Phalip, 13 years ago

Replying to kmtracey:

I experimented with this a bit today, and was able to use the doc to implement my particular need without too much difficulty so that is good.

Thanks for experimenting with the patch, Karen!

One thing I thought was odd though was that my filter's get_query_set is called even if its query_parameter_name is not in request.GET, and as a result my get_query_set must be coded to handle that case properly (return the queryset unchanged). If it is not coded properly the result is that my filter limits the queryset used even when its "All" choice is the selected choice for it. I think it would be preferable to avoid calling the get_query_set for filters whose query parameters aren't in request.GET.

Yes, that's good point. I'll try to normalise that.

There is no mention anywhere I could see of why request is passed into get_choices and get_queryset, nor is it used that I can see anywhere in the samples. For what kinds of filters might this be used?

It is just there for your convenience in case you want to vary the list of choices or the list of results, for example, based on who is logged in. There's a note in the doc for get_choices() but not for get_query_set() yet. I'll try to improve the doc.

My particular use case is a slight modification of an existing admin filter: I want to filter by a related field, but I only want a subset of the existing values to appear in the choices. It was not hard to implement as a ListFilter but after getting get_query_set wrong the first time it occurred to me it would be better if I could just override the get_choices method for the existing admin filter that already filtered the way I wanted. Unfortunately that existing admin filter isn't implemented as a ListFilter so I can't do that, apparently I'd have to choose the scary-sounding alternate option of providing a tuple of field name and class inheriting from FieldListFilter (which is noted as being internal and prone to refactoring). Is it not possible to unify things so that it is easy to both write completely new-and-different filters and ones that are minor tweaks to what admin already provides?

In your use case it might make more sense to override the django.contrib.admin.filterspecs.RelatedFieldListFilter._choices() method. However you'll see that its code is quite opaque and not so fun to play with.

Your remarks make me realise that we should perhaps do a bigger refactor of the filtering system. The "old" filters (which currently inherit from FieldListFilter in my patch) only take care of managing the query string and the options to be displayed on the admin UI, where ChangeList.get_query_set() actually does the filtering on the queryset. This makes sense since both the ChangeList and the filters know how fields work and how they're supposed to be filtered (e.g. by appending "__isnull" to the field name). However, ChangeList.get_query_set() cannot anticipate how the queryset will be filtered by a custom filter, and therefore it has to delegate the actual filtering to the custom ListFilter object.

Also, currently the ListFilter only allows for one query string parameter, mostly for the sake of simplicity. However, some field filters allow for multiple ones, e.g. "fieldname__exact" or "fieldname__isnull" for RelatedFieldListFilter.

So, to make things more consistent, we should perhaps delegate all that logic to the filters themselves, including the queryset filtering which is currently done by the ChangeList object for field names.

I'm also going to try and make the field filters follow the new structure, for example by overriding get_choices() to return a list of option tuples, instead of having to directly yield the iteration items in _choices(). Hopefully this will be possible without too many ugly internal hacks.

by Julien Phalip, 13 years ago

comment:120 by Julien Phalip, 13 years ago

Karen, I have made a few changes in the latest patch. Now the get_query_set() method is only called when necessary (meaning there's no need to return the queryset unchanged by default). The actual queryset filtering is now also fully delegated from the ChangeList to the list filters themselves, which I think makes more sense and is more consistent between FieldListFilter's and ListFilter's behaviours. I think this should cover your use case.

Could you take another look and let me know what you think? Many thanks!

comment:121 by fas, 13 years ago

I have a few comments:

  • get_query_set should have no request parameter. It is passed in the init, you can set self.request there to be available for all the methods of the filter. has_output, title, ... could all depend on request. It is arbitrary that only get_query_set gets passed the request, especially if it can be accessed with self.request.
  • there is no reason why _choices() should have an underscore (as introduced in the latest patch). It is not a private method and intended to be overridden in inheriting filters.
  • the get_query_set method should be called no matter what is in the request parameters. See next point.
  • I think the whole used_params and query_parameter_name thing is too restricting. Who says I use only one parameter?

The better implementation is to provide get_value with the parameter key and a default value, like this:

def get_value(self, name, default):
    return self.params.get(name, default)

You would then usually do something like this:

class MyCustomFilter(FilterSpec):
    ALL = 'all'
    MY_VALUE1 = 'some-param-value'
    MY_VALUE2 = 'some-other-param-value'
    # ...
    DEFAULT_VALUE = MY_VALUE1

    def __init__(self, ...):
        super(...)...
        self.arg = 'my-parameter-key'
        self.value = self.get_value(self.arg, self.DEFAULT_VALUE)

    def get_query_set(self, qs):
        if self.value == self.ALL:
            return ....
        if self.value == self.MY_VALUE1:
            return ...


The get_query_set method should be called no matter what is in the parameters (as there should be no query_parameter_name in the first place).

  • When applying the get_query_set method, if it returns None, it should not be used. The problem of having to check if to call the method based on request parameters disappears, as do custom checks in the method.

On a side note, I have been using filters exactly like this (including all my points) for years and it works extremely well.

What do you think?

in reply to:  121 comment:122 by Julien Phalip, 13 years ago

Easy pickings: unset
Patch needs improvement: set

Thanks for reviewing the patch!

Replying to fas:

  • get_query_set should have no request parameter. It is passed in the init, you can set self.request there to be available for all the methods of the filter. has_output, title, ... could all depend on request. It is arbitrary that only get_query_set gets passed the request, especially if it can be accessed with self.request.

I would prefer passing the request as a parameter for the sake of being explicit. This would also be more consistent with the way things work in other public APIs in Django, for example in ModelAdmin.

  • there is no reason why _choices() should have an underscore (as introduced in the latest patch). It is not a private method and intended to be overridden in inheriting filters.

The two main reasons I've added an underscore is because I haven't come up with a clean and simple API for FieldListFilter yet and also to avoid the developer accidentally overriding it instead of the get_choices() method which is part of the suggested new public API. If we're to remove the underscore then I'd like at least to find a more distinct name for it.

  • I think the whole used_params and query_parameter_name thing is too restricting. Who says I use only one parameter?

The better implementation is to provide get_value with the parameter key and a default value, like this:

def get_value(self, name, default):
    return self.params.get(name, default)

You would then usually do something like this:

class MyCustomFilter(FilterSpec):
    ALL = 'all'
    MY_VALUE1 = 'some-param-value'
    MY_VALUE2 = 'some-other-param-value'
    # ...
    DEFAULT_VALUE = MY_VALUE1

    def __init__(self, ...):
        super(...)...
        self.arg = 'my-parameter-key'
        self.value = self.get_value(self.arg, self.DEFAULT_VALUE)

    def get_query_set(self, qs):
        if self.value == self.ALL:
            return ....
        if self.value == self.MY_VALUE1:
            return ...


The get_query_set method should be called no matter what is in the parameters (as there should be no query_parameter_name in the first place).

  • When applying the get_query_set method, if it returns None, it should not be used. The problem of having to check if to call the method based on request parameters disappears, as do custom checks in the method.

This all seems interesting. Could you elaborate on how the developer would specify the choices/options available in the filter?

Your approach seems valid but it still feels quite complex. I have been trying to come up with a very simple and easy API to cover most use cases. I believe that in most use cases one would just need one parameter. This is why I originally called the class SimpleFilterSpec (and in a later patch SimpleListFilter). But Jacob didn't like the split between SimpleListFilter and FieldListFilter (see his comment in http://groups.google.com/group/django-developers/browse_thread/thread/ec7c4124e7317145). I tend to agree with Jacob in that this split didn't seem ideal, but I also tend to agree with you that the current implementation of ListFilter is a bit too restrictive. Currently my preference would be to reintroduce SimpleListFilter to offer this dead-simple (yet restrictive) approach, and to modify ListFilterBase to accommodate for a less restrictive API (and also let FieldListFilter use that API to validate that it works).

I'll continue thinking this through, but please feel free to provide any other criticisms or insights.

by Julien Phalip, 13 years ago

comment:123 by Julien Phalip, 13 years ago

Patch needs improvement: unset

The latest patch addresses some of fas's concerns, i.e.:

  • get_query_set is always executed. If it returns nothing (or explicitly returns None) then the filter is ignored.
  • The request is not passed as a parameter to the filter's methods any more. Instead it is accessible via self.request as a convenience.
  • _choices() was renamed get_output_choices().

I think these points also address Karen's initial concerns and simplifies the API too.

I have also renamed ListFilter from the previous patch (which also used to be called SimpleListFilter in a patch before that) to SingleQueryParameterListFilter. This is a bit of a mouthful but at least it is explicit. Now, clearly, this is a specialized class rather than something claiming to address all use cases. The goal here is to provide a dead-simple API to cover a very common use case where there is only query parameter to deal with. I see this having a similar use as the simple_tag helper function which allows the creation of simple tags without having to deal with too much boilerplatey code. This approach also leaves the door open to adding more specialized filter classes in the future. By the way, fas, if you wish to have multiple parameters and a more complex logic, then you can continue doing the same as you always have.

The patch includes a pretty comprehensive test suite and doc to reflect how it all works. At this stage I think I have addressed all pending questions while allowing for an easy way of creating simple custom filters.

Any feedback would now be welcome to help move this forward. Thanks!

by Jannis Leidel, 13 years ago

Extended patch with further API refinements based on thorough patch review

comment:124 by Jannis Leidel, 13 years ago

Thanks everyone for the work on this patch, I've just reviewed it and took the liberty to modify the API a bit more:

  • Reverted the Java-esque SingleQueryParameterListFilter to SimpleListFilter again, which has a lookups (instead of get_choices) method as the only difference to the other filter classes.
  • Renamed get_output_choices to choices since it really is the best description for its functionality
  • Cleaned up some unneeded implementation details (e.g. ChangeList.apply_list_filters)
  • Stopped slugifying the title for the parameter name since it'll most likely be a ugettext_lazy variable. Instead the parameter_name is now a required class attribute like the title.

by Julien Phalip, 13 years ago

comment:125 by Julien Phalip, 13 years ago

Thanks Jannis! All your changes make sense. I've made a few more changes in the latest patch:

  • Renamed contrib.admin.filterspecs to contrib.admin.filters. Feels cleaner and makes more sense now that we've renamed everything from FilterSpec to ListFilter.
  • Removed references to parameter slugification in doc and code comments.
  • ChangeList.get_query_set() now receives the request as parameter (instead of setting a request attribute to the ChangeList object as introduced in patch 7).
  • Added tests to ensure that title and parameter_name are correctly defined.

comment:126 by Jannis Leidel, 13 years ago

Resolution: fixed
Status: assignedclosed

In [16144]:

Fixed #5833 -- Modified the admin list filters to be easier to customize. Many thanks to Honza Král, Tom X. Tobin, gerdemb, eandre, sciyoshi, bendavis78 and Julien Phalip for working on this.

comment:127 by Jannis Leidel, 13 years ago

In [16150]:

Corrected the behavior of the SimpleFilter.lookups method to also be able to return None. Also modified example in documentation to be a bite more realistic. Refs #5833. Thanks for the hint, Martin Mahner.

comment:128 by Jeffrey Gelens, 13 years ago

Cc: jeffrey@… removed
UI/UX: unset

comment:129 by anonymous, 13 years ago

Type: New featureBug

I did the upgrade to 1.3 version and this code doesn't work any longer:

class CustomChoiceFilterSpec(ChoicesFilterSpec):
	def __init__(self, f, request, params, model, model_admin):
		super(CustomChoiceFilterSpec, self).__init__(f, request, params, model, model_admin)
		self.lookup_kwarg = '%s__id__exact' % f.name # Change this to match the search of your object
		self.lookup_val = request.GET.get(self.lookup_kwarg, None)
		self.objects = model.objects.all()
		self.foreign_key = f.name
		self.foreign_key_count = {}
		for item in model.objects.values(f.name).annotate(count=Count('pk')):
			self.foreign_key_count[item[f.name]] = item['count']

	def choices(self, cl):
		yield {'selected': self.lookup_val is None,
			   'query_string': cl.get_query_string({}, [self.lookup_kwarg]),
			   'display': ('All')}
		items = set([getattr(i, self.foreign_key) for i in self.objects])
		for k in items:
			if k is None:
				kk = None
			else:
				kk = k.id
			yield {'selected': smart_unicode(kk) == self.lookup_val,
					'query_string': cl.get_query_string({self.lookup_kwarg: kk}), # Change .id to match what you are searching for
					'display': '%s (%s)' % (k, self.foreign_key_count[kk])}

FilterSpec.filter_specs.insert(0, (lambda f: getattr(f, 'compact_filter', False), CustomChoiceFilterSpec))

The error I get is: init() got an unexpected keyword argument 'field_path'

Please help me,

Fabio

comment:130 by anonymous, 13 years ago

Type: BugNew feature

comment:131 by Julien Phalip, 13 years ago

Please don't use Trac for asking "how to" questions and ask on the django-users mailing list instead.

By the way, the issue you're having is not relevant to this ticket but to the change introduced in r14674. Note also that the list_filter specs were still very internal in 1.3 and under, so you will have to update your code when upgrading your version of Django (but, again, ask on django-users if you need further help with that).

comment:132 by tejinderss@…, 12 years ago

Resolution: fixed
Status: closedreopened
Type: New featureBug

I have written a SimpleListFilter, here is the code: http://dpaste.com/639578/

It displays in the admin list properly, but i am having an issue, The selected option does not get highlighted in the custom filter. Only 'All' highlights but not the custom options. Here is the screenshot to illustrate that:

http://imgur.com/IyrYk

comment:133 by Julien Phalip, 12 years ago

Resolution: fixed
Status: reopenedclosed

Thanks for the report, but could you please open a new ticket? This one here is already overloaded with old discussions.

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