#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 , 16 years ago
Cc: | added |
---|
comment:2 by , 16 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
comment:3 by , 16 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 , 16 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 , 16 years ago
Component: | Uncategorized → django.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 , 16 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:8 by , 16 years ago
Cc: | 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 , 16 years ago
Cc: | 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 , 16 years ago
milestone: | 1.1 → 1.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 , 15 years ago
Cc: | added |
---|
comment:12 by , 15 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 , 15 years ago
Cc: | added; 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 , 15 years ago
This last comment is referring to a different problem, recently introduced, see #12962.
comment:15 by , 15 years ago
Resolution: | → duplicate |
---|---|
Status: | new → closed |
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.
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.