Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a new users_on_vacation config which prevents PR assignment #1712

Merged
merged 1 commit into from
Jul 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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"]),
);
}
Loading