Do not approve draft pull requests

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

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.

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:

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"',

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,

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)

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])

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)

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"

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()