From 1d1bd3975a50fa513ec5ada150a218eb2c248c9b Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Tue, 27 Sep 2022 18:24:22 +0200 Subject: [PATCH] Remove current user from contacts Co-Authored-By: Nathan Sobo Co-Authored-By: Mikayla Maki --- crates/collab/src/db.rs | 220 +++-------- crates/collab/src/integration_tests.rs | 342 ++++++++++-------- .../src/add_participant_popover.rs | 37 +- .../src/collab_titlebar_item.rs | 1 - 4 files changed, 264 insertions(+), 336 deletions(-) diff --git a/crates/collab/src/db.rs b/crates/collab/src/db.rs index eeb598413ef86e9b5bdf496f3b6f02baa7114c6c..876d16b60b3f9c4790747b2626e07cb75ab8d351 100644 --- a/crates/collab/src/db.rs +++ b/crates/collab/src/db.rs @@ -842,10 +842,7 @@ impl Db for PostgresDb { .bind(user_id) .fetch(&self.pool); - let mut contacts = vec![Contact::Accepted { - user_id, - should_notify: false, - }]; + let mut contacts = Vec::new(); while let Some(row) = rows.next().await { let (user_id_a, user_id_b, a_to_b, accepted, should_notify) = row?; @@ -2026,13 +2023,7 @@ pub mod tests { let user_3 = db.create_user("user3", None, false).await.unwrap(); // User starts with no contacts - assert_eq!( - db.get_contacts(user_1).await.unwrap(), - vec![Contact::Accepted { - user_id: user_1, - should_notify: false - }], - ); + assert_eq!(db.get_contacts(user_1).await.unwrap(), vec![]); // User requests a contact. Both users see the pending request. db.send_contact_request(user_1, user_2).await.unwrap(); @@ -2040,26 +2031,14 @@ pub mod tests { assert!(!db.has_contact(user_2, user_1).await.unwrap()); assert_eq!( db.get_contacts(user_1).await.unwrap(), - &[ - Contact::Accepted { - user_id: user_1, - should_notify: false - }, - Contact::Outgoing { user_id: user_2 } - ], + &[Contact::Outgoing { user_id: user_2 }], ); assert_eq!( db.get_contacts(user_2).await.unwrap(), - &[ - Contact::Incoming { - user_id: user_1, - should_notify: true - }, - Contact::Accepted { - user_id: user_2, - should_notify: false - }, - ] + &[Contact::Incoming { + user_id: user_1, + should_notify: true + }] ); // User 2 dismisses the contact request notification without accepting or rejecting. @@ -2072,16 +2051,10 @@ pub mod tests { .unwrap(); assert_eq!( db.get_contacts(user_2).await.unwrap(), - &[ - Contact::Incoming { - user_id: user_1, - should_notify: false - }, - Contact::Accepted { - user_id: user_2, - should_notify: false - }, - ] + &[Contact::Incoming { + user_id: user_1, + should_notify: false + }] ); // User can't accept their own contact request @@ -2095,31 +2068,19 @@ pub mod tests { .unwrap(); assert_eq!( db.get_contacts(user_1).await.unwrap(), - &[ - Contact::Accepted { - user_id: user_1, - should_notify: false - }, - Contact::Accepted { - user_id: user_2, - should_notify: true - } - ], + &[Contact::Accepted { + user_id: user_2, + should_notify: true + }], ); assert!(db.has_contact(user_1, user_2).await.unwrap()); assert!(db.has_contact(user_2, user_1).await.unwrap()); assert_eq!( db.get_contacts(user_2).await.unwrap(), - &[ - Contact::Accepted { - user_id: user_1, - should_notify: false, - }, - Contact::Accepted { - user_id: user_2, - should_notify: false, - }, - ] + &[Contact::Accepted { + user_id: user_1, + should_notify: false, + }] ); // Users cannot re-request existing contacts. @@ -2132,16 +2093,10 @@ pub mod tests { .unwrap_err(); assert_eq!( db.get_contacts(user_1).await.unwrap(), - &[ - Contact::Accepted { - user_id: user_1, - should_notify: false - }, - Contact::Accepted { - user_id: user_2, - should_notify: true, - }, - ] + &[Contact::Accepted { + user_id: user_2, + should_notify: true, + }] ); // Users can dismiss notifications of other users accepting their requests. @@ -2150,16 +2105,10 @@ pub mod tests { .unwrap(); assert_eq!( db.get_contacts(user_1).await.unwrap(), - &[ - Contact::Accepted { - user_id: user_1, - should_notify: false - }, - Contact::Accepted { - user_id: user_2, - should_notify: false, - }, - ] + &[Contact::Accepted { + user_id: user_2, + should_notify: false, + },] ); // Users send each other concurrent contact requests and @@ -2169,10 +2118,6 @@ pub mod tests { assert_eq!( db.get_contacts(user_1).await.unwrap(), &[ - Contact::Accepted { - user_id: user_1, - should_notify: false - }, Contact::Accepted { user_id: user_2, should_notify: false, @@ -2185,16 +2130,10 @@ pub mod tests { ); assert_eq!( db.get_contacts(user_3).await.unwrap(), - &[ - Contact::Accepted { - user_id: user_1, - should_notify: false - }, - Contact::Accepted { - user_id: user_3, - should_notify: false - } - ], + &[Contact::Accepted { + user_id: user_1, + should_notify: false + }], ); // User declines a contact request. Both users see that it is gone. @@ -2206,29 +2145,17 @@ pub mod tests { assert!(!db.has_contact(user_3, user_2).await.unwrap()); assert_eq!( db.get_contacts(user_2).await.unwrap(), - &[ - Contact::Accepted { - user_id: user_1, - should_notify: false - }, - Contact::Accepted { - user_id: user_2, - should_notify: false - } - ] + &[Contact::Accepted { + user_id: user_1, + should_notify: false + }] ); assert_eq!( db.get_contacts(user_3).await.unwrap(), - &[ - Contact::Accepted { - user_id: user_1, - should_notify: false - }, - Contact::Accepted { - user_id: user_3, - should_notify: false - } - ], + &[Contact::Accepted { + user_id: user_1, + should_notify: false + }], ); } } @@ -2261,29 +2188,17 @@ pub mod tests { assert_eq!(invite_count, 1); assert_eq!( db.get_contacts(user1).await.unwrap(), - [ - Contact::Accepted { - user_id: user1, - should_notify: false - }, - Contact::Accepted { - user_id: user2, - should_notify: true - } - ] + [Contact::Accepted { + user_id: user2, + should_notify: true + }] ); assert_eq!( db.get_contacts(user2).await.unwrap(), - [ - Contact::Accepted { - user_id: user1, - should_notify: false - }, - Contact::Accepted { - user_id: user2, - should_notify: false - } - ] + [Contact::Accepted { + user_id: user1, + should_notify: false + }] ); // User 3 redeems the invite code and becomes a contact of user 1. @@ -2296,10 +2211,6 @@ pub mod tests { assert_eq!( db.get_contacts(user1).await.unwrap(), [ - Contact::Accepted { - user_id: user1, - should_notify: false - }, Contact::Accepted { user_id: user2, should_notify: true @@ -2312,16 +2223,10 @@ pub mod tests { ); assert_eq!( db.get_contacts(user3).await.unwrap(), - [ - Contact::Accepted { - user_id: user1, - should_notify: false - }, - Contact::Accepted { - user_id: user3, - should_notify: false - }, - ] + [Contact::Accepted { + user_id: user1, + should_notify: false + }] ); // Trying to reedem the code for the third time results in an error. @@ -2346,10 +2251,6 @@ pub mod tests { assert_eq!( db.get_contacts(user1).await.unwrap(), [ - Contact::Accepted { - user_id: user1, - should_notify: false - }, Contact::Accepted { user_id: user2, should_notify: true @@ -2366,16 +2267,10 @@ pub mod tests { ); assert_eq!( db.get_contacts(user4).await.unwrap(), - [ - Contact::Accepted { - user_id: user1, - should_notify: false - }, - Contact::Accepted { - user_id: user4, - should_notify: false - }, - ] + [Contact::Accepted { + user_id: user1, + should_notify: false + }] ); // An existing user cannot redeem invite codes. @@ -2704,10 +2599,7 @@ pub mod tests { async fn get_contacts(&self, id: UserId) -> Result> { self.background.simulate_random_delay().await; - let mut contacts = vec![Contact::Accepted { - user_id: id, - should_notify: false, - }]; + let mut contacts = Vec::new(); for contact in self.contacts.lock().iter() { if contact.requester_id == id { diff --git a/crates/collab/src/integration_tests.rs b/crates/collab/src/integration_tests.rs index c235a31c557f8a07ba73dfb10a536d83ae99a46c..e04bd80c795bb0b4b90f2de98ebef7b9fd376d70 100644 --- a/crates/collab/src/integration_tests.rs +++ b/crates/collab/src/integration_tests.rs @@ -3909,78 +3909,122 @@ async fn test_contacts( .await; deterministic.run_until_parked(); - for (client, cx) in [(&client_a, &cx_a), (&client_b, &cx_b), (&client_c, &cx_c)] { - client.user_store.read_with(*cx, |store, _| { - assert_eq!( - contacts(store), - [ - ("user_a", true, vec![]), - ("user_b", true, vec![]), - ("user_c", true, vec![]) - ], - "{} has the wrong contacts", - client.username - ) - }); - } + assert_eq!( + contacts(&client_a, cx_a), + [ + ("user_b".to_string(), true, vec![]), + ("user_c".to_string(), true, vec![]) + ] + ); + assert_eq!( + contacts(&client_b, cx_b), + [ + ("user_a".to_string(), true, vec![]), + ("user_c".to_string(), true, vec![]) + ] + ); + assert_eq!( + contacts(&client_c, cx_c), + [ + ("user_a".to_string(), true, vec![]), + ("user_b".to_string(), true, vec![]) + ] + ); // Share a project as client A. client_a.fs.create_dir(Path::new("/a")).await.unwrap(); let (project_a, _) = client_a.build_local_project("/a", cx_a).await; deterministic.run_until_parked(); - for (client, cx) in [(&client_a, &cx_a), (&client_b, &cx_b), (&client_c, &cx_c)] { - client.user_store.read_with(*cx, |store, _| { - assert_eq!( - contacts(store), - [ - ("user_a", true, vec![("a", vec![])]), - ("user_b", true, vec![]), - ("user_c", true, vec![]) - ], - "{} has the wrong contacts", - client.username - ) - }); - } + assert_eq!( + contacts(&client_a, cx_a), + [ + ("user_b".to_string(), true, vec![]), + ("user_c".to_string(), true, vec![]) + ] + ); + assert_eq!( + contacts(&client_b, cx_b), + [ + ("user_a".to_string(), true, vec![("a".to_string(), vec![])]), + ("user_c".to_string(), true, vec![]) + ] + ); + assert_eq!( + contacts(&client_c, cx_c), + [ + ("user_a".to_string(), true, vec![("a".to_string(), vec![])]), + ("user_b".to_string(), true, vec![]) + ] + ); let _project_b = client_b.build_remote_project(&project_a, cx_a, cx_b).await; deterministic.run_until_parked(); - for (client, cx) in [(&client_a, &cx_a), (&client_b, &cx_b), (&client_c, &cx_c)] { - client.user_store.read_with(*cx, |store, _| { - assert_eq!( - contacts(store), - [ - ("user_a", true, vec![("a", vec!["user_b"])]), - ("user_b", true, vec![]), - ("user_c", true, vec![]) - ], - "{} has the wrong contacts", - client.username - ) - }); - } + assert_eq!( + contacts(&client_a, cx_a), + [ + ("user_b".to_string(), true, vec![]), + ("user_c".to_string(), true, vec![]) + ] + ); + assert_eq!( + contacts(&client_b, cx_b), + [ + ( + "user_a".to_string(), + true, + vec![("a".to_string(), vec!["user_b".to_string()])] + ), + ("user_c".to_string(), true, vec![]) + ] + ); + assert_eq!( + contacts(&client_c, cx_c), + [ + ( + "user_a".to_string(), + true, + vec![("a".to_string(), vec!["user_b".to_string()])] + ), + ("user_b".to_string(), true, vec![]) + ] + ); // Add a local project as client B client_a.fs.create_dir("/b".as_ref()).await.unwrap(); let (_project_b, _) = client_b.build_local_project("/b", cx_b).await; deterministic.run_until_parked(); - for (client, cx) in [(&client_a, &cx_a), (&client_b, &cx_b), (&client_c, &cx_c)] { - client.user_store.read_with(*cx, |store, _| { - assert_eq!( - contacts(store), - [ - ("user_a", true, vec![("a", vec!["user_b"])]), - ("user_b", true, vec![("b", vec![])]), - ("user_c", true, vec![]) - ], - "{} has the wrong contacts", - client.username - ) - }); - } + assert_eq!( + contacts(&client_a, cx_a), + [ + ("user_b".to_string(), true, vec![("b".to_string(), vec![])]), + ("user_c".to_string(), true, vec![]) + ] + ); + assert_eq!( + contacts(&client_b, cx_b), + [ + ( + "user_a".to_string(), + true, + vec![("a".to_string(), vec!["user_b".to_string()])] + ), + ("user_c".to_string(), true, vec![]) + ] + ); + assert_eq!( + contacts(&client_c, cx_c), + [ + ( + "user_a".to_string(), + true, + vec![("a".to_string(), vec!["user_b".to_string()])] + ), + ("user_b".to_string(), true, vec![("b".to_string(), vec![])]) + ] + ); project_a .condition(cx_a, |project, _| { @@ -3990,41 +4034,46 @@ async fn test_contacts( cx_a.update(move |_| drop(project_a)); deterministic.run_until_parked(); - for (client, cx) in [(&client_a, &cx_a), (&client_b, &cx_b), (&client_c, &cx_c)] { - client.user_store.read_with(*cx, |store, _| { - assert_eq!( - contacts(store), - [ - ("user_a", true, vec![]), - ("user_b", true, vec![("b", vec![])]), - ("user_c", true, vec![]) - ], - "{} has the wrong contacts", - client.username - ) - }); - } + assert_eq!( + contacts(&client_a, cx_a), + [ + ("user_b".to_string(), true, vec![("b".to_string(), vec![])]), + ("user_c".to_string(), true, vec![]) + ] + ); + assert_eq!( + contacts(&client_b, cx_b), + [ + ("user_a".to_string(), true, vec![]), + ("user_c".to_string(), true, vec![]) + ] + ); + assert_eq!( + contacts(&client_c, cx_c), + [ + ("user_a".to_string(), true, vec![]), + ("user_b".to_string(), true, vec![("b".to_string(), vec![])]) + ] + ); server.disconnect_client(client_c.current_user_id(cx_c)); server.forbid_connections(); deterministic.advance_clock(rpc::RECEIVE_TIMEOUT); - for (client, cx) in [(&client_a, &cx_a), (&client_b, &cx_b)] { - client.user_store.read_with(*cx, |store, _| { - assert_eq!( - contacts(store), - [ - ("user_a", true, vec![]), - ("user_b", true, vec![("b", vec![])]), - ("user_c", false, vec![]) - ], - "{} has the wrong contacts", - client.username - ) - }); - } - client_c - .user_store - .read_with(cx_c, |store, _| assert_eq!(contacts(store), [])); + assert_eq!( + contacts(&client_a, cx_a), + [ + ("user_b".to_string(), true, vec![("b".to_string(), vec![])]), + ("user_c".to_string(), false, vec![]) + ] + ); + assert_eq!( + contacts(&client_b, cx_b), + [ + ("user_a".to_string(), true, vec![]), + ("user_c".to_string(), false, vec![]) + ] + ); + assert_eq!(contacts(&client_c, cx_c), []); server.allow_connections(); client_c @@ -4033,40 +4082,52 @@ async fn test_contacts( .unwrap(); deterministic.run_until_parked(); - for (client, cx) in [(&client_a, &cx_a), (&client_b, &cx_b), (&client_c, &cx_c)] { - client.user_store.read_with(*cx, |store, _| { - assert_eq!( - contacts(store), - [ - ("user_a", true, vec![]), - ("user_b", true, vec![("b", vec![])]), - ("user_c", true, vec![]) - ], - "{} has the wrong contacts", - client.username - ) - }); - } + assert_eq!( + contacts(&client_a, cx_a), + [ + ("user_b".to_string(), true, vec![("b".to_string(), vec![])]), + ("user_c".to_string(), true, vec![]) + ] + ); + assert_eq!( + contacts(&client_b, cx_b), + [ + ("user_a".to_string(), true, vec![]), + ("user_c".to_string(), true, vec![]) + ] + ); + assert_eq!( + contacts(&client_c, cx_c), + [ + ("user_a".to_string(), true, vec![]), + ("user_b".to_string(), true, vec![("b".to_string(), vec![])]) + ] + ); #[allow(clippy::type_complexity)] - fn contacts(user_store: &UserStore) -> Vec<(&str, bool, Vec<(&str, Vec<&str>)>)> { - user_store - .contacts() - .iter() - .map(|contact| { - let projects = contact - .projects - .iter() - .map(|p| { - ( - p.visible_worktree_root_names[0].as_str(), - p.guests.iter().map(|p| p.github_login.as_str()).collect(), - ) - }) - .collect(); - (contact.user.github_login.as_str(), contact.online, projects) - }) - .collect() + fn contacts( + client: &TestClient, + cx: &TestAppContext, + ) -> Vec<(String, bool, Vec<(String, Vec)>)> { + client.user_store.read_with(cx, |store, _| { + store + .contacts() + .iter() + .map(|contact| { + let projects = contact + .projects + .iter() + .map(|p| { + ( + p.visible_worktree_root_names[0].clone(), + p.guests.iter().map(|p| p.github_login.clone()).collect(), + ) + }) + .collect(); + (contact.user.github_login.clone(), contact.online, projects) + }) + .collect() + }) } } @@ -4169,18 +4230,18 @@ async fn test_contact_requests( // User B sees user A as their contact now in all client, and the incoming request from them is removed. let contacts_b = client_b.summarize_contacts(cx_b); - assert_eq!(contacts_b.current, &["user_a", "user_b"]); + assert_eq!(contacts_b.current, &["user_a"]); assert_eq!(contacts_b.incoming_requests, &["user_c"]); let contacts_b2 = client_b2.summarize_contacts(cx_b2); - assert_eq!(contacts_b2.current, &["user_a", "user_b"]); + assert_eq!(contacts_b2.current, &["user_a"]); assert_eq!(contacts_b2.incoming_requests, &["user_c"]); // User A sees user B as their contact now in all clients, and the outgoing request to them is removed. let contacts_a = client_a.summarize_contacts(cx_a); - assert_eq!(contacts_a.current, &["user_a", "user_b"]); + assert_eq!(contacts_a.current, &["user_b"]); assert!(contacts_a.outgoing_requests.is_empty()); let contacts_a2 = client_a2.summarize_contacts(cx_a2); - assert_eq!(contacts_a2.current, &["user_a", "user_b"]); + assert_eq!(contacts_a2.current, &["user_b"]); assert!(contacts_a2.outgoing_requests.is_empty()); // Contacts are present upon connecting (tested here via disconnect/reconnect) @@ -4188,19 +4249,13 @@ async fn test_contact_requests( disconnect_and_reconnect(&client_b, cx_b).await; disconnect_and_reconnect(&client_c, cx_c).await; executor.run_until_parked(); - assert_eq!( - client_a.summarize_contacts(cx_a).current, - &["user_a", "user_b"] - ); - assert_eq!( - client_b.summarize_contacts(cx_b).current, - &["user_a", "user_b"] - ); + assert_eq!(client_a.summarize_contacts(cx_a).current, &["user_b"]); + assert_eq!(client_b.summarize_contacts(cx_b).current, &["user_a"]); assert_eq!( client_b.summarize_contacts(cx_b).incoming_requests, &["user_c"] ); - assert_eq!(client_c.summarize_contacts(cx_c).current, &["user_c"]); + assert!(client_c.summarize_contacts(cx_c).current.is_empty()); assert_eq!( client_c.summarize_contacts(cx_c).outgoing_requests, &["user_b"] @@ -4219,18 +4274,18 @@ async fn test_contact_requests( // User B doesn't see user C as their contact, and the incoming request from them is removed. let contacts_b = client_b.summarize_contacts(cx_b); - assert_eq!(contacts_b.current, &["user_a", "user_b"]); + assert_eq!(contacts_b.current, &["user_a"]); assert!(contacts_b.incoming_requests.is_empty()); let contacts_b2 = client_b2.summarize_contacts(cx_b2); - assert_eq!(contacts_b2.current, &["user_a", "user_b"]); + assert_eq!(contacts_b2.current, &["user_a"]); assert!(contacts_b2.incoming_requests.is_empty()); // User C doesn't see user B as their contact, and the outgoing request to them is removed. let contacts_c = client_c.summarize_contacts(cx_c); - assert_eq!(contacts_c.current, &["user_c"]); + assert!(contacts_c.current.is_empty()); assert!(contacts_c.outgoing_requests.is_empty()); let contacts_c2 = client_c2.summarize_contacts(cx_c2); - assert_eq!(contacts_c2.current, &["user_c"]); + assert!(contacts_c2.current.is_empty()); assert!(contacts_c2.outgoing_requests.is_empty()); // Incoming/outgoing requests are not present upon connecting (tested here via disconnect/reconnect) @@ -4238,19 +4293,13 @@ async fn test_contact_requests( disconnect_and_reconnect(&client_b, cx_b).await; disconnect_and_reconnect(&client_c, cx_c).await; executor.run_until_parked(); - assert_eq!( - client_a.summarize_contacts(cx_a).current, - &["user_a", "user_b"] - ); - assert_eq!( - client_b.summarize_contacts(cx_b).current, - &["user_a", "user_b"] - ); + assert_eq!(client_a.summarize_contacts(cx_a).current, &["user_b"]); + assert_eq!(client_b.summarize_contacts(cx_b).current, &["user_a"]); assert!(client_b .summarize_contacts(cx_b) .incoming_requests .is_empty()); - assert_eq!(client_c.summarize_contacts(cx_c).current, &["user_c"]); + assert!(client_c.summarize_contacts(cx_c).current.is_empty()); assert!(client_c .summarize_contacts(cx_c) .outgoing_requests @@ -5655,6 +5704,9 @@ impl TestClient { worktree .read_with(cx, |tree, _| tree.as_local().unwrap().scan_complete()) .await; + project + .update(cx, |project, _| project.next_remote_id()) + .await; (project, worktree.read_with(cx, |tree, _| tree.id())) } diff --git a/crates/collab_titlebar_item/src/add_participant_popover.rs b/crates/collab_titlebar_item/src/add_participant_popover.rs index 95d37849cbb6d0101887ad57b371e19614f19baf..b6fa8125f7566d5889c4f8651879d8b0891a1194 100644 --- a/crates/collab_titlebar_item/src/add_participant_popover.rs +++ b/crates/collab_titlebar_item/src/add_participant_popover.rs @@ -298,36 +298,21 @@ impl AddParticipantPopover { } } - let current_user = user_store.current_user(); - let contacts = user_store.contacts(); if !contacts.is_empty() { // Always put the current user first. self.match_candidates.clear(); - self.match_candidates.reserve(contacts.len()); - self.match_candidates.push(StringMatchCandidate { - id: 0, - string: Default::default(), - char_bag: Default::default(), - }); - for (ix, contact) in contacts.iter().enumerate() { - let candidate = StringMatchCandidate { - id: ix, - string: contact.user.github_login.clone(), - char_bag: contact.user.github_login.chars().collect(), - }; - if current_user - .as_ref() - .map_or(false, |current_user| current_user.id == contact.user.id) - { - self.match_candidates[0] = candidate; - } else { - self.match_candidates.push(candidate); - } - } - if self.match_candidates[0].string.is_empty() { - self.match_candidates.remove(0); - } + self.match_candidates + .extend( + contacts + .iter() + .enumerate() + .map(|(ix, contact)| StringMatchCandidate { + id: ix, + string: contact.user.github_login.clone(), + char_bag: contact.user.github_login.chars().collect(), + }), + ); let matches = executor.block(match_strings( &self.match_candidates, diff --git a/crates/collab_titlebar_item/src/collab_titlebar_item.rs b/crates/collab_titlebar_item/src/collab_titlebar_item.rs index b080242a8fe6348a5f10bc78cf516d74bc8b1f51..e0ad25325190ed00c19393c371a26a4fe07b7385 100644 --- a/crates/collab_titlebar_item/src/collab_titlebar_item.rs +++ b/crates/collab_titlebar_item/src/collab_titlebar_item.rs @@ -81,7 +81,6 @@ impl CollabTitlebarItem { cx.subscribe(&view, |this, _, event, cx| { match event { add_participant_popover::Event::Dismissed => { - dbg!("dismissed"); this.add_participant_popover = None; } }