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

Use recent_private_conversations on PMs screen, when available #4104

Closed
wants to merge 14 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 37 additions & 17 deletions src/__tests__/lib/exampleData.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import deepFreeze from 'deep-freeze';
import { createStore } from 'redux';

import type { CrossRealmBot, Message, PmRecipientUser, Stream, User } from '../../api/modelTypes';
import type { Action, GlobalState, RealmState } from '../../reduxTypes';
import type { Action, GlobalState, MessagesState, RealmState } from '../../reduxTypes';
import type { Auth, Account, Outbox } from '../../types';
import { ZulipVersion } from '../../utils/zulipVersion';
import {
Expand All @@ -16,6 +16,8 @@ import {
import rootReducer from '../../boot/reducers';
import { authOfAccount } from '../../account/accountMisc';
import { HOME_NARROW } from '../../utils/narrow';
import { NULL_OBJECT } from '../../nullObjects';
import { objectFromEntries } from '../../jsBackport';

/* ========================================================================
* Utilities
Expand Down Expand Up @@ -73,8 +75,13 @@ export const randString = () => randInt(2 ** 54).toString(36);
* Users and bots
*/

type UserOrBotPropertiesArgs = {|
name?: string,
user_id?: number,
|};

const randUserId: () => number = makeUniqueRandInt('user IDs', 10000);
const userOrBotProperties = ({ name: _name }) => {
const userOrBotProperties = ({ name: _name, user_id }: UserOrBotPropertiesArgs) => {
const name = _name ?? randString();
const capsName = name.substring(0, 1).toUpperCase() + name.substring(1);
return deepFreeze({
Expand All @@ -88,12 +95,12 @@ const userOrBotProperties = ({ name: _name }) => {
full_name: `${capsName} User`,
is_admin: false,
timezone: 'UTC',
user_id: randUserId(),
user_id: user_id ?? randUserId(),
});
};

/** Beware! These values may not be representative. */
export const makeUser = (args: { name?: string } = {}): User =>
export const makeUser = (args: UserOrBotPropertiesArgs = NULL_OBJECT): User =>
gnprice marked this conversation as resolved.
Show resolved Hide resolved
deepFreeze({
...userOrBotProperties(args),

Expand All @@ -107,7 +114,7 @@ export const makeUser = (args: { name?: string } = {}): User =>
});

/** Beware! These values may not be representative. */
export const makeCrossRealmBot = (args: { name?: string } = {}): CrossRealmBot =>
gnprice marked this conversation as resolved.
Show resolved Hide resolved
export const makeCrossRealmBot = (args: UserOrBotPropertiesArgs = NULL_OBJECT): CrossRealmBot =>
deepFreeze({
...userOrBotProperties(args),
is_bot: true,
Expand Down Expand Up @@ -182,16 +189,17 @@ export const stream: Stream = makeStream({
* Messages
*/

const displayRecipientFromUser = (user: User): PmRecipientUser => {
const { email, full_name, user_id: id } = user;
return deepFreeze({
email,
full_name,
id,
is_mirror_dummy: false,
short_name: '', // what is this, anyway?
const displayRecipientFromUsers = (users: User[]): PmRecipientUser[] =>
users.map(user => {
gnprice marked this conversation as resolved.
Show resolved Hide resolved
const { email, full_name, user_id: id } = user;
return deepFreeze({
email,
full_name,
id,
is_mirror_dummy: false,
short_name: '', // what is this, anyway?
});
});
};

/** Boring properties common to all example Message objects. */
const messagePropertiesBase = deepFreeze({
Expand Down Expand Up @@ -233,14 +241,14 @@ const messagePropertiesFromSender = (user: User) => {
};

/** Beware! These values may not be representative. */
export const pmMessage = (extra?: $Rest<Message, {}>): Message => {
export const pmMessageFromTo = (from: User, to: User[], extra?: $Rest<Message, {}>): Message => {
const baseMessage: Message = {
...messagePropertiesBase,
...messagePropertiesFromSender(otherUser),
...messagePropertiesFromSender(from),

content: 'This is an example PM message.',
content_type: 'text/markdown',
display_recipient: [displayRecipientFromUser(selfUser)],
display_recipient: displayRecipientFromUsers([from, ...to]),
id: 1234567,
recipient_id: 2342,
stream_id: -1,
Expand All @@ -252,6 +260,9 @@ export const pmMessage = (extra?: $Rest<Message, {}>): Message => {
return deepFreeze({ ...baseMessage, ...extra });
};

export const pmMessage = (extra?: $Rest<Message, {}>): Message =>
pmMessageFromTo(selfUser, [otherUser], extra);
gnprice marked this conversation as resolved.
Show resolved Hide resolved

const messagePropertiesFromStream = (stream1: Stream) => {
const { stream_id, name: display_recipient } = stream1;
return deepFreeze({
Expand Down Expand Up @@ -279,6 +290,13 @@ export const streamMessage = (extra?: $Rest<Message, {}>): Message => {
return deepFreeze({ ...baseMessage, ...extra });
};

/** Construct a MessagesState from a list of messages. */
export const makeMessagesState = (messages: Message[]): MessagesState => {
const state: { [id: number]: Message } = objectFromEntries(messages.map(m => [m.id, m]));
// exact object + indexer properties issue; fixed in v0.111.0
return (state: $FlowFixMe);
Comment on lines +296 to +297
Copy link
Member

Choose a reason for hiding this comment

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

This catches my eye because MessagesState may switch from an object-as-map to a Map or Immutable.Map soon, and I'd like to rely on the type-checker to find places we need to update.

When I remove the fixme, one thing I notice is there are actually two errors here -- and one of them isn't related to that Flow bug, I think, but comes from the fact that (a) Message is an inexact type, but (b) MessagesState uses $Exact<Message>. That contrast seems odd and I don't recall why we have it.

I'd feel better if this were explicit about what kind of type difference it's meant to launder. If e.g. this line were to spell out what the MessagesState type is -- which will look very similar to the type on state just above, so the reader can compare to see what's being fudged -- then I think that'd do it. Then Flow would check that that type indeed can be used as MessagesState, so when the latter changes incompatibly it'll give an error here.

};

/* ========================================================================
* Outbox messages
*
Expand Down Expand Up @@ -424,6 +442,7 @@ export const action = deepFreeze({
realm_users: [],
user_id: 4,
realm_user_groups: [],
recent_private_conversations: [],
streams: [],
never_subscribed: [],
subscriptions: [],
Expand Down Expand Up @@ -491,5 +510,6 @@ export const eventNewMessageActionBase /* \: $Diff<EventNewMessageAction, {| mes
type: EVENT_NEW_MESSAGE,
id: 1001,
caughtUp: {},
ownId: selfUser.user_id,
ownEmail: selfAccount.email,
};
1 change: 1 addition & 0 deletions src/actionTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,7 @@ type EventNewMessageAction = {|
...$Diff<MessageEvent, { flags: mixed }>,
type: typeof EVENT_NEW_MESSAGE,
caughtUp: CaughtUpState,
ownId: number,
ownEmail: string,
|};

Expand Down
10 changes: 10 additions & 0 deletions src/api/initialDataTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type {
CrossRealmBot,
RealmEmojiById,
RealmFilter,
RecentPrivateConversation,
Stream,
Subscription,
User,
Expand Down Expand Up @@ -110,6 +111,14 @@ export type InitialDataRealmUserGroups = {|
realm_user_groups?: UserGroup[],
|};

export type InitialDataRecentPmConversations = {|
// * Added in server commit 2.1-dev-384-g4c3c669b41.
// * `user_id` fields are sorted as of commit 2.2-dev-53-g405a529340, which
// was backported to 2.1.1-50-gd452ad31e0 -- meaning that they are _not_
// sorted in either v2.1.0 or v2.1.1.
recent_private_conversations?: RecentPrivateConversation[],
|};

type NeverSubscribedStream = {|
description: string,
invite_only: boolean,
Expand Down Expand Up @@ -287,6 +296,7 @@ export type InitialData = {|
...InitialDataRealmFilters,
...InitialDataRealmUser,
...InitialDataRealmUserGroups,
...InitialDataRecentPmConversations,
...InitialDataStream,
...InitialDataSubscription,
...InitialDataUpdateDisplaySettings,
Expand Down
25 changes: 25 additions & 0 deletions src/api/modelTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -545,3 +545,28 @@ export type Message = $ReadOnly<{
subject: string,
subject_links: $ReadOnlyArray<string>,
}>;

//
//
//
// ===================================================================
// Summaries of messages and conversations.
//
//

/**
* Describes a single recent PM conversation.
*
* For the structure of this type and the meaning of its properties, see:
* https://github.com/zulip/zulip/blob/4c3c669b41/zerver/lib/events.py#L283-L290
*
* `user_ids` does not contain the `user_id` of the current user. Consequently,
* a user's conversation with themselves will be listed here as [], which is
* unlike the behaviour found in some other parts of the codebase.
*/
export type RecentPrivateConversation = {|
max_message_id: number,
// When received from the server, these are guaranteed to be sorted only after
// 2.2-dev-53-g405a529340. To be safe, we always sort them on receipt.
user_ids: number[],
|};
2 changes: 2 additions & 0 deletions src/boot/reducers.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import nav from '../nav/navReducer';
import outbox from '../outbox/outboxReducer';
import presence from '../presence/presenceReducer';
import realm from '../realm/realmReducer';
import recentPrivateConversations from '../pm-conversations/recentPmConversationsReducer';
import session from '../session/sessionReducer';
import settings from '../settings/settingsReducer';
import streams from '../streams/streamsReducer';
Expand Down Expand Up @@ -50,6 +51,7 @@ const reducers = {
outbox,
presence,
realm,
recentPrivateConversations,
session,
settings,
streams,
Expand Down
6 changes: 3 additions & 3 deletions src/boot/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,9 @@ export const storeKeys: Array<$Keys<GlobalState>> = [
* don't have to re-download it.
*/
// prettier-ignore
export const cacheKeys: Array<$Keys<GlobalState>> = [
Copy link
Member

Choose a reason for hiding this comment

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

This bit was in Isham's version, but I just noticed it: do you see a reason this type should get dropped?

'flags', 'messages', 'mute', 'narrows', 'realm', 'streams',
'subscriptions', 'unread', 'userGroups', 'users',
export const cacheKeys = [
'flags', 'messages', 'mute', 'narrows', 'realm', 'recentPrivateConversations',
'streams', 'subscriptions', 'unread', 'userGroups', 'users',
];

/**
Expand Down
1 change: 1 addition & 0 deletions src/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ const config: Config = {
'realm_filters',
'realm_user',
'realm_user_groups',
'recent_private_conversations',
'stream',
'subscription',
'update_display_settings',
Expand Down
4 changes: 4 additions & 0 deletions src/directSelectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import type {
Subscription,
Stream,
Outbox,
RecentPrivateConversation,
User,
UserGroup,
UserStatusState,
Expand Down Expand Up @@ -83,6 +84,9 @@ export const getSubscriptions = (state: GlobalState): Subscription[] => state.su
*/
export const getStreams = (state: GlobalState): Stream[] => state.streams;

export const getRecentPrivateConversations = (state: GlobalState): RecentPrivateConversation[] =>
state.recentPrivateConversations;

export const getPresence = (state: GlobalState): PresenceState => state.presence;

export const getOutbox = (state: GlobalState): Outbox[] => state.outbox;
Expand Down
2 changes: 2 additions & 0 deletions src/events/eventToAction.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,8 @@ export default (state: GlobalState, event: $FlowFixMe): EventAction => {
...event,
type: EVENT_NEW_MESSAGE,
caughtUp: state.caughtUp,
ownId: getOwnUserId(state),
// deprecated; avoid adding new uses (#3764)
ownEmail: getOwnEmail(state),
};

Expand Down
Loading