Opened 8 years ago

Closed 6 years ago

Last modified 5 years ago

#10889 closed (fixed)

ModelAdmin calls .log_deletion after deletion, causing invalid object_id

Reported by: Jeremy Dunck Owned by: nobody
Component: contrib.admin Version: 1.1-beta
Severity: Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

When an object is deleted, its PK value is set to None.
Unfortunately, ModelAdmin calls log_deletion (which results in a LogEntry) after object.delete, so that LogEntry's with DELETION action_flag have u'None' as their PK.

This is less than useful, since I sometimes what to trace the full history of an object through LogEvent, and object_repr isn't really enough to do that.

Attachments (2)

10889.diff (2.0 KB) - added by Jeremy Dunck 8 years ago.
admin-log-deletion.diff (2.3 KB) - added by 3point2 6 years ago.

Download all attachments as: .zip

Change History (12)

Changed 8 years ago by Jeremy Dunck

Attachment: 10889.diff added

comment:1 Changed 8 years ago by Jacob

Triage Stage: UnreviewedAccepted

comment:2 Changed 8 years ago by Alex Gaynor

Component: Contrib appsdjango.contrib.admin

comment:3 Changed 8 years ago by Ramiro Morales

Only (minor?) problem with moving the log_deletion() call to above the actual deletion is this might leave the log event with inaccurate information (reporting a successful action) if the obj.delete() fails for some reason and throws an exception.

comment:4 Changed 8 years ago by Jeremy Dunck

ramiro,

Alex and I discussed that-- unfortunately, I don't think it can be easily fixed, because .log_deletion is a public method of ModelAdmin, which may have been subclassed. log_deletion takes the object being deleted as a parameter, not just the object PK, so the only way I can see reversing this would be some hackery like:

old_obj_pk = obj.pk
obj.delete()
new_obj_pk = obj.pk
try:
   obj.pk = old_obj_pk
   self.log_deletion(...,obj,...)
finally:
   obj.pk = new_obj_pk

This strikes me as more than a little gross.

Opinions?

comment:5 Changed 8 years ago by Jacob

Triage Stage: AcceptedReady for checkin

Nah, this is fine as is.

comment:6 Changed 8 years ago by Jacob

Resolution: fixed
Status: newclosed

Fixed in [10686].

comment:7 in reply to:  6 Changed 8 years ago by Karen Tracey

Replying to jacob:

Fixed in [10686].

And r10720 for 1.0.X

comment:8 Changed 6 years ago by 3point2

milestone: 1.11.3
Resolution: fixed
Status: closedreopened
Triage Stage: Ready for checkinDesign decision needed
Version: 1.1-beta-1SVN

I recently discovered I was getting log messages for failed deletions. I'd like to suggest a different approach and am attaching a patch: log_deletion shouldn't need the deleted object. any information needed for the log message should be recorded prior to calling delete() and then be passed to log_deletion afterward. This means changing the arguments accepted by log_deletion. It's only called in one other place in within Django, which I have included in the patch (the delete_selected action in actions.py).

Although log_deletion isn't a documented method, I'm not sure how the developers will feel about changing its signature. If not now, perhaps in 1.3?

Note that #14017 suggests changing a code comment to reflect the current functionality, and the patch there will conflict with the one I'm uploading.

Changed 6 years ago by 3point2

Attachment: admin-log-deletion.diff added

comment:9 Changed 6 years ago by Ramiro Morales

milestone: 1.31.1
Resolution: fixed
Status: reopenedclosed
Triage Stage: Design decision neededReady for checkin
Version: SVN1.1-beta-1

(restoring ticket properties values)

@3point2:

Please don't reopen tickets closed by a commit fixing the issue reported by them.

If you want to propose a different approach to a desing decision already made, please open a new ticket and/or open a new django-dev thread.

comment:10 Changed 5 years ago by Jacob

milestone: 1.1

Milestone 1.1 deleted

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