From 337b6786fdfcc7a8b2f8eced0de6b966ab5553de Mon Sep 17 00:00:00 2001 From: Ramiro Morales Date: Sun, 13 Feb 2011 22:51:40 +0000 Subject: [PATCH] =?UTF-8?q?Fixed=20#13902=20--=20Fixed=20admin=20list=5Ffi?= =?UTF-8?q?lter=20so=20it=20doesn't=20show=20duplicate=20results=20when=20?= =?UTF-8?q?it=20includes=20a=20field=20spec=20that=20involves=20m2m=20rela?= =?UTF-8?q?tionships=20with=20an=20intermediate=20model.=20Thanks=20Iv?= =?UTF-8?q?=C3=A1n=20Raskovsky=20for=20the=20report=20and=20patch.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit git-svn-id: http://code.djangoproject.com/svn/django/trunk@15526 bcc190cf-cafb-0310-a4f2-bffc1f526a37 --- django/contrib/admin/views/main.py | 29 ++++- .../admin_changelist/models.py | 42 ++++++- .../regressiontests/admin_changelist/tests.py | 111 +++++++++++++++++- 3 files changed, 173 insertions(+), 9 deletions(-) diff --git a/django/contrib/admin/views/main.py b/django/contrib/admin/views/main.py index 00ab9fed211..d0f03504074 100644 --- a/django/contrib/admin/views/main.py +++ b/django/contrib/admin/views/main.py @@ -169,6 +169,8 @@ class ChangeList(object): return order_field, order_type def get_query_set(self): + use_distinct = False + 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): @@ -181,6 +183,17 @@ class ChangeList(object): del lookup_params[key] lookup_params[smart_str(key)] = value + if not use_distinct: + # Check if it's a relationship that might return more than one + # instance + field_name = key.split('__', 1)[0] + try: + f = self.lookup_opts.get_field_by_name(field_name)[0] + except models.FieldDoesNotExist: + raise IncorrectLookupParameters + if hasattr(f, 'rel') and isinstance(f.rel, models.ManyToManyRel): + use_distinct = True + # if key ends with __in, split parameter into separate values if key.endswith('__in'): value = value.split(',') @@ -246,12 +259,18 @@ class ChangeList(object): for bit in self.query.split(): or_queries = [models.Q(**{construct_search(str(field_name)): bit}) for field_name in self.search_fields] qs = qs.filter(reduce(operator.or_, or_queries)) - for field_name in self.search_fields: - if '__' in field_name: - qs = qs.distinct() - break + if not use_distinct: + for search_field in self.search_fields: + field_name = search_field.split('__', 1)[0] + f = self.lookup_opts.get_field_by_name(field_name)[0] + if hasattr(f, 'rel') and isinstance(f.rel, models.ManyToManyRel): + use_distinct = True + break - return qs + if use_distinct: + return qs.distinct() + else: + return qs def url_for_result(self, result): return "%s/" % quote(getattr(result, self.pk_attname)) diff --git a/tests/regressiontests/admin_changelist/models.py b/tests/regressiontests/admin_changelist/models.py index 858d6dfd458..509d37cc8c0 100644 --- a/tests/regressiontests/admin_changelist/models.py +++ b/tests/regressiontests/admin_changelist/models.py @@ -1,5 +1,4 @@ from django.db import models -from django.contrib import admin class Parent(models.Model): name = models.CharField(max_length=128) @@ -7,3 +6,44 @@ class Parent(models.Model): class Child(models.Model): parent = models.ForeignKey(Parent, editable=False, null=True) name = models.CharField(max_length=30, blank=True) + +class Genre(models.Model): + name = models.CharField(max_length=20) + +class Band(models.Model): + name = models.CharField(max_length=20) + nr_of_members = models.PositiveIntegerField() + genres = models.ManyToManyField(Genre) + +class Musician(models.Model): + name = models.CharField(max_length=30) + + def __unicode__(self): + return self.name + +class Group(models.Model): + name = models.CharField(max_length=30) + members = models.ManyToManyField(Musician, through='Membership') + + def __unicode__(self): + return self.name + +class Membership(models.Model): + music = models.ForeignKey(Musician) + group = models.ForeignKey(Group) + role = models.CharField(max_length=15) + +class Quartet(Group): + pass + +class ChordsMusician(Musician): + pass + +class ChordsBand(models.Model): + name = models.CharField(max_length=30) + members = models.ManyToManyField(ChordsMusician, through='Invitation') + +class Invitation(models.Model): + player = models.ForeignKey(ChordsMusician) + band = models.ForeignKey(ChordsBand) + instrument = models.CharField(max_length=15) diff --git a/tests/regressiontests/admin_changelist/tests.py b/tests/regressiontests/admin_changelist/tests.py index c3f6186770c..87ffd6731fb 100644 --- a/tests/regressiontests/admin_changelist/tests.py +++ b/tests/regressiontests/admin_changelist/tests.py @@ -5,7 +5,8 @@ from django.core.paginator import Paginator from django.template import Context, Template from django.test import TransactionTestCase -from regressiontests.admin_changelist.models import Child, Parent +from models import (Child, Parent, Genre, Band, Musician, Group, Quartet, + Membership, ChordsMusician, ChordsBand, Invitation) class ChangeListTests(TransactionTestCase): @@ -135,18 +136,122 @@ class ChangeListTests(TransactionTestCase): cl.get_results(request) self.assertIsInstance(cl.paginator, CustomPaginator) + def test_distinct_for_m2m_in_list_filter(self): + """ + Regression test for #13902: When using a ManyToMany in list_filter, + results shouldn't apper more than once. Basic ManyToMany. + """ + blues = Genre.objects.create(name='Blues') + band = Band.objects.create(name='B.B. King Review', nr_of_members=11) + + band.genres.add(blues) + band.genres.add(blues) + + m = BandAdmin(Band, admin.site) + cl = ChangeList(MockFilteredRequestA(), Band, m.list_display, + m.list_display_links, m.list_filter, m.date_hierarchy, + m.search_fields, m.list_select_related, m.list_per_page, + m.list_editable, m) + + cl.get_results(MockFilteredRequestA()) + + # There's only one Group instance + self.assertEqual(cl.result_count, 1) + + def test_distinct_for_through_m2m_in_list_filter(self): + """ + Regression test for #13902: When using a ManyToMany in list_filter, + results shouldn't apper more than once. With an intermediate model. + """ + lead = Musician.objects.create(name='Vox') + band = Group.objects.create(name='The Hype') + Membership.objects.create(group=band, music=lead, role='lead voice') + Membership.objects.create(group=band, music=lead, role='bass player') + + m = GroupAdmin(Group, admin.site) + cl = ChangeList(MockFilteredRequestB(), Group, m.list_display, + m.list_display_links, m.list_filter, m.date_hierarchy, + m.search_fields, m.list_select_related, m.list_per_page, + m.list_editable, m) + + cl.get_results(MockFilteredRequestB()) + + # There's only one Group instance + self.assertEqual(cl.result_count, 1) + + def test_distinct_for_inherited_m2m_in_list_filter(self): + """ + Regression test for #13902: When using a ManyToMany in list_filter, + results shouldn't apper more than once. Model managed in the + admin inherits from the one that defins the relationship. + """ + lead = Musician.objects.create(name='John') + four = Quartet.objects.create(name='The Beatles') + Membership.objects.create(group=four, music=lead, role='lead voice') + Membership.objects.create(group=four, music=lead, role='guitar player') + + m = QuartetAdmin(Quartet, admin.site) + cl = ChangeList(MockFilteredRequestB(), Quartet, m.list_display, + m.list_display_links, m.list_filter, m.date_hierarchy, + m.search_fields, m.list_select_related, m.list_per_page, + m.list_editable, m) + + cl.get_results(MockFilteredRequestB()) + + # There's only one Quartet instance + self.assertEqual(cl.result_count, 1) + + def test_distinct_for_m2m_to_inherited_in_list_filter(self): + """ + Regression test for #13902: When using a ManyToMany in list_filter, + results shouldn't apper more than once. Target of the relationship + inherits from another. + """ + lead = ChordsMusician.objects.create(name='Player A') + three = ChordsBand.objects.create(name='The Chords Trio') + Invitation.objects.create(band=three, player=lead, instrument='guitar') + Invitation.objects.create(band=three, player=lead, instrument='bass') + + m = ChordsBandAdmin(ChordsBand, admin.site) + cl = ChangeList(MockFilteredRequestB(), ChordsBand, m.list_display, + m.list_display_links, m.list_filter, m.date_hierarchy, + m.search_fields, m.list_select_related, m.list_per_page, + m.list_editable, m) + + cl.get_results(MockFilteredRequestB()) + + # There's only one ChordsBand instance + self.assertEqual(cl.result_count, 1) + class ChildAdmin(admin.ModelAdmin): list_display = ['name', 'parent'] def queryset(self, request): return super(ChildAdmin, self).queryset(request).select_related("parent__name") - class MockRequest(object): GET = {} - class CustomPaginator(Paginator): def __init__(self, queryset, page_size, orphans=0, allow_empty_first_page=True): super(CustomPaginator, self).__init__(queryset, 5, orphans=2, allow_empty_first_page=allow_empty_first_page) + + +class BandAdmin(admin.ModelAdmin): + list_filter = ['genres'] + +class GroupAdmin(admin.ModelAdmin): + list_filter = ['members'] + +class QuartetAdmin(admin.ModelAdmin): + list_filter = ['members'] + +class ChordsBandAdmin(admin.ModelAdmin): + list_filter = ['members'] + +class MockFilteredRequestA(object): + GET = { 'genres': 1 } + +class MockFilteredRequestB(object): + GET = { 'members': 1 }