Refresh the still-useful parts of #4116, on path to using recent-PMs data #4358

gnprice · 2020-12-25T03:30:44Z

After taking care of #4035 (cleanly handling PM recipients, in place of our complex old handling of them) with my series of PRs up to #4356, I'm taking a look back at #3133/#3535, to start using the recent_private_conversations data structure, which had been blocked by that work.

Ray had a PR #4116 that sought to do some of that. Some of it goes in some of the same directions as I've since done with my recent PRs for #4035, and there are some parts that I don't think are helpful or needed. But there are other changes there that were and still are useful. Here I've rebased those changes, and added some related commits of my own.

After this, I'll return to Ray's further PR #4104 (which builds on the changes in #4116) to see what parts of that are also helpful to use now.

chrisbobbe

Thanks! This LGTM except for a few small comments/queries below.

chrisbobbe · 2020-12-29T00:45:57Z

src/__tests__/lib/exampleData.js

  });
 };

 /** Beware! These values may not be representative. */
-export const makeUser = (args: { name?: string } = {}): User =>
+export const makeUser = (args: UserOrBotPropertiesArgs = NULL_OBJECT): User =>


What's the reason for using NULL_OBJECT here?

Good question 😛 #4104 (comment)

I guess it'd be good to mention that in the commit message.

... Or I'll just do the other workaround discussed there, namely to say Object.freeze({}) directly. That's what NULL_OBJECT is defined as anyway, and it perhaps needs less explanation.

chrisbobbe · 2020-12-29T00:56:57Z

src/pm-conversations/__tests__/pmConversationsSelectors-test.js

-          meAndJohnPm2.id,
-          meOnlyPm.id,
-        ],
+        [ALL_PRIVATE_NARROW_STR]: [0, 1, 2, 3, 4],


pm-conversations tests: Simplify using eg.makeMessagesState. There is no significant functional difference, but there are some minor changes to irrelevant data under the hood. [greg: Ray's original change made this file well-typed, when it wasn't before. We've since independently done that, but the change contained some other useful simplifications, reproduced here.]

Hmm, interesting. Among those useful simplifications is that there are fewer lines in the file; that's good. But there's a bit of a cost, right; the reader has to make a bit of extra effort to work out what a literal 0, 1, 2, etc., means, when it doesn't say fooPm.id, etc.

There is a tradeoff here, but I think on net this version is a bit easier to understand.

One key condition of that is that these IDs all stay local -- their meaning is all contained within the test case, and doesn't involve looking out to anything further afield. (I'd like to say the scope of code that can care about one of these IDs all fits on a screen, but that may not be true on laptop screens. It'd be better if each test's code were shorter, so that it did.)

Then on the other hand these names were feeling like a bit of work to decode in themselves. Partly because they're long, perhaps? They're sensible names, and I don't have better ideas for names for these values, so that seems potentially inherent to naming them in these tests.

... In fact. It looks like the data don't actually match up with each other!

{ sender_id: userJohn.user_id, unread_message_ids: [meAndMarkPm1.id, meAndMarkPm2.id], }, { sender_id: userMark.user_id, unread_message_ids: [meAndJohnPm1.id], },

The fact that we didn't notice that until now suggests to me that the names weren't providing a lot of value in making this test data easy to understand. I'll add a fix for that to this branch.

Another thing on the other hand is that the ordering of the message IDs is important -- the output is supposed to be sorted by message ID (descending), and the second test is all about that property. So with the names you have to go consulting the definitions and matching them up to see that the ordering is right, whereas with the IDs it's immediate.

Ah OK, makes sense. And thanks for catching that mismatch!

chrisbobbe · 2020-12-29T01:00:20Z

src/chat/__tests__/narrowsSelectors-test.js

@@ -113,11 +113,11 @@ describe('getFirstMessageId', () => {
      narrows: Immutable.Map({
        [HOME_NARROW_STR]: [1, 2, 3],
      }),
-      messages: {


tests: Use new eg.makeMessagesState where applicable. Note that this actually changes the test data in a couple of spots -- where we were producing a malformed data structure! Looks like a copy-paste error... in which it's probably not a coincidence that the old form here was pretty repetitive.

Does it change the test data? I'm not sure I follow; it looks like a pure refactor to me.

Ah, it's subtle -- there's this (twice):

- messages: { - ...eg.baseReduxState.messages, - [message1.id]: message1, - [message2.id]: message1, - }, + messages: eg.makeMessagesState([message1, message2]),

Note the message1 at message2.id.

I'll make the commit message clearer about that.

Oh, yikes, yeah—that message1/message2 misalignment sure looks fishy; thanks for fixing. I actually noticed this when working on my draft (not yet pushed) for storing state.messages as an Immutable.Map and made a note to return to it in case it was somehow done on purpose, but hadn't gotten around to doing so yet.

chrisbobbe · 2020-12-29T01:01:43Z

src/types.js

@@ -320,9 +320,22 @@ export type TabNavigationOptionsPropsType = {|
 * Summary of a PM conversation (either 1:1 or group PMs).
 */
 export type PmConversationData = {|
+  /**
+   * A comma-separated (numerically-)sorted sequence of the IDs of the users
+   * involved in this conversation. Does not includes the self-user iff there


nit: Does not includes -> (maybe) Excludes?

Thanks, yeah, the negative on the left of this "iff" is confusing.

... but also to simplify further refactoring.

Saying `$Exact<Message>` here is no different from simply `Message`, because since ce5aeb6, `Message` itself is an exact object type.

Additionally, make their arguments exact object types, to raise errors on unrecognized arguments. Making the argument object types exact means we need their default values to be recognized by Flow as having an exact object type, which `{}` isn't. We can fix that with `Object.freeze`.

... an expansion of `pmMessage` allowing (and requiring) the caller to supply the users involved in the message. [greg: adapted significantly atop related changes since]

In particular, use this in most places where the specific sender and recipients are essential to the meaning of the test -- when that's the case it's best to be explicit, and this is shorter than our existing ways of being explicit.

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

There is no significant functional difference, but there are some minor changes to irrelevant data under the hood. [greg: Ray's original change made this file well-typed, when it wasn't before. We've since independently done that, but the change contained some other useful simplifications, reproduced here.]

Note that this actually changes the test data in a couple of spots -- where we were producing a malformed data structure! (With `message1` at the ID of `message2`.) Looks like a copy-paste error... in which it's probably not a coincidence that the old form here was pretty repetitive.

[greg: substantially rebased; edited a bit]

One instance of this, the one in UnreadCards, appeared in a branch of Ray's. It's a handy Flow feature that I'm pretty sure I didn't know about when I first introduced this `$PropertyType<..., 'props'>` pattern a couple of years ago in 706929b. Let's use it everywhere it applies. This also allows it to work properly with the component type TextInput, so that we get to delete a fixme. (Sadly the issue described in the comment by the fixme still exists, as seen at similar fixmes elsewhere in the codebase; this was actually a separate problem which just arose at the same time.) I think the issue was that in general the old pattern worked only on class components, not on React component types defined in other ways, and TextInput is one of the latter.

This hasn't been used since 3a23c49. [greg: In Ray's original branch, `PmConversationList` still needed this data at this stage, because the change corresponding to 3a23c49 was later in the branch; so that component grew a `connect` call to get it directly. The bulk of the diff was the changes to its callers, which are the same here modulo minor rebasing.]

This will help us tighten up this test code a bit.

…een. This reduces each of these tests to about 40-50 lines -- still not short, but hopefully short enough to all fit at once on a wide range of screen sizes.

The `unread` and `messages` data structures in this test data weren't agreeing on what messages existed, or belonged to which conversations. This also means correcting one of the expected values.

gnprice · 2020-12-29T02:06:39Z

Thanks for the review! Pushed a new revision.

chrisbobbe · 2020-12-29T03:10:59Z

Great, thanks! Merged.

gnprice requested a review from chrisbobbe December 25, 2020 03:30

gnprice mentioned this pull request Dec 25, 2020

Remove redundant, stringly-typed fields from PmConversationData #4116

Closed

chrisbobbe reviewed Dec 29, 2020

View reviewed changes

rk-for-zulip and others added 15 commits December 28, 2020 17:18

pm-conversations [nfc]: Rename some variables, mostly for clarity.

f61b766

... but also to simplify further refactoring.

types: Drop a redundant $Exact<...>.

ffc1011

Saying `$Exact<Message>` here is no different from simply `Message`, because since ce5aeb6, `Message` itself is an exact object type.

example data: Explain why we don't sort recipients in pmMessage.

e44b00f

example data: Add auxiliary function pmMessageFromTo.

4248737

... an expansion of `pmMessage` allowing (and requiring) the caller to supply the users involved in the message. [greg: adapted significantly atop related changes since]

tests [nfc]: Use the new eg.pmMessageFromTo more.

863fb01

In particular, use this in most places where the specific sender and recipients are essential to the meaning of the test -- when that's the case it's best to be explicit, and this is shorter than our existing ways of being explicit.

example data: Add auxiliary function makeMessagesState.

0448f11

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

types [nfc]: Add jsdoc for the fields of PmConversationData.

dd491fa

[greg: substantially rebased; edited a bit]

pm-conversations tests [nfc]: Pull out keyForUsers.

d7f3666

This will help us tighten up this test code a bit.

pm-conversations tests [nfc]: Tighten formatting to fit better on scr…

c71a7f5

…een. This reduces each of these tests to about 40-50 lines -- still not short, but hopefully short enough to all fit at once on a wide range of screen sizes.

pm-conversations tests: Fix inconsistent test data.

887b236

The `unread` and `messages` data structures in this test data weren't agreeing on what messages existed, or belonged to which conversations. This also means correcting one of the expected values.

gnprice force-pushed the pr-4116-updated branch from 5257049 to 887b236 Compare December 29, 2020 02:06

chrisbobbe merged commit 887b236 into zulip:master Dec 29, 2020

gnprice deleted the pr-4116-updated branch December 29, 2020 04:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refresh the still-useful parts of #4116, on path to using recent-PMs data #4358

Refresh the still-useful parts of #4116, on path to using recent-PMs data #4358

gnprice commented Dec 25, 2020

chrisbobbe left a comment

chrisbobbe Dec 29, 2020

gnprice Dec 29, 2020

gnprice Dec 29, 2020

chrisbobbe Dec 29, 2020

gnprice Dec 29, 2020

chrisbobbe Dec 29, 2020

chrisbobbe Dec 29, 2020

gnprice Dec 29, 2020

chrisbobbe Dec 29, 2020 •

edited

Loading

chrisbobbe Dec 29, 2020

gnprice Dec 29, 2020

gnprice commented Dec 29, 2020

chrisbobbe commented Dec 29, 2020

Refresh the still-useful parts of #4116, on path to using recent-PMs data #4358

Refresh the still-useful parts of #4116, on path to using recent-PMs data #4358

Conversation

gnprice commented Dec 25, 2020

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Dec 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Dec 29, 2020

chrisbobbe commented Dec 29, 2020

chrisbobbe Dec 29, 2020 •

edited

Loading