Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Send post_delete signals immediately #642

Merged
merged 5 commits into from Jan 14, 2013

Conversation

Projects
None yet
3 participants
Contributor

dcramer commented Jan 11, 2013

In a normal relational construct, if you're listening for an event that signals a child was deleted, you dont expect that the parent was deleted already.

This change ensures that post_delete signals are fired immediately after objects are deleted in the graph.

FWIW we need this at Disqus because we use application triggers to do various things.

Our construct is like:

User:
  profile = ForeignKey(UserProfile)

Comment:
  user = ForeignKey(User)

Comment.post_delete -> publish that a message was deleted, by author,
  referencing something other than the id of the author.

In Django 1.2 this works fine, but in 1.4 the author has already been removed from the database when the Comment post_delete trigger happens.

@dcramer dcramer and 1 other commented on an outdated diff Jan 11, 2013

AUTHORS
@@ -131,6 +131,7 @@ answer newbie questions, and generally made Django that much better:
Sengtha Chay <sengtha@e-khmer.com>
ivan.chelubeev@gmail.com
Bryan Chow <bryan at verdjn dot com>
+ David Cramer <dcramer@gmail.com>
@dcramer

dcramer Jan 11, 2013

Contributor

scumbags never adding me :(

@alex

alex Jan 11, 2013

Member

This isn't alphabetized correctly :)

@alex alex commented on an outdated diff Jan 11, 2013

tests/modeltests/delete/tests.py
@@ -229,6 +229,18 @@ def log_pre_delete(sender, **kwargs):
models.signals.post_delete.disconnect(log_post_delete)
models.signals.post_delete.disconnect(log_pre_delete)
+ def test_relational_post_delete_signals_happen_before_parent_object(self):
+ def log_post_delete(instance, **kwargs):
+ self.assertTrue(R.objects.filter(pk=instance.r_id))
+
+ models.signals.post_delete.connect(log_post_delete, sender=S)
+
+ r = R.objects.create(pk=1)
+ S.objects.create(pk=1, r=r)
+ r.delete()
+
+ models.signals.post_delete.disconnect(log_post_delete)
@alex

alex Jan 11, 2013

Member

This should be in a finally block so it gets disonnected even if it fails

Member

alex commented Jan 12, 2013

Sorry caused a merge conflict here :)

@alex alex commented on an outdated diff Jan 12, 2013

tests/modeltests/delete/tests.py
@@ -229,6 +229,20 @@ def log_pre_delete(sender, **kwargs):
models.signals.post_delete.disconnect(log_post_delete)
models.signals.post_delete.disconnect(log_pre_delete)
+ def test_relational_post_delete_signals_happen_before_parent_object(self):
+ def log_post_delete(instance, **kwargs):
+ self.assertTrue(R.objects.filter(pk=instance.r_id))
@alex

alex Jan 12, 2013

Member

I'd prefer it if this function actually did something something and then the test function made an assertion about it after r.delete(), as is if this function weren't called for some reason the etst would still pass.

Contributor

dcramer commented Jan 14, 2013

I'll get the various things addressed sometime today or tomorrow.

dcramer added some commits Jan 11, 2013

@dcramer dcramer Send post_delete signals immediately
In a normal relational construct, if you're listening for an event
that signals a child was deleted, you dont expect that the parent
was deleted already.

This change ensures that post_delete signals are fired immediately
after objects are deleted in the graph.
272de9e
@dcramer dcramer Add David Cramer to AUTHORS d53e3b1
@dcramer dcramer Move signal disconnect into finally block 6045efa
@dcramer dcramer Move logic seperation as its not longer repetitive abbb888
Contributor

dcramer commented Jan 14, 2013

Should be good to go. I can squash if you need me to.

@alex alex added a commit that referenced this pull request Jan 14, 2013

@alex alex Merge pull request #642 from dcramer/patch-1
Send post_delete signals immediately
c346d35

@alex alex merged commit c346d35 into django:master Jan 14, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment