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

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'

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__ == "<lambda>":
-                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__ == "<lambda>":
+                    label = "--"
+                else:
+                    label = attr.__name__
+            else:
+                label = name
+    if return_attr:
+        return (label, attr)
+    else:
+        return label
 
 
 def display_for_field(value, field):

tests/regressiontests/admin_util/models.py

@@ -1 +1,18 @@
-# needed for tests
+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"

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)
+        )