Opened 7 years ago

Last modified 2 months ago

#6396 new Cleanup/optimization

Remove customization-unfriendly admin template tags

Reported by: korpios Owned by: nobody
Component: contrib.admin Version: master
Severity: Normal Keywords: nfa-someday
Cc: kmike84@…, cmawebsite@… 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 7 years ago.
better_patch-6396.diff (2.3 KB) - added by Martin Diers <martin@…> 7 years ago.
Much less intrusive patch to address this problem.
admin_changelist-6396.diff (2.4 KB) - added by Martin Diers <martin@…> 7 years ago.
Update of previous patch - adding result object to dictionary

Download all attachments as: .zip

Change History (34)

Changed 7 years ago by korpios

comment:1 Changed 7 years ago by korpios

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

Updated the patch to fix a circular import.

comment:2 Changed 7 years ago by korpios

  • Cc korpios@… added

comment:3 Changed 7 years ago by tvrg

  • Triage Stage changed from Unreviewed to Design decision needed

comment:4 Changed 7 years ago by anonymous

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

comment:5 Changed 7 years ago by redduck666

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

comment:6 Changed 7 years ago by almir@…

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 follow-up: Changed 7 years ago by almir@…

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.

comment:8 in reply to: ↑ 7 Changed 7 years ago by almir

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 Changed 7 years ago by anonymous

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 Changed 7 years ago by Karen Tracey <kmtracey@…>

  • Keywords nfa-someday added

comment:11 Changed 7 years ago by Martin Diers <martin@…>

  • Version changed from newforms-admin to SVN

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.

Changed 7 years ago by Martin Diers <martin@…>

Much less intrusive patch to address this problem.

comment:12 Changed 7 years ago by Martin Diers <martin@…>

  • Keywords someday added; nfa-someday removed

comment:13 Changed 7 years ago by Martin Diers <martin@…>

  • Component changed from Template system to Admin interface

comment:14 Changed 7 years ago by brosner

  • 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 Changed 7 years ago by Martin Diers <martin@…>

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.

Changed 7 years ago by Martin Diers <martin@…>

Update of previous patch - adding result object to dictionary

comment:16 Changed 7 years ago by korpios

  • Cc korpios@… removed

comment:17 Changed 7 years ago by exogen@…

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

comment:18 Changed 7 years ago by mwdiers

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

comment:20 in reply to: ↑ 19 ; follow-up: Changed 6 years ago by 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.

comment:21 in reply to: ↑ 20 Changed 5 years ago by sveri

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:22 Changed 5 years ago by anonymous

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

comment:23 Changed 5 years ago by anonymous

  • Resolution wontfix deleted
  • Status changed from closed to reopened

comment:24 Changed 4 years ago by julien

  • Type set to Cleanup/optimization

comment:25 Changed 4 years ago by julien

  • Severity set to Normal

comment:26 Changed 4 years ago by Alex

  • Easy pickings unset
  • Triage Stage changed from Design decision needed to Accepted
  • 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 Changed 3 years ago by kmike

  • Cc kmike84@… added

comment:28 Changed 2 years ago by lrekucki

  • Patch needs improvement set

Needs a rebase to master.

comment:29 Changed 2 years ago by aaugustin

  • Status changed from reopened to new

comment:30 Changed 10 months ago by aaugustin

  • Summary changed from [newforms-admin] Customization-unfriendly admin template tags should die to Remove customization-unfriendly admin template tags

(Aggressive titles don't help.)

comment:31 Changed 2 months ago by collinanderson

  • Cc cmawebsite@… added
Note: See TracTickets for help on using tickets.
Back to Top