diff --git a/crates/collab/src/db/queries/channels.rs b/crates/collab/src/db/queries/channels.rs index f31a1cde5dde738d2d9abfebe353e4a636a9b6a6..0ebd4dd6e1d4a4c90f20c36b3a8d528347f156fd 100644 --- a/crates/collab/src/db/queries/channels.rs +++ b/crates/collab/src/db/queries/channels.rs @@ -726,7 +726,7 @@ impl Database { .await } - pub async fn link_channel(&self, user: UserId, from: ChannelId, to: ChannelId) -> Result<()> { + async fn link_channel(&self, user: UserId, from: ChannelId, to: ChannelId) -> Result<()> { self.transaction(|tx| async move { self.check_user_is_channel_admin(to, user, &*tx).await?; @@ -789,13 +789,33 @@ impl Database { .await } + async fn remove_channel_from_parent(&self, user: UserId, from: ChannelId, parent: ChannelId) -> Result<()> { + todo!() + } + + /// Move a channel from one parent to another. + /// Note that this requires a valid parent_id in the 'from_parent' field. + /// As channels are a DAG, we need to know which parent to remove the channel from. + /// Here's a list of the parameters to this function and their behavior: + /// + /// - (`None`, `None`) Noop + /// - (`None`, `Some(id)`) Link the channel without removing it from any of it's parents + /// - (`Some(id)`, `None`) Remove a channel from a given parent, and leave other parents + /// - (`Some(id)`, `Some(id)`) Move channel from one parent to another, leaving other parents pub async fn move_channel( &self, user: UserId, from: ChannelId, + from_parent: Option, to: Option, ) -> Result<()> { - self.transaction(|tx| async move { todo!() }).await + if let Some(to) = to { + self.link_channel(user, from, to).await?; + } + if let Some(from_parent) = from_parent { + self.remove_channel_from_parent(user, from, from_parent).await?; + } + Ok(()) } } diff --git a/crates/collab/src/db/tests/channel_tests.rs b/crates/collab/src/db/tests/channel_tests.rs index e077950a3a94f8127e64a3d1c6164be5cbb43e1d..95405d4358ab18bf8b88897c305231f77cfc567e 100644 --- a/crates/collab/src/db/tests/channel_tests.rs +++ b/crates/collab/src/db/tests/channel_tests.rs @@ -502,6 +502,9 @@ async fn test_channels_moving(db: &Arc) { .unwrap(); // sanity check + // Initial DAG: + // /- gpui2 + // zed -- crdb - livestreaming - livestreaming_dag let result = db.get_channels_for_user(a_id).await.unwrap(); pretty_assertions::assert_eq!( result.channels, @@ -533,18 +536,33 @@ async fn test_channels_moving(db: &Arc) { }, ] ); - // Initial DAG: - // /- gpui2 - // zed -- crdb - livestreaming - livestreaming_dag // Attemp to make a cycle assert!(db - .link_channel(a_id, zed_id, livestreaming_id) + .move_channel(a_id, zed_id, None, Some(livestreaming_id)) + .await + .is_err()); + + // Attemp to remove an edge that doesn't exist + assert!(db + .move_channel(a_id, crdb_id, Some(gpui2_id), None) + .await + .is_err()); + + // Attemp to move to a channel that doesn't exist + assert!(db + .move_channel(a_id, crdb_id, Some(crate::db::ChannelId(1000)), None) + .await + .is_err()); + + // Attemp to remove an edge that doesn't exist + assert!(db + .move_channel(a_id, crdb_id, None, Some(crate::db::ChannelId(1000))) .await .is_err()); // Make a link - db.link_channel(a_id, livestreaming_id, zed_id) + db.move_channel(a_id, livestreaming_id, None, Some(zed_id)) .await .unwrap(); @@ -552,7 +570,6 @@ async fn test_channels_moving(db: &Arc) { // /- gpui2 // zed -- crdb - livestreaming - livestreaming_dag // \---------/ - let result = db.get_channels_for_user(a_id).await.unwrap(); pretty_assertions::assert_eq!( dbg!(result.channels), @@ -590,8 +607,14 @@ async fn test_channels_moving(db: &Arc) { ] ); + // 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), "6", a_id) + .create_channel( + "livestreaming_dag_sub", + Some(livestreaming_dag_id), + "6", + a_id, + ) .await .unwrap(); @@ -599,7 +622,6 @@ async fn test_channels_moving(db: &Arc) { // /- gpui2 // zed -- crdb - livestreaming - livestreaming_dag - livestreaming_dag_sub_id // \---------/ - let result = db.get_channels_for_user(a_id).await.unwrap(); pretty_assertions::assert_eq!( dbg!(result.channels), @@ -643,7 +665,7 @@ async fn test_channels_moving(db: &Arc) { ); // Make a link - db.link_channel(a_id, livestreaming_dag_sub_id, livestreaming_id) + db.move_channel(a_id, livestreaming_dag_sub_id, None, Some(livestreaming_id)) .await .unwrap(); @@ -651,7 +673,6 @@ async fn test_channels_moving(db: &Arc) { // /- gpui2 /---------------------\ // zed - crdb - livestreaming - livestreaming_dag - livestreaming_dag_sub_id // \--------/ - let result = db.get_channels_for_user(a_id).await.unwrap(); pretty_assertions::assert_eq!( dbg!(result.channels), @@ -700,7 +721,7 @@ async fn test_channels_moving(db: &Arc) { ); // Make another link - db.link_channel(a_id, livestreaming_id, gpui2_id) + db.move_channel(a_id, livestreaming_id, None, Some(gpui2_id)) .await .unwrap(); @@ -708,7 +729,123 @@ async fn test_channels_moving(db: &Arc) { // /- gpui2 -\ /---------------------\ // zed - crdb -- livestreaming - livestreaming_dag - livestreaming_dag_sub_id // \---------/ + let result = db.get_channels_for_user(a_id).await.unwrap(); + pretty_assertions::assert_eq!( + dbg!(result.channels), + vec![ + Channel { + id: zed_id, + name: "zed".to_string(), + parent_id: None, + }, + Channel { + id: crdb_id, + name: "crdb".to_string(), + parent_id: Some(zed_id), + }, + Channel { + id: gpui2_id, + name: "gpui2".to_string(), + parent_id: Some(zed_id), + }, + Channel { + id: livestreaming_id, + name: "livestreaming".to_string(), + parent_id: Some(gpui2_id), + }, + Channel { + id: livestreaming_id, + name: "livestreaming".to_string(), + parent_id: Some(zed_id), + }, + Channel { + id: livestreaming_id, + name: "livestreaming".to_string(), + parent_id: Some(crdb_id), + }, + Channel { + id: livestreaming_dag_id, + name: "livestreaming_dag".to_string(), + parent_id: Some(livestreaming_id), + }, + Channel { + id: livestreaming_dag_sub_id, + name: "livestreaming_dag_sub".to_string(), + parent_id: Some(livestreaming_id), + }, + Channel { + id: livestreaming_dag_sub_id, + name: "livestreaming_dag_sub".to_string(), + parent_id: Some(livestreaming_dag_id), + }, + ] + ); + + // Remove that inner link + db.move_channel(a_id, livestreaming_dag_sub_id, Some(livestreaming_id), None) + .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(); + pretty_assertions::assert_eq!( + dbg!(result.channels), + vec![ + Channel { + id: zed_id, + name: "zed".to_string(), + parent_id: None, + }, + Channel { + id: crdb_id, + name: "crdb".to_string(), + parent_id: Some(zed_id), + }, + Channel { + id: gpui2_id, + name: "gpui2".to_string(), + parent_id: Some(zed_id), + }, + Channel { + id: livestreaming_id, + name: "livestreaming".to_string(), + parent_id: Some(gpui2_id), + }, + Channel { + id: livestreaming_id, + name: "livestreaming".to_string(), + parent_id: Some(zed_id), + }, + Channel { + id: livestreaming_id, + name: "livestreaming".to_string(), + parent_id: Some(crdb_id), + }, + Channel { + id: livestreaming_dag_id, + name: "livestreaming_dag".to_string(), + parent_id: Some(livestreaming_id), + }, + Channel { + id: livestreaming_dag_sub_id, + name: "livestreaming_dag_sub".to_string(), + parent_id: Some(livestreaming_dag_id), + }, + ] + ); + // Remove that outer link + db.move_channel(a_id, livestreaming_id, Some(gpui2_id), None) + .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(); pretty_assertions::assert_eq!( dbg!(result.channels), @@ -751,6 +888,90 @@ async fn test_channels_moving(db: &Arc) { Channel { id: livestreaming_dag_sub_id, name: "livestreaming_dag_sub".to_string(), + parent_id: Some(livestreaming_dag_id), + }, + ] + ); + + // Move livestreaming to be below gpui2 + db.move_channel(a_id, livestreaming_id, Some(crdb_id), Some(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(); + pretty_assertions::assert_eq!( + dbg!(result.channels), + vec![ + Channel { + id: zed_id, + name: "zed".to_string(), + parent_id: None, + }, + Channel { + id: crdb_id, + name: "crdb".to_string(), + parent_id: Some(zed_id), + }, + Channel { + id: gpui2_id, + name: "gpui2".to_string(), + parent_id: Some(zed_id), + }, + Channel { + id: livestreaming_id, + name: "livestreaming".to_string(), + parent_id: Some(gpui2_id), + }, + Channel { + id: livestreaming_id, + name: "livestreaming".to_string(), + parent_id: Some(zed_id), + }, + Channel { + id: livestreaming_dag_id, + name: "livestreaming_dag".to_string(), + parent_id: Some(livestreaming_id), + }, + Channel { + id: livestreaming_dag_sub_id, + name: "livestreaming_dag_sub".to_string(), + parent_id: Some(livestreaming_dag_id), + }, + ] + ); + + // Deleting a channel should not delete children that still have other parents + db.remove_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(); + pretty_assertions::assert_eq!( + dbg!(result.channels), + vec![ + Channel { + id: zed_id, + name: "zed".to_string(), + parent_id: None, + }, + Channel { + id: crdb_id, + name: "crdb".to_string(), + parent_id: Some(zed_id), + }, + Channel { + id: livestreaming_id, + name: "livestreaming".to_string(), + parent_id: Some(zed_id), + }, + Channel { + id: livestreaming_dag_id, + name: "livestreaming_dag".to_string(), parent_id: Some(livestreaming_id), }, Channel { @@ -761,41 +982,13 @@ async fn test_channels_moving(db: &Arc) { ] ); - // // Attempt to make a cycle - // assert!(db - // .move_channel(a_id, zed_id, Some(livestreaming_id)) - // .await - // .is_err()); - - // // Move channel up - // db.move_channel(a_id, livestreaming_id, Some(zed_id)) - // .await - // .unwrap(); - - // let result = db.get_channels_for_user(a_id).await.unwrap(); - // pretty_assertions::assert_eq!( - // result.channels, - // vec![ - // Channel { - // id: zed_id, - // name: "zed".to_string(), - // parent_id: None, - // }, - // Channel { - // id: crdb_id, - // name: "crdb".to_string(), - // parent_id: Some(zed_id), - // }, - // Channel { - // id: crdb_id, - // name: "crdb".to_string(), - // parent_id: Some(livestreaming_id), - // }, - // Channel { - // id: livestreaming_id, - // name: "livestreaming".to_string(), - // parent_id: Some(zed_id), - // }, - // ] - // ); + // But deleting a parent of a DAG should delete the whole DAG: + db.move_channel(a_id, livestreaming_id, None, Some(crdb_id)).await.unwrap(); + // DAG is now: + // zed - crdb - livestreaming - livestreaming_dag - livestreaming_dag_sub + // \--------/ + + db.remove_channel(zed_id, a_id).await.unwrap(); + let result = db.get_channels_for_user(a_id).await.unwrap(); + assert!(result.channels.is_empty()) }