changed code path, removed unneeded params, all the way up

Author: nic-hartley

Diff for: src/controllers/krate/owners.rs

@@ -141,7 +141,7 @@ fn modify_owners(
             msgs.join(",")
         } else {
             for login in &logins {
-                krate.owner_remove(app, conn, user, login)?;
+                krate.owner_remove(conn, login)?;
             }
             if User::owning(&krate, conn)?.is_empty() {
                 return Err(cargo_err(

Diff for: src/models/krate.rs

@@ -397,14 +397,8 @@ impl Crate {
         }
     }
 
-    pub fn owner_remove(
-        &self,
-        app: &App,
-        conn: &mut PgConnection,
-        req_user: &User,
-        login: &str,
-    ) -> AppResult<()> {
-        let owner = Owner::find_or_create_by_login(app, conn, req_user, login)?;
+    pub fn owner_remove(&self, conn: &mut PgConnection, login: &str) -> AppResult<()> {
+        let owner = Owner::find_by_login(conn, login)?;
 
         let target = crate_owners::table.find((self.id(), owner.id(), owner.kind()));
         diesel::update(target)

Diff for: src/models/owner.rs

@@ -1,8 +1,8 @@
 use diesel::pg::Pg;
 use diesel::prelude::*;
 
-use crate::app::App;
 use crate::util::errors::{cargo_err, AppResult};
+use crate::{app::App, schema::teams};
 
 use crate::models::{Crate, Team, User};
 use crate::schema::{crate_owners, users};
@@ -59,6 +59,7 @@ impl Owner {
     /// up-to-date GitHub ID. Fails out if the user isn't found in the
     /// database, the team isn't found on GitHub, or if the user isn't a member
     /// of the team on GitHub.
+    ///
     /// May be a user's GH login or a full team name. This is case
     /// sensitive.
     pub fn find_or_create_by_login(
@@ -82,6 +83,30 @@ impl Owner {
         }
     }
 
+    /// Finds the owner by name. Never recreates a team, to ensure that
+    /// organizations that were deleted after they were added can still be
+    /// removed.
+    ///
+    /// May be a user's GH login or a full team name. This is case
+    /// sensitive.
+    pub fn find_by_login(conn: &mut PgConnection, name: &str) -> AppResult<Owner> {
+        if name.contains(':') {
+            teams::table
+                .filter(lower(teams::login).eq(&name.to_lowercase()))
+                .first(conn)
+                .map(Owner::Team)
+                .map_err(|_| cargo_err(&format_args!("could not find team with login `{name}`")))
+        } else {
+            users::table
+                .filter(lower(users::gh_login).eq(name.to_lowercase()))
+                .filter(users::gh_id.ne(-1))
+                .order(users::gh_id.desc())
+                .first(conn)
+                .map(Owner::User)
+                .map_err(|_| cargo_err(&format_args!("could not find user with login `{name}`")))
+        }
+    }
+
     pub fn kind(&self) -> i32 {
         match *self {
             Owner::User(_) => OwnerKind::User as i32,

Diff for: src/tests/owners.rs

@@ -567,9 +567,7 @@ fn deleted_ownership_isnt_in_owner_user() {
 
     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();
+        krate.owner_remove(conn, &user.gh_login).unwrap();
     });
 
     let json: UserResponse = anon.get("/api/v1/crates/foo_my_packages/owner_user").good();

Diff for: src/tests/routes/crates/list.rs

@@ -839,9 +839,7 @@ fn crates_by_user_id_not_including_deleted_owners() {
 
     app.db(|conn| {
         let krate = CrateBuilder::new("foo_my_packages", user.id).expect_build(conn);
-        krate
-            .owner_remove(app.as_inner(), conn, user, "foo")
-            .unwrap();
+        krate.owner_remove(conn, "foo").unwrap();
     });
 
     let response = anon.search_by_user_id(user.id);

Diff for: src/tests/routes/me/get.rs

@@ -42,9 +42,7 @@ fn test_user_owned_crates_doesnt_include_deleted_ownership() {
 
     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();
+        krate.owner_remove(conn, &user_model.gh_login).unwrap();
     });
 
     let json = user.show_me();

Diff for: src/tests/routes/users/stats.rs

@@ -39,7 +39,7 @@ fn user_total_downloads() {
             .execute(conn)
             .unwrap();
         no_longer_my_krate
-            .owner_remove(app.as_inner(), conn, user, &user.gh_login)
+            .owner_remove(conn, &user.gh_login)
             .unwrap();
     });
 

Diff for: src/tests/team.rs

@@ -449,9 +449,7 @@ fn crates_by_team_id_not_including_deleted_owners() {
 
         let krate = CrateBuilder::new("foo", user.id).expect_build(conn);
         add_team_to_crate(&t, &krate, user, conn).unwrap();
-        krate
-            .owner_remove(app.as_inner(), conn, user, &t.login)
-            .unwrap();
+        krate.owner_remove(conn, &t.login).unwrap();
         t
     });