ModelAdmin calls .log_deletion after deletion, causing invalid object_id

[Jeremy Dunck]

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.

[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.

[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?

[Jacob]

Nah, this is fine as is.

[Jacob]

Fixed in [10686].

[Karen Tracey]

Replying to jacob:

Fixed in [10686].

And r10720 for 1.0.X

[3point2]

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.

[Ramiro Morales]

(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.

[Jacob]

Milestone 1.1 deleted