[Django] #29669: admin.E033 incorrectly raised when adding a calculated field in the 'ordering' tuple of a ModelAdmin
Django
'ordering' tuple of a ModelAdmin
-------------------------------------+-------------------------------------
Reporter: Javier | Owner: nobody
Abadia Miranda |
Type: Bug | Status: new
Component: | Version: 2.1
contrib.admin |
Severity: Normal | Keywords: admin, ordering
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Consider a simple Django app with two models Blog and Author:
{{{
class Blog(models.Model):
title = models.CharField(max_length=200, default='no title')
author = models.ForeignKey(to='Author', on_delete=models.CASCADE)
def __unicode__(self):
return "[%d] %s" % (self.pk, self.title,)
class Author(models.Model):
name = models.CharField(max_length=200)
def __unicode__(self):
return "[%d] %s" % (self.pk, self.name,)
}}}
Consider that I want to order the Author changelist page in the admin by
Blog count
{{{
@admin.register(Author)
class AuthorAdmin(admin.ModelAdmin):
list_display = ('name', 'blog_count')
ordering = ('name', 'blog_count') # this line causes an admin.E033
error to be reported on startup
def get_queryset(self, request):
qs = super().get_queryset(request)
return qs.annotate(_blog_count=Count('blog'))
def blog_count(self, obj):
return obj._blog_count
blog_count.admin_order_field = '_blog_count'
}}}
If the ordering tuple contains only ('name',), then the app starts
correctly and the list can be sorted by name of blog count clicking on the
list headers.
However, if we want a default ordering by blog_count, the check system
complains about 'blog_count' or '_blog_count' not being a field of the
model.
The admin changelist page supports ordering by a calculated field and it
should also be allowed as a default ordering for the page.
I think the issue is near line
https://github.com/django/django/blob/7eb556a6c2b2ac9313158f8b812eebea02a43f20/django/contrib/admin/checks.py#L522
--
Ticket URL: <https://code.djangoproject.com/ticket/29669>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
'ordering' tuple of a ModelAdmin
-------------------------------------+-------------------------------------
Miranda |
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: admin, ordering | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
OK, I can re-produce this.
I'm not sure exactly what to say. `ModelAdmin.ordering` takes the same
values as model `Options.ordering` which is a list of fields or query
expressions, of which an admin method is not one.
But I can see why we'd want this to work maybe.
Accepting provisionally either to fix, or perhaps document as a
limitation.
--
Ticket URL: <https://code.djangoproject.com/ticket/29669#comment:1>
Django
'ordering' tuple of a ModelAdmin
-------------------------------------+-------------------------------------
Miranda |
Type: Bug | Status: new
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Javier Abadia Miranda):
maybe this helps: adding the field to readonly_fields works ok, maybe the
valid field check should be similar
{{{
readonly_fields = ('name', 'blog_count') # passes all checks
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29669#comment:2>
Django
'ordering' tuple of a ModelAdmin
-------------------------------------+-------------------------------------
Miranda | Ramezani
Type: Bug | Status: assigned
Component: contrib.admin | Version: 2.1
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Hasan Ramezani
* status: new => assigned
Comment:
@Carlton
I can prepare a PR to change `ModelAdmin.ordering` to accept admin method.
but it is a little bit complicated.
What is your opinion about to change `ModelAdmin.ordering` to accept admin
method `admin_order_field`. for example ordering in the above example
should change like this:
`ordering = ('name', '_blog_count') `
--
Ticket URL: <https://code.djangoproject.com/ticket/29669#comment:3>
Django
'ordering' tuple of a ModelAdmin
-------------------------------------+-------------------------------------
Miranda | Ramezani
Type: Bug | Status: assigned
Component: contrib.admin | Version: 2.1
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
PR[https://github.com/django/django/pull/10791]
--
Ticket URL: <https://code.djangoproject.com/ticket/29669#comment:4>
Django
'ordering' tuple of a ModelAdmin
-------------------------------------+-------------------------------------
Miranda | Ramezani
Type: Bug | Status: assigned
Component: contrib.admin | Version: 2.1
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
> The admin changelist page supports ordering by a calculated field and it
should also be allowed as a default ordering for the page.
I would argue the following:
The admin changelist allows dynamic filtering and ordering through
`get_queryset` and static ordering through `ordering`. Static ordering is
checked through introspection and cannot refer to dynamic ordering.
With this statement in mind the following works just fine
{{{#!python
@admin.register(Author)
class AuthorAdmin(admin.ModelAdmin):
list_display = ('name', 'blog_count')
ordering = ('name',)
def get_queryset(self, request):
qs = super().get_queryset(request)
return qs.annotate(
_blog_count=Count('blog')
).order_by('_blog_count')
def blog_count(self, obj):
return obj._blog_count
blog_count.admin_order_field = '_blog_count'
}}}
Sorry for your work Hasan but I don't making the check to take
`admin_order_field` in consideration is correct. The following would fail
just the same way
{{{#!python
@admin.register(Author)
class AuthorAdmin(admin.ModelAdmin):
list_display = ('name',)
ordering = ('name', '_blog_count')
def get_queryset(self, request):
qs = super().get_queryset(request)
return qs.annotate(_blog_count=Count('blog'))
}}}
By the way this exact issue is discussed in #17522 so this should probably
be closed as a duplicate. I'll add more comments there as the situation
has evolved since then.
--
Ticket URL: <https://code.djangoproject.com/ticket/29669#comment:5>
Django
'ordering' tuple of a ModelAdmin
-------------------------------------+-------------------------------------
Miranda | Ramezani
Component: contrib.admin | Version: 2.1
Severity: Normal | Resolution: duplicate
Keywords: admin, ordering | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => duplicate
--
Ticket URL: <https://code.djangoproject.com/ticket/29669#comment:6>