From 694840cdd6378cffb0eef83f0ef3658425445a46 Mon Sep 17 00:00:00 2001 From: Joseph Lyons Date: Tue, 6 Dec 2022 17:12:12 -0500 Subject: [PATCH 1/4] Allow overwriting signup data if a user signs up more than once with the same email address --- Cargo.lock | 34 +++++++++++++++ crates/collab/Cargo.toml | 1 + crates/collab/src/db.rs | 25 +++++++++++- crates/collab/src/db/signup.rs | 2 +- crates/collab/src/db/tests.rs | 75 ++++++++++++++++++++++++++++++++++ 5 files changed, 135 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9a4d3a78d1..21966a9673 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1160,6 +1160,7 @@ dependencies = [ "lsp", "nanoid", "parking_lot 0.11.2", + "pretty_assertions", "project", "prometheus", "rand 0.8.5", @@ -1730,6 +1731,12 @@ dependencies = [ "workspace", ] +[[package]] +name = "diff" +version = "0.1.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "56254986775e3233ffa9c4d7d3faaf6d36a2c09d30b20687e9f88bc8bafc16c8" + [[package]] name = "digest" version = "0.9.0" @@ -4005,6 +4012,15 @@ dependencies = [ "workspace", ] +[[package]] +name = "output_vt100" +version = "0.1.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "628223faebab4e3e40667ee0b2336d34a5b960ff60ea743ddfdbcf7770bcfb66" +dependencies = [ + "winapi 0.3.9", +] + [[package]] name = "overload" version = "0.1.1" @@ -4346,6 +4362,18 @@ version = "0.2.17" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5b40af805b3121feab8a3c29f04d8ad262fa8e0561883e7653e024ae4479e6de" +[[package]] +name = "pretty_assertions" +version = "1.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a25e9bcb20aa780fd0bb16b72403a9064d6b3f22f026946029acb941a50af755" +dependencies = [ + "ctor", + "diff", + "output_vt100", + "yansi", +] + [[package]] name = "proc-macro-crate" version = "0.1.5" @@ -8065,6 +8093,12 @@ dependencies = [ "linked-hash-map", ] +[[package]] +name = "yansi" +version = "0.5.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "09041cd90cf85f7f8b2df60c646f853b7f535ce68f85244eb6731cf89fa498ec" + [[package]] name = "zed" version = "0.67.0" diff --git a/crates/collab/Cargo.toml b/crates/collab/Cargo.toml index 8725642ae5..c741341d48 100644 --- a/crates/collab/Cargo.toml +++ b/crates/collab/Cargo.toml @@ -64,6 +64,7 @@ fs = { path = "../fs", features = ["test-support"] } git = { path = "../git", features = ["test-support"] } live_kit_client = { path = "../live_kit_client", features = ["test-support"] } lsp = { path = "../lsp", features = ["test-support"] } +pretty_assertions = "1.3.0" project = { path = "../project", features = ["test-support"] } rpc = { path = "../rpc", features = ["test-support"] } settings = { path = "../settings", features = ["test-support"] } diff --git a/crates/collab/src/db.rs b/crates/collab/src/db.rs index 1cda33c00c..d90c138886 100644 --- a/crates/collab/src/db.rs +++ b/crates/collab/src/db.rs @@ -669,7 +669,15 @@ impl Database { }) .on_conflict( OnConflict::column(signup::Column::EmailAddress) - .update_column(signup::Column::EmailAddress) + .update_columns([ + signup::Column::PlatformMac, + signup::Column::PlatformWindows, + signup::Column::PlatformLinux, + signup::Column::EditorFeatures, + signup::Column::ProgrammingLanguages, + signup::Column::DeviceId, + signup::Column::AddedToMailingList, + ]) .to_owned(), ) .exec(&*tx) @@ -679,6 +687,21 @@ impl Database { .await } + pub async fn get_signup(&self, email_address: &str) -> Result { + self.transaction(|tx| async move { + let signup = signup::Entity::find() + .filter(signup::Column::EmailAddress.eq(email_address)) + .one(&*tx) + .await? + .ok_or_else(|| { + anyhow!("signup with email address {} doesn't exist", email_address) + })?; + + Ok(signup) + }) + .await + } + pub async fn get_waitlist_summary(&self) -> Result { self.transaction(|tx| async move { let query = " diff --git a/crates/collab/src/db/signup.rs b/crates/collab/src/db/signup.rs index ca219736a8..5d5a9a1b61 100644 --- a/crates/collab/src/db/signup.rs +++ b/crates/collab/src/db/signup.rs @@ -34,7 +34,7 @@ pub struct Invite { pub email_confirmation_code: String, } -#[derive(Clone, Deserialize)] +#[derive(Clone, Debug, Deserialize)] pub struct NewSignup { pub email_address: String, pub platform_mac: bool, diff --git a/crates/collab/src/db/tests.rs b/crates/collab/src/db/tests.rs index baa3f87060..298176adf2 100644 --- a/crates/collab/src/db/tests.rs +++ b/crates/collab/src/db/tests.rs @@ -2,6 +2,9 @@ use super::*; use gpui::executor::{Background, Deterministic}; use std::sync::Arc; +#[cfg(test)] +use pretty_assertions::{assert_eq, assert_ne}; + macro_rules! test_both_dbs { ($postgres_test_name:ident, $sqlite_test_name:ident, $db:ident, $body:block) => { #[gpui::test] @@ -727,6 +730,78 @@ async fn test_invite_codes() { assert_eq!(invite_count, 1); } +#[gpui::test] +async fn test_multiple_signup_overwrite() { + let test_db = TestDb::postgres(build_background_executor()); + let db = test_db.db(); + + let email_address = "user_1@example.com".to_string(); + + let signup = NewSignup { + email_address: email_address.clone(), + platform_mac: false, + platform_linux: true, + platform_windows: false, + editor_features: vec!["speed".into()], + programming_languages: vec!["rust".into(), "c".into()], + device_id: Some(format!("device_id")), + added_to_mailing_list: false, + }; + + db.create_signup(&signup).await.unwrap(); + + // TODO: Remove this method and just have create_signup return an instance? + let signup_from_db = db.get_signup(&signup.email_address).await.unwrap(); + + assert_eq!( + signup_from_db.clone(), + signup::Model { + email_address: signup.email_address, + platform_mac: signup.platform_mac, + platform_linux: signup.platform_linux, + platform_windows: signup.platform_windows, + editor_features: Some(signup.editor_features), + programming_languages: Some(signup.programming_languages), + added_to_mailing_list: signup.added_to_mailing_list, + ..signup_from_db + } + ); + + let signup_overwrite = NewSignup { + email_address, + platform_mac: true, + platform_linux: false, + platform_windows: true, + editor_features: vec!["git integration".into(), "clean design".into()], + programming_languages: vec!["d".into(), "elm".into()], + device_id: Some(format!("different_device_id")), + added_to_mailing_list: true, + }; + + db.create_signup(&signup_overwrite).await.unwrap(); + + let signup_overwrite_from_db = db + .get_signup(&signup_overwrite.email_address) + .await + .unwrap(); + + assert_eq!( + signup_overwrite_from_db.clone(), + signup::Model { + platform_mac: signup_overwrite.platform_mac, + platform_linux: signup_overwrite.platform_linux, + platform_windows: signup_overwrite.platform_windows, + editor_features: Some(signup_overwrite.editor_features), + programming_languages: Some(signup_overwrite.programming_languages), + device_id: signup_overwrite.device_id, + added_to_mailing_list: signup_overwrite.added_to_mailing_list, + // shouldn't overwrite their creation Datetime - user shouldn't lose their spot in line + created_at: signup_from_db.created_at, + ..signup_overwrite_from_db + } + ); +} + #[gpui::test] async fn test_signups() { let test_db = TestDb::postgres(build_background_executor()); From 97989b04a0de265f8dd31f8c53238583415479c0 Mon Sep 17 00:00:00 2001 From: Joseph Lyons Date: Tue, 6 Dec 2022 17:18:54 -0500 Subject: [PATCH 2/4] Remove comment --- crates/collab/src/db/tests.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/collab/src/db/tests.rs b/crates/collab/src/db/tests.rs index 298176adf2..46919d1467 100644 --- a/crates/collab/src/db/tests.rs +++ b/crates/collab/src/db/tests.rs @@ -750,7 +750,6 @@ async fn test_multiple_signup_overwrite() { db.create_signup(&signup).await.unwrap(); - // TODO: Remove this method and just have create_signup return an instance? let signup_from_db = db.get_signup(&signup.email_address).await.unwrap(); assert_eq!( From 5f319071270643f040ad4c7e329151e06dca8c59 Mon Sep 17 00:00:00 2001 From: Joseph Lyons Date: Wed, 7 Dec 2022 07:12:27 -0500 Subject: [PATCH 3/4] Clean up test --- crates/collab/src/db/tests.rs | 55 +++++++++++++++++------------------ 1 file changed, 26 insertions(+), 29 deletions(-) diff --git a/crates/collab/src/db/tests.rs b/crates/collab/src/db/tests.rs index 46919d1467..84cf422976 100644 --- a/crates/collab/src/db/tests.rs +++ b/crates/collab/src/db/tests.rs @@ -737,7 +737,7 @@ async fn test_multiple_signup_overwrite() { let email_address = "user_1@example.com".to_string(); - let signup = NewSignup { + let initial_signup = NewSignup { email_address: email_address.clone(), platform_mac: false, platform_linux: true, @@ -748,26 +748,26 @@ async fn test_multiple_signup_overwrite() { added_to_mailing_list: false, }; - db.create_signup(&signup).await.unwrap(); + db.create_signup(&initial_signup).await.unwrap(); - let signup_from_db = db.get_signup(&signup.email_address).await.unwrap(); + let initial_signup_from_db = db.get_signup(&email_address).await.unwrap(); assert_eq!( - signup_from_db.clone(), + initial_signup_from_db.clone(), signup::Model { - email_address: signup.email_address, - platform_mac: signup.platform_mac, - platform_linux: signup.platform_linux, - platform_windows: signup.platform_windows, - editor_features: Some(signup.editor_features), - programming_languages: Some(signup.programming_languages), - added_to_mailing_list: signup.added_to_mailing_list, - ..signup_from_db + email_address: initial_signup.email_address, + platform_mac: initial_signup.platform_mac, + platform_linux: initial_signup.platform_linux, + platform_windows: initial_signup.platform_windows, + editor_features: Some(initial_signup.editor_features), + programming_languages: Some(initial_signup.programming_languages), + added_to_mailing_list: initial_signup.added_to_mailing_list, + ..initial_signup_from_db } ); - let signup_overwrite = NewSignup { - email_address, + let subsequent_signup = NewSignup { + email_address: email_address.clone(), platform_mac: true, platform_linux: false, platform_windows: true, @@ -777,26 +777,23 @@ async fn test_multiple_signup_overwrite() { added_to_mailing_list: true, }; - db.create_signup(&signup_overwrite).await.unwrap(); + db.create_signup(&subsequent_signup).await.unwrap(); - let signup_overwrite_from_db = db - .get_signup(&signup_overwrite.email_address) - .await - .unwrap(); + let subsequent_signup_from_db = db.get_signup(&email_address).await.unwrap(); assert_eq!( - signup_overwrite_from_db.clone(), + subsequent_signup_from_db.clone(), signup::Model { - platform_mac: signup_overwrite.platform_mac, - platform_linux: signup_overwrite.platform_linux, - platform_windows: signup_overwrite.platform_windows, - editor_features: Some(signup_overwrite.editor_features), - programming_languages: Some(signup_overwrite.programming_languages), - device_id: signup_overwrite.device_id, - added_to_mailing_list: signup_overwrite.added_to_mailing_list, + platform_mac: subsequent_signup.platform_mac, + platform_linux: subsequent_signup.platform_linux, + platform_windows: subsequent_signup.platform_windows, + editor_features: Some(subsequent_signup.editor_features), + programming_languages: Some(subsequent_signup.programming_languages), + device_id: subsequent_signup.device_id, + added_to_mailing_list: subsequent_signup.added_to_mailing_list, // shouldn't overwrite their creation Datetime - user shouldn't lose their spot in line - created_at: signup_from_db.created_at, - ..signup_overwrite_from_db + created_at: initial_signup_from_db.created_at, + ..subsequent_signup_from_db } ); } From d71d543337abb108bd3ea4b1f5b24729d78ac5e7 Mon Sep 17 00:00:00 2001 From: Joseph Lyons Date: Wed, 7 Dec 2022 08:15:01 -0500 Subject: [PATCH 4/4] Ensure that subsequent signup happens after initial We can't rely on the fact that the test won't run fast enough such that both `created_at`s are the same time. This ensures the subsequent signup happens after the initial one and that the database doesn't overwrite the initial one. --- crates/collab/src/db/signup.rs | 1 + crates/collab/src/db/tests.rs | 13 +++++++++++++ 2 files changed, 14 insertions(+) diff --git a/crates/collab/src/db/signup.rs b/crates/collab/src/db/signup.rs index 5d5a9a1b61..6368482de9 100644 --- a/crates/collab/src/db/signup.rs +++ b/crates/collab/src/db/signup.rs @@ -44,6 +44,7 @@ pub struct NewSignup { pub programming_languages: Vec, pub device_id: Option, pub added_to_mailing_list: bool, + pub created_at: Option, } #[derive(Clone, Debug, PartialEq, Deserialize, Serialize, FromQueryResult)] diff --git a/crates/collab/src/db/tests.rs b/crates/collab/src/db/tests.rs index 84cf422976..e3819116a0 100644 --- a/crates/collab/src/db/tests.rs +++ b/crates/collab/src/db/tests.rs @@ -737,6 +737,8 @@ async fn test_multiple_signup_overwrite() { let email_address = "user_1@example.com".to_string(); + let initial_signup_created_at_milliseconds = 0; + let initial_signup = NewSignup { email_address: email_address.clone(), platform_mac: false, @@ -746,6 +748,9 @@ async fn test_multiple_signup_overwrite() { programming_languages: vec!["rust".into(), "c".into()], device_id: Some(format!("device_id")), added_to_mailing_list: false, + created_at: Some( + DateTime::from_timestamp_millis(initial_signup_created_at_milliseconds).unwrap(), + ), }; db.create_signup(&initial_signup).await.unwrap(); @@ -775,6 +780,13 @@ async fn test_multiple_signup_overwrite() { programming_languages: vec!["d".into(), "elm".into()], device_id: Some(format!("different_device_id")), added_to_mailing_list: true, + // subsequent signup happens next day + created_at: Some( + DateTime::from_timestamp_millis( + initial_signup_created_at_milliseconds + (1000 * 60 * 60 * 24), + ) + .unwrap(), + ), }; db.create_signup(&subsequent_signup).await.unwrap(); @@ -817,6 +829,7 @@ async fn test_signups() { programming_languages: vec!["rust".into(), "c".into()], device_id: Some(format!("device_id_{i}")), added_to_mailing_list: i != 0, // One user failed to subscribe + created_at: Some(DateTime::from_timestamp_millis(i as i64).unwrap()), // Signups are consecutive }) .collect::>();