Merge pull request #1712 from jyn514/vacation

Add a new `users_on_vacation` config which prevents PR assignment

src/config.rs

@@ -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)]
@@ -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 }),

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<String>,
@@ -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()

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"]),
+    );
+}