Always allow requesting users directly

Allows `r? <user>` even if they are on a vacation or have full review capacity

Author: Kobzol

src/handlers/assign.rs

@@ -991,14 +991,16 @@ async fn candidate_reviewers_from_names<'a>(
             .any(|assignee| name_lower == assignee.login.to_lowercase());
 
         let is_previously_assigned = previous_reviewer_names.contains(&name_lower);
+        let is_directly_requested = reviewer_candidate.origin == ReviewerCandidateOrigin::Direct;
 
         // Record the reason why the candidate was filtered out
         let reason = {
             if is_pr_author {
                 Some(FindReviewerError::ReviewerIsPrAuthor {
                     username: candidate.clone(),
                 })
-            } else if is_on_vacation {
+            // Allow r? <user> even for people on a vacation
+            } else if is_on_vacation && !is_directly_requested {
                 Some(FindReviewerError::ReviewerOffRotation {
                     username: candidate.clone(),
                 })
@@ -1049,6 +1051,12 @@ async fn candidate_reviewers_from_names<'a>(
                 let candidate = candidate?;
                 let username = &candidate.name;
 
+                // If the reviewer was requested directly, do not consider their review preferences
+                let is_directly_requested = candidate.origin == ReviewerCandidateOrigin::Direct;
+                if is_directly_requested {
+                    return Ok(candidate);
+                }
+
                 // If no review prefs were found, we assume the default unlimited
                 // review capacity and being on rotation.
                 let Some(review_prefs) = review_prefs.get(username.as_str()) else {

src/handlers/assign/tests/tests_candidates.rs

@@ -162,18 +162,17 @@ async fn no_review_prefs() {
 
 #[tokio::test]
 async fn at_max_capacity() {
+    let teams = toml::toml!(compiler = ["martin", "diana"]);
     run_db_test(|ctx| async move {
         let user = user("martin", 1);
         review_prefs_test(ctx)
+            .teams(&teams)
             .set_review_prefs(&user, Some(3), RotationMode::OnRotation)
             .await
             .assign_prs(user.id, 3)
-            .check(
-                &["martin"],
-                Err(FindReviewerError::ReviewerAtMaxCapacity {
-                    username: "martin".to_string(),
-                }),
-            )
+            .check(&["martin"], Ok(&["martin"]))
+            .await?
+            .check(&["compiler"], Ok(&["diana"]))
             .await
     })
     .await;
@@ -195,56 +194,53 @@ async fn below_max_capacity() {
 
 #[tokio::test]
 async fn above_max_capacity() {
+    let teams = toml::toml!(compiler = ["martin", "diana"]);
     run_db_test(|ctx| async move {
         let user = user("martin", 1);
         review_prefs_test(ctx)
+            .teams(&teams)
             .set_review_prefs(&user, Some(3), RotationMode::OnRotation)
             .await
             .assign_prs(user.id, 10)
-            .check(
-                &["martin"],
-                Err(FindReviewerError::ReviewerAtMaxCapacity {
-                    username: "martin".to_string(),
-                }),
-            )
+            .check(&["martin"], Ok(&["martin"]))
+            .await?
+            .check(&["compiler"], Ok(&["diana"]))
             .await
     })
     .await;
 }
 
 #[tokio::test]
 async fn max_capacity_zero() {
+    let teams = toml::toml!(compiler = ["martin", "diana"]);
     run_db_test(|ctx| async move {
         let user = user("martin", 1);
         review_prefs_test(ctx)
+            .teams(&teams)
             .set_review_prefs(&user, Some(0), RotationMode::OnRotation)
             .await
             .assign_prs(user.id, 0)
-            .check(
-                &["martin"],
-                Err(FindReviewerError::ReviewerAtMaxCapacity {
-                    username: "martin".to_string(),
-                }),
-            )
+            .check(&["martin"], Ok(&["martin"]))
+            .await?
+            .check(&["compiler"], Ok(&["diana"]))
             .await
     })
     .await;
 }
 
 #[tokio::test]
 async fn ignore_username_case() {
+    let teams = toml::toml!(compiler = ["martin", "diana"]);
     run_db_test(|ctx| async move {
         let user = user("MARtin", 1);
         review_prefs_test(ctx)
+            .teams(&teams)
             .set_review_prefs(&user, Some(3), RotationMode::OnRotation)
             .await
             .assign_prs(user.id, 3)
-            .check(
-                &["MARTIN"],
-                Err(FindReviewerError::ReviewerAtMaxCapacity {
-                    username: "MARTIN".to_string(),
-                }),
-            )
+            .check(&["MARTIN"], Ok(&["MARTIN"]))
+            .await?
+            .check(&["compiler"], Ok(&["diana"]))
             .await
     })
     .await;
@@ -264,24 +260,6 @@ async fn unlimited_capacity() {
     .await;
 }
 
-#[tokio::test]
-async fn ignore_user_off_rotation_direct() {
-    run_db_test(|ctx| async move {
-        let user = user("martin", 1);
-        review_prefs_test(ctx)
-            .set_review_prefs(&user, None, RotationMode::OffRotation)
-            .await
-            .check(
-                &["martin"],
-                Err(FindReviewerError::ReviewerOffRotation {
-                    username: "martin".to_string(),
-                }),
-            )
-            .await
-    })
-    .await;
-}
-
 #[tokio::test]
 async fn ignore_user_off_rotation_through_team() {
     run_db_test(|ctx| async move {
@@ -297,25 +275,6 @@ async fn ignore_user_off_rotation_through_team() {
     .await;
 }
 
-#[tokio::test]
-async fn review_prefs_prefer_capacity_before_rotation() {
-    run_db_test(|ctx| async move {
-        let user = user("martin", 1);
-        review_prefs_test(ctx)
-            .set_review_prefs(&user, Some(1), RotationMode::OffRotation)
-            .await
-            .assign_prs(user.id, 2)
-            .check(
-                &["martin"],
-                Err(FindReviewerError::ReviewerAtMaxCapacity {
-                    username: "martin".to_string(),
-                }),
-            )
-            .await
-    })
-    .await;
-}
-
 #[tokio::test]
 async fn multiple_reviewers() {
     run_db_test(|ctx| async move {
@@ -522,21 +481,17 @@ async fn invalid_org_doesnt_match() {
 }
 
 #[tokio::test]
-async fn vacation() {
+async fn users_on_vacation() {
     let teams = toml::toml!(bootstrap = ["jyn514", "Mark-Simulacrum"]);
     let config = toml::toml!(users_on_vacation = ["jyn514"]);
 
     run_db_test(|ctx| async move {
-        // Test that `r? user` returns a specific error about the user being on vacation.
         basic_test(ctx, config, issue().call())
             .teams(&teams)
-            .check(
-                &["jyn514"],
-                Err(FindReviewerError::ReviewerOffRotation {
-                    username: "jyn514".to_string(),
-                }),
-            )
+            // Allow direct assignment
+            .check(&["jyn514"], Ok(&["jyn514"]))
             .await?
+            // But ignore the user when requesting a team
             .check(&["bootstrap"], Ok(&["Mark-Simulacrum"]))
             .await
     })