Skip to content

Commit 5afc243

Browse files
committed
Ensure user capacity is respected on PR assignment
This check is specifically used when assigning from the Github web UI Signed-off-by: apiraino <apiraino@users.noreply.github.com>
1 parent 73653c5 commit 5afc243

File tree

4 files changed

+61
-14
lines changed

4 files changed

+61
-14
lines changed

.env.sample

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,4 +20,8 @@ GITHUB_WEBHOOK_SECRET=MUST_BE_CONFIGURED
2020
# ZULIP_API_TOKEN=yyy
2121

2222
# Authenticates inbound webhooks from Github
23-
# ZULIP_TOKEN=xxx
23+
# ZULIP_TOKEN=xxx
24+
25+
# Use another endpoint to retrieve teams of the Rust project (useful for local testing)
26+
# default: https://team-api.infra.rust-lang.org/v1
27+
# TEAMS_API_URL=http://localhost:8080

src/handlers.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -207,7 +207,7 @@ macro_rules! issue_handlers {
207207

208208
// Handle events that happened on issues
209209
//
210-
// This is for events that happen only on issues (e.g. label changes).
210+
// This is for events that happen only on issues or pull requests (e.g. label changes or assignments).
211211
// Each module in the list must contain the functions `parse_input` and `handle_input`.
212212
issue_handlers! {
213213
assign,
@@ -328,7 +328,7 @@ macro_rules! command_handlers {
328328
//
329329
// This is for handlers for commands parsed by the `parser` crate.
330330
// Each variant of `parser::command::Command` must be in this list,
331-
// preceded by the module containing the coresponding `handle_command` function
331+
// preceded by the module containing the corresponding `handle_command` function
332332
command_handlers! {
333333
assign: Assign,
334334
glacier: Glacier,

src/handlers/assign.rs

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -728,7 +728,6 @@ impl fmt::Display for FindReviewerError {
728728
f,
729729
"Could not assign reviewer from: `{}`.\n\
730730
User(s) `{}` are either the PR author, already assigned, or on vacation. \
731-
If it's a team, we could not find any candidates.\n\
732731
Please use `r?` to specify someone else to assign.",
733732
initial.join(","),
734733
filtered.join(","),
@@ -820,14 +819,11 @@ SELECT username
820819
FROM review_prefs r
821820
JOIN users on users.user_id=r.user_id
822821
AND username = ANY('{{ {} }}')
823-
AND ARRAY_LENGTH(r.assigned_prs, 1) < max_assigned_prs",
822+
AND CARDINALITY(r.assigned_prs) < LEAST(COALESCE(r.max_assigned_prs,1000000))",
824823
usernames
825824
);
826825
let result = db.query(&q, &[]).await.context("Select DB error")?;
827-
let mut candidates: HashSet<String> = HashSet::new();
828-
for row in result {
829-
candidates.insert(row.get("username"));
830-
}
826+
let candidates: HashSet<String> = result.iter().map(|row| row.get("username")).collect();
831827
Ok(candidates)
832828
}
833829

src/handlers/pr_tracking.rs

Lines changed: 52 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,23 @@
11
//! This module updates the PR workqueue of the Rust project contributors
2+
//! Runs after a PR has been assigned or unassigned
23
//!
34
//! Purpose:
45
//!
5-
//! - Adds the PR to the workqueue of one team member (when the PR has been assigned)
6-
//! - Removes the PR from the workqueue of one team member (when the PR is unassigned or closed)
6+
//! - Adds the PR to the workqueue of one team member (after the PR has been assigned)
7+
//! - Removes the PR from the workqueue of one team member (after the PR has been unassigned or closed)
78
89
use crate::{
910
config::ReviewPrefsConfig,
1011
db::notifications::record_username,
1112
github::{IssuesAction, IssuesEvent},
1213
handlers::Context,
14+
ReviewPrefs,
1315
};
1416
use anyhow::Context as _;
1517
use tokio_postgres::Client as DbClient;
1618

19+
use super::assign::{FindReviewerError, REVIEWER_HAS_NO_CAPACITY, SELF_ASSIGN_HAS_NO_CAPACITY};
20+
1721
pub(super) struct ReviewPrefsInput {}
1822

1923
pub(super) async fn parse_input(
@@ -49,7 +53,7 @@ pub(super) async fn handle_input<'a>(
4953
) -> anyhow::Result<()> {
5054
let db_client = ctx.db.get().await;
5155

52-
// extract the assignee matching the assignment or unassignment enum variants or return and ignore this handler
56+
// extract the assignee or ignore this handler and return
5357
let IssuesEvent {
5458
action: IssuesAction::Assigned { assignee } | IssuesAction::Unassigned { assignee },
5559
..
@@ -66,18 +70,61 @@ pub(super) async fn handle_input<'a>(
6670
if matches!(event.action, IssuesAction::Unassigned { .. }) {
6771
delete_pr_from_workqueue(&db_client, assignee.id, event.issue.number)
6872
.await
69-
.context("Failed to remove PR from workqueue")?;
73+
.context("Failed to remove PR from work ueue")?;
7074
}
7175

76+
// This handler is reached also when assigning a PR using the Github UI
77+
// (i.e. from the "Assignees" dropdown menu).
78+
// We need to also check assignee availability here.
7279
if matches!(event.action, IssuesAction::Assigned { .. }) {
80+
let work_queue = has_user_capacity(&db_client, &assignee.login)
81+
.await
82+
.context("Failed to retrieve user work queue");
83+
84+
// if user has no capacity, revert the PR assignment (GitHub has already assigned it)
85+
// and post a comment suggesting what to do
86+
if let Err(_) = work_queue {
87+
event
88+
.issue
89+
.remove_assignees(&ctx.github, crate::github::Selection::One(&assignee.login))
90+
.await?;
91+
92+
let msg = if assignee.login.to_lowercase() == event.issue.user.login.to_lowercase() {
93+
SELF_ASSIGN_HAS_NO_CAPACITY.replace("{username}", &assignee.login)
94+
} else {
95+
REVIEWER_HAS_NO_CAPACITY.replace("{username}", &assignee.login)
96+
};
97+
event.issue.post_comment(&ctx.github, &msg).await?;
98+
}
99+
73100
upsert_pr_into_workqueue(&db_client, assignee.id, event.issue.number)
74101
.await
75-
.context("Failed to add PR to workqueue")?;
102+
.context("Failed to add PR to work queue")?;
76103
}
77104

78105
Ok(())
79106
}
80107

108+
// TODO: we should just fetch the number of assigned prs and max assigned prs. The caller should do the check.
109+
pub async fn has_user_capacity(
110+
db: &crate::db::PooledClient,
111+
assignee: &str,
112+
) -> anyhow::Result<ReviewPrefs, FindReviewerError> {
113+
let q = "
114+
SELECT username, r.*
115+
FROM review_prefs r
116+
JOIN users ON users.user_id = r.user_id
117+
WHERE username = $1
118+
AND CARDINALITY(r.assigned_prs) < LEAST(COALESCE(r.max_assigned_prs,1000000));";
119+
let rec = db.query_one(q, &[&assignee]).await;
120+
if let Err(_) = rec {
121+
return Err(FindReviewerError::ReviewerHasNoCapacity {
122+
username: assignee.to_string(),
123+
});
124+
}
125+
Ok(rec.unwrap().into())
126+
}
127+
81128
/// Add a PR to the workqueue of a team member.
82129
/// Ensures no accidental PR duplicates.
83130
async fn upsert_pr_into_workqueue(

0 commit comments

Comments
 (0)