Merge pull request #120 from vohoanglong0107/feat-unapprove-on-push

feat: unapprove on push

Author: Kobzol

src/bors/event.rs

@@ -6,6 +6,8 @@ use octocrab::models::RunId;
 pub enum BorsRepositoryEvent {
     /// A comment was posted on a pull request.
     Comment(PullRequestComment),
+    /// When a new commit is pushed to the pull request branch.
+    PullRequestCommitPushed(PullRequestPushed),
     /// When the pull request is edited by its author
     PullRequestEdited(PullRequestEdited),
     /// A workflow run on Github Actions or a check run from external CI system has been started.
@@ -21,10 +23,11 @@ impl BorsRepositoryEvent {
     pub fn repository(&self) -> &GithubRepoName {
         match self {
             BorsRepositoryEvent::Comment(comment) => &comment.repository,
+            BorsRepositoryEvent::PullRequestCommitPushed(payload) => &payload.repository,
+            BorsRepositoryEvent::PullRequestEdited(payload) => &payload.repository,
             BorsRepositoryEvent::WorkflowStarted(workflow) => &workflow.repository,
             BorsRepositoryEvent::WorkflowCompleted(workflow) => &workflow.repository,
             BorsRepositoryEvent::CheckSuiteCompleted(payload) => &payload.repository,
-            BorsRepositoryEvent::PullRequestEdited(payload) => &payload.repository,
         }
     }
 }
@@ -54,6 +57,12 @@ pub struct PullRequestComment {
     pub text: String,
 }
 
+#[derive(Debug)]
+pub struct PullRequestPushed {
+    pub repository: GithubRepoName,
+    pub pull_request: PullRequest,
+}
+
 #[derive(Debug)]
 pub struct PullRequestEdited {
     pub repository: GithubRepoName,

src/bors/handlers/mod.rs

@@ -9,7 +9,7 @@ use crate::bors::handlers::help::command_help;
 use crate::bors::handlers::ping::command_ping;
 use crate::bors::handlers::refresh::refresh_repository;
 use crate::bors::handlers::review::{
-    command_approve, command_unapprove, handle_pull_request_edited,
+    command_approve, command_unapprove, handle_pull_request_edited, handle_push_to_pull_request,
 };
 use crate::bors::handlers::trybuild::{command_try_build, command_try_cancel, TRY_BRANCH_NAME};
 use crate::bors::handlers::workflow::{
@@ -122,6 +122,14 @@ pub async fn handle_bors_repository_event(
                 .instrument(span.clone())
                 .await?;
         }
+        BorsRepositoryEvent::PullRequestCommitPushed(payload) => {
+            let span =
+                tracing::info_span!("Pull request pushed", repo = payload.repository.to_string());
+
+            handle_push_to_pull_request(repo, db, payload)
+                .instrument(span.clone())
+                .await?;
+        }
     }
     Ok(())
 }

src/bors/handlers/review.rs

@@ -2,9 +2,11 @@ use std::sync::Arc;
 
 use crate::bors::command::Approver;
 use crate::bors::event::PullRequestEdited;
+use crate::bors::event::PullRequestPushed;
 use crate::bors::handlers::labels::handle_label_trigger;
 use crate::bors::Comment;
 use crate::bors::RepositoryState;
+use crate::github::CommitSha;
 use crate::github::GithubUser;
 use crate::github::LabelTrigger;
 use crate::github::PullRequest;
@@ -67,7 +69,7 @@ pub(super) async fn handle_pull_request_edited(
     let pr_model = db
         .get_or_create_pull_request(repo_state.repository(), payload.pull_request.number)
         .await?;
-    if pr_model.approved_by.is_none() {
+    if !pr_model.is_approved() {
         return Ok(());
     }
 
@@ -77,6 +79,26 @@ pub(super) async fn handle_pull_request_edited(
     notify_of_edited_pr(&repo_state, pr_number, &payload.pull_request.base.name).await
 }
 
+pub(super) async fn handle_push_to_pull_request(
+    repo_state: Arc<RepositoryState>,
+    db: Arc<PgDbClient>,
+    payload: PullRequestPushed,
+) -> anyhow::Result<()> {
+    let pr = &payload.pull_request;
+    let pr_model = db
+        .get_or_create_pull_request(repo_state.repository(), pr.number)
+        .await?;
+
+    if !pr_model.is_approved() {
+        return Ok(());
+    }
+
+    let pr_number = pr_model.number;
+    db.unapprove(repo_state.repository(), pr_number).await?;
+    handle_label_trigger(&repo_state, pr_number, LabelTrigger::Unapproved).await?;
+    notify_of_pushed_pr(&repo_state, pr_number, pr.head.sha.clone()).await
+}
+
 fn sufficient_approve_permission(repo: Arc<RepositoryState>, author: &GithubUser) -> bool {
     repo.permissions
         .load()
@@ -176,27 +198,37 @@ PR will need to be re-approved."#,
         .await
 }
 
+async fn notify_of_pushed_pr(
+    repo: &RepositoryState,
+    pr_number: PullRequestNumber,
+    head_sha: CommitSha,
+) -> anyhow::Result<()> {
+    repo.client
+        .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."#,
+                head_sha
+            )),
+        )
+        .await
+}
+
 #[cfg(test)]
 mod tests {
     use crate::{
         github::PullRequestNumber,
         tests::mocks::{
-            default_pr_number, default_repo_name, BorsBuilder, BorsTester, Permissions,
+            default_pr_number, default_repo_name, run_test, BorsBuilder, BorsTester, Permissions,
             PullRequestChangeEvent, User, World,
         },
     };
 
     #[sqlx::test]
     async fn default_approve(pool: sqlx::PgPool) {
-        let world = World::default();
-        world.default_repo().lock().set_config(
-            r#"
-[labels]
-approve = ["+approved"]
-"#,
-        );
         BorsBuilder::new(pool)
-            .world(world)
+            .world(create_world_with_approve_config())
             .run_test(|mut tester| async {
                 tester.post_comment("@bors r+").await?;
                 assert_eq!(
@@ -221,15 +253,8 @@ approve = ["+approved"]
 
     #[sqlx::test]
     async fn approve_on_behalf(pool: sqlx::PgPool) {
-        let world = World::default();
-        world.default_repo().lock().set_config(
-            r#"
-[labels]
-approve = ["+approved"]
-"#,
-        );
         BorsBuilder::new(pool)
-            .world(world)
+            .world(create_world_with_approve_config())
             .run_test(|mut tester| async {
                 let approve_user = "user1";
                 tester
@@ -269,15 +294,8 @@ approve = ["+approved"]
 
     #[sqlx::test]
     async fn unapprove(pool: sqlx::PgPool) {
-        let world = World::default();
-        world.default_repo().lock().set_config(
-            r#"
-[labels]
-approve = ["+approved"]
-"#,
-        );
         BorsBuilder::new(pool)
-            .world(world)
+            .world(create_world_with_approve_config())
             .run_test(|mut tester| async {
                 tester.post_comment("@bors r+").await?;
                 assert_eq!(
@@ -307,15 +325,8 @@ approve = ["+approved"]
 
     #[sqlx::test]
     async fn unapprove_on_base_edited(pool: sqlx::PgPool) {
-        let world = World::default();
-        world.default_repo().lock().set_config(
-            r#"
-[labels]
-approve = ["+approved"]
-"#,
-        );
         BorsBuilder::new(pool)
-            .world(world)
+            .world(create_world_with_approve_config())
             .run_test(|mut tester| async {
                 tester.post_comment("@bors r+").await?;
                 assert_eq!(
@@ -348,15 +359,8 @@ PR will need to be re-approved."#,
 
     #[sqlx::test]
     async fn edit_pr_do_nothing_when_base_not_edited(pool: sqlx::PgPool) {
-        let world = World::default();
-        world.default_repo().lock().set_config(
-            r#"
-[labels]
-approve = ["+approved"]
-"#,
-        );
         BorsBuilder::new(pool)
-            .world(world)
+            .world(create_world_with_approve_config())
             .run_test(|mut tester| async {
                 tester.post_comment("@bors r+").await?;
                 assert_eq!(
@@ -389,31 +393,74 @@ approve = ["+approved"]
 
     #[sqlx::test]
     async fn edit_pr_do_nothing_when_not_approved(pool: sqlx::PgPool) {
-        let world = World::default();
-        world.default_repo().lock().set_config(
-            r#"
-[labels]
-approve = ["+approved"]
-"#,
-        );
+        run_test(pool, |mut tester| async {
+            tester
+                .edit_pull_request(
+                    default_pr_number(),
+                    PullRequestChangeEvent {
+                        from_base_sha: Some("main-sha".to_string()),
+                    },
+                )
+                .await?;
+
+            // No comment should be posted
+            Ok(tester)
+        })
+        .await;
+    }
+
+    #[sqlx::test]
+    async fn unapprove_on_push(pool: sqlx::PgPool) {
         BorsBuilder::new(pool)
-            .world(world)
+            .world(create_world_with_approve_config())
             .run_test(|mut tester| async {
-                tester
-                    .edit_pull_request(
+                tester.post_comment("@bors r+").await?;
+                assert_eq!(
+                    tester.get_comment().await?,
+                    format!(
+                        "Commit pr-{}-sha has been approved by `{}`",
                         default_pr_number(),
-                        PullRequestChangeEvent {
-                            from_base_sha: Some("main-sha".to_string()),
-                        },
-                    )
-                    .await?;
+                        User::default_user().name
+                    ),
+                );
+                tester.push_to_pull_request(default_pr_number()).await?;
 
-                // No comment should be posted
+                assert_eq!(
+                    tester.get_comment().await?,
+                    format!(
+                        r#":warning: A new commit `pr-{}-sha` was pushed to the branch, the 
+PR will need to be re-approved."#,
+                        default_pr_number()
+                    )
+                );
+                check_pr_unapproved(&tester, default_pr_number().into()).await;
                 Ok(tester)
             })
             .await;
     }
 
+    #[sqlx::test]
+    async fn push_to_pr_do_nothing_when_not_approved(pool: sqlx::PgPool) {
+        run_test(pool, |mut tester| async {
+            tester.push_to_pull_request(default_pr_number()).await?;
+
+            // No comment should be posted
+            Ok(tester)
+        })
+        .await;
+    }
+
+    fn create_world_with_approve_config() -> World {
+        let world = World::default();
+        world.default_repo().lock().set_config(
+            r#"
+[labels]
+approve = ["+approved"]
+"#,
+        );
+        world
+    }
+
     async fn check_pr_approved_by(
         tester: &BorsTester,
         pr_number: PullRequestNumber,

src/database/mod.rs

@@ -121,6 +121,12 @@ pub struct PullRequestModel {
     pub created_at: DateTime<Utc>,
 }
 
+impl PullRequestModel {
+    pub fn is_approved(&self) -> bool {
+        self.approved_by.is_some()
+    }
+}
+
 /// Describes whether a workflow is a Github Actions workflow or if it's a job from some external
 /// CI.
 #[derive(Debug, PartialEq, sqlx::Type)]