From 5894f5d2e1effb7eae732e1af11ff205d5aa34f8 Mon Sep 17 00:00:00 2001 From: Marshall Bowers Date: Mon, 4 May 2026 13:51:56 +0200 Subject: [PATCH] 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 | 33 ++----- crates/collab/src/db/tables/channel_member.rs | 16 ++++ crates/collab/src/db/tables/user.rs | 15 ++++ crates/collab/src/rpc.rs | 11 ++- .../integration/db_tests/channel_tests.rs | 87 +++++++++++-------- 5 files changed, 100 insertions(+), 62 deletions(-) diff --git a/crates/collab/src/db/queries/channels.rs b/crates/collab/src/db/queries/channels.rs index 7262a5fa40..7b435ba1aa 100644 --- a/crates/collab/src/db/queries/channels.rs +++ b/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, Vec)> { + ) -> Result<(Vec, Vec)> { 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 = Vec::with_capacity(members.len()); + let mut users: Vec = 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(); diff --git a/crates/collab/src/db/tables/channel_member.rs b/crates/collab/src/db/tables/channel_member.rs index a00a380d55..7116b92ab6 100644 --- a/crates/collab/src/db/tables/channel_member.rs +++ b/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 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)] diff --git a/crates/collab/src/db/tables/user.rs b/crates/collab/src/db/tables/user.rs index 68044bc442..933e78ed42 100644 --- a/crates/collab/src/db/tables/user.rs +++ b/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 for crate::entities::User { } } +impl From 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")] diff --git a/crates/collab/src/rpc.rs b/crates/collab/src/rpc.rs index 4c38887b54..39f442bcaf 100644 --- a/crates/collab/src/rpc.rs +++ b/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(()) } diff --git a/crates/collab/tests/integration/db_tests/channel_tests.rs b/crates/collab/tests/integration/db_tests/channel_tests.rs index c78fe0f4ef..473225fc76 100644 --- a/crates/collab/tests/integration/db_tests/channel_tests.rs +++ b/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) { .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::>(); + let ids = members.into_iter().map(|m| m.user_id).collect::>(); 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) { 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) { .into_iter() .map(|channel| channel.id) .collect::>(); - 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) { .into_iter() .map(|channel| channel.id) .collect::>(); - 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::>(); members.sort_by_key(|member| member.user_id); assert_eq!( members, @@ -219,19 +221,24 @@ async fn test_channel_invites(db: &Arc) { ] ); - 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::>(); assert_eq!( members, &[ @@ -727,13 +734,16 @@ async fn test_user_is_channel_participant(db: &Arc) { .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::>(); members.sort_by_key(|member| member.user_id); - assert_eq!( members, &[ @@ -803,13 +813,16 @@ async fn test_user_is_channel_participant(db: &Arc) { .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::>(); members.sort_by_key(|member| member.user_id); - assert_eq!( members, &[ @@ -840,13 +853,16 @@ async fn test_user_is_channel_participant(db: &Arc) { .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::>(); members.sort_by_key(|member| member.user_id); - assert_eq!( members, &[ @@ -910,13 +926,16 @@ async fn test_user_is_channel_participant(db: &Arc) { .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::>(); members.sort_by_key(|member| member.user_id); - assert_eq!( members, &[