Skip to content

Do not write to users table email address columns#6474

Merged
mitchellhenke merged 6 commits intomainfrom
mitchellhenke/stop-saving-user-email-column
Jun 9, 2022
Merged

Do not write to users table email address columns#6474
mitchellhenke merged 6 commits intomainfrom
mitchellhenke/stop-saving-user-email-column

Conversation

@mitchellhenke
Copy link
Contributor

This has been retired for quite some time (almost 3 years), and it doesn't appear that we ever read email_fingerprint or encrypted_email_address from the users table since everything uses the email_addresses table

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/stop-saving-user-email-column branch from 6edfe68 to 7eb222d Compare June 7, 2022 22:04
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/stop-saving-user-email-column branch 2 times, most recently from 32637fc to ef61f62 Compare June 8, 2022 14:48
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this update all of the users email addresses with a confirmed at date, including unconfirmed ones? That may be a problem. A user could add an unconfirmed email, send a password reset to a confirmed email, then confirm all of their email addresses including the unconfirmed one by resetting their password.

I'm not sure what impact bumping the confirmed_at on the email address record has. Since we aren't doing it now it seems like it may be safe to leave off if we can't easily get to the email the reset password token was sent to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a part I'm trying to understand better, but I believe this is when you have an unconfirmed account, reset your password and then set the password, with the goal here being to set the account as confirmed.

This is stuff that would have previously been handled by the EmailAddressCallback here by setting confirmed_at on the user and setting the same value on the first email address.

The window is narrow, but I think we should probably only set it on the first email address (rather than all) like the callback and do all of this in a transaction to ensure things don't get weird in the time between checking the user has no confirmed emails in user.confirmed? and updating the confirmed_at.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh boy, yeah that is gnarly

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 switched to that in fd2c310

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/stop-saving-user-email-column branch 2 times, most recently from 5d77dae to 549b024 Compare June 8, 2022 18:38
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/stop-saving-user-email-column branch from 549b024 to e3035a3 Compare June 8, 2022 20:44
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM 🚢 🚀

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/stop-saving-user-email-column branch from 390c908 to 24dd67c Compare June 9, 2022 14:16
@mitchellhenke mitchellhenke marked this pull request as ready for review June 9, 2022 14:37
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/stop-saving-user-email-column branch from 24dd67c to e61b5ac Compare June 9, 2022 14:40
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/stop-saving-user-email-column branch from 4306d4b to 2d06320 Compare June 9, 2022 17:00
@mitchellhenke mitchellhenke merged commit fdb359a into main Jun 9, 2022
@mitchellhenke mitchellhenke deleted the mitchellhenke/stop-saving-user-email-column branch June 9, 2022 17:42
jmhooper added a commit that referenced this pull request Jun 16, 2022
…cept for callback

This commit reverts a commit that stops writing to the email address table, but still removes the callback. This is necessary for us to be able to deploy in a 50/50 state. We need to stop running the callback then we can drop the email address columns.

This reverts commit fdb359a.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants