narrow: Treat group PMs and 1:1 PMs more symmetrically. #4330
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We have two "constructors" `privateNarrow` and `groupNarrow` for 1:1 PMs and group PMs respectively, but they end up building basically the same data structure. Then in a lot of places in our code we have control flow to handle the group-PM case on the one hand and the 1:1 case on the other, and it ends up just saying the same thing twice to treat them both the same way. (In a few places, like `getNarrowFromMessage`, we have no such logic and already implicitly rely on the fact that they're the same underneath.) In general the Zulip model is that group PMs and 1:1 PMs are very much alike, and the Zulip API generally treats them alike. So let's move toward more consistently handling them alike, and only having conditionals to distinguish between them when we actually want to do something different. As a first step, in this commit, factor out the common logic from both "constructors" and turn them into trivial wrappers.
After all, a group PM narrow is just as "private" or "PM" as a
1:1 PM narrow. This name makes explicit the actual difference:
that this one takes just one email. (We'll rename `groupNarrow`
too, in the next commit.)
$ perl -i -0pe '
s/\bprivateNarrow\b/pmNarrowFromEmail/g
' src/**/*.js
$ tools/fmt
This makes it parallel to `pmNarrowFromEmail` (formerly
`privateNarrow`), and makes clear the actual difference
between them: this one can take the emails of several users.
$ perl -i -0pe '
s/\bgroupNarrow\b/pmNarrowFromEmails/g
' src/**/*.js
$ tools/fmt
Instead of using `narrow[0].operand` and hoping we're treating the types correctly, use the `caseNarrow` family to unpack the components of the narrow in a structured way. This makes conspicuous that the `isPrivateNarrow` / `pm:` case only covers 1:1 PMs, as the argument is a single `email`. We'll fix that next.
This was unintentionally failing to handle group PMs, so that it unconditionally returned true for them, unlike 1:1 PMs. Fix that. Oddly, there was a test checking that this behavior was this way, so it was noticed before. It's not clear why it was thought desirable, though. The test was added in cefbe6d in 2018-05, long after the logic itself was written in b0c7323 in 2017-12, and neither commit explains the discrepancy.
…xplicit.
There are very few places where we really want to distinguish between
PM narrows that are 1:1 and that are with groups. The more natural
category, both in our implementation logic and in the user experience,
is the one that includes both types of narrows as "PM narrows".
So, instead of having a long complicated name (isPrivateOrGroupNarrow)
for the predicate that tests that category and short ones
(isPrivateNarrow, isGroupNarrow) for those that test the more specific
categories, we should have longer and more explicit names for the
latter and a simpler name for the former.
Here, do the first part of that change:
isPrivateNarrow -> is1to1PmNarrow
isGroupNarrow -> isGroupPmNarrow
We'll do the remaining rename in the next commit:
isPrivateOrGroupNarrow -> isPmNarrow
Done automatically, with:
$ perl -i -0pe '
s/isPrivateNarrow/is1to1PmNarrow/g;
s/isGroupNarrow/isGroupPmNarrow/g
' src/**/*.js
As discussed in the parent commit, this category is typically the
more natural one than being specific about 1:1 vs. group PMs.
So, give this one the simpler, shorter name.
The new names also make the relationship between these predicates
clear in a way it wasn't when the term "private" mean 1:1 PMs:
isPmNarrow
is1to1PmNarrow
isGroupPmNarrow
Made the change automatically, like so:
$ perl -i -0pe '
s/isPrivateOrGroupNarrow/isPmNarrow/g
' src/**/*.js
$ tools/fmt
This goes along with our renaming a few commits ago of isGroupNarrow to isGroupPmNarrow. Here again, it just makes it a bit more explicit that these are a specific case of the general class of PM narrows. (Probably all these functions shouldn't exist, either -- the users, or perhaps user IDs, should just get passed down as props to the couple of components these are here for. But that's a different refactor.)
Most call sites get simpler, because they were already doing the same thing in the two cases.
chrisbobbe
reviewed
Dec 8, 2020
| @@ -278,7 +278,7 @@ describe('isNarrowValid', () => { | |||
| expect(result).toBe(false); | |||
| }); | |||
|
|
|||
| test('narrowing to a group chat with non-existing user is not valid', () => { | |||
| test('narrowing to a group chat with existing users is valid', () => { | |||
There was a problem hiding this comment.
4f14e6b narrow: Fix isNarrowValid for group PMs.
Oddly, there was a test checking that this behavior was this way, so
it was noticed before. It's not clear why it was thought desirable,
though. The test was added in cefbe6d in 2018-05, long after the
logic itself was written in b0c7323 in 2017-12, and neither commit
explains the discrepancy.
Weird!
|
LGTM, merged, thanks! |
gnprice
added a commit
to gnprice/zulip-mobile
that referenced
this pull request
Jan 6, 2021
This completes a major objective of the long string of refactoring that appeared in the series of PRs zulip#4330, zulip#4332, zulip#4335, zulip#4339, zulip#4342, then zulip#4346, zulip#4356, zulip#4361, zulip#4364, and most recently zulip#4368. After this change, the portion of zulip#4333 that's about PMs, emails, and user IDs -- aka the portion of zulip#3764 that's about narrows -- is complete. Still open from zulip#4333 is to convert stream and topic narrows from stream names to stream IDs. I'm hopeful that that will be simpler: (a) unlike the situation for PMs, there's just one stream mentioned at a time, so there's no question of sorting, and (b) there isn't the "include self or not" complication that's bedeviled much of our code related to PMs. In other words, stream and topic narrows don't suffer from zulip#4035.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We have two "constructors"
privateNarrowandgroupNarrowfor 1:1 PMs and group PMs respectively, but they end up building basically the same data structure. Then in a lot of places in our code we have control flow to handle the group-PM case on the one hand and the 1:1 case on the other, and it ends up just saying the same thing twice to treat them both the same way. (In a few places, likegetNarrowFromMessage, we have no such logic and already implicitly rely on the fact that they're the same underneath.)In general the Zulip model is that group PMs and 1:1 PMs are very much alike, and the Zulip API generally treats them alike. So let's move toward more consistently handling them alike, and only having conditionals to distinguish between them when we actually want to do something different.
I wrote the bulk of these changes back in February, as part of a larger draft branch for refactoring our handling of narrows. Prompted today by #4317 (comment) , I went and took another look at that branch. Here's a portion of it -- rebased forward, and with proper commit messages added where it didn't have them. (For some other portions of the original branch, I'm either not convinced they're the right direction or they just need a bit more work.)