Skip to content

Pick reviewer who is not previous assignee when r? group #1958

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Pick reviewer who is not previous assignee when r? group #1958

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
117 changes: 88 additions & 29 deletions src/handlers/assign.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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)]
Expand Down Expand Up @@ -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>,
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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!(
Expand All @@ -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?;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When set assignee, we store it into database.


Ok(())
}

/// Determines who to assign the PR to based on either an `r?` command, or
Expand All @@ -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
Expand All @@ -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)),
Expand All @@ -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 { .. },
Comment on lines +321 to +325
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's worth noting here. Should we use unreachable!() except NoReviewer?

) => log::trace!(
"no reviewer could be determined for PR {}: {e}",
event.issue.global_id()
Expand All @@ -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!(
Expand Down Expand Up @@ -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");

Expand Down Expand Up @@ -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 {}
Expand Down Expand Up @@ -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)
)
}
}
}
}
Expand All @@ -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,
Expand All @@ -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?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that #1947 completely refactors these tests and provides them access to an actual database, so it might be better to base the code on top of that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, when #1947 is merged, I'll rewrite the test.

// This uses a relatively primitive random choice algorithm.
// GitHub's CODEOWNERS supports much more sophisticated options, such as:
//
Expand Down Expand Up @@ -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,
Expand All @@ -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);
Comment on lines +841 to +842
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is used to know if the candidate is a previous reviewer.


// Record the reason why the candidate was filtered out
let reason = {
if is_pr_author {
Expand All @@ -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(),
})
Comment on lines +858 to +863
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added logic here to determine if it's a previous Reviewer here only when r? group (expansion happened).

} else {
None
}
Expand Down Expand Up @@ -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
}
Comment on lines +909 to +924
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This corresponds to set_assignee, which stores, and this function is used to read from the database. cfg!(test) returns null in order to pass the tests.

Loading