flags model: Fix bug where mark-all-as-read would *add* unread markers

(Edit: Actually, the previous commit changed the symptom of #5597,
making this commit message a bit inaccurate. At this point, things
have changed so that "Mark as read" won't add unread markers, it'll
just fail to remove them. See discussion on the PR.)

You're not meant to use Object.keys on an Immutable.Map; details at
issue #5597 for how this caused a display-only bug where messages in
view would get marked with unread markers.

Greg points out, at
  #5597 (comment)

> If we'd had type-checking on those tests, Flow would have pointed
> out that the test data needed to change when we changed the
> definition of `MessagesState`, which would have alerted us that
> `flagsReducer` was consuming data of that type and given us a
> better chance to see we needed to update that logic.

We'll start type-checking flagsReducer-test.js soon. But for now,
just for this one test, give input that's unchecked but well formed,
and fix the reducer's implementation. The fixed test passes with
this new reducer implementation, and it would fail with the old,
buggy implementation.

Fixes: #5597

Author: chrisbobbe

src/chat/__tests__/flagsReducer-test.js

@@ -1,5 +1,6 @@
 import deepFreeze from 'deep-freeze';
 
+import * as eg from '../../__tests__/lib/exampleData';
 import flagsReducer from '../flagsReducer';
 import {
   MESSAGE_FETCH_COMPLETE,
@@ -280,40 +281,42 @@ describe('flagsReducer', () => {
     });
 
     test('when all=true, flag=read, and op=add, all messages become read; other flags untouched', () => {
+      const message1 = eg.streamMessage();
+      const message2 = eg.streamMessage();
+      const message3 = eg.streamMessage();
+      const message4 = eg.streamMessage();
+      const message5 = eg.streamMessage();
+
       const prevState = deepFreeze({
+        ...eg.plusReduxState.flags,
         starred: {
-          1: true,
-          4: true,
+          [message1.id]: true,
+          [message4.id]: true,
         },
         read: {
-          1: true,
-          3: true,
+          [message1.id]: true,
+          [message3.id]: true,
         },
       });
 
       const action = deepFreeze({
+        id: 1,
         type: EVENT_UPDATE_MESSAGE_FLAGS,
+        all: true,
+        allMessages: eg.makeMessagesState([message1, message2, message3, message4, message5]),
         messages: [],
         flag: 'read',
         op: 'add',
-        all: true,
-        allMessages: {
-          1: {},
-          2: {},
-          3: {},
-          4: {},
-          5: {},
-        },
       });
 
       const expectedState = {
         ...prevState,
         read: {
-          1: true,
-          2: true,
-          3: true,
-          4: true,
-          5: true,
+          [message1.id]: true,
+          [message2.id]: true,
+          [message3.id]: true,
+          [message4.id]: true,
+          [message5.id]: true,
         },
       };
 

src/chat/flagsReducer.js

@@ -103,7 +103,9 @@ const processFlagsForMessages = (
 const eventUpdateMessageFlags = (state, action) => {
   if (action.all) {
     if (action.op === 'add') {
-      return addFlagsForMessages(state, Object.keys(action.allMessages).map(Number), [action.flag]);
+      return addFlagsForMessages(state, action.allMessages.keySeq().toArray().map(Number), [
+        action.flag,
+      ]);
     }
 
     if (action.op === 'remove') {