Skip to content
4 changes: 2 additions & 2 deletions src/nav/__tests__/navReducer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { NULL_OBJECT } from '../../nullObjects';

describe('navReducer', () => {
describe('LOGIN_SUCCESS', () => {
test('replaces the existing route stack with "main" on sign in', () => {
test('replaces the existing route stack with "loading" on sign in', () => {
const prevState = deepFreeze({
index: 2,
routes: [{ key: 'one' }, { key: 'two' }, { key: 'password' }],
Expand All @@ -18,7 +18,7 @@ describe('navReducer', () => {

const expectedState = {
index: 0,
routes: [{ routeName: 'main' }],
routes: [{ routeName: 'loading' }],
};

const newState = navReducer(prevState, action);
Expand Down
4 changes: 1 addition & 3 deletions src/nav/navReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,8 @@ export default (state: NavigationState = initialState, action: Action): Navigati
return rehydrate(state, action);

case ACCOUNT_SWITCH:
return getStateForRoute('loading');

case LOGIN_SUCCESS:
return getStateForRoute('main');
return getStateForRoute('loading');

case INITIAL_FETCH_COMPLETE:
return state.routes[0].routeName === 'main' ? state : getStateForRoute('main');
Expand Down
11 changes: 11 additions & 0 deletions src/pm-conversations/__tests__/pmConversationsSelectors-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ describe('getRecentConversations', () => {
test('when no messages, return no conversations', () => {
const state = deepFreeze({
realm: { email: 'me@example.com' },
users: [{ user_id: 0, email: 'me@example.com' }],
narrows: {
[ALL_PRIVATE_NARROW_STR]: [],
},
Expand All @@ -24,6 +25,11 @@ describe('getRecentConversations', () => {
test('returns unique list of recipients, includes conversations with self', () => {
const state = deepFreeze({
realm: { email: 'me@example.com' },
users: [
{ user_id: 0, email: 'me@example.com' },
{ user_id: 1, email: 'john@example.com' },
{ user_id: 2, email: 'mark@example.com' },
],
narrows: {
[ALL_PRIVATE_NARROW_STR]: [0, 1, 2, 3, 4],
},
Expand Down Expand Up @@ -126,6 +132,11 @@ describe('getRecentConversations', () => {
test('returns recipients sorted by last activity', () => {
const state = deepFreeze({
realm: { email: 'me@example.com' },
users: [
{ user_id: 0, email: 'me@example.com' },
{ user_id: 1, email: 'john@example.com' },
{ user_id: 2, email: 'mark@example.com' },
],
narrows: {
[ALL_PRIVATE_NARROW_STR]: [1, 2, 3, 4, 5, 6],
},
Expand Down
12 changes: 6 additions & 6 deletions src/pm-conversations/pmConversationsSelectors.js
Original file line number Diff line number Diff line change
@@ -1,26 +1,26 @@
/* @flow strict-local */
import { createSelector } from 'reselect';

import type { Message, PmConversationData, Selector } from '../types';
import type { Message, PmConversationData, Selector, User } from '../types';
import { getPrivateMessages } from '../message/messageSelectors';
import { getOwnEmail } from '../users/userSelectors';
import { getOwnUser } from '../users/userSelectors';
import { getUnreadByPms, getUnreadByHuddles } from '../unread/unreadSelectors';
import { normalizeRecipientsSansMe, pmUnreadsKeyFromMessage } from '../utils/recipient';

export const getRecentConversations: Selector<PmConversationData[]> = createSelector(
getOwnEmail,
getOwnUser,
getPrivateMessages,
getUnreadByPms,
getUnreadByHuddles,
(
ownEmail: string,
ownUser: User,
messages: Message[],
unreadPms: { [number]: number },
unreadHuddles: { [string]: number },
): PmConversationData[] => {
const recipients = messages.map(msg => ({
ids: pmUnreadsKeyFromMessage(msg, ownEmail),
emails: normalizeRecipientsSansMe(msg.display_recipient, ownEmail),
ids: pmUnreadsKeyFromMessage(msg, ownUser.user_id),
emails: normalizeRecipientsSansMe(msg.display_recipient, ownUser.email),
msgId: msg.id,
}));

Expand Down
6 changes: 3 additions & 3 deletions src/realm/__tests__/realmReducer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { NULL_OBJECT } from '../../nullObjects';

describe('realmReducer', () => {
describe('ACCOUNT_SWITCH', () => {
test('resets state to blank state', () => {
test('resets state', () => {
const initialState = NULL_OBJECT;

const action = deepFreeze({
Expand All @@ -21,8 +21,8 @@ describe('realmReducer', () => {
const expectedState = {
canCreateStreams: true,
crossRealmBots: [],
email: '',
user_id: 0,
email: undefined,
user_id: undefined,
isAdmin: false,
twentyFourHourTime: false,
emoji: {},
Expand Down
44 changes: 14 additions & 30 deletions src/realm/realmReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,35 +11,18 @@ import {
} from '../actionConstants';
import { objectFromEntries } from '../jsBackport';

// Initial state
const initialState = {
canCreateStreams: true,
crossRealmBots: [],

nonActiveUsers: [],
filters: [],
emoji: {},

email: undefined,
user_id: undefined,
twentyFourHourTime: false,
emoji: {},
filters: [],
canCreateStreams: true,
isAdmin: false,
nonActiveUsers: [],
};

/**
* A version of `initialState` with some made-up blank data.
*
* On `LOGIN_SUCCESS`, we go straight to showing the main app UI (see
* `navReducer`) even though we're still loading the actual data from the
* server. So we need some fake data that the UI code will swallow.
* TODO: Probably stop doing that.
*
* Also: On `ACCOUNT_SWITCH`, during the transition animation, some old
* components can still be mounted from the UI for the previous account that
* make no sense without server data. Probably ditto `LOGOUT`.
*/
const fakeBlankState = {
...initialState,
email: '',
user_id: 0,
};

const convertRealmEmoji = (data): RealmEmojiById =>
Expand All @@ -50,20 +33,21 @@ export default (state: RealmState = initialState, action: Action): RealmState =>
case LOGOUT:
case LOGIN_SUCCESS:
case ACCOUNT_SWITCH:
return fakeBlankState;
return initialState;

case REALM_INIT: {
return {
...state,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Since this involves spreading a value and type-checking in a reducer, I was vaguely reminded of this Flow bug.

I verified empirically that it isn't in play after this commit. (Making a property undefined trips Flow.) The next commit helps verify that everything's working as expected by reading the code. So in this case it wasn't necessary to go re-digest what exactly that Flow bug is.

Copy link
Copy Markdown
Collaborator

@chrisbobbe chrisbobbe Apr 30, 2020

Choose a reason for hiding this comment

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

On re-scanning the Flow issue Ray linked to, it sounds plausible that maybe the bug was happening before this commit? Anyway, the important thing is that it's not happening after it. 😆

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This PR looks good to me, but Ray may have more to say.

canCreateStreams: action.data.can_create_streams,
crossRealmBots: action.data.cross_realm_bots,

nonActiveUsers: action.data.realm_non_active_users,
filters: action.data.realm_filters,
emoji: convertRealmEmoji(action.data.realm_emoji),

email: action.data.email,
user_id: action.data.user_id,
emoji: convertRealmEmoji(action.data.realm_emoji),
filters: action.data.realm_filters,
isAdmin: action.data.is_admin,
nonActiveUsers: action.data.realm_non_active_users,
twentyFourHourTime: action.data.twenty_four_hour_time,
canCreateStreams: action.data.can_create_streams,
isAdmin: action.data.is_admin,
};
}

Expand Down
1 change: 1 addition & 0 deletions src/reduxTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,7 @@ export type PresenceState = {|
*
* About the user:
* @prop email
* @prop user_id
* @prop twentyFourHourTime
* @prop canCreateStreams
* @prop isAdmin
Expand Down
24 changes: 11 additions & 13 deletions src/utils/recipient.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,35 +108,33 @@ export const pmKeyRecipientsFromMessage = (
* * `UnreadState`, the type of `state.unread`, which is the data structure
* these keys appear in.
*
* @param ownEmail - Required if the message could be a 1:1 PM; optional if
* @param ownUserId - Required if the message could be a 1:1 PM; optional if
* it is definitely a group PM.
*/
// Specifically, this includes all user IDs for group PMs and self-PMs,
// and just the other user ID for non-self 1:1s; and in each case the list
// is sorted numerically and encoded in ASCII-decimal, comma-separated.
// See the `unread_msgs` data structure in `src/api/initialDataTypes.js`.
export const pmUnreadsKeyFromMessage = (message: Message, ownEmail?: string): string => {
export const pmUnreadsKeyFromMessage = (message: Message, ownUserId?: number): string => {
if (message.type !== 'private') {
throw new Error('pmUnreadsKeyFromMessage: expected PM, got stream message');
}
const recipients: PmRecipientUser[] = message.display_recipient;
// This includes all users in the thread; see `Message#display_recipient`.
const recipients = message.display_recipient;
const userIds = recipients.map(r => r.id);

if (recipients.length === 1) {
if (userIds.length === 1) {
// Self-PM.
return recipients[0].id.toString();
} else if (recipients.length === 2) {
return userIds[0].toString();
} else if (userIds.length === 2) {
// Non-self 1:1 PM. Unlike display_recipient, leave out the self user.
if (ownEmail === undefined) {
throw new Error('getRecipientsIds: got 1:1 PM, but ownEmail omitted');
if (ownUserId === undefined) {
throw new Error('getRecipientsIds: got 1:1 PM, but ownUserId omitted');
}
return recipients.filter(r => r.email !== ownEmail)[0].id.toString();
return userIds.filter(userId => userId !== ownUserId)[0].toString();
} else {
// Group PM.
return recipients
.map(s => s.id)
.sort((a, b) => a - b)
.join(',');
return userIds.sort((a, b) => a - b).join(',');
}
};

Expand Down