Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#15008 closed Cleanup/optimization (fixed)

Convert admin views to use TemplateResponse

Reported by: Chris Adams Owned by: Chris Adams
Component: contrib.admin Version: dev
Severity: Normal Keywords: templateresponse
Cc: kmike84@…, carl@…, chris@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

A number of internal projects and several opensource projects have needed substantial amounts of boilerplate code when customizing Django admin views. https://github.com/toastdriven/django-haystack/blob/master/haystack/admin.py illustrates the problem nicely: to customize the search logic requires duplicating most of the standard changelist view rather than simply changing the "cl" context variable.

1.3 adds TemplateResponse which is perfect for this situation and I have a github branch which changes the contrib.auth and contrib.admin views to use TemplateResponse instead of render_to_response:

https://github.com/acdha/django/compare/master...admin-template-response

The attached patch is a copy of the Github patch view:

https://github.com/acdha/django/compare/master...admin-template-response.patch

Attachments (1)

admin-template-response.patch (24.7 KB ) - added by Chris Adams 13 years ago.
Patch against r16086

Download all attachments as: .zip

Change History (16)

comment:1 by Chris Adams, 13 years ago

Patch needs improvement: set

comment:2 by Chris Adams, 13 years ago

Needs documentation: set
Patch needs improvement: unset

Corrected patch; set needs-documentation since there should be some indication that subclassing admin views is now a lot easier

comment:3 by Jannis Leidel, 13 years ago

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

I definitely think that's a good idea but don't like we have to create RequestContexts manually to pass the current app. I suggest opening a separate ticket that adds a current_app parameter to the TemplateResponse which would pass that to the dynamically created RequestContext.

in reply to:  3 comment:4 by Chris Adams, 13 years ago

Replying to jezdez:

I definitely think that's a good idea but don't like we have to create RequestContexts manually to pass the current app. I suggest opening a separate ticket that adds a current_app parameter to the TemplateResponse which would pass that to the dynamically created RequestContext.

Just created #15010 and corresponding branch: https://github.com/acdha/django/compare/master...template-response-current_app. I think this is reasonable but was debating whether it'd be better to allow for future use by accepting a dict of RequestContext parameters; that felt like overkill.

comment:5 by Chris Adams, 13 years ago

Owner: changed from nobody to Chris Adams
Status: newassigned

I've updated the attached patch against #15008; it now avoids creating RequestContexts unnecessarily, which allowed some import cleanup in several cases.

https://github.com/acdha/django/compare/master...15008-admin-template-response

Last edited 13 years ago by Chris Adams (previous) (diff)

comment:6 by Chris Adams, 13 years ago

Updated the patch against current trunk and added some tests to confirm that the views are returning TemplateResponse instances

comment:7 by Mikhail Korobov, 13 years ago

Cc: kmike84@… added

comment:8 by Carl Meyer, 13 years ago

Cc: carl@… added

comment:9 by anonymous, 13 years ago

milestone: 1.3
Severity: Normal
Type: Cleanup/optimization

comment:10 by Chris Adams, 13 years ago

Easy pickings: unset

I've updated the patch against the current trunk (note the new git branch name with the ticket number):

https://github.com/acdha/django/compare/master...15008-admin-template-response

by Chris Adams, 13 years ago

Patch against r16086

comment:11 by Jannis Leidel, 13 years ago

Resolution: fixed
Status: assignedclosed

In [16087]:

Fixed #15008 -- Replaced all calls in the admin to render_to_response with TemplateResponses for easier customization. Thanks to Chris Adams for the initial patch.

comment:12 by Luke Plant, 13 years ago

I'm about to fix to #16004, and I've realised this will render [16087] almost completely useless, because csrf_protect is applied to all or almost all the views affected here, and the fix will make all those views render the respective TemplateResponse objects, so they can no longer be customised. This is a necessary fix to make the admin CSRF protection (and any other CSRF protection) work in the absence of CsrfViewMiddleware - otherwise you'll get 403 errors. (I'm actually experiencing this in my tests for one project. The tests use Twill, and so do more realistic testing than any of Django's built in tests).

I should note that with my fix, none of the existing tests actually fail, because they don't test at what point the TemplateResponse is rendered.

So, what to do? #16004 is a bug, and it's a serious one that ought to have been fixed in 1.3. This ticket is a feature that has been added since 1.3. That means it loses, as far as I can see. I brought this issue up here: http://groups.google.com/group/django-developers/browse_thread/thread/f96e982254fbe5c3?pli=1

Can the relevant people please respond to this thread? Otherwise I will have to go ahead and fix #16004. Then the only sensible thing would be to revert [16087].

comment:13 by Luke Plant, 13 years ago

@acdha - your admin.py looks very different from the Django's trunk ModelAdmin.changelist_view, so it's very difficult to see exactly what you are needing to change in the admin. In fact I cannot find any released version of Django that looks close to a simply copy-paste job for this.

Because of that, it's extremely difficult to analyse where the admin could be easier to customise for your needs.

However, from your description, it's possible that simply overriding the 'get_changelist' method is all you need to do. This has existed since Django 1.2.

in reply to:  13 comment:14 by Chris Adams, 13 years ago

Cc: chris@… added

Replying to lukeplant:

@acdha - your admin.py looks very different from the Django's trunk ModelAdmin.changelist_view, so it's very difficult to see exactly what you are needing to change in the admin. In fact I cannot find any released version of Django that looks close to a simply copy-paste job for this.

That's not actually my code - it was convenient to reference as a public, popular app which needs to customize the admin - but I've run into many cases where it would have been extremely handy to be able to customize the template context before rendering. In this case it would be easy to refactor these classes to have something like a get_context method to make that specific need easier to handle but it feels like csrf_protect should be able to operate lazily as it would otherwise seem to make TemplateResponse useless for many cases where it would otherwise be a good solution.

Chris

comment:15 by Jannis Leidel, 13 years ago

FTR, I've replied in the #16004 ticket already, this needs a better fix. We can easily fix the problem for 1.3.X, but for trunk we need a better story than rendering TemplateResponse useless.

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