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

Review Board
master
d46c0e5...

Reviewers

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
  1. 
      
  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. reviewboard/hostingsvcs/hook_utils.py (Diff revision 1)
     
     
    Show all issues

    This line isn't needed.

  4. 
      
anselina
chipx86
  1. 
      
  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. 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. 
      
anselina
david
  1. Ship It!

  2. 
      
anselina
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.0.x (87be48e). Thanks!