Custom FilterSpecs

[Honza Král]

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.

[jkocherhans]

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.

[Honza Král]

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

[Brian Rosner]

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

[Yuri Baburov]

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

[korpios]

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,
)

[korpios]

Custom FilterSpecs, also allowing fieldless FilterSpecs

[korpios]

Tweaked the summary and description.

[Jacob]

^{that was me; sorry.}

[Honza Král]

updated version of my simple patch,

[elwaywitvac <elwaywitvac@…>]

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.

[elwaywitvac <elwaywitvac@…>]

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

[fas]

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.

[fas]

Custom Filtersets (fieldless) with custom query set manipulation.

[anonymous]

line 103 of filterspec_with_custom_queryset_against_1_0.diff
should read:

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

[gerdemb]

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. :)

[gerdemb]

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 ¶

[gerdemb]

Patch updated to latest HEAD passing all tests

[gerdemb]

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.

[gerdemb]

Developer discussion here: ​http://groups.google.com/group/django-developers/browse_thread/thread/eb08729417d50ef3

[eandre@…]

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.

[eandre@…]

New patch against r9836

[eandre@…]

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.

[Karen Tracey]

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

[Karen Tracey]

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.

[Jacob]

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

[Honza Král]

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.

[Brian Rosner]

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.

[mrts]

Re-targeting for 1.2.

[Samuel Cormier-Iijima]

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

[Samuel Cormier-Iijima]

New patch against 1.1/SVN

[marcob]

When you delete a record, querystring looses return_to_ parameter.

(BTW last patch seems not working for me)

[anonymous]

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

[vrehak]

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: 

[Russell Keith-Magee]

#12173 raised the issue of ORs in filterspecs.

[James Bennett]

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

[Ben Davis]

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 HowYummyFilterSpec(FilterSpec):
    def __init__(self, request, params, model, model_admin):
        super(HowYummyFilterSpec, self).__init__(request, params, model, model_admin)
        self.links = SortedDict((
            ('All', 'all'),
            ('Double Rainbow', 'so_intense'),
            ('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('so_intense'):
            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 yummy?'

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

admin.site.register(models.Food, FoodAdmin)

[Ben Davis]

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

[Ben Davis]

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

[3point2]

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

[anonymous]

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.

[Yeago]

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.

[fas]

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

[Yeago]

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.

[Yeago]

An operator agnostic version of FilterSpec

[Yeago]

switched back to design decision needed.

[Yeago]

switched back to accepted.

[Simon Litchfield]

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.

[jovanb]

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.

[Ben Davis]

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.

[Ben Davis]

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.

[Yeago]

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.

[Ben Davis]

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.

[Ben Davis]

@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...).

[Yeago]

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.

[Ben Davis]

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.

[Ben Davis]

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.

[Yeago]

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

[Ben Davis]

@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.

[Yeago]

Definitely happy with this. Thanks.

[Ben Davis]

Patch for 1.3, updated w/ apply_filter_specs() method

[Simon Charette]

Is this ticket only missing docs and tests?

[Russell Keith-Magee]

1.3 feature deadline has passed.

[Ben Davis]

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

[Ramiro Morales]

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

[Julien Phalip]

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

[Luke Plant]

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).

[Yeago]

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

[Russell Keith-Magee]

@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.

[Karen Tracey]

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.)

[Julien Phalip]

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

[Julien Phalip]

Works with Django 1.3 (still no tests or doc)

[Julien Phalip]

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 :-)

[Julien Phalip]

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.

[Julien Phalip]

Introducing SimpleFilterSpecs?, with tests.

[Julien Phalip]

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

[Julien Phalip]

Patch + tests + doc

[Julien Phalip]

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! ;)

[Julien Phalip]

Cleaner API based on Jacob's feedback so far

[Julien Phalip]

Now get_query_set() receives the request as parameter

[guandalino]

cc add

[Karen Tracey]

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?

[Julien Phalip]

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.

[Julien Phalip]

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!

[fas]

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?

[Julien Phalip]

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.

[Julien Phalip]

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!

[Jannis Leidel]

Extended patch with further API refinements based on thorough patch review

[Jannis Leidel]

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.

[Julien Phalip]

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.

[Jannis Leidel]

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.

[Jannis Leidel]

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.

[anonymous]

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

[Julien Phalip]

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).

[tejinderss@…]

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

[Julien Phalip]

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