Pick reviewer who is not previous assignee when r? group

This commit optimized the logic for `r? group`. Each time an assignee
is set, it is stored in the database, and when we use an r? group, such
as `r? compiler`, it will take out the previously assigned reviewers
from the database to avoid to assign a previous assignee.

Signed-off-by: xizheyin <[email protected]>

Author: xizheyin

src/handlers/assign.rs

@@ -20,9 +20,9 @@
 //! `assign.owners` config, it will auto-select an assignee based on the files
 //! the PR modifies.
 
-use crate::db::review_prefs::{get_review_prefs_batch, RotationMode};
+use crate::db::issue_data::IssueData;
+use crate::db::review_prefs::get_review_prefs_batch;
 use crate::github::UserId;
-use crate::handlers::pr_tracking::ReviewerWorkqueue;
 use crate::{
     config::AssignConfig,
     github::{self, Event, FileDiff, Issue, IssuesAction, Selection},
@@ -36,9 +36,6 @@ use rand::seq::IteratorRandom;
 use rust_team_data::v1::Teams;
 use std::collections::{HashMap, HashSet};
 use std::fmt;
-use std::sync::Arc;
-use tokio::sync::RwLock;
-use tokio_postgres::Client as DbClient;
 use tracing as log;
 
 #[cfg(test)]
@@ -92,9 +89,23 @@ const REVIEWER_ALREADY_ASSIGNED: &str =
 
 Please choose another assignee.";
 
+const REVIEWER_ASSIGNED_BEFORE: &str = "Requested reviewers are 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_REVIEWER_KEY: &str = "previous-reviewer";
+
+/// State stored in the database
+#[derive(Debug, Clone, PartialEq, Default, serde::Deserialize, serde::Serialize)]
+struct Reviewers {
+    /// List of the last warnings in the most recent comment.
+    names: HashSet<String>,
+}
+
 /// Assignment data stored in the issue/PR body.
 #[derive(Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)]
 struct AssignData {
@@ -217,7 +228,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 +260,19 @@ 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_REVIEWER_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 +295,14 @@ 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_string());
+    state.save().await?;
+    Ok(())
 }
 
 /// Determines who to assign the PR to based on either an `r?` command, or
@@ -300,13 +321,11 @@ async fn determine_assignee(
     config: &AssignConfig,
     diff: &[FileDiff],
 ) -> anyhow::Result<(Option<String>, bool)> {
-    let 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,
-            ctx.workqueue.clone(),
+            ctx,
             &teams,
             config,
             &event.issue,
@@ -328,8 +347,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,
-                ctx.workqueue.clone(),
+                ctx,
                 &teams,
                 config,
                 &event.issue,
@@ -347,6 +365,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,8 +387,7 @@ async fn determine_assignee(
 
     if let Some(fallback) = config.adhoc_groups.get("fallback") {
         match find_reviewer_from_names(
-            &db_client,
-            ctx.workqueue.clone(),
+            ctx,
             &teams,
             config,
             &event.issue,
@@ -551,10 +569,8 @@ pub(super) async fn handle_command(
             return Ok(());
         }
 
-        let db_client = ctx.db.get().await;
         let assignee = match find_reviewer_from_names(
-            &db_client,
-            ctx.workqueue.clone(),
+            ctx,
             &teams,
             config,
             issue,
@@ -569,7 +585,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 +696,8 @@ enum FindReviewerError {
     ReviewerIsPrAuthor { username: String },
     /// Requested reviewer is already assigned to that PR
     ReviewerAlreadyAssigned { username: String },
+    /// Requested reviewer is 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 +744,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,8 +773,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,
-    workqueue: Arc<RwLock<ReviewerWorkqueue>>,
+    ctx: &Context,
     teams: &Teams,
     config: &AssignConfig,
     issue: &Issue,
@@ -763,7 +787,7 @@ async fn find_reviewer_from_names(
     }
 
     let candidates =
-        candidate_reviewers_from_names(db, workqueue, teams, config, issue, names).await?;
+        candidate_reviewers_from_names(ctx, teams, config, issue, names).await?;
     assert!(!candidates.is_empty());
 
     // This uses a relatively primitive random choice algorithm.
@@ -916,15 +940,15 @@ 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,
-    workqueue: Arc<RwLock<ReviewerWorkqueue>>,
+    ctx: &Context,
     teams: &'a Teams,
     config: &'a AssignConfig,
     issue: &Issue,
     names: &'a [String],
 ) -> Result<HashSet<String>, FindReviewerError> {
     // Step 1: expand teams and groups into candidate names
     let expanded = expand_teams_and_groups(teams, issue, config, names)?;
+    let expansion_happend = expanded.iter().any(|c| c.origin == ReviewerCandidateOrigin::Expanded);
     let expanded_count = expanded.len();
 
     // Was it a request for a single user, i.e. `r? @username`?
@@ -937,6 +961,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(ctx, issue).await;
 
     // Step 2: pre-filter candidates based on checks that we can perform quickly
     for reviewer_candidate in expanded {
@@ -949,6 +974,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(&reviewer_candidate.name);
+
         // Record the reason why the candidate was filtered out
         let reason = {
             if is_pr_author {
@@ -963,6 +990,12 @@ async fn candidate_reviewers_from_names<'a>(
                 Some(FindReviewerError::ReviewerAlreadyAssigned {
                     username: candidate.clone(),
                 })
+            } else if expansion_happend && 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
             }
@@ -983,12 +1016,14 @@ async fn candidate_reviewers_from_names<'a>(
             .filter_map(|res| res.as_deref().ok().map(|s| s.to_string()))
             .collect();
         let usernames: Vec<&str> = usernames.iter().map(|s| s.as_str()).collect();
-        let review_prefs = get_review_prefs_batch(db, &usernames)
+        
+        let db_client = ctx.db.get().await;
+        let review_prefs = get_review_prefs_batch(&db_client, &usernames)
             .await
             .context("cannot fetch review preferences")
             .map_err(|e| FindReviewerError::DatabaseError(e.to_string()))?;
 
-        let workqueue = workqueue.read().await;
+        let workqueue = ctx.workqueue.read().await;
 
         // Step 4: check review preferences
         candidates = candidates
@@ -1009,9 +1044,6 @@ async fn candidate_reviewers_from_names<'a>(
                         return Err(FindReviewerError::ReviewerAtMaxCapacity { username });
                     }
                 }
-                if review_prefs.rotation_mode == RotationMode::OffRotation {
-                    return Err(FindReviewerError::ReviewerOffRotation { username });
-                }
 
                 return Ok(username);
             })
@@ -1058,3 +1090,14 @@ async fn candidate_reviewers_from_names<'a>(
             .collect())
     }
 }
+
+async fn get_previous_reviewer_names(ctx: &Context, issue: &Issue) -> HashSet<String> {
+    let mut db = ctx.db.get().await;
+    let state: IssueData<'_, Reviewers> =
+        match IssueData::load(&mut db, &issue, PREVIOUS_REVIEWER_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};
 
@@ -79,8 +80,7 @@ impl AssignCtx {
         let names: Vec<_> = names.iter().map(|n| n.to_string()).collect();
 
         let reviewers = candidate_reviewers_from_names(
-            self.test_ctx.db_client(),
-            Arc::new(RwLock::new(self.reviewer_workqueue)),
+            self.test_ctx.handler_ctx(),
             &self.teams,
             &self.config,
             &self.issue,