Merge pull request #115 from vohoanglong0107/feat-unapprove-on-edit

feat: unapprove on PR's base changed

Author: Kobzol

src/bors/event.rs

@@ -1,11 +1,13 @@
 use crate::database::{WorkflowStatus, WorkflowType};
-use crate::github::{CommitSha, GithubRepoName, GithubUser, PullRequestNumber};
+use crate::github::{CommitSha, GithubRepoName, GithubUser, PullRequest, PullRequestNumber};
 use octocrab::models::RunId;
 
 #[derive(Debug)]
 pub enum BorsRepositoryEvent {
     /// A comment was posted on a pull request.
     Comment(PullRequestComment),
+    /// 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.
     WorkflowStarted(WorkflowStarted),
     /// A workflow run on Github Actions or a check run from external CI system has been completed.
@@ -22,6 +24,7 @@ impl BorsRepositoryEvent {
             BorsRepositoryEvent::WorkflowStarted(workflow) => &workflow.repository,
             BorsRepositoryEvent::WorkflowCompleted(workflow) => &workflow.repository,
             BorsRepositoryEvent::CheckSuiteCompleted(payload) => &payload.repository,
+            BorsRepositoryEvent::PullRequestEdited(payload) => &payload.repository,
         }
     }
 }
@@ -50,6 +53,13 @@ pub struct PullRequestComment {
     pub text: String,
 }
 
+#[derive(Debug)]
+pub struct PullRequestEdited {
+    pub repository: GithubRepoName,
+    pub pull_request: PullRequest,
+    pub from_base_sha: Option<CommitSha>,
+}
+
 #[derive(Debug)]
 pub struct WorkflowStarted {
     pub repository: GithubRepoName,

src/bors/handlers/mod.rs

@@ -8,7 +8,9 @@ use crate::bors::event::{BorsGlobalEvent, BorsRepositoryEvent, PullRequestCommen
 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};
+use crate::bors::handlers::review::{
+    command_approve, command_unapprove, handle_pull_request_edited,
+};
 use crate::bors::handlers::trybuild::{command_try_build, command_try_cancel, TRY_BRANCH_NAME};
 use crate::bors::handlers::workflow::{
     handle_check_suite_completed, handle_workflow_completed, handle_workflow_started,
@@ -112,6 +114,14 @@ pub async fn handle_bors_repository_event(
                 .instrument(span.clone())
                 .await?;
         }
+        BorsRepositoryEvent::PullRequestEdited(payload) => {
+            let span =
+                tracing::info_span!("Pull request edited", repo = payload.repository.to_string());
+
+            handle_pull_request_edited(repo, db, payload)
+                .instrument(span.clone())
+                .await?;
+        }
     }
     Ok(())
 }

src/bors/handlers/review.rs

@@ -1,12 +1,15 @@
 use std::sync::Arc;
 
 use crate::bors::command::Approver;
+use crate::bors::event::PullRequestEdited;
 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;
+use crate::github::PullRequestNumber;
 use crate::permissions::PermissionType;
 use crate::PgDbClient;
 
@@ -52,6 +55,29 @@ pub(super) async fn command_unapprove(
     notify_of_unapproval(&repo_state, pr).await
 }
 
+pub(super) async fn handle_pull_request_edited(
+    repo_state: Arc<RepositoryState>,
+    db: Arc<PgDbClient>,
+    payload: PullRequestEdited,
+) -> anyhow::Result<()> {
+    // If the base branch has changed, unapprove the PR
+    let Some(_) = payload.from_base_sha else {
+        return Ok(());
+    };
+
+    let pr_model = db
+        .get_or_create_pull_request(repo_state.repository(), payload.pull_request.number)
+        .await?;
+    if pr_model.approved_by.is_none() {
+        return Ok(());
+    }
+
+    let pr_number = payload.pull_request.number;
+    db.unapprove(repo_state.repository(), pr_number).await?;
+    handle_label_trigger(&repo_state, pr_number, LabelTrigger::Unapproved).await?;
+    notify_of_edited_pr(&repo_state, pr_number, payload.pull_request.head.sha).await
+}
+
 fn sufficient_approve_permission(repo: Arc<RepositoryState>, author: &GithubUser) -> bool {
     repo.permissions
         .load()
@@ -135,12 +161,30 @@ async fn notify_of_unapproval(repo: &RepositoryState, pr: &PullRequest) -> anyho
         .await
 }
 
+async fn notify_of_edited_pr(
+    repo: &RepositoryState,
+    pr_number: PullRequestNumber,
+    head_sha: CommitSha,
+) -> anyhow::Result<()> {
+    repo.client
+        .post_comment(
+            pr_number,
+            Comment::new(format!(
+                r#":warning: The base branch changed to `{}`, and 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, User, World,
+            default_pr_number, default_repo_name, BorsBuilder, BorsTester, Permissions,
+            PullRequestChangeEvent, User, World,
         },
     };
 
@@ -226,7 +270,6 @@ approve = ["+approved"]
     }
 
     #[sqlx::test]
-    #[tracing_test::traced_test]
     async fn unapprove(pool: sqlx::PgPool) {
         let world = World::default();
         world.default_repo().lock().set_config(
@@ -264,6 +307,118 @@ approve = ["+approved"]
             .await;
     }
 
+    #[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)
+            .run_test(|mut tester| async {
+                tester.post_comment("@bors r+").await?;
+                assert_eq!(
+                    tester.get_comment().await?,
+                    format!(
+                        "Commit pr-{}-sha has been approved by `{}`",
+                        default_pr_number(),
+                        User::default_user().name
+                    ),
+                );
+                tester
+                    .edit_pull_request(
+                        default_pr_number(),
+                        PullRequestChangeEvent {
+                            from_base_sha: Some("main-sha".to_string()),
+                        },
+                    )
+                    .await?;
+
+                assert_eq!(
+                    tester.get_comment().await?,
+                    format!(
+                        r#":warning: The base branch changed to `pr-{}-sha`, and 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 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)
+            .run_test(|mut tester| async {
+                tester.post_comment("@bors r+").await?;
+                assert_eq!(
+                    tester.get_comment().await?,
+                    format!(
+                        "Commit pr-{}-sha has been approved by `{}`",
+                        default_pr_number(),
+                        User::default_user().name
+                    ),
+                );
+                tester
+                    .edit_pull_request(
+                        default_pr_number(),
+                        PullRequestChangeEvent {
+                            from_base_sha: None,
+                        },
+                    )
+                    .await?;
+
+                check_pr_approved_by(
+                    &tester,
+                    default_pr_number().into(),
+                    &User::default_user().name,
+                )
+                .await;
+                Ok(tester)
+            })
+            .await;
+    }
+
+    #[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"]
+"#,
+        );
+        BorsBuilder::new(pool)
+            .world(world)
+            .run_test(|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;
+    }
+
     async fn check_pr_approved_by(
         tester: &BorsTester,
         pr_number: PullRequestNumber,

src/github/webhook.rs

@@ -8,7 +8,8 @@ use axum::http::{HeaderMap, HeaderValue, StatusCode};
 use axum::{async_trait, RequestExt};
 use hmac::{Hmac, Mac};
 use octocrab::models::events::payload::{
-    IssueCommentEventAction, IssueCommentEventPayload, PullRequestReviewCommentEventAction,
+    IssueCommentEventAction, IssueCommentEventPayload, PullRequestEventAction,
+    PullRequestEventChangesFrom, PullRequestReviewCommentEventAction,
     PullRequestReviewCommentEventPayload,
 };
 use octocrab::models::pulls::{PullRequest, Review};
@@ -18,7 +19,7 @@ use sha2::Sha256;
 
 use crate::bors::event::{
     BorsEvent, BorsGlobalEvent, BorsRepositoryEvent, CheckSuiteCompleted, PullRequestComment,
-    WorkflowCompleted, WorkflowStarted,
+    PullRequestEdited, WorkflowCompleted, WorkflowStarted,
 };
 use crate::database::{WorkflowStatus, WorkflowType};
 use crate::github::server::ServerStateRef;
@@ -90,6 +91,26 @@ pub struct WebhookPullRequestReviewEvent<'a> {
     sender: Author,
 }
 
+/// Similar to PullRequestEvent from octocrab, but changes field also includes base sha.
+/// https://docs.github.com/en/webhooks/webhook-events-and-payloads#pull_request
+#[derive(Debug, serde::Deserialize)]
+struct WebhookPullRequestEvent {
+    action: PullRequestEventAction,
+    pull_request: PullRequest,
+    changes: Option<WebhookPullRequestChanges>,
+    repository: Repository,
+}
+
+#[derive(Debug, serde::Deserialize)]
+struct WebhookPullRequestChanges {
+    base: Option<WebhookPullRequestBaseChanges>,
+}
+
+#[derive(Debug, serde::Deserialize)]
+struct WebhookPullRequestBaseChanges {
+    sha: Option<PullRequestEventChangesFrom>,
+}
+
 /// axum extractor for GitHub webhook events.
 #[derive(Debug)]
 pub struct GitHubWebhook(pub BorsEvent);
@@ -149,6 +170,7 @@ fn parse_webhook_event(request: Parts, body: &[u8]) -> anyhow::Result<Option<Bor
 
     match event_type.as_bytes() {
         b"issue_comment" => parse_issue_comment_event(body),
+        b"pull_request" => parse_pull_request_events(body),
         b"pull_request_review" => parse_pull_request_review_events(body),
         b"pull_request_review_comment" => parse_pull_request_review_comment_events(body),
         b"installation_repositories" | b"installation" => Ok(Some(BorsEvent::Global(
@@ -179,6 +201,30 @@ 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,
+                pull_request: payload.pull_request.into(),
+                from_base_sha: changes
+                    .base
+                    .and_then(|base| base.sha)
+                    .map(|sha| CommitSha(sha.from)),
+            }),
+        )))
+    } else {
+        Ok(None)
+    }
+}
+
 fn parse_pull_request_review_events(body: &[u8]) -> anyhow::Result<Option<BorsEvent>> {
     let payload: WebhookPullRequestReviewEvent = serde_json::from_slice(body)?;
     if payload.action == "submitted" {

src/tests/mocks/bors.rs

@@ -28,6 +28,8 @@ use crate::{
     ServerState, WebhookSecret,
 };
 
+use super::pull_request::{GitHubPullRequestEventPayload, PullRequestChangeEvent};
+
 pub struct BorsBuilder {
     world: World,
     pool: PgPool,
@@ -266,6 +268,18 @@ impl BorsTester {
         self.webhook_check_suite(check_suite.into()).await
     }
 
+    pub async fn edit_pull_request(
+        &mut self,
+        pr_number: u64,
+        changes: PullRequestChangeEvent,
+    ) -> anyhow::Result<()> {
+        self.send_webhook(
+            "pull_request",
+            GitHubPullRequestEventPayload::new(pr_number, "edited".to_string(), Some(changes)),
+        )
+        .await
+    }
+
     async fn webhook_comment(&mut self, comment: Comment) -> anyhow::Result<()> {
         self.send_webhook(
             "issue_comment",

src/tests/mocks/mod.rs

@@ -18,6 +18,7 @@ pub use bors::BorsTester;
 pub use comment::Comment;
 pub use permissions::Permissions;
 pub use pull_request::default_pr_number;
+pub use pull_request::PullRequestChangeEvent;
 pub use repository::default_repo_name;
 pub use repository::Branch;
 pub use repository::Repo;