Be more specific about clearing (leader, follower) row

Julia and Max Brunsfeld created 2 years ago

Previously anyone unfollowing someone would clear all other rows for
other followers leading to an incorrect state, fix and test

Co-Authored-By: Max Brunsfeld <[e-mail]>

Change summary

crates/collab/src/db.rs                      | 26 ++---
crates/collab/src/tests/integration_tests.rs | 98 +++++++++++++++++++--
2 files changed, 96 insertions(+), 28 deletions(-)

Detailed changes

crates/collab/src/db.rs

@@ -1760,22 +1760,16 @@ impl Database {
                     Condition::all()
                         .add(follower::Column::ProjectId.eq(project_id))
                         .add(
-                            Condition::any()
-                                .add(
-                                    follower::Column::LeaderConnectionServerId
-                                        .eq(leader_connection.owner_id)
-                                        .and(
-                                            follower::Column::LeaderConnectionId
-                                                .eq(leader_connection.id),
-                                        ),
-                                )
-                                .add(
-                                    follower::Column::FollowerConnectionServerId
-                                        .eq(follower_connection.owner_id)
-                                        .and(
-                                            follower::Column::FollowerConnectionId
-                                                .eq(follower_connection.id),
-                                        ),
+                            follower::Column::LeaderConnectionServerId
+                                .eq(leader_connection.owner_id)
+                                .and(follower::Column::LeaderConnectionId.eq(leader_connection.id)),
+                        )
+                        .add(
+                            follower::Column::FollowerConnectionServerId
+                                .eq(follower_connection.owner_id)
+                                .and(
+                                    follower::Column::FollowerConnectionId
+                                        .eq(follower_connection.id),
                                 ),
                         ),
                 )

crates/collab/src/tests/integration_tests.rs

@@ -5786,6 +5786,7 @@ async fn test_following(
     deterministic: Arc<Deterministic>,
     cx_a: &mut TestAppContext,
     cx_b: &mut TestAppContext,
+    cx_c: &mut TestAppContext,
 ) {
     deterministic.forbid_parking();
     cx_a.update(editor::init);
@@ -5794,9 +5795,13 @@ async fn test_following(
     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;
     server
         .create_room(&mut [(&client_a, cx_a), (&client_b, cx_b)])
         .await;
+    server
+        .make_contacts(&mut [(&client_a, cx_a), (&client_c, cx_c)])
+        .await;
     let active_call_a = cx_a.read(ActiveCall::global);
     let active_call_b = cx_b.read(ActiveCall::global);
 
@@ -5827,8 +5832,10 @@ async fn test_following(
         .await
         .unwrap();
 
-    // Client A opens some editors.
     let workspace_a = client_a.build_workspace(&project_a, cx_a);
+    let workspace_b = client_b.build_workspace(&project_b, cx_b);
+
+    // Client A opens some editors.
     let pane_a = workspace_a.read_with(cx_a, |workspace, _| workspace.active_pane().clone());
     let editor_a1 = workspace_a
         .update(cx_a, |workspace, cx| {
@@ -5848,7 +5855,6 @@ async fn test_following(
         .unwrap();
 
     // Client B opens an editor.
-    let workspace_b = client_b.build_workspace(&project_b, cx_b);
     let editor_b1 = workspace_b
         .update(cx_b, |workspace, cx| {
             workspace.open_path((worktree_id, "1.txt"), None, true, cx)
@@ -5858,29 +5864,97 @@ async fn test_following(
         .downcast::<Editor>()
         .unwrap();
 
-    let client_a_id = project_b.read_with(cx_b, |project, _| {
-        project.collaborators().values().next().unwrap().peer_id
-    });
-    let client_b_id = project_a.read_with(cx_a, |project, _| {
-        project.collaborators().values().next().unwrap().peer_id
-    });
+    let peer_id_a = client_a.peer_id().unwrap();
+    let peer_id_b = client_b.peer_id().unwrap();
+    let peer_id_c = client_c.peer_id().unwrap();
 
-    // When client B starts following client A, all visible view states are replicated to client B.
+    // Client A updates their selections in those editors
     editor_a1.update(cx_a, |editor, cx| {
         editor.change_selections(None, cx, |s| s.select_ranges([0..1]))
     });
     editor_a2.update(cx_a, |editor, cx| {
         editor.change_selections(None, cx, |s| s.select_ranges([2..3]))
     });
+
+    // When client B starts following client A, all visible view states are replicated to client B.
     workspace_b
         .update(cx_b, |workspace, cx| {
             workspace
-                .toggle_follow(&ToggleFollow(client_a_id), cx)
+                .toggle_follow(&ToggleFollow(peer_id_a), cx)
+                .unwrap()
+        })
+        .await
+        .unwrap();
+
+    // Client A invites client C to the call.
+    active_call_a
+        .update(cx_a, |call, cx| {
+            call.invite(client_c.current_user_id(cx_c).to_proto(), None, cx)
+        })
+        .await
+        .unwrap();
+    cx_c.foreground().run_until_parked();
+    let active_call_c = cx_c.read(ActiveCall::global);
+    active_call_c
+        .update(cx_c, |call, cx| call.accept_incoming(cx))
+        .await
+        .unwrap();
+    let project_c = client_c.build_remote_project(project_id, cx_c).await;
+    let workspace_c = client_c.build_workspace(&project_c, cx_c);
+    active_call_c
+        .update(cx_c, |call, cx| call.set_location(Some(&project_c), cx))
+        .await
+        .unwrap();
+
+    // Client C also follows client A.
+    workspace_c
+        .update(cx_c, |workspace, cx| {
+            workspace
+                .toggle_follow(&ToggleFollow(peer_id_a), cx)
                 .unwrap()
         })
         .await
         .unwrap();
 
+    // All clients see that clients B and C are following client A.
+    cx_c.foreground().run_until_parked();
+    for (name, active_call, cx) in [
+        ("A", &active_call_a, &cx_a),
+        ("B", &active_call_b, &cx_b),
+        ("C", &active_call_c, &cx_c),
+    ] {
+        active_call.read_with(*cx, |call, cx| {
+            let room = call.room().unwrap().read(cx);
+            assert_eq!(
+                room.followers_for(peer_id_a),
+                &[peer_id_b, peer_id_c],
+                "checking followers for A as {name}"
+            );
+        });
+    }
+
+    // Client C unfollows client A.
+    workspace_c.update(cx_c, |workspace, cx| {
+        workspace.toggle_follow(&ToggleFollow(peer_id_a), cx);
+    });
+
+    // All clients see that clients B is following client A.
+    cx_c.foreground().run_until_parked();
+    for (name, active_call, cx) in [
+        ("A", &active_call_a, &cx_a),
+        ("B", &active_call_b, &cx_b),
+        ("C", &active_call_c, &cx_c),
+    ] {
+        active_call.read_with(*cx, |call, cx| {
+            let room = call.room().unwrap().read(cx);
+            assert_eq!(
+                room.followers_for(peer_id_a),
+                &[peer_id_b],
+                "checking followers for A as {name}"
+            );
+        });
+    }
+
     let editor_b2 = workspace_b.read_with(cx_b, |workspace, cx| {
         workspace
             .active_item(cx)
@@ -6033,14 +6107,14 @@ async fn test_following(
     workspace_a
         .update(cx_a, |workspace, cx| {
             workspace
-                .toggle_follow(&ToggleFollow(client_b_id), cx)
+                .toggle_follow(&ToggleFollow(peer_id_b), cx)
                 .unwrap()
         })
         .await
         .unwrap();
     assert_eq!(
         workspace_a.read_with(cx_a, |workspace, _| workspace.leader_for_pane(&pane_a)),
-        Some(client_b_id)
+        Some(peer_id_b)
     );
     assert_eq!(
         workspace_a.read_with(cx_a, |workspace, cx| workspace