Code

Opened 5 years ago

Closed 4 years ago

Last modified 3 years ago

#10751 closed (duplicate)

Admin actions not calling model's delete() method

Reported by: msurdi Owned by: nobody
Component: contrib.admin Version: master
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: UI/UX:

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.

Attachments (0)

Change History (16)

comment:1 Changed 5 years ago by msurdi

  • Cc matiassurdi@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 5 years ago by russellm

  • Triage Stage changed from Unreviewed to Design 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 5 years ago by Alex

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 5 years ago by kmtracey

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

  • Component changed from Uncategorized to 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 Changed 5 years ago by kmtracey

  • milestone set to 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 5 years ago by runeh

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

comment:8 Changed 5 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 5 years ago by msurdi

  • 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 5 years ago by jacob

  • milestone changed from 1.1 to 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 Changed 4 years ago by gonz

  • Cc gonz added

comment:12 Changed 4 years ago by mrts

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

  • Cc yonsy@… added; gonz 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 Changed 4 years ago by kmtracey

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

comment:15 Changed 4 years ago by jacob

  • Resolution set to duplicate
  • Status changed from new to 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.

comment:16 Changed 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


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

 
Note: See TracTickets for help on using tickets.