Skip to content

notif: Change account when opening a notification, if different account #1993

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

notif: Change account when opening a notification, if different account #1993

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions lib/notifications/open.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import '../model/binding.dart';
import '../model/narrow.dart';
import '../widgets/app.dart';
import '../widgets/dialog.dart';
import '../widgets/home.dart';
import '../widgets/message_list.dart';
import '../widgets/page.dart';
import '../widgets/store.dart';
Expand Down Expand Up @@ -135,7 +136,9 @@ class NotificationOpenService {
final route = routeForNotification(context: context, data: notifNavData);
if (route == null) return; // TODO(log)

// TODO(nav): Better interact with existing nav stack on notif open
if (GlobalStoreWidget.of(context).lastVisitedAccount?.id != route.accountId) {
HomePage.navigate(context, accountId: route.accountId);
}
unawaited(navigator.push(route));
}

Expand All @@ -158,7 +161,9 @@ class NotificationOpenService {
final route = routeForNotification(context: context, data: data);
if (route == null) return; // TODO(log)

// TODO(nav): Better interact with existing nav stack on notif open
if (GlobalStoreWidget.of(context).lastVisitedAccount?.id != route.accountId) {
HomePage.navigate(context, accountId: route.accountId);
}
unawaited(navigator.push(route));
}

Expand Down
64 changes: 54 additions & 10 deletions test/notifications/open_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ void main() {

group('NotificationOpenService', () {
late List<Route<void>> pushedRoutes;
late List<Route<void>> poppedRoutes;

void takeHomePageRouteForAccount(int accountId) {
check(pushedRoutes).first.which(
Expand All @@ -106,8 +107,18 @@ void main() {
Future<void> prepare(WidgetTester tester, {bool early = false}) async {
await init();
pushedRoutes = [];
poppedRoutes = [];
final testNavObserver = TestNavigatorObserver()
..onPushed = (route, prevRoute) => pushedRoutes.add(route);
..onPushed = (route, prevRoute) {
pushedRoutes.add(route);
}
..onPopped = (route, prevRoute) {
poppedRoutes.add(route);
}
..onReplaced = (route, prevRoute) {
poppedRoutes.add(prevRoute!);
pushedRoutes.add(route!);
};
// This uses [ZulipApp] instead of [TestZulipApp] because notification
// logic uses `await ZulipApp.navigator`.
await tester.pumpWidget(ZulipApp(navigatorObservers: [testNavObserver]));
Expand Down Expand Up @@ -178,6 +189,20 @@ void main() {
}
}

void takeHomePageReplacement(int accountId, int prevAccountId) {
check(poppedRoutes).last.which(
(it) => it.isA<MaterialAccountWidgetRoute>()
..accountId.equals(prevAccountId)
..page.isA<HomePage>());
// We only care about HomePage being the last popped route.
poppedRoutes.clear();
Comment on lines +192 to +198
Copy link
Member

@gnprice gnprice Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't match the meaning of our other "takeFoo" helpers, in this or other tests: generally those all consume exactly the items that they check the details of, and consume them in the order they happened.

In this version, it not only ignores any non-HomePage routes before the last, but also any extra HomePage routes before the last one popped. So e.g. if a test popped two different HomePage routes and only then called this function, this would look only at the last one (but then in the next line it would look at the first route pushed, a potentially-confusing mismatch).

If you only ever want to look at the last route popped, probably clearest to only maintain that data: like Route<void>? lastPoppedRoute, instead of a list.

check(pushedRoutes).first.which(
(it) => it.isA<MaterialAccountWidgetRoute>()
..accountId.equals(accountId)
..page.isA<HomePage>());
pushedRoutes.removeAt(0);
Comment on lines +199 to +203
Copy link
Member

@gnprice gnprice Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same as a call to takeHomePageRouteForAccount, right?

}

void matchesNavigation(Subject<Route<void>> route, Account account, Message message) {
route.isA<MaterialAccountWidgetRoute>()
..accountId.equals(account.id)
Expand All @@ -186,8 +211,15 @@ void main() {
selfUserId: account.userId));
}

Future<void> checkOpenNotification(WidgetTester tester, Account account, Message message) async {
Future<void> checkOpenNotification(
WidgetTester tester,
Account account,
Message message, {
bool expectHomePageReplaced = false,
int? prevAccountId,
}) async {
await openNotification(tester, account, message);
if (expectHomePageReplaced) takeHomePageReplacement(account.id, prevAccountId!);
Copy link
Member

@gnprice gnprice Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should also check that we don't pop any routes when that's not expected (when the account didn't change).

matchesNavigation(check(pushedRoutes).single, account, message);
pushedRoutes.clear();
}
Expand Down Expand Up @@ -259,19 +291,29 @@ void main() {
eg.account(id: 1004, realmUrl: realmUrlB, user: user2),
];
await testBinding.globalStore.add(
accounts[0], eg.initialSnapshot(realmUsers: [user1]));
accounts[0], eg.initialSnapshot(realmUsers: [user1]),
markLastVisited: true);
Comment on lines 293 to +295
Copy link
Member

@gnprice gnprice Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think markLastVisited is best avoided in general. It makes the test more complicated, and it does so by making it behave differently from how the real app behaves: in the real app, when an account gets added to the global store, that's always accompanied by visiting that account.

Here, if you want accounts[0] to be the one last visited, that can be done by reversing the order of the globalStore.add calls. Alternatively (and probably clearer), can let accounts[3] be the last visited, and reverse the order of the checkOpenNotification calls.

await testBinding.globalStore.add(
accounts[1], eg.initialSnapshot(realmUsers: [user2]));
accounts[1], eg.initialSnapshot(realmUsers: [user2]),
markLastVisited: false);
await testBinding.globalStore.add(
accounts[2], eg.initialSnapshot(realmUsers: [user1]));
accounts[2], eg.initialSnapshot(realmUsers: [user1]),
markLastVisited: false);
await testBinding.globalStore.add(
accounts[3], eg.initialSnapshot(realmUsers: [user2]));
accounts[3], eg.initialSnapshot(realmUsers: [user2]),
markLastVisited: false);
await prepare(tester);

await checkOpenNotification(tester, accounts[0], eg.streamMessage());
await checkOpenNotification(tester, accounts[1], eg.streamMessage());
await checkOpenNotification(tester, accounts[2], eg.streamMessage());
await checkOpenNotification(tester, accounts[3], eg.streamMessage());
await checkOpenNotification(tester,
accounts[1], eg.streamMessage(),
expectHomePageReplaced: true, prevAccountId: accounts[0].id);
Comment on lines +308 to +310
Copy link
Member

@gnprice gnprice Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifying the previous account ID seems redundant here: the HomePage that gets popped is necessarily going to be the one that had been on the stack in the first place. The key question is whether it gets popped or not.

await checkOpenNotification(tester,
accounts[2], eg.streamMessage(),
expectHomePageReplaced: true, prevAccountId: accounts[1].id);
await checkOpenNotification(tester,
accounts[3], eg.streamMessage(),
expectHomePageReplaced: true, prevAccountId: accounts[2].id);
}, variant: const TargetPlatformVariant({TargetPlatform.android, TargetPlatform.iOS}));

testWidgets('wait for app to become ready', (tester) async {
Expand Down Expand Up @@ -341,7 +383,9 @@ void main() {
await prepare(tester);
check(testBinding.globalStore).lastVisitedAccount.equals(eg.selfAccount);

await checkOpenNotification(tester, eg.otherAccount, eg.streamMessage());
await checkOpenNotification(tester,
eg.otherAccount, eg.streamMessage(),
expectHomePageReplaced: true, prevAccountId: eg.selfAccount.id);
check(testBinding.globalStore).lastVisitedAccount.equals(eg.otherAccount);
}, variant: const TargetPlatformVariant({TargetPlatform.android, TargetPlatform.iOS}));

Expand Down