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.

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

isMessageInNarrow: Fix regression in matchRecipients. #3328

Closed
wants to merge 1 commit into from

Conversation

jainkuniya
Copy link
Member

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: #3324

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
@gnprice gnprice added the review label Feb 5, 2019
Copy link
Member Author

@jainkuniya jainkuniya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gnprice @borisyankov Let's fix this bug too in the next release.

@gnprice
Copy link
Member

gnprice commented Feb 25, 2019

Thanks @jainkuniya ! This bug will be good to fix.

I don't love this solution. The existing code is too complicated and lacks conceptual clarity, which is the root cause of the issue -- and the same thing is still true after this change.

One way to see that: what does this matchRecipients function mean? How would you describe it in a jsdoc comment? I think it will be challenging to write a description that is completely accurate and precise and yet is reasonably short, because the function's meaning is somewhat confused. (Which, again, was already true before this change. 🙂 )

Relatedly: a bugfix like this definitely calls for unit tests. 😛 There should be a test added that demonstrates the bug, and would fail on the old code.

@gnprice
Copy link
Member

gnprice commented Feb 25, 2019

(Let's continue on the issue thread.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants