Implement URL namspacing and permissions checking for the reviews app.

 






 

 


  Review Request #1813
  —
  Created Oct. 5, 2010 and submitted

 


















 

  

   Information

  





 
 

  david
 









 
 

  Review Board
 









 
 

  master
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
Implement URL namspacing and permissions checking for the reviews app.

This change adds LocalSite URL namespacing and permissions checking to the
reviewboard.reviews app. The namespacing is achieved via an optional item in the
URL which is parsed out in the initial regex, which will then be passed as an
optional parameter to any views referenced in reviews/urls.py.

The views then read that local_site_name variable and react accordingly. The
details of what "accordingly" means depends on the view.

When looking up a review request via its ID, if a local site name is given, that
ID is used to query the local_id field instead of the pk. This also verifies
that if a local_site_name is not given, but the review request has one assigned,
that addressing it by its pk will return 403.

For the new_review_request page, changes are:
   - If a local site name is given and the requesting user isn't logged in or
     doesn't have access, this will return a 403.
   - If a local site name is not given, the NewReviewRequestForm will filter the
     list of repositories to include only those which do not have an associated
     LocalSite object.
   - If a local site name is given, the NewReviewRequestForm will filter the
     list of repositories to include only those which have the given site name.

For the review_detail page ("View Reviews"), changes are:
   - Look up the review request according to the common lookup logic that takes
     into account the local_site_name and logged in user.

For these pages,
   - all_review_requests
   - submitter_list
   - group
   - group_members
   - submitter
changes are:
   - Add support to ReviewRequestManager.public() to query only reviews with a
     local_site_name
   - Queries without a local_site_name will show all review requests that
     likewise have no associated site. Queries with a name will only show the
     relevant items on the dashboard.

For these pages,
   - comment_diff_fragments
   - delete_screenshot
   - diff
   - diff_fragment
   - preview_reply_email
   - preview_review_email
   - preview_review_request_email
   - raw_diff
   - review_draft_inline_form
   - search
   - view_screenshot
changes are:
   - Look up the review request according to the common lookup logic that takes
     into account the local_site_name and logged in user.

I've also updated these:
   - ReviewRequest.get_absolute_url
   - Screenshot.get_absolute_url
   - Group.get_absolute_url

Things left to do before this is ready:
   - Fix search form ACTION url
   - Fix sidebar links on dashboard
   - Fix the review number inline on the page to use local_id

 








  


 
 

  
- Tested new review request page, looking at both the bare state and local-site
  specific ones to make sure the repositories listed were restricted as
  appropriate.
- Tested creating new review requests. Verified that the correct local_site was
  set (whether named or None) and that the local_id was set properly in
  ascending order.
- Tested that the new review request redirected to the correct location after
  creating a request with a local_site.
- Tested local_id addressing and permissions for review requests, with both
  local_site set and not.
- Tested local_site namespacing for the various pages of the dashboard view.
- Tested local_site namespacing for group_list view.
- Tested local_site namespacing for watched groups dashboard view.
- Tested diff_fragment view.
- Tested diff view.
- Tested group_members view for both global and site-specific groups.
- Tested review_draft_inline_form view.
- Tested submitter_list view for both global and site-specific URLs.
- Tested group view for both global and site-specific groups (and 404).
- Tested all_review_requests view for both global and site-specific URLs.
- Tested raw_diff view
- Tested preview_review_request_email view
- Tested view_screenshot view
- Tested submitter view
- Tested custom SQL for local_id incrementing across different local sites
- Tested comment_diff_fragments view
- Tested preview_review_email view
- Tested preview_reply_email view
- Tested delete_screenshot view
- Tested local-site filtering of search results
- Ran unit tests.

 







 









 





  








 









 






  

 








 














 



 

  

   




david




   

   

  

 



 















 





 

  

   




chipx86




   

   





  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/datagrids.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    This may not be a problem in reality today, but ignoring the queryset provided kind of sucks. Can we instead do:

   queryset = queryset.filter(local_site=local_site)

or whatever? Then we can eliminate the else: and provide queryset to the parent constructor.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      For some reason I was blanking on backwards relations when I wrote this. Fixing...
      
 
      

      2. 


 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/datagrids.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Blank line.
    
  
    
 
    

 
    
  



 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/forms.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Seems this can be consolidated:

if ((local_site_name and
     repo.local_site and 
     repo.local_site.name == local_site_name) or
    (not local_site_name and repo.local_site is None)):
    valid_repos.append(...)

It's a little wordy perhaps, but we only have one way to append valid repos, and it ends up being that logic anyway.
    
  
    
 
    

 
    
  



 
    

    5. 






  5. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/models.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    This should take into account settings.SITE_ROOT
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Fixed via the new @root_url decorator I added in http://reviews.reviewboard.org/r/1816/
      
 
      

      2. 


 
    


    

 
    

    6. 






  6. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/views.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Can you shorten that to one statement?

Why are we doing this, btw? Forbidden should be doing the right thing, shouldn't it?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I can shorten it, but we still need to render the template. Just returning a bare HttpResponseForbidden will give a blank page with a 403 error code. It's a shame there's nothing like Http404, but I think we'll probably spend some time later on making these errors nicer (right now the template is pretty dumb).
      
 
      

      2. 


 
    


    

 
    

    7. 






  7. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/views.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    One line?
    
  
    
 
    

 
    
  



 
    

    8. 






  8. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/views.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Parens instead of \

Also, instead of "not is_authenticated", you should be able to do "is_anonymous"
    
  
    
 
    

 
    
  



 
    

    9. 






  9. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/views.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    I'm not sure, but I think this will actually load the data for the user, which is overkill. Maybe use count instead.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I think merely evaluating for truthiness won't load anything, but I changed it to use .exists() instead of bool() because that's better.
      
 
      

      2. 


 
    


    

 
    

    10. 






  10. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/views.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Blank line.
    
  
    
 
    

 
    
  



 
    

    11. 






  11. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/views.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Blank line.
    
  
    
 
    

 
    
  



 
    

    12. 






  12. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/views.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Same comments as above with parens, is_anonymous, and count.
    
  
    
 
    

 
    
  



 
    

    13. 






  13. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/views.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Blank line.
    
  
    
 
    

 
    
  



 
    

    14. 






  14. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/views.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Blank line.
    
  
    
 
    

 
    
  



 
    

    15. 






  15. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/views.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    We never use user. Don't we want to do what we do in the other conditional, with get_object_or_404?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      It's more to get the exceptions to raise, which we catch below. I can avoid storing 'user' in order to make that a little more obvious. get_object_or_404 does something very similar.
      
 
      

      2. 


 
    


    

 
    

    16. 






  16. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/views.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Blank line.
    
  
    
 
    

 
    
  



 
    

    17. 






  17. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/views.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Blank line.
    
  
    
 
    

 
    
  



 
    

    18. 






  18. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/views.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Blank line.
    
  
    
 
    

 
    
  



 
    

    19. 






  19. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/views.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Blank line.
    
  
    
 
    

 
    
  



 
    

    20. 






  20. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/views.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Blank line.
    
  
    
 
    

 
    
  



 
    

    21. 






  21. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/views.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Blank line.
    
  
    
 
    

 
    
  



 
    

    22. 






  22. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/views.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Blank line.
    
  
    
 
    

 
    
  



 
    

    23. 






  23. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/views.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Blank line.
    
  
    
 
    

 
    
  



 
    

    24. 






  24. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/views.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Blank line.
    
  
    
 
    

 
    
  



 
    

    25. 






  25. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/urls.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Do we want to allow "." in the site name? Might be useful for namespacing.
    
  
    
 
    

 
    
  



 
    

    26. 




  26. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    27. 




  

 
















 



 

  

   




david




   

   

  

 



 















 



 

  

   




david




   

   

  

 



 















 



 

  

   




david




   

   

  

 



 















 





 

  

   




chipx86




   

   





  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/datagrids.py
      


       (Diff revisions 2 - 5)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    djblets and django are both third party, so there shouldn't be a blank line.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/models.py
      


       (Diff revisions 2 - 5)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Blank line.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/models.py
      


       (Diff revisions 2 - 5)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Blank line.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/models.py
      


       (Diff revisions 2 - 5)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Does this fit in 80 chars?
    
  
    
 
    

 
    
  



 
    

    6. 






  6. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/urls.py
      


       (Diff revisions 2 - 5)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    We're doing this all over now. I wonder if we can do:

localsite_urlpatterns = patterns(...),
    url('^dashboard/$', ...),
    ...
)

urlpatterns += patterns('',
    url(r'^((?P<local_site_name>[...]+)/)?', include(localsite_urlpatterns)),
)


I don't know how that works if local_site_name isn't provided, but it might work? Worth trying. It'll be more maintainable if so.

Also, make sure everything fits on < 80 columns.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      It doesn't work if local_site_name isn't provided because there's nothing to match against, so it won't recurse into the included patterns.

I'll wrap things appropriately.
      
 
      

      2. 


 
    


    

 
    

    7. 






  7. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/urls.py
      


       (Diff revisions 2 - 5)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Why I like this better how it was. It didn't go over 80 columns and it was more readable.
    
  
    
 
    

 
    
  



 
    

    8. 






  8. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/urls.py
      


       (Diff revisions 2 - 5)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Same here.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    9. 




  9. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    10. 




  

 
















 



 

  

   




david




   

   

  

 



 















 



 

  

   




david




   

   

  

 



 















 





 

  

   




chipx86




   

   





  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/managers.py
      


       (Diff revision 7)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    For some of the cache stuff, I had to modify this to take a local_site instead of a local_site_name. It puts the burden on the object fetching on the caller's side, but I think it's actually better, and fixes some hairy lambdas we'd otherwise need in the LocalSiteProfile.

The query below becomes simplified as a result, though, which is nice.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




david




   

   

  

 



 















 





 

  

   




chipx86




   

   





  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/datagrids.py
      


       (Diff revisions 5 - 8)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Can just augment queryset and not define a new qs. That'd let you get rid of the eles:
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Python scoping makes this dumb.
      
 
      

      2. 


 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/datagrids.py
      


       (Diff revisions 5 - 8)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Probably want to remove this.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Way ahead of you.
      
 
      

      2. 


 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/managers.py
      


       (Diff revisions 5 - 8)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Can you do this? I guess I thought you had to AND two Q()s? If not, awesome :)
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/managers.py
      


       (Diff revisions 5 - 8)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Can't you just do:

return self._qyery(..., local_site=local_site)

?
    
  
    
 
    

 
    
  



 
    

    6. 






  6. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/views.py
      


       (Diff revisions 5 - 8)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Are these going to be removed before committing?
    
  
    
 
    

 
    
  



 
    

    7. 






  7. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/views.py
      


       (Diff revisions 5 - 8)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Were we querying all these twice?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      That's how I initially made the groups and watched-groups filter correctly.
      
 
      

      2. 


 
    


    

 
    

    8. 






  8. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/views.py
      


       (Diff revisions 5 - 8)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    That's because get_object_or_none own't find the right LocalSite, and returns None, which is valid.

We probably want to use get_object_or_404 for the LocalSite query too.

Are there other places where we want to check this too?

At some point, we probably should write unit tests that test all our main views with an invalid site name and ensure we get a 404.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Well, I want local_site to be assigned None. What's happening is that it's still returning the group when local_site == None, even when the group's local_site relation isn't. But maybe I was getting confused.
      
 
      

      2. 


 
    


    

 
    

    9. 






  9. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/diffviewer/views.py
      


       (Diff revision 8)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    diffviewer shouldn't know about review requests. Instead, provide an extra_context, and have the wrapper in reviews provide that.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      How about I call this "base_url" and use that? It seems odd to hide a necessary parameter to the template inside of an extra_context dictionary.
      
 
      

      2. 


 
    


    

 
    

    10. 






  10. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/diffviewer/views.py
      


       (Diff revision 8)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    One line.
    
  
    
 
    

 
    
  



 
    

    11. 






  11. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/webapi/urls.py
      


       (Diff revision 8)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    As much as I'd love to get rid of them, we can't until everyone's off of the current RBTools, which requires the rewrite.

As long as this only works with local_site=None, I'm happy. In that case, do we need the _deprecated versions? I'd think we could just use the new ones with local_site=None.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      This should make it so the old API doesn't allow you to see local-site'ed groups. I'm not sure how to "use the new ones with local_site=None" with the way that this is dispatched.
      
 
      

      2. 


 
    


    

 
    

    12. 




  12. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    13. 




  

 
















 



 

  

   




david




   

   

  

 



 















 





 

  

   




chipx86




   

   





  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/diffviewer/views.py
      


       (Diff revision 9)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    May as well combine these three lines.
    
  
    
 
    

 
    
  



 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/managers.py
      


       (Diff revision 9)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    I think we need to make this atomic. Instead of this, do:

LocalSite.objects.filter(pk=local_site.pk).update(next_id=F(next_id) + 1)
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      This is all happening in a transaction, isn't it?
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Yes but it's still not atomic.

If you load LocalSite and next_id is, say, 3, when you save it'll be 4. Even if another request went in and changed it to 4. You want the calculation to happen in a single request without any chance of another request happening during it.

Actually, this is where CounterField comes in. You should make next_id a CounterField and then use its updating stuff, the way we do shipit_count and the new profile fields, to ensure atomic updates.
      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      Hmm. How do we make the local_id assignment part of that atom?
      
 
      

      4. 



    4. 
 
 
      
 
      
  
 
      
 
      
  
      Okay, so I've been trying to figure this out. The problem is really the loading of a number and then evaluating that. What we really want is to operate on the level of the fields.

We may not always be able to guarantee that the following runs in one atomic transaction independent of any other transaction, but I am hoping this would work:

    ReviewRequest.objects.filter(pk=review_request.id).update(local_id=F(local_site__next_id))
    LocalSite.objects.filter(pk=local_site.id).update(next_id=F(next_id) + 1)

This of course requires that the review request is first saved, which is fine. It should quickly move that next_id over to the review request and then increment it. Ideally, nothing would be able to happen between those two operations. In theory, it could, but it doesn't sound like there's a better way that isn't database-specific.

MySQL has something sort of for this case, using some logic with LAST_INSERT_ID(), but I don't know about equivalents for other databases.
      
 
      

      5. 



    5. 
 
 
      
 
      
  
 
      
 
      
  
      Another idea is to get rid of next_id, and instead base this on the latest review request.

So, you could do:

    ReviewRequest.objects.filter(pk=review_request.id).extra(
        select={
            'last_id': 'SELECT MAX(local_id) FROM reviews_reviewrequest LIMIT 1'
        }
    ).update(local_id=F(last_id) + 1))

.. Something like that. 
      
 
      

      6. 



    6. 
 
 
      
 
      
  
 
      
 
      
  
      (Pretend that selected on the local_site too)

So that actually has problems with F(), since select doesn't store the result in a queryable way. Looking into this.
      
 
      

      7. 



    7. 
 
 
      
 
      
  
 
      
 
      
  
      Okay... So Django doesn't support F() with values stored from extra() and they don't plan to. They didn't say no in the bug report I saw about allowing aggregates in queries (which would give us what we need above for that extra() call), but it doesn't support it yet. So, raw SQL FTW. Kind of a headache, but I think it's worth doing. It'll make this atomic and we won't need to store the next_id field.

There may be some ways to play with QuerySet's add_aggregate (kind of making a blend of aggregate() and annotate()) to get what we need, but I don't know.
      
 
      

      8. 



    8. 
 
 
      
 
      
  
 
      
 
      
  
      OKAY :)

Here's what you do:

    from django.db import connections, router

    ...

    # First, build the review request, without the local_id and local_site
    review_request = ReviewRequest(....)
    review_request.save()

    db = router.db_for_write(ReviewRequest)
    cursor = connections[db].cursor()
    cursor.execute(
        'UPDATE %(table)s'
        '  SET local_id = (SELECT MAX(local_id) FROM %(table)s) + 1,'
        '      local_site_id = %(local_site_id)s'
        '  WHERE %(table)s.id = %(id)s' % {
            'table': ReviewRequest._meta.db_table,
            'local_site_id': local_site.pk,
            'id': review_request.pk,
        })


That will keep the update atomic without needing another field. The local_site is set at the same time in order to allow for the unique_together restraint.
      
 
      

      9. 


 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/templates/403.html
      


       (Diff revision 9)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Maybe replace this with a {# .. #} if we don't have the nice text before this goes in.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I'm hoping we can come up with a generic 403 that works for everything (both your and my changes)
      
 
      

      2. 


 
    


    

 
    

    5. 




  5. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 




  

 
















 



 

  

   




david




   

   

  

 



 















 



 

  

   




david




   

   

  

 



 















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Looks good! One tiny, minor thing.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/webapi/resources.py
      


       (Diff revisions 9 - 11)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    The | and & should be consistent at the end.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




david




   

   

  

 



 















 



 

  

   




david




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  








 
Change Summary:

 
- Fixed the "Edit Review" action when a user has a draft
- Fixed changes for the diff fragment views
- Fixed delete_screenshot view (shouldn't this go into the API?)
- Removed RBCOMMONS comments, since they're all done!





 



  

   
Description:

   
    
     
Implement URL namspacing and permissions checking for the reviews app.
     

     
This change adds LocalSite URL namespacing and permissions checking to the
     reviewboard.reviews app. The namespacing is achieved via an optional item in the
     URL which is parsed out in the initial regex, which will then be passed as an
     optional parameter to any views referenced in reviews/urls.py.
     

     
The views then read that local_site_name variable and react accordingly. The
     details of what "accordingly" means depends on the view.
     

     
When looking up a review request via its ID, if a local site name is given, that
     ID is used to query the local_id field instead of the pk. This also verifies
     that if a local_site_name is not given, but the review request has one assigned,
     that addressing it by its pk will return 403.
     

     
For the new_review_request page, changes are:
        - If a local site name is given and the requesting user isn't logged in or
          doesn't have access, this will return a 403.
        - If a local site name is not given, the NewReviewRequestForm will filter the
          list of repositories to include only those which do not have an associated
          LocalSite object.
        - If a local site name is given, the NewReviewRequestForm will filter the
          list of repositories to include only those which have the given site name.
     

     
For the review_detail page ("View Reviews"), changes are:
        - Look up the review request according to the common lookup logic that takes
          into account the local_site_name and logged in user.
     

     
For these pages,
        - all_review_requests
        - submitter_list
        - group
        - group_members
        - submitter
     changes are:
        - Add support to ReviewRequestManager.public() to query only reviews with a
          local_site_name
        - Queries without a local_site_name will show all review requests that
          likewise have no associated site. Queries with a name will only show the
          relevant items on the dashboard.
     

     
For these pages,
        - comment_diff_fragments
        - delete_screenshot
        - diff
        - diff_fragment
        - preview_reply_email
        - preview_review_email
        - preview_review_request_email
        - raw_diff
        - review_draft_inline_form
        - search
        - view_screenshot
     changes are:
        - Look up the review request according to the common lookup logic that takes
          into account the local_site_name and logged in user.
     

     
I've also updated these:
        - ReviewRequest.get_absolute_url
        - Screenshot.get_absolute_url
        - Group.get_absolute_url
     

     
Things left to do before this is ready:
 ~      - Test other URLs
   ~    - Fix search form ACTION url
 -      - Test comment_diff_fragments view
 -      - Test preview_review_email view
 -      - Test preview_reply_email view
 -      - Test delete_screenshot view
 -      - Test search view
        - Fix sidebar links on dashboard
        - Fix the review number inline on the page to use local_id

   		
  




  

   
Testing Done:

   
    
     
  • Tested new review request page, looking at both the bare state and local-site
    
  specific ones to make sure the repositories listed were restricted as
    
  appropriate.
     
  • Tested creating new review requests. Verified that the correct local_site was
    
  set (whether named or None) and that the local_id was set properly in
    
  ascending order.
     
  • Tested that the new review request redirected to the correct location after
    
  creating a request with a local_site.
     
  • Tested local_id addressing and permissions for review requests, with both
    
  local_site set and not.
     
  • Tested local_site namespacing for the various pages of the dashboard view.
     
  • Tested local_site namespacing for group_list view.
     
  • Tested local_site namespacing for watched groups dashboard view.
     
  • Tested diff_fragment view.
     
  • Tested diff view.
     
  • Tested group_members view for both global and site-specific groups.
     
  • Tested review_draft_inline_form view.
     
  • Tested submitter_list view for both global and site-specific URLs.
     
  • Tested group view for both global and site-specific groups (and 404).
     
  • Tested all_review_requests view for both global and site-specific URLs.
     
  • Tested raw_diff view
     
  • Tested preview_review_request_email view
     
  • Tested view_screenshot view
     
  • Tested submitter view
     
  • Tested custom SQL for local_id incrementing across different local sites
 ~   
  • Ran unit tests.
   ~ 
  • Tested comment_diff_fragments view
   + 
  • Tested preview_review_email view
   + 
  • Tested preview_reply_email view
   + 
  • Tested delete_screenshot view
   + 
  • Tested local-site filtering of search results
   + 
  • Ran unit tests.

   		
  




  

   
Diff:

   
    
 Revision 13 (+579 -308)
Show changes
 
    reviewboard/urls.py 
    reviewboard/diffviewer/views.py 
    reviewboard/htdocs/media/rb/js/datastore.js 
    reviewboard/htdocs/media/rb/js/diffviewer.js 
    reviewboard/htdocs/media/rb/js/reviews.js 
    reviewboard/notifications/tests.py 
    reviewboard/reviews/datagrids.py 
    reviewboard/reviews/feeds.py 
  16 more
 

   		
  



 










  

 
















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Only thing I'd say that I didn't see is that the is_accessible_by should eventually consider localsite, but given that we lock down access to the views, it's probably fine.

Looks great though :) Yay!
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I've started keeping track of additional to-dos in a spreadsheet (starting with the 3 currently in here), so I'll add that. Thanks for all the reviews on this!
      
 
      

      2. 


 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   





 FR





   

   





  

 



 

  


   

   

    
 
frigo


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/views.py
      


       (Diff revision 13)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    [insert joke here]
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 









     

    

   

  

  

   
   Loading...