Use recent_private_conversations on PMs screen, when available #4104

rk-for-zulip · 2020-05-12T21:13:24Z

Attempt to acquire recent_private_conversations from the server (zulip/zulip#11946). Add a new implementation for getRecentConversations which will use the data provided therein.

Since there are many Zulip servers still active which predate this server-side feature, we continue to use the old implementation when connecting to those servers, to avoid a significant regression.

This PR is intended to supersede PR #3535, with which it shares some code and history. Notable functional differences include:

(the first two paragraphs, above)
Incoming IDs for recent conversations are always sorted (per this comment).
New functions added have been to exampleData to ease test construction.
The new EVENT_NEW_MESSAGE reducer now properly filters display_recipient. (Noticed while converting its tests to use the new functions.)

Fixes #3133.

Additionally, improve typing on user-creation functions, so that they no longer happily accept (and ignore) `id: 0`.

... an expansion of `pmMessage` allowing (and requiring) the caller to supply the users involved in the message.

... which assembles a `MessagesState` from the `Message`s that compose it.

Translate existing tests from JavaScript to Flow. (There is no significant functional difference, but there are some minor changes to irrelevant data under the hood.)

... in support of a reducer for new state, shortly to be added. (This is also in support of zulip#3764, although it takes no additional stpes towards it.)

The server added in zulip/zulip#11944 basic support for fetching the most recent private conversations that a user was involved in. The endpoint returns the private conversations a user has had within the past 1000 private messages they have sent/received. The endpoint returns data in the form of `[{user_ids, max_message_id}]` where user_ids lists all the people involved in the conversation, and max_message_id returns the message_id of the most recent message in the conversation. This commit adds only the minimum machinery necessary to request this data from the server at `register` time. [ray: Split original initial commit into two parts, this being the first. Minor changes to Flow types.]

This commit introduces the reducers required to introduce this data into the Redux state, during `doInitialFetch`. It only catches the `REALM_INIT` action at the moment, and the rest of the actions will be added in a later commit with details. It does not yet store the data received from the server in REALM_INIT. Some additional work will need to be done to convert the data received from older server versions. [ray: Split original commit into two parts, this being the second. Temporarily disabled REALM_INIT reducer, just in case.]

Code to sort the `user_id`s in `recent_private_conversations` was not added until 2.2-dev-53-g405a529340 (zulip/zulip@405a529340). This was backported to the 2.1 branch as 2.1.1-50-gd452ad31e0 (zulip/zulip@d452ad31e0). Since we can't easily access the current server version here (yet?), just unconditionally sort the data. This is sufficient for us to begin storing the server data.

Since the `PmConversationsCard` needs to be updated when a new message comes in and the data it uses to be updated is in `recentPrivateConversations` key in the Redux state, this event catch adds/amends the data in the state to correctly match the most recent private messages. A unit test file has been created for this use-case as well. It is `strict-local`, and uses `exampleData.js` for its data source. [ray: rewrote test file to use new functions; (bugfix) properly filtered ownId out of `display_recipient`.]

... but also to simplify further refactoring.

We're about to add an alternate implementation of this function, but we'll need to keep the existing version to avoid loss of functionality on pre-2.1 servers. Preemptively factor some fragments of the existing implementation out, as we'll need to use them in more and/or different places.

Add new implementation of `getRecentConversations`, based on new `recent_private_conversations` data. We can't get this data on pre-2.1 servers, so we continue using the legacy data there, to avoid a regression for users on such servers. (Without this, they would never see any old conversations in the PMs window; only those with messages that have come in during the current session would be visible.) In upcoming commits: * New tests for the new implementation (alongside the retained tests for the legacy implementation). * Adjusting the main selector to be more efficient, rather than potentially computing both the modern and legacy implementations on every call. Based in part on work by Isham Mahajan <[email protected]>.

Based closely on the tests for the old implementation. Taken with minimal changes from PR zulip#3535. Co-authored-by: Greg Price <[email protected]> [ray: ported from old PR; wrote new commit message for new context.]

Bind up the decision of whether to use the old or new implementation into a selector of its own. This simultaneously caches that decision and ensures that only the necessary implementation, and not its complement, is ever invoked.

rk-for-zulip · 2020-05-12T21:21:45Z

Suggested future work:

In EVENT_NEW_MESSAGE handlers, switch over from using ownEmail to using ownId where possible.
Remove the "nastiness" mentioned in the previous PR's 5343872 – preferably not by pushing it into recipient.js, which has quite enough nastiness in it already, but instead by making the sole downstream consumer of this code accept a Convert from emails to user IDs to identify users #3764-compliant data structure.

gnprice

Thanks @ray-kraesig ! Glad to see this moving forward. See comments below.

I am concerned about the quantity of new code this adds in pmConversationsSelectors.js. There's going to be a net increase because we don't get to remove the legacy implementation; but this takes it from 61 lines to 181 lines, which is a lot more than I'd expect, and the result does feel like a lot more to read through.

Several of the comments below have specific suggestions for reducing that. Please also look for ways you can simplify in general.

Remove the "nastiness" mentioned in the previous PR's 5343872 – preferably not by pushing it into recipient.js, which has quite enough nastiness in it already, but instead by making the sole downstream consumer of this code accept a #3764-compliant data structure.

I just saw this on rereading your PR comment, and it relates to some comments I made below. I'm all for future changes to eliminate (cases of) this kind of complexity entirely. Until we do, it's important to be clear about what it's doing. Otherwise we're just creating new archeological and reverse-engineering problems for our future selves to solve -- which is the hardest part of the work to remove such things.

src/__tests__/lib/exampleData.js

gnprice · 2020-05-13T21:14:41Z

src/__tests__/lib/exampleData.js

+  // exact object + indexer properties issue; fixed in v0.111.0
+  return (state: $FlowFixMe);


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.

gnprice · 2020-05-13T22:13:43Z