• 
  
    
 
    
 
    

    



    
    
    
    
     
    




    
 
    

  
    



    
 

 

  

  

 

    


  
    





    
 
    



  

    
 
 
    
  
    Add a check to find a review request with a matching commit ID for webhooks.
    
 
    

    



 
    
 
    

  Review Request #5666
  —
  Created April 1, 2014 and submitted 

 
    


    




    




    




 
    
  
    
   Information

  
    



    
 
 
    
  anselina
 
    

    





    
 
 
    
  Review Board
 
    

    





    
 
 
    
  master
 
    

    





    
 
 
    
  
 
    

    





    
 
 
    
  
 
    

    





    
 
 
    
  d46c0e5...
 
    

    



 
    



 
    
  
    
   Reviewers

  
    



    
 
 
    
  reviewboard, students
 
    

    





    
 
 
    
  
 
    

    



 
    



    


    
 
    




  

    
 
 
    
  
    Previously, we relied only on the regex to match a review request ID in the
    
commit message. Now, we use the regex to try to find a matching review request
    
ID in the commit message, and if no match is found, we then use the commit ID
    
to try to find a matching review request.
    
 
    

    





  

    
 
 
    
  
    I pushed different commits to a GitHub repository with the post-receive webhook
    
enabled (and saw the expected behaviour):
    
- Commit that did not match the regex but matched a review request's commit ID
    
- Commit that did not match the regex and did not match a review request's
    
  commit ID
    
- Commit that matched the regex
    
 
    

    




 
    

    





    
 
    









 





    
 
 
 
    
  
    
   
    
    
    
     
    
    
    
      
     
    • 
      
     
      • 
     
    • 
      
     
      • 
     
    • 
      
     
      • 
     
    • 
      
     
      • 
     
    • 
      
     
      • 
    
    
   
    
   
    
    
     
    
      Description
      From
      Last Updated
     
    
        
    



     
    
      
       
       
    While the regex will hopefully be correctly written, I think we should sanity-check by wrapping this in a try ValueError:
    
          		
      
       chipx86chipx86
      
      
       
      
     
    



     
    
      
       
       
    You'll still need to set a value here, or there won't be anything to return. What would the caller do …
    
          		
      
       chipx86chipx86
      
      
       
      
     
    



     
    
      
       
       
    This line isn't needed.
    
          		
      
       chipx86chipx86
      
      
       
      
     
    

        
   
    
  
    
 
    

    




 
    

    



  
    
 
    

    


    


 













    
 





 
    
  
    
   
    



chipx86




   
    
   
    





    
  
    
 
    


 
    
  
    

   
    
   
    
    
     
chipx86


    
    
   
    

  
    
  
    
  
    


      




    1. 
 
      
  
      
   
      
  
      
 
      

 
      
  


      
 
      
 
        

  

 
      


      

 
      

      2. 

















    2. 
 
      
  
  
      
  
 
      
  
      
   
      
    
     
      
      
       reviewboard/hostingsvcs/hook_utils.py
       


        (Diff revision 1)


       
      
     
      
          
    
     
       
      

     
       
      

          
   
      
  
      
 
      

   
      
    
    




      
 
      
  
  
  

  
  
   Show all issues
  
 
      

      



    
      While the regex will hopefully be correctly written, I think we should sanity-check by wrapping this in a try ValueError:
      
  
      
 
      

 
      
  


      
 
      
 
        

  

 
      


      

 
      

      3. 






    3. 
 
      
  
  
      
  
 
      
  
      
   
      
    
     
      
      
       reviewboard/hostingsvcs/hook_utils.py
       


        (Diff revision 1)


       
      
     
      
          
    
     
       
      

     
       
      

          
   
      
  
      
 
      

   
      
    
    




      
 
      
  
  
  

  
  
   Show all issues
  
 
      

      



    
      This line isn't needed.
      
  
      
 
      

 
      
  


      
 
      
 
        

  

 
      


      

 
      

      4. 




    4. 
 
      
  
      
   
      
  
      
 
      

 
      
  


      
 
      
 
        

  

 
      


      

 
      

      5. 


    

  
    
 
    

    












    
 



 
    
  
    
   
    



anselina




   
    
   
    
  
    
 
    


 

    












    
 





 
    
  
    
   
    



chipx86




   
    
   
    





    
  
    
 
    


 
    
  
    

   
    
   
    
    
     
chipx86


    
    
   
    

  
    
  
    
  
    


      




    1. 
 
      
  
      
   
      
  
      
 
      

 
      
  


      
 
      
 
        

  

 
      


      

 
      

      2. 

















    2. 
 
      
  
  
      
  
 
      
  
      
   
      
    
     
      
      
       reviewboard/hostingsvcs/hook_utils.py
       


        (Diff revisions 1 - 2)


       
      
     
      
          
    
     
       
      

     
       
      

     
       
      

          
   
      
  
      
 
      

   
      
    
    




      
 
      
  
  
  

  
  
   Show all issues
  
 
      

      



    
      You'll still need to set a value here, or there won't be anything to return.
      

      What would the caller do if the return value isn't a valid review request ID?
      

      (We also may want to use None instead of 0 to indicate that we don't have the ID.)
      
  
      
 
      

 
      
  


      
 
      
 
        

  


      1. 
 
 
        
 
        
  
 
        
 
        
  
        Ah yes, I forgot to do that! 
        

        Currently, the checking/handling of whether a review request ID is valid happens in close_all_review_requests(). Looking back at this, I think it's better to move this check to get_review_request_id(), and return None if it's invalid (so the caller isn't responsible for it)?
        

        Cool, I didn't realize you could use None as a dictionary key - will make this change.
        
 
        

        2. 



      2. 
 
 
        
 
        
  
 
        
 
        
  
        Never mind, I actually left the resolve() call in close_all_review_requests() to get the local_site argument. Should I bother moving the check for a valid review request ID? (My understanding of how local sites work is limited, so I don't see a way to do this without repeating code.)
        
 
        

        3. 


 
      


      

 
      

      3. 




    3. 
 
      
  
      
   
      
  
      
 
      

 
      
  


      
 
      
 
        

  

 
      


      

 
      

      4. 


    

  
    
 
    

    












    
 



 
    
  
    
   
    



anselina




   
    
   
    
  
    
 
    


 

    












    
 





 
    
  
    
   
    



david




   
    
   
    





    
  
    
 
    


 
    
  
    

   
    
   
    
    
     
david


    
    
   
    

  
    
  
    
  
    


      




    1. 
 
      
  
      
   
      Ship It!
      
  
      
 
      

 
      
  


      
 
      
 
        

  

 
      


      

 
      

      2. 















    2. 
 
      
  
      
   
      
  
      
 
      

 
      
  


      
 
      
 
        

  

 
      


      

 
      

      3. 


    

  
    
 
    

    












    
 



 
    
  
    
   
    



anselina




   
    
   
    
  
    
 
    


 
    
  
    

   
    
   
    
    
     Review request changed
    
    
   
    

  
    
  
    
  
    




    

  
    
   
    
    Status:
   
    
   
    

    Completed

   
    
  
    



  
    
   
    
    Change Summary:
   
    
   
    
    
    Pushed to release-2.0.x (87be48e). Thanks!