From cb996cce059ec6f384d7ee9b67df6208c92eb719 Mon Sep 17 00:00:00 2001 From: Luke Plant Date: Fri, 3 Jun 2011 11:54:29 +0000 Subject: [PATCH] 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 --- .../contrib/admin/templatetags/admin_list.py | 54 +++++----------- django/contrib/admin/views/main.py | 62 +++++++++++++++---- tests/regressiontests/admin_views/models.py | 15 +++++ tests/regressiontests/admin_views/tests.py | 39 +++++++++++- 4 files changed, 118 insertions(+), 52 deletions(-) diff --git a/django/contrib/admin/templatetags/admin_list.py b/django/contrib/admin/templatetags/admin_list.py index 884b45051a..fb7d24769e 100644 --- a/django/contrib/admin/templatetags/admin_list.py +++ b/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: - order_type = ordering_fields.get(ordering_field_name).lower() - sort_pos = ordering_fields.keys().index(ordering_field_name) + 1 + if i in ordering_field_columns: + order_type = ordering_field_columns.get(i).lower() + 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(): - try: - colnum = list_display_info[f]['index'] - except KeyError: - continue - - if f == ordering_field_name: + for j, ot in ordering_field_columns.items(): + if j == i: # Same column # 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: - colnum = list_display_info[ordering_field_name]['index'] - o_list.insert(0, make_qs_param(new_order_type, colnum)) + if i not in ordering_field_columns: + o_list.insert(0, make_qs_param(new_order_type, i)) o_list = '.'.join(o_list) yield { - "text": info['text'], + "text": text, "sortable": True, "ascending": order_type == "asc", "sort_pos": sort_pos, diff --git a/django/contrib/admin/views/main.py b/django/contrib/admin/views/main.py index 1d48140fc5..85b8562659 100644 --- a/django/contrib/admin/views/main.py +++ b/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): - 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. + def _get_default_ordering(self): ordering = [] if self.model_admin.ordering: ordering = self.model_admin.ordering - elif lookup_opts.ordering: - ordering = lookup_opts.ordering + elif self.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): - # Returns a SortedDict of ordering fields and asc/desc + def get_ordering_field_columns(self): + # 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: - none, t, f = o.rpartition('-') - ordering_fields[f] = 'desc' if t == '-' else 'asc' + if ORDER_VAR not in self.params: + # for ordering specified on ModelAdmin or model Meta, we don't know + # 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): diff --git a/tests/regressiontests/admin_views/models.py b/tests/regressiontests/admin_views/models.py index 98873966eb..52d96a93f6 100644 --- a/tests/regressiontests/admin_views/models.py +++ b/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 '%s' % ('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) diff --git a/tests/regressiontests/admin_views/tests.py b/tests/regressiontests/admin_views/tests.py index 4361dc992e..25056b5e04 100644 --- a/tests/regressiontests/admin_views/tests.py +++ b/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 = '