Handle GitHub comment reviews carefully
A comment review should not mask a previously provided approval or changes_requested review from a given user. The GitHub UX follows the same logic. If an approval or a changes_requested review was already provided from a user, that user can issue future comment reviews and it will not change their vote. Our algorithm needed to be updated to handle this scenario, and a test case was added to validate it. Change-Id: Iac10544a1f24362c27220a44edbdb988b7caa989
This commit is contained in:
parent
2062686b82
commit
3f74f6fb04
|
@ -255,6 +255,37 @@ class TestGithubRequirements(ZuulTestCase):
|
||||||
self.assertEqual(len(self.history), 1)
|
self.assertEqual(len(self.history), 1)
|
||||||
self.assertEqual(self.history[0].name, 'project5-reviewuserstate')
|
self.assertEqual(self.history[0].name, 'project5-reviewuserstate')
|
||||||
|
|
||||||
|
@simple_layout('layouts/requirements-github.yaml', driver='github')
|
||||||
|
def test_pipeline_require_review_comment_masked(self):
|
||||||
|
"Test pipeline requirement: review comments on top of votes"
|
||||||
|
|
||||||
|
A = self.fake_github.openFakePullRequest('org/project5', 'master', 'A')
|
||||||
|
# Add derp to writers
|
||||||
|
A.writers.append('derp')
|
||||||
|
# A comment event that we will keep submitting to trigger
|
||||||
|
comment = A.getCommentAddedEvent('test me')
|
||||||
|
self.fake_github.emitEvent(comment)
|
||||||
|
self.waitUntilSettled()
|
||||||
|
# No positive review from derp so should not be enqueued
|
||||||
|
self.assertEqual(len(self.history), 0)
|
||||||
|
|
||||||
|
# The first negative review from derp should not cause it to be
|
||||||
|
# enqueued
|
||||||
|
A.addReview('derp', 'CHANGES_REQUESTED')
|
||||||
|
self.fake_github.emitEvent(comment)
|
||||||
|
self.waitUntilSettled()
|
||||||
|
self.assertEqual(len(self.history), 0)
|
||||||
|
|
||||||
|
# A positive review is required, so provide it
|
||||||
|
A.addReview('derp', 'APPROVED')
|
||||||
|
|
||||||
|
# Add a comment review on top to make sure we can still enqueue
|
||||||
|
A.addReview('derp', 'COMMENTED')
|
||||||
|
self.fake_github.emitEvent(comment)
|
||||||
|
self.waitUntilSettled()
|
||||||
|
self.assertEqual(len(self.history), 1)
|
||||||
|
self.assertEqual(self.history[0].name, 'project5-reviewuserstate')
|
||||||
|
|
||||||
@simple_layout('layouts/requirements-github.yaml', driver='github')
|
@simple_layout('layouts/requirements-github.yaml', driver='github')
|
||||||
def test_require_review_newer_than(self):
|
def test_require_review_newer_than(self):
|
||||||
|
|
||||||
|
|
|
@ -763,8 +763,19 @@ class GithubConnection(BaseConnection):
|
||||||
# if there are multiple reviews per user, keep the newest
|
# if there are multiple reviews per user, keep the newest
|
||||||
# note that this breaks the ability to set the 'older-than'
|
# note that this breaks the ability to set the 'older-than'
|
||||||
# option on a review requirement.
|
# option on a review requirement.
|
||||||
|
# BUT do not keep the latest if it's a 'commented' type and the
|
||||||
|
# previous review was 'approved' or 'changes_requested', as
|
||||||
|
# the GitHub model does not change the vote if a comment is
|
||||||
|
# added after the fact. THANKS GITHUB!
|
||||||
if review['grantedOn'] > reviews[user]['grantedOn']:
|
if review['grantedOn'] > reviews[user]['grantedOn']:
|
||||||
reviews[user] = review
|
if (review['type'] == 'commented' and reviews[user]['type']
|
||||||
|
in ('approved', 'changes_requested')):
|
||||||
|
self.log.debug("Discarding comment review %s due to "
|
||||||
|
"an existing vote %s" % (review,
|
||||||
|
reviews[user]))
|
||||||
|
pass
|
||||||
|
else:
|
||||||
|
reviews[user] = review
|
||||||
|
|
||||||
return reviews.values()
|
return reviews.values()
|
||||||
|
|
||||||
|
|
Loading…
Reference in New Issue