Skip to content

Use CrateOwner::by_owner_kind everywhere to exclude deleted ownerships #1940

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/controllers/user/me.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)?
Expand Down
10 changes: 3 additions & 7 deletions src/controllers/user/other.rs
Original file line number Diff line number Diff line change
@@ -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;

Expand Down Expand Up @@ -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);
Expand Down
16 changes: 16 additions & 0 deletions src/tests/owners.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
29 changes: 28 additions & 1 deletion src/tests/user.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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.
*/
Expand Down