Opened 15 years ago
Closed 13 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 , 15 years ago
| Needs documentation: | set |
|---|---|
| Needs tests: | set |
| Triage Stage: | Unreviewed → Design decision needed |
comment:2 by , 15 years ago
| Severity: | → Normal |
|---|
comment:3 by , 15 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 , 14 years ago
| Type: | Uncategorized → New feature |
|---|
comment:7 by , 13 years ago
| Cc: | added |
|---|
comment:8 by , 13 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
comment:9 by , 13 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.