Skip to content

Commit a52e02e

Browse files
chrisbobbechrisbobbe
committed
login: Parse and validate realm-url input more like we do in zulip-mobile
For the corresponding logic in zulip-mobile, see src/start/RealmInputScreen.js. Between the URL parser we use here and the one we use in zulip-mobile, probably the set of rejected strings won't match exactly, but it'd be a surprising bug if either one rejects reasonable strings that we could've used. As in zulip-mobile, we distinguish validation errors we want to report immediately, from ones we'll wait to report until the user presses submit. (Compare this revision's `shouldDeferFeedback` to the first bit of zulip-mobile's `getSuggestion`.) Also add some tests, in an improvement over zulip-mobile. Fixes-partly: zulip#35
1 parent ce1f54d commit a52e02e

File tree

2 files changed

+161
-14
lines changed

2 files changed

+161
-14
lines changed

‎lib/widgets/login.dart

Lines changed: 117 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,86 @@ class _LoginSequenceRoute extends MaterialPageRoute<void> {
1616
});
1717
}
1818

19+
enum ServerUrlValidationError {
20+
empty,
21+
invalidUrl,
22+
noUseEmail,
23+
unsupportedSchemeZulip,
24+
unsupportedSchemeOther;
25+
26+
/// Whether to wait until the user presses "submit" to give error feedback.
27+
///
28+
/// True for errors that will often happen when the user just hasn't finished
29+
/// typing a good URL. False for errors that strongly signal a wrong path was
30+
/// taken, like when we recognize the form of an email address.
31+
bool shouldDeferFeedback() {
32+
switch (this) {
33+
case empty:
34+
case invalidUrl:
35+
return true;
36+
case noUseEmail:
37+
case unsupportedSchemeZulip:
38+
case unsupportedSchemeOther:
39+
return false;
40+
}
41+
}
42+
43+
String message() { // TODO(i18n)
44+
switch (this) {
45+
case empty:
46+
return 'Please enter a URL.';
47+
case invalidUrl:
48+
return 'Please enter a valid URL.';
49+
case noUseEmail:
50+
return 'Please enter the server URL, not your email.';
51+
case unsupportedSchemeZulip:
52+
case unsupportedSchemeOther:
53+
return 'The server URL must start with http:// or https://.';
54+
}
55+
}
56+
}
57+
58+
class ServerUrlParseResult {
59+
ServerUrlParseResult.ok(this.url) : error = null;
60+
ServerUrlParseResult.error(this.error) : url = null;
61+
62+
final Uri? url;
63+
final ServerUrlValidationError? error;
64+
}
65+
66+
class ServerUrlTextEditingController extends TextEditingController {
67+
ServerUrlParseResult tryParse() {
68+
final trimmedText = text.trim();
69+
70+
if (trimmedText.isEmpty) {
71+
return ServerUrlParseResult.error(ServerUrlValidationError.empty);
72+
}
73+
74+
Uri? url = Uri.tryParse(trimmedText);
75+
if (!RegExp(r'^https?://').hasMatch(trimmedText)) {
76+
if (url != null && url.scheme == 'zulip') {
77+
// Someone might get the idea to try one of the "zulip://" URLs that
78+
// are discussed sometimes.
79+
// TODO(log): Log to Sentry? How much does this happen, if at all? Maybe
80+
// log once when the input enters this error state, but don't spam
81+
// on every keystroke/render while it's in it.
82+
return ServerUrlParseResult.error(ServerUrlValidationError.unsupportedSchemeZulip);
83+
} else if (url != null && url.hasScheme && url.scheme != 'http' && url.scheme != 'https') {
84+
return ServerUrlParseResult.error(ServerUrlValidationError.unsupportedSchemeOther);
85+
}
86+
url = Uri.tryParse('https://$trimmedText');
87+
}
88+
89+
if (url == null || !url.isAbsolute) {
90+
return ServerUrlParseResult.error(ServerUrlValidationError.invalidUrl);
91+
}
92+
if (url.userInfo.isNotEmpty) {
93+
return ServerUrlParseResult.error(ServerUrlValidationError.noUseEmail);
94+
}
95+
return ServerUrlParseResult.ok(url);
96+
}
97+
}
98+
1999
class AddAccountPage extends StatefulWidget {
20100
const AddAccountPage({super.key});
21101

@@ -29,27 +109,40 @@ class AddAccountPage extends StatefulWidget {
29109
}
30110

31111
class _AddAccountPageState extends State<AddAccountPage> {
32-
final TextEditingController _controller = TextEditingController();
112+
final ServerUrlTextEditingController _controller = ServerUrlTextEditingController();
113+
late ServerUrlParseResult _parseResult;
114+
115+
_serverUrlChanged() {
116+
setState(() {
117+
_parseResult = _controller.tryParse();
118+
});
119+
}
120+
121+
@override
122+
void initState() {
123+
super.initState();
124+
_parseResult = _controller.tryParse();
125+
_controller.addListener(_serverUrlChanged);
126+
}
33127

34128
@override
35129
void dispose() {
36130
_controller.dispose();
37131
super.dispose();
38132
}
39133

40-
Future<void> _onSubmitted(BuildContext context, String value) async {
41-
final Uri? url = Uri.tryParse(value);
42-
switch (url) {
43-
case Uri(scheme: 'https' || 'http'):
44-
// TODO(#35): validate realm URL further?
45-
break;
46-
default:
47-
// TODO(#35): give feedback to user on bad realm URL
48-
return;
134+
Future<void> _onSubmitted(BuildContext context) async {
135+
final url = _parseResult.url;
136+
final error = _parseResult.error;
137+
if (error != null) {
138+
showErrorDialog(context: context,
139+
title: 'Invalid input', message: error.message());
140+
return;
49141
}
142+
assert(url != null);
50143

51144
// TODO(#35): show feedback that we're working, while fetching server settings
52-
final serverSettings = await getServerSettings(realmUrl: url);
145+
final serverSettings = await getServerSettings(realmUrl: url!);
53146
// https://github.com/dart-lang/linter/issues/4007
54147
// ignore: use_build_context_synchronously
55148
if (!context.mounted) {
@@ -64,6 +157,11 @@ class _AddAccountPageState extends State<AddAccountPage> {
64157
@override
65158
Widget build(BuildContext context) {
66159
assert(!PerAccountStoreWidget.debugExistsOf(context));
160+
final error = _parseResult.error;
161+
final errorText = error == null || error.shouldDeferFeedback()
162+
? null
163+
: error.message();
164+
67165
// TODO(#35): more help to user on entering realm URL
68166
return Scaffold(
69167
appBar: AppBar(title: const Text('Add an account')),
@@ -75,12 +173,17 @@ class _AddAccountPageState extends State<AddAccountPage> {
75173
child: Column(mainAxisAlignment: MainAxisAlignment.center, children: [
76174
TextField(
77175
controller: _controller,
78-
onSubmitted: (value) => _onSubmitted(context, value),
176+
onSubmitted: (value) => _onSubmitted(context),
79177
keyboardType: TextInputType.url,
80-
decoration: const InputDecoration(labelText: 'Your Zulip server URL')),
178+
decoration: InputDecoration(
179+
labelText: 'Your Zulip server URL',
180+
errorText: errorText,
181+
helperText: kLayoutPinningHelperText)),
81182
const SizedBox(height: 8),
82183
ElevatedButton(
83-
onPressed: () => _onSubmitted(context, _controller.text),
184+
onPressed: errorText == null
185+
? () => _onSubmitted(context)
186+
: null,
84187
child: const Text('Continue')),
85188
])))));
86189
}

‎test/widgets/login_test.dart

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
import 'package:checks/checks.dart';
2+
import 'package:flutter_test/flutter_test.dart';
3+
import 'package:zulip/widgets/login.dart';
4+
5+
void main() {
6+
group('ServerUrlTextEditingController.tryParse', () {
7+
final controller = ServerUrlTextEditingController();
8+
9+
expectUrlFromText(String text, String expectedUrl) {
10+
test('text "$text" gives URL "$expectedUrl"', () {
11+
controller.text = text;
12+
final result = controller.tryParse();
13+
check(result.error).isNull();
14+
check(result.url)
15+
.isNotNull() // catch `null` here instead of by its .toString()
16+
.has((url) => url.toString(), 'toString()').equals(expectedUrl);
17+
});
18+
}
19+
20+
expectErrorFromText(String text, ServerUrlValidationError expectedError) {
21+
test('text "$text" gives error "$expectedError"', () {
22+
controller.text = text;
23+
final result = controller.tryParse();
24+
check(result.url).isNull();
25+
check(result.error).equals(expectedError);
26+
});
27+
}
28+
29+
expectUrlFromText('https://chat.zulip.org', 'https://chat.zulip.org');
30+
expectUrlFromText('https://chat.zulip.org/', 'https://chat.zulip.org/');
31+
expectUrlFromText(' https://chat.zulip.org ', 'https://chat.zulip.org');
32+
expectUrlFromText('http://chat.zulip.org', 'http://chat.zulip.org');
33+
expectUrlFromText('chat.zulip.org', 'https://chat.zulip.org');
34+
expectUrlFromText('192.168.1.21:9991', 'https://192.168.1.21:9991');
35+
expectUrlFromText('http://192.168.1.21:9991', 'http://192.168.1.21:9991');
36+
37+
expectErrorFromText('', ServerUrlValidationError.empty);
38+
expectErrorFromText(' ', ServerUrlValidationError.empty);
39+
expectErrorFromText('zulip://foo', ServerUrlValidationError.unsupportedSchemeZulip);
40+
expectErrorFromText('ftp://foo', ServerUrlValidationError.unsupportedSchemeOther);
41+
expectErrorFromText('!@#*asd;l4fkj', ServerUrlValidationError.invalidUrl);
42+
expectErrorFromText('[email protected]', ServerUrlValidationError.noUseEmail);
43+
});
44+
}

0 commit comments

Comments
 (0)