feat: unapprove on push

vohoanglong0107 · vohoanglong0107 · commit 188c27d92f97 · 2024-07-24T14:18:00.000Z
diff --git a/src/bors/event.rs b/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.
+    PullRequestPushed(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::PullRequestPushed(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,
diff --git a/src/bors/handlers/mod.rs b/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::PullRequestPushed(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(())
 }
diff --git a/src/bors/handlers/review.rs b/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: Branch's head was changed to `{}`, 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: Branch's head was changed to `pr-{}-sha`, 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,
diff --git a/src/database/mod.rs b/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)]
diff --git a/src/github/webhook.rs b/src/github/webhook.rs
@@ -19,7 +19,7 @@ use sha2::Sha256;
 
 use crate::bors::event::{
     BorsEvent, BorsGlobalEvent, BorsRepositoryEvent, CheckSuiteCompleted, PullRequestComment,
-    PullRequestEdited, WorkflowCompleted, WorkflowStarted,
+    PullRequestEdited, PullRequestPushed, WorkflowCompleted, WorkflowStarted,
 };
 use crate::database::{WorkflowStatus, WorkflowType};
 use crate::github::server::ServerStateRef;
@@ -204,24 +204,31 @@ fn parse_issue_comment_event(body: &[u8]) -> anyhow::Result<Option<BorsEvent>> {
 fn parse_pull_request_events(body: &[u8]) -> anyhow::Result<Option<BorsEvent>> {
     let payload: WebhookPullRequestEvent = serde_json::from_slice(body)?;
     let repository_name = parse_repository_name(&payload.repository)?;
-    if payload.action == PullRequestEventAction::Edited {
-        let Some(changes) = payload.changes else {
-            return Err(anyhow::anyhow!(
-                "Edited pull request event should have `changes` field"
-            ));
-        };
-        Ok(Some(BorsEvent::Repository(
-            BorsRepositoryEvent::PullRequestEdited(PullRequestEdited {
-                repository: repository_name,
+    match payload.action {
+        PullRequestEventAction::Edited => {
+            let Some(changes) = payload.changes else {
+                return Err(anyhow::anyhow!(
+                    "Edited pull request event should have `changes` field"
+                ));
+            };
+            Ok(Some(BorsEvent::Repository(
+                BorsRepositoryEvent::PullRequestEdited(PullRequestEdited {
+                    repository: repository_name,
+                    pull_request: payload.pull_request.into(),
+                    from_base_sha: changes
+                        .base
+                        .and_then(|base| base.sha)
+                        .map(|sha| CommitSha(sha.from)),
+                }),
+            )))
+        }
+        PullRequestEventAction::Synchronize => Ok(Some(BorsEvent::Repository(
+            BorsRepositoryEvent::PullRequestPushed(PullRequestPushed {
                 pull_request: payload.pull_request.into(),
-                from_base_sha: changes
-                    .base
-                    .and_then(|base| base.sha)
-                    .map(|sha| CommitSha(sha.from)),
+                repository: repository_name,
             }),
-        )))
-    } else {
-        Ok(None)
+        ))),
+        _ => Ok(None),
     }
 }
 
diff --git a/src/tests/mocks/bors.rs b/src/tests/mocks/bors.rs
@@ -280,6 +280,14 @@ impl BorsTester {
         .await
     }
 
+    pub async fn push_to_pull_request(&mut self, pr_number: u64) -> anyhow::Result<()> {
+        self.send_webhook(
+            "pull_request",
+            GitHubPullRequestEventPayload::new(pr_number, "synchronize".to_string(), None),
+        )
+        .await
+    }
+
     async fn webhook_comment(&mut self, comment: Comment) -> anyhow::Result<()> {
         self.send_webhook(
             "issue_comment",
