From 787cbab167b38cddec82cee3c75095e0f9ae910a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 11 May 2022 21:56:48 -0700 Subject: [PATCH] wip mute: Request and consume new user_topic format in API TODO link issues / server PR In this commit, we start requesting the new `user_topic` format for the data describing the "muted topics" feature. On new servers, that causes the new format to appear, and the old to disappear. Describe the API changes, and start consuming the new format. This should have no user-visible effect -- currently the new format describes exactly the same information as the old. Taking the client plus (a properly behaving) server as one system, this change is therefore NFC. We don't call the change NFC, though, because it does change the client's behavior as witnessed by the server. (This change does switch from stream names to stream IDs in the API... but only for data that we get from the server through the event system, not for data we send or receive in any request outside that system. That means that (unless affected by some server bug) it's always working with a stream name-to-ID mapping that exactly matches what the server had at the time it sent that data. So even if a stream gets renamed concurrently with getting some data here, there's no race and no bug. OTOH when we *change* this at the server on the user's behalf, that does suffer from such a race. But that part of the API was fixed long ago to accept stream IDs; we already have a TODO-server comment, at `api.setTopicMute`, to start relying on that update.) Fixes: # 5380 --- src/__tests__/lib/exampleData.js | 6 +- src/actionTypes.js | 4 +- src/api/eventTypes.js | 9 ++ src/api/initialDataTypes.js | 13 ++- src/api/registerForEvents.js | 3 +- src/events/eventToAction.js | 1 + src/mute/__tests__/mute-testlib.js | 31 ++++--- src/mute/__tests__/muteModel-test.js | 128 +++++++++++++++++++++++++-- src/mute/muteModel.js | 69 +++++++++++++-- 9 files changed, 229 insertions(+), 35 deletions(-) diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index 8cd41409028..57945ac1b02 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -737,9 +737,6 @@ export const action = Object.freeze({ // InitialDataMessage max_message_id: 100, - // InitialDataMutedTopics - muted_topics: [], - // InitialDataMutedUsers muted_users: [], @@ -950,6 +947,9 @@ export const action = Object.freeze({ // InitialDataUserStatus user_status: {}, + + // InitialDataUserTopic + user_topics: [], }, }): RegisterCompleteAction), diff --git a/src/actionTypes.js b/src/actionTypes.js index 754c477adb4..a886c461b88 100644 --- a/src/actionTypes.js +++ b/src/actionTypes.js @@ -75,6 +75,7 @@ import type { RestartEvent, UpdateMessageEvent, RealmUserUpdateEvent, + UserTopicEvent, } from './api/eventTypes'; import type { MutedTopicTuple, PresenceSnapshot } from './api/apiTypes'; import type { MessageMove } from './api/misc'; @@ -336,8 +337,9 @@ type GenericEventAction = $ReadOnly<{| | RestartEvent | RealmUpdateEvent | RealmUpdateDictEvent + | RealmUserUpdateEvent | UserSettingsUpdateEvent - | RealmUserUpdateEvent, + | UserTopicEvent, |}>; type EventNewMessageAction = $ReadOnly<{| diff --git a/src/api/eventTypes.js b/src/api/eventTypes.js index ab75a6fc01d..762e06bd971 100644 --- a/src/api/eventTypes.js +++ b/src/api/eventTypes.js @@ -25,6 +25,7 @@ import type { UserStatusUpdate, UserSettings, ClientPresence, + UserTopic, } from './modelTypes'; import type { RealmDataForUpdate } from './realmDataTypes'; @@ -69,6 +70,7 @@ export const EventTypes = keyMirror({ user_group: null, user_settings: null, user_status: null, + user_topic: null, }); export type EventType = $Keys; @@ -449,3 +451,10 @@ export type RealmUserUpdateEvent = {| // avatar_url_medium: string, |}, |}; + +export type UserTopicEvent = {| + ...EventCommon, + +type: typeof EventTypes.user_topic, + + ...UserTopic, +|}; diff --git a/src/api/initialDataTypes.js b/src/api/initialDataTypes.js index 57ed21d64d3..76ad96a285e 100644 --- a/src/api/initialDataTypes.js +++ b/src/api/initialDataTypes.js @@ -17,6 +17,7 @@ import type { UserId, UserSettings, UserStatusUpdate, + UserTopic, } from './modelTypes'; import type { CreatePublicOrPrivateStreamPolicyT, @@ -90,7 +91,9 @@ export type InitialDataMessage = $ReadOnly<{| |}>; export type InitialDataMutedTopics = $ReadOnly<{| - muted_topics: $ReadOnlyArray, + /** Omitted by new servers that send `user_topics`; read that instead. */ + // TODO(server-6.0): Remove; gone in FL 134 (given that we request `user_topic`). + muted_topics?: $ReadOnlyArray, |}>; export type InitialDataMutedUsers = $ReadOnly<{| @@ -674,6 +677,13 @@ export type InitialDataUserStatus = $ReadOnly<{| |}>, |}>; +/** Initial data for the `user_topic` event type. */ +export type InitialDataUserTopic = {| + /** When absent (on older servers), read `muted_topics` instead. */ + // TODO(server-6.0): Introduced in FL 134. + +user_topics?: $ReadOnlyArray, +|}; + /** * The initial data snapshot sent on `/register`. * @@ -718,6 +728,7 @@ export type InitialData = {| ...InitialDataUpdateMessageFlags, ...InitialDataUserSettings, ...InitialDataUserStatus, + ...InitialDataUserTopic, |}; /** diff --git a/src/api/registerForEvents.js b/src/api/registerForEvents.js index 6d1a161f34c..5d06e52e6ff 100644 --- a/src/api/registerForEvents.js +++ b/src/api/registerForEvents.js @@ -149,7 +149,7 @@ export default async ( 'alert_words', 'custom_profile_fields', 'message', - 'muted_topics', + 'muted_topics', // TODO(server-6.0): Stop requesting, in favor of user_topic 'muted_users', 'presence', 'realm', @@ -170,6 +170,7 @@ export default async ( 'update_message_flags', 'user_settings', 'user_status', + 'user_topic', 'zulip_version', ]), diff --git a/src/events/eventToAction.js b/src/events/eventToAction.js index d59b8de13fd..1f40b26c740 100644 --- a/src/events/eventToAction.js +++ b/src/events/eventToAction.js @@ -168,6 +168,7 @@ export default (state: PerAccountState, event: $FlowFixMe): EventAction | null = case 'custom_profile_fields': case 'stream': case 'user_settings': + case 'user_topic': return { type: EVENT, event, diff --git a/src/mute/__tests__/mute-testlib.js b/src/mute/__tests__/mute-testlib.js index 672c1e31669..c79f5559429 100644 --- a/src/mute/__tests__/mute-testlib.js +++ b/src/mute/__tests__/mute-testlib.js @@ -2,20 +2,27 @@ import { reducer } from '../muteModel'; import type { MuteState } from '../muteModelTypes'; -import type { Stream } from '../../api/modelTypes'; +import { type Stream, type UserTopic, UserTopicVisibilityPolicy } from '../../api/modelTypes'; import * as eg from '../../__tests__/lib/exampleData'; -import { EVENT_MUTED_TOPICS } from '../../actionConstants'; -export const makeMuteState = (data: $ReadOnlyArray<[Stream, string]>): MuteState => { - const streams = new Set(data.map(([stream, topic]) => stream)); +export const makeUserTopic = ( + stream: Stream, + topic: string, + visibility_policy: UserTopicVisibilityPolicy, +): UserTopic => ({ + stream_id: stream.stream_id, + topic_name: topic, + visibility_policy, + last_updated: 12345, // arbitrary value; we ignore last_updated here +}); - return reducer( +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]), - }, - eg.reduxState({ streams: [...streams] }), + eg.mkActionRegisterComplete({ + user_topics: data.map(([stream, topic]) => + makeUserTopic(stream, topic, UserTopicVisibilityPolicy.Muted), + ), + }), + eg.reduxState(), ); -}; diff --git a/src/mute/__tests__/muteModel-test.js b/src/mute/__tests__/muteModel-test.js index 73aa4016f40..4b58ac91b17 100644 --- a/src/mute/__tests__/muteModel-test.js +++ b/src/mute/__tests__/muteModel-test.js @@ -3,11 +3,12 @@ import deepFreeze from 'deep-freeze'; import fullReducer from '../../boot/reducers'; import { getMute, getTopicVisibilityPolicy, isTopicMuted, reducer } from '../muteModel'; -import { EVENT_MUTED_TOPICS } from '../../actionConstants'; +import { EVENT, EVENT_MUTED_TOPICS } from '../../actionConstants'; import * as eg from '../../__tests__/lib/exampleData'; -import { makeMuteState } from './mute-testlib'; +import { makeMuteState, makeUserTopic } from './mute-testlib'; import { tryGetActiveAccountState } from '../../selectors'; import { UserTopicVisibilityPolicy } from '../../api/modelTypes'; +import { EventTypes } from '../../api/eventTypes'; /* eslint jest/expect-expect: ["error", { "assertFunctionNames": ["expect", "check"] }] */ @@ -53,18 +54,42 @@ describe('getters', () => { describe('reducer', () => { describe('REGISTER_COMPLETE', () => { - test('when mute data is provided init state with it: local', () => { - const action = eg.mkActionRegisterComplete({ muted_topics: [[eg.stream.name, 'topic']] }); + test('in modern user_topics format: unit test', () => { + const action = eg.mkActionRegisterComplete({ + user_topics: [makeUserTopic(eg.stream, 'topic', UserTopicVisibilityPolicy.Muted)], + }); expect(reducer(initialState, action, eg.plusReduxState)).toEqual( makeMuteState([[eg.stream, 'topic']]), ); }); - test('when mute data is provided init state with it: end-to-end', () => { + test('in modern user_topics format: end-to-end test', () => { const action = eg.mkActionRegisterComplete({ streams: [eg.stream], subscriptions: [eg.subscription], + user_topics: [makeUserTopic(eg.stream, 'topic', UserTopicVisibilityPolicy.Muted)], + }); + const newState = tryGetActiveAccountState(fullReducer(eg.plusReduxState, action)); + expect(newState).toBeTruthy(); + expect(newState && getMute(newState)).toEqual(makeMuteState([[eg.stream, 'topic']])); + }); + + test('in old muted_topics format: unit test', () => { + const action = eg.mkActionRegisterComplete({ muted_topics: [[eg.stream.name, 'topic']], + user_topics: undefined, + }); + expect(reducer(initialState, action, eg.plusReduxState)).toEqual( + makeMuteState([[eg.stream, 'topic']]), + ); + }); + + test('in old muted_topics format: end-to-end test', () => { + const action = eg.mkActionRegisterComplete({ + streams: [eg.stream], + subscriptions: [eg.subscription], + muted_topics: [[eg.stream.name, 'topic']], + user_topics: undefined, }); const newState = tryGetActiveAccountState(fullReducer(eg.plusReduxState, action)); expect(newState).toBeTruthy(); @@ -72,9 +97,12 @@ describe('reducer', () => { }); // TODO(#5102): Delete; see comment on implementation. - test('when no `mute` data is given reset state', () => { + test('in ancient no-muted-topics format', () => { const state = makeMuteState([[eg.stream, 'topic']]); - const action = eg.mkActionRegisterComplete({ muted_topics: [] }); + const action = eg.mkActionRegisterComplete({ + muted_topics: undefined, + user_topics: undefined, + }); expect(reducer(state, action, eg.plusReduxState)).toEqual(initialState); }); }); @@ -86,7 +114,91 @@ describe('reducer', () => { }); }); - describe('EVENT_MUTED_TOPICS', () => { + describe('EVENT > user_topic', () => { + function mkAction(userTopic) { + return { type: EVENT, event: { id: 0, type: EventTypes.user_topic, ...userTopic } }; + } + + function check(state, userTopic, expected) { + expect(reducer(state, mkAction(userTopic), eg.plusReduxState)).toEqual(expected); + } + + test('add with new stream', () => { + check( + initialState, + makeUserTopic(eg.stream, 'topic', UserTopicVisibilityPolicy.Muted), + makeMuteState([[eg.stream, 'topic']]), + ); + }); + + test('add in existing stream', () => { + check( + makeMuteState([[eg.stream, 'topic']]), + makeUserTopic(eg.stream, 'other topic', UserTopicVisibilityPolicy.Muted), + makeMuteState([ + [eg.stream, 'topic'], + [eg.stream, 'other topic'], + ]), + ); + }); + + test('remove, with others in stream', () => { + check( + makeMuteState([ + [eg.stream, 'topic'], + [eg.stream, 'other topic'], + ]), + makeUserTopic(eg.stream, 'other topic', UserTopicVisibilityPolicy.None), + makeMuteState([[eg.stream, 'topic']]), + ); + }); + + test('remove, as last in stream', () => { + check( + makeMuteState([[eg.stream, 'topic']]), + makeUserTopic(eg.stream, 'topic', UserTopicVisibilityPolicy.None), + initialState, + ); + }); + + describe('redundantly after EVENT_MUTED_TOPICS', () => { + // The server may send both muted_topics events and user_topic events, + // because we don't set event_types in our /register request: + // https://github.com/zulip/zulip/pull/21251#issuecomment-1133466148 + // So we may get one of these after a muted_topics event has already + // set the new state. + // + // (Or we might get user_topic first and then muted_topics, but that + // doesn't require any testing of its own -- when handling the + // muted_topics event it doesn't matter what the previous state was.) + + test('add', () => { + check( + makeMuteState([[eg.stream, 'topic']]), + makeUserTopic(eg.stream, 'topic', UserTopicVisibilityPolicy.Muted), + makeMuteState([[eg.stream, 'topic']]), + ); + }); + + test('remove, leaving others in stream', () => { + check( + makeMuteState([[eg.stream, 'topic']]), + makeUserTopic(eg.stream, 'other topic', UserTopicVisibilityPolicy.None), + makeMuteState([[eg.stream, 'topic']]), + ); + }); + + test('remove, as last in stream', () => { + check( + makeMuteState([]), + makeUserTopic(eg.stream, 'topic', UserTopicVisibilityPolicy.None), + makeMuteState([]), + ); + }); + }); + }); + + describe('EVENT_MUTED_TOPICS (legacy)', () => { test('appends and test a new muted topic', () => { const action = deepFreeze({ type: EVENT_MUTED_TOPICS, diff --git a/src/mute/muteModel.js b/src/mute/muteModel.js index d966fcff6db..647af8cb898 100644 --- a/src/mute/muteModel.js +++ b/src/mute/muteModel.js @@ -3,10 +3,17 @@ import Immutable from 'immutable'; import type { MuteState, PerAccountApplicableAction, PerAccountState } from '../types'; import { UserTopicVisibilityPolicy } from '../api/modelTypes'; -import { REGISTER_COMPLETE, EVENT_MUTED_TOPICS, RESET_ACCOUNT_DATA } from '../actionConstants'; +import { EventTypes } from '../api/eventTypes'; +import { + REGISTER_COMPLETE, + EVENT_MUTED_TOPICS, + RESET_ACCOUNT_DATA, + EVENT, +} from '../actionConstants'; import { getStreamsByName } from '../subscriptions/subscriptionSelectors'; import * as logging from '../utils/logging'; import DefaultMap from '../utils/DefaultMap'; +import { updateAndPrune } from '../immutableUtils'; // // @@ -45,7 +52,24 @@ export function isTopicMuted(streamId: number, topic: string, mute: MuteState): const initialState: MuteState = Immutable.Map(); -function convert(data, streams): MuteState { +/** Consume the old `muted_topics` format. */ +function convertLegacy(data, streams): MuteState { + // Same strategy as in convertInitial, below. + + const byStream = new DefaultMap(() => []); + for (const [streamName, topic] of data) { + const stream = streams.get(streamName); + if (!stream) { + logging.warn('mute: unknown stream'); + continue; + } + byStream.getOrCreate(stream.stream_id).push([topic, UserTopicVisibilityPolicy.Muted]); + } + + return Immutable.Map(Immutable.Seq.Keyed(byStream.map.entries()).map(Immutable.Map)); +} + +function convertInitial(data): MuteState { // Turn the incoming array into a nice, indexed, Immutable data structure // in the same two-phase pattern we use for the unread data, as an // optimization. Here it's probably much less often enough data for the @@ -55,13 +79,13 @@ function convert(data, streams): MuteState { // First, collect together all the data for a given stream, just in a // plain old Array. const byStream = new DefaultMap(() => []); - for (const [streamName, topic] of data) { - const stream = streams.get(streamName); - if (!stream) { - logging.warn('mute: unknown stream'); + for (const { stream_id, topic_name, visibility_policy } of data) { + if (!UserTopicVisibilityPolicy.isValid((visibility_policy: number))) { + // Not a value we expect. Keep it out of our data structures. + logging.warn(`unexpected UserTopicVisibilityPolicy: ${(visibility_policy: number)}`); continue; } - byStream.getOrCreate(stream.stream_id).push([topic, UserTopicVisibilityPolicy.Muted]); + byStream.getOrCreate(stream_id).push([topic_name, visibility_policy]); } // Then, from each of those build an Immutable.Map all in one shot. @@ -78,10 +102,14 @@ export const reducer = ( return initialState; case REGISTER_COMPLETE: + if (action.data.user_topics) { + return convertInitial(action.data.user_topics); + } // 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( + // TODO(server-6.0): Stop caring about that, when we cut muted_topics. + return convertLegacy( action.data.muted_topics // TODO(#5102): Delete fallback once we enforce any threshold for // ancient servers we refuse to connect to. It was added in @@ -94,7 +122,30 @@ export const reducer = ( ); case EVENT_MUTED_TOPICS: - return convert(action.muted_topics, getStreamsByName(globalState)); + return convertLegacy(action.muted_topics, getStreamsByName(globalState)); + + case EVENT: { + const { event } = action; + switch (event.type) { + case EventTypes.user_topic: { + const { stream_id, topic_name, visibility_policy } = event; + if (visibility_policy === UserTopicVisibilityPolicy.None) { + // This is the "zero value" for this type, which our MuteState + // data structure represents by leaving the topic out entirely. + return updateAndPrune( + state, + Immutable.Map(), + stream_id, + (perStream = Immutable.Map()) => perStream.delete(topic_name), + ); + } + return state.updateIn([stream_id, topic_name], () => visibility_policy); + } + + default: + return state; + } + } default: return state;