Improve json usefulness for post-commit hook

Review Request #210 — Created Jan. 25, 2008 and submitted

Information

Review Board SVN (deprecated)

Reviewers

the main point of this change is to allow a staff user to specify submit_as=a-different-user when creating a review request so that user will be the one notified of reviews, etc.  this enables using a post-commit hook to auto-create reviews for all or part of a repository.  there are some side concerns related to this:
 
 * basedir must be allowed to be empty when creating diffs against a svn repository as opposed to a working copy.  that is what the diffviewer change does; for the web interface the functionality is identical, but the json manually rips out the basedir when necessary, allowing a svn repo diff to be treated as "absolute."
 * added is_visible_to and is_mutable_by methods to the Request models to encapsulate the notion that "staff can always view and modify requests"
 * moved sending of mail into the models to avoid duplication of code in json and view
 * json new_diff includes traceback when something unexpected goes wrong
 * add get_changeset to svn to allow update_from_changenum for that scm

 
chipx86
  1. 
      
  2. /trunk/reviewboard/diffviewer/forms.py (Diff revision 1)
     
     
    This should still take into account tool.get_diffs_use_absolute_paths.
    1. it does.  that is what controls whether basedir is in self.fields.
  3. /trunk/reviewboard/reviews/models.py (Diff revision 1)
     
     
    Excess whitespace.
  4. /trunk/reviewboard/reviews/models.py (Diff revision 1)
     
     
     
     
     
     
    We should be making use of Django's permissions models, not specifically checking for is_staff.
    
    People who are staff do not necessarily have the permissions for modifying data. We have people marked as staff who can only add/modify review groups, for instance.
    
    Also, could you add some simple doc blocks?
    1. 
       
    2. > We should be making use of Django's permissions models
      
      but we're not, anywhere.  I'm not competent to perform that massive of a refactor.
    3. > could you add some simple doc blocks
      
      yes
    4. We use Django's permissions in other areas in the codebase.
      
      It's not a huge refactor. The Django docs are pretty good on describing this. This will have to be changed to use permissions before it can go in.
  5. /trunk/reviewboard/reviews/views.py (Diff revision 1)
     
     
    Excess whitespace.
    
    This should start with a capital C and be internationalized.
    1. all I did here was take something that failed silently and made it fail a little more usefully.  if a user ever sees this he has bigger problems; internationalizing this is silly.  if you care about the user seeing this then it needs to be done completely differently; all this is is a hack so the next developer who runs into something failing catastrophically isn't SOL.
    2. We're talking a _("..") and a capital C.
  6. /trunk/reviewboard/scmtools/svn.py (Diff revision 1)
     
     
     
    It'd be nice to move this into SVNTool directly and provide documentation for the method, specifying what it does and the return values.
    1. if you're screwing with this you should read the pysvn docs which document this.
    2. People shouldn't have to read the pysvn docs just to figure out what this function does. More documentation in the product makes it easier for future developers.
  7. /trunk/reviewboard/scmtools/svn.py (Diff revision 1)
     
     
    Can you add a doc block for this and make it clear this is for changes that are already committed?
  8. /trunk/reviewboard/scmtools/svn.py (Diff revision 1)
     
     
     
     
    I'd rather pathinfo not be its own function. This can be done inline.
    
    cs.description = '\n'.join("%s %s" % (path['action'], path['path'])
                               for path in log['changed_paths'])
    
  9. /trunk/reviewboard/scmtools/svn.py (Diff revision 1)
     
     
    Excess whitespace.
  10. /trunk/reviewboard/webapi/json.py (Diff revision 1)
     
     
    This should stay None.
    1. None is the default.
    2. Okay, just checked the code and you're right. It didn't used to be the default.
  11. /trunk/reviewboard/webapi/json.py (Diff revision 1)
     
     
    This should default to None in the parameter.
  12. /trunk/reviewboard/webapi/json.py (Diff revision 1)
     
     
    Permissions checking.
  13. /trunk/reviewboard/webapi/json.py (Diff revision 1)
     
     
     
     
     
    filter is incorrect here. You should be using User.objects.get.
  14. /trunk/reviewboard/webapi/json.py (Diff revision 1)
     
     
    Comments should be in sentence case, with a trailing '.'
    
    This should also have a blank line before it.
    
    If you could rewrite this to make it more clear, it would help. The "Allow no basedir" is grammatically awkward.
    1. good grief, it would be simpler for you to just tweak that post-patch than to have typed all that.  but fine.
    2. Then I'd have to make a note of it and do it later after modifying the patch. It's a lot of extra work that is supposed to be covered during the review process. This is why we have a review process. Helps you to know for next time and saves us all time.
  15. /trunk/reviewboard/webapi/json.py (Diff revision 1)
     
     
     
    I'd rather get rid of this change here. The code that handles the basedir field in forms should take this into account by changing it to "/".
    1. I admit I don't fully understand the code here.  This was a refactor that clearly preserved existing behavior while allowing the repo diff to work.  I will have to defer to someone more knowledgeable if something cleaner is preferred.
    2. 
       
  16. /trunk/reviewboard/webapi/json.py (Diff revision 1)
     
     
    I'm not sure this is correct. A traceback is not really what we want here. It's useful information but perhaps wrong for this field. Maybe 'path_traceback'? Then the caller can decide what to show.
    1. it's no less correct than what is already there.
  17. 
      
chipx86
  1. I think we got everything we're going to get out of this, now that the individual parts have been broken up and reviewed.
  2.