From 699ecda3cef16a33f0c9cc9df16baa4ccd89c5ff Mon Sep 17 00:00:00 2001 From: Camelid Date: Sun, 6 Dec 2020 13:34:23 -0800 Subject: [PATCH 01/10] Gracefully handle `r?` of invalid user Don't crash; instead post a comment saying that the user couldn't be assigned. Also handle `r? @ghost` properly: treat it as clearing all assignees but still do the other things that highfive normally does (e.g. setting `S-waiting-on-review`). --- highfive/newpr.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/highfive/newpr.py b/highfive/newpr.py index 223570e0..3be20f93 100644 --- a/highfive/newpr.py +++ b/highfive/newpr.py @@ -105,16 +105,21 @@ def api_req(self, method, url, data=None, media_type=None): def set_assignee(self, assignee, owner, repo, issue, user, author, to_mention): try: + assignees = [] if assignee == 'ghost' else [assignee] + self.api_req( "PATCH", issue_url % (owner, repo, issue), - {"assignee": assignee} + {"assignees": assignees} )['body'] except urllib.error.HTTPError as e: if e.code == 201: pass else: print(f"failed to assign {assignee} to {owner}/{repo}#{issue}") - raise e + print(f"error was: {e}") + print("posting error comment") + error_msg = f":stop_sign: @{assignee} could not be assigned" + self.post_comment(error_msg, owner, repo, issue) self.run_commands(to_mention, owner, repo, issue, user) From 9795c20a738c4e1da2fbeffc0f8c6204bbe74300 Mon Sep 17 00:00:00 2001 From: Camelid Date: Sun, 6 Dec 2020 13:41:14 -0800 Subject: [PATCH 02/10] Fix tests --- highfive/tests/test_integration_tests.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/highfive/tests/test_integration_tests.py b/highfive/tests/test_integration_tests.py index 072b7418..df6d8750 100644 --- a/highfive/tests/test_integration_tests.py +++ b/highfive/tests/test_integration_tests.py @@ -88,7 +88,7 @@ def test_new_pr_non_contributor(self): ( ( 'PATCH', newpr.issue_url % ('rust-lang', 'rust', '7'), - {'assignee': 'nrc'} + {'assignees': ['nrc']} ), {'body': {}}, ), @@ -130,7 +130,7 @@ def test_new_pr_empty_body(self): ( ( 'PATCH', newpr.issue_url % ('rust-lang', 'rust', '7'), - {'assignee': 'nrc'} + {'assignees': ['nrc']} ), {'body': {}}, ), @@ -171,7 +171,7 @@ def test_new_pr_contributor(self): ( ( 'PATCH', newpr.issue_url % ('rust-lang', 'rust', '7'), - {'assignee': 'nrc'} + {'assignees': ['nrc']} ), {'body': {}}, ), @@ -215,7 +215,7 @@ def test_new_pr_contributor_with_labels(self): ( ( 'PATCH', newpr.issue_url % ('rust-lang', 'rust', '7'), - {'assignee': 'nrc'} + {'assignees': ['nrc']} ), {'body': {}}, ), @@ -266,7 +266,7 @@ def test_author_is_commenter(self): ( ( 'PATCH', newpr.issue_url % ('rust-lang', 'rust', '1'), - {'assignee': 'davidalber'} + {'assignees': ['davidalber']} ), {'body': {}}, ), @@ -293,7 +293,7 @@ def test_author_not_commenter_is_collaborator(self): ( ( 'PATCH', newpr.issue_url % ('rust-lang', 'rust', '1'), - {'assignee': 'davidalber'} + {'assignees': ['davidalber']} ), {'body': {}}, ), From 6130f0a5db34e1964acedeb085e185be802311f1 Mon Sep 17 00:00:00 2001 From: Camelid Date: Sun, 6 Dec 2020 13:47:58 -0800 Subject: [PATCH 03/10] Use raw strings for regexes to avoid deprecation warnings --- highfive/newpr.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/highfive/newpr.py b/highfive/newpr.py index 3be20f93..ecdd63d5 100644 --- a/highfive/newpr.py +++ b/highfive/newpr.py @@ -34,8 +34,8 @@ review_with_reviewer = 'r? @%s\n\n(rust-highfive has picked a reviewer for you, use r? to override)' review_without_reviewer = '@%s: no appropriate reviewer found, use r? to override' -reviewer_re = re.compile("\\b[rR]\?[:\- ]*@([a-zA-Z0-9\-]+)") -submodule_re = re.compile(".*\+Subproject\scommit\s.*", re.DOTALL | re.MULTILINE) +reviewer_re = re.compile(r"\\b[rR]\?[:\- ]*@([a-zA-Z0-9\-]+)") +submodule_re = re.compile(r".*\+Subproject\scommit\s.*", re.DOTALL | re.MULTILINE) rustaceans_api_url = "http://www.ncameron.org/rustaceans/user?username={username}" From a98b249827c14cd2491cdc49d75df1ac5090729a Mon Sep 17 00:00:00 2001 From: Camelid Date: Mon, 7 Dec 2020 14:01:23 -0800 Subject: [PATCH 04/10] Use old behavior (crashing) for `r? @ghost` Apparently people prefer the old `r? @ghost` behavior. Now the only difference is that `r? @ghost` will unassign anyone already assigned (e.g. if you use `r? @ghost` after highfive has already assigned someone). --- highfive/newpr.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/highfive/newpr.py b/highfive/newpr.py index ecdd63d5..24d78c1e 100644 --- a/highfive/newpr.py +++ b/highfive/newpr.py @@ -117,6 +117,8 @@ def set_assignee(self, assignee, owner, repo, issue, user, author, to_mention): else: print(f"failed to assign {assignee} to {owner}/{repo}#{issue}") print(f"error was: {e}") + if assignee == 'ghost': + raise Exception('assigned ghost') print("posting error comment") error_msg = f":stop_sign: @{assignee} could not be assigned" self.post_comment(error_msg, owner, repo, issue) From f70f21d85300ae404453dc1bd7518fd35fe99011 Mon Sep 17 00:00:00 2001 From: Camelid Date: Mon, 7 Dec 2020 14:10:32 -0800 Subject: [PATCH 05/10] Use more helpful error message --- highfive/newpr.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/highfive/newpr.py b/highfive/newpr.py index 24d78c1e..49f8ec85 100644 --- a/highfive/newpr.py +++ b/highfive/newpr.py @@ -120,7 +120,9 @@ def set_assignee(self, assignee, owner, repo, issue, user, author, to_mention): if assignee == 'ghost': raise Exception('assigned ghost') print("posting error comment") - error_msg = f":stop_sign: @{assignee} could not be assigned" + error_msg = f":stop_sign: @{assignee} could not be assigned. " \ + "(They may not be a member of **`@rust-lang`**!) " \ + "Please assign someone else with `r? @person`." self.post_comment(error_msg, owner, repo, issue) self.run_commands(to_mention, owner, repo, issue, user) From f7e89e3d8cce0b6e7bc3359af51e43f852a2484b Mon Sep 17 00:00:00 2001 From: Camelid Date: Mon, 7 Dec 2020 14:12:29 -0800 Subject: [PATCH 06/10] Attempt to fix tests --- highfive/newpr.py | 4 ++-- highfive/tests/test_newpr.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/highfive/newpr.py b/highfive/newpr.py index 49f8ec85..57b1e33c 100644 --- a/highfive/newpr.py +++ b/highfive/newpr.py @@ -427,8 +427,8 @@ def new_comment(self): # Check the commenter is the submitter of the PR or the previous assignee. author = self.payload['issue', 'user', 'login'] if not (author == commenter or ( - self.payload['issue', 'assignee'] \ - and commenter == self.payload['issue', 'assignee', 'login'] + self.payload['issue', 'assignees'] \ + and commenter == self.payload['issue', 'assignees', 'login'] )): # Check if commenter is a collaborator. if not self.is_collaborator(commenter, owner, repo): diff --git a/highfive/tests/test_newpr.py b/highfive/tests/test_newpr.py index cd6a892a..013482a8 100644 --- a/highfive/tests/test_newpr.py +++ b/highfive/tests/test_newpr.py @@ -908,7 +908,7 @@ def make_handler( 'issue': { 'state': state, 'number': issue_number, - 'assignee': None, + 'assignees': [], 'user': { 'login': author, }, @@ -930,7 +930,7 @@ def make_handler( if is_pull_request: payload._payload['issue']['pull_request'] = {} if assignee is not None: - payload._payload['issue']['assignee'] = {'login': assignee} + payload._payload['issue']['assignees'] = [{'login': assignee}] return HighfiveHandlerMock(payload).handler From 302e12bd82c7e366bbb9d7c769506da7e33a1047 Mon Sep 17 00:00:00 2001 From: Camelid Date: Mon, 7 Dec 2020 14:17:32 -0800 Subject: [PATCH 07/10] Fix payload indexing --- highfive/newpr.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/highfive/newpr.py b/highfive/newpr.py index 57b1e33c..a64cbb14 100644 --- a/highfive/newpr.py +++ b/highfive/newpr.py @@ -428,7 +428,7 @@ def new_comment(self): author = self.payload['issue', 'user', 'login'] if not (author == commenter or ( self.payload['issue', 'assignees'] \ - and commenter == self.payload['issue', 'assignees', 'login'] + and commenter == self.payload['issue', 'assignees', 0, 'login'] )): # Check if commenter is a collaborator. if not self.is_collaborator(commenter, owner, repo): From 417ff9edbdba0de466eae25b785c0ba46ac61b8e Mon Sep 17 00:00:00 2001 From: Camelid Date: Mon, 7 Dec 2020 14:20:29 -0800 Subject: [PATCH 08/10] Actually fix assignee payload handling --- highfive/newpr.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/highfive/newpr.py b/highfive/newpr.py index a64cbb14..fbbd2d96 100644 --- a/highfive/newpr.py +++ b/highfive/newpr.py @@ -428,7 +428,7 @@ def new_comment(self): author = self.payload['issue', 'user', 'login'] if not (author == commenter or ( self.payload['issue', 'assignees'] \ - and commenter == self.payload['issue', 'assignees', 0, 'login'] + and commenter in self.payload['issue', 'assignees', 'login'] )): # Check if commenter is a collaborator. if not self.is_collaborator(commenter, owner, repo): From cef777316594d82713ad142f4580fbeb1e8ea880 Mon Sep 17 00:00:00 2001 From: Camelid Date: Mon, 7 Dec 2020 14:22:03 -0800 Subject: [PATCH 09/10] Fix some more test stuff --- highfive/tests/test_newpr.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/highfive/tests/test_newpr.py b/highfive/tests/test_newpr.py index 013482a8..31ca7b4a 100644 --- a/highfive/tests/test_newpr.py +++ b/highfive/tests/test_newpr.py @@ -511,7 +511,7 @@ def assert_api_req_call(self, assignee=''): 'PATCH', 'https://api.github.com/repos/%s/%s/issues/%s' % ( self.owner, self.repo, self.issue - ), {"assignee": assignee} + ), {"assignees": [assignee]} ) def test_api_req_good(self): From d2581d29d1134cf862d54200d041f0127cd42499 Mon Sep 17 00:00:00 2001 From: Camelid Date: Mon, 7 Dec 2020 14:35:56 -0800 Subject: [PATCH 10/10] More test fixing --- highfive/newpr.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/highfive/newpr.py b/highfive/newpr.py index fbbd2d96..606ce48e 100644 --- a/highfive/newpr.py +++ b/highfive/newpr.py @@ -428,7 +428,7 @@ def new_comment(self): author = self.payload['issue', 'user', 'login'] if not (author == commenter or ( self.payload['issue', 'assignees'] \ - and commenter in self.payload['issue', 'assignees', 'login'] + and commenter in map(lambda user: user['login'], self.payload['issue', 'assignees']) )): # Check if commenter is a collaborator. if not self.is_collaborator(commenter, owner, repo):