Skip to content
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.

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

Github user id is not optional #1799

Merged
merged 1 commit into from
Apr 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
27 changes: 15 additions & 12 deletions src/github.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use tracing as log;
#[derive(Debug, PartialEq, Eq, serde::Deserialize)]
pub struct User {
pub login: String,
pub id: Option<u64>,
pub id: u64,
}

impl GithubClient {
Expand Down Expand Up @@ -200,17 +200,20 @@ impl User {
);
Ok(in_all || is_triager || is_pri_member || is_async_member)
}
}

// Returns the ID of the given user, if the user is in the `all` team.
pub async fn get_id<'a>(&'a self, client: &'a GithubClient) -> anyhow::Result<Option<u64>> {
let permission = crate::team_data::teams(client).await?;
let map = permission.teams;
Ok(map["all"]
.members
.iter()
.find(|g| g.github == self.login)
.map(|u| u.github_id))
}
// Returns the ID of the given user, if the user is in the `all` team.
pub async fn get_id_for_username<'a>(
jackh726 marked this conversation as resolved.
Show resolved Hide resolved
client: &'a GithubClient,
login: &str,
) -> anyhow::Result<Option<u64>> {
let permission = crate::team_data::teams(client).await?;
let map = permission.teams;
Ok(map["all"]
.members
.iter()
.find(|g| g.github == login)
.map(|u| u.github_id))
}

pub async fn get_team(
Expand Down Expand Up @@ -2670,7 +2673,7 @@ pub async fn retrieve_pull_requests(
prs_processed.push((
User {
login: user.login.clone(),
id: Some(user_id),
id: user_id,
},
pr.number,
));
Expand Down
28 changes: 12 additions & 16 deletions src/handlers/notification.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
//! Parsing is done in the `parser::command::ping` module.

use crate::db::notifications;
use crate::github::get_id_for_username;
use crate::{
github::{self, Event},
handlers::Context,
Expand Down Expand Up @@ -58,7 +59,7 @@ pub async fn handle(ctx: &Context, event: &Event) -> anyhow::Result<()> {
if let Some(from) = event.comment_from() {
for login in parser::get_mentions(from).into_iter() {
if let Some((users, _)) = id_from_user(ctx, login).await? {
users_notified.extend(users.into_iter().map(|user| user.id.unwrap()));
users_notified.extend(users.into_iter().map(|user| user.id));
}
}
};
Expand All @@ -68,7 +69,7 @@ pub async fn handle(ctx: &Context, event: &Event) -> anyhow::Result<()> {
//
// If the user intended to ping themselves, they can add the GitHub comment
// via the Zulip interface.
match event.user().get_id(&ctx.github).await {
match get_id_for_username(&ctx.github, &event.user().login).await {
Ok(Some(id)) => {
users_notified.insert(id.try_into().unwrap());
}
Expand All @@ -86,12 +87,12 @@ pub async fn handle(ctx: &Context, event: &Event) -> anyhow::Result<()> {

let client = ctx.db.get().await;
for user in users {
if !users_notified.insert(user.id.unwrap()) {
if !users_notified.insert(user.id) {
// Skip users already associated with this event.
continue;
}

if let Err(err) = notifications::record_username(&client, user.id.unwrap(), &user.login)
if let Err(err) = notifications::record_username(&client, user.id, &user.login)
.await
.context("failed to record username")
{
Expand All @@ -101,7 +102,7 @@ pub async fn handle(ctx: &Context, event: &Event) -> anyhow::Result<()> {
if let Err(err) = notifications::record_ping(
&client,
&notifications::Notification {
user_id: user.id.unwrap(),
user_id: user.id,
origin_url: event.html_url().unwrap().to_owned(),
origin_html: body.to_owned(),
time: event.time().unwrap(),
Expand Down Expand Up @@ -166,30 +167,25 @@ async fn id_from_user(
team.members
.into_iter()
.map(|member| github::User {
id: Some(member.github_id),
id: member.github_id,
login: member.github,
})
.collect::<Vec<github::User>>(),
Some(team.name),
)))
} else {
let user = github::User {
login: login.to_owned(),
id: None,
};
let id = user
.get_id(&ctx.github)
let id = get_id_for_username(&ctx.github, login)
.await
.with_context(|| format!("failed to get user {} ID", user.login))?;
.with_context(|| format!("failed to get user {} ID", login))?;
let Some(id) = id else {
// If the user was not in the team(s) then just don't record it.
log::trace!("Skipping {} because no id found", user.login);
log::trace!("Skipping {} because no id found", login);
return Ok(None);
};
Ok(Some((
vec![github::User {
login: user.login.clone(),
id: Some(id),
login: login.to_string(),
id,
}],
None,
)))
Expand Down
6 changes: 3 additions & 3 deletions src/handlers/pr_tracking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,18 +59,18 @@ pub(super) async fn handle_input<'a>(
};

// ensure the team member object of this action exists in the `users` table
record_username(&db_client, assignee.id.unwrap(), &assignee.login)
record_username(&db_client, assignee.id, &assignee.login)
.await
.context("failed to record username")?;

if matches!(event.action, IssuesAction::Unassigned { .. }) {
delete_pr_from_workqueue(&db_client, assignee.id.unwrap(), event.issue.number)
delete_pr_from_workqueue(&db_client, assignee.id, event.issue.number)
.await
.context("Failed to remove PR from workqueue")?;
}

if matches!(event.action, IssuesAction::Assigned { .. }) {
upsert_pr_into_workqueue(&db_client, assignee.id.unwrap(), event.issue.number)
upsert_pr_into_workqueue(&db_client, assignee.id, event.issue.number)
.await
.context("Failed to add PR to workqueue")?;
}
Expand Down
6 changes: 2 additions & 4 deletions src/handlers/pull_requests_assignment_update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,14 @@ impl Job for PullRequestAssignmentUpdate {

// aggregate by user first
let aggregated = prs.into_iter().fold(HashMap::new(), |mut acc, (user, pr)| {
let (_, prs) = acc
.entry(user.id.unwrap())
.or_insert_with(|| (user, Vec::new()));
let (_, prs) = acc.entry(user.id).or_insert_with(|| (user, Vec::new()));
prs.push(pr);
acc
});

// populate the table
for (_user_id, (assignee, prs)) in &aggregated {
let assignee_id = assignee.id.expect("checked");
let assignee_id = assignee.id;
let _ = record_username(&db, assignee_id, &assignee.login).await;
create_team_member_workqueue(&db, assignee_id, &prs).await?;
}
Expand Down
2 changes: 1 addition & 1 deletion src/handlers/rustc_commits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ pub async fn handle(ctx: &Context, event: &Event) -> anyhow::Result<()> {
return Ok(());
}

if event.comment.user.id != Some(BORS_GH_ID) {
if event.comment.user.id != BORS_GH_ID {
log::trace!("Ignoring non-bors comment, user: {:?}", event.comment.user);
return Ok(());
}
Expand Down
12 changes: 4 additions & 8 deletions src/zulip.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::db::notifications::add_metadata;
use crate::db::notifications::{self, delete_ping, move_indices, record_ping, Identifier};
use crate::github::{self, GithubClient};
use crate::github::{get_id_for_username, GithubClient};
use crate::handlers::docs_update::docs_update;
use crate::handlers::pull_requests_assignment_update::get_review_prefs;
use crate::handlers::Context;
Expand Down Expand Up @@ -241,13 +241,9 @@ async fn execute_for_other_user(
Some(username) => username,
None => anyhow::bail!("no username provided"),
};
let user_id = match (github::User {
login: username.to_owned(),
id: None,
})
.get_id(&ctx.github)
.await
.context("getting ID of github user")?
let user_id = match get_id_for_username(&ctx.github, username)
.await
.context("getting ID of github user")?
{
Some(id) => id.try_into().unwrap(),
None => anyhow::bail!("Can only authorize for other GitHub users."),
Expand Down
Loading