Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#15008 closed Cleanup/optimization (fixed)

Convert admin views to use TemplateResponse

Reported by: acdha Owned by: acdha
Component: contrib.admin Version: master
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:

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 acdha 4 years ago.
Patch against r16086

Download all attachments as: .zip

Change History (16)

comment:1 Changed 4 years ago by acdha

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

comment:2 Changed 4 years ago by acdha

  • 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 follow-up: Changed 4 years ago by jezdez

  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

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.

comment:4 in reply to: ↑ 3 Changed 4 years ago by acdha

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

  • Owner changed from nobody to acdha
  • Status changed from new to assigned

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 4 years ago by acdha (previous) (diff)

comment:6 Changed 4 years ago by acdha

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

comment:7 Changed 4 years ago by kmike

  • Cc kmike84@… added

comment:8 Changed 4 years ago by carljm

  • Cc carl@… added

comment:9 Changed 4 years ago by anonymous

  • milestone 1.3 deleted
  • Severity set to Normal
  • Type set to Cleanup/optimization

comment:10 Changed 4 years ago by acdha

  • 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

Changed 4 years ago by acdha

Patch against r16086

comment:11 Changed 4 years ago by jezdez

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

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

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

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.

comment:14 in reply to: ↑ 13 Changed 4 years ago by acdha

  • 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 Changed 4 years ago by jezdez

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