Merge pull request #1958 from xizheyin/reroll

Pick reviewer who is not previous assignee when r? group

Author: Kobzol

src/handlers/assign.rs

@@ -20,6 +20,7 @@
 //! `assign.owners` config, it will auto-select an assignee based on the files
 //! the PR modifies.
 
+use crate::db::issue_data::IssueData;
 use crate::db::review_prefs::{get_review_prefs_batch, RotationMode};
 use crate::github::UserId;
 use crate::handlers::pr_tracking::ReviewerWorkqueue;
@@ -92,9 +93,23 @@ const REVIEWER_ALREADY_ASSIGNED: &str =
 
 Please choose another assignee.";
 
+const REVIEWER_ASSIGNED_BEFORE: &str = "Requested reviewer @{username} was already assigned before.
+
+Please choose another assignee by using `r? @reviewer`.";
+
 // Special account that we use to prevent assignment.
 const GHOST_ACCOUNT: &str = "ghost";
 
+/// Key for the state in the database
+const PREVIOUS_REVIEWERS_KEY: &str = "previous-reviewers";
+
+/// State stored in the database
+#[derive(Debug, Clone, PartialEq, Default, serde::Deserialize, serde::Serialize)]
+struct Reviewers {
+    // names are stored in lowercase
+    names: HashSet<String>,
+}
+
 /// Assignment data stored in the issue/PR body.
 #[derive(Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)]
 struct AssignData {
@@ -217,7 +232,7 @@ pub(super) async fn handle_input(
             None
         };
         if let Some(assignee) = assignee {
-            set_assignee(&event.issue, &ctx.github, &assignee).await;
+            set_assignee(&ctx, &event.issue, &ctx.github, &assignee).await?;
         }
 
         if let Some(welcome) = welcome {
@@ -249,15 +264,24 @@ fn is_self_assign(assignee: &str, pr_author: &str) -> bool {
 }
 
 /// Sets the assignee of a PR, alerting any errors.
-async fn set_assignee(issue: &Issue, github: &GithubClient, username: &str) {
+async fn set_assignee(
+    ctx: &Context,
+    issue: &Issue,
+    github: &GithubClient,
+    username: &str,
+) -> anyhow::Result<()> {
+    let mut db = ctx.db.get().await;
+    let mut state: IssueData<'_, Reviewers> =
+        IssueData::load(&mut db, &issue, PREVIOUS_REVIEWERS_KEY).await?;
+
     // Don't re-assign if already assigned, e.g. on comment edit
     if issue.contain_assignee(&username) {
         log::trace!(
             "ignoring assign PR {} to {}, already assigned",
             issue.global_id(),
             username,
         );
-        return;
+        return Ok(());
     }
     if let Err(err) = issue.set_assignee(github, &username).await {
         log::warn!(
@@ -280,8 +304,15 @@ async fn set_assignee(issue: &Issue, github: &GithubClient, username: &str) {
             .await
         {
             log::warn!("failed to post error comment: {e}");
+            return Err(e);
         }
     }
+
+    // Record the reviewer in the database
+
+    state.data.names.insert(username.to_lowercase());
+    state.save().await?;
+    Ok(())
 }
 
 /// Determines who to assign the PR to based on either an `r?` command, or
@@ -300,12 +331,12 @@ async fn determine_assignee(
     config: &AssignConfig,
     diff: &[FileDiff],
 ) -> anyhow::Result<(Option<String>, bool)> {
-    let db_client = ctx.db.get().await;
+    let mut db_client = ctx.db.get().await;
     let teams = crate::team_data::teams(&ctx.github).await?;
     if let Some(name) = assign_command {
         // User included `r?` in the opening PR body.
         match find_reviewer_from_names(
-            &db_client,
+            &mut db_client,
             ctx.workqueue.clone(),
             &teams,
             config,
@@ -328,7 +359,7 @@ async fn determine_assignee(
     match find_reviewers_from_diff(config, diff) {
         Ok(candidates) if !candidates.is_empty() => {
             match find_reviewer_from_names(
-                &db_client,
+                &mut db_client,
                 ctx.workqueue.clone(),
                 &teams,
                 config,
@@ -347,6 +378,7 @@ async fn determine_assignee(
                     e @ FindReviewerError::NoReviewer { .. }
                     | e @ FindReviewerError::ReviewerIsPrAuthor { .. }
                     | e @ FindReviewerError::ReviewerAlreadyAssigned { .. }
+                    | e @ FindReviewerError::ReviewerPreviouslyAssigned { .. }
                     | e @ FindReviewerError::ReviewerOffRotation { .. }
                     | e @ FindReviewerError::DatabaseError(_)
                     | e @ FindReviewerError::ReviewerAtMaxCapacity { .. },
@@ -368,7 +400,7 @@ async fn determine_assignee(
 
     if let Some(fallback) = config.adhoc_groups.get("fallback") {
         match find_reviewer_from_names(
-            &db_client,
+            &mut db_client,
             ctx.workqueue.clone(),
             &teams,
             config,
@@ -550,10 +582,9 @@ pub(super) async fn handle_command(
             issue.remove_assignees(&ctx.github, Selection::All).await?;
             return Ok(());
         }
-
-        let db_client = ctx.db.get().await;
+        let mut db_client = ctx.db.get().await;
         let assignee = match find_reviewer_from_names(
-            &db_client,
+            &mut db_client,
             ctx.workqueue.clone(),
             &teams,
             config,
@@ -569,7 +600,7 @@ pub(super) async fn handle_command(
             }
         };
 
-        set_assignee(issue, &ctx.github, &assignee).await;
+        set_assignee(ctx, issue, &ctx.github, &assignee).await?;
     } else {
         let e = EditIssueBody::new(&issue, "ASSIGN");
 
@@ -680,6 +711,8 @@ enum FindReviewerError {
     ReviewerIsPrAuthor { username: String },
     /// Requested reviewer is already assigned to that PR
     ReviewerAlreadyAssigned { username: String },
+    /// Requested reviewer was already assigned previously to that PR.
+    ReviewerPreviouslyAssigned { username: String },
     /// Data required for assignment could not be loaded from the DB.
     DatabaseError(String),
     /// The reviewer has too many PRs alreayd assigned.
@@ -726,6 +759,13 @@ impl fmt::Display for FindReviewerError {
                     REVIEWER_ALREADY_ASSIGNED.replace("{username}", username)
                 )
             }
+            FindReviewerError::ReviewerPreviouslyAssigned { username } => {
+                write!(
+                    f,
+                    "{}",
+                    REVIEWER_ASSIGNED_BEFORE.replace("{username}", username)
+                )
+            }
             FindReviewerError::DatabaseError(error) => {
                 write!(f, "Database error: {error}")
             }
@@ -748,7 +788,7 @@ Please select a different reviewer.",
 /// auto-assign groups, or rust-lang team names. It must have at least one
 /// entry.
 async fn find_reviewer_from_names(
-    db: &DbClient,
+    db: &mut DbClient,
     workqueue: Arc<RwLock<ReviewerWorkqueue>>,
     teams: &Teams,
     config: &AssignConfig,
@@ -916,7 +956,7 @@ fn expand_teams_and_groups(
 /// Returns a list of candidate usernames (from relevant teams) to choose as a reviewer.
 /// If no reviewer is available, returns an error.
 async fn candidate_reviewers_from_names<'a>(
-    db: &DbClient,
+    db: &mut DbClient,
     workqueue: Arc<RwLock<ReviewerWorkqueue>>,
     teams: &'a Teams,
     config: &'a AssignConfig,
@@ -937,6 +977,7 @@ async fn candidate_reviewers_from_names<'a>(
     // Set of candidate usernames to choose from.
     // We go through each expanded candidate and store either success or an error for them.
     let mut candidates: Vec<Result<String, FindReviewerError>> = Vec::new();
+    let previous_reviewer_names = get_previous_reviewer_names(db, issue).await;
 
     // Step 2: pre-filter candidates based on checks that we can perform quickly
     for reviewer_candidate in expanded {
@@ -949,6 +990,8 @@ async fn candidate_reviewers_from_names<'a>(
             .iter()
             .any(|assignee| name_lower == assignee.login.to_lowercase());
 
+        let is_previously_assigned = previous_reviewer_names.contains(&name_lower);
+
         // Record the reason why the candidate was filtered out
         let reason = {
             if is_pr_author {
@@ -963,6 +1006,14 @@ async fn candidate_reviewers_from_names<'a>(
                 Some(FindReviewerError::ReviewerAlreadyAssigned {
                     username: candidate.clone(),
                 })
+            } else if reviewer_candidate.origin == ReviewerCandidateOrigin::Expanded
+                && is_previously_assigned
+            {
+                // **Only** when r? group is expanded, we consider the reviewer previously assigned
+                // `r? @reviewer` will not consider the reviewer previously assigned
+                Some(FindReviewerError::ReviewerPreviouslyAssigned {
+                    username: candidate.clone(),
+                })
             } else {
                 None
             }
@@ -1058,3 +1109,13 @@ async fn candidate_reviewers_from_names<'a>(
             .collect())
     }
 }
+
+async fn get_previous_reviewer_names(db: &mut DbClient, issue: &Issue) -> HashSet<String> {
+    let state: IssueData<'_, Reviewers> =
+        match IssueData::load(db, &issue, PREVIOUS_REVIEWERS_KEY).await {
+            Ok(state) => state,
+            Err(_) => return HashSet::new(),
+        };
+
+    state.data.names
+}

src/handlers/assign/tests/tests_candidates.rs

@@ -3,6 +3,7 @@
 use super::super::*;
 use crate::db::review_prefs::{upsert_review_prefs, RotationMode};
 use crate::github::{PullRequestNumber, User};
+use crate::handlers::pr_tracking::ReviewerWorkqueue;
 use crate::tests::github::{issue, user};
 use crate::tests::{run_db_test, TestContext};
 
@@ -71,15 +72,28 @@ impl AssignCtx {
         self
     }
 
+    async fn set_previous_reviewers(mut self, users: HashSet<&User>) -> Self {
+        let mut db = self.test_ctx.db_client_mut();
+        let mut state: IssueData<'_, Reviewers> =
+            IssueData::load(&mut db, &self.issue, PREVIOUS_REVIEWERS_KEY)
+                .await
+                .unwrap();
+
+        // Create a new set with all user names (overwrite existing data)
+        state.data.names = users.iter().map(|user| user.login.to_lowercase()).collect();
+        state.save().await.unwrap();
+        self
+    }
+
     async fn check(
-        self,
+        mut self,
         names: &[&str],
         expected: Result<&[&str], FindReviewerError>,
     ) -> anyhow::Result<TestContext> {
         let names: Vec<_> = names.iter().map(|n| n.to_string()).collect();
 
         let reviewers = candidate_reviewers_from_names(
-            self.test_ctx.db_client(),
+            self.test_ctx.db_client_mut(),
             Arc::new(RwLock::new(self.reviewer_workqueue)),
             &self.teams,
             &self.config,
@@ -527,3 +541,58 @@ async fn vacation() {
     })
     .await;
 }
+
+#[tokio::test]
+async fn previous_reviewers_ignore_in_team_success() {
+    let teams = toml::toml!(compiler = ["martin", "jyn514"]);
+    let config = toml::Table::new();
+    run_db_test(|ctx| async move {
+        let user = user("martin", 1);
+        basic_test(ctx, config, issue().call())
+            .teams(&teams)
+            .set_previous_reviewers(HashSet::from([&user]))
+            .await
+            .check(&["compiler"], Ok(&["jyn514"]))
+            .await
+    })
+    .await;
+}
+
+#[tokio::test]
+async fn previous_reviewers_ignore_in_team_failed() {
+    let teams = toml::toml!(compiler = ["martin", "jyn514"]);
+    let config = toml::Table::new();
+    run_db_test(|ctx| async move {
+        let user1 = user("martin", 1);
+        let user2 = user("jyn514", 2);
+        basic_test(ctx, config, issue().call())
+            .teams(&teams)
+            .set_previous_reviewers(HashSet::from([&user1, &user2]))
+            .await
+            .check(
+                &["compiler"],
+                Err(FindReviewerError::NoReviewer {
+                    initial: vec!["compiler".to_string()],
+                }),
+            )
+            .await
+    })
+    .await
+}
+
+#[tokio::test]
+async fn previous_reviewers_direct_assignee() {
+    let teams = toml::toml!(compiler = ["martin", "jyn514"]);
+    let config = toml::Table::new();
+    run_db_test(|ctx| async move {
+        let user1 = user("martin", 1);
+        let user2 = user("jyn514", 2);
+        basic_test(ctx, config, issue().call())
+            .teams(&teams)
+            .set_previous_reviewers(HashSet::from([&user1, &user2]))
+            .await
+            .check(&["jyn514"], Ok(&["jyn514"]))
+            .await
+    })
+    .await
+}

src/tests/mod.rs

@@ -94,6 +94,10 @@ impl TestContext {
         &self.client
     }
 
+    pub(crate) fn db_client_mut(&mut self) -> &mut PooledClient {
+        &mut self.client
+    }
+
     pub(crate) async fn add_user(&self, name: &str, id: u64) {
         record_username(self.db_client(), id, name)
             .await