From 887e58d1d9e46c7c17f1cca9dd0f070b026431d6 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 17 Sep 2019 22:21:51 -0500 Subject: [PATCH 1/6] eventMiddleware: Set event.message.flags to [] even if absent on event. The conditional described by the comment "move `flags` key from `event` to `event.message` for consistency" was not defaulting `event.message.flags` to an empty array in cases where `flags` is absent on `event`. Defaulting to an empty array will give more resistance to unpredictable input, in a crunchy shell pattern: https://github.com/zulip/zulip-mobile/blob/master/docs/architecture/crunchy-shell.md This commit replaces a test from 25e97ef8 (fixed #3046) that ensured the opposite (!): that `event.message` was not mutated in the case where `event.message.flags` was not set. I didn't see an obvious reason for this test, and an explanation wasn't given in the commit message. If possible, I do think it's important to have a dependable empty value for event.message.flags, so we don't have to constantly check internally if it's defined. --- src/events/__tests__/eventMiddleware-test.js | 14 +++++++------- src/events/eventMiddleware.js | 8 +++++--- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/events/__tests__/eventMiddleware-test.js b/src/events/__tests__/eventMiddleware-test.js index 893a5ac83a8..4ec315c6e17 100644 --- a/src/events/__tests__/eventMiddleware-test.js +++ b/src/events/__tests__/eventMiddleware-test.js @@ -1,9 +1,8 @@ -import deepFreeze from 'deep-freeze'; - import eventMiddleware from '../eventMiddleware'; +import { NULL_ARRAY } from '../../nullObjects'; describe('eventMiddleware', () => { - test('if `event.flags` key exist, move it to `event.message.flags`', () => { + test('if `event.flags` key exists, move it to `event.message.flags`', () => { const state = { session: {} }; const event = { type: 'message', @@ -16,12 +15,13 @@ describe('eventMiddleware', () => { expect(event.message.flags).toEqual(['mentioned']); }); - test('if `event.flags` do not exist, do not mutate event', () => { + test('if `event.flags` does not exist, set message.flags to an empty array', () => { const state = { session: {} }; - const event = deepFreeze({ + const event = { type: 'message', message: {}, - }); - expect(() => eventMiddleware(state, event)).not.toThrow(); + }; + eventMiddleware(state, event); + expect(event.message.flags).toEqual(NULL_ARRAY); }); }); diff --git a/src/events/eventMiddleware.js b/src/events/eventMiddleware.js index e6411fc6d53..eb346dc2016 100644 --- a/src/events/eventMiddleware.js +++ b/src/events/eventMiddleware.js @@ -5,6 +5,7 @@ import type { GeneralEvent, GlobalState, MessageEvent } from '../types'; import { isHomeNarrow, isMessageInNarrow } from '../utils/narrow'; import { getActiveAccount, getChatScreenParams, getOwnEmail, getIsActive } from '../selectors'; import { playMessageSound } from '../utils/sound'; +import { NULL_ARRAY } from '../nullObjects'; export default (state: GlobalState, event_: GeneralEvent) => { switch (event_.type) { @@ -12,10 +13,11 @@ export default (state: GlobalState, event_: GeneralEvent) => { // $FlowFixMe This expresses our unchecked assumptions about `message` events. const event = (event_: MessageEvent); - // move `flags` key from `event` to `event.message` for consistency - if (event.flags && !event.message.flags) { + // move `flags` key from `event` to `event.message` for consistency, and + // default to an empty array if event.flags is not set. + if (!event.message.flags) { // $FlowFixMe Message is readonly to serve our use of it in Redux. - event.message.flags = event.flags; + event.message.flags = event.flags || NULL_ARRAY; delete event.flags; } From 0b58f81a40f88989c3aa8d896b2aff6f22760366 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 17 Sep 2019 21:44:31 -0500 Subject: [PATCH 2/6] isMessageInNarrow: Fix check for starred or mentioned messages. Newly arriving messages did not appear in the is:starred narrow if they were starred, or in the is:mentioned narrow if they contained an @-mention (either direct or wildcard). In isMessageInNarrow (src/utils/narrow.js), instead of checking if message.type is 'starred' or 'mentioned' -- which didn't work, because it never has those values -- we now correctly check for the relevant values in message.flags . Also add tests for this logic. We also assert (with an error) that message.flags is defined. It should be; see parent commit, and if not, this will help us catch the bug in dev. Add a test for this exception, and update existing tests to include message.flags. Fixes: #3592 --- src/chat/__tests__/narrowsReducer-test.js | 12 ++++-- src/utils/__tests__/narrow-test.js | 52 ++++++++++++++++++++++- src/utils/narrow.js | 9 +++- 3 files changed, 67 insertions(+), 6 deletions(-) diff --git a/src/chat/__tests__/narrowsReducer-test.js b/src/chat/__tests__/narrowsReducer-test.js index cae124d6ad2..080cdccafe1 100644 --- a/src/chat/__tests__/narrowsReducer-test.js +++ b/src/chat/__tests__/narrowsReducer-test.js @@ -37,7 +37,7 @@ describe('narrowsReducer', () => { const action = deepFreeze({ type: EVENT_NEW_MESSAGE, - message: { id: 3, display_recipient: 'some stream', subject: 'some topic' }, + message: { id: 3, display_recipient: 'some stream', subject: 'some topic', flags: [] }, caughtUp: {}, }); @@ -57,7 +57,7 @@ describe('narrowsReducer', () => { const action = deepFreeze({ type: EVENT_NEW_MESSAGE, - message: { id: 3 }, + message: { id: 3, flags: [] }, caughtUp: { [HOME_NARROW_STR]: { older: false, @@ -83,7 +83,7 @@ describe('narrowsReducer', () => { const action = deepFreeze({ type: EVENT_NEW_MESSAGE, - message: { id: 3, type: 'stream', display_recipient: 'stream', subject: 'topic' }, + message: { id: 3, type: 'stream', display_recipient: 'stream', subject: 'topic', flags: [] }, caughtUp: {}, }); @@ -104,6 +104,7 @@ describe('narrowsReducer', () => { id: 1, type: 'private', display_recipient: [{ email: 'me@example.com' }, { email: 'a@a.com' }, { email: 'b@b.com' }], + flags: [], }; const action = deepFreeze({ type: EVENT_NEW_MESSAGE, @@ -132,6 +133,7 @@ describe('narrowsReducer', () => { type: 'stream', display_recipient: 'some stream', subject: 'some topic', + flags: [], }; const action = deepFreeze({ type: EVENT_NEW_MESSAGE, @@ -167,6 +169,7 @@ describe('narrowsReducer', () => { const message = { id: 1, display_recipient: [{ email: 'me@example.com' }], + flags: [], }; const action = deepFreeze({ type: EVENT_NEW_MESSAGE, @@ -204,6 +207,7 @@ describe('narrowsReducer', () => { type: 'stream', display_recipient: 'some stream', subject: 'some topic', + flags: [], }; const action = deepFreeze({ type: EVENT_NEW_MESSAGE, @@ -240,6 +244,7 @@ describe('narrowsReducer', () => { type: 'stream', display_recipient: 'stream name', subject: 'some topic', + flags: [], }; const action = deepFreeze({ type: EVENT_NEW_MESSAGE, @@ -274,6 +279,7 @@ describe('narrowsReducer', () => { type: 'private', sender_email: 'someone@example.com', display_recipient: [{ email: 'me@example.com' }, { email: 'mark@example.com' }], + flags: [], }; const action = deepFreeze({ type: EVENT_NEW_MESSAGE, diff --git a/src/utils/__tests__/narrow-test.js b/src/utils/__tests__/narrow-test.js index ab663d03019..50e08d19dc3 100644 --- a/src/utils/__tests__/narrow-test.js +++ b/src/utils/__tests__/narrow-test.js @@ -21,6 +21,7 @@ import { getNarrowFromMessage, parseNarrowString, STARRED_NARROW, + MENTIONED_NARROW, } from '../narrow'; describe('HOME_NARROW', () => { @@ -219,13 +220,16 @@ describe('SEARCH_NARROW', () => { describe('isMessageInNarrow', () => { test('any message is in "Home"', () => { - const message = {}; + const message = { + flags: [], + }; const narrow = HOME_NARROW; expect(isMessageInNarrow(message, narrow)).toBe(true); }); test('message with type "private" is in private narrow if recipient matches', () => { const message = { + flags: [], type: 'private', display_recipient: [{ email: 'me@example.com' }, { email: 'john@example.com' }], }; @@ -236,6 +240,7 @@ describe('isMessageInNarrow', () => { test('message to self is in "private" narrow with self', () => { const message = { + flags: [], type: 'private', display_recipient: [{ email: 'me@example.com' }], }; @@ -247,6 +252,7 @@ describe('isMessageInNarrow', () => { test('message with type "private" is in group narrow if all recipients match ', () => { const message = { type: 'private', + flags: [], display_recipient: [ { email: 'me@example.com' }, { email: 'john@example.com' }, @@ -261,6 +267,7 @@ describe('isMessageInNarrow', () => { test('message with type "private" is always in "private messages" narrow', () => { const message = { + flags: [], type: 'private', display_recipient: [{ email: 'me@example.com' }, { email: 'john@example.com' }], }; @@ -269,6 +276,7 @@ describe('isMessageInNarrow', () => { test('message with type "stream" is in narrow if recipient and current stream match', () => { const message = { + flags: [], type: 'stream', display_recipient: 'some stream', }; @@ -277,11 +285,53 @@ describe('isMessageInNarrow', () => { expect(isMessageInNarrow(message, narrow)).toBe(true); }); + test('message with flags undefined throws an error', () => { + const message = { + // no flags key + }; + expect(() => isMessageInNarrow(message, MENTIONED_NARROW)).toThrow(); + }); + + test('message with flag "mentioned" is in is:mentioned narrow', () => { + const message = { + flags: ['mentioned'], + }; + expect(isMessageInNarrow(message, MENTIONED_NARROW)).toBe(true); + }); + + test('message with flag "wildcard_mentioned" is in is:mentioned narrow', () => { + const message = { + flags: ['wildcard_mentioned'], + }; + expect(isMessageInNarrow(message, MENTIONED_NARROW)).toBe(true); + }); + + test('message without flag "mentioned" or "wildcard_mentioned" is not in is:mentioned narrow', () => { + const message = { + flags: [], + }; + expect(isMessageInNarrow(message, MENTIONED_NARROW)).toBe(false); + }); + + test('message with flag "starred" is in is:starred narrow', () => { + const message = { + flags: ['starred'], + }; + expect(isMessageInNarrow(message, STARRED_NARROW)).toBe(true); + }); + test('message without flag "starred" is not in is:starred narrow', () => { + const message = { + flags: [], + }; + expect(isMessageInNarrow(message, STARRED_NARROW)).toBe(false); + }); + test('message with type stream is in topic narrow if current stream and topic match with its own', () => { const message = { type: 'stream', subject: 'some topic', display_recipient: 'some stream', + flags: [] }; const narrow = topicNarrow('some stream', 'some topic'); diff --git a/src/utils/narrow.js b/src/utils/narrow.js index 07e01ebbf77..b5184b5b10d 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -214,6 +214,11 @@ export const isMessageInNarrow = (message: Message, narrow: Narrow, ownEmail: st return normalizedRecipients === ownEmail || normalizedRecipients === normalizedNarrow; }; + const { flags } = message; + if (!flags) { + throw new Error('`message.flags` should be defined.'); + } + return caseNarrow(narrow, { home: () => true, stream: name => name === message.display_recipient, @@ -221,8 +226,8 @@ export const isMessageInNarrow = (message: Message, narrow: Narrow, ownEmail: st streamName === message.display_recipient && topic === message.subject, pm: email => matchRecipients([email]), groupPm: matchRecipients, - starred: () => message.type === 'starred', - mentioned: () => message.type === 'mentioned', + starred: () => flags.includes('starred'), + mentioned: () => flags.includes('mentioned') || flags.includes('wildcard_mentioned'), allPrivate: () => message.type === 'private', search: () => false, }); From ba258b15682058d8f7528c871b28bfe6610b2368 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 17 Sep 2019 22:49:08 -0500 Subject: [PATCH 3/6] mentions: Check also for 'wildcard_mentioned' flag, in two places. Everywhere that we want to check direct mentions, we also want to check wildcard mentions. Add such a check in two spots. Also convert the case where action.message.flags is missing into an exception, rather than a silent no-flags-are-set, for the same reasons as in the parent commit. --- src/chat/__tests__/narrowsReducer-test.js | 8 +++++++- src/events/eventMiddleware.js | 7 ++++++- src/unread/unreadMentionsReducer.js | 10 +++++++--- src/utils/__tests__/narrow-test.js | 2 +- 4 files changed, 21 insertions(+), 6 deletions(-) diff --git a/src/chat/__tests__/narrowsReducer-test.js b/src/chat/__tests__/narrowsReducer-test.js index 080cdccafe1..b88f5397683 100644 --- a/src/chat/__tests__/narrowsReducer-test.js +++ b/src/chat/__tests__/narrowsReducer-test.js @@ -83,7 +83,13 @@ describe('narrowsReducer', () => { const action = deepFreeze({ type: EVENT_NEW_MESSAGE, - message: { id: 3, type: 'stream', display_recipient: 'stream', subject: 'topic', flags: [] }, + message: { + id: 3, + type: 'stream', + display_recipient: 'stream', + subject: 'topic', + flags: [], + }, caughtUp: {}, }); diff --git a/src/events/eventMiddleware.js b/src/events/eventMiddleware.js index eb346dc2016..af721f68625 100644 --- a/src/events/eventMiddleware.js +++ b/src/events/eventMiddleware.js @@ -23,7 +23,12 @@ export default (state: GlobalState, event_: GeneralEvent) => { const isActive = getIsActive(state); const isPrivateMessage = Array.isArray(event.message.display_recipient); - const isMentioned = event.message.flags && event.message.flags.includes('mentioned'); + + const { flags } = event.message; + if (!flags) { + throw new Error('event.message.flags should be defined'); + } + const isMentioned = flags.includes('mentioned') || flags.includes('wildcard_mentioned'); if (!isActive || !(isPrivateMessage || isMentioned)) { break; } diff --git a/src/unread/unreadMentionsReducer.js b/src/unread/unreadMentionsReducer.js index 2015210aa5c..43e4b0df5e8 100644 --- a/src/unread/unreadMentionsReducer.js +++ b/src/unread/unreadMentionsReducer.js @@ -40,12 +40,16 @@ export default (state: UnreadMentionsState = initialState, action: Action): Unre case REALM_INIT: return (action.data.unread_msgs && action.data.unread_msgs.mentions) || initialState; - case EVENT_NEW_MESSAGE: - return action.message.flags - && action.message.flags.includes('mentioned') + case EVENT_NEW_MESSAGE: { + const { flags } = action.message; + if (!flags) { + throw new Error('action.message.flags should be defined.'); + } + return (flags.includes('mentioned') || flags.includes('wildcard_mentioned')) && !state.includes(action.message.id) ? addItemsToArray(state, [action.message.id]) : state; + } case EVENT_MESSAGE_DELETE: return removeItemsFromArray(state, [action.messageId]); diff --git a/src/utils/__tests__/narrow-test.js b/src/utils/__tests__/narrow-test.js index 50e08d19dc3..ce7c135e296 100644 --- a/src/utils/__tests__/narrow-test.js +++ b/src/utils/__tests__/narrow-test.js @@ -331,7 +331,7 @@ describe('isMessageInNarrow', () => { type: 'stream', subject: 'some topic', display_recipient: 'some stream', - flags: [] + flags: [], }; const narrow = topicNarrow('some stream', 'some topic'); From 1a79929f6aa40718912a0c417d9eab4d2b6c294b Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 17 Sep 2019 23:48:48 -0500 Subject: [PATCH 4/6] narrowsReducer: Handle mentions in EVENT_UPDATE_MESSAGE_FLAGS. The 'starred' flag was already handled for this action type. Add handling for 'mentioned' and 'wildcard_mentioned'. To avoid repeating code, the shared logic was extracted into a helper function updateFlagNarrow. --- src/chat/narrowsReducer.js | 31 ++++++++++++++++++++----------- src/utils/narrow.js | 2 ++ 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/src/chat/narrowsReducer.js b/src/chat/narrowsReducer.js index 2bfee24b01a..3b1926b3b54 100644 --- a/src/chat/narrowsReducer.js +++ b/src/chat/narrowsReducer.js @@ -13,7 +13,7 @@ import { EVENT_UPDATE_MESSAGE_FLAGS, } from '../actionConstants'; import { LAST_MESSAGE_ANCHOR, FIRST_UNREAD_ANCHOR } from '../constants'; -import { isMessageInNarrow, STARRED_NARROW_STR } from '../utils/narrow'; +import { isMessageInNarrow, MENTIONED_NARROW_STR, STARRED_NARROW_STR } from '../utils/narrow'; import { NULL_OBJECT } from '../nullObjects'; const initialState: NarrowsState = NULL_OBJECT; @@ -64,17 +64,26 @@ const eventUpdateMessageFlags = (state, action) => { const messagesSet = new Set(messages); const updates = []; - if (flag === 'starred' && state[STARRED_NARROW_STR]) { - if (operation === 'add') { - updates.push({ - [STARRED_NARROW_STR]: [...state[STARRED_NARROW_STR], ...messages].sort(), - }); - } else { - // operation === 'remove' - updates.push({ - [STARRED_NARROW_STR]: state[STARRED_NARROW_STR].filter(id => !messagesSet.has(id)), - }); + const updateFlagNarrow = narrowStr => { + if (state[narrowStr]) { + if (operation === 'add') { + updates.push({ + [narrowStr]: [...state[narrowStr], ...messages].sort(), + }); + } else if (operation === 'remove') { + updates.push({ + [narrowStr]: state[narrowStr].filter(id => !messagesSet.has(id)), + }); + } } + + return updates; + }; + + if (flag === 'starred') { + updateFlagNarrow(STARRED_NARROW_STR); + } else if (['mentioned', 'wildcard_mentioned'].includes(flag)) { + updateFlagNarrow(MENTIONED_NARROW_STR); } if (!updates.length) { diff --git a/src/utils/narrow.js b/src/utils/narrow.js index b5184b5b10d..0ea5e25c45c 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -41,6 +41,8 @@ export const STARRED_NARROW_STR = JSON.stringify(STARRED_NARROW); export const MENTIONED_NARROW = specialNarrow('mentioned'); +export const MENTIONED_NARROW_STR = JSON.stringify(MENTIONED_NARROW); + export const ALL_PRIVATE_NARROW = specialNarrow('private'); export const ALL_PRIVATE_NARROW_STR = JSON.stringify(ALL_PRIVATE_NARROW); From 6bc328b41af2df2fd896f1ad693769c0985711b0 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 20 Sep 2019 01:22:00 -0500 Subject: [PATCH 5/6] narrowsReducer: Assert operation is add | remove on EVENT_UPDATE_MESSAGE_FLAGS. Add an assertion to match the type in EventUpdateMessageFlagsAction in src/actionTypes.js, so that action.operation can only be 'add' or 'remove'. --- src/chat/narrowsReducer.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/chat/narrowsReducer.js b/src/chat/narrowsReducer.js index 3b1926b3b54..34795363c5b 100644 --- a/src/chat/narrowsReducer.js +++ b/src/chat/narrowsReducer.js @@ -74,6 +74,10 @@ const eventUpdateMessageFlags = (state, action) => { updates.push({ [narrowStr]: state[narrowStr].filter(id => !messagesSet.has(id)), }); + } else { + throw new Error( + `Unexpected operation ${operation} in an EVENT_UPDATE_MESSAGE_FLAGS action`, + ); } } From 4f8ceb04ba088b22b1d529f1e539758a41715921 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 20 Sep 2019 17:04:56 -0700 Subject: [PATCH 6/6] narrowsReducer: Check `operation` match is exhaustive statically, too. This would cause Flow to give an error if there were any way to reach this case consistent with the stated types. The line that throws an exception is still helpful too, because like much of our data this action object comes largely from the server -- so there's no static guarantee that it actually *does* follow the stated types. (This is an area where we don't currently have much of a "crunchy shell".) --- src/chat/narrowsReducer.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/chat/narrowsReducer.js b/src/chat/narrowsReducer.js index 34795363c5b..4f75ad496b4 100644 --- a/src/chat/narrowsReducer.js +++ b/src/chat/narrowsReducer.js @@ -2,6 +2,7 @@ import union from 'lodash.union'; import type { NarrowsState, Action } from '../types'; +import { ensureUnreachable } from '../types'; import { DEAD_QUEUE, LOGOUT, @@ -75,6 +76,7 @@ const eventUpdateMessageFlags = (state, action) => { [narrowStr]: state[narrowStr].filter(id => !messagesSet.has(id)), }); } else { + ensureUnreachable(operation); throw new Error( `Unexpected operation ${operation} in an EVENT_UPDATE_MESSAGE_FLAGS action`, );