Pick reviewer which was not assigned before **only** when r? group

Signed-off-by: xizheyin <[email protected]>

Author: xizheyin

src/handlers/assign.rs

@@ -22,6 +22,7 @@
 
 use crate::{
     config::AssignConfig,
+    db::issue_data::IssueData,
     github::{self, Event, FileDiff, Issue, IssuesAction, Selection},
     handlers::{Context, GithubClient, IssuesEvent},
     interactions::EditIssueBody,
@@ -33,7 +34,6 @@ 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)]
@@ -87,9 +87,23 @@ const REVIEWER_ALREADY_ASSIGNED: &str =
 
 Please choose another assignee.";
 
+const REVIEWER_ASSIGNED_BEFORE: &str = "Requested reviewers are 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_REVIEWER_KEY: &str = "previous-reviewer";
+
+/// State stored in the database
+#[derive(Debug, Default, serde::Deserialize, serde::Serialize)]
+struct Reviewer {
+    /// List of the last warnings in the most recent comment.
+    names: HashSet<String>,
+}
+
 #[derive(Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)]
 struct AssignData {
     user: Option<String>,
@@ -179,7 +193,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 {
@@ -211,15 +225,24 @@ 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<'_, Reviewer> =
+        IssueData::load(&mut db, &issue, PREVIOUS_REVIEWER_KEY).await?;
+
     // Don't re-assign if already assigned, e.g. on comment edit
     if issue.contain_assignee(&username) {
         log::trace!(
             "ignoring assign PR {} to {}, already assigned",
             issue.global_id(),
             username,
         );
-        return;
+        return Ok(());
     }
     if let Err(err) = issue.set_assignee(github, &username).await {
         log::warn!(
@@ -242,8 +265,14 @@ async fn set_assignee(issue: &Issue, github: &GithubClient, username: &str) {
             .await
         {
             log::warn!("failed to post error comment: {e}");
+            return Err(e);
         }
     }
+
+    state.data.names.insert(username.to_string());
+    state.save().await?;
+
+    Ok(())
 }
 
 /// Determines who to assign the PR to based on either an `r?` command, or
@@ -261,11 +290,10 @@ async fn determine_assignee(
     config: &AssignConfig,
     diff: &[FileDiff],
 ) -> anyhow::Result<(Option<String>, 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) {
         // User included `r?` in the opening PR body.
-        match find_reviewer_from_names(&db_client, &teams, config, &event.issue, &[name]).await {
+        match find_reviewer_from_names(&ctx, &teams, config, &event.issue, &[name]).await {
             Ok(assignee) => return Ok((Some(assignee), true)),
             Err(e) => {
                 event
@@ -279,7 +307,7 @@ 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(&db_client, &teams, config, &event.issue, &candidates)
+            match find_reviewer_from_names(&ctx, &teams, config, &event.issue, &candidates)
                 .await
             {
                 Ok(assignee) => return Ok((Some(assignee), false)),
@@ -290,9 +318,11 @@ async fn determine_assignee(
                 ),
                 Err(
                     e @ FindReviewerError::NoReviewer { .. }
+                    // TODO: only NoReviewer can be reached here!
                     | e @ FindReviewerError::ReviewerIsPrAuthor { .. }
                     | e @ FindReviewerError::ReviewerAlreadyAssigned { .. }
-                    | e @ FindReviewerError::ReviewerOnVacation { .. },
+                    | e @ FindReviewerError::ReviewerOnVacation { .. }
+                    | e @ FindReviewerError::ReviewerPreviouslyAssigned { .. },
                 ) => log::trace!(
                     "no reviewer could be determined for PR {}: {e}",
                     event.issue.global_id()
@@ -310,7 +340,7 @@ async fn determine_assignee(
     }
 
     if let Some(fallback) = config.adhoc_groups.get("fallback") {
-        match find_reviewer_from_names(&db_client, &teams, config, &event.issue, fallback).await {
+        match find_reviewer_from_names(&ctx, &teams, config, &event.issue, fallback).await {
             Ok(assignee) => return Ok((Some(assignee), false)),
             Err(e) => {
                 log::trace!(
@@ -485,24 +515,18 @@ pub(super) async fn handle_command(
             return Ok(());
         }
 
-        let db_client = ctx.db.get().await;
-        let assignee = match find_reviewer_from_names(
-            &db_client,
-            &teams,
-            config,
-            issue,
-            &[assignee.to_string()],
-        )
-        .await
-        {
-            Ok(assignee) => assignee,
-            Err(e) => {
-                issue.post_comment(&ctx.github, &e.to_string()).await?;
-                return Ok(());
-            }
-        };
+        let assignee =
+            match find_reviewer_from_names(ctx, &teams, config, issue, &[assignee.to_string()])
+                .await
+            {
+                Ok(assignee) => assignee,
+                Err(e) => {
+                    issue.post_comment(&ctx.github, &e.to_string()).await?;
+                    return Ok(());
+                }
+            };
 
-        set_assignee(issue, &ctx.github, &assignee).await;
+        set_assignee(ctx, issue, &ctx.github, &assignee).await?;
     } else {
         let e = EditIssueBody::new(&issue, "ASSIGN");
 
@@ -612,6 +636,8 @@ pub enum FindReviewerError {
     ReviewerIsPrAuthor { username: String },
     /// Requested reviewer is already assigned to that PR
     ReviewerAlreadyAssigned { username: String },
+    /// Requested reviewer is already assigned previously to that PR
+    ReviewerPreviouslyAssigned { username: String },
 }
 
 impl std::error::Error for FindReviewerError {}
@@ -654,6 +680,13 @@ impl fmt::Display for FindReviewerError {
                     REVIEWER_ALREADY_ASSIGNED.replace("{username}", username)
                 )
             }
+            FindReviewerError::ReviewerPreviouslyAssigned { username } => {
+                write!(
+                    f,
+                    "{}",
+                    REVIEWER_ASSIGNED_BEFORE.replace("{username}", username)
+                )
+            }
         }
     }
 }
@@ -665,7 +698,7 @@ impl fmt::Display for FindReviewerError {
 /// auto-assign groups, or rust-lang team names. It must have at least one
 /// entry.
 async fn find_reviewer_from_names(
-    _db: &DbClient,
+    ctx: &Context,
     teams: &Teams,
     config: &AssignConfig,
     issue: &Issue,
@@ -678,7 +711,7 @@ async fn find_reviewer_from_names(
         }
     }
 
-    let candidates = candidate_reviewers_from_names(teams, config, issue, names)?;
+    let candidates = candidate_reviewers_from_names(ctx, teams, config, issue, names).await?;
     // This uses a relatively primitive random choice algorithm.
     // GitHub's CODEOWNERS supports much more sophisticated options, such as:
     //
@@ -782,7 +815,8 @@ fn expand_teams_and_groups(
 
 /// Returns a list of candidate usernames (from relevant teams) to choose as a reviewer.
 /// If not reviewer is available, returns an error.
-fn candidate_reviewers_from_names<'a>(
+async fn candidate_reviewers_from_names<'a>(
+    ctx: &Context,
     teams: &'a Teams,
     config: &'a AssignConfig,
     issue: &Issue,
@@ -804,6 +838,9 @@ fn candidate_reviewers_from_names<'a>(
             .iter()
             .any(|assignee| name_lower == assignee.login.to_lowercase());
 
+        let previous_reviewer_names = get_previous_reviewer_names(ctx, issue).await;
+        let is_previously_assigned = previous_reviewer_names.contains(&candidate);
+
         // Record the reason why the candidate was filtered out
         let reason = {
             if is_pr_author {
@@ -818,6 +855,12 @@ fn candidate_reviewers_from_names<'a>(
                 Some(FindReviewerError::ReviewerAlreadyAssigned {
                     username: candidate.clone(),
                 })
+            } else if expansion_happened && 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
             }
@@ -863,3 +906,19 @@ fn candidate_reviewers_from_names<'a>(
         Ok(valid_candidates)
     }
 }
+
+async fn get_previous_reviewer_names(ctx: &Context, issue: &Issue) -> HashSet<String> {
+    if cfg!(test) {
+        return HashSet::new();
+    }
+
+    // Get the state of the warnings for this PR in the database.
+    let mut db = ctx.db.get().await;
+    let state: IssueData<'_, Reviewer> =
+        match IssueData::load(&mut db, &issue, PREVIOUS_REVIEWER_KEY).await {
+            Ok(state) => state,
+            Err(_) => return HashSet::new(),
+        };
+
+    state.data.names
+}