Opened 13 years ago

Closed 8 years ago

Last modified 20 months ago

#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 Yeago, 13 years ago

Cc: Yeago added

comment:2 by Carl Meyer, 13 years ago

Triage Stage: UnreviewedDesign decision needed

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.

comment:3 by Adrian Holovaty, 13 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?

comment:4 by Yeago, 13 years ago

What if the delete button wasn't an element of the form but rather a button?

in reply to:  4 comment:5 by Carl Meyer, 13 years ago

Component: django.contrib.adminDocumentation
Triage Stage: Design decision neededAccepted

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 Yeago, 13 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 Alex Gaynor, 13 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 Yeago, 13 years ago

or a 'do you wish to navigate away from this page' alert... or... or... or...

comment:9 by anonymous, 13 years ago

Cc: subsume@… jdunck@… added; Yeago removed

comment:10 by Ubercore, 13 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 Russell Keith-Magee, 13 years ago

Triage Stage: AcceptedDesign 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 Gabriel Hurley, 13 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...

  1. delete the associated Answers,
  2. re-assign the associated Answers to another Question,
  3. 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 Carl Meyer, 13 years ago

Component: Documentationdjango.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 Julien Phalip, 13 years ago

Severity: Normal
Type: Bug

comment:15 by Julien Phalip, 13 years ago

Easy pickings: unset
UI/UX: unset

Related issue: #13106.

comment:16 by Julien Phalip, 13 years ago

UI/UX: set

comment:17 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

comment:18 by Aymeric Augustin, 11 years ago

Triage Stage: Design decision neededAccepted

Django now has configurable cascade behavior (#7539). Moving back to accepted, based on the latest comments by Carl and Gabriel.

comment:19 by ris, 9 years ago

Cc: bugs@… added

comment:20 by ris, 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 Tim Graham, 8 years ago

Resolution: duplicate
Status: newclosed

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 Carsten Fuchs, 20 months ago

Cc: Carsten Fuchs added
Note: See TracTickets for help on using tickets.
Back to Top