store: Handle invalid API key on register-queue

[PIG208]

This is near term fix for a user-reported issue:
https://chat.zulip.org/#narrow/channel/48-mobile/topic/0.2E0.2E19.20Flutter.20.3A.20Cant.20connect.20to.20self.20hosted.20instance/near/2004042

It is not intended to be the full fix. With a better UX, we would bring the user back to the choose-account page without them manually doing so. That's covered by #737 but out-of-scope for this commit.

This cherry-picks #1235, and partially addresses #890, by handling INVALID_API_KEY errors from the PerAccountStoreWidget.

Fixes:

• Handle invalid API key on register-queue #737

[gnprice]

Thanks!

Would you post a screenshot of what the error looks like? Then we can also wordsmith the error message.

[PIG208]

Sure!

[gnprice]

Cool, thanks.

I think "Invalid API key" is probably too technical for this context. @alya do we have an appropriate message handy from elsewhere in the product?

[PIG208]

Perhaps we can rename and reuse the title string we have for login failure:

  "errorLoginCouldNotConnectTitle": "Could not connect",

This sort of feels like an instance of #741 the way we currently handle it, but this message will not be needed after #737.

screenshot

[PIG208]

Updated the PR to implement Chris' suggestion here. We now handle invalid API key by calling logOutAccount, which removes the routes associated with the affected account, pushing a choose-account page route onto the stack if it becomes empty, and showing an error dialog.

[chrisbobbe]

Thanks! Here's a review of the first five commits:

06c5543 l10n [nfc]: Use a generalize name for errorCouldNotConnectTitle
d27b46c log [nfc]: Rename ReportErrorCallback to ReportErrorCancellablyCallback
c5fc2d9 example_data: Start generating account id
652c332 home: Stop assuming account existence from loading page
ab91480 log: Add reportErrorModally

which leaves the last three that I haven't read yet:

58fd4b1 store: Handle invalid API key on register-queue
7a632b2 display test [nfc]: Make openNotification public
2e0bcb9 actions test: Use a realistic example

For the last commit, it's the one with the Fixes: line but it doesn't actually change any app code, it only touches test code. Is that intentional? I'd like to understand if/how these test changes are related to your app-code changes for #737.

[chrisbobbe]

l10n [nfc]: Use a generalize name for errorCouldNotConnectTitle

commit-message nit: "generalized"

[chrisbobbe]

This feels like it duplicates more details than it needs to, like the appBar param and the whole set of Scaffold params that aren't passed. How about applying the condition more precisely to the things that are supposed to be different?

[chrisbobbe]

Setting reportErrorToUserModally to defaultReportErrorToUserBriefly looks odd. How about renaming defaultReportErrorToUserBriefly to something that seems like an appropriate value for both reportErrorToUserBriefly and reportErrorToUserModally?

Maybe dumpErrorToConsole? That name makes it sound like a potential problem if it actually gets called (errors should be reported, not dropped on the floor 🙂)—but that's actually appropriate in this case.

[PIG208]

Good idea! Renaming to reportErrorToConsole because we don't usually dump the error object, which I feel is what "dump" is for.

[chrisbobbe]

I have a commit

test [nfc]: Factor out checkNoErrorDialog helper

in my revision for #1239; how about cherry-picking that as a prep commit and using it here? (See reasoning in the commit)

[PIG208]

The last commit "fixes" the issue because it resolves a TODO comment for the issue. With #737 we can set up the test in a scenario where the API key is invalidated.

[chrisbobbe]

Thanks! Comments below.

[chrisbobbe]

I think my comment in CZO still applies:

[…] the message looks like it can't be literally true. If an action really "cannot" be done, then trying the same action again won't work.

Or, from another angle: the server hasn't said that the account can't be authenticated, so we shouldn't tell the user that. That would be a pretty pathological case: I guess in theory it would be accurate if the database were corrupted in a way that made it impossible to know if any API key was valid.

What we know from the INVALID_API_KEY error is that the current attempt to authenticate failed, because we used the wrong API key. Here's a proposal:

Your account at {url} could not be authenticated. Please try logging in again or use another account.

cc @alya for thoughts on this message.

[chrisbobbe]

It looks like this isn't used anywhere.

[chrisbobbe]

In my manual testing, I'm getting an extra ChooseAccountPage in the nav stack. Do you reproduce? Possibly it's only happening when I have more than one PerAccountStoreWidget mounted for the logged-out account. I think probably what's happening is you expected NavigatorState.canPop() to return false only when the nav stack is empty, but it's returning false when the nav stack consists of one route.

[PIG208]

Yeah, a check for both isFirst and isCurrent would be more appropriate. I think this also means that the current test does not cover this case correctly, will fix that too.

[PIG208]

It turned out that

Yeah, a check for both isFirst and isCurrent would be more appropriate.

is not accurate.

There can be routes that are not removed on logout but are not helpful either for the user to navigate to a choose-account page. DialogRoute is an example of that. So checks based on "if the navigator stack is empty" will likely not work. We need a solution that guarantees a choose-account page route at the root level, and this is probably not the right place to do that.

[chrisbobbe]

All of this gets skipped if there's been a previously successful /register, because in that case the store != null path is taken (some lines above this).

So in this revision, in the refresh-expired-event-queue case (GlobalStore._reloadPerAccount), I think the uncaught error means the user gets stuck with a stale PerAccountStore with no active event queue, and they can't replace that store (because we take the store != null path) except by closing/opening the app, or I guess logging out and back in. We should handle the refresh-expired-event-queue case, with test coverage.

Can we put this reportErrorToUserModally(), the logOutAccount(), and the reset-navigation-state action (previous comment) somewhere that clearly gets invoked once (just once) on every /register / INVALID_API_KEY, including the one in _reloadPerAccount?

[chrisbobbe]

We also need tests to check that the account is in fact logged out / removed, and that some subtle related things are dealt with, like _perAccountStores and _perAccountStoresLoading, calling PerAccountStore.dispose, and notifying listeners. How about using TestGlobalStore.takeDoRemoveAccountCalls for this, because GlobalStore.removeAccount already has tests for those things.

[chrisbobbe]

For example (just for the first-/register case):

  test('GlobalStore.perAccount on INVALID_API_KEY', () => awaitFakeAsync((async) async {
    addTearDown(testBinding.reset);

    await testBinding.globalStore.insertAccount(eg.selfAccount.toCompanion(false));
    testBinding.globalStore.loadPerAccountException = ZulipApiException(
      routeName: '/register', code: 'INVALID_API_KEY', httpStatus: 400,
      data: {}, message: '');
    await check(testBinding.globalStore.perAccount(eg.selfAccount.id))
      .throws<ZulipApiException>();

    // fails
    check(testBinding.globalStore.takeDoRemoveAccountCalls())
      .single.equals(eg.selfAccount.id);
  }));

(to go near the other 'GlobalStore.perAccount' tests)

[PIG208]

I think the right place for calling logOutAccount and reportErrorModally should be loadPerAccount. We throw an AccountNotFoundException after processing the error, and make sure that all call-sites handles that correctly. _PerAccountStoreWidgetState already expects the exception.

For the other call-site, we need to handle it at least for one of _reloadPerAccount, UpdateMachine._handlePollError, or UpdateMachine.poll.

It doesn't seem right to catch it from _reloadPerAccount without rethrowing or at least indicating that the reload was a failure; _handlePollError's caller assume that the store is disposed after calling it (because of a reloaded PerAccountStore), so we should still either rethrow the error or indicate the failure reloading the store. So we probably should just catch it from poll, within another catch block.

---

Because logOutAccount is no longer purely a user action, we might want to revisit #1010 (comment) and figure out a new home to this. While GlobalStore is logically a good fit, I agree that lib/model/store.dart has become too crowded for us to add more to it. Maybe splitting store.dart would be a helpful prep/follow-up change.

[chrisbobbe]

I don't yet understand this CircularProgressIndicator check.

• Does the test expect it to be on a specific page? (meaning the check stops being as useful if another page adds a CircularProgressIndicator)
• If we're expecting it on a non-active/foregrounded page, do we need to use skipOffstage: false?

[chrisbobbe]

Reading that linked discussion and links from there, I'm not finding an explanation for why the TODO involves #737 at all. What Greg meant by not "realistic setup" is just that "These pages don't exist in real life", meaning two MaterialAccountWidgetRoutes with page InboxPageBody. Routes like that don't exist in the app because, since the bottom-tabs update, the Inbox page is never alone on its own route, it's always part of the route returned by HomePage.buildRoute.

The direct fix is to use realistic routes for account1Route and account2Route, e.g. by using MessageListPage.buildRoute instead, making follow-on changes to findAccount{1,2}PageContent, and renaming the makeUnreadTopicInInbox helper. Let's have a commit that just does that and removes the TODO, explaining in the commit message that it's not related to 737 after all.

If you'd like to go further in a follow-up commit (which I think is not a high priority), I'd have some feedback on that direction:

• Take care to include skipOffstage: false where necessary when checking for something on a route that's not the top of the stack.
• I intentionally put account1Route (the one to be removed) under account2Route in the stack, to guard against buggy implementations that might use something like NavigatorState.popUntil (which starts from the top and stops at the first route that shouldn't be removed, even if there are ones that should be removed under it). This would be a nice feature to keep, but I guess not essential.