ref(bot): use reviewDecision instead of custom calculation (#762)

We can use reviewDecision to hopefully provide a more consistent view of reviews for a pull request.

My theory for some of the issues in #761 is that the GitHub API is being inconsistent when called via the v3 API vs the GraphQL API. Using only reviewDecision will help us avoid depending on the v3 API.

I've deployed this change

bot/kodiak/evaluation.py

@@ -2,10 +2,9 @@
 
 import asyncio
 import textwrap
-from collections import defaultdict
 from collections.abc import Iterable
 from dataclasses import dataclass
-from typing import Dict, List, MutableMapping, Optional, Sequence, Set, Union
+from typing import Dict, List, Optional, Sequence, Set, Union
 
 import inflection
 import pydantic
@@ -45,7 +44,6 @@
     Commit,
     MergeableState,
     MergeStateStatus,
-    Permission,
     PRReview,
     PRReviewRequest,
     PRReviewState,
@@ -836,36 +834,6 @@ async def set_status(msg: str, markdown_content: Optional[str] = None) -> None:
         # - failing required status checks
         # - branch not up to date (should be handled before this)
         # - missing required signature
-        if (
-            branch_protection.requiresApprovingReviews
-            and branch_protection.requiredApprovingReviewCount
-        ):
-            reviews_by_author: MutableMapping[str, List[PRReview]] = defaultdict(list)
-            for review in sorted(reviews, key=lambda x: x.createdAt):
-                if review.author.permission not in {Permission.ADMIN, Permission.WRITE}:
-                    continue
-                reviews_by_author[review.author.login].append(review)
-
-            successful_reviews = 0
-            for author_name, review_list in reviews_by_author.items():
-                review_state = review_status(review_list)
-                # blocking review
-                if review_state == PRReviewState.CHANGES_REQUESTED:
-                    await block_merge(
-                        api, pull_request, f"changes requested by {author_name!r}"
-                    )
-                    return
-                # successful review
-                if review_state == PRReviewState.APPROVED:
-                    successful_reviews += 1
-            # missing required review count
-            if successful_reviews < branch_protection.requiredApprovingReviewCount:
-                await block_merge(
-                    api,
-                    pull_request,
-                    f"missing required reviews, have {successful_reviews!r}/{branch_protection.requiredApprovingReviewCount!r}",
-                )
-                return
 
         if pull_request.reviewDecision == PullRequestReviewDecision.REVIEW_REQUIRED:
             await block_merge(api, pull_request, "missing required reviews")

bot/kodiak/tests/evaluation/test_branch_protection.py

@@ -188,136 +188,6 @@ async def test_mergeable_requires_commit_signatures_squash_and_merge() -> None:
         assert api.merge.called is False
 
 
-@pytest.mark.asyncio
-async def test_mergeable_missing_required_approving_reviews() -> None:
-    """
-    Don't merge when branch protection requires approving reviews and we don't
-    have enought approving reviews.
-    """
-    api = create_api()
-    mergeable = create_mergeable()
-    pull_request = create_pull_request()
-    branch_protection = create_branch_protection()
-
-    pull_request.mergeStateStatus = MergeStateStatus.BLOCKED
-    branch_protection.requiresApprovingReviews = True
-    branch_protection.requiredApprovingReviewCount = 1
-
-    await mergeable(
-        api=api,
-        pull_request=pull_request,
-        branch_protection=branch_protection,
-        reviews=[],
-    )
-    assert api.set_status.call_count == 1
-    assert api.dequeue.call_count == 1
-    assert "missing required reviews" in api.set_status.calls[0]["msg"]
-
-    # verify we haven't tried to update/merge the PR
-    assert api.update_branch.called is False
-    assert api.merge.called is False
-    assert api.queue_for_merge.called is False
-
-
-@pytest.mark.asyncio
-async def test_mergeable_missing_required_approving_reviews_has_review_with_missing_perms() -> None:
-    """
-    Don't merge when branch protection requires approving reviews and we don't have enough reviews. If a reviewer doesn't have permissions we should ignore their review.
-    """
-    api = create_api()
-    mergeable = create_mergeable()
-    pull_request = create_pull_request()
-    branch_protection = create_branch_protection()
-    review = create_review()
-
-    pull_request.mergeStateStatus = MergeStateStatus.BLOCKED
-    branch_protection.requiresApprovingReviews = True
-    branch_protection.requiredApprovingReviewCount = 1
-    review.state = PRReviewState.APPROVED
-    review.author.permission = Permission.READ
-
-    await mergeable(
-        api=api,
-        pull_request=pull_request,
-        branch_protection=branch_protection,
-        reviews=[review],
-    )
-    assert api.set_status.call_count == 1
-    assert api.dequeue.call_count == 1
-    assert "missing required reviews" in api.set_status.calls[0]["msg"]
-
-    # verify we haven't tried to update/merge the PR
-    assert api.update_branch.called is False
-    assert api.merge.called is False
-    assert api.queue_for_merge.called is False
-
-
-@pytest.mark.asyncio
-async def test_mergeable_missing_required_approving_reviews_changes_requested() -> None:
-    """
-    Don't merge when branch protection requires approving reviews and a user requested changes.
-    """
-    api = create_api()
-    mergeable = create_mergeable()
-    pull_request = create_pull_request()
-    branch_protection = create_branch_protection()
-    review = create_review()
-
-    pull_request.mergeStateStatus = MergeStateStatus.BLOCKED
-    branch_protection.requiresApprovingReviews = True
-    branch_protection.requiredApprovingReviewCount = 1
-    review.state = PRReviewState.CHANGES_REQUESTED
-    review.author.permission = Permission.WRITE
-
-    await mergeable(
-        api=api,
-        pull_request=pull_request,
-        branch_protection=branch_protection,
-        reviews=[review],
-    )
-    assert api.set_status.call_count == 1
-    assert api.dequeue.call_count == 1
-    assert "changes requested by 'ghost'" in api.set_status.calls[0]["msg"]
-
-    # verify we haven't tried to update/merge the PR
-    assert api.update_branch.called is False
-    assert api.merge.called is False
-    assert api.queue_for_merge.called is False
-
-
-@pytest.mark.asyncio
-async def test_mergeable_missing_required_approving_reviews_missing_approving_review_count() -> None:
-    """
-    Don't merge when branch protection requires approving reviews and we don't have enough.
-    """
-    api = create_api()
-    mergeable = create_mergeable()
-    pull_request = create_pull_request()
-    branch_protection = create_branch_protection()
-    review = create_review()
-
-    pull_request.mergeStateStatus = MergeStateStatus.BLOCKED
-    branch_protection.requiresApprovingReviews = True
-    branch_protection.requiredApprovingReviewCount = 2
-    review.state = PRReviewState.APPROVED
-    review.author.permission = Permission.WRITE
-
-    await mergeable(
-        api=api,
-        pull_request=pull_request,
-        branch_protection=branch_protection,
-        reviews=[review],
-    )
-    assert api.set_status.call_count == 1
-    assert api.dequeue.call_count == 1
-    assert "missing required reviews, have 1/2" in api.set_status.calls[0]["msg"]
-
-    # verify we haven't tried to update/merge the PR
-    assert api.update_branch.called is False
-    assert api.merge.called is False
-    assert api.queue_for_merge.called is False
-
-
 @pytest.mark.asyncio
 async def test_mergeable_missing_required_approving_reviews_code_owners() -> None:
     """
@@ -662,58 +532,6 @@ async def test_mergeable_wait_for_checks() -> None:
     assert api.queue_for_merge.called is False
 
 
-@pytest.mark.asyncio
-async def test_regression_mishandling_multiple_reviews_failing_reviews() -> None:
-    mergeable = create_mergeable()
-    api = create_api()
-    pull_request = create_pull_request()
-    branch_protection = create_branch_protection()
-    review_request = create_review_request()
-    pull_request.mergeStateStatus = MergeStateStatus.BEHIND
-    branch_protection.requiresApprovingReviews = True
-    branch_protection.requiredApprovingReviewCount = 2
-    first_review_date = datetime(2010, 5, 15)
-    latest_review_date = first_review_date + timedelta(minutes=20)
-
-    await mergeable(
-        api=api,
-        pull_request=pull_request,
-        branch_protection=branch_protection,
-        review_requests=[review_request],
-        reviews=[
-            PRReview(
-                state=PRReviewState.CHANGES_REQUESTED,
-                createdAt=first_review_date,
-                author=PRReviewAuthor(login="chdsbd", permission=Permission.WRITE),
-            ),
-            PRReview(
-                state=PRReviewState.COMMENTED,
-                createdAt=latest_review_date,
-                author=PRReviewAuthor(login="chdsbd", permission=Permission.WRITE),
-            ),
-            PRReview(
-                state=PRReviewState.APPROVED,
-                createdAt=latest_review_date,
-                author=PRReviewAuthor(login="ghost", permission=Permission.WRITE),
-            ),
-            PRReview(
-                state=PRReviewState.APPROVED,
-                createdAt=latest_review_date,
-                author=PRReviewAuthor(login="kodiak", permission=Permission.WRITE),
-            ),
-        ],
-    )
-    assert api.set_status.call_count == 1
-    assert "changes requested" in api.set_status.calls[0]["msg"]
-    assert "chdsbd" in api.set_status.calls[0]["msg"]
-    assert api.dequeue.call_count == 1
-
-    # verify we haven't tried to update/merge the PR
-    assert api.update_branch.called is False
-    assert api.merge.called is False
-    assert api.queue_for_merge.called is False
-
-
 @pytest.mark.asyncio
 async def test_regression_mishandling_multiple_reviews_okay_reviews() -> None:
     mergeable = create_mergeable()