Code

Opened 4 years ago

Last modified 16 months ago

#14411 new Bug

Inline delete not prompting cascade delete warning

Reported by: subsume Owned by: nobody
Component: contrib.admin Version: 1.2
Severity: Normal Keywords:
Cc: subsume@…, jdunck@… 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.

Attachments (0)

Change History (18)

comment:1 Changed 4 years ago by subsume

  • Cc subsume added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 4 years ago by carljm

  • Triage Stage changed from Unreviewed to Design 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 Changed 4 years ago by adrian

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 follow-up: Changed 4 years ago by subsume

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

comment:5 in reply to: ↑ 4 Changed 4 years ago by carljm

  • Component changed from django.contrib.admin to Documentation
  • Triage Stage changed from Design decision needed to 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 Changed 4 years ago by subsume

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

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 Changed 4 years ago by subsume

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

comment:9 Changed 4 years ago by anonymous

  • Cc subsume@…, jdunck@… added; subsume removed

comment:10 Changed 4 years ago by Ubercore

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 Changed 4 years ago by russellm

  • Triage Stage changed from Accepted to 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 Changed 4 years ago by gabrielhurley

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 Changed 4 years ago by carljm

  • Component changed from Documentation to 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 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:15 Changed 3 years ago by julien

  • Easy pickings unset
  • UI/UX unset

Related issue: #13106.

comment:16 Changed 3 years ago by julien

  • UI/UX set

comment:17 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:18 Changed 16 months ago by aaugustin

  • Triage Stage changed from Design decision needed to Accepted

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


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

 
Note: See TracTickets for help on using tickets.