Repository: Qiskit/qiskit-bot

Issue:
False positive for "Community PR" tag
The bot tagged one of my Terra PRs (https://github.com/Qiskit/qiskit-terra/pull/8627) with "Community PR", even though I appear as a member of the Qiskit org.  When I `curl`'d the API hook for that PR, my association appears as `"CONTRIBUTOR"`, not `"MEMBER"` (what the bot checks for).

I think we might want to swap to making a second API request to the `pr["user"]["organizations_url"]` API point to ask if the user is part of Qiskit directly, rather than trying to infer it from the association, since contributor seems to take precedence over member.

cc: @javabster (since I couldn't assign you for some reason).

Discussion:

Patch:
diff --git a/qiskit_bot/api.py b/qiskit_bot/api.py
index 9e508c7..5b19ea2 100644
--- a/qiskit_bot/api.py
+++ b/qiskit_bot/api.py
@@ -151,7 +151,8 @@ def on_pull_event(data):
         repo_name = data['repository']['full_name']
         pr_number = data['pull_request']['number']
         if repo_name in REPOS:
-            community.add_community_label(data, REPOS[repo_name])
+            community.add_community_label(data["pull_request"],
+                                          REPOS[repo_name])
             notifications.trigger_notifications(pr_number,
                                                 REPOS[repo_name], CONFIG)
 
diff --git a/qiskit_bot/community.py b/qiskit_bot/community.py
index 42288af..601b7ed 100644
--- a/qiskit_bot/community.py
+++ b/qiskit_bot/community.py
@@ -17,16 +17,15 @@ EXCLUDED_USER_TYPES = ['Bot', 'Organization']
 
 def add_community_label(pr_data, repo):
     """Add community label to PR when author not associated with core team"""
-    # check repo is monitored by community review team
-    if repo.repo_config.get('uses_community_label'):
-        # check if PR was authored by soemone outside core repo team
-        if (pr_data['pull_request']['author_association'] != 'MEMBER'
-            ) and (pr_data['pull_request']['user']['type']
-                   not in EXCLUDED_USER_TYPES):
-            # fetch label data
-            labels = pr_data['pull_request']['labels']
-            label_names = [label['name'] for label in labels]
-            # tag PR with 'community PR's
-            if "Community PR" not in label_names:
-                pr = repo.gh_repo.get_pull(pr_data['pull_request']['number'])
-                pr.add_to_labels("Community PR")
+    if any((
+        not repo.repo_config.get("uses_community_label", False),
+        pr_data["user"]["type"] in EXCLUDED_USER_TYPES,
+        "Community PR" in [label["name"] for label in pr_data["labels"]],
+    )):
+        return
+    # We need to use the bot's API key rather than public data to know if the
+    # user is a private member of the organisation.  PyGitHub doesn't expose
+    # the 'author_association' attribute as part of the typed interface.
+    pr = repo.gh_repo.get_pull(pr_data["number"])
+    if pr.raw_data["author_association"] not in ("MEMBER", "OWNER"):
+        pr.add_to_labels("Community PR")


Test patch:
diff --git a/tests/test_community.py b/tests/test_community.py
index ad1021d..3d51a65 100644
--- a/tests/test_community.py
+++ b/tests/test_community.py
@@ -34,14 +34,13 @@ class TestCommunity(fixtures.TestWithFixtures, unittest.TestCase):
         repo.gh_repo = gh_mock
         repo.repo_config = {'uses_community_label': True}
         gh_mock.get_pull.return_value = pr_mock
-        data = {'pull_request': {
-            'author_association': None,
+        data = {
             'number': 1234,
             'user': {'type': 'User'},
             'labels': [
                 {'name': 'test_label_1'},
                 {'name': 'test_label_2'}
-            ]}}
+            ]}
 
         community.add_community_label(data, repo)
         gh_mock.get_pull.assert_called_once_with(1234)
@@ -56,14 +55,13 @@ class TestCommunity(fixtures.TestWithFixtures, unittest.TestCase):
         repo.gh_repo = gh_mock
         repo.repo_config = {'uses_community_label': False}
         gh_mock.get_pull.return_value = pr_mock
-        data = {'pull_request': {
-            'author_association': None,
+        data = {
             'number': 1234,
             'user': {'type': 'User'},
             'labels': [
                 {'name': 'test_label_1'},
                 {'name': 'test_label_2'}
-            ]}}
+            ]}
 
         community.add_community_label(data, repo)
 
@@ -78,19 +76,19 @@ class TestCommunity(fixtures.TestWithFixtures, unittest.TestCase):
         repo.name = 'qiskit-terra'
         repo.gh_repo = gh_mock
         repo.repo_config = {'uses_community_label': True}
+        pr_mock.raw_data = {"author_association": "MEMBER"}
         gh_mock.get_pull.return_value = pr_mock
-        data = {'pull_request': {
-            'author_association': 'MEMBER',
+        data = {
             'number': 1234,
             'user': {'type': 'User'},
             'labels': [
                 {'name': 'test_label_1'},
                 {'name': 'test_label_2'}
-            ]}}
+            ]}
 
         community.add_community_label(data, repo)
 
-        gh_mock.get_pull.assert_not_called()
+        gh_mock.get_pull.assert_called_once()
         pr_mock.add_to_labels.assert_not_called()
 
     @unittest.mock.patch("multiprocessing.Process")
@@ -102,14 +100,13 @@ class TestCommunity(fixtures.TestWithFixtures, unittest.TestCase):
         repo.gh_repo = gh_mock
         repo.repo_config = {'uses_community_label': True}
         gh_mock.get_pull.return_value = pr_mock
-        data = {'pull_request': {
-            'author_association': None,
+        data = {
             'number': 1234,
             'user': {'type': 'User'},
             'labels': [
                 {'name': 'Community PR'},
                 {'name': 'test_label_2'}
-            ]}}
+            ]}
 
         community.add_community_label(data, repo)
 
@@ -125,16 +122,43 @@ class TestCommunity(fixtures.TestWithFixtures, unittest.TestCase):
         repo.gh_repo = gh_mock
         repo.repo_config = {'uses_community_label': True}
         gh_mock.get_pull.return_value = pr_mock
-        data = {'pull_request': {
-            'author_association': None,
+        data = {
             'number': 1234,
             'user': {'type': 'Bot'},
             'labels': [
                 {'name': 'Community PR'},
                 {'name': 'test_label_2'}
-            ]}}
+            ]}
 
         community.add_community_label(data, repo)
 
         gh_mock.get_pull.assert_not_called()
         pr_mock.add_to_labels.assert_not_called()
+
+    @unittest.mock.patch("multiprocessing.Process")
+    def test_user_is_private_member(self, sub_mock):
+        """The data fed to the bot by the webhook is unprivileged, and may
+        consider the user to be a contributor if their membership of the
+        organisation is private.  The privileged response from `get_pull`
+        should contain the correct information, though."""
+        repo = unittest.mock.MagicMock()
+        pr_mock = unittest.mock.MagicMock()
+        gh_mock = unittest.mock.MagicMock()
+        repo.name = 'qiskit-terra'
+        repo.gh_repo = gh_mock
+        repo.repo_config = {'uses_community_label': True}
+        pr_mock.raw_data = {"author_association": "MEMBER"}
+        gh_mock.get_pull.return_value = pr_mock
+        data = {
+            'author_association': "CONTRIBUTOR",
+            'number': 1234,
+            'user': {'type': 'User'},
+            'labels': [
+                {'name': 'test_label_1'},
+                {'name': 'test_label_2'}
+            ]}
+
+        community.add_community_label(data, repo)
+
+        gh_mock.get_pull.assert_called_once()
+        pr_mock.add_to_labels.assert_not_called()