Code

Opened 6 years ago

Closed 6 years ago

Last modified 3 years ago

#7553 closed (fixed)

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

Reported by: Mnewman Owned by: brosner
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 Mnewman 6 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 Mnewman 6 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 Mnewman 6 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 Mnewman 6 years ago.
Updated based on brosner's comments

Download all attachments as: .zip

Change History (14)

Changed 6 years ago by Mnewman

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

comment:1 Changed 6 years ago by garcia_marc

  • milestone changed from 1.0 alpha to 1.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 6 years ago by brosner

  • Keywords nfa-blocker added
  • milestone changed from 1.0 to 1.0 alpha
  • Owner changed from Mnewman to brosner
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted

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 6 years ago by Alex

  • Component changed from Contrib apps to Admin interface

comment:4 Changed 6 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 6 years ago by Mnewman

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

Changed 6 years ago by Mnewman

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

comment:5 Changed 6 years ago by Mnewman

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 6 years ago by Mnewman

  • Cc newmaniese@… added

comment:7 Changed 6 years ago by CollinAnderson

  • Cc cmawebsite@… added

comment:8 Changed 6 years ago by brosner

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

Changed 6 years ago by Mnewman

Updated based on brosner's comments

comment:9 Changed 6 years ago by brosner

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

(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 3 years ago by jacob

  • milestone 1.0 alpha deleted

Milestone 1.0 alpha deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.