Merge pull request #10586 from Turbo87/membership-options

github: Adjust `org/team_membership()` to return `Result<Option<_>, _>`

Author: Turbo87

crates/crates_io_github/src/lib.rs

@@ -33,13 +33,13 @@ pub trait GitHubClient: Send + Sync {
         team_id: i32,
         username: &str,
         auth: &AccessToken,
-    ) -> Result<GitHubTeamMembership>;
+    ) -> Result<Option<GitHubTeamMembership>>;
     async fn org_membership(
         &self,
         org_id: i32,
         username: &str,
         auth: &AccessToken,
-    ) -> Result<GitHubOrgMembership>;
+    ) -> Result<Option<GitHubOrgMembership>>;
     async fn public_keys(&self, username: &str, password: &str) -> Result<Vec<GitHubPublicKey>>;
 }
 
@@ -120,22 +120,28 @@ impl GitHubClient for RealGitHubClient {
         team_id: i32,
         username: &str,
         auth: &AccessToken,
-    ) -> Result<GitHubTeamMembership> {
+    ) -> Result<Option<GitHubTeamMembership>> {
         let url = format!("/organizations/{org_id}/team/{team_id}/memberships/{username}");
-        self.request(&url, auth).await
+        match self.request(&url, auth).await {
+            Ok(membership) => Ok(Some(membership)),
+            // Officially how `false` is returned
+            Err(GitHubError::NotFound(_)) => Ok(None),
+            Err(err) => Err(err),
+        }
     }
 
     async fn org_membership(
         &self,
         org_id: i32,
         username: &str,
         auth: &AccessToken,
-    ) -> Result<GitHubOrgMembership> {
-        self.request(
-            &format!("/organizations/{org_id}/memberships/{username}"),
-            auth,
-        )
-        .await
+    ) -> Result<Option<GitHubOrgMembership>> {
+        let url = format!("/organizations/{org_id}/memberships/{username}");
+        match self.request(&url, auth).await {
+            Ok(membership) => Ok(Some(membership)),
+            Err(GitHubError::NotFound(_)) => Ok(None),
+            Err(err) => Err(err),
+        }
     }
 
     /// Returns the list of public keys that can be used to verify GitHub secret alert signatures

src/models/team.rs

@@ -213,14 +213,11 @@ async fn is_gh_org_owner(
     user: &User,
 ) -> Result<bool, GitHubError> {
     let token = AccessToken::new(user.gh_access_token.expose_secret().to_string());
-    match gh_client
+    let membership = gh_client
         .org_membership(org_id, &user.gh_login, &token)
-        .await
-    {
-        Ok(membership) => Ok(membership.state == "active" && membership.role == "admin"),
-        Err(GitHubError::NotFound(_)) => Ok(false),
-        Err(e) => Err(e),
-    }
+        .await?;
+
+    Ok(membership.is_some_and(|m| m.state == "active" && m.role == "admin"))
 }
 
 async fn team_with_gh_id_contains_user(
@@ -233,16 +230,11 @@ async fn team_with_gh_id_contains_user(
     // check that "state": "active"
 
     let token = AccessToken::new(user.gh_access_token.expose_secret().to_string());
-    let membership = match gh_client
+    let membership = gh_client
         .team_membership(github_org_id, github_team_id, &user.gh_login, &token)
-        .await
-    {
-        // Officially how `false` is returned
-        Err(GitHubError::NotFound(_)) => return Ok(false),
-        x => x?,
-    };
+        .await?;
 
     // There is also `state: pending` for which we could possibly give
     // some feedback, but it's not obvious how that should work.
-    Ok(membership.state == "active")
+    Ok(membership.is_some_and(|m| m.state == "active"))
 }

src/tests/issues/issue1205.rs

@@ -74,7 +74,7 @@ fn github_mock() -> MockGitHubClient {
     github_mock
         .expect_team_membership()
         .with(eq(1), eq(2), eq("foo"), always())
-        .returning(|_, _, _, _| Ok(active_membership()));
+        .returning(|_, _, _, _| Ok(Some(active_membership())));
 
     github_mock
 }

src/tests/routes/crates/owners/remove.rs

@@ -138,9 +138,9 @@ async fn test_remove_uppercase_team() {
         .expect_team_membership()
         .with(eq(1), eq(2), eq("foo"), always())
         .returning(|_, _, _, _| {
-            Ok(GitHubTeamMembership {
+            Ok(Some(GitHubTeamMembership {
                 state: "active".to_string(),
-            })
+            }))
         });
 
     let (app, _, cookie) = TestApp::full().with_github(github_mock).with_user().await;

src/tests/util/github.rs

@@ -120,7 +120,7 @@ impl MockData {
         org_id: i32,
         team_id: i32,
         username: &str,
-    ) -> Result<GitHubTeamMembership, GitHubError> {
+    ) -> Result<Option<GitHubTeamMembership>, GitHubError> {
         let team = self
             .orgs
             .iter()
@@ -131,40 +131,40 @@ impl MockData {
             .find(|team| team.id == team_id)
             .ok_or_else(not_found)?;
         if team.members.contains(&username) {
-            Ok(GitHubTeamMembership {
+            Ok(Some(GitHubTeamMembership {
                 state: "active".into(),
-            })
+            }))
         } else {
-            Err(not_found())
+            Ok(None)
         }
     }
 
     fn org_membership(
         &self,
         org_id: i32,
         username: &str,
-    ) -> Result<GitHubOrgMembership, GitHubError> {
+    ) -> Result<Option<GitHubOrgMembership>, GitHubError> {
         let org = self
             .orgs
             .iter()
             .find(|org| org.id == org_id)
             .ok_or_else(not_found)?;
         if org.owners.contains(&username) {
-            Ok(GitHubOrgMembership {
+            Ok(Some(GitHubOrgMembership {
                 state: "active".into(),
                 role: "admin".into(),
-            })
+            }))
         } else if org
             .teams
             .iter()
             .any(|team| team.members.contains(&username))
         {
-            Ok(GitHubOrgMembership {
+            Ok(Some(GitHubOrgMembership {
                 state: "active".into(),
                 role: "member".into(),
-            })
+            }))
         } else {
-            Err(not_found())
+            Ok(None)
         }
     }
 }