Ignore new comments

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)
@@ -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
-            )

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/',

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': {}},
             ),
@@ -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()

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):
@@ -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()