Fixed #10743 -- Allowed lookups for related fields in ModelAdmin.list_display. #17357

knyghty · 2023-10-12T17:21:25Z

Following up on #16726

I rebased the PR, changed the wording slightly (still not very sure about it, it's quite a long sentence), used a sentinel, and added a test for when the check fails, which I think addresses everything in the original PR.

nessita

Hi! Thank you for picking this up. Overall looks good, but we should support the lookup in related models even if they are null=True (see error and failing test).

django/contrib/admin/utils.py

docs/releases/5.1.txt

nessita · 2023-10-25T17:25:53Z

I continued my testing with the latest code, and don't ask me how I ran into this (because I stumbled upon this by chance) but when using the o query string that defines ordering, exceptions occurs if the referenced column is obtained from using a lookup in a related field.

It took me a while to understand what was going on, but this test shows the issue:

diff --git a/tests/admin_changelist/tests.py b/tests/admin_changelist/tests.py
index bb73cda87d..13985a02e3 100644
--- a/tests/admin_changelist/tests.py
+++ b/tests/admin_changelist/tests.py
@@ -1642,6 +1642,22 @@ class ChangeListTests(TestCase):
         self.assertContains(response, '<td class="field-parent__name">-</td>')
         self.assertContains(response, '<td class="field-parent__parent__name">-</td>')
 
+    def test_list_display_foreign_field_ordering(self):
+        request = self.factory.get("/grandchild/?o=2")
+        request.user = self.superuser
+
+        class GrandChildAdmin(admin.ModelAdmin):
+            list_display = ["name", "parent__name", "parent__parent__name"]
+
+        m = GrandChildAdmin(GrandChild, custom_site)
+        response = m.changelist_view(request)
+        # we should have a decent assert here!
 
 class GetAdminLogTests(TestCase):
     def test_custom_user_pk_not_named_id(self):

As you can see, this is independent from having any actual entry in the list. The traceback is:

======================================================================
ERROR: test_list_display_foreign_field_ordering (admin_changelist.tests.ChangeListTests.test_list_display_foreign_field_ordering)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/nessita/fellowship/django/django/db/models/options.py", line 670, in get_field
    return self.fields_map[field_name]
           ~~~~~~~~~~~~~~~^^^^^^^^^^^^
KeyError: 'parent__name'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/nessita/fellowship/django/django/contrib/admin/views/main.py", line 364, in get_ordering_field
    field = self.lookup_opts.get_field(field_name)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/nessita/fellowship/django/django/db/models/options.py", line 672, in get_field
    raise FieldDoesNotExist(
django.core.exceptions.FieldDoesNotExist: GrandChild has no field named 'parent__name'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/nessita/fellowship/django/tests/admin_changelist/tests.py", line 1657, in test_list_display_foreign_field_ordering
    response = m.changelist_view(request)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/nessita/fellowship/django/django/utils/decorators.py", line 48, in _wrapper
    return bound_method(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/nessita/fellowship/django/django/utils/decorators.py", line 188, in _view_wrapper
    result = _process_exception(request, e)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/nessita/fellowship/django/django/utils/decorators.py", line 186, in _view_wrapper
    response = view_func(request, *args, **kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/nessita/fellowship/django/django/contrib/admin/options.py", line 1962, in changelist_view
    cl = self.get_changelist_instance(request)
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/nessita/fellowship/django/django/contrib/admin/options.py", line 862, in get_changelist_instance
    return ChangeList(
           ^^^^^^^^^^^
  File "/home/nessita/fellowship/django/django/contrib/admin/views/main.py", line 144, in __init__
    self.queryset = self.get_queryset(request)
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/nessita/fellowship/django/django/contrib/admin/views/main.py", line 573, in get_queryset
    ordering = self.get_ordering(request, qs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/nessita/fellowship/django/django/contrib/admin/views/main.py", line 400, in get_ordering
    order_field = self.get_ordering_field(field_name)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/nessita/fellowship/django/django/contrib/admin/views/main.py", line 374, in get_ordering_field
    attr = getattr(self.model, field_name)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: type object 'GrandChild' has no attribute 'parent__name'

I think this needs to be fixed, at the very least it should not raise an exception. But, this also raises the question of ordering for these newly allowed columns. I think ordering should be supported, specially because the docs says:

The ordering argument supports query lookups to sort by values on related models. [...]

@knyghty What do you think?

knyghty · 2023-10-26T11:39:04Z

@nessita what I think is that this seems hard to do cleanly 🤔 - or am I missing something.

I have pushed a very hacky solution, so hacky I didn't bother to write tests for it, rather I'd like to get feedback first if you can see a less silly way of doing this?

nessita · 2023-10-26T12:08:41Z

@nessita what I think is that this seems hard to do cleanly 🤔 - or am I missing something.

I have pushed a very hacky solution, so hacky I didn't bother to write tests for it, rather I'd like to get feedback first if you can see a less silly way of doing this?

I don't have an answer beforehand, I need to "seat for a while" and do some investigating. I'm happy to do so, likely next week. If, by any chance, you'd find a "less hacky" way of doing it before that, please mention me and push it!

knyghty · 2023-11-02T17:56:12Z

@nessita I made a marginal improvement and added tests for ordering as we'll need those either way.

nessita

The branch is looking great! I added that I would say is a final round of comments after a thorough review. I spent a considerable amount of time testing it, I tried very hard to break it, I got creative but not enough it seems :-)

One thing though, that AFAIK is by design, is that property from related fields are not allowed. Does this match your expectation?

Thank you!

django/contrib/admin/checks.py

django/contrib/admin/utils.py

django/contrib/admin/views/main.py

docs/ref/contrib/admin/index.txt

tests/admin_checks/tests.py

tests/admin_changelist/tests.py

knyghty · 2023-11-09T21:55:00Z

One thing though, that AFAIK is by design, is that property from related fields are not allowed. Does this match your expectation?

Just IMO, but yes, I think this is consistent with the rest of Django. You can't use properties for filtering, and of course we certainly couldn't use them here for ordering. So I think it's expected. If it's desirable or not is another question but I think we should leave that for another ticket.

knyghty · 2023-11-11T10:15:52Z

Thanks for the review @nessita, think I've covered all the bases now, hopefully correctly 😄

nessita · 2024-01-08T19:13:42Z

Hey @knyghty, I know you are busy because of the moving but wanted to check if this PR should be left opened for a little longer, or if we should try to find someone else that adds the necessary Selenium test for the column ordering. Thank you for your work so far! 💯

knyghty · 2024-01-08T19:15:25Z

@nessita please keep this open, I plan to either pair on this with one of my Djangonaut Space cohort, or if nobody is interested, do it myself in the coming weeks.

nessita · 2024-01-08T19:18:28Z

@nessita please keep this open, I plan to either pair on this with one of my Djangonaut Space cohort, or if nobody is interested, do it myself in the coming weeks.

Thank you, pairing with a Djangonaut sounds like an amazing learning opportunity for them ❤️

nessita

This is very close to merging state!

I just added a few nitpicks and a (soft?) request for a few unit tests for the changes in the utils module.

Let me know your thoughts! Thank you.

AUTHORS

django/contrib/admin/checks.py

django/contrib/admin/utils.py

docs/ref/contrib/admin/index.txt

docs/releases/5.1.txt

tests/admin_changelist/tests.py

felixxm · 2024-01-31T07:26:37Z

tests/admin_changelist/tests.py

+        self.assertTrue("Parent C" in result_list_rows[1].text)
+        self.assertTrue("Grandchild Z" in result_list_rows[1].text)
+        self.assertTrue("Parent B" in result_list_rows[2].text)
+        self.assertTrue("Grandchild Y" in result_list_rows[2].text)
+        self.assertTrue("Parent A" in result_list_rows[3].text)
+        self.assertTrue("Grandchild X" in result_list_rows[3].text)


Why not assertIn()?

django/contrib/admin/utils.py

felixxm

I left small suggestions.

tests/admin_changelist/tests.py

tests/admin_utils/tests.py

tests/modeladmin/test_checks.py

nessita

LGTM, thanks @knyghty and @nmenezes0 !

…n.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>

…display. Co-authored-by: Natalia <124304+nessita@users.noreply.github.com> Co-authored-by: Nina Menezes <https://github.com/nmenezes0>

nessita self-requested a review October 12, 2023 20:00

nessita requested changes Oct 13, 2023

View reviewed changes

django/contrib/admin/utils.py Outdated Show resolved Hide resolved

docs/releases/5.1.txt Outdated Show resolved Hide resolved

nessita reviewed Nov 9, 2023

View reviewed changes

knyghty force-pushed the list-display-lookups branch from cf8a965 to 5a00ce8 Compare November 16, 2023 09:17

nessita self-requested a review December 15, 2023 17:44

nessita reviewed Jan 30, 2024

View reviewed changes

felixxm reviewed Jan 31, 2024

View reviewed changes

nessita added the Djangonauts 🚀 label Feb 1, 2024

nmenezes0 mentioned this pull request Feb 4, 2024

List display lookups more changes knyghty/django#6

Merged

nessita force-pushed the list-display-lookups branch 2 times, most recently from 217464d to 846c83d Compare February 5, 2024 18:28

nessita added the selenium Apply to have Selenium tests run on a PR label Feb 5, 2024

nessita force-pushed the list-display-lookups branch from 846c83d to 8fc387b Compare February 5, 2024 19:16

nessita changed the title ~~Fixed #10743 -- Allowed lookup separators in list_display~~ Fixed #10743 -- Allowed lookups for related fields in ModelAdmin.list_display. Feb 5, 2024

felixxm reviewed Feb 5, 2024

View reviewed changes

django/contrib/admin/utils.py Outdated Show resolved Hide resolved

nessita force-pushed the list-display-lookups branch from 8fc387b to 57b835d Compare February 5, 2024 19:32

felixxm reviewed Feb 5, 2024

View reviewed changes

tests/admin_changelist/tests.py Outdated Show resolved Hide resolved

tests/admin_utils/tests.py Outdated Show resolved Hide resolved

tests/modeladmin/test_checks.py Show resolved Hide resolved

nessita force-pushed the list-display-lookups branch 2 times, most recently from ca6f95a to a5bc5df Compare February 5, 2024 22:47

nessita approved these changes Feb 5, 2024

View reviewed changes

Fixed django#10743 -- Allowed lookups for related fields in ModelAdmi…

4ade838

…n.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>

Refs django#10743 -- Enabled ordering for lookups in ModelAdmin.list_…

9cefdfc

…display. Co-authored-by: Natalia <124304+nessita@users.noreply.github.com> Co-authored-by: Nina Menezes <https://github.com/nmenezes0>

nessita force-pushed the list-display-lookups branch from a5bc5df to 9cefdfc Compare February 6, 2024 00:43

nessita merged commit 9cefdfc into django:main Feb 6, 2024
11 of 35 checks passed

felixxm temporarily deployed to schedules February 6, 2024 02:44 — with GitHub Actions Inactive

knyghty deleted the list-display-lookups branch February 16, 2024 14:20

laymonage mentioned this pull request Feb 16, 2024

Using a related field in Snippet/ModelViewSet causes exception when index list requested wagtail/wagtail#11494

Closed

laymonage mentioned this pull request Mar 28, 2024

Add support for related fields in generic IndexView.list_display wagtail/wagtail#11588

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed #10743 -- Allowed lookups for related fields in ModelAdmin.list_display. #17357

Fixed #10743 -- Allowed lookups for related fields in ModelAdmin.list_display. #17357

knyghty commented Oct 12, 2023

nessita left a comment

nessita commented Oct 25, 2023

knyghty commented Oct 26, 2023

nessita commented Oct 26, 2023

knyghty commented Nov 2, 2023

nessita left a comment

knyghty commented Nov 9, 2023

knyghty commented Nov 11, 2023

nessita commented Jan 8, 2024

knyghty commented Jan 8, 2024

nessita commented Jan 8, 2024

nessita left a comment

felixxm Jan 31, 2024

felixxm left a comment

nessita left a comment

Fixed #10743 -- Allowed lookups for related fields in ModelAdmin.list_display. #17357

Fixed #10743 -- Allowed lookups for related fields in ModelAdmin.list_display. #17357

Conversation

knyghty commented Oct 12, 2023

nessita left a comment

Choose a reason for hiding this comment

nessita commented Oct 25, 2023

knyghty commented Oct 26, 2023

nessita commented Oct 26, 2023

knyghty commented Nov 2, 2023

nessita left a comment

Choose a reason for hiding this comment

knyghty commented Nov 9, 2023

knyghty commented Nov 11, 2023

nessita commented Jan 8, 2024

knyghty commented Jan 8, 2024

nessita commented Jan 8, 2024

nessita left a comment

Choose a reason for hiding this comment

felixxm Jan 31, 2024

Choose a reason for hiding this comment

felixxm left a comment

Choose a reason for hiding this comment

nessita left a comment

Choose a reason for hiding this comment