Opened 4 years ago

Closed 21 months ago

Last modified 20 months ago

#32539 closed New feature (fixed)

Support accessing the current filtered queryset from within SimpleListFilter.lookups()

Reported by: Simon Willison Owned by: Sarah Boyce
Component: contrib.admin Version: 3.1
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

It would be really useful if code running inside a custom SimpleListFilter.lookups() method could access the filtered queryset for the current ModelAdmin instance, based on the current request.

Most significantly, this would allow admin filters to display counts for the number of items that would be returned if they were selected - for a classic faceted browse interface.

Sadly this isn't possible at the moment. I wrote about this in more detail here: https://til.simonwillison.net/django/almost-facet-counts-django-admin - but the short version is that the following:

class StateCountFilter(admin.SimpleListFilter):
    # ...
    def lookups(self, request, model_admin):
        changelist = model_admin.get_changelist_instance(request)
        qs = changelist.get_queryset(request)
        states_and_counts = qs.values_list(
            "state__abbreviation", "state__name"
        ).annotate(n = Count('state__abbreviation'))

Raises a RecursionError because the model_admin.get_changelist_instance(request) method itself triggers a call to this .lookups() method.

Change History (19)

comment:1 by Mariusz Felisiak, 4 years ago

Triage Stage: UnreviewedAccepted
Type: UncategorizedNew feature

Tentatively accepted, it looks reasonable but I'm not sure if it's doable with the current objects relations. Would you like to work on a patch?

comment:2 by Simon Willison, 4 years ago

I'd love to, but I'm a bit stuck on where to begin with this.

Like you, I'm skeptical that this is a simple bug fix. I think there are two options here:

  • A medium-sized change to the architecture of the admin, such that .lookups() no longer gets called when the ChangeList is constructed. I'm not at all sure how much work this would be or if it could be implemented without breaking existing custom code.
  • A new, optional mechanism for people who want to use model_admin.get_changelist_instance(request) inside their .lookups() method - potentially even defining a new .lazy_options(...) method that people can implement which deliberately doesn't get called until later on, to avoid this RecursionError

I'd like it if there was a third option, "just rewrite things in an invisible way such that this works" - but my hunch is that's not possible. And the two options I listed above both warrant some discussion with people who are more familiar with the internals of the admin than I am!

comment:3 by Sarah Boyce, 3 years ago

I was wondering if this is being worked on or if I could try to pick it up, happy to work on it in a group as well

comment:4 by Carlton Gibson, 3 years ago

Hi Sarah — you're very welcome to pick it up. 🙂

I'd suggest an initial time-boxed investigation to work out what the state of play is.

At that point you'll be the world expert™ on this issue, so either you'll have an idea or you'll want input. If you open a PR at that point (even just adding comments in the second case) to explain what's going on, it's easier for others to get up to speed and input.

comment:5 by Shreya Bamne, 3 years ago

Owner: changed from nobody to Shreya Bamne
Status: newassigned

I would like to try working on this ticket. I am not sure if someone is already working on this. (Claiming the ticket for now. I am happy to collaborate if someone is already working on this ticket. :))

comment:6 by Shreya Bamne, 3 years ago

I have added comments in few files which would help to understand the issue. I am not sure what next steps are to fix it and I am looking for some input to begin working on this ticket. I hope the comments are helpful.

comment:7 by Shreya Bamne, 3 years ago

Owner: Shreya Bamne removed
Status: assignednew

comment:8 by Sarah Boyce, 22 months ago

Has patch: set
Owner: set to Sarah Boyce
Status: newassigned

comment:9 by Carlton Gibson, 22 months ago

The PR here looks promising. With a simple test project it correctly allows faceted (with counts) filtering.

Version 1, edited 22 months ago by Carlton Gibson (previous) (next) (diff)

comment:10 by Mariusz Felisiak, 22 months ago

Patch needs improvement: set

comment:11 by Sarah Boyce, 22 months ago

Patch needs improvement: unset

Second point is in the PR now, I have tried to improve the documentation, I think I would like to add a picture (as it will illustrate it better) but I'm not sure where the images are coming from - is there a project that is used for the documentation images?

comment:12 by Mariusz Felisiak, 22 months ago

Patch needs improvement: set

See comment.

comment:13 by Sarah Boyce, 22 months ago

Patch needs improvement: unset

comment:14 by Mariusz Felisiak, 21 months ago

Patch needs improvement: set

comment:15 by Mariusz Felisiak, 21 months ago

Patch needs improvement: unset

comment:16 by Mariusz Felisiak, 21 months ago

Triage Stage: AcceptedReady for checkin

comment:17 by Mariusz Felisiak <felisiak.mariusz@…>, 21 months ago

Resolution: fixed
Status: assignedclosed

In 868e2fcd:

Fixed #32539 -- Added toggleable facet filters to ModelAdmin.

Thanks Carlton Gibson, Simon Willison, David Smith, and Mariusz
Felisiak for reviews.

comment:18 by GitHub <noreply@…>, 21 months ago

In 2660bc1:

Refs #32539 -- Fixed facet filter tests on Oracle.

Follow up to 868e2fcddae6720d5713924a785339d1665f1bb9.

comment:19 by GitHub <noreply@…>, 20 months ago

In cffcf0e:

Refs #32539 -- Fixed hide counts icon for RTL languages.

Bug in 868e2fcddae6720d5713924a785339d1665f1bb9.

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