Fixed various bugs related to having multiple columns in admin list_display with the same sort field

Thanks to julien for the report

Refs #11868

git-svn-id: http://code.djangoproject.com/svn/django/trunk@16319 bcc190cf-cafb-0310-a4f2-bffc1f526a37

django/contrib/admin/templatetags/admin_list.py

@@ -85,7 +85,7 @@ def result_headers(cl):
     # We need to know the 'ordering field' that corresponds to each
     # item in list_display, and we need other info, so do a pre-pass
     # on list_display
-    list_display_info = SortedDict()
+    ordering_field_columns = cl.get_ordering_field_columns()
     for i, field_name in enumerate(cl.list_display):
         admin_order_field = None
         text, attr = label_for_field(field_name, cl.model,
@@ -93,36 +93,20 @@ def result_headers(cl):
             return_attr = True
         )
         if attr:
-            admin_order_field = getattr(attr, "admin_order_field", None)
-        if admin_order_field is None:
-            ordering_field_name = field_name
-        else:
-            ordering_field_name = admin_order_field
-        list_display_info[ordering_field_name] = dict(text=text,
-                                                      attr=attr,
-                                                      index=i,
-                                                      admin_order_field=admin_order_field,
-                                                      field_name=field_name)
-
-    del admin_order_field, text, attr
-
-    ordering_fields = cl.get_ordering_fields()
-
-    for ordering_field_name, info in list_display_info.items():
-        if info['attr']:
             # Potentially not sortable
 
             # if the field is the action checkbox: no sorting and special class
-            if info['field_name'] == 'action_checkbox':
+            if field_name == 'action_checkbox':
                 yield {
-                    "text": info['text'],
+                    "text": text,
                     "class_attrib": mark_safe(' class="action-checkbox-column"')
                 }
                 continue
 
-            if not info['admin_order_field']:
+            admin_order_field = getattr(attr, "admin_order_field", None)
+            if not admin_order_field:
                 # Not sortable
-                yield {"text": info['text']}
+                yield {"text": text}
                 continue
 
         # OK, it is sortable if we got this far
@@ -131,9 +115,9 @@ def result_headers(cl):
         new_order_type = 'asc'
         sort_pos = 0
         # Is it currently being sorted on?
-        if ordering_field_name in ordering_fields:
+        if i in ordering_field_columns:
-            order_type = ordering_fields.get(ordering_field_name).lower()
+            order_type = ordering_field_columns.get(i).lower()
-            sort_pos = ordering_fields.keys().index(ordering_field_name) + 1
+            sort_pos = ordering_field_columns.keys().index(i) + 1
             th_classes.append('sorted %sending' % order_type)
             new_order_type = {'asc': 'desc', 'desc': 'asc'}[order_type]
 
@@ -141,27 +125,21 @@ def result_headers(cl):
         o_list = []
         make_qs_param = lambda t, n: ('-' if t == 'desc' else '') + str(n)
 
-        for f, ot in ordering_fields.items():
+        for j, ot in ordering_field_columns.items():
-            try:
+            if j == i: # Same column
-                colnum = list_display_info[f]['index']
-            except KeyError:
-                continue
-
-            if f == ordering_field_name:
                 # We want clicking on this header to bring the ordering to the
                 # front
-                o_list.insert(0, make_qs_param(new_order_type, colnum))
+                o_list.insert(0, make_qs_param(new_order_type, j))
             else:
-                o_list.append(make_qs_param(ot, colnum))
+                o_list.append(make_qs_param(ot, j))
 
-        if ordering_field_name not in ordering_fields:
+        if i not in ordering_field_columns:
-            colnum = list_display_info[ordering_field_name]['index']
+            o_list.insert(0, make_qs_param(new_order_type, i))
-            o_list.insert(0, make_qs_param(new_order_type, colnum))
 
         o_list = '.'.join(o_list)
 
         yield {
-            "text": info['text'],
+            "text": text,
             "sortable": True,
             "ascending": order_type == "asc",
             "sort_pos": sort_pos,

django/contrib/admin/views/main.py

@@ -164,16 +164,20 @@ class ChangeList(object):
         self.multi_page = multi_page
         self.paginator = paginator
 
-    def get_ordering(self):
+    def _get_default_ordering(self):
-        lookup_opts, params = self.lookup_opts, self.params
-        # For ordering, first check the "ordering" parameter in the admin
-        # options, then check the object's default ordering. Finally, a
-        # manually-specified ordering from the query string overrides anything.
         ordering = []
         if self.model_admin.ordering:
             ordering = self.model_admin.ordering
-        elif lookup_opts.ordering:
+        elif self.lookup_opts.ordering:
-            ordering = lookup_opts.ordering
+            ordering = self.lookup_opts.ordering
+        return ordering
+
+    def get_ordering(self):
+        params = self.params
+        # For ordering, first check the "ordering" parameter in the admin
+        # options, then check the object's default ordering. Finally, a
+        # manually-specified ordering from the query string overrides anything.
+        ordering = self._get_default_ordering()
 
         if ORDER_VAR in params:
             # Clear ordering and used params
@@ -184,7 +188,7 @@ class ChangeList(object):
                     none, pfx, idx = p.rpartition('-')
                     field_name = self.list_display[int(idx)]
                     try:
-                        f = lookup_opts.get_field(field_name)
+                        f = self.lookup_opts.get_field(field_name)
                     except models.FieldDoesNotExist:
                         # See whether field_name is a name of a non-field
                         # that allows sorting.
@@ -208,12 +212,44 @@ class ChangeList(object):
 
         return ordering
 
-    def get_ordering_fields(self):
+    def get_ordering_field_columns(self):
-        # Returns a SortedDict of ordering fields and asc/desc
+        # Returns a SortedDict of ordering field column numbers and asc/desc
+
+        # We must cope with more than one column having the same underlying sort
+        # field, so we base things on column numbers.
+        ordering = self._get_default_ordering()
         ordering_fields = SortedDict()
-        for o in self.ordering:
+        if ORDER_VAR not in self.params:
-            none, t, f = o.rpartition('-')
+            # for ordering specified on ModelAdmin or model Meta, we don't know
-            ordering_fields[f] = 'desc' if t == '-' else 'asc'
+            # the right column numbers absolutely, because there might be more
+            # than one column associated with that ordering, so we guess.
+            for f in ordering:
+                if f.startswith('-'):
+                    f = f[1:]
+                    order_type = 'desc'
+                else:
+                    order_type = 'asc'
+                i = None
+                try:
+                    # Search for simply field name first
+                    i = self.list_display.index(f)
+                except ValueError:
+                    # No match, but their might be a match if we take into account
+                    # 'admin_order_field'
+                    for j, attr in enumerate(self.list_display):
+                        if getattr(attr, 'admin_order_field', '') == f:
+                            i = j
+                            break
+                if i is not None:
+                    ordering_fields[i] = order_type
+        else:
+            for p in self.params[ORDER_VAR].split('.'):
+                none, pfx, idx = p.rpartition('-')
+                try:
+                    idx = int(idx)
+                except ValueError:
+                    continue # skip it
+                ordering_fields[idx] = 'desc' if pfx == '-' else 'asc'
         return ordering_fields
 
     def get_lookup_params(self, use_distinct=False):

tests/regressiontests/admin_views/models.py

@@ -811,6 +811,20 @@ class OtherStoryAdmin(admin.ModelAdmin):
     list_editable = ('content', )
     ordering = ["-pk"]
 
+class ComplexSortedPerson(models.Model):
+    name = models.CharField(max_length=100)
+    age = models.PositiveIntegerField()
+    is_employee = models.NullBooleanField()
+
+class ComplexSortedPersonAdmin(admin.ModelAdmin):
+    list_display = ('name', 'age', 'is_employee', 'colored_name')
+    ordering = ('name',)
+
+    def colored_name(self, obj):
+        return '<span style="color: #%s;">%s</span>' % ('ff00ff', obj.name)
+    colored_name.allow_tags = True
+    colored_name.admin_order_field = 'name'
+
 admin.site.register(Article, ArticleAdmin)
 admin.site.register(CustomArticle, CustomArticleAdmin)
 admin.site.register(Section, save_as=True, inlines=[ArticleInline])
@@ -872,3 +886,4 @@ admin.site.register(Album, AlbumAdmin)
 admin.site.register(Question)
 admin.site.register(Answer)
 admin.site.register(PrePopulatedPost, PrePopulatedPostAdmin)
+admin.site.register(ComplexSortedPerson, ComplexSortedPersonAdmin)

tests/regressiontests/admin_views/tests.py

@@ -37,7 +37,8 @@ from models import (Article, BarAccount, CustomArticle, EmptyModel,
     Language, Collector, Widget, Grommet, DooHickey, FancyDoodad, Whatsit,
     Category, Post, Plot, FunkyTag, Chapter, Book, Promo, WorkHour, Employee,
     Question, Answer, Inquisition, Actor, FoodDelivery,
-    RowLevelChangePermissionModel, Paper, CoverLetter, Story, OtherStory)
+    RowLevelChangePermissionModel, Paper, CoverLetter, Story, OtherStory,
+    ComplexSortedPerson)
 
 
 class AdminViewBasicTest(TestCase):
@@ -313,6 +314,42 @@ class AdminViewBasicTest(TestCase):
             response.content.index(link % p1.pk) < response.content.index(link % p2.pk)
         )
 
+    def testMultipleSortSameField(self):
+        # Check that we get the columns we expect if we have two columns
+        # that correspond to the same ordering field
+        dt = datetime.datetime.now()
+        p1 = Podcast.objects.create(name="A", release_date=dt)
+        p2 = Podcast.objects.create(name="B", release_date=dt - datetime.timedelta(10))
+
+        link = '<a href="%s/'
+        response = self.client.get('/test_admin/admin/admin_views/podcast/', {})
+        self.assertEqual(response.status_code, 200)
+        self.assertTrue(
+            response.content.index(link % p1.pk) < response.content.index(link % p2.pk)
+        )
+
+        p1 = ComplexSortedPerson.objects.create(name="Bob", age=10)
+        p2 = ComplexSortedPerson.objects.create(name="Amy", age=20)
+        link = '<a href="%s/'
+
+        response = self.client.get('/test_admin/admin/admin_views/complexsortedperson/', {})
+        self.assertEqual(response.status_code, 200)
+        # Should have 5 columns (including action checkbox col)
+        self.assertContains(response, '<th scope="col"', count=5)
+
+        self.assertContains(response, 'Name')
+        self.assertContains(response, 'Colored name')
+
+        # Check order
+        self.assertTrue(
+            response.content.index('Name') < response.content.index('Colored name')
+        )
+
+        # Check sorting - should be by name
+        self.assertTrue(
+            response.content.index(link % p2.id) < response.content.index(link % p1.id)
+        )
+
     def testLimitedFilter(self):
         """Ensure admin changelist filters do not contain objects excluded via limit_choices_to.
         This also tests relation-spanning filters (e.g. 'color__value').