From 4ade8386ebfeb7a781dc2b62542c1cf5f8b9ddaf Mon Sep 17 00:00:00 2001
From: Tom Carrick <tom@carrick.eu>
Date: Tue, 4 Apr 2023 15:11:11 +0100
Subject: [PATCH] Fixed #10743 -- Allowed lookups for related fields in
 ModelAdmin.list_display.

Co-authored-by: Alex Garcia <me@alexoteiza.com>
Co-authored-by: Natalia <124304+nessita@users.noreply.github.com>
Co-authored-by: Nina Menezes <https://github.com/nmenezes0>
---
 AUTHORS                            |  2 ++
 django/contrib/admin/checks.py     | 28 +++++++--------
 django/contrib/admin/utils.py      | 38 ++++++++++++--------
 django/contrib/admin/views/main.py | 14 +++++---
 docs/ref/checks.txt                |  6 ++--
 docs/ref/contrib/admin/index.txt   | 20 ++++++++---
 docs/releases/5.1.txt              |  3 +-
 tests/admin_changelist/admin.py    |  4 +++
 tests/admin_changelist/models.py   |  5 +++
 tests/admin_changelist/tests.py    | 58 ++++++++++++++++++++++++++++++
 tests/admin_checks/tests.py        | 23 ++++++++++++
 tests/admin_utils/tests.py         | 12 +++++++
 tests/modeladmin/test_checks.py    | 19 ++++++++--
 13 files changed, 186 insertions(+), 46 deletions(-)

diff --git a/AUTHORS b/AUTHORS
index 8c903ff6c5..26cfe71138 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -44,6 +44,7 @@ answer newbie questions, and generally made Django that much better:
     Albert Wang <https://github.com/albertyw/>
     Alcides Fonseca
     Aldian Fazrihady <mobile@aldian.net>
+    Alejandro García Ruiz de Oteiza <https://github.com/AlexOteiza>
     Aleksandra Sendecka <asendecka@hauru.eu>
     Aleksi Häkli <aleksi.hakli@iki.fi>
     Alex Dutton <django@alexdutton.co.uk>
@@ -760,6 +761,7 @@ answer newbie questions, and generally made Django that much better:
     Nicolas Noé <nicolas@niconoe.eu>
     Nikita Marchant <nikita.marchant@gmail.com>
     Nikita Sobolev <mail@sobolevn.me>
+    Nina Menezes <https://github.com/nmenezes0>
     Niran Babalola <niran@niran.org>
     Nis Jørgensen <nis@superlativ.dk>
     Nowell Strite <https://nowell.strite.org/>
diff --git a/django/contrib/admin/checks.py b/django/contrib/admin/checks.py
index aa43718cd6..c1a17af076 100644
--- a/django/contrib/admin/checks.py
+++ b/django/contrib/admin/checks.py
@@ -915,21 +915,19 @@ class ModelAdminChecks(BaseModelAdminChecks):
             try:
                 field = getattr(obj.model, item)
             except AttributeError:
-                return [
-                    checks.Error(
-                        "The value of '%s' refers to '%s', which is not a "
-                        "callable, an attribute of '%s', or an attribute or "
-                        "method on '%s'."
-                        % (
-                            label,
-                            item,
-                            obj.__class__.__name__,
-                            obj.model._meta.label,
-                        ),
-                        obj=obj.__class__,
-                        id="admin.E108",
-                    )
-                ]
+                try:
+                    field = get_fields_from_path(obj.model, item)[-1]
+                except (FieldDoesNotExist, NotRelationField):
+                    return [
+                        checks.Error(
+                            f"The value of '{label}' refers to '{item}', which is not "
+                            f"a callable or attribute of '{obj.__class__.__name__}', "
+                            "or an attribute, method, or field on "
+                            f"'{obj.model._meta.label}'.",
+                            obj=obj.__class__,
+                            id="admin.E108",
+                        )
+                    ]
         if (
             getattr(field, "is_relation", False)
             and (field.many_to_many or field.one_to_many)
diff --git a/django/contrib/admin/utils.py b/django/contrib/admin/utils.py
index 0bcf99ae85..c8e722bcc8 100644
--- a/django/contrib/admin/utils.py
+++ b/django/contrib/admin/utils.py
@@ -289,8 +289,8 @@ def lookup_field(name, obj, model_admin=None):
     try:
         f = _get_non_gfk_field(opts, name)
     except (FieldDoesNotExist, FieldIsAForeignKeyColumnName):
-        # For non-field values, the value is either a method, property or
-        # returned via a callable.
+        # For non-regular field values, the value is either a method,
+        # property, related field, or returned via a callable.
         if callable(name):
             attr = name
             value = attr(obj)
@@ -298,10 +298,17 @@ def lookup_field(name, obj, model_admin=None):
             attr = getattr(model_admin, name)
             value = attr(obj)
         else:
-            attr = getattr(obj, name)
+            sentinel = object()
+            attr = getattr(obj, name, sentinel)
             if callable(attr):
                 value = attr()
             else:
+                if attr is sentinel:
+                    attr = obj
+                    for part in name.split(LOOKUP_SEP):
+                        attr = getattr(attr, part, sentinel)
+                        if attr is sentinel:
+                            return None, None, None
                 value = attr
             if hasattr(model_admin, "model") and hasattr(model_admin.model, name):
                 attr = getattr(model_admin.model, name)
@@ -345,9 +352,10 @@ def label_for_field(name, model, model_admin=None, return_attr=False, form=None)
     """
     Return a sensible label for a field name. The name can be a callable,
     property (but not created with @property decorator), or the name of an
-    object's attribute, as well as a model field. If return_attr is True, also
-    return the resolved attribute (which could be a callable). This will be
-    None if (and only if) the name refers to a field.
+    object's attribute, as well as a model field, including across related
+    objects. If return_attr is True, also return the resolved attribute
+    (which could be a callable). This will be None if (and only if) the name
+    refers to a field.
     """
     attr = None
     try:
@@ -371,15 +379,15 @@ def label_for_field(name, model, model_admin=None, return_attr=False, form=None)
             elif form and name in form.fields:
                 attr = form.fields[name]
             else:
-                message = "Unable to lookup '%s' on %s" % (
-                    name,
-                    model._meta.object_name,
-                )
-                if model_admin:
-                    message += " or %s" % model_admin.__class__.__name__
-                if form:
-                    message += " or %s" % form.__class__.__name__
-                raise AttributeError(message)
+                try:
+                    attr = get_fields_from_path(model, name)[-1]
+                except (FieldDoesNotExist, NotRelationField):
+                    message = f"Unable to lookup '{name}' on {model._meta.object_name}"
+                    if model_admin:
+                        message += f" or {model_admin.__class__.__name__}"
+                    if form:
+                        message += f" or {form.__class__.__name__}"
+                    raise AttributeError(message)
 
             if hasattr(attr, "short_description"):
                 label = attr.short_description
diff --git a/django/contrib/admin/views/main.py b/django/contrib/admin/views/main.py
index 44001f00f9..d8fff50d18 100644
--- a/django/contrib/admin/views/main.py
+++ b/django/contrib/admin/views/main.py
@@ -30,6 +30,7 @@ from django.core.exceptions import (
 )
 from django.core.paginator import InvalidPage
 from django.db.models import F, Field, ManyToOneRel, OrderBy
+from django.db.models.constants import LOOKUP_SEP
 from django.db.models.expressions import Combinable
 from django.urls import reverse
 from django.utils.deprecation import RemovedInDjango60Warning
@@ -356,9 +357,9 @@ class ChangeList:
         """
         Return the proper model field name corresponding to the given
         field_name to use for ordering. field_name may either be the name of a
-        proper model field or the name of a method (on the admin or model) or a
-        callable with the 'admin_order_field' attribute. Return None if no
-        proper model field name can be matched.
+        proper model field, possibly across relations, or the name of a method
+        (on the admin or model) or a callable with the 'admin_order_field'
+        attribute. Return None if no proper model field name can be matched.
         """
         try:
             field = self.lookup_opts.get_field(field_name)
@@ -371,7 +372,12 @@ class ChangeList:
             elif hasattr(self.model_admin, field_name):
                 attr = getattr(self.model_admin, field_name)
             else:
-                attr = getattr(self.model, field_name)
+                try:
+                    attr = getattr(self.model, field_name)
+                except AttributeError:
+                    if LOOKUP_SEP in field_name:
+                        return field_name
+                    raise
             if isinstance(attr, property) and hasattr(attr, "fget"):
                 attr = attr.fget
             return getattr(attr, "admin_order_field", None)
diff --git a/docs/ref/checks.txt b/docs/ref/checks.txt
index f0eeaca268..cf0ab32efa 100644
--- a/docs/ref/checks.txt
+++ b/docs/ref/checks.txt
@@ -726,9 +726,9 @@ with the admin site:
 * **admin.E106**: The value of ``<InlineModelAdmin class>.model`` must be a
   ``Model``.
 * **admin.E107**: The value of ``list_display`` must be a list or tuple.
-* **admin.E108**: The value of ``list_display[n]`` refers to ``<label>``,
-  which is not a callable, an attribute of ``<ModelAdmin class>``, or an
-  attribute or method on ``<model>``.
+* **admin.E108**: The value of ``list_display[n]`` refers to ``<label>``, which
+  is not a callable or attribute of ``<ModelAdmin class>``, or an attribute,
+  method, or field on ``<model>``.
 * **admin.E109**: The value of ``list_display[n]`` must not be a many-to-many
   field or a reverse foreign key.
 * **admin.E110**: The value of ``list_display_links`` must be a list, a tuple,
diff --git a/docs/ref/contrib/admin/index.txt b/docs/ref/contrib/admin/index.txt
index e85ba9c36a..e0e57b9fc0 100644
--- a/docs/ref/contrib/admin/index.txt
+++ b/docs/ref/contrib/admin/index.txt
@@ -315,9 +315,9 @@ subclass::
     For more complex layout needs, see the :attr:`~ModelAdmin.fieldsets` option.
 
     The ``fields`` option accepts the same types of values as
-    :attr:`~ModelAdmin.list_display`, except that callables aren't accepted.
-    Names of model and model admin methods will only be used if they're listed
-    in :attr:`~ModelAdmin.readonly_fields`.
+    :attr:`~ModelAdmin.list_display`, except that callables and ``__`` lookups
+    for related fields aren't accepted. Names of model and model admin methods
+    will only be used if they're listed in :attr:`~ModelAdmin.readonly_fields`.
 
     To display multiple fields on the same line, wrap those fields in their own
     tuple. In this example, the ``url`` and ``title`` fields will display on the
@@ -565,7 +565,7 @@ subclass::
     If you don't set ``list_display``, the admin site will display a single
     column that displays the ``__str__()`` representation of each object.
 
-    There are four types of values that can be used in ``list_display``. All
+    There are five types of values that can be used in ``list_display``. All
     but the simplest may use the  :func:`~django.contrib.admin.display`
     decorator, which is used to customize how the field is presented:
 
@@ -574,6 +574,11 @@ subclass::
           class PersonAdmin(admin.ModelAdmin):
               list_display = ["first_name", "last_name"]
 
+    * The name of a related field, using the ``__`` notation. For example::
+
+          class PersonAdmin(admin.ModelAdmin):
+              list_display = ["city__name"]
+
     * A callable that accepts one argument, the model instance. For example::
 
           @admin.display(description="Name")
@@ -614,6 +619,11 @@ subclass::
           class PersonAdmin(admin.ModelAdmin):
               list_display = ["name", "decade_born_in"]
 
+    .. versionchanged:: 5.1
+
+        Support for using ``__`` lookups was added, when targeting related
+        fields.
+
     A few special cases to note about ``list_display``:
 
     * If the field is a ``ForeignKey``, Django will display the
@@ -831,7 +841,7 @@ subclass::
     * Django will try to interpret every element of ``list_display`` in this
       order:
 
-      * A field of the model.
+      * A field of the model or from a related field.
       * A callable.
       * A string representing a ``ModelAdmin`` attribute.
       * A string representing a model attribute.
diff --git a/docs/releases/5.1.txt b/docs/releases/5.1.txt
index 1d014ceb21..701d686532 100644
--- a/docs/releases/5.1.txt
+++ b/docs/releases/5.1.txt
@@ -32,7 +32,8 @@ Minor features
 :mod:`django.contrib.admin`
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-* ...
+* :attr:`.ModelAdmin.list_display` now supports using ``__`` lookups to list
+  fields from related models.
 
 :mod:`django.contrib.admindocs`
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/tests/admin_changelist/admin.py b/tests/admin_changelist/admin.py
index 8ffc45e391..3e6009b0c5 100644
--- a/tests/admin_changelist/admin.py
+++ b/tests/admin_changelist/admin.py
@@ -53,6 +53,10 @@ class ChildAdmin(admin.ModelAdmin):
         return super().get_queryset(request).select_related("parent")
 
 
+class GrandChildAdmin(admin.ModelAdmin):
+    list_display = ["name", "parent__name", "parent__parent__name"]
+
+
 class CustomPaginationAdmin(ChildAdmin):
     paginator = CustomPaginator
 
diff --git a/tests/admin_changelist/models.py b/tests/admin_changelist/models.py
index aa4656e93e..290a3ea4ec 100644
--- a/tests/admin_changelist/models.py
+++ b/tests/admin_changelist/models.py
@@ -19,6 +19,11 @@ class Child(models.Model):
     age = models.IntegerField(null=True, blank=True)
 
 
+class GrandChild(models.Model):
+    parent = models.ForeignKey(Child, models.SET_NULL, editable=False, null=True)
+    name = models.CharField(max_length=30, blank=True)
+
+
 class Genre(models.Model):
     name = models.CharField(max_length=20)
 
diff --git a/tests/admin_changelist/tests.py b/tests/admin_changelist/tests.py
index b4739b572d..72fac8cd61 100644
--- a/tests/admin_changelist/tests.py
+++ b/tests/admin_changelist/tests.py
@@ -42,6 +42,7 @@ from .admin import (
     EmptyValueChildAdmin,
     EventAdmin,
     FilteredChildAdmin,
+    GrandChildAdmin,
     GroupAdmin,
     InvitationAdmin,
     NoListDisplayLinksParentAdmin,
@@ -61,6 +62,7 @@ from .models import (
     CustomIdUser,
     Event,
     Genre,
+    GrandChild,
     Group,
     Invitation,
     Membership,
@@ -1634,6 +1636,62 @@ class ChangeListTests(TestCase):
                     response, f'0 results (<a href="{href}">1 total</a>)'
                 )
 
+    def test_list_display_related_field(self):
+        parent = Parent.objects.create(name="I am your father")
+        child = Child.objects.create(name="I am your child", parent=parent)
+        GrandChild.objects.create(name="I am your grandchild", parent=child)
+        request = self._mocked_authenticated_request("/grandchild/", self.superuser)
+
+        m = GrandChildAdmin(GrandChild, custom_site)
+        response = m.changelist_view(request)
+        self.assertContains(response, parent.name)
+        self.assertContains(response, child.name)
+
+    def test_list_display_related_field_null(self):
+        GrandChild.objects.create(name="I am parentless", parent=None)
+        request = self._mocked_authenticated_request("/grandchild/", self.superuser)
+
+        m = GrandChildAdmin(GrandChild, custom_site)
+        response = m.changelist_view(request)
+        self.assertContains(response, '<td class="field-parent__name">-</td>')
+        self.assertContains(response, '<td class="field-parent__parent__name">-</td>')
+
+    def test_list_display_related_field_ordering(self):
+        parent_a = Parent.objects.create(name="Alice")
+        parent_z = Parent.objects.create(name="Zara")
+        Child.objects.create(name="Alice's child", parent=parent_a)
+        Child.objects.create(name="Zara's child", parent=parent_z)
+
+        class ChildAdmin(admin.ModelAdmin):
+            list_display = ["name", "parent__name"]
+            list_per_page = 1
+
+        m = ChildAdmin(Child, custom_site)
+
+        # Order ascending.
+        request = self._mocked_authenticated_request("/grandchild/?o=1", self.superuser)
+        response = m.changelist_view(request)
+        self.assertContains(response, parent_a.name)
+        self.assertNotContains(response, parent_z.name)
+
+        # Order descending.
+        request = self._mocked_authenticated_request(
+            "/grandchild/?o=-1", self.superuser
+        )
+        response = m.changelist_view(request)
+        self.assertNotContains(response, parent_a.name)
+        self.assertContains(response, parent_z.name)
+
+    def test_list_display_related_field_ordering_fields(self):
+        class ChildAdmin(admin.ModelAdmin):
+            list_display = ["name", "parent__name"]
+            ordering = ["parent__name"]
+
+        m = ChildAdmin(Child, custom_site)
+        request = self._mocked_authenticated_request("/", self.superuser)
+        cl = m.get_changelist_instance(request)
+        self.assertEqual(cl.get_ordering_field_columns(), {2: "asc"})
+
 
 class GetAdminLogTests(TestCase):
     def test_custom_user_pk_not_named_id(self):
diff --git a/tests/admin_checks/tests.py b/tests/admin_checks/tests.py
index d2d1eb219e..6ca5d6d925 100644
--- a/tests/admin_checks/tests.py
+++ b/tests/admin_checks/tests.py
@@ -1009,3 +1009,26 @@ class SystemChecksTestCase(SimpleTestCase):
             self.assertEqual(errors, [])
         finally:
             Book._meta.apps.ready = True
+
+    def test_related_field_list_display(self):
+        class SongAdmin(admin.ModelAdmin):
+            list_display = ["pk", "original_release", "album__title"]
+
+        errors = SongAdmin(Song, AdminSite()).check()
+        self.assertEqual(errors, [])
+
+    def test_related_field_list_display_wrong_field(self):
+        class SongAdmin(admin.ModelAdmin):
+            list_display = ["pk", "original_release", "album__hello"]
+
+        errors = SongAdmin(Song, AdminSite()).check()
+        expected = [
+            checks.Error(
+                "The value of 'list_display[2]' refers to 'album__hello', which is not "
+                "a callable or attribute of 'SongAdmin', or an attribute, method, or "
+                "field on 'admin_checks.Song'.",
+                obj=SongAdmin,
+                id="admin.E108",
+            )
+        ]
+        self.assertEqual(errors, expected)
diff --git a/tests/admin_utils/tests.py b/tests/admin_utils/tests.py
index 067b47198d..56d46324e0 100644
--- a/tests/admin_utils/tests.py
+++ b/tests/admin_utils/tests.py
@@ -137,6 +137,7 @@ class UtilsTests(SimpleTestCase):
             (simple_function, SIMPLE_FUNCTION),
             ("test_from_model", article.test_from_model()),
             ("non_field", INSTANCE_ATTRIBUTE),
+            ("site__domain", SITE_NAME),
         )
 
         mock_admin = MockModelAdmin()
@@ -294,6 +295,17 @@ class UtilsTests(SimpleTestCase):
 
         self.assertEqual(label_for_field(lambda x: "nothing", Article), "--")
         self.assertEqual(label_for_field("site_id", Article), "Site id")
+        # The correct name and attr are returned when `__` is in the field name.
+        self.assertEqual(label_for_field("site__domain", Article), "Site  domain")
+        self.assertEqual(
+            label_for_field("site__domain", Article, return_attr=True),
+            ("Site  domain", Site._meta.get_field("domain")),
+        )
+
+    def test_label_for_field_failed_lookup(self):
+        msg = "Unable to lookup 'site__unknown' on Article"
+        with self.assertRaisesMessage(AttributeError, msg):
+            label_for_field("site__unknown", Article)
 
         class MockModelAdmin:
             @admin.display(description="not Really the Model")
diff --git a/tests/modeladmin/test_checks.py b/tests/modeladmin/test_checks.py
index 73777f05ab..f767a6c92b 100644
--- a/tests/modeladmin/test_checks.py
+++ b/tests/modeladmin/test_checks.py
@@ -69,7 +69,7 @@ class RawIdCheckTests(CheckTestCase):
 
     def test_missing_field(self):
         class TestModelAdmin(ModelAdmin):
-            raw_id_fields = ("non_existent_field",)
+            raw_id_fields = ["non_existent_field"]
 
         self.assertIsInvalid(
             TestModelAdmin,
@@ -602,8 +602,21 @@ class ListDisplayTests(CheckTestCase):
             TestModelAdmin,
             ValidationTestModel,
             "The value of 'list_display[0]' refers to 'non_existent_field', "
-            "which is not a callable, an attribute of 'TestModelAdmin', "
-            "or an attribute or method on 'modeladmin.ValidationTestModel'.",
+            "which is not a callable or attribute of 'TestModelAdmin', "
+            "or an attribute, method, or field on 'modeladmin.ValidationTestModel'.",
+            "admin.E108",
+        )
+
+    def test_missing_related_field(self):
+        class TestModelAdmin(ModelAdmin):
+            list_display = ("band__non_existent_field",)
+
+        self.assertIsInvalid(
+            TestModelAdmin,
+            ValidationTestModel,
+            "The value of 'list_display[0]' refers to 'band__non_existent_field', "
+            "which is not a callable or attribute of 'TestModelAdmin', "
+            "or an attribute, method, or field on 'modeladmin.ValidationTestModel'.",
             "admin.E108",
         )