Opened 6 years ago
Closed 6 years ago
#29669 closed Bug (duplicate)
admin.E033 incorrectly raised when adding a calculated field in the 'ordering' tuple of a ModelAdmin
Reported by: | Javier Abadia Miranda | Owned by: | Hasan Ramezani |
---|---|---|---|
Component: | contrib.admin | Version: | 2.1 |
Severity: | Normal | Keywords: | admin, ordering |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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
Change History (6)
comment:1 by , 6 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 6 years ago
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
comment:3 by , 6 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
@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')
comment:5 by , 6 years ago
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
@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
@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.
comment:6 by , 6 years ago
Resolution: | → duplicate |
---|---|
Status: | assigned → closed |
OK, I can reproduce this.
I'm not sure exactly what to say.
ModelAdmin.ordering
takes the same values as modelOptions.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.