Ability to add/update user email #921

natboehm · 2017-07-28T20:27:32Z

This PR addresses issue #808, allow editing your own email address. This is the first step in a three part implementation, adding a field to edit an email address. Yet to be implemented before the issue is resolved is (1) the ability to verify emails with users and (2) figuring out how to get everyone to add their email to their account. This PR only implements the addition of a button to the email field such that a user's email can be added or updated, as well as hiding the email field from public view by separating EncodableUser into EncodablePublicUser and EncodablePrivateUser.

… email field then is updated in the database

… ember side, i think

…metimes undefined seems to be fixed

…y null

… in order to try to keep user emails private, update tests to pass

… stored in database specifically problematic if GitHub email were private and crates.io user had added an email, upon signing in the next time the user's email would go back to null as that is the value GitHub passes each time. This means that if users update their email, it must be updated through both GitHub and Crates.io

…to support test, for now

…rrently signed in user's email

…ata updated by crates.io user

sgrif · 2017-07-28T20:40:12Z

src/tests/all.rs

@@ -189,6 +189,19 @@ fn new_user(login: &str) -> NewUser {
    }
 }

+// I want to be able to seed the github id for use in test


Since this is only used in one place, how about just having that place just do:

NewUser { gh_id: whatever, ..new_user(login) }

Cool, that makes more sense, thanks!

sgrif · 2017-07-28T20:41:09Z

src/tests/user.rs

-    let user = ::mock_user(&mut req, ::user("foo"));
-
+    // with GET /me update gives 404 response
+    // let user = ::mock_user(&mut req, ::user("foo"));


How about ::sign_in(&mut req, &app)?

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 calling new_user() and sign_in_as(user) used below except below doesn't cause the 404.

@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.

carols10cents

Tried this out-- it works great! Just a few small things! :)

carols10cents · 2017-08-01T20:23:22Z

app/templates/components/email-input.hbs

+            {{/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


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!"?

carols10cents · 2017-08-01T20:26:08Z

app/components/email-input.js

+        editEmail() {
+            let email = this.get('value');
+            let isEmailNull = function(email) {
+                if (email == null) {


I think we can shorten this by doing:

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

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

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

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.

carols10cents · 2017-08-01T20:36:57Z

app/components/email-input.js

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


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

carols10cents · 2017-08-01T20:42:19Z

src/tests/user.rs

-    let user = ::mock_user(&mut req, ::user("foo"));
-
+    // with GET /me update gives 404 response
+    // let user = ::mock_user(&mut req, ::user("foo"));


@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.

carols10cents · 2017-08-01T20:43:37Z

src/user/mod.rs

+    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);


Println snuck in there! :)

carols10cents · 2017-08-01T20:58:09Z

app/components/email-input.js

+            let user = this.get('user');
+
+            let emailIsProperFormat = function(userEmail) {
+                let regExp = /\S+@\S+\.\S+/;


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!

carols10cents · 2017-08-01T21:01:05Z

app/templates/components/email-input.hbs

-        <dt>Email</dt>
-    </div>
-    {{#if isEditing }}
+{{#if isEditing }}


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

carols10cents · 2017-08-01T21:03:54Z

src/tests/user.rs

@@ -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


😂 I know that feel :)

carols10cents · 2017-08-01T21:04:52Z

src/tests/user.rs

+    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}}"#;


lolololol @ "Mango McMangoface" :) :) :)

carols10cents · 2017-08-01T21:11:51Z

src/user/mod.rs

+    // 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


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 ❤️

…potentially add their email, then garbage on a newline, which would still evaluate as valid

carols10cents

Looks great!!!! Thank you!!!

bors r+

carols10cents · 2017-08-02T23:49:15Z

hmmm

bors: r+

921: Ability to add/update user email r=carols10cents This PR addresses issue #808, allow editing your own email address. This is the first step in a three part implementation, adding a field to edit an email address. Yet to be implemented before the issue is resolved is (1) the ability to verify emails with users and (2) figuring out how to get everyone to add their email to their account. This PR only implements the addition of a button to the email field such that a user's email can be added or updated, as well as hiding the email field from public view by separating `EncodableUser` into `EncodablePublicUser` and `EncodablePrivateUser`.

bors-voyager · 2017-08-03T00:04:07Z

Build succeeded

continuous-integration/travis-ci/push

natboehm added 21 commits July 28, 2017 16:16

string can be input in email field which is sent to the backend, user…

d89c3b0

… email field then is updated in the database

delete unsused variables

0e663a4

added rudimentary user email validation, has empty input error on the…

a39b380

… ember side, i think

delete ember validation import, extract email input into component

0e61301

save button disabled if email field empty, odd bug where input was so…

ccc82e1

…metimes undefined seems to be fixed

added simple email validation

671eeab

add error catch statement, fix javascript linting errors

cb8efee

wrap input submit with form tag

820d1d2

add div to wrap email field

94e1a25

initial basic styling on email label

7bdb157

basic styling for viewing and editing email field

45ce186

only displays 'please add email' message when email field is currentl…

c603bcb

…y null

display 'please add email' message only when email is not already added

33addcf

Split EncodableUser into EncodablePrivateUser and EncodablePublicUser…

28dc5ed

… in order to try to keep user emails private, update tests to pass

add test to put and get updates to user email, had to change GET /me …

593853e

…to support test, for now

tests for empty email and checking that another user cannot change cu…

09786c8

…rrently signed in user's email

add test to check for bug of GitHub login overwriting current email d…

ed1d2ee

…ata updated by crates.io user

add comments to tests, delete debug println

8c4d959

delete unused variable

0dba390

cargo fmt

b2b854c

sgrif reviewed Jul 28, 2017

View reviewed changes

delete function new_user_with_id

96bb903

natboehm mentioned this pull request Aug 1, 2017

Allow editing your own email address #808

Closed

carols10cents requested changes Aug 1, 2017

View reviewed changes

natboehm added 4 commits August 2, 2017 10:40

delete commented out code

33abaa4

reword please add email message

0e618a3

simplify isEmailNull function, edit error message

0b90a74

delete print statement

48774f8

carols10cents added the S-waiting-on-author label Aug 2, 2017

change regex to not match on newline characters - before users could …

45b6669

…potentially add their email, then garbage on a newline, which would still evaluate as valid

carols10cents approved these changes Aug 2, 2017

View reviewed changes

bors-voyager bot merged commit 45b6669 into rust-lang:master Aug 3, 2017

natboehm deleted the add-user-email branch August 3, 2017 14:20

Turbo87 removed the S-waiting-on-author label Dec 3, 2017

Turbo87 mentioned this pull request Aug 30, 2024

Validate email addresses before saving them to the database #9365

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ability to add/update user email #921

Ability to add/update user email #921

natboehm commented Jul 28, 2017

sgrif Jul 28, 2017

natboehm Jul 28, 2017

sgrif Jul 28, 2017

natboehm Jul 28, 2017

carols10cents Aug 1, 2017

carols10cents left a comment

carols10cents Aug 1, 2017

carols10cents Aug 1, 2017

carols10cents Aug 1, 2017

natboehm Aug 2, 2017

carols10cents Aug 1, 2017

carols10cents Aug 1, 2017

carols10cents Aug 1, 2017

carols10cents Aug 1, 2017

carols10cents Aug 1, 2017

carols10cents Aug 1, 2017

carols10cents Aug 1, 2017

carols10cents Aug 1, 2017

carols10cents left a comment

carols10cents commented Aug 2, 2017

bors-voyager bot commented Aug 3, 2017

Ability to add/update user email #921

Ability to add/update user email #921

Conversation

natboehm commented Jul 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carols10cents left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carols10cents left a comment

Choose a reason for hiding this comment

carols10cents commented Aug 2, 2017

bors-voyager bot commented Aug 3, 2017

Build succeeded