Fixed #11868 - Multiple sort in admin changelist.

Many thanks to bendavis78 for the initial patch, and for input from others.

Also fixed #7309. If people were relying on the undocumented default ordering
applied by the admin before, they will need to add 'ordering = ["-pk"]' to
their ModelAdmin.

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

django/contrib/admin/media/css/base.css

@@ -326,6 +326,34 @@ table thead th.descending a {
     background: url(../img/admin/arrow-down.gif) right .4em no-repeat;
 }
 
+table thead th.sorted a span.text {
+   display: block;
+   float: left;
+}
+
+table thead th.sorted a span.sortpos {
+   display: block;
+   float: right;
+   font-size: .6em;
+}
+
+table thead th.sorted a img {
+   vertical-align: bottom;
+}
+
+table thead th.sorted a span.clear {
+   display: block;
+   clear: both;
+}
+
+#sorting-popup-div {
+    position: absolute;
+    background-color: white;
+    border: 1px solid #ddd;
+    z-index: 2000; /* more than filters on right */
+    padding-right: 10px;
+}
+
 /* ORDERABLE TABLES */
 
 table.orderable tbody tr td:hover {

django/contrib/admin/media/css/rtl.css

@@ -91,6 +91,14 @@ table thead th.descending a {
     background-position: left;
 }
 
+table thead th.sorted a span.text {
+   float: right;
+}
+
+table thead th.sorted a span.sortpos {
+   float: left;
+}
+
 /* dashboard styles */
 
 .dashboard .module table td a {

django/contrib/admin/templates/admin/change_list_results.html

@@ -1,3 +1,5 @@
+{% load adminmedia %}
+{% load i18n %}
 {% if result_hidden_fields %}
 <div class="hiddenfields">{# DIV for HTML validation #}
 {% for item in result_hidden_fields %}{{ item }}{% endfor %}
@@ -8,10 +10,18 @@
 <table id="result_list">
 <thead>
 <tr>
-{% for header in result_headers %}<th scope="col"{{ header.class_attrib }}>
-{% if header.sortable %}<a href="{{ header.url }}">{% endif %}
-{{ header.text|capfirst }}
-{% if header.sortable %}</a>{% endif %}</th>{% endfor %}
+{% for header in result_headers %}
+<th scope="col" {{ header.class_attrib }}>
+  {% if header.sortable %}<a href="{{ header.url }}">{% endif %}
+  <span class="text">{{ header.text|capfirst }}</span>
+  {% if header.sortable %}
+    {% if header.sort_pos > 0 %}<span class="sortpos">
+      {% if header.sort_pos == 1 %}<img id="primary-sort-icon" src="{% admin_media_prefix %}img/admin/icon_cog.gif" alt="" />&nbsp;{% endif %}
+      {{ header.sort_pos }}</span>
+    {% endif %}
+    <span class="clear"></span></a>
+  {% endif %}
+</th>{% endfor %}
 </tr>
 </thead>
 <tbody>
@@ -24,4 +34,53 @@
 </tbody>
 </table>
 </div>
+
+{# Sorting popup: #}
+<div style="display: none;" id="sorting-popup-div">
+<p>{% trans "Sorting by:" %}</p>
+<ol>
+{% for header in result_headers|dictsort:"sort_pos" %}
+  {% if header.sort_pos > 0 %}
+    {% if header.ascending %}
+      <li>{% blocktrans with fieldname=header.text %}{{ fieldname }} (ascending){% endblocktrans %}</li>
+    {% else %}
+      <li>{% blocktrans with fieldname=header.text %}{{ fieldname }} (descending){% endblocktrans %}</li>
+    {% endif %}
+  {% endif %}
+{% endfor %}
+</ol>
+<p><a href="{{ reset_sorting_url }}">{% trans "Reset sorting" %}</a></p>
+</div>
+<script type="text/javascript">
+<!--
+(function($) {
+    $(document).ready(function() {
+        var popup = $('#sorting-popup-div');
+        /* These next lines seems necessary to prime the popup: */
+        popup.offset({left:-1000, top:0});
+        popup.show();
+        var popupWidth = popup.width();
+        popup.hide();
+
+        $('#primary-sort-icon').toggle(function(ev) {
+                                          ev.preventDefault();
+                                          var img = $(this);
+                                          var pos = img.offset();
+                                          pos.top += img.height();
+                                          if (pos.left + popupWidth >
+                                              $(window).width()) {
+                                              pos.left -= popupWidth;
+                                          }
+                                          popup.show();
+                                          popup.offset(pos);
+                                      },
+                                      function(ev) {
+                                          ev.preventDefault();
+                                          popup.hide();
+                                      });
+    });
+})(django.jQuery);
+//-->
+</script>
+
 {% endif %}

django/contrib/admin/templatetags/admin_list.py

@@ -7,6 +7,7 @@ from django.contrib.admin.views.main import (ALL_VAR, EMPTY_CHANGELIST_VALUE,
 from django.core.exceptions import ObjectDoesNotExist
 from django.db import models
 from django.utils import formats
+from django.utils.datastructures import SortedDict
 from django.utils.html import escape, conditional_escape
 from django.utils.safestring import mark_safe
 from django.utils.text import capfirst
@@ -81,43 +82,90 @@ def result_headers(cl):
     """
     Generates the list column headers.
     """
-    lookup_opts = cl.lookup_opts
-
+    # 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()
     for i, field_name in enumerate(cl.list_display):
-        header, attr = label_for_field(field_name, cl.model,
+        admin_order_field = None
+        text, attr = label_for_field(field_name, cl.model,
             model_admin = cl.model_admin,
             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 field_name == 'action_checkbox':
+            if info['field_name'] == 'action_checkbox':
                 yield {
-                    "text": header,
+                    "text": info['text'],
                     "class_attrib": mark_safe(' class="action-checkbox-column"')
                 }
                 continue
 
-            # It is a non-field, but perhaps one that is sortable
-            admin_order_field = getattr(attr, "admin_order_field", None)
-            if not admin_order_field:
-                yield {"text": header}
+            if not info['admin_order_field']:
+                # Not sortable
+                yield {"text": info['text']}
                 continue
 
-            # So this _is_ a sortable non-field.  Go to the yield
-            # after the else clause.
-        else:
-            admin_order_field = None
-
+        # OK, it is sortable if we got this far
         th_classes = []
+        order_type = ''
         new_order_type = 'asc'
-        if field_name == cl.order_field or admin_order_field == cl.order_field:
-            th_classes.append('sorted %sending' % cl.order_type.lower())
-            new_order_type = {'asc': 'desc', 'desc': 'asc'}[cl.order_type.lower()]
+        sort_pos = 0
+        # Is it currently being sorted on?
+        if ordering_field_name in ordering_fields:
+            order_type = ordering_fields.get(ordering_field_name).lower()
+            sort_pos = ordering_fields.keys().index(ordering_field_name) + 1
+            th_classes.append('sorted %sending' % order_type)
+            new_order_type = {'asc': 'desc', 'desc': 'asc'}[order_type]
+
+        # build new ordering param
+        o_list = []
+        make_qs_param = lambda t, n: ('-' if t == 'desc' else '') + str(n)
+
+        for f, ot in ordering_fields.items():
+            try:
+                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))
+            else:
+                o_list.append(make_qs_param(ot, colnum))
+
+        if ordering_field_name not in ordering_fields:
+            colnum = list_display_info[ordering_field_name]['index']
+            o_list.insert(0, make_qs_param(new_order_type, colnum))
+
+        o_list = '.'.join(o_list)
 
         yield {
-            "text": header,
+            "text": info['text'],
             "sortable": True,
-            "url": cl.get_query_string({ORDER_VAR: i, ORDER_TYPE_VAR: new_order_type}),
+            "ascending": order_type == "asc",
+            "sort_pos": sort_pos,
+            "url": cl.get_query_string({ORDER_VAR: o_list}),
             "class_attrib": mark_safe(th_classes and ' class="%s"' % ' '.join(th_classes) or '')
         }
 
@@ -228,9 +276,14 @@ def result_list(cl):
     """
     Displays the headers and data list together
     """
+    headers = list(result_headers(cl))
+    for h in headers:
+        # Sorting in templates depends on sort_pos attribute
+        h.setdefault('sort_pos', 0)
     return {'cl': cl,
             'result_hidden_fields': list(result_hidden_fields(cl)),
-            'result_headers': list(result_headers(cl)),
+            'result_headers': headers,
+            'reset_sorting_url': cl.get_query_string(remove=[ORDER_VAR]),
             'results': list(results(cl))}
 
 @register.inclusion_tag('admin/date_hierarchy.html')

django/contrib/admin/views/main.py

@@ -3,6 +3,7 @@ import operator
 from django.core.exceptions import SuspiciousOperation
 from django.core.paginator import InvalidPage
 from django.db import models
+from django.utils.datastructures import SortedDict
 from django.utils.encoding import force_unicode, smart_str
 from django.utils.translation import ugettext, ugettext_lazy
 from django.utils.http import urlencode
@@ -75,7 +76,7 @@ class ChangeList(object):
             self.list_editable = ()
         else:
             self.list_editable = list_editable
-        self.order_field, self.order_type = self.get_ordering()
+        self.ordering = self.get_ordering()
         self.query = request.GET.get(SEARCH_VAR, '')
         self.query_set = self.get_query_set(request)
         self.get_results(request)
@@ -166,40 +167,54 @@ class ChangeList(object):
     def get_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. If neither of
-        # those exist, order descending by ID by default. Finally, look for
-        # manually-specified ordering from the query string.
-        ordering = self.model_admin.ordering or lookup_opts.ordering or ['-' + lookup_opts.pk.name]
+        # 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:
+            ordering = lookup_opts.ordering
 
-        if ordering[0].startswith('-'):
-            order_field, order_type = ordering[0][1:], 'desc'
-        else:
-            order_field, order_type = ordering[0], 'asc'
         if ORDER_VAR in params:
-            try:
-                field_name = self.list_display[int(params[ORDER_VAR])]
+            # Clear ordering and used params
+            ordering = []
+            order_params = params[ORDER_VAR].split('.')
+            for p in order_params:
                 try:
-                    f = lookup_opts.get_field(field_name)
-                except models.FieldDoesNotExist:
-                    # See whether field_name is a name of a non-field
-                    # that allows sorting.
+                    none, pfx, idx = p.rpartition('-')
+                    field_name = self.list_display[int(idx)]
                     try:
-                        if callable(field_name):
-                            attr = field_name
-                        elif hasattr(self.model_admin, field_name):
-                            attr = getattr(self.model_admin, field_name)
-                        else:
-                            attr = getattr(self.model, field_name)
-                        order_field = attr.admin_order_field
-                    except AttributeError:
-                        pass
-                else:
-                    order_field = f.name
-            except (IndexError, ValueError):
-                pass # Invalid ordering specified. Just use the default.
-        if ORDER_TYPE_VAR in params and params[ORDER_TYPE_VAR] in ('asc', 'desc'):
-            order_type = params[ORDER_TYPE_VAR]
-        return order_field, order_type
+                        f = lookup_opts.get_field(field_name)
+                    except models.FieldDoesNotExist:
+                        # See whether field_name is a name of a non-field
+                        # that allows sorting.
+                        try:
+                            if callable(field_name):
+                                attr = field_name
+                            elif hasattr(self.model_admin, field_name):
+                                attr = getattr(self.model_admin, field_name)
+                            else:
+                                attr = getattr(self.model, field_name)
+                            field_name = attr.admin_order_field
+                        except AttributeError:
+                            continue # No 'admin_order_field', skip it
+                    else:
+                        field_name = f.name
+
+                    ordering.append(pfx + field_name)
+
+                except (IndexError, ValueError):
+                    continue # Invalid ordering specified, skip it.
+
+        return ordering
+
+    def get_ordering_fields(self):
+        # Returns a SortedDict of ordering fields and asc/desc
+        ordering_fields = SortedDict()
+        for o in self.ordering:
+            none, t, f = o.rpartition('-')
+            ordering_fields[f] = 'desc' if t == '-' else 'asc'
+        return ordering_fields
 
     def get_lookup_params(self, use_distinct=False):
         lookup_params = self.params.copy() # a dictionary of the query string
@@ -290,8 +305,8 @@ class ChangeList(object):
                             break
 
         # Set ordering.
-        if self.order_field:
-            qs = qs.order_by('%s%s' % ((self.order_type == 'desc' and '-' or ''), self.order_field))
+        if self.ordering:
+            qs = qs.order_by(*self.ordering)
 
         # Apply keyword searches.
         def construct_search(field_name):

docs/ref/contrib/admin/index.txt

@@ -696,10 +696,10 @@ subclass::
     If this isn't provided, the Django admin will use the model's default
     ordering.
 
-    .. admonition:: Note
+    .. versionchanged:: 1.4
 
-        Django will only honor the first element in the list/tuple; any others
-        will be ignored.
+    Django honors all elements in the list/tuple; before 1.4, only the first
+    was respected.
 
 .. attribute:: ModelAdmin.paginator
 

docs/releases/1.4.txt

@@ -46,6 +46,14 @@ not custom filters. This has been rectified with a simple API previously
 known as "FilterSpec" which was used internally. For more details, see the
 documentation for :attr:`~django.contrib.admin.ModelAdmin.list_filter`.
 
+Multiple sort in admin interface
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The admin change list now supports sorting on multiple columns. It respects all
+elements of the :attr:`~django.contrib.admin.ModelAdmin.ordering` attribute, and
+sorting on multiple columns by clicking on headers is designed to work similarly
+to how desktop GUIs do it.
+
 Tools for cryptographic signing
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 

tests/regressiontests/admin_filters/tests.py

@@ -67,11 +67,11 @@ class CustomUserAdmin(UserAdmin):
 
 class BookAdmin(ModelAdmin):
     list_filter = ('year', 'author', 'contributors', 'is_best_seller', 'date_registered', 'no')
-    order_by = '-id'
+    ordering = ('-id',)
 
 class DecadeFilterBookAdmin(ModelAdmin):
     list_filter = ('author', DecadeListFilterWithTitleAndParameter)
-    order_by = '-id'
+    ordering = ('-id',)
 
 class DecadeFilterBookAdminWithoutTitle(ModelAdmin):
     list_filter = (DecadeListFilterWithoutTitle,)

tests/regressiontests/admin_views/models.py

@@ -243,9 +243,6 @@ class Person(models.Model):
     def __unicode__(self):
         return self.name
 
-    class Meta:
-        ordering = ["id"]
-
 class BasePersonModelFormSet(BaseModelFormSet):
     def clean(self):
         for person_dict in self.cleaned_data:
@@ -259,13 +256,17 @@ class PersonAdmin(admin.ModelAdmin):
     list_editable = ('gender', 'alive')
     list_filter = ('gender',)
     search_fields = ('^name',)
-    ordering = ["id"]
     save_as = True
 
     def get_changelist_formset(self, request, **kwargs):
         return super(PersonAdmin, self).get_changelist_formset(request,
             formset=BasePersonModelFormSet, **kwargs)
 
+    def queryset(self, request):
+        # Order by a field that isn't in list display, to be able to test
+        # whether ordering is preserved.
+        return super(PersonAdmin, self).queryset(request).order_by('age')
+
 
 class Persona(models.Model):
     """
@@ -357,6 +358,9 @@ class Media(models.Model):
 class Podcast(Media):
     release_date = models.DateField()
 
+    class Meta:
+        ordering = ('release_date',) # overridden in PodcastAdmin
+
 class PodcastAdmin(admin.ModelAdmin):
     list_display = ('name', 'release_date')
     list_editable = ('release_date',)
@@ -795,6 +799,7 @@ class StoryAdmin(admin.ModelAdmin):
     list_display_links = ('title',) # 'id' not in list_display_links
     list_editable = ('content', )
     form = StoryForm
+    ordering = ["-pk"]
 
 class OtherStory(models.Model):
     title = models.CharField(max_length=100)
@@ -804,6 +809,7 @@ class OtherStoryAdmin(admin.ModelAdmin):
     list_display = ('id', 'title', 'content')
     list_display_links = ('title', 'id') # 'id' in list_display_links
     list_editable = ('content', )
+    ordering = ["-pk"]
 
 admin.site.register(Article, ArticleAdmin)
 admin.site.register(CustomArticle, CustomArticleAdmin)

tests/regressiontests/admin_views/tests.py

@@ -32,7 +32,7 @@ from django.utils import unittest
 
 # local test models
 from models import (Article, BarAccount, CustomArticle, EmptyModel,
-    FooAccount, Gallery, GalleryAdmin, ModelWithStringPrimaryKey,
+    FooAccount, Gallery, PersonAdmin, ModelWithStringPrimaryKey,
     Person, Persona, Picture, Podcast, Section, Subscriber, Vodcast,
     Language, Collector, Widget, Grommet, DooHickey, FancyDoodad, Whatsit,
     Category, Post, Plot, FunkyTag, Chapter, Book, Promo, WorkHour, Employee,
@@ -204,7 +204,7 @@ class AdminViewBasicTest(TestCase):
         Ensure we can sort on a list_display field that is a callable
         (column 2 is callable_year in ArticleAdmin)
         """
-        response = self.client.get('/test_admin/%s/admin_views/article/' % self.urlbit, {'ot': 'asc', 'o': 2})
+        response = self.client.get('/test_admin/%s/admin_views/article/' % self.urlbit, {'o': 2})
         self.assertEqual(response.status_code, 200)
         self.assertTrue(
             response.content.index('Oldest content') < response.content.index('Middle content') and
@@ -217,7 +217,7 @@ class AdminViewBasicTest(TestCase):
         Ensure we can sort on a list_display field that is a Model method
         (colunn 3 is 'model_year' in ArticleAdmin)
         """
-        response = self.client.get('/test_admin/%s/admin_views/article/' % self.urlbit, {'ot': 'dsc', 'o': 3})
+        response = self.client.get('/test_admin/%s/admin_views/article/' % self.urlbit, {'o': '-3'})
         self.assertEqual(response.status_code, 200)
         self.assertTrue(
             response.content.index('Newest content') < response.content.index('Middle content') and
@@ -230,7 +230,7 @@ class AdminViewBasicTest(TestCase):
         Ensure we can sort on a list_display field that is a ModelAdmin method
         (colunn 4 is 'modeladmin_year' in ArticleAdmin)
         """
-        response = self.client.get('/test_admin/%s/admin_views/article/' % self.urlbit, {'ot': 'asc', 'o': 4})
+        response = self.client.get('/test_admin/%s/admin_views/article/' % self.urlbit, {'o': '4'})
         self.assertEqual(response.status_code, 200)
         self.assertTrue(
             response.content.index('Oldest content') < response.content.index('Middle content') and
@@ -238,6 +238,81 @@ class AdminViewBasicTest(TestCase):
             "Results of sorting on ModelAdmin method are out of order."
         )
 
+    def testChangeListSortingMultiple(self):
+        p1 = Person.objects.create(name="Chris", gender=1, alive=True)
+        p2 = Person.objects.create(name="Chris", gender=2, alive=True)
+        p3 = Person.objects.create(name="Bob", gender=1, alive=True)
+        link = '<a href="%s/'
+
+        # Sort by name, gender
+        # This hard-codes the URL because it'll fail if it runs against the
+        # 'admin2' custom admin (which doesn't have the Person model).
+        response = self.client.get('/test_admin/admin/admin_views/person/', {'o': '1.2'})
+        self.assertEqual(response.status_code, 200)
+        self.assertTrue(
+            response.content.index(link % p3.id) < response.content.index(link % p1.id) and
+            response.content.index(link % p1.id) < response.content.index(link % p2.id)
+        )
+
+        # Sort by gender descending, name
+        response = self.client.get('/test_admin/admin/admin_views/person/', {'o': '-2.1'})
+        self.assertEqual(response.status_code, 200)
+        self.assertTrue(
+            response.content.index(link % p2.id) < response.content.index(link % p3.id) and
+            response.content.index(link % p3.id) < response.content.index(link % p1.id)
+        )
+
+    def testChangeListSortingPreserveQuerySetOrdering(self):
+        # If no ordering on ModelAdmin, or query string, the underlying order of
+        # the queryset should not be changed.
+
+        p1 = Person.objects.create(name="Amy", gender=1, alive=True, age=80)
+        p2 = Person.objects.create(name="Bob", gender=1, alive=True, age=70)
+        p3 = Person.objects.create(name="Chris", gender=2, alive=False, age=60)
+        link = '<a href="%s/'
+
+        # This hard-codes the URL because it'll fail if it runs against the
+        # 'admin2' custom admin (which doesn't have the Person model).
+        response = self.client.get('/test_admin/admin/admin_views/person/', {})
+        self.assertEqual(response.status_code, 200)
+        self.assertTrue(
+            response.content.index(link % p3.id) < response.content.index(link % p2.id) and
+            response.content.index(link % p2.id) < response.content.index(link % p1.id)
+        )
+
+    def testChangeListSortingModelMeta(self):
+        # Test ordering on Model Meta is respected
+
+        l1 = Language.objects.create(iso='ur', name='Urdu')
+        l2 = Language.objects.create(iso='ar', name='Arabic')
+        link = '<a href="%s/'
+
+        response = self.client.get('/test_admin/admin/admin_views/language/', {})
+        self.assertEqual(response.status_code, 200)
+        self.assertTrue(
+            response.content.index(link % l2.pk) < response.content.index(link % l1.pk)
+        )
+
+        # Test we can override with query string
+        response = self.client.get('/test_admin/admin/admin_views/language/', {'o':'-1'})
+        self.assertEqual(response.status_code, 200)
+        self.assertTrue(
+            response.content.index(link % l1.pk) < response.content.index(link % l2.pk)
+        )
+
+    def testChangeListSortingModelAdmin(self):
+        # Test ordering on Model Admin is respected, and overrides Model Meta
+        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)
+        )
+
     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').
@@ -1956,7 +2031,7 @@ class AdminActionsTest(TestCase):
             'action' : 'external_mail',
             'index': 0,
         }
-        url = '/test_admin/admin/admin_views/externalsubscriber/?ot=asc&o=1'
+        url = '/test_admin/admin/admin_views/externalsubscriber/?o=1'
         response = self.client.post(url, action_data)
         self.assertRedirects(response, url)