diff --git a/src/handlers/assign.rs b/src/handlers/assign.rs index 3a91e501..f86d9a09 100644 --- a/src/handlers/assign.rs +++ b/src/handlers/assign.rs @@ -7,6 +7,9 @@ //! * `@rustbot release-assignment`: Removes the commenter's assignment. //! * `r? @user`: Assigns to the given user (PRs only). //! +//! Note: this module does not handle review assignments issued from the +//! GitHub "Assignees" dropdown menu +//! //! This is capable of assigning to any user, even if they do not have write //! access to the repo. It does this by fake-assigning the bot and adding a //! "claimed by" section to the top-level comment. @@ -20,7 +23,7 @@ use crate::{ config::AssignConfig, github::{self, Event, FileDiff, Issue, IssuesAction, Selection}, - handlers::{Context, GithubClient, IssuesEvent}, + handlers::{pr_tracking::has_user_capacity, Context, GithubClient, IssuesEvent}, interactions::EditIssueBody, }; use anyhow::{bail, Context as _}; @@ -30,6 +33,7 @@ use rand::seq::IteratorRandom; use rust_team_data::v1::Teams; use std::collections::{HashMap, HashSet}; use std::fmt; +use tokio_postgres::Client as DbClient; use tracing as log; #[cfg(test)] @@ -75,6 +79,27 @@ const NON_DEFAULT_BRANCH: &str = const SUBMODULE_WARNING_MSG: &str = "These commits modify **submodules**."; +pub const SELF_ASSIGN_HAS_NO_CAPACITY: &str = " +You have insufficient capacity to be assigned the pull request at this time. PR assignment has been reverted. + +Please choose another assignee or increase your assignment limit. + +(see [documentation](https://forge.rust-lang.org/triagebot/pr-assignment-tracking.html))"; + +pub const REVIEWER_HAS_NO_CAPACITY: &str = " +`{username}` has insufficient capacity to be assigned the pull request at this time. PR assignment has been reverted. + +Please choose another assignee. + +(see [documentation](https://forge.rust-lang.org/triagebot/pr-assignment-tracking.html))"; + +const NO_REVIEWER_HAS_CAPACITY: &str = " +Could not find a reviewer with enough capacity to be assigned at this time. This is a problem. + +Please contact us on [#t-infra](https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra) on Zulip. + +cc: @**Jack Huey** @**apiraino**"; + fn on_vacation_msg(user: &str) -> String { ON_VACATION_WARNING.replace("{username}", user) } @@ -272,6 +297,8 @@ async fn set_assignee(issue: &Issue, github: &GithubClient, username: &str) { /// Determines who to assign the PR to based on either an `r?` command, or /// based on which files were modified. /// +/// Will also check if candidates have capacity in their work queue. +/// /// Returns `(assignee, from_comment)` where `assignee` is who to assign to /// (or None if no assignee could be found). `from_comment` is a boolean /// indicating if the assignee came from an `r?` command (it is false if @@ -282,13 +309,14 @@ async fn determine_assignee( config: &AssignConfig, diff: &[FileDiff], ) -> anyhow::Result<(Option, bool)> { + let db_client = ctx.db.get().await; let teams = crate::team_data::teams(&ctx.github).await?; if let Some(name) = find_assign_command(ctx, event) { if is_self_assign(&name, &event.issue.user.login) { return Ok((Some(name.to_string()), true)); } // User included `r?` in the opening PR body. - match find_reviewer_from_names(&teams, config, &event.issue, &[name]) { + match find_reviewer_from_names(&db_client, &teams, config, &event.issue, &[name]).await { Ok(assignee) => return Ok((Some(assignee), true)), Err(e) => { event @@ -302,7 +330,9 @@ 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(&teams, config, &event.issue, &candidates) { + match find_reviewer_from_names(&db_client, &teams, config, &event.issue, &candidates) + .await + { Ok(assignee) => return Ok((Some(assignee), false)), Err(FindReviewerError::TeamNotFound(team)) => log::warn!( "team {team} not found via diff from PR {}, \ @@ -312,7 +342,9 @@ async fn determine_assignee( // TODO: post a comment on the PR if the reviewers were filtered due to being on vacation Err( e @ FindReviewerError::NoReviewer { .. } - | e @ FindReviewerError::AllReviewersFiltered { .. }, + | e @ FindReviewerError::AllReviewersFiltered { .. } + | e @ FindReviewerError::NoReviewerHasCapacity + | e @ FindReviewerError::ReviewerHasNoCapacity { .. }, ) => log::trace!( "no reviewer could be determined for PR {}: {e}", event.issue.global_id() @@ -330,7 +362,7 @@ async fn determine_assignee( } if let Some(fallback) = config.adhoc_groups.get("fallback") { - match find_reviewer_from_names(&teams, config, &event.issue, fallback) { + match find_reviewer_from_names(&db_client, &teams, config, &event.issue, fallback).await { Ok(assignee) => return Ok((Some(assignee), false)), Err(e) => { log::trace!( @@ -492,7 +524,20 @@ pub(super) async fn handle_command( // welcome message). return Ok(()); } + let db_client = ctx.db.get().await; if is_self_assign(&name, &event.user().login) { + match has_user_capacity(&db_client, &name).await { + Ok(work_queue) => work_queue.username, + Err(_) => { + issue + .post_comment( + &ctx.github, + &REVIEWER_HAS_NO_CAPACITY.replace("{username}", &name), + ) + .await?; + return Ok(()); + } + }; name.to_string() } else { let teams = crate::team_data::teams(&ctx.github).await?; @@ -512,8 +557,14 @@ pub(super) async fn handle_command( } } } - - match find_reviewer_from_names(&teams, config, issue, &[team_name.to_string()]) + match find_reviewer_from_names( + &db_client, + &teams, + config, + issue, + &[team_name.to_string()], + ) + .await { Ok(assignee) => assignee, Err(e) => { @@ -524,7 +575,11 @@ pub(super) async fn handle_command( } } }; + + // This user is validated and can accept the PR set_assignee(issue, &ctx.github, &username).await; + // This PR will now be registered in the reviewer's work queue + // by the `pr_tracking` handler return Ok(()); } @@ -582,6 +637,7 @@ pub(super) async fn handle_command( e.apply(&ctx.github, String::new(), &data).await?; + // Assign the PR: user's work queue has been checked and can accept this PR match issue.set_assignee(&ctx.github, &to_assign).await { Ok(()) => return Ok(()), // we are done Err(github::AssignmentError::InvalidAssignee) => { @@ -603,7 +659,7 @@ pub(super) async fn handle_command( } #[derive(PartialEq, Debug)] -enum FindReviewerError { +pub enum FindReviewerError { /// User specified something like `r? foo/bar` where that team name could /// not be found. TeamNotFound(String), @@ -621,6 +677,11 @@ enum FindReviewerError { initial: Vec, filtered: Vec, }, + /// No reviewer has capacity to accept a pull request assignment at this time + NoReviewerHasCapacity, + /// The requested reviewer has no capacity to accept a pull request + /// assignment at this time + ReviewerHasNoCapacity { username: String }, } impl std::error::Error for FindReviewerError {} @@ -650,13 +711,23 @@ impl fmt::Display for FindReviewerError { write!( f, "Could not assign reviewer from: `{}`.\n\ - 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.", + User(s) `{}` are either the PR author, already assigned, or on vacation. \ + If it's a team, we could not find any candidates.\n\ + Please use `r?` to specify someone else to assign.", initial.join(","), filtered.join(","), ) } + FindReviewerError::ReviewerHasNoCapacity { username } => { + write!( + f, + "{}", + REVIEWER_HAS_NO_CAPACITY.replace("{username}", username) + ) + } + FindReviewerError::NoReviewerHasCapacity => { + write!(f, "{}", NO_REVIEWER_HAS_CAPACITY) + } } } } @@ -667,7 +738,8 @@ impl fmt::Display for FindReviewerError { /// `@octocat`, or names from the owners map. It can contain GitHub usernames, /// auto-assign groups, or rust-lang team names. It must have at least one /// entry. -fn find_reviewer_from_names( +async fn find_reviewer_from_names( + db: &DbClient, teams: &Teams, config: &AssignConfig, issue: &Issue, @@ -693,14 +765,57 @@ fn find_reviewer_from_names( // // These are all ideas for improving the selection here. However, I'm not // sure they are really worth the effort. - Ok(candidates + + // filter out team members without capacity + let filtered_candidates = filter_by_capacity(db, &candidates) + .await + .expect("Error while filtering out team members"); + + if filtered_candidates.is_empty() { + return Err(FindReviewerError::AllReviewersFiltered { + initial: names.to_vec(), + filtered: names.to_vec(), + }); + } + + log::debug!("Filtered list of candidates: {:?}", filtered_candidates); + + Ok(filtered_candidates .into_iter() .choose(&mut rand::thread_rng()) - .expect("candidate_reviewers_from_names always returns at least one entry") + .expect("candidate_reviewers_from_names should return at least one entry") .to_string()) } -/// Returns a list of candidate usernames to choose as a reviewer. +/// Filter out candidates not having review capacity +async fn filter_by_capacity( + db: &DbClient, + candidates: &HashSet<&str>, +) -> anyhow::Result> { + let usernames = candidates + .iter() + .map(|c| *c) + .collect::>() + .join(","); + + let q = format!( + " +SELECT username +FROM review_prefs r +JOIN users on users.user_id=r.user_id +AND username = ANY('{{ {} }}') +AND ARRAY_LENGTH(r.assigned_prs, 1) < max_assigned_prs", + usernames + ); + let result = db.query(&q, &[]).await.context("Select DB error")?; + let mut candidates: HashSet = HashSet::new(); + for row in result { + candidates.insert(row.get("username")); + } + Ok(candidates) +} + +/// returns a list of candidate usernames (from relevant teams) to choose as a reviewer. fn candidate_reviewers_from_names<'a>( teams: &'a Teams, config: &'a AssignConfig, diff --git a/src/lib.rs b/src/lib.rs index 8aedf117..ad8e7ba4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -137,10 +137,9 @@ pub struct ReviewPrefs { impl ReviewPrefs { fn to_string(&self) -> String { - let capacity = if self.max_assigned_prs.is_some() { - format!("{}", self.max_assigned_prs.expect("This can't fail")) - } else { - String::from("Not set (i.e. unlimited)") + let capacity = match self.max_assigned_prs { + Some(max) => format!("{}", max), + None => String::from("Not set (i.e. unlimited)"), }; let prs = self .assigned_prs