Dismiss all stale Github reviews

bot/code_review_bot/report/github.py

@@ -42,7 +42,10 @@ def publish(self, issues, revision, task_failures, notices, reviewers):
             return
 
         if reviewers:
-            raise NotImplementedError
+            logger.warn(
+                f"These reviewer groups should be assigned, but it's not yet possible on Github: {', '.join(reviewers)}"
+            )
+
         # Avoid publishing a patch from a de-activated analyzer
         publishable_issues = [
             issue
@@ -51,19 +54,19 @@ def publish(self, issues, revision, task_failures, notices, reviewers):
             and issue.analyzer.name not in self.analyzers_skipped
         ]
 
+        # Remove any earlier review to get a clean state
+        self.github_client.cleanup_pr(revision)
+
         if publishable_issues:
             # Publish a review summarizing detected, unresolved and closed issues
-            message = f"{len(issues)} issues have been found in this revision"
-            event = ReviewEvent.RequestChanges
+            self.github_client.publish_review(
+                issues=publishable_issues,
+                revision=revision,
+                message=f"{len(publishable_issues)} issues have been found in this revision",
+                event=ReviewEvent.RequestChanges,
+            )
         else:
             # Simply approve the pull request
-            logger.info("No publishable issue, approving the pull request")
-            message = None
-            event = ReviewEvent.Approved
-
-        self.github_client.publish_review(
-            issues=publishable_issues,
-            revision=revision,
-            message=message,
-            event=event,
-        )
+            logger.info(
+                "No publishable issue, no review nor comment published on Github."
+            )

bot/code_review_bot/sources/github.py

@@ -70,12 +70,7 @@ def publish_review(
         """
         Publish a review from a list of publishable issues, requesting changes to the author.
         """
-
-        if not isinstance(revision, GithubRevision):
-            logger.warning(
-                f"Revision must originate from Github in order to publish a review, skipping {revision}."
-            )
-            return
+        assert isinstance(revision, GithubRevision), "Only for github revisions"
 
         repo = self.api.get_repo(revision.repo_name)
         pull_request = repo.get_pull(revision.pull_number)
@@ -95,3 +90,31 @@ def publish_review(
             event=event.value,
             **attrs,
         )
+
+    def cleanup_pr(self, revision: GithubRevision):
+        """
+        Dismiss previous reviews from the bot
+        """
+        assert isinstance(revision, GithubRevision), "Only for github revisions"
+
+        pr = self.get_pull_request(revision)
+
+        for review in pr.get_reviews():
+            if review.user.login != "mozilla-code-review[bot]":
+                continue
+
+            try:
+                review.dismiss("This review is now deprecated.")
+                logger.info(
+                    "Dismissed previous Github review from the bot",
+                    review=review.id,
+                    submitted=review.submitted_at,
+                )
+            except Exception as e:
+                logger.warn(
+                    "Failed to dismiss previous Github review from the bot",
+                    review=review.id,
+                    submitted=review.submitted_at,
+                    error=e,
+                )
+                raise  # trashme

bot/tests/test_reporter_github.py

@@ -68,12 +68,20 @@ def test_github_review(
     )
     assert issue_coverage.is_publishable()
 
+    # Mock to publish a new review
     responses.add(
         responses.POST,
         "https://api.github.com:443/repos/owner/repo-name/pulls/1/reviews",
         json={},
     )
 
+    # Mock to list existing reviews
+    responses.add(
+        responses.GET,
+        "https://api.github.com:443/repos/owner/repo-name/pulls/1/reviews",
+        json=[],
+    )
+
     reporter.publish([issue_clang_tidy, issue_coverage], revision, [], [], [])
     assert [(call.request.method, call.request.url) for call in responses.calls] == [
         ("GET", "https://github.com/owner/repo-name/pull/1.diff"),
@@ -84,6 +92,18 @@ def test_github_review(
         ),
         ("GET", "https://api.github.com:443/repos/owner/repo-name"),
         ("GET", "https://api.github.com:443/repos/owner/repo-name/pulls/1"),
+        (
+            "GET",
+            "https://api.github.com:443/repos/owner/repo-name/pulls/1/reviews",
+        ),
+        (
+            "GET",
+            "https://api.github.com:443/repos/owner/repo-name",
+        ),
+        (
+            "GET",
+            "https://api.github.com:443/repos/owner/repo-name/pulls/1",
+        ),
         (
             "GET",
             "https://api.github.com:443/repos/owner/repo-name/commits/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
@@ -110,7 +130,7 @@ def test_github_review(
     }
 
 
-def test_github_review_approve(
+def test_github_review_cleanup(
     monkeypatch,
     mock_github,
     mock_config,
@@ -120,7 +140,7 @@ def test_github_review_approve(
     mock_task,
     mock_backend_secret,
 ):
-    """In case no issue is found, the pull request is approved"""
+    """In case no issue is found, previous reviews are dismissed"""
     revision = Revision.from_try_task(mock_try_task, mock_github_decision_task, None)
     revision.lines = {}
     revision.files = ["test.txt", "test.cpp", "another_test.cpp"]
@@ -134,8 +154,21 @@ def test_github_review_approve(
     )
 
     responses.add(
-        responses.POST,
+        responses.GET,
         "https://api.github.com:443/repos/owner/repo-name/pulls/1/reviews",
+        json=[
+            {"id": 1, "user": {"login": "a-moz-developer"}},
+            {
+                "id": 2,
+                "user": {"login": "mozilla-code-review[bot]"},
+                "pull_request_url": "https://api.github.com/repos/owner/repo-name/pulls/2",
+            },
+        ],
+    )
+
+    responses.add(
+        responses.PUT,
+        "https://api.github.com:443/repos/owner/repo-name/pulls/2/reviews/2/dismissals",
         json={},
     )
 
@@ -149,15 +182,9 @@ def test_github_review_approve(
         ),
         ("GET", "https://api.github.com:443/repos/owner/repo-name"),
         ("GET", "https://api.github.com:443/repos/owner/repo-name/pulls/1"),
+        ("GET", "https://api.github.com:443/repos/owner/repo-name/pulls/1/reviews"),
         (
-            "GET",
-            "https://api.github.com:443/repos/owner/repo-name/commits/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+            "PUT",
+            "https://api.github.com:443/repos/owner/repo-name/pulls/2/reviews/2/dismissals",
         ),
-        ("POST", "https://api.github.com:443/repos/owner/repo-name/pulls/1/reviews"),
     ]
-    review_creation = responses.calls[-1]
-    assert json.loads(review_creation.request.body) == {
-        "comments": [],
-        "commit_id": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
-        "event": "APPROVE",
-    }