Auto merge of #1940 - integer32llc:owned-crates-shouldnt-include-dele…

…ted, r=smarnach

Use `CrateOwner::by_owner_kind` everywhere to exclude deleted ownerships

I found some places I missed checking for where we should be using the new `CrateOwner::by_owner_kind` helper method added in 6d94155 that adds the filter for `deleted` being false.

I've also added one test that currently passes and two tests that failed before this fix to hopefully cover everywhere that selects crate owners; the other places already have tests.

src/controllers/user/me.rs

@@ -38,10 +38,9 @@ pub fn me(req: &mut dyn Request) -> AppResult<Response> {
         ))
         .first::<(User, Option<bool>, Option<String>, 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)?

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<Response> {
     let user_id = &req.params()["user_id"].parse::<i32>().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::<Option<i64>>(&*conn)?
         .unwrap_or(0);

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();

src/tests/user.rs

@@ -323,11 +323,22 @@ 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 +612,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.
 */