Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse code

Fixed #15819 - Fixed 1.3 regression from r15526 causing duplicate sea…

…rch results in admin with search_fields traversing to non-M2M related models. Thanks to Adam Kochanowski for the report and Ryan Kaskel for the patch.

git-svn-id: http://code.djangoproject.com/svn/django/trunk@16093 bcc190cf-cafb-0310-a4f2-bffc1f526a37
  • Loading branch information...
commit 065e6b6153a2656a9d60ca7da72236de0843e6a5 1 parent 5bbba4b
Carl Meyer authored April 23, 2011
14  django/contrib/admin/views/main.py
@@ -26,6 +26,15 @@
26 26
 # Text to display within change-list table cells if the value is blank.
27 27
 EMPTY_CHANGELIST_VALUE = ugettext_lazy('(None)')
28 28
 
  29
+def field_needs_distinct(field):
  30
+    if ((hasattr(field, 'rel') and
  31
+         isinstance(field.rel, models.ManyToManyRel)) or
  32
+        (isinstance(field, models.related.RelatedObject) and
  33
+         not field.field.unique)):
  34
+         return True
  35
+    return False
  36
+
  37
+
29 38
 class ChangeList(object):
30 39
     def __init__(self, request, model, list_display, list_display_links, list_filter, date_hierarchy, search_fields, list_select_related, list_per_page, list_editable, model_admin):
31 40
         self.model = model
@@ -189,8 +198,7 @@ def get_query_set(self):
189 198
                     f = self.lookup_opts.get_field_by_name(field_name)[0]
190 199
                 except models.FieldDoesNotExist:
191 200
                     raise IncorrectLookupParameters
192  
-                if hasattr(f, 'rel') and isinstance(f.rel, models.ManyToManyRel):
193  
-                    use_distinct = True
  201
+                use_distinct = field_needs_distinct(f)
194 202
 
195 203
             # if key ends with __in, split parameter into separate values
196 204
             if key.endswith('__in'):
@@ -264,7 +272,7 @@ def construct_search(field_name):
264 272
                 for search_spec in orm_lookups:
265 273
                     field_name = search_spec.split('__', 1)[0]
266 274
                     f = self.lookup_opts.get_field_by_name(field_name)[0]
267  
-                    if hasattr(f, 'rel') and isinstance(f.rel, models.ManyToManyRel):
  275
+                    if field_needs_distinct(f):
268 276
                         use_distinct = True
269 277
                         break
270 278
 
84  tests/regressiontests/admin_changelist/tests.py
... ...
@@ -1,6 +1,6 @@
1 1
 from django.contrib import admin
2 2
 from django.contrib.admin.options import IncorrectLookupParameters
3  
-from django.contrib.admin.views.main import ChangeList
  3
+from django.contrib.admin.views.main import ChangeList, SEARCH_VAR
4 4
 from django.core.paginator import Paginator
5 5
 from django.template import Context, Template
6 6
 from django.test import TransactionTestCase
@@ -148,12 +148,14 @@ def test_distinct_for_m2m_in_list_filter(self):
148 148
         band.genres.add(blues)
149 149
 
150 150
         m = BandAdmin(Band, admin.site)
151  
-        cl = ChangeList(MockFilteredRequestA(blues.pk), Band, m.list_display,
  151
+        request = MockFilterRequest('genres', blues.pk)
  152
+
  153
+        cl = ChangeList(request, Band, m.list_display,
152 154
                 m.list_display_links, m.list_filter, m.date_hierarchy,
153 155
                 m.search_fields, m.list_select_related, m.list_per_page,
154 156
                 m.list_editable, m)
155 157
 
156  
-        cl.get_results(MockFilteredRequestA(blues.pk))
  158
+        cl.get_results(request)
157 159
 
158 160
         # There's only one Group instance
159 161
         self.assertEqual(cl.result_count, 1)
@@ -169,12 +171,14 @@ def test_distinct_for_through_m2m_in_list_filter(self):
169 171
         Membership.objects.create(group=band, music=lead, role='bass player')
170 172
 
171 173
         m = GroupAdmin(Group, admin.site)
172  
-        cl = ChangeList(MockFilteredRequestB(lead.pk), Group, m.list_display,
  174
+        request = MockFilterRequest('members', lead.pk)
  175
+
  176
+        cl = ChangeList(request, Group, m.list_display,
173 177
                 m.list_display_links, m.list_filter, m.date_hierarchy,
174 178
                 m.search_fields, m.list_select_related, m.list_per_page,
175 179
                 m.list_editable, m)
176 180
 
177  
-        cl.get_results(MockFilteredRequestB(lead.pk))
  181
+        cl.get_results(request)
178 182
 
179 183
         # There's only one Group instance
180 184
         self.assertEqual(cl.result_count, 1)
@@ -191,12 +195,14 @@ def test_distinct_for_inherited_m2m_in_list_filter(self):
191 195
         Membership.objects.create(group=four, music=lead, role='guitar player')
192 196
 
193 197
         m = QuartetAdmin(Quartet, admin.site)
194  
-        cl = ChangeList(MockFilteredRequestB(lead.pk), Quartet, m.list_display,
  198
+        request = MockFilterRequest('members', lead.pk)
  199
+
  200
+        cl = ChangeList(request, Quartet, m.list_display,
195 201
                 m.list_display_links, m.list_filter, m.date_hierarchy,
196 202
                 m.search_fields, m.list_select_related, m.list_per_page,
197 203
                 m.list_editable, m)
198 204
 
199  
-        cl.get_results(MockFilteredRequestB(lead.pk))
  205
+        cl.get_results(request)
200 206
 
201 207
         # There's only one Quartet instance
202 208
         self.assertEqual(cl.result_count, 1)
@@ -213,16 +219,59 @@ def test_distinct_for_m2m_to_inherited_in_list_filter(self):
213 219
         Invitation.objects.create(band=three, player=lead, instrument='bass')
214 220
 
215 221
         m = ChordsBandAdmin(ChordsBand, admin.site)
216  
-        cl = ChangeList(MockFilteredRequestB(lead.pk), ChordsBand, m.list_display,
  222
+        request = MockFilterRequest('members', lead.pk)
  223
+
  224
+        cl = ChangeList(request, ChordsBand, m.list_display,
217 225
                 m.list_display_links, m.list_filter, m.date_hierarchy,
218 226
                 m.search_fields, m.list_select_related, m.list_per_page,
219 227
                 m.list_editable, m)
220 228
 
221  
-        cl.get_results(MockFilteredRequestB(lead.pk))
  229
+        cl.get_results(request)
222 230
 
223 231
         # There's only one ChordsBand instance
224 232
         self.assertEqual(cl.result_count, 1)
225 233
 
  234
+    def test_distinct_for_non_unique_related_object_in_list_filter(self):
  235
+        """
  236
+        Regressions tests for #15819: If a field listed in list_filters
  237
+        is a non-unique related object, distinct() must be called.
  238
+        """
  239
+        parent = Parent.objects.create(name='Mary')
  240
+        # Two children with the same name
  241
+        Child.objects.create(parent=parent, name='Daniel')
  242
+        Child.objects.create(parent=parent, name='Daniel')
  243
+
  244
+        m = ParentAdmin(Parent, admin.site)
  245
+        request = MockFilterRequest('child__name', 'Daniel')
  246
+
  247
+        cl = ChangeList(request, Parent, m.list_display, m.list_display_links,
  248
+                        m.list_filter, m.date_hierarchy, m.search_fields,
  249
+                        m.list_select_related, m.list_per_page,
  250
+                        m.list_editable, m)
  251
+
  252
+        # Make sure distinct() was called
  253
+        self.assertEqual(cl.query_set.count(), 1)
  254
+
  255
+    def test_distinct_for_non_unique_related_object_in_search_fields(self):
  256
+        """
  257
+        Regressions tests for #15819: If a field listed in search_fields
  258
+        is a non-unique related object, distinct() must be called.
  259
+        """
  260
+        parent = Parent.objects.create(name='Mary')
  261
+        Child.objects.create(parent=parent, name='Danielle')
  262
+        Child.objects.create(parent=parent, name='Daniel')
  263
+
  264
+        m = ParentAdmin(Parent, admin.site)
  265
+        request = MockSearchRequest('daniel')
  266
+
  267
+        cl = ChangeList(request, Parent, m.list_display, m.list_display_links,
  268
+                        m.list_filter, m.date_hierarchy, m.search_fields,
  269
+                        m.list_select_related, m.list_per_page,
  270
+                        m.list_editable, m)
  271
+
  272
+        # Make sure distinct() was called
  273
+        self.assertEqual(cl.query_set.count(), 1)
  274
+
226 275
     def test_pagination(self):
227 276
         """
228 277
         Regression tests for #12893: Pagination in admins changelist doesn't
@@ -254,6 +303,11 @@ def test_pagination(self):
254 303
         self.assertEqual(cl.paginator.page_range, [1, 2, 3])
255 304
 
256 305
 
  306
+class ParentAdmin(admin.ModelAdmin):
  307
+    list_filter = ['child__name']
  308
+    search_fields = ['child__name']
  309
+
  310
+
257 311
 class ChildAdmin(admin.ModelAdmin):
258 312
     list_display = ['name', 'parent']
259 313
     list_per_page = 10
@@ -288,10 +342,10 @@ class QuartetAdmin(admin.ModelAdmin):
288 342
 class ChordsBandAdmin(admin.ModelAdmin):
289 343
     list_filter = ['members']
290 344
 
291  
-class MockFilteredRequestA(object):
292  
-    def __init__(self, pk):
293  
-        self.GET = { 'genres' : pk }
  345
+class MockFilterRequest(object):
  346
+    def __init__(self, filter, q):
  347
+        self.GET = {filter: q}
294 348
 
295  
-class MockFilteredRequestB(object):
296  
-    def __init__(self, pk):
297  
-        self.GET = { 'members': pk }
  349
+class MockSearchRequest(object):
  350
+    def __init__(self, q):
  351
+        self.GET = {SEARCH_VAR: q}

0 notes on commit 065e6b6

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