Add functionality to assign a random team member with r? @<team-ping>

Author: LeSeulArtichaut

highfive/newpr.py

@@ -36,7 +36,7 @@
 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\-]+)")
+reviewer_re = re.compile("\\b[rR]\?[:\- ]*@(?:([a-zA-Z0-9\-]+)/)?([a-zA-Z0-9\-]+)")
 submodule_re = re.compile(".*\+Subproject\scommit\s.*", re.DOTALL | re.MULTILINE)
 
 rustaceans_api_url = "http://www.ncameron.org/rustaceans/user?username={username}"
@@ -247,27 +247,46 @@ def is_new_contributor(self, username, owner, repo):
             else:
                 raise e
 
-    def find_reviewer(self, msg):
+    def get_groups(self):
+        groups = deepcopy(self.repo_config['groups'])
+
+        # fill in the default groups, ensuring that overwriting is an
+        # error.
+        global_ = self._load_json_file('_global.json')
+        for name, people in global_['groups'].items():
+            assert name not in groups, "group %s overlaps with _global.json" % name
+            groups[name] = people
+
+        return groups
+
+    def find_reviewer(self, msg, exclude):
         """
         If the user specified a reviewer, return the username, otherwise returns
         None.
         """
         if msg is not None:
             match = reviewer_re.search(msg)
-            return match.group(1) if match else None
+            if match:
+                if match.group(1):
+                    # assign someone from the specified team
+                    groups = self.get_groups()
+                    potential = groups.get(match.group(2))
+                    if potential is None:
+                        potential = groups.get("%s/%s" % (match.group(1), match.group(2)))
+                    if potential is None:
+                        potential = groups["all"]
+                    else:
+                        potential.extend(groups["all"])
+
+                    return self.pick_reviewer(groups, potential, exclude)
+                else:
+                    return match.group(2)
 
     def choose_reviewer(self, repo, owner, diff, exclude):
         """Choose a reviewer for the PR."""
         # Get JSON data on reviewers.
         dirs = self.repo_config.get('dirs', {})
-        groups = deepcopy(self.repo_config['groups'])
-
-        # fill in the default groups, ensuring that overwriting is an
-        # error.
-        global_ = self._load_json_file('_global.json')
-        for name, people in global_['groups'].items():
-            assert name not in groups, "group %s overlaps with _global.json" % name
-            groups[name] = people
+        groups = self.get_groups()
 
         most_changed = None
         # If there's directories with specially assigned groups/users
@@ -310,6 +329,9 @@ def choose_reviewer(self, repo, owner, diff, exclude):
         if not potential:
             potential = groups['core']
 
+        return self.pick_reviewer(groups, potential, exclude)
+
+    def pick_reviewer(self, groups, potential, exclude):
         # expand the reviewers list by group
         reviewers = []
         seen = {"all"}
@@ -394,7 +416,7 @@ def new_pr(self):
         if not self.payload['pull_request', 'assignees']:
             # Only try to set an assignee if one isn't already set.
             msg = self.payload['pull_request', 'body']
-            reviewer = self.find_reviewer(msg)
+            reviewer = self.find_reviewer(msg, author)
             post_msg = False
 
             if not reviewer:
@@ -449,7 +471,7 @@ def new_comment(self):
 
         # Check for r? and set the assignee.
         msg = self.payload['comment', 'body']
-        reviewer = self.find_reviewer(msg)
+        reviewer = self.find_reviewer(msg, author)
         if reviewer:
             issue = str(self.payload['issue', 'number'])
             self.set_assignee(

highfive/tests/test_newpr.py

@@ -339,11 +339,11 @@ def test_find_reviewer(self):
         handler = HighfiveHandlerMock(Payload({})).handler
 
         for (msg, reviewer) in found_cases:
-            assert handler.find_reviewer(msg) == reviewer, \
+            assert handler.find_reviewer(msg, None) == reviewer, \
                 "expected '%s' from '%s'" % (reviewer, msg)
 
         for msg in not_found_cases:
-            assert handler.find_reviewer(msg) is None, \
+            assert handler.find_reviewer(msg, None) is None, \
                 "expected '%s' to have no reviewer extracted" % msg
 
     def setup_get_irc_nick_mocks(self, mock_urllib, status_code, data=None):
@@ -833,7 +833,7 @@ def assert_set_assignee_branch_calls(self, reviewer, to_mention):
         self.mocks['api_req'].assert_called_once_with(
             'GET', 'https://the.url/', None, 'application/vnd.github.v3.diff'
         )
-        self.mocks['find_reviewer'].assert_called_once_with('The PR comment.')
+        self.mocks['find_reviewer'].assert_called_once_with('The PR comment.', 'prAuthor')
         self.mocks['set_assignee'].assert_called_once_with(
             reviewer, 'repo-owner', 'repo-name', '7', self.user, 'prAuthor',
             to_mention
@@ -1093,7 +1093,7 @@ def test_no_reviewer(self):
         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['find_reviewer'].assert_called_with('comment!', 'userA')
         self.mocks['set_assignee'].assert_not_called()
 
     def test_has_reviewer(self):
@@ -1102,7 +1102,7 @@ def test_has_reviewer(self):
         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['find_reviewer'].assert_called_with('comment!', 'userA')
         self.mocks['set_assignee'].assert_called_with(
             'userD', 'repo-owner', 'repo-name', '7', 'integrationUser',
             'userA', None