admin_order_field ignored when shadowing model field

[Julian Andrews]

• Create a method on a model admin which shadows a field on the model,
• Set an admin_order_field attribute on that method
• Add the method name to your model admin's list_display attribute

Result:

ChangeList.get_ordering_field will return the field name instead of the associated admin_order_field value.

Expected Behavior:

ChageList.get_ordering_field should return the adrmin_order_field value.

Here's the relevant code:

    def get_ordering_field(self, field_name):
        """
        Returns 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. Returns None if no
        proper model field name can be matched.
        """
        try:
            field = self.lookup_opts.get_field(field_name)
            return field.name
        except FieldDoesNotExist:
            # See whether field_name is a name of a non-field
            # that allows sorting.
            if callable(field_name):
                attr = field_name
            elif hasattr(self.model_admin, field_name):
                attr = getattr(self.model_admin, field_name)
            else:
                attr = getattr(self.model, field_name)
            return getattr(attr, 'admin_order_field', None)

The core problem is that ChangeList.get_ordering_field is doing the field lookup first, and only trying to check for methods and other callables if that fails. I think that if a method or callable with a defined admin_order_field exists, it should return that rather than the field. Since shadowing a model field for the most part works fine, having the order behave strangely is counterintuitive.

It also seems that this code will return None if there's a callable that shadows the method name but has no admin_order_field, which may also be a bug.

I would propose that the order of priority for resolution should be:

1. methods with a defined admin_order_field
2. callables with a defined admin_order_field
3. model fields

I can see an argument for reversing 1 and 2 (if for nothing other than backwards compatibility) but I don't think a field should take precedence over a method with a clearly defined admin_order_field or that a callable without an admin_order_field should prevent ordering by a method.

I'd be happy to submit a patch if I can get a go-ahead to do it either as I described, or flipping 1 and 2.

[Tim Graham]

Ignoring any consideration of admin_order_field, I see that the value from a model field is used in list_display rather than a model admin method of the same name. While I think this probably isn't the most intuitive behavior, I don't see a big reason to change it (if only for backwards-compatibility).

[Julian Andrews]

Replying to timgraham:

Ignoring any consideration of admin_order_field, I see that the value from a model field is used in list_display rather than a model admin method of the same name. While I think this probably isn't the most intuitive behavior, I don't see a big reason to change it (if only for backwards-compatibility).

I would argue that if a user has defined an admin_order_field attribute on the method there's not much danger of backwards incompatibility. The change I'm proposing would only cause problems where someone had defined admin_order_field and it wasn't being used. I would think it would be more likely to fix undetected bugs than create new ones.

In any case the fact that a callable with no admin_order_field prevents using a method seems like a pretty clear bug to me. Why should the presence of a similarly named function in the module scope prevent the use of an admin_order_field? And, again, while this change could conceivably break some existing code, it's much more likely to fix undetected bugs; no one is defining an admin_order_field on a method with the intent that nothing happen.

The only code this change would break is code that has stray admin_order_field attributes that aren't used, and which is most likely not working the way the author intended.

[Tim Graham]

Maybe it will help to give a concrete example.

class Foo(models.Model):
   name = models.CharField()


class FooAdmin(admin.ModelAdmin):
    list_display = ['name']

    def name(self):
        return "BLAH"

For this case, I see the name field used in list_display and the model admin method ignored.
Therefore, I don't see the use case to define a shadow method along with an admin_order_field. Are you doing something differently?

[Julian Andrews]

Here's (more or less) the situation that led me to the bug:

class Author(models.Model):
    name = models.CharField()


class Book(models.Model):
    title = models.CharField()
    author = models.ForeignKey(Author, related_name='books')


class AuthorAdmin(admin.ModelAdmin):
    list_display = ['name', 'books']

    def books(self):
        return book_count

    books.admin_order_field = 'book_count'

    def get_queryset(self, request):
        qs = super().get_queryset(request)
        return qs.annotate(book_count=models.Count('books'))

This fails, because instead of ordering by books_count it orders by books (which also results in the query getting the GROUP BY clause wrong, and getting an entry for every book, not every author).

Of course there's an easy fix - just rename the books list_display item to book_count, but the behavior here is definitely more than a little surprising, especially since if I'm not looking to order this field, it just works fine shadowing books.

Specifically, ordering by the books field instead of book_count when you click the books column is just clearly the wrong behavior; we're displaying one thing, and ordering by a totally different thing. The ordering should be matched to what's displayed: if we're displaying the output of a method, we should also order by the admin_order_field attribute of that method, or not at all.

[Tim Graham]

I wonder if it wouldn't be better to add warnings when shadowing model fields and/or relations in a ModelAdmin. On the other hand, I'm sure there are people relying on it. The main problem seems to be that it doesn't work consistently as seen in the differences between the situations described in comments 3 and 4.