Multi-sort in admin does not respect initial admin_order_field

[Sebastian Goll]

The multi-sort addition to the admin change list (introduced in #11868) fails to set the sort indicators for the default ordering in the following case: the model admin's ordering refers not to a proper field but to the name of a method with admin_order_field set. That method can either be defined in the model admin or the model itself, it doesn't matter.

The problem is easily fixed by the attached patch. We have to resolve the name that was given in list_display both when applying the actual ordering, as well as when getting the default ordering in case no explicit ORDER_VAR has yet been passed to the view.

Since the same functionality is required both within method get_ordering and get_ordering_field_columns, I moved the shared code to the new method _get_admin_order_field which simply does what the name indicates.

[Julien Phalip]

Could you please provide a test case?

[Sebastian Goll]

I attached a new patch, now including testSortIndicatorsAdminOrder() in regressiontests/admin_views.py. The fix itself remains untouched, but the test correctly catches the current unfixed behavior while remaining silent when the fix has been applied.

[Julien Phalip]

Thanks for providing tests, this helps reproducing this issue quite clearly.

However, I'm not entirely happy with the fix, in particular with the fact that it makes the ModelAdmin.ordering option non explicit. For example (based on your tests), to sort by a method called some_order(), you have to set ordering to ('order',).

ModelAdmin.ordering currently only accepts actual model fields. I think it should be modified to also accept any method/function as long as they define a admin_order_field pointing to an actual model field. That way one could explicitly do:

class MyModel(Model):
    foo = models.CharField()

class MyModelAdmin(ModelAdmin):

    def bar(self, obj):
        return obj.foo
    bar.admin_order_field = 'foo'

    list_display = ('bar',)
    ordering = ('bar',)  # <-- Can order by method 'bar'

[Sebastian Goll]

Sure, making ordering more explicit would be nice but IMHO that's outside the scope of this ticket. Here, I'm only concerned with the regression introduced by the multi-sort addition to the admin which causes the initial sort indicator icons to not show up under certain circumstances, i.e. whenever something is sorted with admin_order_field.

This bug is not a functional issue per se, but hiding those icons when the admin change list is actually sorted (which it is) is kind of misleading to the user.

Regarding the proposition of making ordering more explicit, I suggest opening a new ticket for that and keeping this one for the regression at hand. Some questions arise, however, how far this more explicit ordering should reach: should method names also be allowed in models, ie. not only model admins? So would this be allowed:

class MyModel(Model):
    foo = models.CharField()

    def bar(self):
        return self.foo
    bar.admin_order_field = 'foo'

    class Meta:
        ordering = ('bar',)  # <-- Should we be able to order by methods in the model, too?
                             #     If not, the currently analogous behavior of (Model|ModelAdmin).ordering is lost.

You see, I think this needs some more thought which cannot (or rather should not, IMHO) be covered in this purely regression-related ticket.

What do you suggest?

[Sebastian Goll]

One final thought regarding your comment:

However, I'm not entirely happy with the fix, in particular with the fact that it makes the ModelAdmin.ordering option non explicit. For example (based on your tests), to sort by a method called some_order(), you have to set ordering to ('order',).

My patch doesn't change this, I'm simply using the current specification. Having to set ordering explicitly to match the actual model field in question is and was current behavior with and without my patch. In fact, the docs for contrib.admin explicitly say:

From ​https://docs.djangoproject.com/en/dev/ref/contrib/admin/#django.contrib.admin.ModelAdmin.ordering:

ModelAdmin.ordering: Set ordering to specify how lists of objects should be ordered in the Django admin views. This should be a list or tuple in the same format as a model's ordering parameter.

From ​https://docs.djangoproject.com/en/dev/ref/models/options/#django.db.models.Options.ordering:

Options.ordering: The default ordering for the object, for use when obtaining lists of objects: (…) This is a tuple or list of strings. Each string is a field name (…)

[Julien Phalip]

Replying to sebastian:

Sure, making ordering more explicit would be nice but IMHO that's outside the scope of this ticket. Here, I'm only concerned with the regression introduced by the multi-sort addition to the admin which causes the initial sort indicator icons to not show up under certain circumstances, i.e. whenever something is sorted with admin_order_field.

This bug is not a functional issue per se, but hiding those icons when the admin change list is actually sorted (which it is) is kind of misleading to the user.

I confirm that this is a regression, thus marking as blocker. It's ok to address only that regression in this ticket.

Regarding the proposition of making ordering more explicit, I suggest opening a new ticket for that and keeping this one for the regression at hand. Some questions arise, however, how far this more explicit ordering should reach: should method names also be allowed in models, ie. not only model admins?
... snip ...

Yes, a separate ticket could be opened for that. ModelAdmin.ordering should accept exactly the same options as ModelAdmin.list_display, i.e. field names or ModelAdmin methods or Model methods that have a admin_ordering_field pointing to an actual model field.

The patch looks good. The tests, however, don't seem to check which column is actually sorted. They're also a bit fragile as they're testing HTML code (i.e. 'class="sortable sorted ascending"') which is always a bit volatile in the admin templates. I think the tests could be made a little more explicit and less fragile by testing the context value of cl.get_ordering_field_columns(). Could you make that change before we proceed? Thanks!

[Sebastian Goll]

Thanks for the feedback. I revised the test so that it makes use of the ChangeList instance in the response's context, as you suggested. This should make it robust with regard to changes in the admin's HTML markup. It also makes an explicit check for the actual column selected.

[Julien Phalip]

In [17143]:

Fixed #17252 -- Fixed a minor regression introduced by the work in #11868, where the default sorted columns wouldn't correctly be visually represented in the changelist table headers if those columns referred to non model fields. Thanks to sebastian for the report and patch.