Summary:

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

Review Request #5666 — Created April 1, 2014 and submitted April 3, 2014, 2:05 p.m.

Information
Owner:	anselina
Repository:	Review Board
Branch:	master
Bugs:
Depends On:
Commit:	d46c0e5...
Reviewers
Groups:	reviewboard, students
People:

Description

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.

Testing Done

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

Issues

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:	chipx86	April 2, 2014, 1:52 p.m.
You'll still need to set a value here, or there won't be anything to return. What would the caller do …	chipx86	April 2, 2014, 6:28 p.m.
This line isn't needed.	chipx86	April 2, 2014, 12:30 p.m.

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:

reviewboard/hostingsvcs/hook_utils.py (Diff revision 1)
Show all issues
```
This line isn't needed.
```

Change Summary:

Fixed review issues.

Commit:

-	889467210cd208206042ad994d6b4e457afc030e
+	9a83912cf6b476bf05f71ee463e9dcc2b73f1b95

Diff:

Revision 2 (+27 -6)

Show changes

	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/hook_utils.py

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.)

anselina April 2, 2014, 6:17 p.m.

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.

anselina April 2, 2014, 6:41 p.m.

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.)

Change Summary:

Set a missing review_request_id value, and return None instead of 0 when a matching review request ID cannot be found.

Commit:

-	9a83912cf6b476bf05f71ee463e9dcc2b73f1b95
+	d46c0e5626440c8eceaef3f25e63cf4f46553b69

Diff:

Revision 3 (+30 -8)

Show changes

	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/hook_utils.py

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

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