diff --git a/cron/poll_issue_close_stale.py b/cron/poll_issue_close_stale.py index 1826035b..c4aca483 100644 --- a/cron/poll_issue_close_stale.py +++ b/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)) diff --git a/cron/poll_pull_requests.py b/cron/poll_pull_requests.py index 97f09259..d1d3d267 100644 --- a/cron/poll_pull_requests.py +++ b/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) diff --git a/github_api/prs.py b/github_api/prs.py index cda7f8cc..0b9d72da 100644 --- a/github_api/prs.py +++ b/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) diff --git a/tests/github_api/prs_test.py b/tests/github_api/prs_test.py index facff6e0..df4c2084 100644 --- a/tests/github_api/prs_test.py +++ b/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,16 +103,18 @@ 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): @@ -97,9 +122,55 @@ def get_is_mergeable_side_effect(api, urn, pr_num): 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"))