Fix app crash due to invalid realm URL #2064
Conversation
|
Hello @anurag1018, it seems like you have referenced an issue in your pull request, but you have not referenced that issue in your commit message(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged. Please run An example of a correctly-formatted commit: Thank you for your contributions to Zulip! |
src/start/RealmScreen.js
Outdated
| @@ -6,6 +6,7 @@ import type { Actions } from '../types'; | |||
| import connectWithActions from '../connectWithActions'; | |||
| import { ErrorMsg, Label, SmartUrlInput, Screen, ZulipButton } from '../common'; | |||
| import { getServerSettings } from '../api'; | |||
| import { checkUrlValidity } from '../utils/url'; | |||
There was a problem hiding this comment.
When a function is expected to return boolean, we tend to name it in the form of isUserFound, doesFileExist etc. Here it will be isValidUrl
src/start/RealmScreen.js
Outdated
| @@ -83,6 +85,7 @@ class RealmScreen extends PureComponent<Props, State> { | |||
| text="Enter" | |||
| progress={progress} | |||
| onPress={this.tryRealm} | |||
| disabled={!isEnterEnabled} | |||
There was a problem hiding this comment.
We already check for errors in the tryRealm function.
You can do the check and set an error text.
That will be more clear to the user.
src/utils/url.js
Outdated
| @@ -156,3 +156,6 @@ export const autoCompleteUrl = ( | |||
| `${hasProtocol(value) ? '' : protocol}${value || 'your-org'}${ | |||
| value.indexOf('.') === -1 ? append : !value.match(/.+\..+\.+./g) ? shortAppend : '' | |||
| }`; | |||
|
|
|||
| export const checkUrlValidity = (url: string) => | |||
| /https:\/\/[a-zA-Z0-9-]+.[a-zA-Z0-9-]+.[a-zA-Z]+/.test(url) ? undefined : 'Invalid URL'; | |||
There was a problem hiding this comment.
While subdomains hosted on zulipchat.com might have this restriction on them that is unlikely the case for self-hosted apps that might have any domain.
There was a problem hiding this comment.
The fetch api on react-native android has this bug. So if we consider a case where an invalid url is passed, for example special characters are allowed in the subdomain, the app will crash as soon as the fetch is called irrespective of using exception handling.
Ref: facebook/react-native#7436
There was a problem hiding this comment.
This is potentially valid URL: 'hey.this.is.long.com' and it does not pass the Regex.
src/utils/url.js
Outdated
| @@ -156,3 +156,6 @@ export const autoCompleteUrl = ( | |||
| `${hasProtocol(value) ? '' : protocol}${value || 'your-org'}${ | |||
| value.indexOf('.') === -1 ? append : !value.match(/.+\..+\.+./g) ? shortAppend : '' | |||
| }`; | |||
|
|
|||
| export const checkUrlValidity = (url: string) => | |||
There was a problem hiding this comment.
It is better that you split the validity check and the error result. Thus this function should return true / false depending on if the url is valid.
There was a problem hiding this comment.
This function is a great candidate for a unit-test. You should demonstrate via a test both that it catches invalid urls and that it does not incorrectly catch valid urls.
|
Hello @anurag1018, it seems like you have referenced an issue in your pull request, but you have not referenced that issue in your commit message(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged. Please run An example of a correctly-formatted commit: Thank you for your contributions to Zulip! |
A direct match with the acceptable realm URL(comprising of letters, numbers and hyphens) fixes the issue. Displays an error for unacceptable URLs as well as disables the Enter button upon entering invalid URLs.
Fixes #2016