merging PR Chaosthebot#420: Fix PR push times

Chaosthebot#420: Fix PR push times

Description:
Currently if you push on a different branch on your fork, it will reset the time for all PRs from that fork, even if they are completely unrelated. This issue aims to fix that.

:ok_woman: PR passed with a vote of 18 for and 0 against, a weighted total of 17.5 and a threshold of 6.5, and a current meritocracy review.

Vote record:
@Leigende: 1
@PlasmaPower: 1
@Smittyvb: 1
@SylvainThrd: 1
@andrewda: 1
@ankitiitb1069: 1
@anythingbot: 1
@eukaryote31: 1
@flibustier: 1
@hongaar: 1
@ike709: 1
@mark-i-m: 1
@md678685: 1
@phil-r: 1
@qgustavor: 1
@rhengles: 1
@rudehn: 1
@viktorsec: 1

cron/poll_issue_close_stale.py

@@ -16,7 +16,7 @@ def poll_issue_close_stale(api):
 
     __log.info("Checking for stale issues...")
 
-    # Get all issues
+    # Get the oldest open issues
     issues = gh.issues.get_oldest_open_issues(api, settings.URN)
 
     __log.info(str(issues))

cron/poll_pull_requests.py

@@ -63,7 +63,7 @@ def poll_pull_requests(api):
                 voting_window = gh.voting.get_extended_voting_window(api, settings.URN)
 
             # is our PR in voting window?
-            in_window = gh.prs.is_pr_in_voting_window(pr, voting_window)
+            in_window = gh.prs.is_pr_in_voting_window(api, pr, voting_window)
 
             if is_approved:
                 __log.info("PR %d status: will be approved", pr_num)

github_api/prs.py

@@ -123,14 +123,51 @@ def close_pr(api, urn, pr):
     return api("patch", path, json=data)
 
 
-def get_pr_last_updated(pr_data):
+def get_events(api, pr_owner, pr_repo):
+    """
+    a helper for getting the github events on a given repo... useful for
+    finding out the last push on a repo.
+    """
+    # TODO: this only gets us the latest 90 events, should we do more?
+    # i.e. can someone do 90 events on a repo in 30 seconds?
+    events = []
+    for i in range(1, 4):
+        path = "/repos/{pr_owner}/{pr_repo}/events?page={page}".format(
+                pr_owner=pr_owner, pr_repo=pr_repo, page=i)
+        events += api("get", path, json={})
+
+    return events
+
+
+def get_pr_last_updated(api, pr_data):
     """ a helper for finding the utc datetime of the last pr branch
     modifications """
-    repo = pr_data["head"]["repo"]
-    if repo:
-        return arrow.get(repo["pushed_at"])
-    else:
-        return None
+
+    pr_ref = pr_data["head"]["ref"]
+    pr_repo = pr_data["head"]["repo"]["name"]
+    pr_owner = pr_data["user"]["login"]
+
+    events = get_events(api, pr_owner, pr_repo)
+    events = filter(lambda e: e["type"] == "PushEvent", events)
+
+    # Gives the full ref name "ref/heads/my_branch_name", but we just
+    # want my_branch_name, so isolate it...
+    events = list(filter(lambda e: '/'.join(e["payload"]["ref"].split("/")[3:]) == pr_ref,
+                         events))
+
+    if len(events) == 0:
+        # if we can't get good data, fall back to repo push time
+        repo = pr_data["head"]["repo"]
+        if repo:
+            return max(arrow.get(repo["pushed_at"]),
+                       arrow.get(pr_data["created_at"]))
+        else:
+            return None
+
+    last_updated = max(sorted(map(lambda e: e["created_at"], events)))
+    last_updated = arrow.get(last_updated)
+
+    return max(last_updated, arrow.get(pr_data["created_at"]))
 
 
 def get_pr_comments(api, urn, pr_num):
@@ -177,7 +214,7 @@ def get_ready_prs(api, urn, window):
         pr_num = pr["number"]
 
         now = arrow.utcnow()
-        updated = get_pr_last_updated(pr)
+        updated = get_pr_last_updated(api, pr)
         if updated is None:
             comments.leave_deleted_comment(api, urn, pr["number"])
             close_pr(api, urn, pr)
@@ -212,17 +249,17 @@ def get_ready_prs(api, urn, window):
                 close_pr(api, urn, pr)
 
 
-def voting_window_remaining_seconds(pr, window):
+def voting_window_remaining_seconds(api, pr, window):
     now = arrow.utcnow()
-    updated = get_pr_last_updated(pr)
+    updated = get_pr_last_updated(api, pr)
     if updated is None:
         return math.inf
     delta = (now - updated).total_seconds()
     return window - delta
 
 
-def is_pr_in_voting_window(pr, window):
-    return voting_window_remaining_seconds(pr, window) <= 0
+def is_pr_in_voting_window(api, pr, window):
+    return voting_window_remaining_seconds(api, pr, window) <= 0
 
 
 def get_pr_reviews(api, urn, pr_num):
@@ -282,7 +319,7 @@ def post_accepted_status(api, urn, pr, voting_window, votes, total, threshold,
                          meritocracy_satisfied):
     sha = pr["head"]["sha"]
 
-    remaining_seconds = voting_window_remaining_seconds(pr, voting_window)
+    remaining_seconds = voting_window_remaining_seconds(api, pr, voting_window)
     remaining_human = misc.seconds_to_human(remaining_seconds)
     votes_summary = formatted_votes_short_summary(votes, total, threshold, meritocracy_satisfied)
 
@@ -294,7 +331,7 @@ def post_rejected_status(api, urn, pr, voting_window, votes, total, threshold,
                          meritocracy_satisfied):
     sha = pr["head"]["sha"]
 
-    remaining_seconds = voting_window_remaining_seconds(pr, voting_window)
+    remaining_seconds = voting_window_remaining_seconds(api, pr, voting_window)
     remaining_human = misc.seconds_to_human(remaining_seconds)
     votes_summary = formatted_votes_short_summary(votes, total, threshold, meritocracy_satisfied)
 
@@ -306,7 +343,7 @@ def post_pending_status(api, urn, pr, voting_window, votes, total, threshold,
                         meritocracy_satisfied):
     sha = pr["head"]["sha"]
 
-    remaining_seconds = voting_window_remaining_seconds(pr, voting_window)
+    remaining_seconds = voting_window_remaining_seconds(api, pr, voting_window)
     remaining_human = misc.seconds_to_human(remaining_seconds)
     votes_summary = formatted_votes_short_summary(votes, total, threshold, meritocracy_satisfied)
 

tests/github_api/prs_test.py

@@ -5,19 +5,42 @@
 from github_api import prs, API
 
 
-def create_mock_pr(number, title, pushed_at):
+def create_mock_pr(number, title, pushed_at, created_at):
     return {
         "number": number,
         "title": title,
         "statuses_url": "statuses_url/{}".format(number),
         "head": {
             "repo": {
-                "pushed_at": pushed_at
-            }
-        }
+                "pushed_at": pushed_at,
+                "name": "test_repo",
+            },
+            "ref": "test_ref"
+        },
+        "user": {
+            "login": "test_user",
+        },
+        "created_at": created_at,
     }
 
 
+def create_mock_events(events):
+
+    def produce_event(event):
+        if event[0] == "PushEvent":
+            return {
+                    "type": "PushEvent",
+                    "payload": {
+                        "ref": "/ref/heads/%s" % event[1],
+                        },
+                    "created_at": event[2],
+                    }
+        else:
+            return {"type": event[0]}
+
+    return list(map(produce_event, events))
+
+
 class TestPRMethods(unittest.TestCase):
     def test_statuses_returns_passed_travis_build(self):
         test_data = [[{"state": "success",
@@ -80,26 +103,74 @@ def __call__(m, method, path, **kwargs):
 
             self.assertFalse(prs.has_build_passed(api, url))
 
-    @patch("arrow.utcnow")
-    @patch("github_api.prs.get_is_mergeable")
+    @patch("github_api.prs.get_events")
     @patch("github_api.prs.get_open_prs")
-    def test_get_ready_prs(self, mock_get_open_prs, mock_get_is_mergeable, mock_utcnow):
+    @patch("github_api.prs.get_is_mergeable")
+    @patch("arrow.utcnow")
+    def test_get_ready_prs(self, mock_utcnow, mock_get_is_mergeable,
+                           mock_get_open_prs, mock_get_events):
         mock_get_open_prs.return_value = [
-            create_mock_pr(10, "WIP", "2017-01-01T00:00:00Z"),
-            create_mock_pr(11, "OK", "2017-01-01T00:00:00Z"),
-            create_mock_pr(12, "Not in window", "2017-01-01T00:00:10Z"),
-            create_mock_pr(13, "Not mergeable", "2017-01-01T00:00:00Z"),
-            create_mock_pr(14, "Stale", "2016-01-01T00:00:00Z")
+            create_mock_pr(10, "WIP", "2017-01-01T00:00:00Z", "2017-01-01T00:00:00Z"),
+            create_mock_pr(11, "OK", "2017-01-01T00:00:00Z", "2017-01-01T00:00:00Z"),
+            create_mock_pr(12, "Not in window", "2017-01-01T00:00:10Z", "2017-01-01T00:00:00Z"),
+            create_mock_pr(13, "Not mergeable", "2017-01-01T00:00:00Z", "2017-01-01T00:00:00Z"),
+            create_mock_pr(14, "Stale", "2016-01-01T00:00:00Z", "2017-01-01T00:00:00Z")
         ]
 
         def get_is_mergeable_side_effect(api, urn, pr_num):
             return False if pr_num in [13, 14] else True
 
         mock_get_is_mergeable.side_effect = get_is_mergeable_side_effect
         mock_utcnow.return_value = arrow.get("2017-01-01T00:00:10Z")
+        mock_get_events.return_value = []
         api = MagicMock()
         api.BASE_URL = "api_base_url"
         ready_prs = prs.get_ready_prs(api, "urn", 5)
         ready_prs_list = [pr for pr in ready_prs]
-        self.assertTrue(len(ready_prs_list) is 1)
-        self.assertTrue(ready_prs_list[0].get("number") is 11)
+        self.assertEqual(len(ready_prs_list), 1)
+        self.assertEqual(ready_prs_list[0].get("number"), 11)
+
+    @patch("github_api.prs.get_events")
+    def test_get_pr_last_updated_with_early_events(self, mock_get_events):
+        mock_get_events.return_value = \
+            create_mock_events([("PushEvent", "test_ref", "2017-01-01T00:00:10Z"),
+                                ("PushEvent", "blah", "2017-01-03T00:00:10Z")])
+
+        api = MagicMock()
+        api.BASE_URL = "api_base_url"
+
+        last_updated = prs.get_pr_last_updated(api,
+                                               create_mock_pr(10, "OK", "2017-01-05T00:00:00Z",
+                                                              "2017-01-02T00:00:00Z"))
+
+        self.assertEqual(last_updated, arrow.get("2017-01-02T00:00:00Z"))
+
+    @patch("github_api.prs.get_events")
+    def test_get_pr_last_updated_without_events(self, mock_get_events):
+        mock_get_events.return_value = \
+                create_mock_events([("PublicEvent",),
+                                    ("PushEvent", "blah", "2017-01-03T00:00:10Z")])
+
+        api = MagicMock()
+        api.BASE_URL = "api_base_url"
+
+        last_updated = prs.get_pr_last_updated(api,
+                                               create_mock_pr(10, "OK", "2017-01-05T00:00:00Z",
+                                                              "2017-01-02T00:00:00Z"))
+
+        self.assertEqual(last_updated, arrow.get("2017-01-05T00:00:00Z"))
+
+    @patch("github_api.prs.get_events")
+    def test_get_pr_last_updated_with_events(self, mock_get_events):
+        mock_get_events.return_value = \
+            create_mock_events([("PushEvent", "test_ref", "2017-01-04T00:00:10Z"),
+                                ("PushEvent", "blah", "2017-01-03T00:00:10Z")])
+
+        api = MagicMock()
+        api.BASE_URL = "api_base_url"
+
+        last_updated = prs.get_pr_last_updated(api,
+                                               create_mock_pr(10, "OK", "2017-01-05T00:00:00Z",
+                                                              "2017-01-02T00:00:00Z"))
+
+        self.assertEqual(last_updated, arrow.get("2017-01-04T00:00:10Z"))