diff --git a/src/handlers/assign.rs b/src/handlers/assign.rs index 749185929..559409f66 100644 --- a/src/handlers/assign.rs +++ b/src/handlers/assign.rs @@ -20,6 +20,7 @@ //! `assign.owners` config, it will auto-select an assignee based on the files //! the PR modifies. +use crate::db::issue_data::IssueData; use crate::db::review_prefs::{get_review_prefs_batch, RotationMode}; use crate::github::UserId; use crate::handlers::pr_tracking::ReviewerWorkqueue; @@ -92,9 +93,23 @@ const REVIEWER_ALREADY_ASSIGNED: &str = Please choose another assignee."; +const REVIEWER_ASSIGNED_BEFORE: &str = "Requested reviewer @{username} was already assigned before. + +Please choose another assignee by using `r? @reviewer`."; + // Special account that we use to prevent assignment. const GHOST_ACCOUNT: &str = "ghost"; +/// Key for the state in the database +const PREVIOUS_REVIEWERS_KEY: &str = "previous-reviewers"; + +/// State stored in the database +#[derive(Debug, Clone, PartialEq, Default, serde::Deserialize, serde::Serialize)] +struct Reviewers { + // names are stored in lowercase + names: HashSet, +} + /// Assignment data stored in the issue/PR body. #[derive(Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] struct AssignData { @@ -217,7 +232,7 @@ pub(super) async fn handle_input( None }; if let Some(assignee) = assignee { - set_assignee(&event.issue, &ctx.github, &assignee).await; + set_assignee(&ctx, &event.issue, &ctx.github, &assignee).await?; } if let Some(welcome) = welcome { @@ -249,7 +264,16 @@ fn is_self_assign(assignee: &str, pr_author: &str) -> bool { } /// Sets the assignee of a PR, alerting any errors. -async fn set_assignee(issue: &Issue, github: &GithubClient, username: &str) { +async fn set_assignee( + ctx: &Context, + issue: &Issue, + github: &GithubClient, + username: &str, +) -> anyhow::Result<()> { + let mut db = ctx.db.get().await; + let mut state: IssueData<'_, Reviewers> = + IssueData::load(&mut db, &issue, PREVIOUS_REVIEWERS_KEY).await?; + // Don't re-assign if already assigned, e.g. on comment edit if issue.contain_assignee(&username) { log::trace!( @@ -257,7 +281,7 @@ async fn set_assignee(issue: &Issue, github: &GithubClient, username: &str) { issue.global_id(), username, ); - return; + return Ok(()); } if let Err(err) = issue.set_assignee(github, &username).await { log::warn!( @@ -280,8 +304,15 @@ async fn set_assignee(issue: &Issue, github: &GithubClient, username: &str) { .await { log::warn!("failed to post error comment: {e}"); + return Err(e); } } + + // Record the reviewer in the database + + state.data.names.insert(username.to_lowercase()); + state.save().await?; + Ok(()) } /// Determines who to assign the PR to based on either an `r?` command, or @@ -300,12 +331,12 @@ async fn determine_assignee( config: &AssignConfig, diff: &[FileDiff], ) -> anyhow::Result<(Option, bool)> { - let db_client = ctx.db.get().await; + let mut db_client = ctx.db.get().await; let teams = crate::team_data::teams(&ctx.github).await?; if let Some(name) = assign_command { // User included `r?` in the opening PR body. match find_reviewer_from_names( - &db_client, + &mut db_client, ctx.workqueue.clone(), &teams, config, @@ -328,7 +359,7 @@ async fn determine_assignee( match find_reviewers_from_diff(config, diff) { Ok(candidates) if !candidates.is_empty() => { match find_reviewer_from_names( - &db_client, + &mut db_client, ctx.workqueue.clone(), &teams, config, @@ -347,6 +378,7 @@ async fn determine_assignee( e @ FindReviewerError::NoReviewer { .. } | e @ FindReviewerError::ReviewerIsPrAuthor { .. } | e @ FindReviewerError::ReviewerAlreadyAssigned { .. } + | e @ FindReviewerError::ReviewerPreviouslyAssigned { .. } | e @ FindReviewerError::ReviewerOffRotation { .. } | e @ FindReviewerError::DatabaseError(_) | e @ FindReviewerError::ReviewerAtMaxCapacity { .. }, @@ -368,7 +400,7 @@ async fn determine_assignee( if let Some(fallback) = config.adhoc_groups.get("fallback") { match find_reviewer_from_names( - &db_client, + &mut db_client, ctx.workqueue.clone(), &teams, config, @@ -550,10 +582,9 @@ pub(super) async fn handle_command( issue.remove_assignees(&ctx.github, Selection::All).await?; return Ok(()); } - - let db_client = ctx.db.get().await; + let mut db_client = ctx.db.get().await; let assignee = match find_reviewer_from_names( - &db_client, + &mut db_client, ctx.workqueue.clone(), &teams, config, @@ -569,7 +600,7 @@ pub(super) async fn handle_command( } }; - set_assignee(issue, &ctx.github, &assignee).await; + set_assignee(ctx, issue, &ctx.github, &assignee).await?; } else { let e = EditIssueBody::new(&issue, "ASSIGN"); @@ -680,6 +711,8 @@ enum FindReviewerError { ReviewerIsPrAuthor { username: String }, /// Requested reviewer is already assigned to that PR ReviewerAlreadyAssigned { username: String }, + /// Requested reviewer was already assigned previously to that PR. + ReviewerPreviouslyAssigned { username: String }, /// Data required for assignment could not be loaded from the DB. DatabaseError(String), /// The reviewer has too many PRs alreayd assigned. @@ -726,6 +759,13 @@ impl fmt::Display for FindReviewerError { REVIEWER_ALREADY_ASSIGNED.replace("{username}", username) ) } + FindReviewerError::ReviewerPreviouslyAssigned { username } => { + write!( + f, + "{}", + REVIEWER_ASSIGNED_BEFORE.replace("{username}", username) + ) + } FindReviewerError::DatabaseError(error) => { write!(f, "Database error: {error}") } @@ -748,7 +788,7 @@ Please select a different reviewer.", /// auto-assign groups, or rust-lang team names. It must have at least one /// entry. async fn find_reviewer_from_names( - db: &DbClient, + db: &mut DbClient, workqueue: Arc>, teams: &Teams, config: &AssignConfig, @@ -916,7 +956,7 @@ fn expand_teams_and_groups( /// Returns a list of candidate usernames (from relevant teams) to choose as a reviewer. /// If no reviewer is available, returns an error. async fn candidate_reviewers_from_names<'a>( - db: &DbClient, + db: &mut DbClient, workqueue: Arc>, teams: &'a Teams, config: &'a AssignConfig, @@ -937,6 +977,7 @@ async fn candidate_reviewers_from_names<'a>( // Set of candidate usernames to choose from. // We go through each expanded candidate and store either success or an error for them. let mut candidates: Vec> = Vec::new(); + let previous_reviewer_names = get_previous_reviewer_names(db, issue).await; // Step 2: pre-filter candidates based on checks that we can perform quickly for reviewer_candidate in expanded { @@ -949,6 +990,8 @@ async fn candidate_reviewers_from_names<'a>( .iter() .any(|assignee| name_lower == assignee.login.to_lowercase()); + let is_previously_assigned = previous_reviewer_names.contains(&name_lower); + // Record the reason why the candidate was filtered out let reason = { if is_pr_author { @@ -963,6 +1006,14 @@ async fn candidate_reviewers_from_names<'a>( Some(FindReviewerError::ReviewerAlreadyAssigned { username: candidate.clone(), }) + } else if reviewer_candidate.origin == ReviewerCandidateOrigin::Expanded + && is_previously_assigned + { + // **Only** when r? group is expanded, we consider the reviewer previously assigned + // `r? @reviewer` will not consider the reviewer previously assigned + Some(FindReviewerError::ReviewerPreviouslyAssigned { + username: candidate.clone(), + }) } else { None } @@ -1058,3 +1109,13 @@ async fn candidate_reviewers_from_names<'a>( .collect()) } } + +async fn get_previous_reviewer_names(db: &mut DbClient, issue: &Issue) -> HashSet { + let state: IssueData<'_, Reviewers> = + match IssueData::load(db, &issue, PREVIOUS_REVIEWERS_KEY).await { + Ok(state) => state, + Err(_) => return HashSet::new(), + }; + + state.data.names +} diff --git a/src/handlers/assign/tests/tests_candidates.rs b/src/handlers/assign/tests/tests_candidates.rs index e84b0b6a7..3a26fe1f7 100644 --- a/src/handlers/assign/tests/tests_candidates.rs +++ b/src/handlers/assign/tests/tests_candidates.rs @@ -3,6 +3,7 @@ use super::super::*; use crate::db::review_prefs::{upsert_review_prefs, RotationMode}; use crate::github::{PullRequestNumber, User}; +use crate::handlers::pr_tracking::ReviewerWorkqueue; use crate::tests::github::{issue, user}; use crate::tests::{run_db_test, TestContext}; @@ -71,15 +72,28 @@ impl AssignCtx { self } + async fn set_previous_reviewers(mut self, users: HashSet<&User>) -> Self { + let mut db = self.test_ctx.db_client_mut(); + let mut state: IssueData<'_, Reviewers> = + IssueData::load(&mut db, &self.issue, PREVIOUS_REVIEWERS_KEY) + .await + .unwrap(); + + // Create a new set with all user names (overwrite existing data) + state.data.names = users.iter().map(|user| user.login.to_lowercase()).collect(); + state.save().await.unwrap(); + self + } + async fn check( - self, + mut self, names: &[&str], expected: Result<&[&str], FindReviewerError>, ) -> anyhow::Result { let names: Vec<_> = names.iter().map(|n| n.to_string()).collect(); let reviewers = candidate_reviewers_from_names( - self.test_ctx.db_client(), + self.test_ctx.db_client_mut(), Arc::new(RwLock::new(self.reviewer_workqueue)), &self.teams, &self.config, @@ -527,3 +541,58 @@ async fn vacation() { }) .await; } + +#[tokio::test] +async fn previous_reviewers_ignore_in_team_success() { + let teams = toml::toml!(compiler = ["martin", "jyn514"]); + let config = toml::Table::new(); + run_db_test(|ctx| async move { + let user = user("martin", 1); + basic_test(ctx, config, issue().call()) + .teams(&teams) + .set_previous_reviewers(HashSet::from([&user])) + .await + .check(&["compiler"], Ok(&["jyn514"])) + .await + }) + .await; +} + +#[tokio::test] +async fn previous_reviewers_ignore_in_team_failed() { + let teams = toml::toml!(compiler = ["martin", "jyn514"]); + let config = toml::Table::new(); + run_db_test(|ctx| async move { + let user1 = user("martin", 1); + let user2 = user("jyn514", 2); + basic_test(ctx, config, issue().call()) + .teams(&teams) + .set_previous_reviewers(HashSet::from([&user1, &user2])) + .await + .check( + &["compiler"], + Err(FindReviewerError::NoReviewer { + initial: vec!["compiler".to_string()], + }), + ) + .await + }) + .await +} + +#[tokio::test] +async fn previous_reviewers_direct_assignee() { + let teams = toml::toml!(compiler = ["martin", "jyn514"]); + let config = toml::Table::new(); + run_db_test(|ctx| async move { + let user1 = user("martin", 1); + let user2 = user("jyn514", 2); + basic_test(ctx, config, issue().call()) + .teams(&teams) + .set_previous_reviewers(HashSet::from([&user1, &user2])) + .await + .check(&["jyn514"], Ok(&["jyn514"])) + .await + }) + .await +} diff --git a/src/tests/mod.rs b/src/tests/mod.rs index 9e0439617..895433fc3 100644 --- a/src/tests/mod.rs +++ b/src/tests/mod.rs @@ -94,6 +94,10 @@ impl TestContext { &self.client } + pub(crate) fn db_client_mut(&mut self) -> &mut PooledClient { + &mut self.client + } + pub(crate) async fn add_user(&self, name: &str, id: u64) { record_username(self.db_client(), id, name) .await