From 15e0f1b4fb517eebdbbd7df5d76d09e9f5c6e241 Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Tue, 5 Dec 2023 10:58:12 -0500 Subject: [PATCH] feat: assume a closed pr with no osc label is internal #276 --- .../20231205_183822_nedbat_exedx_276.rst | 6 ++++ docs/details.rst | 9 +++-- openedx_webhooks/tasks/pr_tracking.py | 9 ++++- tests/test_pull_request_closed.py | 29 +++++++-------- tests/test_pull_request_opened.py | 32 ----------------- tests/test_rescan.py | 35 ++++++++++--------- 6 files changed, 51 insertions(+), 69 deletions(-) create mode 100644 changelog.d/20231205_183822_nedbat_exedx_276.rst diff --git a/changelog.d/20231205_183822_nedbat_exedx_276.rst b/changelog.d/20231205_183822_nedbat_exedx_276.rst new file mode 100644 index 00000000..2a437f8b --- /dev/null +++ b/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. diff --git a/docs/details.rst b/docs/details.rst index dcf8253e..f43abdb7 100644 --- a/docs/details.rst +++ b/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 diff --git a/openedx_webhooks/tasks/pr_tracking.py b/openedx_webhooks/tasks/pr_tracking.py index a3462dad..75875098 100644 --- a/openedx_webhooks/tasks/pr_tracking.py +++ b/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: diff --git a/tests/test_pull_request_closed.py b/tests/test_pull_request_closed.py index 490aaba0..d30c4e1b 100644 --- a/tests/test_pull_request_closed.py +++ b/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() diff --git a/tests/test_pull_request_opened.py b/tests/test_pull_request_opened.py index e1dd1300..542c62f7 100644 --- a/tests/test_pull_request_opened.py +++ b/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()) diff --git a/tests/test_rescan.py b/tests/test_rescan.py index 88d3f4f8..d9df7c34 100644 --- a/tests/test_rescan.py +++ b/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" ], }