Display errors during login #1689
Merged
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
The code was serializing an entire response object, which didn't actually contain the errors that came back from the API, so we'd never show anything other than "Failed to log in". With this change we now show the error returned by the API if it returned any. Notably, if a user tries to create a new account while we're in read only mode, they will now see an error saying that.
sgrif
added a commit
to sgrif/crates.io
that referenced
this pull request
Mar 21, 2019
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 e8f7bde has been approved by |
bors
added a commit
that referenced
this pull request
Mar 21, 2019
Display errors during login The code was serializing an entire response object, which didn't actually contain the errors that came back from the API, so we'd never show anything other than "Failed to log in". With this change we now show the error returned by the API if it returned any. Notably, if a user tries to create a new account while we're in read only mode, they will now see an error saying that.
|
☀️ Test successful - checks-travis |
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
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.
The code was serializing an entire response object, which didn't
actually contain the errors that came back from the API, so we'd never
show anything other than "Failed to log in". With this change we now
show the error returned by the API if it returned any. Notably, if a
user tries to create a new account while we're in read only mode, they
will now see an error saying that.