#14411 closed Bug (duplicate)
Inline delete not prompting cascade delete warning
Reported by: | Yeago | Owned by: | nobody |
---|---|---|---|
Component: | contrib.admin | Version: | 1.2 |
Severity: | Normal | Keywords: | |
Cc: | subsume@…, jdunck@…, bugs@…, Carsten Fuchs | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | yes |
Description
Models and admin can befound here: http://dpaste.com/hold/254060/
In this situation, no cascading delete warning is given when deleting questions from questionnaires in admin, resulting in the loss of all answer objects associated with the question by an FK.
Setting milestone due to my perceived severity. Please undo if necessary.
Change History (22)
comment:1 by , 14 years ago
Cc: | added |
---|
comment:2 by , 14 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
comment:3 by , 14 years ago
carljm explained the difficulty well. I don't see an elegant code fix here, unfortunately, which makes me believe documenting it clearly is the best way to go. Who can write up some docs?
follow-up: 5 comment:4 by , 14 years ago
What if the delete button wasn't an element of the form but rather a button?
comment:5 by , 14 years ago
Component: | django.contrib.admin → Documentation |
---|---|
Triage Stage: | Design decision needed → Accepted |
Replying to subsume:
What if the delete button wasn't an element of the form but rather a button?
Then we trade this problem for an alternative that's just as bad: potential loss of all the work you may have put into editing other fields on the page before you click that Delete button.
Accepting and setting component to Documentation.
comment:6 by , 14 years ago
Sorry, that seems like an easy fix that allows us to stamp 'TOLDYASO' on situations where serious data loss can occur. Pressing a back-button usually will save your work within a form. I don't think users are often inclined to click links away from forms they are editing and its rather easy to make it obviously not part of the form. We could even have it open in a new window if that's really a huge problem.
comment:7 by , 14 years ago
Possibly solution: turn it into a button, have the button do a popup raw-id-fields style and let it prompt fro deleting, then on completion it can remove the item from the inline.
comment:8 by , 14 years ago
or a 'do you wish to navigate away from this page' alert... or... or... or...
comment:9 by , 14 years ago
Cc: | added; removed |
---|
comment:10 by , 14 years ago
None of the options are perfect, agreed, but I'd rather see a solution that minimizes data loss. Better to document that a user may lose changed data on the admin form than document that a user may lose an entire table's worth of data to a cascading delete. A failure on the user's part to heed the warning is far less catastrophic.
comment:11 by , 14 years ago
Triage Stage: | Accepted → Design decision needed |
---|
IMHO, I don't think documentation alone is the right solution here. We're talking about potentially catastrophic data loss that can occur fairly easily. We don't get a free pass by putting a line in the docs that explains, after the fact, why your entire database was just deleted. Even if we assume people completely read and comprehend the docs, it's trivial for someone who didn't set up the inline to add a foreign key to a model and get massive data loss as a result.
I don't know that I have a good UI solution, but one option at a programmatic level would be to make the behavior specifically opt-in. As an admin validation activity, we are able to inspect whether an inline model has foreign keys that would be affected by a cascading delete. We could set the default behavior to raise a validation error and refuse to allow the admin to start. We could then provide an an opt-in option on the inline that requires you to explicitly nominate that "yes, it's ok for these foreign keys to be cascade deleted as inlines".
This makes inlines safe-by-default, it forces the user explicitly opt-in to any data loss that may occur, and it provides a good reason for people to hunt down the relevant documentation for an explanation of the problem -- first time they see the error, they're going to go looking for the docs on the option for their inline.
comment:12 by , 14 years ago
The opt-in solution might be a decent stopgap measure, though it still doesn't solve the "naive end-user" problem once the developer has opted in.
In the long-term, this does seem like a problem of an implicit behavior which ought to be explicitly user-controlled. Offering an idea somewhat like Alex's, stated in terms of the OP's models:
You click to delete the Question from the Questionnaire, and you are presented a dialog (or intermediate page) with the following options...
- delete the associated Answers,
- re-assign the associated Answers to another Question,
- set their FKs to null (if they are nullable),
It doesn't seem like either the backend code or front-end UI should be terribly complex. This seems to offer a maximum amount of user-control and a minimum amount of potentially catastrophic behavior happening automatically.
Either way, I agree that docs alone aren't the answer here.
comment:13 by , 14 years ago
Component: | Documentation → django.contrib.admin |
---|
I hadn't thought of the admin validation warning or overridable error; that seems like a good idea. Serves the same purpose as docs, but much more effective. Particularly if we can land #7539, which would allow another route around the warning; just make your FK cascade-set-null (or cascade-set-default) instead of cascade-delete.
If a developer explicitly wants a cascade-deleting inline, they can implement any of the end-user warning options proposed here outside Django via ModelAdmin customization. A really good generic solution for that warning could be built-in; my concern there is Hippocratic ("do no harm"). In other words, IMO the bar for including such a warning is that it have zero usability regression for the inline-deletion UI in the common case (no dependent models, no warning needed), not introduce confusing inconsistencies where some inlines have one deletion UI and others another, and generally be consistent with how the rest of the admin UI works. I don't think any of the warning solutions proposed thus far have a very good chance of meeting those criteria -- there are many devils lurking in the details; I can go into more detail on what I think some of them are in a mailing-list thread if there's interest -- but in any case I'd be happy to be proven wrong, preferably in code.
comment:14 by , 13 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:16 by , 13 years ago
UI/UX: | set |
---|
comment:18 by , 12 years ago
Triage Stage: | Design decision needed → Accepted |
---|
Django now has configurable cascade behavior (#7539). Moving back to accepted, based on the latest comments by Carl and Gabriel.
comment:19 by , 9 years ago
Cc: | added |
---|
comment:20 by , 9 years ago
Yeah, we just got bitten by this yesterday. Staff member managed to delete quite a bit of data without warning.
comment:21 by , 9 years ago
Resolution: | → duplicate |
---|---|
Status: | new → closed |
The dpaste that contained the models in the report are gone, but it looks to me like this is a duplicate of #12382.
comment:22 by , 2 years ago
Cc: | added |
---|
There's no obvious fix here; the normal dedicated cascade-delete warning page can't be used in the case of deleting an inline, because deleting an inline isn't all you're doing: you may be editing the main model and other inlines, adding new inlines... trying to preserve all that state across an extra request to the warning page would be quite fragile.
Two possibilities I can think of: either a documentation note that this is simply the risk you take if you use inlines for a model with an FK pointing to it (not the common case anyway), or prepopulate the page with counts of dependent objects for each inline and use that data to provide some kind of JS warning. Not very fond of the latter option: it doesn't scale well to pathological cases with trees of multiple dependent models; also JS warnings are irritating and people are likely to click OK before they think anyway.
So on the whole I think I'd lean towards just documenting this, unless someone has a brilliant solution I've overlooked. Setting to DDN.