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

gnprice · 2020-10-29T23:45:25Z

This grew out of revising #3889 -- after #3889 (comment) it was clear that more tests were needed on this code, and also I rediscovered the isMessageInNarrow function which already does something very similar to what one of my commits in that PR was seeking to do. The branch basically looks like:

I set about overhauling that function's tests, making them table-driven so they're far more focused and more compact in the source.
Then I added a bunch more test cases, now that they're just a line of code each, and now that it's easy to see what all the existing tests are doing and see what cases aren't covered.
Some of the tests failed!
This led to a range of bugfixes: some in our example data for testing, but also the potentially disturbing Self-PM appears in other PM conversations #3324 and the highly annoying Group PM view from notification doesn't see new messages #4293.
- Some manual testing to confirm Group PM view from notification doesn't see new messages #4293 also led me to rediscover the bug fixed by notifications: Correctly stringify pmUsers to fix navigation to group… #3922 -- and go back and merge that PR, which had fallen through a crack -- and to discover and fix Opening a notification from another account causes crash #4290. Those fixes are already merged and not part of this PR.
The investigation of those bugs also led to further pinning down a bunch of information about how different parts of our code actually do represent group PM threads -- which often don't quite agree, particularly in the sorting of the list of users. (But which agree rather more at the end of this branch than at the beginning.) That information goes mostly in a number of comments in src/utils/narrow.js, mostly in commit 2 of the series.

Fixes: #3324
Fixes: #4293

chrisbobbe

It's great news to have this thorough investigation done and these bugs fixed! A few comments below, all small; otherwise, LGTM!

chrisbobbe · 2020-11-02T18:39:05Z

src/__tests__/lib/exampleData.js

+|}): Message => {
+  // The redundant `sender` in the ?? case avoids a Flow issue:
+  // https://github.com/facebook/flow/issues/2386
+  const { sender = otherUser, recipients = [selfUser], ...extra } = args ?? { sender: otherUser };


At one point you found a better-looking workaround for this issue, and I've been using it when I remember to—facebook/flow#2386 (comment).

Ah indeed! Thanks for noticing -- glad you remembered that. :-)

chrisbobbe · 2020-11-02T18:58:12Z

src/utils/__tests__/narrow-test.js

+    ['whole-stream narrow', streamNarrow(eg.stream.name), [
+      ['matching stream message', true, eg.streamMessage()],
+    ]],
+    ['stream conversation', topicNarrow(eg.stream.name, 'cabbages'), [


Hmm, a "stream conversation" is a topic narrow, right? Is "stream conversation" consistent with naming choices elsewhere or might it be clearer to include the word "topic"? (I think of "conversation" as usually referring to PMs.)

I've been using "conversation" to refer to the smallest unit of Zulip threading: either a group PM thread, or a topic within a stream. Every conversation is a narrow, but it's a particularly handy subset of narrows in part because every message belongs to exactly one conversation. It's also a term that works pretty naturally in user-facing text.

The same terminology appears in a few places around the project, though I don't think we've standardized on it. One is in the three-dots menu on a message in the webapp, where there's an option "Copy link to conversation". That's for any message; and it'll produce a link narrowed to the message's conversation in this sense, even if you're seeing it from some interleaved narrow.

Ah OK, got it! Makes sense.

chrisbobbe · 2020-11-02T19:05:25Z

src/utils/__tests__/narrow-test.js

    ]],

    ['1:1 PM conversation, non-self', privateNarrow(eg.otherUser.email), [
-      ['matching PM', true, eg.pmMessage()],


Under '1:1 PM conversation, non-self', should we also test for group-PM messages being excluded?

chrisbobbe · 2020-11-02T19:21:04Z

src/utils/__tests__/internalLinks-test.js

@@ -139,6 +139,7 @@ describe('getNarrowFromLink', () => {
      new URL('https://example.com'),
      usersById,
      new Map(streams.map(s => [s.stream_id, s])),
+      userA.user_id,


This parameter is called ownUserId; would this be a good place to use eg.selfUser?

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.

gnprice · 2020-11-02T23:41:32Z

Thanks @chrisbobbe for the review! Pushed a new revision; please take a look.

chrisbobbe · 2020-11-03T00:42:50Z

Thanks @chrisbobbe for the review! Pushed a new revision; please take a look.

Merged, thanks!

@param

Thanks as always to our kind volunteer translators. i18n: Sync recently-added message strings across languages. webview build: Spell stdin as `-` for reading rsync filter rules. On Windows (in Git Bash) there's no /dev/stdin; but this works instead, as we learned here: https://chat.zulip.org/#narrow/stream/48-mobile/topic/issue/near/1047294 (Things still aren't working there as a whole, but we seem to get past one error and reach another one.) Conversely, this exact construct `--filter='. -'` appears in an example in the rsync man page, even at the ancient rsync 2.6.9 that Apple provides on macOS. Suggested-by: Anders Kaseorg <[email protected]> README: Migrate Travis badge to travis-ci.com. Signed-off-by: Anders Kaseorg <[email protected]> android notif: Correctly stringify pmUsers to fix navigation to group PMs. Navigation to a group PM on pressing a notification was broken because pmUsers was incorrectly stringified in GroupPm.getPmUsersString. E.g., for a group PM among user IDs 13313, 13434, and 13657, it would stringify to (newline added for readability): "GroupPm(pmUsers=[13313, 13434, 13657]), GroupPm(pmUsers=[13313, 13434, 13657]), GroupPm(pmUsers=[13313, 13434, 13657])" It should instead stringify to "13313, 13434, 13657". (Later in this series of commits, we remove the space.) Fix and add a test. notif tests: Ensure tests pass with representative pm_users values. To be reverted in the next commit. In the previous commit, we changed the return value of GroupPm.getPmUsersString in our Kotlin code from garbage separated by ', ' to numbers separated by ', '. This commit aims to prove that ', '-separated numbers will be handled correctly, at least as far as our tests can tell. But we really want it to be ','-separated (no space), which we do in the next commit. notif: Separate ids in pm_users for group PMs with ',' instead of ', '. ', ' would have been handled correctly, but seemingly by accident; in getNarrowFromNotificationData, pm_users was split on ',' to give ['1', ' 2', ' 3'] (note the spaces), then each element of that array was converted to a number. Also, replace the confusing + syntax, as in +idStrs[i], with parseInt. logging jsdoc: Move "see also" before parameters, to fix parse. When writing a call to a function that has jsdoc, VS Code shows a handy popup with the documentation. It shows first the text for the parameter you're currently typing, then the text for the function as a whole. That popup was showing the "See also" as part of the last parameter's documentation, rather than that for the function as a whole. In particular this means it was only visible when typing the last parameter. Fix the jsdoc parse, by moving everything that isn't part of a parameter's documentation to before the first @param marker. notif: Normalize realm_uri by parsing it as a URL. This fixes zulip#4290, a regression in the last release, where trying to open a notification doesn't actually navigate to the conversation. The bug is a bit like a revival of zulip#3567: we get the error "notification realm_uri not found in accounts", and it turns out that all the accounts have URLs with a trailing slash `/`, while the `realm_uri` value in the notification doesn't. On further inspection, it looks like this was introduced when we started using a URL object for `realm` values, in 865914f. Previously, since the fix for zulip#3567, we'd stripped trailing slashes from `realm` values, which were URL strings. But: > new URL('https://example').toString() 'https://example/' parsing as a URL object and converting that to a string normalizes the URL, and one thing that normalization does is *add* a trailing slash to a URL like our realm URLs (or in general, fill in the path as `/` if empty.) When a `realm_uri` with no slash is compared to one of those, it never matches. Fix the issue by doubling down on parsing as URL objects. As a side effect, this normalizes case in the URL's host (and scheme). We'd previously discussed doing that, at zulip#3671 and here: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/realm.20URL/near/795201 and concluded that parsing as URL objects would be the cleanest way. We didn't then have an appropriate `URL` implementation handy, but now we do. :-) Fixes: zulip#4290 UserItem [nfc]: Take user as one structured object. This will help us switch from emails to user IDs in downstream bits of code. Also adjust several of these call sites to use user IDs for `key`, rather than emails. UserItem [nfc]: Pass whole user to callback, rather than email. This allows UserItem call sites whose callbacks are ready to work in terms of user IDs to do so without workarounds. At the same time, passing whole user objects rather than *just* IDs allows other call sites to continue to use emails without similar, inverse workarounds. notif: Always sort user IDs in pm_users. 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. narrow [nfc]: Document more details on identifying group PMs. Which turned up a couple of bugs! We'll fix those later in this series. example data: Take sender and recipients as pmMessage arguments. 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. example data [nfc]: Use cleaner workaround for Flow "unsealed" issue. 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) types: Make some more indexer-using object types inexact. 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.

gnprice added the P1 high-priority label Oct 29, 2020

gnprice requested a review from chrisbobbe October 29, 2020 23:45

gnprice mentioned this pull request Oct 29, 2020

Group PM view from notification doesn't see new messages #4293

Closed

chrisbobbe reviewed Nov 2, 2020

View reviewed changes

gnprice added 12 commits November 2, 2020 14:59

narrow [nfc]: Document more details on identifying group PMs.

7727e29

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

example data [nfc]: Use cleaner workaround for Flow "unsealed" issue.

5fc0840

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)

types: Make some more indexer-using object types inexact.

10ef7c0

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.

example data: Fix recipients in pmMessage defaults.

dfe8e12

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

example data: Add thirdUser, to go with otherUser and selfUser.

6d9f4da

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.

narrow tests [nfc]: Make isMessageInNarrow tests table-driven.

fe6f886

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

narrow tests: Add more test cases for isMessageInNarrow.

891db43

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

gnprice force-pushed the pr-narrow-fixes branch from 6ca5c2a to 8fb6ac7 Compare November 2, 2020 23:38

gnprice added 5 commits November 2, 2020 15:38

internalLinks tests [nfc]: Clean up slightly around usersById.

67919da

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.

narrow: Normalize set of users in parsing group-PM links.

ff872f6

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

gnprice force-pushed the pr-narrow-fixes branch from 8fb6ac7 to ff872f6 Compare November 2, 2020 23:41

chrisbobbe merged commit ff872f6 into zulip:master Nov 3, 2020

gnprice deleted the pr-narrow-fixes branch November 3, 2020 00:43

This was referenced Nov 3, 2020

outbox: Fix some bugs from Narrow confusion; remove Narrow #4299

Merged

narrow [nfc]: Cut Narrow from the Outbox type. #3889

Closed

gnprice mentioned this pull request Dec 11, 2020

narrow/recipient: Always centrally filter and sort PM users, and verify that. #4335

Merged

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

gnprice commented Oct 29, 2020

chrisbobbe left a comment

chrisbobbe Nov 2, 2020

gnprice Nov 2, 2020

chrisbobbe Nov 2, 2020

gnprice Nov 2, 2020

chrisbobbe Nov 2, 2020

chrisbobbe Nov 2, 2020

chrisbobbe Nov 2, 2020

gnprice commented Nov 2, 2020

chrisbobbe commented Nov 3, 2020

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

Conversation

gnprice commented Oct 29, 2020

chrisbobbe left a comment

Choose a reason for hiding this comment

chrisbobbe Nov 2, 2020

Choose a reason for hiding this comment

gnprice Nov 2, 2020

Choose a reason for hiding this comment

chrisbobbe Nov 2, 2020

Choose a reason for hiding this comment

gnprice Nov 2, 2020

Choose a reason for hiding this comment

chrisbobbe Nov 2, 2020

Choose a reason for hiding this comment

chrisbobbe Nov 2, 2020

Choose a reason for hiding this comment

chrisbobbe Nov 2, 2020

Choose a reason for hiding this comment

gnprice commented Nov 2, 2020

chrisbobbe commented Nov 3, 2020