From 0324ca3b081f4785c4cbbad385e0615e0c4826e4 Mon Sep 17 00:00:00 2001 From: Julia Date: Wed, 22 Feb 2023 15:29:20 -0500 Subject: [PATCH] Be more specific about clearing (leader, follower) row 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 --- crates/collab/src/db.rs | 26 ++---- crates/collab/src/tests/integration_tests.rs | 98 +++++++++++++++++--- 2 files changed, 96 insertions(+), 28 deletions(-) diff --git a/crates/collab/src/db.rs b/crates/collab/src/db.rs index 6b74c47d3e..97c9b6d344 100644 --- a/crates/collab/src/db.rs +++ b/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), ), ), ) diff --git a/crates/collab/src/tests/integration_tests.rs b/crates/collab/src/tests/integration_tests.rs index 2fc19b005b..7c4087f540 100644 --- a/crates/collab/src/tests/integration_tests.rs +++ b/crates/collab/src/tests/integration_tests.rs @@ -5786,6 +5786,7 @@ async fn test_following( deterministic: Arc, 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::() .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