Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse code

Fixed #13902 -- Fixed admin list_filter so it doesn't show duplicate …

…results when it includes a field spec that involves m2m relationships with an intermediate model. Thanks Iván Raskovsky for the report and patch.

git-svn-id: http://code.djangoproject.com/svn/django/trunk@15526 bcc190cf-cafb-0310-a4f2-bffc1f526a37
  • Loading branch information...
commit 337b6786fdfcc7a8b2f8eced0de6b966ab5553de 1 parent f6aa469
Ramiro Morales authored February 13, 2011
29  django/contrib/admin/views/main.py
@@ -169,6 +169,8 @@ def get_ordering(self):
169 169
         return order_field, order_type
170 170
 
171 171
     def get_query_set(self):
  172
+        use_distinct = False
  173
+
172 174
         qs = self.root_query_set
173 175
         lookup_params = self.params.copy() # a dictionary of the query string
174 176
         for i in (ALL_VAR, ORDER_VAR, ORDER_TYPE_VAR, SEARCH_VAR, IS_POPUP_VAR):
@@ -181,6 +183,17 @@ def get_query_set(self):
181 183
                 del lookup_params[key]
182 184
                 lookup_params[smart_str(key)] = value
183 185
 
  186
+            if not use_distinct:
  187
+                # Check if it's a relationship that might return more than one
  188
+                # instance
  189
+                field_name = key.split('__', 1)[0]
  190
+                try:
  191
+                    f = self.lookup_opts.get_field_by_name(field_name)[0]
  192
+                except models.FieldDoesNotExist:
  193
+                    raise IncorrectLookupParameters
  194
+                if hasattr(f, 'rel') and isinstance(f.rel, models.ManyToManyRel):
  195
+                    use_distinct = True
  196
+
184 197
             # if key ends with __in, split parameter into separate values
185 198
             if key.endswith('__in'):
186 199
                 value = value.split(',')
@@ -246,12 +259,18 @@ def construct_search(field_name):
246 259
             for bit in self.query.split():
247 260
                 or_queries = [models.Q(**{construct_search(str(field_name)): bit}) for field_name in self.search_fields]
248 261
                 qs = qs.filter(reduce(operator.or_, or_queries))
249  
-            for field_name in self.search_fields:
250  
-                if '__' in field_name:
251  
-                    qs = qs.distinct()
252  
-                    break
  262
+            if not use_distinct:
  263
+                for search_field in self.search_fields:
  264
+                    field_name = search_field.split('__', 1)[0]
  265
+                    f = self.lookup_opts.get_field_by_name(field_name)[0]
  266
+                    if hasattr(f, 'rel') and isinstance(f.rel, models.ManyToManyRel):
  267
+                        use_distinct = True
  268
+                        break
253 269
 
254  
-        return qs
  270
+        if use_distinct:
  271
+            return qs.distinct()
  272
+        else:
  273
+            return qs
255 274
 
256 275
     def url_for_result(self, result):
257 276
         return "%s/" % quote(getattr(result, self.pk_attname))
42  tests/regressiontests/admin_changelist/models.py
... ...
@@ -1,5 +1,4 @@
1 1
 from django.db import models
2  
-from django.contrib import admin
3 2
 
4 3
 class Parent(models.Model):
5 4
     name = models.CharField(max_length=128)
@@ -7,3 +6,44 @@ class Parent(models.Model):
7 6
 class Child(models.Model):
8 7
     parent = models.ForeignKey(Parent, editable=False, null=True)
9 8
     name = models.CharField(max_length=30, blank=True)
  9
+
  10
+class Genre(models.Model):
  11
+    name = models.CharField(max_length=20)
  12
+
  13
+class Band(models.Model):
  14
+    name = models.CharField(max_length=20)
  15
+    nr_of_members = models.PositiveIntegerField()
  16
+    genres = models.ManyToManyField(Genre)
  17
+
  18
+class Musician(models.Model):
  19
+    name = models.CharField(max_length=30)
  20
+
  21
+    def __unicode__(self):
  22
+        return self.name
  23
+
  24
+class Group(models.Model):
  25
+    name = models.CharField(max_length=30)
  26
+    members = models.ManyToManyField(Musician, through='Membership')
  27
+
  28
+    def __unicode__(self):
  29
+        return self.name
  30
+
  31
+class Membership(models.Model):
  32
+    music = models.ForeignKey(Musician)
  33
+    group = models.ForeignKey(Group)
  34
+    role = models.CharField(max_length=15)
  35
+
  36
+class Quartet(Group):
  37
+    pass
  38
+
  39
+class ChordsMusician(Musician):
  40
+    pass
  41
+
  42
+class ChordsBand(models.Model):
  43
+    name = models.CharField(max_length=30)
  44
+    members = models.ManyToManyField(ChordsMusician, through='Invitation')
  45
+
  46
+class Invitation(models.Model):
  47
+    player = models.ForeignKey(ChordsMusician)
  48
+    band = models.ForeignKey(ChordsBand)
  49
+    instrument = models.CharField(max_length=15)
111  tests/regressiontests/admin_changelist/tests.py
@@ -5,7 +5,8 @@
5 5
 from django.template import Context, Template
6 6
 from django.test import TransactionTestCase
7 7
 
8  
-from regressiontests.admin_changelist.models import Child, Parent
  8
+from models import (Child, Parent, Genre, Band, Musician, Group, Quartet,
  9
+    Membership, ChordsMusician, ChordsBand, Invitation)
9 10
 
10 11
 
11 12
 class ChangeListTests(TransactionTestCase):
@@ -135,18 +136,122 @@ def test_custom_paginator(self):
135 136
         cl.get_results(request)
136 137
         self.assertIsInstance(cl.paginator, CustomPaginator)
137 138
 
  139
+    def test_distinct_for_m2m_in_list_filter(self):
  140
+        """
  141
+        Regression test for #13902: When using a ManyToMany in list_filter,
  142
+        results shouldn't apper more than once. Basic ManyToMany.
  143
+        """
  144
+        blues = Genre.objects.create(name='Blues')
  145
+        band = Band.objects.create(name='B.B. King Review', nr_of_members=11)
  146
+
  147
+        band.genres.add(blues)
  148
+        band.genres.add(blues)
  149
+
  150
+        m = BandAdmin(Band, admin.site)
  151
+        cl = ChangeList(MockFilteredRequestA(), Band, m.list_display,
  152
+                m.list_display_links, m.list_filter, m.date_hierarchy,
  153
+                m.search_fields, m.list_select_related, m.list_per_page,
  154
+                m.list_editable, m)
  155
+
  156
+        cl.get_results(MockFilteredRequestA())
  157
+
  158
+        # There's only one Group instance
  159
+        self.assertEqual(cl.result_count, 1)
  160
+
  161
+    def test_distinct_for_through_m2m_in_list_filter(self):
  162
+        """
  163
+        Regression test for #13902: When using a ManyToMany in list_filter,
  164
+        results shouldn't apper more than once. With an intermediate model.
  165
+        """
  166
+        lead = Musician.objects.create(name='Vox')
  167
+        band = Group.objects.create(name='The Hype')
  168
+        Membership.objects.create(group=band, music=lead, role='lead voice')
  169
+        Membership.objects.create(group=band, music=lead, role='bass player')
  170
+
  171
+        m = GroupAdmin(Group, admin.site)
  172
+        cl = ChangeList(MockFilteredRequestB(), Group, m.list_display,
  173
+                m.list_display_links, m.list_filter, m.date_hierarchy,
  174
+                m.search_fields, m.list_select_related, m.list_per_page,
  175
+                m.list_editable, m)
  176
+
  177
+        cl.get_results(MockFilteredRequestB())
  178
+
  179
+        # There's only one Group instance
  180
+        self.assertEqual(cl.result_count, 1)
  181
+
  182
+    def test_distinct_for_inherited_m2m_in_list_filter(self):
  183
+        """
  184
+        Regression test for #13902: When using a ManyToMany in list_filter,
  185
+        results shouldn't apper more than once. Model managed in the
  186
+        admin inherits from the one that defins the relationship.
  187
+        """
  188
+        lead = Musician.objects.create(name='John')
  189
+        four = Quartet.objects.create(name='The Beatles')
  190
+        Membership.objects.create(group=four, music=lead, role='lead voice')
  191
+        Membership.objects.create(group=four, music=lead, role='guitar player')
  192
+
  193
+        m = QuartetAdmin(Quartet, admin.site)
  194
+        cl = ChangeList(MockFilteredRequestB(), Quartet, m.list_display,
  195
+                m.list_display_links, m.list_filter, m.date_hierarchy,
  196
+                m.search_fields, m.list_select_related, m.list_per_page,
  197
+                m.list_editable, m)
  198
+
  199
+        cl.get_results(MockFilteredRequestB())
  200
+
  201
+        # There's only one Quartet instance
  202
+        self.assertEqual(cl.result_count, 1)
  203
+
  204
+    def test_distinct_for_m2m_to_inherited_in_list_filter(self):
  205
+        """
  206
+        Regression test for #13902: When using a ManyToMany in list_filter,
  207
+        results shouldn't apper more than once. Target of the relationship
  208
+        inherits from another.
  209
+        """
  210
+        lead = ChordsMusician.objects.create(name='Player A')
  211
+        three = ChordsBand.objects.create(name='The Chords Trio')
  212
+        Invitation.objects.create(band=three, player=lead, instrument='guitar')
  213
+        Invitation.objects.create(band=three, player=lead, instrument='bass')
  214
+
  215
+        m = ChordsBandAdmin(ChordsBand, admin.site)
  216
+        cl = ChangeList(MockFilteredRequestB(), ChordsBand, m.list_display,
  217
+                m.list_display_links, m.list_filter, m.date_hierarchy,
  218
+                m.search_fields, m.list_select_related, m.list_per_page,
  219
+                m.list_editable, m)
  220
+
  221
+        cl.get_results(MockFilteredRequestB())
  222
+
  223
+        # There's only one ChordsBand instance
  224
+        self.assertEqual(cl.result_count, 1)
  225
+
138 226
 
139 227
 class ChildAdmin(admin.ModelAdmin):
140 228
     list_display = ['name', 'parent']
141 229
     def queryset(self, request):
142 230
         return super(ChildAdmin, self).queryset(request).select_related("parent__name")
143 231
 
144  
-
145 232
 class MockRequest(object):
146 233
     GET = {}
147 234
 
148  
-
149 235
 class CustomPaginator(Paginator):
150 236
     def __init__(self, queryset, page_size, orphans=0, allow_empty_first_page=True):
151 237
         super(CustomPaginator, self).__init__(queryset, 5, orphans=2,
152 238
             allow_empty_first_page=allow_empty_first_page)
  239
+
  240
+
  241
+class BandAdmin(admin.ModelAdmin):
  242
+    list_filter = ['genres']
  243
+
  244
+class GroupAdmin(admin.ModelAdmin):
  245
+    list_filter = ['members']
  246
+
  247
+class QuartetAdmin(admin.ModelAdmin):
  248
+    list_filter = ['members']
  249
+
  250
+class ChordsBandAdmin(admin.ModelAdmin):
  251
+    list_filter = ['members']
  252
+
  253
+class MockFilteredRequestA(object):
  254
+    GET = { 'genres': 1 }
  255
+
  256
+class MockFilteredRequestB(object):
  257
+    GET = { 'members': 1 }

0 notes on commit 337b678

Please sign in to comment.
Something went wrong with that request. Please try again.