oak-team/oak-gpui
3
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Simplify Membership Management (#6747)

Browse Source 
Simplify Zed's collaboration system by:
- Only allowing member management on root channels.
- Disallowing moving sub-channels between different roots.
- Disallowing public channels nested under private channels.

This should make the mental model easier to understand, and makes it
clearer
who has what access. It is also significantly simpler to implement, and
so
hopefully more performant and less buggy.

Still TODO:
- [x] Update collab_ui to match.
- [x] Fix channel buffer tests.

Release Notes:

- Simplified channel membership management.
This commit is contained in:
Conrad Irwin
2024-01-26 11:17:16 -07:00
committed by GitHub
parent b5c64a128e 22034d2083
commit 8bc105ca1d
31 changed files with 900 additions and 1441 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/collab/src/db/tests/buffer_tests.rs
+4 -97
View File
@@ -330,8 +330,7 @@ async fn test_channel_buffers_last_operations(db: &Database) {
.transaction(|tx| {
let buffers = &buffers;
async move {
db.unseen_channel_buffer_changes(
observer_id,
db.latest_channel_buffer_changes(
&[
buffers[0].channel_id,
buffers[1].channel_id,
@@ -348,12 +347,12 @@ async fn test_channel_buffers_last_operations(db: &Database) {
pretty_assertions::assert_eq!(
buffer_changes,
[
rpc::proto::UnseenChannelBufferChange {
rpc::proto::ChannelBufferVersion {
channel_id: buffers[0].channel_id.to_proto(),
epoch: 0,
version: serialize_version(&text_buffers[0].version()),
},
rpc::proto::UnseenChannelBufferChange {
rpc::proto::ChannelBufferVersion {
channel_id: buffers[1].channel_id.to_proto(),
epoch: 1,
version: serialize_version(&text_buffers[1].version())
@@ -362,99 +361,7 @@ async fn test_channel_buffers_last_operations(db: &Database) {
== buffer_changes[1].version.first().unwrap().replica_id)
.collect::<Vec<_>>(),
},
rpc::proto::UnseenChannelBufferChange {
channel_id: buffers[2].channel_id.to_proto(),
epoch: 0,
version: serialize_version(&text_buffers[2].version()),
},
]
);
db.observe_buffer_version(
buffers[1].id,
observer_id,
1,
serialize_version(&text_buffers[1].version()).as_slice(),
)
.await
.unwrap();
let buffer_changes = db
.transaction(|tx| {
let buffers = &buffers;
async move {
db.unseen_channel_buffer_changes(
observer_id,
&[
buffers[0].channel_id,
buffers[1].channel_id,
buffers[2].channel_id,
],
&*tx,
)
.await
}
})
.await
.unwrap();
assert_eq!(
buffer_changes,
[
rpc::proto::UnseenChannelBufferChange {
channel_id: buffers[0].channel_id.to_proto(),
epoch: 0,
version: serialize_version(&text_buffers[0].version()),
},
rpc::proto::UnseenChannelBufferChange {
channel_id: buffers[2].channel_id.to_proto(),
epoch: 0,
version: serialize_version(&text_buffers[2].version()),
},
]
);
// Observe an earlier version of the buffer.
db.observe_buffer_version(
buffers[1].id,
observer_id,
1,
&[rpc::proto::VectorClockEntry {
replica_id: 0,
timestamp: 0,
}],
)
.await
.unwrap();
let buffer_changes = db
.transaction(|tx| {
let buffers = &buffers;
async move {
db.unseen_channel_buffer_changes(
observer_id,
&[
buffers[0].channel_id,
buffers[1].channel_id,
buffers[2].channel_id,
],
&*tx,
)
.await
}
})
.await
.unwrap();
assert_eq!(
buffer_changes,
[
rpc::proto::UnseenChannelBufferChange {
channel_id: buffers[0].channel_id.to_proto(),
epoch: 0,
version: serialize_version(&text_buffers[0].version()),
},
rpc::proto::UnseenChannelBufferChange {
rpc::proto::ChannelBufferVersion {
channel_id: buffers[2].channel_id.to_proto(),
epoch: 0,
version: serialize_version(&text_buffers[2].version()),
crates/collab/src/db/tests/channel_tests.rs
+78 -153
View File
@@ -62,23 +62,13 @@ async fn test_channels(db: &Arc<Database>) {
assert_eq!(
result.channels,
channel_tree(&[
(zed_id, &[], "zed", ChannelRole::Admin),
(crdb_id, &[zed_id], "crdb", ChannelRole::Admin),
(
livestreaming_id,
&[zed_id],
"livestreaming",
ChannelRole::Admin
),
(replace_id, &[zed_id], "replace", ChannelRole::Admin),
(rust_id, &[], "rust", ChannelRole::Admin),
(cargo_id, &[rust_id], "cargo", ChannelRole::Admin),
(
cargo_ra_id,
&[rust_id, cargo_id],
"cargo-ra",
ChannelRole::Admin
)
(zed_id, &[], "zed"),
(crdb_id, &[zed_id], "crdb"),
(livestreaming_id, &[zed_id], "livestreaming",),
(replace_id, &[zed_id], "replace"),
(rust_id, &[], "rust"),
(cargo_id, &[rust_id], "cargo"),
(cargo_ra_id, &[rust_id, cargo_id], "cargo-ra",)
],)
);
@@ -86,15 +76,10 @@ async fn test_channels(db: &Arc<Database>) {
assert_eq!(
result.channels,
channel_tree(&[
(zed_id, &[], "zed", ChannelRole::Member),
(crdb_id, &[zed_id], "crdb", ChannelRole::Member),
(
livestreaming_id,
&[zed_id],
"livestreaming",
ChannelRole::Member
),
(replace_id, &[zed_id], "replace", ChannelRole::Member)
(zed_id, &[], "zed"),
(crdb_id, &[zed_id], "crdb"),
(livestreaming_id, &[zed_id], "livestreaming",),
(replace_id, &[zed_id], "replace")
],)
);
@@ -112,15 +97,10 @@ async fn test_channels(db: &Arc<Database>) {
assert_eq!(
result.channels,
channel_tree(&[
(zed_id, &[], "zed", ChannelRole::Admin),
(crdb_id, &[zed_id], "crdb", ChannelRole::Admin),
(
livestreaming_id,
&[zed_id],
"livestreaming",
ChannelRole::Admin
),
(replace_id, &[zed_id], "replace", ChannelRole::Admin)
(zed_id, &[], "zed"),
(crdb_id, &[zed_id], "crdb"),
(livestreaming_id, &[zed_id], "livestreaming",),
(replace_id, &[zed_id], "replace")
],)
);
@@ -271,14 +251,19 @@ async fn test_channel_invites(db: &Arc<Database>) {
&[
proto::ChannelMember {
user_id: user_1.to_proto(),
kind: proto::channel_member::Kind::AncestorMember.into(),
kind: proto::channel_member::Kind::Member.into(),
role: proto::ChannelRole::Admin.into(),
},
proto::ChannelMember {
user_id: user_2.to_proto(),
kind: proto::channel_member::Kind::AncestorMember.into(),
kind: proto::channel_member::Kind::Member.into(),
role: proto::ChannelRole::Member.into(),
},
proto::ChannelMember {
user_id: user_3.to_proto(),
kind: proto::channel_member::Kind::Invitee.into(),
role: proto::ChannelRole::Admin.into(),
},
]
);
}
@@ -420,13 +405,6 @@ async fn test_db_channel_moving_bugs(db: &Arc<Database>) {
.await
.unwrap();
// Move to same parent should be a no-op
assert!(db
.move_channel(projects_id, Some(zed_id), user_id)
.await
.unwrap()
.is_none());
let result = db.get_channels_for_user(user_id).await.unwrap();
assert_channel_tree(
result.channels,
@@ -437,20 +415,8 @@ async fn test_db_channel_moving_bugs(db: &Arc<Database>) {
],
);
// Move the project channel to the root
db.move_channel(projects_id, None, user_id).await.unwrap();
let result = db.get_channels_for_user(user_id).await.unwrap();
assert_channel_tree(
result.channels,
&[
(zed_id, &[]),
(projects_id, &[]),
(livestreaming_id, &[projects_id]),
],
);
// Can't move a channel into its ancestor
db.move_channel(projects_id, Some(livestreaming_id), user_id)
db.move_channel(projects_id, livestreaming_id, user_id)
.await
.unwrap_err();
let result = db.get_channels_for_user(user_id).await.unwrap();
@@ -458,8 +424,8 @@ async fn test_db_channel_moving_bugs(db: &Arc<Database>) {
result.channels,
&[
(zed_id, &[]),
(projects_id, &[]),
(livestreaming_id, &[projects_id]),
(projects_id, &[zed_id]),
(livestreaming_id, &[zed_id, projects_id]),
],
);
}
@@ -476,32 +442,39 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
let guest = new_test_user(db, "guest@example.com").await;
let zed_channel = db.create_root_channel("zed", admin).await.unwrap();
let active_channel_id = db
let internal_channel_id = db
.create_sub_channel("active", zed_channel, admin)
.await
.unwrap();
let vim_channel_id = db
.create_sub_channel("vim", active_channel_id, admin)
let public_channel_id = db
.create_sub_channel("vim", zed_channel, admin)
.await
.unwrap();
db.set_channel_visibility(vim_channel_id, crate::db::ChannelVisibility::Public, admin)
db.set_channel_visibility(zed_channel, crate::db::ChannelVisibility::Public, admin)
.await
.unwrap();
db.invite_channel_member(active_channel_id, member, admin, ChannelRole::Member)
db.set_channel_visibility(
public_channel_id,
crate::db::ChannelVisibility::Public,
admin,
)
.await
.unwrap();
db.invite_channel_member(zed_channel, member, admin, ChannelRole::Member)
.await
.unwrap();
db.invite_channel_member(vim_channel_id, guest, admin, ChannelRole::Guest)
db.invite_channel_member(zed_channel, guest, admin, ChannelRole::Guest)
.await
.unwrap();
db.respond_to_channel_invite(active_channel_id, member, true)
db.respond_to_channel_invite(zed_channel, member, true)
.await
.unwrap();
db.transaction(|tx| async move {
db.check_user_is_channel_participant(
&db.get_channel_internal(vim_channel_id, &*tx).await?,
&db.get_channel_internal(public_channel_id, &*tx).await?,
admin,
&*tx,
)
@@ -511,7 +484,7 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
.unwrap();
db.transaction(|tx| async move {
db.check_user_is_channel_participant(
&db.get_channel_internal(vim_channel_id, &*tx).await?,
&db.get_channel_internal(public_channel_id, &*tx).await?,
member,
&*tx,
)
@@ -521,7 +494,7 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
.unwrap();
let mut members = db
.get_channel_participant_details(vim_channel_id, admin)
.get_channel_participant_details(public_channel_id, admin)
.await
.unwrap();
@@ -532,12 +505,12 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
&[
proto::ChannelMember {
user_id: admin.to_proto(),
kind: proto::channel_member::Kind::AncestorMember.into(),
kind: proto::channel_member::Kind::Member.into(),
role: proto::ChannelRole::Admin.into(),
},
proto::ChannelMember {
user_id: member.to_proto(),
kind: proto::channel_member::Kind::AncestorMember.into(),
kind: proto::channel_member::Kind::Member.into(),
role: proto::ChannelRole::Member.into(),
},
proto::ChannelMember {
@@ -548,13 +521,13 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
]
);
db.respond_to_channel_invite(vim_channel_id, guest, true)
db.respond_to_channel_invite(zed_channel, guest, true)
.await
.unwrap();
db.transaction(|tx| async move {
db.check_user_is_channel_participant(
&db.get_channel_internal(vim_channel_id, &*tx).await?,
&db.get_channel_internal(public_channel_id, &*tx).await?,
guest,
&*tx,
)
@@ -564,23 +537,29 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
.unwrap();
let channels = db.get_channels_for_user(guest).await.unwrap().channels;
assert_channel_tree(channels, &[(vim_channel_id, &[])]);
assert_channel_tree(
channels,
&[(zed_channel, &[]), (public_channel_id, &[zed_channel])],
);
let channels = db.get_channels_for_user(member).await.unwrap().channels;
assert_channel_tree(
channels,
&[
(active_channel_id, &[]),
(vim_channel_id, &[active_channel_id]),
(zed_channel, &[]),
(internal_channel_id, &[zed_channel]),
(public_channel_id, &[zed_channel]),
],
);
db.set_channel_member_role(vim_channel_id, admin, guest, ChannelRole::Banned)
db.set_channel_member_role(zed_channel, admin, guest, ChannelRole::Banned)
.await
.unwrap();
assert!(db
.transaction(|tx| async move {
db.check_user_is_channel_participant(
&db.get_channel_internal(vim_channel_id, &*tx).await.unwrap(),
&db.get_channel_internal(public_channel_id, &*tx)
.await
.unwrap(),
guest,
&*tx,
)
@@ -590,7 +569,7 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
.is_err());
let mut members = db
.get_channel_participant_details(vim_channel_id, admin)
.get_channel_participant_details(public_channel_id, admin)
.await
.unwrap();
@@ -601,12 +580,12 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
&[
proto::ChannelMember {
user_id: admin.to_proto(),
kind: proto::channel_member::Kind::AncestorMember.into(),
kind: proto::channel_member::Kind::Member.into(),
role: proto::ChannelRole::Admin.into(),
},
proto::ChannelMember {
user_id: member.to_proto(),
kind: proto::channel_member::Kind::AncestorMember.into(),
kind: proto::channel_member::Kind::Member.into(),
role: proto::ChannelRole::Member.into(),
},
proto::ChannelMember {
@@ -617,11 +596,7 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
]
);
db.remove_channel_member(vim_channel_id, guest, admin)
.await
.unwrap();
db.set_channel_visibility(zed_channel, crate::db::ChannelVisibility::Public, admin)
db.remove_channel_member(zed_channel, guest, admin)
.await
.unwrap();
@@ -631,7 +606,7 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
// currently people invited to parent channels are not shown here
let mut members = db
.get_channel_participant_details(vim_channel_id, admin)
.get_channel_participant_details(public_channel_id, admin)
.await
.unwrap();
@@ -642,14 +617,19 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
&[
proto::ChannelMember {
user_id: admin.to_proto(),
kind: proto::channel_member::Kind::AncestorMember.into(),
kind: proto::channel_member::Kind::Member.into(),
role: proto::ChannelRole::Admin.into(),
},
proto::ChannelMember {
user_id: member.to_proto(),
kind: proto::channel_member::Kind::AncestorMember.into(),
kind: proto::channel_member::Kind::Member.into(),
role: proto::ChannelRole::Member.into(),
},
proto::ChannelMember {
user_id: guest.to_proto(),
kind: proto::channel_member::Kind::Invitee.into(),
role: proto::ChannelRole::Guest.into(),
},
]
);
@@ -670,7 +650,7 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
assert!(db
.transaction(|tx| async move {
db.check_user_is_channel_participant(
&db.get_channel_internal(active_channel_id, &*tx)
&db.get_channel_internal(internal_channel_id, &*tx)
.await
.unwrap(),
guest,
@@ -683,7 +663,9 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
db.transaction(|tx| async move {
db.check_user_is_channel_participant(
&db.get_channel_internal(vim_channel_id, &*tx).await.unwrap(),
&db.get_channel_internal(public_channel_id, &*tx)
.await
.unwrap(),
guest,
&*tx,
)
@@ -693,7 +675,7 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
.unwrap();
let mut members = db
.get_channel_participant_details(vim_channel_id, admin)
.get_channel_participant_details(public_channel_id, admin)
.await
.unwrap();
@@ -704,17 +686,17 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
&[
proto::ChannelMember {
user_id: admin.to_proto(),
kind: proto::channel_member::Kind::AncestorMember.into(),
kind: proto::channel_member::Kind::Member.into(),
role: proto::ChannelRole::Admin.into(),
},
proto::ChannelMember {
user_id: member.to_proto(),
kind: proto::channel_member::Kind::AncestorMember.into(),
kind: proto::channel_member::Kind::Member.into(),
role: proto::ChannelRole::Member.into(),
},
proto::ChannelMember {
user_id: guest.to_proto(),
kind: proto::channel_member::Kind::AncestorMember.into(),
kind: proto::channel_member::Kind::Member.into(),
role: proto::ChannelRole::Guest.into(),
},
]
@@ -723,67 +705,10 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
let channels = db.get_channels_for_user(guest).await.unwrap().channels;
assert_channel_tree(
channels,
&[(zed_channel, &[]), (vim_channel_id, &[zed_channel])],
&[(zed_channel, &[]), (public_channel_id, &[zed_channel])],
)
}
test_both_dbs!(
test_user_joins_correct_channel,
test_user_joins_correct_channel_postgres,
test_user_joins_correct_channel_sqlite
);
async fn test_user_joins_correct_channel(db: &Arc<Database>) {
let admin = new_test_user(db, "admin@example.com").await;
let zed_channel = db.create_root_channel("zed", admin).await.unwrap();
let active_channel = db
.create_sub_channel("active", zed_channel, admin)
.await
.unwrap();
let vim_channel = db
.create_sub_channel("vim", active_channel, admin)
.await
.unwrap();
let vim2_channel = db
.create_sub_channel("vim2", vim_channel, admin)
.await
.unwrap();
db.set_channel_visibility(zed_channel, crate::db::ChannelVisibility::Public, admin)
.await
.unwrap();
db.set_channel_visibility(vim_channel, crate::db::ChannelVisibility::Public, admin)
.await
.unwrap();
db.set_channel_visibility(vim2_channel, crate::db::ChannelVisibility::Public, admin)
.await
.unwrap();
let most_public = db
.transaction(|tx| async move {
Ok(db
.public_ancestors_including_self(
&db.get_channel_internal(vim_channel, &*tx).await.unwrap(),
&tx,
)
.await?
.first()
.cloned())
})
.await
.unwrap()
.unwrap()
.id;
assert_eq!(most_public, zed_channel)
}
test_both_dbs!(
test_guest_access,
test_guest_access_postgres,
crates/collab/src/db/tests/message_tests.rs
+14 -80
View File
@@ -15,7 +15,7 @@ test_both_dbs!(
async fn test_channel_message_retrieval(db: &Arc<Database>) {
let user = new_test_user(db, "user@example.com").await;
let channel = db.create_channel("channel", None, user).await.unwrap();
let channel = db.create_channel("channel", None, user).await.unwrap().0;
let owner_id = db.create_server("test").await.unwrap().0 as u32;
db.join_channel_chat(channel.id, rpc::ConnectionId { owner_id, id: 0 }, user)
@@ -235,11 +235,10 @@ async fn test_unseen_channel_messages(db: &Arc<Database>) {
.await
.unwrap();
let second_message = db
let _ = db
.create_channel_message(channel_1, user, "1_2", &[], OffsetDateTime::now_utc(), 2)
.await
.unwrap()
.message_id;
.unwrap();
let third_message = db
.create_channel_message(channel_1, user, "1_3", &[], OffsetDateTime::now_utc(), 3)
@@ -258,97 +257,27 @@ async fn test_unseen_channel_messages(db: &Arc<Database>) {
.message_id;
// Check that observer has new messages
let unseen_messages = db
let latest_messages = db
.transaction(|tx| async move {
db.unseen_channel_messages(observer, &[channel_1, channel_2], &*tx)
db.latest_channel_messages(&[channel_1, channel_2], &*tx)
.await
})
.await
.unwrap();
assert_eq!(
unseen_messages,
latest_messages,
[
rpc::proto::UnseenChannelMessage {
rpc::proto::ChannelMessageId {
channel_id: channel_1.to_proto(),
message_id: third_message.to_proto(),
},
rpc::proto::UnseenChannelMessage {
rpc::proto::ChannelMessageId {
channel_id: channel_2.to_proto(),
message_id: fourth_message.to_proto(),
},
]
);
// Observe the second message
db.observe_channel_message(channel_1, observer, second_message)
.await
.unwrap();
// Make sure the observer still has a new message
let unseen_messages = db
.transaction(|tx| async move {
db.unseen_channel_messages(observer, &[channel_1, channel_2], &*tx)
.await
})
.await
.unwrap();
assert_eq!(
unseen_messages,
[
rpc::proto::UnseenChannelMessage {
channel_id: channel_1.to_proto(),
message_id: third_message.to_proto(),
},
rpc::proto::UnseenChannelMessage {
channel_id: channel_2.to_proto(),
message_id: fourth_message.to_proto(),
},
]
);
// Observe the third message,
db.observe_channel_message(channel_1, observer, third_message)
.await
.unwrap();
// Make sure the observer does not have a new method
let unseen_messages = db
.transaction(|tx| async move {
db.unseen_channel_messages(observer, &[channel_1, channel_2], &*tx)
.await
})
.await
.unwrap();
assert_eq!(
unseen_messages,
[rpc::proto::UnseenChannelMessage {
channel_id: channel_2.to_proto(),
message_id: fourth_message.to_proto(),
}]
);
// Observe the second message again, should not regress our observed state
db.observe_channel_message(channel_1, observer, second_message)
.await
.unwrap();
// Make sure the observer does not have a new message
let unseen_messages = db
.transaction(|tx| async move {
db.unseen_channel_messages(observer, &[channel_1, channel_2], &*tx)
.await
})
.await
.unwrap();
assert_eq!(
unseen_messages,
[rpc::proto::UnseenChannelMessage {
channel_id: channel_2.to_proto(),
message_id: fourth_message.to_proto(),
}]
);
}
test_both_dbs!(
@@ -362,7 +291,12 @@ async fn test_channel_message_mentions(db: &Arc<Database>) {
let user_b = new_test_user(db, "user_b@example.com").await;
let user_c = new_test_user(db, "user_c@example.com").await;
let channel = db.create_channel("channel", None, user_a).await.unwrap().id;
let channel = db
.create_channel("channel", None, user_a)
.await
.unwrap()
.0
.id;
db.invite_channel_member(channel, user_b, user_a, ChannelRole::Member)
.await
.unwrap();