Skip to content

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

Merged
merged 27 commits into from
Aug 3, 2017
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
d89c3b0
string can be input in email field which is sent to the backend, user…
natboehm Jul 20, 2017
0e663a4
delete unsused variables
natboehm Jul 21, 2017
a39b380
added rudimentary user email validation, has empty input error on the…
natboehm Jul 21, 2017
0e61301
delete ember validation import, extract email input into component
natboehm Jul 24, 2017
ccc82e1
save button disabled if email field empty, odd bug where input was so…
natboehm Jul 24, 2017
671eeab
added simple email validation
natboehm Jul 24, 2017
cb8efee
add error catch statement, fix javascript linting errors
natboehm Jul 24, 2017
820d1d2
wrap input submit with form tag
natboehm Jul 24, 2017
94e1a25
add div to wrap email field
natboehm Jul 25, 2017
7bdb157
initial basic styling on email label
natboehm Jul 25, 2017
45ce186
basic styling for viewing and editing email field
natboehm Jul 25, 2017
c603bcb
only displays 'please add email' message when email field is currentl…
natboehm Jul 25, 2017
33addcf
display 'please add email' message only when email is not already added
natboehm Jul 26, 2017
28dc5ed
Split EncodableUser into EncodablePrivateUser and EncodablePublicUser…
natboehm Jul 26, 2017
9fe427f
fixes bug where GitHub login would update and overwrite current email…
natboehm Jul 26, 2017
593853e
add test to put and get updates to user email, had to change GET /me …
natboehm Jul 28, 2017
09786c8
tests for empty email and checking that another user cannot change cu…
natboehm Jul 28, 2017
ed1d2ee
add test to check for bug of GitHub login overwriting current email d…
natboehm Jul 28, 2017
8c4d959
add comments to tests, delete debug println
natboehm Jul 28, 2017
0dba390
delete unused variable
natboehm Jul 28, 2017
b2b854c
cargo fmt
natboehm Jul 28, 2017
96bb903
delete function new_user_with_id
natboehm Jul 28, 2017
33abaa4
delete commented out code
natboehm Aug 2, 2017
0e618a3
reword please add email message
natboehm Aug 2, 2017
0b90a74
simplify isEmailNull function, edit error message
natboehm Aug 2, 2017
48774f8
delete print statement
natboehm Aug 2, 2017
45b6669
change regex to not match on newline characters - before users could …
natboehm Aug 2, 2017
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
66 changes: 66 additions & 0 deletions app/components/email-input.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import Component from '@ember/component';
import { empty } from '@ember/object/computed';

export default Component.extend({
type: '',
value: '',
isEditing: false,
user: null,
disableSave: empty('user.email'),
notValidEmail: false,
prevEmail: '',
emailIsNull: true,

actions: {
editEmail() {
let email = this.get('value');
let isEmailNull = function(email) {
if (email == null) {
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 we can shorten this by doing:

let isEmailNull = function(email) {
    return email == null;
}

Does that work? My javascript is a bit rusty...

Copy link
Member

Choose a reason for hiding this comment

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

OOH WAIT what's this empty function on line 9? could we do empty(email)? or is that special for models?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this might be special for models? I'm not sure exactly how it works on line 9 but when I print out the result of empty it isn't just a boolean type.

return true;
} else {
return false;
}
};

this.set('emailIsNull', isEmailNull(email));
this.set('isEditing', true);
this.set('prevEmail', this.get('value'));
Copy link
Member

Choose a reason for hiding this comment

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

Since we've got this.get('value') in the email variable from line 16, could we use the email var here too?

},

saveEmail() {
let userEmail = this.get('value');
let user = this.get('user');

let emailIsProperFormat = function(userEmail) {
let regExp = /\S+@\S+\.\S+/;
Copy link
Member

Choose a reason for hiding this comment

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

I'm remembering a thing.... where if you don't put "beginning of value" and "end of value" things in the regex, somehow people could put newlines in and the regex would match when it shouldn't have? I forget if javascript is vulnerable to this or not, and if it is, if the things we want at the beginning and end are \A and \z or if it's something different. Let's investigate tomorrow!

return regExp.test(userEmail);
};

if (!emailIsProperFormat(userEmail)) {
this.set('notValidEmail', true);
return;
}

user.set('email', userEmail);
user.save()
.then(() => this.set('serverError', null))
.catch(err => {
let msg;
if (err.errors && err.errors[0] && err.errors[0].detail) {
msg = `An error occurred while saving this email, ${err.errors[0].detail}`;
} else {
msg = 'An unknown error occurred while saving this token.';
Copy link
Member

Choose a reason for hiding this comment

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

Should this error message say email instead of token?

}
this.set('serverError', msg);
});

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

cancelEdit() {
this.set('isEditing', false);
this.set('value', this.get('prevEmail'));
}
}
});
41 changes: 41 additions & 0 deletions app/components/validated-input.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import Ember from 'ember';

const {
computed,
defineProperty
} = Ember;

export default Ember.Component.extend({
classNames: ['validated-input'],
classNameBindings: ['showErrorClass:has-error', 'isValid:has-success'],
model: null,
value: null,
type: 'text',
valuePath: '',
placeholder: '',
validation: null,
showValidations: false,
didValidate: false,

notValidating: computed.not('validation.isValidating').readOnly(),
hasContent: computed.notEmpty('value').readOnly(),
hasWarnings: computed.notEmpty('validation.warnings').readOnly(),
isValid: computed.and('hasContent', 'validation.isTruelyValid').readOnly(),
shouldDisplayValidations: computed.or('showValidations', 'didValidate', 'hasContent').readOnly(),

showErrorClass: computed.and('notValidating', 'showErrorMessage', 'hasContent', 'validation').readOnly(),
showErrorMessage: computed.and('shouldDisplayValidations', 'validation.isInvalid').readOnly(),

init() {
this._super(...arguments);
let valuePath = this.get('valuePath');

defineProperty(this, 'validation', computed.readOnly(`model.validations.attrs.${valuePath}`));
defineProperty(this, 'value', computed.alias(`model.${valuePath}`));
},

focusOut() {
this._super(...arguments);
this.set('showValidations', true);
}
});
42 changes: 42 additions & 0 deletions app/styles/me.scss
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,48 @@
clear: both;
}
dd { float: left; margin-left: 10px; }
.align-center {
@include align-items(center);
}
.row {
width: 100%;
border: 1px solid #d5d3cb;
border-bottom-width: 0px;
&:last-child { border-bottom-width: 1px; }
padding: 10px 20px 10px 0;
@include display-flex;
background-color: $main-bg-dark;
.small-text {
font-size: 90%;
}
.error {
color: rgb(216, 0, 41);
}
.label {
@include flex(1);
margin-right: 0.4em;
font-weight: bold;
}
.actions {
@include display-flex;
@include align-items(center);
}
.space-left {
margin-left: 10px;
}
.space-right {
margin-right: 10px;
}
.no-space-left {
margin-left: 0px;
}
p {
width: 80%;
}
.space-bottom {
margin-bottom: 10px;
}
}
}

@media only screen and (max-width: 550px) {
Expand Down
35 changes: 35 additions & 0 deletions app/templates/components/email-input.hbs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
{{#if isEditing }}
Copy link
Member

Choose a reason for hiding this comment

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

Good call on moving the check for editing out to be able to separate the markup between editing and not editing :) 💯

<div class='row'>
<div class='label'>
<dt>Email</dt>
</div>
<form {{action 'saveEmail' on='submit'}}>
{{input type=type value=value placeholder='Email' class='form-control space-bottom'}}
{{#if notValidEmail }}
<p class='small-text error'>Invalid email format. Please try again.</p>
{{/if}}
{{#if emailIsNull }}
<p class='small-text'> Please add your email address. We need this in
order to contact you just in case anything goes wrong. We promise
Copy link
Member

Choose a reason for hiding this comment

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

Thinking about this phrasing a little bit more, especially since we just came up with a potential use case in ownership notifications that we might want to use email for, which isn't really something going wrong... how about something like "Please add your email address. We will only use it to contact you about your account. We promise we'll never share it!"?

we'll never share it!
</p>
{{/if}}
<div class='actions'>
<button type='submit' class='small yellow-button space-right' disabled={{disableSave}}>Save</button>
<button class='small yellow-button' {{action 'cancelEdit'}}>Cancel</button>
</div>
</form>
</div>
{{else}}
<div class='row align-center'>
<div class='label'>
<dt>Email</dt>
</div>
<div class='email'>
<dd class='no-space-left'>{{ user.email }}</dd>
</div>
<div class='actions'>
<button class='small yellow-button space-left' {{action 'editEmail'}}>Edit</button>
</div>
</div>
{{/if}}
14 changes: 14 additions & 0 deletions app/templates/components/validated-input.hbs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<div class='form-group'>
{{input type=type value=value placeholder=placeholder class='form-control' name=valuePath}}
{{#if isValid}}
<span class='valid-input fa fa-check'></span>
{{/if}}

<div class='input-error'>
{{#if showErrorMessage}}
<div class='error'>
{{v-get model valuePath 'message'}}
</div>
{{/if}}
</div>
</div>
3 changes: 1 addition & 2 deletions app/templates/me/index.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@
<dd>{{ model.user.name }}</dd>
<dt>GitHub Account</dt>
<dd>{{ model.user.login }}</dd>
<dt>Email</dt>
<dd>{{ model.user.email }}</dd>
{{email-input type='email' value=model.user.email user=model.user}}
</dl>
</div>
</div>
Expand Down
1 change: 1 addition & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ pub fn middleware(app: Arc<App>) -> MiddlewareBuilder {
api_router.get("/categories/:category_id", C(category::show));
api_router.get("/category_slugs", C(category::slugs));
api_router.get("/users/:user_id", C(user::show));
api_router.put("/users/:user_id", C(user::update_user));
api_router.get("/users/:user_id/stats", C(user::stats));
api_router.get("/teams/:team_id", C(user::show_team));
let api_router = Arc::new(R404(api_router));
Expand Down
4 changes: 0 additions & 4 deletions src/owner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ pub struct EncodableOwner {
pub id: i32,
pub login: String,
pub kind: String,
pub email: Option<String>,
pub url: Option<String>,
pub name: Option<String>,
pub avatar: Option<String>,
Expand Down Expand Up @@ -361,7 +360,6 @@ impl Owner {
match self {
Owner::User(User {
id,
email,
name,
gh_login,
gh_avatar,
Expand All @@ -371,7 +369,6 @@ impl Owner {
EncodableOwner {
id: id,
login: gh_login,
email: email,
avatar: gh_avatar,
url: Some(url),
name: name,
Expand All @@ -389,7 +386,6 @@ impl Owner {
EncodableOwner {
id: id,
login: login,
email: None,
url: Some(url),
avatar: avatar,
name: name,
Expand Down
4 changes: 2 additions & 2 deletions src/tests/krate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use cargo_registry::owner::EncodableOwner;
use cargo_registry::schema::versions;

use cargo_registry::upload as u;
use cargo_registry::user::EncodableUser;
use cargo_registry::user::EncodablePublicUser;
use cargo_registry::version::EncodableVersion;
use cargo_registry::category::Category;

Expand Down Expand Up @@ -1216,7 +1216,7 @@ fn following() {
fn owners() {
#[derive(Deserialize)]
struct R {
users: Vec<EncodableUser>,
users: Vec<EncodablePublicUser>,
}
#[derive(Deserialize)]
struct O {
Expand Down
Loading