Skip to content

Commit

Permalink
wip mute: Request and consume new user_topic format in API
Browse files Browse the repository at this point in the history
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
  • Loading branch information
gnprice committed Feb 15, 2023
1 parent 00af290 commit 787cbab
Show file tree
Hide file tree
Showing 9 changed files with 229 additions and 35 deletions.
6 changes: 3 additions & 3 deletions src/__tests__/lib/exampleData.js
Original file line number Diff line number Diff line change
Expand Up @@ -737,9 +737,6 @@ export const action = Object.freeze({
// InitialDataMessage
max_message_id: 100,

// InitialDataMutedTopics
muted_topics: [],

// InitialDataMutedUsers
muted_users: [],

Expand Down Expand Up @@ -950,6 +947,9 @@ export const action = Object.freeze({

// InitialDataUserStatus
user_status: {},

// InitialDataUserTopic
user_topics: [],
},
}): RegisterCompleteAction),

Expand Down
4 changes: 3 additions & 1 deletion src/actionTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -336,8 +337,9 @@ type GenericEventAction = $ReadOnly<{|
| RestartEvent
| RealmUpdateEvent
| RealmUpdateDictEvent
| RealmUserUpdateEvent
| UserSettingsUpdateEvent
| RealmUserUpdateEvent,
| UserTopicEvent,
|}>;

type EventNewMessageAction = $ReadOnly<{|
Expand Down
9 changes: 9 additions & 0 deletions src/api/eventTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import type {
UserStatusUpdate,
UserSettings,
ClientPresence,
UserTopic,
} from './modelTypes';
import type { RealmDataForUpdate } from './realmDataTypes';

Expand Down Expand Up @@ -69,6 +70,7 @@ export const EventTypes = keyMirror({
user_group: null,
user_settings: null,
user_status: null,
user_topic: null,
});

export type EventType = $Keys<typeof EventTypes>;
Expand Down Expand Up @@ -449,3 +451,10 @@ export type RealmUserUpdateEvent = {|
// avatar_url_medium: string,
|},
|};

export type UserTopicEvent = {|
...EventCommon,
+type: typeof EventTypes.user_topic,

...UserTopic,
|};
13 changes: 12 additions & 1 deletion src/api/initialDataTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import type {
UserId,
UserSettings,
UserStatusUpdate,
UserTopic,
} from './modelTypes';
import type {
CreatePublicOrPrivateStreamPolicyT,
Expand Down Expand Up @@ -90,7 +91,9 @@ export type InitialDataMessage = $ReadOnly<{|
|}>;

export type InitialDataMutedTopics = $ReadOnly<{|
muted_topics: $ReadOnlyArray<MutedTopicTuple>,
/** 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<MutedTopicTuple>,
|}>;

export type InitialDataMutedUsers = $ReadOnly<{|
Expand Down Expand Up @@ -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<UserTopic>,
|};

/**
* The initial data snapshot sent on `/register`.
*
Expand Down Expand Up @@ -718,6 +728,7 @@ export type InitialData = {|
...InitialDataUpdateMessageFlags,
...InitialDataUserSettings,
...InitialDataUserStatus,
...InitialDataUserTopic,
|};

/**
Expand Down
3 changes: 2 additions & 1 deletion src/api/registerForEvents.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -170,6 +170,7 @@ export default async (
'update_message_flags',
'user_settings',
'user_status',
'user_topic',
'zulip_version',
]),

Expand Down
1 change: 1 addition & 0 deletions src/events/eventToAction.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
31 changes: 19 additions & 12 deletions src/mute/__tests__/mute-testlib.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
);
};
128 changes: 120 additions & 8 deletions src/mute/__tests__/muteModel-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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"] }] */

Expand Down Expand Up @@ -53,28 +54,55 @@ 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();
expect(newState && getMute(newState)).toEqual(makeMuteState([[eg.stream, 'topic']]));
});

// 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);
});
});
Expand All @@ -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,
Expand Down
Loading

0 comments on commit 787cbab

Please sign in to comment.