Opened 18 years ago

Closed 12 months ago

#1873 closed New feature (fixed)

Multi-select admin filter for RelatedFields

Reported by: marc@… Owned by: Sarah Boyce
Component: contrib.admin Version:
Severity: Normal Keywords: nfa-changelist
Cc: msaelices@…, Luis Bruno, Andy Baker, cmawebsite@…, Sarah Boyce 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

Currently, you can only select one value from a RelatedField filter in de admin interface.
This patch provides a multi-select filter, and an AND/OR switch for the values.

The patch is at

Screenshots are at

This replaces the current single-select filter, but could be used side-by-side if there was some mechanism to choose which filter to use for a particular field in the admin interface.

Todo: test what happens with multiple RelatedField filters.

Attachments (1)

multi-select-relatedfilterspec.2.diff (17.6 KB ) - added by marc@… 18 years ago.

Download all attachments as: .zip

Change History (39)

comment:1 by Malcolm Tredinnick <malcolm@…>, 18 years ago

A couple of thoughts from reading through this patch (I have not tested it very thorougly yet):

  1. Some extra changes appear to have snuck in. In particular, the change in contrib/auth/models.py does not look related. Can you check that you are only including changes relevant to this ticket in the diff?
  2. In db/models/fields/related.py, your __repr__() method shoudl really be a __str__() (since it is returning a human-readable version). The problem is that it now does not tell you what the class is, so in general it is less useful. You could create a __str__ method and use that in the place where you want it (in management.py)
  3. Probably should remove the commented code in management.py in the final version. I realise you are explaining your logic at the moment, which is good (it certainly helped me). And then the 'else' clause with the nested 'if' and a 'pass' can just become 'elif', which looks a bit easier to read.
  4. Can you explain the comment in about COUNT(*) in db/models/query.py a bit more? It doesn't seem appropriate for final code, but it would be good to know what needs to be avoided. Is it more complicated than needing to do COUNT(DISTINCT(id_column)) there?
  5. In db/models/query.py, it looks like you are adding a general option for grouping to the extra() args that can be passed to a QuerySet? Is this correct? If so, it might be worth separating that out and getting it right first (multiple grouping clauses and merging extra args with existing grouping are two areas it looks a little fragile at the moment) with tests, rather than as part of this patch.
  6. In a couple of places you have some_string.split(LISTVALUE_SEPARATOR). Isn't this going to fail badly if some of the values are strings contain commas? Or can things only be numeric ids at this point (which does not seem right, since non-numeric primary keys are permitted in Django)? Normally you need to split on commas and then work out how they would be escaped and rejoin if the tail of one fragment is the escape character(s). The escaping character can vary slightly between databases, too, from memory. Need to check how Django is doing this generically so that we can do it in a portable fashion.

Anyway, nice work. The code looks like the right idea to me. Not sure if I'll use it myself, but some people might find it useful. I have no idea if the maintainers want to accept this idea or not (might be a good idea to get an indication on django-developers before going too much further in case it's a flop).

I'd be interested in knowing if you are trying to add a generic 'grouping' section to QuerySet.extra(), because we should then find out if Adrian et al want to add it and get it completely general, if so.

comment:2 by marc@…, 18 years ago

Point 1 is also valid for 2 and 3, these changes are unrelated to the actual ticket, it is just that these snuck in while making the screenshots. I will remove the changes for contrib/auth/models.py, core/management.py and db/models/fields/related.py entirely.

Points 4 through 6 are spot-on and I'll look into that when I have a bit of time (which should be within a day or two). This patch+ticket was mainly intended to see if this idea is acceptable, and if so, if it is going in the right direction.

The thread is on django-dev as http://groups.google.com/group/django-developers/browse_thread/thread/3b209e762f09ac8f

comment:3 by marc@…, 18 years ago

Getting back to point 4, why not a COUNT(*) or COUNT(DISTINCT(*)).

The usual sql statement (simplified, leaving out the joins) looks like

  select count(*)
  from auth_user_groups
  where group_id in (2,3)

This returns one row with the correct count. Note that the in(2,3) means that there are duplicates in the resultset, so a count(distinct(*)) and later a select distinct are necessary.

Now, to implement the and/or functionality, I have modified the sql to look like

  select count(*)
  from auth_user_groups
  where group_id in (2,3)
  group by user_id
  having count(user_id)>= X

with X=1 for OR, and X=len(id_list) for AND.

By using a GROUP BY, I no longer get a single result, but I get a row for each user that satisfies the criteria. The count(*) for each row is now the number of matches for the group_id in (id_list).
For example, the AND case yields

user_idcount
2 2
4 2

And the OR case

user_idcount
3 1
2 2
4 2

Basically, the number of rows returned is once again the actual number of rows.
So, to really tackle this without fetching the rows, I could use (but I don't do that yet) a subselect, like this

  select count(*) from (
    select count(*)
    from auth_user_groups
    where group_id in (2,3)
    group by user_id
    having count(user_id)>= X
  ) as temp

That should work in most database engines (it does in Postgres), but I know older MySQL versions didn't support subselects.

comment:4 by marc@…, 18 years ago

I forgot to mention that the GROUP BY gets rid of the duplicates quite nicely :)

comment:5 by Malcolm Tredinnick, 18 years ago

Came across this again whilst going through all the patches we have outstanding. One comment on the earlier count(*) stuff: if we ever need count(distinct(...)), not that count(distinct(*)) is not supported in SQLite. You have to do count(distint(col_name)) there. Since the primary key is unique and not NULL, it acts as a good substitute for '*'.

Also are selects in the "from" clause supported everywhere (MySQL and SQLite being ther two most likely infringers here). Haven't checked this yet, just noting it as something to resolve. Also, would we be screwing over the MS SQL Server or Firebird guys at all (Oracle does support selects in the from clause, as does PostgreSQL, so no problems with those two).

comment:6 by hi-world cup, 18 years ago

Cc: hi-world cup added
Keywords: rthml tab space editor js added
Summary: [patch] multi-select admin filter for RelatedFieldshi-world cup

comment:7 by Adrian Holovaty, 18 years ago

Summary: hi-world cup[patch] multi-select admin filter for RelatedFields

comment:8 by Gary Wilson <gary.wilson@…>, 17 years ago

Cc: hi-world cup removed
Keywords: rthml tab space editor js removed
Needs documentation: set
Patch needs improvement: set
Triage Stage: UnreviewedDesign decision needed

no more screenshots :(

comment:9 by marc@…, 17 years ago

Oops, sorry, they're back now! (hadn't realized this was still under consideration while cleaning up some stuff)

comment:10 by Jacob, 16 years ago

Triage Stage: Design decision neededSomeday/Maybe

At some point, we'll be reworking the admin interface, and this will be one of the things we examine at that time.

comment:11 by sanrio, 16 years ago

Hi,
I am very interested in this feature. The patch (i.e. the diff file) works off a really old
version of django. Is it possible to update the patch to work with either django 0.96
or trunk code base? I tried to manually update the files, but query.py has changed significantly.
Thanks,

comment:12 by jakub_vysoky, 16 years ago

Keywords: nfa-changelist added

in reply to:  5 comment:13 by dan.p.burke@…, 16 years ago

Replying to mtredinnick:
SQL Server can happily do SELECT in FROM

comment:14 by Manuel Saelices, 15 years ago

Cc: msaelices@… added

comment:15 by Luis Bruno, 13 years ago

Cc: Luis Bruno added

I'm very interested in this patch's functionality.

comment:16 by Luis Bruno, 13 years ago

Owner: changed from nobody to Russell Keith-Magee

Sorry for putting this on your plate, but I think this ticket has slipped through the gaps; mind keeping an eye on it, or putting the ticket on the right guy's plate?

comment:17 by Russell Keith-Magee, 13 years ago

Owner: changed from Russell Keith-Magee to nobody

Don't assign tickets to other people. If you want someone to take a look at it, raise the issue on Django-dev, and see who takes the bait.

comment:18 by Łukasz Rekucki, 13 years ago

Severity: normalNormal
Summary: [patch] multi-select admin filter for RelatedFieldsMulti-select admin filter for RelatedFields
Type: enhancementNew feature

comment:19 by Julien Phalip, 13 years ago

UI/UX: set

comment:20 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:21 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:22 by Aymeric Augustin, 12 years ago

UI/UX: set

Revert accidental batch modification.

comment:23 by Andy Baker, 12 years ago

Cc: Andy Baker added

comment:24 by Julien Phalip, 12 years ago

Triage Stage: Someday/MaybeAccepted

comment:25 by Collin Anderson, 9 years ago

Cc: cmawebsite@… added

comment:26 by Collin Anderson, 8 years ago

I finally thought of a possible way to do this:

Use urls like ?fieldname__in=4&fieldname__in=3

We'd need to support that both in the front end (generating those urls), and in the backend (handling repeated parameters and making sure they become lists).

I recommend only supporting the OR case (or at least starting with that). It's the expected user experience on the web, and I think it would be the most useful. (AND would only be useful in some cases for many to many's.)

comment:27 by Collin Anderson, 7 years ago

Today I learned: Django admin already handles ?somefield__in=6,8,10 as you would hope, there's just no UI for it.

comment:28 by Carlton Gibson, 4 years ago

Duplicates, looking for filters submitting multiple values in #31338 and #29121.

For a query string like ?key=val1&key=val2&key=val3, `ChangeList.__init__()` drops all but the last value:

        self.params = dict(request.GET.items())

Even if we didn't ship filters using this, accepting multiple params would let users implement such filters themselves.

comment:29 by Sarah Boyce, 13 months ago

Cc: Sarah Boyce added

comment:30 by Sarah Boyce, 13 months ago

Owner: changed from nobody to Sarah Boyce
Status: newassigned

comment:31 by Sarah Boyce, 13 months ago

Needs documentation: unset
Patch needs improvement: unset
UI/UX: unset

comment:32 by Mariusz Felisiak, 13 months ago

Patch needs improvement: set

comment:33 by Sarah Boyce, 13 months ago

Patch needs improvement: unset

comment:34 by Mariusz Felisiak, 13 months ago

#27559 was a duplicate.

comment:35 by Mariusz Felisiak, 12 months ago

Triage Stage: AcceptedReady for checkin

comment:36 by Mariusz Felisiak <felisiak.mariusz@…>, 12 months ago

In 8d6f959:

Refs #1873 -- Added test for IncorrectLookupParameters when list of values is passed to RelatedFieldListFilter.

comment:37 by Mariusz Felisiak <felisiak.mariusz@…>, 12 months ago

In d03dc63:

Refs #1873 -- Used GET.lists() in admin filters.

comment:38 by Mariusz Felisiak <felisiak.mariusz@…>, 12 months ago

Resolution: fixed
Status: assignedclosed

In d2b688b9:

Fixed #1873 -- Handled multi-valued query parameters in admin changelist filters.

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