From 6dc161f18a40bc96701c215d39ac8071ee25d702 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Dec 2020 20:58:36 -0800 Subject: [PATCH 1/8] narrow [nfc]: Cut unused function isSameNarrow. This has no callers at all except its own tests. The only one it's ever had was a trivial one inside the `narrow` module -- and that appeared nearly a year after the function itself was introduced. --- src/utils/__tests__/narrow-test.js | 16 ---------------- src/utils/narrow.js | 4 ---- 2 files changed, 20 deletions(-) diff --git a/src/utils/__tests__/narrow-test.js b/src/utils/__tests__/narrow-test.js index d11d60e4620..950181c71bf 100644 --- a/src/utils/__tests__/narrow-test.js +++ b/src/utils/__tests__/narrow-test.js @@ -6,7 +6,6 @@ import { pmNarrowFromEmail, is1to1PmNarrow, pmNarrowFromUsersUnsafe, - specialNarrow, isSpecialNarrow, ALL_PRIVATE_NARROW, streamNarrow, @@ -17,7 +16,6 @@ import { isSearchNarrow, isPmNarrow, isMessageInNarrow, - isSameNarrow, isStreamOrTopicNarrow, getNarrowsForMessage, getNarrowForReply, @@ -342,20 +340,6 @@ describe('getNarrowForReply', () => { }); }); -describe('isSameNarrow', () => { - test('Return true if two narrows are same', () => { - expect(isSameNarrow(streamNarrow('stream'), streamNarrow('stream'))).toBe(true); - expect(isSameNarrow(streamNarrow('stream'), streamNarrow('stream1'))).toBe(false); - expect(isSameNarrow(streamNarrow('stream'), topicNarrow('stream', 'topic'))).toBe(false); - expect(isSameNarrow(topicNarrow('stream', 'topic'), topicNarrow('stream', 'topic'))).toBe(true); - expect(isSameNarrow(topicNarrow('stream', 'topic'), topicNarrow('stream', 'topic1'))).toBe( - false, - ); - expect(isSameNarrow(HOME_NARROW, specialNarrow('private'))).toBe(false); - expect(isSameNarrow(HOME_NARROW, HOME_NARROW)).toBe(true); - }); -}); - describe('keyFromNarrow+parseNarrow', () => { const baseNarrows = [ ['whole stream', streamNarrow(eg.stream.name)], diff --git a/src/utils/narrow.js b/src/utils/narrow.js index 244a95b6814..04e58936db4 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -1,6 +1,5 @@ /* @flow strict-local */ import invariant from 'invariant'; -import isEqual from 'lodash.isequal'; import type { ApiNarrow, Message, Outbox, User, UserOrBot } from '../types'; import { @@ -33,9 +32,6 @@ import { */ export opaque type Narrow = ApiNarrow; -export const isSameNarrow = (narrow1: Narrow, narrow2: Narrow): boolean => - Array.isArray(narrow1) && Array.isArray(narrow2) && isEqual(narrow1, narrow2); - export const HOME_NARROW: Narrow = []; export const HOME_NARROW_STR: string = keyFromNarrow(HOME_NARROW); From cf4207f479b8f484a728ef46e5d253c887c42b2e Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Dec 2020 21:27:29 -0800 Subject: [PATCH 2/8] narrow [nfc]: Switch the Narrow type to a reasonable structure! Because of all the work we've done over the last few series to encapsulate the details of the Narrow type within this module, not much has to change! Just the constructors; the pattern-match engine caseNarrow; and apiNarrowFromNarrow, where we'd been taking a shortcut using the fact that Narrow secretly was ApiNarrow. Well, those and one call site to `specialNarrow`, where we newly enforce an invariant we'd long assumed about what kind of `is:foo` narrows we support. --- src/utils/internalLinks.js | 6 +- src/utils/narrow.js | 113 ++++++++++++++++++------------------- 2 files changed, 59 insertions(+), 60 deletions(-) diff --git a/src/utils/internalLinks.js b/src/utils/internalLinks.js index 3abbb7aaffe..c7842eefa0d 100644 --- a/src/utils/internalLinks.js +++ b/src/utils/internalLinks.js @@ -142,7 +142,11 @@ export const getNarrowFromLink = ( case 'stream': return streamNarrow(parseStreamOperand(paths[1], streamsById)); case 'special': - return specialNarrow(paths[1]); + try { + return specialNarrow(paths[1]); + } catch (e) { + return null; + } default: return null; } diff --git a/src/utils/narrow.js b/src/utils/narrow.js index 04e58936db4..a067cb65a95 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -30,9 +30,14 @@ import { * * `ApiNarrow` for the form we put a narrow in when talking to the * server, and `apiNarrowOfNarrow` for converting to it. */ -export opaque type Narrow = ApiNarrow; +export opaque type Narrow = + | {| type: 'stream', streamName: string |} + | {| type: 'topic', streamName: string, topic: string |} + | {| type: 'pm', joinedEmails: string |} + | {| type: 'search', query: string |} + | {| type: 'all' | 'starred' | 'mentioned' | 'all-pm' |}; -export const HOME_NARROW: Narrow = []; +export const HOME_NARROW: Narrow = Object.freeze({ type: 'all' }); export const HOME_NARROW_STR: string = keyFromNarrow(HOME_NARROW); @@ -46,12 +51,8 @@ export const HOME_NARROW_STR: string = keyFromNarrow(HOME_NARROW); * https://zulipchat.com/api/construct-narrow * https://github.com/zulip/zulip/issues/13167 */ -const pmNarrowByString = (emails: string): Narrow => [ - { - operator: 'pm-with', - operand: emails, - }, -]; +const pmNarrowByString = (emails: string): Narrow => + Object.freeze({ type: 'pm', joinedEmails: emails }); /** * A PM narrow, either 1:1 or group. @@ -136,12 +137,18 @@ export const pmNarrowFromUsersUnsafe = (recipients: UserOrBot[]): Narrow => */ export const pm1to1NarrowFromUser = (user: UserOrBot): Narrow => pmNarrowFromEmails([user.email]); -export const specialNarrow = (operand: string): Narrow => [ - { - operator: 'is', - operand, - }, -]; +export const specialNarrow = (operand: string): Narrow => { + if (operand === 'starred') { + return Object.freeze({ type: 'starred' }); + } + if (operand === 'mentioned') { + return Object.freeze({ type: 'mentioned' }); + } + if (operand === 'private') { + return Object.freeze({ type: 'all-pm' }); + } + throw new Error(`specialNarrow: got unsupported operand: ${operand}`); +}; export const STARRED_NARROW = specialNarrow('starred'); @@ -155,30 +162,13 @@ export const ALL_PRIVATE_NARROW = specialNarrow('private'); export const ALL_PRIVATE_NARROW_STR = keyFromNarrow(ALL_PRIVATE_NARROW); -export const streamNarrow = (stream: string): Narrow => [ - { - operator: 'stream', - operand: stream, - }, -]; - -export const topicNarrow = (stream: string, topic: string): Narrow => [ - { - operator: 'stream', - operand: stream, - }, - { - operator: 'topic', - operand: topic, - }, -]; - -export const SEARCH_NARROW = (query: string): Narrow => [ - { - operator: 'search', - operand: query, - }, -]; +export const streamNarrow = (stream: string): Narrow => + Object.freeze({ type: 'stream', streamName: stream }); + +export const topicNarrow = (stream: string, topic: string): Narrow => + Object.freeze({ type: 'topic', streamName: stream, topic }); + +export const SEARCH_NARROW = (query: string): Narrow => Object.freeze({ type: 'search', query }); type NarrowCases = {| home: () => T, @@ -197,26 +187,18 @@ export function caseNarrow(narrow: Narrow, cases: NarrowCases): T { throw new Error(`bad narrow: ${JSON.stringify(narrow)}`); }; - switch (narrow.length) { - case 0: return cases.home(); - case 1: - switch (narrow[0].operator) { - case 'pm-with': { - const emails = narrow[0].operand.split(','); - return cases.pm(emails); - } - case 'is': - switch (narrow[0].operand) { - case 'starred': return cases.starred(); - case 'mentioned': return cases.mentioned(); - case 'private': return cases.allPrivate(); - default: return err(); - } - case 'stream': return cases.stream(narrow[0].operand); - case 'search': return cases.search(narrow[0].operand); - default: return err(); - } - case 2: return cases.topic(narrow[0].operand, narrow[1].operand); + switch (narrow.type) { + case 'stream': return cases.stream(narrow.streamName); + case 'topic': return cases.topic(narrow.streamName, narrow.topic); + case 'pm': { + const emails = narrow.joinedEmails.split(','); + return cases.pm(emails); + } + case 'search': return cases.search(narrow.query); + case 'all': return cases.home(); + case 'starred': return cases.starred(); + case 'mentioned': return cases.mentioned(); + case 'all-pm': return cases.allPrivate(); default: return err(); } } @@ -465,7 +447,20 @@ export const isSearchNarrow = (narrow?: Narrow): boolean => /** * Convert the narrow into the form used in the Zulip API at get-messages. */ -export const apiNarrowOfNarrow = (narrow: Narrow): ApiNarrow => narrow; +export const apiNarrowOfNarrow = (narrow: Narrow): ApiNarrow => + caseNarrow(narrow, { + stream: streamName => [{ operator: 'stream', operand: streamName }], + topic: (streamName, topic) => [ + { operator: 'stream', operand: streamName }, + { operator: 'topic', operand: topic }, + ], + pm: emails => [{ operator: 'pm-with', operand: emails.join(',') }], + search: query => [{ operator: 'search', operand: query }], + home: () => [], + starred: () => [{ operator: 'is', operand: 'starred' }], + mentioned: () => [{ operator: 'is', operand: 'mentioned' }], + allPrivate: () => [{ operator: 'is', operand: 'private' }], + }); /** * True just if the given message is part of the given narrow. From c18d63469bc305a2f2d8942e910b8d8df0ed908c Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Dec 2020 21:29:52 -0800 Subject: [PATCH 3/8] narrow [nfc]: Stop joining and splitting emails! This isn't as good as not even *using* emails, in favor of user IDs... but it's a step on the way there, as well as a small nice simplification in itself. --- src/utils/narrow.js | 28 ++++++---------------------- 1 file changed, 6 insertions(+), 22 deletions(-) diff --git a/src/utils/narrow.js b/src/utils/narrow.js index a067cb65a95..b96da9b1495 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -33,7 +33,7 @@ import { export opaque type Narrow = | {| type: 'stream', streamName: string |} | {| type: 'topic', streamName: string, topic: string |} - | {| type: 'pm', joinedEmails: string |} + | {| type: 'pm', emails: $ReadOnlyArray |} | {| type: 'search', query: string |} | {| type: 'all' | 'starred' | 'mentioned' | 'all-pm' |}; @@ -41,19 +41,6 @@ export const HOME_NARROW: Narrow = Object.freeze({ type: 'all' }); export const HOME_NARROW_STR: string = keyFromNarrow(HOME_NARROW); -/** - * A PM narrow, either 1:1 or group. - * - * Private to this module because the input format is kind of odd. - * Use `pmNarrowFromEmail` or `pmNarrowFromEmails` instead. - * - * For the quirks of the underlying format in the Zulip API, see: - * https://zulipchat.com/api/construct-narrow - * https://github.com/zulip/zulip/issues/13167 - */ -const pmNarrowByString = (emails: string): Narrow => - Object.freeze({ type: 'pm', joinedEmails: emails }); - /** * A PM narrow, either 1:1 or group. * @@ -73,7 +60,7 @@ const pmNarrowByString = (emails: string): Narrow => * accidentally disagreeing on whether to include the self-user, or on how * to sort the list (by user ID vs. email), or neglecting to sort it at all. */ -const pmNarrowFromEmails = (emails: string[]): Narrow => pmNarrowByString(emails.join()); +const pmNarrowFromEmails = (emails: string[]): Narrow => Object.freeze({ type: 'pm', emails }); /** * DEPRECATED. Use `pm1to1NarrowFromUser` instead. @@ -172,7 +159,7 @@ export const SEARCH_NARROW = (query: string): Narrow => Object.freeze({ type: 's type NarrowCases = {| home: () => T, - pm: (emails: string[]) => T, + pm: (emails: $ReadOnlyArray) => T, starred: () => T, mentioned: () => T, allPrivate: () => T, @@ -190,10 +177,7 @@ export function caseNarrow(narrow: Narrow, cases: NarrowCases): T { switch (narrow.type) { case 'stream': return cases.stream(narrow.streamName); case 'topic': return cases.topic(narrow.streamName, narrow.topic); - case 'pm': { - const emails = narrow.joinedEmails.split(','); - return cases.pm(emails); - } + case 'pm': return cases.pm(narrow.emails); case 'search': return cases.search(narrow.query); case 'all': return cases.home(); case 'starred': return cases.starred(); @@ -360,7 +344,7 @@ export const isGroupPmNarrow = (narrow?: Narrow): boolean => * Any use of this probably means something higher up should be refactored * to use caseNarrow. */ -export const emailsOfGroupPmNarrow = (narrow: Narrow): string[] => +export const emailsOfGroupPmNarrow = (narrow: Narrow): $ReadOnlyArray => caseNarrowPartial(narrow, { pm: emails => { if (emails.length === 1) { @@ -395,7 +379,7 @@ export const emailOfPm1to1Narrow = (narrow: Narrow): string => * This is the same list of users that can appear in a `PmKeyRecipients` or * `PmKeyUsers`, but contains only their emails. */ -export const emailsOfPmNarrow = (narrow: Narrow): string[] => +export const emailsOfPmNarrow = (narrow: Narrow): $ReadOnlyArray => caseNarrowPartial(narrow, { pm: emails => emails }); /** From 0b23c25a5cbb34f6d5707ba70ae5ab49bbed1852 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Dec 2020 21:35:13 -0800 Subject: [PATCH 4/8] tests [nfc]: Replace a few more emails with whole users for PM narrows. This sets up a mass conversion we'll do in the next commit. --- src/chat/__tests__/narrowsSelectors-test.js | 8 +++++--- src/notification/__tests__/notification-test.js | 4 ++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/chat/__tests__/narrowsSelectors-test.js b/src/chat/__tests__/narrowsSelectors-test.js index ebc921df2a6..eec49c6eefa 100644 --- a/src/chat/__tests__/narrowsSelectors-test.js +++ b/src/chat/__tests__/narrowsSelectors-test.js @@ -81,14 +81,14 @@ describe('getMessagesForNarrow', () => { test('do not combine messages and outbox in different narrow', () => { const state = eg.reduxState({ narrows: Immutable.Map({ - [keyFromNarrow(pmNarrowFromEmail('john@example.com'))]: [123], + [keyFromNarrow(pmNarrowFromEmail(eg.otherUser.email))]: [123], }), messages, outbox: [outboxMessage], realm: eg.realmState({ email: eg.selfUser.email }), }); - const result = getMessagesForNarrow(state, pmNarrowFromEmail('john@example.com')); + const result = getMessagesForNarrow(state, pmNarrowFromEmail(eg.otherUser.email)); expect(result).toEqual([message]); }); @@ -193,7 +193,9 @@ describe('getStreamInNarrow', () => { }); test('return NULL_SUBSCRIPTION is narrow is not topic or stream', () => { - expect(getStreamInNarrow(state, pmNarrowFromEmail('abc@zulip.com'))).toEqual(NULL_SUBSCRIPTION); + expect(getStreamInNarrow(state, pmNarrowFromEmail(eg.otherUser.email))).toEqual( + NULL_SUBSCRIPTION, + ); expect(getStreamInNarrow(state, topicNarrow(stream4.name, 'topic'))).toEqual(NULL_SUBSCRIPTION); }); }); diff --git a/src/notification/__tests__/notification-test.js b/src/notification/__tests__/notification-test.js index c48bf1ed312..b0b4bb42020 100644 --- a/src/notification/__tests__/notification-test.js +++ b/src/notification/__tests__/notification-test.js @@ -34,10 +34,10 @@ describe('getNarrowFromNotificationData', () => { test('on notification for a private message returns a PM narrow', () => { const notification = { recipient_type: 'private', - sender_email: 'mark@example.com', + sender_email: eg.otherUser.email, }; const narrow = getNarrowFromNotificationData(notification, DEFAULT_MAP, ownUserId); - expect(narrow).toEqual(pmNarrowFromEmail('mark@example.com')); + expect(narrow).toEqual(pmNarrowFromEmail(eg.otherUser.email)); }); test('on notification for a group message returns a group narrow', () => { From ed9d48e891a92e010f7ee03bba178d552003e866 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Dec 2020 21:38:03 -0800 Subject: [PATCH 5/8] tests [nfc]: Replace all uses of pmNarrowFromEmail in tests. Done with the following commands: $ perl -i -0pe ' s#pmNarrowFromEmail\((.*?)\.email\)#pm1to1NarrowFromUser($1)#g && s/pmNarrowFromEmail\b/pm1to1NarrowFromUser/g ' src/**/__tests__/*.js $ tools/fmt then fixing one duplicate import, in narrow-test.js. --- src/chat/__tests__/narrowsSelectors-test.js | 18 +++++++-------- .../getComposeInputPlaceholder-test.js | 6 ++--- .../__tests__/notification-test.js | 4 ++-- src/title/__tests__/titleSelectors-test.js | 4 ++-- src/utils/__tests__/narrow-test.js | 23 +++++++++---------- 5 files changed, 26 insertions(+), 29 deletions(-) diff --git a/src/chat/__tests__/narrowsSelectors-test.js b/src/chat/__tests__/narrowsSelectors-test.js index eec49c6eefa..f75f4d775ef 100644 --- a/src/chat/__tests__/narrowsSelectors-test.js +++ b/src/chat/__tests__/narrowsSelectors-test.js @@ -11,7 +11,7 @@ import { import { HOME_NARROW, HOME_NARROW_STR, - pmNarrowFromEmail, + pm1to1NarrowFromUser, streamNarrow, topicNarrow, STARRED_NARROW, @@ -81,14 +81,14 @@ describe('getMessagesForNarrow', () => { test('do not combine messages and outbox in different narrow', () => { const state = eg.reduxState({ narrows: Immutable.Map({ - [keyFromNarrow(pmNarrowFromEmail(eg.otherUser.email))]: [123], + [keyFromNarrow(pm1to1NarrowFromUser(eg.otherUser))]: [123], }), messages, outbox: [outboxMessage], realm: eg.realmState({ email: eg.selfUser.email }), }); - const result = getMessagesForNarrow(state, pmNarrowFromEmail(eg.otherUser.email)); + const result = getMessagesForNarrow(state, pm1to1NarrowFromUser(eg.otherUser)); expect(result).toEqual([message]); }); @@ -193,9 +193,7 @@ describe('getStreamInNarrow', () => { }); test('return NULL_SUBSCRIPTION is narrow is not topic or stream', () => { - expect(getStreamInNarrow(state, pmNarrowFromEmail(eg.otherUser.email))).toEqual( - NULL_SUBSCRIPTION, - ); + expect(getStreamInNarrow(state, pm1to1NarrowFromUser(eg.otherUser))).toEqual(NULL_SUBSCRIPTION); expect(getStreamInNarrow(state, topicNarrow(stream4.name, 'topic'))).toEqual(NULL_SUBSCRIPTION); }); }); @@ -258,7 +256,7 @@ describe('isNarrowValid', () => { streams: [], users: [user], }); - const narrow = pmNarrowFromEmail(user.email); + const narrow = pm1to1NarrowFromUser(user); const result = isNarrowValid(state, narrow); @@ -276,7 +274,7 @@ describe('isNarrowValid', () => { streams: [], users: [], }); - const narrow = pmNarrowFromEmail(user.email); + const narrow = pm1to1NarrowFromUser(user); const result = isNarrowValid(state, narrow); @@ -333,7 +331,7 @@ describe('isNarrowValid', () => { streams: [], users: [], }); - const narrow = pmNarrowFromEmail(bot.email); + const narrow = pm1to1NarrowFromUser(bot); const result = isNarrowValid(state, narrow); @@ -351,7 +349,7 @@ describe('isNarrowValid', () => { streams: [], users: [], }); - const narrow = pmNarrowFromEmail(notActiveUser.email); + const narrow = pm1to1NarrowFromUser(notActiveUser); const result = isNarrowValid(state, narrow); diff --git a/src/compose/__tests__/getComposeInputPlaceholder-test.js b/src/compose/__tests__/getComposeInputPlaceholder-test.js index d93deef2c8d..eb056545123 100644 --- a/src/compose/__tests__/getComposeInputPlaceholder-test.js +++ b/src/compose/__tests__/getComposeInputPlaceholder-test.js @@ -3,7 +3,7 @@ import deepFreeze from 'deep-freeze'; import getComposeInputPlaceholder from '../getComposeInputPlaceholder'; import { - pmNarrowFromEmail, + pm1to1NarrowFromUser, streamNarrow, topicNarrow, pmNarrowFromUsersUnsafe, @@ -15,7 +15,7 @@ describe('getComposeInputPlaceholder', () => { const ownEmail = eg.selfUser.email; test('returns "Message @ThisPerson" object for person narrow', () => { - const narrow = deepFreeze(pmNarrowFromEmail(eg.otherUser.email)); + const narrow = deepFreeze(pm1to1NarrowFromUser(eg.otherUser)); const placeholder = getComposeInputPlaceholder(narrow, ownEmail, usersByEmail); expect(placeholder).toEqual({ text: 'Message {recipient}', @@ -24,7 +24,7 @@ describe('getComposeInputPlaceholder', () => { }); test('returns "Jot down something" object for self narrow', () => { - const narrow = deepFreeze(pmNarrowFromEmail(eg.selfUser.email)); + const narrow = deepFreeze(pm1to1NarrowFromUser(eg.selfUser)); const placeholder = getComposeInputPlaceholder(narrow, ownEmail, usersByEmail); expect(placeholder).toEqual({ text: 'Jot down something' }); }); diff --git a/src/notification/__tests__/notification-test.js b/src/notification/__tests__/notification-test.js index b0b4bb42020..6eb8b1c7e76 100644 --- a/src/notification/__tests__/notification-test.js +++ b/src/notification/__tests__/notification-test.js @@ -4,7 +4,7 @@ import deepFreeze from 'deep-freeze'; import type { UserOrBot } from '../../api/modelTypes'; import type { JSONableDict } from '../../utils/jsonable'; import { getNarrowFromNotificationData } from '..'; -import { topicNarrow, pmNarrowFromEmail, pmNarrowFromUsersUnsafe } from '../../utils/narrow'; +import { topicNarrow, pm1to1NarrowFromUser, pmNarrowFromUsersUnsafe } from '../../utils/narrow'; import * as eg from '../../__tests__/lib/exampleData'; import { fromAPNsImpl as extractIosNotificationData } from '../extract'; @@ -37,7 +37,7 @@ describe('getNarrowFromNotificationData', () => { sender_email: eg.otherUser.email, }; const narrow = getNarrowFromNotificationData(notification, DEFAULT_MAP, ownUserId); - expect(narrow).toEqual(pmNarrowFromEmail(eg.otherUser.email)); + expect(narrow).toEqual(pm1to1NarrowFromUser(eg.otherUser)); }); test('on notification for a group message returns a group narrow', () => { diff --git a/src/title/__tests__/titleSelectors-test.js b/src/title/__tests__/titleSelectors-test.js index c944bdaafce..fba7f1c2e8e 100644 --- a/src/title/__tests__/titleSelectors-test.js +++ b/src/title/__tests__/titleSelectors-test.js @@ -1,7 +1,7 @@ /* @flow strict-local */ import { DEFAULT_TITLE_BACKGROUND_COLOR, getTitleBackgroundColor } from '../titleSelectors'; -import { pmNarrowFromUsersUnsafe, streamNarrow, pmNarrowFromEmail } from '../../utils/narrow'; +import { pmNarrowFromUsersUnsafe, streamNarrow, pm1to1NarrowFromUser } from '../../utils/narrow'; import * as eg from '../../__tests__/lib/exampleData'; describe('getTitleBackgroundColor', () => { @@ -24,7 +24,7 @@ describe('getTitleBackgroundColor', () => { }); test('return default for non topic/stream narrow', () => { - expect(getTitleBackgroundColor(state, pmNarrowFromEmail(eg.otherUser.email))).toEqual( + expect(getTitleBackgroundColor(state, pm1to1NarrowFromUser(eg.otherUser))).toEqual( DEFAULT_TITLE_BACKGROUND_COLOR, ); expect( diff --git a/src/utils/__tests__/narrow-test.js b/src/utils/__tests__/narrow-test.js index 950181c71bf..a151d9ed740 100644 --- a/src/utils/__tests__/narrow-test.js +++ b/src/utils/__tests__/narrow-test.js @@ -3,7 +3,7 @@ import { HOME_NARROW, isHomeNarrow, - pmNarrowFromEmail, + pm1to1NarrowFromUser, is1to1PmNarrow, pmNarrowFromUsersUnsafe, isSpecialNarrow, @@ -22,7 +22,6 @@ import { parseNarrow, STARRED_NARROW, MENTIONED_NARROW, - pm1to1NarrowFromUser, keyFromNarrow, streamNameOfNarrow, topicOfNarrow, @@ -37,16 +36,16 @@ describe('HOME_NARROW', () => { }); }); -describe('pmNarrowFromEmail', () => { +describe('pm1to1NarrowFromUser', () => { test('produces a 1:1 narrow', () => { - const narrow = pmNarrowFromEmail(eg.otherUser.email); + const narrow = pm1to1NarrowFromUser(eg.otherUser); expect(is1to1PmNarrow(narrow)).toBeTrue(); expect(emailOfPm1to1Narrow(narrow)).toEqual(eg.otherUser.email); }); test('if operator is "pm-with" and only one email, then it is a private narrow', () => { expect(is1to1PmNarrow(HOME_NARROW)).toBe(false); - expect(is1to1PmNarrow(pmNarrowFromEmail(eg.otherUser.email))).toBe(true); + expect(is1to1PmNarrow(pm1to1NarrowFromUser(eg.otherUser))).toBe(true); }); }); @@ -54,7 +53,7 @@ describe('isPmNarrow', () => { test('a private or group narrow is any "pm-with" narrow', () => { expect(isPmNarrow(undefined)).toBe(false); expect(isPmNarrow(HOME_NARROW)).toBe(false); - expect(isPmNarrow(pmNarrowFromEmail(eg.otherUser.email))).toBe(true); + expect(isPmNarrow(pm1to1NarrowFromUser(eg.otherUser))).toBe(true); expect(isPmNarrow(pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser]))).toBe(true); }); }); @@ -65,7 +64,7 @@ describe('isStreamOrTopicNarrow', () => { expect(isStreamOrTopicNarrow(streamNarrow('some stream'))).toBe(true); expect(isStreamOrTopicNarrow(topicNarrow('some stream', 'some topic'))).toBe(true); expect(isStreamOrTopicNarrow(HOME_NARROW)).toBe(false); - expect(isStreamOrTopicNarrow(pmNarrowFromEmail(eg.otherUser.email))).toBe(false); + expect(isStreamOrTopicNarrow(pm1to1NarrowFromUser(eg.otherUser))).toBe(false); expect(isStreamOrTopicNarrow(pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser]))).toBe( false, ); @@ -141,7 +140,7 @@ describe('isMessageInNarrow', () => { ['PM', false, eg.pmMessage()], ]], - ['1:1 PM conversation, non-self', pmNarrowFromEmail(eg.otherUser.email), [ + ['1:1 PM conversation, non-self', pm1to1NarrowFromUser(eg.otherUser), [ ['matching PM, inbound', true, eg.pmMessage()], ['matching PM, outbound', true, eg.pmMessage({ sender: eg.selfUser })], ['self-1:1 message', false, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser] })], @@ -149,7 +148,7 @@ describe('isMessageInNarrow', () => { ['group-PM including this user, outbound', false, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser, eg.otherUser, eg.thirdUser] })], ['stream message', false, eg.streamMessage()], ]], - ['self-1:1 conversation', pmNarrowFromEmail(eg.selfUser.email), [ + ['self-1:1 conversation', pm1to1NarrowFromUser(eg.selfUser), [ ['self-1:1 message', true, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser] })], ['other 1:1 message, inbound', false, eg.pmMessage()], ['other 1:1 message, outbound', false, eg.pmMessage({ sender: eg.selfUser })], @@ -279,7 +278,7 @@ describe('getNarrowsForMessage', () => { { label: 'PM message with one person', message: eg.pmMessage({ sender: eg.otherUser }), - expectedNarrows: [HOME_NARROW, ALL_PRIVATE_NARROW, pmNarrowFromEmail(eg.otherUser.email)], + expectedNarrows: [HOME_NARROW, ALL_PRIVATE_NARROW, pm1to1NarrowFromUser(eg.otherUser)], }, { label: 'Group PM message', @@ -307,12 +306,12 @@ describe('getNarrowForReply', () => { eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser] }), eg.selfUser, ), - ).toEqual(pmNarrowFromEmail(eg.selfUser.email)); + ).toEqual(pm1to1NarrowFromUser(eg.selfUser)); }); test('for 1:1 PM, returns a 1:1 PM narrow', () => { const message = eg.pmMessage(); - const expectedNarrow = pmNarrowFromEmail(eg.otherUser.email); + const expectedNarrow = pm1to1NarrowFromUser(eg.otherUser); const actualNarrow = getNarrowForReply(message, eg.selfUser); From e9941c025e48da0f5953a02d3064e51e98ef1fc1 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Dec 2020 21:44:39 -0800 Subject: [PATCH 6/8] notif: Look up user promptly in server data, for opening 1:1 notif. We already do this for opening a notification for a group PM; now do the same for 1:1 PMs too. If we manage to have a notification where we don't know of a sender with that email -- which can happen if e.g. the sender has changed it since the notification was originally sent -- then this will cause us not to try to navigate to the conversation at all, which is an improvement over anything the previous behavior might have been: navigating to a conversation where we show something broken-looking, or crash. The motivation for this is so that we pass the Narrow constructor a user ID as well as an email, so that we can start storing user IDs in Narrow objects and start using them in all the code that consumes them. This is the last spot in our codebase where we're not doing so. Really we should use the sender ID from the server. That started in Zulip Server 1.8.0, according to the handy comments in FcmMessage.kt. We leave that be for now, though -- this email use is easy to find with a grep for `email`. --- .../__tests__/notification-test.js | 24 +++++++++++++++---- src/notification/index.js | 6 +++-- src/notification/notificationActions.js | 9 +++++-- 3 files changed, 30 insertions(+), 9 deletions(-) diff --git a/src/notification/__tests__/notification-test.js b/src/notification/__tests__/notification-test.js index 6eb8b1c7e76..1bc46de10f1 100644 --- a/src/notification/__tests__/notification-test.js +++ b/src/notification/__tests__/notification-test.js @@ -17,7 +17,7 @@ describe('getNarrowFromNotificationData', () => { test('unknown notification data returns null', () => { // $FlowFixMe: actually validate APNs messages const notification: Notification = {}; - const narrow = getNarrowFromNotificationData(notification, DEFAULT_MAP, ownUserId); + const narrow = getNarrowFromNotificationData(notification, DEFAULT_MAP, new Map(), ownUserId); expect(narrow).toBe(null); }); @@ -27,22 +27,31 @@ describe('getNarrowFromNotificationData', () => { stream: 'some stream', topic: 'some topic', }; - const narrow = getNarrowFromNotificationData(notification, DEFAULT_MAP, ownUserId); + const narrow = getNarrowFromNotificationData(notification, DEFAULT_MAP, new Map(), ownUserId); expect(narrow).toEqual(topicNarrow('some stream', 'some topic')); }); test('on notification for a private message returns a PM narrow', () => { + const users = [eg.selfUser, eg.otherUser]; + const allUsersById: Map = new Map(users.map(u => [u.user_id, u])); + const allUsersByEmail: Map = new Map(users.map(u => [u.email, u])); const notification = { recipient_type: 'private', sender_email: eg.otherUser.email, }; - const narrow = getNarrowFromNotificationData(notification, DEFAULT_MAP, ownUserId); + const narrow = getNarrowFromNotificationData( + notification, + allUsersById, + allUsersByEmail, + ownUserId, + ); expect(narrow).toEqual(pm1to1NarrowFromUser(eg.otherUser)); }); test('on notification for a group message returns a group narrow', () => { const users = [eg.selfUser, eg.makeUser(), eg.makeUser(), eg.makeUser()]; const allUsersById: Map = new Map(users.map(u => [u.user_id, u])); + const allUsersByEmail: Map = new Map(users.map(u => [u.email, u])); const notification = { recipient_type: 'private', @@ -51,7 +60,12 @@ describe('getNarrowFromNotificationData', () => { const expectedNarrow = pmNarrowFromUsersUnsafe(users.slice(1)); - const narrow = getNarrowFromNotificationData(notification, allUsersById, ownUserId); + const narrow = getNarrowFromNotificationData( + notification, + allUsersById, + allUsersByEmail, + ownUserId, + ); expect(narrow).toEqual(expectedNarrow); }); @@ -63,7 +77,7 @@ describe('getNarrowFromNotificationData', () => { }; const allUsersById = new Map(); - const narrow = getNarrowFromNotificationData(notification, allUsersById, ownUserId); + const narrow = getNarrowFromNotificationData(notification, allUsersById, new Map(), ownUserId); expect(narrow).toBe(null); }); diff --git a/src/notification/index.js b/src/notification/index.js index 03e0b6a7ad0..52300f52b23 100644 --- a/src/notification/index.js +++ b/src/notification/index.js @@ -4,7 +4,7 @@ import NotificationsIOS from 'react-native-notifications'; import type { Notification } from './types'; import type { Auth, Dispatch, Identity, Narrow, UserOrBot } from '../types'; -import { topicNarrow, pmNarrowFromEmail, pmNarrowFromUsers } from '../utils/narrow'; +import { topicNarrow, pmNarrowFromUsers, pm1to1NarrowFromUser } from '../utils/narrow'; import type { JSONable, JSONableDict } from '../utils/jsonable'; import * as api from '../api'; import * as logging from '../utils/logging'; @@ -86,6 +86,7 @@ export const getAccountFromNotificationData = ( export const getNarrowFromNotificationData = ( data: Notification, allUsersById: Map, + allUsersByEmail: Map, ownUserId: number, ): Narrow | null => { if (!data.recipient_type) { @@ -102,7 +103,8 @@ export const getNarrowFromNotificationData = ( } if (data.pm_users === undefined) { - return pmNarrowFromEmail(data.sender_email); + const user = allUsersByEmail.get(data.sender_email); + return (user && pm1to1NarrowFromUser(user)) ?? null; } const ids = data.pm_users.split(',').map(s => parseInt(s, 10)); diff --git a/src/notification/notificationActions.js b/src/notification/notificationActions.js index cdab584e6c9..8698d348164 100644 --- a/src/notification/notificationActions.js +++ b/src/notification/notificationActions.js @@ -14,7 +14,7 @@ import { getAuth, getActiveAccount } from '../selectors'; import { getSession, getAccounts } from '../directSelectors'; import { GOT_PUSH_TOKEN, ACK_PUSH_TOKEN, UNACK_PUSH_TOKEN } from '../actionConstants'; import { identityOfAccount, authOfAccount } from '../account/accountMisc'; -import { getAllUsersById, getOwnUserId } from '../users/userSelectors'; +import { getAllUsersByEmail, getAllUsersById, getOwnUserId } from '../users/userSelectors'; import { doNarrow } from '../message/messagesActions'; import { accountSwitch } from '../account/accountActions'; import { getIdentities } from '../account/accountsSelectors'; @@ -52,7 +52,12 @@ export const narrowToNotification = (data: ?Notification) => ( return; } - const narrow = getNarrowFromNotificationData(data, getAllUsersById(state), getOwnUserId(state)); + const narrow = getNarrowFromNotificationData( + data, + getAllUsersById(state), + getAllUsersByEmail(state), + getOwnUserId(state), + ); if (narrow) { dispatch(doNarrow(narrow)); } From b5e2d132730c91ecc5ce13213221701d751d5072 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Dec 2020 21:46:18 -0800 Subject: [PATCH 7/8] narrow [nfc]: Delete last email-only constructor, pmNarrowFromEmail. --- src/utils/narrow.js | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/utils/narrow.js b/src/utils/narrow.js index b96da9b1495..38a7a8482a4 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -62,15 +62,6 @@ export const HOME_NARROW_STR: string = keyFromNarrow(HOME_NARROW); */ const pmNarrowFromEmails = (emails: string[]): Narrow => Object.freeze({ type: 'pm', emails }); -/** - * DEPRECATED. Use `pm1to1NarrowFromUser` instead. - * - * This function is being replaced by `pm1to1NarrowFromUser`, as part of - * migrating from emails to user IDs to identify users. Don't add new uses - * of this function; use that one instead. - */ -export const pmNarrowFromEmail = (email: string): Narrow => pmNarrowFromEmails([email]); - /** * A PM narrow, either 1:1 or group. * From df6421463b3104b2682ca5d69b057d465f5893d1 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Dec 2020 21:57:04 -0800 Subject: [PATCH 8/8] narrow: Store user IDs for PM narrows! After all that prep work... this just works. Apart from a storage migration, only this module has to change, only in a few places -- and Flow pointed them all out. --- src/boot/store.js | 15 +++++++++++++++ src/utils/narrow.js | 43 ++++++++++++++++++++++++++++--------------- 2 files changed, 43 insertions(+), 15 deletions(-) diff --git a/src/boot/store.js b/src/boot/store.js index f18e5e450de..c944a886ebc 100644 --- a/src/boot/store.js +++ b/src/boot/store.js @@ -239,6 +239,21 @@ const migrations: { [string]: (GlobalState) => GlobalState } = { ), }), + // Change format of keys representing PM narrows, adding user IDs. + '20': state => ({ + ...dropCache(state), + drafts: objectFromEntries( + Object.keys(state.drafts) + // Just drop any old-style, email-only PM keys. Converting them + // would require using additional information to look up the IDs, + // which would make this more complex than any of our other + // migrations. Drafts are inherently short-term, and are already + // discarded whenever switching between accounts. + .filter(key => !key.startsWith('pm:s:')) + .map(key => [key, state.drafts[key]]), + ), + }), + // TIP: When adding a migration, consider just using `dropCache`. }; diff --git a/src/utils/narrow.js b/src/utils/narrow.js index 38a7a8482a4..03c6b8469eb 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -33,7 +33,7 @@ import { export opaque type Narrow = | {| type: 'stream', streamName: string |} | {| type: 'topic', streamName: string, topic: string |} - | {| type: 'pm', emails: $ReadOnlyArray |} + | {| type: 'pm', userIds: $ReadOnlyArray, emails: $ReadOnlyArray |} | {| type: 'search', query: string |} | {| type: 'all' | 'starred' | 'mentioned' | 'all-pm' |}; @@ -60,7 +60,8 @@ export const HOME_NARROW_STR: string = keyFromNarrow(HOME_NARROW); * accidentally disagreeing on whether to include the self-user, or on how * to sort the list (by user ID vs. email), or neglecting to sort it at all. */ -const pmNarrowFromEmails = (emails: string[]): Narrow => Object.freeze({ type: 'pm', emails }); +const pmNarrowInternal = (userIds: $ReadOnlyArray, emails: string[]): Narrow => + Object.freeze({ type: 'pm', userIds, emails }); /** * A PM narrow, either 1:1 or group. @@ -73,7 +74,7 @@ const pmNarrowFromEmails = (emails: string[]): Narrow => Object.freeze({ type: ' * different form of input. */ export const pmNarrowFromRecipients = (recipients: PmKeyRecipients): Narrow => - pmNarrowFromEmails(recipients.map(r => r.email)); + pmNarrowInternal(recipients.map(r => r.id), recipients.map(r => r.email)); /** * A PM narrow, either 1:1 or group. @@ -85,7 +86,7 @@ export const pmNarrowFromRecipients = (recipients: PmKeyRecipients): Narrow => * single specific user. */ export const pmNarrowFromUsers = (recipients: PmKeyUsers): Narrow => - pmNarrowFromEmails(recipients.map(r => r.email)); + pmNarrowInternal(recipients.map(r => r.user_id), recipients.map(r => r.email)); /** * FOR TESTS ONLY. Like pmNarrowFromUsers, but without validation. @@ -103,8 +104,10 @@ export const pmNarrowFromUsers = (recipients: PmKeyUsers): Narrow => */ // It'd be fine for test data to go through the usual filtering logic; the // annoying thing is just that that requires an ownUserId value. -export const pmNarrowFromUsersUnsafe = (recipients: UserOrBot[]): Narrow => - pmNarrowFromEmails(recipients.sort((a, b) => a.user_id - b.user_id).map(r => r.email)); +export const pmNarrowFromUsersUnsafe = (recipients: UserOrBot[]): Narrow => { + recipients.sort((a, b) => a.user_id - b.user_id); + return pmNarrowInternal(recipients.map(r => r.user_id), recipients.map(r => r.email)); +}; /** * A 1:1 PM narrow, possibly with self. @@ -113,7 +116,8 @@ export const pmNarrowFromUsersUnsafe = (recipients: UserOrBot[]): Narrow => * statically has just one other user it's a bit more convenient because it * doesn't require going through our `recipient` helpers. */ -export const pm1to1NarrowFromUser = (user: UserOrBot): Narrow => pmNarrowFromEmails([user.email]); +export const pm1to1NarrowFromUser = (user: UserOrBot): Narrow => + pmNarrowInternal([user.user_id], [user.email]); export const specialNarrow = (operand: string): Narrow => { if (operand === 'starred') { @@ -150,7 +154,7 @@ export const SEARCH_NARROW = (query: string): Narrow => Object.freeze({ type: 's type NarrowCases = {| home: () => T, - pm: (emails: $ReadOnlyArray) => T, + pm: (emails: $ReadOnlyArray, ids: $ReadOnlyArray) => T, starred: () => T, mentioned: () => T, allPrivate: () => T, @@ -168,7 +172,7 @@ export function caseNarrow(narrow: Narrow, cases: NarrowCases): T { switch (narrow.type) { case 'stream': return cases.stream(narrow.streamName); case 'topic': return cases.topic(narrow.streamName, narrow.topic); - case 'pm': return cases.pm(narrow.emails); + case 'pm': return cases.pm(narrow.emails, narrow.userIds); case 'search': return cases.search(narrow.query); case 'all': return cases.home(); case 'starred': return cases.starred(); @@ -237,8 +241,10 @@ export function caseNarrowDefault( // and (b) our use of `\x00` as a delimiter. Also perhaps email addresses, // if it's possible for those to have exciting characters in them. export function keyFromNarrow(narrow: Narrow): string { - // The ":s" bit in several of these is to keep them disjoint, out of an - // abundance of caution, from future keys that use numeric IDs. + // The ":s" (for "string") in several of these is to keep them disjoint, + // out of an abundance of caution, from future keys that use numeric IDs. + // + // Similarly, ":d" ("dual") marks those with both numeric IDs and strings. return caseNarrow(narrow, { // NB if you're changing any of these: be sure to do a migration. // Take a close look at migration 19 and any later related migrations. @@ -248,7 +254,8 @@ export function keyFromNarrow(narrow: Narrow): string { // (See `check_stream_name` in zulip.git:zerver/lib/streams.py.) topic: (streamName, topic) => `topic:s:${streamName}\x00${topic}`, - pm: emails => `pm:s:${emails.join(',')}`, + // An earlier version had `pm:s:`. + pm: (emails, ids) => `pm:d:${ids.join(',')}:${emails.join(',')}`, home: () => 'all', starred: () => 'starred', @@ -289,11 +296,17 @@ export const parseNarrow = (narrowStr: string): Narrow => { } case 'pm:': { - if (!rest.startsWith('s:')) { + // The `/s` regexp flag means the `.` patterns match absolutely + // anything. By default they reject certain "newline" characters, + // which might be impossible in email addresses but it's simplest + // not to have to care. + const match = /^d:(.*?):(.*)/s.exec(rest); + if (!match) { throw makeError(); } - const emails = rest.substr('s:'.length).split(','); - return pmNarrowFromEmails(emails); + const ids = match[1].split(',').map(s => Number.parseInt(s, 10)); + const emails = match[2].split(','); + return pmNarrowInternal(ids, emails); } case 'search:': {