From 783f05172b5361d96a1fff0c73dc0fa94b243ab2 Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Tue, 17 Oct 2023 15:40:23 -0600 Subject: [PATCH 01/12] Make sure guests join as guests --- crates/collab/src/db/queries/channels.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/crates/collab/src/db/queries/channels.rs b/crates/collab/src/db/queries/channels.rs index ee989b2ea068ff362adc635e7616b1c25a2de573..d64d97f2ad6dacfae040c159905706b43b4f8c41 100644 --- a/crates/collab/src/db/queries/channels.rs +++ b/crates/collab/src/db/queries/channels.rs @@ -135,8 +135,7 @@ impl Database { .most_public_ancestor_for_channel(channel_id, &*tx) .await? .unwrap_or(channel_id); - // TODO: change this back to Guest. - role = Some(ChannelRole::Member); + role = Some(ChannelRole::Guest); joined_channel_id = Some(channel_id_to_join); channel_member::Entity::insert(channel_member::ActiveModel { @@ -144,8 +143,7 @@ impl Database { channel_id: ActiveValue::Set(channel_id_to_join), user_id: ActiveValue::Set(user_id), accepted: ActiveValue::Set(true), - // TODO: change this back to Guest. - role: ActiveValue::Set(ChannelRole::Member), + role: ActiveValue::Set(ChannelRole::Guest), }) .exec(&*tx) .await?; From 72ed8a6dd2ae5f2b54a40433ab7fb03d27c07a13 Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Tue, 17 Oct 2023 19:03:02 -0600 Subject: [PATCH 02/12] Allow guests to chat --- crates/collab/src/db/queries/messages.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/crates/collab/src/db/queries/messages.rs b/crates/collab/src/db/queries/messages.rs index 06e954103d18c0916b2dc76ba474a06283a93cf9..de7334425facb79f99a10860d35493c49e398195 100644 --- a/crates/collab/src/db/queries/messages.rs +++ b/crates/collab/src/db/queries/messages.rs @@ -9,7 +9,7 @@ impl Database { user_id: UserId, ) -> Result<()> { self.transaction(|tx| async move { - self.check_user_is_channel_member(channel_id, user_id, &*tx) + self.check_user_is_channel_participant(channel_id, user_id, &*tx) .await?; channel_chat_participant::ActiveModel { id: ActiveValue::NotSet, @@ -77,7 +77,7 @@ impl Database { before_message_id: Option, ) -> Result> { self.transaction(|tx| async move { - self.check_user_is_channel_member(channel_id, user_id, &*tx) + self.check_user_is_channel_participant(channel_id, user_id, &*tx) .await?; let mut condition = @@ -125,6 +125,9 @@ impl Database { nonce: u128, ) -> Result<(MessageId, Vec, Vec)> { self.transaction(|tx| async move { + self.check_user_is_channel_participant(channel_id, user_id, &*tx) + .await?; + let mut rows = channel_chat_participant::Entity::find() .filter(channel_chat_participant::Column::ChannelId.eq(channel_id)) .stream(&*tx) From 70aed4a60571baf321afc842f845e3843f9eed7b Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Tue, 17 Oct 2023 22:48:44 -0600 Subject: [PATCH 03/12] Sync Role as part of channels Begin to fix guest notifications --- crates/channel/src/channel_store.rs | 38 +-- .../src/channel_store/channel_index.rs | 2 + crates/channel/src/channel_store_tests.rs | 2 +- crates/collab/src/db.rs | 2 +- crates/collab/src/db/ids.rs | 2 +- crates/collab/src/db/queries/buffers.rs | 4 +- crates/collab/src/db/queries/channels.rs | 107 ++++---- crates/collab/src/db/tests.rs | 8 +- crates/collab/src/db/tests/channel_tests.rs | 160 ++++++------ crates/collab/src/rpc.rs | 238 +++++++++++------- .../collab/src/tests/channel_buffer_tests.rs | 7 +- crates/collab/src/tests/channel_tests.rs | 5 +- crates/collab_ui/src/chat_panel.rs | 2 +- crates/collab_ui/src/collab_panel.rs | 8 +- crates/rpc/proto/zed.proto | 2 +- 15 files changed, 322 insertions(+), 265 deletions(-) diff --git a/crates/channel/src/channel_store.rs b/crates/channel/src/channel_store.rs index 9c80dcc2b742b07430a752286a2008bc5cfd05b2..de1f35bef9a958f88a81f37b4b2de61f0aaa0116 100644 --- a/crates/channel/src/channel_store.rs +++ b/crates/channel/src/channel_store.rs @@ -9,7 +9,7 @@ use db::RELEASE_CHANNEL; use futures::{channel::mpsc, future::Shared, Future, FutureExt, StreamExt}; use gpui::{AppContext, AsyncAppContext, Entity, ModelContext, ModelHandle, Task, WeakModelHandle}; use rpc::{ - proto::{self, ChannelEdge, ChannelPermission, ChannelRole, ChannelVisibility}, + proto::{self, ChannelEdge, ChannelVisibility}, TypedEnvelope, }; use serde_derive::{Deserialize, Serialize}; @@ -30,7 +30,6 @@ pub struct ChannelStore { channel_index: ChannelIndex, channel_invitations: Vec>, channel_participants: HashMap>>, - channels_with_admin_privileges: HashSet, outgoing_invites: HashSet<(ChannelId, UserId)>, update_channels_tx: mpsc::UnboundedSender, opened_buffers: HashMap>, @@ -50,6 +49,7 @@ pub struct Channel { pub id: ChannelId, pub name: String, pub visibility: proto::ChannelVisibility, + pub role: proto::ChannelRole, pub unseen_note_version: Option<(u64, clock::Global)>, pub unseen_message_id: Option, } @@ -164,7 +164,6 @@ impl ChannelStore { channel_invitations: Vec::default(), channel_index: ChannelIndex::default(), channel_participants: Default::default(), - channels_with_admin_privileges: Default::default(), outgoing_invites: Default::default(), opened_buffers: Default::default(), opened_chats: Default::default(), @@ -419,16 +418,11 @@ impl ChannelStore { .spawn(async move { task.await.map_err(|error| anyhow!("{}", error)) }) } - pub fn is_user_admin(&self, channel_id: ChannelId) -> bool { - self.channel_index.iter().any(|path| { - if let Some(ix) = path.iter().position(|id| *id == channel_id) { - path[..=ix] - .iter() - .any(|id| self.channels_with_admin_privileges.contains(id)) - } else { - false - } - }) + 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 } pub fn channel_participants(&self, channel_id: ChannelId) -> &[Arc] { @@ -469,10 +463,6 @@ impl ChannelStore { proto::UpdateChannels { channels: vec![channel], insert_edge: parent_edge, - channel_permissions: vec![ChannelPermission { - channel_id, - role: ChannelRole::Admin.into(), - }], ..Default::default() }, cx, @@ -881,7 +871,6 @@ impl ChannelStore { self.channel_index.clear(); self.channel_invitations.clear(); self.channel_participants.clear(); - self.channels_with_admin_privileges.clear(); self.channel_index.clear(); self.outgoing_invites.clear(); cx.notify(); @@ -927,6 +916,7 @@ impl ChannelStore { Arc::new(Channel { id: channel.id, visibility: channel.visibility(), + role: channel.role(), name: channel.name, unseen_note_version: None, unseen_message_id: None, @@ -947,8 +937,6 @@ impl ChannelStore { self.channel_index.delete_channels(&payload.delete_channels); self.channel_participants .retain(|channel_id, _| !payload.delete_channels.contains(channel_id)); - self.channels_with_admin_privileges - .retain(|channel_id| !payload.delete_channels.contains(channel_id)); for channel_id in &payload.delete_channels { let channel_id = *channel_id; @@ -992,16 +980,6 @@ impl ChannelStore { } } - for permission in payload.channel_permissions { - if permission.role() == proto::ChannelRole::Admin { - self.channels_with_admin_privileges - .insert(permission.channel_id); - } else { - self.channels_with_admin_privileges - .remove(&permission.channel_id); - } - } - cx.notify(); if payload.channel_participants.is_empty() { return None; diff --git a/crates/channel/src/channel_store/channel_index.rs b/crates/channel/src/channel_store/channel_index.rs index 36379a3942be2f2df3a24b70eb8833aabc214af2..54de15974ed50268be9bb9eaf1ed84dc80ca4d6e 100644 --- a/crates/channel/src/channel_store/channel_index.rs +++ b/crates/channel/src/channel_store/channel_index.rs @@ -125,6 +125,7 @@ impl<'a> ChannelPathsInsertGuard<'a> { if let Some(existing_channel) = self.channels_by_id.get_mut(&channel_proto.id) { let existing_channel = Arc::make_mut(existing_channel); existing_channel.visibility = channel_proto.visibility(); + existing_channel.role = channel_proto.role(); existing_channel.name = channel_proto.name; } else { self.channels_by_id.insert( @@ -132,6 +133,7 @@ impl<'a> ChannelPathsInsertGuard<'a> { Arc::new(Channel { id: channel_proto.id, visibility: channel_proto.visibility(), + role: channel_proto.role(), name: channel_proto.name, unseen_note_version: None, unseen_message_id: None, diff --git a/crates/channel/src/channel_store_tests.rs b/crates/channel/src/channel_store_tests.rs index 23f2e11a03e3fdd577412c5382ff724212e09e0f..6a9560f608c5f65bcdccd0bb80b8eee66652854c 100644 --- a/crates/channel/src/channel_store_tests.rs +++ b/crates/channel/src/channel_store_tests.rs @@ -376,7 +376,7 @@ fn assert_channels( ( depth, channel.name.to_string(), - store.is_user_admin(channel.id), + store.is_channel_admin(channel.id), ) }) .collect::>() diff --git a/crates/collab/src/db.rs b/crates/collab/src/db.rs index 08f78c685dc8a28392733816a8e0473c2d2ca63a..32cf5cb20a7fc5768437a52e6c2dc6f445235d6d 100644 --- a/crates/collab/src/db.rs +++ b/crates/collab/src/db.rs @@ -433,13 +433,13 @@ pub struct Channel { pub id: ChannelId, pub name: String, pub visibility: ChannelVisibility, + pub role: ChannelRole, } #[derive(Debug, PartialEq)] pub struct ChannelsForUser { pub channels: ChannelGraph, pub channel_participants: HashMap>, - pub channels_with_admin_privileges: HashSet, pub unseen_buffer_changes: Vec, pub channel_messages: Vec, } diff --git a/crates/collab/src/db/ids.rs b/crates/collab/src/db/ids.rs index f0de4c255edc7b13eb27656ceaccefb3c5e26c02..55aae7ed3b1397b37274a8f4f9df9f43df53b2c4 100644 --- a/crates/collab/src/db/ids.rs +++ b/crates/collab/src/db/ids.rs @@ -82,7 +82,7 @@ id_type!(UserId); id_type!(ChannelBufferCollaboratorId); id_type!(FlagId); -#[derive(Eq, PartialEq, Copy, Clone, Debug, EnumIter, DeriveActiveEnum, Default)] +#[derive(Eq, PartialEq, Copy, Clone, Debug, EnumIter, DeriveActiveEnum, Default, Hash)] #[sea_orm(rs_type = "String", db_type = "String(None)")] pub enum ChannelRole { #[sea_orm(string_value = "admin")] diff --git a/crates/collab/src/db/queries/buffers.rs b/crates/collab/src/db/queries/buffers.rs index 69f100e6b8351a5e88d8fbda94aeb325e899634e..1b8467c75a169c3d2c22df438a9c3e3ec4973842 100644 --- a/crates/collab/src/db/queries/buffers.rs +++ b/crates/collab/src/db/queries/buffers.rs @@ -16,7 +16,7 @@ impl Database { connection: ConnectionId, ) -> Result { self.transaction(|tx| async move { - self.check_user_is_channel_member(channel_id, user_id, &tx) + self.check_user_is_channel_participant(channel_id, user_id, &tx) .await?; let buffer = channel::Model { @@ -131,7 +131,7 @@ impl Database { for client_buffer in buffers { let channel_id = ChannelId::from_proto(client_buffer.channel_id); if self - .check_user_is_channel_member(channel_id, user_id, &*tx) + .check_user_is_channel_participant(channel_id, user_id, &*tx) .await .is_err() { diff --git a/crates/collab/src/db/queries/channels.rs b/crates/collab/src/db/queries/channels.rs index d64d97f2ad6dacfae040c159905706b43b4f8c41..65393e07f82682968c971c14225f9331573efe44 100644 --- a/crates/collab/src/db/queries/channels.rs +++ b/crates/collab/src/db/queries/channels.rs @@ -132,9 +132,12 @@ impl Database { && channel.as_ref().map(|c| c.visibility) == Some(ChannelVisibility::Public) { let channel_id_to_join = self - .most_public_ancestor_for_channel(channel_id, &*tx) + .public_path_to_channel_internal(channel_id, &*tx) .await? + .first() + .cloned() .unwrap_or(channel_id); + role = Some(ChannelRole::Guest); joined_channel_id = Some(channel_id_to_join); @@ -306,7 +309,8 @@ impl Database { self.transaction(move |tx| async move { let new_name = Self::sanitize_channel_name(new_name)?.to_string(); - self.check_user_is_channel_admin(channel_id, user_id, &*tx) + let role = self + .check_user_is_channel_admin(channel_id, user_id, &*tx) .await?; let channel = channel::ActiveModel { @@ -321,6 +325,7 @@ impl Database { id: channel.id, name: channel.name, visibility: channel.visibility, + role, }) }) .await @@ -398,6 +403,8 @@ impl Database { pub async fn get_channel_invites_for_user(&self, user_id: UserId) -> Result> { self.transaction(|tx| async move { + let mut role_for_channel: HashMap = HashMap::default(); + let channel_invites = channel_member::Entity::find() .filter( channel_member::Column::UserId @@ -407,14 +414,12 @@ impl Database { .all(&*tx) .await?; + for invite in channel_invites { + role_for_channel.insert(invite.channel_id, invite.role); + } + let channels = channel::Entity::find() - .filter( - channel::Column::Id.is_in( - channel_invites - .into_iter() - .map(|channel_member| channel_member.channel_id), - ), - ) + .filter(channel::Column::Id.is_in(role_for_channel.keys().cloned())) .all(&*tx) .await?; @@ -424,6 +429,7 @@ impl Database { id: channel.id, name: channel.name, visibility: channel.visibility, + role: role_for_channel[&channel.id], }) .collect(); @@ -458,19 +464,22 @@ impl Database { ) -> Result { self.transaction(|tx| async move { let tx = tx; - - let channel_membership = channel_member::Entity::find() - .filter( - channel_member::Column::UserId - .eq(user_id) - .and(channel_member::Column::ChannelId.eq(channel_id)) - .and(channel_member::Column::Accepted.eq(true)), - ) - .all(&*tx) + let role = self + .check_user_is_channel_participant(channel_id, user_id, &*tx) .await?; - self.get_user_channels(user_id, channel_membership, &tx) - .await + self.get_user_channels( + user_id, + vec![channel_member::Model { + id: Default::default(), + channel_id, + user_id, + role, + accepted: true, + }], + &tx, + ) + .await }) .await } @@ -509,7 +518,6 @@ impl Database { } let mut channels: Vec = Vec::new(); - let mut channels_with_admin_privileges: HashSet = HashSet::default(); let mut channels_to_remove: HashSet = HashSet::default(); let mut rows = channel::Entity::find() @@ -532,11 +540,8 @@ impl Database { id: channel.id, name: channel.name, visibility: channel.visibility, + role: role, }); - - if role == ChannelRole::Admin { - channels_with_admin_privileges.insert(channel.id); - } } drop(rows); @@ -618,7 +623,6 @@ impl Database { Ok(ChannelsForUser { channels: ChannelGraph { channels, edges }, channel_participants, - channels_with_admin_privileges, unseen_buffer_changes: channel_buffer_changes, channel_messages: unseen_messages, }) @@ -787,9 +791,10 @@ impl Database { channel_id: ChannelId, user_id: UserId, tx: &DatabaseTransaction, - ) -> Result<()> { - match self.channel_role_for_user(channel_id, user_id, tx).await? { - Some(ChannelRole::Admin) => Ok(()), + ) -> Result { + let role = self.channel_role_for_user(channel_id, user_id, tx).await?; + match role { + Some(ChannelRole::Admin) => Ok(role.unwrap()), Some(ChannelRole::Member) | Some(ChannelRole::Banned) | Some(ChannelRole::Guest) @@ -818,10 +823,11 @@ impl Database { channel_id: ChannelId, user_id: UserId, tx: &DatabaseTransaction, - ) -> Result<()> { - match self.channel_role_for_user(channel_id, user_id, tx).await? { + ) -> Result { + let role = self.channel_role_for_user(channel_id, user_id, tx).await?; + match role { Some(ChannelRole::Admin) | Some(ChannelRole::Member) | Some(ChannelRole::Guest) => { - Ok(()) + Ok(role.unwrap()) } Some(ChannelRole::Banned) | None => Err(anyhow!( "user is not a channel participant or channel does not exist" @@ -847,12 +853,26 @@ impl Database { Ok(row) } - pub async fn most_public_ancestor_for_channel( + // ordered from higher in tree to lower + // only considers one path to a channel + // includes the channel itself + pub async fn public_path_to_channel(&self, channel_id: ChannelId) -> Result> { + self.transaction(move |tx| async move { + Ok(self + .public_path_to_channel_internal(channel_id, &*tx) + .await?) + }) + .await + } + + // ordered from higher in tree to lower + // only considers one path to a channel + // includes the channel itself + pub async fn public_path_to_channel_internal( &self, channel_id: ChannelId, tx: &DatabaseTransaction, - ) -> Result> { - // Note: if there are many paths to a channel, this will return just one + ) -> Result> { let arbitary_path = channel_path::Entity::find() .filter(channel_path::Column::ChannelId.eq(channel_id)) .order_by(channel_path::Column::IdPath, sea_orm::Order::Desc) @@ -860,7 +880,7 @@ impl Database { .await?; let Some(path) = arbitary_path else { - return Ok(None); + return Ok(vec![]); }; let ancestor_ids: Vec = path @@ -882,13 +902,10 @@ impl Database { visible_channels.insert(row.id); } - for ancestor in ancestor_ids { - if visible_channels.contains(&ancestor) { - return Ok(Some(ancestor)); - } - } - - Ok(None) + Ok(ancestor_ids + .into_iter() + .filter(|id| visible_channels.contains(id)) + .collect()) } pub async fn channel_role_for_user( @@ -1059,7 +1076,8 @@ impl Database { /// Returns the channel with the given ID pub async fn get_channel(&self, channel_id: ChannelId, user_id: UserId) -> Result { self.transaction(|tx| async move { - self.check_user_is_channel_participant(channel_id, user_id, &*tx) + let role = self + .check_user_is_channel_participant(channel_id, user_id, &*tx) .await?; let channel = channel::Entity::find_by_id(channel_id).one(&*tx).await?; @@ -1070,6 +1088,7 @@ impl Database { Ok(Channel { id: channel.id, visibility: channel.visibility, + role, name: channel.name, }) }) diff --git a/crates/collab/src/db/tests.rs b/crates/collab/src/db/tests.rs index 99a605106eafab55f50b2b37a0a5a027b1cb9457..a53509a59f0a956ce7703dd59c082389f63ddd88 100644 --- a/crates/collab/src/db/tests.rs +++ b/crates/collab/src/db/tests.rs @@ -149,17 +149,21 @@ impl Drop for TestDb { } /// The second tuples are (channel_id, parent) -fn graph(channels: &[(ChannelId, &'static str)], edges: &[(ChannelId, ChannelId)]) -> ChannelGraph { +fn graph( + channels: &[(ChannelId, &'static str, ChannelRole)], + edges: &[(ChannelId, ChannelId)], +) -> ChannelGraph { let mut graph = ChannelGraph { channels: vec![], edges: vec![], }; - for (id, name) in channels { + for (id, name, role) in channels { graph.channels.push(Channel { id: *id, name: name.to_string(), visibility: ChannelVisibility::Members, + role: *role, }) } diff --git a/crates/collab/src/db/tests/channel_tests.rs b/crates/collab/src/db/tests/channel_tests.rs index 40842aff5c3bbd90fa7fc1e38297096e6c2e8bcc..a323f2919ec70e7ad6b3908755449ece3f0ee4e5 100644 --- a/crates/collab/src/db/tests/channel_tests.rs +++ b/crates/collab/src/db/tests/channel_tests.rs @@ -8,45 +8,20 @@ use crate::{ db::{ queries::channels::ChannelGraph, tests::{graph, TEST_RELEASE_CHANNEL}, - ChannelId, ChannelRole, Database, NewUserParams, RoomId, UserId, + ChannelId, ChannelRole, Database, NewUserParams, RoomId, ServerId, UserId, }, test_both_dbs, }; use std::sync::{ - atomic::{AtomicI32, Ordering}, + atomic::{AtomicI32, AtomicU32, Ordering}, Arc, }; test_both_dbs!(test_channels, test_channels_postgres, test_channels_sqlite); async fn test_channels(db: &Arc) { - let a_id = db - .create_user( - "user1@example.com", - false, - NewUserParams { - github_login: "user1".into(), - github_user_id: 5, - invite_count: 0, - }, - ) - .await - .unwrap() - .user_id; - - let b_id = db - .create_user( - "user2@example.com", - false, - NewUserParams { - github_login: "user2".into(), - github_user_id: 6, - invite_count: 0, - }, - ) - .await - .unwrap() - .user_id; + let a_id = new_test_user(db, "user1@example.com").await; + let b_id = new_test_user(db, "user2@example.com").await; let zed_id = db.create_root_channel("zed", a_id).await.unwrap(); @@ -91,13 +66,13 @@ async fn test_channels(db: &Arc) { result.channels, graph( &[ - (zed_id, "zed"), - (crdb_id, "crdb"), - (livestreaming_id, "livestreaming"), - (replace_id, "replace"), - (rust_id, "rust"), - (cargo_id, "cargo"), - (cargo_ra_id, "cargo-ra") + (zed_id, "zed", ChannelRole::Admin), + (crdb_id, "crdb", ChannelRole::Admin), + (livestreaming_id, "livestreaming", ChannelRole::Admin), + (replace_id, "replace", ChannelRole::Admin), + (rust_id, "rust", ChannelRole::Admin), + (cargo_id, "cargo", ChannelRole::Admin), + (cargo_ra_id, "cargo-ra", ChannelRole::Admin) ], &[ (crdb_id, zed_id), @@ -114,10 +89,10 @@ async fn test_channels(db: &Arc) { result.channels, graph( &[ - (zed_id, "zed"), - (crdb_id, "crdb"), - (livestreaming_id, "livestreaming"), - (replace_id, "replace") + (zed_id, "zed", ChannelRole::Member), + (crdb_id, "crdb", ChannelRole::Member), + (livestreaming_id, "livestreaming", ChannelRole::Member), + (replace_id, "replace", ChannelRole::Member) ], &[ (crdb_id, zed_id), @@ -142,10 +117,10 @@ async fn test_channels(db: &Arc) { result.channels, graph( &[ - (zed_id, "zed"), - (crdb_id, "crdb"), - (livestreaming_id, "livestreaming"), - (replace_id, "replace") + (zed_id, "zed", ChannelRole::Admin), + (crdb_id, "crdb", ChannelRole::Admin), + (livestreaming_id, "livestreaming", ChannelRole::Admin), + (replace_id, "replace", ChannelRole::Admin) ], &[ (crdb_id, zed_id), @@ -179,32 +154,8 @@ test_both_dbs!( async fn test_joining_channels(db: &Arc) { let owner_id = db.create_server("test").await.unwrap().0 as u32; - let user_1 = db - .create_user( - "user1@example.com", - false, - NewUserParams { - github_login: "user1".into(), - github_user_id: 5, - invite_count: 0, - }, - ) - .await - .unwrap() - .user_id; - let user_2 = db - .create_user( - "user2@example.com", - false, - NewUserParams { - github_login: "user2".into(), - github_user_id: 6, - invite_count: 0, - }, - ) - .await - .unwrap() - .user_id; + let user_1 = new_test_user(db, "user1@example.com").await; + let user_2 = new_test_user(db, "user2@example.com").await; let channel_1 = db.create_root_channel("channel_1", user_1).await.unwrap(); @@ -523,7 +474,11 @@ async fn test_db_channel_moving(db: &Arc) { pretty_assertions::assert_eq!( returned_channels, graph( - &[(livestreaming_dag_sub_id, "livestreaming_dag_sub")], + &[( + livestreaming_dag_sub_id, + "livestreaming_dag_sub", + ChannelRole::Admin + )], &[(livestreaming_dag_sub_id, livestreaming_id)] ) ); @@ -560,9 +515,17 @@ async fn test_db_channel_moving(db: &Arc) { returned_channels, graph( &[ - (livestreaming_id, "livestreaming"), - (livestreaming_dag_id, "livestreaming_dag"), - (livestreaming_dag_sub_id, "livestreaming_dag_sub"), + (livestreaming_id, "livestreaming", ChannelRole::Admin), + ( + livestreaming_dag_id, + "livestreaming_dag", + ChannelRole::Admin + ), + ( + livestreaming_dag_sub_id, + "livestreaming_dag_sub", + ChannelRole::Admin + ), ], &[ (livestreaming_id, gpui2_id), @@ -1080,15 +1043,48 @@ async fn test_user_joins_correct_channel(db: &Arc) { .unwrap(); let most_public = db - .transaction( - |tx| async move { db.most_public_ancestor_for_channel(vim_channel, &*tx).await }, - ) + .public_path_to_channel(vim_channel) .await - .unwrap(); + .unwrap() + .first() + .cloned(); assert_eq!(most_public, Some(zed_channel)) } +test_both_dbs!( + test_guest_access, + test_guest_access_postgres, + test_guest_access_sqlite +); + +async fn test_guest_access(db: &Arc) { + let server = db.create_server("test").await.unwrap(); + + let admin = new_test_user(db, "admin@example.com").await; + let guest = new_test_user(db, "guest@example.com").await; + let guest_connection = new_test_connection(server); + + let zed_channel = db.create_root_channel("zed", admin).await.unwrap(); + db.set_channel_visibility(zed_channel, crate::db::ChannelVisibility::Public, admin) + .await + .unwrap(); + + assert!(db + .join_channel_chat(zed_channel, guest_connection, guest) + .await + .is_err()); + + db.join_channel(zed_channel, guest, guest_connection, TEST_RELEASE_CHANNEL) + .await + .unwrap(); + + assert!(db + .join_channel_chat(zed_channel, guest_connection, guest) + .await + .is_ok()) +} + #[track_caller] fn assert_dag(actual: ChannelGraph, expected: &[(ChannelId, Option)]) { let mut actual_map: HashMap> = HashMap::default(); @@ -1130,3 +1126,11 @@ async fn new_test_user(db: &Arc, email: &str) -> UserId { .unwrap() .user_id } + +static TEST_CONNECTION_ID: AtomicU32 = AtomicU32::new(1); +fn new_test_connection(server: ServerId) -> ConnectionId { + ConnectionId { + id: TEST_CONNECTION_ID.fetch_add(1, Ordering::SeqCst), + owner_id: server.0 as u32, + } +} diff --git a/crates/collab/src/rpc.rs b/crates/collab/src/rpc.rs index 15ea3b24e133912d030ff15a850c5c3aae70bea8..aa8a4552b347df3f28806b5c7889ecac8a5de356 100644 --- a/crates/collab/src/rpc.rs +++ b/crates/collab/src/rpc.rs @@ -3,8 +3,8 @@ mod connection_pool; use crate::{ auth, db::{ - self, BufferId, ChannelId, ChannelVisibility, ChannelsForUser, Database, MessageId, - ProjectId, RoomId, ServerId, User, UserId, + self, BufferId, ChannelId, ChannelsForUser, Database, MessageId, ProjectId, RoomId, + ServerId, User, UserId, }, executor::Executor, AppState, Result, @@ -2206,14 +2206,13 @@ async fn create_channel( .create_channel(&request.name, parent_id, session.user_id) .await?; - let channel = proto::Channel { - id: id.to_proto(), - name: request.name, - visibility: proto::ChannelVisibility::Members as i32, - }; - response.send(proto::CreateChannelResponse { - channel: Some(channel.clone()), + channel: Some(proto::Channel { + id: id.to_proto(), + name: request.name, + visibility: proto::ChannelVisibility::Members as i32, + role: proto::ChannelRole::Admin.into(), + }), parent_id: request.parent_id, })?; @@ -2221,19 +2220,26 @@ async fn create_channel( return Ok(()); }; - let update = proto::UpdateChannels { - channels: vec![channel], - insert_edge: vec![ChannelEdge { - parent_id: parent_id.to_proto(), - channel_id: id.to_proto(), - }], - ..Default::default() - }; + let members: Vec = db + .get_channel_participant_details(parent_id, session.user_id) + .await? + .into_iter() + .filter(|member| { + member.role() == proto::ChannelRole::Admin + || member.role() == proto::ChannelRole::Member + }) + .collect(); - let user_ids_to_notify = db.get_channel_members(parent_id).await?; + let mut updates: HashMap = HashMap::default(); + + for member in members { + let user_id = UserId::from_proto(member.user_id); + let channels = db.get_channel_for_user(parent_id, user_id).await?; + updates.insert(user_id, build_initial_channels_update(channels, vec![])); + } let connection_pool = session.connection_pool().await; - for user_id in user_ids_to_notify { + for (user_id, update) in updates { for connection_id in connection_pool.user_connection_ids(user_id) { if user_id == session.user_id { continue; @@ -2297,6 +2303,7 @@ async fn invite_channel_member( id: channel.id.to_proto(), visibility: channel.visibility.into(), name: channel.name, + role: request.role().into(), }); for connection_id in session .connection_pool() @@ -2350,18 +2357,23 @@ async fn set_channel_visibility( .set_channel_visibility(channel_id, visibility, session.user_id) .await?; - let mut update = proto::UpdateChannels::default(); - update.channels.push(proto::Channel { - id: channel.id.to_proto(), - name: channel.name, - visibility: channel.visibility.into(), - }); - - let member_ids = db.get_channel_members(channel_id).await?; + let members = db + .get_channel_participant_details(channel_id, session.user_id) + .await?; let connection_pool = session.connection_pool().await; - for member_id in member_ids { - for connection_id in connection_pool.user_connection_ids(member_id) { + // TODO: notify people who were guests and are now not allowed. + for member in members { + for connection_id in connection_pool.user_connection_ids(UserId::from_proto(member.user_id)) + { + let mut update = proto::UpdateChannels::default(); + update.channels.push(proto::Channel { + id: channel.id.to_proto(), + name: channel.name.clone(), + visibility: channel.visibility.into(), + role: member.role.into(), + }); + session.peer.send(connection_id, update.clone())?; } } @@ -2387,13 +2399,17 @@ async fn set_channel_member_role( ) .await?; - let channel = db.get_channel(channel_id, session.user_id).await?; - let mut update = proto::UpdateChannels::default(); if channel_member.accepted { - update.channel_permissions.push(proto::ChannelPermission { - channel_id: channel.id.to_proto(), - role: request.role, + let channels = db.get_channel_for_user(channel_id, member_id).await?; + update = build_initial_channels_update(channels, vec![]); + } else { + let channel = db.get_channel(channel_id, session.user_id).await?; + update.channel_invitations.push(proto::Channel { + id: channel_id.to_proto(), + visibility: channel.visibility.into(), + name: channel.name, + role: request.role().into(), }); } @@ -2420,22 +2436,31 @@ async fn rename_channel( .rename_channel(channel_id, session.user_id, &request.name) .await?; - let channel = proto::Channel { - id: channel.id.to_proto(), - name: channel.name, - visibility: channel.visibility.into(), - }; response.send(proto::RenameChannelResponse { - channel: Some(channel.clone()), + channel: Some(proto::Channel { + id: channel.id.to_proto(), + name: channel.name.clone(), + visibility: channel.visibility.into(), + role: proto::ChannelRole::Admin.into(), + }), })?; - let mut update = proto::UpdateChannels::default(); - update.channels.push(channel); - let member_ids = db.get_channel_members(channel_id).await?; + let members = db + .get_channel_participant_details(channel_id, session.user_id) + .await?; let connection_pool = session.connection_pool().await; - for member_id in member_ids { - for connection_id in connection_pool.user_connection_ids(member_id) { + for member in members { + for connection_id in connection_pool.user_connection_ids(UserId::from_proto(member.user_id)) + { + let mut update = proto::UpdateChannels::default(); + update.channels.push(proto::Channel { + id: channel.id.to_proto(), + name: channel.name.clone(), + visibility: channel.visibility.into(), + role: member.role.into(), + }); + session.peer.send(connection_id, update.clone())?; } } @@ -2463,6 +2488,10 @@ async fn link_channel( id: channel.id.to_proto(), visibility: channel.visibility.into(), name: channel.name, + // TODO: not all these members should be able to see all those channels + // the channels in channels_to_send are from the admin point of view, + // but any public guests should only get updates about public channels. + role: todo!(), }) .collect(), insert_edge: channels_to_send.edges, @@ -2521,6 +2550,12 @@ async fn move_channel( let from_parent = ChannelId::from_proto(request.from); let to = ChannelId::from_proto(request.to); + let from_public_parent = db + .public_path_to_channel(from_parent) + .await? + .last() + .cloned(); + let channels_to_send = db .move_channel(session.user_id, channel_id, from_parent, to) .await?; @@ -2530,38 +2565,68 @@ async fn move_channel( return Ok(()); } - let members_from = db.get_channel_members(from_parent).await?; - let members_to = db.get_channel_members(to).await?; + let to_public_parent = db.public_path_to_channel(to).await?.last().cloned(); - let update = proto::UpdateChannels { - delete_edge: vec![proto::ChannelEdge { + let members_from = db + .get_channel_participant_details(from_parent, session.user_id) + .await? + .into_iter() + .filter(|member| { + member.role() == proto::ChannelRole::Admin || member.role() == proto::ChannelRole::Guest + }); + let members_to = db + .get_channel_participant_details(to, session.user_id) + .await? + .into_iter() + .filter(|member| { + member.role() == proto::ChannelRole::Admin || member.role() == proto::ChannelRole::Guest + }); + + let mut updates: HashMap = HashMap::default(); + + for member in members_to { + let user_id = UserId::from_proto(member.user_id); + let channels = db.get_channel_for_user(to, user_id).await?; + updates.insert(user_id, build_initial_channels_update(channels, vec![])); + } + + if let Some(to_public_parent) = to_public_parent { + // only notify guests of public channels (admins/members are notified by members_to above, and banned users don't care) + let public_members_to = db + .get_channel_participant_details(to, session.user_id) + .await? + .into_iter() + .filter(|member| member.role() == proto::ChannelRole::Guest); + + for member in public_members_to { + let user_id = UserId::from_proto(member.user_id); + if updates.contains_key(&user_id) { + continue; + } + let channels = db.get_channel_for_user(to_public_parent, user_id).await?; + updates.insert(user_id, build_initial_channels_update(channels, vec![])); + } + } + + for member in members_from { + let user_id = UserId::from_proto(member.user_id); + let update = updates + .entry(user_id) + .or_insert(proto::UpdateChannels::default()); + update.delete_edge.push(proto::ChannelEdge { channel_id: channel_id.to_proto(), parent_id: from_parent.to_proto(), - }], - ..Default::default() - }; - let connection_pool = session.connection_pool().await; - for member_id in members_from { - for connection_id in connection_pool.user_connection_ids(member_id) { - session.peer.send(connection_id, update.clone())?; - } + }) } - let update = proto::UpdateChannels { - channels: channels_to_send - .channels - .into_iter() - .map(|channel| proto::Channel { - id: channel.id.to_proto(), - visibility: channel.visibility.into(), - name: channel.name, - }) - .collect(), - insert_edge: channels_to_send.edges, - ..Default::default() - }; - for member_id in members_to { - for connection_id in connection_pool.user_connection_ids(member_id) { + if let Some(_from_public_parent) = from_public_parent { + // TODO: for each guest member of the old public parent + // delete the edge that they could see (from the from_public_parent down) + } + + let connection_pool = session.connection_pool().await; + for (user_id, update) in updates { + for connection_id in connection_pool.user_connection_ids(user_id) { session.peer.send(connection_id, update.clone())?; } } @@ -2628,6 +2693,7 @@ async fn channel_membership_updated( .map(|channel| proto::Channel { id: channel.id.to_proto(), visibility: channel.visibility.into(), + role: channel.role.into(), name: channel.name, }), ); @@ -2645,17 +2711,6 @@ async fn channel_membership_updated( participant_user_ids: user_ids.into_iter().map(UserId::to_proto).collect(), }), ); - update - .channel_permissions - .extend( - result - .channels_with_admin_privileges - .into_iter() - .map(|channel_id| proto::ChannelPermission { - channel_id: channel_id.to_proto(), - role: proto::ChannelRole::Admin.into(), - }), - ); session.peer.send(session.connection_id, update)?; Ok(()) } @@ -3149,6 +3204,7 @@ fn build_initial_channels_update( id: channel.id.to_proto(), name: channel.name, visibility: channel.visibility.into(), + role: channel.role.into(), }); } @@ -3165,24 +3221,12 @@ fn build_initial_channels_update( }); } - update - .channel_permissions - .extend( - channels - .channels_with_admin_privileges - .into_iter() - .map(|id| proto::ChannelPermission { - channel_id: id.to_proto(), - role: proto::ChannelRole::Admin.into(), - }), - ); - for channel in channel_invites { update.channel_invitations.push(proto::Channel { id: channel.id.to_proto(), name: channel.name, - // TODO: Visibility - visibility: ChannelVisibility::Public.into(), + visibility: channel.visibility.into(), + role: channel.role.into(), }); } diff --git a/crates/collab/src/tests/channel_buffer_tests.rs b/crates/collab/src/tests/channel_buffer_tests.rs index 14ae159ab80fd5b46359ef1183e7ea18478f3f08..f2ab2a9d094f9a41d08f38076d8d194011aa84cf 100644 --- a/crates/collab/src/tests/channel_buffer_tests.rs +++ b/crates/collab/src/tests/channel_buffer_tests.rs @@ -413,7 +413,7 @@ async fn test_channel_buffer_disconnect( channel_buffer_a.update(cx_a, |buffer, _| { assert_eq!( buffer.channel().as_ref(), - &channel(channel_id, "the-channel") + &channel(channel_id, "the-channel", proto::ChannelRole::Admin) ); assert!(!buffer.is_connected()); }); @@ -438,15 +438,16 @@ async fn test_channel_buffer_disconnect( channel_buffer_b.update(cx_b, |buffer, _| { assert_eq!( buffer.channel().as_ref(), - &channel(channel_id, "the-channel") + &channel(channel_id, "the-channel", proto::ChannelRole::Member) ); assert!(!buffer.is_connected()); }); } -fn channel(id: u64, name: &'static str) -> Channel { +fn channel(id: u64, name: &'static str, role: proto::ChannelRole) -> Channel { Channel { id, + role, name: name.to_string(), visibility: proto::ChannelVisibility::Members, unseen_note_version: None, diff --git a/crates/collab/src/tests/channel_tests.rs b/crates/collab/src/tests/channel_tests.rs index 1bb8c92ac80ac45b78ccff76de8f15c1468acaef..af4b99c4ef6fbe3bb4d61ea62944512bc2e309d0 100644 --- a/crates/collab/src/tests/channel_tests.rs +++ b/crates/collab/src/tests/channel_tests.rs @@ -152,6 +152,7 @@ async fn test_core_channels( }, ], ); + dbg!("-------"); let channel_c_id = client_a .channel_store() @@ -1295,7 +1296,7 @@ fn assert_channel_invitations( depth: 0, name: channel.name.clone(), id: channel.id, - user_is_admin: store.is_user_admin(channel.id), + user_is_admin: store.is_channel_admin(channel.id), }) .collect::>() }); @@ -1315,7 +1316,7 @@ fn assert_channels( depth, name: channel.name.clone(), id: channel.id, - user_is_admin: store.is_user_admin(channel.id), + user_is_admin: store.is_channel_admin(channel.id), }) .collect::>() }); diff --git a/crates/collab_ui/src/chat_panel.rs b/crates/collab_ui/src/chat_panel.rs index a8c4006cb8ce7c18e3a2eaa09eb1dafc98becd26..f0a6c96cedbe363101ebb676375f6b038021f718 100644 --- a/crates/collab_ui/src/chat_panel.rs +++ b/crates/collab_ui/src/chat_panel.rs @@ -360,7 +360,7 @@ impl ChatPanel { let is_admin = self .channel_store .read(cx) - .is_user_admin(active_chat.channel().id); + .is_channel_admin(active_chat.channel().id); let last_message = active_chat.message(ix.saturating_sub(1)); let this_message = active_chat.message(ix); let is_continuation = last_message.id != this_message.id diff --git a/crates/collab_ui/src/collab_panel.rs b/crates/collab_ui/src/collab_panel.rs index 2e68a1c939c8ba589f1c54a1c6b2abbca3ec8b49..dcebe35b26ae199b4c425c75824592bec2fc464b 100644 --- a/crates/collab_ui/src/collab_panel.rs +++ b/crates/collab_ui/src/collab_panel.rs @@ -2721,7 +2721,11 @@ impl CollabPanel { }, )); - if self.channel_store.read(cx).is_user_admin(path.channel_id()) { + if self + .channel_store + .read(cx) + .is_channel_admin(path.channel_id()) + { let parent_id = path.parent_id(); items.extend([ @@ -3160,7 +3164,7 @@ impl CollabPanel { fn rename_channel(&mut self, action: &RenameChannel, cx: &mut ViewContext) { let channel_store = self.channel_store.read(cx); - if !channel_store.is_user_admin(action.location.channel_id()) { + if !channel_store.is_channel_admin(action.location.channel_id()) { return; } if let Some(channel) = channel_store diff --git a/crates/rpc/proto/zed.proto b/crates/rpc/proto/zed.proto index 0ca10b381ae7cde3c76689f3946abc2a408911ef..20a47954a451a9ce94cc560a2e69c9a6b9bc6caa 100644 --- a/crates/rpc/proto/zed.proto +++ b/crates/rpc/proto/zed.proto @@ -970,7 +970,6 @@ message UpdateChannels { repeated Channel channel_invitations = 5; repeated uint64 remove_channel_invitations = 6; repeated ChannelParticipants channel_participants = 7; - repeated ChannelPermission channel_permissions = 8; repeated UnseenChannelMessage unseen_channel_messages = 9; repeated UnseenChannelBufferChange unseen_channel_buffer_changes = 10; } @@ -1568,6 +1567,7 @@ message Channel { uint64 id = 1; string name = 2; ChannelVisibility visibility = 3; + ChannelRole role = 4; } message Contact { From 0ce1ec5d15158a5ff83c4221b9d59325f032bef5 Mon Sep 17 00:00:00 2001 From: Mikayla Date: Wed, 18 Oct 2023 05:28:05 -0700 Subject: [PATCH 04/12] Restrict DAG-related functionality, but retain infrastructure for implementing symlinks --- crates/channel/src/channel_store_tests.rs | 16 +- crates/collab/src/db/queries/channels.rs | 52 ++- crates/collab/src/rpc.rs | 125 ++++---- crates/collab/src/tests/channel_tests.rs | 371 +++++++++++----------- crates/collab_ui/src/collab_panel.rs | 188 +++-------- crates/rpc/proto/zed.proto | 6 + 6 files changed, 351 insertions(+), 407 deletions(-) diff --git a/crates/channel/src/channel_store_tests.rs b/crates/channel/src/channel_store_tests.rs index 6a9560f608c5f65bcdccd0bb80b8eee66652854c..69c0cd37fc10f14e3bfe70d3fb8e41d05d76c9bf 100644 --- a/crates/channel/src/channel_store_tests.rs +++ b/crates/channel/src/channel_store_tests.rs @@ -19,17 +19,15 @@ fn test_update_channels(cx: &mut AppContext) { id: 1, name: "b".to_string(), visibility: proto::ChannelVisibility::Members as i32, + role: proto::ChannelRole::Admin.into(), }, proto::Channel { id: 2, name: "a".to_string(), visibility: proto::ChannelVisibility::Members as i32, + role: proto::ChannelRole::Member.into(), }, ], - channel_permissions: vec![proto::ChannelPermission { - channel_id: 1, - role: proto::ChannelRole::Admin.into(), - }], ..Default::default() }, cx, @@ -52,11 +50,13 @@ fn test_update_channels(cx: &mut AppContext) { id: 3, name: "x".to_string(), visibility: proto::ChannelVisibility::Members as i32, + role: proto::ChannelRole::Member.into(), }, proto::Channel { id: 4, name: "y".to_string(), visibility: proto::ChannelVisibility::Members as i32, + role: proto::ChannelRole::Member.into(), }, ], insert_edge: vec![ @@ -97,16 +97,19 @@ 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(), }, proto::Channel { id: 1, name: "b".to_string(), visibility: proto::ChannelVisibility::Members as i32, + role: proto::ChannelRole::Admin.into(), }, proto::Channel { id: 2, name: "c".to_string(), visibility: proto::ChannelVisibility::Members as i32, + role: proto::ChannelRole::Admin.into(), }, ], insert_edge: vec![ @@ -119,10 +122,6 @@ fn test_dangling_channel_paths(cx: &mut AppContext) { channel_id: 2, }, ], - channel_permissions: vec![proto::ChannelPermission { - channel_id: 0, - role: proto::ChannelRole::Admin.into(), - }], ..Default::default() }, cx, @@ -166,6 +165,7 @@ 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::Admin.into(), }], ..Default::default() }); diff --git a/crates/collab/src/db/queries/channels.rs b/crates/collab/src/db/queries/channels.rs index 65393e07f82682968c971c14225f9331573efe44..575f55fe0216c9c5040acb21bec41e8714269013 100644 --- a/crates/collab/src/db/queries/channels.rs +++ b/crates/collab/src/db/queries/channels.rs @@ -419,7 +419,7 @@ impl Database { } let channels = channel::Entity::find() - .filter(channel::Column::Id.is_in(role_for_channel.keys().cloned())) + .filter(channel::Column::Id.is_in(role_for_channel.keys().copied())) .all(&*tx) .await?; @@ -633,6 +633,36 @@ impl Database { .await } + pub async fn get_channel_members_and_roles( + &self, + id: ChannelId, + ) -> Result> { + self.transaction(|tx| async move { + #[derive(Copy, Clone, Debug, EnumIter, DeriveColumn)] + enum QueryUserIdsAndRoles { + UserId, + Role, + } + + let ancestor_ids = self.get_channel_ancestors(id, &*tx).await?; + let user_ids_and_roles = channel_member::Entity::find() + .distinct() + .filter( + channel_member::Column::ChannelId + .is_in(ancestor_ids.iter().copied()) + .and(channel_member::Column::Accepted.eq(true)), + ) + .select_only() + .column(channel_member::Column::UserId) + .column(channel_member::Column::Role) + .into_values::<_, QueryUserIdsAndRoles>() + .all(&*tx) + .await?; + Ok(user_ids_and_roles) + }) + .await + } + pub async fn set_channel_member_role( &self, channel_id: ChannelId, @@ -1138,9 +1168,6 @@ impl Database { to: ChannelId, ) -> Result { self.transaction(|tx| async move { - // Note that even with these maxed permissions, this linking operation - // is still insecure because you can't remove someone's permissions to a - // channel if they've linked the channel to one where they're an admin. self.check_user_is_channel_admin(channel, user, &*tx) .await?; @@ -1327,6 +1354,23 @@ impl Database { }) .await } + + pub async fn assert_root_channel(&self, channel: ChannelId) -> Result<()> { + self.transaction(|tx| async move { + let path = channel_path::Entity::find() + .filter(channel_path::Column::ChannelId.eq(channel)) + .one(&*tx) + .await? + .ok_or_else(|| anyhow!("no such channel found"))?; + + let mut id_parts = path.id_path.trim_matches('/').split('/'); + + (id_parts.next().is_some() && id_parts.next().is_none()) + .then_some(()) + .ok_or_else(|| anyhow!("channel is not a root channel").into()) + }) + .await + } } #[derive(Copy, Clone, Debug, EnumIter, DeriveColumn)] diff --git a/crates/collab/src/rpc.rs b/crates/collab/src/rpc.rs index aa8a4552b347df3f28806b5c7889ecac8a5de356..c7a2ba88b14d1637b249fe3b913fa85850554461 100644 --- a/crates/collab/src/rpc.rs +++ b/crates/collab/src/rpc.rs @@ -3,8 +3,8 @@ mod connection_pool; use crate::{ auth, db::{ - self, BufferId, ChannelId, ChannelsForUser, Database, MessageId, ProjectId, RoomId, - ServerId, User, UserId, + self, BufferId, ChannelId, ChannelRole, ChannelVisibility, ChannelsForUser, Database, + MessageId, ProjectId, RoomId, ServerId, User, UserId, }, executor::Executor, AppState, Result, @@ -38,8 +38,8 @@ use lazy_static::lazy_static; use prometheus::{register_int_gauge, IntGauge}; use rpc::{ proto::{ - self, Ack, AnyTypedEnvelope, ChannelEdge, EntityMessage, EnvelopedMessage, - LiveKitConnectionInfo, RequestMessage, UpdateChannelBufferCollaborators, + self, Ack, AnyTypedEnvelope, EntityMessage, EnvelopedMessage, LiveKitConnectionInfo, + RequestMessage, UpdateChannelBufferCollaborators, }, Connection, ConnectionId, Peer, Receipt, TypedEnvelope, }; @@ -2366,15 +2366,18 @@ async fn set_channel_visibility( for member in members { for connection_id in connection_pool.user_connection_ids(UserId::from_proto(member.user_id)) { - let mut update = proto::UpdateChannels::default(); - update.channels.push(proto::Channel { - id: channel.id.to_proto(), - name: channel.name.clone(), - visibility: channel.visibility.into(), - role: member.role.into(), - }); - - session.peer.send(connection_id, update.clone())?; + session.peer.send( + connection_id, + proto::UpdateChannels { + channels: vec![proto::Channel { + id: channel.id.to_proto(), + name: channel.name.clone(), + visibility: channel.visibility.into(), + role: member.role.into(), + }], + ..Default::default() + }, + )?; } } @@ -2468,6 +2471,8 @@ async fn rename_channel( Ok(()) } +// TODO: Implement in terms of symlinks +// Current behavior of this is more like 'Move root channel' async fn link_channel( request: proto::LinkChannel, response: Response, @@ -2476,30 +2481,46 @@ async fn link_channel( let db = session.db().await; let channel_id = ChannelId::from_proto(request.channel_id); let to = ChannelId::from_proto(request.to); - let channels_to_send = db.link_channel(session.user_id, channel_id, to).await?; - let members = db.get_channel_members(to).await?; + // TODO: Remove this restriction once we have symlinks + db.assert_root_channel(channel_id).await?; + + let channels_to_send = db.link_channel(session.user_id, channel_id, to).await?; + let members = db.get_channel_members_and_roles(to).await?; let connection_pool = session.connection_pool().await; - let update = proto::UpdateChannels { - channels: channels_to_send - .channels - .into_iter() - .map(|channel| proto::Channel { - id: channel.id.to_proto(), - visibility: channel.visibility.into(), - name: channel.name, - // TODO: not all these members should be able to see all those channels - // the channels in channels_to_send are from the admin point of view, - // but any public guests should only get updates about public channels. - role: todo!(), - }) - .collect(), - insert_edge: channels_to_send.edges, - ..Default::default() - }; - for member_id in members { + + for (member_id, role) in members { + let build_channel_proto = |channel: &db::Channel| proto::Channel { + id: channel.id.to_proto(), + visibility: channel.visibility.into(), + name: channel.name.clone(), + role: role.into(), + }; + for connection_id in connection_pool.user_connection_ids(member_id) { - session.peer.send(connection_id, update.clone())?; + let channels: Vec<_> = if role == ChannelRole::Guest { + channels_to_send + .channels + .iter() + .filter(|channel| channel.visibility != ChannelVisibility::Public) + .map(build_channel_proto) + .collect() + } else { + channels_to_send + .channels + .iter() + .map(build_channel_proto) + .collect() + }; + + session.peer.send( + connection_id, + proto::UpdateChannels { + channels, + insert_edge: channels_to_send.edges.clone(), + ..Default::default() + }, + )?; } } @@ -2508,36 +2529,13 @@ async fn link_channel( Ok(()) } +// TODO: Implement in terms of symlinks async fn unlink_channel( - request: proto::UnlinkChannel, - response: Response, - session: Session, + _request: proto::UnlinkChannel, + _response: Response, + _session: Session, ) -> Result<()> { - let db = session.db().await; - let channel_id = ChannelId::from_proto(request.channel_id); - let from = ChannelId::from_proto(request.from); - - db.unlink_channel(session.user_id, channel_id, from).await?; - - let members = db.get_channel_members(from).await?; - - let update = proto::UpdateChannels { - delete_edge: vec![proto::ChannelEdge { - channel_id: channel_id.to_proto(), - parent_id: from.to_proto(), - }], - ..Default::default() - }; - let connection_pool = session.connection_pool().await; - for member_id in members { - for connection_id in connection_pool.user_connection_ids(member_id) { - session.peer.send(connection_id, update.clone())?; - } - } - - response.send(Ack {})?; - - Ok(()) + Err(anyhow!("unimplemented").into()) } async fn move_channel( @@ -2554,7 +2552,7 @@ async fn move_channel( .public_path_to_channel(from_parent) .await? .last() - .cloned(); + .copied(); let channels_to_send = db .move_channel(session.user_id, channel_id, from_parent, to) @@ -2574,6 +2572,7 @@ async fn move_channel( .filter(|member| { member.role() == proto::ChannelRole::Admin || member.role() == proto::ChannelRole::Guest }); + let members_to = db .get_channel_participant_details(to, session.user_id) .await? diff --git a/crates/collab/src/tests/channel_tests.rs b/crates/collab/src/tests/channel_tests.rs index af4b99c4ef6fbe3bb4d61ea62944512bc2e309d0..f982f05ee325bac3afeb811f6e9509f55b002611 100644 --- a/crates/collab/src/tests/channel_tests.rs +++ b/crates/collab/src/tests/channel_tests.rs @@ -1031,14 +1031,14 @@ async fn test_invite_access( async fn test_channel_moving( deterministic: Arc, cx_a: &mut TestAppContext, - cx_b: &mut TestAppContext, - cx_c: &mut TestAppContext, + _cx_b: &mut TestAppContext, + _cx_c: &mut TestAppContext, ) { deterministic.forbid_parking(); let mut server = TestServer::start(&deterministic).await; let client_a = server.create_client(cx_a, "user_a").await; - let client_b = server.create_client(cx_b, "user_b").await; - let client_c = server.create_client(cx_c, "user_c").await; + // let client_b = server.create_client(cx_b, "user_b").await; + // let client_c = server.create_client(cx_c, "user_c").await; let channels = server .make_channel_tree( @@ -1091,187 +1091,188 @@ async fn test_channel_moving( ], ); - client_a - .channel_store() - .update(cx_a, |channel_store, cx| { - channel_store.link_channel(channel_d_id, channel_c_id, cx) - }) - .await - .unwrap(); - - // Current shape for A: - // /------\ - // a - b -- c -- d - assert_channels_list_shape( - client_a.channel_store(), - cx_a, - &[ - (channel_a_id, 0), - (channel_b_id, 1), - (channel_c_id, 2), - (channel_d_id, 3), - (channel_d_id, 2), - ], - ); - - let b_channels = server - .make_channel_tree( - &[ - ("channel-mu", None), - ("channel-gamma", Some("channel-mu")), - ("channel-epsilon", Some("channel-mu")), - ], - (&client_b, cx_b), - ) - .await; - let channel_mu_id = b_channels[0]; - let channel_ga_id = b_channels[1]; - let channel_ep_id = b_channels[2]; - - // Current shape for B: - // /- ep - // mu -- ga - assert_channels_list_shape( - client_b.channel_store(), - cx_b, - &[(channel_mu_id, 0), (channel_ep_id, 1), (channel_ga_id, 1)], - ); - - client_a - .add_admin_to_channel((&client_b, cx_b), channel_b_id, cx_a) - .await; - - // Current shape for B: - // /- ep - // mu -- ga - // /---------\ - // b -- c -- d - assert_channels_list_shape( - client_b.channel_store(), - cx_b, - &[ - // New channels from a - (channel_b_id, 0), - (channel_c_id, 1), - (channel_d_id, 2), - (channel_d_id, 1), - // B's old channels - (channel_mu_id, 0), - (channel_ep_id, 1), - (channel_ga_id, 1), - ], - ); - - client_b - .add_admin_to_channel((&client_c, cx_c), channel_ep_id, cx_b) - .await; - - // Current shape for C: - // - ep - assert_channels_list_shape(client_c.channel_store(), cx_c, &[(channel_ep_id, 0)]); - - client_b - .channel_store() - .update(cx_b, |channel_store, cx| { - channel_store.link_channel(channel_b_id, channel_ep_id, cx) - }) - .await - .unwrap(); - - // Current shape for B: - // /---------\ - // /- ep -- b -- c -- d - // mu -- ga - assert_channels_list_shape( - client_b.channel_store(), - cx_b, - &[ - (channel_mu_id, 0), - (channel_ep_id, 1), - (channel_b_id, 2), - (channel_c_id, 3), - (channel_d_id, 4), - (channel_d_id, 3), - (channel_ga_id, 1), - ], - ); - - // Current shape for C: - // /---------\ - // ep -- b -- c -- d - assert_channels_list_shape( - client_c.channel_store(), - cx_c, - &[ - (channel_ep_id, 0), - (channel_b_id, 1), - (channel_c_id, 2), - (channel_d_id, 3), - (channel_d_id, 2), - ], - ); - - client_b - .channel_store() - .update(cx_b, |channel_store, cx| { - channel_store.link_channel(channel_ga_id, channel_b_id, cx) - }) - .await - .unwrap(); - - // Current shape for B: - // /---------\ - // /- ep -- b -- c -- d - // / \ - // mu ---------- ga - assert_channels_list_shape( - client_b.channel_store(), - cx_b, - &[ - (channel_mu_id, 0), - (channel_ep_id, 1), - (channel_b_id, 2), - (channel_c_id, 3), - (channel_d_id, 4), - (channel_d_id, 3), - (channel_ga_id, 3), - (channel_ga_id, 1), - ], - ); - - // Current shape for A: - // /------\ - // a - b -- c -- d - // \-- ga - assert_channels_list_shape( - client_a.channel_store(), - cx_a, - &[ - (channel_a_id, 0), - (channel_b_id, 1), - (channel_c_id, 2), - (channel_d_id, 3), - (channel_d_id, 2), - (channel_ga_id, 2), - ], - ); - - // Current shape for C: - // /-------\ - // ep -- b -- c -- d - // \-- ga - assert_channels_list_shape( - client_c.channel_store(), - cx_c, - &[ - (channel_ep_id, 0), - (channel_b_id, 1), - (channel_c_id, 2), - (channel_d_id, 3), - (channel_d_id, 2), - (channel_ga_id, 2), - ], - ); + // TODO: Restore this test once we have a way to make channel symlinks + // client_a + // .channel_store() + // .update(cx_a, |channel_store, cx| { + // channel_store.link_channel(channel_d_id, channel_c_id, cx) + // }) + // .await + // .unwrap(); + + // // Current shape for A: + // // /------\ + // // a - b -- c -- d + // assert_channels_list_shape( + // client_a.channel_store(), + // cx_a, + // &[ + // (channel_a_id, 0), + // (channel_b_id, 1), + // (channel_c_id, 2), + // (channel_d_id, 3), + // (channel_d_id, 2), + // ], + // ); + // + // let b_channels = server + // .make_channel_tree( + // &[ + // ("channel-mu", None), + // ("channel-gamma", Some("channel-mu")), + // ("channel-epsilon", Some("channel-mu")), + // ], + // (&client_b, cx_b), + // ) + // .await; + // let channel_mu_id = b_channels[0]; + // let channel_ga_id = b_channels[1]; + // let channel_ep_id = b_channels[2]; + + // // Current shape for B: + // // /- ep + // // mu -- ga + // assert_channels_list_shape( + // client_b.channel_store(), + // cx_b, + // &[(channel_mu_id, 0), (channel_ep_id, 1), (channel_ga_id, 1)], + // ); + + // client_a + // .add_admin_to_channel((&client_b, cx_b), channel_b_id, cx_a) + // .await; + + // // Current shape for B: + // // /- ep + // // mu -- ga + // // /---------\ + // // b -- c -- d + // assert_channels_list_shape( + // client_b.channel_store(), + // cx_b, + // &[ + // // New channels from a + // (channel_b_id, 0), + // (channel_c_id, 1), + // (channel_d_id, 2), + // (channel_d_id, 1), + // // B's old channels + // (channel_mu_id, 0), + // (channel_ep_id, 1), + // (channel_ga_id, 1), + // ], + // ); + + // client_b + // .add_admin_to_channel((&client_c, cx_c), channel_ep_id, cx_b) + // .await; + + // // Current shape for C: + // // - ep + // assert_channels_list_shape(client_c.channel_store(), cx_c, &[(channel_ep_id, 0)]); + + // client_b + // .channel_store() + // .update(cx_b, |channel_store, cx| { + // channel_store.link_channel(channel_b_id, channel_ep_id, cx) + // }) + // .await + // .unwrap(); + + // // Current shape for B: + // // /---------\ + // // /- ep -- b -- c -- d + // // mu -- ga + // assert_channels_list_shape( + // client_b.channel_store(), + // cx_b, + // &[ + // (channel_mu_id, 0), + // (channel_ep_id, 1), + // (channel_b_id, 2), + // (channel_c_id, 3), + // (channel_d_id, 4), + // (channel_d_id, 3), + // (channel_ga_id, 1), + // ], + // ); + + // // Current shape for C: + // // /---------\ + // // ep -- b -- c -- d + // assert_channels_list_shape( + // client_c.channel_store(), + // cx_c, + // &[ + // (channel_ep_id, 0), + // (channel_b_id, 1), + // (channel_c_id, 2), + // (channel_d_id, 3), + // (channel_d_id, 2), + // ], + // ); + + // client_b + // .channel_store() + // .update(cx_b, |channel_store, cx| { + // channel_store.link_channel(channel_ga_id, channel_b_id, cx) + // }) + // .await + // .unwrap(); + + // // Current shape for B: + // // /---------\ + // // /- ep -- b -- c -- d + // // / \ + // // mu ---------- ga + // assert_channels_list_shape( + // client_b.channel_store(), + // cx_b, + // &[ + // (channel_mu_id, 0), + // (channel_ep_id, 1), + // (channel_b_id, 2), + // (channel_c_id, 3), + // (channel_d_id, 4), + // (channel_d_id, 3), + // (channel_ga_id, 3), + // (channel_ga_id, 1), + // ], + // ); + + // // Current shape for A: + // // /------\ + // // a - b -- c -- d + // // \-- ga + // assert_channels_list_shape( + // client_a.channel_store(), + // cx_a, + // &[ + // (channel_a_id, 0), + // (channel_b_id, 1), + // (channel_c_id, 2), + // (channel_d_id, 3), + // (channel_d_id, 2), + // (channel_ga_id, 2), + // ], + // ); + + // // Current shape for C: + // // /-------\ + // // ep -- b -- c -- d + // // \-- ga + // assert_channels_list_shape( + // client_c.channel_store(), + // cx_c, + // &[ + // (channel_ep_id, 0), + // (channel_b_id, 1), + // (channel_c_id, 2), + // (channel_d_id, 3), + // (channel_d_id, 2), + // (channel_ga_id, 2), + // ], + // ); } #[derive(Debug, PartialEq)] diff --git a/crates/collab_ui/src/collab_panel.rs b/crates/collab_ui/src/collab_panel.rs index dcebe35b26ae199b4c425c75824592bec2fc464b..70ea87cfdd2e31789743b16f848dd516ed5e413b 100644 --- a/crates/collab_ui/src/collab_panel.rs +++ b/crates/collab_ui/src/collab_panel.rs @@ -120,22 +120,11 @@ struct StartLinkChannelFor { parent_id: Option, } -#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] -struct LinkChannel { - to: ChannelId, -} - #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] struct MoveChannel { to: ChannelId, } -#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] -struct UnlinkChannel { - channel_id: ChannelId, - parent_id: ChannelId, -} - type DraggedChannel = (Channel, Option); actions!( @@ -147,8 +136,7 @@ actions!( CollapseSelectedChannel, ExpandSelectedChannel, StartMoveChannel, - StartLinkChannel, - MoveOrLinkToSelected, + MoveSelected, InsertSpace, ] ); @@ -166,11 +154,8 @@ impl_actions!( JoinChannelCall, JoinChannelChat, CopyChannelLink, - LinkChannel, StartMoveChannelFor, - StartLinkChannelFor, MoveChannel, - UnlinkChannel, ToggleSelectedIx ] ); @@ -185,7 +170,7 @@ struct ChannelMoveClipboard { #[derive(Debug, Copy, Clone, PartialEq, Eq)] enum ClipboardIntent { Move, - Link, + // Link, } const COLLABORATION_PANEL_KEY: &'static str = "CollaborationPanel"; @@ -238,18 +223,6 @@ pub fn init(cx: &mut AppContext) { }, ); - cx.add_action( - |panel: &mut CollabPanel, - action: &StartLinkChannelFor, - _: &mut ViewContext| { - panel.channel_clipboard = Some(ChannelMoveClipboard { - channel_id: action.channel_id, - parent_id: action.parent_id, - intent: ClipboardIntent::Link, - }) - }, - ); - cx.add_action( |panel: &mut CollabPanel, _: &StartMoveChannel, _: &mut ViewContext| { if let Some((_, path)) = panel.selected_channel() { @@ -263,86 +236,51 @@ pub fn init(cx: &mut AppContext) { ); cx.add_action( - |panel: &mut CollabPanel, _: &StartLinkChannel, _: &mut ViewContext| { - if let Some((_, path)) = panel.selected_channel() { - panel.channel_clipboard = Some(ChannelMoveClipboard { - channel_id: path.channel_id(), - parent_id: path.parent_id(), - intent: ClipboardIntent::Link, - }) - } - }, - ); - - cx.add_action( - |panel: &mut CollabPanel, _: &MoveOrLinkToSelected, cx: &mut ViewContext| { + |panel: &mut CollabPanel, _: &MoveSelected, cx: &mut ViewContext| { let clipboard = panel.channel_clipboard.take(); if let Some(((selected_channel, _), clipboard)) = panel.selected_channel().zip(clipboard) { match clipboard.intent { - ClipboardIntent::Move if clipboard.parent_id.is_some() => { - let parent_id = clipboard.parent_id.unwrap(); - panel.channel_store.update(cx, |channel_store, cx| { - channel_store - .move_channel( - clipboard.channel_id, - parent_id, - selected_channel.id, - cx, - ) - .detach_and_log_err(cx) - }) - } - _ => panel.channel_store.update(cx, |channel_store, cx| { - channel_store - .link_channel(clipboard.channel_id, selected_channel.id, cx) - .detach_and_log_err(cx) + ClipboardIntent::Move => panel.channel_store.update(cx, |channel_store, cx| { + match clipboard.parent_id { + Some(parent_id) => channel_store.move_channel( + clipboard.channel_id, + parent_id, + selected_channel.id, + cx, + ), + None => channel_store.link_channel( + clipboard.channel_id, + selected_channel.id, + cx, + ), + } + .detach_and_log_err(cx) }), } } }, ); - cx.add_action( - |panel: &mut CollabPanel, action: &LinkChannel, cx: &mut ViewContext| { - if let Some(clipboard) = panel.channel_clipboard.take() { - panel.channel_store.update(cx, |channel_store, cx| { - channel_store - .link_channel(clipboard.channel_id, action.to, cx) - .detach_and_log_err(cx) - }) - } - }, - ); - cx.add_action( |panel: &mut CollabPanel, action: &MoveChannel, cx: &mut ViewContext| { if let Some(clipboard) = panel.channel_clipboard.take() { panel.channel_store.update(cx, |channel_store, cx| { - if let Some(parent) = clipboard.parent_id { - channel_store - .move_channel(clipboard.channel_id, parent, action.to, cx) - .detach_and_log_err(cx) - } else { - channel_store - .link_channel(clipboard.channel_id, action.to, cx) - .detach_and_log_err(cx) + match clipboard.parent_id { + Some(parent_id) => channel_store.move_channel( + clipboard.channel_id, + parent_id, + action.to, + cx, + ), + None => channel_store.link_channel(clipboard.channel_id, action.to, cx), } + .detach_and_log_err(cx) }) } }, ); - - cx.add_action( - |panel: &mut CollabPanel, action: &UnlinkChannel, cx: &mut ViewContext| { - panel.channel_store.update(cx, |channel_store, cx| { - channel_store - .unlink_channel(action.channel_id, action.parent_id, cx) - .detach_and_log_err(cx) - }) - }, - ); } #[derive(Debug)] @@ -2235,33 +2173,23 @@ impl CollabPanel { this.deploy_channel_context_menu(Some(e.position), &path, ix, cx); } }) - .on_up(MouseButton::Left, move |e, this, cx| { + .on_up(MouseButton::Left, move |_, this, cx| { if let Some((_, dragged_channel)) = cx .global::>() .currently_dragged::(cx.window()) { - if e.modifiers.alt { - this.channel_store.update(cx, |channel_store, cx| { - channel_store - .link_channel(dragged_channel.0.id, channel_id, cx) - .detach_and_log_err(cx) - }) - } else { - this.channel_store.update(cx, |channel_store, cx| { - match dragged_channel.1 { - Some(parent_id) => channel_store.move_channel( - dragged_channel.0.id, - parent_id, - channel_id, - cx, - ), - None => { - channel_store.link_channel(dragged_channel.0.id, channel_id, cx) - } - } - .detach_and_log_err(cx) - }) - } + this.channel_store.update(cx, |channel_store, cx| { + match dragged_channel.1 { + Some(parent_id) => channel_store.move_channel( + dragged_channel.0.id, + parent_id, + channel_id, + cx, + ), + None => channel_store.link_channel(dragged_channel.0.id, channel_id, cx), + } + .detach_and_log_err(cx) + }) } }) .on_move({ @@ -2288,18 +2216,10 @@ impl CollabPanel { }) .as_draggable( (channel.clone(), path.parent_id()), - move |modifiers, (channel, _), cx: &mut ViewContext| { + move |_, (channel, _), cx: &mut ViewContext| { let theme = &theme::current(cx).collab_panel; Flex::::row() - .with_children(modifiers.alt.then(|| { - Svg::new("icons/plus.svg") - .with_color(theme.channel_hash.color) - .constrained() - .with_width(theme.channel_hash.width) - .aligned() - .left() - })) .with_child( Svg::new("icons/hash.svg") .with_color(theme.channel_hash.color) @@ -2743,19 +2663,6 @@ impl CollabPanel { }, ), ContextMenuItem::Separator, - ]); - - if let Some(parent_id) = parent_id { - items.push(ContextMenuItem::action( - "Unlink from parent", - UnlinkChannel { - channel_id: path.channel_id(), - parent_id, - }, - )); - } - - items.extend([ ContextMenuItem::action( "Move this channel", StartMoveChannelFor { @@ -2763,13 +2670,6 @@ impl CollabPanel { parent_id, }, ), - ContextMenuItem::action( - "Link this channel", - StartLinkChannelFor { - channel_id: path.channel_id(), - parent_id, - }, - ), ]); if let Some(channel_name) = channel_name { @@ -2780,12 +2680,6 @@ impl CollabPanel { to: path.channel_id(), }, )); - items.push(ContextMenuItem::action( - format!("Link '#{}' here", channel_name), - LinkChannel { - to: path.channel_id(), - }, - )); } items.extend([ diff --git a/crates/rpc/proto/zed.proto b/crates/rpc/proto/zed.proto index 20a47954a451a9ce94cc560a2e69c9a6b9bc6caa..1bf54dedb70be5cdffa6a89d524a50f4d3956407 100644 --- a/crates/rpc/proto/zed.proto +++ b/crates/rpc/proto/zed.proto @@ -970,10 +970,16 @@ message UpdateChannels { repeated Channel channel_invitations = 5; repeated uint64 remove_channel_invitations = 6; repeated ChannelParticipants channel_participants = 7; + //repeated ChannelRoles channel_roles = 8; repeated UnseenChannelMessage unseen_channel_messages = 9; repeated UnseenChannelBufferChange unseen_channel_buffer_changes = 10; } +//message ChannelRoles { +// ChannelRole role = 1; +// uint64 channel_id = 2; +//} + message UnseenChannelMessage { uint64 channel_id = 1; uint64 message_id = 2; From 2b114635678d0176fcba5ef53c28bbf8a44e15b6 Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Wed, 18 Oct 2023 15:43:26 -0600 Subject: [PATCH 05/12] Fix notifications on channel changes --- crates/channel/src/channel_store.rs | 7 + crates/collab/src/db.rs | 17 ++ crates/collab/src/db/ids.rs | 16 + crates/collab/src/db/queries/channels.rs | 360 +++++++++++++++-------- crates/collab/src/rpc.rs | 225 ++++++-------- crates/collab/src/tests/channel_tests.rs | 205 +++++++++++++ 6 files changed, 583 insertions(+), 247 deletions(-) diff --git a/crates/channel/src/channel_store.rs b/crates/channel/src/channel_store.rs index de1f35bef9a958f88a81f37b4b2de61f0aaa0116..4891b5a18ede737c43836d2dd401c79a05844561 100644 --- a/crates/channel/src/channel_store.rs +++ b/crates/channel/src/channel_store.rs @@ -940,6 +940,13 @@ impl ChannelStore { for channel_id in &payload.delete_channels { let channel_id = *channel_id; + if payload + .channels + .iter() + .any(|channel| channel.id == channel_id) + { + continue; + } if let Some(OpenedModelHandle::Open(buffer)) = self.opened_buffers.remove(&channel_id) { diff --git a/crates/collab/src/db.rs b/crates/collab/src/db.rs index 32cf5cb20a7fc5768437a52e6c2dc6f445235d6d..4d73d27a4747e84b435a5b49ecc2aa3c33637a26 100644 --- a/crates/collab/src/db.rs +++ b/crates/collab/src/db.rs @@ -436,6 +436,23 @@ pub struct Channel { pub role: ChannelRole, } +#[derive(Debug, PartialEq, Eq, Hash)] +pub struct ChannelMember { + pub role: ChannelRole, + pub user_id: UserId, + pub kind: proto::channel_member::Kind, +} + +impl ChannelMember { + pub fn to_proto(&self) -> proto::ChannelMember { + proto::ChannelMember { + role: self.role.into(), + user_id: self.user_id.to_proto(), + kind: self.kind.into(), + } + } +} + #[derive(Debug, PartialEq)] pub struct ChannelsForUser { pub channels: ChannelGraph, diff --git a/crates/collab/src/db/ids.rs b/crates/collab/src/db/ids.rs index 55aae7ed3b1397b37274a8f4f9df9f43df53b2c4..4c6e7116eeb1f81ad2eb06d59ddc432702ade793 100644 --- a/crates/collab/src/db/ids.rs +++ b/crates/collab/src/db/ids.rs @@ -114,6 +114,22 @@ impl ChannelRole { other } } + + pub fn can_see_all_descendants(&self) -> bool { + use ChannelRole::*; + match self { + Admin | Member => true, + Guest | Banned => false, + } + } + + pub fn can_only_see_public_descendants(&self) -> bool { + use ChannelRole::*; + match self { + Guest => true, + Admin | Member | Banned => false, + } + } } impl From for ChannelRole { diff --git a/crates/collab/src/db/queries/channels.rs b/crates/collab/src/db/queries/channels.rs index 575f55fe0216c9c5040acb21bec41e8714269013..36d162d0ae9280a03fb734afa5ea62da411579cb 100644 --- a/crates/collab/src/db/queries/channels.rs +++ b/crates/collab/src/db/queries/channels.rs @@ -633,32 +633,84 @@ impl Database { .await } - pub async fn get_channel_members_and_roles( + pub async fn participants_to_notify_for_channel_change( &self, - id: ChannelId, - ) -> Result> { + new_parent: ChannelId, + admin_id: UserId, + ) -> Result> { self.transaction(|tx| async move { - #[derive(Copy, Clone, Debug, EnumIter, DeriveColumn)] - enum QueryUserIdsAndRoles { - UserId, - Role, - } + let mut results: Vec<(UserId, ChannelsForUser)> = Vec::new(); - let ancestor_ids = self.get_channel_ancestors(id, &*tx).await?; - let user_ids_and_roles = channel_member::Entity::find() - .distinct() - .filter( - channel_member::Column::ChannelId - .is_in(ancestor_ids.iter().copied()) - .and(channel_member::Column::Accepted.eq(true)), - ) - .select_only() - .column(channel_member::Column::UserId) - .column(channel_member::Column::Role) - .into_values::<_, QueryUserIdsAndRoles>() - .all(&*tx) + let members = self + .get_channel_participant_details_internal(new_parent, admin_id, &*tx) .await?; - Ok(user_ids_and_roles) + + dbg!(&members); + + 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, + vec![channel_member::Model { + id: Default::default(), + channel_id: new_parent, + user_id: member.user_id, + role: member.role, + accepted: true, + }], + &*tx, + ) + .await?, + )) + } + + let public_parent = self + .public_path_to_channel_internal(new_parent, &*tx) + .await? + .last() + .copied(); + + 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, admin_id, &*tx) + .await? + }; + + dbg!(&public_members); + + 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, + vec![channel_member::Model { + id: Default::default(), + channel_id: public_parent, + user_id: member.user_id, + role: member.role, + accepted: true, + }], + &*tx, + ) + .await?, + )) + } + + Ok(results) }) .await } @@ -696,103 +748,119 @@ impl Database { .await } - pub async fn get_channel_participant_details( + pub async fn get_channel_participant_details_internal( &self, channel_id: ChannelId, admin_id: UserId, - ) -> Result> { - self.transaction(|tx| async move { - self.check_user_is_channel_admin(channel_id, admin_id, &*tx) - .await?; + tx: &DatabaseTransaction, + ) -> Result> { + self.check_user_is_channel_admin(channel_id, admin_id, &*tx) + .await?; - let channel_visibility = channel::Entity::find() - .filter(channel::Column::Id.eq(channel_id)) - .one(&*tx) - .await? - .map(|channel| channel.visibility) - .unwrap_or(ChannelVisibility::Members); + let channel_visibility = channel::Entity::find() + .filter(channel::Column::Id.eq(channel_id)) + .one(&*tx) + .await? + .map(|channel| channel.visibility) + .unwrap_or(ChannelVisibility::Members); - #[derive(Copy, Clone, Debug, EnumIter, DeriveColumn)] - enum QueryMemberDetails { - UserId, - Role, - IsDirectMember, - Accepted, - Visibility, - } + #[derive(Copy, Clone, Debug, EnumIter, DeriveColumn)] + enum QueryMemberDetails { + UserId, + Role, + IsDirectMember, + Accepted, + Visibility, + } - let tx = tx; - let ancestor_ids = self.get_channel_ancestors(channel_id, &*tx).await?; - let mut stream = channel_member::Entity::find() - .left_join(channel::Entity) - .filter(channel_member::Column::ChannelId.is_in(ancestor_ids.iter().copied())) - .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 tx = tx; + let ancestor_ids = self.get_channel_ancestors(channel_id, &*tx).await?; + let mut stream = channel_member::Entity::find() + .left_join(channel::Entity) + .filter(channel_member::Column::ChannelId.is_in(ancestor_ids.iter().copied())) + .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?; - struct UserDetail { - kind: Kind, - channel_role: ChannelRole, + 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; } - 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 let Some(details_mut) = user_details.get_mut(&user_id) { - if channel_role.should_override(details_mut.channel_role) { - details_mut.channel_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, UserDetail { kind, 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(|(user_id, details)| proto::ChannelMember { - user_id: user_id.to_proto(), - kind: details.kind.into(), - role: details.channel_role.into(), - }) - .collect()) - }) - .await + Ok(user_details + .into_iter() + .map(|(_, details)| details) + .collect()) + } + + pub async fn get_channel_participant_details( + &self, + channel_id: ChannelId, + admin_id: UserId, + ) -> Result> { + let members = self + .transaction(move |tx| async move { + Ok(self + .get_channel_participant_details_internal(channel_id, admin_id, &*tx) + .await?) + }) + .await?; + + Ok(members + .into_iter() + .map(|channel_member| channel_member.to_proto()) + .collect()) } pub async fn get_channel_participants_internal( @@ -886,19 +954,35 @@ impl Database { // ordered from higher in tree to lower // only considers one path to a channel // includes the channel itself - pub async fn public_path_to_channel(&self, channel_id: ChannelId) -> Result> { + pub async fn path_to_channel(&self, channel_id: ChannelId) -> Result> { self.transaction(move |tx| async move { - Ok(self - .public_path_to_channel_internal(channel_id, &*tx) - .await?) + Ok(self.path_to_channel_internal(channel_id, &*tx).await?) }) .await } - // ordered from higher in tree to lower - // only considers one path to a channel - // includes the channel itself - pub async fn public_path_to_channel_internal( + pub async fn parent_channel_id(&self, channel_id: ChannelId) -> Result> { + let path = self.path_to_channel(channel_id).await?; + if path.len() >= 2 { + Ok(Some(path[path.len() - 2])) + } else { + Ok(None) + } + } + + pub async fn public_parent_channel_id( + &self, + channel_id: ChannelId, + ) -> Result> { + let path = self.path_to_channel(channel_id).await?; + if path.len() >= 2 && path.last().copied() == Some(channel_id) { + Ok(Some(path[path.len() - 2])) + } else { + Ok(path.last().copied()) + } + } + + pub async fn path_to_channel_internal( &self, channel_id: ChannelId, tx: &DatabaseTransaction, @@ -913,12 +997,35 @@ impl Database { return Ok(vec![]); }; - let ancestor_ids: Vec = path + Ok(path .id_path .trim_matches('/') .split('/') .map(|id| ChannelId::from_proto(id.parse().unwrap())) - .collect(); + .collect()) + } + + // ordered from higher in tree to lower + // only considers one path to a channel + // includes the channel itself + pub async fn public_path_to_channel(&self, channel_id: ChannelId) -> Result> { + self.transaction(move |tx| async move { + Ok(self + .public_path_to_channel_internal(channel_id, &*tx) + .await?) + }) + .await + } + + // ordered from higher in tree to lower + // only considers one path to a channel + // includes the channel itself + pub async fn public_path_to_channel_internal( + &self, + channel_id: ChannelId, + tx: &DatabaseTransaction, + ) -> Result> { + let ancestor_ids = self.path_to_channel_internal(channel_id, &*tx).await?; let rows = channel::Entity::find() .filter(channel::Column::Id.is_in(ancestor_ids.iter().copied())) @@ -1044,6 +1151,27 @@ impl Database { Ok(channel_ids) } + // returns all ids of channels in the tree under this channel_id. + pub async fn get_channel_descendant_ids( + &self, + channel_id: ChannelId, + ) -> Result> { + self.transaction(|tx| async move { + let pairs = self.get_channel_descendants([channel_id], &*tx).await?; + + let mut results: HashSet = HashSet::default(); + for ChannelEdge { + parent_id: _, + channel_id, + } in pairs + { + results.insert(ChannelId::from_proto(channel_id)); + } + Ok(results) + }) + .await + } + // Returns the channel desendants as a sorted list of edges for further processing. // The edges are sorted such that you will see unknown channel ids as children // before you see them as parents. diff --git a/crates/collab/src/rpc.rs b/crates/collab/src/rpc.rs index c7a2ba88b14d1637b249fe3b913fa85850554461..18fbc0d7bc227fc197cac739cce3ee6b53a17e2f 100644 --- a/crates/collab/src/rpc.rs +++ b/crates/collab/src/rpc.rs @@ -2220,26 +2220,13 @@ async fn create_channel( return Ok(()); }; - let members: Vec = db - .get_channel_participant_details(parent_id, session.user_id) - .await? - .into_iter() - .filter(|member| { - member.role() == proto::ChannelRole::Admin - || member.role() == proto::ChannelRole::Member - }) - .collect(); - - let mut updates: HashMap = HashMap::default(); - - for member in members { - let user_id = UserId::from_proto(member.user_id); - let channels = db.get_channel_for_user(parent_id, user_id).await?; - updates.insert(user_id, build_initial_channels_update(channels, vec![])); - } + let updates = db + .participants_to_notify_for_channel_change(parent_id, session.user_id) + .await?; let connection_pool = session.connection_pool().await; - for (user_id, update) in updates { + for (user_id, channels) in updates { + let update = build_initial_channels_update(channels, vec![]); for connection_id in connection_pool.user_connection_ids(user_id) { if user_id == session.user_id { continue; @@ -2353,31 +2340,55 @@ async fn set_channel_visibility( let channel_id = ChannelId::from_proto(request.channel_id); let visibility = request.visibility().into(); - let channel = db - .set_channel_visibility(channel_id, visibility, session.user_id) + let previous_members = db + .get_channel_participant_details(channel_id, session.user_id) .await?; - let members = db - .get_channel_participant_details(channel_id, session.user_id) + db.set_channel_visibility(channel_id, visibility, session.user_id) .await?; + let mut updates: HashMap = db + .participants_to_notify_for_channel_change(channel_id, session.user_id) + .await? + .into_iter() + .collect(); + + let mut participants_who_lost_access: HashSet = HashSet::default(); + match visibility { + ChannelVisibility::Members => { + for member in previous_members { + if ChannelRole::from(member.role()).can_only_see_public_descendants() { + participants_who_lost_access.insert(UserId::from_proto(member.user_id)); + } + } + } + ChannelVisibility::Public => { + if let Some(public_parent_id) = db.public_parent_channel_id(channel_id).await? { + let parent_updates = db + .participants_to_notify_for_channel_change(public_parent_id, session.user_id) + .await?; + + for (user_id, channels) in parent_updates { + updates.insert(user_id, channels); + } + } + } + } + let connection_pool = session.connection_pool().await; - // TODO: notify people who were guests and are now not allowed. - for member in members { - for connection_id in connection_pool.user_connection_ids(UserId::from_proto(member.user_id)) - { - session.peer.send( - connection_id, - proto::UpdateChannels { - channels: vec![proto::Channel { - id: channel.id.to_proto(), - name: channel.name.clone(), - visibility: channel.visibility.into(), - role: member.role.into(), - }], - ..Default::default() - }, - )?; + for (user_id, channels) in updates { + let update = build_initial_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_who_lost_access { + let update = proto::UpdateChannels { + delete_channels: vec![channel_id.to_proto()], + ..Default::default() + }; + for connection_id in connection_pool.user_connection_ids(user_id) { + session.peer.send(connection_id, update.clone())?; } } @@ -2485,42 +2496,20 @@ async fn link_channel( // TODO: Remove this restriction once we have symlinks db.assert_root_channel(channel_id).await?; - let channels_to_send = db.link_channel(session.user_id, channel_id, to).await?; - let members = db.get_channel_members_and_roles(to).await?; - let connection_pool = session.connection_pool().await; + db.link_channel(session.user_id, channel_id, to).await?; - for (member_id, role) in members { - let build_channel_proto = |channel: &db::Channel| proto::Channel { - id: channel.id.to_proto(), - visibility: channel.visibility.into(), - name: channel.name.clone(), - role: role.into(), - }; + let member_updates = db + .participants_to_notify_for_channel_change(to, session.user_id) + .await?; - for connection_id in connection_pool.user_connection_ids(member_id) { - let channels: Vec<_> = if role == ChannelRole::Guest { - channels_to_send - .channels - .iter() - .filter(|channel| channel.visibility != ChannelVisibility::Public) - .map(build_channel_proto) - .collect() - } else { - channels_to_send - .channels - .iter() - .map(build_channel_proto) - .collect() - }; + dbg!(&member_updates); - session.peer.send( - connection_id, - proto::UpdateChannels { - channels, - insert_edge: channels_to_send.edges.clone(), - ..Default::default() - }, - )?; + let connection_pool = session.connection_pool().await; + + for (member_id, channels) in member_updates { + let update = build_initial_channels_update(channels, vec![]); + for connection_id in connection_pool.user_connection_ids(member_id) { + session.peer.send(connection_id, update.clone())?; } } @@ -2548,11 +2537,11 @@ async fn move_channel( let from_parent = ChannelId::from_proto(request.from); let to = ChannelId::from_proto(request.to); - let from_public_parent = db - .public_path_to_channel(from_parent) - .await? - .last() - .copied(); + let previous_participants = db + .get_channel_participant_details(channel_id, session.user_id) + .await?; + + debug_assert_eq!(db.parent_channel_id(channel_id).await?, Some(from_parent)); let channels_to_send = db .move_channel(session.user_id, channel_id, from_parent, to) @@ -2563,68 +2552,42 @@ async fn move_channel( return Ok(()); } - let to_public_parent = db.public_path_to_channel(to).await?.last().cloned(); - - let members_from = db - .get_channel_participant_details(from_parent, session.user_id) - .await? - .into_iter() - .filter(|member| { - member.role() == proto::ChannelRole::Admin || member.role() == proto::ChannelRole::Guest - }); - - let members_to = db - .get_channel_participant_details(to, session.user_id) - .await? - .into_iter() - .filter(|member| { - member.role() == proto::ChannelRole::Admin || member.role() == proto::ChannelRole::Guest - }); - - let mut updates: HashMap = HashMap::default(); - - for member in members_to { - let user_id = UserId::from_proto(member.user_id); - let channels = db.get_channel_for_user(to, user_id).await?; - updates.insert(user_id, build_initial_channels_update(channels, vec![])); - } + let updates = db + .participants_to_notify_for_channel_change(to, session.user_id) + .await?; - if let Some(to_public_parent) = to_public_parent { - // only notify guests of public channels (admins/members are notified by members_to above, and banned users don't care) - let public_members_to = db - .get_channel_participant_details(to, session.user_id) - .await? - .into_iter() - .filter(|member| member.role() == proto::ChannelRole::Guest); + let mut participants_who_lost_access: HashSet = HashSet::default(); + let mut channels_to_delete = db.get_channel_descendant_ids(channel_id).await?; + channels_to_delete.insert(channel_id); - for member in public_members_to { - let user_id = UserId::from_proto(member.user_id); - if updates.contains_key(&user_id) { - continue; - } - let channels = db.get_channel_for_user(to_public_parent, user_id).await?; - updates.insert(user_id, build_initial_channels_update(channels, vec![])); + for previous_participant in previous_participants.iter() { + let user_id = UserId::from_proto(previous_participant.user_id); + if previous_participant.kind() == proto::channel_member::Kind::AncestorMember { + participants_who_lost_access.insert(user_id); } } - for member in members_from { - let user_id = UserId::from_proto(member.user_id); - let update = updates - .entry(user_id) - .or_insert(proto::UpdateChannels::default()); - update.delete_edge.push(proto::ChannelEdge { - channel_id: channel_id.to_proto(), - parent_id: from_parent.to_proto(), - }) - } - - if let Some(_from_public_parent) = from_public_parent { - // TODO: for each guest member of the old public parent - // delete the edge that they could see (from the from_public_parent down) + let connection_pool = session.connection_pool().await; + for (user_id, channels) in updates { + let mut update = build_initial_channels_update(channels, vec![]); + update.delete_channels = channels_to_delete + .iter() + .map(|channel_id| channel_id.to_proto()) + .collect(); + participants_who_lost_access.remove(&user_id); + for connection_id in connection_pool.user_connection_ids(user_id) { + session.peer.send(connection_id, update.clone())?; + } } - let connection_pool = session.connection_pool().await; - for (user_id, update) in updates { + for user_id in participants_who_lost_access { + let update = proto::UpdateChannels { + delete_channels: channels_to_delete + .iter() + .map(|channel_id| channel_id.to_proto()) + .collect(), + ..Default::default() + }; for connection_id in connection_pool.user_connection_ids(user_id) { session.peer.send(connection_id, update.clone())?; } diff --git a/crates/collab/src/tests/channel_tests.rs b/crates/collab/src/tests/channel_tests.rs index f982f05ee325bac3afeb811f6e9509f55b002611..b9425cc62999ae845378d6c74eabdc1bb44ff708 100644 --- a/crates/collab/src/tests/channel_tests.rs +++ b/crates/collab/src/tests/channel_tests.rs @@ -5,6 +5,7 @@ use crate::{ use call::ActiveCall; use channel::{ChannelId, ChannelMembership, ChannelStore}; use client::User; +use futures::future::try_join_all; use gpui::{executor::Deterministic, ModelHandle, TestAppContext}; use rpc::{ proto::{self, ChannelRole}, @@ -913,6 +914,210 @@ async fn test_lost_channel_creation( ], ); } + +#[gpui::test] +async fn test_channel_link_notifications( + deterministic: Arc, + cx_a: &mut TestAppContext, + cx_b: &mut TestAppContext, + cx_c: &mut TestAppContext, +) { + deterministic.forbid_parking(); + + let mut server = TestServer::start(&deterministic).await; + let client_a = server.create_client(cx_a, "user_a").await; + let client_b = server.create_client(cx_b, "user_b").await; + let client_c = server.create_client(cx_c, "user_c").await; + + let user_b = client_b.user_id().unwrap(); + let user_c = client_c.user_id().unwrap(); + + let channels = server + .make_channel_tree(&[("zed", None)], (&client_a, cx_a)) + .await; + let zed_channel = channels[0]; + + 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.invite_member(zed_channel, user_b, proto::ChannelRole::Member, cx), + channel_store.invite_member(zed_channel, user_c, proto::ChannelRole::Guest, cx), + ] + })) + .await + .unwrap(); + + deterministic.run_until_parked(); + + client_b + .channel_store() + .update(cx_b, |channel_store, _| { + channel_store.respond_to_channel_invite(zed_channel, true) + }) + .await + .unwrap(); + + client_c + .channel_store() + .update(cx_c, |channel_store, _| { + channel_store.respond_to_channel_invite(zed_channel, true) + }) + .await + .unwrap(); + + deterministic.run_until_parked(); + + // we have an admin (a), member (b) and guest (c) all part of the zed channel. + + // create a new private sub-channel + // create a new priate channel, make it public, and move it under the previous one, and verify it shows for b and c + let active_channel = client_a + .channel_store() + .update(cx_a, |channel_store, cx| { + channel_store.create_channel("active", Some(zed_channel), cx) + }) + .await + .unwrap(); + + // the new channel shows for b and not c + assert_channels_list_shape( + client_a.channel_store(), + cx_a, + &[(zed_channel, 0), (active_channel, 1)], + ); + assert_channels_list_shape( + client_b.channel_store(), + cx_b, + &[(zed_channel, 0), (active_channel, 1)], + ); + assert_channels_list_shape(client_c.channel_store(), cx_c, &[(zed_channel, 0)]); + + let vim_channel = client_a + .channel_store() + .update(cx_a, |channel_store, cx| { + channel_store.create_channel("vim", None, cx) + }) + .await + .unwrap(); + + client_a + .channel_store() + .update(cx_a, |channel_store, cx| { + channel_store.set_channel_visibility(vim_channel, proto::ChannelVisibility::Public, cx) + }) + .await + .unwrap(); + + client_a + .channel_store() + .update(cx_a, |channel_store, cx| { + channel_store.link_channel(vim_channel, active_channel, cx) + }) + .await + .unwrap(); + + deterministic.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)], + ); + assert_channels_list_shape( + client_b.channel_store(), + cx_b, + &[(zed_channel, 0), (active_channel, 1), (vim_channel, 2)], + ); + assert_channels_list_shape( + client_c.channel_store(), + cx_c, + &[(zed_channel, 0), (vim_channel, 1)], + ); + + let helix_channel = client_a + .channel_store() + .update(cx_a, |channel_store, cx| { + channel_store.create_channel("helix", None, cx) + }) + .await + .unwrap(); + + client_a + .channel_store() + .update(cx_a, |channel_store, cx| { + channel_store.link_channel(helix_channel, vim_channel, cx) + }) + .await + .unwrap(); + + client_a + .channel_store() + .update(cx_a, |channel_store, cx| { + channel_store.set_channel_visibility( + helix_channel, + proto::ChannelVisibility::Public, + cx, + ) + }) + .await + .unwrap(); + + // the new channel shows for b and c + assert_channels_list_shape( + client_b.channel_store(), + cx_b, + &[ + (zed_channel, 0), + (active_channel, 1), + (vim_channel, 2), + (helix_channel, 3), + ], + ); + assert_channels_list_shape( + client_c.channel_store(), + 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(); + + // 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), + ], + ); + client_b + .channel_store() + .read_with(cx_b, |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), (helix_channel, 1)], + ); +} + #[gpui::test] async fn test_guest_access( deterministic: Arc, From 3853009d920b95defec88e87fa8760b5bcf4f875 Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Wed, 18 Oct 2023 19:27:00 -0600 Subject: [PATCH 06/12] Refactor to avoid some (mostly hypothetical) races Tidy up added code to reduce duplicity of X and X_internals. --- crates/channel/src/channel_store_tests.rs | 44 +- crates/collab/src/db.rs | 36 + crates/collab/src/db/queries/buffers.rs | 4 +- crates/collab/src/db/queries/channels.rs | 462 +++++++----- crates/collab/src/db/queries/messages.rs | 4 +- crates/collab/src/db/queries/rooms.rs | 14 +- crates/collab/src/db/tests/channel_tests.rs | 705 +++++++++--------- crates/collab/src/db/tests/message_tests.rs | 8 +- crates/collab/src/rpc.rs | 222 ++---- .../src/tests/random_channel_buffer_tests.rs | 2 +- crates/collab/src/tests/test_server.rs | 32 - crates/collab_ui/src/collab_panel.rs | 1 - crates/rpc/proto/zed.proto | 6 - 13 files changed, 745 insertions(+), 795 deletions(-) diff --git a/crates/channel/src/channel_store_tests.rs b/crates/channel/src/channel_store_tests.rs index 69c0cd37fc10f14e3bfe70d3fb8e41d05d76c9bf..1358378d1603bb9a757e66af6f666973c0270b69 100644 --- a/crates/channel/src/channel_store_tests.rs +++ b/crates/channel/src/channel_store_tests.rs @@ -36,8 +36,8 @@ fn test_update_channels(cx: &mut AppContext) { &channel_store, &[ // - (0, "a".to_string(), false), - (0, "b".to_string(), true), + (0, "a".to_string(), proto::ChannelRole::Member), + (0, "b".to_string(), proto::ChannelRole::Admin), ], cx, ); @@ -50,7 +50,7 @@ fn test_update_channels(cx: &mut AppContext) { id: 3, name: "x".to_string(), visibility: proto::ChannelVisibility::Members as i32, - role: proto::ChannelRole::Member.into(), + role: proto::ChannelRole::Admin.into(), }, proto::Channel { id: 4, @@ -76,10 +76,10 @@ fn test_update_channels(cx: &mut AppContext) { assert_channels( &channel_store, &[ - (0, "a".to_string(), false), - (1, "y".to_string(), false), - (0, "b".to_string(), true), - (1, "x".to_string(), true), + (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), ], cx, ); @@ -131,9 +131,9 @@ fn test_dangling_channel_paths(cx: &mut AppContext) { &channel_store, &[ // - (0, "a".to_string(), true), - (1, "b".to_string(), true), - (2, "c".to_string(), true), + (0, "a".to_string(), proto::ChannelRole::Admin), + (1, "b".to_string(), proto::ChannelRole::Admin), + (2, "c".to_string(), proto::ChannelRole::Admin), ], cx, ); @@ -148,7 +148,11 @@ 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(), true)], cx); + assert_channels( + &channel_store, + &[(0, "a".to_string(), proto::ChannelRole::Admin)], + cx, + ); } #[gpui::test] @@ -165,13 +169,17 @@ 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::Admin.into(), + role: proto::ChannelRole::Member.into(), }], ..Default::default() }); cx.foreground().run_until_parked(); cx.read(|cx| { - assert_channels(&channel_store, &[(0, "the-channel".to_string(), false)], cx); + assert_channels( + &channel_store, + &[(0, "the-channel".to_string(), proto::ChannelRole::Member)], + cx, + ); }); let get_users = server.receive::().await.unwrap(); @@ -366,19 +374,13 @@ fn update_channels( #[track_caller] fn assert_channels( channel_store: &ModelHandle, - expected_channels: &[(usize, String, bool)], + expected_channels: &[(usize, String, proto::ChannelRole)], cx: &AppContext, ) { let actual = channel_store.read_with(cx, |store, _| { store .channel_dag_entries() - .map(|(depth, channel)| { - ( - depth, - channel.name.to_string(), - store.is_channel_admin(channel.id), - ) - }) + .map(|(depth, channel)| (depth, channel.name.to_string(), channel.role)) .collect::>() }); assert_eq!(actual, expected_channels); diff --git a/crates/collab/src/db.rs b/crates/collab/src/db.rs index 4d73d27a4747e84b435a5b49ecc2aa3c33637a26..4f49b7ca39c284635f328d6d0dac74011995fe5e 100644 --- a/crates/collab/src/db.rs +++ b/crates/collab/src/db.rs @@ -428,6 +428,31 @@ pub struct NewUserResult { pub signup_device_id: Option, } +#[derive(Debug)] +pub struct MoveChannelResult { + pub participants_to_update: HashMap, + pub participants_to_remove: HashSet, + pub moved_channels: HashSet, +} + +#[derive(Debug)] +pub struct RenameChannelResult { + pub channel: Channel, + pub participants_to_update: HashMap, +} + +#[derive(Debug)] +pub struct CreateChannelResult { + pub channel: Channel, + pub participants_to_update: Vec<(UserId, ChannelsForUser)>, +} + +#[derive(Debug)] +pub struct SetChannelVisibilityResult { + pub participants_to_update: HashMap, + pub participants_to_remove: HashSet, +} + #[derive(FromQueryResult, Debug, PartialEq, Eq, Hash)] pub struct Channel { pub id: ChannelId, @@ -436,6 +461,17 @@ pub struct Channel { pub role: ChannelRole, } +impl Channel { + pub fn to_proto(&self) -> proto::Channel { + proto::Channel { + id: self.id.to_proto(), + name: self.name.clone(), + visibility: self.visibility.into(), + role: self.role.into(), + } + } +} + #[derive(Debug, PartialEq, Eq, Hash)] pub struct ChannelMember { pub role: ChannelRole, diff --git a/crates/collab/src/db/queries/buffers.rs b/crates/collab/src/db/queries/buffers.rs index 1b8467c75a169c3d2c22df438a9c3e3ec4973842..3aa9cff17116a378c52d459fab7c37cf3bbb85c3 100644 --- a/crates/collab/src/db/queries/buffers.rs +++ b/crates/collab/src/db/queries/buffers.rs @@ -482,9 +482,7 @@ impl Database { ) .await?; - channel_members = self - .get_channel_participants_internal(channel_id, &*tx) - .await?; + channel_members = self.get_channel_participants(channel_id, &*tx).await?; let collaborators = self .get_channel_buffer_collaborators_internal(channel_id, &*tx) .await?; diff --git a/crates/collab/src/db/queries/channels.rs b/crates/collab/src/db/queries/channels.rs index 36d162d0ae9280a03fb734afa5ea62da411579cb..f7b7f6085fc8b9484b64219d63df0a7a02624eff 100644 --- a/crates/collab/src/db/queries/channels.rs +++ b/crates/collab/src/db/queries/channels.rs @@ -16,20 +16,39 @@ impl Database { .await } + #[cfg(test)] pub async fn create_root_channel(&self, name: &str, creator_id: UserId) -> Result { - self.create_channel(name, None, creator_id).await + Ok(self + .create_channel(name, None, creator_id) + .await? + .channel + .id) } - pub async fn create_channel( + #[cfg(test)] + pub async fn create_sub_channel( &self, name: &str, - parent: Option, + parent: ChannelId, creator_id: UserId, ) -> Result { + Ok(self + .create_channel(name, Some(parent), creator_id) + .await? + .channel + .id) + } + + pub async fn create_channel( + &self, + name: &str, + parent: Option, + admin_id: UserId, + ) -> Result { let name = Self::sanitize_channel_name(name)?; self.transaction(move |tx| async move { if let Some(parent) = parent { - self.check_user_is_channel_admin(parent, creator_id, &*tx) + self.check_user_is_channel_admin(parent, admin_id, &*tx) .await?; } @@ -71,17 +90,34 @@ impl Database { .await?; } - channel_member::ActiveModel { - id: ActiveValue::NotSet, - channel_id: ActiveValue::Set(channel.id), - user_id: ActiveValue::Set(creator_id), - accepted: ActiveValue::Set(true), - role: ActiveValue::Set(ChannelRole::Admin), + 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?; } - .insert(&*tx) - .await?; - Ok(channel.id) + let participants_to_update = if let Some(parent) = parent { + self.participants_to_notify_for_channel_change(parent, &*tx) + .await? + } else { + vec![] + }; + + Ok(CreateChannelResult { + channel: Channel { + id: channel.id, + visibility: channel.visibility, + name: channel.name, + role: ChannelRole::Admin, + }, + participants_to_update, + }) }) .await } @@ -132,7 +168,7 @@ impl Database { && channel.as_ref().map(|c| c.visibility) == Some(ChannelVisibility::Public) { let channel_id_to_join = self - .public_path_to_channel_internal(channel_id, &*tx) + .public_path_to_channel(channel_id, &*tx) .await? .first() .cloned() @@ -178,13 +214,17 @@ impl Database { &self, channel_id: ChannelId, visibility: ChannelVisibility, - user_id: UserId, - ) -> Result { + admin_id: UserId, + ) -> Result { self.transaction(move |tx| async move { - self.check_user_is_channel_admin(channel_id, user_id, &*tx) + self.check_user_is_channel_admin(channel_id, admin_id, &*tx) .await?; - let channel = channel::ActiveModel { + let previous_members = self + .get_channel_participant_details_internal(channel_id, &*tx) + .await?; + + channel::ActiveModel { id: ActiveValue::Unchanged(channel_id), visibility: ActiveValue::Set(visibility), ..Default::default() @@ -192,7 +232,40 @@ impl Database { .update(&*tx) .await?; - Ok(channel) + let mut participants_to_update: HashMap = self + .participants_to_notify_for_channel_change(channel_id, &*tx) + .await? + .into_iter() + .collect(); + + let mut participants_to_remove: HashSet = HashSet::default(); + match visibility { + ChannelVisibility::Members => { + 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_id) = + self.public_parent_channel_id(channel_id, &*tx).await? + { + let parent_updates = self + .participants_to_notify_for_channel_change(public_parent_id, &*tx) + .await?; + + for (user_id, channels) in parent_updates { + participants_to_update.insert(user_id, channels); + } + } + } + } + + Ok(SetChannelVisibilityResult { + participants_to_update, + participants_to_remove, + }) }) .await } @@ -303,14 +376,14 @@ impl Database { pub async fn rename_channel( &self, channel_id: ChannelId, - user_id: UserId, + admin_id: UserId, new_name: &str, - ) -> Result { + ) -> Result { self.transaction(move |tx| async move { let new_name = Self::sanitize_channel_name(new_name)?.to_string(); let role = self - .check_user_is_channel_admin(channel_id, user_id, &*tx) + .check_user_is_channel_admin(channel_id, admin_id, &*tx) .await?; let channel = channel::ActiveModel { @@ -321,11 +394,31 @@ impl Database { .update(&*tx) .await?; - Ok(Channel { - id: channel.id, - name: channel.name, - visibility: channel.visibility, - role, + let participants = self + .get_channel_participant_details_internal(channel_id, &*tx) + .await?; + + Ok(RenameChannelResult { + channel: Channel { + id: channel.id, + name: channel.name, + visibility: channel.visibility, + role, + }, + participants_to_update: participants + .iter() + .map(|participant| { + ( + participant.user_id, + Channel { + id: channel.id, + name: new_name.clone(), + visibility: channel.visibility, + role: participant.role, + }, + ) + }) + .collect(), }) }) .await @@ -628,91 +721,83 @@ impl Database { }) } - pub async fn get_channel_members(&self, id: ChannelId) -> Result> { - self.transaction(|tx| async move { self.get_channel_participants_internal(id, &*tx).await }) - .await - } - - pub async fn participants_to_notify_for_channel_change( + async fn participants_to_notify_for_channel_change( &self, new_parent: ChannelId, - admin_id: UserId, + tx: &DatabaseTransaction, ) -> Result> { - self.transaction(|tx| async move { - let mut results: Vec<(UserId, ChannelsForUser)> = Vec::new(); + let mut results: Vec<(UserId, ChannelsForUser)> = Vec::new(); - let members = self - .get_channel_participant_details_internal(new_parent, admin_id, &*tx) - .await?; + let members = self + .get_channel_participant_details_internal(new_parent, &*tx) + .await?; - dbg!(&members); + dbg!(&members); - 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, - vec![channel_member::Model { - id: Default::default(), - channel_id: new_parent, - user_id: member.user_id, - role: member.role, - accepted: true, - }], - &*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, + vec![channel_member::Model { + id: Default::default(), + channel_id: new_parent, + user_id: member.user_id, + role: member.role, + accepted: true, + }], + &*tx, + ) + .await?, + )) + } - let public_parent = self - .public_path_to_channel_internal(new_parent, &*tx) - .await? - .last() - .copied(); + let public_parent = self + .public_path_to_channel(new_parent, &*tx) + .await? + .last() + .copied(); - let Some(public_parent) = public_parent else { - return Ok(results); - }; + 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, admin_id, &*tx) - .await? - }; + // 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? + }; - dbg!(&public_members); + dbg!(&public_members); - for member in public_members { - if !member.role.can_only_see_public_descendants() { - continue; - }; - results.push(( + 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, - self.get_user_channels( - member.user_id, - vec![channel_member::Model { - id: Default::default(), - channel_id: public_parent, - user_id: member.user_id, - role: member.role, - accepted: true, - }], - &*tx, - ) - .await?, - )) - } + vec![channel_member::Model { + id: Default::default(), + channel_id: public_parent, + user_id: member.user_id, + role: member.role, + accepted: true, + }], + &*tx, + ) + .await?, + )) + } - Ok(results) - }) - .await + Ok(results) } pub async fn set_channel_member_role( @@ -748,15 +833,11 @@ impl Database { .await } - pub async fn get_channel_participant_details_internal( + async fn get_channel_participant_details_internal( &self, channel_id: ChannelId, - admin_id: UserId, tx: &DatabaseTransaction, ) -> Result> { - self.check_user_is_channel_admin(channel_id, admin_id, &*tx) - .await?; - let channel_visibility = channel::Entity::find() .filter(channel::Column::Id.eq(channel_id)) .one(&*tx) @@ -851,8 +932,11 @@ impl Database { ) -> Result> { let members = self .transaction(move |tx| async move { + self.check_user_is_channel_admin(channel_id, admin_id, &*tx) + .await?; + Ok(self - .get_channel_participant_details_internal(channel_id, admin_id, &*tx) + .get_channel_participant_details_internal(channel_id, &*tx) .await?) }) .await?; @@ -863,25 +947,18 @@ impl Database { .collect()) } - pub async fn get_channel_participants_internal( + pub async fn get_channel_participants( &self, - id: ChannelId, + channel_id: ChannelId, tx: &DatabaseTransaction, ) -> Result> { - let ancestor_ids = self.get_channel_ancestors(id, tx).await?; - let user_ids = channel_member::Entity::find() - .distinct() - .filter( - channel_member::Column::ChannelId - .is_in(ancestor_ids.iter().copied()) - .and(channel_member::Column::Accepted.eq(true)), - ) - .select_only() - .column(channel_member::Column::UserId) - .into_values::<_, QueryUserIds>() - .all(&*tx) + let participants = self + .get_channel_participant_details_internal(channel_id, &*tx) .await?; - Ok(user_ids) + Ok(participants + .into_iter() + .map(|member| member.user_id) + .collect()) } pub async fn check_user_is_channel_admin( @@ -951,18 +1028,12 @@ impl Database { Ok(row) } - // ordered from higher in tree to lower - // only considers one path to a channel - // includes the channel itself - pub async fn path_to_channel(&self, channel_id: ChannelId) -> Result> { - self.transaction(move |tx| async move { - Ok(self.path_to_channel_internal(channel_id, &*tx).await?) - }) - .await - } - - pub async fn parent_channel_id(&self, channel_id: ChannelId) -> Result> { - let path = self.path_to_channel(channel_id).await?; + pub async fn parent_channel_id( + &self, + channel_id: ChannelId, + tx: &DatabaseTransaction, + ) -> Result> { + let path = self.path_to_channel(channel_id, &*tx).await?; if path.len() >= 2 { Ok(Some(path[path.len() - 2])) } else { @@ -973,8 +1044,9 @@ impl Database { pub async fn public_parent_channel_id( &self, channel_id: ChannelId, + tx: &DatabaseTransaction, ) -> Result> { - let path = self.path_to_channel(channel_id).await?; + let path = self.public_path_to_channel(channel_id, &*tx).await?; if path.len() >= 2 && path.last().copied() == Some(channel_id) { Ok(Some(path[path.len() - 2])) } else { @@ -982,7 +1054,7 @@ impl Database { } } - pub async fn path_to_channel_internal( + pub async fn path_to_channel( &self, channel_id: ChannelId, tx: &DatabaseTransaction, @@ -1005,27 +1077,12 @@ impl Database { .collect()) } - // ordered from higher in tree to lower - // only considers one path to a channel - // includes the channel itself - pub async fn public_path_to_channel(&self, channel_id: ChannelId) -> Result> { - self.transaction(move |tx| async move { - Ok(self - .public_path_to_channel_internal(channel_id, &*tx) - .await?) - }) - .await - } - - // ordered from higher in tree to lower - // only considers one path to a channel - // includes the channel itself - pub async fn public_path_to_channel_internal( + pub async fn public_path_to_channel( &self, channel_id: ChannelId, tx: &DatabaseTransaction, ) -> Result> { - let ancestor_ids = self.path_to_channel_internal(channel_id, &*tx).await?; + let ancestor_ids = self.path_to_channel(channel_id, &*tx).await?; let rows = channel::Entity::find() .filter(channel::Column::Id.is_in(ancestor_ids.iter().copied())) @@ -1151,27 +1208,6 @@ impl Database { Ok(channel_ids) } - // returns all ids of channels in the tree under this channel_id. - pub async fn get_channel_descendant_ids( - &self, - channel_id: ChannelId, - ) -> Result> { - self.transaction(|tx| async move { - let pairs = self.get_channel_descendants([channel_id], &*tx).await?; - - let mut results: HashSet = HashSet::default(); - for ChannelEdge { - parent_id: _, - channel_id, - } in pairs - { - results.insert(ChannelId::from_proto(channel_id)); - } - Ok(results) - }) - .await - } - // Returns the channel desendants as a sorted list of edges for further processing. // The edges are sorted such that you will see unknown channel ids as children // before you see them as parents. @@ -1388,9 +1424,6 @@ impl Database { from: ChannelId, ) -> Result<()> { self.transaction(|tx| async move { - // Note that even with these maxed permissions, this linking operation - // is still insecure because you can't remove someone's permissions to a - // channel if they've linked the channel to one where they're an admin. self.check_user_is_channel_admin(channel, user, &*tx) .await?; @@ -1433,6 +1466,8 @@ impl Database { .await? == 0; + dbg!(is_stranded, &paths); + // Make sure that there is always at least one path to the channel if is_stranded { let root_paths: Vec<_> = paths @@ -1445,6 +1480,8 @@ impl Database { } }) .collect(); + + dbg!(is_stranded, &root_paths); channel_path::Entity::insert_many(root_paths) .exec(&*tx) .await?; @@ -1453,49 +1490,64 @@ impl Database { Ok(()) } - /// Move a channel from one parent to another, returns the - /// Channels that were moved for notifying clients + /// Move a channel from one parent to another pub async fn move_channel( &self, - user: UserId, - channel: ChannelId, - from: ChannelId, - to: ChannelId, - ) -> Result { - if from == to { - return Ok(ChannelGraph { - channels: vec![], - edges: vec![], - }); - } - + channel_id: ChannelId, + old_parent_id: Option, + new_parent_id: ChannelId, + admin_id: UserId, + ) -> Result> { self.transaction(|tx| async move { - self.check_user_is_channel_admin(channel, user, &*tx) + self.check_user_is_channel_admin(channel_id, admin_id, &*tx) .await?; - let moved_channels = self.link_channel_internal(user, channel, to, &*tx).await?; + debug_assert_eq!( + self.parent_channel_id(channel_id, &*tx).await?, + old_parent_id + ); - self.unlink_channel_internal(user, channel, from, &*tx) + if old_parent_id == Some(new_parent_id) { + return Ok(None); + } + let previous_participants = self + .get_channel_participant_details_internal(channel_id, &*tx) .await?; - Ok(moved_channels) - }) - .await - } + self.link_channel_internal(admin_id, channel_id, new_parent_id, &*tx) + .await?; - pub async fn assert_root_channel(&self, channel: ChannelId) -> Result<()> { - self.transaction(|tx| async move { - let path = channel_path::Entity::find() - .filter(channel_path::Column::ChannelId.eq(channel)) - .one(&*tx) + if let Some(from) = old_parent_id { + self.unlink_channel_internal(admin_id, channel_id, from, &*tx) + .await?; + } + + let participants_to_update: HashMap = self + .participants_to_notify_for_channel_change(new_parent_id, &*tx) .await? - .ok_or_else(|| anyhow!("no such channel found"))?; + .into_iter() + .collect(); - let mut id_parts = path.id_path.trim_matches('/').split('/'); + let mut moved_channels: HashSet = HashSet::default(); + moved_channels.insert(channel_id); + for edge in self.get_channel_descendants([channel_id], &*tx).await? { + moved_channels.insert(ChannelId::from_proto(edge.channel_id)); + } + + let mut participants_to_remove: HashSet = HashSet::default(); + for participant in 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); + } + } + } - (id_parts.next().is_some() && id_parts.next().is_none()) - .then_some(()) - .ok_or_else(|| anyhow!("channel is not a root channel").into()) + Ok(Some(MoveChannelResult { + participants_to_remove, + participants_to_update, + moved_channels, + })) }) .await } diff --git a/crates/collab/src/db/queries/messages.rs b/crates/collab/src/db/queries/messages.rs index de7334425facb79f99a10860d35493c49e398195..4bcac025c58c31f44a890f70883ee1c030450f5b 100644 --- a/crates/collab/src/db/queries/messages.rs +++ b/crates/collab/src/db/queries/messages.rs @@ -183,9 +183,7 @@ impl Database { ) .await?; - let mut channel_members = self - .get_channel_participants_internal(channel_id, &*tx) - .await?; + let mut channel_members = self.get_channel_participants(channel_id, &*tx).await?; channel_members.retain(|member| !participant_user_ids.contains(member)); Ok(( diff --git a/crates/collab/src/db/queries/rooms.rs b/crates/collab/src/db/queries/rooms.rs index d2120495b05063550765eeb7fbe151512d524403..630d51cfe693cb8440b1b3507fa30557915073ef 100644 --- a/crates/collab/src/db/queries/rooms.rs +++ b/crates/collab/src/db/queries/rooms.rs @@ -53,9 +53,7 @@ impl Database { let (channel_id, room) = self.get_channel_room(room_id, &tx).await?; let channel_members; if let Some(channel_id) = channel_id { - channel_members = self - .get_channel_participants_internal(channel_id, &tx) - .await?; + channel_members = self.get_channel_participants(channel_id, &tx).await?; } else { channel_members = Vec::new(); @@ -423,9 +421,7 @@ impl Database { .await?; let room = self.get_room(room_id, &tx).await?; - let channel_members = self - .get_channel_participants_internal(channel_id, &tx) - .await?; + let channel_members = self.get_channel_participants(channel_id, &tx).await?; Ok(JoinRoom { room, channel_id: Some(channel_id), @@ -724,8 +720,7 @@ impl Database { let (channel_id, room) = self.get_channel_room(room_id, &tx).await?; let channel_members = if let Some(channel_id) = channel_id { - self.get_channel_participants_internal(channel_id, &tx) - .await? + self.get_channel_participants(channel_id, &tx).await? } else { Vec::new() }; @@ -883,8 +878,7 @@ impl Database { }; let channel_members = if let Some(channel_id) = channel_id { - self.get_channel_participants_internal(channel_id, &tx) - .await? + self.get_channel_participants(channel_id, &tx).await? } else { Vec::new() }; diff --git a/crates/collab/src/db/tests/channel_tests.rs b/crates/collab/src/db/tests/channel_tests.rs index a323f2919ec70e7ad6b3908755449ece3f0ee4e5..1767a773ff5484bfabb5b870c78c8caf418a1bc7 100644 --- a/crates/collab/src/db/tests/channel_tests.rs +++ b/crates/collab/src/db/tests/channel_tests.rs @@ -36,28 +36,28 @@ async fn test_channels(db: &Arc) { .await .unwrap(); - let crdb_id = db.create_channel("crdb", Some(zed_id), a_id).await.unwrap(); + let crdb_id = db.create_sub_channel("crdb", zed_id, a_id).await.unwrap(); let livestreaming_id = db - .create_channel("livestreaming", Some(zed_id), a_id) + .create_sub_channel("livestreaming", zed_id, a_id) .await .unwrap(); let replace_id = db - .create_channel("replace", Some(zed_id), a_id) + .create_sub_channel("replace", zed_id, a_id) .await .unwrap(); - let mut members = db.get_channel_members(replace_id).await.unwrap(); + let mut members = db + .transaction(|tx| async move { Ok(db.get_channel_participants(replace_id, &*tx).await?) }) + .await + .unwrap(); members.sort(); assert_eq!(members, &[a_id, b_id]); let rust_id = db.create_root_channel("rust", a_id).await.unwrap(); - let cargo_id = db - .create_channel("cargo", Some(rust_id), a_id) - .await - .unwrap(); + let cargo_id = db.create_sub_channel("cargo", rust_id, a_id).await.unwrap(); let cargo_ra_id = db - .create_channel("cargo-ra", Some(cargo_id), a_id) + .create_sub_channel("cargo-ra", cargo_id, a_id) .await .unwrap(); @@ -264,7 +264,7 @@ async fn test_channel_invites(db: &Arc) { .unwrap(); let channel_1_3 = db - .create_channel("channel_3", Some(channel_1_1), user_1) + .create_sub_channel("channel_3", channel_1_1, user_1) .await .unwrap(); @@ -277,7 +277,7 @@ async fn test_channel_invites(db: &Arc) { &[ proto::ChannelMember { user_id: user_1.to_proto(), - kind: proto::channel_member::Kind::Member.into(), + kind: proto::channel_member::Kind::AncestorMember.into(), role: proto::ChannelRole::Admin.into(), }, proto::ChannelMember { @@ -369,20 +369,17 @@ async fn test_db_channel_moving(db: &Arc) { let zed_id = db.create_root_channel("zed", a_id).await.unwrap(); - let crdb_id = db.create_channel("crdb", Some(zed_id), a_id).await.unwrap(); + let crdb_id = db.create_sub_channel("crdb", zed_id, a_id).await.unwrap(); - let gpui2_id = db - .create_channel("gpui2", Some(zed_id), a_id) - .await - .unwrap(); + let gpui2_id = db.create_sub_channel("gpui2", zed_id, a_id).await.unwrap(); let livestreaming_id = db - .create_channel("livestreaming", Some(crdb_id), a_id) + .create_sub_channel("livestreaming", crdb_id, a_id) .await .unwrap(); let livestreaming_dag_id = db - .create_channel("livestreaming_dag", Some(livestreaming_id), a_id) + .create_sub_channel("livestreaming_dag", livestreaming_id, a_id) .await .unwrap(); @@ -409,311 +406,311 @@ async fn test_db_channel_moving(db: &Arc) { .await .is_err()); - // ======================================================================== - // Make a link - db.link_channel(a_id, livestreaming_id, zed_id) - .await - .unwrap(); - - // DAG is now: - // /- gpui2 - // zed -- crdb - livestreaming - livestreaming_dag - // \---------/ - let result = db.get_channels_for_user(a_id).await.unwrap(); - assert_dag( - result.channels, - &[ - (zed_id, None), - (crdb_id, Some(zed_id)), - (gpui2_id, Some(zed_id)), - (livestreaming_id, Some(zed_id)), - (livestreaming_id, Some(crdb_id)), - (livestreaming_dag_id, Some(livestreaming_id)), - ], - ); - - // ======================================================================== - // Create a new channel below a channel with multiple parents - let livestreaming_dag_sub_id = db - .create_channel("livestreaming_dag_sub", Some(livestreaming_dag_id), a_id) - .await - .unwrap(); - - // DAG is now: - // /- gpui2 - // zed -- crdb - livestreaming - livestreaming_dag - livestreaming_dag_sub_id - // \---------/ - let result = db.get_channels_for_user(a_id).await.unwrap(); - assert_dag( - result.channels, - &[ - (zed_id, None), - (crdb_id, Some(zed_id)), - (gpui2_id, Some(zed_id)), - (livestreaming_id, Some(zed_id)), - (livestreaming_id, Some(crdb_id)), - (livestreaming_dag_id, Some(livestreaming_id)), - (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), - ], - ); - - // ======================================================================== - // Test a complex DAG by making another link - let returned_channels = db - .link_channel(a_id, livestreaming_dag_sub_id, livestreaming_id) - .await - .unwrap(); - - // DAG is now: - // /- gpui2 /---------------------\ - // zed - crdb - livestreaming - livestreaming_dag - livestreaming_dag_sub_id - // \--------/ - - // make sure we're getting just the new link - // Not using the assert_dag helper because we want to make sure we're returning the full data - pretty_assertions::assert_eq!( - returned_channels, - graph( - &[( - livestreaming_dag_sub_id, - "livestreaming_dag_sub", - ChannelRole::Admin - )], - &[(livestreaming_dag_sub_id, livestreaming_id)] - ) - ); - - let result = db.get_channels_for_user(a_id).await.unwrap(); - assert_dag( - result.channels, - &[ - (zed_id, None), - (crdb_id, Some(zed_id)), - (gpui2_id, Some(zed_id)), - (livestreaming_id, Some(zed_id)), - (livestreaming_id, Some(crdb_id)), - (livestreaming_dag_id, Some(livestreaming_id)), - (livestreaming_dag_sub_id, Some(livestreaming_id)), - (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), - ], - ); - - // ======================================================================== - // Test a complex DAG by making another link - let returned_channels = db - .link_channel(a_id, livestreaming_id, gpui2_id) - .await - .unwrap(); - - // DAG is now: - // /- gpui2 -\ /---------------------\ - // zed - crdb -- livestreaming - livestreaming_dag - livestreaming_dag_sub_id - // \---------/ - - // Make sure that we're correctly getting the full sub-dag - pretty_assertions::assert_eq!( - returned_channels, - graph( - &[ - (livestreaming_id, "livestreaming", ChannelRole::Admin), - ( - livestreaming_dag_id, - "livestreaming_dag", - ChannelRole::Admin - ), - ( - livestreaming_dag_sub_id, - "livestreaming_dag_sub", - ChannelRole::Admin - ), - ], - &[ - (livestreaming_id, gpui2_id), - (livestreaming_dag_id, livestreaming_id), - (livestreaming_dag_sub_id, livestreaming_id), - (livestreaming_dag_sub_id, livestreaming_dag_id), - ] - ) - ); - - let result = db.get_channels_for_user(a_id).await.unwrap(); - assert_dag( - result.channels, - &[ - (zed_id, None), - (crdb_id, Some(zed_id)), - (gpui2_id, Some(zed_id)), - (livestreaming_id, Some(zed_id)), - (livestreaming_id, Some(crdb_id)), - (livestreaming_id, Some(gpui2_id)), - (livestreaming_dag_id, Some(livestreaming_id)), - (livestreaming_dag_sub_id, Some(livestreaming_id)), - (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), - ], - ); - - // ======================================================================== - // Test unlinking in a complex DAG by removing the inner link - db.unlink_channel(a_id, livestreaming_dag_sub_id, livestreaming_id) - .await - .unwrap(); - - // DAG is now: - // /- gpui2 -\ - // zed - crdb -- livestreaming - livestreaming_dag - livestreaming_dag_sub - // \---------/ - - let result = db.get_channels_for_user(a_id).await.unwrap(); - assert_dag( - result.channels, - &[ - (zed_id, None), - (crdb_id, Some(zed_id)), - (gpui2_id, Some(zed_id)), - (livestreaming_id, Some(gpui2_id)), - (livestreaming_id, Some(zed_id)), - (livestreaming_id, Some(crdb_id)), - (livestreaming_dag_id, Some(livestreaming_id)), - (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), - ], - ); - - // ======================================================================== - // Test unlinking in a complex DAG by removing the inner link - db.unlink_channel(a_id, livestreaming_id, gpui2_id) - .await - .unwrap(); - - // DAG is now: - // /- gpui2 - // zed - crdb -- livestreaming - livestreaming_dag - livestreaming_dag_sub - // \---------/ - let result = db.get_channels_for_user(a_id).await.unwrap(); - assert_dag( - result.channels, - &[ - (zed_id, None), - (crdb_id, Some(zed_id)), - (gpui2_id, Some(zed_id)), - (livestreaming_id, Some(zed_id)), - (livestreaming_id, Some(crdb_id)), - (livestreaming_dag_id, Some(livestreaming_id)), - (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), - ], - ); - - // ======================================================================== - // Test moving DAG nodes by moving livestreaming to be below gpui2 - db.move_channel(a_id, livestreaming_id, crdb_id, gpui2_id) - .await - .unwrap(); - - // DAG is now: - // /- gpui2 -- livestreaming - livestreaming_dag - livestreaming_dag_sub - // zed - crdb / - // \---------/ - let result = db.get_channels_for_user(a_id).await.unwrap(); - assert_dag( - result.channels, - &[ - (zed_id, None), - (crdb_id, Some(zed_id)), - (gpui2_id, Some(zed_id)), - (livestreaming_id, Some(zed_id)), - (livestreaming_id, Some(gpui2_id)), - (livestreaming_dag_id, Some(livestreaming_id)), - (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), - ], - ); - - // ======================================================================== - // Deleting a channel should not delete children that still have other parents - db.delete_channel(gpui2_id, a_id).await.unwrap(); - - // DAG is now: - // zed - crdb - // \- livestreaming - livestreaming_dag - livestreaming_dag_sub - let result = db.get_channels_for_user(a_id).await.unwrap(); - assert_dag( - result.channels, - &[ - (zed_id, None), - (crdb_id, Some(zed_id)), - (livestreaming_id, Some(zed_id)), - (livestreaming_dag_id, Some(livestreaming_id)), - (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), - ], - ); - - // ======================================================================== - // Unlinking a channel from it's parent should automatically promote it to a root channel - db.unlink_channel(a_id, crdb_id, zed_id).await.unwrap(); - - // DAG is now: - // crdb - // zed - // \- livestreaming - livestreaming_dag - livestreaming_dag_sub - - let result = db.get_channels_for_user(a_id).await.unwrap(); - assert_dag( - result.channels, - &[ - (zed_id, None), - (crdb_id, None), - (livestreaming_id, Some(zed_id)), - (livestreaming_dag_id, Some(livestreaming_id)), - (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), - ], - ); - - // ======================================================================== - // You should be able to move a root channel into a non-root channel - db.link_channel(a_id, crdb_id, zed_id).await.unwrap(); - - // DAG is now: - // zed - crdb - // \- livestreaming - livestreaming_dag - livestreaming_dag_sub - - let result = db.get_channels_for_user(a_id).await.unwrap(); - assert_dag( - result.channels, - &[ - (zed_id, None), - (crdb_id, Some(zed_id)), - (livestreaming_id, Some(zed_id)), - (livestreaming_dag_id, Some(livestreaming_id)), - (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), - ], - ); - - // ======================================================================== - // Prep for DAG deletion test - db.link_channel(a_id, livestreaming_id, crdb_id) - .await - .unwrap(); - - // DAG is now: - // zed - crdb - livestreaming - livestreaming_dag - livestreaming_dag_sub - // \--------/ - - let result = db.get_channels_for_user(a_id).await.unwrap(); - assert_dag( - result.channels, - &[ - (zed_id, None), - (crdb_id, Some(zed_id)), - (livestreaming_id, Some(zed_id)), - (livestreaming_id, Some(crdb_id)), - (livestreaming_dag_id, Some(livestreaming_id)), - (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), - ], - ); - - // Deleting the parent of a DAG should delete the whole DAG: - db.delete_channel(zed_id, a_id).await.unwrap(); - let result = db.get_channels_for_user(a_id).await.unwrap(); - - assert!(result.channels.is_empty()) + // // ======================================================================== + // // Make a link + // db.link_channel(a_id, livestreaming_id, zed_id) + // .await + // .unwrap(); + + // // DAG is now: + // // /- gpui2 + // // zed -- crdb - livestreaming - livestreaming_dag + // // \---------/ + // let result = db.get_channels_for_user(a_id).await.unwrap(); + // assert_dag( + // result.channels, + // &[ + // (zed_id, None), + // (crdb_id, Some(zed_id)), + // (gpui2_id, Some(zed_id)), + // (livestreaming_id, Some(zed_id)), + // (livestreaming_id, Some(crdb_id)), + // (livestreaming_dag_id, Some(livestreaming_id)), + // ], + // ); + + // // ======================================================================== + // // Create a new channel below a channel with multiple parents + // let livestreaming_dag_sub_id = db + // .create_channel("livestreaming_dag_sub", Some(livestreaming_dag_id), a_id) + // .await + // .unwrap(); + + // // DAG is now: + // // /- gpui2 + // // zed -- crdb - livestreaming - livestreaming_dag - livestreaming_dag_sub_id + // // \---------/ + // let result = db.get_channels_for_user(a_id).await.unwrap(); + // assert_dag( + // result.channels, + // &[ + // (zed_id, None), + // (crdb_id, Some(zed_id)), + // (gpui2_id, Some(zed_id)), + // (livestreaming_id, Some(zed_id)), + // (livestreaming_id, Some(crdb_id)), + // (livestreaming_dag_id, Some(livestreaming_id)), + // (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), + // ], + // ); + + // // ======================================================================== + // // Test a complex DAG by making another link + // let returned_channels = db + // .link_channel(a_id, livestreaming_dag_sub_id, livestreaming_id) + // .await + // .unwrap(); + + // // DAG is now: + // // /- gpui2 /---------------------\ + // // zed - crdb - livestreaming - livestreaming_dag - livestreaming_dag_sub_id + // // \--------/ + + // // make sure we're getting just the new link + // // Not using the assert_dag helper because we want to make sure we're returning the full data + // pretty_assertions::assert_eq!( + // returned_channels, + // graph( + // &[( + // livestreaming_dag_sub_id, + // "livestreaming_dag_sub", + // ChannelRole::Admin + // )], + // &[(livestreaming_dag_sub_id, livestreaming_id)] + // ) + // ); + + // let result = db.get_channels_for_user(a_id).await.unwrap(); + // assert_dag( + // result.channels, + // &[ + // (zed_id, None), + // (crdb_id, Some(zed_id)), + // (gpui2_id, Some(zed_id)), + // (livestreaming_id, Some(zed_id)), + // (livestreaming_id, Some(crdb_id)), + // (livestreaming_dag_id, Some(livestreaming_id)), + // (livestreaming_dag_sub_id, Some(livestreaming_id)), + // (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), + // ], + // ); + + // // ======================================================================== + // // Test a complex DAG by making another link + // let returned_channels = db + // .link_channel(a_id, livestreaming_id, gpui2_id) + // .await + // .unwrap(); + + // // DAG is now: + // // /- gpui2 -\ /---------------------\ + // // zed - crdb -- livestreaming - livestreaming_dag - livestreaming_dag_sub_id + // // \---------/ + + // // Make sure that we're correctly getting the full sub-dag + // pretty_assertions::assert_eq!( + // returned_channels, + // graph( + // &[ + // (livestreaming_id, "livestreaming", ChannelRole::Admin), + // ( + // livestreaming_dag_id, + // "livestreaming_dag", + // ChannelRole::Admin + // ), + // ( + // livestreaming_dag_sub_id, + // "livestreaming_dag_sub", + // ChannelRole::Admin + // ), + // ], + // &[ + // (livestreaming_id, gpui2_id), + // (livestreaming_dag_id, livestreaming_id), + // (livestreaming_dag_sub_id, livestreaming_id), + // (livestreaming_dag_sub_id, livestreaming_dag_id), + // ] + // ) + // ); + + // let result = db.get_channels_for_user(a_id).await.unwrap(); + // assert_dag( + // result.channels, + // &[ + // (zed_id, None), + // (crdb_id, Some(zed_id)), + // (gpui2_id, Some(zed_id)), + // (livestreaming_id, Some(zed_id)), + // (livestreaming_id, Some(crdb_id)), + // (livestreaming_id, Some(gpui2_id)), + // (livestreaming_dag_id, Some(livestreaming_id)), + // (livestreaming_dag_sub_id, Some(livestreaming_id)), + // (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), + // ], + // ); + + // // ======================================================================== + // // Test unlinking in a complex DAG by removing the inner link + // db.unlink_channel(a_id, livestreaming_dag_sub_id, livestreaming_id) + // .await + // .unwrap(); + + // // DAG is now: + // // /- gpui2 -\ + // // zed - crdb -- livestreaming - livestreaming_dag - livestreaming_dag_sub + // // \---------/ + + // let result = db.get_channels_for_user(a_id).await.unwrap(); + // assert_dag( + // result.channels, + // &[ + // (zed_id, None), + // (crdb_id, Some(zed_id)), + // (gpui2_id, Some(zed_id)), + // (livestreaming_id, Some(gpui2_id)), + // (livestreaming_id, Some(zed_id)), + // (livestreaming_id, Some(crdb_id)), + // (livestreaming_dag_id, Some(livestreaming_id)), + // (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), + // ], + // ); + + // // ======================================================================== + // // Test unlinking in a complex DAG by removing the inner link + // db.unlink_channel(a_id, livestreaming_id, gpui2_id) + // .await + // .unwrap(); + + // // DAG is now: + // // /- gpui2 + // // zed - crdb -- livestreaming - livestreaming_dag - livestreaming_dag_sub + // // \---------/ + // let result = db.get_channels_for_user(a_id).await.unwrap(); + // assert_dag( + // result.channels, + // &[ + // (zed_id, None), + // (crdb_id, Some(zed_id)), + // (gpui2_id, Some(zed_id)), + // (livestreaming_id, Some(zed_id)), + // (livestreaming_id, Some(crdb_id)), + // (livestreaming_dag_id, Some(livestreaming_id)), + // (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), + // ], + // ); + + // // ======================================================================== + // // Test moving DAG nodes by moving livestreaming to be below gpui2 + // db.move_channel(livestreaming_id, Some(crdb_id), gpui2_id, a_id) + // .await + // .unwrap(); + + // // DAG is now: + // // /- gpui2 -- livestreaming - livestreaming_dag - livestreaming_dag_sub + // // zed - crdb / + // // \---------/ + // let result = db.get_channels_for_user(a_id).await.unwrap(); + // assert_dag( + // result.channels, + // &[ + // (zed_id, None), + // (crdb_id, Some(zed_id)), + // (gpui2_id, Some(zed_id)), + // (livestreaming_id, Some(zed_id)), + // (livestreaming_id, Some(gpui2_id)), + // (livestreaming_dag_id, Some(livestreaming_id)), + // (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), + // ], + // ); + + // // ======================================================================== + // // Deleting a channel should not delete children that still have other parents + // db.delete_channel(gpui2_id, a_id).await.unwrap(); + + // // DAG is now: + // // zed - crdb + // // \- livestreaming - livestreaming_dag - livestreaming_dag_sub + // let result = db.get_channels_for_user(a_id).await.unwrap(); + // assert_dag( + // result.channels, + // &[ + // (zed_id, None), + // (crdb_id, Some(zed_id)), + // (livestreaming_id, Some(zed_id)), + // (livestreaming_dag_id, Some(livestreaming_id)), + // (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), + // ], + // ); + + // // ======================================================================== + // // Unlinking a channel from it's parent should automatically promote it to a root channel + // db.unlink_channel(a_id, crdb_id, zed_id).await.unwrap(); + + // // DAG is now: + // // crdb + // // zed + // // \- livestreaming - livestreaming_dag - livestreaming_dag_sub + + // let result = db.get_channels_for_user(a_id).await.unwrap(); + // assert_dag( + // result.channels, + // &[ + // (zed_id, None), + // (crdb_id, None), + // (livestreaming_id, Some(zed_id)), + // (livestreaming_dag_id, Some(livestreaming_id)), + // (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), + // ], + // ); + + // // ======================================================================== + // // You should be able to move a root channel into a non-root channel + // db.link_channel(a_id, crdb_id, zed_id).await.unwrap(); + + // // DAG is now: + // // zed - crdb + // // \- livestreaming - livestreaming_dag - livestreaming_dag_sub + + // let result = db.get_channels_for_user(a_id).await.unwrap(); + // assert_dag( + // result.channels, + // &[ + // (zed_id, None), + // (crdb_id, Some(zed_id)), + // (livestreaming_id, Some(zed_id)), + // (livestreaming_dag_id, Some(livestreaming_id)), + // (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), + // ], + // ); + + // // ======================================================================== + // // Prep for DAG deletion test + // db.link_channel(a_id, livestreaming_id, crdb_id) + // .await + // .unwrap(); + + // // DAG is now: + // // zed - crdb - livestreaming - livestreaming_dag - livestreaming_dag_sub + // // \--------/ + + // let result = db.get_channels_for_user(a_id).await.unwrap(); + // assert_dag( + // result.channels, + // &[ + // (zed_id, None), + // (crdb_id, Some(zed_id)), + // (livestreaming_id, Some(zed_id)), + // (livestreaming_id, Some(crdb_id)), + // (livestreaming_dag_id, Some(livestreaming_id)), + // (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), + // ], + // ); + + // // Deleting the parent of a DAG should delete the whole DAG: + // db.delete_channel(zed_id, a_id).await.unwrap(); + // let result = db.get_channels_for_user(a_id).await.unwrap(); + + // assert!(result.channels.is_empty()) } test_both_dbs!( @@ -740,12 +737,12 @@ async fn test_db_channel_moving_bugs(db: &Arc) { let zed_id = db.create_root_channel("zed", user_id).await.unwrap(); let projects_id = db - .create_channel("projects", Some(zed_id), user_id) + .create_sub_channel("projects", zed_id, user_id) .await .unwrap(); let livestreaming_id = db - .create_channel("livestreaming", Some(projects_id), user_id) + .create_sub_channel("livestreaming", projects_id, user_id) .await .unwrap(); @@ -753,25 +750,37 @@ async fn test_db_channel_moving_bugs(db: &Arc) { // Move to same parent should be a no-op assert!(db - .move_channel(user_id, projects_id, zed_id, zed_id) + .move_channel(projects_id, Some(zed_id), zed_id, user_id) .await .unwrap() - .is_empty()); - - // Stranding a channel should retain it's sub channels - db.unlink_channel(user_id, projects_id, zed_id) - .await - .unwrap(); + .is_none()); let result = db.get_channels_for_user(user_id).await.unwrap(); assert_dag( result.channels, &[ (zed_id, None), - (projects_id, None), + (projects_id, Some(zed_id)), (livestreaming_id, Some(projects_id)), ], ); + + // Stranding a channel should retain it's sub channels + // Commented out as we don't fix permissions when this happens yet. + // + // db.unlink_channel(user_id, projects_id, zed_id) + // .await + // .unwrap(); + + // let result = db.get_channels_for_user(user_id).await.unwrap(); + // assert_dag( + // result.channels, + // &[ + // (zed_id, None), + // (projects_id, None), + // (livestreaming_id, Some(projects_id)), + // ], + // ); } test_both_dbs!( @@ -787,11 +796,11 @@ async fn test_user_is_channel_participant(db: &Arc) { let zed_channel = db.create_root_channel("zed", admin).await.unwrap(); let active_channel = db - .create_channel("active", Some(zed_channel), admin) + .create_sub_channel("active", zed_channel, admin) .await .unwrap(); let vim_channel = db - .create_channel("vim", Some(active_channel), admin) + .create_sub_channel("vim", active_channel, admin) .await .unwrap(); @@ -834,7 +843,7 @@ async fn test_user_is_channel_participant(db: &Arc) { &[ proto::ChannelMember { user_id: admin.to_proto(), - kind: proto::channel_member::Kind::Member.into(), + kind: proto::channel_member::Kind::AncestorMember.into(), role: proto::ChannelRole::Admin.into(), }, proto::ChannelMember { @@ -892,7 +901,7 @@ async fn test_user_is_channel_participant(db: &Arc) { &[ proto::ChannelMember { user_id: admin.to_proto(), - kind: proto::channel_member::Kind::Member.into(), + kind: proto::channel_member::Kind::AncestorMember.into(), role: proto::ChannelRole::Admin.into(), }, proto::ChannelMember { @@ -933,7 +942,7 @@ async fn test_user_is_channel_participant(db: &Arc) { &[ proto::ChannelMember { user_id: admin.to_proto(), - kind: proto::channel_member::Kind::Member.into(), + kind: proto::channel_member::Kind::AncestorMember.into(), role: proto::ChannelRole::Admin.into(), }, proto::ChannelMember { @@ -981,7 +990,7 @@ async fn test_user_is_channel_participant(db: &Arc) { &[ proto::ChannelMember { user_id: admin.to_proto(), - kind: proto::channel_member::Kind::Member.into(), + kind: proto::channel_member::Kind::AncestorMember.into(), role: proto::ChannelRole::Admin.into(), }, proto::ChannelMember { @@ -1016,17 +1025,17 @@ async fn test_user_joins_correct_channel(db: &Arc) { let zed_channel = db.create_root_channel("zed", admin).await.unwrap(); let active_channel = db - .create_channel("active", Some(zed_channel), admin) + .create_sub_channel("active", zed_channel, admin) .await .unwrap(); let vim_channel = db - .create_channel("vim", Some(active_channel), admin) + .create_sub_channel("vim", active_channel, admin) .await .unwrap(); let vim2_channel = db - .create_channel("vim2", Some(vim_channel), admin) + .create_sub_channel("vim2", vim_channel, admin) .await .unwrap(); @@ -1043,11 +1052,15 @@ async fn test_user_joins_correct_channel(db: &Arc) { .unwrap(); let most_public = db - .public_path_to_channel(vim_channel) + .transaction(|tx| async move { + Ok(db + .public_path_to_channel(vim_channel, &tx) + .await? + .first() + .cloned()) + }) .await - .unwrap() - .first() - .cloned(); + .unwrap(); assert_eq!(most_public, Some(zed_channel)) } diff --git a/crates/collab/src/db/tests/message_tests.rs b/crates/collab/src/db/tests/message_tests.rs index 272d8e01009ac8758e887d2c0ec4f04570464923..f9d97e2181be4878fb44c6186a5cc92202c31a09 100644 --- a/crates/collab/src/db/tests/message_tests.rs +++ b/crates/collab/src/db/tests/message_tests.rs @@ -25,7 +25,7 @@ async fn test_channel_message_retrieval(db: &Arc) { .await .unwrap() .user_id; - let channel = db.create_channel("channel", None, user).await.unwrap(); + let channel = db.create_root_channel("channel", user).await.unwrap(); let owner_id = db.create_server("test").await.unwrap().0 as u32; db.join_channel_chat(channel, rpc::ConnectionId { owner_id, id: 0 }, user) @@ -87,7 +87,7 @@ async fn test_channel_message_nonces(db: &Arc) { .await .unwrap() .user_id; - let channel = db.create_channel("channel", None, user).await.unwrap(); + let channel = db.create_root_channel("channel", user).await.unwrap(); let owner_id = db.create_server("test").await.unwrap().0 as u32; @@ -151,9 +151,9 @@ async fn test_channel_message_new_notification(db: &Arc) { .unwrap() .user_id; - let channel_1 = db.create_channel("channel", None, user).await.unwrap(); + let channel_1 = db.create_root_channel("channel", user).await.unwrap(); - let channel_2 = db.create_channel("channel-2", None, user).await.unwrap(); + let channel_2 = db.create_root_channel("channel-2", user).await.unwrap(); db.invite_channel_member(channel_1, observer, user, ChannelRole::Member) .await diff --git a/crates/collab/src/rpc.rs b/crates/collab/src/rpc.rs index 18fbc0d7bc227fc197cac739cce3ee6b53a17e2f..f8648a2b148a32398bf52da1d07d6563032cb2a2 100644 --- a/crates/collab/src/rpc.rs +++ b/crates/collab/src/rpc.rs @@ -3,8 +3,9 @@ mod connection_pool; use crate::{ auth, db::{ - self, BufferId, ChannelId, ChannelRole, ChannelVisibility, ChannelsForUser, Database, - MessageId, ProjectId, RoomId, ServerId, User, UserId, + self, BufferId, ChannelId, ChannelsForUser, CreateChannelResult, Database, MessageId, + MoveChannelResult, ProjectId, RenameChannelResult, RoomId, ServerId, + SetChannelVisibilityResult, User, UserId, }, executor::Executor, AppState, Result, @@ -590,7 +591,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_initial_channels_update( + this.peer.send(connection_id, build_channels_update( channels_for_user, channel_invites ))?; @@ -2202,31 +2203,21 @@ async fn create_channel( let db = session.db().await; let parent_id = request.parent_id.map(|id| ChannelId::from_proto(id)); - let id = db + let CreateChannelResult { + channel, + participants_to_update, + } = db .create_channel(&request.name, parent_id, session.user_id) .await?; response.send(proto::CreateChannelResponse { - channel: Some(proto::Channel { - id: id.to_proto(), - name: request.name, - visibility: proto::ChannelVisibility::Members as i32, - role: proto::ChannelRole::Admin.into(), - }), + channel: Some(channel.to_proto()), parent_id: request.parent_id, })?; - let Some(parent_id) = parent_id else { - return Ok(()); - }; - - let updates = db - .participants_to_notify_for_channel_change(parent_id, session.user_id) - .await?; - let connection_pool = session.connection_pool().await; - for (user_id, channels) in updates { - let update = build_initial_channels_update(channels, vec![]); + 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; @@ -2340,49 +2331,21 @@ async fn set_channel_visibility( let channel_id = ChannelId::from_proto(request.channel_id); let visibility = request.visibility().into(); - let previous_members = db - .get_channel_participant_details(channel_id, session.user_id) - .await?; - - db.set_channel_visibility(channel_id, visibility, session.user_id) + let SetChannelVisibilityResult { + participants_to_update, + participants_to_remove, + } = db + .set_channel_visibility(channel_id, visibility, session.user_id) .await?; - let mut updates: HashMap = db - .participants_to_notify_for_channel_change(channel_id, session.user_id) - .await? - .into_iter() - .collect(); - - let mut participants_who_lost_access: HashSet = HashSet::default(); - match visibility { - ChannelVisibility::Members => { - for member in previous_members { - if ChannelRole::from(member.role()).can_only_see_public_descendants() { - participants_who_lost_access.insert(UserId::from_proto(member.user_id)); - } - } - } - ChannelVisibility::Public => { - if let Some(public_parent_id) = db.public_parent_channel_id(channel_id).await? { - let parent_updates = db - .participants_to_notify_for_channel_change(public_parent_id, session.user_id) - .await?; - - for (user_id, channels) in parent_updates { - updates.insert(user_id, channels); - } - } - } - } - let connection_pool = session.connection_pool().await; - for (user_id, channels) in updates { - let update = build_initial_channels_update(channels, vec![]); + 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_who_lost_access { + for user_id in participants_to_remove { let update = proto::UpdateChannels { delete_channels: vec![channel_id.to_proto()], ..Default::default() @@ -2416,7 +2379,7 @@ async fn set_channel_member_role( let mut update = proto::UpdateChannels::default(); if channel_member.accepted { let channels = db.get_channel_for_user(channel_id, member_id).await?; - update = build_initial_channels_update(channels, vec![]); + update = build_channels_update(channels, vec![]); } else { let channel = db.get_channel(channel_id, session.user_id).await?; update.channel_invitations.push(proto::Channel { @@ -2446,34 +2409,24 @@ async fn rename_channel( ) -> Result<()> { let db = session.db().await; let channel_id = ChannelId::from_proto(request.channel_id); - let channel = db + let RenameChannelResult { + channel, + participants_to_update, + } = db .rename_channel(channel_id, session.user_id, &request.name) .await?; response.send(proto::RenameChannelResponse { - channel: Some(proto::Channel { - id: channel.id.to_proto(), - name: channel.name.clone(), - visibility: channel.visibility.into(), - role: proto::ChannelRole::Admin.into(), - }), + channel: Some(channel.to_proto()), })?; - let members = db - .get_channel_participant_details(channel_id, session.user_id) - .await?; - let connection_pool = session.connection_pool().await; - for member in members { - for connection_id in connection_pool.user_connection_ids(UserId::from_proto(member.user_id)) - { - let mut update = proto::UpdateChannels::default(); - update.channels.push(proto::Channel { - id: channel.id.to_proto(), - name: channel.name.clone(), - visibility: channel.visibility.into(), - role: member.role.into(), - }); + 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() + }; session.peer.send(connection_id, update.clone())?; } @@ -2493,25 +2446,12 @@ async fn link_channel( let channel_id = ChannelId::from_proto(request.channel_id); let to = ChannelId::from_proto(request.to); - // TODO: Remove this restriction once we have symlinks - db.assert_root_channel(channel_id).await?; - - db.link_channel(session.user_id, channel_id, to).await?; - - let member_updates = db - .participants_to_notify_for_channel_change(to, session.user_id) + let result = db + .move_channel(channel_id, None, to, session.user_id) .await?; + drop(db); - dbg!(&member_updates); - - let connection_pool = session.connection_pool().await; - - for (member_id, channels) in member_updates { - let update = build_initial_channels_update(channels, vec![]); - for connection_id in connection_pool.user_connection_ids(member_id) { - session.peer.send(connection_id, update.clone())?; - } - } + notify_channel_moved(result, session).await?; response.send(Ack {})?; @@ -2537,64 +2477,46 @@ async fn move_channel( let from_parent = ChannelId::from_proto(request.from); let to = ChannelId::from_proto(request.to); - let previous_participants = db - .get_channel_participant_details(channel_id, session.user_id) + let result = db + .move_channel(channel_id, Some(from_parent), to, session.user_id) .await?; + drop(db); - debug_assert_eq!(db.parent_channel_id(channel_id).await?, Some(from_parent)); + notify_channel_moved(result, session).await?; - let channels_to_send = db - .move_channel(session.user_id, channel_id, from_parent, to) - .await?; + response.send(Ack {})?; + Ok(()) +} - if channels_to_send.is_empty() { - response.send(Ack {})?; +async fn notify_channel_moved(result: Option, session: Session) -> Result<()> { + let Some(MoveChannelResult { + participants_to_remove, + participants_to_update, + moved_channels, + }) = result + else { return Ok(()); - } - - let updates = db - .participants_to_notify_for_channel_change(to, session.user_id) - .await?; - - let mut participants_who_lost_access: HashSet = HashSet::default(); - let mut channels_to_delete = db.get_channel_descendant_ids(channel_id).await?; - channels_to_delete.insert(channel_id); - - for previous_participant in previous_participants.iter() { - let user_id = UserId::from_proto(previous_participant.user_id); - if previous_participant.kind() == proto::channel_member::Kind::AncestorMember { - participants_who_lost_access.insert(user_id); - } - } + }; + let moved_channels: Vec = moved_channels.iter().map(|id| id.to_proto()).collect(); let connection_pool = session.connection_pool().await; - for (user_id, channels) in updates { - let mut update = build_initial_channels_update(channels, vec![]); - update.delete_channels = channels_to_delete - .iter() - .map(|channel_id| channel_id.to_proto()) - .collect(); - participants_who_lost_access.remove(&user_id); + 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_who_lost_access { + for user_id in participants_to_remove { let update = proto::UpdateChannels { - delete_channels: channels_to_delete - .iter() - .map(|channel_id| channel_id.to_proto()) - .collect(), + 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())?; } } - - response.send(Ack {})?; - Ok(()) } @@ -2641,38 +2563,12 @@ async fn channel_membership_updated( channel_id: ChannelId, session: &Session, ) -> Result<(), crate::Error> { - let mut update = proto::UpdateChannels::default(); + let result = db.get_channel_for_user(channel_id, session.user_id).await?; + let mut update = build_channels_update(result, vec![]); update .remove_channel_invitations .push(channel_id.to_proto()); - let result = db.get_channel_for_user(channel_id, session.user_id).await?; - update.channels.extend( - result - .channels - .channels - .into_iter() - .map(|channel| proto::Channel { - id: channel.id.to_proto(), - visibility: channel.visibility.into(), - role: channel.role.into(), - name: channel.name, - }), - ); - update.unseen_channel_messages = result.channel_messages; - update.unseen_channel_buffer_changes = result.unseen_buffer_changes; - update.insert_edge = result.channels.edges; - update - .channel_participants - .extend( - result - .channel_participants - .into_iter() - .map(|(channel_id, user_ids)| proto::ChannelParticipants { - channel_id: channel_id.to_proto(), - participant_user_ids: user_ids.into_iter().map(UserId::to_proto).collect(), - }), - ); session.peer.send(session.connection_id, update)?; Ok(()) } @@ -3155,7 +3051,7 @@ fn to_tungstenite_message(message: AxumMessage) -> TungsteniteMessage { } } -fn build_initial_channels_update( +fn build_channels_update( channels: ChannelsForUser, channel_invites: Vec, ) -> proto::UpdateChannels { diff --git a/crates/collab/src/tests/random_channel_buffer_tests.rs b/crates/collab/src/tests/random_channel_buffer_tests.rs index 1b24c7a3d2f45e5e374c5a70cd54a426b4043753..f4eca6b5abf49e41a51fa02319299d386d0b852b 100644 --- a/crates/collab/src/tests/random_channel_buffer_tests.rs +++ b/crates/collab/src/tests/random_channel_buffer_tests.rs @@ -48,7 +48,7 @@ impl RandomizedTest for RandomChannelBufferTest { let db = &server.app_state.db; for ix in 0..CHANNEL_COUNT { let id = db - .create_channel(&format!("channel-{ix}"), None, users[0].user_id) + .create_root_channel(&format!("channel-{ix}"), users[0].user_id) .await .unwrap(); for user in &users[1..] { diff --git a/crates/collab/src/tests/test_server.rs b/crates/collab/src/tests/test_server.rs index c37ea19d528d9f30ef4cc69de31b3c3d2370a86a..81d86943fce6e7fb89248a2f9645b72d63ee118b 100644 --- a/crates/collab/src/tests/test_server.rs +++ b/crates/collab/src/tests/test_server.rs @@ -604,38 +604,6 @@ impl TestClient { ) -> WindowHandle { cx.add_window(|cx| Workspace::new(0, project.clone(), self.app_state.clone(), cx)) } - - pub async fn add_admin_to_channel( - &self, - user: (&TestClient, &mut TestAppContext), - channel: u64, - cx_self: &mut TestAppContext, - ) { - let (other_client, other_cx) = user; - - cx_self - .read(ChannelStore::global) - .update(cx_self, |channel_store, cx| { - channel_store.invite_member( - channel, - other_client.user_id().unwrap(), - ChannelRole::Admin, - cx, - ) - }) - .await - .unwrap(); - - cx_self.foreground().run_until_parked(); - - other_cx - .read(ChannelStore::global) - .update(other_cx, |channel_store, _| { - channel_store.respond_to_channel_invite(channel, true) - }) - .await - .unwrap(); - } } impl Drop for TestClient { diff --git a/crates/collab_ui/src/collab_panel.rs b/crates/collab_ui/src/collab_panel.rs index 70ea87cfdd2e31789743b16f848dd516ed5e413b..1d1092be6e73ba4c6bdbd2e88df24693e1b21633 100644 --- a/crates/collab_ui/src/collab_panel.rs +++ b/crates/collab_ui/src/collab_panel.rs @@ -2662,7 +2662,6 @@ impl CollabPanel { location: path.clone(), }, ), - ContextMenuItem::Separator, ContextMenuItem::action( "Move this channel", StartMoveChannelFor { diff --git a/crates/rpc/proto/zed.proto b/crates/rpc/proto/zed.proto index 1bf54dedb70be5cdffa6a89d524a50f4d3956407..20a47954a451a9ce94cc560a2e69c9a6b9bc6caa 100644 --- a/crates/rpc/proto/zed.proto +++ b/crates/rpc/proto/zed.proto @@ -970,16 +970,10 @@ message UpdateChannels { repeated Channel channel_invitations = 5; repeated uint64 remove_channel_invitations = 6; repeated ChannelParticipants channel_participants = 7; - //repeated ChannelRoles channel_roles = 8; repeated UnseenChannelMessage unseen_channel_messages = 9; repeated UnseenChannelBufferChange unseen_channel_buffer_changes = 10; } -//message ChannelRoles { -// ChannelRole role = 1; -// uint64 channel_id = 2; -//} - message UnseenChannelMessage { uint64 channel_id = 1; uint64 message_id = 2; From 0eff7c6ca93267da4502bbc13ae0e4b8ca1c6e0b Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Thu, 19 Oct 2023 13:03:44 -0600 Subject: [PATCH 07/12] Add read-only channel notes support Fix some bugs where ChannelNotes and ChannelChat had old cached channel instances --- Cargo.lock | 1 + assets/keymaps/default.json | 198 ++++-------------- crates/call/src/room.rs | 2 +- crates/channel/src/channel_buffer.rs | 28 ++- crates/channel/src/channel_chat.rs | 27 +-- crates/channel/src/channel_store.rs | 46 +++- .../src/channel_store/channel_index.rs | 21 +- .../collab/src/tests/channel_buffer_tests.rs | 4 +- .../src/tests/random_channel_buffer_tests.rs | 13 +- crates/collab_ui/Cargo.toml | 1 + crates/collab_ui/src/channel_view.rs | 46 +++- crates/collab_ui/src/chat_panel.rs | 16 +- 12 files changed, 185 insertions(+), 218 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ecbe076711ccbf9c0657e828930d8e57442efc3c..d827b314c82b5417dd24e2bf75c2e98e259302b9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1569,6 +1569,7 @@ dependencies = [ "serde", "serde_derive", "settings", + "smallvec", "theme", "theme_selector", "time", diff --git a/assets/keymaps/default.json b/assets/keymaps/default.json index 8422d53abc9143fb88ee92d2ad87ec944126e4cd..ef6a655bdcead3cd64f29e9aa25b90d0d4e4d626 100644 --- a/assets/keymaps/default.json +++ b/assets/keymaps/default.json @@ -370,42 +370,15 @@ { "context": "Pane", "bindings": { - "ctrl-1": [ - "pane::ActivateItem", - 0 - ], - "ctrl-2": [ - "pane::ActivateItem", - 1 - ], - "ctrl-3": [ - "pane::ActivateItem", - 2 - ], - "ctrl-4": [ - "pane::ActivateItem", - 3 - ], - "ctrl-5": [ - "pane::ActivateItem", - 4 - ], - "ctrl-6": [ - "pane::ActivateItem", - 5 - ], - "ctrl-7": [ - "pane::ActivateItem", - 6 - ], - "ctrl-8": [ - "pane::ActivateItem", - 7 - ], - "ctrl-9": [ - "pane::ActivateItem", - 8 - ], + "ctrl-1": ["pane::ActivateItem", 0], + "ctrl-2": ["pane::ActivateItem", 1], + "ctrl-3": ["pane::ActivateItem", 2], + "ctrl-4": ["pane::ActivateItem", 3], + "ctrl-5": ["pane::ActivateItem", 4], + "ctrl-6": ["pane::ActivateItem", 5], + "ctrl-7": ["pane::ActivateItem", 6], + "ctrl-8": ["pane::ActivateItem", 7], + "ctrl-9": ["pane::ActivateItem", 8], "ctrl-0": "pane::ActivateLastItem", "ctrl--": "pane::GoBack", "ctrl-_": "pane::GoForward", @@ -416,42 +389,15 @@ { "context": "Workspace", "bindings": { - "cmd-1": [ - "workspace::ActivatePane", - 0 - ], - "cmd-2": [ - "workspace::ActivatePane", - 1 - ], - "cmd-3": [ - "workspace::ActivatePane", - 2 - ], - "cmd-4": [ - "workspace::ActivatePane", - 3 - ], - "cmd-5": [ - "workspace::ActivatePane", - 4 - ], - "cmd-6": [ - "workspace::ActivatePane", - 5 - ], - "cmd-7": [ - "workspace::ActivatePane", - 6 - ], - "cmd-8": [ - "workspace::ActivatePane", - 7 - ], - "cmd-9": [ - "workspace::ActivatePane", - 8 - ], + "cmd-1": ["workspace::ActivatePane", 0], + "cmd-2": ["workspace::ActivatePane", 1], + "cmd-3": ["workspace::ActivatePane", 2], + "cmd-4": ["workspace::ActivatePane", 3], + "cmd-5": ["workspace::ActivatePane", 4], + "cmd-6": ["workspace::ActivatePane", 5], + "cmd-7": ["workspace::ActivatePane", 6], + "cmd-8": ["workspace::ActivatePane", 7], + "cmd-9": ["workspace::ActivatePane", 8], "cmd-b": "workspace::ToggleLeftDock", "cmd-r": "workspace::ToggleRightDock", "cmd-j": "workspace::ToggleBottomDock", @@ -494,38 +440,14 @@ }, { "bindings": { - "cmd-k cmd-left": [ - "workspace::ActivatePaneInDirection", - "Left" - ], - "cmd-k cmd-right": [ - "workspace::ActivatePaneInDirection", - "Right" - ], - "cmd-k cmd-up": [ - "workspace::ActivatePaneInDirection", - "Up" - ], - "cmd-k cmd-down": [ - "workspace::ActivatePaneInDirection", - "Down" - ], - "cmd-k shift-left": [ - "workspace::SwapPaneInDirection", - "Left" - ], - "cmd-k shift-right": [ - "workspace::SwapPaneInDirection", - "Right" - ], - "cmd-k shift-up": [ - "workspace::SwapPaneInDirection", - "Up" - ], - "cmd-k shift-down": [ - "workspace::SwapPaneInDirection", - "Down" - ] + "cmd-k cmd-left": ["workspace::ActivatePaneInDirection", "Left"], + "cmd-k cmd-right": ["workspace::ActivatePaneInDirection", "Right"], + "cmd-k cmd-up": ["workspace::ActivatePaneInDirection", "Up"], + "cmd-k cmd-down": ["workspace::ActivatePaneInDirection", "Down"], + "cmd-k shift-left": ["workspace::SwapPaneInDirection", "Left"], + "cmd-k shift-right": ["workspace::SwapPaneInDirection", "Right"], + "cmd-k shift-up": ["workspace::SwapPaneInDirection", "Up"], + "cmd-k shift-down": ["workspace::SwapPaneInDirection", "Down"] } }, // Bindings from Atom @@ -627,14 +549,6 @@ "space": "collab_panel::InsertSpace" } }, - { - "context": "(CollabPanel && not_editing) > Editor", - "bindings": { - "cmd-c": "collab_panel::StartLinkChannel", - "cmd-x": "collab_panel::StartMoveChannel", - "cmd-v": "collab_panel::MoveOrLinkToSelected" - } - }, { "context": "ChannelModal", "bindings": { @@ -655,57 +569,21 @@ "cmd-v": "terminal::Paste", "cmd-k": "terminal::Clear", // Some nice conveniences - "cmd-backspace": [ - "terminal::SendText", - "\u0015" - ], - "cmd-right": [ - "terminal::SendText", - "\u0005" - ], - "cmd-left": [ - "terminal::SendText", - "\u0001" - ], + "cmd-backspace": ["terminal::SendText", "\u0015"], + "cmd-right": ["terminal::SendText", "\u0005"], + "cmd-left": ["terminal::SendText", "\u0001"], // Terminal.app compatibility - "alt-left": [ - "terminal::SendText", - "\u001bb" - ], - "alt-right": [ - "terminal::SendText", - "\u001bf" - ], + "alt-left": ["terminal::SendText", "\u001bb"], + "alt-right": ["terminal::SendText", "\u001bf"], // There are conflicting bindings for these keys in the global context. // these bindings override them, remove at your own risk: - "up": [ - "terminal::SendKeystroke", - "up" - ], - "pageup": [ - "terminal::SendKeystroke", - "pageup" - ], - "down": [ - "terminal::SendKeystroke", - "down" - ], - "pagedown": [ - "terminal::SendKeystroke", - "pagedown" - ], - "escape": [ - "terminal::SendKeystroke", - "escape" - ], - "enter": [ - "terminal::SendKeystroke", - "enter" - ], - "ctrl-c": [ - "terminal::SendKeystroke", - "ctrl-c" - ] + "up": ["terminal::SendKeystroke", "up"], + "pageup": ["terminal::SendKeystroke", "pageup"], + "down": ["terminal::SendKeystroke", "down"], + "pagedown": ["terminal::SendKeystroke", "pagedown"], + "escape": ["terminal::SendKeystroke", "escape"], + "enter": ["terminal::SendKeystroke", "enter"], + "ctrl-c": ["terminal::SendKeystroke", "ctrl-c"] } } ] diff --git a/crates/call/src/room.rs b/crates/call/src/room.rs index 4e52f57f60b36c0f83400279c2feb45ee9e2b713..86045e981a73aad6ce6ce7741ff9d3b246405265 100644 --- a/crates/call/src/room.rs +++ b/crates/call/src/room.rs @@ -55,7 +55,7 @@ pub enum Event { pub struct Room { id: u64, - channel_id: Option, + pub channel_id: Option, live_kit: Option, status: RoomStatus, shared_projects: HashSet>, diff --git a/crates/channel/src/channel_buffer.rs b/crates/channel/src/channel_buffer.rs index ab7ea78ac1b70e5c5b0c028289f2b9f55fe924f9..9089973d3203696e6646d9249125ce6fbfdd541b 100644 --- a/crates/channel/src/channel_buffer.rs +++ b/crates/channel/src/channel_buffer.rs @@ -1,4 +1,4 @@ -use crate::Channel; +use crate::{Channel, ChannelId, ChannelStore}; use anyhow::Result; use client::{Client, Collaborator, UserStore}; use collections::HashMap; @@ -19,10 +19,11 @@ pub(crate) fn init(client: &Arc) { } pub struct ChannelBuffer { - pub(crate) channel: Arc, + pub channel_id: ChannelId, connected: bool, collaborators: HashMap, user_store: ModelHandle, + channel_store: ModelHandle, buffer: ModelHandle, buffer_epoch: u64, client: Arc, @@ -34,6 +35,7 @@ pub enum ChannelBufferEvent { CollaboratorsChanged, Disconnected, BufferEdited, + ChannelChanged, } impl Entity for ChannelBuffer { @@ -46,7 +48,7 @@ impl Entity for ChannelBuffer { } self.client .send(proto::LeaveChannelBuffer { - channel_id: self.channel.id, + channel_id: self.channel_id, }) .log_err(); } @@ -58,6 +60,7 @@ impl ChannelBuffer { channel: Arc, client: Arc, user_store: ModelHandle, + channel_store: ModelHandle, mut cx: AsyncAppContext, ) -> Result> { let response = client @@ -90,9 +93,10 @@ impl ChannelBuffer { connected: true, collaborators: Default::default(), acknowledge_task: None, - channel, + channel_id: channel.id, subscription: Some(subscription.set_model(&cx.handle(), &mut cx.to_async())), user_store, + channel_store, }; this.replace_collaborators(response.collaborators, cx); this @@ -179,7 +183,7 @@ impl ChannelBuffer { let operation = language::proto::serialize_operation(operation); self.client .send(proto::UpdateChannelBuffer { - channel_id: self.channel.id, + channel_id: self.channel_id, operations: vec![operation], }) .log_err(); @@ -223,12 +227,15 @@ impl ChannelBuffer { &self.collaborators } - pub fn channel(&self) -> Arc { - self.channel.clone() + pub fn channel(&self, cx: &AppContext) -> Option> { + self.channel_store + .read(cx) + .channel_for_id(self.channel_id) + .cloned() } pub(crate) fn disconnect(&mut self, cx: &mut ModelContext) { - log::info!("channel buffer {} disconnected", self.channel.id); + log::info!("channel buffer {} disconnected", self.channel_id); if self.connected { self.connected = false; self.subscription.take(); @@ -237,6 +244,11 @@ impl ChannelBuffer { } } + pub(crate) fn channel_changed(&mut self, cx: &mut ModelContext) { + cx.emit(ChannelBufferEvent::ChannelChanged); + cx.notify() + } + pub fn is_connected(&self) -> bool { self.connected } diff --git a/crates/channel/src/channel_chat.rs b/crates/channel/src/channel_chat.rs index 734182886b3bebeacd03dbc177bf8ffcb8ab64e2..299c91759cdf2277a9603ea71601c76a78c81d40 100644 --- a/crates/channel/src/channel_chat.rs +++ b/crates/channel/src/channel_chat.rs @@ -14,7 +14,7 @@ use time::OffsetDateTime; use util::{post_inc, ResultExt as _, TryFutureExt}; pub struct ChannelChat { - channel: Arc, + pub channel_id: ChannelId, messages: SumTree, channel_store: ModelHandle, loaded_all_messages: bool, @@ -74,7 +74,7 @@ impl Entity for ChannelChat { fn release(&mut self, _: &mut AppContext) { self.rpc .send(proto::LeaveChannelChat { - channel_id: self.channel.id, + channel_id: self.channel_id, }) .log_err(); } @@ -99,7 +99,7 @@ impl ChannelChat { Ok(cx.add_model(|cx| { let mut this = Self { - channel, + channel_id: channel.id, user_store, channel_store, rpc: client, @@ -116,8 +116,11 @@ impl ChannelChat { })) } - pub fn channel(&self) -> &Arc { - &self.channel + pub fn channel(&self, cx: &AppContext) -> Option> { + self.channel_store + .read(cx) + .channel_for_id(self.channel_id) + .cloned() } pub fn send_message( @@ -135,7 +138,7 @@ impl ChannelChat { .current_user() .ok_or_else(|| anyhow!("current_user is not present"))?; - let channel_id = self.channel.id; + let channel_id = self.channel_id; let pending_id = ChannelMessageId::Pending(post_inc(&mut self.next_pending_message_id)); let nonce = self.rng.gen(); self.insert_messages( @@ -178,7 +181,7 @@ impl ChannelChat { pub fn remove_message(&mut self, id: u64, cx: &mut ModelContext) -> Task> { let response = self.rpc.request(proto::RemoveChannelMessage { - channel_id: self.channel.id, + channel_id: self.channel_id, message_id: id, }); cx.spawn(|this, mut cx| async move { @@ -195,7 +198,7 @@ impl ChannelChat { if !self.loaded_all_messages { let rpc = self.rpc.clone(); let user_store = self.user_store.clone(); - let channel_id = self.channel.id; + let channel_id = self.channel_id; if let Some(before_message_id) = self.messages.first().and_then(|message| match message.id { ChannelMessageId::Saved(id) => Some(id), @@ -236,13 +239,13 @@ impl ChannelChat { { self.rpc .send(proto::AckChannelMessage { - channel_id: self.channel.id, + channel_id: self.channel_id, message_id: latest_message_id, }) .ok(); self.last_acknowledged_id = Some(latest_message_id); self.channel_store.update(cx, |store, cx| { - store.acknowledge_message_id(self.channel.id, latest_message_id, cx); + store.acknowledge_message_id(self.channel_id, latest_message_id, cx); }); } } @@ -251,7 +254,7 @@ impl ChannelChat { pub fn rejoin(&mut self, cx: &mut ModelContext) { let user_store = self.user_store.clone(); let rpc = self.rpc.clone(); - let channel_id = self.channel.id; + let channel_id = self.channel_id; cx.spawn(|this, mut cx| { async move { let response = rpc.request(proto::JoinChannelChat { channel_id }).await?; @@ -348,7 +351,7 @@ impl ChannelChat { this.update(&mut cx, |this, cx| { this.insert_messages(SumTree::from_item(message, &()), cx); cx.emit(ChannelChatEvent::NewMessage { - channel_id: this.channel.id, + channel_id: this.channel_id, message_id, }) }); diff --git a/crates/channel/src/channel_store.rs b/crates/channel/src/channel_store.rs index 4891b5a18ede737c43836d2dd401c79a05844561..82cb0432d39075a417ebb884eb3aefe3085a8776 100644 --- a/crates/channel/src/channel_store.rs +++ b/crates/channel/src/channel_store.rs @@ -72,6 +72,10 @@ impl Channel { slug.trim_matches(|c| c == '-').to_string() } + + pub fn can_edit_notes(&self) -> bool { + self.role == proto::ChannelRole::Member || self.role == proto::ChannelRole::Admin + } } #[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Serialize, Deserialize)] @@ -265,10 +269,11 @@ impl ChannelStore { ) -> Task>> { let client = self.client.clone(); let user_store = self.user_store.clone(); + let channel_store = cx.handle(); self.open_channel_resource( channel_id, |this| &mut this.opened_buffers, - |channel, cx| ChannelBuffer::new(channel, client, user_store, cx), + |channel, cx| ChannelBuffer::new(channel, client, user_store, channel_store, cx), cx, ) } @@ -778,7 +783,7 @@ impl ChannelStore { let channel_buffer = buffer.read(cx); let buffer = channel_buffer.buffer().read(cx); buffer_versions.push(proto::ChannelBufferVersion { - channel_id: channel_buffer.channel().id, + channel_id: channel_buffer.channel_id, epoch: channel_buffer.epoch(), version: language::proto::serialize_version(&buffer.version()), }); @@ -805,13 +810,13 @@ impl ChannelStore { }; channel_buffer.update(cx, |channel_buffer, cx| { - let channel_id = channel_buffer.channel().id; + let channel_id = channel_buffer.channel_id; if let Some(remote_buffer) = response .buffers .iter_mut() .find(|buffer| buffer.channel_id == channel_id) { - let channel_id = channel_buffer.channel().id; + let channel_id = channel_buffer.channel_id; let remote_version = language::proto::deserialize_version(&remote_buffer.version); @@ -934,11 +939,27 @@ impl ChannelStore { if channels_changed { if !payload.delete_channels.is_empty() { - self.channel_index.delete_channels(&payload.delete_channels); + let mut channels_to_delete: Vec = Vec::new(); + let mut channels_to_rehome: Vec = Vec::new(); + for channel_id in payload.delete_channels { + if payload + .channels + .iter() + .any(|channel| channel.id == channel_id) + { + channels_to_rehome.push(channel_id) + } else { + channels_to_delete.push(channel_id) + } + } + + self.channel_index.delete_channels(&channels_to_delete); + self.channel_index + .delete_paths_through_channels(&channels_to_rehome); self.channel_participants - .retain(|channel_id, _| !payload.delete_channels.contains(channel_id)); + .retain(|channel_id, _| !channels_to_delete.contains(channel_id)); - for channel_id in &payload.delete_channels { + for channel_id in &channels_to_delete { let channel_id = *channel_id; if payload .channels @@ -959,7 +980,16 @@ impl ChannelStore { let mut index = self.channel_index.bulk_insert(); for channel in payload.channels { - index.insert(channel) + let id = channel.id; + let channel_changed = index.insert(channel); + + if channel_changed { + if let Some(OpenedModelHandle::Open(buffer)) = self.opened_buffers.get(&id) { + if let Some(buffer) = buffer.upgrade(cx) { + buffer.update(cx, ChannelBuffer::channel_changed); + } + } + } } for unseen_buffer_change in payload.unseen_channel_buffer_changes { diff --git a/crates/channel/src/channel_store/channel_index.rs b/crates/channel/src/channel_store/channel_index.rs index 54de15974ed50268be9bb9eaf1ed84dc80ca4d6e..e5dc75c8b91c6e23f3eafd26840e7328f7fc2603 100644 --- a/crates/channel/src/channel_store/channel_index.rs +++ b/crates/channel/src/channel_store/channel_index.rs @@ -24,12 +24,16 @@ impl ChannelIndex { /// Delete the given channels from this index. pub fn delete_channels(&mut self, channels: &[ChannelId]) { + dbg!("delete_channels", &channels); self.channels_by_id .retain(|channel_id, _| !channels.contains(channel_id)); - self.paths.retain(|path| { - path.iter() - .all(|channel_id| self.channels_by_id.contains_key(channel_id)) - }); + self.delete_paths_through_channels(channels) + } + + pub fn delete_paths_through_channels(&mut self, channels: &[ChannelId]) { + dbg!("rehome_channels", &channels); + self.paths + .retain(|path| !path.iter().any(|channel_id| channels.contains(channel_id))); } pub fn bulk_insert(&mut self) -> ChannelPathsInsertGuard { @@ -121,9 +125,15 @@ impl<'a> ChannelPathsInsertGuard<'a> { insert_new_message(&mut self.channels_by_id, channel_id, message_id) } - pub fn insert(&mut self, channel_proto: proto::Channel) { + pub fn insert(&mut self, channel_proto: proto::Channel) -> bool { + let mut ret = false; if let Some(existing_channel) = self.channels_by_id.get_mut(&channel_proto.id) { 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.visibility = channel_proto.visibility(); existing_channel.role = channel_proto.role(); existing_channel.name = channel_proto.name; @@ -141,6 +151,7 @@ impl<'a> ChannelPathsInsertGuard<'a> { ); self.insert_root(channel_proto.id); } + ret } pub fn insert_edge(&mut self, channel_id: ChannelId, parent_id: ChannelId) { diff --git a/crates/collab/src/tests/channel_buffer_tests.rs b/crates/collab/src/tests/channel_buffer_tests.rs index f2ab2a9d094f9a41d08f38076d8d194011aa84cf..01174fe3beceebada80d7f09710903d012a2b443 100644 --- a/crates/collab/src/tests/channel_buffer_tests.rs +++ b/crates/collab/src/tests/channel_buffer_tests.rs @@ -412,7 +412,7 @@ async fn test_channel_buffer_disconnect( channel_buffer_a.update(cx_a, |buffer, _| { assert_eq!( - buffer.channel().as_ref(), + buffer.channel(cx).unwrap().as_ref(), &channel(channel_id, "the-channel", proto::ChannelRole::Admin) ); assert!(!buffer.is_connected()); @@ -437,7 +437,7 @@ async fn test_channel_buffer_disconnect( // Channel buffer observed the deletion channel_buffer_b.update(cx_b, |buffer, _| { assert_eq!( - buffer.channel().as_ref(), + buffer.channel(cx).unwrap().as_ref(), &channel(channel_id, "the-channel", proto::ChannelRole::Member) ); assert!(!buffer.is_connected()); diff --git a/crates/collab/src/tests/random_channel_buffer_tests.rs b/crates/collab/src/tests/random_channel_buffer_tests.rs index f4eca6b5abf49e41a51fa02319299d386d0b852b..9d05c3017f7c0e9fc1688b6fba0ce773fe3f7155 100644 --- a/crates/collab/src/tests/random_channel_buffer_tests.rs +++ b/crates/collab/src/tests/random_channel_buffer_tests.rs @@ -98,7 +98,8 @@ impl RandomizedTest for RandomChannelBufferTest { 30..=40 => { if let Some(buffer) = channel_buffers.iter().choose(rng) { - let channel_name = buffer.read_with(cx, |b, _| b.channel().name.clone()); + let channel_name = + buffer.read_with(cx, |b, _| b.channel(cx).unwrap().name.clone()); break ChannelBufferOperation::LeaveChannelNotes { channel_name }; } } @@ -106,7 +107,7 @@ impl RandomizedTest for RandomChannelBufferTest { _ => { if let Some(buffer) = channel_buffers.iter().choose(rng) { break buffer.read_with(cx, |b, _| { - let channel_name = b.channel().name.clone(); + let channel_name = b.channel(cx).unwrap().name.clone(); let edits = b .buffer() .read_with(cx, |buffer, _| buffer.get_random_edits(rng, 3)); @@ -153,7 +154,7 @@ impl RandomizedTest for RandomChannelBufferTest { let buffer = cx.update(|cx| { let mut left_buffer = Err(TestError::Inapplicable); client.channel_buffers().retain(|buffer| { - if buffer.read(cx).channel().name == channel_name { + if buffer.read(cx).channel(cx).unwrap().name == channel_name { left_buffer = Ok(buffer.clone()); false } else { @@ -179,7 +180,9 @@ impl RandomizedTest for RandomChannelBufferTest { client .channel_buffers() .iter() - .find(|buffer| buffer.read(cx).channel().name == channel_name) + .find(|buffer| { + buffer.read(cx).channel(cx).unwrap().name == channel_name + }) .cloned() }) .ok_or_else(|| TestError::Inapplicable)?; @@ -250,7 +253,7 @@ impl RandomizedTest for RandomChannelBufferTest { if let Some(channel_buffer) = client .channel_buffers() .iter() - .find(|b| b.read(cx).channel().id == channel_id.to_proto()) + .find(|b| b.read(cx).channel_id == channel_id.to_proto()) { let channel_buffer = channel_buffer.read(cx); diff --git a/crates/collab_ui/Cargo.toml b/crates/collab_ui/Cargo.toml index 98790778c98d69afa90743f8e40d94aa397cf886..aac4c924f866750e4dac9f8f135a8325354e84f2 100644 --- a/crates/collab_ui/Cargo.toml +++ b/crates/collab_ui/Cargo.toml @@ -58,6 +58,7 @@ postage.workspace = true serde.workspace = true serde_derive.workspace = true time.workspace = true +smallvec.workspace = true [dev-dependencies] call = { path = "../call", features = ["test-support"] } diff --git a/crates/collab_ui/src/channel_view.rs b/crates/collab_ui/src/channel_view.rs index e62ee8ef4b7b0c091000cdfd5b272e62c6fee7f7..1bdcebd01855e00948505fa48011ccccde7565f7 100644 --- a/crates/collab_ui/src/channel_view.rs +++ b/crates/collab_ui/src/channel_view.rs @@ -15,13 +15,14 @@ use gpui::{ ViewContext, ViewHandle, }; use project::Project; +use smallvec::SmallVec; use std::{ any::{Any, TypeId}, sync::Arc, }; use util::ResultExt; use workspace::{ - item::{FollowableItem, Item, ItemHandle}, + item::{FollowableItem, Item, ItemEvent, ItemHandle}, register_followable_item, searchable::SearchableItemHandle, ItemNavHistory, Pane, SaveIntent, ViewId, Workspace, WorkspaceId, @@ -140,6 +141,12 @@ impl ChannelView { editor.set_collaboration_hub(Box::new(ChannelBufferCollaborationHub( channel_buffer.clone(), ))); + editor.set_read_only( + !channel_buffer + .read(cx) + .channel(cx) + .is_some_and(|c| c.can_edit_notes()), + ); editor }); let _editor_event_subscription = cx.subscribe(&editor, |_, _, e, cx| cx.emit(e.clone())); @@ -157,8 +164,8 @@ impl ChannelView { } } - pub fn channel(&self, cx: &AppContext) -> Arc { - self.channel_buffer.read(cx).channel() + pub fn channel(&self, cx: &AppContext) -> Option> { + self.channel_buffer.read(cx).channel(cx) } fn handle_channel_buffer_event( @@ -172,6 +179,13 @@ impl ChannelView { editor.set_read_only(true); cx.notify(); }), + ChannelBufferEvent::ChannelChanged => { + self.editor.update(cx, |editor, cx| { + editor.set_read_only(!self.channel(cx).is_some_and(|c| c.can_edit_notes())); + cx.emit(editor::Event::TitleChanged); + cx.notify() + }); + } ChannelBufferEvent::BufferEdited => { if cx.is_self_focused() || self.editor.is_focused(cx) { self.acknowledge_buffer_version(cx); @@ -179,7 +193,7 @@ impl ChannelView { self.channel_store.update(cx, |store, cx| { let channel_buffer = self.channel_buffer.read(cx); store.notes_changed( - channel_buffer.channel().id, + channel_buffer.channel_id, channel_buffer.epoch(), &channel_buffer.buffer().read(cx).version(), cx, @@ -187,7 +201,7 @@ impl ChannelView { }); } } - _ => {} + ChannelBufferEvent::CollaboratorsChanged => {} } } @@ -195,7 +209,7 @@ impl ChannelView { self.channel_store.update(cx, |store, cx| { let channel_buffer = self.channel_buffer.read(cx); store.acknowledge_notes_version( - channel_buffer.channel().id, + channel_buffer.channel_id, channel_buffer.epoch(), &channel_buffer.buffer().read(cx).version(), cx, @@ -250,11 +264,17 @@ impl Item for ChannelView { style: &theme::Tab, cx: &gpui::AppContext, ) -> AnyElement { - let channel_name = &self.channel_buffer.read(cx).channel().name; - let label = if self.channel_buffer.read(cx).is_connected() { - format!("#{}", channel_name) + let label = if let Some(channel) = self.channel(cx) { + match ( + channel.can_edit_notes(), + self.channel_buffer.read(cx).is_connected(), + ) { + (true, true) => format!("#{}", channel.name), + (false, true) => format!("#{} (read-only)", channel.name), + (_, false) => format!("#{} (disconnected)", channel.name), + } } else { - format!("#{} (disconnected)", channel_name) + format!("channel notes (disconnected)") }; Label::new(label, style.label.to_owned()).into_any() } @@ -298,6 +318,10 @@ impl Item for ChannelView { fn pixel_position_of_cursor(&self, cx: &AppContext) -> Option { self.editor.read(cx).pixel_position_of_cursor(cx) } + + fn to_item_events(event: &Self::Event) -> SmallVec<[ItemEvent; 2]> { + editor::Editor::to_item_events(event) + } } impl FollowableItem for ChannelView { @@ -313,7 +337,7 @@ impl FollowableItem for ChannelView { Some(proto::view::Variant::ChannelView( proto::view::ChannelView { - channel_id: channel_buffer.channel().id, + channel_id: channel_buffer.channel_id, editor: if let Some(proto::view::Variant::Editor(proto)) = self.editor.read(cx).to_state_proto(cx) { diff --git a/crates/collab_ui/src/chat_panel.rs b/crates/collab_ui/src/chat_panel.rs index f0a6c96cedbe363101ebb676375f6b038021f718..8f492e2e36869d0946e32ed1bd58a7e6c8d00249 100644 --- a/crates/collab_ui/src/chat_panel.rs +++ b/crates/collab_ui/src/chat_panel.rs @@ -267,11 +267,15 @@ impl ChatPanel { fn set_active_chat(&mut self, chat: ModelHandle, cx: &mut ViewContext) { if self.active_chat.as_ref().map(|e| &e.0) != Some(&chat) { - let id = chat.read(cx).channel().id; + let id = chat.read(cx).channel_id; { let chat = chat.read(cx); self.message_list.reset(chat.message_count()); - let placeholder = format!("Message #{}", chat.channel().name); + let placeholder = if let Some(channel) = chat.channel(cx) { + format!("Message #{}", channel.name) + } else { + "Message Channel".to_string() + }; self.input_editor.update(cx, move |editor, cx| { editor.set_placeholder_text(placeholder, cx); }); @@ -360,7 +364,7 @@ impl ChatPanel { let is_admin = self .channel_store .read(cx) - .is_channel_admin(active_chat.channel().id); + .is_channel_admin(active_chat.channel_id); let last_message = active_chat.message(ix.saturating_sub(1)); let this_message = active_chat.message(ix); let is_continuation = last_message.id != this_message.id @@ -645,7 +649,7 @@ impl ChatPanel { cx: &mut ViewContext, ) -> Task> { if let Some((chat, _)) = &self.active_chat { - if chat.read(cx).channel().id == selected_channel_id { + if chat.read(cx).channel_id == selected_channel_id { return Task::ready(Ok(())); } } @@ -664,7 +668,7 @@ impl ChatPanel { fn open_notes(&mut self, _: &OpenChannelNotes, cx: &mut ViewContext) { if let Some((chat, _)) = &self.active_chat { - let channel_id = chat.read(cx).channel().id; + let channel_id = chat.read(cx).channel_id; if let Some(workspace) = self.workspace.upgrade(cx) { ChannelView::open(channel_id, workspace, cx).detach(); } @@ -673,7 +677,7 @@ impl ChatPanel { fn join_call(&mut self, _: &JoinCall, cx: &mut ViewContext) { if let Some((chat, _)) = &self.active_chat { - let channel_id = chat.read(cx).channel().id; + let channel_id = chat.read(cx).channel_id; ActiveCall::global(cx) .update(cx, |call, cx| call.join_channel(channel_id, cx)) .detach_and_log_err(cx); From aa4b8d72463e2a6c497fac28f0673f4a4c7e1706 Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Thu, 19 Oct 2023 14:41:20 -0600 Subject: [PATCH 08/12] Fix notifications for membership changes too --- crates/channel/src/channel_store.rs | 22 +- .../src/channel_store/channel_index.rs | 6 - crates/collab/src/db.rs | 13 + crates/collab/src/db/queries/channels.rs | 298 +++++++++++------- crates/collab/src/db/tests/channel_tests.rs | 2 +- crates/collab/src/rpc.rs | 172 +++++----- .../collab/src/tests/channel_buffer_tests.rs | 12 +- crates/collab/src/tests/channel_tests.rs | 170 ++++++++-- .../src/tests/random_channel_buffer_tests.rs | 4 +- crates/live_kit_client/src/test.rs | 10 + crates/live_kit_server/src/api.rs | 10 + crates/live_kit_server/src/token.rs | 9 + 12 files changed, 469 insertions(+), 259 deletions(-) diff --git a/crates/channel/src/channel_store.rs b/crates/channel/src/channel_store.rs index 82cb0432d39075a417ebb884eb3aefe3085a8776..2e1e92431e256717b9c81db1c151783bc402b0c2 100644 --- a/crates/channel/src/channel_store.rs +++ b/crates/channel/src/channel_store.rs @@ -939,27 +939,11 @@ impl ChannelStore { if channels_changed { if !payload.delete_channels.is_empty() { - let mut channels_to_delete: Vec = Vec::new(); - let mut channels_to_rehome: Vec = Vec::new(); - for channel_id in payload.delete_channels { - if payload - .channels - .iter() - .any(|channel| channel.id == channel_id) - { - channels_to_rehome.push(channel_id) - } else { - channels_to_delete.push(channel_id) - } - } - - self.channel_index.delete_channels(&channels_to_delete); - self.channel_index - .delete_paths_through_channels(&channels_to_rehome); + self.channel_index.delete_channels(&payload.delete_channels); self.channel_participants - .retain(|channel_id, _| !channels_to_delete.contains(channel_id)); + .retain(|channel_id, _| !&payload.delete_channels.contains(channel_id)); - for channel_id in &channels_to_delete { + for channel_id in &payload.delete_channels { let channel_id = *channel_id; if payload .channels diff --git a/crates/channel/src/channel_store/channel_index.rs b/crates/channel/src/channel_store/channel_index.rs index e5dc75c8b91c6e23f3eafd26840e7328f7fc2603..b221ce1b020372ef9f6beeed5d0055e8a968c6c8 100644 --- a/crates/channel/src/channel_store/channel_index.rs +++ b/crates/channel/src/channel_store/channel_index.rs @@ -24,14 +24,8 @@ impl ChannelIndex { /// Delete the given channels from this index. pub fn delete_channels(&mut self, channels: &[ChannelId]) { - dbg!("delete_channels", &channels); self.channels_by_id .retain(|channel_id, _| !channels.contains(channel_id)); - self.delete_paths_through_channels(channels) - } - - pub fn delete_paths_through_channels(&mut self, channels: &[ChannelId]) { - dbg!("rehome_channels", &channels); self.paths .retain(|path| !path.iter().any(|channel_id| channels.contains(channel_id))); } diff --git a/crates/collab/src/db.rs b/crates/collab/src/db.rs index 4f49b7ca39c284635f328d6d0dac74011995fe5e..936b6552003d12c999115b56270603e8c83be915 100644 --- a/crates/collab/src/db.rs +++ b/crates/collab/src/db.rs @@ -453,6 +453,19 @@ pub struct SetChannelVisibilityResult { pub participants_to_remove: HashSet, } +#[derive(Debug)] +pub struct MembershipUpdated { + pub channel_id: ChannelId, + pub new_channels: ChannelsForUser, + pub removed_channels: Vec, +} + +#[derive(Debug)] +pub enum SetMemberRoleResult { + InviteUpdated(Channel), + MembershipUpdated(MembershipUpdated), +} + #[derive(FromQueryResult, Debug, PartialEq, Eq, Hash)] pub struct Channel { pub id: ChannelId, diff --git a/crates/collab/src/db/queries/channels.rs b/crates/collab/src/db/queries/channels.rs index f7b7f6085fc8b9484b64219d63df0a7a02624eff..dbc701e4ae07e0ccb13b36772a2d26dde78e32e7 100644 --- a/crates/collab/src/db/queries/channels.rs +++ b/crates/collab/src/db/queries/channels.rs @@ -128,9 +128,9 @@ impl Database { user_id: UserId, connection: ConnectionId, environment: &str, - ) -> Result<(JoinRoom, Option)> { + ) -> Result<(JoinRoom, Option, ChannelRole)> { self.transaction(move |tx| async move { - let mut joined_channel_id = None; + let mut accept_invite_result = None; let channel = channel::Entity::find() .filter(channel::Column::Id.eq(channel_id)) @@ -147,9 +147,7 @@ impl Database { .await? { // note, this may be a parent channel - joined_channel_id = Some(invitation.channel_id); role = Some(invitation.role); - channel_member::Entity::update(channel_member::ActiveModel { accepted: ActiveValue::Set(true), ..invitation.into_active_model() @@ -157,6 +155,11 @@ impl Database { .exec(&*tx) .await?; + accept_invite_result = Some( + self.calculate_membership_updated(channel_id, user_id, &*tx) + .await?, + ); + debug_assert!( self.channel_role_for_user(channel_id, user_id, &*tx) .await? @@ -167,6 +170,7 @@ impl Database { if role.is_none() && channel.as_ref().map(|c| c.visibility) == Some(ChannelVisibility::Public) { + role = Some(ChannelRole::Guest); let channel_id_to_join = self .public_path_to_channel(channel_id, &*tx) .await? @@ -174,9 +178,6 @@ impl Database { .cloned() .unwrap_or(channel_id); - role = Some(ChannelRole::Guest); - joined_channel_id = Some(channel_id_to_join); - channel_member::Entity::insert(channel_member::ActiveModel { id: ActiveValue::NotSet, channel_id: ActiveValue::Set(channel_id_to_join), @@ -187,6 +188,11 @@ impl Database { .exec(&*tx) .await?; + accept_invite_result = Some( + self.calculate_membership_updated(channel_id, user_id, &*tx) + .await?, + ); + debug_assert!( self.channel_role_for_user(channel_id, user_id, &*tx) .await? @@ -205,7 +211,7 @@ impl Database { self.join_channel_room_internal(channel_id, room_id, user_id, connection, &*tx) .await - .map(|jr| (jr, joined_channel_id)) + .map(|jr| (jr, accept_invite_result, role.unwrap())) }) .await } @@ -345,7 +351,7 @@ impl Database { invitee_id: UserId, admin_id: UserId, role: ChannelRole, - ) -> Result<()> { + ) -> Result { self.transaction(move |tx| async move { self.check_user_is_channel_admin(channel_id, admin_id, &*tx) .await?; @@ -360,7 +366,17 @@ impl Database { .insert(&*tx) .await?; - Ok(()) + let channel = channel::Entity::find_by_id(channel_id) + .one(&*tx) + .await? + .unwrap(); + + Ok(Channel { + id: channel.id, + visibility: channel.visibility, + name: channel.name, + role, + }) }) .await } @@ -429,10 +445,10 @@ impl Database { channel_id: ChannelId, user_id: UserId, accept: bool, - ) -> Result<()> { + ) -> Result> { self.transaction(move |tx| async move { - let rows_affected = if accept { - channel_member::Entity::update_many() + if accept { + let rows_affected = channel_member::Entity::update_many() .set(channel_member::ActiveModel { accepted: ActiveValue::Set(accept), ..Default::default() @@ -445,33 +461,96 @@ impl Database { ) .exec(&*tx) .await? - .rows_affected - } else { - channel_member::ActiveModel { - channel_id: ActiveValue::Unchanged(channel_id), - user_id: ActiveValue::Unchanged(user_id), - ..Default::default() + .rows_affected; + + if rows_affected == 0 { + Err(anyhow!("no such invitation"))?; } - .delete(&*tx) - .await? - .rows_affected - }; + + return Ok(Some( + self.calculate_membership_updated(channel_id, user_id, &*tx) + .await?, + )); + } + + let rows_affected = channel_member::ActiveModel { + channel_id: ActiveValue::Unchanged(channel_id), + user_id: ActiveValue::Unchanged(user_id), + ..Default::default() + } + .delete(&*tx) + .await? + .rows_affected; if rows_affected == 0 { Err(anyhow!("no such invitation"))?; } - Ok(()) + Ok(None) }) .await } + async fn calculate_membership_updated( + &self, + channel_id: ChannelId, + user_id: UserId, + tx: &DatabaseTransaction, + ) -> Result { + let mut channel_to_refresh = channel_id; + let mut removed_channels: Vec = Vec::new(); + + // if the user was previously a guest of a parent public channel they may have seen this + // channel (or its descendants) in the tree already. + // Now they have new permissions, the graph of channels needs updating from that point. + if let Some(public_parent) = self.public_parent_channel_id(channel_id, &*tx).await? { + if self + .channel_role_for_user(public_parent, user_id, &*tx) + .await? + == Some(ChannelRole::Guest) + { + channel_to_refresh = public_parent; + } + } + + // remove all descendant channels from the user's tree + removed_channels.append( + &mut self + .get_channel_descendants(vec![channel_to_refresh], &*tx) + .await? + .into_iter() + .map(|edge| ChannelId::from_proto(edge.channel_id)) + .collect(), + ); + + let new_channels = self + .get_user_channels(user_id, Some(channel_to_refresh), &*tx) + .await?; + + // We only add the current channel to "moved" if the user has lost access, + // otherwise it would be made a root channel on the client. + if !new_channels + .channels + .channels + .iter() + .any(|c| c.id == channel_id) + { + removed_channels.push(channel_id); + } + + Ok(MembershipUpdated { + channel_id, + new_channels, + removed_channels, + }) + } + pub async fn remove_channel_member( &self, channel_id: ChannelId, member_id: UserId, admin_id: UserId, - ) -> Result<()> { + ) -> Result { self.transaction(|tx| async move { self.check_user_is_channel_admin(channel_id, admin_id, &*tx) .await?; @@ -489,7 +568,9 @@ impl Database { Err(anyhow!("no such member"))?; } - Ok(()) + Ok(self + .calculate_membership_updated(channel_id, member_id, &*tx) + .await?) }) .await } @@ -535,44 +616,7 @@ impl Database { self.transaction(|tx| async move { let tx = tx; - let channel_memberships = channel_member::Entity::find() - .filter( - channel_member::Column::UserId - .eq(user_id) - .and(channel_member::Column::Accepted.eq(true)), - ) - .all(&*tx) - .await?; - - self.get_user_channels(user_id, channel_memberships, &tx) - .await - }) - .await - } - - pub async fn get_channel_for_user( - &self, - channel_id: ChannelId, - user_id: UserId, - ) -> Result { - self.transaction(|tx| async move { - let tx = tx; - let role = self - .check_user_is_channel_participant(channel_id, user_id, &*tx) - .await?; - - self.get_user_channels( - user_id, - vec![channel_member::Model { - id: Default::default(), - channel_id, - user_id, - role, - accepted: true, - }], - &tx, - ) - .await + self.get_user_channels(user_id, None, &tx).await }) .await } @@ -580,19 +624,42 @@ impl Database { pub async fn get_user_channels( &self, user_id: UserId, - channel_memberships: Vec, + parent_channel_id: Option, tx: &DatabaseTransaction, ) -> Result { + // note: we could (maybe) win some efficiency here when parent_channel_id + // is set by getting just the role for that channel, then getting descendants + // with roles attached; but that's not as straightforward as it sounds + // because we need to calculate the path to the channel to make the query + // efficient, which currently requires an extra round trip to the database. + // Fix this later... + let channel_memberships = channel_member::Entity::find() + .filter( + channel_member::Column::UserId + .eq(user_id) + .and(channel_member::Column::Accepted.eq(true)), + ) + .all(&*tx) + .await?; + + dbg!((user_id, &channel_memberships)); + let mut edges = self .get_channel_descendants(channel_memberships.iter().map(|m| m.channel_id), &*tx) .await?; - let mut role_for_channel: HashMap = HashMap::default(); + dbg!((user_id, &edges)); + + let mut role_for_channel: HashMap = HashMap::default(); for membership in channel_memberships.iter() { - role_for_channel.insert(membership.channel_id, membership.role); + let included = + parent_channel_id.is_none() || membership.channel_id == parent_channel_id.unwrap(); + role_for_channel.insert(membership.channel_id, (membership.role, included)); } + dbg!((&role_for_channel, parent_channel_id)); + for ChannelEdge { parent_id, channel_id, @@ -601,14 +668,26 @@ impl Database { let parent_id = ChannelId::from_proto(*parent_id); let channel_id = ChannelId::from_proto(*channel_id); debug_assert!(role_for_channel.get(&parent_id).is_some()); - let parent_role = role_for_channel[&parent_id]; - if let Some(existing_role) = role_for_channel.get(&channel_id) { - if existing_role.should_override(parent_role) { - continue; - } + let (parent_role, parent_included) = role_for_channel[&parent_id]; + + if let Some((existing_role, included)) = role_for_channel.get(&channel_id) { + role_for_channel.insert( + channel_id, + (existing_role.max(parent_role), *included || parent_included), + ); + } else { + role_for_channel.insert( + channel_id, + ( + parent_role, + parent_included + || parent_channel_id.is_none() + || Some(channel_id) == parent_channel_id, + ), + ); } - role_for_channel.insert(channel_id, parent_role); } + dbg!((&role_for_channel, parent_channel_id)); let mut channels: Vec = Vec::new(); let mut channels_to_remove: HashSet = HashSet::default(); @@ -620,11 +699,13 @@ impl Database { while let Some(row) = rows.next().await { let channel = row?; - let role = role_for_channel[&channel.id]; + let (role, included) = role_for_channel[&channel.id]; - if role == ChannelRole::Banned + if !included + || role == ChannelRole::Banned || role == ChannelRole::Guest && channel.visibility != ChannelVisibility::Public { + dbg!("remove", channel.id); channels_to_remove.insert(channel.id.0 as u64); continue; } @@ -633,7 +714,7 @@ impl Database { id: channel.id, name: channel.name, visibility: channel.visibility, - role: role, + role, }); } drop(rows); @@ -740,18 +821,8 @@ impl Database { } results.push(( member.user_id, - self.get_user_channels( - member.user_id, - vec![channel_member::Model { - id: Default::default(), - channel_id: new_parent, - user_id: member.user_id, - role: member.role, - accepted: true, - }], - &*tx, - ) - .await?, + self.get_user_channels(member.user_id, Some(new_parent), &*tx) + .await?, )) } @@ -782,18 +853,8 @@ impl Database { }; results.push(( member.user_id, - self.get_user_channels( - member.user_id, - vec![channel_member::Model { - id: Default::default(), - channel_id: public_parent, - user_id: member.user_id, - role: member.role, - accepted: true, - }], - &*tx, - ) - .await?, + self.get_user_channels(member.user_id, Some(public_parent), &*tx) + .await?, )) } @@ -806,7 +867,7 @@ impl Database { admin_id: UserId, for_user: UserId, role: ChannelRole, - ) -> Result { + ) -> Result { self.transaction(|tx| async move { self.check_user_is_channel_admin(channel_id, admin_id, &*tx) .await?; @@ -828,7 +889,24 @@ impl Database { update.role = ActiveValue::Set(role); let updated = channel_member::Entity::update(update).exec(&*tx).await?; - Ok(updated) + if !updated.accepted { + let channel = channel::Entity::find_by_id(channel_id) + .one(&*tx) + .await? + .unwrap(); + + return Ok(SetMemberRoleResult::InviteUpdated(Channel { + id: channel.id, + visibility: channel.visibility, + name: channel.name, + role, + })); + } + + Ok(SetMemberRoleResult::MembershipUpdated( + self.calculate_membership_updated(channel_id, for_user, &*tx) + .await?, + )) }) .await } @@ -1396,16 +1474,7 @@ impl Database { .await?; } - let membership = channel_member::Entity::find() - .filter( - channel_member::Column::ChannelId - .eq(channel) - .and(channel_member::Column::UserId.eq(user)), - ) - .all(tx) - .await?; - - let mut channel_info = self.get_user_channels(user, membership, &*tx).await?; + let mut channel_info = self.get_user_channels(user, Some(channel), &*tx).await?; channel_info.channels.edges.push(ChannelEdge { channel_id: channel.to_proto(), @@ -1466,8 +1535,6 @@ impl Database { .await? == 0; - dbg!(is_stranded, &paths); - // Make sure that there is always at least one path to the channel if is_stranded { let root_paths: Vec<_> = paths @@ -1481,7 +1548,6 @@ impl Database { }) .collect(); - dbg!(is_stranded, &root_paths); channel_path::Entity::insert_many(root_paths) .exec(&*tx) .await?; @@ -1528,6 +1594,8 @@ impl Database { .into_iter() .collect(); + dbg!(&participants_to_update); + let mut moved_channels: HashSet = HashSet::default(); moved_channels.insert(channel_id); for edge in self.get_channel_descendants([channel_id], &*tx).await? { diff --git a/crates/collab/src/db/tests/channel_tests.rs b/crates/collab/src/db/tests/channel_tests.rs index 1767a773ff5484bfabb5b870c78c8caf418a1bc7..405839ba187528172dabc4433c54bdb7c86701db 100644 --- a/crates/collab/src/db/tests/channel_tests.rs +++ b/crates/collab/src/db/tests/channel_tests.rs @@ -160,7 +160,7 @@ async fn test_joining_channels(db: &Arc) { let channel_1 = db.create_root_channel("channel_1", user_1).await.unwrap(); // can join a room with membership to its channel - let (joined_room, _) = db + let (joined_room, _, _) = db .join_channel( channel_1, user_1, diff --git a/crates/collab/src/rpc.rs b/crates/collab/src/rpc.rs index f8648a2b148a32398bf52da1d07d6563032cb2a2..3372fe1e35f75b2ae4ba408d5a489af84f482fa3 100644 --- a/crates/collab/src/rpc.rs +++ b/crates/collab/src/rpc.rs @@ -3,9 +3,9 @@ mod connection_pool; use crate::{ auth, db::{ - self, BufferId, ChannelId, ChannelsForUser, CreateChannelResult, Database, MessageId, - MoveChannelResult, ProjectId, RenameChannelResult, RoomId, ServerId, - SetChannelVisibilityResult, User, UserId, + self, BufferId, ChannelId, ChannelRole, ChannelsForUser, CreateChannelResult, Database, + MembershipUpdated, MessageId, MoveChannelResult, ProjectId, RenameChannelResult, RoomId, + ServerId, SetChannelVisibilityResult, User, UserId, }, executor::Executor, AppState, Result, @@ -2266,23 +2266,20 @@ async fn invite_channel_member( let db = session.db().await; let channel_id = ChannelId::from_proto(request.channel_id); let invitee_id = UserId::from_proto(request.user_id); - db.invite_channel_member( - channel_id, - invitee_id, - session.user_id, - request.role().into(), - ) - .await?; + let channel = db + .invite_channel_member( + channel_id, + invitee_id, + session.user_id, + request.role().into(), + ) + .await?; - let channel = db.get_channel(channel_id, session.user_id).await?; + let update = proto::UpdateChannels { + channel_invitations: vec![channel.to_proto()], + ..Default::default() + }; - let mut update = proto::UpdateChannels::default(); - update.channel_invitations.push(proto::Channel { - id: channel.id.to_proto(), - visibility: channel.visibility.into(), - name: channel.name, - role: request.role().into(), - }); for connection_id in session .connection_pool() .await @@ -2304,19 +2301,13 @@ async fn remove_channel_member( let channel_id = ChannelId::from_proto(request.channel_id); let member_id = UserId::from_proto(request.user_id); - db.remove_channel_member(channel_id, member_id, session.user_id) + let membership_updated = db + .remove_channel_member(channel_id, member_id, session.user_id) .await?; - let mut update = proto::UpdateChannels::default(); - update.delete_channels.push(channel_id.to_proto()); + dbg!(&membership_updated); - for connection_id in session - .connection_pool() - .await - .user_connection_ids(member_id) - { - session.peer.send(connection_id, update.clone())?; - } + notify_membership_updated(membership_updated, member_id, &session).await?; response.send(proto::Ack {})?; Ok(()) @@ -2347,6 +2338,9 @@ async fn set_channel_visibility( } for user_id in participants_to_remove { let update = proto::UpdateChannels { + // for public participants we only need to remove the current channel + // (not descendants) + // because they can still see any public descendants delete_channels: vec![channel_id.to_proto()], ..Default::default() }; @@ -2367,7 +2361,7 @@ async fn set_channel_member_role( let db = session.db().await; let channel_id = ChannelId::from_proto(request.channel_id); let member_id = UserId::from_proto(request.user_id); - let channel_member = db + let result = db .set_channel_member_role( channel_id, session.user_id, @@ -2376,26 +2370,24 @@ async fn set_channel_member_role( ) .await?; - let mut update = proto::UpdateChannels::default(); - if channel_member.accepted { - let channels = db.get_channel_for_user(channel_id, member_id).await?; - update = build_channels_update(channels, vec![]); - } else { - let channel = db.get_channel(channel_id, session.user_id).await?; - update.channel_invitations.push(proto::Channel { - id: channel_id.to_proto(), - visibility: channel.visibility.into(), - name: channel.name, - role: request.role().into(), - }); - } + match result { + db::SetMemberRoleResult::MembershipUpdated(membership_update) => { + notify_membership_updated(membership_update, member_id, &session).await?; + } + db::SetMemberRoleResult::InviteUpdated(channel) => { + let update = proto::UpdateChannels { + channel_invitations: vec![channel.to_proto()], + ..Default::default() + }; - for connection_id in session - .connection_pool() - .await - .user_connection_ids(member_id) - { - session.peer.send(connection_id, update.clone())?; + for connection_id in session + .connection_pool() + .await + .user_connection_ids(member_id) + { + session.peer.send(connection_id, update.clone())?; + } + } } response.send(proto::Ack {})?; @@ -2541,35 +2533,26 @@ async fn respond_to_channel_invite( ) -> Result<()> { let db = session.db().await; let channel_id = ChannelId::from_proto(request.channel_id); - db.respond_to_channel_invite(channel_id, session.user_id, request.accept) + let result = db + .respond_to_channel_invite(channel_id, session.user_id, request.accept) .await?; - if request.accept { - channel_membership_updated(db, channel_id, &session).await?; + if let Some(accept_invite_result) = result { + notify_membership_updated(accept_invite_result, session.user_id, &session).await?; } else { - let mut update = proto::UpdateChannels::default(); - update - .remove_channel_invitations - .push(channel_id.to_proto()); - session.peer.send(session.connection_id, update)?; - } - response.send(proto::Ack {})?; + let update = proto::UpdateChannels { + remove_channel_invitations: vec![channel_id.to_proto()], + ..Default::default() + }; - Ok(()) -} + let connection_pool = session.connection_pool().await; + for connection_id in connection_pool.user_connection_ids(session.user_id) { + session.peer.send(connection_id, update.clone())?; + } + }; -async fn channel_membership_updated( - db: tokio::sync::MutexGuard<'_, DbHandle>, - channel_id: ChannelId, - session: &Session, -) -> Result<(), crate::Error> { - let result = db.get_channel_for_user(channel_id, session.user_id).await?; - let mut update = build_channels_update(result, vec![]); - update - .remove_channel_invitations - .push(channel_id.to_proto()); + response.send(proto::Ack {})?; - session.peer.send(session.connection_id, update)?; Ok(()) } @@ -2605,7 +2588,7 @@ async fn join_channel_internal( leave_room_for_session(&session).await?; let db = session.db().await; - let (joined_room, joined_channel) = db + let (joined_room, accept_invite_result, role) = db .join_channel( channel_id, session.user_id, @@ -2615,12 +2598,21 @@ async fn join_channel_internal( .await?; let live_kit_connection_info = session.live_kit_client.as_ref().and_then(|live_kit| { - let token = live_kit - .room_token( - &joined_room.room.live_kit_room, - &session.user_id.to_string(), - ) - .trace_err()?; + let token = if role == ChannelRole::Guest { + live_kit + .guest_token( + &joined_room.room.live_kit_room, + &session.user_id.to_string(), + ) + .trace_err()? + } else { + live_kit + .room_token( + &joined_room.room.live_kit_room, + &session.user_id.to_string(), + ) + .trace_err()? + }; Some(LiveKitConnectionInfo { server_url: live_kit.url().into(), @@ -2634,8 +2626,8 @@ async fn join_channel_internal( live_kit_connection_info, })?; - if let Some(joined_channel) = joined_channel { - channel_membership_updated(db, joined_channel, &session).await? + if let Some(accept_invite_result) = accept_invite_result { + notify_membership_updated(accept_invite_result, session.user_id, &session).await?; } room_updated(&joined_room.room, &session.peer); @@ -3051,6 +3043,26 @@ fn to_tungstenite_message(message: AxumMessage) -> TungsteniteMessage { } } +async fn notify_membership_updated( + result: MembershipUpdated, + user_id: UserId, + session: &Session, +) -> Result<()> { + let mut update = build_channels_update(result.new_channels, vec![]); + update.delete_channels = result + .removed_channels + .into_iter() + .map(|id| id.to_proto()) + .collect(); + update.remove_channel_invitations = vec![result.channel_id.to_proto()]; + + let connection_pool = session.connection_pool().await; + for connection_id in connection_pool.user_connection_ids(user_id) { + session.peer.send(connection_id, update.clone())?; + } + Ok(()) +} + fn build_channels_update( channels: ChannelsForUser, channel_invites: Vec, diff --git a/crates/collab/src/tests/channel_buffer_tests.rs b/crates/collab/src/tests/channel_buffer_tests.rs index 01174fe3beceebada80d7f09710903d012a2b443..cb51c7c5f36f27875be38160bb3c091929fdce44 100644 --- a/crates/collab/src/tests/channel_buffer_tests.rs +++ b/crates/collab/src/tests/channel_buffer_tests.rs @@ -410,7 +410,7 @@ async fn test_channel_buffer_disconnect( server.disconnect_client(client_a.peer_id().unwrap()); deterministic.advance_clock(RECEIVE_TIMEOUT + RECONNECT_TIMEOUT); - channel_buffer_a.update(cx_a, |buffer, _| { + channel_buffer_a.update(cx_a, |buffer, cx| { assert_eq!( buffer.channel(cx).unwrap().as_ref(), &channel(channel_id, "the-channel", proto::ChannelRole::Admin) @@ -435,7 +435,7 @@ async fn test_channel_buffer_disconnect( deterministic.run_until_parked(); // Channel buffer observed the deletion - channel_buffer_b.update(cx_b, |buffer, _| { + channel_buffer_b.update(cx_b, |buffer, cx| { assert_eq!( buffer.channel(cx).unwrap().as_ref(), &channel(channel_id, "the-channel", proto::ChannelRole::Member) @@ -699,7 +699,7 @@ async fn test_following_to_channel_notes_without_a_shared_project( .await .unwrap(); channel_view_1_a.update(cx_a, |notes, cx| { - assert_eq!(notes.channel(cx).name, "channel-1"); + assert_eq!(notes.channel(cx).unwrap().name, "channel-1"); notes.editor.update(cx, |editor, cx| { editor.insert("Hello from A.", cx); editor.change_selections(None, cx, |selections| { @@ -731,7 +731,7 @@ async fn test_following_to_channel_notes_without_a_shared_project( .expect("active item is not a channel view") }); channel_view_1_b.read_with(cx_b, |notes, cx| { - assert_eq!(notes.channel(cx).name, "channel-1"); + assert_eq!(notes.channel(cx).unwrap().name, "channel-1"); let editor = notes.editor.read(cx); assert_eq!(editor.text(cx), "Hello from A."); assert_eq!(editor.selections.ranges::(cx), &[3..4]); @@ -743,7 +743,7 @@ async fn test_following_to_channel_notes_without_a_shared_project( .await .unwrap(); channel_view_2_a.read_with(cx_a, |notes, cx| { - assert_eq!(notes.channel(cx).name, "channel-2"); + assert_eq!(notes.channel(cx).unwrap().name, "channel-2"); }); // Client B is taken to the notes for channel 2. @@ -760,7 +760,7 @@ async fn test_following_to_channel_notes_without_a_shared_project( .expect("active item is not a channel view") }); channel_view_2_b.read_with(cx_b, |notes, cx| { - assert_eq!(notes.channel(cx).name, "channel-2"); + assert_eq!(notes.channel(cx).unwrap().name, "channel-2"); }); } diff --git a/crates/collab/src/tests/channel_tests.rs b/crates/collab/src/tests/channel_tests.rs index b9425cc62999ae845378d6c74eabdc1bb44ff708..3df9597777dca715ea64fc7de2be472a6d6a8fc0 100644 --- a/crates/collab/src/tests/channel_tests.rs +++ b/crates/collab/src/tests/channel_tests.rs @@ -48,13 +48,13 @@ async fn test_core_channels( id: channel_a_id, name: "channel-a".to_string(), depth: 0, - user_is_admin: true, + role: ChannelRole::Admin, }, ExpectedChannel { id: channel_b_id, name: "channel-b".to_string(), depth: 1, - user_is_admin: true, + role: ChannelRole::Admin, }, ], ); @@ -95,7 +95,7 @@ async fn test_core_channels( id: channel_a_id, name: "channel-a".to_string(), depth: 0, - user_is_admin: false, + role: ChannelRole::Member, }], ); @@ -142,13 +142,13 @@ async fn test_core_channels( ExpectedChannel { id: channel_a_id, name: "channel-a".to_string(), - user_is_admin: false, + role: ChannelRole::Member, depth: 0, }, ExpectedChannel { id: channel_b_id, name: "channel-b".to_string(), - user_is_admin: false, + role: ChannelRole::Member, depth: 1, }, ], @@ -171,19 +171,19 @@ async fn test_core_channels( ExpectedChannel { id: channel_a_id, name: "channel-a".to_string(), - user_is_admin: false, + role: ChannelRole::Member, depth: 0, }, ExpectedChannel { id: channel_b_id, name: "channel-b".to_string(), - user_is_admin: false, + role: ChannelRole::Member, depth: 1, }, ExpectedChannel { id: channel_c_id, name: "channel-c".to_string(), - user_is_admin: false, + role: ChannelRole::Member, depth: 2, }, ], @@ -215,19 +215,19 @@ async fn test_core_channels( id: channel_a_id, name: "channel-a".to_string(), depth: 0, - user_is_admin: true, + role: ChannelRole::Admin, }, ExpectedChannel { id: channel_b_id, name: "channel-b".to_string(), depth: 1, - user_is_admin: true, + role: ChannelRole::Admin, }, ExpectedChannel { id: channel_c_id, name: "channel-c".to_string(), depth: 2, - user_is_admin: true, + role: ChannelRole::Admin, }, ], ); @@ -249,7 +249,7 @@ async fn test_core_channels( id: channel_a_id, name: "channel-a".to_string(), depth: 0, - user_is_admin: true, + role: ChannelRole::Admin, }], ); assert_channels( @@ -259,7 +259,7 @@ async fn test_core_channels( id: channel_a_id, name: "channel-a".to_string(), depth: 0, - user_is_admin: true, + role: ChannelRole::Admin, }], ); @@ -282,7 +282,7 @@ async fn test_core_channels( id: channel_a_id, name: "channel-a".to_string(), depth: 0, - user_is_admin: true, + role: ChannelRole::Admin, }], ); @@ -304,7 +304,7 @@ async fn test_core_channels( id: channel_a_id, name: "channel-a".to_string(), depth: 0, - user_is_admin: true, + role: ChannelRole::Admin, }], ); } @@ -412,7 +412,7 @@ async fn test_channel_room( id: zed_id, name: "zed".to_string(), depth: 0, - user_is_admin: false, + role: ChannelRole::Member, }], ); client_b.channel_store().read_with(cx_b, |channels, _| { @@ -645,7 +645,7 @@ async fn test_permissions_update_while_invited( depth: 0, id: rust_id, name: "rust".to_string(), - user_is_admin: false, + role: ChannelRole::Member, }], ); assert_channels(client_b.channel_store(), cx_b, &[]); @@ -673,7 +673,7 @@ async fn test_permissions_update_while_invited( depth: 0, id: rust_id, name: "rust".to_string(), - user_is_admin: false, + role: ChannelRole::Member, }], ); assert_channels(client_b.channel_store(), cx_b, &[]); @@ -713,7 +713,7 @@ async fn test_channel_rename( depth: 0, id: rust_id, name: "rust-archive".to_string(), - user_is_admin: true, + role: ChannelRole::Admin, }], ); @@ -725,7 +725,7 @@ async fn test_channel_rename( depth: 0, id: rust_id, name: "rust-archive".to_string(), - user_is_admin: false, + role: ChannelRole::Member, }], ); } @@ -848,7 +848,7 @@ async fn test_lost_channel_creation( depth: 0, id: channel_id, name: "x".to_string(), - user_is_admin: false, + role: ChannelRole::Member, }], ); @@ -872,13 +872,13 @@ async fn test_lost_channel_creation( depth: 0, id: channel_id, name: "x".to_string(), - user_is_admin: true, + role: ChannelRole::Admin, }, ExpectedChannel { depth: 1, id: subchannel_id, name: "subchannel".to_string(), - user_is_admin: true, + role: ChannelRole::Admin, }, ], ); @@ -903,13 +903,13 @@ async fn test_lost_channel_creation( depth: 0, id: channel_id, name: "x".to_string(), - user_is_admin: false, + role: ChannelRole::Member, }, ExpectedChannel { depth: 1, id: subchannel_id, name: "subchannel".to_string(), - user_is_admin: false, + role: ChannelRole::Member, }, ], ); @@ -969,8 +969,7 @@ async fn test_channel_link_notifications( // we have an admin (a), member (b) and guest (c) all part of the zed channel. - // create a new private sub-channel - // create a new priate channel, make it public, and move it under the previous one, and verify it shows for b and c + // create a new private channel, make it public, and move it under the previous one, and verify it shows for b and not c let active_channel = client_a .channel_store() .update(cx_a, |channel_store, cx| { @@ -1118,6 +1117,117 @@ async fn test_channel_link_notifications( ); } +#[gpui::test] +async fn test_channel_membership_notifications( + deterministic: Arc, + cx_a: &mut TestAppContext, + cx_b: &mut TestAppContext, +) { + deterministic.forbid_parking(); + + deterministic.forbid_parking(); + + let mut server = TestServer::start(&deterministic).await; + let client_a = server.create_client(cx_a, "user_a").await; + let client_b = server.create_client(cx_b, "user_c").await; + + 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), + ) + .await; + let zed_channel = channels[0]; + let _active_channel = channels[1]; + let vim_channel = channels[2]; + + 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), + ] + })) + .await + .unwrap(); + + deterministic.run_until_parked(); + + client_b + .channel_store() + .update(cx_b, |channel_store, _| { + channel_store.respond_to_channel_invite(zed_channel, true) + }) + .await + .unwrap(); + + client_b + .channel_store() + .update(cx_b, |channel_store, _| { + channel_store.respond_to_channel_invite(vim_channel, true) + }) + .await + .unwrap(); + + deterministic.run_until_parked(); + + // we have an admin (a), and a guest (b) with access to all of zed, and membership in vim. + assert_channels( + client_b.channel_store(), + cx_b, + &[ + ExpectedChannel { + depth: 0, + id: zed_channel, + name: "zed".to_string(), + role: ChannelRole::Guest, + }, + ExpectedChannel { + depth: 1, + id: vim_channel, + name: "vim".to_string(), + role: ChannelRole::Member, + }, + ], + ); + + client_a + .channel_store() + .update(cx_a, |channel_store, cx| { + channel_store.remove_member(vim_channel, user_b, cx) + }) + .await + .unwrap(); + + deterministic.run_until_parked(); + + assert_channels( + client_b.channel_store(), + cx_b, + &[ + ExpectedChannel { + depth: 0, + id: zed_channel, + name: "zed".to_string(), + role: ChannelRole::Guest, + }, + ExpectedChannel { + depth: 1, + id: vim_channel, + name: "vim".to_string(), + role: ChannelRole::Guest, + }, + ], + ) +} + #[gpui::test] async fn test_guest_access( deterministic: Arc, @@ -1485,7 +1595,7 @@ struct ExpectedChannel { depth: usize, id: ChannelId, name: String, - user_is_admin: bool, + role: ChannelRole, } #[track_caller] @@ -1502,7 +1612,7 @@ fn assert_channel_invitations( depth: 0, name: channel.name.clone(), id: channel.id, - user_is_admin: store.is_channel_admin(channel.id), + role: channel.role, }) .collect::>() }); @@ -1522,7 +1632,7 @@ fn assert_channels( depth, name: channel.name.clone(), id: channel.id, - user_is_admin: store.is_channel_admin(channel.id), + role: channel.role, }) .collect::>() }); diff --git a/crates/collab/src/tests/random_channel_buffer_tests.rs b/crates/collab/src/tests/random_channel_buffer_tests.rs index 9d05c3017f7c0e9fc1688b6fba0ce773fe3f7155..64fecd5e62cd3727339406d9ae9f83462c0b2353 100644 --- a/crates/collab/src/tests/random_channel_buffer_tests.rs +++ b/crates/collab/src/tests/random_channel_buffer_tests.rs @@ -99,14 +99,14 @@ impl RandomizedTest for RandomChannelBufferTest { 30..=40 => { if let Some(buffer) = channel_buffers.iter().choose(rng) { let channel_name = - buffer.read_with(cx, |b, _| b.channel(cx).unwrap().name.clone()); + buffer.read_with(cx, |b, cx| b.channel(cx).unwrap().name.clone()); break ChannelBufferOperation::LeaveChannelNotes { channel_name }; } } _ => { if let Some(buffer) = channel_buffers.iter().choose(rng) { - break buffer.read_with(cx, |b, _| { + break buffer.read_with(cx, |b, cx| { let channel_name = b.channel(cx).unwrap().name.clone(); let edits = b .buffer() diff --git a/crates/live_kit_client/src/test.rs b/crates/live_kit_client/src/test.rs index 8df8ab4abb6142fea292aec7f3377e86e93cb388..b39ad333d2c2d14cbae35a69ac6072d343b31bbd 100644 --- a/crates/live_kit_client/src/test.rs +++ b/crates/live_kit_client/src/test.rs @@ -306,6 +306,16 @@ impl live_kit_server::api::Client for TestApiClient { token::VideoGrant::to_join(room), ) } + + fn guest_token(&self, room: &str, identity: &str) -> Result { + let server = TestServer::get(&self.url)?; + token::create( + &server.api_key, + &server.secret_key, + Some(identity), + token::VideoGrant::for_guest(room), + ) + } } pub type Sid = String; diff --git a/crates/live_kit_server/src/api.rs b/crates/live_kit_server/src/api.rs index 417a17bdc9859111d57806200ca35595866c79ef..2c1e174fb41551d22b498b75d9ce36011ca5a5a9 100644 --- a/crates/live_kit_server/src/api.rs +++ b/crates/live_kit_server/src/api.rs @@ -12,6 +12,7 @@ pub trait Client: Send + Sync { async fn delete_room(&self, name: String) -> Result<()>; async fn remove_participant(&self, room: String, identity: String) -> Result<()>; fn room_token(&self, room: &str, identity: &str) -> Result; + fn guest_token(&self, room: &str, identity: &str) -> Result; } #[derive(Clone)] @@ -138,4 +139,13 @@ impl Client for LiveKitClient { token::VideoGrant::to_join(room), ) } + + fn guest_token(&self, room: &str, identity: &str) -> Result { + token::create( + &self.key, + &self.secret, + Some(identity), + token::VideoGrant::for_guest(room), + ) + } } diff --git a/crates/live_kit_server/src/token.rs b/crates/live_kit_server/src/token.rs index 072a8be0c9c6fc93f4b320a63650cc3a3f148c9c..b98f5892aea9ead8472b614da8491bdf7f2a38b4 100644 --- a/crates/live_kit_server/src/token.rs +++ b/crates/live_kit_server/src/token.rs @@ -57,6 +57,15 @@ impl<'a> VideoGrant<'a> { ..Default::default() } } + + pub fn for_guest(room: &'a str) -> Self { + Self { + room: Some(Cow::Borrowed(room)), + room_join: Some(true), + can_subscribe: Some(true), + ..Default::default() + } + } } pub fn create( From e03e5364d22d41cf356d633835e2983ebb349700 Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Thu, 19 Oct 2023 23:23:33 -0600 Subject: [PATCH 09/12] Wire through LiveKit permissions --- crates/call/src/room.rs | 26 ++++++++++----- crates/collab/src/rpc.rs | 35 ++++++++++++-------- crates/collab_ui/src/collab_titlebar_item.rs | 15 ++++++--- crates/rpc/proto/zed.proto | 1 + 4 files changed, 51 insertions(+), 26 deletions(-) diff --git a/crates/call/src/room.rs b/crates/call/src/room.rs index 86045e981a73aad6ce6ce7741ff9d3b246405265..594d1b7892a72723c652382e1979a8334703d9c9 100644 --- a/crates/call/src/room.rs +++ b/crates/call/src/room.rs @@ -122,6 +122,10 @@ impl Room { } } + pub fn can_publish(&self) -> bool { + self.live_kit.as_ref().is_some_and(|room| room.can_publish) + } + fn new( id: u64, channel_id: Option, @@ -181,20 +185,23 @@ impl Room { }); let connect = room.connect(&connection_info.server_url, &connection_info.token); - cx.spawn(|this, mut cx| async move { - connect.await?; + if connection_info.can_publish { + cx.spawn(|this, mut cx| async move { + connect.await?; - if !cx.read(Self::mute_on_join) { - this.update(&mut cx, |this, cx| this.share_microphone(cx)) - .await?; - } + if !cx.read(Self::mute_on_join) { + this.update(&mut cx, |this, cx| this.share_microphone(cx)) + .await?; + } - anyhow::Ok(()) - }) - .detach_and_log_err(cx); + anyhow::Ok(()) + }) + .detach_and_log_err(cx); + } Some(LiveKitRoom { room, + can_publish: connection_info.can_publish, screen_track: LocalTrack::None, microphone_track: LocalTrack::None, next_publish_id: 0, @@ -1498,6 +1505,7 @@ struct LiveKitRoom { deafened: bool, speaking: bool, next_publish_id: usize, + can_publish: bool, _maintain_room: Task<()>, _maintain_tracks: [Task<()>; 2], } diff --git a/crates/collab/src/rpc.rs b/crates/collab/src/rpc.rs index 3372fe1e35f75b2ae4ba408d5a489af84f482fa3..9974ded821da3fb4e22bf3121f8bf91bf438e630 100644 --- a/crates/collab/src/rpc.rs +++ b/crates/collab/src/rpc.rs @@ -948,6 +948,7 @@ async fn create_room( Some(proto::LiveKitConnectionInfo { server_url: live_kit.url().into(), token, + can_publish: true, }) }) } @@ -1028,6 +1029,7 @@ async fn join_room( Some(proto::LiveKitConnectionInfo { server_url: live_kit.url().into(), token, + can_publish: true, }) } else { None @@ -2598,25 +2600,32 @@ async fn join_channel_internal( .await?; let live_kit_connection_info = session.live_kit_client.as_ref().and_then(|live_kit| { - let token = if role == ChannelRole::Guest { - live_kit - .guest_token( - &joined_room.room.live_kit_room, - &session.user_id.to_string(), - ) - .trace_err()? + let (can_publish, token) = if role == ChannelRole::Guest { + ( + false, + live_kit + .guest_token( + &joined_room.room.live_kit_room, + &session.user_id.to_string(), + ) + .trace_err()?, + ) } else { - live_kit - .room_token( - &joined_room.room.live_kit_room, - &session.user_id.to_string(), - ) - .trace_err()? + ( + true, + live_kit + .room_token( + &joined_room.room.live_kit_room, + &session.user_id.to_string(), + ) + .trace_err()?, + ) }; Some(LiveKitConnectionInfo { server_url: live_kit.url().into(), token, + can_publish, }) }); diff --git a/crates/collab_ui/src/collab_titlebar_item.rs b/crates/collab_ui/src/collab_titlebar_item.rs index 211ee863e89f6ce7bfe3aa44826c0a8f827a6f85..1adc03d5ea353578ddd0055c058fa3a6d67cf9de 100644 --- a/crates/collab_ui/src/collab_titlebar_item.rs +++ b/crates/collab_ui/src/collab_titlebar_item.rs @@ -88,8 +88,10 @@ impl View for CollabTitlebarItem { .zip(peer_id) .zip(ActiveCall::global(cx).read(cx).room().cloned()) { - right_container - .add_children(self.render_in_call_share_unshare_button(&workspace, &theme, cx)); + if room.read(cx).can_publish() { + right_container + .add_children(self.render_in_call_share_unshare_button(&workspace, &theme, cx)); + } right_container.add_child(self.render_leave_call(&theme, cx)); let muted = room.read(cx).is_muted(cx); let speaking = room.read(cx).is_speaking(); @@ -97,9 +99,14 @@ impl View for CollabTitlebarItem { self.render_current_user(&workspace, &theme, &user, peer_id, muted, speaking, cx), ); left_container.add_children(self.render_collaborators(&workspace, &theme, &room, cx)); - right_container.add_child(self.render_toggle_mute(&theme, &room, cx)); + if room.read(cx).can_publish() { + right_container.add_child(self.render_toggle_mute(&theme, &room, cx)); + } right_container.add_child(self.render_toggle_deafen(&theme, &room, cx)); - right_container.add_child(self.render_toggle_screen_sharing_button(&theme, &room, cx)); + if room.read(cx).can_publish() { + right_container + .add_child(self.render_toggle_screen_sharing_button(&theme, &room, cx)); + } } let status = workspace.read(cx).client().status(); diff --git a/crates/rpc/proto/zed.proto b/crates/rpc/proto/zed.proto index 20a47954a451a9ce94cc560a2e69c9a6b9bc6caa..5d0068b43eb0e6f6e9f3be5ae34ad6c78072a3cb 100644 --- a/crates/rpc/proto/zed.proto +++ b/crates/rpc/proto/zed.proto @@ -335,6 +335,7 @@ message RoomUpdated { message LiveKitConnectionInfo { string server_url = 1; string token = 2; + bool can_publish = 3; } message ShareProject { From e6087e0ed9ecb1fc5ae898e6e30c8fe312fa2742 Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Tue, 24 Oct 2023 09:46:46 +0200 Subject: [PATCH 10/12] Fix tests --- crates/collab/src/tests/channel_buffer_tests.rs | 5 +---- crates/collab/src/tests/channel_tests.rs | 14 +++++++++----- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/crates/collab/src/tests/channel_buffer_tests.rs b/crates/collab/src/tests/channel_buffer_tests.rs index cb51c7c5f36f27875be38160bb3c091929fdce44..931610f5ffc8fe56010b4cda3614fcff6fa992bc 100644 --- a/crates/collab/src/tests/channel_buffer_tests.rs +++ b/crates/collab/src/tests/channel_buffer_tests.rs @@ -436,10 +436,7 @@ async fn test_channel_buffer_disconnect( // Channel buffer observed the deletion channel_buffer_b.update(cx_b, |buffer, cx| { - assert_eq!( - buffer.channel(cx).unwrap().as_ref(), - &channel(channel_id, "the-channel", proto::ChannelRole::Member) - ); + assert!(buffer.channel(cx).is_none()); assert!(!buffer.is_connected()); }); } diff --git a/crates/collab/src/tests/channel_tests.rs b/crates/collab/src/tests/channel_tests.rs index 63cc78b65180c84384e229f2aa2bb6bb1659b4ca..85503f8ea352f49076419ebf97e9873c0deba9b5 100644 --- a/crates/collab/src/tests/channel_tests.rs +++ b/crates/collab/src/tests/channel_tests.rs @@ -1,4 +1,5 @@ use crate::{ + db::{self, UserId}, rpc::RECONNECT_TIMEOUT, tests::{room_participants, RoomParticipants, TestServer}, }; @@ -292,11 +293,14 @@ async fn test_core_channels( server.disconnect_client(client_a.peer_id().unwrap()); deterministic.advance_clock(RECEIVE_TIMEOUT + RECONNECT_TIMEOUT); - client_b - .channel_store() - .update(cx_b, |channel_store, cx| { - channel_store.rename(channel_a_id, "channel-a-renamed", cx) - }) + server + .app_state + .db + .rename_channel( + db::ChannelId::from_proto(channel_a_id), + UserId::from_proto(client_a.id()), + "channel-a-renamed", + ) .await .unwrap(); From aa6990bb6b966c3424090b20fd4afa0ca87611db Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Tue, 24 Oct 2023 10:41:21 +0200 Subject: [PATCH 11/12] Fix set_channel_visibility for public channels --- crates/collab/src/db.rs | 1 + crates/collab/src/db/queries/channels.rs | 36 ++++++++----- crates/collab/src/rpc.rs | 12 ++--- crates/collab/src/tests/channel_tests.rs | 65 ++++++++++++++++++------ 4 files changed, 78 insertions(+), 36 deletions(-) diff --git a/crates/collab/src/db.rs b/crates/collab/src/db.rs index 8c6bca211370f2ffab62d11f71f6d9ceb77582c1..f4e2602cc6061a76b7806d12afb5eb6fa2e5eb11 100644 --- a/crates/collab/src/db.rs +++ b/crates/collab/src/db.rs @@ -458,6 +458,7 @@ pub struct CreateChannelResult { pub struct SetChannelVisibilityResult { pub participants_to_update: HashMap, pub participants_to_remove: HashSet, + pub channels_to_remove: Vec, } #[derive(Debug)] diff --git a/crates/collab/src/db/queries/channels.rs b/crates/collab/src/db/queries/channels.rs index 0abd36049dc750c2202f358d45e1b5d03a547830..9e7b1cabf5623f53051e1c6f482712496262687e 100644 --- a/crates/collab/src/db/queries/channels.rs +++ b/crates/collab/src/db/queries/channels.rs @@ -244,9 +244,30 @@ impl Database { .into_iter() .collect(); + 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(vec![channel_id], &*tx) + .await? + .into_iter() + .map(|edge| ChannelId::from_proto(edge.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); @@ -271,6 +292,7 @@ impl Database { Ok(SetChannelVisibilityResult { participants_to_update, participants_to_remove, + channels_to_remove, }) }) .await @@ -694,14 +716,10 @@ impl Database { .all(&*tx) .await?; - dbg!((user_id, &channel_memberships)); - let mut edges = self .get_channel_descendants(channel_memberships.iter().map(|m| m.channel_id), &*tx) .await?; - dbg!((user_id, &edges)); - let mut role_for_channel: HashMap = HashMap::default(); for membership in channel_memberships.iter() { @@ -710,8 +728,6 @@ impl Database { role_for_channel.insert(membership.channel_id, (membership.role, included)); } - dbg!((&role_for_channel, parent_channel_id)); - for ChannelEdge { parent_id, channel_id, @@ -739,7 +755,6 @@ impl Database { ); } } - dbg!((&role_for_channel, parent_channel_id)); let mut channels: Vec = Vec::new(); let mut channels_to_remove: HashSet = HashSet::default(); @@ -757,7 +772,6 @@ impl Database { || role == ChannelRole::Banned || role == ChannelRole::Guest && channel.visibility != ChannelVisibility::Public { - dbg!("remove", channel.id); channels_to_remove.insert(channel.id.0 as u64); continue; } @@ -865,8 +879,6 @@ impl Database { .get_channel_participant_details_internal(new_parent, &*tx) .await?; - dbg!(&members); - for member in members.iter() { if !member.role.can_see_all_descendants() { continue; @@ -897,8 +909,6 @@ impl Database { .await? }; - dbg!(&public_members); - for member in public_members { if !member.role.can_only_see_public_descendants() { continue; @@ -1666,8 +1676,6 @@ impl Database { .into_iter() .collect(); - dbg!(&participants_to_update); - let mut moved_channels: HashSet = HashSet::default(); moved_channels.insert(channel_id); for edge in self.get_channel_descendants([channel_id], &*tx).await? { diff --git a/crates/collab/src/rpc.rs b/crates/collab/src/rpc.rs index 2d89ec47a2ecda0b090f401c72346db435a57ecc..dda638e107a069e089124ece025d627604253906 100644 --- a/crates/collab/src/rpc.rs +++ b/crates/collab/src/rpc.rs @@ -2366,6 +2366,7 @@ async fn set_channel_visibility( let SetChannelVisibilityResult { participants_to_update, participants_to_remove, + channels_to_remove, } = db .set_channel_visibility(channel_id, visibility, session.user_id) .await?; @@ -2379,10 +2380,7 @@ async fn set_channel_visibility( } for user_id in participants_to_remove { let update = proto::UpdateChannels { - // for public participants we only need to remove the current channel - // (not descendants) - // because they can still see any public descendants - delete_channels: vec![channel_id.to_proto()], + delete_channels: channels_to_remove.iter().map(|id| id.to_proto()).collect(), ..Default::default() }; for connection_id in connection_pool.user_connection_ids(user_id) { @@ -2645,7 +2643,7 @@ async fn join_channel_internal( leave_room_for_session(&session).await?; let db = session.db().await; - let (joined_room, accept_invite_result, role) = db + let (joined_room, membership_updated, role) = db .join_channel( channel_id, session.user_id, @@ -2691,10 +2689,10 @@ async fn join_channel_internal( })?; let connection_pool = session.connection_pool().await; - if let Some(accept_invite_result) = accept_invite_result { + if let Some(membership_updated) = membership_updated { notify_membership_updated( &connection_pool, - accept_invite_result, + membership_updated, session.user_id, &session.peer, ); diff --git a/crates/collab/src/tests/channel_tests.rs b/crates/collab/src/tests/channel_tests.rs index 85503f8ea352f49076419ebf97e9873c0deba9b5..8f3017287c82dc74be2e73f1451891be3a2fdeb3 100644 --- a/crates/collab/src/tests/channel_tests.rs +++ b/crates/collab/src/tests/channel_tests.rs @@ -1122,7 +1122,7 @@ async fn test_channel_link_notifications( assert_channels_list_shape( client_c.channel_store(), cx_c, - &[(zed_channel, 0), (helix_channel, 1)], + &[(zed_channel, 0)], ); } @@ -1250,44 +1250,79 @@ async fn test_guest_access( let client_b = server.create_client(cx_b, "user_b").await; let channels = server - .make_channel_tree(&[("channel-a", None)], (&client_a, cx_a)) + .make_channel_tree( + &[("channel-a", None), ("channel-b", Some("channel-a"))], + (&client_a, cx_a), + ) .await; - let channel_a_id = channels[0]; + let channel_a = channels[0]; + let channel_b = channels[1]; let active_call_b = cx_b.read(ActiveCall::global); - // should not be allowed to join + // Non-members should not be allowed to join assert!(active_call_b - .update(cx_b, |call, cx| call.join_channel(channel_a_id, cx)) + .update(cx_b, |call, cx| call.join_channel(channel_a, cx)) .await .is_err()); + // Make channels A and B public client_a .channel_store() .update(cx_a, |channel_store, cx| { - channel_store.set_channel_visibility(channel_a_id, proto::ChannelVisibility::Public, cx) + channel_store.set_channel_visibility(channel_a, proto::ChannelVisibility::Public, cx) + }) + .await + .unwrap(); + client_a + .channel_store() + .update(cx_a, |channel_store, cx| { + channel_store.set_channel_visibility(channel_b, proto::ChannelVisibility::Public, cx) }) .await .unwrap(); + // Client B joins channel A as a guest active_call_b - .update(cx_b, |call, cx| call.join_channel(channel_a_id, cx)) + .update(cx_b, |call, cx| call.join_channel(channel_a, cx)) .await .unwrap(); deterministic.run_until_parked(); - - assert!(client_b - .channel_store() - .update(cx_b, |channel_store, _| channel_store - .channel_for_id(channel_a_id) - .is_some())); + assert_channels_list_shape( + client_a.channel_store(), + cx_a, + &[(channel_a, 0), (channel_b, 1)], + ); + assert_channels_list_shape( + client_b.channel_store(), + cx_b, + &[(channel_a, 0), (channel_b, 1)], + ); client_a.channel_store().update(cx_a, |channel_store, _| { - let participants = channel_store.channel_participants(channel_a_id); + let participants = channel_store.channel_participants(channel_a); 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(); + + 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(); + + deterministic.run_until_parked(); + assert_channels_list_shape(client_b.channel_store(), cx_b, &[(channel_b, 0)]); } #[gpui::test] From 3358420f6a3386893b3434c77fcbf81a2f79e601 Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Tue, 24 Oct 2023 11:17:17 +0200 Subject: [PATCH 12/12] fix format --- crates/collab/src/tests/channel_tests.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/crates/collab/src/tests/channel_tests.rs b/crates/collab/src/tests/channel_tests.rs index 8f3017287c82dc74be2e73f1451891be3a2fdeb3..5b0bf02f8fb11fc85b9fef07472b3a7e106ab5f9 100644 --- a/crates/collab/src/tests/channel_tests.rs +++ b/crates/collab/src/tests/channel_tests.rs @@ -1119,11 +1119,7 @@ async fn test_channel_link_notifications( ) }); - assert_channels_list_shape( - client_c.channel_store(), - cx_c, - &[(zed_channel, 0)], - ); + assert_channels_list_shape(client_c.channel_store(), cx_c, &[(zed_channel, 0)]); } #[gpui::test]