Skip to content

Commit

Permalink
narrow: Re-normalize group PM user-set in isMessageInNarrow.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
gnprice committed Oct 29, 2020
1 parent 51c8d55 commit 3270388
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 7 deletions.
8 changes: 8 additions & 0 deletions src/utils/__tests__/narrow-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,14 @@ describe('isMessageInNarrow', () => {
['self-1:1 message', false, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser] })],
['stream message', false, eg.streamMessage()],
]],
['group-PM conversation, including self', groupNarrow([eg.selfUser.email, eg.otherUser.email, eg.thirdUser.email]), [
['matching group-PM, inbound', true, eg.pmMessage({ recipients: [eg.selfUser, eg.otherUser, eg.thirdUser] })],
['matching group-PM, outbound', true, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser, eg.otherUser, eg.thirdUser] })],
['1:1 within group, inbound', false, eg.pmMessage()],
['1:1 within group, outbound', false, eg.pmMessage({ sender: eg.selfUser })],
['self-1:1 message', false, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser] })],
['stream message', false, eg.streamMessage()],
]],
['all-PMs narrow', ALL_PRIVATE_NARROW, [
['a PM', true, eg.pmMessage()],
['stream message', false, eg.streamMessage()],
Expand Down
16 changes: 9 additions & 7 deletions src/utils/narrow.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ export const privateNarrow = (email: string): Narrow => [
* A group PM narrow.
*
* The users represented in `emails` should agree, as a (multi)set, with
* `pmKeyRecipientsFromMessage`.
* `pmKeyRecipientsFromMessage`. But this isn't checked, and there may be
* bugs where they don't; some consumers of this data re-normalize to be sure.
*
* They might not have a consistent sorting. (This would be good to fix.)
* Consumers of this data should sort for themselves when making comparisons.
Expand All @@ -46,11 +47,8 @@ export const privateNarrow = (email: string): Narrow => [
//
// But we also have some callers that don't even ensure the set is the right
// one, with the self-user properly there or not. Known call stacks:
// * BUG #4293: getNarrowFromNotificationData: comes from notification's
// pm_users... which is sorted but not filtered. This means if you
// follow a group PM notif, then get another message in that
// conversation, it won't appear. (And if you send, it'll promptly
// disappear.)
// * BUG, at least latent: getNarrowFromNotificationData: comes from
// notification's pm_users... which is sorted but not filtered.
// * BUG, ish: getNarrowFromLink doesn't ensure this precondition is met.
// And... there's basically a bug in the webapp, where the URL that
// appears in the location bar for a group PM conversation excludes
Expand Down Expand Up @@ -261,7 +259,11 @@ export const isMessageInNarrow = (message: Message, narrow: Narrow, ownEmail: st
return false;
}
const recipients: PmRecipientUser[] = message.display_recipient;
return normalizeRecipientsSansMe(recipients, ownEmail) === emails.sort().join(',');
const narrowAsRecipients = emails.map(email => ({ email }));
return (
normalizeRecipientsSansMe(recipients, ownEmail)
=== normalizeRecipientsSansMe(narrowAsRecipients, ownEmail)
);
};

const { flags } = message;
Expand Down

0 comments on commit 3270388

Please sign in to comment.