Opened 8 years ago

Closed 8 years ago

Last modified 5 years ago

#7553 closed (fixed)

return None in sites.py doesn't work with decorators

Reported by: Michael Newman Owned by: Brian Rosner
Component: contrib.admin Version: newforms-admin
Severity: Keywords: admin nfa-blocker
Cc: newmaniese@…, cmawebsite@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Simply, adding the cache to the login view in the new admin broke the way it handled persistent data. http://buildbot.djangoproject.com/newforms-admin%202.4%20sqlite/builds/237/step-test/0

Attachments (4)

7553.diff (696 bytes) - added by Michael Newman 8 years ago.
Simple patch that uses the request path to get the response from model_page instead
7553-sites-decorators-excpt.diff (3.7 KB) - added by Michael Newman 8 years ago.
A proof-of-concept patch using an exception to catch data that may fall through.
7553-sites-decorators-ref.diff (3.6 KB) - added by Michael Newman 8 years ago.
Another way to fix this problem using refactored code, without a try and except. Still not the cleanest.
7553-sites-decorators-ref.2.diff (2.5 KB) - added by Michael Newman 8 years ago.
Updated based on brosner's comments

Download all attachments as: .zip

Change History (14)

Changed 8 years ago by Michael Newman

Attachment: 7553.diff added

Simple patch that uses the request path to get the response from model_page instead

comment:1 Changed 8 years ago by Marc Garcia

milestone: 1.0 alpha1.0
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

According to ticket organization defined in http://code.djangoproject.com/wiki/VersionOneRoadmap#how-you-can-help 1.0 alpha tickets should be just features in the Must have (http://code.djangoproject.com/wiki/VersionOneRoadmap#must-have-features) list.

As bug, it should be fixed before 1.0 milestone.

comment:2 Changed 8 years ago by Brian Rosner

Keywords: nfa-blocker added
milestone: 1.01.0 alpha
Owner: changed from Michael Newman to Brian Rosner
Status: newassigned
Triage Stage: UnreviewedAccepted

This is broken functionality since [7737]. It needs to block the merge. Tagging as so and milestone for 1.0 alpha. I will have it fixed shortly.

comment:3 Changed 8 years ago by Alex Gaynor

Component: Contrib appsAdmin interface

comment:4 Changed 8 years ago by Simon Willison

I fixed this in [7824] without realising there was a ticket for it. brosner: you should check that my fix is appropriate (the fix in the patch attached to this ticket may be a better option).

Changed 8 years ago by Michael Newman

A proof-of-concept patch using an exception to catch data that may fall through.

Changed 8 years ago by Michael Newman

Another way to fix this problem using refactored code, without a try and except. Still not the cleanest.

comment:5 Changed 8 years ago by Michael Newman

Here are two patches which fix this problem. One is raising an exception from the view, that can be caught when there is Post Data that might be lost and the second reuses the root function with the self.root_url variable that is available to just return the root function. I prefer the later (Catching a manufactured exception is a little ... well...). Simon, your revision fixes the problem, but Brian and I decided that this needs to be fixed in the admin and that the views shouldn't be changed to handle the admin. Also, login is a view function, except that one time when it returns None and it becomes just another function.

comment:6 Changed 8 years ago by Michael Newman

Cc: newmaniese@… added

comment:7 Changed 8 years ago by Collin Anderson

Cc: cmawebsite@… added

comment:8 Changed 8 years ago by Brian Rosner

What is the reason for moving delete_test_cookie and removing the is_active check?

Changed 8 years ago by Michael Newman

Updated based on brosner's comments

comment:9 Changed 8 years ago by Brian Rosner

Resolution: fixed
Status: assignedclosed

(In [7933]) newforms-admin: Fixed #7553 -- Reverted [7824] in favor of a better fix in #7553. The never_cache decorator is no longer special casing None. Thanks Michael Newman for the patch.

comment:10 Changed 5 years ago by Jacob

milestone: 1.0 alpha

Milestone 1.0 alpha deleted

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