Code

Opened 3 years ago

Closed 3 years ago

#17091 closed Bug (fixed)

Djano 1.4 SimpleListFilter 'selected' option issue

Reported by: tejinderss@… Owned by: julien
Component: contrib.admin Version: master
Severity: Normal Keywords: SimpleListFilter, FilterSpec
Cc: Triage Stage: Design decision needed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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

Attachments (1)

17091.changelist-lookup-filters-untangling.diff (25.5 KB) - added by julien 3 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 3 years ago by julien

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

What is happening here is that your parameter name ends with "__in" which is interpreted as if the value passed is a list. So for example "?status__in=0,30,40" is interpreted as if the value passed is [0,30,40], not "0,30,40". To be honest, at this stage I'm not sure if it's a bug or a feature.

If you renamed your parameter to "status_in" (with just one underscore) then it should just work. I'd like to understand your use case better though. Could you please explain your original motivation for having 2 underscores in the parameter name?

I'm keen to know to figure out whether we should treat it as a bug or rather as an opportunity to build a new feature around it. Thanks!

comment:2 Changed 3 years ago by anonymous

I am using in operator to filter results by multiple status values, i.e. ?statusin=0,30,40 filters results with status values 0, 30 and 40. Thats what i want. status_in (with single _) does not work as expected, it just redirects to ?e=1 without filtered results. Even the default filters use 2 _ notation. Like if i use default filter "Status", the url is ?statusexact=30. Thats the default notation i guess and only works. Here is the example of default django filter behavior.

http://i.imgur.com/RwRTI.png

Version 0, edited 3 years ago by anonymous (next)

comment:3 Changed 3 years ago by tejinderss@…

I am using in operator to filter results by multiple status values, i.e. !?status__in=0,30,40 filters results with status values 0, 30 and 40. Thats what i want. status_in (with single _) does not work as expected, it just redirects to ?e=1 without filtered results. Even the default filters use 2 _ notation. Like if i use default filter "Status", the url is ?status__exact=30. Thats the default notation i guess and only __ works. Here is the example of default django filter behavior.

http://i.imgur.com/RwRTI.png

Edit: Sorry for messed up formatting in previous reply.

comment:4 Changed 3 years ago by julien

Could you please provide all your relevant admin and model definitions? I think this might be a case where the SimpleListFilter should *not* be used and where the Django documentation could be improved.

comment:5 Changed 3 years ago by anonymous

Here is the model definition:
http://dpaste.com/640035/

Here is the ResAdmin definition:
http://dpaste.com/640036/

comment:6 Changed 3 years ago by anonymous

How else i can achieve this? Filtering result using multiple choices if not SimpleListFilter ?

comment:7 Changed 3 years ago by anonymous

Actually I am exporting the filtered results to CSV file, hence i chose this parameter name so that export_csv file can get the parameter status__in=0,30,40 and get fetch the filtered queryset and export to csv file. This the the reason of using the status__in parameter in the url.

comment:8 Changed 3 years ago by anonymous

s/export_csv file/export_csv view

comment:9 Changed 3 years ago by julien

In theory, I think the proper way would be to do:

class ResAdminListFilter(SimpleListFilter):
    title = 'Multiple Status' 

    parameter_name = "status"  # Do not put double underscores in here

    def lookups(self, request, model_admin):
        return (
            ('0,30,40', _('All but BN')),
            ('200,300', _('Only BN')),
        )
    def queryset(self, request, queryset): # Write the code below (instead of just "pass")
        statuses = self.value().split(',')
        return queryset.filter(status__in=statuses)
class ResAdmin(MultilingualAdminMixin, ResAdminMixin, ModelAdminDMY):
    list_filter = ('supplier', 'created', ResAdminListFilter, )  # Same as before but without 'status'
    ...

Could you try that and see if that works?

comment:10 Changed 3 years ago by anonymous

Database error
Something's wrong with your database installation. Make sure the appropriate database tables have been created, and make sure the database is readable by the appropriate user.

comment:11 Changed 3 years ago by anonymous

Ok this thing worked:

def queryset(self, request, queryset):

if self.value() == 'nobn':

return queryset.filter(statusin=(0, 30, 40))

if self.value() == 'bn':

return queryset.filter(statusin=(200, 300))

Alternatively i also tried overriding the value() method in my custom filter like this:

def value(self): #Bug fix, passing '30,40' changes to list and it does not match the params val

val = self.params.get(self.parameter_name, None)
if isinstance(val, str):

return val

if isinstance(val, list):

return ','.join(val)

Maybe we can add this feature? But i guess the first approach is better. Thanks for the help. I guess this ticket is closed now.

comment:12 Changed 3 years ago by julien

  • Owner changed from nobody to julien

Well, the first approach (which works) is the expected use and the documented one :-)

Now there's still one small bug i.e. that the value of a parameter whose name finishes with "__in" should not be interpreted as a list. I'll fix that soon, so you can leave this ticket open.

I'll also see if the doc can be clarified to avoid any confusion. Thanks for your patience!

comment:13 Changed 3 years ago by anonymous

Thanks a lot.

comment:14 Changed 3 years ago by julien

While looking at the ChangeList class' internals for the needs of this ticket, I realized that there were a few issues with the sequence in which admin filters and other query string lookups were processed. This led me to operate a significant refactor of those internals to untangle things a bit. With this patch, the query string lookups are processed once instead of twice and some bugs are avoided —in particular the SimpleListFilter parameter name being mistaken for a model field in some cases. The code for the ChangeList's get_filters() and get_query_set() methods is now also much more streamlined.

comment:15 Changed 3 years ago by julien

In [17145]:

(The changeset message doesn't reference this ticket)

comment:16 Changed 3 years ago by julien

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

I don't think we need to modify the documentation as it's clear-enough in relation to the problem reported in this ticket. I believe the original confusion here was in fact caused by that bug, which is now fixed. Therefore considering this ticket fixed as well.

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.