feat: assume a closed pr with no osc label is internal #276

changelog.d/20231205_183822_nedbat_exedx_276.rst

@@ -0,0 +1,6 @@
+.. A new scriv changelog fragment.
+
+- When a pull request is closed, if it doesn't have a
+  "open-source-contribution" label on it, it is considered an internal pull
+  request.  We assume that it was processed when it was opened, and since it
+  didn't get the label then, the author must have been internal at the time.

docs/details.rst

@@ -76,8 +76,13 @@ draft.
 When a pull request is closed
 -----------------------------
 
-The bot leaves a comment asking the author to complete a survey about the pull
-request.
+On internal pull requests, the bot leaves a comment asking the author to
+complete a survey about the pull request.
+
+When a pull request is being closed, it will be considered an internal pull
+request if it has no "open-source-contribution" label, on the assumption that
+it was processed when it was opened, so the lack of a label means the author
+was internal at the time the pull request was created.
 
 
 When a pull request is re-opened

openedx_webhooks/tasks/pr_tracking.py

@@ -199,10 +199,18 @@ def desired_support_state(pr: PrDict) -> PrDesiredInfo:
     user = pr["user"]["login"]
     repo = pr["base"]["repo"]["full_name"]
     num = pr["number"]
+    label_names = set(lbl["name"] for lbl in pr["labels"])
 
     user_is_bot = is_bot_pull_request(pr)
     no_cla_is_needed = is_private_repo_no_cla_pull_request(pr)
     is_internal = is_internal_pull_request(pr)
+    if not is_internal:
+        if pr["state"] == "closed" and "open-source-contribution" not in label_names:
+            # If we are closing a PR, and it isn't already an OSPR, then it
+            # shouldn't be considered one now.
+            logger.info(f"{repo}#{num} is closing, but seemed to be internal originally")
+            is_internal = True
+
     if user_is_bot:
         logger.info(f"@{user} is a bot, not an ospr.")
     elif no_cla_is_needed:
@@ -224,7 +232,6 @@ def desired_support_state(pr: PrDict) -> PrDesiredInfo:
         state = "closed"
 
     # A label of jira:xyz means we want a Jira issue in the xyz Jira.
-    label_names = set(lbl["name"] for lbl in pr["labels"])
     desired.jira_nicks = {name.partition(":")[-1] for name in label_names if name.startswith("jira:")}
 
     if "crash!123" in label_names:

tests/test_pull_request_closed.py

@@ -11,6 +11,7 @@
     CLA_STATUS_GOOD,
     CLA_STATUS_NO_CONTRIBUTIONS,
 )
+from openedx_webhooks.gh_projects import pull_request_projects
 from openedx_webhooks.tasks.github import pull_request_changed
 from .helpers import check_issue_link_in_markdown, random_text
 
@@ -36,6 +37,7 @@ def closed_pull_request(is_merged, fake_github, fake_jira):
     Returns (pr, issue_key)
     """
     pr = fake_github.make_pull_request(
+        owner="openedx",
         user="tusbar",
         title=random_text(),
         )
@@ -105,36 +107,29 @@ def test_cc_pr_closed(fake_github, fake_jira, is_merged):
     assert len(pr_comments) == 2    # 1 welcome, 1 survey
 
 
-@pytest.mark.parametrize("user", ["tusbar", "feanil"])
-def test_pr_in_nocontrib_repo_closed(fake_github, user):
-    repo = fake_github.make_repo("edx", "some-public-repo")
-    pr = repo.make_pull_request(user=user)
-    pr.close(merge=False)
-    result = pull_request_changed(pr.as_json())
-    assert not result.jira_issues
-    # We don't want the bot to write a comment on a closed pull request.
-    assert len(pr.list_comments()) == 0
-    # User has a cla, but we aren't accepting contributions.
-    assert pr.status(CLA_CONTEXT) == CLA_STATUS_NO_CONTRIBUTIONS
-
-
-def test_pr_closed_after_employee_leaves(fake_github, mocker):
+@pytest.mark.parametrize("org", ["openedx", "edx"])
+def test_pr_closed_after_employee_leaves(org, is_merged, fake_github, mocker):
     # Ned is internal.
-    pr = fake_github.make_pull_request("edx", user="nedbat")
+    pr = fake_github.make_pull_request(org, user="nedbat")
     result = pull_request_changed(pr.as_json())
 
     assert not result.jira_issues
+    # No comments, labels or projects because Ned is internal.
     assert len(pr.list_comments()) == 0
     assert pr.status(CLA_CONTEXT) == CLA_STATUS_GOOD
+    assert pr.labels == set()
+    assert pull_request_projects(pr.as_json()) == set()
 
     # Ned is fired for malfeasance.
     mocker.patch("openedx_webhooks.tasks.pr_tracking.is_internal_pull_request", lambda pr: False)
 
     # His PR is closed.
-    pr.close(merge=False)
+    pr.close(merge=is_merged)
     result = pull_request_changed(pr.as_json())
 
     assert not result.jira_issues
     # We don't want the bot to write a comment on a closed pull request.
     assert len(pr.list_comments()) == 0
-    assert pr.status(CLA_CONTEXT) == CLA_STATUS_NO_CONTRIBUTIONS
+    assert pr.status(CLA_CONTEXT) == CLA_STATUS_GOOD
+    assert pr.labels == set()
+    assert pull_request_projects(pr.as_json()) == set()

tests/test_pull_request_opened.py

@@ -278,38 +278,6 @@ def test_draft_pr_opened(pr_type, fake_github, mocker):
     assert 'click "Ready for Review"' in body
 
 
-def test_handle_closed_pr(is_merged, fake_github):
-    pr = fake_github.make_pull_request(user="tusbar", number=11237, state="closed", merged=is_merged)
-    prj = pr.as_json()
-    result = pull_request_changed(prj)
-    assert not result.jira_issues
-
-    # Check the GitHub comment that was created.
-    pr_comments = pr.list_comments()
-    assert len(pr_comments) == 1
-    body = pr_comments[0].body
-    check_issue_link_in_markdown(body, None)
-    if is_merged:
-        assert "Although this pull request is already merged," in body
-    else:
-        assert "Although this pull request is already closed," in body
-    assert is_comment_kind(BotComment.WELCOME, body)
-    assert is_comment_kind(BotComment.WELCOME_CLOSED, body)
-    assert not is_comment_kind(BotComment.NEED_CLA, body)
-
-    # Check the GitHub labels that got applied.
-    expected_labels = {"open-source-contribution"}
-    assert pr.labels == expected_labels
-    assert pull_request_projects(pr.as_json()) == {settings.GITHUB_OSPR_PROJECT}
-
-    # Rescan the pull request.
-    result2 = pull_request_changed(pr.as_json())
-    assert not result2.jira_issues
-
-    # No new GitHub comment was created.
-    assert len(pr.list_comments()) == 1
-
-
 def test_dont_add_internal_prs_to_project(fake_github):
     pr = fake_github.make_pull_request(owner="openedx", repo="credentials", user="nedbat")
     pull_request_changed(pr.as_json())

tests/test_rescan.py

@@ -42,8 +42,10 @@ def make_rescannable_repo(fake_github, org_name="an-org", repo_name="a-repo"):
         closed_at=datetime(2020,7,1))
     repo.make_pull_request(user="feanil", number=105, created_at=datetime(2019, 4, 1))
     repo.make_pull_request(user="tusbar", number=106, created_at=datetime(2019, 5, 1))
-    repo.make_pull_request(user="tusbar", number=108, state="closed", created_at=datetime(2019, 6, 1),
-        closed_at=datetime(2020,7,1))
+    # A closed PR that had been processed when it opened.
+    pr = repo.make_pull_request(user="tusbar", number=108, created_at=datetime(2019, 6, 1), closed_at=datetime(2020,7,1))
+    pull_request_changed(pr.as_json())
+    pr.close(merge=False)
     # One of the PRs already has a bot comment with a Jira issue.
     pr = repo.make_pull_request(user="tusbar", number=110, state="closed", merged=True, created_at=datetime(2019, 7, 1),
         closed_at=datetime(2020,7,1))
@@ -84,10 +86,14 @@ def test_rescan_repository(rescannable_repo, pull_request_changed_fn, allpr):
 
 
 def test_rescan_repository_dry_run(rescannable_repo, fake_github, fake_jira, pull_request_changed_fn):
+    # The fixtures could have made GitHub or Jira writes, reset them.
+    fake_github.reset_mock()
+    fake_jira.reset_mock()
+
     # Rescan as a dry run.
     ret = rescan_repository(rescannable_repo.full_name, allpr=True, dry_run=True)
 
-    # We shouldn't have made any writes to GitHub or Jira.
+    # We shouldn't have made any writes to GitHub or Jira because dry_run=True.
     fake_github.assert_readonly()
     fake_jira.assert_readonly()
 
@@ -96,35 +102,30 @@ def test_rescan_repository_dry_run(rescannable_repo, fake_github, fake_jira, pul
 
     # Get the names of the actions. We won't worry about the details, those
     # are tested in the non-dry-run tests of rescanning pull requests.
+    import json,sys; json.dump(ret["dry_run_actions"], sys.stdout, indent=4)
     actions = {k: [name for name, kwargs in actions] for k, actions in ret["dry_run_actions"].items()}
     assert actions == {
         102: [
             "initial_state",
-            "set_cla_status",
-            "update_labels_on_pull_request",
-            "add_comment_to_pull_request",
+            "set_cla_status",                   # "The author is authorized to contribute"
+            "update_labels_on_pull_request",    # ["open-source-contribution"]
+            "add_comment_to_pull_request",      # "Thanks for the pull request, @tusbar!"
             "add_pull_request_to_project",
         ],
         106: [
             "initial_state",
-            "set_cla_status",
-            "update_labels_on_pull_request",
-            "add_comment_to_pull_request",
+            "set_cla_status",                   # "The author is authorized to contribute"
+            "update_labels_on_pull_request",    # ["open-source-contribution"]
+            "add_comment_to_pull_request",      # "Thanks for the pull request, @tusbar!"
             "add_pull_request_to_project",
         ],
         108: [
             "initial_state",
-            "set_cla_status",
-            "update_labels_on_pull_request",
-            "add_comment_to_pull_request",
-            "add_pull_request_to_project",
+            "add_comment_to_pull_request",      # "Even though your pull request wasn't merged"
         ],
         110: [
             "initial_state",
-            "set_cla_status",
-            "update_labels_on_pull_request",
-            "add_comment_to_pull_request",
-            "add_pull_request_to_project",
+            "set_cla_status",                   # "The author is authorized to contribute"
         ],
     }