From 0dad0e5b438579b1dabdfb8456eba2b172b33368 Mon Sep 17 00:00:00 2001 From: Jaydev Date: Wed, 7 Oct 2026 16:23:01 +0530 Subject: [PATCH 1/2] tests: restore the configuration after set_config and test_switchable set_config restored the new value instead of the saved one, and test_switchable_method left BOT_TASKS set to its last value, so every switchable task called by a later test was silently disabled. --- tests/common.py | 2 +- tests/test_switchable.py | 28 ++++++++++++++++------------ 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/tests/common.py b/tests/common.py index 2fe04ab7..3c5b3aab 100644 --- a/tests/common.py +++ b/tests/common.py @@ -41,4 +41,4 @@ def set_config(**kwargs): yield finally: for key in saved: - setattr(config, key, kwargs[key]) + setattr(config, key, saved[key]) diff --git a/tests/test_switchable.py b/tests/test_switchable.py index d93d21fa..1937c97f 100644 --- a/tests/test_switchable.py +++ b/tests/test_switchable.py @@ -3,6 +3,8 @@ from oca_github_bot import config +from .common import set_config + @config.switchable() def ping_method(msg): @@ -18,16 +20,18 @@ def test_switchable_method(): assert config.BOT_TASKS == ["all"] assert ping_method("test") == "test" assert ping_method_2("test") == "test" - config.BOT_TASKS_DISABLED = ["ping_method"] - assert ping_method("test") is None - assert ping_method_2("test") == "test" - config.BOT_TASKS_DISABLED = [] - config.BOT_TASKS = [] - assert ping_method("test") is None - assert ping_method_2("test") is None - config.BOT_TASKS = ["ping_method"] - assert ping_method("test") == "test" - assert ping_method_2("test") is None - config.BOT_TASKS = ["ping_method", "ping_method_switch"] + with set_config(BOT_TASKS_DISABLED=["ping_method"]): + assert ping_method("test") is None + assert ping_method_2("test") == "test" + with set_config(BOT_TASKS=[]): + assert ping_method("test") is None + assert ping_method_2("test") is None + with set_config(BOT_TASKS=["ping_method"]): + assert ping_method("test") == "test" + assert ping_method_2("test") is None + with set_config(BOT_TASKS=["ping_method", "ping_method_switch"]): + assert ping_method("test") == "test" + assert ping_method_2("test") == "test" + # the configuration is restored after the test + assert config.BOT_TASKS == ["all"] assert ping_method("test") == "test" - assert ping_method_2("test") == "test" From 6ff976a5f6f2f85e248414c91b10d1d333f0b977 Mon Sep 17 00:00:00 2001 From: Jaydev Date: Wed, 7 Oct 2026 16:23:01 +0530 Subject: [PATCH 2/2] Do not approve draft pull requests A draft PR is not ready for review, so tag_approved no longer sets the approved label on it and removes the approved and ready to merge labels if it has them. Converting a PR to draft and marking it ready for review now re-evaluate the labels, and the ready to merge search skips drafts. Fixes #305 --- README.rst | 2 + newsfragments/305.bugfix | 2 + src/oca_github_bot/tasks/tag_approved.py | 6 +- .../tasks/tag_ready_to_merge.py | 1 + src/oca_github_bot/webhooks/__init__.py | 1 + src/oca_github_bot/webhooks/on_pr_draft.py | 17 +++ tests/test_on_pr_draft.py | 20 ++++ tests/test_tag_approved.py | 103 ++++++++++++++++++ 8 files changed, 151 insertions(+), 1 deletion(-) create mode 100644 newsfragments/305.bugfix create mode 100644 src/oca_github_bot/webhooks/on_pr_draft.py create mode 100644 tests/test_on_pr_draft.py create mode 100644 tests/test_tag_approved.py diff --git a/README.rst b/README.rst index ad32f72b..43ecf0b8 100644 --- a/README.rst +++ b/README.rst @@ -67,6 +67,8 @@ On Pull Request review ---------------------- When there are two approvals, set the ``approved`` label. +A draft PR is never ``approved``: converting a PR to draft removes the +``approved`` and ``ready to merge`` labels. When the PR is at least 5 days old, set the ``ready to merge`` label. On Pull Request CI status diff --git a/newsfragments/305.bugfix b/newsfragments/305.bugfix new file mode 100644 index 00000000..a2bd8ccb --- /dev/null +++ b/newsfragments/305.bugfix @@ -0,0 +1,2 @@ +Do not set the ``approved`` and ``ready to merge`` labels on draft PRs, and remove +them when an approved PR is converted to draft. diff --git a/src/oca_github_bot/tasks/tag_approved.py b/src/oca_github_bot/tasks/tag_approved.py index ce032173..438e355b 100644 --- a/src/oca_github_bot/tasks/tag_approved.py +++ b/src/oca_github_bot/tasks/tag_approved.py @@ -43,8 +43,12 @@ def tag_approved(org, repo, pr, dry_run=False): review_users_by_state[state].add(login) gh_issue = gh_call(gh_pr.issue) labels = [label.name for label in gh_issue.labels()] + if gh_pr.draft: + # a draft PR is not ready for review, so it can't be approved + _logger.info(f"{gh_pr.url} is a draft, not approved") if ( - len(review_users_by_state["APPROVED"]) >= APPROVALS_REQUIRED + not gh_pr.draft + and len(review_users_by_state["APPROVED"]) >= APPROVALS_REQUIRED and not review_users_by_state["CHANGES_REQUESTED"] ): if LABEL_APPROVED not in labels: diff --git a/src/oca_github_bot/tasks/tag_ready_to_merge.py b/src/oca_github_bot/tasks/tag_ready_to_merge.py index 25e199c0..649f3b04 100644 --- a/src/oca_github_bot/tasks/tag_ready_to_merge.py +++ b/src/oca_github_bot/tasks/tag_ready_to_merge.py @@ -30,6 +30,7 @@ def tag_ready_to_merge(org, repo=None, dry_run=False): query = [ "type:pr", "state:open", + "draft:false", "status:success", "label:approved", '-label:"ready to merge"', diff --git a/src/oca_github_bot/webhooks/__init__.py b/src/oca_github_bot/webhooks/__init__.py index 8a2d6e1f..8e8fa264 100644 --- a/src/oca_github_bot/webhooks/__init__.py +++ b/src/oca_github_bot/webhooks/__init__.py @@ -4,6 +4,7 @@ from . import ( on_command, on_pr_close_delete_branch, + on_pr_draft, on_pr_green_label_needs_review, on_pr_label_modified_addons, on_pr_open_label_new_contributor, diff --git a/src/oca_github_bot/webhooks/on_pr_draft.py b/src/oca_github_bot/webhooks/on_pr_draft.py new file mode 100644 index 00000000..40486a74 --- /dev/null +++ b/src/oca_github_bot/webhooks/on_pr_draft.py @@ -0,0 +1,17 @@ +# Distributed under the MIT License (http://opensource.org/licenses/MIT). + +from ..router import router +from ..tasks.tag_approved import tag_approved + + +@router.register("pull_request", action="converted_to_draft") +@router.register("pull_request", action="ready_for_review") +async def on_pr_draft(event, gh, *args, **kwargs): + """On conversion of a PR to/from draft, re-evaluate the approved label. + + A draft PR must not keep the ``approved`` and ``ready to merge`` labels, + and a PR which becomes ready for review may already have enough approvals. + """ + org, repo = event.data["repository"]["full_name"].split("/") + pr = event.data["pull_request"]["number"] + tag_approved.delay(org, repo, pr) diff --git a/tests/test_on_pr_draft.py b/tests/test_on_pr_draft.py new file mode 100644 index 00000000..a166b129 --- /dev/null +++ b/tests/test_on_pr_draft.py @@ -0,0 +1,20 @@ +# Distributed under the MIT License (http://opensource.org/licenses/MIT). + +import pytest + +from oca_github_bot.webhooks import on_pr_draft + +from .common import EventMock + + +@pytest.mark.asyncio +async def test_on_pr_draft(mocker): + mocker.patch("oca_github_bot.webhooks.on_pr_draft.tag_approved.delay") + event = EventMock( + data={ + "repository": {"full_name": "OCA/some-repo"}, + "pull_request": {"number": 1, "draft": True}, + } + ) + await on_pr_draft.on_pr_draft(event, None) + on_pr_draft.tag_approved.delay.assert_called_once_with("OCA", "some-repo", 1) diff --git a/tests/test_tag_approved.py b/tests/test_tag_approved.py new file mode 100644 index 00000000..9e7f68ec --- /dev/null +++ b/tests/test_tag_approved.py @@ -0,0 +1,103 @@ +# Distributed under the MIT License (http://opensource.org/licenses/MIT). + +from unittest.mock import MagicMock + +import pytest + +from oca_github_bot.tasks import tag_approved as tag_approved_module +from oca_github_bot.tasks.tag_approved import LABEL_APPROVED, tag_approved +from oca_github_bot.tasks.tag_ready_to_merge import LABEL_READY_TO_MERGE + + +def _review(login, state): + review = MagicMock() + review.state = state + review.user.login = login + return review + + +def _label(name): + label = MagicMock() + label.name = name + return label + + +@pytest.fixture +def gh_pr(mocker): + """Mock the GitHub API so that ``tag_approved`` works on a fake PR.""" + github_mock = mocker.patch("oca_github_bot.tasks.tag_approved.github") + gh_repo = github_mock.repository.return_value.__enter__.return_value + pr = MagicMock() + pr.url = "https://github.com/OCA/some-repo/pull/1" + pr.mergeable = True + pr.draft = False + gh_repo.pull_request.return_value = pr + # gh_call just calls the function + mocker.patch( + "oca_github_bot.tasks.tag_approved.gh_call", + side_effect=lambda func, *args, **kwargs: func(*args, **kwargs), + ) + mocker.patch("oca_github_bot.tasks.tag_approved.tag_ready_to_merge.delay") + mocker.patch("oca_github_bot.tasks.tag_approved.APPROVALS_REQUIRED", 2) + return pr + + +def _set_labels(gh_pr, *names): + gh_pr.issue.return_value.labels.return_value = [_label(name) for name in names] + + +def test_tag_approved_adds_label(gh_pr): + gh_pr.reviews.return_value = [ + _review("reviewer1", "APPROVED"), + _review("reviewer2", "APPROVED"), + ] + _set_labels(gh_pr) + tag_approved("OCA", "some-repo", 1) + gh_pr.issue.return_value.add_labels.assert_called_once_with(LABEL_APPROVED) + gh_pr.issue.return_value.remove_label.assert_not_called() + tag_approved_module.tag_ready_to_merge.delay.assert_called_once_with("OCA") + + +def test_tag_approved_removes_label_on_changes_requested(gh_pr): + gh_pr.reviews.return_value = [ + _review("reviewer1", "APPROVED"), + _review("reviewer2", "CHANGES_REQUESTED"), + ] + _set_labels(gh_pr, LABEL_APPROVED, LABEL_READY_TO_MERGE) + tag_approved("OCA", "some-repo", 1) + gh_pr.issue.return_value.add_labels.assert_not_called() + assert gh_pr.issue.return_value.remove_label.call_args_list == [ + ((LABEL_APPROVED,),), + ((LABEL_READY_TO_MERGE,),), + ] + tag_approved_module.tag_ready_to_merge.delay.assert_not_called() + + +def test_tag_approved_draft_pr_not_approved(gh_pr): + """A draft PR must not be approved, even with enough approvals.""" + gh_pr.draft = True + gh_pr.reviews.return_value = [ + _review("reviewer1", "APPROVED"), + _review("reviewer2", "APPROVED"), + ] + _set_labels(gh_pr) + tag_approved("OCA", "some-repo", 1) + gh_pr.issue.return_value.add_labels.assert_not_called() + tag_approved_module.tag_ready_to_merge.delay.assert_not_called() + + +def test_tag_approved_draft_pr_removes_labels(gh_pr): + """Converting an approved PR to draft removes the approved labels.""" + gh_pr.draft = True + gh_pr.reviews.return_value = [ + _review("reviewer1", "APPROVED"), + _review("reviewer2", "APPROVED"), + ] + _set_labels(gh_pr, LABEL_APPROVED, LABEL_READY_TO_MERGE) + tag_approved("OCA", "some-repo", 1) + gh_pr.issue.return_value.add_labels.assert_not_called() + assert gh_pr.issue.return_value.remove_label.call_args_list == [ + ((LABEL_APPROVED,),), + ((LABEL_READY_TO_MERGE,),), + ] + tag_approved_module.tag_ready_to_merge.delay.assert_not_called()