login: Reject earlier (at get-server-settings) when server is too old #1783

chrisbobbe · 2025-08-04T23:26:56Z

Stacked atop #1782, which is an improvement to the tests that I found while working this (actually while pairing with @sm-sayedi 🙂) plus a fix for something we overlooked in #1017.

There's a planned change to make zulipMergeBase required in
GetServerSettingsResult (see #904). When we do that, we'd otherwise
just treat a missing zulipMergeBase on an ancient server the same
way we treat any malformed response. Better to check if the server
is an ancient one we don't support, just like when the /register
response is malformed.

gnprice

Thanks for taking care of this! Looks good; small comments.

gnprice · 2025-08-06T22:55:23Z

lib/model/server_support.dart

+import '../api/core.dart';
+import '../api/exception.dart';
+import '../api/model/initial_snapshot.dart';
+import '../api/route/realm.dart';


nit: third commit removes this line; second commit could omit adding it

gnprice · 2025-08-06T22:56:27Z

test/widgets/login_test.dart

+      connection.prepare(json: serverSettings.toJson());
+      await tester.tap(find.text('Continue'));
+      await tester.pump(Duration.zero);
+      checkNoDialog(tester);


third commit has an error at this line

gnprice · 2025-08-06T23:03:37Z

test/widgets/login_test.dart

+      await tester.pump((pushedRoute as TransitionRoute).transitionDuration);
+      await tester.pump();


Would this do the job?

Suggested change

await tester.pump((pushedRoute as TransitionRoute).transitionDuration);

await tester.pump();

await testNavObserver.pumpPastTransition(tester);

(Then also pushedRoute above can be inlined.)

Oh, TIL we made TestNavigatorObserver extend TransitionDurationObserver :) in d7fac5d

gnprice · 2025-08-06T23:05:58Z

test/widgets/login_test.dart

+      testBinding.globalStore.useCachedApiConnections = true;
+      connection = testBinding.globalStore.apiConnection(
+        realmUrl: serverSettings.realmUrl,
+        zulipFeatureLevel: null);
+
+      await tester.enterText(find.byType(TextField),
+        serverSettings.realmUrl.toString());
+
+      connection.prepare(json: serverSettings.toJson());


I feel like the making an API connection (to go into the global cache), and then preparing a response, go logically together and separate from the entering text into the input.

So like:

Suggested change

testBinding.globalStore.useCachedApiConnections = true;

connection = testBinding.globalStore.apiConnection(

realmUrl: serverSettings.realmUrl,

zulipFeatureLevel: null);

await tester.enterText(find.byType(TextField),

serverSettings.realmUrl.toString());

connection.prepare(json: serverSettings.toJson());

await tester.enterText(find.byType(TextField),

serverSettings.realmUrl.toString());

testBinding.globalStore.useCachedApiConnections = true;

connection = testBinding.globalStore.apiConnection(

realmUrl: serverSettings.realmUrl,

zulipFeatureLevel: null);

connection.prepare(json: serverSettings.toJson());

Looks like this slipped through CI, introduced recently in 8d3c128.

…ignore

…file

chrisbobbe · 2025-08-06T23:49:03Z

Thanks, revision pushed! PTAL.

gnprice

Thanks! One remaining comment.

gnprice · 2025-08-06T23:55:24Z

test/widgets/login_test.dart

+      testBinding.globalStore.useCachedApiConnections = true;
+      connection = testBinding.globalStore.apiConnection(
+        realmUrl: serverSettings.realmUrl,
+        zulipFeatureLevel: null);
+
+      await tester.enterText(find.byType(TextField),
+        serverSettings.realmUrl.toString());
+
+      connection.prepare(json: serverSettings.toJson());


bump #1783 (comment) 🙂

Also I suspect this group of tests would be clarified by a little helper. Something like:

await attempt(tester, serverSettings.realmUrl, serverSettings.toJson());

which would cover all these lines and the tap/pump below, and leave the caller to finish up with just the check steps.

gnprice · 2025-08-06T23:57:37Z

test/widgets/dialog_test.dart

@@ -7,7 +7,6 @@ import 'package:url_launcher/url_launcher.dart';
 import 'package:zulip/model/settings.dart';
 import 'package:zulip/widgets/app.dart';
 import 'package:zulip/widgets/dialog.dart';
-import 'package:zulip/widgets/store.dart';


dialog test [nfc]: Remove an unused import Looks like this slipped through CI, introduced recently in 8d3c1284e.

Ah, oops. I guess that changed in your revision of #1782, and I merged before CI finished running.

… In fact I'm pretty sure I ran flutter test on this very file before merging, because I saw CI wasn't done. But I guess I should have checked the analyzer output too.

chrisbobbe · 2025-08-07T00:26:12Z

Thanks, revision pushed!

gnprice

Thanks! Just nits below; otherwise all looks good.

gnprice · 2025-08-07T00:32:28Z

test/widgets/login_test.dart

+      await testNavObserver.pumpPastTransition(tester);
+    }
+
+    Future<void> attempt(WidgetTester tester,


nit: these two commits can be squashed 🙂

0b07032 login test: Add happy-path test for AddAccountPage
0b5f728 login test [nfc]: Pull out an attempt helper in AddAccountPage test

gnprice · 2025-08-07T00:32:37Z

test/widgets/login_test.dart

+        .throws<void>();
+
+
+      await attempt(tester, serverSettings.realmUrl, serverSettingsMalformedJson);


nit: double blank line

chrisbobbe · 2025-08-07T00:34:52Z

Thanks, fixed!

There's a planned change to make zulipMergeBase required in GetServerSettingsResult (see zulip#904). When we do that, we'd otherwise just treat a missing zulipMergeBase on an ancient server the same way we treat any malformed response. Better to check if the server is an ancient one we don't support, just like when the /register response is malformed.

gnprice · 2025-08-07T00:41:09Z

Thanks! Looks good; merging.

See "Feature level 88" from Zulip API changelog: https://zulip.com/api/changelog This is one of the few places (along with registerQueue) where it's important that we smoothly handle attempting to connect to an ancient server where this field might indeed be missing. Happily, we do handle those since commit 733bb88 (zulip#1783).

chrisbobbe requested a review from gnprice August 4, 2025 23:26

chrisbobbe assigned gnprice Aug 4, 2025

chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Aug 4, 2025

chrisbobbe mentioned this pull request Aug 4, 2025

Start Requiring Zulip Server 5 #904

Merged

gnprice reviewed Aug 6, 2025

View reviewed changes

chrisbobbe added 3 commits August 6, 2025 16:47

dialog test [nfc]: Remove an unused import

a55d8a9

Looks like this slipped through CI, introduced recently in 8d3c128.

login [nfc]: Tighten up some if-unmounted-returns; remove stale lint-…

c67a4cc

…ignore

server_support [nfc]: Move some server-support logic out to this new …

6d02f42

…file

chrisbobbe force-pushed the pr-ancient-servers-server-settings branch from 10e5f91 to 96f59ba Compare August 6, 2025 23:48

gnprice reviewed Aug 6, 2025

View reviewed changes

chrisbobbe force-pushed the pr-ancient-servers-server-settings branch from 96f59ba to 73be315 Compare August 7, 2025 00:26

gnprice reviewed Aug 7, 2025

View reviewed changes

chrisbobbe added 2 commits August 6, 2025 17:38

login test: Add happy-path test for AddAccountPage

06f1b87

chrisbobbe force-pushed the pr-ancient-servers-server-settings branch from 73be315 to 733bb88 Compare August 7, 2025 00:39

gnprice merged commit 733bb88 into zulip:main Aug 7, 2025
1 check passed

		await tester.pump((pushedRoute as TransitionRoute).transitionDuration);
		await tester.pump();

	await tester.pump((pushedRoute as TransitionRoute).transitionDuration);
	await tester.pump();
	await testNavObserver.pumpPastTransition(tester);

		.throws<void>();


		await attempt(tester, serverSettings.realmUrl, serverSettingsMalformedJson);

login: Reject earlier (at get-server-settings) when server is too old #1783

login: Reject earlier (at get-server-settings) when server is too old #1783

Uh oh!

Conversation

chrisbobbe commented Aug 4, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Aug 6, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Aug 7, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Aug 7, 2025

Uh oh!

gnprice commented Aug 7, 2025

Uh oh!

Uh oh!

Uh oh!