Opened 4 years ago

Closed 4 years ago

#17252 closed Bug (fixed)

Multi-sort in admin does not respect initial admin_order_field

Reported by: sebastian Owned by: nobody
Component: contrib.admin Version: master
Severity: Release blocker Keywords: multi-sort, admin, ordering
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: yes UI/UX: yes

Description

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.

Attachments (3)

r17104-admin-order-field.diff (3.5 KB) - added by sebastian 4 years ago.
r17106-admin-order-field-with-test.diff (9.7 KB) - added by sebastian 4 years ago.
r17130-admin-order-field-better-test.diff (9.9 KB) - added by sebastian 4 years ago.

Download all attachments as: .zip

Change History (12)

Changed 4 years ago by sebastian

comment:1 Changed 4 years ago by julien

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Could you please provide a test case?

comment:2 Changed 4 years ago by julien

  • Needs tests set

Changed 4 years ago by sebastian

comment:3 Changed 4 years ago by sebastian

  • Needs tests unset

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

Last edited 4 years ago by sebastian (previous) (diff)

comment:4 follow-up: Changed 4 years ago by julien

  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

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'

comment:5 follow-up: Changed 4 years ago by 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.

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?

comment:6 in reply to: ↑ 4 Changed 4 years ago by sebastian

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 (…)

comment:7 in reply to: ↑ 5 Changed 4 years ago by julien

  • Severity changed from Normal to Release blocker

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!

comment:8 Changed 4 years ago by sebastian

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.

Changed 4 years ago by sebastian

comment:9 Changed 4 years ago by julien

  • Resolution set to fixed
  • Status changed from new to closed

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.

Note: See TracTickets for help on using tickets.
Back to Top