Opened 20 months ago

Last modified 20 months ago

#27752 assigned Bug

Fix and test admin_order_field set for the __str__ of a model

Reported by: Claude Paroz Owned by: Renato Oliveira
Component: contrib.admin Version: master
Severity: Normal Keywords:
Cc: renato+github@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

While doing some force_str cleanup on Django 2.0, Tim Graham and me noticed a possible issue with admin_order_field on a __str__method (in django.contrib.admin.utils.label_for_field). It's apparently untested. See this related commit [1], with a test that was later removed [2].

https://github.com/django/django/commit/2304ca423640a7b06390530bf61c879f9ff9e4ba
https://github.com/django/django/commit/65cc646c4801e3f3e1df1ab06dfaa763fa4b7b22

So a test for the proper label and sorting is wanted. See test_change_list_sorting_model_admin in tests/admin_views/tests.py for a similar test.

Change History (8)

comment:1 Changed 20 months ago by Renato Oliveira

Cc: renato+github@… added
Owner: changed from nobody to Renato Oliveira
Status: newassigned

comment:2 Changed 20 months ago by Renato Oliveira

Cc: renato+github@… removed
Owner: Renato Oliveira deleted
Status: assignednew

comment:3 Changed 20 months ago by Renato Oliveira

Cc: renato+github@… added

comment:4 Changed 20 months ago by Tim Graham

For example, adding to the Question model in the tutorial:

__str__.admin_order_field = 'question_text'

and to QuestionAdmin:

list_display = ('__str__', ...)

should reproduce this. You'll see that the __str__ column isn't clickable for sorting like it should be. In the lable_for_field function, I tried changing attr = str to attr = model.__str__ -- this makes the column clickable but sorting still doesn't seem to work correctly.

comment:5 Changed 20 months ago by Renato Oliveira

Owner: set to Renato Oliveira
Status: newassigned

comment:6 Changed 20 months ago by Renato Oliveira

Hey, I think I found the issue here.

Changing as you said attr = str to attr = model.__str__ it makes the column clickable and adds the field position to the o querystring. The problem is that when it gets to the admin view (here: https://github.com/django/django/blob/master/django/contrib/admin/views/main.py#L227-L233) it verifies first if the attribute is an instance of the model_admin class and it turns out that ModelAdmin has a str field, not carrying the model.__str__.admin_order_field attribute to the ordering query.

this is the attr representation with the model_admin elif <bound method ModelAdmin.__str__ of <core.admin.QuestionAdmin object at 0x7f82d7bc7748>

for testing purposes I removed the model_admin and got this

<function Question.__str__ at 0x7f04c9ea5598> and it had the admin_order_field attribute, making ordering work properly.

I still don't know how to solve this and would be awesome to have your help on this.

Thanks

comment:7 Changed 20 months ago by Tim Graham

Is it worth adding a special case in get_ordering_field() to get __str__() from the model instead of the ModelAdmin (which probably wouldn't have a useful __str__()? I think probably not. We might just document that admin_order_field isn't usable with Model.__str__() and move on to more important issues.

comment:8 Changed 20 months ago by Renato Oliveira

I'm trying to write something here but nothing seems to fit. Maybe because I'm not fluent on English

    * The ``__str__()`` method is just as valid in ``list_display`` as any
      other model method, so it's perfectly OK to do this::

          list_display = ('__str__', 'some_other_field')

    * Usually, elements of ``list_display`` that aren't actual database
      fields can't be used in sorting (because Django does all the sorting
      at the database level).

      However, if an element of ``list_display`` represents a certain
      database field, you can indicate this fact by setting the
      ``admin_order_field`` attribute of the item. 
      This rule doesn't apply to ``__str__`` method.

It's missing why it's not applicable, but It's not clear to me how to say that

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