Skip to content

Commit

Permalink
Do not add message to state if not caught up (zulip#1261)
Browse files Browse the repository at this point in the history
  • Loading branch information
borisyankov authored Oct 6, 2017
1 parent 1030ed9 commit e09648c
Show file tree
Hide file tree
Showing 6 changed files with 69 additions and 13 deletions.
51 changes: 49 additions & 2 deletions src/chat/__tests__/chatReducers-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,24 @@ describe('chatReducers', () => {
});

describe('EVENT_NEW_MESSAGE', () => {
test('if not caught up in narrow, do not add message', () => {
const initialState = deepFreeze({
messages: {
[homeNarrowStr]: [{ id: 1 }, { id: 2 }],
},
});

const action = deepFreeze({
type: EVENT_NEW_MESSAGE,
message: { id: 3 },
caughtUp: {},
});

const newState = chatReducers(initialState, action);

expect(newState).toBe(initialState);
});

test('appends message to state producing a copy of messages', () => {
const initialState = deepFreeze({
messages: {
Expand All @@ -62,6 +80,12 @@ describe('chatReducers', () => {
const action = deepFreeze({
type: EVENT_NEW_MESSAGE,
message: { id: 3 },
caughtUp: {
[homeNarrowStr]: {
older: false,
newer: true,
},
},
});

const expectedState = {
Expand All @@ -87,14 +111,20 @@ describe('chatReducers', () => {
const action = deepFreeze({
type: EVENT_NEW_MESSAGE,
message: { id: 2 },
caughtUp: {
[homeNarrowStr]: {
older: false,
newer: true,
},
},
});

const newState = chatReducers(initialState, action);

expect(newState).toBe(initialState);
});

test('Message sent to self is stored correctly', () => {
test('message sent to self is stored correctly', () => {
const narrowWithSelfStr = JSON.stringify(privateNarrow('[email protected]'));
const initialState = deepFreeze({
messages: {
Expand All @@ -111,6 +141,10 @@ describe('chatReducers', () => {
type: EVENT_NEW_MESSAGE,
ownEmail: '[email protected]',
message,
caughtUp: {
[homeNarrowStr]: { older: false, newer: true },
[narrowWithSelfStr]: { older: false, newer: true },
},
});

const expectedState = {
Expand All @@ -126,7 +160,7 @@ describe('chatReducers', () => {
expect(newState).not.toBe(initialState);
});

test('appends stream message to all cached narrows that match', () => {
test('appends stream message to all cached narrows that match and are caught up', () => {
const initialState = deepFreeze({
messages: {
[homeNarrowStr]: [{ id: 1 }, { id: 2 }],
Expand All @@ -147,6 +181,11 @@ describe('chatReducers', () => {
const action = deepFreeze({
type: EVENT_NEW_MESSAGE,
message,
caughtUp: {
[homeNarrowStr]: { older: false, newer: true },
[streamNarrowStr]: { older: false, newer: true },
[topicNarrowStr]: { older: false, newer: true },
},
});

const expectedState = {
Expand Down Expand Up @@ -182,6 +221,9 @@ describe('chatReducers', () => {
const action = deepFreeze({
type: EVENT_NEW_MESSAGE,
message,
caughtUp: {
[homeNarrowStr]: { older: false, newer: true },
},
});

const expectedState = {
Expand Down Expand Up @@ -218,6 +260,11 @@ describe('chatReducers', () => {
type: EVENT_NEW_MESSAGE,
message,
ownEmail: '[email protected]',
caughtUp: {
[homeNarrowStr]: { older: false, newer: true },
[allPrivateNarrowStr]: { older: false, newer: true },
[privateNarrowStr]: { older: false, newer: true },
},
});

const expectedState = {
Expand Down
10 changes: 9 additions & 1 deletion src/chat/chatReducers.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
EVENT_REACTION_REMOVE,
EVENT_UPDATE_MESSAGE,
} from '../actionConstants';
import { homeNarrow, isMessageInNarrow } from '../utils/narrow';
import { homeNarrow, isMessageInNarrow, getNarrowFromMessage } from '../utils/narrow';
import chatUpdater from './chatUpdater';
import { getMessagesById } from '../selectors';
import { NULL_ARRAY } from '../nullObjects';
Expand Down Expand Up @@ -86,13 +86,21 @@ export default (state: ChatState = initialState, action: Action) => {
}));

case EVENT_NEW_MESSAGE: {
// const narrow = getNarrowFromMessage(action.message);
// const caughtUp = action.caughtUp[JSON.stringify(narrow)];
// console.log('YOLO', narrow, caughtUp);
// if (!caughtUp || !caughtUp.newer) {
// return state;
// }

let stateChange = false;
const newState = {
...state,
messages: Object.keys(state.messages).reduce((msg, key) => {
const isInNarrow = isMessageInNarrow(action.message, JSON.parse(key), action.ownEmail);
if (
isInNarrow &&
(action.caughtUp[key] && action.caughtUp[key].newer) &&
state.messages[key].find(item => action.message.id === item.id) === undefined
) {
stateChange = true;
Expand Down
1 change: 1 addition & 0 deletions src/events/eventToAction.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ export default (state: GlobalState, event: Object) => {
return {
type: EVENT_NEW_MESSAGE,
message: event.message,
caughtUp: state.caughtUp,
ownEmail: state.accounts[0].email,
localMessageId: event.local_message_id,
};
Expand Down
6 changes: 3 additions & 3 deletions src/message/messageActionSheet.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* @flow */
import { Clipboard, Share } from 'react-native';
import type { Actions, Auth, Message, ActionSheetButtonType } from '../types';
import { narrowFromMessage, isHomeNarrow, isStreamNarrow, isSpecialNarrow } from '../utils/narrow';
import { getNarrowFromMessage, isHomeNarrow, isStreamNarrow, isSpecialNarrow } from '../utils/narrow';
import { getSingleMessage } from '../api';
import { isTopicMuted } from '../utils/message';
import muteTopicApi from '../api/muteTopic';
Expand Down Expand Up @@ -86,14 +86,14 @@ type AuthMessageAndNarrow = {
const isAnOutboxMessage = ({ message }: Message): boolean => message.isOutbox;

const narrowToConversation = ({ message, actions, auth, currentRoute }: MessageAndDoNarrowType) => {
actions.doNarrow(narrowFromMessage(message, auth.email), message.id);
actions.doNarrow(getNarrowFromMessage(message, auth.email), message.id);
if (currentRoute === 'search') {
actions.navigateBack();
}
};

const reply = ({ message, actions, auth, currentRoute, onReplySelect }: ReplyOptionType) => {
actions.doNarrow(narrowFromMessage(message, auth.email), message.id);
actions.doNarrow(getNarrowFromMessage(message, auth.email), message.id);
if (onReplySelect) onReplySelect(); // focus message input
if (currentRoute === 'search') {
actions.navigateBack();
Expand Down
12 changes: 6 additions & 6 deletions src/utils/__tests__/narrow-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
isPrivateOrGroupNarrow,
isMessageInNarrow,
isStreamOrTopicNarrow,
narrowFromMessage,
getNarrowFromMessage,
} from '../narrow';

describe('homeNarrow', () => {
Expand Down Expand Up @@ -274,7 +274,7 @@ describe('isMessageInNarrow', () => {
});
});

describe('narrowFromMessage', () => {
describe('getNarrowFromMessage', () => {
test('message with single recipient, returns a private narrow', () => {
const message = {
display_recipient: [{ email: '[email protected]' }],
Expand All @@ -284,7 +284,7 @@ describe('narrowFromMessage', () => {
};
const expectedNarrow = privateNarrow('[email protected]');

const actualNarrow = narrowFromMessage(message, auth.email);
const actualNarrow = getNarrowFromMessage(message, auth.email);

expect(actualNarrow).toEqual(expectedNarrow);
});
Expand All @@ -298,7 +298,7 @@ describe('narrowFromMessage', () => {
};
const expectedNarrow = groupNarrow(['[email protected]', '[email protected]']);

const actualNarrow = narrowFromMessage(message, auth.email);
const actualNarrow = getNarrowFromMessage(message, auth.email);

expect(actualNarrow).toEqual(expectedNarrow);
});
Expand All @@ -309,7 +309,7 @@ describe('narrowFromMessage', () => {
};
const expectedNarrow = streamNarrow('stream');

const actualNarrow = narrowFromMessage(message);
const actualNarrow = getNarrowFromMessage(message);

expect(actualNarrow).toEqual(expectedNarrow);
});
Expand All @@ -321,7 +321,7 @@ describe('narrowFromMessage', () => {
};
const expectedNarrow = topicNarrow('stream', 'subject');

const actualNarrow = narrowFromMessage(message);
const actualNarrow = getNarrowFromMessage(message);

expect(actualNarrow).toEqual(expectedNarrow);
});
Expand Down
2 changes: 1 addition & 1 deletion src/utils/narrow.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ export const canSendToNarrow = (narrow: Narrow): boolean =>
isStreamNarrow(narrow) ||
isTopicNarrow(narrow);

export const narrowFromMessage = (message: Message, email: string) => {
export const getNarrowFromMessage = (message: Message, email: string) => {
if (Array.isArray(message.display_recipient)) {
const recipient =
message.display_recipient.length > 1
Expand Down

0 comments on commit e09648c

Please sign in to comment.