Merge pull request #1799 from apiraino/github-id-not-optional

Github user id is not optional

Author: jackh726

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<u64>,
+    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<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>(
+    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(
@@ -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,
                     ));

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,
                 &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(),
@@ -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,
         )))

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")?;
     }

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?;
         }

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(());
     }

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."),