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

narrow: Treat group PMs and 1:1 PMs more symmetrically. #4330

Merged
merged 10 commits into from
Dec 8, 2020
Merged

narrow: Treat group PMs and 1:1 PMs more symmetrically. #4330

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
82a8246
narrow [nfc]: Factor out pmNarrowByString.
gnprice Feb 5, 2020
8c99f75
narrow [nfc]: Rename pmNarrowFromEmail, from privateNarrow.
gnprice Feb 5, 2020
c1982b7
narrow [nfc]: Rename pmNarrowFromEmails, from groupNarrow.
gnprice Feb 5, 2020
97cdfac
narrow [nfc]: Make pmNarrowFromEmail a wrapper for its plural; add js…
gnprice Feb 5, 2020
1d02855
narrow [nfc]: Unpack narrow structurally in isNarrowValid.
gnprice Feb 6, 2020
4f14e6b
narrow: Fix isNarrowValid for group PMs.
gnprice Feb 6, 2020
33a08ec
narrow [nfc]: Rename some surprisingly-narrow predicates to be more e…
gnprice Feb 6, 2020
fd6a997
narrow [nfc]: Rename isPmNarrow, from isPrivateOrGroupNarrow.
gnprice Feb 6, 2020
f22a2bd
narrow [nfc]: Rename a few more "group narrow" -> "group PM narrow".
gnprice Feb 6, 2020
d284584
narrow [nfc]: Combine 1:1 and group PMs as a single case in caseNarrow.
gnprice Feb 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions src/chat/__tests__/narrowsSelectors-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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!

const john = eg.makeUser({ name: 'john' });
const mark = eg.makeUser({ name: 'mark' });

Expand All @@ -298,21 +298,23 @@ describe('isNarrowValid', () => {
expect(result).toBe(true);
});

test('narrowing to a group chat with non-existing users is also valid', () => {
test('narrowing to a group chat with non-existing users is not valid', () => {
const john = eg.makeUser({ name: 'john' });
const mark = eg.makeUser({ name: 'mark' });
const state = eg.reduxState({
realm: {
...eg.realmState(),
crossRealmBots: [],
nonActiveUsers: [],
},
streams: [],
users: [],
users: [john],
});
const narrow = pmNarrowFromEmails(['john@example.com', 'mark@example.com']);
const narrow = pmNarrowFromEmails([john.email, mark.email]);

const result = isNarrowValid(state, narrow);

expect(result).toBe(true);
expect(result).toBe(false);
});

test('narrowing to a PM with bots is valid', () => {
Expand Down
1 change: 1 addition & 0 deletions src/chat/narrowsSelectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ export const isNarrowValid: Selector<boolean, Narrow> = createSelector(
stream: streamName => streams.find(s => s.name === streamName) !== undefined,
topic: streamName => streams.find(s => s.name === streamName) !== undefined,
pm: email => allUsersByEmail.get(email) !== undefined,
groupPm: emails => emails.every(email => allUsersByEmail.get(email) !== undefined),
},
() => true,
),
Expand Down