From 58f87c0b15ef86ca04cc9a95715b57d18463a828 Mon Sep 17 00:00:00 2001 From: apiraino Date: Mon, 22 Apr 2024 15:46:38 +0200 Subject: [PATCH] Github user id is not optional This patch removes the Optional id in the GitHub User struct. Unsure why it was this way, but I think it is reasonable to expect that any GitHub User has an id. --- src/github.rs | 27 ++++++++++-------- src/handlers/notification.rs | 28 ++++++++----------- src/handlers/pr_tracking.rs | 6 ++-- .../pull_requests_assignment_update.rs | 6 ++-- src/handlers/rustc_commits.rs | 2 +- src/zulip.rs | 12 +++----- 6 files changed, 37 insertions(+), 44 deletions(-) diff --git a/src/github.rs b/src/github.rs index 0a81a263..1c9d0db3 100644 --- a/src/github.rs +++ b/src/github.rs @@ -18,7 +18,7 @@ use tracing as log; #[derive(Debug, PartialEq, Eq, serde::Deserialize)] pub struct User { pub login: String, - pub id: Option, + pub id: u64, } impl GithubClient { @@ -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> { - 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>( + client: &'a GithubClient, + login: &str, +) -> anyhow::Result> { + 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( @@ -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, )); diff --git a/src/handlers/notification.rs b/src/handlers/notification.rs index 37c25a67..87fe2053 100644 --- a/src/handlers/notification.rs +++ b/src/handlers/notification.rs @@ -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, @@ -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)); } } }; @@ -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()); } @@ -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") { @@ -101,7 +102,7 @@ pub async fn handle(ctx: &Context, event: &Event) -> anyhow::Result<()> { if let Err(err) = notifications::record_ping( &client, ¬ifications::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(), @@ -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::>(), 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, ))) diff --git a/src/handlers/pr_tracking.rs b/src/handlers/pr_tracking.rs index 2e30ab36..980067d5 100644 --- a/src/handlers/pr_tracking.rs +++ b/src/handlers/pr_tracking.rs @@ -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")?; } diff --git a/src/handlers/pull_requests_assignment_update.rs b/src/handlers/pull_requests_assignment_update.rs index 41ee7335..fe544afe 100644 --- a/src/handlers/pull_requests_assignment_update.rs +++ b/src/handlers/pull_requests_assignment_update.rs @@ -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?; } diff --git a/src/handlers/rustc_commits.rs b/src/handlers/rustc_commits.rs index ff10ad0a..b9f5ec15 100644 --- a/src/handlers/rustc_commits.rs +++ b/src/handlers/rustc_commits.rs @@ -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(()); } diff --git a/src/zulip.rs b/src/zulip.rs index 65bcf198..5c86aeea 100644 --- a/src/zulip.rs +++ b/src/zulip.rs @@ -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; @@ -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."),