-
Notifications
You must be signed in to change notification settings - Fork 663
Ability to add/update user email #921
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
Changes from 1 commit
d89c3b0
0e663a4
a39b380
0e61301
ccc82e1
671eeab
cb8efee
820d1d2
94e1a25
7bdb157
45ce186
c603bcb
33addcf
28dc5ed
9fe427f
593853e
09786c8
ed1d2ee
8c4d959
0dba390
b2b854c
96bb903
33abaa4
0e618a3
0b90a74
48774f8
45b6669
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,12 +66,18 @@ fn user_insert() { | |
#[test] | ||
fn me() { | ||
let (_b, app, middle) = ::app(); | ||
let mut req = ::req(app, Method::Get, "/me"); | ||
let mut req = ::req(app.clone(), Method::Get, "/me"); | ||
let response = t_resp!(middle.call(&mut req)); | ||
assert_eq!(response.status.0, 403); | ||
|
||
let user = ::mock_user(&mut req, ::user("foo")); | ||
|
||
// with GET /me update gives 404 response | ||
// let user = ::mock_user(&mut req, ::user("foo")); | ||
let user = { | ||
let conn = app.diesel_database.get().unwrap(); | ||
let user = ::new_user("foo").create_or_update(&conn).unwrap(); | ||
::sign_in_as(&mut req, &user); | ||
user | ||
}; | ||
let mut response = ok_resp!(middle.call(&mut req)); | ||
let json: UserShowResponse = ::json(&mut response); | ||
|
||
|
@@ -291,3 +297,76 @@ fn updating_existing_user_doesnt_change_api_token() { | |
assert_eq!("bar", user.gh_login); | ||
assert_eq!("bar_token", user.gh_access_token); | ||
} | ||
|
||
/* Email GitHub private overwrite bug | ||
Please find a better description, that is not english | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😂 I know that feel :) |
||
*/ | ||
#[test] | ||
fn test_github_login_does_not_overwrite_email() { | ||
|
||
|
||
} | ||
|
||
/* Make sure that what is passed into the database is | ||
also what is extracted out of the database | ||
*/ | ||
#[test] | ||
fn test_email_get_and_put() { | ||
#[derive(Deserialize)] | ||
struct R { | ||
user: EncodablePrivateUser, | ||
} | ||
|
||
#[derive(Deserialize)] | ||
struct S { | ||
ok: bool, | ||
} | ||
|
||
let (_b, app, middle) = ::app(); | ||
let mut req = ::req(app.clone(), Method::Get, "/me"); | ||
let user = { | ||
let conn = app.diesel_database.get().unwrap(); | ||
let user = ::new_user("mango").create_or_update(&conn).unwrap(); | ||
::sign_in_as(&mut req, &user); | ||
user | ||
}; | ||
|
||
let mut response = ok_resp!(middle.call(req.with_path("/me").with_method(Method::Get))); | ||
let r = ::json::<R>(&mut response); | ||
assert_eq!(r.user.email, None); | ||
assert_eq!(r.user.login, "mango"); | ||
|
||
let body = r#"{"user":{"email":"[email protected]","name":"Mango McMangoface","login":"mango","avatar":"https://avatars0.githubusercontent.com","url":"https://github.com/mango","kind":null}}"#; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lolololol @ "Mango McMangoface" :) :) :) |
||
let mut response = ok_resp!( | ||
middle.call( | ||
req.with_path(&format!("/api/v1/users/{}", user.id)) | ||
.with_method(Method::Put) | ||
.with_body(body.as_bytes()) | ||
) | ||
); | ||
assert!(::json::<S>(&mut response).ok); | ||
|
||
let mut response = ok_resp!(middle.call(req.with_path("/me").with_method(Method::Get))); | ||
let r = ::json::<R>(&mut response); | ||
assert_eq!(r.user.email.unwrap(), "[email protected]"); | ||
assert_eq!(r.user.login, "mango"); | ||
} | ||
|
||
/* Make sure that empty strings will error and are | ||
not added to the database | ||
*/ | ||
#[test] | ||
fn test_empty_email_not_added() { | ||
|
||
} | ||
|
||
/* A user cannot change the email of another user | ||
Two users in database, one signed in, the other | ||
not signed in, from one that is not signed in try to | ||
change signed in's email | ||
|
||
*/ | ||
#[test] | ||
fn test_this_user_cannot_change_that_user_email() { | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -381,11 +381,27 @@ pub fn logout(req: &mut Request) -> CargoResult<Response> { | |
|
||
/// Handles the `GET /me` route. | ||
pub fn me(req: &mut Request) -> CargoResult<Response> { | ||
// Changed to getting User information from database because in | ||
// src/tests/user.rs, when testing put and get on updating email, | ||
// request seems to be somehow 'cached'. When we try to get a | ||
// request from the /me route with the just updated user (call | ||
// this function) the user is the same as the initial GET request | ||
// and does not seem to get the updated user information from the | ||
// database | ||
// This change is not preferable, we'd rather fix the request, | ||
// perhaps adding `req.mut_extensions().insert(user)` to the | ||
// update_user route, however this somehow does not seem to work | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a great comment!! I'm so excited to find this later when I'm trying to remember why we had to do this this way and this will explain everything ❤️ |
||
use self::users::dsl::{users, id}; | ||
let user_id = req.user()?.id; | ||
let conn = req.db_conn()?; | ||
let user = users.filter(id.eq(user_id)).first::<User>(&*conn)?; | ||
println!("user id: {:?} user_id: {:?}", user.id, user_id); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Println snuck in there! :) |
||
|
||
#[derive(Serialize)] | ||
struct R { | ||
user: EncodablePrivateUser, | ||
} | ||
Ok(req.json(&R { user: req.user()?.clone().encodable_private() })) | ||
Ok(req.json(&R { user: user.encodable_private() })) | ||
} | ||
|
||
/// Handles the `GET /users/:user_id` route. | ||
|
@@ -521,6 +537,8 @@ pub fn update_user(req: &mut Request) -> CargoResult<Response> { | |
let user_email = user_update.user.email.unwrap(); | ||
let user_email = user_email.trim(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice! |
||
|
||
println!("update_user email: {:?}", user_email); | ||
|
||
update(users.filter(gh_login.eq(&user.gh_login))) | ||
.set(email.eq(user_email)) | ||
.execute(&*conn)?; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
::sign_in(&mut req, &app)
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand the difference, could you clarify? I forgot to delete these commented out lines, what I was getting at is that
mock_user()
seems to do the same thing as callingnew_user()
andsign_in_as(user)
used below except below doesn't cause the 404.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sgrif
sign_in
doesn't let us pass in a user, nor does it return the user, and we need the user on line 84 for the last assertion in this test that checks that the user's email is correctly in the JSON.