collab: Stop mixing concerns in get_channel_participant_details (#55568)

This PR updates the `get_channel_participant_details` method to reduce
the mixing of concerns within the method.

The authorization check for the caller has been moved to the outside,
along with the conversions from the database representations to the RPC
proto representations.

Now the method is just responsible for dealing with the data fetching,
which will make it easier to swap out.

Release Notes:

- N/A

crates/collab/src/db/queries/channels.rs

@@ -2,7 +2,7 @@ use super::*;
 use anyhow::Context as _;
 use rpc::{
     ErrorCode, ErrorCodeExt,
-    proto::{ChannelBufferVersion, VectorClockEntry, channel_member::Kind},
+    proto::{ChannelBufferVersion, VectorClockEntry},
 };
 use sea_orm::{ActiveValue, DbBackend, TryGetableMany};
 
@@ -687,16 +687,12 @@ impl Database {
     /// Returns the details for the specified channel member.
     pub async fn get_channel_participant_details(
         &self,
-        channel_id: ChannelId,
+        channel: &Channel,
         filter: &str,
         limit: u64,
-        user_id: UserId,
-    ) -> Result<(Vec<proto::ChannelMember>, Vec<proto::User>)> {
+    ) -> Result<(Vec<channel_member::Model>, Vec<user::Model>)> {
         let members = self
             .transaction(move |tx| async move {
-                let channel = self.get_channel_internal(channel_id, &tx).await?;
-                self.check_user_is_channel_participant(&channel, user_id, &tx)
-                    .await?;
                 let mut query = channel_member::Entity::find()
                     .find_also_related(user::Entity)
                     .filter(channel_member::Column::ChannelId.eq(channel.root_id()));
@@ -726,32 +722,15 @@ impl Database {
             })
             .await?;
 
-        let mut users: Vec<proto::User> = Vec::with_capacity(members.len());
+        let mut users: Vec<user::Model> = Vec::with_capacity(members.len());
 
         let members = members
             .into_iter()
             .map(|(member, user)| {
                 if let Some(user) = user {
-                    users.push(proto::User {
-                        id: user.id.to_proto(),
-                        avatar_url: format!(
-                            "https://avatars.githubusercontent.com/u/{}?s=128&v=4",
-                            user.github_user_id
-                        ),
-                        github_login: user.github_login,
-                        name: user.name,
-                    })
-                }
-                proto::ChannelMember {
-                    role: member.role.into(),
-                    user_id: member.user_id.to_proto(),
-                    kind: if member.accepted {
-                        Kind::Member
-                    } else {
-                        Kind::Invitee
-                    }
-                    .into(),
+                    users.push(user)
                 }
+                member
             })
             .collect();
 

crates/collab/src/db/tables/channel_member.rs

@@ -1,4 +1,5 @@
 use crate::db::{ChannelId, ChannelMemberId, ChannelRole, UserId, channel_member};
+use rpc::proto;
 use sea_orm::entity::prelude::*;
 
 #[derive(Clone, Debug, PartialEq, Eq, DeriveEntityModel)]
@@ -12,6 +13,21 @@ pub struct Model {
     pub role: ChannelRole,
 }
 
+impl From<Model> for proto::ChannelMember {
+    fn from(member: Model) -> Self {
+        Self {
+            role: member.role.into(),
+            user_id: member.user_id.to_proto(),
+            kind: if member.accepted {
+                proto::channel_member::Kind::Member
+            } else {
+                proto::channel_member::Kind::Invitee
+            }
+            .into(),
+        }
+    }
+}
+
 impl ActiveModelBehavior for ActiveModel {}
 
 #[derive(Copy, Clone, Debug, EnumIter, DeriveRelation)]

crates/collab/src/db/tables/user.rs

@@ -1,5 +1,6 @@
 use crate::db::UserId;
 use chrono::NaiveDateTime;
+use rpc::proto;
 use sea_orm::entity::prelude::*;
 use serde::Serialize;
 
@@ -30,6 +31,20 @@ impl From<Model> for crate::entities::User {
     }
 }
 
+impl From<Model> for proto::User {
+    fn from(user: Model) -> Self {
+        Self {
+            id: user.id.to_proto(),
+            avatar_url: format!(
+                "https://avatars.githubusercontent.com/u/{}?s=128&v=4",
+                user.github_user_id
+            ),
+            github_login: user.github_login,
+            name: user.name,
+        }
+    }
+}
+
 #[derive(Copy, Clone, Debug, EnumIter, DeriveRelation)]
 pub enum Relation {
     #[sea_orm(has_one = "super::room_participant::Entity")]

crates/collab/src/rpc.rs

@@ -3160,9 +3160,18 @@ async fn get_channel_members(
     } else {
         request.limit
     };
+
+    let channel = db.get_channel(channel_id, session.user_id()).await?;
+
     let (members, users) = db
-        .get_channel_participant_details(channel_id, &request.query, limit, session.user_id())
+        .get_channel_participant_details(&channel, &request.query, limit)
         .await?;
+    let members = members
+        .into_iter()
+        .map(proto::ChannelMember::from)
+        .collect();
+    let users = users.into_iter().map(proto::User::from).collect();
+
     response.send(proto::GetChannelMembersResponse { members, users })?;
     Ok(())
 }

crates/collab/tests/integration/db_tests/channel_tests.rs

@@ -1,6 +1,6 @@
 use super::{assert_channel_tree_matches, channel_tree, new_test_user};
 use crate::test_both_dbs;
-use collab::db::{Channel, ChannelId, ChannelRole, Database, NewUserParams, RoomId, UserId};
+use collab::db::{Channel, ChannelId, ChannelRole, Database, NewUserParams, RoomId};
 use rpc::{
     ConnectionId,
     proto::{self, reorder_channel},
@@ -36,14 +36,12 @@ async fn test_channels(db: &Arc<Database>) {
         .await
         .unwrap();
 
+    let replace_channel = db.get_channel(replace_id, a_id).await.unwrap();
     let (members, _) = db
-        .get_channel_participant_details(replace_id, "", 10, a_id)
+        .get_channel_participant_details(&replace_channel, "", 10)
         .await
         .unwrap();
-    let ids = members
-        .into_iter()
-        .map(|m| UserId::from_proto(m.user_id))
-        .collect::<Vec<_>>();
+    let ids = members.into_iter().map(|m| m.user_id).collect::<Vec<_>>();
     assert_eq!(ids, &[a_id, b_id]);
 
     let rust_id = db.create_root_channel("rust", a_id).await.unwrap();
@@ -158,17 +156,17 @@ async fn test_channel_invites(db: &Arc<Database>) {
     let user_2 = new_test_user(db, "user2@example.com").await;
     let user_3 = new_test_user(db, "user3@example.com").await;
 
-    let channel_1_1 = db.create_root_channel("channel_1", user_1).await.unwrap();
+    let channel_1_1_id = db.create_root_channel("channel_1", user_1).await.unwrap();
 
     let channel_1_2 = db.create_root_channel("channel_2", user_1).await.unwrap();
 
-    db.invite_channel_member(channel_1_1, user_2, user_1, ChannelRole::Member)
+    db.invite_channel_member(channel_1_1_id, user_2, user_1, ChannelRole::Member)
         .await
         .unwrap();
     db.invite_channel_member(channel_1_2, user_2, user_1, ChannelRole::Member)
         .await
         .unwrap();
-    db.invite_channel_member(channel_1_1, user_3, user_1, ChannelRole::Admin)
+    db.invite_channel_member(channel_1_1_id, user_3, user_1, ChannelRole::Admin)
         .await
         .unwrap();
 
@@ -180,7 +178,7 @@ async fn test_channel_invites(db: &Arc<Database>) {
         .into_iter()
         .map(|channel| channel.id)
         .collect::<Vec<_>>();
-    assert_eq!(user_2_invites, &[channel_1_1, channel_1_2]);
+    assert_eq!(user_2_invites, &[channel_1_1_id, channel_1_2]);
 
     let user_3_invites = db
         .get_channels_for_user(user_3)
@@ -190,13 +188,17 @@ async fn test_channel_invites(db: &Arc<Database>) {
         .into_iter()
         .map(|channel| channel.id)
         .collect::<Vec<_>>();
-    assert_eq!(user_3_invites, &[channel_1_1]);
+    assert_eq!(user_3_invites, &[channel_1_1_id]);
 
-    let (mut members, _) = db
-        .get_channel_participant_details(channel_1_1, "", 100, user_1)
+    let channel_1_1 = db.get_channel(channel_1_1_id, user_1).await.unwrap();
+    let (members, _) = db
+        .get_channel_participant_details(&channel_1_1, "", 100)
         .await
         .unwrap();
-
+    let mut members = members
+        .into_iter()
+        .map(proto::ChannelMember::from)
+        .collect::<Vec<_>>();
     members.sort_by_key(|member| member.user_id);
     assert_eq!(
         members,
@@ -219,19 +221,24 @@ async fn test_channel_invites(db: &Arc<Database>) {
         ]
     );
 
-    db.respond_to_channel_invite(channel_1_1, user_2, true)
+    db.respond_to_channel_invite(channel_1_1_id, user_2, true)
         .await
         .unwrap();
 
-    let channel_1_3 = db
-        .create_sub_channel("channel_3", channel_1_1, user_1)
+    let channel_1_3_id = db
+        .create_sub_channel("channel_3", channel_1_1_id, user_1)
         .await
         .unwrap();
 
+    let channel_1_3 = db.get_channel(channel_1_3_id, user_1).await.unwrap();
     let (members, _) = db
-        .get_channel_participant_details(channel_1_3, "", 100, user_1)
+        .get_channel_participant_details(&channel_1_3, "", 100)
         .await
         .unwrap();
+    let members = members
+        .into_iter()
+        .map(proto::ChannelMember::from)
+        .collect::<Vec<_>>();
     assert_eq!(
         members,
         &[
@@ -727,13 +734,16 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
     .await
     .unwrap();
 
-    let (mut members, _) = db
-        .get_channel_participant_details(public_channel_id, "", 100, admin)
+    let public_channel = db.get_channel(public_channel_id, admin).await.unwrap();
+    let (members, _) = db
+        .get_channel_participant_details(&public_channel, "", 100)
         .await
         .unwrap();
-
+    let mut members = members
+        .into_iter()
+        .map(proto::ChannelMember::from)
+        .collect::<Vec<_>>();
     members.sort_by_key(|member| member.user_id);
-
     assert_eq!(
         members,
         &[
@@ -803,13 +813,16 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
         .is_err()
     );
 
-    let (mut members, _) = db
-        .get_channel_participant_details(public_channel_id, "", 100, admin)
+    let public_channel = db.get_channel(public_channel_id, admin).await.unwrap();
+    let (members, _) = db
+        .get_channel_participant_details(&public_channel, "", 100)
         .await
         .unwrap();
-
+    let mut members = members
+        .into_iter()
+        .map(proto::ChannelMember::from)
+        .collect::<Vec<_>>();
     members.sort_by_key(|member| member.user_id);
-
     assert_eq!(
         members,
         &[
@@ -840,13 +853,16 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
         .unwrap();
 
     // currently people invited to parent channels are not shown here
-    let (mut members, _) = db
-        .get_channel_participant_details(public_channel_id, "", 100, admin)
+    let public_channel = db.get_channel(public_channel_id, admin).await.unwrap();
+    let (members, _) = db
+        .get_channel_participant_details(&public_channel, "", 100)
         .await
         .unwrap();
-
+    let mut members = members
+        .into_iter()
+        .map(proto::ChannelMember::from)
+        .collect::<Vec<_>>();
     members.sort_by_key(|member| member.user_id);
-
     assert_eq!(
         members,
         &[
@@ -910,13 +926,16 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
     .await
     .unwrap();
 
-    let (mut members, _) = db
-        .get_channel_participant_details(public_channel_id, "", 100, admin)
+    let public_channel = db.get_channel(public_channel_id, admin).await.unwrap();
+    let (members, _) = db
+        .get_channel_participant_details(&public_channel, "", 100)
         .await
         .unwrap();
-
+    let mut members = members
+        .into_iter()
+        .map(proto::ChannelMember::from)
+        .collect::<Vec<_>>();
     members.sort_by_key(|member| member.user_id);
-
     assert_eq!(
         members,
         &[