Skip to content

Email polish #1069

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 5 commits into from
Sep 22, 2017
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
49 changes: 29 additions & 20 deletions app/components/email-input.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,16 @@ export default Component.extend({
}),
isError: false,
emailError: '',
disableResend: false,
resendButtonText: computed('disableResend', 'user.email_verification_sent', function() {
if (this.get('disableResend')) {
return 'Sent!';
} else if (this.get('user.email_verification_sent')) {
return 'Resend';
} else {
return 'Send verification email';
}
}),

actions: {
editEmail() {
Expand Down Expand Up @@ -55,7 +65,11 @@ export default Component.extend({

user.set('email', userEmail);
user.save()
.then(() => this.set('serverError', null))
.then(() => {
this.set('serverError', null);
this.set('user.email_verification_sent', true);
this.set('user.email_verified', false);
})
.catch(err => {
let msg;
if (err.errors && err.errors[0] && err.errors[0].detail) {
Expand All @@ -70,6 +84,7 @@ export default Component.extend({

this.set('isEditing', false);
this.set('notValidEmail', false);
this.set('disableResend', false);
},

cancelEdit() {
Expand All @@ -80,25 +95,19 @@ export default Component.extend({
resendEmail() {
let user = this.get('user');

this.get('ajax').raw(`/api/v1/users/${user.id}/resend`, { method: 'PUT',
user: {
avatar: user.avatar,
email: user.email,
email_verified: user.email_verified,
kind: user.kind,
login: user.login,
name: user.name,
url: user.url
}
}).catch((error) => {
if (error.payload) {
this.set('isError', true);
this.set('emailError', `Error in resending message: ${error.payload.errors[0].detail}`);
} else {
this.set('isError', true);
this.set('emailError', 'Unknown error in resending message');
}
});
this.get('ajax').raw(`/api/v1/users/${user.id}/resend`, {
method: 'PUT'
})
.then(() => this.set('disableResend', true))
.catch((error) => {
if (error.payload) {
this.set('isError', true);
this.set('emailError', `Error in resending message: ${error.payload.errors[0].detail}`);
} else {
this.set('isError', true);
this.set('emailError', 'Unknown error in resending message');
}
});
}
}
});
1 change: 1 addition & 0 deletions app/models/user.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import DS from 'ember-data';
export default DS.Model.extend({
email: DS.attr('string'),
email_verified: DS.attr('boolean'),
email_verification_sent: DS.attr('boolean'),
name: DS.attr('string'),
login: DS.attr('string'),
avatar: DS.attr('string'),
Expand Down
10 changes: 10 additions & 0 deletions app/styles/me.scss
Original file line number Diff line number Diff line change
Expand Up @@ -86,14 +86,24 @@
}
.email {
@include flex(20);
.verified {
color: green;
font-weight: bold;
}
}
.actions {
@include display-flex;
@include align-items(center);
img { margin-left: 10px }
}
.email-form {
@include flex(10);
display: inline-flex;
justify-content: space-between;
flex-wrap: wrap;
input.ember-text-field {
width: 400px;
}
}
.space-right {
margin-right: 10px;
Expand Down
12 changes: 10 additions & 2 deletions app/templates/components/email-input.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,12 @@
<dt>Email</dt>
</div>
<div class='email'>
<dd class='no-space-left'>{{ user.email }}</dd>
<dd class='no-space-left'>
{{ user.email }}
{{#if user.email_verified}}
<span class='verified'>Verified!</span>
{{/if}}
</dd>
</div>
<div class='actions'>
<button class='small yellow-button space-left' {{action 'editEmail'}}>Edit</button>
Expand All @@ -39,10 +44,13 @@
{{#if emailNotVerified }}
<div class='row'>
<div class='label'>
{{#if user.email_verification_sent}}
<p class='small-text'>We have sent a verification email to your address.</p>
{{/if}}
<p class='small-text'>Your email has not yet been verified.</p>
</div>
<div class='actions'>
<button class='small yellow-button space-left' {{action 'resendEmail'}}>Resend</button>
<button class='small yellow-button space-left' {{action 'resendEmail'}} disabled={{disableResend}}>{{resendButtonText}}</button>
</div>
</div>
{{/if}}
Expand Down
60 changes: 59 additions & 1 deletion src/tests/user.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ use conduit::{Handler, Method};

use cargo_registry::token::ApiToken;
use cargo_registry::krate::EncodableCrate;
use cargo_registry::user::{User, NewUser, EncodablePrivateUser, EncodablePublicUser, Email, Token};
use cargo_registry::user::{User, NewUser, EncodablePrivateUser, EncodablePublicUser, Email,
NewEmail, Token};
use cargo_registry::version::EncodableVersion;

use diesel::prelude::*;
Expand Down Expand Up @@ -417,6 +418,8 @@ fn test_email_get_and_put() {
let r = ::json::<R>(&mut response);
assert_eq!(r.user.email.unwrap(), "[email protected]");
assert_eq!(r.user.login, "mango");
assert!(!r.user.email_verified);
assert!(r.user.email_verification_sent);
}

/* Given a crates.io user, check to make sure that the user
Expand Down Expand Up @@ -607,6 +610,8 @@ fn test_insert_into_email_table_with_email_change() {
let r = ::json::<R>(&mut response);
assert_eq!(r.user.email.unwrap(), "[email protected]");
assert_eq!(r.user.login, "potato");
assert!(!r.user.email_verified);
assert!(r.user.email_verification_sent);

let body = r#"{"user":{"email":"[email protected]","name":"potato","login":"potato","avatar":"https://avatars0.githubusercontent.com","url":"https://github.com/potato","kind":null}}"#;
let mut response = ok_resp!(
Expand Down Expand Up @@ -638,6 +643,8 @@ fn test_insert_into_email_table_with_email_change() {
));
let r = ::json::<R>(&mut response);
assert_eq!(r.user.email.unwrap(), "[email protected]");
assert!(!r.user.email_verified);
assert!(r.user.email_verification_sent);
assert_eq!(r.user.login, "potato");
}

Expand Down Expand Up @@ -703,4 +710,55 @@ fn test_confirm_user_email() {
assert_eq!(r.user.email.unwrap(), "[email protected]");
assert_eq!(r.user.login, "potato");
assert!(r.user.email_verified);
assert!(r.user.email_verification_sent);
}

/* Given a user who existed before we added email confirmation,
test that `email_verification_sent` is false so that we don't
make the user think we've sent an email when we haven't.
*/
#[test]
fn test_existing_user_email() {
use cargo_registry::schema::{emails, users};
use diesel::insert;

#[derive(Deserialize)]
struct R {
user: EncodablePrivateUser,
}

let (_b, app, middle) = ::app();
let mut req = ::req(app.clone(), Method::Get, "/me");
{
let conn = app.diesel_database.get().unwrap();
let user = ::new_user("potahto");

// Deliberately not using User::create_or_update since that
// will try to send a verification email; we want to simulate
// a user who already had an email before we added verification.
let user = insert(&user)
.into(users::table)
.get_result::<User>(&*conn)
.unwrap();

println!("{:?}", user);

let email = NewEmail {
user_id: user.id,
email: String::from("[email protected]"),
verified: false,
};

insert(&email).into(emails::table).execute(&*conn).unwrap();

::sign_in_as(&mut req, &user);
}

let mut response = ok_resp!(middle.call(
req.with_path("/api/v1/me").with_method(Method::Get),
));
let r = ::json::<R>(&mut response);
assert_eq!(r.user.email.unwrap(), "[email protected]");
assert!(!r.user.email_verified);
assert!(!r.user.email_verification_sent);
}
35 changes: 28 additions & 7 deletions src/user/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ pub struct EncodablePrivateUser {
pub login: String,
pub email: Option<String>,
pub email_verified: bool,
pub email_verification_sent: bool,
pub name: Option<String>,
pub avatar: Option<String>,
pub url: Option<String>,
Expand Down Expand Up @@ -240,7 +241,11 @@ impl User {
}

/// Converts this `User` model into an `EncodablePrivateUser` for JSON serialization.
pub fn encodable_private(self, email_verified: bool) -> EncodablePrivateUser {
pub fn encodable_private(
self,
email_verified: bool,
email_verification_sent: bool,
) -> EncodablePrivateUser {
let User {
id,
email,
Expand All @@ -254,6 +259,7 @@ impl User {
id: id,
email: email,
email_verified,
email_verification_sent,
avatar: gh_avatar,
login: gh_login,
name: name,
Expand Down Expand Up @@ -415,16 +421,31 @@ pub fn me(req: &mut Request) -> CargoResult<Response> {

use self::users::dsl::{users, id};
use self::emails::dsl::{emails, user_id};
use diesel::select;
use diesel::expression::dsl::exists;

let u_id = req.user()?.id;
let conn = req.db_conn()?;

let user_info = users.filter(id.eq(u_id)).first::<User>(&*conn)?;
let email_result = emails.filter(user_id.eq(u_id)).first::<Email>(&*conn);

let (email, verified): (Option<String>, bool) = match email_result {
Ok(response) => (Some(response.email), response.verified),
Err(_) => (None, false),
let (email, verified, verification_sent): (Option<String>, bool, bool) = match email_result {
Ok(email_record) => {
let verification_sent = if email_record.verified {
true
} else {
select(exists(Token::belonging_to(&email_record)))
.get_result(&*conn)
.unwrap_or(false)
};
(
Some(email_record.email),
email_record.verified,
verification_sent,
)
}
Err(_) => (None, false, false),
};

let user = User {
Expand All @@ -436,7 +457,9 @@ pub fn me(req: &mut Request) -> CargoResult<Response> {
struct R {
user: EncodablePrivateUser,
}
Ok(req.json(&R { user: user.encodable_private(verified) }))
Ok(req.json(&R {
user: user.encodable_private(verified, verification_sent),
}))
}

/// Handles the `GET /users/:user_id` route.
Expand Down Expand Up @@ -692,8 +715,6 @@ pub fn regenerate_token_and_send(req: &mut Request) -> CargoResult<Response> {
use time;
use self::tokens::dsl::email_id;

let mut body = String::new();
req.body().read_to_string(&mut body)?;
let user = req.user()?;
let name = &req.params()["user_id"].parse::<i32>().ok().unwrap();
let conn = req.db_conn()?;
Expand Down