notif: Change account when opening a notification, if different account #1993
Conversation
Previously, opening a notification from a different account would leave the home page on a different account than the newly opened message list. Now, if the notification's account is different from currently viewing account, we change the whole navigation stack: pop all routes, push HomePage, push MessageList Fixes: zulip#1210
rajveermalviya
added
the
maintainer review
chrisbobbe
left a comment
chrisbobbe
left a comment
There was a problem hiding this comment.
Thanks! LGTM, marking for Greg's review.
chrisbobbe
added
integration review
gnprice
left a comment
gnprice
left a comment
There was a problem hiding this comment.
Thanks for taking care of this! Comments below.
Let's also add a short test case that's specifically about this behavior. The existing test cases that exercise it do so kind of by accident.
| int? prevAccountId, | ||
| }) async { | ||
| await openNotification(tester, account, message); | ||
| if (expectHomePageReplaced) takeHomePageReplacement(account.id, prevAccountId!); |
There was a problem hiding this comment.
Should also check that we don't pop any routes when that's not expected (when the account didn't change).
| 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(); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
This is the same as a call to takeHomePageRouteForAccount, right?
| await checkOpenNotification(tester, | ||
| accounts[1], eg.streamMessage(), | ||
| expectHomePageReplaced: true, prevAccountId: accounts[0].id); |
There was a problem hiding this comment.
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 testBinding.globalStore.add( | ||
| accounts[0], eg.initialSnapshot(realmUsers: [user1])); | ||
| accounts[0], eg.initialSnapshot(realmUsers: [user1]), | ||
| markLastVisited: true); |
There was a problem hiding this comment.
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.
3 participants
Previously, opening a notification from a different account would leave the home page on a different account than the newly opened message list.
Now, if the notification's account is different from currently viewing account, we change the whole navigation stack: pop all routes, push HomePage, push MessageList
Fixes: #1210