mute: Keep out unexpected enum values from events, too

This way we consistently maintain the invariant that we only see
valid values.

src/mute/__tests__/muteModel-test.js

@@ -15,6 +15,7 @@ import { makeMuteState, makeUserTopic } from './mute-testlib';
 import { tryGetActiveAccountState } from '../../selectors';
 import { UserTopicVisibilityPolicy } from '../../api/modelTypes';
 import { EventTypes } from '../../api/eventTypes';
+import * as logging from '../../utils/logging';
 
 /* eslint jest/expect-expect: ["error", { "assertFunctionNames": ["expect", "check"] }] */
 
@@ -139,6 +140,25 @@ describe('reducer', () => {
       );
     });
 
+    test('in modern user_topics format: invalid enum values discarded', () => {
+      // $FlowFixMe[prop-missing]: Jest mock
+      logging.warn.mockReturnValue();
+
+      const action1 = eg.mkActionRegisterComplete({
+        user_topics: [
+          // $FlowIgnore[incompatible-call]: simulates a future server
+          makeUserTopic(eg.stream, 'topic', 42),
+          makeUserTopic(eg.stream, 'other topic', UserTopicVisibilityPolicy.Muted),
+        ],
+      });
+      const action2 = eg.mkActionRegisterComplete({
+        user_topics: [makeUserTopic(eg.stream, 'other topic', UserTopicVisibilityPolicy.Muted)],
+      });
+      expect(reducer(initialState, action1, eg.plusReduxState)).toEqual(
+        reducer(initialState, action2, eg.plusReduxState),
+      );
+    });
+
     test('in old muted_topics format: unit test', () => {
       const action = eg.mkActionRegisterComplete({
         muted_topics: [[eg.stream.name, 'topic']],
@@ -231,6 +251,21 @@ describe('reducer', () => {
         initialState,
       );
     });
+
+    test('treat invalid enum value as removing', () => {
+      // $FlowFixMe[prop-missing]: Jest mock
+      logging.warn.mockReturnValue();
+
+      check(
+        makeMuteState([
+          [eg.stream, 'topic'],
+          [eg.stream, 'other topic'],
+        ]),
+        // $FlowIgnore[incompatible-call]: simulates a future server
+        makeUserTopic(eg.stream, 'topic', 999),
+        makeMuteState([[eg.stream, 'other topic']]),
+      );
+    });
   });
 
   describe('EVENT_MUTED_TOPICS (legacy)', () => {

src/mute/muteModel.js

@@ -100,6 +100,21 @@ export function isTopicVisible(
 
 const initialState: MuteState = Immutable.Map();
 
+/**
+ * Warn and return true on an unexpected value; else return false.
+ *
+ * This lets us keep out of our data structures any values we
+ * don't expect in our types.
+ */
+function warnInvalidVisibilityPolicy(visibility_policy: UserTopicVisibilityPolicy): boolean {
+  if (!UserTopicVisibilityPolicy.isValid((visibility_policy: number))) {
+    // Not a value we expect.  Keep it out of our data structures.
+    logging.warn(`unexpected UserTopicVisibilityPolicy: ${(visibility_policy: number)}`);
+    return true;
+  }
+  return false;
+}
+
 /** Consume the old `muted_topics` format. */
 function convertLegacy(data, streams): MuteState {
   // Same strategy as in convertInitial, below.
@@ -128,9 +143,7 @@ function convertInitial(data): MuteState {
   // plain old Array.
   const byStream = new DefaultMap(() => []);
   for (const { stream_id, topic_name, visibility_policy } of data) {
-    if (!UserTopicVisibilityPolicy.isValid((visibility_policy: number))) {
-      // Not a value we expect.  Keep it out of our data structures.
-      logging.warn(`unexpected UserTopicVisibilityPolicy: ${(visibility_policy: number)}`);
+    if (warnInvalidVisibilityPolicy(visibility_policy)) {
       continue;
     }
     byStream.getOrCreate(stream_id).push([topic_name, visibility_policy]);
@@ -186,7 +199,11 @@ export const reducer = (
       const { event } = action;
       switch (event.type) {
         case EventTypes.user_topic: {
-          const { stream_id, topic_name, visibility_policy } = event;
+          const { stream_id, topic_name } = event;
+          let { visibility_policy } = event;
+          if (warnInvalidVisibilityPolicy(visibility_policy)) {
+            visibility_policy = UserTopicVisibilityPolicy.None;
+          }
           if (visibility_policy === UserTopicVisibilityPolicy.None) {
             // This is the "zero value" for this type, which our MuteState
             // data structure represents by leaving the topic out entirely.

src/storage/__tests__/migrations-test.js

@@ -105,7 +105,7 @@ describe('migrations', () => {
   // What `base` becomes after all migrations.
   const endBase = {
     ...base52,
-    migrations: { version: 59 },
+    migrations: { version: 60 },
   };
 
   for (const [desc, before, after] of [
@@ -128,9 +128,9 @@ describe('migrations', () => {
     // redundant with this one, because none of the migration steps notice
     // whether any properties outside `storeKeys` are present or not.
     [
-      'check dropCache at 59',
+      'check dropCache at 60',
       // Just before the `dropCache`, plus a `cacheKeys` property, plus junk.
-      { ...base52, migrations: { version: 58 }, mute: [], nonsense: [1, 2, 3] },
+      { ...base52, migrations: { version: 59 }, mute: [], nonsense: [1, 2, 3] },
       // Should wind up with the same result as without the extra properties.
       endBase,
     ],

src/storage/migrations.js

@@ -514,6 +514,9 @@ const migrationsInner: {| [string]: (LessPartialState) => LessPartialState |} =
 
   // Changed `state.presence` (twice), but no migration because that's in `discardKeys`.
 
+  // Discard invalid enum values from `state.mute`.
+  '60': dropCache,
+
   // TIP: When adding a migration, consider just using `dropCache`.
   //   (See its jsdoc for guidance on when that's the right answer.)
 };