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

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

lib/widgets/launch_url.dart

test/widgets/login_test.dart

chrisbobbe · 2024-05-22T23:15:36Z

Hi @VatsalBhesaniya, are you planning to return to this?

- 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

VatsalBhesaniya · 2024-05-23T04:23:34Z

Hi @VatsalBhesaniya, are you planning to return to this?

Sorry for the delay. I have made all the changes requested.

chrisbobbe · 2024-06-05T23:08:56Z

Great, thank you! This revision LGTM; I'll mark this for Greg's review now.

alya · 2024-06-07T20:31:23Z

I don't understand the "Unable to open link" screenshots in the PR description. Is that something users will see?

gnprice

Thanks @VatsalBhesaniya, and thanks @chrisbobbe for the previous revisions! Code comments below.

Echoing Alya's question: what are the circumstances where you'd expect a user to see that error dialog?

In addition to the two screen recordings, which are helpful for the interaction of opening the link: I think a very helpful thing to include as a screenshot would be just this add-account screen itself. That'd help for being able to look at the new helper text and gauge how it looks. (A video is less good for that because it's hard to get it in full resolution.)

gnprice · 2024-06-08T00:52:00Z

lib/widgets/launch_url.dart

+/// Launches a URL without considering a realm base URL.
+void launchUrlWithoutRealm(BuildContext context, Uri url) async {


From the name, I can't really tell what this does; I don't know what "without realm" means.

I think what this function is really about in contrast with launchUrlWithRealm is that it requires the URL to be already resolved to an absolute URL — it won't accept a relative link. So we can rename it accordingly.

We can also revise the doc to be more specific about what "launch" means, following the upstream launchUrl doc; and add an assert checking that expectation of an absolute URL:

Suggested change

/// Launches a URL without considering a realm base URL.

void launchUrlWithoutRealm(BuildContext context, Uri url) async {

/// Pass a URL to the underlying platform for handling.

///

/// [url] must be an absolute URL.

/// For possibly-relative URLs, consider [launchUrlWithRealm].

void launchAbsoluteUrl(BuildContext context, Uri url) async {

assert(url.hasScheme);

gnprice · 2024-06-08T00:55:38Z

lib/widgets/launch_url.dart

+}
+
+/// Launches a URL considering a realm base URL.
+void launchUrlWithRealm(BuildContext context, String urlString) async {


Similarly I don't feel like "with realm" quite makes clear what the relationship to the realm is. I think launchUrlWithRealmBase would do it.

gnprice · 2024-06-08T00:57:02Z

lib/widgets/launch_url.dart

+/// Handles showing an error dialog with a customizable message.
+Future<void> _showError(BuildContext context, String? message, String urlString) {


This doc doesn't really add anything beyond the function's name and parameters; so better to leave it out.

See:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#avoid-useless-documentation

gnprice · 2024-06-08T00:59:10Z

lib/widgets/content.dart

-  } on PlatformException catch (e) {
-    errorMessage = e.message;
-  }
-  if (!launched) { // TODO(log)


This // TODO(log) got dropped. Was that intended?

gnprice · 2024-06-08T01:01:30Z

assets/l10n/app_en.arb

+  "@errorUnableToOpenLinkMessage": {
+    "description": "Error message when a link fails to open.",
+    "placeholders": {
+      "url": {"type": "String", "example": "http://example.com/"}


nit: even in examples, let's stick to HTTPS everywhere:

Suggested change

"url": {"type": "String", "example": "http://example.com/"}

"url": {"type": "String", "example": "https://example.com/"}

gnprice · 2024-06-08T01:06:31Z

test/widgets/login_test.dart

@@ -61,7 +64,44 @@ void main() {
    expectErrorFromText('[email protected]', ServerUrlValidationError.noUseEmail);
  });

-  // TODO test AddAccountPage


There's lots more to test on this page 🙂 — so this TODO is still relevant and let's keep it.

gnprice · 2024-06-08T01:08:57Z

test/widgets/login_test.dart

+      ));
+    }
+
+    final zulipLocalizations = GlobalLocalizations.zulipLocalizations;


This GlobalLocalizations.zulipLocalizations is application state that can change. So reading it should happen within individual test cases (including helpers they call), i.e. individual testWidgets callbacks, not outside them.

gnprice · 2024-06-08T01:17:05Z

test/widgets/login_test.dart

+
+    testWidgets('shows error dialog when URL fails to open', (tester) async {
+      await prepareAddAccountPage(tester);
+      testBinding.launchUrlResult = false;


When a test manipulates testBinding, it needs to call addTearDown(testBinding.reset) somewhere so that the state gets reset before the next test.

(In the "error dialog if invalid link" in widgets/content_test.dart which this one is presumably based on, that gets taken care of by the prepare helper, because the latter does other things with testBinding that also need a reset.)

You can reproduce the problem with the command:

$ flutter test test/widgets/login_test.dart --test-randomize-ordering-seed=random

At your current revision, that often fails because launchUrlResult remains set to false when the other new test tries to run. After you fix the problem with a reset, that command should pass every time.

PIG208 · 2024-10-03T18:28:54Z

Hi @VatsalBhesaniya! Thanks for the previous revisions. Any plan to continue working on this?

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 force-pushed the login-server-url-doc-link branch 10 times, most recently 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

VatsalBhesaniya force-pushed the login-server-url-doc-link branch 3 times, most recently from f1adf84 to 54ce074 Compare May 23, 2024 03:38

VatsalBhesaniya added 3 commits May 23, 2024 09:17

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

ed81d05

- 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

5a0a178

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

560a4e0

Fixes: zulip#109

VatsalBhesaniya force-pushed the login-server-url-doc-link branch from 54ce074 to 560a4e0 Compare May 23, 2024 03:47

VatsalBhesaniya requested a review from chrisbobbe May 23, 2024 04:19

chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Jun 5, 2024

gnprice reviewed Jun 8, 2024

View reviewed changes

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

Loading

abelaba commented Mar 27, 2024 •

edited

Loading

abelaba commented Mar 27, 2024 •

edited

Loading

chrisbobbe left a comment

VatsalBhesaniya commented Mar 28, 2024

VatsalBhesaniya commented Mar 28, 2024 •

edited

Loading

chrisbobbe commented Mar 28, 2024

gnprice left a comment

gnprice left a comment

chrisbobbe commented Apr 10, 2024 •

edited

Loading

VatsalBhesaniya commented Apr 11, 2024 •

edited

Loading

chrisbobbe left a comment •

edited

Loading

chrisbobbe commented May 22, 2024

VatsalBhesaniya commented May 23, 2024

chrisbobbe commented Jun 5, 2024

alya commented Jun 7, 2024

gnprice left a comment

gnprice Jun 8, 2024

gnprice Jun 8, 2024

gnprice Jun 8, 2024

gnprice Jun 8, 2024

gnprice Jun 8, 2024

gnprice Jun 8, 2024

gnprice Jun 8, 2024

gnprice Jun 8, 2024

PIG208 commented Oct 3, 2024

		/// Launches a URL without considering a realm base URL.
		void launchUrlWithoutRealm(BuildContext context, Uri url) async {

-/// Launches a URL without considering a realm base URL.
-void launchUrlWithoutRealm(BuildContext context, Uri url) async {
+/// Pass a URL to the underlying platform for handling.
+///
+/// [url] must be an absolute URL.
+/// For possibly-relative URLs, consider [launchUrlWithRealm].
+void launchAbsoluteUrl(BuildContext context, Uri url) async {
+  assert(url.hasScheme);

		/// Handles showing an error dialog with a customizable message.
		Future<void> _showError(BuildContext context, String? message, String urlString) {

	"url": {"type": "String", "example": "http://example.com/"}
	"url": {"type": "String", "example": "https://example.com/"}

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 Loading

abelaba commented Mar 27, 2024 • edited Loading

abelaba commented Mar 27, 2024 • edited Loading

chrisbobbe left a comment

Choose a reason for hiding this comment

VatsalBhesaniya commented Mar 28, 2024

VatsalBhesaniya commented Mar 28, 2024 • edited Loading

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

chrisbobbe commented Apr 10, 2024 • edited Loading

VatsalBhesaniya commented Apr 11, 2024 • edited Loading

chrisbobbe left a comment • edited Loading

Choose a reason for hiding this comment

chrisbobbe commented May 22, 2024

VatsalBhesaniya commented May 23, 2024

chrisbobbe commented Jun 5, 2024

alya commented Jun 7, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Oct 3, 2024

VatsalBhesaniya commented Mar 27, 2024 •

edited

Loading

abelaba commented Mar 27, 2024 •

edited

Loading

abelaba commented Mar 27, 2024 •

edited

Loading

VatsalBhesaniya commented Mar 28, 2024 •

edited

Loading

chrisbobbe commented Apr 10, 2024 •

edited

Loading

VatsalBhesaniya commented Apr 11, 2024 •

edited

Loading

chrisbobbe left a comment •

edited

Loading