Merge pull request #336 from Kobzol/approve-non-open-prs

Disallow approving non open PRs

Author: Kobzol

src/bors/comment.rs

@@ -163,6 +163,10 @@ handled during merge and rebase. This is normal, and you should still perform st
     Comment::new(message)
 }
 
+pub fn approve_non_open_pr_comment() -> Comment {
+    Comment::new("Only open, non-draft PRs can be approved".to_string())
+}
+
 fn list_workflows_status(workflows: &[WorkflowModel]) -> String {
     workflows
         .iter()

src/bors/handlers/pr_events.rs

@@ -269,7 +269,8 @@ async fn notify_of_edited_pr(
         .post_comment(
             pr_number,
             Comment::new(format!(
-                ":warning: The base branch changed to `{base_name}`, and the PR will need to be re-approved.",
+                r#":warning: The base branch changed to `{base_name}`, and the
+PR will need to be re-approved."#,
             )),
         )
         .await
@@ -284,7 +285,8 @@ async fn notify_of_pushed_pr(
         .post_comment(
             pr_number,
             Comment::new(format!(
-                r#":warning: A new commit {} was pushed to the branch, the PR will need to be re-approved."#,
+                r#":warning: A new commit `{}` was pushed to the branch, the
+PR will need to be re-approved."#,
                 head_sha
             )),
         )
@@ -345,7 +347,10 @@ mod tests {
 
             insta::assert_snapshot!(
                 tester.get_comment().await?,
-                @":warning: The base branch changed to `beta`, and the PR will need to be re-approved."
+                @r"
+            :warning: The base branch changed to `beta`, and the
+            PR will need to be re-approved.
+            "
             );
             tester.default_pr().await.expect_unapproved();
             Ok(tester)
@@ -398,7 +403,10 @@ mod tests {
 
             insta::assert_snapshot!(
                 tester.get_comment().await?,
-                @":warning: A new commit pr-1-commit-1 was pushed to the branch, the PR will need to be re-approved."
+                @r"
+            :warning: A new commit `pr-1-commit-1` was pushed to the branch, the
+            PR will need to be re-approved.
+            "
             );
             tester.default_pr().await.expect_unapproved();
             Ok(tester)
@@ -478,7 +486,9 @@ mod tests {
                     pr.pr_status == PullRequestStatus::Open
                 })
                 .await?;
-            tester.close_pr(default_repo_name(), pr.number.0).await?;
+            tester
+                .set_pr_status_closed(default_repo_name(), pr.number.0)
+                .await?;
             tester
                 .wait_for_pr(default_repo_name(), pr.number.0, |pr| {
                     pr.pr_status == PullRequestStatus::Closed
@@ -505,7 +515,7 @@ mod tests {
                 })
                 .await?;
             tester
-                .ready_for_review(default_repo_name(), pr.number.0)
+                .set_pr_status_ready_for_review(default_repo_name(), pr.number.0)
                 .await?;
             tester
                 .wait_for_pr(default_repo_name(), pr.number.0, |pr| {
@@ -527,7 +537,7 @@ mod tests {
                 })
                 .await?;
             tester
-                .convert_to_draft(default_repo_name(), pr.number.0)
+                .set_pr_status_draft(default_repo_name(), pr.number.0)
                 .await?;
             tester
                 .wait_for_pr(default_repo_name(), pr.number.0, |pr| {
@@ -595,7 +605,9 @@ mod tests {
                     pr.pr_status == PullRequestStatus::Open
                 })
                 .await?;
-            tester.merge_pr(default_repo_name(), pr.number.0).await?;
+            tester
+                .set_pr_status_merged(default_repo_name(), pr.number.0)
+                .await?;
             tester
                 .wait_for_pr(default_repo_name(), pr.number.0, |pr| {
                     pr.pr_status == PullRequestStatus::Merged

src/bors/handlers/refresh.rs

@@ -339,7 +339,9 @@ timeout = 3600
                 .await?;
             tester
                 .with_blocked_webhooks(async |tester| {
-                    tester.close_pr(default_repo_name(), pr.number.0).await
+                    tester
+                        .set_pr_status_closed(default_repo_name(), pr.number.0)
+                        .await
                 })
                 .await?;
             tester.refresh_prs().await;
@@ -359,7 +361,7 @@ timeout = 3600
             tester
                 .with_blocked_webhooks(async |tester| {
                     tester
-                        .convert_to_draft(default_repo_name(), pr.number.0)
+                        .set_pr_status_draft(default_repo_name(), pr.number.0)
                         .await
                 })
                 .await?;

src/bors/handlers/review.rs

@@ -1,13 +1,14 @@
 use std::sync::Arc;
 
 use crate::PgDbClient;
-use crate::bors::Comment;
 use crate::bors::RepositoryState;
 use crate::bors::command::Approver;
 use crate::bors::command::RollupMode;
+use crate::bors::comment::approve_non_open_pr_comment;
 use crate::bors::handlers::has_permission;
 use crate::bors::handlers::labels::handle_label_trigger;
 use crate::bors::handlers::{PullRequestData, deny_request};
+use crate::bors::{Comment, PullRequestStatus};
 use crate::database::ApprovalInfo;
 use crate::database::DelegatedPermission;
 use crate::database::TreeState;
@@ -31,6 +32,15 @@ pub(super) async fn command_approve(
         deny_request(&repo_state, pr.number(), author, PermissionType::Review).await?;
         return Ok(());
     };
+
+    if !matches!(pr.github.status, PullRequestStatus::Open) {
+        repo_state
+            .client
+            .post_comment(pr.number(), approve_non_open_pr_comment())
+            .await?;
+        return Ok(());
+    }
+
     let approver = match approver {
         Approver::Myself => author.username.clone(),
         Approver::Specified(approver) => approver.clone(),
@@ -1040,4 +1050,46 @@ mod tests {
         })
         .await;
     }
+
+    #[sqlx::test]
+    async fn approve_draft_pr(pool: sqlx::PgPool) {
+        run_test(pool, |mut tester| async {
+            tester
+                .set_pr_status_draft(default_repo_name(), default_pr_number())
+                .await?;
+            tester.post_comment("@bors r+").await?;
+            insta::assert_snapshot!(tester.get_comment().await?, @"Only open, non-draft PRs can be approved");
+            tester.default_pr().await.expect_unapproved();
+            Ok(tester)
+        })
+        .await;
+    }
+
+    #[sqlx::test]
+    async fn approve_closed_pr(pool: sqlx::PgPool) {
+        run_test(pool, |mut tester| async {
+            tester
+                .set_pr_status_closed(default_repo_name(), default_pr_number())
+                .await?;
+            tester.post_comment("@bors r+").await?;
+            insta::assert_snapshot!(tester.get_comment().await?, @"Only open, non-draft PRs can be approved");
+            tester.default_pr().await.expect_unapproved();
+            Ok(tester)
+        })
+            .await;
+    }
+
+    #[sqlx::test]
+    async fn approve_merged_pr(pool: sqlx::PgPool) {
+        run_test(pool, |mut tester| async {
+            tester
+                .set_pr_status_merged(default_repo_name(), default_pr_number())
+                .await?;
+            tester.post_comment("@bors r+").await?;
+            insta::assert_snapshot!(tester.get_comment().await?, @"Only open, non-draft PRs can be approved");
+            tester.default_pr().await.expect_unapproved();
+            Ok(tester)
+        })
+            .await;
+    }
 }

src/tests/mocks/bors.rs

@@ -427,7 +427,7 @@ impl BorsTester {
         Ok(pr)
     }
 
-    pub async fn close_pr(
+    pub async fn reopen_pr(
         &mut self,
         repo_name: GithubRepoName,
         pr_number: u64,
@@ -438,19 +438,19 @@ impl BorsTester {
             let pr = repo
                 .pull_requests
                 .get_mut(&pr_number)
-                .expect("PR must be opened before closing it");
-            pr.close_pr();
+                .expect("PR must exist before being reopened");
+            pr.reopen_pr();
             pr.clone()
         };
         self.send_webhook(
             "pull_request",
-            GitHubPullRequestEventPayload::new(pr.clone(), "closed", None),
+            GitHubPullRequestEventPayload::new(pr.clone(), "reopened", None),
         )
         .await?;
         Ok(())
     }
 
-    pub async fn reopen_pr(
+    pub async fn set_pr_status_closed(
         &mut self,
         repo_name: GithubRepoName,
         pr_number: u64,
@@ -461,19 +461,19 @@ impl BorsTester {
             let pr = repo
                 .pull_requests
                 .get_mut(&pr_number)
-                .expect("PR must exist before being reopened");
-            pr.reopen_pr();
+                .expect("PR must be opened before closing it");
+            pr.close_pr();
             pr.clone()
         };
         self.send_webhook(
             "pull_request",
-            GitHubPullRequestEventPayload::new(pr.clone(), "reopened", None),
+            GitHubPullRequestEventPayload::new(pr.clone(), "closed", None),
         )
         .await?;
         Ok(())
     }
 
-    pub async fn convert_to_draft(
+    pub async fn set_pr_status_draft(
         &mut self,
         repo_name: GithubRepoName,
         pr_number: u64,
@@ -496,7 +496,7 @@ impl BorsTester {
         Ok(())
     }
 
-    pub async fn ready_for_review(
+    pub async fn set_pr_status_ready_for_review(
         &mut self,
         repo_name: GithubRepoName,
         pr_number: u64,
@@ -519,7 +519,7 @@ impl BorsTester {
         Ok(())
     }
 
-    pub async fn merge_pr(
+    pub async fn set_pr_status_merged(
         &mut self,
         repo_name: GithubRepoName,
         pr_number: u64,