Skip to content

Add token_count to GET /api/v1/me response #3071

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

Add token_count to GET /api/v1/me response #3071

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5bab212
user: Add `has_token` field
dsavchuk-vineti Dec 26, 2019
099e402
user: Add additional `has_tokens` assertion
Turbo87 Dec 4, 2020
974caff
user: Move `has_tokens` property from `user` to a `meta` object
Turbo87 Dec 4, 2020
ded1aa4
user: Convert `has_tokens` to `token_count`
Turbo87 Dec 4, 2020
aaee1c2
user: Use `count()` to count tokens
Turbo87 Dec 4, 2020
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
15 changes: 11 additions & 4 deletions src/controllers/user/me.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,15 @@ use crate::email;

use crate::controllers::helpers::pagination::Paginated;
use crate::models::{
CrateOwner, Email, Follow, NewEmail, OwnerKind, User, Version, VersionOwnerAction,
ApiToken, CrateOwner, Email, Follow, NewEmail, OwnerKind, User, Version, VersionOwnerAction,
};
use crate::schema::{crate_owners, crates, emails, follows, users, versions};
use crate::views::{EncodableMe, EncodableVersion, OwnedCrate};
use crate::schema::{api_tokens, crate_owners, crates, emails, follows, users, versions};
use crate::views::{EncodableMe, EncodableMeMeta, EncodableVersion, OwnedCrate};

/// Handles the `GET /me` route.
pub fn me(req: &mut dyn RequestExt) -> EndpointResult {
let user_id = req.authenticate()?.user_id();
let user = req.authenticate()?;
let user_id = user.user_id();
let conn = req.db_conn()?;

let (user, verified, email, verification_sent): (User, Option<bool>, Option<String>, bool) =
Expand All @@ -29,6 +30,11 @@ pub fn me(req: &mut dyn RequestExt) -> EndpointResult {
))
.first(&*conn)?;

let token_count = ApiToken::belonging_to(&user)
.filter(api_tokens::revoked.eq(false))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would make me feel better if one of the tokens in the test was revoked and we asserted that it didn't count towards this total.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thinking some more about it, I wonder if this revoked: false query really makes sense for us here. if our primary goal is to teach the user that he needs to create his first token to interact with cargo then, if he has a revoked token already, he apparently doesn't need to be taught this anymore :D

.count()
.get_result(&*conn)?;

let owned_crates = CrateOwner::by_owner_kind(OwnerKind::User)
.inner_join(crates::table)
.filter(crate_owners::owner_id.eq(user_id))
Expand All @@ -48,6 +54,7 @@ pub fn me(req: &mut dyn RequestExt) -> EndpointResult {
Ok(req.json(&EncodableMe {
user: user.encodable_private(email, verified, verification_sent),
owned_crates,
meta: EncodableMeMeta { token_count },
}))
}

Expand Down
26 changes: 24 additions & 2 deletions src/tests/user.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@ use crate::{
OkBool, TestApp,
};
use cargo_registry::{
models::{Email, NewUser, User},
models::{ApiToken, Email, NewUser, User},
schema::crate_owners,
views::{EncodablePrivateUser, EncodablePublicUser, EncodableVersion, OwnedCrate},
views::{
EncodableMeMeta, EncodablePrivateUser, EncodablePublicUser, EncodableVersion, OwnedCrate,
},
};

use diesel::prelude::*;
Expand All @@ -27,6 +29,7 @@ pub struct UserShowPublicResponse {
pub struct UserShowPrivateResponse {
pub user: EncodablePrivateUser,
pub owned_crates: Vec<OwnedCrate>,
pub meta: EncodableMeMeta,
}

#[derive(Deserialize)]
Expand Down Expand Up @@ -748,3 +751,22 @@ fn test_update_email_notifications_not_owned() {
// There should be no change to the `email_notifications` value for a crate not belonging to me
assert!(email_notifications);
}

#[test]
fn shows_that_user_has_tokens() {
let (app, _, user) = TestApp::init().with_user();

let json = user.show_me();
assert_eq!(json.meta.token_count, 0);

let user_id = user.as_model().id;
app.db(|conn| {
vec![
assert_ok!(ApiToken::insert(conn, user_id, "bar")),
assert_ok!(ApiToken::insert(conn, user_id, "baz")),
]
});

let json = user.show_me();
assert_eq!(json.meta.token_count, 2);
}
6 changes: 6 additions & 0 deletions src/views.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,12 @@ pub struct OwnedCrate {
pub struct EncodableMe {
pub user: EncodablePrivateUser,
pub owned_crates: Vec<OwnedCrate>,
pub meta: EncodableMeMeta,
}

#[derive(Serialize, Deserialize, Debug, Clone, Copy)]
pub struct EncodableMeMeta {
pub token_count: i64,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder, what's the criteria for information being in EncodableMe or EncodableMeMeta? Should owned_crates be in EncodableMeMeta, for example?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for asking this question. when I was working on this it seemed obvious to me, but now I'm not so sure anymore either. 😅

I'm now wondering if it would be better to instead include the list of token IDs, like in a proper relationship declaration, but that would probably slightly slow down the query and for what we're trying to build it doesn't make a difference.

Maybe my confusion comes from using user for both other (GitHub) users, but also the logged in user account, but with exta information in the latter case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this a little bit more. I think my reason for putting this in meta was that this is derived data, which corresponds to the number of tokens that will be returned from the GET /api/v1/me/tokens request. It is not actually a property on the user resource, but rather the number of token relationships that this user has.

Should owned_crates be in EncodableMeMeta, for example?

I guess it would be best if this was encoded as a proper relationship instead

}

/// The serialization format for the `User` model.
Expand Down