Opened 14 years ago
Closed 12 years ago
#15694 closed New feature (fixed)
Overriding Django admin views and nested transactions
Reported by: | Dave Hall | Owned by: | Aymeric Augustin |
---|---|---|---|
Component: | contrib.admin | Version: | 1.2 |
Severity: | Normal | Keywords: | |
Cc: | Dave Hall, mmitar@… | Triage Stage: | Design decision needed |
Has patch: | no | Needs documentation: | yes |
Needs tests: | yes | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
This problem is related to #2227, but has a potentially easier solution.
Because the Django admin site directly decorates it's methods with @commit_on_success, and @commit_on_success is unaware on nested outer transactions, it's currently impossible to override an admin view and maintain data integrity. This is a problem for django-reversion, which augments many of the django admin methods by wrapping them in a revision. Ideally, I would be able to save the primary model and it's associated revision data in a single transaction. Currently, however, the superclass view commits the transaction and thus forces the associated revision data to be saved in it's own transaction.
Obviously, a satisfactory fix for #2227 would solve this. However, this ticket seems to be a bit dead, due to conflicting ways of solving it.
Fixing the problem of overriding admin views is easier, however. My proposed solution is to move the existing @commit_on_success and @csrf_protect decorators off the methods, and apply them in the admin site under AdminSite.admin_view instead. This would mean they are applied after all subclassing has taken place.
I've implemented a tentative patch here:
https://github.com/etianen/django/commit/6682fc31c810f763f08dcf14505c5762ab1d18a1
Change History (9)
comment:1 by , 14 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
Triage Stage: | Unreviewed → Design decision needed |
comment:2 by , 14 years ago
Severity: | → Normal |
---|
comment:3 by , 14 years ago
Cc: | added |
---|---|
Type: | → Uncategorized |
I'm very happy to write the tests, documentation and the like. Once (if) this gets accepted, I'll get right on it.
comment:4 by , 13 years ago
Type: | Uncategorized → New feature |
---|
comment:7 by , 12 years ago
Cc: | added |
---|
comment:8 by , 12 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:9 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
I think that's a good idea.
However, if this was accepted it still needs tests, documentation and probably a note in the BackwardsIncompatibleChanges page.