Opened 12 years ago

Closed 12 years ago

Last modified 10 years ago

#10751 closed (duplicate)

Admin actions not calling model's delete() method

Reported by: Matias Surdi Owned by: nobody
Component: contrib.admin Version: dev
Severity: Keywords:
Cc: matiassurdi@…, yonsy@… Triage Stage: Design decision needed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


When you delete an object from the admin interface, the delete() method is called on the model. But when deleted many objects at the same time using the default admin action "Delete selected [object name]" this doesn't happen.

I think that for consistency this should happen.

Change History (16)

comment:1 Changed 12 years ago by Matias Surdi

Cc: matiassurdi@… added

comment:2 Changed 12 years ago by Russell Keith-Magee

Triage Stage: UnreviewedDesign decision needed

It's arguable that this is an inconsistency. After all, calling delete() on a queryset _doesn't_ call the delete() method on the model. This is well documented behavior. What we need to decide is whether the admin bulk actions are "delete lots of individual objects", or a "bulk deletion of objects". I'm happy with either outcome, but I'd be happy to hear opinions.

comment:3 Changed 12 years ago by Alex Gaynor

You just referred to it as "admin bulk actions", in fact that's been what it's been refered to for quite a while I think. I believe that stands for itself.

comment:4 Changed 12 years ago by Karen Tracey

I agree it makes sense from the programmer's point of view that bulk delete bypasses individual delete overrides. On the other side, though, I believe there are plenty of users of the admin interface who never have anything to do with writing code, haven't the vaguest idea what a queryset is, and therefore might be rather surprised that bulk delete from admin behaves somewhat differently than individual delete. One might argue that the people writing the code behind that -- who have customized delete() and probably been careful to not use delete() on a queryset if there is something in their custom delete() they want to be sure always happens -- should be responsible for ensuring that bulk delete isn't available from their admin or is customized to do the right thing (can it be? I've done nothing with bulk actions in admin -- could one replace the default delete method with a different one that calls individual deletes?). At a minimum we probably need to mention this in the admin actions doc. If we could provide guidance on how to avoid it, if desired, while still allowing the user the convenience of bulk delete, that would be even better.

comment:5 Changed 12 years ago by Alex Gaynor

Component: Uncategorizeddjango.contrib.admin

Right now the way to avoid it is to write your own delete items action, which frankly I'm in favor of. Though we aren't paragons of efficiency I think we generally try to avoid executing wanton queries.

comment:6 Changed 12 years ago by Karen Tracey

milestone: 1.1

#10754 brought this up again, mentioning that signal handlers aren't called. I think beefing up the doc on admin actions to mention this behavior and indicate how to avoid it would address the issue. Given we've had two people notice this so far it'd be good to get some doc mention for it into 1.1, so I set the milestone for that.

comment:7 Changed 12 years ago by runeh

#10754 said the opposite. Signal handlers are called but not delete()

comment:8 Changed 12 years ago by motiejus

Cc: matiassurdi@… removed

I was about to submit a bug report, but thankfully found this post.

My own models have foreign relations (like external DB tables), and upon deleting a model the foreign items are deleted too.
In this case they are left as garbage. No good.

So anouther vouch for bulk delete.

comment:9 Changed 12 years ago by Matias Surdi

Cc: matiassurdi@… added

I agree with kmtracey, maybe some clarification on the docs about this behaviour an how to override the default admin action with a custom one will do the job. I think that calling delete() on every object will mean a separate SQL query for each row, instead of just one to delete everything and this will be a performance killer.

comment:10 Changed 12 years ago by Jacob

milestone: 1.11.2

I'm punting on the design decision until after 1.1, but #11022 tracks the need for some docs for now.

comment:11 Changed 12 years ago by Gonzalo Saavedra

Cc: Gonzalo Saavedra added

comment:12 Changed 12 years ago by mrts

Quoting contrib/admin/

            for obj in queryset:
                obj_display = force_unicode(obj)
                modeladmin.log_deletion(request, obj, obj_display)

As such, the queryset is already iterated over and the code already behaves a more like "delete lots of individual objects" than "bulk deletion of objects" by logging the individual deletions. As the INSERT in modeladmin.log_deletion() is probably much more costly than obj.delete(), the bulk delete optimization does not give the "dramatic" improvement in efficiency it would without log_deletion().

In my humble opinion, the following is both more natural (and more consistent by logging deletion only after it succeeds):

            for obj in queryset:
                obj_display = force_unicode(obj)
                modeladmin.log_deletion(request, obj, obj_display)

comment:13 Changed 12 years ago by anonymous

Cc: yonsy@… added; Gonzalo Saavedra removed

the problem is not with the delete method, is with the delete action from the admin panel when i enable actions for a model.

the problem is that the action never come to contrib/admin/

the problem is in contrib/admin/ in the response_action method

the problem is that when u call to confirm the action, the request.POST method don't have the 'index' key.

i change the line 701 from

        if 'index' not in request.POST:


        if 'index' not in request.POST and 'post' not in request.POST:

and the problem get corrected

btw, this litle fix correct another serious bug related to this, if i for example use a modelform with editable_fields and use the actions then when i confirm the bug, for some dark magic code in django that get in another dimension in the code, i obtain an strange exception:

[u'ManagementForm data is missing or has been tampered with']

the fix above correct this bug too

i put this one here to some django coder attend this, the fix is very little and correct and severe problem that need to be fixed before 1.2 release

sorry if i post this anonymous i dont have time now, i need to take a bus in a few minutes and find the bug in an app that i am coding with django trunk code

comment:14 Changed 12 years ago by Karen Tracey

This last comment is referring to a different problem, recently introduced, see #12962.

comment:15 Changed 12 years ago by Jacob

Resolution: duplicate
Status: newclosed

Now that the docs are in, I'm closing this as a dup of #11022. Please open another ticket for the other thing, if it's indeed a bug.

comment:16 Changed 10 years ago by Jacob

milestone: 1.2

Milestone 1.2 deleted

Note: See TracTickets for help on using tickets.
Back to Top