Add helper text for "server URL" with link to documentation #595

VatsalBhesaniya · 2024-03-27T14:21:06Z

Add helper text below the "server URL" field that, when tapped, opens relevant Zulip documentation explaining what a server URL is and how to find theirs. It will help users understand the "server URL" field during login.

Fixes: #109

Screen.Recording.2024-03-29.at.10.53.04.AM.mov

Screen.Recording.2024-03-28.at.7.06.06.PM.mov

abelaba · 2024-03-27T18:56:06Z

Hey, so one thing I would say is that your commit message should have a description. As stated here
https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#commit-description

abelaba · 2024-03-27T19:00:50Z

You must also add new tests for your code so that maintainers will review your code in detail.
https://github.com/zulip/zulip-flutter?tab=readme-ov-file#writing-tests

chrisbobbe

Thanks @VatsalBhesaniya, I'm looking forward to this! This will help people get what they need to start using the app.

Small comments below. Also, this will need tests. Here are the things I would test:

Basic happy case: When the text is tapped, the URL is opened. For this, see our uses of testBinding.takeLaunchUrlCalls().
Error case: When the URL fails to be opened, we show an error dialog. For this, see our uses of testBinding.launchUrlResult and checkErrorDialog.

Also, please mark the commit with a Fixes line. I see you've already done this in the GitHub PR description. 🙂

lib/widgets/login.dart

assets/l10n/app_en.arb

lib/widgets/login.dart

VatsalBhesaniya · 2024-03-28T14:33:44Z

Hey, so one thing I would say is that your commit message should have a description. As stated here https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#commit-description

@abelaba Thank you for your feedback. 🙂

VatsalBhesaniya · 2024-03-28T14:38:54Z

Thanks @VatsalBhesaniya, I'm looking forward to this! This will help people get what they need to start using the app.

Small comments below. Also, this will need tests. Here are the things I would test:

Basic happy case: When the text is tapped, the URL is opened. For this, see our uses of testBinding.takeLaunchUrlCalls().

Error case: When the URL fails to be opened, we show an error dialog. For this, see our uses of testBinding.launchUrlResult and checkErrorDialog.

Also, please mark the commit with a Fixes line. I see you've already done this in the GitHub PR description. 🙂

Thanks for the positive feedback! 🙂
I appreciate your suggestions on tests. I've added tests covering the success and the error cases.

I've also updated the commit message to include the Fixes line.

Please let me know if you have any further feedback or require any additional changes to the tests.

chrisbobbe · 2024-03-28T19:49:45Z

Hi! One small comment I meant to leave last time, and I think Greg intends to do the next full review. 🙂

There's still a TODO(#109) in the code that should be removed in the commit that's marked as fixing #109. I've proposed an automated way to catch things like this:

test: If a new commit on the current branch has a Fixes: #… line, check codebase for TODO(#…)s #599

gnprice

Thanks @VatsalBhesaniya for adding this (and for the upstream PR flutter/flutter#145157 that added this helper field!) See comments below.

lib/widgets/login.dart

test/widgets/login_test.dart

assets/l10n/app_en.arb

gnprice

Thanks @VatsalBhesaniya for the revision!

In this version, the first commit introduces a duplicate _launchUrl function which is missing the mode logic, much like in the previous revision as discussed at #595 (comment) . Then the second commit deduplicates the two functions into one combined place.

As part of writing clear and coherent commits, we want to avoid that intermediate step where we're adding a _launchUrl function that has the wrong behavior. So instead a good commit sequence would be:

Move the existing _launchUrl to the new file, renaming it, perhaps splitting into pieces, but maintaining the exact same behavior. This will be an NFC commit.
Add i18n for that function.
Add the UI that resolves login: Link to doc to help users understand what a "server URL" is and how to find theirs #109, calling that function.

After that's done it'll again be possible to do a fine-grained review. In the meantime, here's a couple of comments from a high-level look.

lib/widgets/launch_url.dart

VatsalBhesaniya · 2024-04-02T12:49:55Z

Thanks @VatsalBhesaniya for the revision!

In this version, the first commit introduces a duplicate _launchUrl function which is missing the mode logic, much like in the previous revision as discussed at #595 (comment) . Then the second commit deduplicates the two functions into one combined place.

As part of writing clear and coherent commits, we want to avoid that intermediate step where we're adding a _launchUrl function that has the wrong behavior. So instead a good commit sequence would be:

Move the existing _launchUrl to the new file, renaming it, perhaps splitting into pieces, but maintaining the exact same behavior. This will be an NFC commit.

Add i18n for that function.

Add the UI that resolves login: Link to doc to help users understand what a "server URL" is and how to find theirs #109, calling that function.

After that's done it'll again be possible to do a fine-grained review. In the meantime, here's a couple of comments from a high-level look.

Thank you for your detailed feedback and guidance!

I have followed your suggested approach and updated the commits accordingly. Let me know if you have any further suggestions.

chrisbobbe

Thanks @VatsalBhesaniya! Small comments below.

Also, please wrap the body text of each commit message; some lines have gotten too long: https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#formatting-guidelines

lib/widgets/launch_url.dart

assets/l10n/app_en.arb

lib/widgets/launch_url.dart

lib/widgets/login.dart

test/widgets/login_test.dart

lib/widgets/content.dart

assets/l10n/app_en.arb

- Move the existing launchUrl logic from content.dart to a new file named launch_url.dart. - Split function into pieces to handle realm-based and non-realm URLs. - Refactor error handling into a private function.

Fixes: zulip#109

chrisbobbe · 2024-04-10T19:06:33Z

Hi @VatsalBhesaniya, it looks like you've updated the PR branch. Is it ready for another review? When you push updates and it's ready for another review, it's helpful if you post a comment saying so, for explicitness; that way reviewers don't have to guess. 🙂

VatsalBhesaniya · 2024-04-11T00:43:38Z

Hi @VatsalBhesaniya, it looks like you've updated the PR branch. Is it ready for another review? When you push updates and it's ready for another review, it's helpful if you post a comment saying so, for explicitness; that way reviewers don't have to guess. 🙂

Hi @chrisbobbe, Yes, the PR branch is ready for another review! I apologize for the confusion. I made updates locally, but for some reason, they weren't reflecting on GitHub 🤔. This hasn't happened to me before, so I hesitated to request another review until I was certain everything was pushed correctly.
Now I can see all of my updates. 🙂

chrisbobbe

Thanks @VatsalBhesaniya! This is very close. Small comments below, and also #595 (comment) (which doesn't appear below for some reason).

lib/widgets/login.dart

chrisbobbe · 2024-04-26T20:52:57Z

lib/widgets/launch_url.dart

+
+  final internalNarrow = parseInternalLink(url, store);
+  if (internalNarrow != null) {
+    Navigator.push(context, MessageListPage.buildRoute(context: context, narrow: internalNarrow));


Suggested change

Navigator.push(context, MessageListPage.buildRoute(context: context, narrow: internalNarrow));

Navigator.push(context,

MessageListPage.buildRoute(context: context, narrow: internalNarrow));

nit: break very long line with important information at the end

chrisbobbe · 2024-04-26T20:56:26Z

test/widgets/login_test.dart

@@ -61,7 +64,48 @@ void main() {
    expectErrorFromText('email@example.com', ServerUrlValidationError.noUseEmail);
  });

-  // TODO test AddAccountPage
+  group('AddAccountPage Server URL Helper Text', () {


Suggested change

group('AddAccountPage Server URL Helper Text', () {

group('AddAccountPage server URL helper text', () {

nit: match capitalization to other groups

chrisbobbe · 2024-04-26T21:02:47Z

test/widgets/login_test.dart

+      final helper = await findHelperText(tester);
+      await tester.tap(helper);


Suggested change

final helper = await findHelperText(tester);

await tester.tap(helper);

await tester.tap(await findHelperText(tester));

nit: could save a line by inlining this expression (here and in the other test)

chrisbobbe · 2024-04-26T21:04:12Z

test/widgets/login_test.dart

+      await tester.tap(find.byWidget(checkErrorDialog(
+        tester,
+        expectedTitle: zulipLocalizations.errorUnableToOpenLinkTitle,
+        expectedMessage: zulipLocalizations.errorUnableToOpenLinkMessage(
+          AddAccountPage.serverUrlHelpUrl.toString(),
+        ),
+      )));


Suggested change

await tester.tap(find.byWidget(checkErrorDialog(

tester,

expectedTitle: zulipLocalizations.errorUnableToOpenLinkTitle,

expectedMessage: zulipLocalizations.errorUnableToOpenLinkMessage(

AddAccountPage.serverUrlHelpUrl.toString(),

),

)));

await tester.tap(find.byWidget(checkErrorDialog(tester,

expectedTitle: zulipLocalizations.errorUnableToOpenLinkTitle,

expectedMessage: zulipLocalizations.errorUnableToOpenLinkMessage(

AddAccountPage.serverUrlHelpUrl.toString(),

))));

nit: could be denser here, using fewer lines

chrisbobbe requested changes Mar 27, 2024

View reviewed changes

lib/widgets/login.dart Outdated Show resolved Hide resolved

assets/l10n/app_en.arb Outdated Show resolved Hide resolved

lib/widgets/login.dart Outdated Show resolved Hide resolved

lib/widgets/login.dart Show resolved Hide resolved

lib/widgets/login.dart Outdated Show resolved Hide resolved

VatsalBhesaniya force-pushed the login-server-url-doc-link branch 4 times, most recently from 130093f to 3d0f90a Compare March 28, 2024 13:25

VatsalBhesaniya requested a review from chrisbobbe March 28, 2024 14:44

gnprice reviewed Mar 28, 2024

View reviewed changes

VatsalBhesaniya force-pushed the login-server-url-doc-link branch 12 times, most recently from 3585ecd to 98f2e2f Compare March 29, 2024 07:46

VatsalBhesaniya requested a review from gnprice March 29, 2024 08:46

gnprice reviewed Apr 1, 2024

View reviewed changes

lib/widgets/launch_url.dart Outdated Show resolved Hide resolved

lib/widgets/launch_url.dart Outdated Show resolved Hide resolved

VatsalBhesaniya force-pushed the login-server-url-doc-link branch 4 times, most recently from fbbcc13 to a683136 Compare April 2, 2024 11:57

VatsalBhesaniya requested a review from gnprice April 2, 2024 13:12

VatsalBhesaniya force-pushed the login-server-url-doc-link branch from a683136 to 5f8ff32 Compare April 6, 2024 13:10

chrisbobbe reviewed Apr 10, 2024

View reviewed changes

VatsalBhesaniya force-pushed the login-server-url-doc-link branch 9 times, most recently from 1f6de5b to f5efcb1 Compare April 10, 2024 10:36

VatsalBhesaniya added 3 commits April 10, 2024 16:15

launch_url [nfc]: Move launchUrl logic to a new file

f160282

- Move the existing launchUrl logic from content.dart to a new file named launch_url.dart. - Split function into pieces to handle realm-based and non-realm URLs. - Refactor error handling into a private function.

launch_url: Add i18n for launchUrl error messages

870b438

login: Link to doc for what "server URL" is and how to find it

e003a66

Fixes: zulip#109

VatsalBhesaniya force-pushed the login-server-url-doc-link branch from f5efcb1 to e003a66 Compare April 10, 2024 10:49

VatsalBhesaniya requested a review from chrisbobbe April 11, 2024 00:43

chrisbobbe reviewed Apr 26, 2024

View reviewed changes

gnprice added the completion candidate PR with reviews that may unblock merging label May 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add helper text for "server URL" with link to documentation #595

Add helper text for "server URL" with link to documentation #595

VatsalBhesaniya commented Mar 27, 2024 •

edited

abelaba commented Mar 27, 2024 •

edited

abelaba commented Mar 27, 2024 •

edited

chrisbobbe left a comment

VatsalBhesaniya commented Mar 28, 2024

VatsalBhesaniya commented Mar 28, 2024 •

edited

chrisbobbe commented Mar 28, 2024

gnprice left a comment

gnprice left a comment

VatsalBhesaniya commented Apr 2, 2024

chrisbobbe left a comment

chrisbobbe commented Apr 10, 2024 •

edited

VatsalBhesaniya commented Apr 11, 2024 •

edited

chrisbobbe left a comment •

edited

chrisbobbe Apr 26, 2024

chrisbobbe Apr 26, 2024

chrisbobbe Apr 26, 2024

chrisbobbe Apr 26, 2024

	Navigator.push(context, MessageListPage.buildRoute(context: context, narrow: internalNarrow));
	Navigator.push(context,
	MessageListPage.buildRoute(context: context, narrow: internalNarrow));

	group('AddAccountPage Server URL Helper Text', () {
	group('AddAccountPage server URL helper text', () {

		final helper = await findHelperText(tester);
		await tester.tap(helper);

	final helper = await findHelperText(tester);
	await tester.tap(helper);
	await tester.tap(await findHelperText(tester));

Add helper text for "server URL" with link to documentation #595

Are you sure you want to change the base?

Add helper text for "server URL" with link to documentation #595

Conversation

VatsalBhesaniya commented Mar 27, 2024 • edited

abelaba commented Mar 27, 2024 • edited

abelaba commented Mar 27, 2024 • edited

chrisbobbe left a comment

Choose a reason for hiding this comment

VatsalBhesaniya commented Mar 28, 2024

VatsalBhesaniya commented Mar 28, 2024 • edited

chrisbobbe commented Mar 28, 2024

gnprice left a comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

VatsalBhesaniya commented Apr 2, 2024

chrisbobbe left a comment

Choose a reason for hiding this comment

chrisbobbe commented Apr 10, 2024 • edited

VatsalBhesaniya commented Apr 11, 2024 • edited

chrisbobbe left a comment • edited

Choose a reason for hiding this comment

chrisbobbe Apr 26, 2024

Choose a reason for hiding this comment

chrisbobbe Apr 26, 2024

Choose a reason for hiding this comment

chrisbobbe Apr 26, 2024

Choose a reason for hiding this comment

chrisbobbe Apr 26, 2024

Choose a reason for hiding this comment

VatsalBhesaniya commented Mar 27, 2024 •

edited

abelaba commented Mar 27, 2024 •

edited

abelaba commented Mar 27, 2024 •

edited

VatsalBhesaniya commented Mar 28, 2024 •

edited

chrisbobbe commented Apr 10, 2024 •

edited

VatsalBhesaniya commented Apr 11, 2024 •

edited

chrisbobbe left a comment •

edited