Merge pull request #1895 from apiraino/track-reasons-filtered-out-candidates

Better tracking when candidates for PR assignment are filtered out

Author: Kobzol

src/handlers/assign.rs

@@ -70,7 +70,9 @@ Use `r?` to explicitly pick a reviewer";
 const RETURNING_USER_WELCOME_MESSAGE_NO_REVIEWER: &str =
     "@{author}: no appropriate reviewer found, use `r?` to override";
 
-const ON_VACATION_WARNING: &str = "{username} is on vacation. Please do not assign them to PRs.";
+const ON_VACATION_WARNING: &str = "{username} is on vacation.
+
+Please choose another assignee.";
 
 const NON_DEFAULT_BRANCH: &str =
     "Pull requests are usually filed against the {default} branch for this repo, \
@@ -105,9 +107,14 @@ Please contact us on [#t-infra](https://rust-lang.zulipchat.com/#narrow/stream/2
 
 cc: @jackh726 @apiraino";
 
-fn on_vacation_msg(user: &str) -> String {
-    ON_VACATION_WARNING.replace("{username}", user)
-}
+const REVIEWER_IS_PR_AUTHOR: &str = "Pull request author cannot be assigned as reviewer.
+
+Please choose another assignee.";
+
+const REVIEWER_ALREADY_ASSIGNED: &str =
+    "Requested reviewer is already assigned to this pull request.
+
+Please choose another assignee.";
 
 #[derive(Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)]
 struct AssignData {
@@ -354,16 +361,24 @@ async fn determine_assignee(
                     is there maybe a misconfigured group?",
                     event.issue.global_id()
                 ),
-                // TODO: post a comment on the PR if the reviewers were filtered due to being on vacation
                 Err(
                     e @ FindReviewerError::NoReviewer { .. }
                     | e @ FindReviewerError::AllReviewersFiltered { .. }
                     | e @ FindReviewerError::NoReviewerHasCapacity
-                    | e @ FindReviewerError::ReviewerHasNoCapacity { .. },
+                    | e @ FindReviewerError::ReviewerHasNoCapacity { .. }
+                    | e @ FindReviewerError::ReviewerIsPrAuthor { .. }
+                    | e @ FindReviewerError::ReviewerAlreadyAssigned { .. },
                 ) => log::trace!(
                     "no reviewer could be determined for PR {}: {e}",
                     event.issue.global_id()
                 ),
+                Err(e @ FindReviewerError::ReviewerOnVacation { .. }) => {
+                    // TODO: post a comment on the PR if the reviewer(s) were filtered due to being on vacation
+                    log::trace!(
+                        "no reviewer could be determined for PR {}: {e}",
+                        event.issue.global_id()
+                    )
+                }
             }
         }
         // If no owners matched the diff, fall-through.
@@ -508,7 +523,10 @@ pub(super) async fn handle_command(
                 {
                     // This is a comment, so there must already be a reviewer assigned. No need to assign anyone else.
                     issue
-                        .post_comment(&ctx.github, &on_vacation_msg(&username))
+                        .post_comment(
+                            &ctx.github,
+                            &ON_VACATION_WARNING.replace("{username}", &username),
+                        )
                         .await?;
                     return Ok(());
                 }
@@ -544,8 +562,9 @@ pub(super) async fn handle_command(
                     let work_queue = has_user_capacity(&db_client, &name).await;
                     if work_queue.is_err() {
                         // NOTE: disabled for now, just log
-                        log::info!(
-                            "DB reported that user {} has no review capacity. Ignoring.",
+                        log::warn!(
+                            "[#{}] PR self-assign failed, DB reported that user {} has no review capacity. Ignoring.",
+                            issue.number,
                             name
                         );
                         // issue
@@ -702,6 +721,13 @@ pub enum FindReviewerError {
     /// The requested reviewer has no capacity to accept a pull request
     /// assignment at this time
     ReviewerHasNoCapacity { username: String },
+    /// Requested reviewer is on vacation
+    /// (i.e. username is in [users_on_vacation] in the triagebot.toml)
+    ReviewerOnVacation { username: String },
+    /// Requested reviewer is PR author
+    ReviewerIsPrAuthor { username: String },
+    /// Requested reviewer is already assigned to that PR
+    ReviewerAlreadyAssigned { username: String },
 }
 
 impl std::error::Error for FindReviewerError {}
@@ -747,6 +773,23 @@ impl fmt::Display for FindReviewerError {
             FindReviewerError::NoReviewerHasCapacity => {
                 write!(f, "{}", NO_REVIEWER_HAS_CAPACITY)
             }
+            FindReviewerError::ReviewerOnVacation { username } => {
+                write!(f, "{}", ON_VACATION_WARNING.replace("{username}", username))
+            }
+            FindReviewerError::ReviewerIsPrAuthor { username } => {
+                write!(
+                    f,
+                    "{}",
+                    REVIEWER_IS_PR_AUTHOR.replace("{username}", username)
+                )
+            }
+            FindReviewerError::ReviewerAlreadyAssigned { username } => {
+                write!(
+                    f,
+                    "{}",
+                    REVIEWER_ALREADY_ASSIGNED.replace("{username}", username)
+                )
+            }
         }
     }
 }
@@ -785,7 +828,11 @@ async 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.
 
-    log::info!("Initial unfiltered list of candidates: {:?}", candidates);
+    log::info!(
+        "[#{}] Initial unfiltered list of candidates: {:?}",
+        issue.number,
+        candidates
+    );
 
     // Special case user "ghost", we always skip filtering
     if candidates.contains("ghost") {
@@ -799,14 +846,18 @@ async fn find_reviewer_from_names(
 
     if filtered_candidates.is_empty() {
         // NOTE: disabled for now, just log
-        log::info!("Filtered list of PR assignee is empty");
+        log::info!("[#{}] Filtered list of PR assignee is empty", issue.number);
         // return Err(FindReviewerError::AllReviewersFiltered {
         //     initial: names.to_vec(),
         //     filtered: names.to_vec(),
         // });
     }
 
-    log::info!("Filtered list of candidates: {:?}", filtered_candidates);
+    log::info!(
+        "[#{}] Filtered list of candidates: {:?}",
+        issue.number,
+        filtered_candidates
+    );
 
     // Return unfiltered list of candidates
     Ok(candidates
@@ -862,21 +913,45 @@ fn candidate_reviewers_from_names<'a>(
     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();
+    // For debugging purposes, keep track about /why/ candidates were filtered out
+    let mut filtered_debug: HashMap<String, Option<FindReviewerError>> = HashMap::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 mut filter = |name: &&str| -> bool {
         let name_lower = name.to_lowercase();
-        let ok = name_lower != issue.user.login.to_lowercase()
-            && !config.is_on_vacation(name)
-            && !issue
-                .assignees
-                .iter()
-                .any(|assignee| name_lower == assignee.login.to_lowercase());
-        if !ok {
+        let is_pr_author = name_lower == issue.user.login.to_lowercase();
+        let is_on_vacation = config.is_on_vacation(name);
+        let is_already_assigned = issue
+            .assignees
+            .iter()
+            .any(|assignee| name_lower == assignee.login.to_lowercase());
+
+        // Record the reason why the candidate was filtered out
+        let reason = {
+            if is_pr_author {
+                Some(FindReviewerError::ReviewerIsPrAuthor {
+                    username: name.to_string(),
+                })
+            } else if is_on_vacation {
+                Some(FindReviewerError::ReviewerOnVacation {
+                    username: name.to_string(),
+                })
+            } else if is_already_assigned {
+                Some(FindReviewerError::ReviewerAlreadyAssigned {
+                    username: name.to_string(),
+                })
+            } else {
+                None
+            }
+        };
+
+        let can_be_assigned = !is_pr_author && !is_on_vacation && !is_already_assigned;
+        if !can_be_assigned {
             filtered.push(name.to_string());
+            filtered_debug.insert(name.to_string(), reason);
         }
-        ok
+        can_be_assigned
     };
 
     // Loop over groups to recursively expand them.
@@ -934,6 +1009,12 @@ fn candidate_reviewers_from_names<'a>(
         if filtered.is_empty() {
             Err(FindReviewerError::NoReviewer { initial })
         } else {
+            log::warn!(
+                "[#{}] Initial list of candidates {:?}, filtered-out with reasons: {:?}",
+                issue.number,
+                initial,
+                filtered_debug
+            );
             Err(FindReviewerError::AllReviewersFiltered { initial, filtered })
         }
     } else {

src/handlers/pr_tracking.rs

@@ -85,8 +85,9 @@ pub(super) async fn handle_input<'a>(
         // if user has no capacity, revert the PR assignment (GitHub has already assigned it)
         // and post a comment suggesting what to do
         if let Err(_) = work_queue {
-            log::info!(
-                "DB reported that user {} has no review capacity. Ignoring.",
+            log::warn!(
+                "[#{}] DB reported that user {} has no review capacity. Ignoring.",
+                event.issue.number,
                 &assignee.login
             );