From c26e5a1f4bd8cae3a61beb19c42e18977349345c Mon Sep 17 00:00:00 2001 From: jyn Date: Wed, 26 Jul 2023 14:30:28 -0700 Subject: [PATCH] Add a new `users_on_vacation` config which prevents PR assignment People keep assigning me to PRs (usually new contributors who haven't seen my Zulip message). I very much do not want this. Prevent it in triagebot. - If `r? jyn514` is posted after a comment after the PR is opened, post a useful error but otherwise do nothing - If `r? jyn514` is posted in the PR body, fall back to assigning from the diff (as if `r?` wasn't present) - If `r? bootstrap` is posted anywhere, filter me out from the team. I am not actually on the review rotation currently, but this would be useful if I were (and maybe other people will find it useful?). --- src/config.rs | 13 ++++++++ src/handlers/assign.rs | 24 +++++++++++++-- src/handlers/assign/tests/tests_candidates.rs | 30 +++++++++++++++++++ 3 files changed, 65 insertions(+), 2 deletions(-) diff --git a/src/config.rs b/src/config.rs index 931a4ce0..38e81fce 100644 --- a/src/config.rs +++ b/src/config.rs @@ -90,6 +90,17 @@ pub(crate) struct AssignConfig { /// usernames, team names, or ad-hoc groups. #[serde(default)] pub(crate) owners: HashMap>, + #[serde(default)] + pub(crate) users_on_vacation: HashSet, +} + +impl AssignConfig { + pub(crate) fn is_on_vacation(&self, user: &str) -> bool { + let name_lower = user.to_lowercase(); + self.users_on_vacation + .iter() + .any(|vacationer| name_lower == vacationer.to_lowercase()) + } } #[derive(PartialEq, Eq, Debug, serde::Deserialize)] @@ -337,6 +348,7 @@ mod tests { ] [assign] + users_on_vacation = ["jyn514"] [note] @@ -393,6 +405,7 @@ mod tests { contributing_url: None, adhoc_groups: HashMap::new(), owners: HashMap::new(), + users_on_vacation: HashSet::from(["jyn514".into()]), }), note: Some(NoteConfig { _empty: () }), ping: Some(PingConfig { teams: ping_teams }), diff --git a/src/handlers/assign.rs b/src/handlers/assign.rs index fb2231f5..4a7d7f1d 100644 --- a/src/handlers/assign.rs +++ b/src/handlers/assign.rs @@ -61,6 +61,8 @@ const RETURNING_USER_WELCOME_MESSAGE: &str = "r? @{assignee} 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 NON_DEFAULT_BRANCH: &str = "Pull requests are usually filed against the {default} branch for this repo, \ but this one is against {target}. \ @@ -68,6 +70,10 @@ const NON_DEFAULT_BRANCH: &str = const SUBMODULE_WARNING_MSG: &str = "These commits modify **submodules**."; +fn on_vacation_msg(user: &str) -> String { + ON_VACATION_WARNING.replace("{username}", user) +} + #[derive(Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] struct AssignData { user: Option, @@ -295,6 +301,7 @@ 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 { .. }, @@ -438,7 +445,19 @@ pub(super) async fn handle_command( } let username = match cmd { AssignCommand::Own => event.user().login.clone(), - AssignCommand::User { username } => username, + AssignCommand::User { username } => { + // Allow users on vacation to assign themselves to a PR, but not anyone else. + if config.is_on_vacation(&username) + && event.user().login.to_lowercase() != username.to_lowercase() + { + // 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)) + .await?; + return Ok(()); + } + username + } AssignCommand::Release => { log::trace!( "ignoring release on PR {:?}, must always have assignee", @@ -604,7 +623,7 @@ impl fmt::Display for FindReviewerError { write!( f, "Could not assign reviewer from: `{}`.\n\ - User(s) `{}` are either the PR author or are already assigned, \ + User(s) `{}` are either the PR author, already assigned, or on vacation, \ and there are no other candidates.\n\ Use r? to specify someone else to assign.", initial.join(","), @@ -680,6 +699,7 @@ fn candidate_reviewers_from_names<'a>( 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() diff --git a/src/handlers/assign/tests/tests_candidates.rs b/src/handlers/assign/tests/tests_candidates.rs index 5dc172a8..f78aeca4 100644 --- a/src/handlers/assign/tests/tests_candidates.rs +++ b/src/handlers/assign/tests/tests_candidates.rs @@ -265,3 +265,33 @@ fn invalid_org_doesnt_match() { )), ); } + +#[test] +fn vacation() { + let teams = toml::toml!(bootstrap = ["jyn514", "Mark-Simulacrum"]); + 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_from_names( + Some(teams.clone()), + config.clone(), + issue.clone(), + &["jyn514"], + Err(FindReviewerError::AllReviewersFiltered { + initial: err_names.clone(), + filtered: err_names, + }), + ); + + // Test that `r? bootstrap` doesn't assign from users on vacation. + test_from_names( + Some(teams.clone()), + config.clone(), + issue, + &["bootstrap"], + Ok(&["Mark-Simulacrum"]), + ); +}