From 5076aae16f5bcc1502c6d55c47265a1c85e86cc6 Mon Sep 17 00:00:00 2001 From: Wesley Aptekar-Cassels Date: Sat, 1 May 2021 16:57:23 +0800 Subject: [PATCH] unreads: Don't add messages with "read" flag to unreads. Previously, the main (only?) case where we wouldn't add a message to unreads was if we were the one who sent it. This check was not correct - instead, we want to check to see if the message has the "read" flag, which is set by the server. In the past, these checks were very close to the same (with the exception of "Notification Bot" announcing a stream that you created, which would be "read" for you), but with the addition of muted users, the check is now incorrect. This commit removes the logic to not mark a message as read when we are the one who sent it, and instead just looks at the server flag, since that should be the canonical place for that information to be. --- .../__tests__/unreadHuddlesReducer-test.js | 5 +++-- .../__tests__/unreadMentionsReducer-test.js | 16 ++++++++++++++++ src/unread/__tests__/unreadModel-test.js | 4 ++-- src/unread/__tests__/unreadPmsReducer-test.js | 5 +++-- src/unread/unreadHuddlesReducer.js | 2 +- src/unread/unreadMentionsReducer.js | 3 +++ src/unread/unreadModel.js | 3 +-- src/unread/unreadPmsReducer.js | 2 +- 8 files changed, 30 insertions(+), 10 deletions(-) diff --git a/src/unread/__tests__/unreadHuddlesReducer-test.js b/src/unread/__tests__/unreadHuddlesReducer-test.js index 6d2fa1042fd..0f9a28e6172 100644 --- a/src/unread/__tests__/unreadHuddlesReducer-test.js +++ b/src/unread/__tests__/unreadHuddlesReducer-test.js @@ -118,7 +118,7 @@ describe('unreadHuddlesReducer', () => { expect(actualState).toBe(initialState); }); - test('if message is sent by self, do not mutate state', () => { + test('if message has "read" flag, do not mutate state', () => { const selfUser = { ...eg.selfUser, user_id: makeUserId(1) }; const user2 = { ...eg.otherUser, user_id: makeUserId(2) }; const user3 = { ...eg.thirdUser, user_id: makeUserId(3) }; @@ -126,8 +126,9 @@ describe('unreadHuddlesReducer', () => { const initialState = deepFreeze([]); const message2 = eg.pmMessage({ - sender: selfUser, + sender: user2, recipients: [selfUser, user2, user3], + flags: ['read'], }); const action = deepFreeze({ diff --git a/src/unread/__tests__/unreadMentionsReducer-test.js b/src/unread/__tests__/unreadMentionsReducer-test.js index 239ea6d7cef..7ce7311591a 100644 --- a/src/unread/__tests__/unreadMentionsReducer-test.js +++ b/src/unread/__tests__/unreadMentionsReducer-test.js @@ -67,6 +67,22 @@ describe('unreadMentionsReducer', () => { expect(actualState).toBe(initialState); }); + test('if message has "read" flag, do not mutate state', () => { + const initialState = deepFreeze([]); + + const action = deepFreeze({ + type: EVENT_NEW_MESSAGE, + message: { + id: 2, + flags: ['mentioned', 'read'], + }, + }); + + const actualState = unreadMentionsReducer(initialState, action); + + expect(actualState).toBe(initialState); + }); + test('if message id already exists, do not mutate state', () => { const initialState = deepFreeze([1, 2]); diff --git a/src/unread/__tests__/unreadModel-test.js b/src/unread/__tests__/unreadModel-test.js index 0cc91bad201..9aa3227ba4b 100644 --- a/src/unread/__tests__/unreadModel-test.js +++ b/src/unread/__tests__/unreadModel-test.js @@ -84,10 +84,10 @@ describe('stream substate', () => { expect(state.streams).toBe(baseState.streams); }); - test('if message is sent by self, do not mutate state', () => { + test('if message has "read" flag, do not mutate state', () => { const state = reducer( baseState, - action(eg.streamMessage({ sender: eg.selfUser })), + action(eg.streamMessage({ sender: eg.selfUser, flags: ['read'] })), eg.plusReduxState, ); expect(state).toBe(baseState); diff --git a/src/unread/__tests__/unreadPmsReducer-test.js b/src/unread/__tests__/unreadPmsReducer-test.js index a571c0c27c6..16f14edfc27 100644 --- a/src/unread/__tests__/unreadPmsReducer-test.js +++ b/src/unread/__tests__/unreadPmsReducer-test.js @@ -111,11 +111,12 @@ describe('unreadPmsReducer', () => { expect(actualState).toBe(initialState); }); - test('if message is sent by self, do not mutate state', () => { + test('if message is marked read, do not mutate state', () => { const initialState = deepFreeze([]); const message1 = eg.pmMessage({ - sender: eg.selfUser, + sender: eg.otherUser, recipients: [eg.otherUser, eg.selfUser], + flags: ['read'], }); const action = deepFreeze({ diff --git a/src/unread/unreadHuddlesReducer.js b/src/unread/unreadHuddlesReducer.js index efa06de2820..b04a653c407 100644 --- a/src/unread/unreadHuddlesReducer.js +++ b/src/unread/unreadHuddlesReducer.js @@ -24,7 +24,7 @@ const eventNewMessage = (state, action) => { return state; } - if (action.ownUserId === action.message.sender_id) { + if (action.message.flags?.includes('read')) { return state; } diff --git a/src/unread/unreadMentionsReducer.js b/src/unread/unreadMentionsReducer.js index 60024d1b027..37bdf29cdcd 100644 --- a/src/unread/unreadMentionsReducer.js +++ b/src/unread/unreadMentionsReducer.js @@ -46,6 +46,9 @@ export default (state: UnreadMentionsState = initialState, action: Action): Unre if (!flags) { throw new Error('action.message.flags should be defined.'); } + if (flags.includes('read')) { + return state; + } return (flags.includes('mentioned') || flags.includes('wildcard_mentioned')) && !state.includes(action.message.id) ? addItemsToArray(state, [action.message.id]) diff --git a/src/unread/unreadModel.js b/src/unread/unreadModel.js index f8c4deac1e0..b47d036bd11 100644 --- a/src/unread/unreadModel.js +++ b/src/unread/unreadModel.js @@ -22,7 +22,6 @@ import { MESSAGE_FETCH_COMPLETE, REALM_INIT, } from '../actionConstants'; -import { getOwnUserId } from '../users/userSelectors'; // // @@ -146,7 +145,7 @@ function streamsReducer( return state; } - if (message.sender_id === getOwnUserId(globalState)) { + if (message.flags?.includes('read')) { return state; } diff --git a/src/unread/unreadPmsReducer.js b/src/unread/unreadPmsReducer.js index 6562789fc7a..6bf04512f2b 100644 --- a/src/unread/unreadPmsReducer.js +++ b/src/unread/unreadPmsReducer.js @@ -24,7 +24,7 @@ const eventNewMessage = (state, action) => { return state; } - if (action.ownUserId === action.message.sender_id) { + if (action.message.flags?.includes('read')) { return state; }