Opened 6 years ago

Closed 4 years ago

Last modified 4 years ago

#10889 closed (fixed)

ModelAdmin calls .log_deletion after deletion, causing invalid object_id

Reported by: jdunck 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 jdunck 6 years ago.
admin-log-deletion.diff (2.3 KB) - added by 3point2 5 years ago.

Download all attachments as: .zip

Change History (12)

Changed 6 years ago by jdunck

comment:1 Changed 6 years ago by jacob

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 6 years ago by Alex

  • Component changed from Contrib apps to django.contrib.admin

comment:3 Changed 6 years ago by ramiro

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 6 years ago by jdunck

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

  • Triage Stage changed from Accepted to Ready for checkin

Nah, this is fine as is.

comment:6 follow-up: Changed 6 years ago by jacob

  • Resolution set to fixed
  • Status changed from new to closed

Fixed in [10686].

comment:7 in reply to: ↑ 6 Changed 6 years ago by kmtracey

Replying to jacob:

Fixed in [10686].

And r10720 for 1.0.X

comment:8 Changed 5 years ago by 3point2

  • milestone changed from 1.1 to 1.3
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Ready for checkin to Design decision needed
  • Version changed from 1.1-beta-1 to SVN

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

comment:9 Changed 4 years ago by ramiro

  • milestone changed from 1.3 to 1.1
  • Resolution set to fixed
  • Status changed from reopened to closed
  • Triage Stage changed from Design decision needed to Ready for checkin
  • Version changed from SVN to 1.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 4 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 deleted

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