From c728fbb1cb5fafd97fbc3d9dabca911b3e1c5e89 Mon Sep 17 00:00:00 2001 From: Noratrieb <48135649+Noratrieb@users.noreply.github.com> Date: Mon, 31 Mar 2025 19:34:20 +0200 Subject: [PATCH 1/2] Do not filter directly assigned users If someone assigns a person directly, they probably know what they want, don't filter anything. Let someone assign the author, someone on vacation, or an already assigned reviewer. This is important because of the vacation part, we want people to be able to assign people on vacation. --- src/handlers/assign.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/handlers/assign.rs b/src/handlers/assign.rs index 93d94748b..a416c446a 100644 --- a/src/handlers/assign.rs +++ b/src/handlers/assign.rs @@ -978,9 +978,7 @@ fn candidate_reviewers_from_names<'a>( } // Assume it is a user. - if filter(&group_or_user) { - candidates.insert(group_or_user); - } + candidates.insert(group_or_user); } if candidates.is_empty() { let initial = names.iter().cloned().collect(); From eaf9522d846c82747b438484d6919624bb7135dc Mon Sep 17 00:00:00 2001 From: Noratrieb <48135649+Noratrieb@users.noreply.github.com> Date: Mon, 31 Mar 2025 19:35:22 +0200 Subject: [PATCH 2/2] Print warning when assigning someone on vacation People on vacation will usually not (they better >:() respond during their vacation, but they probably will be able to respond after. There is no reason why you wouldn't be able to assign to someone on vacation manually if this person is the best reviewer and you have enough patience for them to return, so don't prevent it. Do print a warning though to set expectations for both the author and triage. --- src/handlers/assign.rs | 50 +++++++++++++++---- src/handlers/assign/tests/tests_candidates.rs | 10 ++-- 2 files changed, 43 insertions(+), 17 deletions(-) diff --git a/src/handlers/assign.rs b/src/handlers/assign.rs index a416c446a..0e95a06f7 100644 --- a/src/handlers/assign.rs +++ b/src/handlers/assign.rs @@ -72,7 +72,7 @@ const RETURNING_USER_WELCOME_MESSAGE_NO_REVIEWER: &str = const ON_VACATION_WARNING: &str = "{username} is on vacation. -Please choose another assignee."; +They may take a while to respond."; const NON_DEFAULT_BRANCH: &str = "Pull requests are usually filed against the {default} branch for this repo, \ @@ -342,7 +342,8 @@ async fn determine_assignee( return Ok((Some(name.to_string()), true)); } // User included `r?` in the opening PR body. - match find_reviewer_from_names(&db_client, &teams, config, &event.issue, &[name]).await { + match find_reviewer_from_names(&db_client, &teams, config, &event.issue, ctx, &[name]).await + { Ok(assignee) => return Ok((Some(assignee), true)), Err(e) => { event @@ -356,8 +357,15 @@ async fn determine_assignee( // Errors fall-through to try fallback group. match find_reviewers_from_diff(config, diff) { Ok(candidates) if !candidates.is_empty() => { - match find_reviewer_from_names(&db_client, &teams, config, &event.issue, &candidates) - .await + match find_reviewer_from_names( + &db_client, + &teams, + config, + &event.issue, + ctx, + &candidates, + ) + .await { Ok(assignee) => return Ok((Some(assignee), false)), Err(FindReviewerError::TeamNotFound(team)) => log::warn!( @@ -396,7 +404,9 @@ async fn determine_assignee( } if let Some(fallback) = config.adhoc_groups.get("fallback") { - match find_reviewer_from_names(&db_client, &teams, config, &event.issue, fallback).await { + match find_reviewer_from_names(&db_client, &teams, config, &event.issue, ctx, fallback) + .await + { Ok(assignee) => return Ok((Some(assignee), false)), Err(e) => { log::trace!( @@ -507,6 +517,10 @@ pub(super) async fn handle_command( // posts contain commands to instruct the user, not things that the bot // should respond to. if event.user().login == ctx.username.as_str() { + tracing::debug!( + "Received command from {}, which is the bot itself, ignoring", + ctx.username.as_str() + ); return Ok(()); } @@ -605,6 +619,7 @@ pub(super) async fn handle_command( &teams, config, issue, + ctx, &[team_name.to_string()], ) .await @@ -809,6 +824,7 @@ async fn find_reviewer_from_names( teams: &Teams, config: &AssignConfig, issue: &Issue, + ctx: &Context, names: &[String], ) -> Result { let candidates = candidate_reviewers_from_names(teams, config, issue, names)?; @@ -843,6 +859,24 @@ async fn find_reviewer_from_names( return Ok("ghost".to_string()); } + let pick = candidates + .into_iter() + .choose(&mut rand::thread_rng()) + .expect("candidate_reviewers_from_names should return at least one entry") + .to_string(); + + if config.is_on_vacation(&pick) { + let result = issue + .post_comment( + &ctx.github, + &ON_VACATION_WARNING.replace("{username}", &pick), + ) + .await; + if let Err(err) = result { + tracing::error!(?err, "Failed to post vacation warning"); + } + } + // filter out team members without capacity // let filtered_candidates = filter_by_capacity(db, &candidates) // .await @@ -864,11 +898,7 @@ async fn find_reviewer_from_names( // ); // Return unfiltered list of candidates - Ok(candidates - .into_iter() - .choose(&mut rand::thread_rng()) - .expect("candidate_reviewers_from_names should return at least one entry") - .to_string()) + Ok(pick) } /// Returns a list of candidate usernames (from relevant teams) to choose as a reviewer. diff --git a/src/handlers/assign/tests/tests_candidates.rs b/src/handlers/assign/tests/tests_candidates.rs index 3febb1033..d6b720cef 100644 --- a/src/handlers/assign/tests/tests_candidates.rs +++ b/src/handlers/assign/tests/tests_candidates.rs @@ -273,18 +273,14 @@ fn vacation() { let config = toml::toml!(users_on_vacation = ["jyn514"]); let issue = generic_issue("octocat", "rust-lang/rust"); - // Test that `r? user` falls through to assigning from the team. - // See `determine_assignee` - ideally we would test that function directly instead of indirectly through `find_reviewer_from_names`. - let err_names = vec!["jyn514".into()]; + // Test that the user on vacation can still be assigned manually. test_from_names( Some(teams.clone()), config.clone(), issue.clone(), &["jyn514"], - Err(FindReviewerError::AllReviewersFiltered { - initial: err_names.clone(), - filtered: err_names, - }), + Ok(&["jyn514"]), + ); // Test that `r? bootstrap` doesn't assign from users on vacation.