Code

Opened 6 years ago

Closed 3 years ago

Last modified 3 years ago

#8054 closed (fixed)

Move method properties for admin list customisation to ModelAdmin

Reported by: Daniel Pope <dan@…> Owned by: brosner
Component: contrib.admin Version: master
Severity: Keywords:
Cc: andy@…, kamedov@…, renatopedigoni@…, alexkoshelev Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The current approach for customising list_display seems contrary to the spirit of newforms-admin. Method properties such as .short_description, .boolean and .allow_tags rarely belong in the model definition.

In fact, I noted that these options, for customising the look of the list column, duplicate filter functionality. .allow_tags is exactly |safe; |boolean could be a filter provided by django.contrib.admin.templatetags. The case is much more convincing when you take into account user-defined filters: if I already have a filter for presenting values for user consumption, I should not need to provide a model method to replicate or apply that filter.

I propose a syntax like this:

class AccountAdmin(admin.ModelAdmin):
    foo_column = admin.ListColumn('foo', heading='Foo Description', filter='boolean')
    get_bar_column = admin.ListColumn('get_bar', filter='safe', order_field='bar')

    list_display = [foo_column, get_bar_column, 'baz', 'bonk']

Filter would be specified in exactly template syntax for convenience, so

   get_bar_column = admin.ListColumn('get_bar', filter='truncatewords:"2"|slugify')

would render exactly the same as

{{row.get_bar|truncatewords:"2"|slugify}}

Attachments (6)

8054-list-column.diff (17.0 KB) - added by dan@… 6 years ago.
Initial patch
8054-list-column.2.diff (18.0 KB) - added by Daniel Pope <dan@…> 6 years ago.
Fixed sorting with ListColumns
8054-list-column.3.diff (21.8 KB) - added by alekam 4 years ago.
8054-list-column.4.diff (22.3 KB) - added by alekam 4 years ago.
Update patch. Now it passes all existing regression tests.
8054-list-column.5.diff (244.0 KB) - added by alekam 4 years ago.
Update patch. Add regression tests and fix some bugs.
8054-list-column.6.diff (245.7 KB) - added by alekam 3 years ago.

Download all attachments as: .zip

Change History (29)

comment:1 Changed 6 years ago by ericholscher

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

comment:2 Changed 6 years ago by Daniel Pope <dan@…>

  • Component changed from Uncategorized to Admin interface

The reason I suggested filters be specified like this was because I thought it would be simpler for users to re-use this element of template syntax than to import the actual filter functions, especially because these would have to be curried to pass arguments.

ListColumn would probably also need to take a load_filters=[] kwarg to address templatetag library loading.

Changed 6 years ago by dan@…

Initial patch

comment:3 Changed 6 years ago by Daniel Pope <dan@…>

  • Has patch set
  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set

I have tried coding up an implementation of this ticket (exactly as described above except the kwarg is 'header' not 'heading'). Patch needs improvement as it isn't pep8-compliant.

comment:4 Changed 6 years ago by brosner

  • milestone set to post-1.0

Not much time to get this in before feature-freeze.

Changed 6 years ago by Daniel Pope <dan@…>

Fixed sorting with ListColumns

comment:5 Changed 6 years ago by brosner

  • milestone changed from post-1.0 to 1.0 beta
  • Owner changed from nobody to brosner
  • Status changed from new to assigned
  • Triage Stage changed from Design decision needed to Accepted

I am going to take a step back and put this on 1.0-beta. However, the implementation is a bit too much for this. I want to just decouple the list_display methods from the model. It will be backward incompatible.

comment:6 Changed 6 years ago by mtredinnick

@brosner: allow list_display methods/functions to also exist on the model instance would be good, though. There are plenty of methods that naturally belong on models that are useful in the admin display.

comment:7 Changed 6 years ago by brosner

@malcolmt: Good point. I was just talking with Joseph and I was wondering about the naming of admin_order_field. If we have methods in ModelAdmin that admin bit is redundant. Should that be dropped or maintained?

comment:8 Changed 6 years ago by Daniel Pope <dan@…>

Would you just search both the model instance and the ModelAdmin for callables matching the list_display column? My patch should be backwards compatible, so it could be backwards compatible with yours if you are implementing it like that.

This turned out to be quite a nice refactor, as it broke up the monolithic functions for rendering ChangeLists (actually they were template inclusion tags) into distinct stages of working out how to render each column, then what to render, then rendering it, so that's something else to be said for developing this post-1.0, not least the neat separation between content and presentation that the filters kwarg allows.

comment:9 Changed 6 years ago by brosner

@Daniel: I have sort of hijacked this ticket. You sort of described two different things one of which needs to get in before 1.0. The new API bits you have described above are worthy once we look at modified the API a bit more for the ChangeList stuff. The way I have it setup is it looks at the ModelAdmin first and if not found looks at the model level for non-field, non-callable (a new feature i am adding) in list_display.

comment:10 Changed 6 years ago by Daniel Pope <dan@…>

That's fine. So you'll also implement #7503?

Yes, I realise it was really two separate things. It started out as just noting that in ModelAdmin there's now space to tidy up the system of method attributes, but somewhere along the way my main motivation became wanting to apply template filters. I noticed it made a lot of sense not just for the existing modifiers I mentioned, but practical sense in several of my apps, and that's a much bigger win than just API pointling.

For instance, you can add useful columns entirely with ListColumn, for example:

class SpecialOffer(models.Model):
    name = models.CharField(...)
    ends = models.DateTimeField()

class SpecialOfferAdmin(admin.ModelAdmin):
    list_display = [
        'name',
        'ends',
         admin.ListColumn('ends', filter='timeuntil', heading="Ending in")
    ]

comment:11 Changed 6 years ago by brosner

(In [8352]) Fixed #7503 -- Allow callables in list_display. This also does a lookup on the ModelAdmin for the method if the value is a string before looking on the model. Refs #8054. Thanks qmanic and Daniel Pope for tickets and patches.

comment:12 Changed 6 years ago by brosner

  • milestone changed from 1.0 beta to post-1.0

Moving to post-1.0 for the API changing parts. This should be looked at with a ChangeList and templatetag refactor of the admin. Not sure when or how, but this is related.

comment:13 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:14 Changed 5 years ago by anonymous

  • Cc andy@… added

Changed 4 years ago by alekam

comment:15 Changed 4 years ago by alekam

update patch to django rev. 14124

comment:16 Changed 4 years ago by alekam

  • Cc kamedov@… added

Changed 4 years ago by alekam

Update patch. Now it passes all existing regression tests.

comment:17 Changed 4 years ago by renatopedigoni

  • Cc renatopedigoni@… added

Changed 4 years ago by alekam

Update patch. Add regression tests and fix some bugs.

comment:18 Changed 4 years ago by alekam

  • milestone set to 1.3
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Wiki page with this ticket goals and documentation - ListColumns

comment:19 Changed 3 years ago by alexkoshelev

  • Cc alexkoshelev added

comment:20 Changed 3 years ago by buriy

  • Needs documentation set

Patch is looking good, except few small things:

1) value_map is not a function what can be read from its name, but a dict.
If it is used mostly for choices, it should be named so.

2) There's no line in docs mentioning value_map is applied *before* filters?

3) I'm not sure if we need 200 Kb fixture file called listcolumns.json
Can we strip it to few lines, showcasing all the features?
Tests themselves are very good!

4)
This is backward compatibility hack one copied from django.contrib.admin.util.label_for_field, but it's completely undocumented (and i'm not sure if it's required in django 1.3!):

 	145	            if self.field_name == '__unicode__': 
 	146	                self.header = force_unicode(model._meta.verbose_name) 
 	147	            elif self.field_name == '__str__': 
 	148	                self.header = smart_str(model._meta.verbose_name)

Why can't you just reuse label_for_field to always provide header info for you?

5) Wiki docs don't mention how ListColumn can be used together with list_display_links.

6) list_display_links should take ListColumn, or some special instance, but not its field_name attribute, or you might have 2 fields selected if they just have different filters applied to the same model or modeladmin attribute.
However this might be mention in docs as a known limitation and workaround shown.

7) Typos in wiki docs: 'tagging_autocomplite_tags' -> 'tagging_autocomplete_tags', 'calable' -> 'callable'

8) Wiki docs don't list admin filters that can be used now (boolean_icon)

Changed 3 years ago by alekam

comment:21 Changed 3 years ago by alekam

Update patch and wiki page according to Yuri Baburov (buriy) recomendations

comment:22 Changed 3 years ago by carljm

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

Closing this ticket to avoid any further confusion, based on mailing list discussion here: http://groups.google.com/group/django-developers/browse_thread/thread/e263cb72eb25f8f8

Given that there doesn't appear to be any core dev support for the list_display syntax changes in the patch here, and the feature described by the summary/title is already present in trunk, I don't see any benefit to keeping this ticket open, and there appears to be a significant cost in confusion. The only actual "todo" I see remaining here is that the admin ChangeList needs a refactor; if that needs a ticket, it should get a new one that more clearly states that goal alone.

Resolving as fixed because all of the capabilities requested in the ticket are now possible, albeit with a different syntax than originally proposed.

comment:23 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.