Code

Opened 7 years ago

Closed 3 years ago

#5833 closed Bug (fixed)

Custom FilterSpecs

Reported by: Honza_Kral Owned by: jkocherhans
Component: contrib.admin Version: master
Severity: Normal Keywords: nfa-someday list_filter filterspec nfa-changelist ep2008
Cc: simon29, eallik@…, semente@…, andreas@…, mbencun@…, chenyuejie@…, djangoproject@…, jan.rzepecki@…, kmike84@…, eandre@…, gonz, ciantic@…, rvdrijst, eppsilon, ramusus@…, peimei@…, marcoberi@…, david@…, rmanocha@…, sciyoshi@…, velmont, seanl, alexkoshelev, marinho, danfairs, timothy.broder@…, manfre, internet@…, isometry, bendavis78@…, 3point2, SafPlusPlus, krasniyrus@…, subsume@…, dirleyrls@…, vitek_pliska, spinyol, charettes, 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_Kral 7 years ago.
custom_filterspecs_plus_fieldless.patch (10.1 KB) - added by korpios 6 years ago.
Custom FilterSpecs, also allowing fieldless FilterSpecs
5833-against-7875.patch (3.7 KB) - added by Honza_Kral 6 years ago.
updated version of my simple patch,
5833-against-7875.2.patch (3.6 KB) - added by Honza_Kral 6 years ago.
5833-against-7875.3.patch (3.7 KB) - added by Honza_Kral 6 years ago.
filterspec_with_custom_queryset_against_1_0.diff (11.3 KB) - added by fas 6 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 5 years ago.
5833-against-9820.patch (11.4 KB) - added by gerdemb 5 years ago.
Patch updated to latest HEAD passing all tests
5833-against-9836-new-proper.patch (14.3 KB) - added by eandre@… 5 years ago.
New patch against r9836
5833.patch (14.6 KB) - added by sciyoshi 5 years ago.
New patch against 1.1/SVN
5833-custom-filter-spec-1.2.1.diff (14.7 KB) - added by bendavis78 4 years ago.
Fixed non-field filterspec support, updated for django 1.2.1 (fixed)
5833-metapatch.patch (2.6 KB) - added by subsume 4 years ago.
An operator agnostic version of FilterSpec
5833-custom-filter-spec-1.3.diff (15.6 KB) - added by bendavis78 4 years ago.
Patch for 1.3, updated w/ apply_filter_specs() method
5833.custom-filterspecs.diff (15.5 KB) - added by julien 3 years ago.
Works with Django 1.3 (still no tests or doc)
5833.custom-filterspecs.2.diff (34.6 KB) - added by julien 3 years ago.
Introducing SimpleFilterSpecs?, with tests.
5833.custom-filterspecs.3.diff (49.1 KB) - added by julien 3 years ago.
Patch + tests + doc
5833.custom-filterspecs.4.diff (50.0 KB) - added by julien 3 years ago.
Cleaner API based on Jacob's feedback so far
5833.custom-filterspecs.5.diff (53.3 KB) - added by julien 3 years ago.
Now get_query_set() receives the request as parameter
5833.custom-filterspecs.6.diff (67.1 KB) - added by julien 3 years ago.
5833.custom-filterspecs.7.diff (69.9 KB) - added by julien 3 years ago.
5833.custom-filterspecs.8.diff (75.9 KB) - added by jezdez 3 years ago.
Extended patch with further API refinements based on thorough patch review
5833.custom-filterspecs.9.diff (81.4 KB) - added by julien 3 years ago.

Download all attachments as: .zip

Change History (155)

Changed 7 years ago by Honza_Kral

comment:1 Changed 7 years ago by jkocherhans

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to jkocherhans
  • Patch needs improvement unset
  • Status changed from new to assigned

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 Changed 7 years ago by Honza_Kral

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

comment:3 Changed 7 years ago by brosner

  • Keywords nfa-someday added; newforms admin removed

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

comment:4 Changed 7 years ago by buriy

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

comment:5 Changed 6 years ago by 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,
)

Changed 6 years ago by korpios

Custom FilterSpecs, also allowing fieldless FilterSpecs

comment:6 Changed 6 years ago by korpios

  • Cc korpios@… added

comment:7 Changed 6 years ago by korpios

  • Description modified (diff)
  • Summary changed from newforms-admin enable registering custom FilterSpecs to [newforms-admin] Custom FilterSpecs

Tweaked the summary and description.

comment:8 Changed 6 years ago by anonymous

  • Triage Stage changed from Unreviewed to Design decision needed

comment:9 Changed 6 years ago by jacob

that was me; sorry.

comment:10 Changed 6 years ago by ales_zoulek

  • Keywords nfa-changelist added

comment:11 Changed 6 years ago by RaceCondition

  • Cc eallik@… added

Changed 6 years ago by Honza_Kral

updated version of my simple patch,

comment:12 Changed 6 years ago by Honza_Kral

  • Keywords ep2008 added

Changed 6 years ago by Honza_Kral

Changed 6 years ago by Honza_Kral

comment:13 Changed 6 years ago by Alex

  • Version changed from newforms-admin to SVN

comment:14 follow-up: Changed 6 years ago by elwaywitvac <elwaywitvac@…>

  • 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 Changed 6 years ago by elwaywitvac <elwaywitvac@…>

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

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

  • Cc semente@… added

comment:17 Changed 6 years ago by korpios

  • Cc korpios@… removed

comment:18 Changed 6 years ago by anonymous

  • Cc andreas@… added

comment:19 Changed 6 years ago by fas

  • Cc mbencun@… added

comment:20 in reply to: ↑ 14 Changed 6 years ago by 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.

Changed 6 years ago by fas

Custom Filtersets (fieldless) with custom query set manipulation.

comment:21 Changed 6 years ago by yuejie

  • Cc chenyuejie@… added

comment:22 Changed 6 years ago by anonymous

  • Cc bpederse@… added

comment:23 Changed 6 years ago by anonymous

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 Changed 6 years ago by gerdemb

  • 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 Changed 6 years ago by niels

  • Cc niels.busch@… added

comment:26 Changed 6 years ago by xtrqt

  • Cc jan.rzepecki@… added

comment:27 Changed 5 years ago by 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 ¶

comment:28 Changed 5 years ago by kmike

  • Cc kmike84@… added

Changed 5 years ago by gerdemb

Patch updated to latest HEAD passing all tests

comment:29 Changed 5 years ago by 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.

comment:31 Changed 5 years ago by eandre@…

  • Cc eandre@… added
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Design decision needed to Accepted

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.

Changed 5 years ago by eandre@…

New patch against r9836

comment:32 Changed 5 years ago by 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.

comment:33 Changed 5 years ago by kmtracey

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

comment:34 Changed 5 years ago by gonz

  • Cc gonz added

comment:35 Changed 5 years ago by niels

  • Cc niels.busch@… removed

comment:36 Changed 5 years ago by jacob

  • milestone set to 1.1 beta

comment:37 Changed 5 years ago by anonymous

  • Cc ciantic@… added

comment:38 Changed 5 years ago by rvdrijst

  • Cc rvdrijst added

comment:39 Changed 5 years ago by kmtracey

  • Needs documentation set
  • Summary changed from [newforms-admin] Custom FilterSpecs to Custom 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 follow-up: Changed 5 years ago by jacob

  • milestone 1.1 beta deleted

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

comment:41 in reply to: ↑ 40 Changed 5 years ago by Honza_Kral

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 Changed 5 years ago by brosner

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 Changed 5 years ago by mrts

  • milestone set to 1.2

Re-targeting for 1.2.

comment:44 Changed 5 years ago by eppsilon

  • Cc eppsilon added

comment:45 Changed 5 years ago by anonymous

  • Cc ramusus@… added

comment:46 Changed 5 years ago by anonymous

  • Cc peimei@… added

comment:47 Changed 5 years ago by marcob

  • Cc marcoberi@… added

comment:48 Changed 5 years ago by anonymous

  • Cc david@… added

comment:49 Changed 5 years ago by anonymous

  • Cc sciyoshi@… added

comment:50 Changed 5 years ago by sciyoshi

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

Changed 5 years ago by sciyoshi

New patch against 1.1/SVN

comment:51 Changed 5 years ago by marcob

When you delete a record, querystring looses return_to_ parameter.

(BTW last patch seems not working for me)

comment:52 Changed 5 years ago by anonymous

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

comment:53 Changed 5 years ago by velmont

  • Cc velmont added

comment:54 Changed 5 years ago by anonymous

  • Cc seanl added

comment:55 Changed 5 years ago by anonymous

comment:56 Changed 5 years ago by alexkoshelev

  • Cc alexkoshelev added

comment:57 Changed 5 years ago by marinho

  • Cc marinho added

comment:58 Changed 4 years ago by danfairs

  • Cc danfairs added

comment:59 Changed 4 years ago by 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: 

comment:60 Changed 4 years ago by russellm

#12173 raised the issue of ORs in filterspecs.

comment:61 Changed 4 years ago by anonymous

  • Cc rmanocha@… added

comment:62 Changed 4 years ago by ubernostrum

  • milestone 1.2 deleted

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

comment:63 Changed 4 years ago by broderboy

  • Cc timothy.broder@… added

comment:64 Changed 4 years ago by manfre

  • Cc manfre added

comment:65 Changed 4 years ago by dominicrodger

  • Cc internet@… added

comment:66 Changed 4 years ago by isometry

  • Cc isometry added

comment:67 Changed 4 years ago by bendavis78

  • Cc bendavis78@… added

comment:68 Changed 4 years ago by bendavis78

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 3 years ago by bendavis78 (previous) (diff)

comment:69 Changed 4 years ago by bendavis78

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

Changed 4 years ago by bendavis78

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

comment:70 Changed 4 years ago by 3point2

  • Cc 3point2 added

comment:71 Changed 4 years ago by 3point2

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

comment:72 Changed 4 years ago by anonymous

  • Cc SafPlusPlus added

comment:73 in reply to: ↑ description Changed 4 years ago by anonymous

  • 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 Changed 4 years ago by subsume

  • 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 Changed 4 years ago by fas

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

comment:76 Changed 4 years ago by subsume

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.

Changed 4 years ago by subsume

An operator agnostic version of FilterSpec

comment:77 Changed 4 years ago by subsume

  • Triage Stage changed from Accepted to Design decision needed

switched back to design decision needed.

comment:78 Changed 4 years ago by subsume

  • Triage Stage changed from Design decision needed to Accepted

switched back to accepted.

comment:79 Changed 4 years ago by simon29

  • Cc simon29 added

comment:80 Changed 4 years ago by dirleyrls

  • Cc dirleyrls@… added

comment:81 Changed 4 years ago by simon29

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 Changed 4 years ago by vitek_pliska

  • Cc vitek_pliska added

comment:83 Changed 4 years ago by brentp

  • Cc bpederse@… removed

comment:84 Changed 4 years ago by 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.

comment:85 Changed 4 years ago by bendavis78

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 Changed 4 years ago by bendavis78

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 Changed 4 years ago by subsume

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 Changed 4 years ago by bendavis78

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 Changed 4 years ago by bendavis78

@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 Changed 4 years ago by subsume

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 Changed 4 years ago by bendavis78

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 Changed 4 years ago by bendavis78

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 Changed 4 years ago by subsume

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

comment:94 Changed 4 years ago by bendavis78

@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 Changed 4 years ago by subsume

Definitely happy with this. Thanks.

Changed 4 years ago by bendavis78

Patch for 1.3, updated w/ apply_filter_specs() method

comment:96 Changed 4 years ago by spinyol

  • Cc spinyol added

comment:97 Changed 4 years ago by charettes

  • Cc charettes added

comment:98 Changed 3 years ago by shanx

  • Cc remco@… added

comment:99 Changed 3 years ago by charettes

Is this ticket only missing docs and tests?

comment:100 Changed 3 years ago by anonymous

  • Cc rohit.aggarwal@… added

comment:101 Changed 3 years ago by anonymous

  • milestone set to 1.3

comment:102 Changed 3 years ago by russellm

  • milestone 1.3 deleted

1.3 feature deadline has passed.

comment:103 Changed 3 years ago by jgelens

  • Cc jeffrey@… added

comment:104 Changed 3 years ago by bendavis78

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

comment:105 Changed 3 years ago by ramiro

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

comment:106 Changed 3 years ago by julien

  • milestone set to 1.4

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

comment:107 Changed 3 years ago by lukeplant

  • milestone 1.4 deleted

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 Changed 3 years ago by subsume

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

comment:109 Changed 3 years ago by russellm

@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 Changed 3 years ago by kmtracey

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 Changed 3 years ago by julien

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 Changed 3 years ago by gabrielhurley

  • Severity set to Normal
  • Type set to New feature

Changed 3 years ago by julien

Works with Django 1.3 (still no tests or doc)

comment:113 Changed 3 years ago by julien

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 Changed 3 years ago by julien

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.

Changed 3 years ago by julien

Introducing SimpleFilterSpecs?, with tests.

comment:115 Changed 3 years ago by julien

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

Last edited 3 years ago by julien (previous) (diff)

Changed 3 years ago by julien

Patch + tests + doc

comment:116 Changed 3 years ago by julien

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

Changed 3 years ago by julien

Cleaner API based on Jacob's feedback so far

Changed 3 years ago by julien

Now get_query_set() receives the request as parameter

comment:117 Changed 3 years ago by guandalino

  • Cc guandalino added

cc add

comment:118 follow-up: Changed 3 years ago by 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.

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?

comment:119 in reply to: ↑ 118 Changed 3 years ago by julien

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.

Changed 3 years ago by julien

comment:120 Changed 3 years ago by julien

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 follow-up: Changed 3 years ago by 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?

comment:122 in reply to: ↑ 121 Changed 3 years ago by julien

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

Changed 3 years ago by julien

comment:123 Changed 3 years ago by julien

  • 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!

Changed 3 years ago by jezdez

Extended patch with further API refinements based on thorough patch review

comment:124 Changed 3 years ago by jezdez

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.

Changed 3 years ago by julien

comment:125 Changed 3 years ago by julien

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 Changed 3 years ago by jezdez

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

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 Changed 3 years ago by jezdez

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 Changed 3 years ago by jgelens

  • Cc jeffrey@… removed
  • UI/UX unset

comment:129 Changed 3 years ago by anonymous

  • Type changed from New feature to Bug

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 Changed 3 years ago by anonymous

  • Type changed from Bug to New feature

comment:131 Changed 3 years ago by julien

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 Changed 3 years ago by tejinderss@…

  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Type changed from New feature to Bug

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 Changed 3 years ago by julien

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

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.