Allow existing users to sign in while READ_ONLY_MODE=1 #1690
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently whenever a user signs in we attempt to update their profile (display name, avatar, email, etc). If we're in read only mode, we can't do that. However, preventing them from signing in entirely seems unfortunate. If we get a read-only error, we will now try to look for an existing user, returning that user if found (or any errors that occurred loading it), and continue to return `ReadOnlyMode` if we couldn't find an existing user. A user will have to sign out and back in again after maintenance has completed if they want to update their information. Ideally we'd display a warning informing them of this fact, but we don't currently have code in place to support that, and with < 3 hours remaining until maintenance time, there's not enough time to write it. I'd also love to add some explicit tests for this, but none of our existing tests handle GitHub oauth, and again, there's not enough time for me to figure out what our recordings need to look like for that. If we're not comfortable shipping this in this imperfect state (no warning, no tests), then we should just close this, and folks won't be able to log in during the maintenance period. That's not the end of the world. However, I've tested this locally to make sure it behaves properly (with rust-lang#1689 as well to make sure we display the right error), so I'd like to ship this to minimize disruption during the DB maintenance
|
@bors r+ |
|
📌 Commit 27c5236 has been approved by |
bors
added a commit
that referenced
this pull request
Mar 21, 2019
Allow existing users to sign in while READ_ONLY_MODE=1 Currently whenever a user signs in we attempt to update their profile (display name, avatar, email, etc). If we're in read only mode, we can't do that. However, preventing them from signing in entirely seems unfortunate. If we get a read-only error, we will now try to look for an existing user, returning that user if found (or any errors that occurred loading it), and continue to return `ReadOnlyMode` if we couldn't find an existing user. A user will have to sign out and back in again after maintenance has completed if they want to update their information. Ideally we'd display a warning informing them of this fact, but we don't currently have code in place to support that, and with < 3 hours remaining until maintenance time, there's not enough time to write it. I'd also love to add some explicit tests for this, but none of our existing tests handle GitHub oauth, and again, there's not enough time for me to figure out what our recordings need to look like for that. If we're not comfortable shipping this in this imperfect state (no warning, no tests), then we should just close this, and folks won't be able to log in during the maintenance period. That's not the end of the world. However, I've tested this locally to make sure it behaves properly (with #1689 as well to make sure we display the right error), so I'd like to ship this to minimize disruption during the DB maintenance
|
☀️ Test successful - checks-travis |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently whenever a user signs in we attempt to update their profile
(display name, avatar, email, etc). If we're in read only mode, we can't
do that. However, preventing them from signing in entirely seems
unfortunate.
If we get a read-only error, we will now try to look for an existing
user, returning that user if found (or any errors that occurred loading
it), and continue to return
ReadOnlyModeif we couldn't find anexisting user.
A user will have to sign out and back in again after maintenance has
completed if they want to update their information. Ideally we'd display
a warning informing them of this fact, but we don't currently have code
in place to support that, and with < 3 hours remaining until maintenance
time, there's not enough time to write it.
I'd also love to add some explicit tests for this, but none of our
existing tests handle GitHub oauth, and again, there's not enough time
for me to figure out what our recordings need to look like for that.
If we're not comfortable shipping this in this imperfect state (no
warning, no tests), then we should just close this, and folks won't be
able to log in during the maintenance period. That's not the end of the
world. However, I've tested this locally to make sure it behaves
properly (with #1689 as well to make sure we display the right error),
so I'd like to ship this to minimize disruption during the DB
maintenance