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:
  

  Closed (submitted)

  
 




 
Change Summary:

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





 


 










  

 









     

    

   

  

  

   
   Loading...