From 9cf8dce1233036fd2fb30334dcd39c209a772b56 Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Thu, 5 Dec 2019 14:15:16 -0500 Subject: [PATCH 1/3] Add a test that passes but will catch ownership deletion regressions --- src/tests/owners.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/tests/owners.rs b/src/tests/owners.rs index 24f11051ffd..58ea26f13ab 100644 --- a/src/tests/owners.rs +++ b/src/tests/owners.rs @@ -268,6 +268,22 @@ fn check_ownership_one_crate() { assert_eq!(json.users[0].name, user.name); } +#[test] +fn deleted_ownership_isnt_in_owner_user() { + let (app, anon, user) = TestApp::init().with_user(); + let user = user.as_model(); + + app.db(|conn| { + let krate = CrateBuilder::new("foo_my_packages", user.id).expect_build(conn); + krate + .owner_remove(app.as_inner(), conn, user, &user.gh_login) + .unwrap(); + }); + + let json: UserResponse = anon.get("/api/v1/crates/foo_my_packages/owner_user").good(); + assert_eq!(json.users.len(), 0); +} + #[test] fn invitations_are_empty_by_default() { let (_, _, user) = TestApp::init().with_user(); From ae0a3fd0d57eba469f41002db8c4fb43bd901130 Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Thu, 5 Dec 2019 14:19:22 -0500 Subject: [PATCH 2/3] Add failing tests for deleted ownerships in user stats and email notifications --- src/tests/user.rs | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/src/tests/user.rs b/src/tests/user.rs index fde2c94a39b..c90a634d449 100644 --- a/src/tests/user.rs +++ b/src/tests/user.rs @@ -323,11 +323,20 @@ fn user_total_downloads() { .set(&another_krate) .execute(conn) .unwrap(); + + let mut no_longer_my_krate = CrateBuilder::new("nacho", user.id).expect_build(conn); + no_longer_my_krate.downloads = 5; + update(&no_longer_my_krate).set(&no_longer_my_krate).execute(conn).unwrap(); + no_longer_my_krate + .owner_remove(app.as_inner(), conn, user, &user.gh_login) + .unwrap(); + }); let url = format!("/api/v1/users/{}/stats", user.id); let stats: UserStats = anon.get(&url).good(); - assert_eq!(stats.total_downloads, 30); // instead of 32 + // does not include crates user never owned (2) or no longer owns (5) + assert_eq!(stats.total_downloads, 30); } #[test] @@ -601,6 +610,22 @@ fn test_existing_user_email() { assert!(!json.user.email_verification_sent); } +#[test] +fn test_user_owned_crates_doesnt_include_deleted_ownership() { + let (app, _, user) = TestApp::init().with_user(); + let user_model = user.as_model(); + + app.db(|conn| { + let krate = CrateBuilder::new("foo_my_packages", user_model.id).expect_build(conn); + krate + .owner_remove(app.as_inner(), conn, user_model, &user_model.gh_login) + .unwrap(); + }); + + let json = user.show_me(); + assert_eq!(json.owned_crates.len(), 0); +} + /* 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. */ From 9c32906726ccad5fe0e72801f0c09e154f8dc14e Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Thu, 5 Dec 2019 14:20:21 -0500 Subject: [PATCH 3/3] Use `CrateOwner::by_owner_kind` to exclude deleted crate ownerships --- src/controllers/user/me.rs | 3 +-- src/controllers/user/other.rs | 10 +++------- src/tests/user.rs | 6 ++++-- 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/src/controllers/user/me.rs b/src/controllers/user/me.rs index 65391eb9e45..a67681f17d3 100644 --- a/src/controllers/user/me.rs +++ b/src/controllers/user/me.rs @@ -38,10 +38,9 @@ pub fn me(req: &mut dyn Request) -> AppResult { )) .first::<(User, Option, Option, bool)>(&*conn)?; - let owned_crates = crate_owners::table + let owned_crates = CrateOwner::by_owner_kind(OwnerKind::User) .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)? diff --git a/src/controllers/user/other.rs b/src/controllers/user/other.rs index b2e9cb598d5..3af2d33ea03 100644 --- a/src/controllers/user/other.rs +++ b/src/controllers/user/other.rs @@ -1,6 +1,6 @@ use crate::controllers::prelude::*; -use crate::models::{OwnerKind, User}; +use crate::models::{CrateOwner, OwnerKind, User}; use crate::schema::{crate_owners, crates, users}; use crate::views::EncodablePublicUser; @@ -31,13 +31,9 @@ pub fn stats(req: &mut dyn Request) -> AppResult { let user_id = &req.params()["user_id"].parse::().ok().unwrap(); let conn = req.db_conn()?; - let data = crate_owners::table + let data = CrateOwner::by_owner_kind(OwnerKind::User) .inner_join(crates::table) - .filter( - crate_owners::owner_id - .eq(user_id) - .and(crate_owners::owner_kind.eq(OwnerKind::User as i32)), - ) + .filter(crate_owners::owner_id.eq(user_id)) .select(sum(crates::downloads)) .first::>(&*conn)? .unwrap_or(0); diff --git a/src/tests/user.rs b/src/tests/user.rs index c90a634d449..1ca0e95771a 100644 --- a/src/tests/user.rs +++ b/src/tests/user.rs @@ -326,11 +326,13 @@ fn user_total_downloads() { let mut no_longer_my_krate = CrateBuilder::new("nacho", user.id).expect_build(conn); no_longer_my_krate.downloads = 5; - update(&no_longer_my_krate).set(&no_longer_my_krate).execute(conn).unwrap(); + update(&no_longer_my_krate) + .set(&no_longer_my_krate) + .execute(conn) + .unwrap(); no_longer_my_krate .owner_remove(app.as_inner(), conn, user, &user.gh_login) .unwrap(); - }); let url = format!("/api/v1/users/{}/stats", user.id);