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

We can handle the `NotFound` case inside the lib, to make it easier for users of the fn.

Author: Turbo87

crates/crates_io_github/src/lib.rs

@@ -33,7 +33,7 @@ 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,
@@ -120,9 +120,14 @@ 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(

src/models/team.rs

@@ -233,16 +233,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,11 +131,11 @@ 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)
         }
     }