Ensure PRs arent rejected for stale negative reviews

This updates the github source to only use the most recent review
from each user reviewing a PR.  This avoids having obsolete negative
reviews trump a newer positive review.

I've added a new test for good measure to ensure it works for the
github case, where we are doing some conversion of github formatted
timestamps to internal timestamps.

Change-Id: I5607901def856c9363ec786a4116bfec19c9c97c
Co-Authored-By: Jesse Keating <omgjlk@us.ibm.com>
Signed-off-by: Adam Gandelman <adamg@ubuntu.com>

tests/base.py

@@ -16,6 +16,7 @@
 # under the License.
 
 from six.moves import configparser as ConfigParser
+import datetime
 import gc
 import hashlib
 import json
@@ -777,23 +778,37 @@ class FakeGithubPullRequest(object):
         }))
 
     def addReview(self, user, state, granted_on=None):
-        # Each user will only have one review at a time, so replace
-        # any existing reviews
-        # FIXME(jlk): this isn't quite right, reviews stack, we only
-        # consider the latest for a user. Thanks GitHub!!
-        for review in self.reviews:
-            if review['user']['login'] == user:
-                self.reviews.remove(review)
+        gh_time_format = '%Y-%m-%dT%H:%M:%SZ'
+        # convert the timestamp to a str format that would be returned
+        # from github as 'submitted_at' in the API response
+
+        if granted_on:
+            granted_on = datetime.datetime.utcfromtimestamp(granted_on)
+            submitted_at = time.strftime(
+                gh_time_format, granted_on.timetuple())
+        else:
+            # github timestamps only down to the second, so we need to make
+            # sure reviews that tests add appear to be added over a period of
+            # time in the past and not all at once.
+            if not self.reviews:
+                # the first review happens 10 mins ago
+                offset = 600
+            else:
+                # subsequent reviews happen 1 minute closer to now
+                offset = 600 - (len(self.reviews) * 60)
+
+            granted_on = datetime.datetime.utcfromtimestamp(
+                time.time() - offset)
+            submitted_at = time.strftime(
+                gh_time_format, granted_on.timetuple())
 
-        if not granted_on:
-            granted_on = time.time()
         self.reviews.append({
             'state': state,
             'user': {
                 'login': user,
                 'email': user + "@derp.com",
             },
-            'provided': int(granted_on),
+            'submitted_at': submitted_at,
         })
 
     def _getPRReference(self):

tests/fixtures/layouts/requirements-github.yaml

@@ -53,6 +53,11 @@
         review:
           - type: approved
             permission: write
+    reject:
+      github:
+        review:
+          - type: changes_requested
+            permission: write
     trigger:
       github:
         - event: pull_request
@@ -71,6 +76,47 @@
           - username: 'derp'
             type: approved
             permission: write
+    reject:
+      github:
+        review:
+          - type: changes_requested
+            permission: write
+    trigger:
+      github:
+        - event: pull_request
+          action: comment
+          comment: 'test me'
+    success:
+      github:
+        comment: true
+
+- pipeline:
+    name: newer_than
+    manager: independent
+    require:
+      github:
+        review:
+          - type: approved
+            permission: write
+            newer-than: 1d
+    trigger:
+      github:
+        - event: pull_request
+          action: comment
+          comment: 'test me'
+    success:
+      github:
+        comment: true
+
+- pipeline:
+    name: older_than
+    manager: independent
+    require:
+      github:
+        review:
+          - type: approved
+            permission: write
+            older-than: 1d
     trigger:
       github:
         - event: pull_request
@@ -90,6 +136,10 @@
     name: project4-reviewreq
 - job:
     name: project5-reviewuserstate
+- job:
+    name: project6-newerthan
+- job:
+    name: project7-olderthan
 
 - project:
     name: org/project1
@@ -120,3 +170,15 @@
     reviewuserstate:
       jobs:
         - project5-reviewuserstate
+
+- project:
+    name: org/project6
+    newer_than:
+      jobs:
+        - project6-newerthan
+
+- project:
+    name: org/project7
+    older_than:
+      jobs:
+        - project7-olderthan

tests/unit/test_github_requirements.py

@@ -13,6 +13,8 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
+import time
+
 from tests.base import ZuulTestCase, simple_layout
 
 
@@ -171,3 +173,94 @@ class TestGithubRequirements(ZuulTestCase):
         self.waitUntilSettled()
         self.assertEqual(len(self.history), 1)
         self.assertEqual(self.history[0].name, 'project5-reviewuserstate')
+
+# TODO: Implement reject on approval username/state
+
+    @simple_layout('layouts/requirements-github.yaml', driver='github')
+    def test_pipeline_require_review_latest_user_state(self):
+        "Test pipeline requirement: review state from user"
+
+        A = self.fake_github.openFakePullRequest('org/project5', 'master', 'A')
+        # Add derp and herp to writers
+        A.writers.extend(('derp', 'herp'))
+        # 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
+        for i in range(1, 4):
+            submitted_at = time.time() - 72 * 60 * 60
+            A.addReview('derp', 'CHANGES_REQUESTED',
+                        submitted_at)
+            self.fake_github.emitEvent(comment)
+            self.waitUntilSettled()
+            self.assertEqual(len(self.history), 0)
+
+        # A positive review from derp should cause it to be enqueued
+        A.addReview('derp', 'APPROVED')
+        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')
+    def test_require_review_newer_than(self):
+
+        A = self.fake_github.openFakePullRequest('org/project6', 'master', 'A')
+        # Add derp and herp to writers
+        A.writers.extend(('derp', 'herp'))
+        # 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)
+
+        # Add a too-old positive review, should not be enqueued
+        submitted_at = time.time() - 72 * 60 * 60
+        A.addReview('derp', 'APPROVED',
+                    submitted_at)
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 0)
+
+        # Add a recent positive review
+        submitted_at = time.time() - 12 * 60 * 60
+        A.addReview('derp', 'APPROVED', submitted_at)
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, 'project6-newerthan')
+
+    @simple_layout('layouts/requirements-github.yaml', driver='github')
+    def test_require_review_older_than(self):
+
+        A = self.fake_github.openFakePullRequest('org/project7', 'master', 'A')
+        # Add derp and herp to writers
+        A.writers.extend(('derp', 'herp'))
+        # 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)
+
+        # Add a too-new positive, should not be enqueued
+        submitted_at = time.time() - 12 * 60 * 60
+        A.addReview('derp', 'APPROVED',
+                    submitted_at)
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 0)
+
+        # Add an old enough positive, should enqueue
+        submitted_at = time.time() - 72 * 60 * 60
+        A.addReview('herp', 'APPROVED', submitted_at)
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, 'project7-olderthan')

zuul/driver/github/githubconnection.py

@@ -434,29 +434,39 @@ class GithubConnection(BaseConnection):
 
         revs = self._getPullReviews(owner, proj, number)
 
-        reviews = []
+        reviews = {}
         for rev in revs:
+            user = rev.get('user').get('login')
             review = {
                 'by': {
-                    'username': rev.get('user').get('login'),
+                    'username': user,
                     'email': rev.get('user').get('email'),
                 },
-                'grantedOn': rev.get('provided'),
+                'grantedOn': int(time.mktime(self._ghTimestampToDate(
+                                             rev.get('submitted_at')))),
             }
 
             review['type'] = rev.get('state').lower()
+            review['submitted_at'] = rev.get('submitted_at')
 
             # Get user's rights. A user always has read to leave a review
             review['permission'] = 'read'
-            permission = self.getRepoPermission(
-                project.name, rev.get('user').get('login'))
+            permission = self.getRepoPermission(project.name, user)
             if permission == 'write':
                 review['permission'] = 'write'
             if permission == 'admin':
                 review['permission'] = 'admin'
 
-            reviews.append(review)
-        return reviews
+            if user not in reviews:
+                reviews[user] = review
+            else:
+                # if there are multiple reviews per user, keep the newest
+                # note that this breaks the ability to set the 'older-than'
+                # option on a review requirement.
+                if review['grantedOn'] > reviews[user]['grantedOn']:
+                    reviews[user] = review
+
+        return reviews.values()
 
     def _getPullReviews(self, owner, project, number):
         # make a list out of the reviews so that we complete our

zuul/driver/github/githubmodel.py

@@ -90,11 +90,11 @@ class GithubReviewFilter(object):
                         return False
             elif k == 'newer-than':
                 t = now - v
-                if (review['provided'] < t):
+                if (review['grantedOn'] < t):
                         return False
             elif k == 'older-than':
                 t = now - v
-                if (review['provided'] >= t):
+                if (review['grantedOn'] >= t):
                     return False
             elif k == 'type':
                 if review['type'] != v:

zuul/driver/github/githubsource.py

@@ -121,4 +121,5 @@ def getRequireSchema():
 
 
 def getRejectSchema():
-    return {}
+    reject = {'review': scalar_or_list(review)}
+    return reject