From 99221c0dd3d6628b26c9472dea6cb33fd3a9561e Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 31 Jan 2022 21:59:18 -0800 Subject: [PATCH 01/28] action types [nfc]: The mute event doesn't care what our state shape is These event actions are based on what's in the event from the server, not what's in our internal state. These happen to be the same at the moment, but won't stay that way; so have this say what it really means. --- src/actionTypes.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/actionTypes.js b/src/actionTypes.js index 3e28dc15010..ff4380982b8 100644 --- a/src/actionTypes.js +++ b/src/actionTypes.js @@ -70,6 +70,7 @@ import type { SubmessageEvent, RestartEvent, } from './api/eventTypes'; +import type { MuteTuple } from './api/apiTypes'; import type { Orientation, @@ -90,7 +91,6 @@ import type { RealmEmojiById, GlobalSettingsState, CaughtUpState, - MuteState, AlertWordsState, UserId, UserStatusEvent, @@ -460,7 +460,7 @@ type EventUserUpdateAction = $ReadOnly<{| type EventMutedTopicsAction = $ReadOnly<{| ...ServerEvent, type: typeof EVENT_MUTED_TOPICS, - muted_topics: MuteState, + muted_topics: $ReadOnlyArray, |}>; type EventMutedUsersAction = $ReadOnly<{| From bbb0cd63f5039c6fb8d9324accdf0c2e79d1c151 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 31 Jan 2022 22:25:08 -0800 Subject: [PATCH 02/28] api types [nfc]: Move MuteTuple to modelTypes After all, it appears not only in the initial data but also in events. --- src/api/initialDataTypes.js | 3 +-- src/api/modelTypes.js | 9 +++++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/api/initialDataTypes.js b/src/api/initialDataTypes.js index 6c6bb57cdfb..a4047f21389 100644 --- a/src/api/initialDataTypes.js +++ b/src/api/initialDataTypes.js @@ -2,6 +2,7 @@ import type { CrossRealmBot, + MuteTuple, MutedUser, RealmEmojiById, RealmFilter, @@ -66,8 +67,6 @@ export type InitialDataMessage = $ReadOnly<{| max_message_id: number, |}>; -export type MuteTuple = [string, string]; - export type InitialDataMutedTopics = $ReadOnly<{| muted_topics: $ReadOnlyArray, |}>; diff --git a/src/api/modelTypes.js b/src/api/modelTypes.js index 9b9dda304b7..b4e992ed931 100644 --- a/src/api/modelTypes.js +++ b/src/api/modelTypes.js @@ -329,6 +329,15 @@ export type Topic = $ReadOnly<{| max_id: number, |}>; +/** + * A muted topic. + * + * Found in the initial data, and in `muted_topics` events. + * + * The elements are the stream name, then topic. + */ +export type MuteTuple = [string, string]; + // // // From 44726b0c4ef9152921f133f2ae4116678226c119 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 7 Feb 2022 12:49:31 -0800 Subject: [PATCH 03/28] mute [nfc]: Add TODO comments for converting to stream IDs --- src/api/modelTypes.js | 2 ++ src/reduxTypes.js | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/src/api/modelTypes.js b/src/api/modelTypes.js index b4e992ed931..44f2658a942 100644 --- a/src/api/modelTypes.js +++ b/src/api/modelTypes.js @@ -336,6 +336,8 @@ export type Topic = $ReadOnly<{| * * The elements are the stream name, then topic. */ +// Server issue for using stream IDs (#3918) for muted topics, not names: +// https://github.com/zulip/zulip/issues/21015 export type MuteTuple = [string, string]; // diff --git a/src/reduxTypes.js b/src/reduxTypes.js index 72f957948c7..9ab3b523f0f 100644 --- a/src/reduxTypes.js +++ b/src/reduxTypes.js @@ -178,6 +178,10 @@ export type MigrationsState = $ReadOnly<{| version?: number, |}>; +// TODO(#3918): Use stream IDs for muted topics, not stream names. +// Server issue for fixing that in the API: https://github.com/zulip/zulip/issues/21015 +// Meanwhile, even while the server is still sending stream names, +// we should convert to stream IDs at the edge. export type MuteState = $ReadOnlyArray; /** A map from user IDs to the Unix timestamp in seconds when they were muted. */ From 1a2455e40207e70a13e7ebcb8f684ea35daeb63d Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 31 Jan 2022 22:26:41 -0800 Subject: [PATCH 04/28] api types [nfc]: Name MutedTopicTuple better (was MuteTuple) The term "mute" is used for both topics and users, so best to be explicit. --- src/actionTypes.js | 4 ++-- src/api/initialDataTypes.js | 4 ++-- src/api/modelTypes.js | 2 +- src/reduxTypes.js | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/actionTypes.js b/src/actionTypes.js index ff4380982b8..c22b74f99f8 100644 --- a/src/actionTypes.js +++ b/src/actionTypes.js @@ -70,7 +70,7 @@ import type { SubmessageEvent, RestartEvent, } from './api/eventTypes'; -import type { MuteTuple } from './api/apiTypes'; +import type { MutedTopicTuple } from './api/apiTypes'; import type { Orientation, @@ -460,7 +460,7 @@ type EventUserUpdateAction = $ReadOnly<{| type EventMutedTopicsAction = $ReadOnly<{| ...ServerEvent, type: typeof EVENT_MUTED_TOPICS, - muted_topics: $ReadOnlyArray, + muted_topics: $ReadOnlyArray, |}>; type EventMutedUsersAction = $ReadOnly<{| diff --git a/src/api/initialDataTypes.js b/src/api/initialDataTypes.js index a4047f21389..4d12440f753 100644 --- a/src/api/initialDataTypes.js +++ b/src/api/initialDataTypes.js @@ -2,7 +2,7 @@ import type { CrossRealmBot, - MuteTuple, + MutedTopicTuple, MutedUser, RealmEmojiById, RealmFilter, @@ -68,7 +68,7 @@ export type InitialDataMessage = $ReadOnly<{| |}>; export type InitialDataMutedTopics = $ReadOnly<{| - muted_topics: $ReadOnlyArray, + muted_topics: $ReadOnlyArray, |}>; export type InitialDataMutedUsers = $ReadOnly<{| diff --git a/src/api/modelTypes.js b/src/api/modelTypes.js index 44f2658a942..9a32184ef50 100644 --- a/src/api/modelTypes.js +++ b/src/api/modelTypes.js @@ -338,7 +338,7 @@ export type Topic = $ReadOnly<{| */ // Server issue for using stream IDs (#3918) for muted topics, not names: // https://github.com/zulip/zulip/issues/21015 -export type MuteTuple = [string, string]; +export type MutedTopicTuple = [string, string]; // // diff --git a/src/reduxTypes.js b/src/reduxTypes.js index 9ab3b523f0f..4f2a620e06c 100644 --- a/src/reduxTypes.js +++ b/src/reduxTypes.js @@ -15,7 +15,7 @@ import type { Action, DispatchableWithoutAccountAction } from './actionTypes'; import type { Topic, Message, - MuteTuple, + MutedTopicTuple, CrossRealmBot, RealmEmojiById, RealmFilter, @@ -182,7 +182,7 @@ export type MigrationsState = $ReadOnly<{| // Server issue for fixing that in the API: https://github.com/zulip/zulip/issues/21015 // Meanwhile, even while the server is still sending stream names, // we should convert to stream IDs at the edge. -export type MuteState = $ReadOnlyArray; +export type MuteState = $ReadOnlyArray; /** A map from user IDs to the Unix timestamp in seconds when they were muted. */ export type MutedUsersState = Immutable.Map; From 996e19b72863c975dd556687caeb13ac2ae02feb Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 31 Jan 2022 22:29:55 -0800 Subject: [PATCH 05/28] api types: Give MutedTopicTuple its third element, the timestamp --- src/api/modelTypes.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/api/modelTypes.js b/src/api/modelTypes.js index 9a32184ef50..030ac88800c 100644 --- a/src/api/modelTypes.js +++ b/src/api/modelTypes.js @@ -334,11 +334,12 @@ export type Topic = $ReadOnly<{| * * Found in the initial data, and in `muted_topics` events. * - * The elements are the stream name, then topic. + * The elements are the stream name, then topic, then possibly timestamp. */ // Server issue for using stream IDs (#3918) for muted topics, not names: // https://github.com/zulip/zulip/issues/21015 -export type MutedTopicTuple = [string, string]; +// TODO(server-3.0): Simplify away the no-timestamp version, new in FL 1. +export type MutedTopicTuple = [string, string] | [string, string, number]; // // From 6197b1da9c25e0fee0d04244f8362b442bd016f2 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 7 Feb 2022 14:58:44 -0800 Subject: [PATCH 06/28] example data [nfc]: In makeStream take stream_id, and use that Call sites updated automatically, with: $ perl -i -0pe ' s<\{ \.\.\.eg\.makeStream\(\{ (.*?) \}\), (stream_id: \d+) \}> g ' src/**/*.js --- src/__tests__/lib/exampleData.js | 7 +++---- src/topics/__tests__/topicsSelectors-test.js | 6 +++--- src/unread/__tests__/unread-testlib.js | 4 ++-- src/utils/__tests__/internalLinks-test.js | 2 +- .../__tests__/generateInboundEventEditSequence-test.js | 4 ++-- 5 files changed, 11 insertions(+), 12 deletions(-) diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index 1c01f95ccc1..f7e04ea4a19 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -246,14 +246,13 @@ export const crossRealmBot: CrossRealmBot = makeCrossRealmBot({ name: 'bot' }); const randStreamId: () => number = makeUniqueRandInt('stream IDs', 1000); export const makeStream = ( - args: {| name?: string, description?: string |} = Object.freeze({}), + args: {| stream_id?: number, name?: string, description?: string |} = Object.freeze({}), ): Stream => { const name = args.name ?? randString(); - const description = args.description ?? `On the ${randString()} of ${name}`; return deepFreeze({ - stream_id: randStreamId(), + stream_id: args.stream_id ?? randStreamId(), name, - description, + description: args.description ?? `On the ${randString()} of ${name}`, invite_only: false, is_announcement_only: false, history_public_to_subscribers: true, diff --git a/src/topics/__tests__/topicsSelectors-test.js b/src/topics/__tests__/topicsSelectors-test.js index fc4edb8f4c5..a0e9e6db4c1 100644 --- a/src/topics/__tests__/topicsSelectors-test.js +++ b/src/topics/__tests__/topicsSelectors-test.js @@ -14,7 +14,7 @@ describe('getTopicsForNarrow', () => { }); test('when there are topics in the active narrow, return them as string array', () => { - const stream = { ...eg.makeStream({ name: 'hello' }), stream_id: 123 }; + const stream = eg.makeStream({ stream_id: 123, name: 'hello' }); const state = eg.reduxState({ streams: [stream], topics: { @@ -43,7 +43,7 @@ describe('getTopicsForStream', () => { }); test('when topics loaded for given stream return them', () => { - const stream = { ...eg.makeStream({ name: 'stream 123' }), stream_id: 123 }; + const stream = eg.makeStream({ stream_id: 123, name: 'stream 123' }); const state = eg.reduxState({ streams: [stream], topics: { @@ -58,7 +58,7 @@ describe('getTopicsForStream', () => { }); test('Return list of topic object with isMuted, unreadCount, topic name and max id in it.', () => { - const stream = { ...eg.makeStream({ name: 'stream 1' }), stream_id: 1 }; + const stream = eg.makeStream({ stream_id: 1, name: 'stream 1' }); const state = eg.reduxStatePlus({ streams: [stream], diff --git a/src/unread/__tests__/unread-testlib.js b/src/unread/__tests__/unread-testlib.js index 695cbb16e17..c16defbaf6f 100644 --- a/src/unread/__tests__/unread-testlib.js +++ b/src/unread/__tests__/unread-testlib.js @@ -11,8 +11,8 @@ export const initialState: UnreadState = reducer( eg.baseReduxState, ); -export const stream0: Stream = { ...eg.makeStream({ name: 'stream 0' }), stream_id: 0 }; -export const stream2: Stream = { ...eg.makeStream({ name: 'stream 2' }), stream_id: 2 }; +export const stream0: Stream = eg.makeStream({ stream_id: 0, name: 'stream 0' }); +export const stream2: Stream = eg.makeStream({ stream_id: 2, name: 'stream 2' }); const [user0, user1, user2, user3, user4, user5] = [0, 1, 2, 3, 4, 5].map(user_id => eg.makeUser({ user_id }), diff --git a/src/utils/__tests__/internalLinks-test.js b/src/utils/__tests__/internalLinks-test.js index 638a3d6ef67..25427bb1bf3 100644 --- a/src/utils/__tests__/internalLinks-test.js +++ b/src/utils/__tests__/internalLinks-test.js @@ -309,7 +309,7 @@ describe('getNarrowFromLink', () => { }); test('on ambiguous new- or old-style: new wins', () => { - const collider = { ...eg.makeStream({ name: 'collider' }), stream_id: 311 }; + const collider = eg.makeStream({ stream_id: 311, name: 'collider' }); expectStream('311', [numbers, collider], collider); expectStream('311-', [numbersHyphen, collider], collider); expectStream('311-help', [numbersPlus, collider], collider); diff --git a/src/webview/__tests__/generateInboundEventEditSequence-test.js b/src/webview/__tests__/generateInboundEventEditSequence-test.js index 33976f7d713..c6160722843 100644 --- a/src/webview/__tests__/generateInboundEventEditSequence-test.js +++ b/src/webview/__tests__/generateInboundEventEditSequence-test.js @@ -29,8 +29,8 @@ const user1 = eg.makeUser({ user_id: 1, name: 'nonrandom name one' }); const user2 = eg.makeUser({ user_id: 2, name: 'nonrandom name two' }); const user3 = eg.makeUser({ user_id: 3, name: 'nonrandom name three' }); -const stream1 = { ...eg.makeStream({ name: 'stream 1' }), stream_id: 1 }; -const stream2 = { ...eg.makeStream({ name: 'stream 2' }), stream_id: 2 }; +const stream1 = eg.makeStream({ stream_id: 1, name: 'stream 1' }); +const stream2 = eg.makeStream({ stream_id: 2, name: 'stream 2' }); const topic1 = 'topic 1'; const topic2 = 'topic 2'; From 6875eeb87cbd5e663569a1e76c8aa68f1c661ac2 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 7 Feb 2022 15:05:55 -0800 Subject: [PATCH 07/28] example data [nfc]: In makeUser take email, and use that Call sites updated mostly automatically, with: $ perl -i -0pe ' s<\{ \.\.\.eg\.makeUser\(\{ (.*?) \}\), (email: \S+) \}> g ' src/**/*.js $ perl -i -0pe ' s<\{ \.\.\.eg\.makeUser\(\), (email: \S+) \}> g ' src/**/*.js plus manually handling the one call site each in render-test and fetchActions-test. --- src/__tests__/lib/exampleData.js | 10 +- src/message/__tests__/fetchActions-test.js | 3 +- src/users/__tests__/userHelpers-test.js | 142 ++++++++++----------- src/webview/html/__tests__/render-test.js | 6 +- 4 files changed, 79 insertions(+), 82 deletions(-) diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index f7e04ea4a19..beb6408f007 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -125,13 +125,15 @@ export const diverseCharacters: string = type UserOrBotPropertiesArgs = {| name?: string, user_id?: number, // accept a plain number, for convenience in tests + email?: string, |}; const randUserId: () => UserId = (mk => () => makeUserId(mk()))( makeUniqueRandInt('user IDs', 10000), ); -const userOrBotProperties = ({ name: _name, user_id }: UserOrBotPropertiesArgs) => { - const name = _name ?? randString(); +const userOrBotProperties = (args: UserOrBotPropertiesArgs) => { + const name = args.name ?? randString(); + const email = args.email ?? `${name}@example.org`; const capsName = name.substring(0, 1).toUpperCase() + name.substring(1); return deepFreeze({ // Internally the UploadedAvatarURL mutates itself for memoization. @@ -143,11 +145,11 @@ const userOrBotProperties = ({ name: _name, user_id }: UserOrBotPropertiesArgs) .toString() .padStart(2, '0')}`, - email: `${name}@example.org`, + email, full_name: `${capsName} User`, is_admin: false, timezone: 'UTC', - user_id: user_id != null ? makeUserId(user_id) : randUserId(), + user_id: args.user_id != null ? makeUserId(args.user_id) : randUserId(), }); }; diff --git a/src/message/__tests__/fetchActions-test.js b/src/message/__tests__/fetchActions-test.js index 7a5d3f8a52b..1a8db21baa6 100644 --- a/src/message/__tests__/fetchActions-test.js +++ b/src/message/__tests__/fetchActions-test.js @@ -231,8 +231,7 @@ describe('fetchActions', () => { describe('fetchMessages', () => { const email = 'abc123@example.com'; const sender = { - ...eg.makeUser(), - email, + ...eg.makeUser({ email }), avatar_url: GravatarURL.validateAndConstructInstance({ email }), }; const message1 = eg.streamMessage({ id: 1, sender }); diff --git a/src/users/__tests__/userHelpers-test.js b/src/users/__tests__/userHelpers-test.js index 27440a0fb24..9ef2b6afb1d 100644 --- a/src/users/__tests__/userHelpers-test.js +++ b/src/users/__tests__/userHelpers-test.js @@ -45,11 +45,11 @@ describe('filterUserList', () => { }); test('searches in name, email and is case insensitive', () => { - const user1 = { ...eg.makeUser({ name: 'match' }), email: 'any@example.com' }; - const user2 = { ...eg.makeUser({ name: 'partial match' }), email: 'any@example.com' }; - const user3 = { ...eg.makeUser({ name: 'Case Insensitive MaTcH' }), email: 'any@example.com' }; - const user4 = { ...eg.makeUser({ name: 'Any Name' }), email: 'match@example.com' }; - const user5 = { ...eg.makeUser({ name: 'some name' }), email: 'another@example.com' }; + const user1 = eg.makeUser({ name: 'match', email: 'any@example.com' }); + const user2 = eg.makeUser({ name: 'partial match', email: 'any@example.com' }); + const user3 = eg.makeUser({ name: 'Case Insensitive MaTcH', email: 'any@example.com' }); + const user4 = eg.makeUser({ name: 'Any Name', email: 'match@example.com' }); + const user5 = eg.makeUser({ name: 'some name', email: 'another@example.com' }); const allUsers = deepFreeze([user1, user2, user3, user4, user5]); const shouldMatch = [user1, user2, user3, user4]; @@ -98,11 +98,11 @@ describe('getAutocompleteSuggestion', () => { }); test('searches in name, email and is case insensitive', () => { - const user1 = { ...eg.makeUser({ name: 'match' }), email: 'any1@example.com' }; - const user2 = { ...eg.makeUser({ name: 'match this' }), email: 'any2@example.com' }; - const user3 = { ...eg.makeUser({ name: 'MaTcH Case Insensitive' }), email: 'any3@example.com' }; - const user4 = { ...eg.makeUser({ name: 'some name' }), email: 'another@example.com' }; - const user5 = { ...eg.makeUser({ name: 'Example' }), email: 'match@example.com' }; + const user1 = eg.makeUser({ name: 'match', email: 'any1@example.com' }); + const user2 = eg.makeUser({ name: 'match this', email: 'any2@example.com' }); + const user3 = eg.makeUser({ name: 'MaTcH Case Insensitive', email: 'any3@example.com' }); + const user4 = eg.makeUser({ name: 'some name', email: 'another@example.com' }); + const user5 = eg.makeUser({ name: 'Example', email: 'match@example.com' }); const allUsers = deepFreeze([user1, user2, user3, user4, user5]); const shouldMatch = [user1, user2, user3, user5]; @@ -116,17 +116,17 @@ describe('getAutocompleteSuggestion', () => { }); test('result should be in priority of startsWith, initials, contains in name, matches in email', () => { - const user1 = { ...eg.makeUser({ name: 'M Apple' }), email: 'any1@example.com' }; // satisfy initials condition - const user2 = { ...eg.makeUser({ name: 'Normal boy' }), email: 'any2@example.com' }; // satisfy full_name contains condition - const user3 = { ...eg.makeUser({ name: 'example' }), email: 'example@example.com' }; // random entry - const user4 = { ...eg.makeUser({ name: 'Example' }), email: 'match@example.com' }; // satisfy email match condition - const user5 = { ...eg.makeUser({ name: 'match' }), email: 'any@example.com' }; // satisfy full_name starts with condition - const user6 = { ...eg.makeUser({ name: 'match' }), email: 'normal@example.com' }; // satisfy starts with and email condition - const user7 = { ...eg.makeUser({ name: 'Match App Normal' }), email: 'any3@example.com' }; // satisfy all conditions - const user8 = { ...eg.makeUser({ name: 'match' }), email: 'any@example.com' }; // duplicate - const user9 = { ...eg.makeUser({ name: 'Laptop' }), email: 'laptop@example.com' }; // random entry - const user10 = { ...eg.makeUser({ name: 'Mobile App' }), email: 'any@match.com' }; // satisfy initials and email condition - const user11 = { ...eg.makeUser({ name: 'Normal' }), email: 'match2@example.com' }; // satisfy contains in name and matches in email condition + const user1 = eg.makeUser({ name: 'M Apple', email: 'any1@example.com' }); // satisfy initials condition + const user2 = eg.makeUser({ name: 'Normal boy', email: 'any2@example.com' }); // satisfy full_name contains condition + const user3 = eg.makeUser({ name: 'example', email: 'example@example.com' }); // random entry + const user4 = eg.makeUser({ name: 'Example', email: 'match@example.com' }); // satisfy email match condition + const user5 = eg.makeUser({ name: 'match', email: 'any@example.com' }); // satisfy full_name starts with condition + const user6 = eg.makeUser({ name: 'match', email: 'normal@example.com' }); // satisfy starts with and email condition + const user7 = eg.makeUser({ name: 'Match App Normal', email: 'any3@example.com' }); // satisfy all conditions + const user8 = eg.makeUser({ name: 'match', email: 'any@example.com' }); // duplicate + const user9 = eg.makeUser({ name: 'Laptop', email: 'laptop@example.com' }); // random entry + const user10 = eg.makeUser({ name: 'Mobile App', email: 'any@match.com' }); // satisfy initials and email condition + const user11 = eg.makeUser({ name: 'Normal', email: 'match2@example.com' }); // satisfy contains in name and matches in email condition const allUsers = deepFreeze([ user1, user2, @@ -199,10 +199,10 @@ describe('sortUserList', () => { }); test('prioritizes status', () => { - const user1 = { ...eg.makeUser({ name: 'Mark' }), email: 'mark@example.com' }; - const user2 = { ...eg.makeUser({ name: 'John' }), email: 'john@example.com' }; - const user3 = { ...eg.makeUser({ name: 'Bob' }), email: 'bob@example.com' }; - const user4 = { ...eg.makeUser({ name: 'Rick' }), email: 'rick@example.com' }; + const user1 = eg.makeUser({ name: 'Mark', email: 'mark@example.com' }); + const user2 = eg.makeUser({ name: 'John', email: 'john@example.com' }); + const user3 = eg.makeUser({ name: 'Bob', email: 'bob@example.com' }); + const user4 = eg.makeUser({ name: 'Rick', email: 'rick@example.com' }); const users = deepFreeze([user1, user2, user3, user4]); const presences = { [user1.email]: { @@ -232,13 +232,13 @@ describe('sortUserList', () => { describe('sortAlphabetically', () => { test('alphabetically sort user list by full_name', () => { - const user1 = { ...eg.makeUser({ name: 'zoe' }), email: 'allen@example.com' }; - const user2 = { ...eg.makeUser({ name: 'Ring' }), email: 'got@example.com' }; - const user3 = { ...eg.makeUser({ name: 'watch' }), email: 'see@example.com' }; - const user4 = { ...eg.makeUser({ name: 'mobile' }), email: 'phone@example.com' }; - const user5 = { ...eg.makeUser({ name: 'Ring' }), email: 'got@example.com' }; - const user6 = { ...eg.makeUser({ name: 'hardware' }), email: 'software@example.com' }; - const user7 = { ...eg.makeUser({ name: 'Bob' }), email: 'tester@example.com' }; + const user1 = eg.makeUser({ name: 'zoe', email: 'allen@example.com' }); + const user2 = eg.makeUser({ name: 'Ring', email: 'got@example.com' }); + const user3 = eg.makeUser({ name: 'watch', email: 'see@example.com' }); + const user4 = eg.makeUser({ name: 'mobile', email: 'phone@example.com' }); + const user5 = eg.makeUser({ name: 'Ring', email: 'got@example.com' }); + const user6 = eg.makeUser({ name: 'hardware', email: 'software@example.com' }); + const user7 = eg.makeUser({ name: 'Bob', email: 'tester@example.com' }); const users = deepFreeze([user1, user2, user3, user4, user5, user6, user7]); const expectedUsers = [user7, user6, user4, user2, user5, user3, user1]; @@ -248,12 +248,12 @@ describe('sortAlphabetically', () => { describe('filterUserStartWith', () => { test('returns users whose name starts with filter excluding self', () => { - const user1 = { ...eg.makeUser({ name: 'Apple' }), email: 'a@example.com' }; - const user2 = { ...eg.makeUser({ name: 'bob' }), email: 'f@app.com' }; - const user3 = { ...eg.makeUser({ name: 'app' }), email: 'p@p.com' }; - const user4 = { ...eg.makeUser({ name: 'Mobile app' }), email: 'p3@p.com' }; - const user5 = { ...eg.makeUser({ name: 'Mac App' }), email: 'p@p2.com' }; - const selfUser = { ...eg.makeUser({ name: 'app' }), email: 'own@example.com' }; + const user1 = eg.makeUser({ name: 'Apple', email: 'a@example.com' }); + const user2 = eg.makeUser({ name: 'bob', email: 'f@app.com' }); + const user3 = eg.makeUser({ name: 'app', email: 'p@p.com' }); + const user4 = eg.makeUser({ name: 'Mobile app', email: 'p3@p.com' }); + const user5 = eg.makeUser({ name: 'Mac App', email: 'p@p2.com' }); + const selfUser = eg.makeUser({ name: 'app', email: 'own@example.com' }); const users = deepFreeze([user1, user2, user3, user4, user5, selfUser]); const expectedUsers = [user1, user3]; @@ -263,13 +263,13 @@ describe('filterUserStartWith', () => { describe('filterUserByInitials', () => { test('returns users whose full_name initials matches filter excluding self', () => { - const user1 = { ...eg.makeUser({ name: 'Apple' }), email: 'a@example.com' }; - const user2 = { ...eg.makeUser({ name: 'mam' }), email: 'f@app.com' }; - const user3 = { ...eg.makeUser({ name: 'app' }), email: 'p@p.com' }; - const user4 = { ...eg.makeUser({ name: 'Mobile Application' }), email: 'p3@p.com' }; - const user5 = { ...eg.makeUser({ name: 'Mac App' }), email: 'p@p2.com' }; - const user6 = { ...eg.makeUser({ name: 'app' }), email: 'p@p.com' }; - const selfUser = { ...eg.makeUser({ name: 'app' }), email: 'own@example.com' }; + const user1 = eg.makeUser({ name: 'Apple', email: 'a@example.com' }); + const user2 = eg.makeUser({ name: 'mam', email: 'f@app.com' }); + const user3 = eg.makeUser({ name: 'app', email: 'p@p.com' }); + const user4 = eg.makeUser({ name: 'Mobile Application', email: 'p3@p.com' }); + const user5 = eg.makeUser({ name: 'Mac App', email: 'p@p2.com' }); + const user6 = eg.makeUser({ name: 'app', email: 'p@p.com' }); + const selfUser = eg.makeUser({ name: 'app', email: 'own@example.com' }); const users = deepFreeze([user1, user2, user3, user4, user5, user6, selfUser]); @@ -288,10 +288,10 @@ describe('groupUsersByStatus', () => { }); test('sort input by status, when no presence entry consider offline', () => { - const user1 = { ...eg.makeUser(), email: 'allen@example.com' }; - const user2 = { ...eg.makeUser(), email: 'bob@example.com' }; - const user3 = { ...eg.makeUser(), email: 'carter@example.com' }; - const user4 = { ...eg.makeUser(), email: 'dan@example.com' }; + const user1 = eg.makeUser({ email: 'allen@example.com' }); + const user2 = eg.makeUser({ email: 'bob@example.com' }); + const user3 = eg.makeUser({ email: 'carter@example.com' }); + const user4 = eg.makeUser({ email: 'dan@example.com' }); const users = deepFreeze([user1, user2, user3, user4]); const presence = { [user1.email]: { @@ -318,13 +318,13 @@ describe('groupUsersByStatus', () => { describe('filterUserThatContains', () => { test('returns users whose full_name contains filter excluding self', () => { - const user1 = { ...eg.makeUser({ name: 'Apple' }), email: 'a@example.com' }; - const user2 = { ...eg.makeUser({ name: 'mam' }), email: 'f@app.com' }; - const user3 = { ...eg.makeUser({ name: 'app' }), email: 'p@p.com' }; - const user4 = { ...eg.makeUser({ name: 'Mobile app' }), email: 'p3@p.com' }; - const user5 = { ...eg.makeUser({ name: 'Mac App' }), email: 'p@p2.com' }; - const user6 = { ...eg.makeUser({ name: 'app' }), email: 'p@p.com' }; - const selfUser = { ...eg.makeUser({ name: 'app' }), email: 'own@example.com' }; + const user1 = eg.makeUser({ name: 'Apple', email: 'a@example.com' }); + const user2 = eg.makeUser({ name: 'mam', email: 'f@app.com' }); + const user3 = eg.makeUser({ name: 'app', email: 'p@p.com' }); + const user4 = eg.makeUser({ name: 'Mobile app', email: 'p3@p.com' }); + const user5 = eg.makeUser({ name: 'Mac App', email: 'p@p2.com' }); + const user6 = eg.makeUser({ name: 'app', email: 'p@p.com' }); + const selfUser = eg.makeUser({ name: 'app', email: 'own@example.com' }); const users = deepFreeze([user1, user2, user3, user4, user5, user6, selfUser]); @@ -335,13 +335,13 @@ describe('filterUserThatContains', () => { describe('filterUserMatchesEmail', () => { test('returns users whose email matches filter excluding self', () => { - const user1 = { ...eg.makeUser({ name: 'Apple' }), email: 'a@example.com' }; - const user2 = { ...eg.makeUser({ name: 'mam' }), email: 'f@app.com' }; - const user3 = { ...eg.makeUser({ name: 'app' }), email: 'p@p.com' }; - const user4 = { ...eg.makeUser({ name: 'Mobile app' }), email: 'p3@p.com' }; - const user5 = { ...eg.makeUser({ name: 'Mac App' }), email: 'p@p2.com' }; - const user6 = { ...eg.makeUser({ name: 'app' }), email: 'p@p.com' }; - const selfUser = { ...eg.makeUser({ name: 'app' }), email: 'own@example.com' }; + const user1 = eg.makeUser({ name: 'Apple', email: 'a@example.com' }); + const user2 = eg.makeUser({ name: 'mam', email: 'f@app.com' }); + const user3 = eg.makeUser({ name: 'app', email: 'p@p.com' }); + const user4 = eg.makeUser({ name: 'Mobile app', email: 'p3@p.com' }); + const user5 = eg.makeUser({ name: 'Mac App', email: 'p@p2.com' }); + const user6 = eg.makeUser({ name: 'app', email: 'p@p.com' }); + const selfUser = eg.makeUser({ name: 'app', email: 'own@example.com' }); const users = deepFreeze([user1, user2, user3, user4, user5, user6, selfUser]); const expectedUsers = [user1]; @@ -351,14 +351,14 @@ describe('filterUserMatchesEmail', () => { describe('getUniqueUsers', () => { test('returns unique users check by email', () => { - const user1 = { ...eg.makeUser({ name: 'Apple' }), email: 'a@example.com' }; - const user2 = { ...eg.makeUser({ name: 'Apple' }), email: 'a@example.com' }; - const user3 = { ...eg.makeUser({ name: 'app' }), email: 'p@p.com' }; - const user4 = { ...eg.makeUser({ name: 'app' }), email: 'p@p.com' }; - const user5 = { ...eg.makeUser({ name: 'Mac App' }), email: 'p@p2.com' }; - const user6 = { ...eg.makeUser({ name: 'Mac App' }), email: 'p@p2.com' }; - const user7 = { ...eg.makeUser({ name: 'Mac App 2' }), email: 'p@p2.com' }; - const user8 = { ...eg.makeUser({ name: 'app' }), email: 'own@example.com' }; + const user1 = eg.makeUser({ name: 'Apple', email: 'a@example.com' }); + const user2 = eg.makeUser({ name: 'Apple', email: 'a@example.com' }); + const user3 = eg.makeUser({ name: 'app', email: 'p@p.com' }); + const user4 = eg.makeUser({ name: 'app', email: 'p@p.com' }); + const user5 = eg.makeUser({ name: 'Mac App', email: 'p@p2.com' }); + const user6 = eg.makeUser({ name: 'Mac App', email: 'p@p2.com' }); + const user7 = eg.makeUser({ name: 'Mac App 2', email: 'p@p2.com' }); + const user8 = eg.makeUser({ name: 'app', email: 'own@example.com' }); const users = deepFreeze([user1, user2, user3, user4, user5, user6, user7, user8]); diff --git a/src/webview/html/__tests__/render-test.js b/src/webview/html/__tests__/render-test.js index 7212d0cb7c6..16392effe72 100644 --- a/src/webview/html/__tests__/render-test.js +++ b/src/webview/html/__tests__/render-test.js @@ -5,11 +5,7 @@ import * as eg from '../../../__tests__/lib/exampleData'; describe('typing', () => { it('escapes &< (e.g., in `avatar_url` and `email`', () => { const name = '& Date: Mon, 7 Feb 2022 15:10:25 -0800 Subject: [PATCH 08/28] example data [nfc]: Use user_id parameter to makeUser Now all eg.makeUser call sites are specifying their extra properties in the argument, rather than with their own spread. --- src/unread/__tests__/unreadHuddlesReducer-test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/unread/__tests__/unreadHuddlesReducer-test.js b/src/unread/__tests__/unreadHuddlesReducer-test.js index ec9dee48087..70480822ca9 100644 --- a/src/unread/__tests__/unreadHuddlesReducer-test.js +++ b/src/unread/__tests__/unreadHuddlesReducer-test.js @@ -68,9 +68,9 @@ describe('unreadHuddlesReducer', () => { describe('EVENT_NEW_MESSAGE', () => { test('if message id already exists, do not mutate state', () => { - const user1 = { ...eg.makeUser(), user_id: makeUserId(1) }; - const user2 = { ...eg.makeUser(), user_id: makeUserId(2) }; - const user3 = { ...eg.makeUser(), user_id: makeUserId(3) }; + const user1 = eg.makeUser({ user_id: 1 }); + const user2 = eg.makeUser({ user_id: 2 }); + const user3 = eg.makeUser({ user_id: 3 }); const message1 = eg.pmMessage({ id: 1, recipients: [user1, user2, user3] }); const message2 = eg.pmMessage({ id: 2, recipients: [user1, user2, user3] }); From 9b90e4a25971e93c196b6e3114c9c8b7b56624c3 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 7 Feb 2022 15:13:01 -0800 Subject: [PATCH 09/28] example data [nfc]: In makeUser, take avatar_url, and use it --- src/__tests__/lib/exampleData.js | 6 +++++- src/message/__tests__/fetchActions-test.js | 6 +++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index beb6408f007..545b3cf8ec3 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -34,6 +34,7 @@ import type { import type { Auth, Account, StreamOutbox } from '../../types'; import { dubJointState } from '../../reduxTypes'; import { UploadedAvatarURL } from '../../utils/avatar'; +import type { AvatarURL } from '../../utils/avatar'; import { ZulipVersion } from '../../utils/zulipVersion'; import { ACCOUNT_SWITCH, @@ -126,6 +127,7 @@ type UserOrBotPropertiesArgs = {| name?: string, user_id?: number, // accept a plain number, for convenience in tests email?: string, + avatar_url?: AvatarURL, |}; const randUserId: () => UserId = (mk => () => makeUserId(mk()))( @@ -139,7 +141,9 @@ const userOrBotProperties = (args: UserOrBotPropertiesArgs) => { // Internally the UploadedAvatarURL mutates itself for memoization. // That conflicts with the deepFreeze we do for tests; so construct it // here with a full-blown URL object in the first place to prevent that. - avatar_url: new UploadedAvatarURL(new URL(`https://zulip.example.org/yo/avatar-${name}.png`)), + avatar_url: + args.avatar_url + ?? new UploadedAvatarURL(new URL(`https://zulip.example.org/yo/avatar-${name}.png`)), date_joined: `2014-04-${randInt(30) .toString() diff --git a/src/message/__tests__/fetchActions-test.js b/src/message/__tests__/fetchActions-test.js index 1a8db21baa6..bef30292c75 100644 --- a/src/message/__tests__/fetchActions-test.js +++ b/src/message/__tests__/fetchActions-test.js @@ -230,10 +230,10 @@ describe('fetchActions', () => { describe('fetchMessages', () => { const email = 'abc123@example.com'; - const sender = { - ...eg.makeUser({ email }), + const sender = eg.makeUser({ + email, avatar_url: GravatarURL.validateAndConstructInstance({ email }), - }; + }); const message1 = eg.streamMessage({ id: 1, sender }); // message1 exactly as we receive it from the server, before our From ad3fca7ac20ea921df788cd876ee56326a439fc4 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 7 Feb 2022 14:45:26 -0800 Subject: [PATCH 10/28] example data [nfc]: In makeSubscription, take color and pin_to_top, and use We already have one test where we want to customize `color`; we'll add more soon, plus some for `pin_to_top`. --- src/__tests__/lib/exampleData.js | 15 ++++++++++----- .../__tests__/subscriptionSelectors-test.js | 2 +- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index 545b3cf8ec3..804a09c3916 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -276,14 +276,19 @@ export const otherStream: Stream = makeStream({ /** A subscription, by default to eg.stream. */ export const makeSubscription = ( - args: {| stream?: Stream, in_home_view?: boolean |} = Object.freeze({}), + args: {| + stream?: Stream, + in_home_view?: boolean, + pin_to_top?: boolean, + color?: string, + |} = Object.freeze({}), ): Subscription => { - const { stream: streamInner = stream, in_home_view } = args; + const { stream: streamInner = stream } = args; return deepFreeze({ ...streamInner, - color: '#123456', - in_home_view: in_home_view ?? true, - pin_to_top: false, + color: args.color ?? '#123456', + in_home_view: args.in_home_view ?? true, + pin_to_top: args.pin_to_top ?? false, audible_notifications: false, desktop_notifications: false, email_address: '??? make this value representative before using in a test :)', diff --git a/src/subscriptions/__tests__/subscriptionSelectors-test.js b/src/subscriptions/__tests__/subscriptionSelectors-test.js index d65af205558..4864ceb4a50 100644 --- a/src/subscriptions/__tests__/subscriptionSelectors-test.js +++ b/src/subscriptions/__tests__/subscriptionSelectors-test.js @@ -80,7 +80,7 @@ describe('getIsActiveStreamSubscribed', () => { describe('getStreamColorForNarrow', () => { const exampleColor = '#fff'; const state = eg.reduxState({ - subscriptions: [{ ...eg.makeSubscription({ stream: eg.stream }), color: exampleColor }], + subscriptions: [eg.makeSubscription({ stream: eg.stream, color: exampleColor })], }); test('return stream color for stream and topic narrow', () => { From 4c057ff71d68cf96a78e8f2135b14eb34b705bbe Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 7 Feb 2022 15:16:35 -0800 Subject: [PATCH 11/28] example data [nfc]: Use in_home_view parameter to makeSubscription Now all eg.makeSubscription call sites are using the argument to customize the result, rather than spreading it. --- src/chat/__tests__/narrowsSelectors-test.js | 4 ++-- src/subscriptions/__tests__/subscriptionsReducer-test.js | 7 +------ 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/src/chat/__tests__/narrowsSelectors-test.js b/src/chat/__tests__/narrowsSelectors-test.js index 690acef7c9d..5103edaa891 100644 --- a/src/chat/__tests__/narrowsSelectors-test.js +++ b/src/chat/__tests__/narrowsSelectors-test.js @@ -297,8 +297,8 @@ describe('getStreamInNarrow', () => { const stream2 = eg.makeStream({ name: 'stream2' }); const stream3 = eg.makeStream({ name: 'stream3' }); const stream4 = eg.makeStream({ name: 'stream4' }); - const sub1 = { ...eg.makeSubscription({ stream: stream1 }), in_home_view: false }; - const sub2 = { ...eg.makeSubscription({ stream: stream2 }), in_home_view: true }; + const sub1 = eg.makeSubscription({ stream: stream1, in_home_view: false }); + const sub2 = eg.makeSubscription({ stream: stream2, in_home_view: true }); const state = eg.reduxState({ streams: [stream1, stream2, stream3], diff --git a/src/subscriptions/__tests__/subscriptionsReducer-test.js b/src/subscriptions/__tests__/subscriptionsReducer-test.js index 20c32067122..55b0f485a22 100644 --- a/src/subscriptions/__tests__/subscriptionsReducer-test.js +++ b/src/subscriptions/__tests__/subscriptionsReducer-test.js @@ -133,12 +133,7 @@ describe('subscriptionsReducer', () => { describe('EVENT_SUBSCRIPTION -> update', () => { test('Change the in_home_view property', () => { - const subNotInHomeView = deepFreeze({ - ...eg.makeSubscription({ - stream: stream1, - }), - in_home_view: false, - }); + const subNotInHomeView = eg.makeSubscription({ stream: stream1, in_home_view: false }); const initialState = deepFreeze([subNotInHomeView, sub2, sub3]); From 1cbc92f44b6787263eb9bd3bdcde5b8509766478 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 7 Feb 2022 14:05:33 -0800 Subject: [PATCH 12/28] unread tests: Well-type unreadSelectors tests, using example data --- src/unread/__tests__/unreadSelectors-test.js | 207 +++++-------------- 1 file changed, 51 insertions(+), 156 deletions(-) diff --git a/src/unread/__tests__/unreadSelectors-test.js b/src/unread/__tests__/unreadSelectors-test.js index a319c247f0d..30d0b00539d 100644 --- a/src/unread/__tests__/unreadSelectors-test.js +++ b/src/unread/__tests__/unreadSelectors-test.js @@ -1,4 +1,4 @@ -import deepFreeze from 'deep-freeze'; +/* @flow strict-local */ import { reducer } from '../unreadModel'; import { @@ -15,14 +15,15 @@ import { } from '../unreadSelectors'; import * as eg from '../../__tests__/lib/exampleData'; -import { initialState, selectorBaseState as unreadState } from './unread-testlib'; +import { initialState, selectorBaseState as unreadState, stream0, stream2 } from './unread-testlib'; + +const subscription0 = eg.makeSubscription({ stream: stream0, color: 'red' }); +const subscription2 = eg.makeSubscription({ stream: stream2, color: 'blue' }); +const subscriptions = [subscription0, subscription2]; describe('getUnreadByStream', () => { test('when no items in streams key, the result is an empty object', () => { - const state = deepFreeze({ - subscriptions: [], - unread: initialState, - }); + const state = eg.reduxState(); const unreadByStream = getUnreadByStream(state); @@ -30,19 +31,8 @@ describe('getUnreadByStream', () => { }); test('when there are unread stream messages, returns their counts', () => { - const state = deepFreeze({ - subscriptions: [ - { - stream_id: 0, - name: 'stream 0', - in_home_view: true, - }, - { - stream_id: 2, - name: 'stream 2', - in_home_view: true, - }, - ], + const state = eg.reduxStatePlus({ + subscriptions, unread: unreadState, mute: [['stream 0', 'a topic']], }); @@ -55,7 +45,7 @@ describe('getUnreadByStream', () => { describe('getUnreadStreamTotal', () => { test('when no items in "streams" key, there are unread message', () => { - const state = deepFreeze({ + const state = eg.reduxStatePlus({ unread: initialState, subscriptions: [], mute: [], @@ -67,22 +57,8 @@ describe('getUnreadStreamTotal', () => { }); test('count all the unread messages listed in "streams" key', () => { - const state = deepFreeze({ + const state = eg.reduxStatePlus({ unread: unreadState, - subscriptions: [ - { - stream_id: 0, - in_home_view: true, - }, - { - stream_id: 0, - in_home_view: true, - }, - { - stream_id: 2, - in_home_view: true, - }, - ], mute: [], }); @@ -94,7 +70,7 @@ describe('getUnreadStreamTotal', () => { describe('getUnreadByPms', () => { test('when no items in streams key, the result is an empty array', () => { - const state = deepFreeze({ + const state = eg.reduxStatePlus({ unread: initialState, }); @@ -104,7 +80,7 @@ describe('getUnreadByPms', () => { }); test('when there are unread private messages, returns counts by sender_id', () => { - const state = deepFreeze({ + const state = eg.reduxStatePlus({ unread: unreadState, }); @@ -116,7 +92,7 @@ describe('getUnreadByPms', () => { describe('getUnreadPmsTotal', () => { test('when no items in "pms" key, there are unread private messages', () => { - const state = deepFreeze({ + const state = eg.reduxStatePlus({ unread: initialState, }); @@ -126,7 +102,7 @@ describe('getUnreadPmsTotal', () => { }); test('when there are keys in "pms", sum up all unread private message counts', () => { - const state = deepFreeze({ + const state = eg.reduxStatePlus({ unread: unreadState, }); @@ -138,7 +114,7 @@ describe('getUnreadPmsTotal', () => { describe('getUnreadByHuddles', () => { test('when no items in streams key, the result is an empty array', () => { - const state = deepFreeze({ + const state = eg.reduxStatePlus({ unread: initialState, }); @@ -148,7 +124,7 @@ describe('getUnreadByHuddles', () => { }); test('when there are unread stream messages, returns a ', () => { - const state = deepFreeze({ + const state = eg.reduxStatePlus({ unread: unreadState, }); @@ -160,7 +136,7 @@ describe('getUnreadByHuddles', () => { describe('getUnreadHuddlesTotal', () => { test('when no items in "huddles" key, there are unread group messages', () => { - const state = deepFreeze({ + const state = eg.reduxStatePlus({ unread: initialState, }); @@ -170,7 +146,7 @@ describe('getUnreadHuddlesTotal', () => { }); test('when there are keys in "huddles", sum up all unread group message counts', () => { - const state = deepFreeze({ + const state = eg.reduxStatePlus({ unread: unreadState, }); @@ -182,7 +158,7 @@ describe('getUnreadHuddlesTotal', () => { describe('getUnreadMentionsTotal', () => { test('unread mentions count is equal to the unread array length', () => { - const state = deepFreeze({ + const state = eg.reduxStatePlus({ unread: unreadState, }); @@ -194,7 +170,7 @@ describe('getUnreadMentionsTotal', () => { describe('getUnreadTotal', () => { test('if no key has any items then no unread messages', () => { - const state = deepFreeze({ + const state = eg.reduxStatePlus({ unread: initialState, subscriptions: [], mute: [], @@ -206,22 +182,8 @@ describe('getUnreadTotal', () => { }); test('calculates total unread of streams + pms + huddles', () => { - const state = deepFreeze({ + const state = eg.reduxStatePlus({ unread: unreadState, - subscriptions: [ - { - stream_id: 0, - in_home_view: true, - }, - { - stream_id: 0, - in_home_view: true, - }, - { - stream_id: 2, - in_home_view: true, - }, - ], mute: [], }); @@ -233,7 +195,7 @@ describe('getUnreadTotal', () => { describe('getUnreadStreamsAndTopics', () => { test('if no key has any items then no unread messages', () => { - const state = deepFreeze({ + const state = eg.reduxStatePlus({ subscriptions: [], unread: initialState, }); @@ -244,21 +206,8 @@ describe('getUnreadStreamsAndTopics', () => { }); test('muted streams are included', () => { - const state = deepFreeze({ - subscriptions: [ - { - stream_id: 0, - name: 'stream 0', - color: 'red', - in_home_view: false, - }, - { - stream_id: 2, - name: 'stream 2', - color: 'blue', - in_home_view: false, - }, - ], + const state = eg.reduxStatePlus({ + subscriptions: subscriptions.map(s => ({ ...s, in_home_view: false })), unread: unreadState, mute: [], }); @@ -285,6 +234,8 @@ describe('getUnreadStreamsAndTopics', () => { }, ], isMuted: true, + isPinned: false, + isPrivate: false, key: 'stream:stream 0', streamId: 0, streamName: 'stream 0', @@ -302,6 +253,8 @@ describe('getUnreadStreamsAndTopics', () => { }, ], isMuted: true, + isPinned: false, + isPrivate: false, key: 'stream:stream 2', streamId: 2, streamName: 'stream 2', @@ -311,21 +264,8 @@ describe('getUnreadStreamsAndTopics', () => { }); test('muted topics inside non muted streams are included', () => { - const state = deepFreeze({ - subscriptions: [ - { - stream_id: 0, - name: 'stream 0', - color: 'red', - in_home_view: true, - }, - { - stream_id: 2, - name: 'stream 2', - color: 'blue', - in_home_view: true, - }, - ], + const state = eg.reduxStatePlus({ + subscriptions, unread: unreadState, mute: [['stream 0', 'a topic']], }); @@ -352,7 +292,8 @@ describe('getUnreadStreamsAndTopics', () => { }, ], isMuted: false, - isPrivate: undefined, + isPinned: false, + isPrivate: false, key: 'stream:stream 0', streamId: 0, streamName: 'stream 0', @@ -370,6 +311,8 @@ describe('getUnreadStreamsAndTopics', () => { }, ], isMuted: false, + isPinned: false, + isPrivate: false, key: 'stream:stream 2', streamId: 2, streamName: 'stream 2', @@ -379,21 +322,8 @@ describe('getUnreadStreamsAndTopics', () => { }); test('group data by stream and topics inside, count unread', () => { - const state = deepFreeze({ - subscriptions: [ - { - stream_id: 0, - name: 'stream 0', - color: 'red', - in_home_view: true, - }, - { - stream_id: 2, - name: 'stream 2', - color: 'blue', - in_home_view: true, - }, - ], + const state = eg.reduxStatePlus({ + subscriptions, unread: unreadState, mute: [], }); @@ -408,6 +338,8 @@ describe('getUnreadStreamsAndTopics', () => { color: 'red', unread: 5, isMuted: false, + isPinned: false, + isPrivate: false, data: [ { key: 'another topic', @@ -426,6 +358,8 @@ describe('getUnreadStreamsAndTopics', () => { color: 'blue', unread: 2, isMuted: false, + isPinned: false, + isPrivate: false, data: [ { key: 'some other topic', @@ -440,32 +374,12 @@ describe('getUnreadStreamsAndTopics', () => { }); test('streams are sorted alphabetically, case-insensitive, topics by last activity, pinned stream on top', () => { + const stream1 = eg.makeStream({ name: 'xyz stream', stream_id: 1 }); const state = eg.reduxStatePlus({ subscriptions: [ - { - stream_id: 2, - color: 'green', - name: 'def stream', - in_home_view: true, - invite_only: false, - pin_to_top: false, - }, - { - stream_id: 1, - color: 'blue', - name: 'xyz stream', - in_home_view: true, - invite_only: false, - pin_to_top: true, - }, - { - stream_id: 0, - color: 'red', - name: 'abc stream', - in_home_view: true, - invite_only: false, - pin_to_top: false, - }, + { ...subscription2, color: 'green', name: 'def stream' }, + eg.makeSubscription({ stream: stream1, color: 'blue', pin_to_top: true }), + { ...subscription0, name: 'abc stream' }, ], unread: [ eg.streamMessage({ stream_id: 0, subject: 'z topic', id: 1 }), @@ -537,21 +451,8 @@ describe('getUnreadStreamsAndTopics', () => { describe('getUnreadStreamsAndTopicsSansMuted', () => { test('muted streams are not included', () => { - const state = deepFreeze({ - subscriptions: [ - { - stream_id: 0, - name: 'stream 0', - color: 'red', - in_home_view: false, - }, - { - stream_id: 2, - name: 'stream 2', - color: 'blue', - in_home_view: false, - }, - ], + const state = eg.reduxStatePlus({ + subscriptions: subscriptions.map(s => ({ ...s, in_home_view: false })), unread: unreadState, mute: [], }); @@ -562,15 +463,8 @@ describe('getUnreadStreamsAndTopicsSansMuted', () => { }); test('muted topics inside non muted streams are not included', () => { - const state = deepFreeze({ - subscriptions: [ - { - stream_id: 0, - name: 'stream 0', - color: 'red', - in_home_view: true, - }, - ], + const state = eg.reduxStatePlus({ + subscriptions: [subscription0], unread: unreadState, mute: [['stream 0', 'a topic']], }); @@ -590,7 +484,8 @@ describe('getUnreadStreamsAndTopicsSansMuted', () => { }, ], isMuted: false, - isPrivate: undefined, + isPinned: false, + isPrivate: false, key: 'stream:stream 0', streamId: 0, streamName: 'stream 0', From be105b4e957b067a1d9898c132aa1aaff6b21d39 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 7 Feb 2022 13:08:36 -0800 Subject: [PATCH 13/28] topic tests [nfc]: Fewer magic values, more exampleData --- src/topics/__tests__/topicsSelectors-test.js | 49 +++++++++----------- 1 file changed, 21 insertions(+), 28 deletions(-) diff --git a/src/topics/__tests__/topicsSelectors-test.js b/src/topics/__tests__/topicsSelectors-test.js index a0e9e6db4c1..b273a73e38c 100644 --- a/src/topics/__tests__/topicsSelectors-test.js +++ b/src/topics/__tests__/topicsSelectors-test.js @@ -6,7 +6,7 @@ import * as eg from '../../__tests__/lib/exampleData'; describe('getTopicsForNarrow', () => { test('when no topics return an empty list', () => { - const state = eg.reduxState(); + const state = eg.reduxStatePlus(); const topics = getTopicsForNarrow(state, HOME_NARROW); @@ -14,16 +14,14 @@ describe('getTopicsForNarrow', () => { }); test('when there are topics in the active narrow, return them as string array', () => { - const stream = eg.makeStream({ stream_id: 123, name: 'hello' }); - const state = eg.reduxState({ - streams: [stream], + const state = eg.reduxStatePlus({ topics: { // prettier-ignore - [stream.stream_id]: [{ name: 'hi', max_id: 123 }, { name: 'wow', max_id: 234 }] + [eg.stream.stream_id]: [{ name: 'hi', max_id: 123 }, { name: 'wow', max_id: 234 }] }, }); - const topics = getTopicsForNarrow(state, streamNarrow(stream.stream_id)); + const topics = getTopicsForNarrow(state, streamNarrow(eg.stream.stream_id)); expect(topics).toEqual(['hi', 'wow']); }); @@ -31,39 +29,31 @@ describe('getTopicsForNarrow', () => { describe('getTopicsForStream', () => { test('when no topics loaded for given stream return undefined', () => { - const state = eg.reduxState({ - streams: [], + const state = eg.reduxStatePlus({ topics: {}, - mute: [], }); - const topics = getTopicsForStream(state, 123); + const topics = getTopicsForStream(state, eg.stream.stream_id); expect(topics).toEqual(undefined); }); test('when topics loaded for given stream return them', () => { - const stream = eg.makeStream({ stream_id: 123, name: 'stream 123' }); - const state = eg.reduxState({ - streams: [stream], + const state = eg.reduxStatePlus({ topics: { - [stream.stream_id]: [{ name: 'topic', max_id: 456 }], + [eg.stream.stream_id]: [{ name: 'topic', max_id: 456 }], }, - mute: [], }); - const topics = getTopicsForStream(state, stream.stream_id); + const topics = getTopicsForStream(state, eg.stream.stream_id); expect(topics).toEqual([{ name: 'topic', max_id: 456, isMuted: false, unreadCount: 0 }]); }); test('Return list of topic object with isMuted, unreadCount, topic name and max id in it.', () => { - const stream = eg.makeStream({ stream_id: 1, name: 'stream 1' }); - const state = eg.reduxStatePlus({ - streams: [stream], topics: { - [stream.stream_id]: [ + [eg.stream.stream_id]: [ { name: 'topic 1', max_id: 5 }, { name: 'topic 2', max_id: 6 }, { name: 'topic 3', max_id: 7 }, @@ -71,14 +61,17 @@ describe('getTopicsForStream', () => { { name: 'topic 5', max_id: 9 }, ], }, - // prettier-ignore - mute: [['stream 1', 'topic 1'], ['stream 1', 'topic 3'], ['stream 2', 'topic 2']], + mute: [ + [eg.stream.name, 'topic 1'], + [eg.stream.name, 'topic 3'], + [eg.otherStream.name, 'topic 2'], + ], unread: [ - eg.streamMessage({ stream_id: 1, subject: 'topic 2', id: 1 }), - eg.streamMessage({ stream_id: 1, subject: 'topic 2', id: 5 }), - eg.streamMessage({ stream_id: 1, subject: 'topic 2', id: 6 }), - eg.streamMessage({ stream_id: 1, subject: 'topic 4', id: 7 }), - eg.streamMessage({ stream_id: 1, subject: 'topic 4', id: 8 }), + eg.streamMessage({ stream: eg.stream, subject: 'topic 2', id: 1 }), + eg.streamMessage({ stream: eg.stream, subject: 'topic 2', id: 5 }), + eg.streamMessage({ stream: eg.stream, subject: 'topic 2', id: 6 }), + eg.streamMessage({ stream: eg.stream, subject: 'topic 4', id: 7 }), + eg.streamMessage({ stream: eg.stream, subject: 'topic 4', id: 8 }), ].reduce( (st, message) => unreadReducer(st, eg.mkActionEventNewMessage(message), eg.plusReduxState), eg.plusReduxState.unread, @@ -92,7 +85,7 @@ describe('getTopicsForStream', () => { { name: 'topic 5', max_id: 9, isMuted: false, unreadCount: 0 }, ]; - const topics = getTopicsForStream(state, 1); + const topics = getTopicsForStream(state, eg.stream.stream_id); expect(topics).toEqual(expected); }); From 6ff0b9a281de036180b48db60586239f9bb020a2 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 7 Feb 2022 13:29:17 -0800 Subject: [PATCH 14/28] example data [nfc]: Make eg.backgroundData more directly from baseReduxState This decouples it from changes in the respective data structures for these various parts of our state. --- src/__tests__/lib/exampleData.js | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index 804a09c3916..7ccee635340 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -49,6 +49,7 @@ import { authOfAccount } from '../../account/accountMisc'; import { HOME_NARROW } from '../../utils/narrow'; import type { BackgroundData } from '../../webview/MessageList'; import { getSettings, getStreamsById, getSubscriptionsById } from '../../selectors'; +import { getGlobalSettings } from '../../directSelectors'; /* ======================================================================== * Utilities @@ -846,20 +847,20 @@ export const mkActionEventNewMessage = ( */ export const backgroundData: BackgroundData = deepFreeze({ - alertWords: [], - allImageEmojiById: action.register_complete.data.realm_emoji, + alertWords: baseReduxState.alertWords, + allImageEmojiById: {}, // in reality would also have :zulip: auth: selfAuth, debug: baseReduxState.session.debug, doNotMarkMessagesAsRead: baseReduxState.settings.doNotMarkMessagesAsRead, flags: baseReduxState.flags, allUsersById: new Map([[selfUser.user_id, selfUser]]), - mute: [], - mutedUsers: Immutable.Map(), + mute: baseReduxState.mute, + mutedUsers: baseReduxState.mutedUsers, ownUser: selfUser, streams: getStreamsById(baseReduxState), subscriptions: getSubscriptionsById(baseReduxState), unread: baseReduxState.unread, - theme: 'default', - twentyFourHourTime: false, + theme: getGlobalSettings(baseReduxState).theme, + twentyFourHourTime: baseReduxState.realm.twentyFourHourTime, userSettingStreamNotification: getSettings(baseReduxState).streamNotification, }); From 20d536b32888c6bb38b43088009929d072c6e731 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 7 Feb 2022 16:28:45 -0800 Subject: [PATCH 15/28] example data [nfc]: Factor out mkActionRegisterComplete Call sites updated automatically, like so: $ perl -i -0pe ' s<\{\s* \.\.\.eg\.action\.register_complete,\s* data:\s* \{\s* \.\.\.eg\.action\.register_complete\.data,\s* unread_msgs:\s* \{\s* \.\.\.eg\.action\.register_complete\.data\.unread_msgs,\s* (.*?) \},?\s* \},?\s* \}> xsg ' src/**/*.js $ perl -i -0pe ' s<\{\s* \.\.\.eg\.action\.register_complete,\s* data:\s* \{\s* \.\.\.eg\.action\.register_complete\.data, (.*?) \},?\s* \}> xsg ' src/**/*.js $ perl -i -0pe ' s<\bdeepFreeze\(\s* ( eg\.mkActionRegisterComplete\(\{ .*? \}\) ) ,?\s* \)> <$1>xsg ' src/**/*.js $ tools/fmt --- src/__tests__/lib/exampleData.js | 36 ++++++++++++++++++- src/account/__tests__/accountsReducer-test.js | 18 +++------- src/mute/__tests__/mutedUsersReducer-test.js | 8 ++--- .../__tests__/pmConversationsModel-test.js | 5 +-- src/session/__tests__/sessionReducer-test.js | 5 +-- .../__tests__/settingsReducer-test.js | 12 +++---- .../__tests__/subscriptionsReducer-test.js | 16 +++------ .../__tests__/unreadHuddlesReducer-test.js | 27 ++++++-------- .../__tests__/unreadMentionsReducer-test.js | 16 ++------- src/unread/__tests__/unreadModel-test.js | 31 +++++++--------- src/unread/__tests__/unreadPmsReducer-test.js | 27 ++++++-------- src/users/__tests__/usersReducer-test.js | 8 ++--- 12 files changed, 91 insertions(+), 118 deletions(-) diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index 7ccee635340..b064408a4b9 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -17,6 +17,7 @@ import type { UserId, } from '../../api/modelTypes'; import { makeUserId } from '../../api/idTypes'; +import type { InitialData } from '../../api/apiTypes'; import type { AccountSwitchAction, LoginSuccessAction, @@ -628,7 +629,15 @@ export const action = Object.freeze({ apiKey: selfAccount.apiKey, }): LoginSuccessAction), - /** Beware! Data contained may not be representative. */ + /** + * A minimal well-typed REGISTER_COMPLETE action. + * + * Beware! The data here may not be representative. Generally each test + * should specify the particular properties that it cares about. + * + * See also `eg.mkActionRegisterComplete`, for combining this data with + * test-specific other data. + */ register_complete: (deepFreeze({ type: REGISTER_COMPLETE, data: { @@ -778,6 +787,7 @@ export const action = Object.freeze({ user_status: {}, }, }): RegisterCompleteAction), + message_fetch_start: (deepFreeze({ type: MESSAGE_FETCH_START, narrow: HOME_NARROW, @@ -816,6 +826,30 @@ export const action = Object.freeze({ * each test might as well define the action values it needs directly. */ +/** + * A REGISTER_COMPLETE action. + * + * The result will be based on `eg.action.register_complete`, but with the + * additional data given in the argument. + */ +export const mkActionRegisterComplete = (extra: {| + ...$Rest, + unread_msgs?: $Rest<$PropertyType, { ... }>, +|}): PerAccountAction => { + const { unread_msgs, ...rest } = extra; + return deepFreeze({ + ...action.register_complete, + data: { + ...action.register_complete.data, + ...rest, + unread_msgs: { + ...action.register_complete.data.unread_msgs, + ...unread_msgs, + }, + }, + }); +}; + /** * An EVENT_NEW_MESSAGE action. * diff --git a/src/account/__tests__/accountsReducer-test.js b/src/account/__tests__/accountsReducer-test.js index 5ea8272edff..2c90139e75f 100644 --- a/src/account/__tests__/accountsReducer-test.js +++ b/src/account/__tests__/accountsReducer-test.js @@ -25,10 +25,7 @@ describe('accountsReducer', () => { expect( accountsReducer( deepFreeze([account1, account2, account3]), - deepFreeze({ - ...eg.action.register_complete, - data: { ...eg.action.register_complete.data, zulip_version: newZulipVersion.raw() }, - }), + eg.mkActionRegisterComplete({ zulip_version: newZulipVersion.raw() }), ), ).toEqual([{ ...account1, zulipVersion: newZulipVersion }, account2, account3]); }); @@ -38,10 +35,7 @@ describe('accountsReducer', () => { expect( accountsReducer( deepFreeze([account1, account2, account3]), - deepFreeze({ - ...eg.action.register_complete, - data: { ...eg.action.register_complete.data, user_id: newUserId }, - }), + eg.mkActionRegisterComplete({ user_id: newUserId }), ), ).toEqual([{ ...account1, userId: newUserId }, account2, account3]); }); @@ -51,12 +45,8 @@ describe('accountsReducer', () => { expect( accountsReducer( deepFreeze([account1, account2, account3]), - deepFreeze({ - ...eg.action.register_complete, - data: { - ...eg.action.register_complete.data, - zulip_feature_level: newZulipFeatureLevel, - }, + eg.mkActionRegisterComplete({ + zulip_feature_level: newZulipFeatureLevel, }), ), ).toEqual([{ ...account1, zulipFeatureLevel: newZulipFeatureLevel }, account2, account3]); diff --git a/src/mute/__tests__/mutedUsersReducer-test.js b/src/mute/__tests__/mutedUsersReducer-test.js index 098d777a57d..24c65f13aeb 100644 --- a/src/mute/__tests__/mutedUsersReducer-test.js +++ b/src/mute/__tests__/mutedUsersReducer-test.js @@ -11,12 +11,8 @@ describe('mutedUsersReducer', () => { describe('REGISTER_COMPLETE', () => { test('when `muted_users` data is provided init state with it', () => { - const action = deepFreeze({ - ...eg.action.register_complete, - data: { - ...eg.action.register_complete.data, - muted_users: [{ id: eg.otherUser.user_id, timestamp: 1618822632 }], - }, + const action = eg.mkActionRegisterComplete({ + muted_users: [{ id: eg.otherUser.user_id, timestamp: 1618822632 }], }); expect(mutedUsersReducer(eg.baseReduxState.mutedUsers, action)).toEqual( Immutable.Map([[eg.otherUser.user_id, 1618822632]]), diff --git a/src/pm-conversations/__tests__/pmConversationsModel-test.js b/src/pm-conversations/__tests__/pmConversationsModel-test.js index ddb2a6e6aef..b24db846bc1 100644 --- a/src/pm-conversations/__tests__/pmConversationsModel-test.js +++ b/src/pm-conversations/__tests__/pmConversationsModel-test.js @@ -45,10 +45,7 @@ describe('reducer', () => { sorted: Immutable.List(['1,2', '', '1']), }; expect( - reducer(someState, { - ...eg.action.register_complete, - data: { ...eg.action.register_complete.data, recent_private_conversations }, - }), + reducer(someState, eg.mkActionRegisterComplete({ recent_private_conversations })), ).toEqual(expected); }); }); diff --git a/src/session/__tests__/sessionReducer-test.js b/src/session/__tests__/sessionReducer-test.js index 2892c2ba4e1..57be0fe3b91 100644 --- a/src/session/__tests__/sessionReducer-test.js +++ b/src/session/__tests__/sessionReducer-test.js @@ -60,10 +60,7 @@ describe('sessionReducer', () => { test('REGISTER_COMPLETE', () => { const state = deepFreeze({ ...baseState, needsInitialFetch: true, loading: true }); - const action = deepFreeze({ - ...eg.action.register_complete, - data: { ...eg.action.register_complete.data, queue_id: '100' }, - }); + const action = eg.mkActionRegisterComplete({ queue_id: '100' }); const newState = sessionReducer(state, action); expect(newState).toEqual({ ...baseState, diff --git a/src/settings/__tests__/settingsReducer-test.js b/src/settings/__tests__/settingsReducer-test.js index 573c481360e..a1c54e7c14b 100644 --- a/src/settings/__tests__/settingsReducer-test.js +++ b/src/settings/__tests__/settingsReducer-test.js @@ -20,14 +20,10 @@ describe('settingsReducer', () => { streamNotification: false, }); - const action = deepFreeze({ - ...eg.action.register_complete, - data: { - ...eg.action.register_complete.data, - enable_offline_push_notifications: true, - enable_online_push_notifications: true, - enable_stream_push_notifications: true, - }, + const action = eg.mkActionRegisterComplete({ + enable_offline_push_notifications: true, + enable_online_push_notifications: true, + enable_stream_push_notifications: true, }); const actualState = settingsReducer(prevState, action); diff --git a/src/subscriptions/__tests__/subscriptionsReducer-test.js b/src/subscriptions/__tests__/subscriptionsReducer-test.js index 55b0f485a22..7be94f6ccfe 100644 --- a/src/subscriptions/__tests__/subscriptionsReducer-test.js +++ b/src/subscriptions/__tests__/subscriptionsReducer-test.js @@ -21,12 +21,8 @@ describe('subscriptionsReducer', () => { test('when `subscriptions` data is provided init state with it', () => { const initialState = deepFreeze([]); - const action = deepFreeze({ - ...eg.action.register_complete, - data: { - ...eg.action.register_complete.data, - subscriptions: [sub1], - }, + const action = eg.mkActionRegisterComplete({ + subscriptions: [sub1], }); const actualState = subscriptionsReducer(initialState, action); @@ -37,12 +33,8 @@ describe('subscriptionsReducer', () => { test('when `subscriptions` is an empty array, reset state', () => { const initialState = deepFreeze([sub1]); - const action = deepFreeze({ - ...eg.action.register_complete, - data: { - ...eg.action.register_complete.data, - subscriptions: [], - }, + const action = eg.mkActionRegisterComplete({ + subscriptions: [], }); const expectedState = []; diff --git a/src/unread/__tests__/unreadHuddlesReducer-test.js b/src/unread/__tests__/unreadHuddlesReducer-test.js index 70480822ca9..3a4d5fdebe5 100644 --- a/src/unread/__tests__/unreadHuddlesReducer-test.js +++ b/src/unread/__tests__/unreadHuddlesReducer-test.js @@ -34,22 +34,17 @@ describe('unreadHuddlesReducer', () => { test('received data from "unread_msgs.huddles" key replaces the current state ', () => { const initialState = deepFreeze([]); - const action = deepFreeze({ - ...eg.action.register_complete, - data: { - ...eg.action.register_complete.data, - unread_msgs: { - ...eg.action.register_complete.data.unread_msgs, - streams: [], - huddles: [ - { - user_ids_string: '0,1,2', - unread_message_ids: [1, 2, 4, 5], - }, - ], - pms: [], - mentions: [1, 2, 3], - }, + const action = eg.mkActionRegisterComplete({ + unread_msgs: { + streams: [], + huddles: [ + { + user_ids_string: '0,1,2', + unread_message_ids: [1, 2, 4, 5], + }, + ], + pms: [], + mentions: [1, 2, 3], }, }); diff --git a/src/unread/__tests__/unreadMentionsReducer-test.js b/src/unread/__tests__/unreadMentionsReducer-test.js index 7cc53653393..19951502301 100644 --- a/src/unread/__tests__/unreadMentionsReducer-test.js +++ b/src/unread/__tests__/unreadMentionsReducer-test.js @@ -29,19 +29,9 @@ describe('unreadMentionsReducer', () => { test('received data from "unread_msgs.mentioned" key replaces the current state ', () => { const initialState = deepFreeze([]); - const action = { - ...eg.action.register_complete, - data: { - ...eg.action.register_complete.data, - unread_msgs: { - ...eg.action.register_complete.data.unread_msgs, - streams: [], - huddles: [], - pms: [], - mentions: [1, 2, 3], - }, - }, - }; + const action = eg.mkActionRegisterComplete({ + unread_msgs: { streams: [], huddles: [], pms: [], mentions: [1, 2, 3] }, + }); const expectedState = [1, 2, 3]; diff --git a/src/unread/__tests__/unreadModel-test.js b/src/unread/__tests__/unreadModel-test.js index 2e5f3084dd2..fbc27025a7a 100644 --- a/src/unread/__tests__/unreadModel-test.js +++ b/src/unread/__tests__/unreadModel-test.js @@ -40,25 +40,20 @@ describe('stream substate', () => { test('received data from "unread_msgs.streams" key replaces the current state ', () => { const message1 = eg.streamMessage({ id: 1 }); - const action = { - ...eg.action.register_complete, - data: { - ...eg.action.register_complete.data, - unread_msgs: { - ...eg.action.register_complete.data.unread_msgs, - streams: [ - { - stream_id: message1.stream_id, - topic: message1.subject, - unread_message_ids: [message1.id, 2], - }, - ], - huddles: [], - pms: [], - mentions: [message1.id, 2, 3], - }, + const action = eg.mkActionRegisterComplete({ + unread_msgs: { + streams: [ + { + stream_id: message1.stream_id, + topic: message1.subject, + unread_message_ids: [message1.id, 2], + }, + ], + huddles: [], + pms: [], + mentions: [message1.id, 2, 3], }, - }; + }); // prettier-ignore expect(summary(reducer(initialState, action, eg.plusReduxState))).toEqual(Immutable.Map([ diff --git a/src/unread/__tests__/unreadPmsReducer-test.js b/src/unread/__tests__/unreadPmsReducer-test.js index b049fb35d2d..deb1c07f89a 100644 --- a/src/unread/__tests__/unreadPmsReducer-test.js +++ b/src/unread/__tests__/unreadPmsReducer-test.js @@ -40,22 +40,17 @@ describe('unreadPmsReducer', () => { const message4 = eg.pmMessage({ id: 4, sender_id: 1 }); const message5 = eg.pmMessage({ id: 5, sender_id: 1 }); - const action = deepFreeze({ - ...eg.action.register_complete, - data: { - ...eg.action.register_complete.data, - unread_msgs: { - ...eg.action.register_complete.data.unread_msgs, - streams: [], - huddles: [], - pms: [ - { - sender_id: message1.sender_id, - unread_message_ids: [message1.id, message2.id, message4.id, message5.id], - }, - ], - mentions: [message1.id, message2.id, message3.id], - }, + const action = eg.mkActionRegisterComplete({ + unread_msgs: { + streams: [], + huddles: [], + pms: [ + { + sender_id: message1.sender_id, + unread_message_ids: [message1.id, message2.id, message4.id, message5.id], + }, + ], + mentions: [message1.id, message2.id, message3.id], }, }); diff --git a/src/users/__tests__/usersReducer-test.js b/src/users/__tests__/usersReducer-test.js index e2c2830cb15..b7eecad6c09 100644 --- a/src/users/__tests__/usersReducer-test.js +++ b/src/users/__tests__/usersReducer-test.js @@ -14,12 +14,8 @@ describe('usersReducer', () => { test('when `users` data is provided init state with it', () => { const prevState = deepFreeze([]); - const action = deepFreeze({ - ...eg.action.register_complete, - data: { - ...eg.action.register_complete.data, - realm_users: [user1], - }, + const action = eg.mkActionRegisterComplete({ + realm_users: [user1], }); const actualState = usersReducer(prevState, action); From 20b406750b67695f27bbd72e89ebd3a06ca3d2f0 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 31 Jan 2022 21:49:58 -0800 Subject: [PATCH 16/28] message [nfc]: Cut unused mute/subscription inputs to getFirstUnreadIdInNarrow The work these used to do got moved a while ago into getShownMessagesForNarrow. --- src/message/messageSelectors.js | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/message/messageSelectors.js b/src/message/messageSelectors.js index 3be52f5a0c4..32efd0156a3 100644 --- a/src/message/messageSelectors.js +++ b/src/message/messageSelectors.js @@ -3,13 +3,7 @@ import { createSelector, defaultMemoize } from 'reselect'; import invariant from 'invariant'; import type { Message, PmMessage, Outbox, Narrow, Selector, MessageListElement } from '../types'; -import { - getAllNarrows, - getFlags, - getMessages, - getMute, - getSubscriptions, -} from '../directSelectors'; +import { getAllNarrows, getFlags, getMessages } from '../directSelectors'; import * as logging from '../utils/logging'; import { getShownMessagesForNarrow } from '../chat/narrowsSelectors'; import getMessageListElements from './getMessageListElements'; @@ -79,9 +73,7 @@ export const getMessageListElementsMemoized: ( export const getFirstUnreadIdInNarrow: Selector = createSelector( (state, narrow) => getShownMessagesForNarrow(state, narrow), getFlags, - getSubscriptions, - getMute, - (messages, flags, subscriptions, mute) => { + (messages, flags) => { const firstUnread = messages.find(msg => !flags.read[msg.id]); return firstUnread?.id ?? null; }, From 9555104845443a1370e031736e30fd8b15bc815f Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 7 Feb 2022 16:52:36 -0800 Subject: [PATCH 17/28] mute tests: Well-type muteReducer tests -- and fix complete nonsense This is some legacy code that's been barely touched since 2017. And it turns out... not a single one of these test cases has the data in the form it actually appears in. Or for that matter in a form the data *could* possibly appear in in order for this feature to work. Happily, when we bring the type-checker to bear on this file, it immediately tells us about that! Do so, and fix the brokenness. --- src/mute/__tests__/muteReducer-test.js | 31 ++++++++++---------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/src/mute/__tests__/muteReducer-test.js b/src/mute/__tests__/muteReducer-test.js index 1331e08f29f..f85ff69e95a 100644 --- a/src/mute/__tests__/muteReducer-test.js +++ b/src/mute/__tests__/muteReducer-test.js @@ -1,30 +1,24 @@ +/* @flow strict-local */ import deepFreeze from 'deep-freeze'; import muteReducer from '../muteReducer'; -import { ACCOUNT_SWITCH, EVENT_MUTED_TOPICS, REGISTER_COMPLETE } from '../../actionConstants'; +import { EVENT_MUTED_TOPICS } from '../../actionConstants'; +import * as eg from '../../__tests__/lib/exampleData'; describe('muteReducer', () => { describe('REGISTER_COMPLETE', () => { test('when `mute` data is provided init state with it', () => { const initialState = deepFreeze([]); - const action = deepFreeze({ - type: REGISTER_COMPLETE, - data: { - muted_topics: [['stream'], ['topic']], - }, - }); + const action = eg.mkActionRegisterComplete({ muted_topics: [['stream', 'topic']] }); const actualState = muteReducer(initialState, action); - expect(actualState).toEqual([['stream'], ['topic']]); + expect(actualState).toEqual([['stream', 'topic']]); }); test('when no `mute` data is given reset state', () => { - const initialState = deepFreeze([['stream'], ['topic']]); - const action = deepFreeze({ - type: REGISTER_COMPLETE, - data: {}, - }); + const initialState = deepFreeze([['stream', 'topic']]); + const action = eg.mkActionRegisterComplete({ muted_topics: [] }); const expectedState = []; const actualState = muteReducer(initialState, action); @@ -35,11 +29,9 @@ describe('muteReducer', () => { describe('ACCOUNT_SWITCH', () => { test('resets state to initial state', () => { - const initialState = deepFreeze(['some_topic']); + const initialState = deepFreeze([['stream', 'some_topic']]); - const action = deepFreeze({ - type: ACCOUNT_SWITCH, - }); + const action = eg.action.account_switch; const expectedState = []; @@ -55,10 +47,11 @@ describe('muteReducer', () => { const action = deepFreeze({ type: EVENT_MUTED_TOPICS, - muted_topics: [[['stream'], ['topic']]], + id: -1, + muted_topics: [['stream', 'topic']], }); - const expectedState = [[['stream'], ['topic']]]; + const expectedState = [['stream', 'topic']]; const newState = muteReducer(initialState, action); From 7c875fe0d2f68f5297e3cbd964145da6477a8206 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 7 Feb 2022 17:03:22 -0800 Subject: [PATCH 18/28] mute tests [nfc]: Tighten up muteModel tests a lot Less boilerplate, so it's easier to see what the tests are actually doing, and also so there's less to edit to adapt to changes. --- src/mute/__tests__/muteReducer-test.js | 37 ++++++-------------------- 1 file changed, 8 insertions(+), 29 deletions(-) diff --git a/src/mute/__tests__/muteReducer-test.js b/src/mute/__tests__/muteReducer-test.js index f85ff69e95a..495d8d36574 100644 --- a/src/mute/__tests__/muteReducer-test.js +++ b/src/mute/__tests__/muteReducer-test.js @@ -5,58 +5,37 @@ import muteReducer from '../muteReducer'; import { EVENT_MUTED_TOPICS } from '../../actionConstants'; import * as eg from '../../__tests__/lib/exampleData'; +const initialState = deepFreeze([]); + describe('muteReducer', () => { describe('REGISTER_COMPLETE', () => { test('when `mute` data is provided init state with it', () => { - const initialState = deepFreeze([]); const action = eg.mkActionRegisterComplete({ muted_topics: [['stream', 'topic']] }); - - const actualState = muteReducer(initialState, action); - - expect(actualState).toEqual([['stream', 'topic']]); + expect(muteReducer(initialState, action)).toEqual([['stream', 'topic']]); }); test('when no `mute` data is given reset state', () => { - const initialState = deepFreeze([['stream', 'topic']]); + const state = deepFreeze([['stream', 'topic']]); const action = eg.mkActionRegisterComplete({ muted_topics: [] }); - const expectedState = []; - - const actualState = muteReducer(initialState, action); - - expect(actualState).toEqual(expectedState); + expect(muteReducer(state, action)).toEqual(initialState); }); }); describe('ACCOUNT_SWITCH', () => { test('resets state to initial state', () => { - const initialState = deepFreeze([['stream', 'some_topic']]); - - const action = eg.action.account_switch; - - const expectedState = []; - - const actualState = muteReducer(initialState, action); - - expect(actualState).toEqual(expectedState); + const state = deepFreeze([['stream', 'some_topic']]); + expect(muteReducer(state, eg.action.account_switch)).toEqual(initialState); }); }); describe('EVENT_MUTED_TOPICS', () => { test('appends and test a new muted topic', () => { - const initialState = deepFreeze([]); - const action = deepFreeze({ type: EVENT_MUTED_TOPICS, id: -1, muted_topics: [['stream', 'topic']], }); - - const expectedState = [['stream', 'topic']]; - - const newState = muteReducer(initialState, action); - - expect(newState).toEqual(expectedState); - expect(newState).not.toBe(initialState); + expect(muteReducer(initialState, action)).toEqual([['stream', 'topic']]); }); }); }); From 6fc04d6acccc67508ff442d7f6fa7042a3215378 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 31 Jan 2022 21:48:44 -0800 Subject: [PATCH 19/28] mute [nfc]: Organize the muted-topics code as a model Following the, er, model of unreadModel. --- src/boot/reducers.js | 2 +- src/chat/narrowsSelectors.js | 3 +- src/directSelectors.js | 3 -- ...{muteReducer-test.js => muteModel-test.js} | 12 +++--- src/mute/muteModel.js | 43 ++++++++++++++++++- src/mute/muteModelTypes.js | 9 ++++ src/mute/muteReducer.js | 23 ---------- src/reduxTypes.js | 9 +--- src/streams/TopicItem.js | 10 +---- src/title/TitleStream.js | 2 +- src/topics/topicSelectors.js | 4 +- src/unread/unreadSelectors.js | 3 +- src/webview/MessageList.js | 2 +- 13 files changed, 68 insertions(+), 57 deletions(-) rename src/mute/__tests__/{muteReducer-test.js => muteModel-test.js} (73%) create mode 100644 src/mute/muteModelTypes.js delete mode 100644 src/mute/muteReducer.js diff --git a/src/boot/reducers.js b/src/boot/reducers.js index aaff0400d30..db41ae9cf53 100644 --- a/src/boot/reducers.js +++ b/src/boot/reducers.js @@ -18,7 +18,7 @@ import fetching from '../chat/fetchingReducer'; import flags from '../chat/flagsReducer'; import narrows from '../chat/narrowsReducer'; import messages from '../message/messagesReducer'; -import mute from '../mute/muteReducer'; +import { reducer as mute } from '../mute/muteModel'; import mutedUsers from '../mute/mutedUsersReducer'; import outbox from '../outbox/outboxReducer'; import { reducer as pmConversations } from '../pm-conversations/pmConversationsModel'; diff --git a/src/chat/narrowsSelectors.js b/src/chat/narrowsSelectors.js index 8aa78a6dd62..2a5f7617bb2 100644 --- a/src/chat/narrowsSelectors.js +++ b/src/chat/narrowsSelectors.js @@ -16,7 +16,6 @@ import { getAllNarrows, getSubscriptions, getMessages, - getMute, getOutbox, getFlags, } from '../directSelectors'; @@ -30,7 +29,7 @@ import { caseNarrow, streamIdOfNarrow, } from '../utils/narrow'; -import { isTopicMuted } from '../mute/muteModel'; +import { getMute, isTopicMuted } from '../mute/muteModel'; import { streamNameOfStreamMessage } from '../utils/recipient'; import { NULL_ARRAY, NULL_SUBSCRIPTION } from '../nullObjects'; import * as logging from '../utils/logging'; diff --git a/src/directSelectors.js b/src/directSelectors.js index b7983378861..e7b56c9b2cc 100644 --- a/src/directSelectors.js +++ b/src/directSelectors.js @@ -12,7 +12,6 @@ import type { FetchingState, FlagsState, MessagesState, - MuteState, MutedUsersState, NarrowsState, TopicsState, @@ -48,8 +47,6 @@ export const getLoading = (state: PerAccountState): boolean => state.session.loa export const getMessages = (state: PerAccountState): MessagesState => state.messages; -export const getMute = (state: PerAccountState): MuteState => state.mute; - export const getMutedUsers = (state: PerAccountState): MutedUsersState => state.mutedUsers; export const getTyping = (state: PerAccountState): TypingState => state.typing; diff --git a/src/mute/__tests__/muteReducer-test.js b/src/mute/__tests__/muteModel-test.js similarity index 73% rename from src/mute/__tests__/muteReducer-test.js rename to src/mute/__tests__/muteModel-test.js index 495d8d36574..27ed095573a 100644 --- a/src/mute/__tests__/muteReducer-test.js +++ b/src/mute/__tests__/muteModel-test.js @@ -1,30 +1,30 @@ /* @flow strict-local */ import deepFreeze from 'deep-freeze'; -import muteReducer from '../muteReducer'; +import { reducer } from '../muteModel'; import { EVENT_MUTED_TOPICS } from '../../actionConstants'; import * as eg from '../../__tests__/lib/exampleData'; const initialState = deepFreeze([]); -describe('muteReducer', () => { +describe('reducer', () => { describe('REGISTER_COMPLETE', () => { test('when `mute` data is provided init state with it', () => { const action = eg.mkActionRegisterComplete({ muted_topics: [['stream', 'topic']] }); - expect(muteReducer(initialState, action)).toEqual([['stream', 'topic']]); + expect(reducer(initialState, action)).toEqual([['stream', 'topic']]); }); test('when no `mute` data is given reset state', () => { const state = deepFreeze([['stream', 'topic']]); const action = eg.mkActionRegisterComplete({ muted_topics: [] }); - expect(muteReducer(state, action)).toEqual(initialState); + expect(reducer(state, action)).toEqual(initialState); }); }); describe('ACCOUNT_SWITCH', () => { test('resets state to initial state', () => { const state = deepFreeze([['stream', 'some_topic']]); - expect(muteReducer(state, eg.action.account_switch)).toEqual(initialState); + expect(reducer(state, eg.action.account_switch)).toEqual(initialState); }); }); @@ -35,7 +35,7 @@ describe('muteReducer', () => { id: -1, muted_topics: [['stream', 'topic']], }); - expect(muteReducer(initialState, action)).toEqual([['stream', 'topic']]); + expect(reducer(initialState, action)).toEqual([['stream', 'topic']]); }); }); }); diff --git a/src/mute/muteModel.js b/src/mute/muteModel.js index 819b6d9b099..1694ea4fe6b 100644 --- a/src/mute/muteModel.js +++ b/src/mute/muteModel.js @@ -1,5 +1,46 @@ /* @flow strict-local */ -import type { MuteState } from '../types'; +import type { MuteState, PerAccountApplicableAction, PerAccountState } from '../types'; +import { REGISTER_COMPLETE, LOGOUT, ACCOUNT_SWITCH, EVENT_MUTED_TOPICS } from '../actionConstants'; +import { NULL_ARRAY } from '../nullObjects'; + +// +// +// Selectors. +// + +export const getMute = (state: PerAccountState): MuteState => state.mute; + +// +// +// Getters. +// export const isTopicMuted = (stream: string, topic: string, mute: MuteState): boolean => mute.some(x => x[0] === stream && x[1] === topic); + +// +// +// Reducer. +// + +const initialState: MuteState = NULL_ARRAY; + +export const reducer = ( + state: MuteState = initialState, + action: PerAccountApplicableAction, +): MuteState => { + switch (action.type) { + case LOGOUT: + case ACCOUNT_SWITCH: + return initialState; + + case REGISTER_COMPLETE: + return action.data.muted_topics || initialState; + + case EVENT_MUTED_TOPICS: + return action.muted_topics; + + default: + return state; + } +}; diff --git a/src/mute/muteModelTypes.js b/src/mute/muteModelTypes.js new file mode 100644 index 00000000000..c7b51ca1c56 --- /dev/null +++ b/src/mute/muteModelTypes.js @@ -0,0 +1,9 @@ +// @flow strict-local + +import type { MutedTopicTuple } from '../api/apiTypes'; + +// TODO(#3918): Use stream IDs for muted topics, not stream names. +// Server issue for fixing that in the API: https://github.com/zulip/zulip/issues/21015 +// Meanwhile, even while the server is still sending stream names, +// we should convert to stream IDs at the edge. +export type MuteState = $ReadOnlyArray; diff --git a/src/mute/muteReducer.js b/src/mute/muteReducer.js deleted file mode 100644 index 0e2ecab87ad..00000000000 --- a/src/mute/muteReducer.js +++ /dev/null @@ -1,23 +0,0 @@ -/* @flow strict-local */ -import type { MuteState, PerAccountApplicableAction } from '../types'; -import { REGISTER_COMPLETE, LOGOUT, ACCOUNT_SWITCH, EVENT_MUTED_TOPICS } from '../actionConstants'; -import { NULL_ARRAY } from '../nullObjects'; - -const initialState: MuteState = NULL_ARRAY; - -export default (state: MuteState = initialState, action: PerAccountApplicableAction): MuteState => { - switch (action.type) { - case LOGOUT: - case ACCOUNT_SWITCH: - return initialState; - - case REGISTER_COMPLETE: - return action.data.muted_topics || initialState; - - case EVENT_MUTED_TOPICS: - return action.muted_topics; - - default: - return state; - } -}; diff --git a/src/reduxTypes.js b/src/reduxTypes.js index 4f2a620e06c..0e858034633 100644 --- a/src/reduxTypes.js +++ b/src/reduxTypes.js @@ -15,7 +15,6 @@ import type { Action, DispatchableWithoutAccountAction } from './actionTypes'; import type { Topic, Message, - MutedTopicTuple, CrossRealmBot, RealmEmojiById, RealmFilter, @@ -32,9 +31,11 @@ import type { GlobalSessionState, SessionState, } from './session/sessionReducer'; +import type { MuteState } from './mute/muteModelTypes'; import type { PmConversationsState } from './pm-conversations/pmConversationsModel'; import type { UnreadState } from './unread/unreadModelTypes'; +export type { MuteState } from './mute/muteModelTypes'; export type * from './actionTypes'; /** @@ -178,12 +179,6 @@ export type MigrationsState = $ReadOnly<{| version?: number, |}>; -// TODO(#3918): Use stream IDs for muted topics, not stream names. -// Server issue for fixing that in the API: https://github.com/zulip/zulip/issues/21015 -// Meanwhile, even while the server is still sending stream names, -// we should convert to stream IDs at the edge. -export type MuteState = $ReadOnlyArray; - /** A map from user IDs to the Unix timestamp in seconds when they were muted. */ export type MutedUsersState = Immutable.Map; diff --git a/src/streams/TopicItem.js b/src/streams/TopicItem.js index a7fe7b73b6d..ae62d239760 100644 --- a/src/streams/TopicItem.js +++ b/src/streams/TopicItem.js @@ -11,14 +11,8 @@ import { showTopicActionSheet } from '../action-sheets'; import type { ShowActionSheetWithOptions } from '../action-sheets'; import { TranslationContext } from '../boot/TranslationProvider'; import { useDispatch, useSelector } from '../react-redux'; -import { - getAuth, - getMute, - getFlags, - getSubscriptionsById, - getStreamsById, - getOwnUser, -} from '../selectors'; +import { getAuth, getFlags, getSubscriptionsById, getStreamsById, getOwnUser } from '../selectors'; +import { getMute } from '../mute/muteModel'; import { getUnread } from '../unread/unreadModel'; const componentStyles = createStyleSheet({ diff --git a/src/title/TitleStream.js b/src/title/TitleStream.js index 160475020ba..4f65431fa76 100644 --- a/src/title/TitleStream.js +++ b/src/title/TitleStream.js @@ -14,7 +14,6 @@ import StreamIcon from '../streams/StreamIcon'; import { isTopicNarrow, topicOfNarrow } from '../utils/narrow'; import { getAuth, - getMute, getFlags, getSubscriptionsById, getStreamsById, @@ -22,6 +21,7 @@ import { getStreamInNarrow, getSettings, } from '../selectors'; +import { getMute } from '../mute/muteModel'; import { showStreamActionSheet, showTopicActionSheet } from '../action-sheets'; import type { ShowActionSheetWithOptions } from '../action-sheets'; import { getUnread } from '../unread/unreadModel'; diff --git a/src/topics/topicSelectors.js b/src/topics/topicSelectors.js index 506d93d6f2d..8fa0676b796 100644 --- a/src/topics/topicSelectors.js +++ b/src/topics/topicSelectors.js @@ -2,12 +2,12 @@ import { createSelector } from 'reselect'; import type { Narrow, Selector, TopicExtended, TopicsState } from '../types'; -import { getMute, getTopics } from '../directSelectors'; +import { getTopics } from '../directSelectors'; import { getUnread, getUnreadCountForTopic } from '../unread/unreadModel'; import { getStreamsById } from '../subscriptions/subscriptionSelectors'; import { NULL_ARRAY } from '../nullObjects'; import { isStreamNarrow, streamIdOfNarrow } from '../utils/narrow'; -import { isTopicMuted } from '../mute/muteModel'; +import { getMute, isTopicMuted } from '../mute/muteModel'; export const getTopicsForNarrow: Selector<$ReadOnlyArray, Narrow> = createSelector( (state, narrow) => narrow, diff --git a/src/unread/unreadSelectors.js b/src/unread/unreadSelectors.js index 6a482df707a..11c11701ee8 100644 --- a/src/unread/unreadSelectors.js +++ b/src/unread/unreadSelectors.js @@ -3,10 +3,9 @@ import { createSelector } from 'reselect'; import type { Narrow, Selector, UnreadStreamItem } from '../types'; import { caseInsensitiveCompareFunc } from '../utils/misc'; -import { getMute } from '../directSelectors'; +import { getMute, isTopicMuted } from '../mute/muteModel'; import { getOwnUserId } from '../users/userSelectors'; import { getSubscriptionsById, getStreamsById } from '../subscriptions/subscriptionSelectors'; -import { isTopicMuted } from '../mute/muteModel'; import { caseNarrow } from '../utils/narrow'; import { NULL_SUBSCRIPTION } from '../nullObjects'; import { diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index 25dd64a696d..543f448e2ad 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -39,7 +39,6 @@ import { getFlags, getFetchingForNarrow, getAllUsersById, - getMute, getMutedUsers, getOwnUser, getSettings, @@ -48,6 +47,7 @@ import { getStreamsById, getRealm, } from '../selectors'; +import { getMute } from '../mute/muteModel'; import { withGetText } from '../boot/TranslationProvider'; import type { ShowActionSheetWithOptions } from '../action-sheets'; import { getMessageListElementsMemoized } from '../message/messageSelectors'; From 8f82bdf697cbc11928e955bcd1e0ebabd26ce4ea Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 7 Feb 2022 13:18:20 -0800 Subject: [PATCH 20/28] mute tests [nfc]: Factor out helper makeMuteState Now the tests no longer try to construct a MuteState directly; the data structure is encapsulated in the model. --- .../__tests__/action-sheet-test.js | 5 +++-- src/chat/__tests__/narrowsSelectors-test.js | 3 ++- src/mute/__tests__/mute-testlib.js | 13 +++++++++++++ src/mute/__tests__/muteModel-test.js | 15 ++++++++------- src/topics/__tests__/topicsSelectors-test.js | 11 ++++++----- src/unread/__tests__/unreadSelectors-test.js | 17 ++++++----------- 6 files changed, 38 insertions(+), 26 deletions(-) create mode 100644 src/mute/__tests__/mute-testlib.js diff --git a/src/action-sheets/__tests__/action-sheet-test.js b/src/action-sheets/__tests__/action-sheet-test.js index 115261ef496..b9c1e9989d1 100644 --- a/src/action-sheets/__tests__/action-sheet-test.js +++ b/src/action-sheets/__tests__/action-sheet-test.js @@ -10,6 +10,7 @@ import { } from '../index'; import { reducer } from '../../unread/unreadModel'; import { initialState } from '../../unread/__tests__/unread-testlib'; +import { makeMuteState } from '../../mute/__tests__/mute-testlib'; const buttonTitles = buttons => buttons.map(button => button.title); @@ -82,7 +83,7 @@ describe('constructTopicActionButtons', () => { }); test('show Unmute topic option if topic is muted', () => { - const mute = deepFreeze([[stream.name, topic]]); + const mute = makeMuteState([[stream, topic]]); const buttons = constructTopicActionButtons({ backgroundData: { ...eg.backgroundData, streams, mute }, streamId, @@ -93,7 +94,7 @@ describe('constructTopicActionButtons', () => { test('show mute topic option if topic is not muted', () => { const buttons = constructTopicActionButtons({ - backgroundData: { ...eg.backgroundData, streams, mute: [] }, + backgroundData: { ...eg.backgroundData, streams, mute: makeMuteState([]) }, streamId, topic, }); diff --git a/src/chat/__tests__/narrowsSelectors-test.js b/src/chat/__tests__/narrowsSelectors-test.js index 5103edaa891..d57f0d2f30f 100644 --- a/src/chat/__tests__/narrowsSelectors-test.js +++ b/src/chat/__tests__/narrowsSelectors-test.js @@ -22,6 +22,7 @@ import { } from '../../utils/narrow'; import { NULL_SUBSCRIPTION } from '../../nullObjects'; import * as eg from '../../__tests__/lib/exampleData'; +import { makeMuteState } from '../../mute/__tests__/mute-testlib'; describe('getMessagesForNarrow', () => { const message = eg.streamMessage({ id: 123 }); @@ -102,7 +103,7 @@ describe('getShownMessagesForNarrow', () => { const message = eg.streamMessage(); const subscription = eg.subscription; const mutedSubscription = { ...subscription, in_home_view: false }; - const mutes = [[stream.name, message.subject]]; + const mutes = makeMuteState([[stream, message.subject]]); const makeStateGeneral = (message, narrow, extra) => eg.reduxStatePlus({ diff --git a/src/mute/__tests__/mute-testlib.js b/src/mute/__tests__/mute-testlib.js new file mode 100644 index 00000000000..4a5a4a1d76e --- /dev/null +++ b/src/mute/__tests__/mute-testlib.js @@ -0,0 +1,13 @@ +/* @flow strict-local */ + +import { reducer } from '../muteModel'; +import type { MuteState } from '../muteModelTypes'; +import type { Stream } from '../../api/modelTypes'; +import { EVENT_MUTED_TOPICS } from '../../actionConstants'; + +export const makeMuteState = (data: $ReadOnlyArray<[Stream, string]>): MuteState => + reducer(undefined, { + type: EVENT_MUTED_TOPICS, + id: -1, + muted_topics: data.map(([stream, topic]) => [stream.name, topic]), + }); diff --git a/src/mute/__tests__/muteModel-test.js b/src/mute/__tests__/muteModel-test.js index 27ed095573a..58b559ad792 100644 --- a/src/mute/__tests__/muteModel-test.js +++ b/src/mute/__tests__/muteModel-test.js @@ -4,18 +4,19 @@ import deepFreeze from 'deep-freeze'; import { reducer } from '../muteModel'; import { EVENT_MUTED_TOPICS } from '../../actionConstants'; import * as eg from '../../__tests__/lib/exampleData'; +import { makeMuteState } from './mute-testlib'; -const initialState = deepFreeze([]); +const initialState = makeMuteState([]); describe('reducer', () => { describe('REGISTER_COMPLETE', () => { test('when `mute` data is provided init state with it', () => { - const action = eg.mkActionRegisterComplete({ muted_topics: [['stream', 'topic']] }); - expect(reducer(initialState, action)).toEqual([['stream', 'topic']]); + const action = eg.mkActionRegisterComplete({ muted_topics: [[eg.stream.name, 'topic']] }); + expect(reducer(initialState, action)).toEqual(makeMuteState([[eg.stream, 'topic']])); }); test('when no `mute` data is given reset state', () => { - const state = deepFreeze([['stream', 'topic']]); + const state = makeMuteState([[eg.stream, 'topic']]); const action = eg.mkActionRegisterComplete({ muted_topics: [] }); expect(reducer(state, action)).toEqual(initialState); }); @@ -23,7 +24,7 @@ describe('reducer', () => { describe('ACCOUNT_SWITCH', () => { test('resets state to initial state', () => { - const state = deepFreeze([['stream', 'some_topic']]); + const state = makeMuteState([[eg.stream, 'some_topic']]); expect(reducer(state, eg.action.account_switch)).toEqual(initialState); }); }); @@ -33,9 +34,9 @@ describe('reducer', () => { const action = deepFreeze({ type: EVENT_MUTED_TOPICS, id: -1, - muted_topics: [['stream', 'topic']], + muted_topics: [[eg.stream.name, 'topic']], }); - expect(reducer(initialState, action)).toEqual([['stream', 'topic']]); + expect(reducer(initialState, action)).toEqual(makeMuteState([[eg.stream, 'topic']])); }); }); }); diff --git a/src/topics/__tests__/topicsSelectors-test.js b/src/topics/__tests__/topicsSelectors-test.js index b273a73e38c..2baec8be4bb 100644 --- a/src/topics/__tests__/topicsSelectors-test.js +++ b/src/topics/__tests__/topicsSelectors-test.js @@ -3,6 +3,7 @@ import { getTopicsForNarrow, getTopicsForStream } from '../topicSelectors'; import { HOME_NARROW, streamNarrow } from '../../utils/narrow'; import { reducer as unreadReducer } from '../../unread/unreadModel'; import * as eg from '../../__tests__/lib/exampleData'; +import { makeMuteState } from '../../mute/__tests__/mute-testlib'; describe('getTopicsForNarrow', () => { test('when no topics return an empty list', () => { @@ -61,11 +62,11 @@ describe('getTopicsForStream', () => { { name: 'topic 5', max_id: 9 }, ], }, - mute: [ - [eg.stream.name, 'topic 1'], - [eg.stream.name, 'topic 3'], - [eg.otherStream.name, 'topic 2'], - ], + mute: makeMuteState([ + [eg.stream, 'topic 1'], + [eg.stream, 'topic 3'], + [eg.otherStream, 'topic 2'], + ]), unread: [ eg.streamMessage({ stream: eg.stream, subject: 'topic 2', id: 1 }), eg.streamMessage({ stream: eg.stream, subject: 'topic 2', id: 5 }), diff --git a/src/unread/__tests__/unreadSelectors-test.js b/src/unread/__tests__/unreadSelectors-test.js index 30d0b00539d..11f08d40819 100644 --- a/src/unread/__tests__/unreadSelectors-test.js +++ b/src/unread/__tests__/unreadSelectors-test.js @@ -16,6 +16,7 @@ import { import * as eg from '../../__tests__/lib/exampleData'; import { initialState, selectorBaseState as unreadState, stream0, stream2 } from './unread-testlib'; +import { makeMuteState } from '../../mute/__tests__/mute-testlib'; const subscription0 = eg.makeSubscription({ stream: stream0, color: 'red' }); const subscription2 = eg.makeSubscription({ stream: stream2, color: 'blue' }); @@ -34,7 +35,7 @@ describe('getUnreadByStream', () => { const state = eg.reduxStatePlus({ subscriptions, unread: unreadState, - mute: [['stream 0', 'a topic']], + mute: makeMuteState([[stream0, 'a topic']]), }); const unreadByStream = getUnreadByStream(state); @@ -48,7 +49,6 @@ describe('getUnreadStreamTotal', () => { const state = eg.reduxStatePlus({ unread: initialState, subscriptions: [], - mute: [], }); const unreadCount = getUnreadStreamTotal(state); @@ -59,7 +59,6 @@ describe('getUnreadStreamTotal', () => { test('count all the unread messages listed in "streams" key', () => { const state = eg.reduxStatePlus({ unread: unreadState, - mute: [], }); const unreadCount = getUnreadStreamTotal(state); @@ -173,7 +172,6 @@ describe('getUnreadTotal', () => { const state = eg.reduxStatePlus({ unread: initialState, subscriptions: [], - mute: [], }); const unreadCount = getUnreadTotal(state); @@ -184,7 +182,6 @@ describe('getUnreadTotal', () => { test('calculates total unread of streams + pms + huddles', () => { const state = eg.reduxStatePlus({ unread: unreadState, - mute: [], }); const unreadCount = getUnreadTotal(state); @@ -209,7 +206,6 @@ describe('getUnreadStreamsAndTopics', () => { const state = eg.reduxStatePlus({ subscriptions: subscriptions.map(s => ({ ...s, in_home_view: false })), unread: unreadState, - mute: [], }); const unreadCount = getUnreadStreamsAndTopics(state); @@ -267,7 +263,7 @@ describe('getUnreadStreamsAndTopics', () => { const state = eg.reduxStatePlus({ subscriptions, unread: unreadState, - mute: [['stream 0', 'a topic']], + mute: makeMuteState([[stream0, 'a topic']]), }); const unreadCount = getUnreadStreamsAndTopics(state); @@ -325,7 +321,6 @@ describe('getUnreadStreamsAndTopics', () => { const state = eg.reduxStatePlus({ subscriptions, unread: unreadState, - mute: [], }); const unreadCount = getUnreadStreamsAndTopics(state); @@ -397,7 +392,8 @@ describe('getUnreadStreamsAndTopics', () => { (st, message) => reducer(st, eg.mkActionEventNewMessage(message), eg.plusReduxState), eg.plusReduxState.unread, ), - mute: [['def stream', 'c topic']], + // TODO yuck at constructing this modified stream as a throwaway, with magic string + mute: makeMuteState([[{ ...stream2, name: 'def stream' }, 'c topic']]), }); const unreadCount = getUnreadStreamsAndTopics(state); @@ -454,7 +450,6 @@ describe('getUnreadStreamsAndTopicsSansMuted', () => { const state = eg.reduxStatePlus({ subscriptions: subscriptions.map(s => ({ ...s, in_home_view: false })), unread: unreadState, - mute: [], }); const unreadCount = getUnreadStreamsAndTopicsSansMuted(state); @@ -466,7 +461,7 @@ describe('getUnreadStreamsAndTopicsSansMuted', () => { const state = eg.reduxStatePlus({ subscriptions: [subscription0], unread: unreadState, - mute: [['stream 0', 'a topic']], + mute: makeMuteState([[stream0, 'a topic']]), }); const unreadCount = getUnreadStreamsAndTopicsSansMuted(state); From a9428df7dfa5af8ea3b5cc7856034b437e0de4d1 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 7 Feb 2022 15:32:22 -0800 Subject: [PATCH 21/28] storage: Say class name in "unexpected class" error And mention "stringify", to give a bit more context. --- src/storage/replaceRevive.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storage/replaceRevive.js b/src/storage/replaceRevive.js index 109a0aef688..808884ae72b 100644 --- a/src/storage/replaceRevive.js +++ b/src/storage/replaceRevive.js @@ -113,7 +113,7 @@ function replacer(key, value) { // Flow bug: https://github.com/facebook/flow/issues/6110 origValuePrototype === (Object.prototype: $FlowIssue) || origValuePrototype === (Array.prototype: $FlowIssue), - 'unexpected class', + `stringify: unexpected class: ${origValuePrototype.constructor.name}`, ); // Ensure that objects with a [SERIALIZED_TYPE_FIELD_NAME] property From a14148b16d311b486c36f19b7036f4589693cbad Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 7 Feb 2022 15:44:51 -0800 Subject: [PATCH 22/28] replace-revive: Add support for plain JS Maps We'll use these in the muted-topics model, because the server doesn't send incremental updates for them anyway. --- src/storage/__tests__/replaceRevive-test.js | 10 ++++++++++ src/storage/replaceRevive.js | 4 ++++ 2 files changed, 14 insertions(+) diff --git a/src/storage/__tests__/replaceRevive-test.js b/src/storage/__tests__/replaceRevive-test.js index 3f05d603bc4..3bc474d952d 100644 --- a/src/storage/__tests__/replaceRevive-test.js +++ b/src/storage/__tests__/replaceRevive-test.js @@ -11,6 +11,12 @@ import * as eg from '../../__tests__/lib/exampleData'; import { makeUserId } from '../../api/idTypes'; const data = { + // prettier-ignore + jsMap: new Map([['a', 1], ['b', 2], ['c', 3]]), + // prettier-ignore + jsMapWithTypeKey: new Map([['a', 1], [SERIALIZED_TYPE_FIELD_NAME, { b: [2] }]]), + // prettier-ignore + jsMapNumKeys: new Map([[1, 2], [3, 4]]), list: Immutable.List([1, 2, 'a', null]), map: Immutable.Map({ a: 1, b: 2, c: 3, d: 4 }), mapWithTypeKey: Immutable.Map({ @@ -46,6 +52,10 @@ const data = { }; const stringified = { + jsMap: '{"data":[["a",1],["b",2],["c",3]],"__serializedType__":"Map"}', + jsMapWithTypeKey: + '{"data":[["a",1],["__serializedType__",{"b":[2]}]],"__serializedType__":"Map"}', + jsMapNumKeys: '{"data":[[1,2],[3,4]],"__serializedType__":"Map"}', list: '{"data":[1,2,"a",null],"__serializedType__":"ImmutableList"}', map: '{"data":{"a":1,"b":2,"c":3,"d":4},"__serializedType__":"ImmutableMap"}', mapWithTypeKey: diff --git a/src/storage/replaceRevive.js b/src/storage/replaceRevive.js index 808884ae72b..f6932cd0ae8 100644 --- a/src/storage/replaceRevive.js +++ b/src/storage/replaceRevive.js @@ -59,6 +59,8 @@ function replacer(key, value) { // Flow bug: https://github.com/facebook/flow/issues/6110 case (Date.prototype: $FlowIssue): return { data: value, [SERIALIZED_TYPE_FIELD_NAME]: 'Date' }; + case (Map.prototype: $FlowIssue): + return { data: [...value.entries()], [SERIALIZED_TYPE_FIELD_NAME]: 'Map' }; case (ZulipVersion.prototype: $FlowIssue): return { data: value.raw(), [SERIALIZED_TYPE_FIELD_NAME]: 'ZulipVersion' }; case (URL.prototype: $FlowIssue): @@ -144,6 +146,8 @@ function reviver(key, value) { switch (value[SERIALIZED_TYPE_FIELD_NAME]) { case 'Date': return new Date(data); + case 'Map': + return new Map(data); case 'ZulipVersion': return new ZulipVersion(data); case 'URL': From 2e3d51b38e7b162f9f5e403ce81eec2ac3a0a515 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 7 Feb 2022 15:50:53 -0800 Subject: [PATCH 23/28] replace-revive: Add support for plain JS Sets --- src/storage/__tests__/replaceRevive-test.js | 2 ++ src/storage/replaceRevive.js | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/src/storage/__tests__/replaceRevive-test.js b/src/storage/__tests__/replaceRevive-test.js index 3bc474d952d..ed0a372c701 100644 --- a/src/storage/__tests__/replaceRevive-test.js +++ b/src/storage/__tests__/replaceRevive-test.js @@ -17,6 +17,7 @@ const data = { jsMapWithTypeKey: new Map([['a', 1], [SERIALIZED_TYPE_FIELD_NAME, { b: [2] }]]), // prettier-ignore jsMapNumKeys: new Map([[1, 2], [3, 4]]), + jsSet: new Set([1, 2, 'a', null, { b: [3] }]), list: Immutable.List([1, 2, 'a', null]), map: Immutable.Map({ a: 1, b: 2, c: 3, d: 4 }), mapWithTypeKey: Immutable.Map({ @@ -56,6 +57,7 @@ const stringified = { jsMapWithTypeKey: '{"data":[["a",1],["__serializedType__",{"b":[2]}]],"__serializedType__":"Map"}', jsMapNumKeys: '{"data":[[1,2],[3,4]],"__serializedType__":"Map"}', + jsSet: '{"data":[1,2,"a",null,{"b":[3]}],"__serializedType__":"Set"}', list: '{"data":[1,2,"a",null],"__serializedType__":"ImmutableList"}', map: '{"data":{"a":1,"b":2,"c":3,"d":4},"__serializedType__":"ImmutableMap"}', mapWithTypeKey: diff --git a/src/storage/replaceRevive.js b/src/storage/replaceRevive.js index f6932cd0ae8..335839fd712 100644 --- a/src/storage/replaceRevive.js +++ b/src/storage/replaceRevive.js @@ -59,6 +59,8 @@ function replacer(key, value) { // Flow bug: https://github.com/facebook/flow/issues/6110 case (Date.prototype: $FlowIssue): return { data: value, [SERIALIZED_TYPE_FIELD_NAME]: 'Date' }; + case (Set.prototype: $FlowIssue): + return { data: [...value], [SERIALIZED_TYPE_FIELD_NAME]: 'Set' }; case (Map.prototype: $FlowIssue): return { data: [...value.entries()], [SERIALIZED_TYPE_FIELD_NAME]: 'Map' }; case (ZulipVersion.prototype: $FlowIssue): @@ -146,6 +148,8 @@ function reviver(key, value) { switch (value[SERIALIZED_TYPE_FIELD_NAME]) { case 'Date': return new Date(data); + case 'Set': + return new Set(data); case 'Map': return new Map(data); case 'ZulipVersion': From 4def2053f4345ac307e9eedd50cc5b20f36405a9 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 7 Feb 2022 17:27:05 -0800 Subject: [PATCH 24/28] reducer: Split out the initialization case That is, split the case where the previous state is void, and so the action must be the store initialization action, from the case where it's any other action and the previous state must be of its usual object type. This makes for a bit more boilerplate, and one more line to update when adding or removing a sub-reducer. But both the split versions are simpler, and that will help us add new complications that we need. --- src/boot/reducers.js | 89 ++++++++++++++++++++++++++++---------------- 1 file changed, 56 insertions(+), 33 deletions(-) diff --git a/src/boot/reducers.js b/src/boot/reducers.js index db41ae9cf53..991cbef5116 100644 --- a/src/boot/reducers.js +++ b/src/boot/reducers.js @@ -1,12 +1,6 @@ /* @flow strict-local */ import config from '../config'; -import type { - Action, - PerAccountState, - PerAccountApplicableAction, - GlobalState, - MigrationsState, -} from '../types'; +import type { Action, PerAccountApplicableAction, GlobalState, MigrationsState } from '../types'; import { dubPerAccountState } from '../reduxTypes'; import { isPerAccountApplicableAction } from '../actionTypes'; @@ -95,39 +89,68 @@ function applyReducer & $Keys, State> // Based on Redux upstream's combineReducers. export default (globalState: void | GlobalState, origAction: Action): GlobalState => { let nextPerAccountState = globalState; - if (!nextPerAccountState || isPerAccountApplicableAction(origAction)) { - // Update the per-account state. We do this when the action is a - // PerAccountApplicableAction... and also when it's the store - // initialization action, signalled by the previous state being void. + if (!nextPerAccountState) { + // Initialize the per-account state. This must be the store + // initialization action, because the previous state is void. + + /* $FlowFixMe[incompatible-type]: */ + const action: PerAccountApplicableAction = origAction; + + // prettier-ignore + nextPerAccountState = { + alertWords: applyReducer('alertWords', alertWords, undefined, action, undefined), + caughtUp: applyReducer('caughtUp', caughtUp, undefined, action, undefined), + drafts: applyReducer('drafts', drafts, undefined, action, undefined), + fetching: applyReducer('fetching', fetching, undefined, action, undefined), + flags: applyReducer('flags', flags, undefined, action, undefined), + messages: applyReducer('messages', messages, undefined, action, undefined), + narrows: applyReducer('narrows', narrows, undefined, action, undefined), + mute: applyReducer('mute', mute, undefined, action, undefined), + mutedUsers: applyReducer('mutedUsers', mutedUsers, undefined, action, undefined), + outbox: applyReducer('outbox', outbox, undefined, action, undefined), + pmConversations: applyReducer('pmConversations', pmConversations, undefined, action, undefined), + presence: applyReducer('presence', presence, undefined, action, undefined), + realm: applyReducer('realm', realm, undefined, action, undefined), + streams: applyReducer('streams', streams, undefined, action, undefined), + subscriptions: applyReducer('subscriptions', subscriptions, undefined, action, undefined), + topics: applyReducer('topics', topics, undefined, action, undefined), + typing: applyReducer('typing', typing, undefined, action, undefined), + unread: applyReducer('unread', unread, undefined, action, undefined), + userGroups: applyReducer('userGroups', userGroups, undefined, action, undefined), + userStatus: applyReducer('userStatus', userStatus, undefined, action, undefined), + users: applyReducer('users', users, undefined, action, undefined), + }; + } else if (isPerAccountApplicableAction(origAction)) { + // Update the per-account state, for this PerAccountApplicableAction. /* $FlowFixMe[incompatible-type]: TODO teach Flow that isPerAccountApplicableAction checks this */ const action: PerAccountApplicableAction = origAction; - const state: void | PerAccountState = globalState ? dubPerAccountState(globalState) : undefined; + const state = dubPerAccountState(nextPerAccountState); // prettier-ignore nextPerAccountState = { - alertWords: applyReducer('alertWords', alertWords, state?.alertWords, action, state), - caughtUp: applyReducer('caughtUp', caughtUp, state?.caughtUp, action, state), - drafts: applyReducer('drafts', drafts, state?.drafts, action, state), - fetching: applyReducer('fetching', fetching, state?.fetching, action, state), - flags: applyReducer('flags', flags, state?.flags, action, state), - messages: applyReducer('messages', messages, state?.messages, action, state), - narrows: applyReducer('narrows', narrows, state?.narrows, action, state), - mute: applyReducer('mute', mute, state?.mute, action, state), - mutedUsers: applyReducer('mutedUsers', mutedUsers, state?.mutedUsers, action, state), - outbox: applyReducer('outbox', outbox, state?.outbox, action, state), - pmConversations: applyReducer('pmConversations', pmConversations, state?.pmConversations, action, state), - presence: applyReducer('presence', presence, state?.presence, action, state), - realm: applyReducer('realm', realm, state?.realm, action, state), - streams: applyReducer('streams', streams, state?.streams, action, state), - subscriptions: applyReducer('subscriptions', subscriptions, state?.subscriptions, action, state), - topics: applyReducer('topics', topics, state?.topics, action, state), - typing: applyReducer('typing', typing, state?.typing, action, state), - unread: applyReducer('unread', unread, state?.unread, action, state), - userGroups: applyReducer('userGroups', userGroups, state?.userGroups, action, state), - userStatus: applyReducer('userStatus', userStatus, state?.userStatus, action, state), - users: applyReducer('users', users, state?.users, action, state), + alertWords: applyReducer('alertWords', alertWords, state.alertWords, action, state), + caughtUp: applyReducer('caughtUp', caughtUp, state.caughtUp, action, state), + drafts: applyReducer('drafts', drafts, state.drafts, action, state), + fetching: applyReducer('fetching', fetching, state.fetching, action, state), + flags: applyReducer('flags', flags, state.flags, action, state), + messages: applyReducer('messages', messages, state.messages, action, state), + narrows: applyReducer('narrows', narrows, state.narrows, action, state), + mute: applyReducer('mute', mute, state.mute, action, state), + mutedUsers: applyReducer('mutedUsers', mutedUsers, state.mutedUsers, action, state), + outbox: applyReducer('outbox', outbox, state.outbox, action, state), + pmConversations: applyReducer('pmConversations', pmConversations, state.pmConversations, action, state), + presence: applyReducer('presence', presence, state.presence, action, state), + realm: applyReducer('realm', realm, state.realm, action, state), + streams: applyReducer('streams', streams, state.streams, action, state), + subscriptions: applyReducer('subscriptions', subscriptions, state.subscriptions, action, state), + topics: applyReducer('topics', topics, state.topics, action, state), + typing: applyReducer('typing', typing, state.typing, action, state), + unread: applyReducer('unread', unread, state.unread, action, state), + userGroups: applyReducer('userGroups', userGroups, state.userGroups, action, state), + userStatus: applyReducer('userStatus', userStatus, state.userStatus, action, state), + users: applyReducer('users', users, state.users, action, state), }; } From 6d9146372d5901d7e7fb797dd38fed3e98da100b Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 31 Jan 2022 22:10:38 -0800 Subject: [PATCH 25/28] mute [nfc]: Pass stream ID to isTopicMuted This will let us alter the data structure so that it relies on stream IDs instead of stream names. --- src/action-sheets/index.js | 2 +- src/chat/narrowsSelectors.js | 4 ++-- src/mute/muteModel.js | 8 ++++++-- src/topics/topicSelectors.js | 2 +- src/unread/unreadSelectors.js | 5 +++-- 5 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/action-sheets/index.js b/src/action-sheets/index.js index 500b918e1d9..ce0c6983e11 100644 --- a/src/action-sheets/index.js +++ b/src/action-sheets/index.js @@ -369,7 +369,7 @@ export const constructTopicActionButtons = ({ } const stream = streams.get(streamId); invariant(stream !== undefined, 'Stream with provided streamId not found.'); - if (isTopicMuted(stream.name, topic, mute)) { + if (isTopicMuted(stream.stream_id, stream.name, topic, mute)) { buttons.push(unmuteTopic); } else { buttons.push(muteTopic); diff --git a/src/chat/narrowsSelectors.js b/src/chat/narrowsSelectors.js index 2a5f7617bb2..9f338857ee3 100644 --- a/src/chat/narrowsSelectors.js +++ b/src/chat/narrowsSelectors.js @@ -138,7 +138,7 @@ export const getShownMessagesForNarrow: Selector<$ReadOnlyArray state.mute; // Getters. // -export const isTopicMuted = (stream: string, topic: string, mute: MuteState): boolean => - mute.some(x => x[0] === stream && x[1] === topic); +export const isTopicMuted = ( + streamId: number, + streamName: string, + topic: string, + mute: MuteState, +): boolean => mute.some(x => x[0] === streamName && x[1] === topic); // // diff --git a/src/topics/topicSelectors.js b/src/topics/topicSelectors.js index 8fa0676b796..cef49edf609 100644 --- a/src/topics/topicSelectors.js +++ b/src/topics/topicSelectors.js @@ -36,7 +36,7 @@ export const getTopicsForStream: Selector, number } return topicList.map(({ name, max_id }) => { - const isMuted = isTopicMuted(stream.name, name, mute); + const isMuted = isTopicMuted(stream.stream_id, stream.name, name, mute); const unreadCount = getUnreadCountForTopic(unread, stream.stream_id, name); return { name, max_id, isMuted, unreadCount }; }); diff --git a/src/unread/unreadSelectors.js b/src/unread/unreadSelectors.js index 11c11701ee8..96b8deea80c 100644 --- a/src/unread/unreadSelectors.js +++ b/src/unread/unreadSelectors.js @@ -29,6 +29,7 @@ export const getUnreadByStream: Selector<{| [number]: number |}> = createSelecto let total = 0; for (const [topic, msgIds] of streamData) { const isMuted = isTopicMuted( + streamId, (subscriptionsById.get(streamId) || NULL_SUBSCRIPTION).name, topic, mute, @@ -154,7 +155,7 @@ export const getUnreadStreamsAndTopics: Selector<$ReadOnlyArray = createSelector( unread.streams .get(stream.stream_id) ?.entrySeq() - .filterNot(([topic, _]) => isTopicMuted(stream.name, topic, mute)) + .filterNot(([topic, _]) => isTopicMuted(stream.stream_id, stream.name, topic, mute)) .map(([_, msgIds]) => msgIds.size) .reduce((s, x) => s + x, 0) ?? 0 From b159ceafdac031d608e805d641af23e81f3aab1e Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 31 Jan 2022 22:03:13 -0800 Subject: [PATCH 26/28] mute: Maintain muted-topics state by stream ID, not name This represents one of the last significant pieces left for #3918. --- src/boot/reducers.js | 14 +++++++--- src/mute/__tests__/mute-testlib.js | 20 +++++++++----- src/mute/__tests__/muteModel-test.js | 29 +++++++++++++++----- src/mute/muteModel.js | 34 ++++++++++++++++++++---- src/mute/muteModelTypes.js | 10 +++---- src/storage/__tests__/migrations-test.js | 6 ++--- src/storage/migrations.js | 3 +++ 7 files changed, 86 insertions(+), 30 deletions(-) diff --git a/src/boot/reducers.js b/src/boot/reducers.js index 991cbef5116..7a4cfdf25c9 100644 --- a/src/boot/reducers.js +++ b/src/boot/reducers.js @@ -126,10 +126,13 @@ export default (globalState: void | GlobalState, origAction: Action): GlobalStat /* $FlowFixMe[incompatible-type]: TODO teach Flow that isPerAccountApplicableAction checks this */ const action: PerAccountApplicableAction = origAction; - const state = dubPerAccountState(nextPerAccountState); + let state = dubPerAccountState(nextPerAccountState); // prettier-ignore - nextPerAccountState = { + state = { + /* $FlowFixMe[not-an-object]: TODO(#5006) this fixme should go away + when PerAccountState is no longer opaque */ + ...state, alertWords: applyReducer('alertWords', alertWords, state.alertWords, action, state), caughtUp: applyReducer('caughtUp', caughtUp, state.caughtUp, action, state), drafts: applyReducer('drafts', drafts, state.drafts, action, state), @@ -137,7 +140,7 @@ export default (globalState: void | GlobalState, origAction: Action): GlobalStat flags: applyReducer('flags', flags, state.flags, action, state), messages: applyReducer('messages', messages, state.messages, action, state), narrows: applyReducer('narrows', narrows, state.narrows, action, state), - mute: applyReducer('mute', mute, state.mute, action, state), + // mute is below mutedUsers: applyReducer('mutedUsers', mutedUsers, state.mutedUsers, action, state), outbox: applyReducer('outbox', outbox, state.outbox, action, state), pmConversations: applyReducer('pmConversations', pmConversations, state.pmConversations, action, state), @@ -152,6 +155,11 @@ export default (globalState: void | GlobalState, origAction: Action): GlobalStat userStatus: applyReducer('userStatus', userStatus, state.userStatus, action, state), users: applyReducer('users', users, state.users, action, state), }; + + // mute must come after streams, for REGISTER_COMPLETE events + state.mute = applyReducer('mute', mute, state.mute, action, state); + + nextPerAccountState = state; } const state = globalState; diff --git a/src/mute/__tests__/mute-testlib.js b/src/mute/__tests__/mute-testlib.js index 4a5a4a1d76e..672c1e31669 100644 --- a/src/mute/__tests__/mute-testlib.js +++ b/src/mute/__tests__/mute-testlib.js @@ -3,11 +3,19 @@ import { reducer } from '../muteModel'; import type { MuteState } from '../muteModelTypes'; import type { Stream } from '../../api/modelTypes'; +import * as eg from '../../__tests__/lib/exampleData'; import { EVENT_MUTED_TOPICS } from '../../actionConstants'; -export const makeMuteState = (data: $ReadOnlyArray<[Stream, string]>): MuteState => - reducer(undefined, { - type: EVENT_MUTED_TOPICS, - id: -1, - muted_topics: data.map(([stream, topic]) => [stream.name, topic]), - }); +export const makeMuteState = (data: $ReadOnlyArray<[Stream, string]>): MuteState => { + const streams = new Set(data.map(([stream, topic]) => stream)); + + return reducer( + undefined, + { + type: EVENT_MUTED_TOPICS, + id: -1, + muted_topics: data.map(([stream, topic]) => [stream.name, topic]), + }, + eg.reduxState({ streams: [...streams] }), + ); +}; diff --git a/src/mute/__tests__/muteModel-test.js b/src/mute/__tests__/muteModel-test.js index 58b559ad792..080201e462d 100644 --- a/src/mute/__tests__/muteModel-test.js +++ b/src/mute/__tests__/muteModel-test.js @@ -1,31 +1,46 @@ /* @flow strict-local */ import deepFreeze from 'deep-freeze'; -import { reducer } from '../muteModel'; +import fullReducer from '../../boot/reducers'; +import { getMute, reducer } from '../muteModel'; import { EVENT_MUTED_TOPICS } from '../../actionConstants'; import * as eg from '../../__tests__/lib/exampleData'; import { makeMuteState } from './mute-testlib'; +import { tryGetActiveAccountState } from '../../selectors'; const initialState = makeMuteState([]); describe('reducer', () => { describe('REGISTER_COMPLETE', () => { - test('when `mute` data is provided init state with it', () => { + test('when mute data is provided init state with it: local', () => { const action = eg.mkActionRegisterComplete({ muted_topics: [[eg.stream.name, 'topic']] }); - expect(reducer(initialState, action)).toEqual(makeMuteState([[eg.stream, 'topic']])); + expect(reducer(initialState, action, eg.plusReduxState)).toEqual( + makeMuteState([[eg.stream, 'topic']]), + ); + }); + + test('when mute data is provided init state with it: end-to-end', () => { + const action = eg.mkActionRegisterComplete({ + streams: [eg.stream], + subscriptions: [eg.subscription], + muted_topics: [[eg.stream.name, 'topic']], + }); + const newState = tryGetActiveAccountState(fullReducer(eg.baseReduxState, action)); + expect(newState).toBeTruthy(); + expect(newState && getMute(newState)).toEqual(makeMuteState([[eg.stream, 'topic']])); }); test('when no `mute` data is given reset state', () => { const state = makeMuteState([[eg.stream, 'topic']]); const action = eg.mkActionRegisterComplete({ muted_topics: [] }); - expect(reducer(state, action)).toEqual(initialState); + expect(reducer(state, action, eg.plusReduxState)).toEqual(initialState); }); }); describe('ACCOUNT_SWITCH', () => { test('resets state to initial state', () => { const state = makeMuteState([[eg.stream, 'some_topic']]); - expect(reducer(state, eg.action.account_switch)).toEqual(initialState); + expect(reducer(state, eg.action.account_switch, eg.plusReduxState)).toEqual(initialState); }); }); @@ -36,7 +51,9 @@ describe('reducer', () => { id: -1, muted_topics: [[eg.stream.name, 'topic']], }); - expect(reducer(initialState, action)).toEqual(makeMuteState([[eg.stream, 'topic']])); + expect(reducer(initialState, action, eg.plusReduxState)).toEqual( + makeMuteState([[eg.stream, 'topic']]), + ); }); }); }); diff --git a/src/mute/muteModel.js b/src/mute/muteModel.js index cd268548a9e..b90c9dc3832 100644 --- a/src/mute/muteModel.js +++ b/src/mute/muteModel.js @@ -1,7 +1,9 @@ /* @flow strict-local */ + import type { MuteState, PerAccountApplicableAction, PerAccountState } from '../types'; import { REGISTER_COMPLETE, LOGOUT, ACCOUNT_SWITCH, EVENT_MUTED_TOPICS } from '../actionConstants'; -import { NULL_ARRAY } from '../nullObjects'; +import { getStreamsByName } from '../subscriptions/subscriptionSelectors'; +import * as logging from '../utils/logging'; // // @@ -20,18 +22,37 @@ export const isTopicMuted = ( streamName: string, topic: string, mute: MuteState, -): boolean => mute.some(x => x[0] === streamName && x[1] === topic); +): boolean => mute.get(streamId)?.has(topic) ?? false; // // // Reducer. // -const initialState: MuteState = NULL_ARRAY; +const initialState: MuteState = new Map(); + +function convert(data, streams): MuteState { + const result = new Map(); + for (const [streamName, topic] of data) { + const stream = streams.get(streamName); + if (!stream) { + logging.warn('mute: unknown stream'); + continue; + } + let perStream = result.get(stream.stream_id); + if (!perStream) { + perStream = new Set(); + result.set(stream.stream_id, perStream); + } + perStream.add(topic); + } + return result; +} export const reducer = ( state: MuteState = initialState, action: PerAccountApplicableAction, + globalState: PerAccountState, ): MuteState => { switch (action.type) { case LOGOUT: @@ -39,10 +60,13 @@ export const reducer = ( return initialState; case REGISTER_COMPLETE: - return action.data.muted_topics || initialState; + // We require this `globalState` to reflect the `streams` sub-reducer + // already having run, so that `getStreamsByName` gives the data we + // just received. See this sub-reducer's call site in `reducers.js`. + return convert(action.data.muted_topics ?? [], getStreamsByName(globalState)); case EVENT_MUTED_TOPICS: - return action.muted_topics; + return convert(action.muted_topics, getStreamsByName(globalState)); default: return state; diff --git a/src/mute/muteModelTypes.js b/src/mute/muteModelTypes.js index c7b51ca1c56..dee51b5d8f8 100644 --- a/src/mute/muteModelTypes.js +++ b/src/mute/muteModelTypes.js @@ -1,9 +1,5 @@ // @flow strict-local -import type { MutedTopicTuple } from '../api/apiTypes'; - -// TODO(#3918): Use stream IDs for muted topics, not stream names. -// Server issue for fixing that in the API: https://github.com/zulip/zulip/issues/21015 -// Meanwhile, even while the server is still sending stream names, -// we should convert to stream IDs at the edge. -export type MuteState = $ReadOnlyArray; +// No need for Immutable, because -- the server API actually doesn't support +// incremental changes! When something changes, it sends the entire new list. +export type MuteState = Map>; // stream ID, topic diff --git a/src/storage/__tests__/migrations-test.js b/src/storage/__tests__/migrations-test.js index a1ceeed6dc8..7f646ddad7f 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: 39 }, + migrations: { version: 40 }, }; for (const [desc, before, after] of [ @@ -110,8 +110,8 @@ describe('migrations', () => { // redundant with this one, because none of the migration steps notice // whether any properties outside `storeKeys` are present or not. [ - 'check dropCache at 37', - { ...endBase, migrations: { version: 36 }, mute: [], nonsense: [1, 2, 3] }, + 'check dropCache at 40', + { ...endBase, migrations: { version: 39 }, mute: [], nonsense: [1, 2, 3] }, endBase, ], diff --git a/src/storage/migrations.js b/src/storage/migrations.js index 3eaa9afb58f..2f23603d1fd 100644 --- a/src/storage/migrations.js +++ b/src/storage/migrations.js @@ -375,6 +375,9 @@ const migrationsInner: {| [string]: (LessPartialState) => LessPartialState |} = ), }), + // Change `state.mute` data structure: was an array with stream names. + '40': dropCache, + // TIP: When adding a migration, consider just using `dropCache`. }; From 512de57dbc95e0908ccfe862ee9d94c399646072 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 31 Jan 2022 22:18:48 -0800 Subject: [PATCH 27/28] mute: Stop taking stream name in isTopicMuted In the previous commit, we already stopped looking at this argument in favor of the stream ID. This change should be NFC except in the case where the given stream didn't exist in our data structures, at the several call sites that were looking it up in order to find the name. Those now simply go and look up the mute status, where they presumably find nothing. In particular if it's possible to reach the call site in constructTopicActionButtons in that case, then previously we would crash; now we offer to mute the topic. --- src/action-sheets/index.js | 7 ++----- src/chat/narrowsSelectors.js | 5 ++--- src/mute/muteModel.js | 8 ++------ src/topics/topicSelectors.js | 11 +++++------ src/unread/unreadSelectors.js | 35 ++++++++++------------------------- 5 files changed, 21 insertions(+), 45 deletions(-) diff --git a/src/action-sheets/index.js b/src/action-sheets/index.js index ce0c6983e11..8f75ba558ca 100644 --- a/src/action-sheets/index.js +++ b/src/action-sheets/index.js @@ -344,13 +344,12 @@ export const constructStreamActionButtons = ({ }; export const constructTopicActionButtons = ({ - backgroundData: { mute, ownUser, streams, subscriptions, unread }, + backgroundData: { mute, ownUser, subscriptions, unread }, streamId, topic, }: {| backgroundData: $ReadOnly<{ mute: MuteState, - streams: Map, subscriptions: Map, unread: UnreadState, ownUser: User, @@ -367,9 +366,7 @@ export const constructTopicActionButtons = ({ if (unreadCount > 0) { buttons.push(markTopicAsRead); } - const stream = streams.get(streamId); - invariant(stream !== undefined, 'Stream with provided streamId not found.'); - if (isTopicMuted(stream.stream_id, stream.name, topic, mute)) { + if (isTopicMuted(streamId, topic, mute)) { buttons.push(unmuteTopic); } else { buttons.push(muteTopic); diff --git a/src/chat/narrowsSelectors.js b/src/chat/narrowsSelectors.js index 9f338857ee3..55626be0e00 100644 --- a/src/chat/narrowsSelectors.js +++ b/src/chat/narrowsSelectors.js @@ -138,7 +138,7 @@ export const getShownMessagesForNarrow: Selector<$ReadOnlyArray state.mute; // Getters. // -export const isTopicMuted = ( - streamId: number, - streamName: string, - topic: string, - mute: MuteState, -): boolean => mute.get(streamId)?.has(topic) ?? false; +export const isTopicMuted = (streamId: number, topic: string, mute: MuteState): boolean => + mute.get(streamId)?.has(topic) ?? false; // // diff --git a/src/topics/topicSelectors.js b/src/topics/topicSelectors.js index cef49edf609..bbd085b2d14 100644 --- a/src/topics/topicSelectors.js +++ b/src/topics/topicSelectors.js @@ -4,7 +4,6 @@ import { createSelector } from 'reselect'; import type { Narrow, Selector, TopicExtended, TopicsState } from '../types'; import { getTopics } from '../directSelectors'; import { getUnread, getUnreadCountForTopic } from '../unread/unreadModel'; -import { getStreamsById } from '../subscriptions/subscriptionSelectors'; import { NULL_ARRAY } from '../nullObjects'; import { isStreamNarrow, streamIdOfNarrow } from '../utils/narrow'; import { getMute, isTopicMuted } from '../mute/muteModel'; @@ -26,18 +25,18 @@ export const getTopicsForNarrow: Selector<$ReadOnlyArray, Narrow> = crea ); export const getTopicsForStream: Selector, number> = createSelector( + (state, streamId) => streamId, (state, streamId) => getTopics(state)[streamId], state => getMute(state), - (state, streamId) => getStreamsById(state).get(streamId), state => getUnread(state), - (topicList, mute, stream, unread) => { - if (!topicList || !stream) { + (streamId, topicList, mute, unread) => { + if (!topicList) { return undefined; } return topicList.map(({ name, max_id }) => { - const isMuted = isTopicMuted(stream.stream_id, stream.name, name, mute); - const unreadCount = getUnreadCountForTopic(unread, stream.stream_id, name); + const isMuted = isTopicMuted(streamId, name, mute); + const unreadCount = getUnreadCountForTopic(unread, streamId, name); return { name, max_id, isMuted, unreadCount }; }); }, diff --git a/src/unread/unreadSelectors.js b/src/unread/unreadSelectors.js index 96b8deea80c..b7c2d1e38ba 100644 --- a/src/unread/unreadSelectors.js +++ b/src/unread/unreadSelectors.js @@ -21,19 +21,13 @@ import { /** The number of unreads in each stream, excluding muted topics, by stream ID. */ export const getUnreadByStream: Selector<{| [number]: number |}> = createSelector( getUnreadStreams, - getSubscriptionsById, getMute, - (unreadStreams, subscriptionsById, mute) => { + (unreadStreams, mute) => { const totals = ({}: {| [number]: number |}); for (const [streamId, streamData] of unreadStreams.entries()) { let total = 0; for (const [topic, msgIds] of streamData) { - const isMuted = isTopicMuted( - streamId, - (subscriptionsById.get(streamId) || NULL_SUBSCRIPTION).name, - topic, - mute, - ); + const isMuted = isTopicMuted(streamId, topic, mute); total += isMuted ? 0 : msgIds.size; } totals[streamId] = total; @@ -155,7 +149,7 @@ export const getUnreadStreamsAndTopics: Selector<$ReadOnlyArray = createSelector( caseNarrow(narrow, { home: () => unreadTotal, - stream: streamId => { - const stream = streams.get(streamId); - if (!stream) { - return 0; - } - // prettier-ignore - return ( - unread.streams - .get(stream.stream_id) - ?.entrySeq() - .filterNot(([topic, _]) => isTopicMuted(stream.stream_id, stream.name, topic, mute)) - .map(([_, msgIds]) => msgIds.size) - .reduce((s, x) => s + x, 0) - ?? 0 - ); - }, + stream: streamId => + unread.streams + .get(streamId) + ?.entrySeq() + .filterNot(([topic, _]) => isTopicMuted(streamId, topic, mute)) + .map(([_, msgIds]) => msgIds.size) + .reduce((s, x) => s + x, 0) ?? 0, topic: (streamId, topic) => getUnreadCountForTopic(unread, streamId, topic), From 2f581fd74218bb337ad939525be84fe33f32b87f Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 7 Feb 2022 19:21:46 -0800 Subject: [PATCH 28/28] DefaultMap: Add this small utility, and use it Nothing radical, but we have these two instances of this pattern now and I think they both get a bit simpler to read this way. --- src/mute/muteModel.js | 12 ++++------ src/unread/unreadModel.js | 12 ++++------ src/utils/DefaultMap.js | 33 ++++++++++++++++++++++++++ src/utils/__tests__/DefaultMap-test.js | 24 +++++++++++++++++++ 4 files changed, 65 insertions(+), 16 deletions(-) create mode 100644 src/utils/DefaultMap.js create mode 100644 src/utils/__tests__/DefaultMap-test.js diff --git a/src/mute/muteModel.js b/src/mute/muteModel.js index 11ed6cc4830..e859be7e4d9 100644 --- a/src/mute/muteModel.js +++ b/src/mute/muteModel.js @@ -4,6 +4,7 @@ import type { MuteState, PerAccountApplicableAction, PerAccountState } from '../ import { REGISTER_COMPLETE, LOGOUT, ACCOUNT_SWITCH, EVENT_MUTED_TOPICS } from '../actionConstants'; import { getStreamsByName } from '../subscriptions/subscriptionSelectors'; import * as logging from '../utils/logging'; +import DefaultMap from '../utils/DefaultMap'; // // @@ -28,21 +29,16 @@ export const isTopicMuted = (streamId: number, topic: string, mute: MuteState): const initialState: MuteState = new Map(); function convert(data, streams): MuteState { - const result = new Map(); + const result = new DefaultMap(() => new Set()); for (const [streamName, topic] of data) { const stream = streams.get(streamName); if (!stream) { logging.warn('mute: unknown stream'); continue; } - let perStream = result.get(stream.stream_id); - if (!perStream) { - perStream = new Set(); - result.set(stream.stream_id, perStream); - } - perStream.add(topic); + result.getOrCreate(stream.stream_id).add(topic); } - return result; + return result.map; } export const reducer = ( diff --git a/src/unread/unreadModel.js b/src/unread/unreadModel.js index ea2f99430b9..b0bc9164da4 100644 --- a/src/unread/unreadModel.js +++ b/src/unread/unreadModel.js @@ -28,6 +28,7 @@ import { REGISTER_COMPLETE, } from '../actionConstants'; import * as logging from '../utils/logging'; +import DefaultMap from '../utils/DefaultMap'; // // @@ -159,16 +160,11 @@ function streamsReducer( // First, collect together all the data for a given stream, just in a // plain old Array. - const byStream = new Map(); + const byStream = new DefaultMap(() => []); for (const { stream_id, topic, unread_message_ids } of data) { - let perStream = byStream.get(stream_id); - if (!perStream) { - perStream = []; - byStream.set(stream_id, perStream); - } // unread_message_ids is already sorted; see comment at its // definition in src/api/initialDataTypes.js. - perStream.push([topic, Immutable.List(unread_message_ids)]); + byStream.getOrCreate(stream_id).push([topic, Immutable.List(unread_message_ids)]); } // Then, for each of those plain Arrays build an Immutable.Map from it @@ -176,7 +172,7 @@ function streamsReducer( // incrementally. For a user with lots of unreads in a busy org, we // can be handling 50k message IDs here, across perhaps 2-5k threads // in dozens of streams, so the effect is significant. - return Immutable.Map(Immutable.Seq.Keyed(byStream.entries()).map(Immutable.Map)); + return Immutable.Map(Immutable.Seq.Keyed(byStream.map.entries()).map(Immutable.Map)); } case MESSAGE_FETCH_COMPLETE: diff --git a/src/utils/DefaultMap.js b/src/utils/DefaultMap.js new file mode 100644 index 00000000000..4737d59a618 --- /dev/null +++ b/src/utils/DefaultMap.js @@ -0,0 +1,33 @@ +// @flow strict-local + +/** + * A wrapper around Map, akin to Python's `defaultdict`. + */ +// This could be a subclass of Map instead. That would also mean it gets +// all the handy methods of Map. +// +// The slightly tricky bit is that that would mean it could enter our Redux +// state where the type says simply `Map`. So `replaceRevive` would need to +// handle it, and some picky tests might need adjustment too. +// +// We haven't done that yet mainly because we haven't yet had much of a use +// case for using this outside of small local contexts where we don't need +// the other methods of Map. +export default class DefaultMap { + map: Map = new Map(); + default: () => V; + + constructor(default_: () => V) { + this.default = default_; + } + + /** Return the value at the given key, creating it if necessary. */ + getOrCreate(key: K): V { + let value = this.map.get(key); + if (value === undefined) { + value = this.default(); + this.map.set(key, value); + } + return value; + } +} diff --git a/src/utils/__tests__/DefaultMap-test.js b/src/utils/__tests__/DefaultMap-test.js new file mode 100644 index 00000000000..ac21ab1703a --- /dev/null +++ b/src/utils/__tests__/DefaultMap-test.js @@ -0,0 +1,24 @@ +// @flow strict-local + +import DefaultMap from '../DefaultMap'; + +describe('DefaultMap', () => { + test('smoke', () => { + const m = new DefaultMap(() => []); + expect([...m.map.entries()].sort()).toEqual([]); + + // Create a value. + m.getOrCreate('a').push(1); + expect([...m.map.entries()].sort()).toEqual([['a', [1]]]); + + // Different key gets a fresh value. + m.getOrCreate('b').push(2); + // prettier-ignore + expect([...m.map.entries()].sort()).toEqual([['a', [1]], ['b', [2]]]); + + // Existing key gets the existing value. + m.getOrCreate('a').push(3); + // prettier-ignore + expect([...m.map.entries()].sort()).toEqual([['a', [1, 3]], ['b', [2]]]); + }); +});