Added signal hook for deleted review requests.

Review Request #8089 — Created April 4, 2016 and discarded

Information

Review Board
master

Reviewers

Added signal hook for deleted review requests.

We use signal hooks for building an upgrade compatible database representation of the state of Review Board that we are interested in. This involves us tracking when a reply/review/request is created and so forth. There becomes a disconnect between the internal representation on Review Board and our external store when a review request gets deleted.

Thus, we internally added tracking for when a review request gets deleted (often by a Review Board admin).

Wired up an extension to test this use case.

We are currently running this with an extension on a staging server.

Ran the unit tests.

Description From Last Updated

This is an extra line that can be removed.

daviddavid

Please add one more blank line here.

daviddavid
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/signals.py
        reviewboard/reviews/models/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/signals.py
        reviewboard/reviews/models/review_request.py
    
    
  2. 
      
david
  1. Thanks for this--it looks pretty good. One trivial comment:

  2. reviewboard/reviews/signals.py (Diff revision 1)
     
     
    Show all issues

    This is an extra line that can be removed.

  3. 
      
JA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/signals.py
        reviewboard/reviews/models/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/signals.py
        reviewboard/reviews/models/review_request.py
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/reviews/signals.py (Diff revision 2)
     
     
    Show all issues

    Please add one more blank line here.

    1. Done! Curious, what is the code pattern here? Noticed that there are some signals with 2 lines in between and some with 1 in this file?

  3. 
      
chipx86
  1. Django includes a built-in signal for model deletions, which you can use to listen to deleted review requests. Can't that be used instead? Deleting review requests is a superuser thing, and not common behavior that a lot of code will need to consider.

    1. To give an example here, today you can do:

      from django.db.models.signals import post_delete
      from reviewboard.reviews.models import ReviewRequest
      
      
      post_delete.connect(my_callback, sender=ReviewRequest)
      
    2. Thanks for the feedback Christian, we felt that it would be helpful to extend the current set of common signals (we have a fair number of admins due to org size) to abstract the need to identify the appropriate models but feel free to close this one out if you believe that should be the recommended practice!

    3. In this particular case, I think the built-in one is sufficient. Deletion is already an uncommon operation only allowed by superusers (and generally shouldn't be used in place of discard for most things, hence superuser only), and since we have a signal today, I don't want to risk confusion as to which signal should be used.

      There are cases that Django's signal will handle that ours cannot not. post_delete will be called when doing something like:

      ReviewRequest.objects.filter(submitter__username='somespammer').delete()
      

      Those will make their way to post_delete, but not ours. Which is maybe okay, but it means the built-in one is going to be more accurate anyway.

  2. 
      
JA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/signals.py
        reviewboard/reviews/models/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/signals.py
        reviewboard/reviews/models/review_request.py
    
    
  2. 
      
david
Review request changed

Status: Discarded

Loading...