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

Self-PM appears in other PM conversations #3324

Closed
jainkuniya opened this issue Feb 4, 2019 · 4 comments · Fixed by #4294
Closed

Self-PM appears in other PM conversations #3324

jainkuniya opened this issue Feb 4, 2019 · 4 comments · Fixed by #4294
Assignees
@gnprice

Comments

@jainkuniya
Copy link
Member

Steps to reproduce.

  • Open any PM narrow other than self, so that newer caughtup for that narrow become true.
  • Now open self narrow and send new message.
  • Now again narrow back to same narrow which you picked in step 1.

Observation
The message which we posted in step two (to self) is also visible in other narrow (which we picked in step 1,3)

Restart the app, all works fine.

Issue is why message got added to list of other narrow?

Because there is an issue in our matchRecipients of isMessageInNarrow in narrow.js.

As for all PM narrow, normalizedRecipients === ownEmail is true for self message.
Because in this condition we are not taking care of narrow, it just compare recipient in message and ownEmail.

@zulipbot claim

Will send a PR soon for this.
@borisyankov
Copy link
Contributor

Is that a bug though? This is how that works in the web app.

@jainkuniya
Copy link
Member Author

@borisyankov By "all private narrow" , I doesn't mean "All private message".
I mean all other private narrows.

(editing title to all other private narrows )

@jainkuniya jainkuniya changed the title Messages send to self appears in all private narrow. Messages send to self appears in all other private narrows. Feb 4, 2019
jainkuniya added a commit to jainkuniya/zulip-mobile that referenced this issue Feb 5, 2019
@jainkuniya
isMessageInNarrow: Fix regression in matchRecipients.
0858ab5 
Remove `normalizedRecipients === ownEmail` condition,
as it is not taking `narrow` in consideration.
For self message, `display_recipient` will only consist of
`ownEmail`. Thus for calling `isMessageInNarrow` with self
message and any private narrow will return `true`.
So remove this redundant consition.

Fixes: zulip#3324
jainkuniya added a commit to jainkuniya/zulip-mobile that referenced this issue Feb 5, 2019
@jainkuniya
isMessageInNarrow: Fix regression in matchRecipients.
51ad0b0 
Remove `normalizedRecipients === ownEmail` condition, as it is not
taking `narrow` in consideration. For self message, `display_recipient`
will only consist of `ownEmail`. Thus for calling `isMessageInNarrow`
with self message and any private narrow was returning `true` (because
of `normalizedRecipients === ownEmail`). So remove this condition.

Also improve `normalizedNarrow`, so that it consist of only unique
values. Now this change will take care of logic which was intended to be
taken by previous condition (which got removed, by this commi).

Fixes: zulip#3324
jainkuniya added a commit to jainkuniya/zulip-mobile that referenced this issue Feb 5, 2019
@jainkuniya
isMessageInNarrow: Fix regression in matchRecipients.
3336e85 
Remove `normalizedRecipients === ownEmail` condition, as it is not
taking `narrow` in consideration. For self message, `display_recipient`
will only consist of `ownEmail`. Thus for calling `isMessageInNarrow`
with self message and any private narrow was returning `true` (because
of `normalizedRecipients === ownEmail`). So remove this condition.

Also improve `normalizedNarrow`, so that it consist of only unique
values. Now this change will take care of logic which was intended to be
taken by `normalizedRecipients === ownEmail` (which is getting removed,
by this commit).

Fixes: zulip#3324
@jainkuniya jainkuniya mentioned this issue Feb 5, 2019
Closed
@gnprice
Copy link
Member

gnprice commented Feb 25, 2019

Thanks @jainkuniya !

Following up on my comments on #3328, here's what I think would make a good fix for this bug:

  • Add some unit tests that clearly demonstrate the bug. They should fail if run on master.
  • I think the root cause of the bug is that matchRecipients is too complicated. So let's fix that.
  • I think one of the reasons it became too complicated is that it's handling both 1:1 PMs and group PMs. Let's write each case separately.
  • The group case can be a lot simpler than that matchRecipients function. Try writing it very simply.
  • The 1:1 case can also be simpler. Try writing it out more explicitly. I think a conditional like email === ownEmail ? ... may help.

@sumukhah sumukhah mentioned this issue Mar 28, 2020
Closed
gnprice added a commit to gnprice/zulip-mobile that referenced this issue Oct 29, 2020
@gnprice
narrow: Fix bug where self-1:1 "is in" every PM conversation.
51c8d55 
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
@gnprice gnprice self-assigned this Oct 29, 2020
@gnprice gnprice mentioned this issue Oct 29, 2020
Merged
@gnprice
Copy link
Member

gnprice commented Oct 29, 2020 •
edited
Loading

FTR, although #3328 refers to a "regression in matchRecipients", this bug is much older, and older than that function. It dates back to 2017-01 with commit d8faae9; it's present at that commit, and absent at its parent.

Before that point, the code had the opposite bug on self-1:1 messages: they didn't appear even in their own conversation narrow. That commit aimed to fix that, and introduced the puzzling normalizedRecipients === ownEmail condition mentioned above, and with it introduced this bug. For details see the fix: 51c8d55 (or its merged-to-master version in the future.)

@gnprice gnprice changed the title Messages send to self appears in all other private narrows. Self-PM appears in other PM conversations Oct 29, 2020
abhi0504 pushed a commit to abhi0504/zulip-mobile that referenced this issue Nov 24, 2020
@gnprice @abhi0504
narrow: Fix bug where self-1:1 "is in" every PM conversation.
c8b96f8 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gnprice gnprice

Labels
None yet
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
3 participants
@gnprice @borisyankov @jainkuniya