Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

narrow: Treat group PMs and 1:1 PMs more symmetrically. #4330

Merged
merged 10 commits into from
Dec 8, 2020
4 changes: 2 additions & 2 deletions src/account-info/AccountDetailsScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { createStyleSheet } from '../styles';
import { connect } from '../react-redux';
import { Screen, ZulipButton, Label } from '../common';
import { IconPrivateChat } from '../common/Icons';
import { privateNarrow } from '../utils/narrow';
import { pmNarrowFromEmail } from '../utils/narrow';
import AccountDetails from './AccountDetails';
import { doNarrow } from '../actions';
import { getUserIsActive, getUserForId } from '../users/userSelectors';
Expand Down Expand Up @@ -43,7 +43,7 @@ type Props = $ReadOnly<{|
class AccountDetailsScreen extends PureComponent<Props> {
handleChatPress = () => {
const { user, dispatch } = this.props;
dispatch(doNarrow(privateNarrow(user.email)));
dispatch(doNarrow(pmNarrowFromEmail(user.email)));
};

render() {
Expand Down
16 changes: 9 additions & 7 deletions src/chat/__tests__/narrowsReducer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ import narrowsReducer from '../narrowsReducer';
import {
HOME_NARROW,
HOME_NARROW_STR,
privateNarrow,
pmNarrowFromEmail,
ALL_PRIVATE_NARROW_STR,
groupNarrow,
pmNarrowFromEmails,
streamNarrow,
topicNarrow,
STARRED_NARROW_STR,
Expand All @@ -22,8 +22,10 @@ import { LAST_MESSAGE_ANCHOR, FIRST_UNREAD_ANCHOR } from '../../anchor';
import * as eg from '../../__tests__/lib/exampleData';

describe('narrowsReducer', () => {
const privateNarrowStr = JSON.stringify(privateNarrow(eg.otherUser.email));
const groupNarrowStr = JSON.stringify(groupNarrow([eg.otherUser.email, eg.thirdUser.email]));
const privateNarrowStr = JSON.stringify(pmNarrowFromEmail(eg.otherUser.email));
const groupNarrowStr = JSON.stringify(
pmNarrowFromEmails([eg.otherUser.email, eg.thirdUser.email]),
);
const streamNarrowStr = JSON.stringify(streamNarrow(eg.stream.name));
const egTopic = eg.streamMessage().subject;
const topicNarrowStr = JSON.stringify(topicNarrow(eg.stream.name, egTopic));
Expand Down Expand Up @@ -157,7 +159,7 @@ describe('narrowsReducer', () => {
});

test('message sent to self is stored correctly', () => {
const narrowWithSelfStr = JSON.stringify(privateNarrow(eg.selfUser.email));
const narrowWithSelfStr = JSON.stringify(pmNarrowFromEmail(eg.selfUser.email));
const initialState = deepFreeze({
[HOME_NARROW_STR]: [],
[narrowWithSelfStr]: [],
Expand Down Expand Up @@ -406,7 +408,7 @@ describe('narrowsReducer', () => {
const action = deepFreeze({
type: MESSAGE_FETCH_COMPLETE,
anchor: 2,
narrow: privateNarrow(eg.otherUser.email),
narrow: pmNarrowFromEmail(eg.otherUser.email),
messages: [],
numBefore: 100,
numAfter: 100,
Expand All @@ -416,7 +418,7 @@ describe('narrowsReducer', () => {

const expectedState = {
[HOME_NARROW_STR]: [1, 2, 3],
[JSON.stringify(privateNarrow(eg.otherUser.email))]: [],
[JSON.stringify(pmNarrowFromEmail(eg.otherUser.email))]: [],
};

const newState = narrowsReducer(initialState, action);
Expand Down
32 changes: 17 additions & 15 deletions src/chat/__tests__/narrowsSelectors-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@ import {
import {
HOME_NARROW,
HOME_NARROW_STR,
privateNarrow,
pmNarrowFromEmail,
streamNarrow,
topicNarrow,
STARRED_NARROW,
groupNarrow,
pmNarrowFromEmails,
} from '../../utils/narrow';
import { NULL_SUBSCRIPTION } from '../../nullObjects';
import * as eg from '../../__tests__/lib/exampleData';
Expand Down Expand Up @@ -78,14 +78,14 @@ describe('getMessagesForNarrow', () => {
test('do not combine messages and outbox in different narrow', () => {
const state = eg.reduxState({
narrows: {
[JSON.stringify(privateNarrow('john@example.com'))]: [123],
[JSON.stringify(pmNarrowFromEmail('john@example.com'))]: [123],
},
messages,
outbox: [outboxMessage],
realm: eg.realmState({ email: eg.selfUser.email }),
});

const result = getMessagesForNarrow(state, privateNarrow('john@example.com'));
const result = getMessagesForNarrow(state, pmNarrowFromEmail('john@example.com'));

expect(result).toEqual([message]);
});
Expand Down Expand Up @@ -190,7 +190,7 @@ describe('getStreamInNarrow', () => {
});

test('return NULL_SUBSCRIPTION is narrow is not topic or stream', () => {
expect(getStreamInNarrow(state, privateNarrow('abc@zulip.com'))).toEqual(NULL_SUBSCRIPTION);
expect(getStreamInNarrow(state, pmNarrowFromEmail('abc@zulip.com'))).toEqual(NULL_SUBSCRIPTION);
expect(getStreamInNarrow(state, topicNarrow(stream4.name, 'topic'))).toEqual(NULL_SUBSCRIPTION);
});
});
Expand Down Expand Up @@ -253,7 +253,7 @@ describe('isNarrowValid', () => {
streams: [],
users: [user],
});
const narrow = privateNarrow(user.email);
const narrow = pmNarrowFromEmail(user.email);

const result = isNarrowValid(state, narrow);

Expand All @@ -271,14 +271,14 @@ describe('isNarrowValid', () => {
streams: [],
users: [],
});
const narrow = privateNarrow(user.email);
const narrow = pmNarrowFromEmail(user.email);

const result = isNarrowValid(state, narrow);

expect(result).toBe(false);
});

test('narrowing to a group chat with non-existing user is not valid', () => {
test('narrowing to a group chat with existing users is valid', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4f14e6b narrow: Fix isNarrowValid for group PMs.

Oddly, there was a test checking that this behavior was this way, so
it was noticed before.  It's not clear why it was thought desirable,
though.  The test was added in cefbe6d in 2018-05, long after the
logic itself was written in b0c7323 in 2017-12, and neither commit
explains the discrepancy.

Weird!

const john = eg.makeUser({ name: 'john' });
const mark = eg.makeUser({ name: 'mark' });

Expand All @@ -291,28 +291,30 @@ describe('isNarrowValid', () => {
streams: [],
users: [john, mark],
});
const narrow = groupNarrow([john.email, mark.email]);
const narrow = pmNarrowFromEmails([john.email, mark.email]);

const result = isNarrowValid(state, narrow);

expect(result).toBe(true);
});

test('narrowing to a group chat with non-existing users is also valid', () => {
test('narrowing to a group chat with non-existing users is not valid', () => {
const john = eg.makeUser({ name: 'john' });
const mark = eg.makeUser({ name: 'mark' });
const state = eg.reduxState({
realm: {
...eg.realmState(),
crossRealmBots: [],
nonActiveUsers: [],
},
streams: [],
users: [],
users: [john],
});
const narrow = groupNarrow(['john@example.com', 'mark@example.com']);
const narrow = pmNarrowFromEmails([john.email, mark.email]);

const result = isNarrowValid(state, narrow);

expect(result).toBe(true);
expect(result).toBe(false);
});

test('narrowing to a PM with bots is valid', () => {
Expand All @@ -326,7 +328,7 @@ describe('isNarrowValid', () => {
streams: [],
users: [],
});
const narrow = privateNarrow(bot.email);
const narrow = pmNarrowFromEmail(bot.email);

const result = isNarrowValid(state, narrow);

Expand All @@ -344,7 +346,7 @@ describe('isNarrowValid', () => {
streams: [],
users: [],
});
const narrow = privateNarrow(notActiveUser.email);
const narrow = pmNarrowFromEmail(notActiveUser.email);

const result = isNarrowValid(state, narrow);

Expand Down
24 changes: 12 additions & 12 deletions src/chat/narrowsSelectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@ import {
import { getCaughtUpForNarrow } from '../caughtup/caughtUpSelectors';
import { getAllUsersByEmail, getOwnEmail } from '../users/userSelectors';
import {
isPrivateNarrow,
isStreamOrTopicNarrow,
emailsOfGroupNarrow,
isMessageInNarrow,
caseNarrowDefault,
} from '../utils/narrow';
import { shouldBeMuted } from '../utils/message';
import { NULL_ARRAY, NULL_SUBSCRIPTION } from '../nullObjects';
Expand Down Expand Up @@ -146,15 +146,15 @@ export const isNarrowValid: Selector<boolean, Narrow> = createSelector(
(state, narrow) => narrow,
state => getStreams(state),
state => getAllUsersByEmail(state),
(narrow, streams, allUsersByEmail) => {
if (isStreamOrTopicNarrow(narrow)) {
return streams.find(s => s.name === narrow[0].operand) !== undefined;
}

if (isPrivateNarrow(narrow)) {
return allUsersByEmail.get(narrow[0].operand) !== undefined;
}

return true;
},
(narrow, streams, allUsersByEmail) =>
caseNarrowDefault(
narrow,
{
stream: streamName => streams.find(s => s.name === streamName) !== undefined,
topic: streamName => streams.find(s => s.name === streamName) !== undefined,
pm: email => allUsersByEmail.get(email) !== undefined,
groupPm: emails => emails.every(email => allUsersByEmail.get(email) !== undefined),
},
() => true,
),
);
13 changes: 9 additions & 4 deletions src/compose/__tests__/getComposeInputPlaceholder-test.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
import deepFreeze from 'deep-freeze';

import getComposeInputPlaceholder from '../getComposeInputPlaceholder';
import { privateNarrow, streamNarrow, topicNarrow, groupNarrow } from '../../utils/narrow';
import {
pmNarrowFromEmail,
streamNarrow,
topicNarrow,
pmNarrowFromEmails,
} from '../../utils/narrow';

describe('getComposeInputPlaceholder', () => {
test('returns "Message @ThisPerson" object for person narrow', () => {
const narrow = deepFreeze(privateNarrow('abc@zulip.com'));
const narrow = deepFreeze(pmNarrowFromEmail('abc@zulip.com'));

const ownEmail = 'hamlet@zulip.com';

Expand Down Expand Up @@ -33,7 +38,7 @@ describe('getComposeInputPlaceholder', () => {
});

test('returns "Jot down something" object for self narrow', () => {
const narrow = deepFreeze(privateNarrow('abc@zulip.com'));
const narrow = deepFreeze(pmNarrowFromEmail('abc@zulip.com'));

const ownEmail = 'abc@zulip.com';

Expand All @@ -58,7 +63,7 @@ describe('getComposeInputPlaceholder', () => {
});

test('returns "Message group" object for group narrow', () => {
const narrow = deepFreeze(groupNarrow(['abc@zulip.com, xyz@zulip.com']));
const narrow = deepFreeze(pmNarrowFromEmails(['abc@zulip.com, xyz@zulip.com']));

const placeholder = getComposeInputPlaceholder(narrow);
expect(placeholder).toEqual({ text: 'Message group' });
Expand Down
6 changes: 3 additions & 3 deletions src/notification/__tests__/notification-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import deepFreeze from 'deep-freeze';
import type { User } from '../../api/modelTypes';
import type { JSONableDict } from '../../utils/jsonable';
import { getNarrowFromNotificationData } from '..';
import { topicNarrow, privateNarrow, groupNarrow } from '../../utils/narrow';
import { topicNarrow, pmNarrowFromEmail, pmNarrowFromEmails } from '../../utils/narrow';

import * as eg from '../../__tests__/lib/exampleData';
import { fromAPNsImpl as extractIosNotificationData } from '../extract';
Expand Down Expand Up @@ -37,7 +37,7 @@ describe('getNarrowFromNotificationData', () => {
sender_email: 'mark@example.com',
};
const narrow = getNarrowFromNotificationData(notification, DEFAULT_MAP, ownUserId);
expect(narrow).toEqual(privateNarrow('mark@example.com'));
expect(narrow).toEqual(pmNarrowFromEmail('mark@example.com'));
});

test('on notification for a group message returns a group narrow', () => {
Expand All @@ -49,7 +49,7 @@ describe('getNarrowFromNotificationData', () => {
pm_users: users.map(u => u.user_id).join(','),
};

const expectedNarrow = groupNarrow(users.slice(1).map(u => u.email));
const expectedNarrow = pmNarrowFromEmails(users.slice(1).map(u => u.email));

const narrow = getNarrowFromNotificationData(notification, usersById, ownUserId);

Expand Down
6 changes: 3 additions & 3 deletions src/notification/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import NotificationsIOS from 'react-native-notifications';

import type { Notification } from './types';
import type { Auth, Dispatch, Identity, Narrow, User } from '../types';
import { topicNarrow, privateNarrow, groupNarrow } from '../utils/narrow';
import { topicNarrow, pmNarrowFromEmail, pmNarrowFromEmails } from '../utils/narrow';
import type { JSONable, JSONableDict } from '../utils/jsonable';
import * as api from '../api';
import * as logging from '../utils/logging';
Expand Down Expand Up @@ -102,12 +102,12 @@ export const getNarrowFromNotificationData = (
}

if (data.pm_users === undefined) {
return privateNarrow(data.sender_email);
return pmNarrowFromEmail(data.sender_email);
}

const ids = data.pm_users.split(',').map(s => parseInt(s, 10));
const users = pmKeyRecipientsFromIds(ids, usersById, ownUserId);
return users && groupNarrow(users.map(u => u.email));
return users && pmNarrowFromEmails(users.map(u => u.email));
};

const getInitialNotification = async (): Promise<Notification | null> => {
Expand Down
6 changes: 3 additions & 3 deletions src/pm-conversations/PmConversationList.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { FlatList } from 'react-native';

import type { Dispatch, PmConversationData, UserOrBot } from '../types';
import { createStyleSheet } from '../styles';
import { privateNarrow, groupNarrow } from '../utils/narrow';
import { pmNarrowFromEmail, pmNarrowFromEmails } from '../utils/narrow';
import UserItem from '../users/UserItem';
import GroupPmConversationItem from './GroupPmConversationItem';
import { doNarrow } from '../actions';
Expand All @@ -27,11 +27,11 @@ type Props = $ReadOnly<{|
* */
export default class PmConversationList extends PureComponent<Props> {
handleUserNarrow = (user: UserOrBot) => {
this.props.dispatch(doNarrow(privateNarrow(user.email)));
this.props.dispatch(doNarrow(pmNarrowFromEmail(user.email)));
};

handleGroupNarrow = (email: string) => {
this.props.dispatch(doNarrow(groupNarrow(email.split(','))));
this.props.dispatch(doNarrow(pmNarrowFromEmails(email.split(','))));
};

render() {
Expand Down
10 changes: 5 additions & 5 deletions src/title/__tests__/titleSelectors-test.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import deepFreeze from 'deep-freeze';

import { DEFAULT_TITLE_BACKGROUND_COLOR, getTitleBackgroundColor } from '../titleSelectors';
import { groupNarrow, streamNarrow, privateNarrow } from '../../utils/narrow';
import { pmNarrowFromEmails, streamNarrow, pmNarrowFromEmail } from '../../utils/narrow';

const subscriptions = [{ name: 'all', color: '#fff' }, { name: 'announce', color: '#000' }];

Expand Down Expand Up @@ -35,11 +35,11 @@ describe('getTitleBackgroundColor', () => {
subscriptions,
});

expect(getTitleBackgroundColor(state, privateNarrow('abc@zulip.com'))).toEqual(
DEFAULT_TITLE_BACKGROUND_COLOR,
);
expect(getTitleBackgroundColor(state, groupNarrow(['abc@zulip.com', 'def@zulip.com']))).toEqual(
expect(getTitleBackgroundColor(state, pmNarrowFromEmail('abc@zulip.com'))).toEqual(
DEFAULT_TITLE_BACKGROUND_COLOR,
);
expect(
getTitleBackgroundColor(state, pmNarrowFromEmails(['abc@zulip.com', 'def@zulip.com'])),
).toEqual(DEFAULT_TITLE_BACKGROUND_COLOR);
});
});
Loading