Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
gnprice opened this issue Oct 29, 2020 · 1 comment · Fixed by #4294
Closed

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

gnprice opened this issue Oct 29, 2020 · 1 comment · Fixed by #4294
Assignees
@gnprice
Labels
a-notifications P1 high-priority

Comments

@gnprice
Copy link
Member

gnprice commented Oct 29, 2020

Steps to repro:

  • Get a notification for a group PM.
  • Open the notification. The app will launch as needed, and navigate to the group PM conversation. (Provided you've already been using the same account.) The latest messages up to this point will show up.
  • Send a message.
  • Expected: The message shows up in the message list as usual.
  • Actual: The message briefly shows up in the message list, with a spinner, while it's in the outbox; as soon as it's actually sent, it disappears.

Or, similarly:

  • Same steps 1 and 2.
  • Have another participant in the conversation send another message.
  • Expected: The message shows up in the message list.
  • Actual: Nothing happens. The message never appears.

What's happening here is that 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 some of our code relies on that assumption.

But in most of the server API, the preferred representation of a group PM conversation includes all users in it, including self. In particular, this is what's used in the pm_users property of a notification. And when you open a group PM notification, we end up taking you to a narrow described by that full list, breaking that assumption.

One particular piece of code that relies on that assumption is the function isMessageInNarrow, which we use when handling an incoming new-message event from the server to decide whether to add the new message to each of the narrows we're tracking. Because the narrow's description breaks that assumption, the message isn't seen as belonging to the narrow, and we have the bug.

This issue was masked for a long time by the issue fixed in #3922: opening a group PM notification didn't successfully navigate to the conversation at all. But now it does! So this issue is visible -- and it's probably actually more frustrating, because it looks like e.g. the message you sent didn't send at all.
@gnprice gnprice self-assigned this Oct 29, 2020
gnprice added a commit to gnprice/zulip-mobile that referenced this issue Oct 29, 2020
@gnprice
narrow: Re-normalize group PM user-set in isMessageInNarrow.
3270388 
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
gnprice added a commit to gnprice/zulip-mobile that referenced this issue Oct 29, 2020
@gnprice
notif: Normalize set of users to represent a group PM thread.
1492711 
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.
@gnprice gnprice mentioned this issue Oct 29, 2020
Merged
@gnprice
Copy link
Member Author

gnprice commented Oct 29, 2020

BTW this is an old, old bug -- I remember seeing these symptoms in 2018, before the issue fixed in #3922 appeared and blocked getting as far as running into them. Glad to be making progress now, with work like #4294 (which fixes this bug), on unraveling the tangled "narrows" logic, at the center of the app's data model, which turned out to be at the root of it.

gnprice added a commit to gnprice/zulip-mobile that referenced this issue Nov 2, 2020
@gnprice
narrow: Re-normalize group PM user-set in isMessageInNarrow.
e831e64 
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
gnprice added a commit to gnprice/zulip-mobile that referenced this issue Nov 2, 2020
@gnprice
notif: Normalize set of users to represent a group PM thread.
1c6ac87 
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.
gnprice added a commit to gnprice/zulip-mobile that referenced this issue Nov 2, 2020
@gnprice
notif: Normalize set of users to represent a group PM thread.
f95ea03 
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.
abhi0504 pushed a commit to abhi0504/zulip-mobile that referenced this issue Nov 24, 2020
@gnprice @abhi0504
narrow: Re-normalize group PM user-set in isMessageInNarrow.
8254d72 
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
abhi0504 pushed a commit to abhi0504/zulip-mobile that referenced this issue Nov 24, 2020
@gnprice @abhi0504
notif: Normalize set of users to represent a group PM thread.
43311d6 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gnprice gnprice

Labels
a-notifications P1 high-priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix some "is message in narrow" bugs, and test much more thoroughly gnprice/zulip-mobile
1 participant
@gnprice