Add support for default reviewers

Review Request #169 — Created Oct. 14, 2007 and submitted

Information

Review Board SVN (deprecated)
trunk
30

Reviewers

This is a generic solution to the question of adding default reviewers.
It's generic enough to be useful both for installations like reviews.r-b.o
where there's a single group for all reviews, and the extraordinarily
complex case we've got at VMware, where different groups own different
parts of a single huge codebase.

There's a new model which basically has a regular expression and a list
of default people/groups.  When a new diff is created, it iterates through
the list of filenames in the diff and checks them against each of the
DefaultReviewer regexps.  If one matches, those defaults are added to the
draft's list of target reviewers.
Did a couple tests with different diffs to make sure it added the correct
reviewers.  This might blow up with diffs that contain added or removed
files, I'm not sure.  I'll do some more testing with that before I commit.
chipx86
  1. Sweet. With this, we'll be able to add a --publish option to post-review that just lets you fire and forget a review request in most cases.
  2. trunk/reviewboard/reviews/models.py (Diff revision 1)
     
     
     
     
     
    Even if they change, we could do something about that. Keep a dict of regex -> compiled regex mappings and just check if it's in there, then use the compiled one. Though we'd want to purge this on occasion.
    1. I think for now, I'm going to not try to do this.  If this becomes a bottleneck
      we can optimize it, but a TODO is good enough at the moment.
  3. trunk/reviewboard/reviews/models.py (Diff revision 1)
     
     
     
    I believe calling self.diffset.files.all() each time will cause a database hit. We can optimize this by storing the queryset outside the first for loop.
    
    filediffs = self.diffset.files.all()
    
    for default in DefaultReviewer.objecs.all():
        for filediff in filediffs:
    
    
    filediffs will contain the cached entries, whereas self.diffset.files.all() should hit the database each time the queryset is recreated.
    
    We may want to also compile the regex at the start of the first for loop.
  4. trunk/reviewboard/reviews/models.py (Diff revision 1)
     
     
    This is inefficient for the same reason as above.
    1. I think it actually caches the results, but it doesn't hurt.  Done.
    2. I thought it did from what I read too, but when investigating the memory leak, the only caching I could find was on the MySQL backend side, so I'm not sure this is always the case. The queryset, however, is guaranteed to cache them for the lifetime of the queryset.
      
      Anyhow, this looks good. Ship it :)
  5. 
      
Loading...