From a04c8f2b9491a27f18deba13ed8bf88e671e730c Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 26 Jan 2022 19:31:32 -0800 Subject: [PATCH 01/24] lint: Disable no-control-regex When you need these, you need them. It doesn't seem helpful to have to add a local eslint-disable line; it's not like these look confusingly like something else. --- .eslintrc.yaml | 3 ++- src/utils/__tests__/narrow-test.js | 1 - src/utils/narrow.js | 1 - 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/.eslintrc.yaml b/.eslintrc.yaml index 21038810cb8..540aa2e506a 100644 --- a/.eslintrc.yaml +++ b/.eslintrc.yaml @@ -75,12 +75,13 @@ rules: # of Airbnb's settings where we don't want a full repeal. # - # Tricky code. + # Tricky-looking code. These are all sometimes perfectly legitimate. no-bitwise: off no-confusing-arrow: off no-continue: off no-plusplus: off no-nested-ternary: off + no-control-regex: off # We prefer `let foo = undefined;` over `let foo;`, not vice versa. # diff --git a/src/utils/__tests__/narrow-test.js b/src/utils/__tests__/narrow-test.js index 7664639f65f..a1fc8551087 100644 --- a/src/utils/__tests__/narrow-test.js +++ b/src/utils/__tests__/narrow-test.js @@ -348,7 +348,6 @@ describe('keyFromNarrow+parseNarrow', () => { // The only character not allowed in Zulip stream names is '\x00'. // (See `check_stream_name` in zulip.git:zerver/lib/streams.py.) // Try approximately everything else. - /* eslint-disable no-control-regex */ const diverseCharacters = eg.diverseCharacters.replace(/\x00/g, ''); const htmlEntities = 'h & t & &lquo;ml"'; const awkwardNarrows = [ diff --git a/src/utils/narrow.js b/src/utils/narrow.js index 22e405fb7b5..7429bb13b12 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -291,7 +291,6 @@ export const parseNarrow = (narrowStr: string): Narrow => { // The `/s` regexp flag means the `.` patterns match absolutely // anything. By default they reject certain "newline" characters, // which in principle could appear in stream names or topics. - // eslint-disable-next-line no-control-regex const match = /^s:(.*?)\x00(.*)/s.exec(rest); if (!match) { throw makeError(); From 9c063e9473974f8a50245cc2b5646bda11e7c1a6 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 15 Oct 2021 00:10:44 -0700 Subject: [PATCH 02/24] notif [nfc]: Say `stream_name` for a stream name, vs just `stream` Using snake_case to make it blend in with the other properties on this JSON blob, which come from the server. --- .../com/zulipmobile/notifications/FcmMessage.kt | 4 ++-- .../notifications/NotificationUiManager.kt | 4 ++-- .../zulipmobile/notifications/FcmMessageTest.kt | 2 +- src/notification/__tests__/notification-test.js | 15 ++++++++++----- src/notification/extract.js | 6 +++--- src/notification/index.js | 2 +- src/notification/types.js | 2 +- 7 files changed, 20 insertions(+), 15 deletions(-) diff --git a/android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt b/android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt index 95d67d2964a..7cde10d8f44 100644 --- a/android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt +++ b/android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt @@ -55,7 +55,7 @@ sealed class Recipient { } /** A stream message. */ - data class Stream(val stream: String, val topic: String) : Recipient() + data class Stream(val streamName: String, val topic: String) : Recipient() } /** @@ -116,7 +116,7 @@ data class MessageFcmMessage( when (recipient) { is Recipient.Stream -> { putString("recipient_type", "stream") - putString("stream", recipient.stream) + putString("stream_name", recipient.streamName) putString("topic", recipient.topic) } is Recipient.GroupPm -> { diff --git a/android/app/src/main/java/com/zulipmobile/notifications/NotificationUiManager.kt b/android/app/src/main/java/com/zulipmobile/notifications/NotificationUiManager.kt index 8edb55c2584..7b46520d911 100644 --- a/android/app/src/main/java/com/zulipmobile/notifications/NotificationUiManager.kt +++ b/android/app/src/main/java/com/zulipmobile/notifications/NotificationUiManager.kt @@ -259,7 +259,7 @@ private fun extractConversationKey(fcmMessage: MessageFcmMessage): String { // So long as this does use the stream name, we use `\u0000` as the delimiter because // it's the one character not allowed in Zulip stream names. // (See `check_stream_name` in zulip.git:zerver/lib/streams.py.) - is Recipient.Stream -> "stream:${fcmMessage.recipient.stream}\u0000${fcmMessage.recipient.topic}" + is Recipient.Stream -> "stream:${fcmMessage.recipient.streamName}\u0000${fcmMessage.recipient.topic}" is Recipient.GroupPm -> "groupPM:${fcmMessage.recipient.pmUsers.toString()}" is Recipient.Pm -> "private:${fcmMessage.sender.id}" } @@ -321,7 +321,7 @@ private fun updateNotification( // group-PM threads (pending #5116) get titled with the latest sender, rather than // the first. messagingStyle.setConversationTitle(when (fcmMessage.recipient) { - is Recipient.Stream -> "#${fcmMessage.recipient.stream} > ${fcmMessage.recipient.topic}" + is Recipient.Stream -> "#${fcmMessage.recipient.streamName} > ${fcmMessage.recipient.topic}" // TODO(#5116): use proper title for GroupPM, for which we will need // to have a way to get names of PM users here. is Recipient.GroupPm -> context.resources.getQuantityString( diff --git a/android/app/src/test/java/com/zulipmobile/notifications/FcmMessageTest.kt b/android/app/src/test/java/com/zulipmobile/notifications/FcmMessageTest.kt index a875843ef84..3efd95471d9 100644 --- a/android/app/src/test/java/com/zulipmobile/notifications/FcmMessageTest.kt +++ b/android/app/src/test/java/com/zulipmobile/notifications/FcmMessageTest.kt @@ -127,7 +127,7 @@ class MessageFcmMessageTest : FcmMessageTestBase() { ), zulipMessageId = 12345, recipient = Recipient.Stream( - stream = Example.stream["stream"]!!, + streamName = Example.stream["stream"]!!, topic = Example.stream["topic"]!! ), content = Example.stream["content"]!!, diff --git a/src/notification/__tests__/notification-test.js b/src/notification/__tests__/notification-test.js index 0b20be04d6a..5247997fae5 100644 --- a/src/notification/__tests__/notification-test.js +++ b/src/notification/__tests__/notification-test.js @@ -28,7 +28,7 @@ describe('getNarrowFromNotificationData', () => { const notification = { realm_uri, recipient_type: 'stream', - stream: 'some stream', + stream_name: 'some stream', topic: 'some topic', }; const narrow = getNarrowFromNotificationData(notification, new Map(), ownUserId); @@ -78,9 +78,14 @@ describe('extract iOS notification data', () => { for (const [type, data] of objectEntries(barebones)) { test(`${type} notification`, () => { + const expected = (() => { + const { stream: stream_name = undefined, ...rest } = data; + return stream_name !== undefined ? { ...rest, stream_name } : data; + })(); + // barebones 1.9.0-style message is accepted const msg = data; - expect(verify(msg)).toEqual(msg); + expect(verify(msg)).toEqual(expected); // new(-ish) optional user_id is accepted and copied // TODO: Rewrite so modern-style payloads are the baseline, e.g., @@ -88,15 +93,15 @@ describe('extract iOS notification data', () => { // individual tests for supporting older-style payloads, and mark // those for future deletion, like with `TODO(1.9.0)`. const msg1 = { ...msg, user_id }; - expect(verify(msg1)).toEqual(msg1); + expect(verify(msg1)).toEqual({ ...expected, user_id }); // unused fields are not copied const msg2 = { ...msg, realm_id: 8675309 }; - expect(verify(msg2)).toEqual(msg); + expect(verify(msg2)).toEqual(expected); // unknown fields are ignored and not copied const msg2a = { ...msg, unknown_data: ['unknown_data'] }; - expect(verify(msg2a)).toEqual(msg); + expect(verify(msg2a)).toEqual(expected); }); } }); diff --git a/src/notification/extract.js b/src/notification/extract.js index e109a19b1b6..3fa3dd0209d 100644 --- a/src/notification/extract.js +++ b/src/notification/extract.js @@ -125,14 +125,14 @@ export const fromAPNsImpl = (rawData: ?JSONableDict): Notification | void => { }; if (recipient_type === 'stream') { - const { stream, topic } = zulip; - if (typeof stream !== 'string' || typeof topic !== 'string') { + const { stream: stream_name, topic } = zulip; + if (typeof stream_name !== 'string' || typeof topic !== 'string') { throw err('invalid'); } return { ...identity, recipient_type: 'stream', - stream, + stream_name, topic, }; } else { diff --git a/src/notification/index.js b/src/notification/index.js index 8fde87d68cc..c99d8274c19 100644 --- a/src/notification/index.js +++ b/src/notification/index.js @@ -131,7 +131,7 @@ export const getNarrowFromNotificationData = ( } if (data.recipient_type === 'stream') { - return topicNarrow(data.stream, data.topic); + return topicNarrow(data.stream_name, data.topic); } if (data.pm_users === undefined) { diff --git a/src/notification/types.js b/src/notification/types.js index 52188d7c682..432084f59a1 100644 --- a/src/notification/types.js +++ b/src/notification/types.js @@ -19,7 +19,7 @@ type NotificationBase = {| */ // NOTE: Keep the Android-side code in sync with this type definition. export type Notification = - | {| ...NotificationBase, recipient_type: 'stream', stream: string, topic: string |} + | {| ...NotificationBase, recipient_type: 'stream', stream_name: string, topic: string |} // Group PM messages have `pm_users`, which is sorted, comma-separated IDs. | {| ...NotificationBase, recipient_type: 'private', pm_users: string |} // 1:1 PM messages lack `pm_users`. From b23c7b1968663c21c9fe40a4f578e02d3145ccdb Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 15 Oct 2021 00:19:45 -0700 Subject: [PATCH 03/24] notif: Find stream on opening a stream-message notification In a notification for a stream message, the server doesn't send us the stream's ID, only its name. We'll soon want to use stream IDs in our Narrow values, which means we'll need the stream's ID up front when the user opens such a notification. So, start looking up the stream in our data structures at this point. There's an open issue for the server to start sending stream IDs here: https://github.com/zulip/zulip/issues/18067 When it does, we can start looking for that and using it when present, but we'll want to keep this lookup as a fallback for a while after. This change causes getNarrowFromNotificationData to start returning null if the notification is for a stream we don't know about, where previously it would go ahead and return a narrow. The old behavior is that we'd navigate to that narrow, and it would show an error message "That conversation doesn't seem to exist" (via InvalidNarrow). The new behavior is that we just won't act on the notification at all. That's less good, but tolerable, and what we already do for 1:1 PMs; leave a TODO comment explaining it. --- .../__tests__/notification-test.js | 22 +++++++++--- src/notification/index.js | 34 +++++++++++++++++-- src/notification/notificationActions.js | 3 +- 3 files changed, 51 insertions(+), 8 deletions(-) diff --git a/src/notification/__tests__/notification-test.js b/src/notification/__tests__/notification-test.js index 5247997fae5..d505a11ae0a 100644 --- a/src/notification/__tests__/notification-test.js +++ b/src/notification/__tests__/notification-test.js @@ -20,19 +20,21 @@ describe('getNarrowFromNotificationData', () => { test('unknown notification data returns null', () => { // $FlowFixMe[incompatible-type]: actually validate APNs messages const notification: Notification = {}; - const narrow = getNarrowFromNotificationData(notification, new Map(), ownUserId); + const narrow = getNarrowFromNotificationData(notification, new Map(), new Map(), ownUserId); expect(narrow).toBe(null); }); test('recognizes stream notifications and returns topic narrow', () => { + const stream = eg.makeStream({ name: 'some stream' }); + const streamsByName = new Map([[stream.name, stream]]); const notification = { realm_uri, recipient_type: 'stream', stream_name: 'some stream', topic: 'some topic', }; - const narrow = getNarrowFromNotificationData(notification, new Map(), ownUserId); - expect(narrow).toEqual(topicNarrow('some stream', 'some topic')); + const narrow = getNarrowFromNotificationData(notification, new Map(), streamsByName, ownUserId); + expect(narrow).toEqual(topicNarrow(stream.name, 'some topic')); }); test('on notification for a private message returns a PM narrow', () => { @@ -43,7 +45,12 @@ describe('getNarrowFromNotificationData', () => { recipient_type: 'private', sender_email: eg.otherUser.email, }; - const narrow = getNarrowFromNotificationData(notification, allUsersByEmail, ownUserId); + const narrow = getNarrowFromNotificationData( + notification, + allUsersByEmail, + new Map(), + ownUserId, + ); expect(narrow).toEqual(pm1to1NarrowFromUser(eg.otherUser)); }); @@ -59,7 +66,12 @@ describe('getNarrowFromNotificationData', () => { const expectedNarrow = pmNarrowFromUsersUnsafe(users.slice(1)); - const narrow = getNarrowFromNotificationData(notification, allUsersByEmail, ownUserId); + const narrow = getNarrowFromNotificationData( + notification, + allUsersByEmail, + new Map(), + ownUserId, + ); expect(narrow).toEqual(expectedNarrow); }); diff --git a/src/notification/index.js b/src/notification/index.js index c99d8274c19..068a756f497 100644 --- a/src/notification/index.js +++ b/src/notification/index.js @@ -4,7 +4,16 @@ import PushNotificationIOS from '@react-native-community/push-notification-ios'; import type { PushNotificationEventName } from '@react-native-community/push-notification-ios'; import type { Notification } from './types'; -import type { Auth, Dispatch, GlobalDispatch, Account, Narrow, UserId, UserOrBot } from '../types'; +import type { + Auth, + Dispatch, + GlobalDispatch, + Account, + Narrow, + Stream, + UserId, + UserOrBot, +} from '../types'; import { topicNarrow, pm1to1NarrowFromUser, pmNarrowFromRecipients } from '../utils/narrow'; import type { JSONable, JSONableDict } from '../utils/jsonable'; import * as api from '../api'; @@ -119,6 +128,7 @@ export const getAccountFromNotificationData = ( export const getNarrowFromNotificationData = ( data: Notification, allUsersByEmail: Map, + streamsByName: Map, ownUserId: UserId, ): Narrow | null => { if (!data.recipient_type) { @@ -130,8 +140,28 @@ export const getNarrowFromNotificationData = ( return null; } + // TODO: If the notification is in an unknown stream, or a 1:1 PM from an + // unknown user, give a better error. (This can happen for the stream + // case if we were removed from the stream after the notification's + // message was sent. It can also happen if the user or stream was just + // created, and we haven't yet learned about it in the event queue; see + // e068771d7, which fixed this issue for group PMs.) + // + // This version just silently ignores the notification. + // + // A nicer version would navigate to ChatScreen for that unknown + // conversation, which would show InvalidNarrow (with its sensible error + // message) and whatever the notification did tell us about the + // stream/user: in particular, the stream name. + // + // But once Narrow objects stop relying on stream names (coming soon), + // doing that will require some alternate plumbing to pass the stream + // name through. For now, we skip dealing with that; this should be an + // uncommon case, so we settle for not crashing. + if (data.recipient_type === 'stream') { - return topicNarrow(data.stream_name, data.topic); + const stream = streamsByName.get(data.stream_name); + return (stream && topicNarrow(stream.name, data.topic)) ?? null; } if (data.pm_users === undefined) { diff --git a/src/notification/notificationActions.js b/src/notification/notificationActions.js index ac92b2258f5..aeb2168649e 100644 --- a/src/notification/notificationActions.js +++ b/src/notification/notificationActions.js @@ -19,7 +19,7 @@ import { getAccountFromNotificationData, } from '.'; import type { Notification } from './types'; -import { getAuth } from '../selectors'; +import { getAuth, getStreamsByName } from '../selectors'; import { getGlobalSession, getAccounts } from '../directSelectors'; import { GOT_PUSH_TOKEN, ACK_PUSH_TOKEN, UNACK_PUSH_TOKEN } from '../actionConstants'; import { identityOfAccount, authOfAccount } from '../account/accountMisc'; @@ -76,6 +76,7 @@ export const narrowToNotification = (data: ?Notification): GlobalThunkAction Date: Wed, 29 Dec 2021 15:57:50 -0800 Subject: [PATCH 04/24] drafts tests: Type-check draftsReducer-test.js Conveniently, this code turns out to already be well-typed. The type-checker will help us keep it that way as things change. --- src/drafts/__tests__/draftsReducer-test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/drafts/__tests__/draftsReducer-test.js b/src/drafts/__tests__/draftsReducer-test.js index 46d88e412b3..fac7ecf1ce4 100644 --- a/src/drafts/__tests__/draftsReducer-test.js +++ b/src/drafts/__tests__/draftsReducer-test.js @@ -1,3 +1,4 @@ +// @flow strict-local import deepFreeze from 'deep-freeze'; import { NULL_OBJECT } from '../../nullObjects'; From e9a7b515e60d25db8a8bba46eb359c3fab08e045 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 11 Jan 2022 21:22:27 -0800 Subject: [PATCH 05/24] stream [nfc]: Delete redundant, inefficient getSubscribedStreams This just takes each of the Subscription objects we have, and spreads the corresponding Stream object on top of them. But the way these objects work in the server API is that a Subscription object already contains all the properties of the corresponding Stream in the first place. So the end result should have exactly the same contents as the Subscription object we started from. When this selector was introduced in 27483c9a8 back in 2017, the motivation was apparently "because streams change events are updated in streams" -- i.e., because we were *not* keeping the Subscription objects up to date when things changed on the stream. But in that case the real bug was that we were keeping around an object with out-of-date data; having only an alternative wrapper off to the side where one can get an up-to-date version is a recipe for an ongoing stream of bugs where we use the version that's in the obvious place, not realizing it can be stale. Fortunately that did get corrected the next year, in 152b06e25. Since then, the Subscription objects are just as up to date as the Stream objects, and this selector has no reason to exist. Drop it, and have its callers get the subscription data directly. --- src/autocomplete/StreamAutocomplete.js | 4 +-- src/streams/SubscriptionsCard.js | 4 +-- .../__tests__/subscriptionSelectors-test.js | 28 ------------------- src/subscriptions/subscriptionSelectors.js | 10 ------- 4 files changed, 4 insertions(+), 42 deletions(-) diff --git a/src/autocomplete/StreamAutocomplete.js b/src/autocomplete/StreamAutocomplete.js index 46035682826..066a4a99509 100644 --- a/src/autocomplete/StreamAutocomplete.js +++ b/src/autocomplete/StreamAutocomplete.js @@ -6,7 +6,7 @@ import { FlatList } from 'react-native'; import { useSelector } from '../react-redux'; import { Popup } from '../common'; -import { getSubscribedStreams } from '../subscriptions/subscriptionSelectors'; +import { getSubscriptions } from '../directSelectors'; import StreamItem from '../streams/StreamItem'; type Props = $ReadOnly<{| @@ -16,7 +16,7 @@ type Props = $ReadOnly<{| export default function StreamAutocomplete(props: Props): Node { const { filter, onAutocomplete } = props; - const subscriptions = useSelector(getSubscribedStreams); + const subscriptions = useSelector(getSubscriptions); const handleStreamItemAutocomplete = useCallback( (streamId: number, streamName: string): void => { diff --git a/src/streams/SubscriptionsCard.js b/src/streams/SubscriptionsCard.js index af652523c70..4b3f05949c9 100644 --- a/src/streams/SubscriptionsCard.js +++ b/src/streams/SubscriptionsCard.js @@ -12,7 +12,7 @@ import StreamList from './StreamList'; import { LoadingBanner } from '../common'; import { streamNarrow } from '../utils/narrow'; import { getUnreadByStream } from '../selectors'; -import { getSubscribedStreams } from '../subscriptions/subscriptionSelectors'; +import { getSubscriptions } from '../directSelectors'; import { doNarrow } from '../actions'; const styles = createStyleSheet({ @@ -29,7 +29,7 @@ type Props = $ReadOnly<{| export default function SubscriptionsCard(props: Props): Node { const dispatch = useDispatch(); - const subscriptions = useSelector(getSubscribedStreams); + const subscriptions = useSelector(getSubscriptions); const unreadByStream = useSelector(getUnreadByStream); const handleNarrow = useCallback( diff --git a/src/subscriptions/__tests__/subscriptionSelectors-test.js b/src/subscriptions/__tests__/subscriptionSelectors-test.js index abdd4f73194..89f79827b30 100644 --- a/src/subscriptions/__tests__/subscriptionSelectors-test.js +++ b/src/subscriptions/__tests__/subscriptionSelectors-test.js @@ -4,7 +4,6 @@ import { getStreamsById, getSubscriptionsById, getIsActiveStreamSubscribed, - getSubscribedStreams, getStreamColorForNarrow, } from '../subscriptionSelectors'; import { @@ -100,33 +99,6 @@ describe('getIsActiveStreamSubscribed', () => { }); }); -describe('getSubscribedStreams', () => { - test('get all subscribed streams', () => { - const state = deepFreeze({ - streams: [ - { stream_id: 1, name: 'all', description: 'stream for all' }, - { stream_id: 2, name: 'new announce', description: 'stream for announce' }, - { stream_id: 3, name: 'Denmark', description: 'Denmark is awesome' }, - { stream_id: 4, name: 'general', description: 'stream for general' }, - ], - subscriptions: [ - { stream_id: 1, name: 'all', color: '#001' }, - { stream_id: 2, name: 'announce', color: '#002' }, - { stream_id: 4, name: 'general', color: '#003' }, - ], - }); - - const expectedResult = [ - { stream_id: 1, name: 'all', color: '#001', description: 'stream for all' }, - { stream_id: 2, name: 'new announce', color: '#002', description: 'stream for announce' }, - { stream_id: 4, name: 'general', color: '#003', description: 'stream for general' }, - ]; - - const actualResult = getSubscribedStreams(state); - expect(actualResult).toEqual(expectedResult); - }); -}); - describe('getStreamColorForNarrow', () => { const exampleColor = '#fff'; const state = eg.reduxState({ diff --git a/src/subscriptions/subscriptionSelectors.js b/src/subscriptions/subscriptionSelectors.js index 2cebeaf49a1..d7942dbf280 100644 --- a/src/subscriptions/subscriptionSelectors.js +++ b/src/subscriptions/subscriptionSelectors.js @@ -50,16 +50,6 @@ export const getIsActiveStreamSubscribed: Selector = createSele }, ); -export const getSubscribedStreams: Selector<$ReadOnlyArray> = createSelector( - getStreams, - getSubscriptions, - (allStreams, allSubscriptions) => - allSubscriptions.map(subscription => ({ - ...subscription, - ...allStreams.find(stream => stream.stream_id === subscription.stream_id), - })), -); - /** * The stream with the given ID; throws if none. * From e13c879789d35df9e82f59ddfc7d914570f7139d Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 29 Dec 2021 16:12:01 -0800 Subject: [PATCH 06/24] subscriptions tests: Well-type, using exampleData --- src/__tests__/lib/exampleData.js | 11 ++- .../__tests__/subscriptionSelectors-test.js | 95 ++++++++----------- 2 files changed, 46 insertions(+), 60 deletions(-) diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index 4417800d95d..97a4a2ee472 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -269,6 +269,10 @@ export const stream: Stream = makeStream({ name: 'a stream', description: 'An example stream.', }); +export const otherStream: Stream = makeStream({ + name: 'another stream', + description: 'Another example stream.', +}); /** A subscription, by default to eg.stream. */ export const makeSubscription = ( @@ -292,6 +296,9 @@ export const makeSubscription = ( /** A subscription to eg.stream. */ export const subscription: Subscription = makeSubscription(); +/** A subscription to eg.otherStream. */ +export const otherSubscription: Subscription = makeSubscription({ stream: otherStream }); + /* ======================================================================== * Messages */ @@ -572,8 +579,8 @@ export const plusReduxState: GlobalState & PerAccountState = reduxState({ realm: { ...baseReduxState.realm, user_id: selfUser.user_id, email: selfUser.email }, // TODO add crossRealmBot users: [selfUser, otherUser, thirdUser], - streams: [stream], - subscriptions: [subscription], + streams: [stream, otherStream], + subscriptions: [subscription, otherSubscription], }); /** diff --git a/src/subscriptions/__tests__/subscriptionSelectors-test.js b/src/subscriptions/__tests__/subscriptionSelectors-test.js index 89f79827b30..dd57ebd8530 100644 --- a/src/subscriptions/__tests__/subscriptionSelectors-test.js +++ b/src/subscriptions/__tests__/subscriptionSelectors-test.js @@ -1,4 +1,4 @@ -import deepFreeze from 'deep-freeze'; +// @flow strict-local import { getStreamsById, @@ -16,86 +16,64 @@ import { import * as eg from '../../__tests__/lib/exampleData'; describe('getStreamsById', () => { - test('returns empty object for an empty input', () => { - const state = deepFreeze({ - streams: [], - }); + test('returns empty map for an empty input', () => { + const state = eg.reduxState({ streams: [] }); expect(getStreamsById(state)).toEqual(new Map()); }); - test('returns an object with stream id as keys', () => { - const state = deepFreeze({ - streams: [{ stream_id: 1 }, { stream_id: 2 }], - }); - - // prettier-ignore - const expectedState = new Map([[1, { stream_id: 1 }], [2, { stream_id: 2 }]]); - - const streamsById = getStreamsById(state); - - expect(streamsById).toEqual(expectedState); + test('returns a map with stream id as keys', () => { + const state = eg.reduxState({ streams: [eg.stream, eg.otherStream] }); + expect(getStreamsById(state)).toEqual( + new Map([ + [eg.stream.stream_id, eg.stream], + [eg.otherStream.stream_id, eg.otherStream], + ]), + ); }); }); describe('getSubscriptionsById', () => { - test('returns empty object for an empty input', () => { - const state = deepFreeze({ - subscriptions: [], - }); + test('returns empty map for an empty input', () => { + const state = eg.reduxState({ subscriptions: [] }); expect(getSubscriptionsById(state)).toEqual(new Map()); }); - test('returns an object with stream id as keys', () => { - const state = deepFreeze({ - subscriptions: [{ stream_id: 1 }, { stream_id: 2 }], - }); - - // prettier-ignore - const expectedState = new Map([[1, { stream_id: 1 }], [2, { stream_id: 2 }]]); - - const subscriptionsById = getSubscriptionsById(state); - - expect(subscriptionsById).toEqual(expectedState); + test('returns a map with stream id as keys', () => { + const state = eg.reduxState({ subscriptions: [eg.subscription, eg.otherSubscription] }); + expect(getSubscriptionsById(state)).toEqual( + new Map([ + [eg.subscription.stream_id, eg.subscription], + [eg.otherSubscription.stream_id, eg.otherSubscription], + ]), + ); }); }); describe('getIsActiveStreamSubscribed', () => { - test('return true for narrows other than stream and topic', () => { - const state = deepFreeze({}); + const state = eg.reduxStatePlus({ subscriptions: [eg.subscription] }); + test('return true for narrows other than stream and topic', () => { expect(getIsActiveStreamSubscribed(state, HOME_NARROW)).toBe(true); }); test('return true if current narrowed stream is subscribed', () => { - const state = deepFreeze({ - subscriptions: [{ name: 'announce' }], - }); - - expect(getIsActiveStreamSubscribed(state, streamNarrow('announce'))).toBe(true); + const narrow = streamNarrow(eg.stream.name); + expect(getIsActiveStreamSubscribed(state, narrow)).toBe(true); }); test('return false if current narrowed stream is not subscribed', () => { - const state = deepFreeze({ - subscriptions: [{ name: 'announce' }], - }); - - expect(getIsActiveStreamSubscribed(state, streamNarrow('all'))).toBe(false); + const narrow = streamNarrow(eg.otherStream.name); + expect(getIsActiveStreamSubscribed(state, narrow)).toBe(false); }); test('return true if stream of current narrowed topic is subscribed', () => { - const state = deepFreeze({ - subscriptions: [{ name: 'announce' }], - }); - - expect(getIsActiveStreamSubscribed(state, topicNarrow('announce', 'news'))).toBe(true); + const narrow = topicNarrow(eg.stream.name, 'news'); + expect(getIsActiveStreamSubscribed(state, narrow)).toBe(true); }); test('return false if stream of current narrowed topic is not subscribed', () => { - const state = deepFreeze({ - subscriptions: [{ name: 'announce' }], - }); - - expect(getIsActiveStreamSubscribed(state, topicNarrow('all', 'news'))).toBe(false); + const narrow = topicNarrow(eg.otherStream.name, 'news'); + expect(getIsActiveStreamSubscribed(state, narrow)).toBe(false); }); }); @@ -106,18 +84,19 @@ describe('getStreamColorForNarrow', () => { }); test('return stream color for stream and topic narrow', () => { - expect(getStreamColorForNarrow(state, streamNarrow(eg.stream.name))).toEqual(exampleColor); + const narrow = streamNarrow(eg.stream.name); + expect(getStreamColorForNarrow(state, narrow)).toEqual(exampleColor); }); test('return null stream color for invalid stream or unknown subscriptions', () => { const unknownStream = eg.makeStream(); - expect(getStreamColorForNarrow(state, streamNarrow(unknownStream.name))).toEqual('gray'); + const narrow = streamNarrow(unknownStream.name); + expect(getStreamColorForNarrow(state, narrow)).toEqual('gray'); }); test('return undefined for non topic/stream narrow', () => { expect(getStreamColorForNarrow(state, pm1to1NarrowFromUser(eg.otherUser))).toEqual(undefined); - expect( - getStreamColorForNarrow(state, pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser])), - ).toEqual(undefined); + const narrow = pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser]); + expect(getStreamColorForNarrow(state, narrow)).toEqual(undefined); }); }); From 298ac7149d8eae33e72dd25e04b48f8bf13d9c0d Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 29 Dec 2021 13:00:26 -0800 Subject: [PATCH 07/24] tests [nfc]: Use more whole streams rather than names Plus some trivial other refactors. Together these will help make the diff where the Narrow constructors start taking stream IDs along with names, coming soon, be as boring and obvious as possible in most of its length. --- src/chat/__tests__/fetchingReducer-test.js | 5 +++-- src/chat/__tests__/narrowsSelectors-test.js | 2 +- .../__tests__/getComposeInputPlaceholder-test.js | 9 ++++++--- src/drafts/__tests__/draftsReducer-test.js | 5 +++-- src/message/__tests__/scrollStrategy-test.js | 3 ++- src/utils/__tests__/narrow-test.js | 10 ++++++---- 6 files changed, 21 insertions(+), 13 deletions(-) diff --git a/src/chat/__tests__/fetchingReducer-test.js b/src/chat/__tests__/fetchingReducer-test.js index 85294e84f73..7271362d5d9 100644 --- a/src/chat/__tests__/fetchingReducer-test.js +++ b/src/chat/__tests__/fetchingReducer-test.js @@ -41,16 +41,17 @@ describe('fetchingReducer', () => { [HOME_NARROW_STR]: { older: false, newer: false }, }); + const narrow = streamNarrow(eg.stream.name); const action = deepFreeze({ type: MESSAGE_FETCH_START, - narrow: streamNarrow('some stream'), + narrow, numBefore: 10, numAfter: 0, }); const expectedState = { [HOME_NARROW_STR]: { older: false, newer: false }, - [keyFromNarrow(streamNarrow('some stream'))]: { older: true, newer: false }, + [keyFromNarrow(narrow)]: { older: true, newer: false }, }; const newState = fetchingReducer(initialState, action); diff --git a/src/chat/__tests__/narrowsSelectors-test.js b/src/chat/__tests__/narrowsSelectors-test.js index ce7664dc2a3..3b6418d509f 100644 --- a/src/chat/__tests__/narrowsSelectors-test.js +++ b/src/chat/__tests__/narrowsSelectors-test.js @@ -356,7 +356,7 @@ describe('isNarrowValid', () => { const state = eg.reduxState({ streams: [], }); - const narrow = streamNarrow('nonexisting'); + const narrow = streamNarrow(eg.stream.name); const result = isNarrowValid(state, narrow); diff --git a/src/compose/__tests__/getComposeInputPlaceholder-test.js b/src/compose/__tests__/getComposeInputPlaceholder-test.js index a661fa377f6..be7fa23b0df 100644 --- a/src/compose/__tests__/getComposeInputPlaceholder-test.js +++ b/src/compose/__tests__/getComposeInputPlaceholder-test.js @@ -30,13 +30,16 @@ describe('getComposeInputPlaceholder', () => { }); test('returns "Message #streamName" for stream narrow', () => { - const narrow = deepFreeze(streamNarrow('Denmark')); + const narrow = deepFreeze(streamNarrow(eg.stream.name)); const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById); - expect(placeholder).toEqual({ text: 'Message {recipient}', values: { recipient: '#Denmark' } }); + expect(placeholder).toEqual({ + text: 'Message {recipient}', + values: { recipient: `#${eg.stream.name}` }, + }); }); test('returns properly for topic narrow', () => { - const narrow = deepFreeze(topicNarrow('Denmark', 'Copenhagen')); + const narrow = deepFreeze(topicNarrow(eg.stream.name, 'Copenhagen')); const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById); expect(placeholder).toEqual({ text: 'Reply' }); }); diff --git a/src/drafts/__tests__/draftsReducer-test.js b/src/drafts/__tests__/draftsReducer-test.js index fac7ecf1ce4..b0b361370a9 100644 --- a/src/drafts/__tests__/draftsReducer-test.js +++ b/src/drafts/__tests__/draftsReducer-test.js @@ -5,9 +5,10 @@ import { NULL_OBJECT } from '../../nullObjects'; import draftsReducer from '../draftsReducer'; import { DRAFT_UPDATE } from '../../actionConstants'; import { topicNarrow, keyFromNarrow } from '../../utils/narrow'; +import * as eg from '../../__tests__/lib/exampleData'; describe('draftsReducer', () => { - const testNarrow = topicNarrow('denmark', 'denmark2'); + const testNarrow = topicNarrow(eg.stream.name, 'denmark2'); const testNarrowStr = keyFromNarrow(testNarrow); describe('DRAFT_UPDATE', () => { @@ -71,7 +72,7 @@ describe('draftsReducer', () => { const action = { type: DRAFT_UPDATE, content: ' ', - narrow: topicNarrow('denmark', 'denmark2'), + narrow: testNarrow, }; const expectedState = {}; diff --git a/src/message/__tests__/scrollStrategy-test.js b/src/message/__tests__/scrollStrategy-test.js index 0313ad8d6b1..5a95f6565d7 100644 --- a/src/message/__tests__/scrollStrategy-test.js +++ b/src/message/__tests__/scrollStrategy-test.js @@ -5,7 +5,8 @@ import { getScrollStrategy } from '../scrollStrategy'; import * as eg from '../../__tests__/lib/exampleData'; const someNarrow = streamNarrow(eg.stream.name); -const anotherNarrow = streamNarrow(eg.makeStream().name); +const anotherStream = eg.makeStream(); +const anotherNarrow = streamNarrow(anotherStream.name); describe('getScrollStrategy', () => { test('initial load positions at anchor (first unread)', () => { diff --git a/src/utils/__tests__/narrow-test.js b/src/utils/__tests__/narrow-test.js index a1fc8551087..f43e30cf0c1 100644 --- a/src/utils/__tests__/narrow-test.js +++ b/src/utils/__tests__/narrow-test.js @@ -350,16 +350,18 @@ describe('keyFromNarrow+parseNarrow', () => { // Try approximately everything else. const diverseCharacters = eg.diverseCharacters.replace(/\x00/g, ''); const htmlEntities = 'h & t & &lquo;ml"'; + const diverseCharactersStream = eg.makeStream({ name: diverseCharacters }); + const htmlEntitiesStream = eg.makeStream({ name: htmlEntities }); const awkwardNarrows = [ - ['whole stream about awkward characters', streamNarrow(diverseCharacters)], - ['whole stream about HTML entities', streamNarrow(htmlEntities)], + ['whole stream about awkward characters', streamNarrow(diverseCharactersStream.name)], + ['whole stream about HTML entities', streamNarrow(htmlEntitiesStream.name)], [ 'stream conversation about awkward characters', - topicNarrow(diverseCharacters, `regarding ${diverseCharacters}`), + topicNarrow(diverseCharactersStream.name, `regarding ${diverseCharacters}`), ], [ 'stream conversation about HTML entities', - topicNarrow(htmlEntities, `regarding ${htmlEntities}`), + topicNarrow(htmlEntitiesStream.name, `regarding ${htmlEntities}`), ], ['search narrow for awkward characters', SEARCH_NARROW(diverseCharacters)], ['search narrow for HTML entities', SEARCH_NARROW(htmlEntities)], From 3ea6473ea41656982be5b938ab33015d58c972b5 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 29 Dec 2021 13:09:54 -0800 Subject: [PATCH 08/24] narrow [nfc]: Accept optional stream ID in Narrow constructors This will let us make the boring mass change of starting to pass these stream IDs, separately from the more interesting changes involved in actually storing and using them. --- src/utils/narrow.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/utils/narrow.js b/src/utils/narrow.js index 7429bb13b12..143606ac809 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -148,11 +148,12 @@ export const ALL_PRIVATE_NARROW: Narrow = specialNarrow('private'); export const ALL_PRIVATE_NARROW_STR: string = keyFromNarrow(ALL_PRIVATE_NARROW); -export const streamNarrow = (stream: string): Narrow => - Object.freeze({ type: 'stream', streamName: stream }); +export const streamNarrow = (streamName: string, streamId?: number): Narrow => + Object.freeze({ type: 'stream', streamName }); -export const topicNarrow = (stream: string, topic: string): Narrow => - Object.freeze({ type: 'topic', streamName: stream, topic }); +export const topicNarrow = (streamName: string, ...args: [string] | [number, string]): Narrow => + // $FlowFixMe[incompatible-cast] + Object.freeze({ type: 'topic', streamName, topic: (args[args.length - 1]: string) }); export const SEARCH_NARROW = (query: string): Narrow => Object.freeze({ type: 'search', query }); From ba6d08188f172a37a1c692cba73d080043df5bea Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 14 Oct 2021 00:35:46 -0700 Subject: [PATCH 09/24] narrow [nfc]: Pass stream IDs to Narrow constructors, almost everywhere This is all made possible by the preceding series of changes where we parsed, tracked, or otherwise made stream IDs available in a variety of places where we'd previously had only a stream name. This leaves just a handful of call sites not passing IDs -- in particular, where the stream we want to refer to comes itself from some Narrow value. Those we'll take care of together with actually storing the stream ID in the Narrow. --- src/chat/__tests__/fetchingReducer-test.js | 2 +- src/chat/__tests__/narrowsReducer-test.js | 4 +- src/chat/__tests__/narrowsSelectors-test.js | 20 ++++--- .../getComposeInputPlaceholder-test.js | 4 +- src/drafts/__tests__/draftsReducer-test.js | 2 +- src/drafts/__tests__/draftsSelectors-test.js | 9 ++- src/message/__tests__/fetchActions-test.js | 2 +- src/message/__tests__/messageActions-test.js | 2 +- src/message/__tests__/scrollStrategy-test.js | 4 +- src/nav/ChatNavBar.js | 2 +- .../__tests__/notification-test.js | 2 +- src/notification/index.js | 2 +- src/sharing/ShareToStream.js | 4 +- src/sharing/ShareWrapper.js | 4 +- src/streams/SubscriptionsCard.js | 2 +- src/subscriptions/StreamListCard.js | 2 +- .../__tests__/subscriptionSelectors-test.js | 12 ++-- src/topics/TopicListScreen.js | 2 +- src/topics/__tests__/topicsSelectors-test.js | 2 +- src/unread/UnreadCards.js | 4 +- src/utils/__tests__/internalLinks-test.js | 32 +++++----- src/utils/__tests__/narrow-test.js | 60 +++++++++++++------ src/utils/internalLinks.js | 4 +- src/utils/narrow.js | 6 +- .../__tests__/messageListElementHtml-test.js | 4 +- src/webview/html/header.js | 6 +- 26 files changed, 116 insertions(+), 83 deletions(-) diff --git a/src/chat/__tests__/fetchingReducer-test.js b/src/chat/__tests__/fetchingReducer-test.js index 7271362d5d9..c33a2059955 100644 --- a/src/chat/__tests__/fetchingReducer-test.js +++ b/src/chat/__tests__/fetchingReducer-test.js @@ -41,7 +41,7 @@ describe('fetchingReducer', () => { [HOME_NARROW_STR]: { older: false, newer: false }, }); - const narrow = streamNarrow(eg.stream.name); + const narrow = streamNarrow(eg.stream.name, eg.stream.stream_id); const action = deepFreeze({ type: MESSAGE_FETCH_START, narrow, diff --git a/src/chat/__tests__/narrowsReducer-test.js b/src/chat/__tests__/narrowsReducer-test.js index 5462b972db1..8ba6af89949 100644 --- a/src/chat/__tests__/narrowsReducer-test.js +++ b/src/chat/__tests__/narrowsReducer-test.js @@ -27,9 +27,9 @@ import * as eg from '../../__tests__/lib/exampleData'; describe('narrowsReducer', () => { const privateNarrowStr = keyFromNarrow(pm1to1NarrowFromUser(eg.otherUser)); const groupNarrowStr = keyFromNarrow(pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser])); - const streamNarrowStr = keyFromNarrow(streamNarrow(eg.stream.name)); + const streamNarrowStr = keyFromNarrow(streamNarrow(eg.stream.name, eg.stream.stream_id)); const egTopic = eg.streamMessage().subject; - const topicNarrowStr = keyFromNarrow(topicNarrow(eg.stream.name, egTopic)); + const topicNarrowStr = keyFromNarrow(topicNarrow(eg.stream.name, eg.stream.stream_id, egTopic)); describe('EVENT_NEW_MESSAGE', () => { test('if not caught up in narrow, do not add message in home narrow', () => { diff --git a/src/chat/__tests__/narrowsSelectors-test.js b/src/chat/__tests__/narrowsSelectors-test.js index 3b6418d509f..fdf9a503543 100644 --- a/src/chat/__tests__/narrowsSelectors-test.js +++ b/src/chat/__tests__/narrowsSelectors-test.js @@ -146,7 +146,7 @@ describe('getShownMessagesForNarrow', () => { }); describe('stream narrow', () => { - const narrow = streamNarrow(stream.name); + const narrow = streamNarrow(stream.name, stream.stream_id); const makeState = extra => makeStateGeneral(message, narrow, extra); const shown = state => shownGeneral(state, narrow); @@ -171,7 +171,7 @@ describe('getShownMessagesForNarrow', () => { }); describe('topic narrow', () => { - const narrow = topicNarrow(stream.name, message.subject); + const narrow = topicNarrow(stream.name, stream.stream_id, message.subject); const makeState = extra => makeStateGeneral(message, narrow, extra); const shown = state => shownGeneral(state, narrow); @@ -306,26 +306,28 @@ describe('getStreamInNarrow', () => { }); test('return subscription if stream in narrow is subscribed', () => { - const narrow = streamNarrow(stream1.name); + const narrow = streamNarrow(stream1.name, stream1.stream_id); expect(getStreamInNarrow(state, narrow)).toEqual(sub1); }); test('return stream if stream in narrow is not subscribed', () => { - const narrow = streamNarrow(stream3.name); + const narrow = streamNarrow(stream3.name, stream3.stream_id); expect(getStreamInNarrow(state, narrow)).toEqual({ ...stream3, in_home_view: true }); }); test('return NULL_SUBSCRIPTION if stream in narrow is not valid', () => { - const narrow = streamNarrow(stream4.name); + const narrow = streamNarrow(stream4.name, stream4.stream_id); expect(getStreamInNarrow(state, narrow)).toEqual(NULL_SUBSCRIPTION); }); test('return NULL_SUBSCRIPTION is narrow is not topic or stream', () => { expect(getStreamInNarrow(state, pm1to1NarrowFromUser(eg.otherUser))).toEqual(NULL_SUBSCRIPTION); - expect(getStreamInNarrow(state, topicNarrow(stream4.name, 'topic'))).toEqual(NULL_SUBSCRIPTION); + expect(getStreamInNarrow(state, topicNarrow(stream4.name, stream4.stream_id, 'topic'))).toEqual( + NULL_SUBSCRIPTION, + ); }); }); @@ -345,7 +347,7 @@ describe('isNarrowValid', () => { const state = eg.reduxState({ streams: [stream], }); - const narrow = streamNarrow(stream.name); + const narrow = streamNarrow(stream.name, stream.stream_id); const result = isNarrowValid(state, narrow); @@ -356,7 +358,7 @@ describe('isNarrowValid', () => { const state = eg.reduxState({ streams: [], }); - const narrow = streamNarrow(eg.stream.name); + const narrow = streamNarrow(eg.stream.name, eg.stream.stream_id); const result = isNarrowValid(state, narrow); @@ -369,7 +371,7 @@ describe('isNarrowValid', () => { const state = eg.reduxState({ streams: [stream], }); - const narrow = topicNarrow(stream.name, 'topic does not matter'); + const narrow = topicNarrow(stream.name, stream.stream_id, 'topic does not matter'); const result = isNarrowValid(state, narrow); diff --git a/src/compose/__tests__/getComposeInputPlaceholder-test.js b/src/compose/__tests__/getComposeInputPlaceholder-test.js index be7fa23b0df..8edf0fb1f61 100644 --- a/src/compose/__tests__/getComposeInputPlaceholder-test.js +++ b/src/compose/__tests__/getComposeInputPlaceholder-test.js @@ -30,7 +30,7 @@ describe('getComposeInputPlaceholder', () => { }); test('returns "Message #streamName" for stream narrow', () => { - const narrow = deepFreeze(streamNarrow(eg.stream.name)); + const narrow = deepFreeze(streamNarrow(eg.stream.name, eg.stream.stream_id)); const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById); expect(placeholder).toEqual({ text: 'Message {recipient}', @@ -39,7 +39,7 @@ describe('getComposeInputPlaceholder', () => { }); test('returns properly for topic narrow', () => { - const narrow = deepFreeze(topicNarrow(eg.stream.name, 'Copenhagen')); + const narrow = deepFreeze(topicNarrow(eg.stream.name, eg.stream.stream_id, 'Copenhagen')); const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById); expect(placeholder).toEqual({ text: 'Reply' }); }); diff --git a/src/drafts/__tests__/draftsReducer-test.js b/src/drafts/__tests__/draftsReducer-test.js index b0b361370a9..d01dd8ebed2 100644 --- a/src/drafts/__tests__/draftsReducer-test.js +++ b/src/drafts/__tests__/draftsReducer-test.js @@ -8,7 +8,7 @@ import { topicNarrow, keyFromNarrow } from '../../utils/narrow'; import * as eg from '../../__tests__/lib/exampleData'; describe('draftsReducer', () => { - const testNarrow = topicNarrow(eg.stream.name, 'denmark2'); + const testNarrow = topicNarrow(eg.stream.name, eg.stream.stream_id, 'denmark2'); const testNarrowStr = keyFromNarrow(testNarrow); describe('DRAFT_UPDATE', () => { diff --git a/src/drafts/__tests__/draftsSelectors-test.js b/src/drafts/__tests__/draftsSelectors-test.js index ec07fd65f21..3a85fa00d8b 100644 --- a/src/drafts/__tests__/draftsSelectors-test.js +++ b/src/drafts/__tests__/draftsSelectors-test.js @@ -6,7 +6,7 @@ import * as eg from '../../__tests__/lib/exampleData'; describe('getDraftForNarrow', () => { test('return content of draft if exists', () => { - const narrow = topicNarrow(eg.stream.name, 'topic'); + const narrow = topicNarrow(eg.stream.name, eg.stream.stream_id, 'topic'); const state = eg.reduxState({ drafts: { [keyFromNarrow(narrow)]: 'content', @@ -19,14 +19,17 @@ describe('getDraftForNarrow', () => { }); test('return empty string if not exists', () => { - const narrow = topicNarrow(eg.stream.name, 'topic'); + const narrow = topicNarrow(eg.stream.name, eg.stream.stream_id, 'topic'); const state = eg.reduxState({ drafts: { [keyFromNarrow(narrow)]: 'content', }, }); - const draft = getDraftForNarrow(state, topicNarrow(eg.stream.name, 'topic1')); + const draft = getDraftForNarrow( + state, + topicNarrow(eg.stream.name, eg.stream.stream_id, 'topic1'), + ); expect(draft).toEqual(''); }); diff --git a/src/message/__tests__/fetchActions-test.js b/src/message/__tests__/fetchActions-test.js index 521266f0215..7bf5d47630c 100644 --- a/src/message/__tests__/fetchActions-test.js +++ b/src/message/__tests__/fetchActions-test.js @@ -26,7 +26,7 @@ import * as logging from '../../utils/logging'; const mockStore = configureStore([thunk]); -const narrow = streamNarrow(eg.stream.name); +const narrow = streamNarrow(eg.stream.name, eg.stream.stream_id); const streamNarrowStr = keyFromNarrow(narrow); global.FormData = class FormData {}; diff --git a/src/message/__tests__/messageActions-test.js b/src/message/__tests__/messageActions-test.js index 625e54f85b5..f660f0ead51 100644 --- a/src/message/__tests__/messageActions-test.js +++ b/src/message/__tests__/messageActions-test.js @@ -13,7 +13,7 @@ import type { Action } from '../../actionTypes'; const mockStore = configureStore([thunk]); -const streamNarrowObj = streamNarrow(eg.stream.name); +const streamNarrowObj = streamNarrow(eg.stream.name, eg.stream.stream_id); describe('messageActions', () => { describe('doNarrow', () => { diff --git a/src/message/__tests__/scrollStrategy-test.js b/src/message/__tests__/scrollStrategy-test.js index 5a95f6565d7..3a299f297bd 100644 --- a/src/message/__tests__/scrollStrategy-test.js +++ b/src/message/__tests__/scrollStrategy-test.js @@ -4,9 +4,9 @@ import { getScrollStrategy } from '../scrollStrategy'; import * as eg from '../../__tests__/lib/exampleData'; -const someNarrow = streamNarrow(eg.stream.name); +const someNarrow = streamNarrow(eg.stream.name, eg.stream.stream_id); const anotherStream = eg.makeStream(); -const anotherNarrow = streamNarrow(anotherStream.name); +const anotherNarrow = streamNarrow(anotherStream.name, anotherStream.stream_id); describe('getScrollStrategy', () => { test('initial load positions at anchor (first unread)', () => { diff --git a/src/nav/ChatNavBar.js b/src/nav/ChatNavBar.js index 78bbf6f74d4..ba8b0f0c194 100644 --- a/src/nav/ChatNavBar.js +++ b/src/nav/ChatNavBar.js @@ -53,7 +53,7 @@ function ExtraNavButtonTopic(props): Node { const streamName = streamNameOfNarrow(narrow); const stream = streams.find(x => x.name === streamName); if (stream) { - dispatch(doNarrow(streamNarrow(stream.name))); + dispatch(doNarrow(streamNarrow(stream.name, stream.stream_id))); } }, [dispatch, narrow, streams]); diff --git a/src/notification/__tests__/notification-test.js b/src/notification/__tests__/notification-test.js index d505a11ae0a..cf0895882d5 100644 --- a/src/notification/__tests__/notification-test.js +++ b/src/notification/__tests__/notification-test.js @@ -34,7 +34,7 @@ describe('getNarrowFromNotificationData', () => { topic: 'some topic', }; const narrow = getNarrowFromNotificationData(notification, new Map(), streamsByName, ownUserId); - expect(narrow).toEqual(topicNarrow(stream.name, 'some topic')); + expect(narrow).toEqual(topicNarrow(stream.name, stream.stream_id, 'some topic')); }); test('on notification for a private message returns a PM narrow', () => { diff --git a/src/notification/index.js b/src/notification/index.js index 068a756f497..1851e0bc91a 100644 --- a/src/notification/index.js +++ b/src/notification/index.js @@ -161,7 +161,7 @@ export const getNarrowFromNotificationData = ( if (data.recipient_type === 'stream') { const stream = streamsByName.get(data.stream_name); - return (stream && topicNarrow(stream.name, data.topic)) ?? null; + return (stream && topicNarrow(stream.name, stream.stream_id, data.topic)) ?? null; } if (data.pm_users === undefined) { diff --git a/src/sharing/ShareToStream.js b/src/sharing/ShareToStream.js index a8c2f7bca57..68856ee74ba 100644 --- a/src/sharing/ShareToStream.js +++ b/src/sharing/ShareToStream.js @@ -81,7 +81,7 @@ class ShareToStreamInner extends React.Component { // We have an actual stream selected. Fetch its recent topic names. // TODO: why do we do this? `TopicAutocomplete` does the same thing, // with an effect that reruns when the stream changes. - const narrow = streamNarrow(streamName); + const narrow = streamNarrow(streamName, streamId); dispatch(fetchTopicsForStream(narrow)); } // If what's entered in the stream-name field isn't an actual stream, @@ -147,7 +147,7 @@ class ShareToStreamInner extends React.Component { render() { const { sharedData } = this.props.route.params; const { streamName, streamId, topic, isStreamFocused, isTopicFocused } = this.state; - const narrow = streamId !== null ? streamNarrow(streamName) : null; + const narrow = streamId !== null ? streamNarrow(streamName, streamId) : null; const sendTo = { streamName, /* $FlowFixMe[incompatible-cast]: ShareWrapper will only look at this diff --git a/src/sharing/ShareWrapper.js b/src/sharing/ShareWrapper.js index e5e84953f35..97991e8e3a0 100644 --- a/src/sharing/ShareWrapper.js +++ b/src/sharing/ShareWrapper.js @@ -210,8 +210,8 @@ class ShareWrapperInner extends React.Component { } case 'stream': { - const { streamName } = sendTo; - const narrow = streamNarrow(streamName); + const { streamName, streamId } = sendTo; + const narrow = streamNarrow(streamName, streamId); NavigationService.dispatch(replaceWithChat(narrow)); break; } diff --git a/src/streams/SubscriptionsCard.js b/src/streams/SubscriptionsCard.js index 4b3f05949c9..2a7166d4791 100644 --- a/src/streams/SubscriptionsCard.js +++ b/src/streams/SubscriptionsCard.js @@ -34,7 +34,7 @@ export default function SubscriptionsCard(props: Props): Node { const handleNarrow = useCallback( (streamId: number, streamName: string) => { - dispatch(doNarrow(streamNarrow(streamName))); + dispatch(doNarrow(streamNarrow(streamName, streamId))); }, [dispatch], ); diff --git a/src/subscriptions/StreamListCard.js b/src/subscriptions/StreamListCard.js index 5a55ab79243..47effc88ad0 100644 --- a/src/subscriptions/StreamListCard.js +++ b/src/subscriptions/StreamListCard.js @@ -56,7 +56,7 @@ export default function StreamListCard(props: Props): Node { const handleNarrow = useCallback( (streamId: number, streamName: string) => { - dispatch(doNarrow(streamNarrow(streamName))); + dispatch(doNarrow(streamNarrow(streamName, streamId))); }, [dispatch], ); diff --git a/src/subscriptions/__tests__/subscriptionSelectors-test.js b/src/subscriptions/__tests__/subscriptionSelectors-test.js index dd57ebd8530..ea5254343ea 100644 --- a/src/subscriptions/__tests__/subscriptionSelectors-test.js +++ b/src/subscriptions/__tests__/subscriptionSelectors-test.js @@ -57,22 +57,22 @@ describe('getIsActiveStreamSubscribed', () => { }); test('return true if current narrowed stream is subscribed', () => { - const narrow = streamNarrow(eg.stream.name); + const narrow = streamNarrow(eg.stream.name, eg.stream.stream_id); expect(getIsActiveStreamSubscribed(state, narrow)).toBe(true); }); test('return false if current narrowed stream is not subscribed', () => { - const narrow = streamNarrow(eg.otherStream.name); + const narrow = streamNarrow(eg.otherStream.name, eg.otherStream.stream_id); expect(getIsActiveStreamSubscribed(state, narrow)).toBe(false); }); test('return true if stream of current narrowed topic is subscribed', () => { - const narrow = topicNarrow(eg.stream.name, 'news'); + const narrow = topicNarrow(eg.stream.name, eg.stream.stream_id, 'news'); expect(getIsActiveStreamSubscribed(state, narrow)).toBe(true); }); test('return false if stream of current narrowed topic is not subscribed', () => { - const narrow = topicNarrow(eg.otherStream.name, 'news'); + const narrow = topicNarrow(eg.otherStream.name, eg.otherStream.stream_id, 'news'); expect(getIsActiveStreamSubscribed(state, narrow)).toBe(false); }); }); @@ -84,13 +84,13 @@ describe('getStreamColorForNarrow', () => { }); test('return stream color for stream and topic narrow', () => { - const narrow = streamNarrow(eg.stream.name); + const narrow = streamNarrow(eg.stream.name, eg.stream.stream_id); expect(getStreamColorForNarrow(state, narrow)).toEqual(exampleColor); }); test('return null stream color for invalid stream or unknown subscriptions', () => { const unknownStream = eg.makeStream(); - const narrow = streamNarrow(unknownStream.name); + const narrow = streamNarrow(unknownStream.name, unknownStream.stream_id); expect(getStreamColorForNarrow(state, narrow)).toEqual('gray'); }); diff --git a/src/topics/TopicListScreen.js b/src/topics/TopicListScreen.js index e3b3ce4fda7..d536c6a7f89 100644 --- a/src/topics/TopicListScreen.js +++ b/src/topics/TopicListScreen.js @@ -27,7 +27,7 @@ export default function TopicListScreen(props: Props): Node { const handlePress = useCallback( (streamId: number, streamName: string, topic: string) => { - dispatch(doNarrow(topicNarrow(streamName, topic))); + dispatch(doNarrow(topicNarrow(streamName, streamId, topic))); }, [dispatch], ); diff --git a/src/topics/__tests__/topicsSelectors-test.js b/src/topics/__tests__/topicsSelectors-test.js index 8dbd721137f..61b367c4bd3 100644 --- a/src/topics/__tests__/topicsSelectors-test.js +++ b/src/topics/__tests__/topicsSelectors-test.js @@ -23,7 +23,7 @@ describe('getTopicsForNarrow', () => { }, }); - const topics = getTopicsForNarrow(state, streamNarrow(stream.name)); + const topics = getTopicsForNarrow(state, streamNarrow(stream.name, stream.stream_id)); expect(topics).toEqual(['hi', 'wow']); }); diff --git a/src/unread/UnreadCards.js b/src/unread/UnreadCards.js index d532b8f102d..573de4350d6 100644 --- a/src/unread/UnreadCards.js +++ b/src/unread/UnreadCards.js @@ -55,7 +55,7 @@ export default function UnreadCards(props: Props): Node { backgroundColor={section.color} unreadCount={section.unread} onPress={(streamId: number, streamName: string) => { - setTimeout(() => dispatch(doNarrow(streamNarrow(streamName)))); + setTimeout(() => dispatch(doNarrow(streamNarrow(streamName, streamId)))); }} /> ) @@ -72,7 +72,7 @@ export default function UnreadCards(props: Props): Node { isSelected={false} unreadCount={item.unread} onPress={(streamId: number, streamName: string, topic: string) => { - setTimeout(() => dispatch(doNarrow(topicNarrow(streamName, topic)))); + setTimeout(() => dispatch(doNarrow(topicNarrow(streamName, streamId, topic)))); }} /> ) diff --git a/src/utils/__tests__/internalLinks-test.js b/src/utils/__tests__/internalLinks-test.js index 7fe73eccce2..ece009c4138 100644 --- a/src/utils/__tests__/internalLinks-test.js +++ b/src/utils/__tests__/internalLinks-test.js @@ -275,7 +275,9 @@ describe('getNarrowFromLink', () => { /* eslint jest/expect-expect: ["error", { "assertFunctionNames": ["expect", "expectStream"] }] */ const expectStream = (operand, streams, expectedStream: null | Stream) => { expect(get(`#narrow/stream/${operand}`, streams)).toEqual( - expectedStream === null ? null : streamNarrow(expectedStream.name), + expectedStream === null + ? null + : streamNarrow(expectedStream.name, expectedStream.stream_id), ); }; @@ -320,7 +322,7 @@ describe('getNarrowFromLink', () => { const expectNameMatch = (operand, streamName) => { const stream = eg.makeStream({ name: streamName }); expect(get(`https://example.com/#narrow/stream/${operand}`, [stream])).toEqual( - streamNarrow(stream.name), + streamNarrow(stream.name, stream.stream_id), ); }; expectNameMatch('jest', 'jest'); @@ -334,10 +336,10 @@ describe('getNarrowFromLink', () => { test('on old stream link, without realm info', () => { expect(get(`/#narrow/stream/${eg.stream.name}`, [eg.stream])).toEqual( - streamNarrow(eg.stream.name), + streamNarrow(eg.stream.name, eg.stream.stream_id), ); expect(get(`#narrow/stream/${eg.stream.name}`, [eg.stream])).toEqual( - streamNarrow(eg.stream.name), + streamNarrow(eg.stream.name, eg.stream.stream_id), ); }); }); @@ -346,7 +348,9 @@ describe('getNarrowFromLink', () => { test('basic', () => { const expectBasic = (operand, expectedTopic) => { const url = `#narrow/stream/${streamGeneral.stream_id}-general/topic/${operand}`; - expect(get(url, [streamGeneral])).toEqual(topicNarrow(streamGeneral.name, expectedTopic)); + expect(get(url, [streamGeneral])).toEqual( + topicNarrow(streamGeneral.name, streamGeneral.stream_id, expectedTopic), + ); }; expectBasic('(no.20topic)', '(no topic)'); @@ -356,11 +360,11 @@ describe('getNarrowFromLink', () => { test('on old topic link, with dot-encoding', () => { expect( get(`https://example.com/#narrow/stream/${eg.stream.name}/topic/(no.20topic)`, [eg.stream]), - ).toEqual(topicNarrow(eg.stream.name, '(no topic)')); + ).toEqual(topicNarrow(eg.stream.name, eg.stream.stream_id, '(no topic)')); expect( get(`https://example.com/#narrow/stream/${eg.stream.name}/topic/google.2Ecom`, [eg.stream]), - ).toEqual(topicNarrow(eg.stream.name, 'google.com')); + ).toEqual(topicNarrow(eg.stream.name, eg.stream.stream_id, 'google.com')); expect(() => get(`https://example.com/#narrow/stream/${eg.stream.name}/topic/google.com`, [eg.stream]), @@ -368,23 +372,23 @@ describe('getNarrowFromLink', () => { expect( get(`https://example.com/#narrow/stream/${eg.stream.name}/topic/topic.20name`, [eg.stream]), - ).toEqual(topicNarrow(eg.stream.name, 'topic name')); + ).toEqual(topicNarrow(eg.stream.name, eg.stream.stream_id, 'topic name')); expect( get(`https://example.com/#narrow/stream/${eg.stream.name}/topic/stream`, [eg.stream]), - ).toEqual(topicNarrow(eg.stream.name, 'stream')); + ).toEqual(topicNarrow(eg.stream.name, eg.stream.stream_id, 'stream')); expect( get(`https://example.com/#narrow/stream/${eg.stream.name}/topic/topic`, [eg.stream]), - ).toEqual(topicNarrow(eg.stream.name, 'topic')); + ).toEqual(topicNarrow(eg.stream.name, eg.stream.stream_id, 'topic')); }); test('on old topic link, without realm info', () => { expect(get(`/#narrow/stream/${eg.stream.name}/topic/topic`, [eg.stream])).toEqual( - topicNarrow(eg.stream.name, 'topic'), + topicNarrow(eg.stream.name, eg.stream.stream_id, 'topic'), ); expect(get(`#narrow/stream/${eg.stream.name}/topic/topic`, [eg.stream])).toEqual( - topicNarrow(eg.stream.name, 'topic'), + topicNarrow(eg.stream.name, eg.stream.stream_id, 'topic'), ); }); }); @@ -416,11 +420,11 @@ describe('getNarrowFromLink', () => { expect( get(`https://example.com/#narrow/stream/${eg.stream.name}/topic/test/near/1`, [eg.stream]), - ).toEqual(topicNarrow(eg.stream.name, 'test')); + ).toEqual(topicNarrow(eg.stream.name, eg.stream.stream_id, 'test')); expect( get(`https://example.com/#narrow/stream/${eg.stream.name}/subject/test/near/1`, [eg.stream]), - ).toEqual(topicNarrow(eg.stream.name, 'test')); + ).toEqual(topicNarrow(eg.stream.name, eg.stream.stream_id, 'test')); }); }); diff --git a/src/utils/__tests__/narrow-test.js b/src/utils/__tests__/narrow-test.js index f43e30cf0c1..2d361d16985 100644 --- a/src/utils/__tests__/narrow-test.js +++ b/src/utils/__tests__/narrow-test.js @@ -61,8 +61,10 @@ describe('isPmNarrow', () => { describe('isStreamOrTopicNarrow', () => { test('check for stream or topic narrow', () => { expect(isStreamOrTopicNarrow(undefined)).toBe(false); - expect(isStreamOrTopicNarrow(streamNarrow(eg.stream.name))).toBe(true); - expect(isStreamOrTopicNarrow(topicNarrow(eg.stream.name, 'some topic'))).toBe(true); + expect(isStreamOrTopicNarrow(streamNarrow(eg.stream.name, eg.stream.stream_id))).toBe(true); + expect( + isStreamOrTopicNarrow(topicNarrow(eg.stream.name, eg.stream.stream_id, 'some topic')), + ).toBe(true); expect(isStreamOrTopicNarrow(HOME_NARROW)).toBe(false); expect(isStreamOrTopicNarrow(pm1to1NarrowFromUser(eg.otherUser))).toBe(false); expect(isStreamOrTopicNarrow(pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser]))).toBe( @@ -76,14 +78,14 @@ describe('specialNarrow', () => { test('only narrowing with the "is" operator is special narrow', () => { expect(isSpecialNarrow(undefined)).toBe(false); expect(isSpecialNarrow(HOME_NARROW)).toBe(false); - expect(isSpecialNarrow(streamNarrow(eg.stream.name))).toBe(false); + expect(isSpecialNarrow(streamNarrow(eg.stream.name, eg.stream.stream_id))).toBe(false); expect(isSpecialNarrow(STARRED_NARROW)).toBe(true); }); }); describe('streamNarrow', () => { test('narrows to messages from a specific stream', () => { - const narrow = streamNarrow(eg.stream.name); + const narrow = streamNarrow(eg.stream.name, eg.stream.stream_id); expect(isStreamNarrow(narrow)).toBeTrue(); expect(streamNameOfNarrow(narrow)).toEqual(eg.stream.name); }); @@ -91,13 +93,13 @@ describe('streamNarrow', () => { test('only narrow with operator of "stream" is a stream narrow', () => { expect(isStreamNarrow(undefined)).toBe(false); expect(isStreamNarrow(HOME_NARROW)).toBe(false); - expect(isStreamNarrow(streamNarrow(eg.stream.name))).toBe(true); + expect(isStreamNarrow(streamNarrow(eg.stream.name, eg.stream.stream_id))).toBe(true); }); }); describe('topicNarrow', () => { test('narrows to a specific topic within a specified stream', () => { - const narrow = topicNarrow(eg.stream.name, 'some topic'); + const narrow = topicNarrow(eg.stream.name, eg.stream.stream_id, 'some topic'); expect(isTopicNarrow(narrow)).toBeTrue(); expect(streamNameOfNarrow(narrow)).toEqual(eg.stream.name); expect(topicOfNarrow(narrow)).toEqual('some topic'); @@ -106,7 +108,9 @@ describe('topicNarrow', () => { test('only narrow with two items, one for stream, one for topic is a topic narrow', () => { expect(isTopicNarrow(undefined)).toBe(false); expect(isTopicNarrow(HOME_NARROW)).toBe(false); - expect(isTopicNarrow(topicNarrow(eg.stream.name, 'some topic'))).toBe(true); + expect(isTopicNarrow(topicNarrow(eg.stream.name, eg.stream.stream_id, 'some topic'))).toBe( + true, + ); }); }); @@ -127,12 +131,12 @@ describe('isMessageInNarrow', () => { ['a message', true, eg.streamMessage()], ]], - ['whole-stream narrow', streamNarrow(eg.stream.name), [ + ['whole-stream narrow', streamNarrow(eg.stream.name, eg.stream.stream_id), [ ['matching stream message', true, eg.streamMessage()], ['other-stream message', false, eg.streamMessage({ stream: otherStream })], ['PM', false, eg.pmMessage()], ]], - ['stream conversation', topicNarrow(eg.stream.name, 'cabbages'), [ + ['stream conversation', topicNarrow(eg.stream.name, eg.stream.stream_id, 'cabbages'), [ ['matching message', true, eg.streamMessage({ subject: 'cabbages' })], ['message in same stream but other topic', false, eg.streamMessage({ subject: 'kings' })], ['other-stream message', false, eg.streamMessage({ stream: otherStream })], @@ -253,8 +257,8 @@ describe('getNarrowsForMessage', () => { }, expectedNarrows: [ HOME_NARROW, - streamNarrow(eg.stream.name), - topicNarrow(eg.stream.name, 'myTopic'), + streamNarrow(eg.stream.name, eg.stream.stream_id), + topicNarrow(eg.stream.name, eg.stream.stream_id, 'myTopic'), ], }, { @@ -273,7 +277,11 @@ describe('getNarrowsForMessage', () => { ...eg.streamMessage({ stream: eg.stream }), subject: '', }, - expectedNarrows: [HOME_NARROW, streamNarrow(eg.stream.name), topicNarrow(eg.stream.name, '')], + expectedNarrows: [ + HOME_NARROW, + streamNarrow(eg.stream.name, eg.stream.stream_id), + topicNarrow(eg.stream.name, eg.stream.stream_id, ''), + ], }, { label: 'PM message with one person', @@ -323,7 +331,7 @@ describe('getNarrowForReply', () => { test('for stream message with nonempty topic, returns a topic narrow', () => { const message = eg.streamMessage(); - const expectedNarrow = topicNarrow(eg.stream.name, message.subject); + const expectedNarrow = topicNarrow(eg.stream.name, eg.stream.stream_id, message.subject); const actualNarrow = getNarrowForReply(message, eg.selfUser.user_id); @@ -333,8 +341,8 @@ describe('getNarrowForReply', () => { describe('keyFromNarrow+parseNarrow', () => { const baseNarrows = [ - ['whole stream', streamNarrow(eg.stream.name)], - ['stream conversation', topicNarrow(eg.stream.name, 'a topic')], + ['whole stream', streamNarrow(eg.stream.name, eg.stream.stream_id)], + ['stream conversation', topicNarrow(eg.stream.name, eg.stream.stream_id, 'a topic')], ['1:1 PM conversation, non-self', pm1to1NarrowFromUser(eg.otherUser)], ['self-1:1 conversation', pm1to1NarrowFromUser(eg.selfUser)], ['group-PM conversation', pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser])], @@ -353,15 +361,29 @@ describe('keyFromNarrow+parseNarrow', () => { const diverseCharactersStream = eg.makeStream({ name: diverseCharacters }); const htmlEntitiesStream = eg.makeStream({ name: htmlEntities }); const awkwardNarrows = [ - ['whole stream about awkward characters', streamNarrow(diverseCharactersStream.name)], - ['whole stream about HTML entities', streamNarrow(htmlEntitiesStream.name)], + [ + 'whole stream about awkward characters', + streamNarrow(diverseCharactersStream.name, diverseCharactersStream.stream_id), + ], + [ + 'whole stream about HTML entities', + streamNarrow(htmlEntitiesStream.name, htmlEntitiesStream.stream_id), + ], [ 'stream conversation about awkward characters', - topicNarrow(diverseCharactersStream.name, `regarding ${diverseCharacters}`), + topicNarrow( + diverseCharactersStream.name, + diverseCharactersStream.stream_id, + `regarding ${diverseCharacters}`, + ), ], [ 'stream conversation about HTML entities', - topicNarrow(htmlEntitiesStream.name, `regarding ${htmlEntities}`), + topicNarrow( + htmlEntitiesStream.name, + htmlEntitiesStream.stream_id, + `regarding ${htmlEntities}`, + ), ], ['search narrow for awkward characters', SEARCH_NARROW(diverseCharacters)], ['search narrow for HTML entities', SEARCH_NARROW(htmlEntities)], diff --git a/src/utils/internalLinks.js b/src/utils/internalLinks.js index 71a3f1f50cd..9b493d8d6cd 100644 --- a/src/utils/internalLinks.js +++ b/src/utils/internalLinks.js @@ -189,11 +189,11 @@ export const getNarrowFromLink = ( } case 'topic': { const streamNameAndId = parseStreamOperand(paths[1], streamsById, streamsByName); - return streamNameAndId && topicNarrow(streamNameAndId[0], parseTopicOperand(paths[3])); + return streamNameAndId && topicNarrow(...streamNameAndId, parseTopicOperand(paths[3])); } case 'stream': { const streamNameAndId = parseStreamOperand(paths[1], streamsById, streamsByName); - return streamNameAndId && streamNarrow(streamNameAndId[0]); + return streamNameAndId && streamNarrow(...streamNameAndId); } case 'special': try { diff --git a/src/utils/narrow.js b/src/utils/narrow.js index 143606ac809..aef1edcbee5 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -533,8 +533,8 @@ export const getNarrowsForMessage = ( result.push(pmNarrowFromRecipients(pmKeyRecipientsFromMessage(message, ownUserId))); } else { const streamName = streamNameOfStreamMessage(message); - result.push(topicNarrow(streamName, message.subject)); - result.push(streamNarrow(streamName)); + result.push(topicNarrow(streamName, message.stream_id, message.subject)); + result.push(streamNarrow(streamName, message.stream_id)); } if (flags.includes('mentioned') || flags.includes('wildcard_mentioned')) { @@ -562,6 +562,6 @@ export const getNarrowForReply = (message: Message | Outbox, ownUserId: UserId): return pmNarrowFromRecipients(pmKeyRecipientsFromMessage(message, ownUserId)); } else { const streamName = streamNameOfStreamMessage(message); - return topicNarrow(streamName, message.subject); + return topicNarrow(streamName, message.stream_id, message.subject); } }; diff --git a/src/webview/html/__tests__/messageListElementHtml-test.js b/src/webview/html/__tests__/messageListElementHtml-test.js index cc25e4e38ef..d47221d6e3d 100644 --- a/src/webview/html/__tests__/messageListElementHtml-test.js +++ b/src/webview/html/__tests__/messageListElementHtml-test.js @@ -273,7 +273,7 @@ describe('messages -> piece descriptors -> content HTML is stable/sensible', () ].forEach(testCase => check(testCase)); }); - const streamNarrow1 = streamNarrow(stream1.name); + const streamNarrow1 = streamNarrow(stream1.name, stream1.stream_id); test(`${keyFromNarrow(streamNarrow1)}`, () => { [ { narrow: streamNarrow1, messages: streamMessages1 }, @@ -294,7 +294,7 @@ describe('messages -> piece descriptors -> content HTML is stable/sensible', () ].forEach(testCase => check(testCase)); }); - const topicNarrow1 = topicNarrow(stream1.name, topic1); + const topicNarrow1 = topicNarrow(stream1.name, stream1.stream_id, topic1); test(`${keyFromNarrow(topicNarrow1)}`, () => { [ { narrow: topicNarrow1, messages: streamMessages1 }, diff --git a/src/webview/html/header.js b/src/webview/html/header.js index e264af6c4c2..f3044d0059f 100644 --- a/src/webview/html/header.js +++ b/src/webview/html/header.js @@ -39,7 +39,9 @@ export default ( if (message.type === 'stream') { const streamName = streamNameOfStreamMessage(message); - const topicNarrowStr = keyFromNarrow(topicNarrow(streamName, message.subject)); + const topicNarrowStr = keyFromNarrow( + topicNarrow(streamName, message.stream_id, message.subject), + ); const topicHtml = renderSubject(message); if (headerStyle === 'topic+date') { @@ -57,7 +59,7 @@ export default ( const subscription = subscriptions.get(message.stream_id); const backgroundColor = subscription ? subscription.color : 'hsl(0, 0%, 80%)'; const textColor = foregroundColorFromBackground(backgroundColor); - const streamNarrowStr = keyFromNarrow(streamNarrow(streamName)); + const streamNarrowStr = keyFromNarrow(streamNarrow(streamName, message.stream_id)); return template`
Date: Wed, 29 Dec 2021 14:11:15 -0800 Subject: [PATCH 10/24] narrow: Store stream IDs, not only names! In the migration tests, it is kind of messy that we have to update a previous migration's test. That happens because we're running all the migrations to completion. As the comment at the "whole sequence" test says about a related point: this is probably not a great design for continuing to add more migrations in this sequence. But pretty soon we're going to cap it off and write future migrations in a new framework, which will come with its own way of writing tests. So for the moment just live with it. Fixes-partly: #4333 --- src/compose/ComposeBox.js | 4 +- src/storage/__tests__/migrations-test.js | 35 ++++++-- src/storage/migrations.js | 12 +++ src/utils/__tests__/narrow-test.js | 3 + src/utils/narrow.js | 51 +++++++---- ...essageListElementHtml-test.js.snap.android | 90 +++++++++---------- .../messageListElementHtml-test.js.snap.ios | 90 +++++++++---------- 7 files changed, 169 insertions(+), 116 deletions(-) diff --git a/src/compose/ComposeBox.js b/src/compose/ComposeBox.js index 64b021143ae..14509ab9aad 100644 --- a/src/compose/ComposeBox.js +++ b/src/compose/ComposeBox.js @@ -35,6 +35,7 @@ import { isStreamNarrow, isStreamOrTopicNarrow, isTopicNarrow, + streamIdOfNarrow, streamNameOfNarrow, topicNarrow, topicOfNarrow, @@ -402,8 +403,9 @@ class ComposeBoxInner extends PureComponent { const { narrow, isEditing } = this.props; if (isStreamNarrow(narrow) || (isTopicNarrow(narrow) && isEditing)) { const streamName = streamNameOfNarrow(narrow); + const streamId = streamIdOfNarrow(narrow); const topic = this.state.topic.trim(); - return topicNarrow(streamName, topic || apiConstants.NO_TOPIC_TOPIC); + return topicNarrow(streamName, streamId, topic || apiConstants.NO_TOPIC_TOPIC); } invariant(isConversationNarrow(narrow), 'destination narrow must be conversation'); return narrow; diff --git a/src/storage/__tests__/migrations-test.js b/src/storage/__tests__/migrations-test.js index 2c516de1dae..2ed553e5061 100644 --- a/src/storage/__tests__/migrations-test.js +++ b/src/storage/__tests__/migrations-test.js @@ -59,8 +59,8 @@ describe('migrations', () => { ], }; - // What `base` becomes after all migrations. - const endBase = { + // What `base` becomes after migrations up through 37. + const base37 = { migrations: { version: 37 }, accounts: [ { @@ -84,14 +84,20 @@ describe('migrations', () => { }, }; + // What `base` becomes after all migrations. + const endBase = { + ...base37, + migrations: { version: 38 }, + }; + for (const [desc, before, after] of [ // Test the behavior with no migration state, which doesn't apply any // of the specific migrations. - ['empty state -> just store version', {}, { migrations: { version: 37 } }], + ['empty state -> just store version', {}, { migrations: endBase.migrations }], [ 'no migration state -> just store version, leave everything else', { nonsense: [1, 2, 3] }, - { migrations: { version: 37 }, nonsense: [1, 2, 3] }, + { migrations: endBase.migrations, nonsense: [1, 2, 3] }, ], // Test the whole sequence all together. This covers many of the @@ -144,9 +150,12 @@ describe('migrations', () => { migrations: { version: 21 }, drafts: { 'pm:d:12:other@example.com': 'text', 'topic:s:general\x00stuff': 'other text' }, }, - { ...endBase, drafts: { 'pm:12': 'text', 'topic:s:general\x00stuff': 'other text' } }, - // NB the original version of this migration was buggy; it resulted in: - // { ...endBase, drafts: { 'topic:s:general\x00stuff': 'other text' } }, // WRONG + // This migration itself leaves the `topic:` draft in place. But it + // gets dropped later by migration 38. + { ...endBase, drafts: { 'pm:12': 'text' } }, + // NB the original version of this migration was buggy; it would drop + // the PM drafts entirely, so this test case would end up as: + // { ...endBase, drafts: {} }, // WRONG // Should have written tests for it the first time. :-) ], [ @@ -204,6 +213,18 @@ describe('migrations', () => { { ...base15, settings: { ...base15.settings, doNotMarkMessagesAsRead: true } }, { ...endBase, settings: { ...endBase.settings, doNotMarkMessagesAsRead: true } }, ], + [ + 'check 38', + { + ...base37, + drafts: { + 'topic:s:general\x00stuff': 'text', + 'stream:s:general': 'more text', + 'pm:12': 'pm text', + }, + }, + { ...endBase, drafts: { 'pm:12': 'pm text' } }, + ], ]) { /* eslint-disable no-loop-func */ test(desc, async () => { diff --git a/src/storage/migrations.js b/src/storage/migrations.js index 3ad1f4dbda9..b4c2273ad61 100644 --- a/src/storage/migrations.js +++ b/src/storage/migrations.js @@ -350,6 +350,18 @@ const migrationsInner: {| [string]: (LessPartialState) => LessPartialState |} = }, }), + // Change format of keys representing stream and topic narrows, adding IDs. + '38': state => ({ + ...state, + drafts: objectFromEntries( + // Just drop drafts for stream and topic narrows, for the same reasons + // as for PM narrows in migration 21 above. + Object.keys(state.drafts) + .filter(key => !key.startsWith('stream:') && !key.startsWith('topic:')) + .map(key => [key, state.drafts[key]]), + ), + }), + // TIP: When adding a migration, consider just using `dropCache`. }; diff --git a/src/utils/__tests__/narrow-test.js b/src/utils/__tests__/narrow-test.js index 2d361d16985..7f41ffd0dbd 100644 --- a/src/utils/__tests__/narrow-test.js +++ b/src/utils/__tests__/narrow-test.js @@ -26,6 +26,7 @@ import { streamNameOfNarrow, topicOfNarrow, caseNarrowPartial, + streamIdOfNarrow, } from '../narrow'; import type { Narrow, Message } from '../../types'; import * as eg from '../../__tests__/lib/exampleData'; @@ -87,6 +88,7 @@ describe('streamNarrow', () => { test('narrows to messages from a specific stream', () => { const narrow = streamNarrow(eg.stream.name, eg.stream.stream_id); expect(isStreamNarrow(narrow)).toBeTrue(); + expect(streamIdOfNarrow(narrow)).toEqual(eg.stream.stream_id); expect(streamNameOfNarrow(narrow)).toEqual(eg.stream.name); }); @@ -101,6 +103,7 @@ describe('topicNarrow', () => { test('narrows to a specific topic within a specified stream', () => { const narrow = topicNarrow(eg.stream.name, eg.stream.stream_id, 'some topic'); expect(isTopicNarrow(narrow)).toBeTrue(); + expect(streamIdOfNarrow(narrow)).toEqual(eg.stream.stream_id); expect(streamNameOfNarrow(narrow)).toEqual(eg.stream.name); expect(topicOfNarrow(narrow)).toEqual('some topic'); }); diff --git a/src/utils/narrow.js b/src/utils/narrow.js index aef1edcbee5..ecdc4f55957 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -34,8 +34,8 @@ import { * server, and `apiNarrowOfNarrow` for converting to it. */ export opaque type Narrow = - | {| type: 'stream', streamName: string |} - | {| type: 'topic', streamName: string, topic: string |} + | {| type: 'stream', streamName: string, streamId: number |} + | {| type: 'topic', streamName: string, streamId: number, topic: string |} | {| type: 'pm', userIds: PmKeyRecipients |} | {| type: 'search', query: string |} | {| type: 'all' | 'starred' | 'mentioned' | 'all-pm' |}; @@ -148,12 +148,11 @@ export const ALL_PRIVATE_NARROW: Narrow = specialNarrow('private'); export const ALL_PRIVATE_NARROW_STR: string = keyFromNarrow(ALL_PRIVATE_NARROW); -export const streamNarrow = (streamName: string, streamId?: number): Narrow => - Object.freeze({ type: 'stream', streamName }); +export const streamNarrow = (streamName: string, streamId: number): Narrow => + Object.freeze({ type: 'stream', streamName, streamId }); -export const topicNarrow = (streamName: string, ...args: [string] | [number, string]): Narrow => - // $FlowFixMe[incompatible-cast] - Object.freeze({ type: 'topic', streamName, topic: (args[args.length - 1]: string) }); +export const topicNarrow = (streamName: string, streamId: number, topic: string): Narrow => + Object.freeze({ type: 'topic', streamName, streamId, topic }); export const SEARCH_NARROW = (query: string): Narrow => Object.freeze({ type: 'search', query }); @@ -163,8 +162,8 @@ type NarrowCases = {| starred: () => T, mentioned: () => T, allPrivate: () => T, - stream: (name: string) => T, - topic: (streamName: string, topic: string) => T, + stream: (name: string, streamId: number) => T, + topic: (streamName: string, topic: string, streamId: number) => T, search: (query: string) => T, |}; @@ -175,8 +174,8 @@ 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 'stream': return cases.stream(narrow.streamName, narrow.streamId); + case 'topic': return cases.topic(narrow.streamName, narrow.topic, narrow.streamId); case 'pm': return cases.pm(narrow.userIds); case 'search': return cases.search(narrow.query); case 'all': return cases.home(); @@ -253,10 +252,13 @@ export function keyFromNarrow(narrow: Narrow): string { // 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. - stream: name => `stream:s:${name}`, + // An earlier version had `stream:s:`. + stream: (name, streamId) => `stream:d:${streamId}:${name}`, + + // An earlier version had `topic:s:`. // '\x00' is the one character not allowed in Zulip stream names. // (See `check_stream_name` in zulip.git:zerver/lib/streams.py.) - topic: (streamName, topic) => `topic:s:${streamName}\x00${topic}`, + topic: (streamName, topic, streamId) => `topic:d:${streamId}:${streamName}\x00${topic}`, // An earlier version had `pm:s:`. // Another had `pm:d:`. @@ -281,22 +283,25 @@ export const parseNarrow = (narrowStr: string): Narrow => { // invariant: tag + rest === narrowStr switch (tag) { case 'stream:': { - if (!rest.startsWith('s:')) { + // The `/s` regexp flag means the `.` pattern matches absolutely + // anything. By default it rejects certain "newline" characters, + // which in principle could appear in stream names. + const match = /^d:(\d+):(.*)/s.exec(rest); + if (!match) { throw makeError(); } - const name = rest.substr('s:'.length); - return streamNarrow(name); + return streamNarrow(match[2], Number.parseInt(match[1], 10)); } case 'topic:': { // The `/s` regexp flag means the `.` patterns match absolutely // anything. By default they reject certain "newline" characters, // which in principle could appear in stream names or topics. - const match = /^s:(.*?)\x00(.*)/s.exec(rest); + const match = /^d:(\d+):(.*?)\x00(.*)/s.exec(rest); if (!match) { throw makeError(); } - return topicNarrow(match[1], match[2]); + return topicNarrow(match[2], Number.parseInt(match[1], 10), match[3]); } case 'pm:': { @@ -358,6 +363,16 @@ export const userIdsOfPmNarrow = (narrow: Narrow): PmKeyRecipients => export const streamNameOfNarrow = (narrow: Narrow): string => caseNarrowPartial(narrow, { stream: name => name, topic: streamName => streamName }); +/** + * The stream ID for a stream or topic narrow; else error. + * + * Most callers of this should probably be getting passed a stream ID + * instead of a Narrow in the first place; or if they do handle other kinds + * of narrows, should be using `caseNarrow`. + */ +export const streamIdOfNarrow = (narrow: Narrow): number => + caseNarrowPartial(narrow, { stream: (n, id) => id, topic: (n, t, id) => id }); + /** * The topic for a topic narrow; else error. * diff --git a/src/webview/html/__tests__/__snapshots__/messageListElementHtml-test.js.snap.android b/src/webview/html/__tests__/__snapshots__/messageListElementHtml-test.js.snap.android index 7d7a0e0eefc..0370b7b0217 100644 --- a/src/webview/html/__tests__/__snapshots__/messageListElementHtml-test.js.snap.android +++ b/src/webview/html/__tests__/__snapshots__/messageListElementHtml-test.js.snap.android @@ -10,11 +10,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoxOnN0cmVhbSAxAHRvcGljIDE=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MTpzdHJlYW0gMQ==\\"> # stream 1
topic 1
@@ -88,11 +88,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoxOnN0cmVhbSAxAHRvcGljIDE=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MTpzdHJlYW0gMQ==\\"> # stream 1
topic 1
@@ -171,11 +171,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoxOnN0cmVhbSAxAHRvcGljIDE=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MTpzdHJlYW0gMQ==\\"> # stream 1
topic 1
@@ -214,11 +214,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoxOnN0cmVhbSAxAHRvcGljIDI=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MTpzdHJlYW0gMQ==\\"> # stream 1
topic 2
@@ -267,11 +267,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoxOnN0cmVhbSAxAHRvcGljIDE=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MTpzdHJlYW0gMQ==\\"> # stream 1
topic 1
@@ -310,11 +310,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoyOnN0cmVhbSAyAHRvcGljIDI=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MjpzdHJlYW0gMg==\\"> # stream 2
topic 2
@@ -363,11 +363,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoxOnN0cmVhbSAxAHRvcGljIDE=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MTpzdHJlYW0gMQ==\\"> # stream 1
topic 1
@@ -910,11 +910,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoxOnN0cmVhbSAxAHRvcGljIDE=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MTpzdHJlYW0gMQ==\\"> # stream 1
topic 1
@@ -1056,11 +1056,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoxOnN0cmVhbSAxAHRvcGljIDE=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MTpzdHJlYW0gMQ==\\"> # stream 1
topic 1
@@ -1099,11 +1099,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoxOnN0cmVhbSAxAHRvcGljIDI=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MTpzdHJlYW0gMQ==\\"> # stream 1
topic 2
@@ -1402,11 +1402,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoxOnN0cmVhbSAxAHRvcGljIDE=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MTpzdHJlYW0gMQ==\\"> # stream 1
topic 1
@@ -1445,11 +1445,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoyOnN0cmVhbSAyAHRvcGljIDI=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MjpzdHJlYW0gMg==\\"> # stream 2
topic 2
@@ -1488,11 +1488,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoxOnN0cmVhbSAxAHRvcGljIDE=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MTpzdHJlYW0gMQ==\\"> # stream 1
topic 1
@@ -1622,11 +1622,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoxOnN0cmVhbSAxAHRvcGljIDE=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MTpzdHJlYW0gMQ==\\"> # stream 1
topic 1
@@ -3373,7 +3373,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible pm:1,2 " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:s:stream 1 1`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:d:1:stream 1 1`] = ` "
@@ -3383,7 +3383,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3447,7 +3447,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:s:stream 1 2`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:d:1:stream 1 2`] = ` "
@@ -3457,7 +3457,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3526,7 +3526,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:s:stream 1 3`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:d:1:stream 1 3`] = ` "
@@ -3536,7 +3536,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3575,7 +3575,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 2
@@ -3614,7 +3614,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:s:stream 1 4`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:d:1:stream 1 4`] = ` "
@@ -3624,7 +3624,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3699,7 +3699,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:s:stream 1 5`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:d:1:stream 1 5`] = ` "
@@ -3709,7 +3709,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3809,7 +3809,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 2
@@ -3854,7 +3854,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3989,7 +3989,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:s:stream 1topic 1 1`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:d:1:stream 1topic 1 1`] = ` "
@@ -4054,7 +4054,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible topic: " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:s:stream 1topic 1 2`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:d:1:stream 1topic 1 2`] = ` "
@@ -4124,7 +4124,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible topic: " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:s:stream 1topic 1 3`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:d:1:stream 1topic 1 3`] = ` "
@@ -4200,7 +4200,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible topic: " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:s:stream 1topic 1 4`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:d:1:stream 1topic 1 4`] = ` "
diff --git a/src/webview/html/__tests__/__snapshots__/messageListElementHtml-test.js.snap.ios b/src/webview/html/__tests__/__snapshots__/messageListElementHtml-test.js.snap.ios index 7d7a0e0eefc..0370b7b0217 100644 --- a/src/webview/html/__tests__/__snapshots__/messageListElementHtml-test.js.snap.ios +++ b/src/webview/html/__tests__/__snapshots__/messageListElementHtml-test.js.snap.ios @@ -10,11 +10,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoxOnN0cmVhbSAxAHRvcGljIDE=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MTpzdHJlYW0gMQ==\\"> # stream 1
topic 1
@@ -88,11 +88,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoxOnN0cmVhbSAxAHRvcGljIDE=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MTpzdHJlYW0gMQ==\\"> # stream 1
topic 1
@@ -171,11 +171,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoxOnN0cmVhbSAxAHRvcGljIDE=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MTpzdHJlYW0gMQ==\\"> # stream 1
topic 1
@@ -214,11 +214,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoxOnN0cmVhbSAxAHRvcGljIDI=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MTpzdHJlYW0gMQ==\\"> # stream 1
topic 2
@@ -267,11 +267,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoxOnN0cmVhbSAxAHRvcGljIDE=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MTpzdHJlYW0gMQ==\\"> # stream 1
topic 1
@@ -310,11 +310,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoyOnN0cmVhbSAyAHRvcGljIDI=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MjpzdHJlYW0gMg==\\"> # stream 2
topic 2
@@ -363,11 +363,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoxOnN0cmVhbSAxAHRvcGljIDE=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MTpzdHJlYW0gMQ==\\"> # stream 1
topic 1
@@ -910,11 +910,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoxOnN0cmVhbSAxAHRvcGljIDE=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MTpzdHJlYW0gMQ==\\"> # stream 1
topic 1
@@ -1056,11 +1056,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoxOnN0cmVhbSAxAHRvcGljIDE=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MTpzdHJlYW0gMQ==\\"> # stream 1
topic 1
@@ -1099,11 +1099,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoxOnN0cmVhbSAxAHRvcGljIDI=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MTpzdHJlYW0gMQ==\\"> # stream 1
topic 2
@@ -1402,11 +1402,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoxOnN0cmVhbSAxAHRvcGljIDE=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MTpzdHJlYW0gMQ==\\"> # stream 1
topic 1
@@ -1445,11 +1445,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoyOnN0cmVhbSAyAHRvcGljIDI=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MjpzdHJlYW0gMg==\\"> # stream 2
topic 2
@@ -1488,11 +1488,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoxOnN0cmVhbSAxAHRvcGljIDE=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MTpzdHJlYW0gMQ==\\"> # stream 1
topic 1
@@ -1622,11 +1622,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6ZDoxOnN0cmVhbSAxAHRvcGljIDE=\\">
+ data-narrow=\\"c3RyZWFtOmQ6MTpzdHJlYW0gMQ==\\"> # stream 1
topic 1
@@ -3373,7 +3373,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible pm:1,2 " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:s:stream 1 1`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:d:1:stream 1 1`] = ` "
@@ -3383,7 +3383,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3447,7 +3447,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:s:stream 1 2`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:d:1:stream 1 2`] = ` "
@@ -3457,7 +3457,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3526,7 +3526,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:s:stream 1 3`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:d:1:stream 1 3`] = ` "
@@ -3536,7 +3536,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3575,7 +3575,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 2
@@ -3614,7 +3614,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:s:stream 1 4`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:d:1:stream 1 4`] = ` "
@@ -3624,7 +3624,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3699,7 +3699,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:s:stream 1 5`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:d:1:stream 1 5`] = ` "
@@ -3709,7 +3709,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3809,7 +3809,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 2
@@ -3854,7 +3854,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3989,7 +3989,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:s:stream 1topic 1 1`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:d:1:stream 1topic 1 1`] = ` "
@@ -4054,7 +4054,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible topic: " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:s:stream 1topic 1 2`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:d:1:stream 1topic 1 2`] = ` "
@@ -4124,7 +4124,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible topic: " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:s:stream 1topic 1 3`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:d:1:stream 1topic 1 3`] = ` "
@@ -4200,7 +4200,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible topic: " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:s:stream 1topic 1 4`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:d:1:stream 1topic 1 4`] = ` "
From 8761c2b038f6bf7e64ba378ce155b0337f8a93eb Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 28 Jan 2022 13:33:55 -0800 Subject: [PATCH 11/24] nav: Omit "topic list", "stream info" buttons on unknown streams If we actually make it to either of these screens (TopicListScreen or StreamSettingsScreen) with an unknown stream, they'll try to look up our data on the stream and will promptly crash. We're allowing this ChatNavBar component to tolerate unknown streams, because one can sometimes navigate to a narrow in such a stream and we try to represent that as a proper screen: ChatScreen will show InvalidNarrow in that case instead of MessageList, and will show a ChatNavBar as usual. But then let's draw the line at navigating to any other screens. This one already delivers the message that there's nothing here; no need to have other screens handle this case just to deliver the same message. At the moment, these buttons don't do anything in this case anyway: if the stream is unknown, then the callback won't find the stream when it looks it up, and will silently do nothing. But in our migration toward stream IDs, we'll shortly have these callbacks navigate directly by stream ID rather than have to look up the name in our streams data. Without this change, that would result in navigating to the destination screen with an invalid stream. --- src/nav/ChatNavBar.js | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/nav/ChatNavBar.js b/src/nav/ChatNavBar.js index ba8b0f0c194..8ed467172f2 100644 --- a/src/nav/ChatNavBar.js +++ b/src/nav/ChatNavBar.js @@ -23,12 +23,18 @@ import { doNarrow, } from '../actions'; import * as NavigationService from './NavigationService'; -import { getStreams } from '../selectors'; +import { getStreams, isNarrowValid as getIsNarrowValid } from '../selectors'; import NavButton from './NavButton'; function ExtraNavButtonStream(props): Node { const { color, narrow } = props; const streams = useSelector(getStreams); + + const isNarrowValid = useSelector(state => getIsNarrowValid(state, narrow)); + if (!isNarrowValid) { + return null; + } + return ( getIsNarrowValid(state, narrow)); + if (!isNarrowValid) { + return null; + } + return ( Date: Wed, 29 Dec 2021 14:28:03 -0800 Subject: [PATCH 12/24] narrow: Cut all unneeded calls to streamNameOfNarrow In almost all the places we were using streamNameOfNarrow, what we really want is just the stream ID, not its name; or an actual Stream or Subscription object. Now that the narrows contain the stream ID, we can get it directly, and look up the Stream or Subscription by that instead of by name. The only exceptions are where we construct new Narrows from old ones, either to go from a stream narrow to a topic narrow or vice versa. After this commit, the only call sites of streamNameForNarrow are those and its own tests. This change should be NFC except in cases where the stream name and ID don't agree -- e.g., because the stream has been renamed. In those cases, it fixes a bug. --- src/chat/MarkAsReadButton.js | 21 ++++---------- src/chat/narrowsSelectors.js | 13 +++++---- src/compose/ComposeBox.js | 5 ++-- src/nav/ChatNavBar.js | 32 ++++++++-------------- src/subscriptions/subscriptionSelectors.js | 20 ++++++-------- src/topics/topicActions.js | 16 ++--------- src/topics/topicSelectors.js | 22 +++++---------- 7 files changed, 44 insertions(+), 85 deletions(-) diff --git a/src/chat/MarkAsReadButton.js b/src/chat/MarkAsReadButton.js index c0b124669d0..f2df6965a5d 100644 --- a/src/chat/MarkAsReadButton.js +++ b/src/chat/MarkAsReadButton.js @@ -8,14 +8,14 @@ import { createStyleSheet } from '../styles'; import { useSelector } from '../react-redux'; import { ZulipButton } from '../common'; import * as api from '../api'; -import { getAuth, getStreams, getOwnUserId } from '../selectors'; +import { getAuth, getOwnUserId } from '../selectors'; import { getUnread, getUnreadIdsForPmNarrow } from '../unread/unreadModel'; import { isHomeNarrow, isStreamNarrow, isTopicNarrow, isPmNarrow, - streamNameOfNarrow, + streamIdOfNarrow, topicOfNarrow, } from '../utils/narrow'; @@ -35,7 +35,6 @@ type Props = $ReadOnly<{| export default function MarkAsReadButton(props: Props): Node { const { narrow } = props; const auth = useSelector(getAuth); - const streams = useSelector(getStreams); const unread = useSelector(getUnread); const ownUserId = useSelector(getOwnUserId); @@ -44,20 +43,12 @@ export default function MarkAsReadButton(props: Props): Node { }, [auth]); const markStreamAsRead = useCallback(() => { - const streamName = streamNameOfNarrow(narrow); - const stream = streams.find(s => s.name === streamName); - if (stream) { - api.markStreamAsRead(auth, stream.stream_id); - } - }, [auth, narrow, streams]); + api.markStreamAsRead(auth, streamIdOfNarrow(narrow)); + }, [auth, narrow]); const markTopicAsRead = useCallback(() => { - const streamName = streamNameOfNarrow(narrow); - const stream = streams.find(s => s.name === streamName); - if (stream) { - api.markTopicAsRead(auth, stream.stream_id, topicOfNarrow(narrow)); - } - }, [auth, narrow, streams]); + api.markTopicAsRead(auth, streamIdOfNarrow(narrow), topicOfNarrow(narrow)); + }, [auth, narrow]); const markPmAsRead = useCallback(() => { // The message IDs come from our unread-messages data, which is diff --git a/src/chat/narrowsSelectors.js b/src/chat/narrowsSelectors.js index e20baf422b0..ccc32f77b45 100644 --- a/src/chat/narrowsSelectors.js +++ b/src/chat/narrowsSelectors.js @@ -28,13 +28,14 @@ import { isMessageInNarrow, caseNarrowDefault, keyFromNarrow, - streamNameOfNarrow, caseNarrow, + streamIdOfNarrow, } from '../utils/narrow'; import { isTopicMuted } from '../mute/muteModel'; import { streamNameOfStreamMessage } from '../utils/recipient'; import { NULL_ARRAY, NULL_SUBSCRIPTION } from '../nullObjects'; import * as logging from '../utils/logging'; +import { getStreamsById, getSubscriptionsById } from '../selectors'; export const outboxMessagesForNarrow: Selector<$ReadOnlyArray, Narrow> = createSelector( (state, narrow) => narrow, @@ -196,20 +197,20 @@ export const getLastMessageId = (state: PerAccountState, narrow: Narrow): number // TODO: clean up what this returns; possibly to just `Stream` export const getStreamInNarrow: Selector = createSelector( (state, narrow) => narrow, - state => getSubscriptions(state), - state => getStreams(state), + state => getSubscriptionsById(state), + state => getStreamsById(state), (narrow, subscriptions, streams) => { if (!isStreamOrTopicNarrow(narrow)) { return NULL_SUBSCRIPTION; } - const streamName = streamNameOfNarrow(narrow); + const streamId = streamIdOfNarrow(narrow); - const subscription = subscriptions.find(x => x.name === streamName); + const subscription = subscriptions.get(streamId); if (subscription) { return subscription; } - const stream = streams.find(x => x.name === streamName); + const stream = streams.get(streamId); if (stream) { return { ...stream, diff --git a/src/compose/ComposeBox.js b/src/compose/ComposeBox.js index 14509ab9aad..6f2711ce134 100644 --- a/src/compose/ComposeBox.js +++ b/src/compose/ComposeBox.js @@ -402,10 +402,9 @@ class ComposeBoxInner extends PureComponent { getDestinationNarrow = (): Narrow => { const { narrow, isEditing } = this.props; if (isStreamNarrow(narrow) || (isTopicNarrow(narrow) && isEditing)) { - const streamName = streamNameOfNarrow(narrow); const streamId = streamIdOfNarrow(narrow); - const topic = this.state.topic.trim(); - return topicNarrow(streamName, streamId, topic || apiConstants.NO_TOPIC_TOPIC); + const topic = this.state.topic.trim() || apiConstants.NO_TOPIC_TOPIC; + return topicNarrow(streamNameOfNarrow(narrow), streamId, topic); } invariant(isConversationNarrow(narrow), 'destination narrow must be conversation'); return narrow; diff --git a/src/nav/ChatNavBar.js b/src/nav/ChatNavBar.js index 8ed467172f2..9b1c25e4275 100644 --- a/src/nav/ChatNavBar.js +++ b/src/nav/ChatNavBar.js @@ -14,7 +14,12 @@ import Title from '../title/Title'; import NavBarBackButton from './NavBarBackButton'; import { getStreamColorForNarrow } from '../subscriptions/subscriptionSelectors'; import { foregroundColorFromBackground } from '../utils/color'; -import { caseNarrowDefault, streamNameOfNarrow, streamNarrow } from '../utils/narrow'; +import { + caseNarrowDefault, + streamIdOfNarrow, + streamNameOfNarrow, + streamNarrow, +} from '../utils/narrow'; import { navigateToStream, navigateToAccountDetails, @@ -23,12 +28,11 @@ import { doNarrow, } from '../actions'; import * as NavigationService from './NavigationService'; -import { getStreams, isNarrowValid as getIsNarrowValid } from '../selectors'; +import { isNarrowValid as getIsNarrowValid } from '../selectors'; import NavButton from './NavButton'; function ExtraNavButtonStream(props): Node { const { color, narrow } = props; - const streams = useSelector(getStreams); const isNarrowValid = useSelector(state => getIsNarrowValid(state, narrow)); if (!isNarrowValid) { @@ -40,11 +44,7 @@ function ExtraNavButtonStream(props): Node { name="list" color={color} onPress={() => { - const streamName = streamNameOfNarrow(narrow); - const stream = streams.find(x => x.name === streamName); - if (stream) { - NavigationService.dispatch(navigateToTopicList(stream.stream_id)); - } + NavigationService.dispatch(navigateToTopicList(streamIdOfNarrow(narrow))); }} /> ); @@ -53,22 +53,16 @@ function ExtraNavButtonStream(props): Node { function ExtraNavButtonTopic(props): Node { const { narrow, color } = props; const dispatch = useDispatch(); - const streams = useSelector(getStreams); const handlePress = useCallback(() => { - const streamName = streamNameOfNarrow(narrow); - const stream = streams.find(x => x.name === streamName); - if (stream) { - dispatch(doNarrow(streamNarrow(stream.name, stream.stream_id))); - } - }, [dispatch, narrow, streams]); + dispatch(doNarrow(streamNarrow(streamNameOfNarrow(narrow), streamIdOfNarrow(narrow)))); + }, [dispatch, narrow]); return ; } function InfoNavButtonStream(props): Node { const { color, narrow } = props; - const streams = useSelector(getStreams); const isNarrowValid = useSelector(state => getIsNarrowValid(state, narrow)); if (!isNarrowValid) { @@ -80,11 +74,7 @@ function InfoNavButtonStream(props): Node { name="info" color={color} onPress={() => { - const streamName = streamNameOfNarrow(narrow); - const stream = streams.find(x => x.name === streamName); - if (stream) { - NavigationService.dispatch(navigateToStream(stream.stream_id)); - } + NavigationService.dispatch(navigateToStream(streamIdOfNarrow(narrow))); }} /> ); diff --git a/src/subscriptions/subscriptionSelectors.js b/src/subscriptions/subscriptionSelectors.js index d7942dbf280..c1fff6fe3c5 100644 --- a/src/subscriptions/subscriptionSelectors.js +++ b/src/subscriptions/subscriptionSelectors.js @@ -2,7 +2,7 @@ import { createSelector } from 'reselect'; import type { PerAccountState, Narrow, Selector, Stream, Subscription } from '../types'; -import { isStreamOrTopicNarrow, streamNameOfNarrow } from '../utils/narrow'; +import { isStreamOrTopicNarrow, streamIdOfNarrow } from '../utils/narrow'; import { getSubscriptions, getStreams } from '../directSelectors'; /** @@ -39,14 +39,12 @@ export const getSubscriptionsByName: Selector> = creat export const getIsActiveStreamSubscribed: Selector = createSelector( (state, narrow) => narrow, - state => getSubscriptions(state), + state => getSubscriptionsById(state), (narrow, subscriptions) => { if (!isStreamOrTopicNarrow(narrow)) { return true; } - const streamName = streamNameOfNarrow(narrow); - - return subscriptions.find(sub => streamName === sub.name) !== undefined; + return subscriptions.get(streamIdOfNarrow(narrow)) !== undefined; }, ); @@ -70,14 +68,12 @@ export const getStreamForId = (state: PerAccountState, streamId: number): Stream export const getIsActiveStreamAnnouncementOnly: Selector = createSelector( (state, narrow) => narrow, - state => getStreams(state), + state => getStreamsById(state), (narrow, streams) => { if (!isStreamOrTopicNarrow(narrow)) { return false; } - const streamName = streamNameOfNarrow(narrow); - - const stream = streams.find(stream_ => streamName === stream_.name); + const stream = streams.get(streamIdOfNarrow(narrow)); return stream ? stream.is_announcement_only : false; }, ); @@ -92,7 +88,7 @@ export const getStreamColorForNarrow = (state: PerAccountState, narrow: Narrow): return undefined; } - const subscriptionsByName = getSubscriptionsByName(state); - const streamName = streamNameOfNarrow(narrow); - return subscriptionsByName.get(streamName)?.color ?? 'gray'; + const subscriptionsById = getSubscriptionsById(state); + const streamId = streamIdOfNarrow(narrow); + return subscriptionsById.get(streamId)?.color ?? 'gray'; }; diff --git a/src/topics/topicActions.js b/src/topics/topicActions.js index b91259673b7..af8b700841c 100644 --- a/src/topics/topicActions.js +++ b/src/topics/topicActions.js @@ -2,8 +2,8 @@ import type { Narrow, Topic, PerAccountAction, ThunkAction, Outbox } from '../types'; import * as api from '../api'; import { INIT_TOPICS } from '../actionConstants'; -import { isStreamNarrow, streamNameOfNarrow } from '../utils/narrow'; -import { getAuth, getStreams } from '../selectors'; +import { isStreamNarrow, streamIdOfNarrow } from '../utils/narrow'; +import { getAuth } from '../selectors'; import { deleteOutboxMessage } from '../actions'; import { getOutbox } from '../directSelectors'; @@ -26,20 +26,10 @@ export const fetchTopicsForStream = (narrow: Narrow): ThunkAction> dispatch, getState, ) => { - const state = getState(); - if (!isStreamNarrow(narrow)) { return; } - const streamName = streamNameOfNarrow(narrow); - - const streams = getStreams(state); - // TODO (#4333): Look for the stream by its ID, not its name. - const stream = streams.find(sub => streamName === sub.name); - if (!stream) { - return; - } - dispatch(fetchTopics(stream.stream_id)); + dispatch(fetchTopics(streamIdOfNarrow(narrow))); }; export const deleteMessagesForTopic = ( diff --git a/src/topics/topicSelectors.js b/src/topics/topicSelectors.js index fe11baafcad..506d93d6f2d 100644 --- a/src/topics/topicSelectors.js +++ b/src/topics/topicSelectors.js @@ -1,35 +1,27 @@ /* @flow strict-local */ import { createSelector } from 'reselect'; -import type { Narrow, Selector, StreamsState, TopicExtended, TopicsState } from '../types'; -import { getMute, getStreams, getTopics } from '../directSelectors'; +import type { Narrow, Selector, TopicExtended, TopicsState } from '../types'; +import { getMute, getTopics } from '../directSelectors'; import { getUnread, getUnreadCountForTopic } from '../unread/unreadModel'; import { getStreamsById } from '../subscriptions/subscriptionSelectors'; import { NULL_ARRAY } from '../nullObjects'; -import { isStreamNarrow, streamNameOfNarrow } from '../utils/narrow'; +import { isStreamNarrow, streamIdOfNarrow } from '../utils/narrow'; import { isTopicMuted } from '../mute/muteModel'; export const getTopicsForNarrow: Selector<$ReadOnlyArray, Narrow> = createSelector( (state, narrow) => narrow, state => getTopics(state), - state => getStreams(state), - (narrow: Narrow, topics: TopicsState, streams: StreamsState) => { + (narrow: Narrow, topics: TopicsState) => { if (!isStreamNarrow(narrow)) { return NULL_ARRAY; } - const streamName = streamNameOfNarrow(narrow); + const streamId = streamIdOfNarrow(narrow); - // TODO (#4333): Look for the stream by its ID, not its name. One - // expected consequence of the current code is that - // `TopicAutocomplete` would stop showing any topics, if someone - // changed the stream name while you were looking at - // `TopicAutocomplete`. - const stream = streams.find(x => x.name === streamName); - if (!stream || !topics[stream.stream_id]) { + if (!topics[streamId]) { return NULL_ARRAY; } - - return topics[stream.stream_id].map(x => x.name); + return topics[streamId].map(x => x.name); }, ); From f0e6d1bab5a8416fb3d64a015b4c928bc8920831 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 29 Dec 2021 16:46:14 -0800 Subject: [PATCH 13/24] subscriptions [nfc]: Cut now-disused getSubscriptionsByName --- src/subscriptions/subscriptionSelectors.js | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/subscriptions/subscriptionSelectors.js b/src/subscriptions/subscriptionSelectors.js index c1fff6fe3c5..f3440fca848 100644 --- a/src/subscriptions/subscriptionSelectors.js +++ b/src/subscriptions/subscriptionSelectors.js @@ -32,11 +32,6 @@ export const getSubscriptionsById: Selector> = createS new Map(subscriptions.map(subscription => [subscription.stream_id, subscription])), ); -export const getSubscriptionsByName: Selector> = createSelector( - getSubscriptions, - subscriptions => new Map(subscriptions.map(subscription => [subscription.name, subscription])), -); - export const getIsActiveStreamSubscribed: Selector = createSelector( (state, narrow) => narrow, state => getSubscriptionsById(state), From 45e86579964edca861cdd6a03dd903255b32da1d Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 29 Dec 2021 16:51:24 -0800 Subject: [PATCH 14/24] webview [nfc]: Cut streamsByName from BackgroundData We stopped needing this a little while ago, in 8b3332150, after we started having stream IDs on StreamOutbox as well as StreamMessage. --- src/__tests__/lib/exampleData.js | 8 +------- src/streams/TopicItem.js | 2 -- src/webview/MessageList.js | 3 --- 3 files changed, 1 insertion(+), 12 deletions(-) diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index 97a4a2ee472..1c01f95ccc1 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -47,12 +47,7 @@ import rootReducer from '../../boot/reducers'; import { authOfAccount } from '../../account/accountMisc'; import { HOME_NARROW } from '../../utils/narrow'; import type { BackgroundData } from '../../webview/MessageList'; -import { - getSettings, - getStreamsById, - getStreamsByName, - getSubscriptionsById, -} from '../../selectors'; +import { getSettings, getStreamsById, getSubscriptionsById } from '../../selectors'; /* ======================================================================== * Utilities @@ -852,7 +847,6 @@ export const backgroundData: BackgroundData = deepFreeze({ mutedUsers: Immutable.Map(), ownUser: selfUser, streams: getStreamsById(baseReduxState), - streamsByName: getStreamsByName(baseReduxState), subscriptions: getSubscriptionsById(baseReduxState), unread: baseReduxState.unread, theme: 'default', diff --git a/src/streams/TopicItem.js b/src/streams/TopicItem.js index e1856b223c4..a7fe7b73b6d 100644 --- a/src/streams/TopicItem.js +++ b/src/streams/TopicItem.js @@ -17,7 +17,6 @@ import { getFlags, getSubscriptionsById, getStreamsById, - getStreamsByName, getOwnUser, } from '../selectors'; import { getUnread } from '../unread/unreadModel'; @@ -66,7 +65,6 @@ export default function TopicItem(props: Props): Node { auth: getAuth(state), mute: getMute(state), streams: getStreamsById(state), - streamsByName: getStreamsByName(state), subscriptions: getSubscriptionsById(state), unread: getUnread(state), ownUser: getOwnUser(state), diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index 248c23e49a6..f2d76d36e0e 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -47,7 +47,6 @@ import { getGlobalSettings, getSubscriptionsById, getStreamsById, - getStreamsByName, getRealm, } from '../selectors'; import { withGetText } from '../boot/TranslationProvider'; @@ -92,7 +91,6 @@ export type BackgroundData = $ReadOnly<{| mutedUsers: MutedUsersState, ownUser: User, streams: Map, - streamsByName: Map, subscriptions: Map, unread: UnreadState, theme: ThemeName, @@ -364,7 +362,6 @@ const MessageList: ComponentType = connect( mutedUsers: getMutedUsers(state), ownUser: getOwnUser(state), streams: getStreamsById(state), - streamsByName: getStreamsByName(state), subscriptions: getSubscriptionsById(state), unread: getUnread(state), theme: globalSettings.theme, From 0e152a5feb7fc32c5f5680eb1de346372e458e57 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 11 Jan 2022 21:58:27 -0800 Subject: [PATCH 15/24] narrow [nfc]: Drop unused parameters at some caseNarrow call sites This allows these callbacks to not have to change around (and also to not become misleading in their parameters' names) as we rearrange what arguments are given to these callbacks, as part of replacing stream names with stream IDs. --- src/message/fetchActions.js | 6 +++--- src/nav/ChatNavBar.js | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/message/fetchActions.js b/src/message/fetchActions.js index d4238b98017..695891c2d9a 100644 --- a/src/message/fetchActions.js +++ b/src/message/fetchActions.js @@ -147,14 +147,14 @@ export const fetchMessages = (fetchArgs: {| // Describe the narrow without sending sensitive data to Sentry. narrow: caseNarrow(fetchArgs.narrow, { - stream: name => 'stream', - topic: (streamName, topic) => 'topic', + stream: () => 'stream', + topic: () => 'topic', pm: ids => (ids.length > 1 ? 'pm (group)' : 'pm (1:1)'), home: () => 'all', starred: () => 'starred', mentioned: () => 'mentioned', allPrivate: () => 'all-pm', - search: query => 'search', + search: () => 'search', }), anchor: fetchArgs.anchor, diff --git a/src/nav/ChatNavBar.js b/src/nav/ChatNavBar.js index 9b1c25e4275..ceb8b3350c4 100644 --- a/src/nav/ChatNavBar.js +++ b/src/nav/ChatNavBar.js @@ -117,13 +117,13 @@ const ActionItems: ComponentType<{| +color: string, +narrow: Narrow |}> = props caseNarrowDefault( props.narrow, { - stream: streamName => ( + stream: () => ( <> ), - topic: (streamName, topic) => ( + topic: () => ( <> From 8f58e77df1110ef4f175ec7e79a5c3e76b67a44c Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 29 Dec 2021 15:37:18 -0800 Subject: [PATCH 16/24] narrow: Switch to stream ID at some caseNarrow call sites This covers the spots where this conversion is local and trivial. The change should be NFC except in cases where the stream name and ID don't agree (e.g., because the stream's name has been changed since we formed this Narrow object). In those cases, it fixes a bug. To find these, and the other call sites in the next few commits, I manually audited through all calls of the `caseNarrow*` functions. Ideally for this sort of refactor one would like the type-checker to do that work, but sadly here it isn't able to completely do so: in getComposeInputPlaceholder we pass the callback's argument to string interpolation, and in two other places we used it only for `===`. (If there were a `JSON.stringify`, that'd have the same problem.) And those accept numeric IDs just the same as string names. --- src/chat/ChatScreen.js | 4 +++- src/chat/narrowsSelectors.js | 7 +++---- src/outbox/outboxActions.js | 10 +++++----- src/unread/unreadSelectors.js | 20 +++++++------------- src/utils/narrow.js | 8 +++----- 5 files changed, 21 insertions(+), 28 deletions(-) diff --git a/src/chat/ChatScreen.js b/src/chat/ChatScreen.js index 23eb933f40e..6c62bf0b52e 100644 --- a/src/chat/ChatScreen.js +++ b/src/chat/ChatScreen.js @@ -138,7 +138,9 @@ export default function ChatScreen(props: Props): Node { const content = editMessage.content !== message ? message : undefined; const subject = caseNarrowDefault( destinationNarrow, - { topic: (stream, topic) => (topic !== editMessage.topic ? topic : undefined) }, + { + topic: (_1, topic, streamId) => (topic !== editMessage.topic ? topic : undefined), + }, () => undefined, ); diff --git a/src/chat/narrowsSelectors.js b/src/chat/narrowsSelectors.js index ccc32f77b45..a1db21a74a4 100644 --- a/src/chat/narrowsSelectors.js +++ b/src/chat/narrowsSelectors.js @@ -17,7 +17,6 @@ import { getSubscriptions, getMessages, getMute, - getStreams, getOutbox, getFlags, } from '../directSelectors'; @@ -224,14 +223,14 @@ export const getStreamInNarrow: Selector = createSelector( (state, narrow) => narrow, - state => getStreams(state), + state => getStreamsById(state), state => getAllUsersById(state), (narrow, streams, allUsersById) => caseNarrowDefault( narrow, { - stream: streamName => streams.find(s => s.name === streamName) !== undefined, - topic: streamName => streams.find(s => s.name === streamName) !== undefined, + stream: (_, streamId) => streams.get(streamId) !== undefined, + topic: (_, t, streamId) => streams.get(streamId) !== undefined, pm: ids => ids.every(id => allUsersById.get(id) !== undefined), }, () => true, diff --git a/src/outbox/outboxActions.js b/src/outbox/outboxActions.js index 818f0ccce79..43cc8c01637 100644 --- a/src/outbox/outboxActions.js +++ b/src/outbox/outboxActions.js @@ -23,7 +23,7 @@ import { DELETE_OUTBOX_MESSAGE, MESSAGE_SEND_COMPLETE, } from '../actionConstants'; -import { getAuth, getStreamsByName } from '../selectors'; +import { getAuth, getStreamsById } from '../selectors'; import * as api from '../api'; import { getAllUsersById, getOwnUser } from '../users/userSelectors'; import { getUsersAndWildcards } from '../users/userHelpers'; @@ -133,7 +133,7 @@ type DataFromNarrow = const outboxPropertiesForNarrow = ( destinationNarrow: Narrow, - streamsByName: Map, + streamsById: Map, allUsersById: Map, ownUser: UserOrBot, ): DataFromNarrow => @@ -144,8 +144,8 @@ const outboxPropertiesForNarrow = ( subject: '', }), - topic: (streamName, topic) => { - const stream = streamsByName.get(streamName); + topic: (_, topic, streamId) => { + const stream = streamsById.get(streamId); invariant(stream, 'narrow must be known stream'); return { type: 'stream', @@ -185,7 +185,7 @@ export const addToOutbox = ( isSent: false, ...outboxPropertiesForNarrow( destinationNarrow, - getStreamsByName(state), + getStreamsById(state), getAllUsersById(state), ownUser, ), diff --git a/src/unread/unreadSelectors.js b/src/unread/unreadSelectors.js index 4995fa4fd40..c55375da4c0 100644 --- a/src/unread/unreadSelectors.js +++ b/src/unread/unreadSelectors.js @@ -3,9 +3,9 @@ import { createSelector } from 'reselect'; import type { Narrow, Selector, UnreadStreamItem } from '../types'; import { caseInsensitiveCompareFunc } from '../utils/misc'; -import { getMute, getStreams } from '../directSelectors'; +import { getMute } from '../directSelectors'; import { getOwnUserId } from '../users/userSelectors'; -import { getSubscriptionsById } from '../subscriptions/subscriptionSelectors'; +import { getSubscriptionsById, getStreamsById } from '../subscriptions/subscriptionSelectors'; import { isTopicMuted } from '../mute/muteModel'; import { caseNarrow } from '../utils/narrow'; import { NULL_SUBSCRIPTION } from '../nullObjects'; @@ -222,7 +222,7 @@ export const getUnreadByHuddlesMentionsAndPMs: Selector = createSelector */ export const getUnreadCountForNarrow: Selector = createSelector( (state, narrow) => narrow, - state => getStreams(state), + state => getStreamsById(state), state => getOwnUserId(state), state => getUnreadTotal(state), state => getUnread(state), @@ -231,8 +231,8 @@ export const getUnreadCountForNarrow: Selector = createSelector( caseNarrow(narrow, { home: () => unreadTotal, - stream: name => { - const stream = streams.find(s => s.name === name); + stream: (_1, streamId) => { + const stream = streams.get(streamId); if (!stream) { return 0; } @@ -241,20 +241,14 @@ export const getUnreadCountForNarrow: Selector = createSelector( unread.streams .get(stream.stream_id) ?.entrySeq() - .filterNot(([topic, _]) => isTopicMuted(name, topic, mute)) + .filterNot(([topic, _]) => isTopicMuted(stream.name, topic, mute)) .map(([_, msgIds]) => msgIds.size) .reduce((s, x) => s + x, 0) ?? 0 ); }, - topic: (streamName, topic) => { - const stream = streams.find(s => s.name === streamName); - if (!stream) { - return 0; - } - return getUnreadCountForTopic(unread, stream.stream_id, topic); - }, + topic: (_, topic, streamId) => getUnreadCountForTopic(unread, streamId, topic), pm: _ => getUnreadIdsForPmNarrow(unread, narrow, ownUserId).length, diff --git a/src/utils/narrow.js b/src/utils/narrow.js index ecdc4f55957..d63bfc7d95f 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -477,11 +477,9 @@ export const isMessageInNarrow = ( ): boolean => caseNarrow(narrow, { home: () => true, - stream: name => message.type === 'stream' && name === streamNameOfStreamMessage(message), - topic: (streamName, topic) => - message.type === 'stream' - && streamName === streamNameOfStreamMessage(message) - && topic === message.subject, + stream: (_, streamId) => message.type === 'stream' && streamId === message.stream_id, + topic: (_, topic, streamId) => + message.type === 'stream' && streamId === message.stream_id && topic === message.subject, pm: ids => { if (message.type !== 'private') { return false; From bb7a8cfe658315f5f156de66f06828c0f0201de7 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 11 Jan 2022 21:51:32 -0800 Subject: [PATCH 17/24] narrow: Use stream ID in getComposeInputPlaceholder --- src/compose/ComposeBox.js | 6 +++++- .../__tests__/getComposeInputPlaceholder-test.js | 12 +++++++----- src/compose/getComposeInputPlaceholder.js | 14 +++++++++----- 3 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/compose/ComposeBox.js b/src/compose/ComposeBox.js index 6f2711ce134..10a5cec5537 100644 --- a/src/compose/ComposeBox.js +++ b/src/compose/ComposeBox.js @@ -49,6 +49,7 @@ import { getAuth, getIsAdmin, getStreamInNarrow, + getStreamsById, getVideoChatProvider, getRealm, } from '../selectors'; @@ -72,6 +73,7 @@ type SelectorProps = {| videoChatProvider: VideoChatProvider | null, mandatoryTopics: boolean, stream: Subscription | {| ...Stream, in_home_view: boolean |}, + streamsById: Map, |}; type OuterProps = $ReadOnly<{| @@ -543,6 +545,7 @@ class ComposeBoxInner extends PureComponent { isAnnouncementOnly, isSubscribed, stream, + streamsById, videoChatProvider, _, } = this.props; @@ -556,7 +559,7 @@ class ComposeBoxInner extends PureComponent { return ; } - const placeholder = getComposeInputPlaceholder(narrow, ownUserId, allUsersById); + const placeholder = getComposeInputPlaceholder(narrow, ownUserId, allUsersById, streamsById); const style = { paddingBottom: insets.bottom, backgroundColor: 'hsla(0, 0%, 50%, 0.1)', @@ -677,6 +680,7 @@ const ComposeBox: ComponentType = compose( isAnnouncementOnly: getIsActiveStreamAnnouncementOnly(state, props.narrow), isSubscribed: getIsActiveStreamSubscribed(state, props.narrow), stream: getStreamInNarrow(state, props.narrow), + streamsById: getStreamsById(state), videoChatProvider: getVideoChatProvider(state), mandatoryTopics: getRealm(state).mandatoryTopics, })), diff --git a/src/compose/__tests__/getComposeInputPlaceholder-test.js b/src/compose/__tests__/getComposeInputPlaceholder-test.js index 8edf0fb1f61..7bcb39cb7bc 100644 --- a/src/compose/__tests__/getComposeInputPlaceholder-test.js +++ b/src/compose/__tests__/getComposeInputPlaceholder-test.js @@ -9,14 +9,16 @@ import { pmNarrowFromUsersUnsafe, } from '../../utils/narrow'; import * as eg from '../../__tests__/lib/exampleData'; +import { getStreamsById } from '../../selectors'; describe('getComposeInputPlaceholder', () => { const usersById = new Map([eg.selfUser, eg.otherUser, eg.thirdUser].map(u => [u.user_id, u])); const ownUserId = eg.selfUser.user_id; + const streamsById = getStreamsById(eg.plusReduxState); test('returns "Message @ThisPerson" object for person narrow', () => { const narrow = deepFreeze(pm1to1NarrowFromUser(eg.otherUser)); - const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById); + const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById, streamsById); expect(placeholder).toEqual({ text: 'Message {recipient}', values: { recipient: `@${eg.otherUser.full_name}` }, @@ -25,13 +27,13 @@ describe('getComposeInputPlaceholder', () => { test('returns "Jot down something" object for self narrow', () => { const narrow = deepFreeze(pm1to1NarrowFromUser(eg.selfUser)); - const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById); + const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById, streamsById); expect(placeholder).toEqual({ text: 'Jot down something' }); }); test('returns "Message #streamName" for stream narrow', () => { const narrow = deepFreeze(streamNarrow(eg.stream.name, eg.stream.stream_id)); - const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById); + const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById, streamsById); expect(placeholder).toEqual({ text: 'Message {recipient}', values: { recipient: `#${eg.stream.name}` }, @@ -40,13 +42,13 @@ describe('getComposeInputPlaceholder', () => { test('returns properly for topic narrow', () => { const narrow = deepFreeze(topicNarrow(eg.stream.name, eg.stream.stream_id, 'Copenhagen')); - const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById); + const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById, streamsById); expect(placeholder).toEqual({ text: 'Reply' }); }); test('returns "Message group" object for group narrow', () => { const narrow = deepFreeze(pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser])); - const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById); + const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById, streamsById); expect(placeholder).toEqual({ text: 'Message group' }); }); }); diff --git a/src/compose/getComposeInputPlaceholder.js b/src/compose/getComposeInputPlaceholder.js index 8c9cd9915a9..d0bd0b9b7a6 100644 --- a/src/compose/getComposeInputPlaceholder.js +++ b/src/compose/getComposeInputPlaceholder.js @@ -1,11 +1,12 @@ /* @flow strict-local */ -import type { Narrow, UserId, UserOrBot, LocalizableText } from '../types'; +import type { Narrow, Stream, UserId, UserOrBot, LocalizableText } from '../types'; import { caseNarrowDefault } from '../utils/narrow'; export default ( narrow: Narrow, ownUserId: UserId, allUsersById: Map, + streamsById: Map, ): LocalizableText => caseNarrowDefault( narrow, @@ -30,10 +31,13 @@ export default ( values: { recipient: `@${user.full_name}` }, }; }, - stream: name => ({ - text: 'Message {recipient}', - values: { recipient: `#${name}` }, - }), + stream: (_, streamId) => { + const stream = streamsById.get(streamId); + if (!stream) { + return { text: 'Type a message' }; + } + return { text: 'Message {recipient}', values: { recipient: `#${stream.name}` } }; + }, topic: () => ({ text: 'Reply' }), }, () => ({ text: 'Type a message' }), From ffc0fce1f0324d491264a9073102050380483a34 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 12 Jan 2022 22:15:09 -0800 Subject: [PATCH 18/24] narrow: Use narrow's stream ID to get name in apiNarrowOfNarrow We still need a stream name here for the actual request to the server, until we start requiring server 2.1. But we can look up the name by ID in the overall state, just as we do for PM narrows. In particular this means that we'll now behave correctly if the stream got renamed since we formed this Narrow object (although until we push IDs all the way through to the server, there'll still be a race if the stream gets renamed concurrently with our request.) --- src/message/fetchActions.js | 13 +++++++++++-- src/utils/narrow.js | 35 +++++++++++++++++++++-------------- 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/src/message/fetchActions.js b/src/message/fetchActions.js index 695891c2d9a..857e818ec4c 100644 --- a/src/message/fetchActions.js +++ b/src/message/fetchActions.js @@ -24,6 +24,7 @@ import { getFetchingForNarrow, getIsAdmin, getIdentity, + getStreamsById, } from '../selectors'; import config from '../config'; import { @@ -121,7 +122,11 @@ export const fetchMessages = (fetchArgs: {| await tryFetch(() => api.getMessages(getAuth(getState()), { ...fetchArgs, - narrow: apiNarrowOfNarrow(fetchArgs.narrow, getAllUsersById(getState())), + narrow: apiNarrowOfNarrow( + fetchArgs.narrow, + getAllUsersById(getState()), + getStreamsById(getState()), + ), useFirstUnread: fetchArgs.anchor === FIRST_UNREAD_ANCHOR, // TODO: don't use this; see #4203 }), ); @@ -336,7 +341,11 @@ export const fetchMessagesInNarrow = ( const fetchPrivateMessages = () => async (dispatch, getState) => { const auth = getAuth(getState()); const { messages, found_newest, found_oldest } = await api.getMessages(auth, { - narrow: apiNarrowOfNarrow(ALL_PRIVATE_NARROW, getAllUsersById(getState())), + narrow: apiNarrowOfNarrow( + ALL_PRIVATE_NARROW, + getAllUsersById(getState()), + getStreamsById(getState()), + ), anchor: LAST_MESSAGE_ANCHOR, numBefore: 100, numAfter: 0, diff --git a/src/utils/narrow.js b/src/utils/narrow.js index d63bfc7d95f..a1e482f4121 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -1,7 +1,7 @@ /* @flow strict-local */ import { makeUserId } from '../api/idTypes'; -import type { ApiNarrow, Message, Outbox, UserId, UserOrBot } from '../types'; +import type { ApiNarrow, Message, Outbox, Stream, UserId, UserOrBot } from '../types'; import { normalizeRecipientsAsUserIdsSansMe, pmKeyRecipientsFromMessage, @@ -429,22 +429,28 @@ export const isConversationNarrow = (narrow: Narrow): boolean => export const apiNarrowOfNarrow = ( narrow: Narrow, allUsersById: Map, -): ApiNarrow => - caseNarrow(narrow, { - stream: streamName => [{ operator: 'stream', operand: streamName }], - topic: (streamName, topic) => [ - { operator: 'stream', operand: streamName }, + streamsById: Map, +): ApiNarrow => { + const get = (description, map: Map, key: K): V => { + const result = map.get(key); + if (result === undefined) { + throw new Error(`apiNarrowOfNarrow: missing ${description}`); + } + return result; + }; + + return caseNarrow(narrow, { + stream: (_, streamId) => [ + // TODO(server-2.1): just send stream ID instead + { operator: 'stream', operand: get('stream', streamsById, streamId).name }, + ], + topic: (_, topic, streamId) => [ + // TODO(server-2.1): just send stream ID instead + { operator: 'stream', operand: get('stream', streamsById, streamId).name }, { operator: 'topic', operand: topic }, ], pm: ids => { - const emails = []; - for (const id of ids) { - const email = allUsersById.get(id)?.email; - if (email === undefined) { - throw new Error('apiNarrowOfNarrow: missing user'); - } - emails.push(email); - } + const emails = ids.map(id => get('user', allUsersById, id).email); // TODO(server-2.1): just send IDs instead return [{ operator: 'pm-with', operand: emails.join(',') }]; }, @@ -454,6 +460,7 @@ export const apiNarrowOfNarrow = ( mentioned: () => [{ operator: 'is', operand: 'mentioned' }], allPrivate: () => [{ operator: 'is', operand: 'private' }], }); +}; /** * True just if the given message is part of the given narrow. From 80687a0b232ad419cad83c66d225b94dc8c6ae63 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 12 Jan 2022 22:27:16 -0800 Subject: [PATCH 19/24] narrow: Stop storing stream names! This is nearly NFC, because we've already eliminated all the places that we actually consumed these stream names... other than in `keyFromNarrow`. Those keys are used not only to reconstitute Narrow objects (in `parseNarrow`) but also to uniquely identify a narrow in our data structures. So there is a subtle bug this fixes: if a stream gets renamed, then before this change we could get duplicate data for it, and conversely could fail to find an old draft. We leave a few things to clean up later in larger, boring, NFC commits, in order to keep this interesting change tightly focused: * The constructors streamNarrow and topicNarrow still accept a "stream name" argument, but they ignore it and allow it to be void. * The streamNameOfNarrow function still exists, but returns void. All its call sites have already been eliminated, other than for passing the result directly back to a Narrow constructor. * The `caseNarrow*` functions still pass a "stream name" argument to the `stream` and `topic` callbacks, but it's always void. All callers have already switched to ignoring that argument in favor of the stream ID, other than the few we switch over in this commit. Fixes: #4333 --- src/notification/index.js | 8 +- src/storage/__tests__/migrations-test.js | 18 +++- src/storage/migrations.js | 13 +++ src/utils/__tests__/narrow-test.js | 3 - src/utils/narrow.js | 70 +++++++-------- ...essageListElementHtml-test.js.snap.android | 90 +++++++++---------- .../messageListElementHtml-test.js.snap.ios | 90 +++++++++---------- 7 files changed, 158 insertions(+), 134 deletions(-) diff --git a/src/notification/index.js b/src/notification/index.js index 1851e0bc91a..c1a020de34a 100644 --- a/src/notification/index.js +++ b/src/notification/index.js @@ -154,10 +154,10 @@ export const getNarrowFromNotificationData = ( // message) and whatever the notification did tell us about the // stream/user: in particular, the stream name. // - // But once Narrow objects stop relying on stream names (coming soon), - // doing that will require some alternate plumbing to pass the stream - // name through. For now, we skip dealing with that; this should be an - // uncommon case, so we settle for not crashing. + // But because Narrow objects don't carry stream names, doing that will + // require some alternate plumbing to pass the stream name through. For + // now, we skip dealing with that; this should be an uncommon case, so + // we settle for not crashing. if (data.recipient_type === 'stream') { const stream = streamsByName.get(data.stream_name); diff --git a/src/storage/__tests__/migrations-test.js b/src/storage/__tests__/migrations-test.js index 2ed553e5061..a1ceeed6dc8 100644 --- a/src/storage/__tests__/migrations-test.js +++ b/src/storage/__tests__/migrations-test.js @@ -87,7 +87,7 @@ describe('migrations', () => { // What `base` becomes after all migrations. const endBase = { ...base37, - migrations: { version: 38 }, + migrations: { version: 39 }, }; for (const [desc, before, after] of [ @@ -225,6 +225,22 @@ describe('migrations', () => { }, { ...endBase, drafts: { 'pm:12': 'pm text' } }, ], + [ + 'check 39', + { + ...base37, + migrations: { version: 38 }, + drafts: { + 'topic:d:8:general\x00stuff': 'text', + 'stream:d:8:general': 'more text', + 'pm:12': 'pm text', + }, + }, + { + ...endBase, + drafts: { 'topic:8:stuff': 'text', 'stream:8': 'more text', 'pm:12': 'pm text' }, + }, + ], ]) { /* eslint-disable no-loop-func */ test(desc, async () => { diff --git a/src/storage/migrations.js b/src/storage/migrations.js index b4c2273ad61..3eaa9afb58f 100644 --- a/src/storage/migrations.js +++ b/src/storage/migrations.js @@ -362,6 +362,19 @@ const migrationsInner: {| [string]: (LessPartialState) => LessPartialState |} = ), }), + // Change format of keys representing stream/topic narrows, dropping names. + '39': state => ({ + ...state, + drafts: objectFromEntries( + Object.keys(state.drafts).map(key => [ + key + .replace(/^stream:d:(\d+):.*/s, 'stream:$1') + .replace(/^topic:d:(\d+):.*?\x00(.*)/s, 'topic:$1:$2'), + state.drafts[key], + ]), + ), + }), + // TIP: When adding a migration, consider just using `dropCache`. }; diff --git a/src/utils/__tests__/narrow-test.js b/src/utils/__tests__/narrow-test.js index 7f41ffd0dbd..595b1a028a2 100644 --- a/src/utils/__tests__/narrow-test.js +++ b/src/utils/__tests__/narrow-test.js @@ -23,7 +23,6 @@ import { STARRED_NARROW, MENTIONED_NARROW, keyFromNarrow, - streamNameOfNarrow, topicOfNarrow, caseNarrowPartial, streamIdOfNarrow, @@ -89,7 +88,6 @@ describe('streamNarrow', () => { const narrow = streamNarrow(eg.stream.name, eg.stream.stream_id); expect(isStreamNarrow(narrow)).toBeTrue(); expect(streamIdOfNarrow(narrow)).toEqual(eg.stream.stream_id); - expect(streamNameOfNarrow(narrow)).toEqual(eg.stream.name); }); test('only narrow with operator of "stream" is a stream narrow', () => { @@ -104,7 +102,6 @@ describe('topicNarrow', () => { const narrow = topicNarrow(eg.stream.name, eg.stream.stream_id, 'some topic'); expect(isTopicNarrow(narrow)).toBeTrue(); expect(streamIdOfNarrow(narrow)).toEqual(eg.stream.stream_id); - expect(streamNameOfNarrow(narrow)).toEqual(eg.stream.name); expect(topicOfNarrow(narrow)).toEqual('some topic'); }); diff --git a/src/utils/narrow.js b/src/utils/narrow.js index a1e482f4121..cd782dfdef6 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -34,8 +34,8 @@ import { * server, and `apiNarrowOfNarrow` for converting to it. */ export opaque type Narrow = - | {| type: 'stream', streamName: string, streamId: number |} - | {| type: 'topic', streamName: string, streamId: number, topic: string |} + | {| type: 'stream', streamId: number |} + | {| type: 'topic', streamId: number, topic: string |} | {| type: 'pm', userIds: PmKeyRecipients |} | {| type: 'search', query: string |} | {| type: 'all' | 'starred' | 'mentioned' | 'all-pm' |}; @@ -148,11 +148,14 @@ export const ALL_PRIVATE_NARROW: Narrow = specialNarrow('private'); export const ALL_PRIVATE_NARROW_STR: string = keyFromNarrow(ALL_PRIVATE_NARROW); -export const streamNarrow = (streamName: string, streamId: number): Narrow => - Object.freeze({ type: 'stream', streamName, streamId }); +export const streamNarrow = (streamNameIgnored: string | void, streamId: number): Narrow => + Object.freeze({ type: 'stream', streamId }); -export const topicNarrow = (streamName: string, streamId: number, topic: string): Narrow => - Object.freeze({ type: 'topic', streamName, streamId, topic }); +export const topicNarrow = ( + streamNameIgnored: string | void, + streamId: number, + topic: string, +): Narrow => Object.freeze({ type: 'topic', streamId, topic }); export const SEARCH_NARROW = (query: string): Narrow => Object.freeze({ type: 'search', query }); @@ -162,8 +165,8 @@ type NarrowCases = {| starred: () => T, mentioned: () => T, allPrivate: () => T, - stream: (name: string, streamId: number) => T, - topic: (streamName: string, topic: string, streamId: number) => T, + stream: (name: void, streamId: number) => T, + topic: (streamName: void, topic: string, streamId: number) => T, search: (query: string) => T, |}; @@ -174,8 +177,8 @@ export function caseNarrow(narrow: Narrow, cases: NarrowCases): T { }; switch (narrow.type) { - case 'stream': return cases.stream(narrow.streamName, narrow.streamId); - case 'topic': return cases.topic(narrow.streamName, narrow.topic, narrow.streamId); + case 'stream': return cases.stream(undefined, narrow.streamId); + case 'topic': return cases.topic(undefined, narrow.topic, narrow.streamId); case 'pm': return cases.pm(narrow.userIds); case 'search': return cases.search(narrow.query); case 'all': return cases.home(); @@ -241,27 +244,25 @@ export function caseNarrowDefault( * channel (like an HTML attribute) that only allows certain codepoints, use * something like `base64Utf8Encode` to encode into the permitted subset. */ -// The arbitrary Unicode codepoints come from (a) stream names and topics, -// and (b) our use of `\x00` as a delimiter. +// The arbitrary Unicode codepoints come from topics. (These aren't +// *completely* arbitrary, but close enough that there's little to gain +// from relying on constraints on them.) export function keyFromNarrow(narrow: Narrow): string { - // 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. + // The ":s" (for "string") in several of these was to keep them disjoint, + // out of an abundance of caution, from later keys that use numeric IDs. // - // Similarly, ":d" ("dual") marks those with both numeric IDs and strings. + // Similarly, ":d" ("dual") marked 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. - // An earlier version had `stream:s:`. - stream: (name, streamId) => `stream:d:${streamId}:${name}`, + // An earlier version had `stream:s:`. Another had `stream:d:`. + stream: (_, streamId) => `stream:${streamId}`, - // An earlier version had `topic:s:`. - // '\x00' is the one character not allowed in Zulip stream names. - // (See `check_stream_name` in zulip.git:zerver/lib/streams.py.) - topic: (streamName, topic, streamId) => `topic:d:${streamId}:${streamName}\x00${topic}`, + // An earlier version had `topic:s:`. Another had `topic:d:`. + topic: (_, topic, streamId) => `topic:${streamId}:${topic}`, - // An earlier version had `pm:s:`. - // Another had `pm:d:`. + // An earlier version had `pm:s:`. Another had `pm:d:`. pm: ids => `pm:${ids.join(',')}`, home: () => 'all', @@ -283,25 +284,21 @@ export const parseNarrow = (narrowStr: string): Narrow => { // invariant: tag + rest === narrowStr switch (tag) { case 'stream:': { - // The `/s` regexp flag means the `.` pattern matches absolutely - // anything. By default it rejects certain "newline" characters, - // which in principle could appear in stream names. - const match = /^d:(\d+):(.*)/s.exec(rest); - if (!match) { + if (!/^\d+$/.test(rest)) { throw makeError(); } - return streamNarrow(match[2], Number.parseInt(match[1], 10)); + return streamNarrow(undefined, Number.parseInt(rest, 10)); } case 'topic:': { - // The `/s` regexp flag means the `.` patterns match absolutely - // anything. By default they reject certain "newline" characters, - // which in principle could appear in stream names or topics. - const match = /^d:(\d+):(.*?)\x00(.*)/s.exec(rest); + // The `/s` regexp flag means the `.` pattern matches absolutely + // anything. By default it rejects certain "newline" characters, + // which in principle could appear in topics. + const match = /^(\d+):(.*)/s.exec(rest); if (!match) { throw makeError(); } - return topicNarrow(match[2], Number.parseInt(match[1], 10), match[3]); + return topicNarrow(undefined, Number.parseInt(match[1], 10), match[2]); } case 'pm:': { @@ -360,8 +357,9 @@ export const userIdsOfPmNarrow = (narrow: Narrow): PmKeyRecipients => * instead of a Narrow in the first place; or if they do handle other kinds * of narrows, should be using `caseNarrow`. */ -export const streamNameOfNarrow = (narrow: Narrow): string => - caseNarrowPartial(narrow, { stream: name => name, topic: streamName => streamName }); +// TODO delete +export const streamNameOfNarrow = (narrow: Narrow): void => + caseNarrowPartial(narrow, { stream: () => undefined, topic: () => undefined }); /** * The stream ID for a stream or topic narrow; else error. diff --git a/src/webview/html/__tests__/__snapshots__/messageListElementHtml-test.js.snap.android b/src/webview/html/__tests__/__snapshots__/messageListElementHtml-test.js.snap.android index 0370b7b0217..5c4d809a112 100644 --- a/src/webview/html/__tests__/__snapshots__/messageListElementHtml-test.js.snap.android +++ b/src/webview/html/__tests__/__snapshots__/messageListElementHtml-test.js.snap.android @@ -10,11 +10,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -88,11 +88,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -171,11 +171,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -214,11 +214,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAy\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 2
@@ -267,11 +267,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -310,11 +310,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6Mjp0b3BpYyAy\\">
+ data-narrow=\\"c3RyZWFtOjI=\\"> # stream 2
topic 2
@@ -363,11 +363,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -910,11 +910,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -1056,11 +1056,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -1099,11 +1099,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAy\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 2
@@ -1402,11 +1402,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -1445,11 +1445,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6Mjp0b3BpYyAy\\">
+ data-narrow=\\"c3RyZWFtOjI=\\"> # stream 2
topic 2
@@ -1488,11 +1488,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -1622,11 +1622,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -3373,7 +3373,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible pm:1,2 " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:d:1:stream 1 1`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:1 1`] = ` "
@@ -3383,7 +3383,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3447,7 +3447,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:d:1:stream 1 2`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:1 2`] = ` "
@@ -3457,7 +3457,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3526,7 +3526,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:d:1:stream 1 3`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:1 3`] = ` "
@@ -3536,7 +3536,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3575,7 +3575,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 2
@@ -3614,7 +3614,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:d:1:stream 1 4`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:1 4`] = ` "
@@ -3624,7 +3624,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3699,7 +3699,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:d:1:stream 1 5`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:1 5`] = ` "
@@ -3709,7 +3709,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3809,7 +3809,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 2
@@ -3854,7 +3854,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3989,7 +3989,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:d:1:stream 1topic 1 1`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:1:topic 1 1`] = ` "
@@ -4054,7 +4054,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible topic: " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:d:1:stream 1topic 1 2`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:1:topic 1 2`] = ` "
@@ -4124,7 +4124,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible topic: " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:d:1:stream 1topic 1 3`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:1:topic 1 3`] = ` "
@@ -4200,7 +4200,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible topic: " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:d:1:stream 1topic 1 4`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:1:topic 1 4`] = ` "
diff --git a/src/webview/html/__tests__/__snapshots__/messageListElementHtml-test.js.snap.ios b/src/webview/html/__tests__/__snapshots__/messageListElementHtml-test.js.snap.ios index 0370b7b0217..5c4d809a112 100644 --- a/src/webview/html/__tests__/__snapshots__/messageListElementHtml-test.js.snap.ios +++ b/src/webview/html/__tests__/__snapshots__/messageListElementHtml-test.js.snap.ios @@ -10,11 +10,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -88,11 +88,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -171,11 +171,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -214,11 +214,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAy\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 2
@@ -267,11 +267,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -310,11 +310,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6Mjp0b3BpYyAy\\">
+ data-narrow=\\"c3RyZWFtOjI=\\"> # stream 2
topic 2
@@ -363,11 +363,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -910,11 +910,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -1056,11 +1056,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -1099,11 +1099,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAy\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 2
@@ -1402,11 +1402,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -1445,11 +1445,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6Mjp0b3BpYyAy\\">
+ data-narrow=\\"c3RyZWFtOjI=\\"> # stream 2
topic 2
@@ -1488,11 +1488,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -1622,11 +1622,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -3373,7 +3373,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible pm:1,2 " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:d:1:stream 1 1`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:1 1`] = ` "
@@ -3383,7 +3383,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3447,7 +3447,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:d:1:stream 1 2`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:1 2`] = ` "
@@ -3457,7 +3457,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3526,7 +3526,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:d:1:stream 1 3`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:1 3`] = ` "
@@ -3536,7 +3536,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3575,7 +3575,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 2
@@ -3614,7 +3614,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:d:1:stream 1 4`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:1 4`] = ` "
@@ -3624,7 +3624,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3699,7 +3699,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:d:1:stream 1 5`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:1 5`] = ` "
@@ -3709,7 +3709,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3809,7 +3809,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 2
@@ -3854,7 +3854,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3989,7 +3989,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:d:1:stream 1topic 1 1`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:1:topic 1 1`] = ` "
@@ -4054,7 +4054,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible topic: " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:d:1:stream 1topic 1 2`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:1:topic 1 2`] = ` "
@@ -4124,7 +4124,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible topic: " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:d:1:stream 1topic 1 3`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:1:topic 1 3`] = ` "
@@ -4200,7 +4200,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible topic: " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:d:1:stream 1topic 1 4`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:1:topic 1 4`] = ` "
From 88f4825668e418b5d2cdf72b0e0e17109ebdb076 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 29 Dec 2021 14:32:51 -0800 Subject: [PATCH 20/24] narrow [nfc]: Drop stream names from caseNarrow callbacks We cover all the same call sites that were switched over to using stream IDs in the preceding few commits, which followed a manual audit of all `caseNarrow*` call sites. --- src/chat/ChatScreen.js | 4 +--- src/chat/narrowsSelectors.js | 4 ++-- src/compose/getComposeInputPlaceholder.js | 2 +- src/outbox/outboxActions.js | 2 +- src/unread/unreadSelectors.js | 4 ++-- src/utils/narrow.js | 24 +++++++++++------------ 6 files changed, 19 insertions(+), 21 deletions(-) diff --git a/src/chat/ChatScreen.js b/src/chat/ChatScreen.js index 6c62bf0b52e..11ad875efb6 100644 --- a/src/chat/ChatScreen.js +++ b/src/chat/ChatScreen.js @@ -138,9 +138,7 @@ export default function ChatScreen(props: Props): Node { const content = editMessage.content !== message ? message : undefined; const subject = caseNarrowDefault( destinationNarrow, - { - topic: (_1, topic, streamId) => (topic !== editMessage.topic ? topic : undefined), - }, + { topic: (streamId, topic) => (topic !== editMessage.topic ? topic : undefined) }, () => undefined, ); diff --git a/src/chat/narrowsSelectors.js b/src/chat/narrowsSelectors.js index a1db21a74a4..8aa78a6dd62 100644 --- a/src/chat/narrowsSelectors.js +++ b/src/chat/narrowsSelectors.js @@ -229,8 +229,8 @@ export const isNarrowValid: Selector = createSelector( caseNarrowDefault( narrow, { - stream: (_, streamId) => streams.get(streamId) !== undefined, - topic: (_, t, streamId) => streams.get(streamId) !== undefined, + stream: streamId => streams.get(streamId) !== undefined, + topic: streamId => streams.get(streamId) !== undefined, pm: ids => ids.every(id => allUsersById.get(id) !== undefined), }, () => true, diff --git a/src/compose/getComposeInputPlaceholder.js b/src/compose/getComposeInputPlaceholder.js index d0bd0b9b7a6..2211cab8af8 100644 --- a/src/compose/getComposeInputPlaceholder.js +++ b/src/compose/getComposeInputPlaceholder.js @@ -31,7 +31,7 @@ export default ( values: { recipient: `@${user.full_name}` }, }; }, - stream: (_, streamId) => { + stream: streamId => { const stream = streamsById.get(streamId); if (!stream) { return { text: 'Type a message' }; diff --git a/src/outbox/outboxActions.js b/src/outbox/outboxActions.js index 43cc8c01637..c3616d14738 100644 --- a/src/outbox/outboxActions.js +++ b/src/outbox/outboxActions.js @@ -144,7 +144,7 @@ const outboxPropertiesForNarrow = ( subject: '', }), - topic: (_, topic, streamId) => { + topic: (streamId, topic) => { const stream = streamsById.get(streamId); invariant(stream, 'narrow must be known stream'); return { diff --git a/src/unread/unreadSelectors.js b/src/unread/unreadSelectors.js index c55375da4c0..6a482df707a 100644 --- a/src/unread/unreadSelectors.js +++ b/src/unread/unreadSelectors.js @@ -231,7 +231,7 @@ export const getUnreadCountForNarrow: Selector = createSelector( caseNarrow(narrow, { home: () => unreadTotal, - stream: (_1, streamId) => { + stream: streamId => { const stream = streams.get(streamId); if (!stream) { return 0; @@ -248,7 +248,7 @@ export const getUnreadCountForNarrow: Selector = createSelector( ); }, - topic: (_, topic, streamId) => getUnreadCountForTopic(unread, streamId, topic), + topic: (streamId, topic) => getUnreadCountForTopic(unread, streamId, topic), pm: _ => getUnreadIdsForPmNarrow(unread, narrow, ownUserId).length, diff --git a/src/utils/narrow.js b/src/utils/narrow.js index cd782dfdef6..184bc6e8f25 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -165,8 +165,8 @@ type NarrowCases = {| starred: () => T, mentioned: () => T, allPrivate: () => T, - stream: (name: void, streamId: number) => T, - topic: (streamName: void, topic: string, streamId: number) => T, + stream: (streamId: number) => T, + topic: (streamId: number, topic: string) => T, search: (query: string) => T, |}; @@ -177,8 +177,8 @@ export function caseNarrow(narrow: Narrow, cases: NarrowCases): T { }; switch (narrow.type) { - case 'stream': return cases.stream(undefined, narrow.streamId); - case 'topic': return cases.topic(undefined, narrow.topic, narrow.streamId); + case 'stream': return cases.stream(narrow.streamId); + case 'topic': return cases.topic(narrow.streamId, narrow.topic); case 'pm': return cases.pm(narrow.userIds); case 'search': return cases.search(narrow.query); case 'all': return cases.home(); @@ -257,10 +257,10 @@ export function keyFromNarrow(narrow: Narrow): string { // Take a close look at migration 19 and any later related migrations. // An earlier version had `stream:s:`. Another had `stream:d:`. - stream: (_, streamId) => `stream:${streamId}`, + stream: streamId => `stream:${streamId}`, // An earlier version had `topic:s:`. Another had `topic:d:`. - topic: (_, topic, streamId) => `topic:${streamId}:${topic}`, + topic: (streamId, topic) => `topic:${streamId}:${topic}`, // An earlier version had `pm:s:`. Another had `pm:d:`. pm: ids => `pm:${ids.join(',')}`, @@ -369,7 +369,7 @@ export const streamNameOfNarrow = (narrow: Narrow): void => * of narrows, should be using `caseNarrow`. */ export const streamIdOfNarrow = (narrow: Narrow): number => - caseNarrowPartial(narrow, { stream: (n, id) => id, topic: (n, t, id) => id }); + caseNarrowPartial(narrow, { stream: id => id, topic: id => id }); /** * The topic for a topic narrow; else error. @@ -379,7 +379,7 @@ export const streamIdOfNarrow = (narrow: Narrow): number => * other kinds of narrows, should be using `caseNarrow`. */ export const topicOfNarrow = (narrow: Narrow): string => - caseNarrowPartial(narrow, { topic: (streamName, topic) => topic }); + caseNarrowPartial(narrow, { topic: (id, topic) => topic }); export const isPmNarrow = (narrow?: Narrow): boolean => !!narrow && caseNarrowDefault(narrow, { pm: () => true }, () => false); @@ -438,11 +438,11 @@ export const apiNarrowOfNarrow = ( }; return caseNarrow(narrow, { - stream: (_, streamId) => [ + stream: streamId => [ // TODO(server-2.1): just send stream ID instead { operator: 'stream', operand: get('stream', streamsById, streamId).name }, ], - topic: (_, topic, streamId) => [ + topic: (streamId, topic) => [ // TODO(server-2.1): just send stream ID instead { operator: 'stream', operand: get('stream', streamsById, streamId).name }, { operator: 'topic', operand: topic }, @@ -482,8 +482,8 @@ export const isMessageInNarrow = ( ): boolean => caseNarrow(narrow, { home: () => true, - stream: (_, streamId) => message.type === 'stream' && streamId === message.stream_id, - topic: (_, topic, streamId) => + stream: streamId => message.type === 'stream' && streamId === message.stream_id, + topic: (streamId, topic) => message.type === 'stream' && streamId === message.stream_id && topic === message.subject, pm: ids => { if (message.type !== 'private') { From e58b62d3d8be153589137993185e8228f16eb5bd Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 12 Jan 2022 22:31:29 -0800 Subject: [PATCH 21/24] narrow [nfc]: Delete now-vacuous streamNameOfNarrow --- src/compose/ComposeBox.js | 3 +-- src/nav/ChatNavBar.js | 9 ++------- src/utils/narrow.js | 11 ----------- 3 files changed, 3 insertions(+), 20 deletions(-) diff --git a/src/compose/ComposeBox.js b/src/compose/ComposeBox.js index 10a5cec5537..470ba2dc44d 100644 --- a/src/compose/ComposeBox.js +++ b/src/compose/ComposeBox.js @@ -36,7 +36,6 @@ import { isStreamOrTopicNarrow, isTopicNarrow, streamIdOfNarrow, - streamNameOfNarrow, topicNarrow, topicOfNarrow, } from '../utils/narrow'; @@ -406,7 +405,7 @@ class ComposeBoxInner extends PureComponent { if (isStreamNarrow(narrow) || (isTopicNarrow(narrow) && isEditing)) { const streamId = streamIdOfNarrow(narrow); const topic = this.state.topic.trim() || apiConstants.NO_TOPIC_TOPIC; - return topicNarrow(streamNameOfNarrow(narrow), streamId, topic); + return topicNarrow(undefined, streamId, topic); } invariant(isConversationNarrow(narrow), 'destination narrow must be conversation'); return narrow; diff --git a/src/nav/ChatNavBar.js b/src/nav/ChatNavBar.js index ceb8b3350c4..58463254f01 100644 --- a/src/nav/ChatNavBar.js +++ b/src/nav/ChatNavBar.js @@ -14,12 +14,7 @@ import Title from '../title/Title'; import NavBarBackButton from './NavBarBackButton'; import { getStreamColorForNarrow } from '../subscriptions/subscriptionSelectors'; import { foregroundColorFromBackground } from '../utils/color'; -import { - caseNarrowDefault, - streamIdOfNarrow, - streamNameOfNarrow, - streamNarrow, -} from '../utils/narrow'; +import { caseNarrowDefault, streamIdOfNarrow, streamNarrow } from '../utils/narrow'; import { navigateToStream, navigateToAccountDetails, @@ -55,7 +50,7 @@ function ExtraNavButtonTopic(props): Node { const dispatch = useDispatch(); const handlePress = useCallback(() => { - dispatch(doNarrow(streamNarrow(streamNameOfNarrow(narrow), streamIdOfNarrow(narrow)))); + dispatch(doNarrow(streamNarrow(undefined, streamIdOfNarrow(narrow)))); }, [dispatch, narrow]); return ; diff --git a/src/utils/narrow.js b/src/utils/narrow.js index 184bc6e8f25..50a8f1c52f0 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -350,17 +350,6 @@ export const isGroupPmNarrow = (narrow?: Narrow): boolean => export const userIdsOfPmNarrow = (narrow: Narrow): PmKeyRecipients => caseNarrowPartial(narrow, { pm: ids => ids }); -/** - * The stream name for a stream or topic narrow; else error. - * - * Most callers of this should probably be getting passed a stream name - * instead of a Narrow in the first place; or if they do handle other kinds - * of narrows, should be using `caseNarrow`. - */ -// TODO delete -export const streamNameOfNarrow = (narrow: Narrow): void => - caseNarrowPartial(narrow, { stream: () => undefined, topic: () => undefined }); - /** * The stream ID for a stream or topic narrow; else error. * From 3d1b77e21cc53d83cff80e8e56658e634e1bf9eb Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 12 Jan 2022 22:36:48 -0800 Subject: [PATCH 22/24] narrow [nfc]: Stop taking stream name in constructors After editing the constructor definitions themselves, the call sites were updated mostly automatically: $ perl -i -0pe ' s/\b (streamNarrow|topicNarrow) \( \K .*? , \s*//xsg ' src/**/*.js $ tools/fmt with manual fixup just in internalLinks.js, plus deleting a handful of unused variables pointed out by lint. --- src/chat/__tests__/fetchingReducer-test.js | 2 +- src/chat/__tests__/narrowsReducer-test.js | 4 +- src/chat/__tests__/narrowsSelectors-test.js | 18 +++--- src/compose/ComposeBox.js | 2 +- .../getComposeInputPlaceholder-test.js | 4 +- src/drafts/__tests__/draftsReducer-test.js | 2 +- src/drafts/__tests__/draftsSelectors-test.js | 9 +-- src/message/__tests__/fetchActions-test.js | 2 +- src/message/__tests__/messageActions-test.js | 2 +- src/message/__tests__/scrollStrategy-test.js | 4 +- src/nav/ChatNavBar.js | 2 +- .../__tests__/notification-test.js | 2 +- src/notification/index.js | 2 +- src/sharing/ShareToStream.js | 6 +- src/sharing/ShareWrapper.js | 4 +- src/streams/SubscriptionsCard.js | 2 +- src/subscriptions/StreamListCard.js | 2 +- .../__tests__/subscriptionSelectors-test.js | 12 ++-- src/topics/TopicListScreen.js | 2 +- src/topics/__tests__/topicsSelectors-test.js | 2 +- src/unread/UnreadCards.js | 4 +- src/utils/__tests__/internalLinks-test.js | 30 +++++----- src/utils/__tests__/narrow-test.js | 58 +++++++------------ src/utils/internalLinks.js | 5 +- src/utils/narrow.js | 22 +++---- .../__tests__/messageListElementHtml-test.js | 4 +- src/webview/html/header.js | 6 +- 27 files changed, 92 insertions(+), 122 deletions(-) diff --git a/src/chat/__tests__/fetchingReducer-test.js b/src/chat/__tests__/fetchingReducer-test.js index c33a2059955..2cff2ba1145 100644 --- a/src/chat/__tests__/fetchingReducer-test.js +++ b/src/chat/__tests__/fetchingReducer-test.js @@ -41,7 +41,7 @@ describe('fetchingReducer', () => { [HOME_NARROW_STR]: { older: false, newer: false }, }); - const narrow = streamNarrow(eg.stream.name, eg.stream.stream_id); + const narrow = streamNarrow(eg.stream.stream_id); const action = deepFreeze({ type: MESSAGE_FETCH_START, narrow, diff --git a/src/chat/__tests__/narrowsReducer-test.js b/src/chat/__tests__/narrowsReducer-test.js index 8ba6af89949..822ebcfc938 100644 --- a/src/chat/__tests__/narrowsReducer-test.js +++ b/src/chat/__tests__/narrowsReducer-test.js @@ -27,9 +27,9 @@ import * as eg from '../../__tests__/lib/exampleData'; describe('narrowsReducer', () => { const privateNarrowStr = keyFromNarrow(pm1to1NarrowFromUser(eg.otherUser)); const groupNarrowStr = keyFromNarrow(pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser])); - const streamNarrowStr = keyFromNarrow(streamNarrow(eg.stream.name, eg.stream.stream_id)); + const streamNarrowStr = keyFromNarrow(streamNarrow(eg.stream.stream_id)); const egTopic = eg.streamMessage().subject; - const topicNarrowStr = keyFromNarrow(topicNarrow(eg.stream.name, eg.stream.stream_id, egTopic)); + const topicNarrowStr = keyFromNarrow(topicNarrow(eg.stream.stream_id, egTopic)); describe('EVENT_NEW_MESSAGE', () => { test('if not caught up in narrow, do not add message in home narrow', () => { diff --git a/src/chat/__tests__/narrowsSelectors-test.js b/src/chat/__tests__/narrowsSelectors-test.js index fdf9a503543..690acef7c9d 100644 --- a/src/chat/__tests__/narrowsSelectors-test.js +++ b/src/chat/__tests__/narrowsSelectors-test.js @@ -146,7 +146,7 @@ describe('getShownMessagesForNarrow', () => { }); describe('stream narrow', () => { - const narrow = streamNarrow(stream.name, stream.stream_id); + const narrow = streamNarrow(stream.stream_id); const makeState = extra => makeStateGeneral(message, narrow, extra); const shown = state => shownGeneral(state, narrow); @@ -171,7 +171,7 @@ describe('getShownMessagesForNarrow', () => { }); describe('topic narrow', () => { - const narrow = topicNarrow(stream.name, stream.stream_id, message.subject); + const narrow = topicNarrow(stream.stream_id, message.subject); const makeState = extra => makeStateGeneral(message, narrow, extra); const shown = state => shownGeneral(state, narrow); @@ -306,26 +306,26 @@ describe('getStreamInNarrow', () => { }); test('return subscription if stream in narrow is subscribed', () => { - const narrow = streamNarrow(stream1.name, stream1.stream_id); + const narrow = streamNarrow(stream1.stream_id); expect(getStreamInNarrow(state, narrow)).toEqual(sub1); }); test('return stream if stream in narrow is not subscribed', () => { - const narrow = streamNarrow(stream3.name, stream3.stream_id); + const narrow = streamNarrow(stream3.stream_id); expect(getStreamInNarrow(state, narrow)).toEqual({ ...stream3, in_home_view: true }); }); test('return NULL_SUBSCRIPTION if stream in narrow is not valid', () => { - const narrow = streamNarrow(stream4.name, stream4.stream_id); + const narrow = streamNarrow(stream4.stream_id); expect(getStreamInNarrow(state, narrow)).toEqual(NULL_SUBSCRIPTION); }); test('return NULL_SUBSCRIPTION is narrow is not topic or stream', () => { expect(getStreamInNarrow(state, pm1to1NarrowFromUser(eg.otherUser))).toEqual(NULL_SUBSCRIPTION); - expect(getStreamInNarrow(state, topicNarrow(stream4.name, stream4.stream_id, 'topic'))).toEqual( + expect(getStreamInNarrow(state, topicNarrow(stream4.stream_id, 'topic'))).toEqual( NULL_SUBSCRIPTION, ); }); @@ -347,7 +347,7 @@ describe('isNarrowValid', () => { const state = eg.reduxState({ streams: [stream], }); - const narrow = streamNarrow(stream.name, stream.stream_id); + const narrow = streamNarrow(stream.stream_id); const result = isNarrowValid(state, narrow); @@ -358,7 +358,7 @@ describe('isNarrowValid', () => { const state = eg.reduxState({ streams: [], }); - const narrow = streamNarrow(eg.stream.name, eg.stream.stream_id); + const narrow = streamNarrow(eg.stream.stream_id); const result = isNarrowValid(state, narrow); @@ -371,7 +371,7 @@ describe('isNarrowValid', () => { const state = eg.reduxState({ streams: [stream], }); - const narrow = topicNarrow(stream.name, stream.stream_id, 'topic does not matter'); + const narrow = topicNarrow(stream.stream_id, 'topic does not matter'); const result = isNarrowValid(state, narrow); diff --git a/src/compose/ComposeBox.js b/src/compose/ComposeBox.js index 470ba2dc44d..40160457be3 100644 --- a/src/compose/ComposeBox.js +++ b/src/compose/ComposeBox.js @@ -405,7 +405,7 @@ class ComposeBoxInner extends PureComponent { if (isStreamNarrow(narrow) || (isTopicNarrow(narrow) && isEditing)) { const streamId = streamIdOfNarrow(narrow); const topic = this.state.topic.trim() || apiConstants.NO_TOPIC_TOPIC; - return topicNarrow(undefined, streamId, topic); + return topicNarrow(streamId, topic); } invariant(isConversationNarrow(narrow), 'destination narrow must be conversation'); return narrow; diff --git a/src/compose/__tests__/getComposeInputPlaceholder-test.js b/src/compose/__tests__/getComposeInputPlaceholder-test.js index 7bcb39cb7bc..61351dc8ad6 100644 --- a/src/compose/__tests__/getComposeInputPlaceholder-test.js +++ b/src/compose/__tests__/getComposeInputPlaceholder-test.js @@ -32,7 +32,7 @@ describe('getComposeInputPlaceholder', () => { }); test('returns "Message #streamName" for stream narrow', () => { - const narrow = deepFreeze(streamNarrow(eg.stream.name, eg.stream.stream_id)); + const narrow = deepFreeze(streamNarrow(eg.stream.stream_id)); const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById, streamsById); expect(placeholder).toEqual({ text: 'Message {recipient}', @@ -41,7 +41,7 @@ describe('getComposeInputPlaceholder', () => { }); test('returns properly for topic narrow', () => { - const narrow = deepFreeze(topicNarrow(eg.stream.name, eg.stream.stream_id, 'Copenhagen')); + const narrow = deepFreeze(topicNarrow(eg.stream.stream_id, 'Copenhagen')); const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById, streamsById); expect(placeholder).toEqual({ text: 'Reply' }); }); diff --git a/src/drafts/__tests__/draftsReducer-test.js b/src/drafts/__tests__/draftsReducer-test.js index d01dd8ebed2..d3a70d993d6 100644 --- a/src/drafts/__tests__/draftsReducer-test.js +++ b/src/drafts/__tests__/draftsReducer-test.js @@ -8,7 +8,7 @@ import { topicNarrow, keyFromNarrow } from '../../utils/narrow'; import * as eg from '../../__tests__/lib/exampleData'; describe('draftsReducer', () => { - const testNarrow = topicNarrow(eg.stream.name, eg.stream.stream_id, 'denmark2'); + const testNarrow = topicNarrow(eg.stream.stream_id, 'denmark2'); const testNarrowStr = keyFromNarrow(testNarrow); describe('DRAFT_UPDATE', () => { diff --git a/src/drafts/__tests__/draftsSelectors-test.js b/src/drafts/__tests__/draftsSelectors-test.js index 3a85fa00d8b..efe7d2bff12 100644 --- a/src/drafts/__tests__/draftsSelectors-test.js +++ b/src/drafts/__tests__/draftsSelectors-test.js @@ -6,7 +6,7 @@ import * as eg from '../../__tests__/lib/exampleData'; describe('getDraftForNarrow', () => { test('return content of draft if exists', () => { - const narrow = topicNarrow(eg.stream.name, eg.stream.stream_id, 'topic'); + const narrow = topicNarrow(eg.stream.stream_id, 'topic'); const state = eg.reduxState({ drafts: { [keyFromNarrow(narrow)]: 'content', @@ -19,17 +19,14 @@ describe('getDraftForNarrow', () => { }); test('return empty string if not exists', () => { - const narrow = topicNarrow(eg.stream.name, eg.stream.stream_id, 'topic'); + const narrow = topicNarrow(eg.stream.stream_id, 'topic'); const state = eg.reduxState({ drafts: { [keyFromNarrow(narrow)]: 'content', }, }); - const draft = getDraftForNarrow( - state, - topicNarrow(eg.stream.name, eg.stream.stream_id, 'topic1'), - ); + const draft = getDraftForNarrow(state, topicNarrow(eg.stream.stream_id, 'topic1')); expect(draft).toEqual(''); }); diff --git a/src/message/__tests__/fetchActions-test.js b/src/message/__tests__/fetchActions-test.js index 7bf5d47630c..7a5d3f8a52b 100644 --- a/src/message/__tests__/fetchActions-test.js +++ b/src/message/__tests__/fetchActions-test.js @@ -26,7 +26,7 @@ import * as logging from '../../utils/logging'; const mockStore = configureStore([thunk]); -const narrow = streamNarrow(eg.stream.name, eg.stream.stream_id); +const narrow = streamNarrow(eg.stream.stream_id); const streamNarrowStr = keyFromNarrow(narrow); global.FormData = class FormData {}; diff --git a/src/message/__tests__/messageActions-test.js b/src/message/__tests__/messageActions-test.js index f660f0ead51..1b9abe778f5 100644 --- a/src/message/__tests__/messageActions-test.js +++ b/src/message/__tests__/messageActions-test.js @@ -13,7 +13,7 @@ import type { Action } from '../../actionTypes'; const mockStore = configureStore([thunk]); -const streamNarrowObj = streamNarrow(eg.stream.name, eg.stream.stream_id); +const streamNarrowObj = streamNarrow(eg.stream.stream_id); describe('messageActions', () => { describe('doNarrow', () => { diff --git a/src/message/__tests__/scrollStrategy-test.js b/src/message/__tests__/scrollStrategy-test.js index 3a299f297bd..cac58da74af 100644 --- a/src/message/__tests__/scrollStrategy-test.js +++ b/src/message/__tests__/scrollStrategy-test.js @@ -4,9 +4,9 @@ import { getScrollStrategy } from '../scrollStrategy'; import * as eg from '../../__tests__/lib/exampleData'; -const someNarrow = streamNarrow(eg.stream.name, eg.stream.stream_id); +const someNarrow = streamNarrow(eg.stream.stream_id); const anotherStream = eg.makeStream(); -const anotherNarrow = streamNarrow(anotherStream.name, anotherStream.stream_id); +const anotherNarrow = streamNarrow(anotherStream.stream_id); describe('getScrollStrategy', () => { test('initial load positions at anchor (first unread)', () => { diff --git a/src/nav/ChatNavBar.js b/src/nav/ChatNavBar.js index 58463254f01..6585cf61327 100644 --- a/src/nav/ChatNavBar.js +++ b/src/nav/ChatNavBar.js @@ -50,7 +50,7 @@ function ExtraNavButtonTopic(props): Node { const dispatch = useDispatch(); const handlePress = useCallback(() => { - dispatch(doNarrow(streamNarrow(undefined, streamIdOfNarrow(narrow)))); + dispatch(doNarrow(streamNarrow(streamIdOfNarrow(narrow)))); }, [dispatch, narrow]); return ; diff --git a/src/notification/__tests__/notification-test.js b/src/notification/__tests__/notification-test.js index cf0895882d5..6897f22c76e 100644 --- a/src/notification/__tests__/notification-test.js +++ b/src/notification/__tests__/notification-test.js @@ -34,7 +34,7 @@ describe('getNarrowFromNotificationData', () => { topic: 'some topic', }; const narrow = getNarrowFromNotificationData(notification, new Map(), streamsByName, ownUserId); - expect(narrow).toEqual(topicNarrow(stream.name, stream.stream_id, 'some topic')); + expect(narrow).toEqual(topicNarrow(stream.stream_id, 'some topic')); }); test('on notification for a private message returns a PM narrow', () => { diff --git a/src/notification/index.js b/src/notification/index.js index c1a020de34a..3fce46b6b94 100644 --- a/src/notification/index.js +++ b/src/notification/index.js @@ -161,7 +161,7 @@ export const getNarrowFromNotificationData = ( if (data.recipient_type === 'stream') { const stream = streamsByName.get(data.stream_name); - return (stream && topicNarrow(stream.name, stream.stream_id, data.topic)) ?? null; + return (stream && topicNarrow(stream.stream_id, data.topic)) ?? null; } if (data.pm_users === undefined) { diff --git a/src/sharing/ShareToStream.js b/src/sharing/ShareToStream.js index 68856ee74ba..2ec0ce047c2 100644 --- a/src/sharing/ShareToStream.js +++ b/src/sharing/ShareToStream.js @@ -74,14 +74,14 @@ class ShareToStreamInner extends React.Component { }; focusTopic = () => { - const { streamName, streamId } = this.state; + const { streamId } = this.state; const { dispatch } = this.props; if (streamId !== null) { // We have an actual stream selected. Fetch its recent topic names. // TODO: why do we do this? `TopicAutocomplete` does the same thing, // with an effect that reruns when the stream changes. - const narrow = streamNarrow(streamName, streamId); + const narrow = streamNarrow(streamId); dispatch(fetchTopicsForStream(narrow)); } // If what's entered in the stream-name field isn't an actual stream, @@ -147,7 +147,7 @@ class ShareToStreamInner extends React.Component { render() { const { sharedData } = this.props.route.params; const { streamName, streamId, topic, isStreamFocused, isTopicFocused } = this.state; - const narrow = streamId !== null ? streamNarrow(streamName, streamId) : null; + const narrow = streamId !== null ? streamNarrow(streamId) : null; const sendTo = { streamName, /* $FlowFixMe[incompatible-cast]: ShareWrapper will only look at this diff --git a/src/sharing/ShareWrapper.js b/src/sharing/ShareWrapper.js index 97991e8e3a0..5a32721bcf8 100644 --- a/src/sharing/ShareWrapper.js +++ b/src/sharing/ShareWrapper.js @@ -210,8 +210,8 @@ class ShareWrapperInner extends React.Component { } case 'stream': { - const { streamName, streamId } = sendTo; - const narrow = streamNarrow(streamName, streamId); + const { streamId } = sendTo; + const narrow = streamNarrow(streamId); NavigationService.dispatch(replaceWithChat(narrow)); break; } diff --git a/src/streams/SubscriptionsCard.js b/src/streams/SubscriptionsCard.js index 2a7166d4791..17e0608b763 100644 --- a/src/streams/SubscriptionsCard.js +++ b/src/streams/SubscriptionsCard.js @@ -34,7 +34,7 @@ export default function SubscriptionsCard(props: Props): Node { const handleNarrow = useCallback( (streamId: number, streamName: string) => { - dispatch(doNarrow(streamNarrow(streamName, streamId))); + dispatch(doNarrow(streamNarrow(streamId))); }, [dispatch], ); diff --git a/src/subscriptions/StreamListCard.js b/src/subscriptions/StreamListCard.js index 47effc88ad0..029336f00af 100644 --- a/src/subscriptions/StreamListCard.js +++ b/src/subscriptions/StreamListCard.js @@ -56,7 +56,7 @@ export default function StreamListCard(props: Props): Node { const handleNarrow = useCallback( (streamId: number, streamName: string) => { - dispatch(doNarrow(streamNarrow(streamName, streamId))); + dispatch(doNarrow(streamNarrow(streamId))); }, [dispatch], ); diff --git a/src/subscriptions/__tests__/subscriptionSelectors-test.js b/src/subscriptions/__tests__/subscriptionSelectors-test.js index ea5254343ea..d65af205558 100644 --- a/src/subscriptions/__tests__/subscriptionSelectors-test.js +++ b/src/subscriptions/__tests__/subscriptionSelectors-test.js @@ -57,22 +57,22 @@ describe('getIsActiveStreamSubscribed', () => { }); test('return true if current narrowed stream is subscribed', () => { - const narrow = streamNarrow(eg.stream.name, eg.stream.stream_id); + const narrow = streamNarrow(eg.stream.stream_id); expect(getIsActiveStreamSubscribed(state, narrow)).toBe(true); }); test('return false if current narrowed stream is not subscribed', () => { - const narrow = streamNarrow(eg.otherStream.name, eg.otherStream.stream_id); + const narrow = streamNarrow(eg.otherStream.stream_id); expect(getIsActiveStreamSubscribed(state, narrow)).toBe(false); }); test('return true if stream of current narrowed topic is subscribed', () => { - const narrow = topicNarrow(eg.stream.name, eg.stream.stream_id, 'news'); + const narrow = topicNarrow(eg.stream.stream_id, 'news'); expect(getIsActiveStreamSubscribed(state, narrow)).toBe(true); }); test('return false if stream of current narrowed topic is not subscribed', () => { - const narrow = topicNarrow(eg.otherStream.name, eg.otherStream.stream_id, 'news'); + const narrow = topicNarrow(eg.otherStream.stream_id, 'news'); expect(getIsActiveStreamSubscribed(state, narrow)).toBe(false); }); }); @@ -84,13 +84,13 @@ describe('getStreamColorForNarrow', () => { }); test('return stream color for stream and topic narrow', () => { - const narrow = streamNarrow(eg.stream.name, eg.stream.stream_id); + const narrow = streamNarrow(eg.stream.stream_id); expect(getStreamColorForNarrow(state, narrow)).toEqual(exampleColor); }); test('return null stream color for invalid stream or unknown subscriptions', () => { const unknownStream = eg.makeStream(); - const narrow = streamNarrow(unknownStream.name, unknownStream.stream_id); + const narrow = streamNarrow(unknownStream.stream_id); expect(getStreamColorForNarrow(state, narrow)).toEqual('gray'); }); diff --git a/src/topics/TopicListScreen.js b/src/topics/TopicListScreen.js index d536c6a7f89..d04cabe17b9 100644 --- a/src/topics/TopicListScreen.js +++ b/src/topics/TopicListScreen.js @@ -27,7 +27,7 @@ export default function TopicListScreen(props: Props): Node { const handlePress = useCallback( (streamId: number, streamName: string, topic: string) => { - dispatch(doNarrow(topicNarrow(streamName, streamId, topic))); + dispatch(doNarrow(topicNarrow(streamId, topic))); }, [dispatch], ); diff --git a/src/topics/__tests__/topicsSelectors-test.js b/src/topics/__tests__/topicsSelectors-test.js index 61b367c4bd3..fc4edb8f4c5 100644 --- a/src/topics/__tests__/topicsSelectors-test.js +++ b/src/topics/__tests__/topicsSelectors-test.js @@ -23,7 +23,7 @@ describe('getTopicsForNarrow', () => { }, }); - const topics = getTopicsForNarrow(state, streamNarrow(stream.name, stream.stream_id)); + const topics = getTopicsForNarrow(state, streamNarrow(stream.stream_id)); expect(topics).toEqual(['hi', 'wow']); }); diff --git a/src/unread/UnreadCards.js b/src/unread/UnreadCards.js index 573de4350d6..553e403965f 100644 --- a/src/unread/UnreadCards.js +++ b/src/unread/UnreadCards.js @@ -55,7 +55,7 @@ export default function UnreadCards(props: Props): Node { backgroundColor={section.color} unreadCount={section.unread} onPress={(streamId: number, streamName: string) => { - setTimeout(() => dispatch(doNarrow(streamNarrow(streamName, streamId)))); + setTimeout(() => dispatch(doNarrow(streamNarrow(streamId)))); }} /> ) @@ -72,7 +72,7 @@ export default function UnreadCards(props: Props): Node { isSelected={false} unreadCount={item.unread} onPress={(streamId: number, streamName: string, topic: string) => { - setTimeout(() => dispatch(doNarrow(topicNarrow(streamName, streamId, topic)))); + setTimeout(() => dispatch(doNarrow(topicNarrow(streamId, topic)))); }} /> ) diff --git a/src/utils/__tests__/internalLinks-test.js b/src/utils/__tests__/internalLinks-test.js index ece009c4138..638a3d6ef67 100644 --- a/src/utils/__tests__/internalLinks-test.js +++ b/src/utils/__tests__/internalLinks-test.js @@ -275,9 +275,7 @@ describe('getNarrowFromLink', () => { /* eslint jest/expect-expect: ["error", { "assertFunctionNames": ["expect", "expectStream"] }] */ const expectStream = (operand, streams, expectedStream: null | Stream) => { expect(get(`#narrow/stream/${operand}`, streams)).toEqual( - expectedStream === null - ? null - : streamNarrow(expectedStream.name, expectedStream.stream_id), + expectedStream === null ? null : streamNarrow(expectedStream.stream_id), ); }; @@ -322,7 +320,7 @@ describe('getNarrowFromLink', () => { const expectNameMatch = (operand, streamName) => { const stream = eg.makeStream({ name: streamName }); expect(get(`https://example.com/#narrow/stream/${operand}`, [stream])).toEqual( - streamNarrow(stream.name, stream.stream_id), + streamNarrow(stream.stream_id), ); }; expectNameMatch('jest', 'jest'); @@ -336,10 +334,10 @@ describe('getNarrowFromLink', () => { test('on old stream link, without realm info', () => { expect(get(`/#narrow/stream/${eg.stream.name}`, [eg.stream])).toEqual( - streamNarrow(eg.stream.name, eg.stream.stream_id), + streamNarrow(eg.stream.stream_id), ); expect(get(`#narrow/stream/${eg.stream.name}`, [eg.stream])).toEqual( - streamNarrow(eg.stream.name, eg.stream.stream_id), + streamNarrow(eg.stream.stream_id), ); }); }); @@ -349,7 +347,7 @@ describe('getNarrowFromLink', () => { const expectBasic = (operand, expectedTopic) => { const url = `#narrow/stream/${streamGeneral.stream_id}-general/topic/${operand}`; expect(get(url, [streamGeneral])).toEqual( - topicNarrow(streamGeneral.name, streamGeneral.stream_id, expectedTopic), + topicNarrow(streamGeneral.stream_id, expectedTopic), ); }; @@ -360,11 +358,11 @@ describe('getNarrowFromLink', () => { test('on old topic link, with dot-encoding', () => { expect( get(`https://example.com/#narrow/stream/${eg.stream.name}/topic/(no.20topic)`, [eg.stream]), - ).toEqual(topicNarrow(eg.stream.name, eg.stream.stream_id, '(no topic)')); + ).toEqual(topicNarrow(eg.stream.stream_id, '(no topic)')); expect( get(`https://example.com/#narrow/stream/${eg.stream.name}/topic/google.2Ecom`, [eg.stream]), - ).toEqual(topicNarrow(eg.stream.name, eg.stream.stream_id, 'google.com')); + ).toEqual(topicNarrow(eg.stream.stream_id, 'google.com')); expect(() => get(`https://example.com/#narrow/stream/${eg.stream.name}/topic/google.com`, [eg.stream]), @@ -372,23 +370,23 @@ describe('getNarrowFromLink', () => { expect( get(`https://example.com/#narrow/stream/${eg.stream.name}/topic/topic.20name`, [eg.stream]), - ).toEqual(topicNarrow(eg.stream.name, eg.stream.stream_id, 'topic name')); + ).toEqual(topicNarrow(eg.stream.stream_id, 'topic name')); expect( get(`https://example.com/#narrow/stream/${eg.stream.name}/topic/stream`, [eg.stream]), - ).toEqual(topicNarrow(eg.stream.name, eg.stream.stream_id, 'stream')); + ).toEqual(topicNarrow(eg.stream.stream_id, 'stream')); expect( get(`https://example.com/#narrow/stream/${eg.stream.name}/topic/topic`, [eg.stream]), - ).toEqual(topicNarrow(eg.stream.name, eg.stream.stream_id, 'topic')); + ).toEqual(topicNarrow(eg.stream.stream_id, 'topic')); }); test('on old topic link, without realm info', () => { expect(get(`/#narrow/stream/${eg.stream.name}/topic/topic`, [eg.stream])).toEqual( - topicNarrow(eg.stream.name, eg.stream.stream_id, 'topic'), + topicNarrow(eg.stream.stream_id, 'topic'), ); expect(get(`#narrow/stream/${eg.stream.name}/topic/topic`, [eg.stream])).toEqual( - topicNarrow(eg.stream.name, eg.stream.stream_id, 'topic'), + topicNarrow(eg.stream.stream_id, 'topic'), ); }); }); @@ -420,11 +418,11 @@ describe('getNarrowFromLink', () => { expect( get(`https://example.com/#narrow/stream/${eg.stream.name}/topic/test/near/1`, [eg.stream]), - ).toEqual(topicNarrow(eg.stream.name, eg.stream.stream_id, 'test')); + ).toEqual(topicNarrow(eg.stream.stream_id, 'test')); expect( get(`https://example.com/#narrow/stream/${eg.stream.name}/subject/test/near/1`, [eg.stream]), - ).toEqual(topicNarrow(eg.stream.name, eg.stream.stream_id, 'test')); + ).toEqual(topicNarrow(eg.stream.stream_id, 'test')); }); }); diff --git a/src/utils/__tests__/narrow-test.js b/src/utils/__tests__/narrow-test.js index 595b1a028a2..2827cc4562d 100644 --- a/src/utils/__tests__/narrow-test.js +++ b/src/utils/__tests__/narrow-test.js @@ -61,10 +61,8 @@ describe('isPmNarrow', () => { describe('isStreamOrTopicNarrow', () => { test('check for stream or topic narrow', () => { expect(isStreamOrTopicNarrow(undefined)).toBe(false); - expect(isStreamOrTopicNarrow(streamNarrow(eg.stream.name, eg.stream.stream_id))).toBe(true); - expect( - isStreamOrTopicNarrow(topicNarrow(eg.stream.name, eg.stream.stream_id, 'some topic')), - ).toBe(true); + expect(isStreamOrTopicNarrow(streamNarrow(eg.stream.stream_id))).toBe(true); + expect(isStreamOrTopicNarrow(topicNarrow(eg.stream.stream_id, 'some topic'))).toBe(true); expect(isStreamOrTopicNarrow(HOME_NARROW)).toBe(false); expect(isStreamOrTopicNarrow(pm1to1NarrowFromUser(eg.otherUser))).toBe(false); expect(isStreamOrTopicNarrow(pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser]))).toBe( @@ -78,14 +76,14 @@ describe('specialNarrow', () => { test('only narrowing with the "is" operator is special narrow', () => { expect(isSpecialNarrow(undefined)).toBe(false); expect(isSpecialNarrow(HOME_NARROW)).toBe(false); - expect(isSpecialNarrow(streamNarrow(eg.stream.name, eg.stream.stream_id))).toBe(false); + expect(isSpecialNarrow(streamNarrow(eg.stream.stream_id))).toBe(false); expect(isSpecialNarrow(STARRED_NARROW)).toBe(true); }); }); describe('streamNarrow', () => { test('narrows to messages from a specific stream', () => { - const narrow = streamNarrow(eg.stream.name, eg.stream.stream_id); + const narrow = streamNarrow(eg.stream.stream_id); expect(isStreamNarrow(narrow)).toBeTrue(); expect(streamIdOfNarrow(narrow)).toEqual(eg.stream.stream_id); }); @@ -93,13 +91,13 @@ describe('streamNarrow', () => { test('only narrow with operator of "stream" is a stream narrow', () => { expect(isStreamNarrow(undefined)).toBe(false); expect(isStreamNarrow(HOME_NARROW)).toBe(false); - expect(isStreamNarrow(streamNarrow(eg.stream.name, eg.stream.stream_id))).toBe(true); + expect(isStreamNarrow(streamNarrow(eg.stream.stream_id))).toBe(true); }); }); describe('topicNarrow', () => { test('narrows to a specific topic within a specified stream', () => { - const narrow = topicNarrow(eg.stream.name, eg.stream.stream_id, 'some topic'); + const narrow = topicNarrow(eg.stream.stream_id, 'some topic'); expect(isTopicNarrow(narrow)).toBeTrue(); expect(streamIdOfNarrow(narrow)).toEqual(eg.stream.stream_id); expect(topicOfNarrow(narrow)).toEqual('some topic'); @@ -108,9 +106,7 @@ describe('topicNarrow', () => { test('only narrow with two items, one for stream, one for topic is a topic narrow', () => { expect(isTopicNarrow(undefined)).toBe(false); expect(isTopicNarrow(HOME_NARROW)).toBe(false); - expect(isTopicNarrow(topicNarrow(eg.stream.name, eg.stream.stream_id, 'some topic'))).toBe( - true, - ); + expect(isTopicNarrow(topicNarrow(eg.stream.stream_id, 'some topic'))).toBe(true); }); }); @@ -131,12 +127,12 @@ describe('isMessageInNarrow', () => { ['a message', true, eg.streamMessage()], ]], - ['whole-stream narrow', streamNarrow(eg.stream.name, eg.stream.stream_id), [ + ['whole-stream narrow', streamNarrow(eg.stream.stream_id), [ ['matching stream message', true, eg.streamMessage()], ['other-stream message', false, eg.streamMessage({ stream: otherStream })], ['PM', false, eg.pmMessage()], ]], - ['stream conversation', topicNarrow(eg.stream.name, eg.stream.stream_id, 'cabbages'), [ + ['stream conversation', topicNarrow(eg.stream.stream_id, 'cabbages'), [ ['matching message', true, eg.streamMessage({ subject: 'cabbages' })], ['message in same stream but other topic', false, eg.streamMessage({ subject: 'kings' })], ['other-stream message', false, eg.streamMessage({ stream: otherStream })], @@ -257,8 +253,8 @@ describe('getNarrowsForMessage', () => { }, expectedNarrows: [ HOME_NARROW, - streamNarrow(eg.stream.name, eg.stream.stream_id), - topicNarrow(eg.stream.name, eg.stream.stream_id, 'myTopic'), + streamNarrow(eg.stream.stream_id), + topicNarrow(eg.stream.stream_id, 'myTopic'), ], }, { @@ -279,8 +275,8 @@ describe('getNarrowsForMessage', () => { }, expectedNarrows: [ HOME_NARROW, - streamNarrow(eg.stream.name, eg.stream.stream_id), - topicNarrow(eg.stream.name, eg.stream.stream_id, ''), + streamNarrow(eg.stream.stream_id), + topicNarrow(eg.stream.stream_id, ''), ], }, { @@ -331,7 +327,7 @@ describe('getNarrowForReply', () => { test('for stream message with nonempty topic, returns a topic narrow', () => { const message = eg.streamMessage(); - const expectedNarrow = topicNarrow(eg.stream.name, eg.stream.stream_id, message.subject); + const expectedNarrow = topicNarrow(eg.stream.stream_id, message.subject); const actualNarrow = getNarrowForReply(message, eg.selfUser.user_id); @@ -341,8 +337,8 @@ describe('getNarrowForReply', () => { describe('keyFromNarrow+parseNarrow', () => { const baseNarrows = [ - ['whole stream', streamNarrow(eg.stream.name, eg.stream.stream_id)], - ['stream conversation', topicNarrow(eg.stream.name, eg.stream.stream_id, 'a topic')], + ['whole stream', streamNarrow(eg.stream.stream_id)], + ['stream conversation', topicNarrow(eg.stream.stream_id, 'a topic')], ['1:1 PM conversation, non-self', pm1to1NarrowFromUser(eg.otherUser)], ['self-1:1 conversation', pm1to1NarrowFromUser(eg.selfUser)], ['group-PM conversation', pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser])], @@ -361,29 +357,15 @@ describe('keyFromNarrow+parseNarrow', () => { const diverseCharactersStream = eg.makeStream({ name: diverseCharacters }); const htmlEntitiesStream = eg.makeStream({ name: htmlEntities }); const awkwardNarrows = [ - [ - 'whole stream about awkward characters', - streamNarrow(diverseCharactersStream.name, diverseCharactersStream.stream_id), - ], - [ - 'whole stream about HTML entities', - streamNarrow(htmlEntitiesStream.name, htmlEntitiesStream.stream_id), - ], + ['whole stream about awkward characters', streamNarrow(diverseCharactersStream.stream_id)], + ['whole stream about HTML entities', streamNarrow(htmlEntitiesStream.stream_id)], [ 'stream conversation about awkward characters', - topicNarrow( - diverseCharactersStream.name, - diverseCharactersStream.stream_id, - `regarding ${diverseCharacters}`, - ), + topicNarrow(diverseCharactersStream.stream_id, `regarding ${diverseCharacters}`), ], [ 'stream conversation about HTML entities', - topicNarrow( - htmlEntitiesStream.name, - htmlEntitiesStream.stream_id, - `regarding ${htmlEntities}`, - ), + topicNarrow(htmlEntitiesStream.stream_id, `regarding ${htmlEntities}`), ], ['search narrow for awkward characters', SEARCH_NARROW(diverseCharacters)], ['search narrow for HTML entities', SEARCH_NARROW(htmlEntities)], diff --git a/src/utils/internalLinks.js b/src/utils/internalLinks.js index 9b493d8d6cd..3a6aaf53c42 100644 --- a/src/utils/internalLinks.js +++ b/src/utils/internalLinks.js @@ -123,6 +123,7 @@ export const decodeHashComponent = (string: string): string => { * * Return null if the operand doesn't match any known stream. */ +// TODO simplify: return only stream ID, not name const parseStreamOperand = (operand, streamsById, streamsByName): null | [string, number] => { // "New" (2018) format: ${stream_id}-${stream_name} . const match = /^([\d]+)(?:-.*)?$/.exec(operand); @@ -189,11 +190,11 @@ export const getNarrowFromLink = ( } case 'topic': { const streamNameAndId = parseStreamOperand(paths[1], streamsById, streamsByName); - return streamNameAndId && topicNarrow(...streamNameAndId, parseTopicOperand(paths[3])); + return streamNameAndId && topicNarrow(streamNameAndId[1], parseTopicOperand(paths[3])); } case 'stream': { const streamNameAndId = parseStreamOperand(paths[1], streamsById, streamsByName); - return streamNameAndId && streamNarrow(...streamNameAndId); + return streamNameAndId && streamNarrow(streamNameAndId[1]); } case 'special': try { diff --git a/src/utils/narrow.js b/src/utils/narrow.js index 50a8f1c52f0..7c3c2d3335b 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -6,7 +6,6 @@ import { normalizeRecipientsAsUserIdsSansMe, pmKeyRecipientsFromMessage, recipientsOfPrivateMessage, - streamNameOfStreamMessage, type PmKeyRecipients, type PmKeyUsers, pmKeyRecipientsFromPmKeyUsers, @@ -148,14 +147,11 @@ export const ALL_PRIVATE_NARROW: Narrow = specialNarrow('private'); export const ALL_PRIVATE_NARROW_STR: string = keyFromNarrow(ALL_PRIVATE_NARROW); -export const streamNarrow = (streamNameIgnored: string | void, streamId: number): Narrow => +export const streamNarrow = (streamId: number): Narrow => Object.freeze({ type: 'stream', streamId }); -export const topicNarrow = ( - streamNameIgnored: string | void, - streamId: number, - topic: string, -): Narrow => Object.freeze({ type: 'topic', streamId, topic }); +export const topicNarrow = (streamId: number, topic: string): Narrow => + Object.freeze({ type: 'topic', streamId, topic }); export const SEARCH_NARROW = (query: string): Narrow => Object.freeze({ type: 'search', query }); @@ -287,7 +283,7 @@ export const parseNarrow = (narrowStr: string): Narrow => { if (!/^\d+$/.test(rest)) { throw makeError(); } - return streamNarrow(undefined, Number.parseInt(rest, 10)); + return streamNarrow(Number.parseInt(rest, 10)); } case 'topic:': { @@ -298,7 +294,7 @@ export const parseNarrow = (narrowStr: string): Narrow => { if (!match) { throw makeError(); } - return topicNarrow(undefined, Number.parseInt(match[1], 10), match[2]); + return topicNarrow(Number.parseInt(match[1], 10), match[2]); } case 'pm:': { @@ -539,9 +535,8 @@ export const getNarrowsForMessage = ( result.push(ALL_PRIVATE_NARROW); result.push(pmNarrowFromRecipients(pmKeyRecipientsFromMessage(message, ownUserId))); } else { - const streamName = streamNameOfStreamMessage(message); - result.push(topicNarrow(streamName, message.stream_id, message.subject)); - result.push(streamNarrow(streamName, message.stream_id)); + result.push(topicNarrow(message.stream_id, message.subject)); + result.push(streamNarrow(message.stream_id)); } if (flags.includes('mentioned') || flags.includes('wildcard_mentioned')) { @@ -568,7 +563,6 @@ export const getNarrowForReply = (message: Message | Outbox, ownUserId: UserId): if (message.type === 'private') { return pmNarrowFromRecipients(pmKeyRecipientsFromMessage(message, ownUserId)); } else { - const streamName = streamNameOfStreamMessage(message); - return topicNarrow(streamName, message.stream_id, message.subject); + return topicNarrow(message.stream_id, message.subject); } }; diff --git a/src/webview/html/__tests__/messageListElementHtml-test.js b/src/webview/html/__tests__/messageListElementHtml-test.js index d47221d6e3d..0b7a07afad9 100644 --- a/src/webview/html/__tests__/messageListElementHtml-test.js +++ b/src/webview/html/__tests__/messageListElementHtml-test.js @@ -273,7 +273,7 @@ describe('messages -> piece descriptors -> content HTML is stable/sensible', () ].forEach(testCase => check(testCase)); }); - const streamNarrow1 = streamNarrow(stream1.name, stream1.stream_id); + const streamNarrow1 = streamNarrow(stream1.stream_id); test(`${keyFromNarrow(streamNarrow1)}`, () => { [ { narrow: streamNarrow1, messages: streamMessages1 }, @@ -294,7 +294,7 @@ describe('messages -> piece descriptors -> content HTML is stable/sensible', () ].forEach(testCase => check(testCase)); }); - const topicNarrow1 = topicNarrow(stream1.name, stream1.stream_id, topic1); + const topicNarrow1 = topicNarrow(stream1.stream_id, topic1); test(`${keyFromNarrow(topicNarrow1)}`, () => { [ { narrow: topicNarrow1, messages: streamMessages1 }, diff --git a/src/webview/html/header.js b/src/webview/html/header.js index f3044d0059f..6c2b9e7a36b 100644 --- a/src/webview/html/header.js +++ b/src/webview/html/header.js @@ -39,9 +39,7 @@ export default ( if (message.type === 'stream') { const streamName = streamNameOfStreamMessage(message); - const topicNarrowStr = keyFromNarrow( - topicNarrow(streamName, message.stream_id, message.subject), - ); + const topicNarrowStr = keyFromNarrow(topicNarrow(message.stream_id, message.subject)); const topicHtml = renderSubject(message); if (headerStyle === 'topic+date') { @@ -59,7 +57,7 @@ export default ( const subscription = subscriptions.get(message.stream_id); const backgroundColor = subscription ? subscription.color : 'hsl(0, 0%, 80%)'; const textColor = foregroundColorFromBackground(backgroundColor); - const streamNarrowStr = keyFromNarrow(streamNarrow(streamName, message.stream_id)); + const streamNarrowStr = keyFromNarrow(streamNarrow(message.stream_id)); return template`
Date: Wed, 12 Jan 2022 22:39:32 -0800 Subject: [PATCH 23/24] internal links [nfc]: Return whole Stream from parseStreamOperand This makes its interface a bit simpler, and gets rid of the stream name there. --- src/utils/internalLinks.js | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/utils/internalLinks.js b/src/utils/internalLinks.js index 3a6aaf53c42..2c07103b16f 100644 --- a/src/utils/internalLinks.js +++ b/src/utils/internalLinks.js @@ -119,19 +119,18 @@ export const decodeHashComponent = (string: string): string => { }; /** - * Parse the operand of a `stream` operator, returning a stream ID and name. + * Parse the operand of a `stream` operator. * * Return null if the operand doesn't match any known stream. */ -// TODO simplify: return only stream ID, not name -const parseStreamOperand = (operand, streamsById, streamsByName): null | [string, number] => { +const parseStreamOperand = (operand, streamsById, streamsByName): null | Stream => { // "New" (2018) format: ${stream_id}-${stream_name} . const match = /^([\d]+)(?:-.*)?$/.exec(operand); if (match) { const streamId = parseInt(match[1], 10); const stream = streamsById.get(streamId); if (stream) { - return [stream.name, streamId]; + return stream; } } @@ -140,7 +139,7 @@ const parseStreamOperand = (operand, streamsById, streamsByName): null | [string const streamName = decodeHashComponent(operand); const stream = streamsByName.get(streamName); if (stream) { - return [streamName, stream.stream_id]; + return stream; } // Not any stream we know. (Most likely this means a stream the user @@ -189,12 +188,12 @@ export const getNarrowFromLink = ( return pmNarrowFromRecipients(pmKeyRecipientsFromIds(ids, ownUserId)); } case 'topic': { - const streamNameAndId = parseStreamOperand(paths[1], streamsById, streamsByName); - return streamNameAndId && topicNarrow(streamNameAndId[1], parseTopicOperand(paths[3])); + const stream = parseStreamOperand(paths[1], streamsById, streamsByName); + return stream && topicNarrow(stream.stream_id, parseTopicOperand(paths[3])); } case 'stream': { - const streamNameAndId = parseStreamOperand(paths[1], streamsById, streamsByName); - return streamNameAndId && streamNarrow(streamNameAndId[1]); + const stream = parseStreamOperand(paths[1], streamsById, streamsByName); + return stream && streamNarrow(stream.stream_id); } case 'special': try { From 97e86e63f059ffd21a4b2bcbcfdc5561368e793a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 27 Jan 2022 08:49:17 -0800 Subject: [PATCH 24/24] stream [nfc]: Drop a few more stream names, after ID-only narrows These were each needed only because of a Narrow constructor, and those no longer take a stream name. This isn't yet enough to let us stop passing stream names *to* these callbacks, because each of these has another use site where the callback does still use the stream name. We'll get to those separately later. --- src/streams/SubscriptionsCard.js | 4 +--- src/subscriptions/StreamListCard.js | 4 +--- src/topics/TopicListScreen.js | 2 +- src/unread/UnreadCards.js | 4 ++-- 4 files changed, 5 insertions(+), 9 deletions(-) diff --git a/src/streams/SubscriptionsCard.js b/src/streams/SubscriptionsCard.js index 17e0608b763..0754849721b 100644 --- a/src/streams/SubscriptionsCard.js +++ b/src/streams/SubscriptionsCard.js @@ -33,9 +33,7 @@ export default function SubscriptionsCard(props: Props): Node { const unreadByStream = useSelector(getUnreadByStream); const handleNarrow = useCallback( - (streamId: number, streamName: string) => { - dispatch(doNarrow(streamNarrow(streamId))); - }, + (streamId: number) => dispatch(doNarrow(streamNarrow(streamId))), [dispatch], ); diff --git a/src/subscriptions/StreamListCard.js b/src/subscriptions/StreamListCard.js index 029336f00af..1667de57ee4 100644 --- a/src/subscriptions/StreamListCard.js +++ b/src/subscriptions/StreamListCard.js @@ -55,9 +55,7 @@ export default function StreamListCard(props: Props): Node { ); const handleNarrow = useCallback( - (streamId: number, streamName: string) => { - dispatch(doNarrow(streamNarrow(streamId))); - }, + (streamId: number) => dispatch(doNarrow(streamNarrow(streamId))), [dispatch], ); diff --git a/src/topics/TopicListScreen.js b/src/topics/TopicListScreen.js index d04cabe17b9..9a6f5d834a0 100644 --- a/src/topics/TopicListScreen.js +++ b/src/topics/TopicListScreen.js @@ -26,7 +26,7 @@ export default function TopicListScreen(props: Props): Node { const [filter, setFilter] = useState(''); const handlePress = useCallback( - (streamId: number, streamName: string, topic: string) => { + (streamId: number, _ignored_streamName: string, topic: string) => { dispatch(doNarrow(topicNarrow(streamId, topic))); }, [dispatch], diff --git a/src/unread/UnreadCards.js b/src/unread/UnreadCards.js index 553e403965f..b1a470d187c 100644 --- a/src/unread/UnreadCards.js +++ b/src/unread/UnreadCards.js @@ -54,7 +54,7 @@ export default function UnreadCards(props: Props): Node { isPrivate={section.isPrivate} backgroundColor={section.color} unreadCount={section.unread} - onPress={(streamId: number, streamName: string) => { + onPress={(streamId: number) => { setTimeout(() => dispatch(doNarrow(streamNarrow(streamId)))); }} /> @@ -71,7 +71,7 @@ export default function UnreadCards(props: Props): Node { isMuted={section.isMuted || item.isMuted} isSelected={false} unreadCount={item.unread} - onPress={(streamId: number, streamName: string, topic: string) => { + onPress={(streamId: number, _ignored_streamName: string, topic: string) => { setTimeout(() => dispatch(doNarrow(topicNarrow(streamId, topic)))); }} />