diff --git a/migrations/2019-11-15-182353_Add email notifications to crate owners/down.sql b/migrations/2019-11-15-182353_Add email notifications to crate owners/down.sql new file mode 100644 index 00000000000..99a22c0cd02 --- /dev/null +++ b/migrations/2019-11-15-182353_Add email notifications to crate owners/down.sql @@ -0,0 +1 @@ +ALTER TABLE crate_owners DROP COLUMN email_notifications; diff --git a/migrations/2019-11-15-182353_Add email notifications to crate owners/up.sql b/migrations/2019-11-15-182353_Add email notifications to crate owners/up.sql new file mode 100644 index 00000000000..0dfd3c6ce56 --- /dev/null +++ b/migrations/2019-11-15-182353_Add email notifications to crate owners/up.sql @@ -0,0 +1 @@ +ALTER TABLE crate_owners ADD COLUMN email_notifications BOOLEAN NOT NULL DEFAULT TRUE; diff --git a/src/controllers/crate_owner_invitation.rs b/src/controllers/crate_owner_invitation.rs index 89cf5c35cdc..600659e7d52 100644 --- a/src/controllers/crate_owner_invitation.rs +++ b/src/controllers/crate_owner_invitation.rs @@ -69,6 +69,7 @@ fn accept_invite( owner_id: user_id, created_by: pending_crate_owner.invited_by_user_id, owner_kind: OwnerKind::User as i32, + email_notifications: true, }) .on_conflict(crate_owners::table.primary_key()) .do_update() diff --git a/src/controllers/krate/search.rs b/src/controllers/krate/search.rs index d2c100e451b..21484981727 100644 --- a/src/controllers/krate/search.rs +++ b/src/controllers/krate/search.rs @@ -5,7 +5,7 @@ use diesel_full_text_search::*; use crate::controllers::helpers::Paginate; use crate::controllers::prelude::*; -use crate::models::{Crate, CrateBadge, CrateVersions, OwnerKind, Version}; +use crate::models::{Crate, CrateBadge, CrateOwner, CrateVersions, OwnerKind, Version}; use crate::schema::*; use crate::views::EncodableCrate; @@ -118,21 +118,17 @@ pub fn search(req: &mut dyn Request) -> CargoResult { } else if let Some(user_id) = params.get("user_id").and_then(|s| s.parse::().ok()) { query = query.filter( crates::id.eq_any( - crate_owners::table + CrateOwner::by_owner_kind(OwnerKind::User) .select(crate_owners::crate_id) - .filter(crate_owners::owner_id.eq(user_id)) - .filter(crate_owners::deleted.eq(false)) - .filter(crate_owners::owner_kind.eq(OwnerKind::User as i32)), + .filter(crate_owners::owner_id.eq(user_id)), ), ); } else if let Some(team_id) = params.get("team_id").and_then(|s| s.parse::().ok()) { query = query.filter( crates::id.eq_any( - crate_owners::table + CrateOwner::by_owner_kind(OwnerKind::Team) .select(crate_owners::crate_id) - .filter(crate_owners::owner_id.eq(team_id)) - .filter(crate_owners::deleted.eq(false)) - .filter(crate_owners::owner_kind.eq(OwnerKind::Team as i32)), + .filter(crate_owners::owner_id.eq(team_id)), ), ); } else if params.get("following").is_some() { diff --git a/src/controllers/user/me.rs b/src/controllers/user/me.rs index 295e3259a3f..d7b36c2d60f 100644 --- a/src/controllers/user/me.rs +++ b/src/controllers/user/me.rs @@ -1,3 +1,5 @@ +use std::collections::HashMap; + use crate::controllers::prelude::*; use crate::controllers::helpers::*; @@ -5,9 +7,9 @@ use crate::email; use crate::util::bad_request; use crate::util::errors::CargoError; -use crate::models::{Email, Follow, NewEmail, User, Version}; -use crate::schema::{crates, emails, follows, users, versions}; -use crate::views::{EncodableMe, EncodableVersion}; +use crate::models::{CrateOwner, Email, Follow, NewEmail, OwnerKind, User, Version}; +use crate::schema::{crate_owners, crates, emails, follows, users, versions}; +use crate::views::{EncodableMe, EncodableVersion, OwnedCrate}; /// Handles the `GET /me` route. pub fn me(req: &mut dyn Request) -> CargoResult { @@ -22,11 +24,11 @@ pub fn me(req: &mut dyn Request) -> CargoResult { // perhaps adding `req.mut_extensions().insert(user)` to the // update_user route, however this somehow does not seem to work - let id = req.user()?.id; + let user_id = req.user()?.id; let conn = req.db_conn()?; let (user, verified, email, verification_sent) = users::table - .find(id) + .find(user_id) .left_join(emails::table) .select(( users::all_columns, @@ -36,12 +38,28 @@ pub fn me(req: &mut dyn Request) -> CargoResult { )) .first::<(User, Option, Option, bool)>(&*conn)?; + let owned_crates = crate_owners::table + .inner_join(crates::table) + .filter(crate_owners::owner_id.eq(user_id)) + .filter(crate_owners::owner_kind.eq(OwnerKind::User as i32)) + .select((crates::id, crates::name, crate_owners::email_notifications)) + .order(crates::name.asc()) + .load(&*conn)? + .into_iter() + .map(|(id, name, email_notifications)| OwnedCrate { + id, + name, + email_notifications, + }) + .collect(); + let verified = verified.unwrap_or(false); let verification_sent = verified || verification_sent; let user = User { email, ..user }; Ok(req.json(&EncodableMe { user: user.encodable_private(verified, verification_sent), + owned_crates, })) } @@ -212,3 +230,60 @@ pub fn regenerate_token_and_send(req: &mut dyn Request) -> CargoResult } Ok(req.json(&R { ok: true })) } + +/// Handles `PUT /me/email_notifications` route +pub fn update_email_notifications(req: &mut dyn Request) -> CargoResult { + use self::crate_owners::dsl::*; + use diesel::pg::upsert::excluded; + + #[derive(Deserialize)] + struct CrateEmailNotifications { + id: i32, + email_notifications: bool, + } + + let mut body = String::new(); + req.body().read_to_string(&mut body)?; + let updates: HashMap = serde_json::from_str::>(&body) + .map_err(|_| bad_request("invalid json request"))? + .iter() + .map(|c| (c.id, c.email_notifications)) + .collect(); + + let user = req.user()?; + let conn = req.db_conn()?; + + // Build inserts from existing crates beloning to the current user + let to_insert = CrateOwner::by_owner_kind(OwnerKind::User) + .filter(owner_id.eq(user.id)) + .select((crate_id, owner_id, owner_kind, email_notifications)) + .load(&*conn)? + .into_iter() + // Remove records whose `email_notifications` will not change from their current value + .map( + |(c_id, o_id, o_kind, e_notifications): (i32, i32, i32, bool)| { + let current_e_notifications = *updates.get(&c_id).unwrap_or(&e_notifications); + ( + crate_id.eq(c_id), + owner_id.eq(o_id), + owner_kind.eq(o_kind), + email_notifications.eq(current_e_notifications), + ) + }, + ) + .collect::>(); + + // Upsert crate owners; this should only actually exectute updates + diesel::insert_into(crate_owners) + .values(&to_insert) + .on_conflict((crate_id, owner_id, owner_kind)) + .do_update() + .set(email_notifications.eq(excluded(email_notifications))) + .execute(&*conn)?; + + #[derive(Serialize)] + struct R { + ok: bool, + } + Ok(req.json(&R { ok: true })) +} diff --git a/src/models/krate.rs b/src/models/krate.rs index 2e3885212e2..804cef42d03 100644 --- a/src/models/krate.rs +++ b/src/models/krate.rs @@ -195,6 +195,7 @@ impl<'a> NewCrate<'a> { owner_id: user_id, created_by: user_id, owner_kind: OwnerKind::User as i32, + email_notifications: true, }; diesel::insert_into(crate_owners::table) .values(&owner) @@ -400,18 +401,17 @@ impl Crate { } pub fn owners(&self, conn: &PgConnection) -> CargoResult> { - let base_query = CrateOwner::belonging_to(self).filter(crate_owners::deleted.eq(false)); - let users = base_query + let users = CrateOwner::by_owner_kind(OwnerKind::User) + .filter(crate_owners::crate_id.eq(self.id)) .inner_join(users::table) .select(users::all_columns) - .filter(crate_owners::owner_kind.eq(OwnerKind::User as i32)) .load(conn)? .into_iter() .map(Owner::User); - let teams = base_query + let teams = CrateOwner::by_owner_kind(OwnerKind::Team) + .filter(crate_owners::crate_id.eq(self.id)) .inner_join(teams::table) .select(teams::all_columns) - .filter(crate_owners::owner_kind.eq(OwnerKind::Team as i32)) .load(conn)? .into_iter() .map(Owner::Team); @@ -455,6 +455,7 @@ impl Crate { owner_id: owner.id(), created_by: req_user.id, owner_kind: OwnerKind::Team as i32, + email_notifications: true, }) .on_conflict(crate_owners::table.primary_key()) .do_update() diff --git a/src/models/owner.rs b/src/models/owner.rs index 46611336c2a..a1f83586a28 100644 --- a/src/models/owner.rs +++ b/src/models/owner.rs @@ -1,3 +1,4 @@ +use diesel::pg::Pg; use diesel::prelude::*; use crate::app::App; @@ -19,6 +20,22 @@ pub struct CrateOwner { pub owner_id: i32, pub created_by: i32, pub owner_kind: i32, + pub email_notifications: bool, +} + +type BoxedQuery<'a> = crate_owners::BoxedQuery<'a, Pg, crate_owners::SqlType>; + +impl CrateOwner { + /// Returns a base crate owner query filtered by the owner kind argument. This query also + /// filters out deleted records. + pub fn by_owner_kind(kind: OwnerKind) -> BoxedQuery<'static> { + use self::crate_owners::dsl::*; + + crate_owners + .filter(deleted.eq(false)) + .filter(owner_kind.eq(kind as i32)) + .into_boxed() + } } #[derive(Debug, Clone, Copy)] diff --git a/src/models/user.rs b/src/models/user.rs index 2f22c20a19e..8fb9c59cd57 100644 --- a/src/models/user.rs +++ b/src/models/user.rs @@ -129,11 +129,10 @@ impl User { } pub fn owning(krate: &Crate, conn: &PgConnection) -> CargoResult> { - let base_query = CrateOwner::belonging_to(krate).filter(crate_owners::deleted.eq(false)); - let users = base_query + let users = CrateOwner::by_owner_kind(OwnerKind::User) .inner_join(users::table) .select(users::all_columns) - .filter(crate_owners::owner_kind.eq(OwnerKind::User as i32)) + .filter(crate_owners::crate_id.eq(krate.id)) .load(conn)? .into_iter() .map(Owner::User); diff --git a/src/router.rs b/src/router.rs index 6c653f4825f..d324228889c 100644 --- a/src/router.rs +++ b/src/router.rs @@ -89,6 +89,10 @@ pub fn build_router(app: &App) -> R404 { "/me/crate_owner_invitations/:crate_id", C(crate_owner_invitation::handle_invite), ); + api_router.put( + "/me/email_notifications", + C(user::me::update_email_notifications), + ); api_router.get("/summary", C(krate::metadata::summary)); api_router.put("/confirm/:email_token", C(user::me::confirm_user_email)); api_router.put( diff --git a/src/schema.rs b/src/schema.rs index 6b692eeada9..26651270b57 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -261,6 +261,12 @@ table! { /// /// (Automatically generated by Diesel.) owner_kind -> Int4, + /// The `email_notifications` column of the `crate_owners` table. + /// + /// Its SQL type is `Bool`. + /// + /// (Automatically generated by Diesel.) + email_notifications -> Bool, } } diff --git a/src/tasks/dump_db/dump-db.toml b/src/tasks/dump_db/dump-db.toml index 12302c891ba..37042483dc2 100644 --- a/src/tasks/dump_db/dump-db.toml +++ b/src/tasks/dump_db/dump-db.toml @@ -66,8 +66,9 @@ owner_id = "public" created_at = "public" created_by = "private" deleted = "private" -updated_at = "public" +updated_at = "private" owner_kind = "public" +email_notifications = "private" [crates.columns] id = "public" diff --git a/src/tests/all.rs b/src/tests/all.rs index 7be4db4f9b7..b3c0b067208 100644 --- a/src/tests/all.rs +++ b/src/tests/all.rs @@ -242,6 +242,7 @@ fn add_team_to_crate(t: &Team, krate: &Crate, u: &User, conn: &PgConnection) -> owner_id: t.id, created_by: u.id, owner_kind: 1, // Team owner kind is 1 according to owner.rs + email_notifications: true, }; diesel::insert_into(crate_owners::table) diff --git a/src/tests/user.rs b/src/tests/user.rs index db90c51e106..5f22111d08d 100644 --- a/src/tests/user.rs +++ b/src/tests/user.rs @@ -6,7 +6,8 @@ use crate::{ }; use cargo_registry::{ models::{Email, NewUser, User}, - views::{EncodablePrivateUser, EncodablePublicUser, EncodableVersion}, + schema::crate_owners, + views::{EncodablePrivateUser, EncodablePublicUser, EncodableVersion, OwnedCrate}, }; use diesel::prelude::*; @@ -25,6 +26,7 @@ pub struct UserShowPublicResponse { #[derive(Deserialize)] pub struct UserShowPrivateResponse { pub user: EncodablePrivateUser, + pub owned_crates: Vec, } #[derive(Deserialize)] @@ -32,6 +34,12 @@ struct UserStats { total_downloads: i64, } +#[derive(Serialize)] +struct EmailNotificationsUpdate { + id: i32, + email_notifications: bool, +} + impl crate::util::MockCookieUser { fn show_me(&self) -> UserShowPrivateResponse { let url = "/api/v1/me"; @@ -65,6 +73,14 @@ impl crate::util::MockCookieUser { let url = format!("/api/v1/confirm/{}", email_token); self.put(&url, &[]).good() } + + fn update_email_notifications(&self, updates: Vec) -> OkBool { + self.put( + "/api/v1/me/email_notifications", + json!(updates).to_string().as_bytes(), + ) + .good() + } } impl crate::util::MockAnonymousUser { @@ -108,9 +124,14 @@ fn me() { anon.get(url).assert_forbidden(); let user = app.db_new_user("foo"); + app.db(|conn| { + CrateBuilder::new("foo_my_packages", user.as_model().id).expect_build(conn); + }); + let json = user.show_me(); assert_eq!(json.user.email, user.as_model().email); + assert_eq!(json.owned_crates.len(), 1); } #[test] @@ -569,3 +590,121 @@ fn test_existing_user_email() { assert!(!json.user.email_verified); assert!(!json.user.email_verification_sent); } + +/* A user should be able to update the email notifications for crates they own. Only the crates that + were sent in the request should be updated to the corresponding `email_notifications` value. +*/ +#[test] +fn test_update_email_notifications() { + let (app, _, user) = TestApp::init().with_user(); + + let my_crates = app.db(|conn| { + vec![ + CrateBuilder::new("test_package", user.as_model().id).expect_build(&conn), + CrateBuilder::new("another_package", user.as_model().id).expect_build(&conn), + ] + }); + + let a_id = my_crates.get(0).unwrap().id; + let b_id = my_crates.get(1).unwrap().id; + + // Update crate_a: email_notifications = false + // crate_a should be false, crate_b should be true + user.update_email_notifications(vec![EmailNotificationsUpdate { + id: a_id, + email_notifications: false, + }]); + let json = user.show_me(); + + assert_eq!( + json.owned_crates + .iter() + .find(|c| c.id == a_id) + .unwrap() + .email_notifications, + false + ); + assert_eq!( + json.owned_crates + .iter() + .find(|c| c.id == b_id) + .unwrap() + .email_notifications, + true + ); + + // Update crate_b: email_notifications = false + // Both should be false now + user.update_email_notifications(vec![EmailNotificationsUpdate { + id: b_id, + email_notifications: false, + }]); + let json = user.show_me(); + + assert_eq!( + json.owned_crates + .iter() + .find(|c| c.id == a_id) + .unwrap() + .email_notifications, + false + ); + assert_eq!( + json.owned_crates + .iter() + .find(|c| c.id == b_id) + .unwrap() + .email_notifications, + false + ); + + // Update crate_a and crate_b: email_notifications = true + // Both should be true + user.update_email_notifications(vec![ + EmailNotificationsUpdate { + id: a_id, + email_notifications: true, + }, + EmailNotificationsUpdate { + id: b_id, + email_notifications: true, + }, + ]); + let json = user.show_me(); + + json.owned_crates.iter().for_each(|c| { + assert!(c.email_notifications); + }) +} + +/* A user should not be able to update the `email_notifications` value for a crate that is not + owned by them. +*/ +#[test] +fn test_update_email_notifications_not_owned() { + let (app, _, user) = TestApp::init().with_user(); + + let not_my_crate = app.db(|conn| { + let u = new_user("arbitrary_username") + .create_or_update(&conn) + .unwrap(); + CrateBuilder::new("test_package", u.id).expect_build(&conn) + }); + + user.update_email_notifications(vec![EmailNotificationsUpdate { + id: not_my_crate.id, + email_notifications: false, + }]); + + let email_notifications = app + .db(|conn| { + crate_owners::table + .select(crate_owners::email_notifications) + .filter(crate_owners::crate_id.eq(not_my_crate.id)) + .first::(&*conn) + }) + .unwrap(); + + // There should be no change to the `email_notifications` value for a crate not belonging to me + assert!(email_notifications); +} diff --git a/src/views.rs b/src/views.rs index 9506ac00a05..7d6af5655a8 100644 --- a/src/views.rs +++ b/src/views.rs @@ -149,9 +149,17 @@ pub struct EncodableApiTokenWithToken { pub last_used_at: Option, } +#[derive(Deserialize, Serialize, Debug)] +pub struct OwnedCrate { + pub id: i32, + pub name: String, + pub email_notifications: bool, +} + #[derive(Serialize, Deserialize, Debug)] pub struct EncodableMe { pub user: EncodablePrivateUser, + pub owned_crates: Vec, } /// The serialization format for the `User` model.