Update get_channel_participant_details to include guests

Conrad Irwin created

Change summary

crates/collab/src/db/ids.rs                 |  12 +
crates/collab/src/db/queries/channels.rs    | 106 +++++++++--
crates/collab/src/db/tests/channel_tests.rs | 203 +++++++++++++++-------
3 files changed, 231 insertions(+), 90 deletions(-)

Detailed changes

crates/collab/src/db/ids.rs 🔗

@@ -95,6 +95,18 @@ pub enum ChannelRole {
     Banned,
 }
 
+impl ChannelRole {
+    pub fn should_override(&self, other: Self) -> bool {
+        use ChannelRole::*;
+        match self {
+            Admin => matches!(other, Member | Banned | Guest),
+            Member => matches!(other, Banned | Guest),
+            Banned => matches!(other, Guest),
+            Guest => false,
+        }
+    }
+}
+
 impl From<proto::ChannelRole> for ChannelRole {
     fn from(value: proto::ChannelRole) -> Self {
         match value {

crates/collab/src/db/queries/channels.rs 🔗

@@ -1,5 +1,7 @@
+use std::cmp::Ordering;
+
 use super::*;
-use rpc::proto::ChannelEdge;
+use rpc::proto::{channel_member::Kind, ChannelEdge};
 use smallvec::SmallVec;
 
 type ChannelDescendants = HashMap<ChannelId, SmallSet<ChannelId>>;
@@ -539,12 +541,19 @@ impl Database {
     pub async fn get_channel_participant_details(
         &self,
         channel_id: ChannelId,
-        user_id: UserId,
+        admin_id: UserId,
     ) -> Result<Vec<proto::ChannelMember>> {
         self.transaction(|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_visibility = channel::Entity::find()
+                .filter(channel::Column::Id.eq(channel_id))
+                .one(&*tx)
+                .await?
+                .map(|channel| channel.visibility)
+                .unwrap_or(ChannelVisibility::ChannelMembers);
+
             #[derive(Copy, Clone, Debug, EnumIter, DeriveColumn)]
             enum QueryMemberDetails {
                 UserId,
@@ -552,12 +561,13 @@ impl Database {
                 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()
-                .distinct()
+                .left_join(channel::Entity)
                 .filter(channel_member::Column::ChannelId.is_in(ancestor_ids.iter().copied()))
                 .select_only()
                 .column(channel_member::Column::UserId)
@@ -568,19 +578,32 @@ impl Database {
                     QueryMemberDetails::IsDirectMember,
                 )
                 .column(channel_member::Column::Accepted)
-                .order_by_asc(channel_member::Column::UserId)
+                .column(channel::Column::Visibility)
                 .into_values::<_, QueryMemberDetails>()
                 .stream(&*tx)
                 .await?;
 
-            let mut rows = Vec::<proto::ChannelMember>::new();
+            struct UserDetail {
+                kind: Kind,
+                channel_role: ChannelRole,
+            }
+            let mut user_details: HashMap<UserId, UserDetail> = HashMap::default();
+
             while let Some(row) = stream.next().await {
-                let (user_id, is_admin, channel_role, is_direct_member, is_invite_accepted): (
+                let (
+                    user_id,
+                    is_admin,
+                    channel_role,
+                    is_direct_member,
+                    is_invite_accepted,
+                    visibility,
+                ): (
                     UserId,
                     bool,
                     Option<ChannelRole>,
                     bool,
                     bool,
+                    ChannelVisibility,
                 ) = row?;
                 let kind = match (is_direct_member, is_invite_accepted) {
                     (true, true) => proto::channel_member::Kind::Member,
@@ -593,25 +616,64 @@ impl Database {
                 } else {
                     ChannelRole::Member
                 });
-                let user_id = user_id.to_proto();
-                let kind = kind.into();
-                if let Some(last_row) = rows.last_mut() {
-                    if last_row.user_id == user_id {
-                        if is_direct_member {
-                            last_row.kind = kind;
-                            last_row.role = channel_role.into()
-                        }
-                        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.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 });
                 }
-                rows.push(proto::ChannelMember {
-                    user_id,
-                    kind,
-                    role: channel_role.into(),
-                });
             }
 
-            Ok(rows)
+            // sort by permissions descending, within each section, show members, then ancestor members, then invitees.
+            let mut results: Vec<(UserId, UserDetail)> = user_details.into_iter().collect();
+            results.sort_by(|a, b| {
+                if a.1.channel_role.should_override(b.1.channel_role) {
+                    return Ordering::Less;
+                } else if b.1.channel_role.should_override(a.1.channel_role) {
+                    return Ordering::Greater;
+                }
+
+                if a.1.kind == Kind::Member && b.1.kind != Kind::Member {
+                    return Ordering::Less;
+                } else if b.1.kind == Kind::Member && a.1.kind != Kind::Member {
+                    return Ordering::Greater;
+                }
+
+                if a.1.kind == Kind::AncestorMember && b.1.kind != Kind::AncestorMember {
+                    return Ordering::Less;
+                } else if b.1.kind == Kind::AncestorMember && a.1.kind != Kind::AncestorMember {
+                    return Ordering::Greater;
+                }
+
+                // would be nice to sort alphabetically instead of by user id.
+                // (or defer all sorting to the UI, but we need something to help the tests)
+                return a.0.cmp(&b.0);
+            });
+
+            Ok(results
+                .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
     }

crates/collab/src/db/tests/channel_tests.rs 🔗

@@ -246,46 +246,9 @@ test_both_dbs!(
 async fn test_channel_invites(db: &Arc<Database>) {
     db.create_server("test").await.unwrap();
 
-    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_3 = db
-        .create_user(
-            "user3@example.com",
-            false,
-            NewUserParams {
-                github_login: "user3".into(),
-                github_user_id: 7,
-                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 user_3 = new_test_user(db, "user3@example.com").await;
 
     let channel_1_1 = db.create_root_channel("channel_1", user_1).await.unwrap();
 
@@ -334,14 +297,14 @@ async fn test_channel_invites(db: &Arc<Database>) {
                 role: proto::ChannelRole::Admin.into(),
             },
             proto::ChannelMember {
-                user_id: user_2.to_proto(),
+                user_id: user_3.to_proto(),
                 kind: proto::channel_member::Kind::Invitee.into(),
-                role: proto::ChannelRole::Member.into(),
+                role: proto::ChannelRole::Admin.into(),
             },
             proto::ChannelMember {
-                user_id: user_3.to_proto(),
+                user_id: user_2.to_proto(),
                 kind: proto::channel_member::Kind::Invitee.into(),
-                role: proto::ChannelRole::Admin.into(),
+                role: proto::ChannelRole::Member.into(),
             },
         ]
     );
@@ -860,92 +823,198 @@ test_both_dbs!(
 );
 
 async fn test_user_is_channel_participant(db: &Arc<Database>) {
-    let admin_id = new_test_user(db, "admin@example.com").await;
-    let member_id = new_test_user(db, "member@example.com").await;
-    let guest_id = new_test_user(db, "guest@example.com").await;
+    let admin = new_test_user(db, "admin@example.com").await;
+    let member = new_test_user(db, "member@example.com").await;
+    let guest = new_test_user(db, "guest@example.com").await;
 
-    let zed_id = db.create_root_channel("zed", admin_id).await.unwrap();
-    let intermediate_id = db
-        .create_channel("active", Some(zed_id), admin_id)
+    let zed_channel = db.create_root_channel("zed", admin).await.unwrap();
+    let active_channel = db
+        .create_channel("active", Some(zed_channel), admin)
         .await
         .unwrap();
-    let public_id = db
-        .create_channel("active", Some(intermediate_id), admin_id)
+    let vim_channel = db
+        .create_channel("vim", Some(active_channel), admin)
         .await
         .unwrap();
 
-    db.set_channel_visibility(public_id, crate::db::ChannelVisibility::Public, admin_id)
+    db.set_channel_visibility(vim_channel, crate::db::ChannelVisibility::Public, admin)
+        .await
+        .unwrap();
+    db.invite_channel_member(active_channel, member, admin, ChannelRole::Member)
         .await
         .unwrap();
-    db.invite_channel_member(intermediate_id, member_id, admin_id, ChannelRole::Member)
+    db.invite_channel_member(vim_channel, guest, admin, ChannelRole::Guest)
         .await
         .unwrap();
-    db.invite_channel_member(public_id, guest_id, admin_id, ChannelRole::Guest)
+
+    db.respond_to_channel_invite(active_channel, member, true)
         .await
         .unwrap();
 
     db.transaction(|tx| async move {
-        db.check_user_is_channel_participant(public_id, admin_id, &*tx)
+        db.check_user_is_channel_participant(vim_channel, admin, &*tx)
             .await
     })
     .await
     .unwrap();
     db.transaction(|tx| async move {
-        db.check_user_is_channel_participant(public_id, member_id, &*tx)
+        db.check_user_is_channel_participant(vim_channel, member, &*tx)
             .await
     })
     .await
     .unwrap();
     db.transaction(|tx| async move {
-        db.check_user_is_channel_participant(public_id, guest_id, &*tx)
+        db.check_user_is_channel_participant(vim_channel, guest, &*tx)
             .await
     })
     .await
     .unwrap();
 
-    db.set_channel_member_role(public_id, admin_id, guest_id, ChannelRole::Banned)
+    let members = db
+        .get_channel_participant_details(vim_channel, admin)
+        .await
+        .unwrap();
+    assert_eq!(
+        members,
+        &[
+            proto::ChannelMember {
+                user_id: admin.to_proto(),
+                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(),
+                role: proto::ChannelRole::Member.into(),
+            },
+            proto::ChannelMember {
+                user_id: guest.to_proto(),
+                kind: proto::channel_member::Kind::Invitee.into(),
+                role: proto::ChannelRole::Guest.into(),
+            },
+        ]
+    );
+
+    db.set_channel_member_role(vim_channel, admin, guest, ChannelRole::Banned)
         .await
         .unwrap();
     assert!(db
         .transaction(|tx| async move {
-            db.check_user_is_channel_participant(public_id, guest_id, &*tx)
+            db.check_user_is_channel_participant(vim_channel, guest, &*tx)
                 .await
         })
         .await
         .is_err());
 
-    db.remove_channel_member(public_id, guest_id, admin_id)
+    let members = db
+        .get_channel_participant_details(vim_channel, admin)
+        .await
+        .unwrap();
+
+    assert_eq!(
+        members,
+        &[
+            proto::ChannelMember {
+                user_id: admin.to_proto(),
+                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(),
+                role: proto::ChannelRole::Member.into(),
+            },
+            proto::ChannelMember {
+                user_id: guest.to_proto(),
+                kind: proto::channel_member::Kind::Invitee.into(),
+                role: proto::ChannelRole::Banned.into(),
+            },
+        ]
+    );
+
+    db.remove_channel_member(vim_channel, guest, admin)
         .await
         .unwrap();
 
-    db.set_channel_visibility(zed_id, crate::db::ChannelVisibility::Public, admin_id)
+    db.set_channel_visibility(zed_channel, crate::db::ChannelVisibility::Public, admin)
         .await
         .unwrap();
 
-    db.invite_channel_member(zed_id, guest_id, admin_id, ChannelRole::Guest)
+    db.invite_channel_member(zed_channel, guest, admin, ChannelRole::Guest)
         .await
         .unwrap();
 
     db.transaction(|tx| async move {
-        db.check_user_is_channel_participant(zed_id, guest_id, &*tx)
+        db.check_user_is_channel_participant(zed_channel, guest, &*tx)
             .await
     })
     .await
     .unwrap();
     assert!(db
         .transaction(|tx| async move {
-            db.check_user_is_channel_participant(intermediate_id, guest_id, &*tx)
+            db.check_user_is_channel_participant(active_channel, guest, &*tx)
                 .await
         })
         .await
         .is_err(),);
 
     db.transaction(|tx| async move {
-        db.check_user_is_channel_participant(public_id, guest_id, &*tx)
+        db.check_user_is_channel_participant(vim_channel, guest, &*tx)
             .await
     })
     .await
     .unwrap();
+
+    // currently people invited to parent channels are not shown here
+    // (though they *do* have permissions!)
+    let members = db
+        .get_channel_participant_details(vim_channel, admin)
+        .await
+        .unwrap();
+    assert_eq!(
+        members,
+        &[
+            proto::ChannelMember {
+                user_id: admin.to_proto(),
+                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(),
+                role: proto::ChannelRole::Member.into(),
+            },
+        ]
+    );
+
+    db.respond_to_channel_invite(zed_channel, guest, true)
+        .await
+        .unwrap();
+
+    let members = db
+        .get_channel_participant_details(vim_channel, admin)
+        .await
+        .unwrap();
+    assert_eq!(
+        members,
+        &[
+            proto::ChannelMember {
+                user_id: admin.to_proto(),
+                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(),
+                role: proto::ChannelRole::Member.into(),
+            },
+            proto::ChannelMember {
+                user_id: guest.to_proto(),
+                kind: proto::channel_member::Kind::AncestorMember.into(),
+                role: proto::ChannelRole::Guest.into(),
+            },
+        ]
+    );
 }
 
 #[track_caller]
@@ -976,8 +1045,6 @@ fn assert_dag(actual: ChannelGraph, expected: &[(ChannelId, Option<ChannelId>)])
 static GITHUB_USER_ID: AtomicI32 = AtomicI32::new(5);
 
 async fn new_test_user(db: &Arc<Database>, email: &str) -> UserId {
-    let gid = GITHUB_USER_ID.fetch_add(1, Ordering::SeqCst);
-
     db.create_user(
         email,
         false,