Skip to content

Commit

Permalink
Add a new users_on_vacation config which prevents PR assignment
Browse files Browse the repository at this point in the history
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?).
  • Loading branch information
jyn514 committed Jul 26, 2023
1 parent 37d04d7 commit c26e5a1
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 2 deletions.
13 changes: 13 additions & 0 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,17 @@ pub(crate) struct AssignConfig {
/// usernames, team names, or ad-hoc groups.
#[serde(default)]
pub(crate) owners: HashMap<String, Vec<String>>,
#[serde(default)]
pub(crate) users_on_vacation: HashSet<String>,
}

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)]
Expand Down Expand Up @@ -337,6 +348,7 @@ mod tests {
]
[assign]
users_on_vacation = ["jyn514"]
[note]
Expand Down Expand Up @@ -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 }),
Expand Down
24 changes: 22 additions & 2 deletions src/handlers/assign.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,19 @@ 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}. \
Please double check that you specified the right target!";

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<String>,
Expand Down Expand Up @@ -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 { .. },
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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(","),
Expand Down Expand Up @@ -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()
Expand Down
30 changes: 30 additions & 0 deletions src/handlers/assign/tests/tests_candidates.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"]),
);
}

0 comments on commit c26e5a1

Please sign in to comment.