From 4b672621d3687f260ac5950106a6ad3c2934e47c Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Thu, 25 Jan 2024 16:43:18 -0700 Subject: [PATCH] Aggressively simplify channel permissions: - Only allow setting permissions on the root channel - Only allow public channels to be children of public channels --- crates/channel/src/channel_buffer.rs | 5 +- crates/channel/src/channel_store.rs | 58 +- .../src/channel_store/channel_index.rs | 7 +- crates/channel/src/channel_store_tests.rs | 42 +- crates/collab/src/db.rs | 37 +- crates/collab/src/db/ids.rs | 9 + crates/collab/src/db/queries/channels.rs | 622 +++++------------- crates/collab/src/db/tables/channel.rs | 8 + crates/collab/src/db/tests.rs | 5 +- crates/collab/src/db/tests/channel_tests.rs | 231 +++---- crates/collab/src/db/tests/message_tests.rs | 9 +- crates/collab/src/rpc.rs | 175 ++--- .../collab/src/tests/channel_guest_tests.rs | 7 + crates/collab/src/tests/channel_tests.rs | 161 +---- crates/collab/src/tests/test_server.rs | 1 + crates/collab_ui/src/collab_panel.rs | 43 +- .../src/collab_panel/channel_modal.rs | 16 +- crates/rpc/proto/zed.proto | 11 +- 18 files changed, 477 insertions(+), 970 deletions(-) diff --git a/crates/channel/src/channel_buffer.rs b/crates/channel/src/channel_buffer.rs index b5f4a06b97..e87954edf6 100644 --- a/crates/channel/src/channel_buffer.rs +++ b/crates/channel/src/channel_buffer.rs @@ -61,11 +61,12 @@ impl ChannelBuffer { .map(language::proto::deserialize_operation) .collect::, _>>()?; - let buffer = cx.new_model(|_| { + let buffer = cx.new_model(|cx| { + let capability = channel_store.read(cx).channel_capability(channel.id); language::Buffer::remote( response.buffer_id, response.replica_id as u16, - channel.channel_buffer_capability(), + capability, base_text, ) })?; diff --git a/crates/channel/src/channel_store.rs b/crates/channel/src/channel_store.rs index 831e7de431..62363bf91a 100644 --- a/crates/channel/src/channel_store.rs +++ b/crates/channel/src/channel_store.rs @@ -13,11 +13,11 @@ use gpui::{ }; use language::Capability; use rpc::{ - proto::{self, ChannelVisibility}, + proto::{self, ChannelRole, ChannelVisibility}, TypedEnvelope, }; use std::{mem, sync::Arc, time::Duration}; -use util::{async_maybe, ResultExt}; +use util::{async_maybe, maybe, ResultExt}; pub fn init(client: &Arc, user_store: Model, cx: &mut AppContext) { let channel_store = @@ -58,7 +58,6 @@ pub struct Channel { pub id: ChannelId, pub name: SharedString, pub visibility: proto::ChannelVisibility, - pub role: proto::ChannelRole, pub parent_path: Vec, } @@ -68,6 +67,7 @@ pub struct ChannelState { latest_notes_versions: Option, observed_chat_message: Option, observed_notes_versions: Option, + role: Option, } impl Channel { @@ -90,11 +90,12 @@ impl Channel { } pub fn channel_buffer_capability(&self) -> Capability { - if self.role == proto::ChannelRole::Member || self.role == proto::ChannelRole::Admin { - Capability::ReadWrite - } else { - Capability::ReadOnly - } + todo!() // go ask the channel store + // if self.role == proto::ChannelRole::Member || self.role == proto::ChannelRole::Admin { + // Capability::ReadWrite + // } else { + // Capability::ReadOnly + // } } } @@ -114,8 +115,7 @@ impl ChannelMembership { }, kind_order: match self.kind { proto::channel_member::Kind::Member => 0, - proto::channel_member::Kind::AncestorMember => 1, - proto::channel_member::Kind::Invitee => 2, + proto::channel_member::Kind::Invitee => 1, }, username_order: self.user.github_login.as_str(), } @@ -479,10 +479,25 @@ impl ChannelStore { } pub fn is_channel_admin(&self, channel_id: ChannelId) -> bool { - let Some(channel) = self.channel_for_id(channel_id) else { - return false; - }; - channel.role == proto::ChannelRole::Admin + self.channel_role(channel_id) == proto::ChannelRole::Admin + } + + pub fn channel_capability(&self, channel_id: ChannelId) -> Capability { + match self.channel_role(channel_id) { + ChannelRole::Admin | ChannelRole::Member => Capability::ReadWrite, + _ => Capability::ReadOnly, + } + } + + pub fn channel_role(&self, channel_id: ChannelId) -> proto::ChannelRole { + maybe!({ + let channel = self.channel_for_id(channel_id)?; + let root_channel_id = channel.parent_path.first()?; + let root_channel_state = self.channel_states.get(&root_channel_id); + debug_assert!(root_channel_state.is_some()); + root_channel_state?.role + }) + .unwrap_or(proto::ChannelRole::Guest) } pub fn channel_participants(&self, channel_id: ChannelId) -> &[Arc] { @@ -533,7 +548,7 @@ impl ChannelStore { pub fn move_channel( &mut self, channel_id: ChannelId, - to: Option, + to: ChannelId, cx: &mut ModelContext, ) -> Task> { let client = self.client.clone(); @@ -791,6 +806,14 @@ impl ChannelStore { for message_id in message.payload.observed_channel_message_id { this.acknowledge_message_id(message_id.channel_id, message_id.message_id, cx); } + for membership in message.payload.channel_memberships { + if let Some(role) = ChannelRole::from_i32(membership.role) { + this.channel_states + .entry(membership.channel_id) + .or_insert_with(|| ChannelState::default()) + .set_role(role) + } + } }) } @@ -956,7 +979,6 @@ impl ChannelStore { Arc::new(Channel { id: channel.id, visibility: channel.visibility(), - role: channel.role(), name: channel.name.into(), parent_path: channel.parent_path, }), @@ -1071,6 +1093,10 @@ impl ChannelStore { } impl ChannelState { + fn set_role(&mut self, role: ChannelRole) { + self.role = Some(role); + } + fn has_channel_buffer_changed(&self) -> bool { if let Some(latest_version) = &self.latest_notes_versions { if let Some(observed_version) = &self.observed_notes_versions { diff --git a/crates/channel/src/channel_store/channel_index.rs b/crates/channel/src/channel_store/channel_index.rs index ca2eb3345e..e70b3e4c46 100644 --- a/crates/channel/src/channel_store/channel_index.rs +++ b/crates/channel/src/channel_store/channel_index.rs @@ -54,19 +54,18 @@ impl<'a> ChannelPathsInsertGuard<'a> { let existing_channel = Arc::make_mut(existing_channel); ret = existing_channel.visibility != channel_proto.visibility() - || existing_channel.role != channel_proto.role() - || existing_channel.name != channel_proto.name; + || existing_channel.name != channel_proto.name + || existing_channel.parent_path != channel_proto.parent_path; existing_channel.visibility = channel_proto.visibility(); - existing_channel.role = channel_proto.role(); existing_channel.name = channel_proto.name.into(); + existing_channel.parent_path = channel_proto.parent_path.into(); } else { self.channels_by_id.insert( channel_proto.id, Arc::new(Channel { id: channel_proto.id, visibility: channel_proto.visibility(), - role: channel_proto.role(), name: channel_proto.name.into(), parent_path: channel_proto.parent_path, }), diff --git a/crates/channel/src/channel_store_tests.rs b/crates/channel/src/channel_store_tests.rs index 0b07918acf..ad2098766d 100644 --- a/crates/channel/src/channel_store_tests.rs +++ b/crates/channel/src/channel_store_tests.rs @@ -19,14 +19,12 @@ fn test_update_channels(cx: &mut AppContext) { id: 1, name: "b".to_string(), visibility: proto::ChannelVisibility::Members as i32, - role: proto::ChannelRole::Admin.into(), parent_path: Vec::new(), }, proto::Channel { id: 2, name: "a".to_string(), visibility: proto::ChannelVisibility::Members as i32, - role: proto::ChannelRole::Member.into(), parent_path: Vec::new(), }, ], @@ -38,8 +36,8 @@ fn test_update_channels(cx: &mut AppContext) { &channel_store, &[ // - (0, "a".to_string(), proto::ChannelRole::Member), - (0, "b".to_string(), proto::ChannelRole::Admin), + (0, "a".to_string()), + (0, "b".to_string()), ], cx, ); @@ -52,14 +50,12 @@ fn test_update_channels(cx: &mut AppContext) { id: 3, name: "x".to_string(), visibility: proto::ChannelVisibility::Members as i32, - role: proto::ChannelRole::Admin.into(), parent_path: vec![1], }, proto::Channel { id: 4, name: "y".to_string(), visibility: proto::ChannelVisibility::Members as i32, - role: proto::ChannelRole::Member.into(), parent_path: vec![2], }, ], @@ -70,10 +66,10 @@ fn test_update_channels(cx: &mut AppContext) { assert_channels( &channel_store, &[ - (0, "a".to_string(), proto::ChannelRole::Member), - (1, "y".to_string(), proto::ChannelRole::Member), - (0, "b".to_string(), proto::ChannelRole::Admin), - (1, "x".to_string(), proto::ChannelRole::Admin), + (0, "a".to_string()), + (1, "y".to_string()), + (0, "b".to_string()), + (1, "x".to_string()), ], cx, ); @@ -91,21 +87,18 @@ fn test_dangling_channel_paths(cx: &mut AppContext) { id: 0, name: "a".to_string(), visibility: proto::ChannelVisibility::Members as i32, - role: proto::ChannelRole::Admin.into(), parent_path: vec![], }, proto::Channel { id: 1, name: "b".to_string(), visibility: proto::ChannelVisibility::Members as i32, - role: proto::ChannelRole::Admin.into(), parent_path: vec![0], }, proto::Channel { id: 2, name: "c".to_string(), visibility: proto::ChannelVisibility::Members as i32, - role: proto::ChannelRole::Admin.into(), parent_path: vec![0, 1], }, ], @@ -118,9 +111,9 @@ fn test_dangling_channel_paths(cx: &mut AppContext) { &channel_store, &[ // - (0, "a".to_string(), proto::ChannelRole::Admin), - (1, "b".to_string(), proto::ChannelRole::Admin), - (2, "c".to_string(), proto::ChannelRole::Admin), + (0, "a".to_string()), + (1, "b".to_string()), + (2, "c".to_string()), ], cx, ); @@ -135,11 +128,7 @@ fn test_dangling_channel_paths(cx: &mut AppContext) { ); // Make sure that the 1/2/3 path is gone - assert_channels( - &channel_store, - &[(0, "a".to_string(), proto::ChannelRole::Admin)], - cx, - ); + assert_channels(&channel_store, &[(0, "a".to_string())], cx); } #[gpui::test] @@ -156,18 +145,13 @@ async fn test_channel_messages(cx: &mut TestAppContext) { id: channel_id, name: "the-channel".to_string(), visibility: proto::ChannelVisibility::Members as i32, - role: proto::ChannelRole::Member.into(), parent_path: vec![], }], ..Default::default() }); cx.executor().run_until_parked(); cx.update(|cx| { - assert_channels( - &channel_store, - &[(0, "the-channel".to_string(), proto::ChannelRole::Member)], - cx, - ); + assert_channels(&channel_store, &[(0, "the-channel".to_string())], cx); }); let get_users = server.receive::().await.unwrap(); @@ -368,13 +352,13 @@ fn update_channels( #[track_caller] fn assert_channels( channel_store: &Model, - expected_channels: &[(usize, String, proto::ChannelRole)], + expected_channels: &[(usize, String)], cx: &mut AppContext, ) { let actual = channel_store.update(cx, |store, _| { store .ordered_channels() - .map(|(depth, channel)| (depth, channel.name.to_string(), channel.role)) + .map(|(depth, channel)| (depth, channel.name.to_string())) .collect::>() }); assert_eq!(actual, expected_channels); diff --git a/crates/collab/src/db.rs b/crates/collab/src/db.rs index 918d1405a2..2e8540641b 100644 --- a/crates/collab/src/db.rs +++ b/crates/collab/src/db.rs @@ -40,7 +40,7 @@ use std::{ sync::Arc, time::Duration, }; -use tables::*; +pub use tables::*; use tokio::sync::{Mutex, OwnedMutexGuard}; pub use ids::*; @@ -502,35 +502,6 @@ pub struct NewUserResult { pub signup_device_id: Option, } -/// The result of moving a channel. -#[derive(Debug)] -pub struct MoveChannelResult { - pub previous_participants: Vec, - pub descendent_ids: Vec, -} - -/// The result of renaming a channel. -#[derive(Debug)] -pub struct RenameChannelResult { - pub channel: Channel, - pub participants_to_update: HashMap, -} - -/// The result of creating a channel. -#[derive(Debug)] -pub struct CreateChannelResult { - pub channel: Channel, - pub participants_to_update: Vec<(UserId, ChannelsForUser)>, -} - -/// The result of setting a channel's visibility. -#[derive(Debug)] -pub struct SetChannelVisibilityResult { - pub participants_to_update: HashMap, - pub participants_to_remove: HashSet, - pub channels_to_remove: Vec, -} - /// The result of updating a channel membership. #[derive(Debug)] pub struct MembershipUpdated { @@ -570,18 +541,16 @@ pub struct Channel { pub id: ChannelId, pub name: String, pub visibility: ChannelVisibility, - pub role: ChannelRole, /// parent_path is the channel ids from the root to this one (not including this one) pub parent_path: Vec, } impl Channel { - fn from_model(value: channel::Model, role: ChannelRole) -> Self { + fn from_model(value: channel::Model) -> Self { Channel { id: value.id, visibility: value.visibility, name: value.clone().name, - role, parent_path: value.ancestors().collect(), } } @@ -591,7 +560,6 @@ impl Channel { id: self.id.to_proto(), name: self.name.clone(), visibility: self.visibility.into(), - role: self.role.into(), parent_path: self.parent_path.iter().map(|c| c.to_proto()).collect(), } } @@ -617,6 +585,7 @@ impl ChannelMember { #[derive(Debug, PartialEq)] pub struct ChannelsForUser { pub channels: Vec, + pub channel_memberships: Vec, pub channel_participants: HashMap>, pub latest_buffer_versions: Vec, pub latest_channel_messages: Vec, diff --git a/crates/collab/src/db/ids.rs b/crates/collab/src/db/ids.rs index 9a6a1e78f3..d69e19643a 100644 --- a/crates/collab/src/db/ids.rs +++ b/crates/collab/src/db/ids.rs @@ -129,6 +129,15 @@ impl ChannelRole { } } + pub fn can_see_channel(&self, visibility: ChannelVisibility) -> bool { + use ChannelRole::*; + match self { + Admin | Member => true, + Guest => visibility == ChannelVisibility::Public, + Banned => false, + } + } + /// True if the role allows access to all descendant channels pub fn can_see_all_descendants(&self) -> bool { use ChannelRole::*; diff --git a/crates/collab/src/db/queries/channels.rs b/crates/collab/src/db/queries/channels.rs index f1b4c8af7b..b1df4e7cbe 100644 --- a/crates/collab/src/db/queries/channels.rs +++ b/crates/collab/src/db/queries/channels.rs @@ -19,7 +19,7 @@ impl Database { #[cfg(test)] pub async fn create_root_channel(&self, name: &str, creator_id: UserId) -> Result { - Ok(self.create_channel(name, None, creator_id).await?.id) + Ok(self.create_channel(name, None, creator_id).await?.0.id) } #[cfg(test)] @@ -32,6 +32,7 @@ impl Database { Ok(self .create_channel(name, Some(parent), creator_id) .await? + .0 .id) } @@ -41,10 +42,15 @@ impl Database { name: &str, parent_channel_id: Option, admin_id: UserId, - ) -> Result { + ) -> Result<( + Channel, + Option, + Vec, + )> { let name = Self::sanitize_channel_name(name)?; self.transaction(move |tx| async move { let mut parent = None; + let mut membership = None; if let Some(parent_channel_id) = parent_channel_id { let parent_channel = self.get_channel_internal(parent_channel_id, &*tx).await?; @@ -68,18 +74,25 @@ impl Database { .await?; if parent.is_none() { - channel_member::ActiveModel { - id: ActiveValue::NotSet, - channel_id: ActiveValue::Set(channel.id), - user_id: ActiveValue::Set(admin_id), - accepted: ActiveValue::Set(true), - role: ActiveValue::Set(ChannelRole::Admin), - } - .insert(&*tx) - .await?; + membership = Some( + channel_member::ActiveModel { + id: ActiveValue::NotSet, + channel_id: ActiveValue::Set(channel.id), + user_id: ActiveValue::Set(admin_id), + accepted: ActiveValue::Set(true), + role: ActiveValue::Set(ChannelRole::Admin), + } + .insert(&*tx) + .await?, + ); } - Ok(Channel::from_model(channel, ChannelRole::Admin)) + let channel_members = channel_member::Entity::find() + .filter(channel_member::Column::ChannelId.eq(channel.root_id())) + .all(&*tx) + .await?; + + Ok((Channel::from_model(channel), membership, channel_members)) }) .await } @@ -122,16 +135,9 @@ impl Database { ); } else if channel.visibility == ChannelVisibility::Public { role = Some(ChannelRole::Guest); - let channel_to_join = self - .public_ancestors_including_self(&channel, &*tx) - .await? - .first() - .cloned() - .unwrap_or(channel.clone()); - channel_member::Entity::insert(channel_member::ActiveModel { id: ActiveValue::NotSet, - channel_id: ActiveValue::Set(channel_to_join.id), + channel_id: ActiveValue::Set(channel.root_id()), user_id: ActiveValue::Set(user_id), accepted: ActiveValue::Set(true), role: ActiveValue::Set(ChannelRole::Guest), @@ -140,7 +146,7 @@ impl Database { .await?; accept_invite_result = Some( - self.calculate_membership_updated(&channel_to_join, user_id, &*tx) + self.calculate_membership_updated(&channel, user_id, &*tx) .await?, ); @@ -173,76 +179,43 @@ impl Database { channel_id: ChannelId, visibility: ChannelVisibility, admin_id: UserId, - ) -> Result { + ) -> Result<(Channel, Vec)> { self.transaction(move |tx| async move { let channel = self.get_channel_internal(channel_id, &*tx).await?; - self.check_user_is_channel_admin(&channel, admin_id, &*tx) .await?; - let previous_members = self - .get_channel_participant_details_internal(&channel, &*tx) - .await?; + if visibility == ChannelVisibility::Public { + if let Some(parent_id) = channel.parent_id() { + let parent = self.get_channel_internal(parent_id, &*tx).await?; + + if parent.visibility != ChannelVisibility::Public { + Err(anyhow!("public channels must descend from public channels"))?; + } + } + } else if visibility == ChannelVisibility::Members { + if self + .get_channel_descendants_including_self(vec![channel_id], &*tx) + .await? + .into_iter() + .any(|channel| { + channel.id != channel_id && channel.visibility == ChannelVisibility::Public + }) + { + Err(anyhow!("cannot make a parent of a public channel private"))?; + } + } let mut model = channel.into_active_model(); model.visibility = ActiveValue::Set(visibility); let channel = model.update(&*tx).await?; - let mut participants_to_update: HashMap = self - .participants_to_notify_for_channel_change(&channel, &*tx) - .await? - .into_iter() - .collect(); + let channel_members = channel_member::Entity::find() + .filter(channel_member::Column::ChannelId.eq(channel.root_id())) + .all(&*tx) + .await?; - let mut channels_to_remove: Vec = vec![]; - let mut participants_to_remove: HashSet = HashSet::default(); - match visibility { - ChannelVisibility::Members => { - let all_descendents: Vec = self - .get_channel_descendants_including_self(vec![channel_id], &*tx) - .await? - .into_iter() - .map(|channel| channel.id) - .collect(); - - channels_to_remove = channel::Entity::find() - .filter( - channel::Column::Id - .is_in(all_descendents) - .and(channel::Column::Visibility.eq(ChannelVisibility::Public)), - ) - .all(&*tx) - .await? - .into_iter() - .map(|channel| channel.id) - .collect(); - - channels_to_remove.push(channel_id); - - for member in previous_members { - if member.role.can_only_see_public_descendants() { - participants_to_remove.insert(member.user_id); - } - } - } - ChannelVisibility::Public => { - if let Some(public_parent) = self.public_parent_channel(&channel, &*tx).await? { - let parent_updates = self - .participants_to_notify_for_channel_change(&public_parent, &*tx) - .await?; - - for (user_id, channels) in parent_updates { - participants_to_update.insert(user_id, channels); - } - } - } - } - - Ok(SetChannelVisibilityResult { - participants_to_update, - participants_to_remove, - channels_to_remove, - }) + Ok((Channel::from_model(channel), channel_members)) }) .await } @@ -275,7 +248,7 @@ impl Database { .await?; let members_to_notify: Vec = channel_member::Entity::find() - .filter(channel_member::Column::ChannelId.is_in(channel.ancestors_including_self())) + .filter(channel_member::Column::ChannelId.eq(channel.root_id())) .select_only() .column(channel_member::Column::UserId) .distinct() @@ -312,6 +285,9 @@ impl Database { let channel = self.get_channel_internal(channel_id, &*tx).await?; self.check_user_is_channel_admin(&channel, inviter_id, &*tx) .await?; + if !channel.is_root() { + Err(ErrorCode::NotARootChannel.anyhow())? + } channel_member::ActiveModel { id: ActiveValue::NotSet, @@ -323,7 +299,7 @@ impl Database { .insert(&*tx) .await?; - let channel = Channel::from_model(channel, role); + let channel = Channel::from_model(channel); let notifications = self .create_notification( @@ -362,35 +338,24 @@ impl Database { channel_id: ChannelId, admin_id: UserId, new_name: &str, - ) -> Result { + ) -> Result<(Channel, Vec)> { self.transaction(move |tx| async move { let new_name = Self::sanitize_channel_name(new_name)?.to_string(); let channel = self.get_channel_internal(channel_id, &*tx).await?; - let role = self - .check_user_is_channel_admin(&channel, admin_id, &*tx) + self.check_user_is_channel_admin(&channel, admin_id, &*tx) .await?; let mut model = channel.into_active_model(); model.name = ActiveValue::Set(new_name.clone()); let channel = model.update(&*tx).await?; - let participants = self - .get_channel_participant_details_internal(&channel, &*tx) + let channel_members = channel_member::Entity::find() + .filter(channel_member::Column::ChannelId.eq(channel.root_id())) + .all(&*tx) .await?; - Ok(RenameChannelResult { - channel: Channel::from_model(channel.clone(), role), - participants_to_update: participants - .iter() - .map(|participant| { - ( - participant.user_id, - Channel::from_model(channel.clone(), participant.role), - ) - }) - .collect(), - }) + Ok((Channel::from_model(channel), channel_members)) }) .await } @@ -565,10 +530,7 @@ impl Database { let channels = channels .into_iter() - .filter_map(|channel| { - let role = *role_for_channel.get(&channel.id)?; - Some(Channel::from_model(channel, role)) - }) + .filter_map(|channel| Some(Channel::from_model(channel))) .collect(); Ok(channels) @@ -576,6 +538,26 @@ impl Database { .await } + pub async fn get_channel_memberships( + &self, + user_id: UserId, + ) -> Result<(Vec, Vec)> { + self.transaction(|tx| async move { + let memberships = channel_member::Entity::find() + .filter(channel_member::Column::UserId.eq(user_id)) + .all(&*tx) + .await?; + let channels = self + .get_channel_descendants_including_self( + memberships.iter().map(|m| m.channel_id), + &*tx, + ) + .await?; + Ok((memberships, channels)) + }) + .await + } + /// Returns all channels for the user with the given ID. pub async fn get_channels_for_user(&self, user_id: UserId) -> Result { self.transaction(|tx| async move { @@ -594,12 +576,16 @@ impl Database { ancestor_channel: Option<&channel::Model>, tx: &DatabaseTransaction, ) -> Result { + let mut filter = channel_member::Column::UserId + .eq(user_id) + .and(channel_member::Column::Accepted.eq(true)); + + if let Some(ancestor) = ancestor_channel { + filter = filter.and(channel_member::Column::ChannelId.eq(ancestor.root_id())); + } + let channel_memberships = channel_member::Entity::find() - .filter( - channel_member::Column::UserId - .eq(user_id) - .and(channel_member::Column::Accepted.eq(true)), - ) + .filter(filter) .all(&*tx) .await?; @@ -610,56 +596,20 @@ impl Database { ) .await?; - let mut roles_by_channel_id: HashMap = HashMap::default(); - for membership in channel_memberships.iter() { - roles_by_channel_id.insert(membership.channel_id, membership.role); - } - - let mut visible_channel_ids: HashSet = HashSet::default(); + let roles_by_channel_id = channel_memberships + .iter() + .map(|membership| (membership.channel_id, membership.role)) + .collect::>(); let channels: Vec = descendants .into_iter() .filter_map(|channel| { - let parent_role = channel - .parent_id() - .and_then(|parent_id| roles_by_channel_id.get(&parent_id)); - - let role = if let Some(parent_role) = parent_role { - let role = if let Some(existing_role) = roles_by_channel_id.get(&channel.id) { - existing_role.max(*parent_role) - } else { - *parent_role - }; - roles_by_channel_id.insert(channel.id, role); - role + let parent_role = roles_by_channel_id.get(&channel.root_id())?; + if parent_role.can_see_channel(channel.visibility) { + Some(Channel::from_model(channel)) } else { - *roles_by_channel_id.get(&channel.id)? - }; - - let can_see_parent_paths = role.can_see_all_descendants() - || role.can_only_see_public_descendants() - && channel.visibility == ChannelVisibility::Public; - if !can_see_parent_paths { - return None; + None } - - visible_channel_ids.insert(channel.id); - - if let Some(ancestor) = ancestor_channel { - if !channel - .ancestors_including_self() - .any(|id| id == ancestor.id) - { - return None; - } - } - - let mut channel = Channel::from_model(channel, role); - channel - .parent_path - .retain(|id| visible_channel_ids.contains(&id)); - - Some(channel) }) .collect(); @@ -694,6 +644,7 @@ impl Database { let latest_messages = self.latest_channel_messages(&channel_ids, &*tx).await?; Ok(ChannelsForUser { + channel_memberships, channels, channel_participants, latest_buffer_versions, @@ -701,73 +652,6 @@ impl Database { }) } - pub async fn new_participants_to_notify( - &self, - parent_channel_id: ChannelId, - ) -> Result> { - self.weak_transaction(|tx| async move { - let parent_channel = self.get_channel_internal(parent_channel_id, &*tx).await?; - self.participants_to_notify_for_channel_change(&parent_channel, &*tx) - .await - }) - .await - } - - // TODO: this is very expensive, and we should rethink - async fn participants_to_notify_for_channel_change( - &self, - new_parent: &channel::Model, - tx: &DatabaseTransaction, - ) -> Result> { - let mut results: Vec<(UserId, ChannelsForUser)> = Vec::new(); - - let members = self - .get_channel_participant_details_internal(new_parent, &*tx) - .await?; - - for member in members.iter() { - if !member.role.can_see_all_descendants() { - continue; - } - results.push(( - member.user_id, - self.get_user_channels(member.user_id, Some(new_parent), &*tx) - .await?, - )) - } - - let public_parents = self - .public_ancestors_including_self(new_parent, &*tx) - .await?; - let public_parent = public_parents.last(); - - let Some(public_parent) = public_parent else { - return Ok(results); - }; - - // could save some time in the common case by skipping this if the - // new channel is not public and has no public descendants. - let public_members = if public_parent == new_parent { - members - } else { - self.get_channel_participant_details_internal(public_parent, &*tx) - .await? - }; - - for member in public_members { - if !member.role.can_only_see_public_descendants() { - continue; - }; - results.push(( - member.user_id, - self.get_user_channels(member.user_id, Some(public_parent), &*tx) - .await?, - )) - } - - Ok(results) - } - /// Sets the role for the specified channel member. pub async fn set_channel_member_role( &self, @@ -805,7 +689,7 @@ impl Database { )) } else { Ok(SetMemberRoleResult::InviteUpdated(Channel::from_model( - channel, role, + channel, ))) } }) @@ -835,22 +719,30 @@ impl Database { if role == ChannelRole::Admin { Ok(members .into_iter() - .map(|channel_member| channel_member.to_proto()) + .map(|channel_member| proto::ChannelMember { + role: channel_member.role.into(), + user_id: channel_member.user_id.to_proto(), + kind: if channel_member.accepted { + Kind::Member + } else { + Kind::Invitee + } + .into(), + }) .collect()) } else { return Ok(members .into_iter() .filter_map(|member| { - if member.kind == proto::channel_member::Kind::Invitee { + if !member.accepted { return None; } - Some(ChannelMember { - role: member.role, - user_id: member.user_id, - kind: proto::channel_member::Kind::Member, + Some(proto::ChannelMember { + role: member.role.into(), + user_id: member.user_id.to_proto(), + kind: Kind::Member.into(), }) }) - .map(|channel_member| channel_member.to_proto()) .collect()); } } @@ -859,83 +751,11 @@ impl Database { &self, channel: &channel::Model, tx: &DatabaseTransaction, - ) -> Result> { - #[derive(Copy, Clone, Debug, EnumIter, DeriveColumn)] - enum QueryMemberDetails { - UserId, - Role, - IsDirectMember, - Accepted, - Visibility, - } - - let mut stream = channel_member::Entity::find() - .left_join(channel::Entity) - .filter(channel_member::Column::ChannelId.is_in(channel.ancestors_including_self())) - .select_only() - .column(channel_member::Column::UserId) - .column(channel_member::Column::Role) - .column_as( - channel_member::Column::ChannelId.eq(channel.id), - QueryMemberDetails::IsDirectMember, - ) - .column(channel_member::Column::Accepted) - .column(channel::Column::Visibility) - .into_values::<_, QueryMemberDetails>() - .stream(&*tx) - .await?; - - let mut user_details: HashMap = HashMap::default(); - - while let Some(user_membership) = stream.next().await { - let (user_id, channel_role, is_direct_member, is_invite_accepted, visibility): ( - UserId, - ChannelRole, - bool, - bool, - ChannelVisibility, - ) = user_membership?; - let kind = match (is_direct_member, is_invite_accepted) { - (true, true) => proto::channel_member::Kind::Member, - (true, false) => proto::channel_member::Kind::Invitee, - (false, true) => proto::channel_member::Kind::AncestorMember, - (false, false) => continue, - }; - - if channel_role == ChannelRole::Guest - && visibility != ChannelVisibility::Public - && channel.visibility != ChannelVisibility::Public - { - continue; - } - - if let Some(details_mut) = user_details.get_mut(&user_id) { - if channel_role.should_override(details_mut.role) { - details_mut.role = channel_role; - } - if kind == Kind::Member { - details_mut.kind = kind; - // the UI is going to be a bit confusing if you already have permissions - // that are greater than or equal to the ones you're being invited to. - } else if kind == Kind::Invitee && details_mut.kind == Kind::AncestorMember { - details_mut.kind = kind; - } - } else { - user_details.insert( - user_id, - ChannelMember { - user_id, - kind, - role: channel_role, - }, - ); - } - } - - Ok(user_details - .into_iter() - .map(|(_, details)| details) - .collect()) + ) -> Result> { + Ok(channel_member::Entity::find() + .filter(channel_member::Column::ChannelId.eq(channel.root_id())) + .all(tx) + .await?) } /// Returns the participants in the given channel. @@ -1014,7 +834,7 @@ impl Database { tx: &DatabaseTransaction, ) -> Result> { let row = channel_member::Entity::find() - .filter(channel_member::Column::ChannelId.is_in(channel.ancestors_including_self())) + .filter(channel_member::Column::ChannelId.eq(channel.root_id())) .filter(channel_member::Column::UserId.eq(user_id)) .filter(channel_member::Column::Accepted.eq(false)) .one(&*tx) @@ -1023,33 +843,6 @@ impl Database { Ok(row) } - async fn public_parent_channel( - &self, - channel: &channel::Model, - tx: &DatabaseTransaction, - ) -> Result> { - let mut path = self.public_ancestors_including_self(channel, &*tx).await?; - if path.last().unwrap().id == channel.id { - path.pop(); - } - Ok(path.pop()) - } - - pub(crate) async fn public_ancestors_including_self( - &self, - channel: &channel::Model, - tx: &DatabaseTransaction, - ) -> Result> { - let visible_channels = channel::Entity::find() - .filter(channel::Column::Id.is_in(channel.ancestors_including_self())) - .filter(channel::Column::Visibility.eq(ChannelVisibility::Public)) - .order_by_asc(channel::Column::ParentPath) - .all(&*tx) - .await?; - - Ok(visible_channels) - } - /// Returns the role for a user in the given channel. pub async fn channel_role_for_user( &self, @@ -1057,77 +850,25 @@ impl Database { user_id: UserId, tx: &DatabaseTransaction, ) -> Result> { - #[derive(Copy, Clone, Debug, EnumIter, DeriveColumn)] - enum QueryChannelMembership { - ChannelId, - Role, - Visibility, - } - - let mut rows = channel_member::Entity::find() - .left_join(channel::Entity) + let membership = channel_member::Entity::find() .filter( channel_member::Column::ChannelId - .is_in(channel.ancestors_including_self()) + .eq(channel.root_id()) .and(channel_member::Column::UserId.eq(user_id)) .and(channel_member::Column::Accepted.eq(true)), ) - .select_only() - .column(channel_member::Column::ChannelId) - .column(channel_member::Column::Role) - .column(channel::Column::Visibility) - .into_values::<_, QueryChannelMembership>() - .stream(&*tx) + .one(&*tx) .await?; - let mut user_role: Option = None; + let Some(membership) = membership else { + return Ok(None); + }; - let mut is_participant = false; - let mut current_channel_visibility = None; - - // note these channels are not iterated in any particular order, - // our current logic takes the highest permission available. - while let Some(row) = rows.next().await { - let (membership_channel, role, visibility): ( - ChannelId, - ChannelRole, - ChannelVisibility, - ) = row?; - - match role { - ChannelRole::Admin | ChannelRole::Member | ChannelRole::Banned => { - if let Some(users_role) = user_role { - user_role = Some(users_role.max(role)); - } else { - user_role = Some(role) - } - } - ChannelRole::Guest if visibility == ChannelVisibility::Public => { - is_participant = true - } - ChannelRole::Guest => {} - } - if channel.id == membership_channel { - current_channel_visibility = Some(visibility); - } - } - // free up database connection - drop(rows); - - if is_participant && user_role.is_none() { - if current_channel_visibility.is_none() { - current_channel_visibility = channel::Entity::find() - .filter(channel::Column::Id.eq(channel.id)) - .one(&*tx) - .await? - .map(|channel| channel.visibility); - } - if current_channel_visibility == Some(ChannelVisibility::Public) { - user_role = Some(ChannelRole::Guest); - } + if !membership.role.can_see_channel(channel.visibility) { + return Ok(None); } - Ok(user_role) + Ok(Some(membership.role)) } // Get the descendants of the given set if channels, ordered by their @@ -1180,11 +921,10 @@ impl Database { pub async fn get_channel(&self, channel_id: ChannelId, user_id: UserId) -> Result { self.transaction(|tx| async move { let channel = self.get_channel_internal(channel_id, &*tx).await?; - let role = self - .check_user_is_channel_participant(&channel, user_id, &*tx) + self.check_user_is_channel_participant(&channel, user_id, &*tx) .await?; - Ok(Channel::from_model(channel, role)) + Ok(Channel::from_model(channel)) }) .await } @@ -1241,61 +981,39 @@ impl Database { pub async fn move_channel( &self, channel_id: ChannelId, - new_parent_id: Option, + new_parent_id: ChannelId, admin_id: UserId, - ) -> Result> { + ) -> Result<(Vec, Vec)> { self.transaction(|tx| async move { let channel = self.get_channel_internal(channel_id, &*tx).await?; self.check_user_is_channel_admin(&channel, admin_id, &*tx) .await?; + let new_parent = self.get_channel_internal(new_parent_id, &*tx).await?; - let new_parent_path; - let new_parent_channel; - if let Some(new_parent_id) = new_parent_id { - let new_parent = self.get_channel_internal(new_parent_id, &*tx).await?; - self.check_user_is_channel_admin(&new_parent, admin_id, &*tx) - .await?; - - if new_parent - .ancestors_including_self() - .any(|id| id == channel.id) - { - Err(anyhow!("cannot move a channel into one of its descendants"))?; - } - - new_parent_path = new_parent.path(); - new_parent_channel = Some(new_parent); - } else { - new_parent_path = String::new(); - new_parent_channel = None; - }; - - let previous_participants = self - .get_channel_participant_details_internal(&channel, &*tx) - .await?; - - let old_path = format!("{}{}/", channel.parent_path, channel.id); - let new_path = format!("{}{}/", new_parent_path, channel.id); - - if old_path == new_path { - return Ok(None); + if new_parent.root_id() != channel.root_id() { + Err(anyhow!("cannot move a channel into a different root"))?; } + if new_parent + .ancestors_including_self() + .any(|id| id == channel.id) + { + Err(anyhow!("cannot move a channel into one of its descendants"))?; + } + + if channel.visibility == ChannelVisibility::Public + && new_parent.visibility != ChannelVisibility::Public + { + Err(anyhow!("public channels must descend from public channels"))?; + } + + let root_id = channel.root_id(); + let old_path = format!("{}{}/", channel.parent_path, channel.id); + let new_path = format!("{}{}/", new_parent.path(), channel.id); + let mut model = channel.into_active_model(); - model.parent_path = ActiveValue::Set(new_parent_path); - model.update(&*tx).await?; - - if new_parent_channel.is_none() { - channel_member::ActiveModel { - id: ActiveValue::NotSet, - channel_id: ActiveValue::Set(channel_id), - user_id: ActiveValue::Set(admin_id), - accepted: ActiveValue::Set(true), - role: ActiveValue::Set(ChannelRole::Admin), - } - .insert(&*tx) - .await?; - } + model.parent_path = ActiveValue::Set(new_parent.path()); + let channel = model.update(&*tx).await?; let descendent_ids = ChannelId::find_by_statement::(Statement::from_sql_and_values( @@ -1310,10 +1028,22 @@ impl Database { .all(&*tx) .await?; - Ok(Some(MoveChannelResult { - previous_participants, - descendent_ids, - })) + let all_moved_ids = Some(channel.id).into_iter().chain(descendent_ids); + + let channels = channel::Entity::find() + .filter(channel::Column::Id.is_in(all_moved_ids)) + .all(&*tx) + .await? + .into_iter() + .map(|c| Channel::from_model(c)) + .collect::>(); + + let channel_members = channel_member::Entity::find() + .filter(channel_member::Column::ChannelId.eq(root_id)) + .all(&*tx) + .await?; + + Ok((channels, channel_members)) }) .await } diff --git a/crates/collab/src/db/tables/channel.rs b/crates/collab/src/db/tables/channel.rs index a35913a705..7b38218d67 100644 --- a/crates/collab/src/db/tables/channel.rs +++ b/crates/collab/src/db/tables/channel.rs @@ -17,6 +17,14 @@ impl Model { self.ancestors().last() } + pub fn is_root(&self) -> bool { + self.parent_path.is_empty() + } + + pub fn root_id(&self) -> ChannelId { + self.ancestors().next().unwrap_or(self.id) + } + pub fn ancestors(&self) -> impl Iterator + '_ { self.parent_path .trim_end_matches('/') diff --git a/crates/collab/src/db/tests.rs b/crates/collab/src/db/tests.rs index 4a9c98f022..a47e6330d3 100644 --- a/crates/collab/src/db/tests.rs +++ b/crates/collab/src/db/tests.rs @@ -150,14 +150,13 @@ impl Drop for TestDb { } } -fn channel_tree(channels: &[(ChannelId, &[ChannelId], &'static str, ChannelRole)]) -> Vec { +fn channel_tree(channels: &[(ChannelId, &[ChannelId], &'static str)]) -> Vec { channels .iter() - .map(|(id, parent_path, name, role)| Channel { + .map(|(id, parent_path, name)| Channel { id: *id, name: name.to_string(), visibility: ChannelVisibility::Members, - role: *role, parent_path: parent_path.to_vec(), }) .collect() diff --git a/crates/collab/src/db/tests/channel_tests.rs b/crates/collab/src/db/tests/channel_tests.rs index 8a7a19ed3a..a5e083f935 100644 --- a/crates/collab/src/db/tests/channel_tests.rs +++ b/crates/collab/src/db/tests/channel_tests.rs @@ -62,23 +62,13 @@ async fn test_channels(db: &Arc) { assert_eq!( result.channels, channel_tree(&[ - (zed_id, &[], "zed", ChannelRole::Admin), - (crdb_id, &[zed_id], "crdb", ChannelRole::Admin), - ( - livestreaming_id, - &[zed_id], - "livestreaming", - ChannelRole::Admin - ), - (replace_id, &[zed_id], "replace", ChannelRole::Admin), - (rust_id, &[], "rust", ChannelRole::Admin), - (cargo_id, &[rust_id], "cargo", ChannelRole::Admin), - ( - cargo_ra_id, - &[rust_id, cargo_id], - "cargo-ra", - ChannelRole::Admin - ) + (zed_id, &[], "zed"), + (crdb_id, &[zed_id], "crdb"), + (livestreaming_id, &[zed_id], "livestreaming",), + (replace_id, &[zed_id], "replace"), + (rust_id, &[], "rust"), + (cargo_id, &[rust_id], "cargo"), + (cargo_ra_id, &[rust_id, cargo_id], "cargo-ra",) ],) ); @@ -86,15 +76,10 @@ async fn test_channels(db: &Arc) { assert_eq!( result.channels, channel_tree(&[ - (zed_id, &[], "zed", ChannelRole::Member), - (crdb_id, &[zed_id], "crdb", ChannelRole::Member), - ( - livestreaming_id, - &[zed_id], - "livestreaming", - ChannelRole::Member - ), - (replace_id, &[zed_id], "replace", ChannelRole::Member) + (zed_id, &[], "zed"), + (crdb_id, &[zed_id], "crdb"), + (livestreaming_id, &[zed_id], "livestreaming",), + (replace_id, &[zed_id], "replace") ],) ); @@ -112,15 +97,10 @@ async fn test_channels(db: &Arc) { assert_eq!( result.channels, channel_tree(&[ - (zed_id, &[], "zed", ChannelRole::Admin), - (crdb_id, &[zed_id], "crdb", ChannelRole::Admin), - ( - livestreaming_id, - &[zed_id], - "livestreaming", - ChannelRole::Admin - ), - (replace_id, &[zed_id], "replace", ChannelRole::Admin) + (zed_id, &[], "zed"), + (crdb_id, &[zed_id], "crdb"), + (livestreaming_id, &[zed_id], "livestreaming",), + (replace_id, &[zed_id], "replace") ],) ); @@ -271,14 +251,19 @@ async fn test_channel_invites(db: &Arc) { &[ proto::ChannelMember { user_id: user_1.to_proto(), - kind: proto::channel_member::Kind::AncestorMember.into(), + kind: proto::channel_member::Kind::Member.into(), role: proto::ChannelRole::Admin.into(), }, proto::ChannelMember { user_id: user_2.to_proto(), - kind: proto::channel_member::Kind::AncestorMember.into(), + kind: proto::channel_member::Kind::Member.into(), role: proto::ChannelRole::Member.into(), }, + proto::ChannelMember { + user_id: user_3.to_proto(), + kind: proto::channel_member::Kind::Invitee.into(), + role: proto::ChannelRole::Admin.into(), + }, ] ); } @@ -420,13 +405,6 @@ async fn test_db_channel_moving_bugs(db: &Arc) { .await .unwrap(); - // Move to same parent should be a no-op - assert!(db - .move_channel(projects_id, Some(zed_id), user_id) - .await - .unwrap() - .is_none()); - let result = db.get_channels_for_user(user_id).await.unwrap(); assert_channel_tree( result.channels, @@ -437,20 +415,8 @@ async fn test_db_channel_moving_bugs(db: &Arc) { ], ); - // Move the project channel to the root - db.move_channel(projects_id, None, user_id).await.unwrap(); - let result = db.get_channels_for_user(user_id).await.unwrap(); - assert_channel_tree( - result.channels, - &[ - (zed_id, &[]), - (projects_id, &[]), - (livestreaming_id, &[projects_id]), - ], - ); - // Can't move a channel into its ancestor - db.move_channel(projects_id, Some(livestreaming_id), user_id) + db.move_channel(projects_id, livestreaming_id, user_id) .await .unwrap_err(); let result = db.get_channels_for_user(user_id).await.unwrap(); @@ -458,8 +424,8 @@ async fn test_db_channel_moving_bugs(db: &Arc) { result.channels, &[ (zed_id, &[]), - (projects_id, &[]), - (livestreaming_id, &[projects_id]), + (projects_id, &[zed_id]), + (livestreaming_id, &[zed_id, projects_id]), ], ); } @@ -476,32 +442,39 @@ async fn test_user_is_channel_participant(db: &Arc) { let guest = new_test_user(db, "guest@example.com").await; let zed_channel = db.create_root_channel("zed", admin).await.unwrap(); - let active_channel_id = db + let internal_channel_id = db .create_sub_channel("active", zed_channel, admin) .await .unwrap(); - let vim_channel_id = db - .create_sub_channel("vim", active_channel_id, admin) + let public_channel_id = db + .create_sub_channel("vim", zed_channel, admin) .await .unwrap(); - db.set_channel_visibility(vim_channel_id, crate::db::ChannelVisibility::Public, admin) + db.set_channel_visibility(zed_channel, crate::db::ChannelVisibility::Public, admin) .await .unwrap(); - db.invite_channel_member(active_channel_id, member, admin, ChannelRole::Member) + db.set_channel_visibility( + public_channel_id, + crate::db::ChannelVisibility::Public, + admin, + ) + .await + .unwrap(); + db.invite_channel_member(zed_channel, member, admin, ChannelRole::Member) .await .unwrap(); - db.invite_channel_member(vim_channel_id, guest, admin, ChannelRole::Guest) + db.invite_channel_member(zed_channel, guest, admin, ChannelRole::Guest) .await .unwrap(); - db.respond_to_channel_invite(active_channel_id, member, true) + db.respond_to_channel_invite(zed_channel, member, true) .await .unwrap(); db.transaction(|tx| async move { db.check_user_is_channel_participant( - &db.get_channel_internal(vim_channel_id, &*tx).await?, + &db.get_channel_internal(public_channel_id, &*tx).await?, admin, &*tx, ) @@ -511,7 +484,7 @@ async fn test_user_is_channel_participant(db: &Arc) { .unwrap(); db.transaction(|tx| async move { db.check_user_is_channel_participant( - &db.get_channel_internal(vim_channel_id, &*tx).await?, + &db.get_channel_internal(public_channel_id, &*tx).await?, member, &*tx, ) @@ -521,7 +494,7 @@ async fn test_user_is_channel_participant(db: &Arc) { .unwrap(); let mut members = db - .get_channel_participant_details(vim_channel_id, admin) + .get_channel_participant_details(public_channel_id, admin) .await .unwrap(); @@ -532,12 +505,12 @@ async fn test_user_is_channel_participant(db: &Arc) { &[ proto::ChannelMember { user_id: admin.to_proto(), - kind: proto::channel_member::Kind::AncestorMember.into(), + kind: proto::channel_member::Kind::Member.into(), role: proto::ChannelRole::Admin.into(), }, proto::ChannelMember { user_id: member.to_proto(), - kind: proto::channel_member::Kind::AncestorMember.into(), + kind: proto::channel_member::Kind::Member.into(), role: proto::ChannelRole::Member.into(), }, proto::ChannelMember { @@ -548,13 +521,13 @@ async fn test_user_is_channel_participant(db: &Arc) { ] ); - db.respond_to_channel_invite(vim_channel_id, guest, true) + db.respond_to_channel_invite(zed_channel, guest, true) .await .unwrap(); db.transaction(|tx| async move { db.check_user_is_channel_participant( - &db.get_channel_internal(vim_channel_id, &*tx).await?, + &db.get_channel_internal(public_channel_id, &*tx).await?, guest, &*tx, ) @@ -564,23 +537,29 @@ async fn test_user_is_channel_participant(db: &Arc) { .unwrap(); let channels = db.get_channels_for_user(guest).await.unwrap().channels; - assert_channel_tree(channels, &[(vim_channel_id, &[])]); + assert_channel_tree( + channels, + &[(zed_channel, &[]), (public_channel_id, &[zed_channel])], + ); let channels = db.get_channels_for_user(member).await.unwrap().channels; assert_channel_tree( channels, &[ - (active_channel_id, &[]), - (vim_channel_id, &[active_channel_id]), + (zed_channel, &[]), + (internal_channel_id, &[zed_channel]), + (public_channel_id, &[zed_channel]), ], ); - db.set_channel_member_role(vim_channel_id, admin, guest, ChannelRole::Banned) + db.set_channel_member_role(zed_channel, admin, guest, ChannelRole::Banned) .await .unwrap(); assert!(db .transaction(|tx| async move { db.check_user_is_channel_participant( - &db.get_channel_internal(vim_channel_id, &*tx).await.unwrap(), + &db.get_channel_internal(public_channel_id, &*tx) + .await + .unwrap(), guest, &*tx, ) @@ -590,7 +569,7 @@ async fn test_user_is_channel_participant(db: &Arc) { .is_err()); let mut members = db - .get_channel_participant_details(vim_channel_id, admin) + .get_channel_participant_details(public_channel_id, admin) .await .unwrap(); @@ -601,12 +580,12 @@ async fn test_user_is_channel_participant(db: &Arc) { &[ proto::ChannelMember { user_id: admin.to_proto(), - kind: proto::channel_member::Kind::AncestorMember.into(), + kind: proto::channel_member::Kind::Member.into(), role: proto::ChannelRole::Admin.into(), }, proto::ChannelMember { user_id: member.to_proto(), - kind: proto::channel_member::Kind::AncestorMember.into(), + kind: proto::channel_member::Kind::Member.into(), role: proto::ChannelRole::Member.into(), }, proto::ChannelMember { @@ -617,11 +596,7 @@ async fn test_user_is_channel_participant(db: &Arc) { ] ); - db.remove_channel_member(vim_channel_id, guest, admin) - .await - .unwrap(); - - db.set_channel_visibility(zed_channel, crate::db::ChannelVisibility::Public, admin) + db.remove_channel_member(zed_channel, guest, admin) .await .unwrap(); @@ -631,7 +606,7 @@ async fn test_user_is_channel_participant(db: &Arc) { // currently people invited to parent channels are not shown here let mut members = db - .get_channel_participant_details(vim_channel_id, admin) + .get_channel_participant_details(public_channel_id, admin) .await .unwrap(); @@ -642,14 +617,19 @@ async fn test_user_is_channel_participant(db: &Arc) { &[ proto::ChannelMember { user_id: admin.to_proto(), - kind: proto::channel_member::Kind::AncestorMember.into(), + kind: proto::channel_member::Kind::Member.into(), role: proto::ChannelRole::Admin.into(), }, proto::ChannelMember { user_id: member.to_proto(), - kind: proto::channel_member::Kind::AncestorMember.into(), + kind: proto::channel_member::Kind::Member.into(), role: proto::ChannelRole::Member.into(), }, + proto::ChannelMember { + user_id: guest.to_proto(), + kind: proto::channel_member::Kind::Invitee.into(), + role: proto::ChannelRole::Guest.into(), + }, ] ); @@ -670,7 +650,7 @@ async fn test_user_is_channel_participant(db: &Arc) { assert!(db .transaction(|tx| async move { db.check_user_is_channel_participant( - &db.get_channel_internal(active_channel_id, &*tx) + &db.get_channel_internal(internal_channel_id, &*tx) .await .unwrap(), guest, @@ -683,7 +663,9 @@ async fn test_user_is_channel_participant(db: &Arc) { db.transaction(|tx| async move { db.check_user_is_channel_participant( - &db.get_channel_internal(vim_channel_id, &*tx).await.unwrap(), + &db.get_channel_internal(public_channel_id, &*tx) + .await + .unwrap(), guest, &*tx, ) @@ -693,7 +675,7 @@ async fn test_user_is_channel_participant(db: &Arc) { .unwrap(); let mut members = db - .get_channel_participant_details(vim_channel_id, admin) + .get_channel_participant_details(public_channel_id, admin) .await .unwrap(); @@ -704,17 +686,17 @@ async fn test_user_is_channel_participant(db: &Arc) { &[ proto::ChannelMember { user_id: admin.to_proto(), - kind: proto::channel_member::Kind::AncestorMember.into(), + kind: proto::channel_member::Kind::Member.into(), role: proto::ChannelRole::Admin.into(), }, proto::ChannelMember { user_id: member.to_proto(), - kind: proto::channel_member::Kind::AncestorMember.into(), + kind: proto::channel_member::Kind::Member.into(), role: proto::ChannelRole::Member.into(), }, proto::ChannelMember { user_id: guest.to_proto(), - kind: proto::channel_member::Kind::AncestorMember.into(), + kind: proto::channel_member::Kind::Member.into(), role: proto::ChannelRole::Guest.into(), }, ] @@ -723,67 +705,10 @@ async fn test_user_is_channel_participant(db: &Arc) { let channels = db.get_channels_for_user(guest).await.unwrap().channels; assert_channel_tree( channels, - &[(zed_channel, &[]), (vim_channel_id, &[zed_channel])], + &[(zed_channel, &[]), (public_channel_id, &[zed_channel])], ) } -test_both_dbs!( - test_user_joins_correct_channel, - test_user_joins_correct_channel_postgres, - test_user_joins_correct_channel_sqlite -); - -async fn test_user_joins_correct_channel(db: &Arc) { - let admin = new_test_user(db, "admin@example.com").await; - - let zed_channel = db.create_root_channel("zed", admin).await.unwrap(); - - let active_channel = db - .create_sub_channel("active", zed_channel, admin) - .await - .unwrap(); - - let vim_channel = db - .create_sub_channel("vim", active_channel, admin) - .await - .unwrap(); - - let vim2_channel = db - .create_sub_channel("vim2", vim_channel, admin) - .await - .unwrap(); - - db.set_channel_visibility(zed_channel, crate::db::ChannelVisibility::Public, admin) - .await - .unwrap(); - - db.set_channel_visibility(vim_channel, crate::db::ChannelVisibility::Public, admin) - .await - .unwrap(); - - db.set_channel_visibility(vim2_channel, crate::db::ChannelVisibility::Public, admin) - .await - .unwrap(); - - let most_public = db - .transaction(|tx| async move { - Ok(db - .public_ancestors_including_self( - &db.get_channel_internal(vim_channel, &*tx).await.unwrap(), - &tx, - ) - .await? - .first() - .cloned()) - }) - .await - .unwrap() - .unwrap() - .id; - - assert_eq!(most_public, zed_channel) -} - test_both_dbs!( test_guest_access, test_guest_access_postgres, diff --git a/crates/collab/src/db/tests/message_tests.rs b/crates/collab/src/db/tests/message_tests.rs index 613830d38f..fea3ae9805 100644 --- a/crates/collab/src/db/tests/message_tests.rs +++ b/crates/collab/src/db/tests/message_tests.rs @@ -15,7 +15,7 @@ test_both_dbs!( async fn test_channel_message_retrieval(db: &Arc) { let user = new_test_user(db, "user@example.com").await; - let channel = db.create_channel("channel", None, user).await.unwrap(); + let channel = db.create_channel("channel", None, user).await.unwrap().0; let owner_id = db.create_server("test").await.unwrap().0 as u32; db.join_channel_chat(channel.id, rpc::ConnectionId { owner_id, id: 0 }, user) @@ -291,7 +291,12 @@ async fn test_channel_message_mentions(db: &Arc) { let user_b = new_test_user(db, "user_b@example.com").await; let user_c = new_test_user(db, "user_c@example.com").await; - let channel = db.create_channel("channel", None, user_a).await.unwrap().id; + let channel = db + .create_channel("channel", None, user_a) + .await + .unwrap() + .0 + .id; db.invite_channel_member(channel, user_b, user_a, ChannelRole::Member) .await .unwrap(); diff --git a/crates/collab/src/rpc.rs b/crates/collab/src/rpc.rs index a6984e3edd..6f24c9e8cf 100644 --- a/crates/collab/src/rpc.rs +++ b/crates/collab/src/rpc.rs @@ -5,8 +5,7 @@ use crate::{ db::{ self, BufferId, ChannelId, ChannelRole, ChannelsForUser, CreatedChannelMessage, Database, InviteMemberResult, MembershipUpdated, MessageId, NotificationId, ProjectId, - RemoveChannelMemberResult, RenameChannelResult, RespondToChannelInvite, RoomId, ServerId, - SetChannelVisibilityResult, User, UserId, + RemoveChannelMemberResult, RespondToChannelInvite, RoomId, ServerId, User, UserId, }, executor::Executor, AppState, Error, Result, @@ -602,6 +601,7 @@ impl Server { let mut pool = this.connection_pool.lock(); pool.add_connection(connection_id, user_id, user.admin); this.peer.send(connection_id, build_initial_contacts_update(contacts, &pool))?; + this.peer.send(connection_id, build_update_user_channels(&channels_for_user.channel_memberships))?; this.peer.send(connection_id, build_channels_update( channels_for_user, channel_invites @@ -2300,7 +2300,7 @@ async fn create_channel( let db = session.db().await; let parent_id = request.parent_id.map(|id| ChannelId::from_proto(id)); - let channel = db + let (channel, owner, channel_members) = db .create_channel(&request.name, parent_id, session.user_id) .await?; @@ -2309,20 +2309,30 @@ async fn create_channel( parent_id: request.parent_id, })?; - let participants_to_update; - if let Some(parent) = parent_id { - participants_to_update = db.new_participants_to_notify(parent).await?; - } else { - participants_to_update = vec![]; + let connection_pool = session.connection_pool().await; + if let Some(owner) = owner { + let update = proto::UpdateUserChannels { + channel_memberships: vec![proto::ChannelMembership { + channel_id: owner.channel_id.to_proto(), + role: owner.role.into(), + }], + ..Default::default() + }; + for connection_id in connection_pool.user_connection_ids(owner.user_id) { + session.peer.send(connection_id, update.clone())?; + } } - let connection_pool = session.connection_pool().await; - for (user_id, channels) in participants_to_update { - let update = build_channels_update(channels, vec![]); - for connection_id in connection_pool.user_connection_ids(user_id) { - if user_id == session.user_id { - continue; - } + for channel_member in channel_members { + if !channel_member.role.can_see_channel(channel.visibility) { + continue; + } + + let update = proto::UpdateChannels { + channels: vec![channel.to_proto()], + ..Default::default() + }; + for connection_id in connection_pool.user_connection_ids(channel_member.user_id) { session.peer.send(connection_id, update.clone())?; } } @@ -2439,7 +2449,9 @@ async fn remove_channel_member( Ok(()) } -/// Toggle the channel between public and private +/// Toggle the channel between public and private. +/// Care is taken to maintain the invariant that public channels only descend from public channels, +/// (though members-only channels can appear at any point in the heirarchy). async fn set_channel_visibility( request: proto::SetChannelVisibility, response: Response, @@ -2449,27 +2461,25 @@ async fn set_channel_visibility( let channel_id = ChannelId::from_proto(request.channel_id); let visibility = request.visibility().into(); - let SetChannelVisibilityResult { - participants_to_update, - participants_to_remove, - channels_to_remove, - } = db + let (channel, channel_members) = db .set_channel_visibility(channel_id, visibility, session.user_id) .await?; let connection_pool = session.connection_pool().await; - for (user_id, channels) in participants_to_update { - let update = build_channels_update(channels, vec![]); - for connection_id in connection_pool.user_connection_ids(user_id) { - session.peer.send(connection_id, update.clone())?; - } - } - for user_id in participants_to_remove { - let update = proto::UpdateChannels { - delete_channels: channels_to_remove.iter().map(|id| id.to_proto()).collect(), - ..Default::default() + for member in channel_members { + let update = if member.role.can_see_channel(channel.visibility) { + proto::UpdateChannels { + channels: vec![channel.to_proto()], + ..Default::default() + } + } else { + proto::UpdateChannels { + delete_channels: vec![channel.id.to_proto()], + ..Default::default() + } }; - for connection_id in connection_pool.user_connection_ids(user_id) { + + for connection_id in connection_pool.user_connection_ids(member.user_id) { session.peer.send(connection_id, update.clone())?; } } @@ -2478,7 +2488,7 @@ async fn set_channel_visibility( Ok(()) } -/// Alter the role for a user in the channel +/// Alter the role for a user in the channel. async fn set_channel_member_role( request: proto::SetChannelMemberRole, response: Response, @@ -2534,10 +2544,7 @@ async fn rename_channel( ) -> Result<()> { let db = session.db().await; let channel_id = ChannelId::from_proto(request.channel_id); - let RenameChannelResult { - channel, - participants_to_update, - } = db + let (channel, channel_members) = db .rename_channel(channel_id, session.user_id, &request.name) .await?; @@ -2546,13 +2553,15 @@ async fn rename_channel( })?; let connection_pool = session.connection_pool().await; - for (user_id, channel) in participants_to_update { - for connection_id in connection_pool.user_connection_ids(user_id) { - let update = proto::UpdateChannels { - channels: vec![channel.to_proto()], - ..Default::default() - }; - + for channel_member in channel_members { + if !channel_member.role.can_see_channel(channel.visibility) { + continue; + } + let update = proto::UpdateChannels { + channels: vec![channel.to_proto()], + ..Default::default() + }; + for connection_id in connection_pool.user_connection_ids(channel_member.user_id) { session.peer.send(connection_id, update.clone())?; } } @@ -2567,57 +2576,38 @@ async fn move_channel( session: Session, ) -> Result<()> { let channel_id = ChannelId::from_proto(request.channel_id); - let to = request.to.map(ChannelId::from_proto); + let to = ChannelId::from_proto(request.to); - let result = session + let (channels, channel_members) = session .db() .await .move_channel(channel_id, to, session.user_id) .await?; - if let Some(result) = result { - let participants_to_update: HashMap<_, _> = session - .db() - .await - .new_participants_to_notify(to.unwrap_or(channel_id)) - .await? - .into_iter() - .collect(); - - let mut moved_channels: HashSet = HashSet::default(); - for id in result.descendent_ids { - moved_channels.insert(id); - } - moved_channels.insert(channel_id); - - let mut participants_to_remove: HashSet = HashSet::default(); - for participant in result.previous_participants { - if participant.kind == proto::channel_member::Kind::AncestorMember { - if !participants_to_update.contains_key(&participant.user_id) { - participants_to_remove.insert(participant.user_id); + let connection_pool = session.connection_pool().await; + for member in channel_members { + let channels = channels + .iter() + .filter_map(|channel| { + if member.role.can_see_channel(channel.visibility) { + Some(channel.to_proto()) + } else { + None } - } + }) + .collect::>(); + if channels.is_empty() { + continue; } - let moved_channels: Vec = moved_channels.iter().map(|id| id.to_proto()).collect(); + let update = proto::UpdateChannels { + channels, + ..Default::default() + }; + dbg!(&member, &update); - let connection_pool = session.connection_pool().await; - for (user_id, channels) in participants_to_update { - let mut update = build_channels_update(channels, vec![]); - update.delete_channels = moved_channels.clone(); - for connection_id in connection_pool.user_connection_ids(user_id) { - session.peer.send(connection_id, update.clone())?; - } - } - - for user_id in participants_to_remove { - let update = proto::UpdateChannels { - delete_channels: moved_channels.clone(), - ..Default::default() - }; - for connection_id in connection_pool.user_connection_ids(user_id) { - session.peer.send(connection_id, update.clone())?; - } + for connection_id in connection_pool.user_connection_ids(member.user_id) { + session.peer.send(connection_id, update.clone())?; } } @@ -3305,6 +3295,21 @@ fn notify_membership_updated( } } +fn build_update_user_channels( + memberships: &Vec, +) -> proto::UpdateUserChannels { + proto::UpdateUserChannels { + channel_memberships: memberships + .iter() + .map(|m| proto::ChannelMembership { + channel_id: m.channel_id.to_proto(), + role: m.role.into(), + }) + .collect(), + ..Default::default() + } +} + fn build_channels_update( channels: ChannelsForUser, channel_invites: Vec, diff --git a/crates/collab/src/tests/channel_guest_tests.rs b/crates/collab/src/tests/channel_guest_tests.rs index 26e9c56a4b..bb1f493f0c 100644 --- a/crates/collab/src/tests/channel_guest_tests.rs +++ b/crates/collab/src/tests/channel_guest_tests.rs @@ -195,6 +195,13 @@ async fn test_channel_requires_zed_cla(cx_a: &mut TestAppContext, cx_b: &mut Tes }) .await .unwrap(); + client_a + .channel_store() + .update(cx_a, |store, cx| { + store.set_channel_visibility(parent_channel_id, proto::ChannelVisibility::Public, cx) + }) + .await + .unwrap(); client_a .channel_store() .update(cx_a, |store, cx| { diff --git a/crates/collab/src/tests/channel_tests.rs b/crates/collab/src/tests/channel_tests.rs index 7fbdf8ba7f..56511df207 100644 --- a/crates/collab/src/tests/channel_tests.rs +++ b/crates/collab/src/tests/channel_tests.rs @@ -48,13 +48,11 @@ async fn test_core_channels( id: channel_a_id, name: "channel-a".into(), depth: 0, - role: ChannelRole::Admin, }, ExpectedChannel { id: channel_b_id, name: "channel-b".into(), depth: 1, - role: ChannelRole::Admin, }, ], ); @@ -94,7 +92,6 @@ async fn test_core_channels( id: channel_a_id, name: "channel-a".into(), depth: 0, - role: ChannelRole::Member, }], ); @@ -141,13 +138,11 @@ async fn test_core_channels( ExpectedChannel { id: channel_a_id, name: "channel-a".into(), - role: ChannelRole::Member, depth: 0, }, ExpectedChannel { id: channel_b_id, name: "channel-b".into(), - role: ChannelRole::Member, depth: 1, }, ], @@ -169,19 +164,16 @@ async fn test_core_channels( ExpectedChannel { id: channel_a_id, name: "channel-a".into(), - role: ChannelRole::Member, depth: 0, }, ExpectedChannel { id: channel_b_id, name: "channel-b".into(), - role: ChannelRole::Member, depth: 1, }, ExpectedChannel { id: channel_c_id, name: "channel-c".into(), - role: ChannelRole::Member, depth: 2, }, ], @@ -213,19 +205,16 @@ async fn test_core_channels( id: channel_a_id, name: "channel-a".into(), depth: 0, - role: ChannelRole::Admin, }, ExpectedChannel { id: channel_b_id, name: "channel-b".into(), depth: 1, - role: ChannelRole::Admin, }, ExpectedChannel { id: channel_c_id, name: "channel-c".into(), depth: 2, - role: ChannelRole::Admin, }, ], ); @@ -247,7 +236,6 @@ async fn test_core_channels( id: channel_a_id, name: "channel-a".into(), depth: 0, - role: ChannelRole::Admin, }], ); assert_channels( @@ -257,7 +245,6 @@ async fn test_core_channels( id: channel_a_id, name: "channel-a".into(), depth: 0, - role: ChannelRole::Admin, }], ); @@ -280,7 +267,6 @@ async fn test_core_channels( id: channel_a_id, name: "channel-a".into(), depth: 0, - role: ChannelRole::Admin, }], ); @@ -311,7 +297,6 @@ async fn test_core_channels( id: channel_a_id, name: "channel-a-renamed".into(), depth: 0, - role: ChannelRole::Admin, }], ); } @@ -420,7 +405,6 @@ async fn test_channel_room( id: zed_id, name: "zed".into(), depth: 0, - role: ChannelRole::Member, }], ); cx_b.read(|cx| { @@ -681,7 +665,6 @@ async fn test_permissions_update_while_invited( depth: 0, id: rust_id, name: "rust".into(), - role: ChannelRole::Member, }], ); assert_channels(client_b.channel_store(), cx_b, &[]); @@ -709,7 +692,6 @@ async fn test_permissions_update_while_invited( depth: 0, id: rust_id, name: "rust".into(), - role: ChannelRole::Member, }], ); assert_channels(client_b.channel_store(), cx_b, &[]); @@ -748,7 +730,6 @@ async fn test_channel_rename( depth: 0, id: rust_id, name: "rust-archive".into(), - role: ChannelRole::Admin, }], ); @@ -760,7 +741,6 @@ async fn test_channel_rename( depth: 0, id: rust_id, name: "rust-archive".into(), - role: ChannelRole::Member, }], ); } @@ -889,7 +869,6 @@ async fn test_lost_channel_creation( depth: 0, id: channel_id, name: "x".into(), - role: ChannelRole::Member, }], ); @@ -913,13 +892,11 @@ async fn test_lost_channel_creation( depth: 0, id: channel_id, name: "x".into(), - role: ChannelRole::Admin, }, ExpectedChannel { depth: 1, id: subchannel_id, name: "subchannel".into(), - role: ChannelRole::Admin, }, ], ); @@ -944,13 +921,11 @@ async fn test_lost_channel_creation( depth: 0, id: channel_id, name: "x".into(), - role: ChannelRole::Member, }, ExpectedChannel { depth: 1, id: subchannel_id, name: "subchannel".into(), - role: ChannelRole::Member, }, ], ); @@ -1035,7 +1010,7 @@ async fn test_channel_link_notifications( let vim_channel = client_a .channel_store() .update(cx_a, |channel_store, cx| { - channel_store.create_channel("vim", None, cx) + channel_store.create_channel("vim", Some(zed_channel), cx) }) .await .unwrap(); @@ -1048,26 +1023,16 @@ async fn test_channel_link_notifications( .await .unwrap(); - client_a - .channel_store() - .update(cx_a, |channel_store, cx| { - channel_store.move_channel(vim_channel, Some(active_channel), cx) - }) - .await - .unwrap(); - - executor.run_until_parked(); - // the new channel shows for b and c assert_channels_list_shape( client_a.channel_store(), cx_a, - &[(zed_channel, 0), (active_channel, 1), (vim_channel, 2)], + &[(zed_channel, 0), (active_channel, 1), (vim_channel, 1)], ); assert_channels_list_shape( client_b.channel_store(), cx_b, - &[(zed_channel, 0), (active_channel, 1), (vim_channel, 2)], + &[(zed_channel, 0), (active_channel, 1), (vim_channel, 1)], ); assert_channels_list_shape( client_c.channel_store(), @@ -1078,7 +1043,7 @@ async fn test_channel_link_notifications( let helix_channel = client_a .channel_store() .update(cx_a, |channel_store, cx| { - channel_store.create_channel("helix", None, cx) + channel_store.create_channel("helix", Some(zed_channel), cx) }) .await .unwrap(); @@ -1086,7 +1051,7 @@ async fn test_channel_link_notifications( client_a .channel_store() .update(cx_a, |channel_store, cx| { - channel_store.move_channel(helix_channel, Some(vim_channel), cx) + channel_store.move_channel(helix_channel, vim_channel, cx) }) .await .unwrap(); @@ -1102,6 +1067,7 @@ async fn test_channel_link_notifications( }) .await .unwrap(); + cx_a.run_until_parked(); // the new channel shows for b and c assert_channels_list_shape( @@ -1110,8 +1076,8 @@ async fn test_channel_link_notifications( &[ (zed_channel, 0), (active_channel, 1), - (vim_channel, 2), - (helix_channel, 3), + (vim_channel, 1), + (helix_channel, 2), ], ); assert_channels_list_shape( @@ -1119,41 +1085,6 @@ async fn test_channel_link_notifications( cx_c, &[(zed_channel, 0), (vim_channel, 1), (helix_channel, 2)], ); - - client_a - .channel_store() - .update(cx_a, |channel_store, cx| { - channel_store.set_channel_visibility(vim_channel, proto::ChannelVisibility::Members, cx) - }) - .await - .unwrap(); - - executor.run_until_parked(); - - // the members-only channel is still shown for c, but hidden for b - assert_channels_list_shape( - client_b.channel_store(), - cx_b, - &[ - (zed_channel, 0), - (active_channel, 1), - (vim_channel, 2), - (helix_channel, 3), - ], - ); - cx_b.read(|cx| { - client_b.channel_store().read_with(cx, |channel_store, _| { - assert_eq!( - channel_store - .channel_for_id(vim_channel) - .unwrap() - .visibility, - proto::ChannelVisibility::Members - ) - }) - }); - - assert_channels_list_shape(client_c.channel_store(), cx_c, &[(zed_channel, 0)]); } #[gpui::test] @@ -1169,24 +1100,15 @@ async fn test_channel_membership_notifications( let user_b = client_b.user_id().unwrap(); let channels = server - .make_channel_tree( - &[ - ("zed", None), - ("active", Some("zed")), - ("vim", Some("active")), - ], - (&client_a, cx_a), - ) + .make_channel_tree(&[("zed", None), ("vim", Some("zed"))], (&client_a, cx_a)) .await; let zed_channel = channels[0]; - let _active_channel = channels[1]; - let vim_channel = channels[2]; + let vim_channel = channels[1]; try_join_all(client_a.channel_store().update(cx_a, |channel_store, cx| { [ channel_store.set_channel_visibility(zed_channel, proto::ChannelVisibility::Public, cx), channel_store.set_channel_visibility(vim_channel, proto::ChannelVisibility::Public, cx), - channel_store.invite_member(vim_channel, user_b, proto::ChannelRole::Member, cx), channel_store.invite_member(zed_channel, user_b, proto::ChannelRole::Guest, cx), ] })) @@ -1203,14 +1125,6 @@ async fn test_channel_membership_notifications( .await .unwrap(); - client_b - .channel_store() - .update(cx_b, |channel_store, cx| { - channel_store.respond_to_channel_invite(vim_channel, true, cx) - }) - .await - .unwrap(); - executor.run_until_parked(); // we have an admin (a), and a guest (b) with access to all of zed, and membership in vim. @@ -1222,45 +1136,14 @@ async fn test_channel_membership_notifications( depth: 0, id: zed_channel, name: "zed".into(), - role: ChannelRole::Guest, }, ExpectedChannel { depth: 1, id: vim_channel, name: "vim".into(), - role: ChannelRole::Member, }, ], ); - - client_a - .channel_store() - .update(cx_a, |channel_store, cx| { - channel_store.remove_member(vim_channel, user_b, cx) - }) - .await - .unwrap(); - - executor.run_until_parked(); - - assert_channels( - client_b.channel_store(), - cx_b, - &[ - ExpectedChannel { - depth: 0, - id: zed_channel, - name: "zed".into(), - role: ChannelRole::Guest, - }, - ExpectedChannel { - depth: 1, - id: vim_channel, - name: "vim".into(), - role: ChannelRole::Guest, - }, - ], - ) } #[gpui::test] @@ -1329,25 +1212,6 @@ async fn test_guest_access( assert_eq!(participants.len(), 1); assert_eq!(participants[0].id, client_b.user_id().unwrap()); }); - - client_a - .channel_store() - .update(cx_a, |channel_store, cx| { - channel_store.set_channel_visibility(channel_a, proto::ChannelVisibility::Members, cx) - }) - .await - .unwrap(); - executor.run_until_parked(); - - assert_channels_list_shape(client_b.channel_store(), cx_b, &[]); - - active_call_b - .update(cx_b, |call, cx| call.join_channel(channel_b, cx)) - .await - .unwrap(); - - executor.run_until_parked(); - assert_channels_list_shape(client_b.channel_store(), cx_b, &[(channel_b, 0)]); } #[gpui::test] @@ -1451,7 +1315,7 @@ async fn test_channel_moving( client_a .channel_store() .update(cx_a, |channel_store, cx| { - channel_store.move_channel(channel_d_id, Some(channel_b_id), cx) + channel_store.move_channel(channel_d_id, channel_b_id, cx) }) .await .unwrap(); @@ -1476,7 +1340,6 @@ struct ExpectedChannel { depth: usize, id: ChannelId, name: SharedString, - role: ChannelRole, } #[track_caller] @@ -1494,7 +1357,6 @@ fn assert_channel_invitations( depth: 0, name: channel.name.clone(), id: channel.id, - role: channel.role, }) .collect::>() }) @@ -1516,7 +1378,6 @@ fn assert_channels( depth, name: channel.name.clone().into(), id: channel.id, - role: channel.role, }) .collect::>() }) diff --git a/crates/collab/src/tests/test_server.rs b/crates/collab/src/tests/test_server.rs index c0386f4785..009561d9ae 100644 --- a/crates/collab/src/tests/test_server.rs +++ b/crates/collab/src/tests/test_server.rs @@ -125,6 +125,7 @@ impl TestServer { let channel_id = server .make_channel("a", None, (&client_a, cx_a), &mut [(&client_b, cx_b)]) .await; + cx_a.run_until_parked(); (client_a, client_b, channel_id) } diff --git a/crates/collab_ui/src/collab_panel.rs b/crates/collab_ui/src/collab_panel.rs index 098ba02a58..f16c634bc6 100644 --- a/crates/collab_ui/src/collab_panel.rs +++ b/crates/collab_ui/src/collab_panel.rs @@ -1548,7 +1548,7 @@ impl CollabPanel { if let Some(clipboard) = self.channel_clipboard.take() { self.channel_store .update(cx, |channel_store, cx| { - channel_store.move_channel(clipboard.channel_id, Some(to_channel_id), cx) + channel_store.move_channel(clipboard.channel_id, to_channel_id, cx) }) .detach_and_prompt_err("Failed to move channel", cx, |_, _| None) } @@ -1980,32 +1980,19 @@ impl CollabPanel { | Section::Offline => true, }; - h_flex() - .w_full() - .group("section-header") - .child( - ListHeader::new(text) - .when(can_collapse, |header| { - header.toggle(Some(!is_collapsed)).on_toggle(cx.listener( - move |this, _, cx| { - this.toggle_section_expanded(section, cx); - }, - )) - }) - .inset(true) - .end_slot::(button) - .selected(is_selected), - ) - .when(section == Section::Channels, |el| { - el.drag_over::(|style| style.bg(cx.theme().colors().ghost_element_hover)) - .on_drop(cx.listener(move |this, dragged_channel: &Channel, cx| { - this.channel_store - .update(cx, |channel_store, cx| { - channel_store.move_channel(dragged_channel.id, None, cx) - }) - .detach_and_prompt_err("Failed to move channel", cx, |_, _| None) - })) - }) + h_flex().w_full().group("section-header").child( + ListHeader::new(text) + .when(can_collapse, |header| { + header + .toggle(Some(!is_collapsed)) + .on_toggle(cx.listener(move |this, _, cx| { + this.toggle_section_expanded(section, cx); + })) + }) + .inset(true) + .end_slot::(button) + .selected(is_selected), + ) } fn render_contact( @@ -2276,7 +2263,7 @@ impl CollabPanel { .on_drop(cx.listener(move |this, dragged_channel: &Channel, cx| { this.channel_store .update(cx, |channel_store, cx| { - channel_store.move_channel(dragged_channel.id, Some(channel_id), cx) + channel_store.move_channel(dragged_channel.id, channel_id, cx) }) .detach_and_prompt_err("Failed to move channel", cx, |_, _| None) })) diff --git a/crates/collab_ui/src/collab_panel/channel_modal.rs b/crates/collab_ui/src/collab_panel/channel_modal.rs index 891c609316..826a61eb48 100644 --- a/crates/collab_ui/src/collab_panel/channel_modal.rs +++ b/crates/collab_ui/src/collab_panel/channel_modal.rs @@ -10,7 +10,6 @@ use gpui::{ WeakView, }; use picker::{Picker, PickerDelegate}; -use rpc::proto::channel_member; use std::sync::Arc; use ui::{prelude::*, Avatar, Checkbox, ContextMenu, ListItem, ListItemSpacing}; use util::TryFutureExt; @@ -359,10 +358,8 @@ impl PickerDelegate for ChannelModalDelegate { Some(proto::channel_member::Kind::Invitee) => { self.remove_member(selected_user.id, cx); } - Some(proto::channel_member::Kind::AncestorMember) | None => { - self.invite_member(selected_user, cx) - } Some(proto::channel_member::Kind::Member) => {} + None => self.invite_member(selected_user, cx), }, } } @@ -402,10 +399,6 @@ impl PickerDelegate for ChannelModalDelegate { .children( if request_status == Some(proto::channel_member::Kind::Invitee) { Some(Label::new("Invited")) - } else if membership.map(|m| m.kind) - == Some(channel_member::Kind::AncestorMember) - { - Some(Label::new("Parent")) } else { None }, @@ -563,16 +556,9 @@ impl ChannelModalDelegate { let Some(membership) = self.member_at_index(ix) else { return; }; - if membership.kind == proto::channel_member::Kind::AncestorMember { - return; - } let user_id = membership.user.id; let picker = cx.view().clone(); let context_menu = ContextMenu::build(cx, |mut menu, _cx| { - if membership.kind == channel_member::Kind::AncestorMember { - return menu.entry("Inherited membership", None, |_| {}); - }; - let role = membership.role; if role == ChannelRole::Admin || role == ChannelRole::Member { diff --git a/crates/rpc/proto/zed.proto b/crates/rpc/proto/zed.proto index f18ff5cf2e..5278bbba66 100644 --- a/crates/rpc/proto/zed.proto +++ b/crates/rpc/proto/zed.proto @@ -212,6 +212,7 @@ enum ErrorCode { Forbidden = 5; WrongReleaseChannel = 6; NeedsCla = 7; + NotARootChannel = 8; } message Test { @@ -1001,6 +1002,12 @@ message UpdateChannels { message UpdateUserChannels { repeated ChannelMessageId observed_channel_message_id = 1; repeated ChannelBufferVersion observed_channel_buffer_version = 2; + repeated ChannelMembership channel_memberships = 3; +} + +message ChannelMembership { + uint64 channel_id = 1; + ChannelRole role = 2; } message ChannelMessageId { @@ -1042,7 +1049,6 @@ message ChannelMember { enum Kind { Member = 0; Invitee = 1; - AncestorMember = 2; } } @@ -1149,7 +1155,7 @@ message GetChannelMessagesById { message MoveChannel { uint64 channel_id = 1; - optional uint64 to = 2; + uint64 to = 2; } message JoinChannelBuffer { @@ -1587,7 +1593,6 @@ message Channel { uint64 id = 1; string name = 2; ChannelVisibility visibility = 3; - ChannelRole role = 4; repeated uint64 parent_path = 5; }