Opened 17 years ago

Last modified 8 months ago

#6396 new Cleanup/optimization

Remove customization-unfriendly admin template tags

Reported by: korpios Owned by: nobody
Component: contrib.admin Version: dev
Severity: Normal Keywords: nfa-someday
Cc: kmike84@…, cmawebsite@…, Ülgen Sarıkavak Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

The admin interface makes use of various template tags that are opaque, and make customization difficult.

An example from admin/change_list.html:

{% block result_list %}{% result_list cl %}{% endblock %}

This calls an inclusion_tag called result_list that is used exactly once in the admin (i.e., here); that inclusion_tag drops in some extra context variables before rendering admin/change_list_results.html. This doesn't work with the common [app_label]/[object_name]/template.html magic, since inclusion_tag sets the template names upon registration. If there's a good reason for using a template tag here, I'm not seeing it. :)

The attached patch pulls the template code from change_list_results.html into change_list.html, and updates the latter's context with the extra context variables from the former. I intend to start yanking out similar uses of one-off template tags as I come across them in the admin, so I'll be dropping updated patches on this ticket as I go along.

The rearranging of the functions result_headers, etc., is to avoid some circular import nastiness; I dropped them after ChangeList, which seems to make the most sense.

Attachments (3)

ticket_6396.patch (18.3 KB ) - added by korpios 17 years ago.
better_patch-6396.diff (2.3 KB ) - added by Martin Diers <martin@…> 16 years ago.
Much less intrusive patch to address this problem.
admin_changelist-6396.diff (2.4 KB ) - added by Martin Diers <martin@…> 16 years ago.
Update of previous patch - adding result object to dictionary

Download all attachments as: .zip

Change History (32)

by korpios, 17 years ago

Attachment: ticket_6396.patch added

comment:1 by korpios, 17 years ago

Updated the patch to fix a circular import.

comment:2 by korpios, 17 years ago

Cc: korpios@… added

comment:3 by Tom Vergote, 17 years ago

Triage Stage: UnreviewedDesign decision needed

comment:4 by anonymous, 17 years ago

the patch seems to be outdated? django/contrib/admin/options.py doesn't seem to exist (revision 7135).

comment:5 by redduck666, 17 years ago

OK NVM, ićm an idiot :) (was using 'ordinary' django trunk)

comment:6 by almir@…, 17 years ago

i have a model with DateField and TimeField, if i use the provided patch the 'Today | <little calendar pic>' and 'Now | <little clock pic>' are not present, while they seem to be present with not patched newforms-admin. am i overlooking something obvious?

comment:7 by almir@…, 17 years ago

i have another issue, when i try to submit an edited model (either using 'save' or 'save and add another' buttons) from admin interface i get an error, while if i use 'save and continue editing' it edits the model correctly without any errors.

the error is:

Page not found (404)
Request Method: GET
Request URL: <mysite>:<my port>/admin/todo/

The requested admin page does not exist.

You're seeing this error because you have DEBUG = True in your Django settings file. Change that to False, and Django will display a standard 404 page.

in reply to:  7 comment:8 by almir, 17 years ago

Replying to almir@kiberpipa.org:

i have another issue, when i try to submit an edited model (either using 'save' or 'save and add another' buttons) from admin interface i get an error, while if i use 'save and continue editing' it edits the model correctly without any errors.

the error is:

Page not found (404)
Request Method: GET
Request URL: <mysite>:<my port>/admin/todo/

The requested admin page does not exist.

You're seeing this error because you have DEBUG = True in your Django settings file. Change that to False, and Django will display a standard 404 page.

this error is due to django not putting 'task' (which happens to be my model) after the 'todo' (which happens to be app in which task model is saved)

comment:9 by anonymous, 17 years ago

i found that the 404 error is not specific to this patch, i encounter it everytime i 'by hand' enter the url in in my browser, for example if i access http://my_django_project/admin/todo/task the urls will be missing 'task/' and cause a 404 when clicking on any of my object instances.

comment:10 by Karen Tracey <kmtracey@…>, 16 years ago

Keywords: nfa-someday added

comment:11 by Martin Diers <martin@…>, 16 years ago

Version: newforms-adminSVN

I recently ran into this problem, when I needed to customize the rows in a change-list.

There is a much more elegant solution that will require very few changes to the (now trunk) newforms-admin code: Define your own tag, which inserts your own version of change_list_results.html.

Attached is a patch which changes the admin.templatetags.admin_list.items_for_result function to return a dictionary, rather than a complete row with embedded html. With it is a patched version of change_list_results.html which uses the dictionary fields to move all tag rendering into the template. In all, only about 5 lines of code are changed.

The result is that it is now possible to create your own tag, and override change_list.html to use it.

Patch will follow.

by Martin Diers <martin@…>, 16 years ago

Attachment: better_patch-6396.diff added

Much less intrusive patch to address this problem.

comment:12 by Martin Diers <martin@…>, 16 years ago

Keywords: someday added; nfa-someday removed

comment:13 by Martin Diers <martin@…>, 16 years ago

Component: Template systemAdmin interface

comment:14 by Brian Rosner, 16 years ago

Keywords: nfa-someday added; someday removed

At this point don't change the keywords from nfa-someday to someday. That just adds additional confusion at this point and may make some tickets lost. :)

comment:15 by Martin Diers <martin@…>, 16 years ago

Noted. Thanks for the pointer.

In the course of using this, I found that it is almost a requirement to include a reference to the result object in the dictionary which the custom tag is using to render the rows. Otherwise, there's no real way to access the data upon which one is working in a custom tag, if, for instance, you need to make a decision based upon the value of a field in a row.

Sorry for the rapid update, but I have included that patch, replacing my previous one.

by Martin Diers <martin@…>, 16 years ago

Attachment: admin_changelist-6396.diff added

Update of previous patch - adding result object to dictionary

comment:16 by korpios, 16 years ago

Cc: korpios@… removed

comment:17 by exogen@…, 16 years ago

FWIW, this result_list templatetag is the biggest pain in batchadmin's side. These really do make customization more difficult.

comment:18 by Martin Diers, 16 years ago

I would argue that the template tag is not the problem. The real problem is that the row results themselves are not being rendered in a template at all - meaning that there is no way to tweak the result list row items. Moving the code out of the template tag, as the first patch does, does not solve the problem. It only moves it to a new location.

It is easy to see why a custom tag was chosen here: Because the change_list template becomes especially cumbersome without it. Using the tag has encapsulated a chunk of code.

By doing nothing other than moving the row rendering into the admin_list template tag itself, the problem is solved. It is trivial to create your own version of the admin_list template tag, and use it in your own overridden change_list template.

This is still the one and only patch I apply to my Django installations. It is also backward-compatible. I hope it gets some attention soon.

comment:19 by dvine, 16 years ago

I absolutely support the patch by Martin Diers and would like to see it included. Still I also agree with korpios, that a [app_label]/[object_name]/template.html behaviore of all admin templates, thus eliminating the need for an custom template tag here, would be very desirable. Just wanted to bring up the topic again.

in reply to:  19 ; comment:20 by athena_rd, 15 years ago

Replying to dvine:

I absolutely support the patch by Martin Diers and would like to see it included. Still I also agree with korpios, that a [app_label]/[object_name]/template.html behaviore of all admin templates, thus eliminating the need for an custom template tag here, would be very desirable. Just wanted to bring up the topic again.


Seconded. I too would like to see this added as it would make adding items (i.e. searchbar) to custom admin views much easier.

in reply to:  20 comment:21 by sveri, 15 years ago

Replying to athena_rd:

Replying to dvine:

I absolutely support the patch by Martin Diers and would like to see it included. Still I also agree with korpios, that a [app_label]/[object_name]/template.html behaviore of all admin templates, thus eliminating the need for an custom template tag here, would be very desirable. Just wanted to bring up the topic again.


Seconded. I too would like to see this added as it would make adding items (i.e. searchbar) to custom admin views much easier.

Yea, it definitely would. It would be great to have such a feature implemented. And i think it would make more sense, at least from my point of view.

comment:24 by Julien Phalip, 14 years ago

Type: Cleanup/optimization

comment:25 by Julien Phalip, 13 years ago

Severity: Normal

comment:26 by Alex Gaynor, 13 years ago

Easy pickings: unset
Triage Stage: Design decision neededAccepted
UI/UX: unset

I think the situation has improved recently, much code has been split off into utility functions, as a meta-point cleaning up code like this is always of value.

comment:27 by Mikhail Korobov, 13 years ago

Cc: kmike84@… added

comment:28 by Łukasz Rekucki, 12 years ago

Patch needs improvement: set

Needs a rebase to master.

comment:30 by Aymeric Augustin, 10 years ago

Summary: [newforms-admin] Customization-unfriendly admin template tags should dieRemove customization-unfriendly admin template tags

(Aggressive titles don't help.)

comment:31 by Collin Anderson, 10 years ago

Cc: cmawebsite@… added

comment:32 by Ülgen Sarıkavak, 8 months ago

Cc: Ülgen Sarıkavak added
Note: See TracTickets for help on using tickets.
Back to Top