Provide a better error message when r? fails

Author: ehuss

src/handlers/assign.rs

@@ -282,8 +282,11 @@ async fn determine_assignee(
                     is there maybe a misconfigured group?",
                     event.issue.global_id()
                 ),
-                Err(FindReviewerError::NoReviewer(names)) => log::trace!(
-                    "no reviewer could be determined for PR {} with candidate name {names:?}",
+                Err(
+                    e @ FindReviewerError::NoReviewer { .. }
+                    | e @ FindReviewerError::AllReviewersFiltered { .. },
+                ) => log::trace!(
+                    "no reviewer could be determined for PR {}: {e}",
                     event.issue.global_id()
                 ),
             }
@@ -536,32 +539,61 @@ pub(super) async fn handle_command(
     Ok(())
 }
 
-#[derive(Debug)]
+#[derive(PartialEq, Debug)]
 enum FindReviewerError {
     /// User specified something like `r? foo/bar` where that team name could
     /// not be found.
     TeamNotFound(String),
-    /// No reviewer could be found. The field is the list of candidate names
-    /// that were used to seed the selection. One example where this happens
-    /// is if the given name was for a team where the PR author is the only
-    /// member.
-    NoReviewer(Vec<String>),
+    /// No reviewer could be found.
+    ///
+    /// This could happen if there is a cyclical group or other misconfiguration.
+    /// `initial` is the initial list of candidate names.
+    NoReviewer { initial: Vec<String> },
+    /// All potential candidates were excluded. `initial` is the list of
+    /// candidate names that were used to seed the selection. `filtered` is
+    /// the users who were prevented from being assigned. One example where
+    /// this happens is if the given name was for a team where the PR author
+    /// is the only member.
+    AllReviewersFiltered {
+        initial: Vec<String>,
+        filtered: Vec<String>,
+    },
 }
 
 impl std::error::Error for FindReviewerError {}
 
 impl fmt::Display for FindReviewerError {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> {
         match self {
-            FindReviewerError::TeamNotFound(team) => write!(f, "Team or group `{team}` not found.\n\
-                \n\
-                rust-lang team names can be found at https://github.com/rust-lang/team/tree/master/teams.\n\
-                Reviewer group names can be found in `triagebot.toml` in this repo."),
-            FindReviewerError::NoReviewer(names) => write!(
-                f,
-                "Could not determine reviewer from `{}`.",
-                names.join(",")
-            ),
+            FindReviewerError::TeamNotFound(team) => {
+                write!(
+                    f,
+                    "Team or group `{team}` not found.\n\
+                    \n\
+                    rust-lang team names can be found at https://github.com/rust-lang/team/tree/master/teams.\n\
+                    Reviewer group names can be found in `triagebot.toml` in this repo."
+                )
+            }
+            FindReviewerError::NoReviewer { initial } => {
+                write!(
+                    f,
+                    "No reviewers could be found from initial choice `{}`\n\
+                     This repo may be misconfigured.\n\
+                     Use r? to specify someone else to assign.",
+                    initial.join(",")
+                )
+            }
+            FindReviewerError::AllReviewersFiltered { initial, filtered } => {
+                write!(
+                    f,
+                    "Could not assign reviewer from: `{}`.\n\
+                     User(s) `{}` are either the PR author or are already assigned, \
+                     and there are no other candidates.\n\
+                     Use r? to specify someone else to assign.",
+                    initial.join(","),
+                    filtered.join(","),
+                )
+            }
         }
     }
 }
@@ -598,12 +630,11 @@ fn find_reviewer_from_names(
     //
     // These are all ideas for improving the selection here. However, I'm not
     // sure they are really worth the effort.
-    match candidates.into_iter().choose(&mut rand::thread_rng()) {
-        Some(candidate) => Ok(candidate.to_string()),
-        None => Err(FindReviewerError::NoReviewer(
-            names.iter().map(|n| n.to_string()).collect(),
-        )),
-    }
+    Ok(candidates
+        .into_iter()
+        .choose(&mut rand::thread_rng())
+        .expect("candidate_reviewers_from_names always returns at least one entry")
+        .to_string())
 }
 
 /// Returns a list of candidate usernames to choose as a reviewer.
@@ -624,16 +655,22 @@ fn candidate_reviewers_from_names<'a>(
     // below will pop from this and then append the expanded results of teams.
     // Usernames will be added to `candidates`.
     let mut group_expansion: Vec<&str> = names.iter().map(|n| n.as_str()).collect();
+    // Keep track of which users get filtered out for a better error message.
+    let mut filtered = Vec::new();
     let repo = issue.repository();
     let org_prefix = format!("{}/", repo.organization);
     // Don't allow groups or teams to include the current author or assignee.
-    let filter = |name: &&str| -> bool {
+    let mut filter = |name: &&str| -> bool {
         let name_lower = name.to_lowercase();
-        name_lower != issue.user.login.to_lowercase()
+        let ok = name_lower != issue.user.login.to_lowercase()
             && !issue
                 .assignees
                 .iter()
-                .any(|assignee| name_lower == assignee.login.to_lowercase())
+                .any(|assignee| name_lower == assignee.login.to_lowercase());
+        if !ok {
+            filtered.push(name.to_string());
+        }
+        ok
     };
 
     // Loop over groups to recursively expand them.
@@ -652,7 +689,7 @@ fn candidate_reviewers_from_names<'a>(
                     group_members
                         .iter()
                         .map(|member| member.as_str())
-                        .filter(filter),
+                        .filter(&mut filter),
                 );
             }
             continue;
@@ -672,7 +709,7 @@ fn candidate_reviewers_from_names<'a>(
                 team.members
                     .iter()
                     .map(|member| member.github.as_str())
-                    .filter(filter),
+                    .filter(&mut filter),
             );
             continue;
         }
@@ -686,5 +723,14 @@ fn candidate_reviewers_from_names<'a>(
             candidates.insert(group_or_user);
         }
     }
-    Ok(candidates)
+    if candidates.is_empty() {
+        let initial = names.iter().cloned().collect();
+        if filtered.is_empty() {
+            Err(FindReviewerError::NoReviewer { initial })
+        } else {
+            Err(FindReviewerError::AllReviewersFiltered { initial, filtered })
+        }
+    } else {
+        Ok(candidates)
+    }
 }

src/handlers/assign/tests/tests_candidates.rs

@@ -8,15 +8,26 @@ fn test_from_names(
     config: toml::Value,
     issue: serde_json::Value,
     names: &[&str],
-    expected: &[&str],
+    expected: Result<&[&str], FindReviewerError>,
 ) {
     let (teams, config, issue) = convert_simplified(teams, config, issue);
     let names: Vec<_> = names.iter().map(|n| n.to_string()).collect();
-    let candidates = candidate_reviewers_from_names(&teams, &config, &issue, &names).unwrap();
-    let mut candidates: Vec<_> = candidates.into_iter().collect();
-    candidates.sort();
-    let expected: Vec<_> = expected.iter().map(|x| *x).collect();
-    assert_eq!(candidates, expected);
+    match (
+        candidate_reviewers_from_names(&teams, &config, &issue, &names),
+        expected,
+    ) {
+        (Ok(candidates), Ok(expected)) => {
+            let mut candidates: Vec<_> = candidates.into_iter().collect();
+            candidates.sort();
+            let expected: Vec<_> = expected.iter().map(|x| *x).collect();
+            assert_eq!(candidates, expected);
+        }
+        (Err(actual), Err(expected)) => {
+            assert_eq!(actual, expected)
+        }
+        (Ok(candidates), Err(_)) => panic!("expected Err, got Ok: {candidates:?}"),
+        (Err(e), Ok(_)) => panic!("expected Ok, got Err: {e}"),
+    }
 }
 
 /// Convert the simplified input in preparation for `candidate_reviewers_from_names`.
@@ -78,7 +89,15 @@ fn circular_groups() {
         other = ["compiler"]
     );
     let issue = generic_issue("octocat", "rust-lang/rust");
-    test_from_names(None, config, issue, &["compiler"], &[]);
+    test_from_names(
+        None,
+        config,
+        issue,
+        &["compiler"],
+        Err(FindReviewerError::NoReviewer {
+            initial: vec!["compiler".to_string()],
+        }),
+    );
 }
 
 #[test]
@@ -91,7 +110,7 @@ fn nested_groups() {
         c = ["a", "b"]
     );
     let issue = generic_issue("octocat", "rust-lang/rust");
-    test_from_names(None, config, issue, &["c"], &["nrc", "pnkfelix"]);
+    test_from_names(None, config, issue, &["c"], Ok(&["nrc", "pnkfelix"]));
 }
 
 #[test]
@@ -102,7 +121,16 @@ fn candidate_filtered_author_only_candidate() {
         compiler = ["nikomatsakis"]
     );
     let issue = generic_issue("nikomatsakis", "rust-lang/rust");
-    test_from_names(None, config, issue, &["compiler"], &[]);
+    test_from_names(
+        None,
+        config,
+        issue,
+        &["compiler"],
+        Err(FindReviewerError::AllReviewersFiltered {
+            initial: vec!["compiler".to_string()],
+            filtered: vec!["nikomatsakis".to_string()],
+        }),
+    );
 }
 
 #[test]
@@ -119,7 +147,7 @@ fn candidate_filtered_author() {
         config,
         issue,
         &["compiler"],
-        &["user1", "user3", "user4"],
+        Ok(&["user1", "user3", "user4"]),
     );
 }
 
@@ -135,7 +163,7 @@ fn candidate_filtered_assignee() {
         {"login": "user1", "id": 1},
         {"login": "user3", "id": 3},
     ]);
-    test_from_names(None, config, issue, &["compiler"], &["user4"]);
+    test_from_names(None, config, issue, &["compiler"], Ok(&["user4"]));
 }
 
 #[test]
@@ -155,7 +183,7 @@ fn groups_teams_users() {
         config,
         issue,
         &["team1", "group1", "user3"],
-        &["t-user1", "t-user2", "user1", "user3"],
+        Ok(&["t-user1", "t-user2", "user1", "user3"]),
     );
 }
 
@@ -173,14 +201,14 @@ fn group_team_user_precedence() {
         config.clone(),
         issue.clone(),
         &["compiler"],
-        &["user2"],
+        Ok(&["user2"]),
     );
     test_from_names(
         Some(teams.clone()),
         config.clone(),
         issue.clone(),
         &["rust-lang/compiler"],
-        &["user2"],
+        Ok(&["user2"]),
     );
 }
 
@@ -200,22 +228,22 @@ fn what_do_slashes_mean() {
         config.clone(),
         issue.clone(),
         &["foo/bar"],
-        &["foo-user"],
+        Ok(&["foo-user"]),
     );
     // Since this is rust-lang-nursery, it uses the rust-lang team, not the group.
     test_from_names(
         Some(teams.clone()),
         config.clone(),
         issue.clone(),
         &["rust-lang/compiler"],
-        &["t-user1"],
+        Ok(&["t-user1"]),
     );
     test_from_names(
         Some(teams.clone()),
         config.clone(),
         issue.clone(),
         &["rust-lang-nursery/compiler"],
-        &["user2"],
+        Ok(&["user2"]),
     );
 }
 
@@ -227,11 +255,13 @@ fn invalid_org_doesnt_match() {
         compiler = ["user2"]
     );
     let issue = generic_issue("octocat", "rust-lang/rust");
-    let (teams, config, issue) = convert_simplified(Some(teams), config, issue);
-    let names = vec!["github/compiler".to_string()];
-    match candidate_reviewers_from_names(&teams, &config, &issue, &names) {
-        Ok(x) => panic!("expected err, got {x:?}"),
-        Err(FindReviewerError::TeamNotFound(_)) => {}
-        Err(e) => panic!("unexpected error {e:?}"),
-    }
+    test_from_names(
+        Some(teams),
+        config,
+        issue,
+        &["github/compiler"],
+        Err(FindReviewerError::TeamNotFound(
+            "github/compiler".to_string(),
+        )),
+    );
 }