From 1d805b447a833582644c803ea14efd49da5fc83d Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Mon, 30 Mar 2020 12:37:01 -0400 Subject: [PATCH 1/2] Ignore new comments This removes the handling of `r?`. --- highfive/newpr.py | 37 ---- highfive/tests/fakes.py | 6 - highfive/tests/fakes/create-comment.payload | 190 -------------------- highfive/tests/test_integration_tests.py | 42 ----- highfive/tests/test_newpr.py | 146 --------------- 5 files changed, 421 deletions(-) delete mode 100644 highfive/tests/fakes/create-comment.payload diff --git a/highfive/newpr.py b/highfive/newpr.py index a3345746..e8439ed7 100644 --- a/highfive/newpr.py +++ b/highfive/newpr.py @@ -68,9 +68,6 @@ def run(self, event): elif event == "pull_request" and self.payload["action"] == "opened": self.new_pr() return 'OK\n' - elif event == "issue_comment" and self.payload["action"] == "created": - self.new_comment() - return 'OK\n' else: return 'Unsupported webhook event.\n' @@ -398,37 +395,3 @@ def new_pr(self): if self.repo_config.get("new_pr_labels"): self.add_labels(owner, repo, issue) - - def new_comment(self): - # Check the issue is a PR and is open. - if self.payload['issue', 'state'] != 'open' \ - or 'pull_request' not in self.payload['issue']: - return - - commenter = self.payload['comment', 'user', 'login'] - # Ignore our own comments. - if commenter == self.integration_user: - return - - owner = self.payload['repository', 'owner', 'login'] - repo = self.payload['repository', 'name'] - - # 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'] - )): - # Check if commenter is a collaborator. - if not self.is_collaborator(commenter, owner, repo): - return - - # Check for r? and set the assignee. - msg = self.payload['comment', 'body'] - reviewer = self.find_reviewer(msg) - if reviewer: - issue = str(self.payload['issue', 'number']) - self.set_assignee( - reviewer, owner, repo, issue, self.integration_user, - author, None - ) diff --git a/highfive/tests/fakes.py b/highfive/tests/fakes.py index d2c941a6..d74408a7 100644 --- a/highfive/tests/fakes.py +++ b/highfive/tests/fakes.py @@ -65,12 +65,6 @@ def get_global_configs(): class Payload(object): - @staticmethod - def new_comment(): - with open(get_fake_filename('create-comment.payload'), 'r') as fin: - p = json.load(fin) - return payload.Payload(p) - @staticmethod def new_pr( number=7, pr_body='The PR comment.', pr_url='https://the.url/', diff --git a/highfive/tests/fakes/create-comment.payload b/highfive/tests/fakes/create-comment.payload deleted file mode 100644 index 2e10070e..00000000 --- a/highfive/tests/fakes/create-comment.payload +++ /dev/null @@ -1,190 +0,0 @@ -{ - "action": "created", - "issue": { - "url": "https://api.github.com/repos/rust-lang/rust/issues/1", - "repository_url": "https://api.github.com/repos/rust-lang/rust", - "labels_url": "https://api.github.com/repos/rust-lang/rust/issues/1/labels{/name}", - "comments_url": "https://api.github.com/repos/rust-lang/rust/issues/1/comments", - "events_url": "https://api.github.com/repos/rust-lang/rust/issues/1/events", - "html_url": "https://github.com/rust-lang/rust/pull/1", - "id": 300114954, - "number": 1, - "title": "Test PR", - "user": { - "login": "davidalber", - "id": 933552, - "avatar_url": "https://avatars3.githubusercontent.com/u/933552?v=4", - "gravatar_id": "", - "url": "https://api.github.com/users/davidalber", - "html_url": "https://github.com/davidalber", - "followers_url": "https://api.github.com/users/davidalber/followers", - "following_url": "https://api.github.com/users/davidalber/following{/other_user}", - "gists_url": "https://api.github.com/users/davidalber/gists{/gist_id}", - "starred_url": "https://api.github.com/users/davidalber/starred{/owner}{/repo}", - "subscriptions_url": "https://api.github.com/users/davidalber/subscriptions", - "organizations_url": "https://api.github.com/users/davidalber/orgs", - "repos_url": "https://api.github.com/users/davidalber/repos", - "events_url": "https://api.github.com/users/davidalber/events{/privacy}", - "received_events_url": "https://api.github.com/users/davidalber/received_events", - "type": "User", - "site_admin": false - }, - "labels": [], - "state": "open", - "locked": false, - "assignee": null, - "assignees": [], - "milestone": null, - "comments": 0, - "created_at": "2018-02-26T05:35:05Z", - "updated_at": "2018-02-26T05:38:24Z", - "closed_at": null, - "author_association": "CONTRIBUTOR", - "pull_request": { - "url": "https://api.github.com/repos/rust-lang/rust/pulls/1", - "html_url": "https://github.com/rust-lang/rust/pull/1", - "diff_url": "https://github.com/rust-lang/rust/pull/1.diff", - "patch_url": "https://github.com/rust-lang/rust/pull/1.patch" - }, - "body": "This is a fake PR for unit testing highfive." - }, - "comment": { - "url": "https://api.github.com/repos/rust-lang/rust/issues/comments/368395486", - "html_url": "https://github.com/rust-lang/rust/pull/1#issuecomment-368395486", - "issue_url": "https://api.github.com/repos/rust-lang/rust/issues/1", - "id": 368395486, - "user": { - "login": "davidalber", - "id": 933552, - "avatar_url": "https://avatars3.githubusercontent.com/u/933552?v=4", - "gravatar_id": "", - "url": "https://api.github.com/users/davidalber", - "html_url": "https://github.com/davidalber", - "followers_url": "https://api.github.com/users/davidalber/followers", - "following_url": "https://api.github.com/users/davidalber/following{/other_user}", - "gists_url": "https://api.github.com/users/davidalber/gists{/gist_id}", - "starred_url": "https://api.github.com/users/davidalber/starred{/owner}{/repo}", - "subscriptions_url": "https://api.github.com/users/davidalber/subscriptions", - "organizations_url": "https://api.github.com/users/davidalber/orgs", - "repos_url": "https://api.github.com/users/davidalber/repos", - "events_url": "https://api.github.com/users/davidalber/events{/privacy}", - "received_events_url": "https://api.github.com/users/davidalber/received_events", - "type": "User", - "site_admin": false - }, - "created_at": "2018-02-26T05:38:24Z", - "updated_at": "2018-02-26T05:38:24Z", - "author_association": "OWNER", - "body": "r? @davidalber" - }, - "repository": { - "id": 120859723, - "name": "rust", - "full_name": "rust-lang/rust", - "owner": { - "login": "rust-lang", - "id": 5430905, - "avatar_url": "https://avatars1.githubusercontent.com/u/5430905?v=4", - "gravatar_id": "", - "url": "https://api.github.com/users/rust-lang", - "html_url": "https://github.com/rust-lang", - "followers_url": "https://api.github.com/users/rust-lang/followers", - "following_url": "https://api.github.com/users/rust-lang/following{/other_user}", - "gists_url": "https://api.github.com/users/rust-lang/gists{/gist_id}", - "starred_url": "https://api.github.com/users/rust-lang/starred{/owner}{/repo}", - "subscriptions_url": "https://api.github.com/users/rust-lang/subscriptions", - "organizations_url": "https://api.github.com/users/rust-lang/orgs", - "repos_url": "https://api.github.com/users/rust-lang/repos", - "events_url": "https://api.github.com/users/rust-lang/events{/privacy}", - "received_events_url": "https://api.github.com/users/rust-lang/received_events", - "type": "Organization", - "site_admin": false - }, - "private": false, - "html_url": "https://github.com/rust-lang/rust", - "description": "The Rust Language", - "fork": true, - "url": "https://api.github.com/repos/rust-lang/rust", - "forks_url": "https://api.github.com/repos/rust-lang/rust/forks", - "keys_url": "https://api.github.com/repos/rust-lang/rust/keys{/key_id}", - "collaborators_url": "https://api.github.com/repos/rust-lang/rust/collaborators{/collaborator}", - "teams_url": "https://api.github.com/repos/rust-lang/rust/teams", - "hooks_url": "https://api.github.com/repos/rust-lang/rust/hooks", - "issue_events_url": "https://api.github.com/repos/rust-lang/rust/issues/events{/number}", - "events_url": "https://api.github.com/repos/rust-lang/rust/events", - "assignees_url": "https://api.github.com/repos/rust-lang/rust/assignees{/user}", - "branches_url": "https://api.github.com/repos/rust-lang/rust/branches{/branch}", - "tags_url": "https://api.github.com/repos/rust-lang/rust/tags", - "blobs_url": "https://api.github.com/repos/rust-lang/rust/git/blobs{/sha}", - "git_tags_url": "https://api.github.com/repos/rust-lang/rust/git/tags{/sha}", - "git_refs_url": "https://api.github.com/repos/rust-lang/rust/git/refs{/sha}", - "trees_url": "https://api.github.com/repos/rust-lang/rust/git/trees{/sha}", - "statuses_url": "https://api.github.com/repos/rust-lang/rust/statuses/{sha}", - "languages_url": "https://api.github.com/repos/rust-lang/rust/languages", - "stargazers_url": "https://api.github.com/repos/rust-lang/rust/stargazers", - "contributors_url": "https://api.github.com/repos/rust-lang/rust/contributors", - "subscribers_url": "https://api.github.com/repos/rust-lang/rust/subscribers", - "subscription_url": "https://api.github.com/repos/rust-lang/rust/subscription", - "commits_url": "https://api.github.com/repos/rust-lang/rust/commits{/sha}", - "git_commits_url": "https://api.github.com/repos/rust-lang/rust/git/commits{/sha}", - "comments_url": "https://api.github.com/repos/rust-lang/rust/comments{/number}", - "issue_comment_url": "https://api.github.com/repos/rust-lang/rust/issues/comments{/number}", - "contents_url": "https://api.github.com/repos/rust-lang/rust/contents/{+path}", - "compare_url": "https://api.github.com/repos/rust-lang/rust/compare/{base}...{head}", - "merges_url": "https://api.github.com/repos/rust-lang/rust/merges", - "archive_url": "https://api.github.com/repos/rust-lang/rust/{archive_format}{/ref}", - "downloads_url": "https://api.github.com/repos/rust-lang/rust/downloads", - "issues_url": "https://api.github.com/repos/rust-lang/rust/issues{/number}", - "pulls_url": "https://api.github.com/repos/rust-lang/rust/pulls{/number}", - "milestones_url": "https://api.github.com/repos/rust-lang/rust/milestones{/number}", - "notifications_url": "https://api.github.com/repos/rust-lang/rust/notifications{?since,all,participating}", - "labels_url": "https://api.github.com/repos/rust-lang/rust/labels{/name}", - "releases_url": "https://api.github.com/repos/rust-lang/rust/releases{/id}", - "deployments_url": "https://api.github.com/repos/rust-lang/rust/deployments", - "created_at": "2018-02-09T05:11:02Z", - "updated_at": "2018-02-09T05:11:04Z", - "pushed_at": "2018-02-26T05:35:06Z", - "git_url": "git://github.com/rust-lang/rust.git", - "ssh_url": "git@github.com:rust-lang/rust.git", - "clone_url": "https://github.com/rust-lang/rust.git", - "svn_url": "https://github.com/rust-lang/rust", - "homepage": null, - "size": 133, - "stargazers_count": 0, - "watchers_count": 0, - "language": "Rust", - "has_issues": false, - "has_projects": true, - "has_downloads": true, - "has_wiki": true, - "has_pages": false, - "forks_count": 0, - "mirror_url": null, - "archived": false, - "open_issues_count": 1, - "license": null, - "forks": 0, - "open_issues": 1, - "watchers": 0, - "default_branch": "master" - }, - "sender": { - "login": "davidalber", - "id": 933552, - "avatar_url": "https://avatars3.githubusercontent.com/u/933552?v=4", - "gravatar_id": "", - "url": "https://api.github.com/users/davidalber", - "html_url": "https://github.com/davidalber", - "followers_url": "https://api.github.com/users/davidalber/followers", - "following_url": "https://api.github.com/users/davidalber/following{/other_user}", - "gists_url": "https://api.github.com/users/davidalber/gists{/gist_id}", - "starred_url": "https://api.github.com/users/davidalber/starred{/owner}{/repo}", - "subscriptions_url": "https://api.github.com/users/davidalber/subscriptions", - "organizations_url": "https://api.github.com/users/davidalber/orgs", - "repos_url": "https://api.github.com/users/davidalber/repos", - "events_url": "https://api.github.com/users/davidalber/events{/privacy}", - "received_events_url": "https://api.github.com/users/davidalber/received_events", - "type": "User", - "site_admin": false - } -} diff --git a/highfive/tests/test_integration_tests.py b/highfive/tests/test_integration_tests.py index 81ad904f..f6a51b67 100644 --- a/highfive/tests/test_integration_tests.py +++ b/highfive/tests/test_integration_tests.py @@ -258,45 +258,3 @@ def make_mocks(cls, patcherize): config_mock = mock.Mock() config_mock.get.side_effect = ('integration-user', 'integration-token') cls.mocks['ConfigParser'].RawConfigParser.return_value = config_mock - - def test_author_is_commenter(self): - payload = fakes.Payload.new_comment() - handler = newpr.HighfiveHandler(payload, dummy_config()) - api_req_mock = ApiReqMocker([ - ( - ( - 'PATCH', newpr.issue_url % ('rust-lang', 'rust', '1'), - {'assignee': 'davidalber'} - ), - {'body': {}}, - ), - ]) - handler.new_comment() - api_req_mock.verify_calls() - - def test_author_not_commenter_is_collaborator(self): - payload = fakes.Payload.new_comment() - payload._payload['issue']['user']['login'] = 'foouser' - - handler = newpr.HighfiveHandler(payload, dummy_config()) - api_req_mock = ApiReqMocker([ - ( - ( - "GET", - newpr.user_collabo_url % ( - 'rust-lang', 'rust', 'davidalber' - ), - None - ), - {'body': {}}, - ), - ( - ( - 'PATCH', newpr.issue_url % ('rust-lang', 'rust', '1'), - {'assignee': 'davidalber'} - ), - {'body': {}}, - ), - ]) - handler.new_comment() - api_req_mock.verify_calls() diff --git a/highfive/tests/test_newpr.py b/highfive/tests/test_newpr.py index 4c1b8635..8750f595 100644 --- a/highfive/tests/test_newpr.py +++ b/highfive/tests/test_newpr.py @@ -889,142 +889,6 @@ def test_empty_pr_labels(self): self.mocks['add_labels'].assert_not_called() -class TestNewComment(TestNewPR): - @pytest.fixture(autouse=True) - def make_mocks(cls, patcherize): - cls.mocks = patcherize(( - ('is_collaborator', 'highfive.newpr.HighfiveHandler.is_collaborator'), - ('find_reviewer', 'highfive.newpr.HighfiveHandler.find_reviewer'), - ('set_assignee', 'highfive.newpr.HighfiveHandler.set_assignee'), - )) - - @staticmethod - def make_handler( - state='open', is_pull_request=True, commenter='userA', - repo='repo-name', owner='repo-owner', author='userB', - comment='comment!', issue_number=7, assignee=None - ): - payload = Payload({ - 'issue': { - 'state': state, - 'number': issue_number, - 'assignee': None, - 'user': { - 'login': author, - }, - }, - 'comment': { - 'user': { - 'login': commenter, - }, - 'body': comment, - }, - 'repository': { - 'name': repo, - 'owner': { - 'login': owner, - }, - }, - }) - - if is_pull_request: - payload._payload['issue']['pull_request'] = {} - if assignee is not None: - payload._payload['issue']['assignee'] = {'login': assignee} - - return HighfiveHandlerMock(payload).handler - - def test_not_open(self): - handler = self.make_handler(state='closed') - - assert handler.new_comment() is None - self.mocks['is_collaborator'].assert_not_called() - self.mocks['find_reviewer'].assert_not_called() - self.mocks['set_assignee'].assert_not_called() - - def test_not_pr(self): - handler = self.make_handler(is_pull_request=False) - - handler.new_comment() is None - self.mocks['is_collaborator'].assert_not_called() - self.mocks['find_reviewer'].assert_not_called() - self.mocks['set_assignee'].assert_not_called() - - def test_commenter_is_integration_user(self): - handler = self.make_handler(commenter='integrationUser') - - assert handler.new_comment() is None - self.mocks['is_collaborator'].assert_not_called() - self.mocks['find_reviewer'].assert_not_called() - self.mocks['set_assignee'].assert_not_called() - - def test_unauthorized_assigner(self): - handler = self.make_handler( - author='userA', commenter='userB', assignee='userC' - ) - - self.mocks['is_collaborator'].return_value = False - assert handler.new_comment() is None - self.mocks['is_collaborator'].assert_called_with( - 'userB', 'repo-owner', 'repo-name' - ) - self.mocks['find_reviewer'].assert_not_called() - self.mocks['set_assignee'].assert_not_called() - - # There are three ways to make it past the authorized assigner - # check. The next three methods excercise those paths. - def test_authorized_assigner_author_is_commenter(self): - handler = self.make_handler( - author='userA', commenter='userA', assignee='userC' - ) - - handler.new_comment() - self.mocks['is_collaborator'].assert_not_called() - self.mocks['find_reviewer'].assert_called() - - def test_authorized_assigner_commenter_is_assignee(self): - handler = self.make_handler( - author='userA', commenter='userB', assignee='userB' - ) - - handler.new_comment() - self.mocks['is_collaborator'].assert_not_called() - self.mocks['find_reviewer'].assert_called() - - def test_authorized_assigner_commenter_is_collaborator(self): - handler = self.make_handler( - author='userA', commenter='userB', assignee='userC' - ) - - self.mocks['is_collaborator'].return_value = True - handler.new_comment() - self.mocks['is_collaborator'].assert_called_with( - 'userB', 'repo-owner', 'repo-name' - ) - self.mocks['find_reviewer'].assert_called() - - def test_no_reviewer(self): - handler = self.make_handler(author='userA', commenter='userA') - - self.mocks['find_reviewer'].return_value = None - handler.new_comment() - self.mocks['is_collaborator'].assert_not_called() - self.mocks['find_reviewer'].assert_called_with('comment!') - self.mocks['set_assignee'].assert_not_called() - - def test_has_reviewer(self): - handler = self.make_handler(author='userA', commenter='userA') - - self.mocks['find_reviewer'].return_value = 'userD' - handler.new_comment() - self.mocks['is_collaborator'].assert_not_called() - self.mocks['find_reviewer'].assert_called_with('comment!') - self.mocks['set_assignee'].assert_called_with( - 'userD', 'repo-owner', 'repo-name', '7', 'integrationUser', - 'userA', None - ) - - class TestChooseReviewer(TestNewPR): @pytest.fixture(autouse=True) def make_fakes(cls): @@ -1197,7 +1061,6 @@ class TestRun(TestNewPR): def make_mocks(cls, patcherize): cls.mocks = patcherize(( ('new_pr', 'highfive.newpr.HighfiveHandler.new_pr'), - ('new_comment', 'highfive.newpr.HighfiveHandler.new_comment'), )) def handler_mock(self, payload): @@ -1210,18 +1073,9 @@ def test_newpr(self): m = self.handler_mock(payload) assert m.handler.run('pull_request') == 'OK\n' self.mocks['new_pr'].assert_called_once_with() - self.mocks['new_comment'].assert_not_called() - - def test_new_comment(self): - payload = Payload({'action': 'created'}) - m = self.handler_mock(payload) - assert m.handler.run('issue_comment') == 'OK\n' - self.mocks['new_pr'].assert_not_called() - self.mocks['new_comment'].assert_called_once_with() def test_unsupported_payload(self): payload = Payload({'action': 'something-not-supported'}) m = self.handler_mock(payload) assert m.handler.run('issue_comment') != 'OK\n' self.mocks['new_pr'].assert_not_called() - self.mocks['new_comment'].assert_not_called() From 83a243953435449d2bf18724aaef038726bd8026 Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Tue, 31 Mar 2020 11:08:50 -0400 Subject: [PATCH 2/2] Amend messages to avoid bot communication --- highfive/newpr.py | 4 ++-- highfive/tests/test_integration_tests.py | 4 ++-- highfive/tests/test_newpr.py | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/highfive/newpr.py b/highfive/newpr.py index e8439ed7..e4f1ef0e 100644 --- a/highfive/newpr.py +++ b/highfive/newpr.py @@ -31,8 +31,8 @@ submodule_warning_msg = 'These commits modify **submodules**.' surprise_branch_warning = "Pull requests are usually filed against the %s branch for this repo, but this one is against %s. Please double check that you specified the right target!" -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' +review_with_reviewer = '@%s has been chosen as the reviewer.\n\n(We\'ve picked a reviewer for you, use `r? @user` to override)' +review_without_reviewer = '@%s: no appropriate reviewer found, use `r? @user` to set one.' reviewer_re = re.compile("\\b[rR]\?[:\- ]*@([a-zA-Z0-9\-]+)") submodule_re = re.compile(".*\+Subproject\scommit\s.*", re.DOTALL | re.MULTILINE) diff --git a/highfive/tests/test_integration_tests.py b/highfive/tests/test_integration_tests.py index f6a51b67..4648a683 100644 --- a/highfive/tests/test_integration_tests.py +++ b/highfive/tests/test_integration_tests.py @@ -185,7 +185,7 @@ def test_new_pr_contributor(self): ( ( 'POST', newpr.post_comment_url % ('rust-lang', 'rust', '7'), - {'body': 'r? @nrc\n\n(rust_highfive has picked a reviewer for you, use r? to override)'} + {'body': '@nrc has been chosen as the reviewer.\n\n(We\'ve picked a reviewer for you, use `r? @user` to override)'} ), {'body': {}}, ), @@ -229,7 +229,7 @@ def test_new_pr_contributor_with_labels(self): ( ( 'POST', newpr.post_comment_url % ('rust-lang', 'rust', '7'), - {'body': 'r? @nrc\n\n(rust_highfive has picked a reviewer for you, use r? to override)'} + {'body': '@nrc has been chosen as the reviewer.\n\n(We\'ve picked a reviewer for you, use `r? @user` to override)'} ), {'body': {}}, ), diff --git a/highfive/tests/test_newpr.py b/highfive/tests/test_newpr.py index 8750f595..41c7449b 100644 --- a/highfive/tests/test_newpr.py +++ b/highfive/tests/test_newpr.py @@ -153,11 +153,11 @@ def test_review_msg(self): # No reviewer. handler = HighfiveHandlerMock(Payload({})).handler assert handler.review_msg(None, 'userB') == \ - '@userB: no appropriate reviewer found, use r? to override' + '@userB: no appropriate reviewer found, use `r? @user` to set one.' # Has reviewer. assert handler.review_msg('userA', 'userB') == \ - 'r? @userA\n\n(rust_highfive has picked a reviewer for you, use r? to override)' + '@userA has been chosen as the reviewer.\n\n(We\'ve picked a reviewer for you, use `r? @user` to override)' @mock.patch('os.path.dirname') def test_load_json_file(self, mock_dirname):