accounts: Invariant that active account exists when we expect it to

In its handling of the actions REGISTER_COMPLETE, LOGOUT,
DISMISS_SERVER_PUSH_SETUP_NOTICE, and EVENT, the `accounts` reducer
has been making the unchecked assumption that there is an active
account.

When there isn't an active account (perhaps because the last account
was removed with ACCOUNT_REMOVE), the reducer has been creating
badly typed partial `Account`s, by creating an object from a spread
of `state[0]` which is `undefined`. For example, with
REGISTER_COMPLETE:

  {
    ...state[0],
    userId: action.data.user_id,
    zulipFeatureLevel: action.data.zulip_feature_level,
    zulipVersion: action.data.zulip_version,
    lastDismissedServerPushSetupNotice: action.data.realm_push_notifications_enabled
      ? null
      : state[0].lastDismissedServerPushSetupNotice,
  }

That will make an object with only userId, zulipFeatureLevel,
zulipVersion, and lastDismissedServerPushSetupNotice. When code
expects a well-formed Account including realm, apiKey, email, and
ackedPushToken, it'll error.

Flow has basically let this happen because it assumes (wrongly, in
this case) that `state[0]` is an in-bounds access that will give an
Account.

We've had a few mysterious Sentry reports where it seems `realm` is
missing in the active account. E.g., for those with access:
  https://sentry.io/organizations/zulip/issues/3900597161/?project=191284
  https://sentry.io/organizations/zulip/issues/3900597257/?project=191284
  https://sentry.io/organizations/zulip/issues/3900597246/?project=191284
  https://sentry.io/organizations/zulip/issues/3900596985/?project=191284

Seeing that the faulty reducer code could cause that symptom, add an
`invariant` that there is an active account. (And fix some
mute-model test data where the data was unrepresentative because it
was missing an active account.)

N.B.: The new invariant ensures that there *is* an active account,
but it doesn't check that it's the account the action was supposed
to act on. We should fix that (so, add TODOs about it), but at least
now we hope to be rid of these malformed `Account` objects.

Not seeing anything else that could cause these malformed Account
objects, add a migration so they don't come up from storage.

Author: chrisbobbe

src/account/accountsReducer.js

@@ -27,22 +27,29 @@ const findAccount = (state: AccountsState, identity: Identity): number => {
   );
 };
 
+function updateActiveAccount(state, change) {
+  const activeAccount: Account | void = state[0];
+  invariant(activeAccount, 'accounts reducer: expected active account');
+
+  return [{ ...activeAccount, ...change }, ...state.slice(1)];
+}
+
 // eslint-disable-next-line default-param-last
 export default (state: AccountsState = initialState, action: Action): AccountsState => {
   switch (action.type) {
     case REGISTER_COMPLETE:
-      return [
-        {
-          ...state[0],
-          userId: action.data.user_id,
-          zulipFeatureLevel: action.data.zulip_feature_level,
-          zulipVersion: action.data.zulip_version,
-          lastDismissedServerPushSetupNotice: action.data.realm_push_notifications_enabled
-            ? null
-            : state[0].lastDismissedServerPushSetupNotice,
-        },
-        ...state.slice(1),
-      ];
+      // TODO(#5009): Before implementing a multi-account schema (#5006),
+      //   this will be the wrong account if the active account changed
+      //   while the register was in progress. After #5006, this per-account
+      //   action should naturally act on its own per-account state.
+      return updateActiveAccount(state, {
+        userId: action.data.user_id,
+        zulipFeatureLevel: action.data.zulip_feature_level,
+        zulipVersion: action.data.zulip_version,
+        lastDismissedServerPushSetupNotice: action.data.realm_push_notifications_enabled
+          ? null
+          : state[0].lastDismissedServerPushSetupNotice,
+      });
 
     case ACCOUNT_SWITCH: {
       const index = state.findIndex(
@@ -108,11 +115,15 @@ export default (state: AccountsState = initialState, action: Action): AccountsSt
     }
 
     case LOGOUT: {
-      return [{ ...state[0], apiKey: '', ackedPushToken: null }, ...state.slice(1)];
+      // TODO: This will be the wrong account if the active account changed
+      //   between pressing "logout" and confirming with the confirmation
+      //   dialog. Fix, perhaps by having the LOGOUT action include an
+      //   Identity in its payload.
+      return updateActiveAccount(state, { apiKey: '', ackedPushToken: null });
     }
 
     case DISMISS_SERVER_PUSH_SETUP_NOTICE: {
-      return [{ ...state[0], lastDismissedServerPushSetupNotice: action.date }, ...state.slice(1)];
+      return updateActiveAccount(state, { lastDismissedServerPushSetupNotice: action.date });
     }
 
     case ACCOUNT_REMOVE: {
@@ -135,14 +146,10 @@ export default (state: AccountsState = initialState, action: Action): AccountsSt
 
           // TODO: Detect if the feature level has changed, indicating an upgrade;
           //   if so, trigger a full refetch of server data.  See #4793.
-          return [
-            {
-              ...state[0],
-              zulipVersion: new ZulipVersion(zulip_version),
-              zulipFeatureLevel: zulip_feature_level,
-            },
-            ...state.slice(1),
-          ];
+          return updateActiveAccount(state, {
+            zulipVersion: new ZulipVersion(zulip_version),
+            zulipFeatureLevel: zulip_feature_level,
+          });
         }
         default:
           return state;

src/mute/__tests__/muteModel-test.js

@@ -25,7 +25,7 @@ describe('reducer', () => {
         subscriptions: [eg.subscription],
         muted_topics: [[eg.stream.name, 'topic']],
       });
-      const newState = tryGetActiveAccountState(fullReducer(eg.baseReduxState, action));
+      const newState = tryGetActiveAccountState(fullReducer(eg.plusReduxState, action));
       expect(newState).toBeTruthy();
       expect(newState && getMute(newState)).toEqual(makeMuteState([[eg.stream, 'topic']]));
     });

src/storage/__tests__/migrations-test.js

@@ -5,6 +5,7 @@ import { historicalStoreKeys, migrations } from '../migrations';
 import { storeKeys } from '../../boot/store';
 import { createMigrationFunction } from '../../redux-persist-migrate';
 import { ZulipVersion } from '../../utils/zulipVersion';
+import * as eg from '../../__tests__/lib/exampleData';
 
 describe('historicalStoreKeys', () => {
   test('equals current storeKeys', () => {
@@ -104,7 +105,7 @@ describe('migrations', () => {
   // What `base` becomes after all migrations.
   const endBase = {
     ...base52,
-    migrations: { version: 57 },
+    migrations: { version: 58 },
   };
 
   for (const [desc, before, after] of [
@@ -283,6 +284,23 @@ describe('migrations', () => {
       { ...base52, migrations: { version: 56 }, accounts: [...base37.accounts, undefined] },
       { ...endBase, accounts: [...base37.accounts] },
     ],
+    [
+      'check 58 with a malformed Account in state.accounts',
+      {
+        ...base52,
+        migrations: { version: 57 },
+        accounts: [
+          {
+            userId: eg.selfUser.user_id,
+            zulipFeatureLevel: eg.recentZulipFeatureLevel,
+            zulipVersion: eg.recentZulipVersion,
+            lastDismissedServerPushSetupNotice: null,
+          },
+          ...base37.accounts,
+        ],
+      },
+      { ...endBase, accounts: [...base37.accounts] },
+    ],
   ]) {
     /* eslint-disable no-loop-func */
     test(desc, async () => {

src/storage/migrations.js

@@ -475,6 +475,33 @@ const migrationsInner: {| [string]: (LessPartialState) => LessPartialState |} =
   // Fix tiny case where an `undefined` could sneak in on ACCOUNT_SWITCH
   '57': state => ({ ...state, accounts: state.accounts.filter(Boolean) }),
 
+  // Remove malformed partial `Account`s. Perhaps we could salvage some, if
+  // they're just missing e.g. `zulipFeatureLevel`… but we don't think the
+  // bug affected many people, and when it did, it quickly followed a state
+  // where `accounts` was empty, probably because the user asked to remove
+  // the last account.
+  '58': state => ({
+    ...state,
+    accounts: state.accounts.filter(account =>
+      [
+        'realm',
+        'apiKey',
+        'email',
+        'userId',
+        'zulipVersion',
+        'zulipFeatureLevel',
+        'ackedPushToken',
+        'lastDismissedServerPushSetupNotice',
+      ].every(key =>
+        /* $FlowIgnore[method-unbinding]: This is the standard way to call
+           `hasOwnProperty`. See discussion:
+             https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20158.20errors/near/1375563
+         */
+        Object.prototype.hasOwnProperty.call(account, key),
+      ),
+    ),
+  }),
+
   // TIP: When adding a migration, consider just using `dropCache`.
   //   (See its jsdoc for guidance on when that's the right answer.)
 };