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!