Drop Email column (merge and deploy the fix for #1888 first!) #1891

mugoh · 2019-11-12T04:18:04Z

Fixes #1889

This cleans up the uses of the field email from User by:

dropping the email field from User
clearing references to the dropped email field
migrating and updating the schema changes

rust-highfive · 2019-11-12T04:18:15Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @carols10cents (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

mugoh · 2019-11-12T05:56:42Z

How should the test database visibility configuration be updated after modifying the schema?
It fails with:

---- tasks::dump_db::gen_scripts::tests::check_visibility_config stdout ----
thread 'tasks::dump_db::gen_scripts::tests::check_visibility_config' panicked at 'The visibility configuration does not match the database schema:
Column users.email does not exist in the database.', src/tasks/dump_db/gen_scripts.rs:204:9

carols10cents · 2019-11-13T13:41:22Z

How should the test database visibility configuration be updated after modifying the schema?
It fails with:

---- tasks::dump_db::gen_scripts::tests::check_visibility_config stdout ----
thread 'tasks::dump_db::gen_scripts::tests::check_visibility_config' panicked at 'The visibility configuration does not match the database schema:
Column users.email does not exist in the database.', src/tasks/dump_db/gen_scripts.rs:204:9

Removing the users.email line from the dump-db.toml file should do it. The dump-db.toml file is configuration for the public database dumps we make, and this test ensures the database schema is always in sync with this configuration so that we don't accidentally leak data.

I'll make the error message for that test a bit clearer to point at what needs to be done.

- `do_nothing` on `emails` insertion violates unique constraint on `user_id`

mugoh · 2019-11-13T14:32:01Z

Thanks @carols10cents! That sorted it out.
Dumping to the emails table originally required the user: email column - what would be the reason for getting a unique key violation on emails: user_id after dropping the users: email column.

crates.io/migrations/20170804200817_add_email_table/up.sql

Lines 16 to 17 in 23f187a

    
           INSERT INTO emails (user_id, email) 
        
               SELECT id, email FROM users WHERE email IS NOT NULL;

carols10cents · 2019-11-14T19:21:10Z

Dumping to the emails table originally required the user: email column - what would be the reason for getting a unique key violation on emails: user_id after dropping the users: email column.

I'm confused, are you getting a unique key violation? Can you give me steps to reproduce?

carols10cents · 2019-11-14T19:21:52Z

Oh I see the errors in travis now. Investigating!

carols10cents

Some comments that should help you get past the test failures! Please let me know if you have more questions!

carols10cents · 2019-11-14T19:23:33Z

migrations/2019-11-11-162609_drop_email_from_user/up.sql

@@ -0,0 +1,2 @@
+-- Your SQL goes here


You can remove this comment :)

carols10cents · 2019-11-14T19:24:25Z

migrations/2019-11-11-162609_drop_email_from_user/up.sql

@@ -0,0 +1,2 @@
+-- Your SQL goes here
+ALTER TABLE  users DROP COLUMN email CASCADE;


This is super picky, but can you remove the extra space between TABLE and users?

carols10cents · 2019-11-14T19:25:20Z

src/controllers/user/me.rs

    let verified = verified.unwrap_or(false);
    let verification_sent = verified || verification_sent;
-    let user = User { email, ..user };
+    let user = User { ..user };


I think this line can be completely removed now, actually! I think user = User { ..user } is doing a no-op.

carols10cents · 2019-11-14T19:26:56Z

src/models/user.rs

        email_verified: bool,
        email_verification_sent: bool,
-    ) -> EncodablePrivateUser {
+    ) -> CargoResult<EncodablePrivateUser> {


Can you explain why this function needed to be changed to return a Result? It doesn't look like it's necessary to me because I don't see anywhere returning an Err variant. Am I missing something?

It isn't necessary. I was previously reading this email variable from the database with an Err

crates.io/src/models/user.rs

Line 86 in 3544b91

if let Some(user_email) = email {

variant result before realizing that was already "done" and the email could simply be passed to the src/models/users/encodable_private function as an argument.
I definitely forgot to undo that modification to the return type - will do so now

carols10cents · 2019-11-14T19:31:51Z

src/models/user.rs

    pub gh_id: i32,
 }

+/// Represents a new user record insertible to the `users` table


Small fix: can you change insertible to insertable in this comment please?

carols10cents · 2019-11-14T19:43:26Z

src/tests/util.rs

-            user.email = Some(email.to_string());
+            let email = "[email protected]";
+            let user = crate::new_user(username)
+                .create_or_update(Some(email), conn)


Aha, so the unique constraint that's causing the tests to fail is that here in the test setup you're creating a new user with an email set, then lines 142-149 right after this the test setup is inserting the email into the table directly, which violates the constraint we have that each user may only have one email.

To fix this, you can pass None to create_or_update here and the next part will take care of inserting the email (and setting it to verified, which is important for the tests).

- retain email registered with when github email is modified

carols10cents

Just one question, thanks!

carols10cents · 2019-11-15T15:30:45Z

src/tests/user.rs


    // Simulate logging in via GitHub. Don't use app.db_new_user because it inserts a verified
    // email directly into the database and we want to test the verification flow here.
+    let email = "cow@mammals@milk";


I'm curious, why is the value different here? Any particular reason, and any reason why it's not a valid email address (two @s)?

mugoh · 2019-11-16T15:26:14Z

Resolved. The invalid email address was a mistake I hadn't noted

carols10cents · 2019-11-18T15:45:12Z

Thank you so much!! This looks great, but I'm not going to merge it yet-- we need to merge and deploy the fix for #1888 to remove all the uses of the email column first, so that there aren't any connections using the email field when we deploy this PR.

bors · 2019-11-22T14:06:02Z

☔ The latest upstream changes (presumably #1901) made this pull request unmergeable. Please resolve the merge conflicts.

@jtgeibel

Missed a conversion from UserNoEmailType to User When reviewing #1911, I missed a spot. I reverted the PR merge on master, so this PR reverts the revert and adds the fix. I tried to write a test that would have caught this, but I utterly failed at making a real-enough app/request that would have `.user()` but would let me set the `session()` :( Also a lot of this code is temporary and will be undone once I've rebased #1891, but this needs to be deployed separately from #1891 to ensure there aren't any uses of the email column. r? @jtgeibel

bors · 2019-12-02T16:45:45Z

☔ The latest upstream changes (presumably #1700) made this pull request unmergeable. Please resolve the merge conflicts.

carols10cents · 2019-12-02T16:48:31Z

I removed some more of the temporary code that was needed to deploy the code that stopped using the email column (and that has been deployed). I also introduced more merge conflicts by merging another PR just now; I'll be fixing those in a few hours.

carols10cents · 2019-12-02T18:38:53Z

I think this is ready to go now!! Thank you!!!

@bors r+

bors · 2019-12-02T18:38:54Z

📌 Commit c7ac5cb has been approved by carols10cents

Drop Email column (merge and deploy the fix for #1888 first!) #### Fixes #1889 This cleans up the uses of the field `email` from `User` by: - dropping the `email` field from `User` - clearing references to the dropped email field - migrating and updating the schema changes

bors · 2019-12-02T18:39:01Z

⌛ Testing commit c7ac5cb with merge dbe39ae...

bors · 2019-12-02T18:49:12Z

☀️ Test successful - checks-travis
Approved by: carols10cents
Pushing dbe39ae to master...

rust-highfive assigned carols10cents Nov 12, 2019

rust-highfive added the S-waiting-on-review label Nov 12, 2019

mugoh force-pushed the drop_email_column branch from 789892f to 521beb7 Compare November 12, 2019 04:40

mugoh added 2 commits November 12, 2019 07:50

remove uses of email field on User

521beb7

update tests to match altered fields

5ef002d

mugoh force-pushed the drop_email_column branch from 5be765d to afa3641 Compare November 13, 2019 13:31

mugoh force-pushed the drop_email_column branch from afa3641 to 3544b91 Compare November 13, 2019 14:09

remove email field from dump

3544b91

- `do_nothing` on `emails` insertion violates unique constraint on `user_id`

carols10cents requested changes Nov 14, 2019

View reviewed changes

carols10cents added S-waiting-on-author and removed S-waiting-on-review labels Nov 14, 2019

add requested changes

fc01ad4

mugoh force-pushed the drop_email_column branch from f74d1ff to 3be2b55 Compare November 15, 2019 12:37

mugoh requested a review from carols10cents November 15, 2019 12:37

retain original email

3be2b55

- retain email registered with when github email is modified

carols10cents requested changes Nov 15, 2019

View reviewed changes

use valid email pattern in test

59f7169

mugoh requested a review from carols10cents November 16, 2019 15:32

carols10cents mentioned this pull request Nov 18, 2019

Cleanup: remove email field on User in Rust code #1888

Closed

carols10cents added blocked and removed S-waiting-on-author labels Nov 18, 2019

carols10cents approved these changes Nov 18, 2019

View reviewed changes

carols10cents changed the title ~~Drop Email column~~ Drop Email column (merge and deploy #1888 first!) Nov 18, 2019

carols10cents changed the title ~~Drop Email column (merge and deploy #1888 first!)~~ Drop Email column (merge and deploy the fix for #1888 first!) Nov 18, 2019

carols10cents mentioned this pull request Nov 25, 2019

Missed a conversion from UserNoEmailType to User #1927

Merged

mugoh force-pushed the drop_email_column branch 8 times, most recently from f400318 to 8a8a1d3 Compare November 27, 2019 13:02

mugoh and others added 2 commits November 27, 2019 16:10

fix upstream conflicts

8a8a1d3

Remove more temporary code that was needed to remove user.email

2410f44

Merge remote-tracking branch 'upstream/master' into drop_email_column

c7ac5cb

bors merged commit c7ac5cb into rust-lang:master Dec 2, 2019

This was referenced Dec 2, 2019

Error response refactoring step 2 #1929

Merged

Newest updated #1931

Merged

		@@ -0,0 +1,2 @@
		-- Your SQL goes here
		ALTER TABLE users DROP COLUMN email CASCADE;

Drop Email column (merge and deploy the fix for #1888 first!) #1891

Drop Email column (merge and deploy the fix for #1888 first!) #1891

Uh oh!

Conversation

mugoh commented Nov 12, 2019

Fixes #1889

Uh oh!

rust-highfive commented Nov 12, 2019

Uh oh!

mugoh commented Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carols10cents commented Nov 13, 2019

Uh oh!

mugoh commented Nov 13, 2019

Uh oh!

carols10cents commented Nov 14, 2019

Uh oh!

carols10cents commented Nov 14, 2019

Uh oh!

carols10cents left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carols10cents left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mugoh commented Nov 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carols10cents commented Nov 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Nov 22, 2019

Uh oh!

bors commented Dec 2, 2019

Uh oh!

carols10cents commented Dec 2, 2019

Uh oh!

carols10cents commented Dec 2, 2019

Uh oh!

bors commented Dec 2, 2019

Uh oh!

bors commented Dec 2, 2019

Uh oh!

bors commented Dec 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mugoh commented Nov 12, 2019 •

edited

Loading

mugoh commented Nov 16, 2019 •

edited

Loading

carols10cents commented Nov 18, 2019 •

edited

Loading