realm screen: Give early error when server URL is an email address. #4181
Conversation
src/start/RealmScreen.js
Outdated
| @@ -53,6 +54,15 @@ class RealmScreen extends PureComponent<Props, State> { | |||
|
|
|||
| const { dispatch } = this.props; | |||
|
|
|||
| if (new WhatwgURL(realm).username !== '') { | |||
There was a problem hiding this comment.
new WhatwgURL(realm) will throw an error if realm is an invalid URL: https://developer.mozilla.org/en-US/docs/Web/API/URL/URL
There was a problem hiding this comment.
I just tested this by hardcoding something wrong for 'realm'. I got this:
Possible Unhandled Promise Rejection (id: 0):
TypeError: Invalid URL: asdf
I don't get a white-screen crash (the error was thrown from a React event handler), but the spinner does go on indefinitely.
I think the solution is to isolate new WhatwgUrl(realm) into its own try/catch block, and, on this error, do a setState similar to this one, with an appropriate error message saying it's an invalid URL.
Then the email can be checked below that.
There was a problem hiding this comment.
Ah! Initially, I did add a try-catch for the URL constructor. But then I saw that we already validate the URL in the login button using a regular expression - but I never considered that the form could be submitted by simply tapping the enter key from the keyboard, without needing to touch the button.
src/start/RealmScreen.js
Outdated
| this.setState({ | ||
| progress: false, | ||
| error: | ||
| 'Please enter the server URL, not your email. You can enter your ceredentials in the next screen.', |
There was a problem hiding this comment.
Nit: "credentials" (also in messages_en.json)
|
Thanks for the review. I've made the suggested changes. |
src/start/RealmScreen.js
Outdated
|
|
||
| const { dispatch } = this.props; | ||
|
|
||
| let parsedRealm; | ||
| try { | ||
| parsedRealm = new WhatwgURL(realm); | ||
| } catch (err) { | ||
| this.setState({ | ||
| progress: false, | ||
| error: 'Please enter a valid URL', | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (parsedRealm.username !== '') { | ||
| this.setState({ | ||
| progress: false, | ||
| error: | ||
| 'Please enter the server URL, not your email. You can enter your credentials in the next screen.', | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const serverSettings: ApiResponseServerSettings = await api.getServerSettings(realm); | ||
| dispatch(realmAdd(realm, new ZulipVersion(serverSettings.zulip_version))); |
There was a problem hiding this comment.
This function gets somewhat difficult to follow with all the stuff going on in it now. I think one reason is that the setState at the top is setting up the start of a story you basically have to read the whole function to see the different endings of.
One thing that would simplify it is to not set progress: true until after the realm-parsing is done and those errors haven't happened. That way those don't need to say progress: false, and the whole question of the progress state is limited to just where we're actually contacting the server, which is what it's for.
Relatedly, we can cut the line already that sets realm in that first setState. It's a no-op because we just read realm from the state. Looks like it's a legacy of an unfinished refactor in 252fed2 -- before then, this line actually set realm to a different value than it already had, and so was doing something useful.
Similarly to moving the setState but smaller: the const { dispatch } = … line can wait until just above the try.
There was a problem hiding this comment.
Okay, also, I moved the error:null to just above the try block too.
|
Thanks for the review, @gnprice. I've pushed some changes. |
The 'ErrorMsg' component currently has a 'height' style property, set to 'CONTROL_SIZE / 2'. However, because of this, longer error messages do not wrap and get hidden. So, remove this property.
We just got `realm` from the state a couple of lines ago, so setting it back to the same value here has no useful effect. It's confusing, so remove it. This appears to be the legacy of an unfinished refactor a couple of years ago in 252fed2. Before that, this was the place where we invoked `fixRealm` (now `fixRealmUrl`) and set the `realm` state property accordingly. With that change, that responsibility moved to within the new `SmartUrlInput`, and this line was edited into being a no-op.
Show an error message when the user tries to enter an email in the server URL input field, even before trying to connect to the server. Fixes: zulip#4058
|
Thanks @agrawal-d ! Merged. I made one change first, inserting a commit to remove |
Show an error message when the user tries to enter an email in the
server URL input field, even before trying to connect to the
server.
Fixes: #4058