Opened 15 years ago

Closed 14 years ago

Last modified 12 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

Description

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 by Matias Surdi, 15 years ago

Cc: matiassurdi@… added

comment:2 by Russell Keith-Magee, 15 years ago

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 by Alex Gaynor, 15 years ago

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 by Karen Tracey, 15 years ago

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 by Alex Gaynor, 15 years ago

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 by Karen Tracey, 15 years ago

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 by runeh, 15 years ago

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

comment:8 by motiejus, 15 years ago

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 by Matias Surdi, 15 years ago

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 by Jacob, 15 years ago

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 by Gonzalo Saavedra, 14 years ago

Cc: Gonzalo Saavedra added

comment:12 by mrts, 14 years ago

Quoting contrib/admin/actions.py:

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

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)
                obj.delete()
                modeladmin.log_deletion(request, obj, obj_display)

comment:13 by anonymous, 14 years ago

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/actions.py

the problem is in contrib/admin/options.py:701 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:

to

        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 by Karen Tracey, 14 years ago

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

comment:15 by Jacob, 14 years ago

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 by Jacob, 12 years ago

milestone: 1.2

Milestone 1.2 deleted

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