Repository: rust-lang/highfive

Issue:
Assign a random team member with r+ priviliges on r? @rust-lang/<team>
If someone writes:
```
r? @rust-lang/docs 
```
Then we could for example assign steveklabnik since they have r+ rights.

This mistake happens from time to time; what happens now when someone writes the above is to leave the PR without a reviewer...

cc https://github.com/rust-lang/rust/pull/57357#issuecomment-451675541

Discussion:
camelid: @LeSeulArtichaut this would be great to have! Are you planning on getting back to this?
LeSeulArtichaut: This depends on whether @Mark-Simulacrum wants to proceed with https://github.com/rust-lang/triagebot/pull/433 first, but as that PR encountered some difficulties, maybe this can be revisited?
Mark-Simulacrum: Yeah, this seems like the right approach in the interim while we wait for GitHub to resolve the problems preventing full rollout of r? for assignment in triagebot. From a quick scroll through it looks like this wants to add team groups for the existing teams (probably at least libs and compiler perhaps?) to the real files too? I guess we can do that in follow-up PRs.
LeSeulArtichaut: *This code looks really bad, I wonder who wrote it... Oh, hey me from (exactly) 9 months ago!*

> From a quick scroll through it looks like this wants to add team groups for the existing teams (probably at least libs and compiler perhaps?) to the real files too? I guess we can do that in follow-up PRs.

If I understand the code correctly, it first checks if a group matches the team name, so in `r? @rust-lang/libs` it's going to check if `libs` exists :
```python
groups.get(match.group(2))
```
before checking if `rust-lang/libs` exists :
```python
potential = groups.get("%s/%s" % (match.group(1), match.group(2)))
```
So I think we don't need to change the data files, because these teams already exist in https://github.com/rust-lang/highfive/blob/f7434f5b191f65b08c0faf35301f0edc585f75dd/highfive/configs/rust-lang/rust.json#L2-L12
wesleywiser: I would **love** to see this feature added. Is there anything I can do to help get this merged? I see this needs a rebase and maybe another review? 

Patch:
diff --git a/highfive/newpr.py b/highfive/newpr.py
index 60c56cf..8b00b72 100644
--- a/highfive/newpr.py
+++ b/highfive/newpr.py
@@ -33,7 +33,7 @@ surprise_branch_warning = "Pull requests are usually filed against the %s branch
 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 @@ class HighfiveHandler(object):
             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 @@ class HighfiveHandler(object):
         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 @@ class HighfiveHandler(object):
         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 @@ class HighfiveHandler(object):
 
         # 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(


Test patch:
diff --git a/highfive/tests/fakes.py b/highfive/tests/fakes.py
index ec7cf8f..7fb5604 100644
--- a/highfive/tests/fakes.py
+++ b/highfive/tests/fakes.py
@@ -75,6 +75,9 @@ def get_repo_configs():
                     "reviewers": ["@JohnTitor"],
                 }
             }
+        },
+        'teams': {
+            "groups": {"all": [], "a": ["@pnkfelix"], "b/c": ["@nrc"]}
         }
     }
 
diff --git a/highfive/tests/test_newpr.py b/highfive/tests/test_newpr.py
index 3634ede..c72ca00 100644
--- a/highfive/tests/test_newpr.py
+++ b/highfive/tests/test_newpr.py
@@ -351,11 +351,11 @@ Please see [the contribution instructions](%s) for more information.
         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 @@ class TestNewPrFunction(TestNewPR):
         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 @@ class TestNewComment(TestNewPR):
         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 @@ class TestNewComment(TestNewPR):
         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 @@ class TestChooseReviewer(TestNewPR):
         )
         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):