From a689ba95947e29094dbb7d92d2784edb5e58482c Mon Sep 17 00:00:00 2001 From: Brian Rosner Date: Sun, 10 Jan 2010 03:36:59 +0000 Subject: [PATCH] Fixed #12455 -- corrected an oversight in result_headers not honoring admin_order_field This finishes some slightly refactoring that went into the admin_list template tags. Thanks to kegan for discovering the oversight. git-svn-id: http://code.djangoproject.com/svn/django/trunk@12157 bcc190cf-cafb-0310-a4f2-bffc1f526a37 --- .../contrib/admin/templatetags/admin_list.py | 20 ++--- django/contrib/admin/util.py | 51 +++++++----- tests/regressiontests/admin_util/models.py | 19 ++++- tests/regressiontests/admin_util/tests.py | 81 ++++++++++++++++++- 4 files changed, 139 insertions(+), 32 deletions(-) diff --git a/django/contrib/admin/templatetags/admin_list.py b/django/contrib/admin/templatetags/admin_list.py index 8626ef0c23..7cd3f0a3af 100644 --- a/django/contrib/admin/templatetags/admin_list.py +++ b/django/contrib/admin/templatetags/admin_list.py @@ -76,17 +76,17 @@ def result_headers(cl): lookup_opts = cl.lookup_opts for i, field_name in enumerate(cl.list_display): - attr = None - try: - f = lookup_opts.get_field(field_name) - admin_order_field = None - header = f.verbose_name - except models.FieldDoesNotExist: - header = label_for_field(field_name, cl.model, cl.model_admin) + header, attr = label_for_field(field_name, cl.model, + model_admin = cl.model_admin, + return_attr = True + ) + if attr: # if the field is the action checkbox: no sorting and special class if field_name == 'action_checkbox': - yield {"text": header, - "class_attrib": mark_safe(' class="action-checkbox-column"')} + yield { + "text": header, + "class_attrib": mark_safe(' class="action-checkbox-column"') + } continue header = pretty_name(header) @@ -98,6 +98,8 @@ def result_headers(cl): # So this _is_ a sortable non-field. Go to the yield # after the else clause. + else: + admin_order_field = None th_classes = [] new_order_type = 'asc' diff --git a/django/contrib/admin/util.py b/django/contrib/admin/util.py index dc7ebcbb91..d252a805ab 100644 --- a/django/contrib/admin/util.py +++ b/django/contrib/admin/util.py @@ -250,32 +250,41 @@ def lookup_field(name, obj, model_admin=None): value = getattr(obj, f.attname) return f, attr, value -def label_for_field(name, model, model_admin): +def label_for_field(name, model, model_admin=None, return_attr=False): + attr = None try: - return model._meta.get_field_by_name(name)[0].verbose_name + label = model._meta.get_field_by_name(name)[0].verbose_name except models.FieldDoesNotExist: if name == "__unicode__": - return force_unicode(model._meta.verbose_name) - if name == "__str__": - return smart_str(model._meta.verbose_name) - if callable(name): - attr = name - elif hasattr(model_admin, name): - attr = getattr(model_admin, name) - elif hasattr(model, name): - attr = getattr(model, name) + label = force_unicode(model._meta.verbose_name) + elif name == "__str__": + label = smart_str(model._meta.verbose_name) else: - raise AttributeError - - if hasattr(attr, "short_description"): - return attr.short_description - elif callable(attr): - if attr.__name__ == "": - return "--" + if callable(name): + attr = name + elif model_admin is not None and hasattr(model_admin, name): + attr = getattr(model_admin, name) + elif hasattr(model, name): + attr = getattr(model, name) else: - return attr.__name__ - else: - return name + message = "Unable to lookup '%s' on %s" % (name, model._meta.object_name) + if model_admin: + message += " or %s" % (model_admin.__name__,) + raise AttributeError(message) + + if hasattr(attr, "short_description"): + label = attr.short_description + elif callable(attr): + if attr.__name__ == "": + label = "--" + else: + label = attr.__name__ + else: + label = name + if return_attr: + return (label, attr) + else: + return label def display_for_field(value, field): diff --git a/tests/regressiontests/admin_util/models.py b/tests/regressiontests/admin_util/models.py index b40f364b76..c4022b3814 100644 --- a/tests/regressiontests/admin_util/models.py +++ b/tests/regressiontests/admin_util/models.py @@ -1 +1,18 @@ -# needed for tests \ No newline at end of file +from django.db import models + + + +class Article(models.Model): + """ + A simple Article model for testing + """ + title = models.CharField(max_length=100) + title2 = models.CharField(max_length=100, verbose_name="another name") + created = models.DateTimeField() + + def test_from_model(self): + return "nothing" + + def test_from_model_with_override(self): + return "nothing" + test_from_model_with_override.short_description = "not what you expect" diff --git a/tests/regressiontests/admin_util/tests.py b/tests/regressiontests/admin_util/tests.py index 11e71e20ec..e7f163aa94 100644 --- a/tests/regressiontests/admin_util/tests.py +++ b/tests/regressiontests/admin_util/tests.py @@ -3,12 +3,15 @@ import unittest from django.db import models from django.contrib import admin -from django.contrib.admin.util import display_for_field +from django.contrib.admin.util import display_for_field, label_for_field from django.contrib.admin.views.main import EMPTY_CHANGELIST_VALUE +from models import Article + class UtilTests(unittest.TestCase): + def test_null_display_for_field(self): """ Regression test for #12550: display_for_field should handle None @@ -38,3 +41,79 @@ class UtilTests(unittest.TestCase): display_value = display_for_field(None, models.FloatField()) self.assertEqual(display_value, EMPTY_CHANGELIST_VALUE) + + def test_label_for_field(self): + """ + Tests for label_for_field + """ + self.assertEquals( + label_for_field("title", Article), + "title" + ) + self.assertEquals( + label_for_field("title2", Article), + "another name" + ) + self.assertEquals( + label_for_field("title2", Article, return_attr=True), + ("another name", None) + ) + + self.assertEquals( + label_for_field("__unicode__", Article), + "article" + ) + self.assertEquals( + label_for_field("__str__", Article), + "article" + ) + + self.assertRaises( + AttributeError, + lambda: label_for_field("unknown", Article) + ) + + def test_callable(obj): + return "nothing" + self.assertEquals( + label_for_field(test_callable, Article), + "test_callable" + ) + self.assertEquals( + label_for_field(test_callable, Article, return_attr=True), + ("test_callable", test_callable) + ) + + self.assertEquals( + label_for_field("test_from_model", Article), + "test_from_model" + ) + self.assertEquals( + label_for_field("test_from_model", Article, return_attr=True), + ("test_from_model", Article.test_from_model) + ) + self.assertEquals( + label_for_field("test_from_model_with_override", Article), + "not what you expect" + ) + + self.assertEquals( + label_for_field(lambda x: "nothing", Article), + "--" + ) + + class MockModelAdmin(object): + def test_from_model(self, obj): + return "nothing" + test_from_model.short_description = "not really the model" + self.assertEquals( + label_for_field("test_from_model", Article, model_admin=MockModelAdmin), + "not really the model" + ) + self.assertEquals( + label_for_field("test_from_model", Article, + model_admin = MockModelAdmin, + return_attr = True + ), + ("not really the model", MockModelAdmin.test_from_model) + )