Fix some "is message in narrow" bugs, and test much more thoroughly #4294

We already ensure this in the Android case (in FcmMessage.kt); do so in the iOS case too, and document it in the type. In practice the list should already have always been sorted: the server sends it in that form, and has always done so since the pm_users field was introduced in server commit 1.7.0-2360-g693a9a5e7. (To see this in the history, try the following Git commands: git log -L :get_message_payload:zerver/lib/push_notifications.py git log -L :huddle_users:zerver/lib/message.py .) So the only way this could have gone wrong is if a rogue server changed that behavior for some reason; and the main effect of this commit is really just to document this invariant.

Which turned up a couple of bugs! We'll fix those later in this series.

As demonstrated, this allows callers to customize these a lot more cleanly than they can by overriding the actual message properties directly. There are a few call sites we don't update here, in narrowsReducer-test.js; that file hasn't yet been upgraded to be well-typed, and so those call sites don't have real User objects to provide.

We discovered this nicer one after having used the other one here. Reminded of the contrast in discussion on other changes in this file: zulip#4294 (comment)

I just ran into this issue with CaughtUpState when making another change. Apply the workaround there and on the remaining example in this file, and mark all instances with a conditional TODO.

The data in this test case has never made much sense since 7a5da95, shortly after the test was added -- testing a PM with a sender not in the display_recipient, which never happens. With e09648c it grew another kind of testing mistake which helped mask that problem: the `caughtUp` state was false for the narrows we didn't expect the message to get added to, so we didn't actually test whether the logic believed the message belonged to them. Then a bug in eg.pmMessage made the message differently malformed, after 4077e88 had this test start using that function; the message would match all PM narrows, and the test kept passing only because of that `caughtUp` masking. We're about to fix that bug in eg.pmMessage. That will cause this test to fail (with the message matching no conversation narrows) unless adjusted. Just fix the whole test to make more sense.

The display_recipient of a PM always contains all users involved, including the self user. See comment on the Message type.

We have a fair number of codepaths specific to group PM conversations and messages. When testing those, it's essential to have at least three distinct users. So this is a pretty widespread need.

Mostly this involves using real Message objects instead of objects with a handful of the same properties, and passing the full list of arguments to some function calls where the tests were getting away without the last one because the control path we're exercising happens not to use it. We also discard some test fragments that just check behavior on ill-typed data that doesn't particularly correspond to any data we believe our actual code produces, like `[{}, {}]` to the isFooNarrow functions or `undefined` to `isSameNarrow`. There's a lot more that could be done to simplify these tests and further improve them, but this gives us a basis for any refactoring.

This will make it a lot easier to see what cases aren't covered, and to add more cases.

Including two failing tests! Commented out. We'll fix the bug those uncovered shortly.

We had a hilarious bug: if you've already visited some group PM conversation (so that we're following it in `state.narrows`), and then you send yourself a 1:1 PM -- either from some other client, or from this same client -- then the 1:1 will appear to show up in that group conversation. This doesn't affect what messages exist on the server, or what messages anyone else sees. And it can only happen with a message you yourself sent, because it's limited to self-PMs. Still -- it could potentially *look* to you like something quite awkward had happened! Which would, itself, be awkward. The root of the bug is a funny `normalizedRecipients === ownEmail` case in the `isMessageInNarrow` function. That, in turn, seems to be there (it was added in d8faae9, which introduced this bug) to try to correct for where we add `ownEmail` to the list of emails on the line before... which in turn is to try to correct for how in many places we leave out the self-user from describing certain PM narrows. We can fix the bug by consistently using that latter normalization, quirky though it is, within that function. The email list representing a group PM narrow should already be normalized in that way, leaving out the self user; and the old code here was already relying on that. Fixes: zulip#3324

In most places in the app, we represent a group PM conversation by just the users in it *other* than self. This aligns with how the webapp represents the narrow in the URL: the fragment may look like `#narrow/pm-with/101,104-group` for a conversation with a total of 3 users in it. In particular, most of our code consistently generates group-narrow values following this convention, and in some places our code relies on that assumption. But we don't always fulfill it. One place we don't is when you open a notification for a group PM: the server represents the conversation there with the full list of users, and we don't normalize it to the other convention. Then one place we rely on the assumption is here in `isMessageInNarrow`... which we use to decide whether a new message should be shown in the conversation. The result is zulip#4293, that the narrow doesn't see any replies you send or any further messages that arrive. The ultimate fix here is an overhaul of how we represent narrows, to something more structured, with more explicit, checkable, checked invariants. Pending that, we'll fix the issue at hand at both ends -- either one suffices to fix the issue, and each one potentially fixes other, unknown issues. In this commit, we fix the consumer of the data, to no longer rely on this assumption. Fixes: zulip#4293

This fixes zulip#4293 from the other direction as we did in the previous commit. As a result we avoid the at-least-latent bug where we can end up with duplicate narrows in our Redux state; and we also avoid any unknown other bug where, like with zulip#4293, we're relying on the set of users having been filtered this way.

Does much the same job as pmKeyRecipientsFromMessage, but starting from a list of user IDs. We have this logic already to handle notifications for group PMs, which come with such a list encoded as comma-separated strings. We'll want similar logic for group-PM narrow URLs, which do the same thing; so factor it out to be shared.

This makes it just a bit more explicit what each of these bits are doing, before we make some other changes in the next commit.

In the tests, we replace `userA` with `eg.selfUser`, to make it explicit which one is self now that that affects the logic.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some "is message in narrow" bugs, and test much more thoroughly #4294

Fix some "is message in narrow" bugs, and test much more thoroughly #4294

Commits on Nov 2, 2020