Merge pull request #249 from LeSeulArtichaut/assign-team

Assign a random team member on r? @rust-lang/<team>

Author: Mark-Simulacrum

highfive/newpr.py

@@ -33,7 +33,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)
 target_re = re.compile("^[+-]{3} [ab]/compiler/rustc_target/src/spec/", re.MULTILINE)
 
@@ -230,27 +230,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
@@ -293,6 +312,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"}
@@ -382,7 +404,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:
@@ -437,7 +459,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/fakes.py

@@ -75,6 +75,9 @@ def get_repo_configs():
                     "reviewers": ["@JohnTitor"],
                 }
             }
+        },
+        'teams': {
+            "groups": {"all": [], "a": ["@pnkfelix"], "b/c": ["@nrc"]}
         }
     }
 

highfive/tests/test_newpr.py

@@ -351,11 +351,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
 
 class TestApiReq(TestNewPR):
@@ -792,7 +792,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
@@ -1052,7 +1052,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):
@@ -1061,7 +1061,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
@@ -1260,6 +1260,31 @@ def test_mentions_without_dirs(self):
         )
         assert set(["@JohnTitor"]) == mentions
 
+    def test_with_team_ping(self):
+        """Test choosing a reviewer when passed a team ping"""
+        handler = HighfiveHandlerMock(
+            Payload({}), repo_config=self.fakes['config']['teams']
+        ).handler
+
+        found_cases = (
+            ("r? @foo/a", "pnkfelix"),
+            ("r? @b/c", "nrc"),
+        )
+
+        not_found_cases = (
+            "r? @/a",
+            "r? @a/b",
+        )
+
+        for (msg, reviewer) in found_cases:
+            assert handler.find_reviewer(msg, None) == reviewer, \
+                "expected '%s' from '%s'" % (reviewer, msg)
+
+        for msg in not_found_cases:
+            assert handler.find_reviewer(msg, None) is None, \
+                "expected '%s' to have no reviewer extracted" % msg
+
+
 class TestRun(TestNewPR):
     @pytest.fixture(autouse=True)
     def make_mocks(cls, patcherize):