Code

Opened 20 months ago

Closed 20 months ago

Last modified 20 months ago

#19318 closed Bug (fixed)

Admin's SimpleListFilter option not being displayed as selected if the lookup's first element is not a string

Reported by: sebasmagri Owned by: nobody
Component: contrib.admin Version: 1.4
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: yes UI/UX: no

Description

I had the following lookups method for a SimpleListFilter:

def lookups(self, request, model_admin):
    return set(itertools.chain(*[
        [(o.deliverable.id, o.deliverable.name)
         for o in user.orders.all()]
        for user in model_admin.queryset(request).all()
    ]))

In this case, the filters in the change list didn't display the option as selected on activation.

Changing the above code to

def lookups(self, request, model_admin):
    return set(itertools.chain(*[
        [(str(o.deliverable.id), o.deliverable.name)
         for o in user.orders.all()]
        for user in model_admin.queryset(request).all()
    ]))

Fixed the issue.

This seems to be caused by django/contrib/admin/filters.py:105, where

yield {
    'selected': self.value() == lookup,
    'query_string': cl.get_query_string({
         self.parameter_name: lookup,
    }, []),
    'display': title,
}

Might be

yield {
    'selected': self.value() == str(lookup),
    'query_string': cl.get_query_string({
         self.parameter_name: lookup,
    }, []),
    'display': title,
}

I've created a pull request at GitHub with this change.

Attachments (0)

Change History (7)

comment:1 Changed 20 months ago by julien

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted
  • UI/UX unset

Thanks for the report. Using force_unicode would be more appropriate than str.

comment:2 Changed 20 months ago by sebasmagri

I have updated the pull request to use force_unicode instead of str.

Regarding tests, I would like to get some help on it as I don't have too much experience with testing.

comment:3 Changed 20 months ago by julien

Take a look at the existing tests in tests/regressiontests/admin_filters/tests.py. You should find multiple examples to get started. Have a go at it and then come back with questions if you still need any further assistance.

comment:4 Changed 20 months ago by sebasmagri

I've closed the old pull requests and I have created two new pull requests, one for 1.4.x and the other for 1.5.x. The fix in different in both cases. Because of the py3k compatibility in 1.5 the force_unicode utility function have been renamed to force_text.

Pull requests are 539 and 540.

Please let me know if there is any other requirement.

Best Regards,

comment:5 Changed 20 months ago by Julien Phalip <jphalip@…>

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

In 88e17156393be86df64d44554de8ab77a6d3ed7d:

Fixed #19318 -- Ensured that the admin's SimpleListFilter options can be displayed as selected even if the lookup's first element is not a string.

comment:6 Changed 20 months ago by Julien Phalip <jphalip@…>

In 237a404d3ec678d1088dcd0b3630a3ea67ce08ea:

[1.5.x] Fixed #19318 -- Ensured that the admin's SimpleListFilter options can be displayed as selected even if the lookup's first element is not a string.
Backport of 88e17156393b

comment:7 Changed 20 months ago by Julien Phalip <jphalip@…>

In c72172244ef81f793d1eca9a54f58a11f27b0917:

[1.4.x] Fixed #19318 -- Ensured that the admin's SimpleListFilter options can be displayed as selected even if the lookup's first element is not a string.
Backport of 88e17156393b

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.