Opened 4 years ago

Closed 4 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 Changed 4 years ago by Carlton Gibson

Triage Stage: UnreviewedAccepted

OK, I can reproduce 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.

Last edited 4 years ago by Tim Graham (previous) (diff)

comment:2 Changed 4 years ago 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

comment:3 Changed 4 years ago by Hasan Ramezani

Owner: changed from nobody to Hasan Ramezani
Status: newassigned

@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:4 Changed 4 years ago by Hasan Ramezani

Has patch: set

comment:5 Changed 4 years ago 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

@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 Changed 4 years ago by Simon Charette

Resolution: duplicate
Status: assignedclosed
Note: See TracTickets for help on using tickets.
Back to Top