Opened 12 years ago

Closed 12 years ago

#17252 closed Bug (fixed)

Multi-sort in admin does not respect initial admin_order_field

Reported by: Sebastian Goll Owned by: nobody
Component: contrib.admin Version: dev
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 Goll 12 years ago.
r17106-admin-order-field-with-test.diff (9.7 KB ) - added by Sebastian Goll 12 years ago.
r17130-admin-order-field-better-test.diff (9.9 KB ) - added by Sebastian Goll 12 years ago.

Download all attachments as: .zip

Change History (12)

by Sebastian Goll, 12 years ago

comment:1 by Julien Phalip, 12 years ago

Could you please provide a test case?

comment:2 by Julien Phalip, 12 years ago

Needs tests: set

by Sebastian Goll, 12 years ago

comment:3 by Sebastian Goll, 12 years ago

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 12 years ago by Sebastian Goll (previous) (diff)

comment:4 by Julien Phalip, 12 years ago

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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 by Sebastian Goll, 12 years ago

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?

in reply to:  4 comment:6 by Sebastian Goll, 12 years ago

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

in reply to:  5 comment:7 by Julien Phalip, 12 years ago

Severity: NormalRelease 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 by Sebastian Goll, 12 years ago

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.

by Sebastian Goll, 12 years ago

comment:9 by Julien Phalip, 12 years ago

Resolution: fixed
Status: newclosed

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