From 7e7f0a80e7d700b7da6e4ee6591fab675c5fc708 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 20 Mar 2023 14:49:37 -0700 Subject: [PATCH] mute: Recognize unmuted topics in muted streams Fixes: #5362 --- src/action-sheets/index.js | 1 + src/api/modelTypes.js | 5 + src/chat/__tests__/narrowsSelectors-test.js | 8 ++ src/chat/narrowsSelectors.js | 12 +-- src/mute/__tests__/mute-testlib.js | 18 +++- src/mute/__tests__/muteModel-test.js | 101 +++++++++++++++++-- src/mute/muteModel.js | 45 ++++++++- src/topics/topicSelectors.js | 15 +-- src/unread/__tests__/unreadSelectors-test.js | 33 ++++++ src/unread/unreadSelectors.js | 10 +- 10 files changed, 211 insertions(+), 37 deletions(-) diff --git a/src/action-sheets/index.js b/src/action-sheets/index.js index f64d017b91e..01a127c0e6e 100644 --- a/src/action-sheets/index.js +++ b/src/action-sheets/index.js @@ -653,6 +653,7 @@ export const constructTopicActionButtons = (args: {| buttons.push(unmuteTopic); break; case UserTopicVisibilityPolicy.None: + case UserTopicVisibilityPolicy.Unmuted: buttons.push(muteTopic); break; } diff --git a/src/api/modelTypes.js b/src/api/modelTypes.js index c7bc7dd1021..eaf9072dd87 100644 --- a/src/api/modelTypes.js +++ b/src/api/modelTypes.js @@ -643,6 +643,11 @@ export type Topic = $ReadOnly<{| export enum UserTopicVisibilityPolicy { None = 0, Muted = 1, + // Not in the API docs yet, but the API has been agreed on: + // https://chat.zulip.org/#narrow/stream/378-api-design/topic/muted.20topics/near/1501574 + // https://chat.zulip.org/#narrow/stream/378-api-design/topic/muted.20topics/near/1528885 + // TODO(server): delete this comment once documented + Unmuted = 2, } /** diff --git a/src/chat/__tests__/narrowsSelectors-test.js b/src/chat/__tests__/narrowsSelectors-test.js index 1a217252666..eeb91709f84 100644 --- a/src/chat/__tests__/narrowsSelectors-test.js +++ b/src/chat/__tests__/narrowsSelectors-test.js @@ -23,6 +23,7 @@ import { import { NULL_SUBSCRIPTION } from '../../nullObjects'; import * as eg from '../../__tests__/lib/exampleData'; import { makeMuteState } from '../../mute/__tests__/mute-testlib'; +import { UserTopicVisibilityPolicy } from '../../api/modelTypes'; describe('getMessagesForNarrow', () => { const message = eg.streamMessage({ id: 123 }); @@ -96,6 +97,7 @@ describe('getShownMessagesForNarrow', () => { const subscription = eg.subscription; const mutedSubscription = { ...subscription, in_home_view: false }; const muteTopic = makeMuteState([[stream, message.subject]]); + const unmuteTopic = makeMuteState([[stream, message.subject, UserTopicVisibilityPolicy.Unmuted]]); const makeStateGeneral = (message, narrow, extra) => eg.reduxStatePlus({ @@ -126,6 +128,12 @@ describe('getShownMessagesForNarrow', () => { expect(shown(makeState({ subscriptions: [mutedSubscription] }))).toEqual(false); }); + test('stream message shown if topic unmuted, even though stream muted', () => { + expect(shown(makeState({ subscriptions: [mutedSubscription], mute: unmuteTopic }))).toEqual( + true, + ); + }); + test('stream message hidden if topic muted', () => { expect(shown(makeState({ mute: muteTopic }))).toEqual(false); }); diff --git a/src/chat/narrowsSelectors.js b/src/chat/narrowsSelectors.js index 14c5898a139..de56597b0c8 100644 --- a/src/chat/narrowsSelectors.js +++ b/src/chat/narrowsSelectors.js @@ -23,7 +23,7 @@ import { caseNarrow, streamIdOfNarrow, } from '../utils/narrow'; -import { getMute, isTopicVisibleInStream } from '../mute/muteModel'; +import { getMute, isTopicVisibleInStream, isTopicVisible } from '../mute/muteModel'; import { NULL_ARRAY, NULL_SUBSCRIPTION } from '../nullObjects'; import * as logging from '../utils/logging'; import { getStreamsById, getSubscriptionsById } from '../subscriptions/subscriptionSelectors'; @@ -115,14 +115,12 @@ export const getShownMessagesForNarrow: Selector<$ReadOnlyArray diff --git a/src/mute/__tests__/mute-testlib.js b/src/mute/__tests__/mute-testlib.js index c79f5559429..7a86b873e25 100644 --- a/src/mute/__tests__/mute-testlib.js +++ b/src/mute/__tests__/mute-testlib.js @@ -16,13 +16,23 @@ export const makeUserTopic = ( last_updated: 12345, // arbitrary value; we ignore last_updated here }); -export const makeMuteState = (data: $ReadOnlyArray<[Stream, string]>): MuteState => +/** + * Convenience constructor for a MuteState. + * + * The tuples are (stream, topic, policy). + * The policy defaults to UserTopicVisibilityPolicy.Muted. + */ +export const makeMuteState = ( + data: $ReadOnlyArray<[Stream, string] | [Stream, string, UserTopicVisibilityPolicy]>, +): MuteState => reducer( undefined, eg.mkActionRegisterComplete({ - user_topics: data.map(([stream, topic]) => - makeUserTopic(stream, topic, UserTopicVisibilityPolicy.Muted), - ), + user_topics: data.map(args => { + // $FlowIgnore[invalid-tuple-index]: we're supplying a default + const [stream, topic, policy = UserTopicVisibilityPolicy.Muted] = args; + return makeUserTopic(stream, topic, policy); + }), }), eg.reduxState(), ); diff --git a/src/mute/__tests__/muteModel-test.js b/src/mute/__tests__/muteModel-test.js index 49581890933..c3b56ace2ab 100644 --- a/src/mute/__tests__/muteModel-test.js +++ b/src/mute/__tests__/muteModel-test.js @@ -2,7 +2,13 @@ import deepFreeze from 'deep-freeze'; import fullReducer from '../../boot/reducers'; -import { getMute, getTopicVisibilityPolicy, isTopicVisibleInStream, reducer } from '../muteModel'; +import { + getMute, + getTopicVisibilityPolicy, + isTopicVisible, + isTopicVisibleInStream, + reducer, +} from '../muteModel'; import { EVENT, EVENT_MUTED_TOPICS } from '../../actionConstants'; import * as eg from '../../__tests__/lib/exampleData'; import { makeMuteState, makeUserTopic } from './mute-testlib'; @@ -31,6 +37,13 @@ describe('getters', () => { test('with topic muted', () => { check(makeMuteState([[eg.stream, 'topic']]), UserTopicVisibilityPolicy.Muted); }); + + test('with topic unmuted', () => { + check( + makeMuteState([[eg.stream, 'topic', UserTopicVisibilityPolicy.Unmuted]]), + UserTopicVisibilityPolicy.Unmuted, + ); + }); }); describe('isTopicVisibleInStream', () => { @@ -49,6 +62,44 @@ describe('getters', () => { test('with topic muted', () => { check(makeMuteState([[eg.stream, 'topic']]), false); }); + + test('with topic unmuted', () => { + check(makeMuteState([[eg.stream, 'topic', UserTopicVisibilityPolicy.Unmuted]]), true); + }); + }); + + describe('isTopicVisible', () => { + function check(streamMuted, topicPolicy, expected) { + const subscription = { ...eg.subscription, in_home_view: !streamMuted }; + const state = makeMuteState( + topicPolicy === UserTopicVisibilityPolicy.None ? [] : [[eg.stream, 'topic', topicPolicy]], + ); + expect(isTopicVisible(eg.stream.stream_id, 'topic', subscription, state)).toEqual(expected); + } + + test('stream unmuted, topic-policy None', () => { + check(false, UserTopicVisibilityPolicy.None, true); + }); + + test('stream unmuted, topic-policy Muted', () => { + check(false, UserTopicVisibilityPolicy.Muted, false); + }); + + test('stream unmuted, topic-policy Unmuted', () => { + check(false, UserTopicVisibilityPolicy.Unmuted, true); + }); + + test('stream muted, topic-policy None', () => { + check(true, UserTopicVisibilityPolicy.None, false); + }); + + test('stream muted, topic-policy Muted', () => { + check(true, UserTopicVisibilityPolicy.Muted, false); + }); + + test('stream muted, topic-policy Unmuted', () => { + check(true, UserTopicVisibilityPolicy.Unmuted, true); + }); }); }); @@ -56,10 +107,16 @@ describe('reducer', () => { describe('REGISTER_COMPLETE', () => { test('in modern user_topics format: unit test', () => { const action = eg.mkActionRegisterComplete({ - user_topics: [makeUserTopic(eg.stream, 'topic', UserTopicVisibilityPolicy.Muted)], + user_topics: [ + makeUserTopic(eg.stream, 'topic', UserTopicVisibilityPolicy.Muted), + makeUserTopic(eg.stream, 'other topic', UserTopicVisibilityPolicy.Unmuted), + ], }); expect(reducer(initialState, action, eg.plusReduxState)).toEqual( - makeMuteState([[eg.stream, 'topic']]), + makeMuteState([ + [eg.stream, 'topic', UserTopicVisibilityPolicy.Muted], + [eg.stream, 'other topic', UserTopicVisibilityPolicy.Unmuted], + ]), ); }); @@ -67,11 +124,19 @@ describe('reducer', () => { const action = eg.mkActionRegisterComplete({ streams: [eg.stream], subscriptions: [eg.subscription], - user_topics: [makeUserTopic(eg.stream, 'topic', UserTopicVisibilityPolicy.Muted)], + user_topics: [ + makeUserTopic(eg.stream, 'topic', UserTopicVisibilityPolicy.Muted), + makeUserTopic(eg.stream, 'other topic', UserTopicVisibilityPolicy.Unmuted), + ], }); const newState = tryGetActiveAccountState(fullReducer(eg.plusReduxState, action)); expect(newState).toBeTruthy(); - expect(newState && getMute(newState)).toEqual(makeMuteState([[eg.stream, 'topic']])); + expect(newState && getMute(newState)).toEqual( + makeMuteState([ + [eg.stream, 'topic', UserTopicVisibilityPolicy.Muted], + [eg.stream, 'other topic', UserTopicVisibilityPolicy.Unmuted], + ]), + ); }); test('in old muted_topics format: unit test', () => { @@ -117,21 +182,37 @@ describe('reducer', () => { check( initialState, makeUserTopic(eg.stream, 'topic', UserTopicVisibilityPolicy.Muted), - makeMuteState([[eg.stream, 'topic']]), + makeMuteState([[eg.stream, 'topic', UserTopicVisibilityPolicy.Muted]]), ); }); test('add in existing stream', () => { check( - makeMuteState([[eg.stream, 'topic']]), - makeUserTopic(eg.stream, 'other topic', UserTopicVisibilityPolicy.Muted), + makeMuteState([[eg.stream, 'topic', UserTopicVisibilityPolicy.Muted]]), + makeUserTopic(eg.stream, 'other topic', UserTopicVisibilityPolicy.Unmuted), makeMuteState([ - [eg.stream, 'topic'], - [eg.stream, 'other topic'], + [eg.stream, 'topic', UserTopicVisibilityPolicy.Muted], + [eg.stream, 'other topic', UserTopicVisibilityPolicy.Unmuted], ]), ); }); + test('change Muted -> Unmuted', () => { + check( + makeMuteState([[eg.stream, 'topic', UserTopicVisibilityPolicy.Muted]]), + makeUserTopic(eg.stream, 'topic', UserTopicVisibilityPolicy.Unmuted), + makeMuteState([[eg.stream, 'topic', UserTopicVisibilityPolicy.Unmuted]]), + ); + }); + + test('change Unmuted -> Muted', () => { + check( + makeMuteState([[eg.stream, 'topic', UserTopicVisibilityPolicy.Unmuted]]), + makeUserTopic(eg.stream, 'topic', UserTopicVisibilityPolicy.Muted), + makeMuteState([[eg.stream, 'topic', UserTopicVisibilityPolicy.Muted]]), + ); + }); + test('remove, with others in stream', () => { check( makeMuteState([ diff --git a/src/mute/muteModel.js b/src/mute/muteModel.js index 3496c01f90f..ed972b478ce 100644 --- a/src/mute/muteModel.js +++ b/src/mute/muteModel.js @@ -1,7 +1,12 @@ /* @flow strict-local */ import Immutable from 'immutable'; -import type { MuteState, PerAccountApplicableAction, PerAccountState } from '../types'; +import type { + MuteState, + PerAccountApplicableAction, + PerAccountState, + Subscription, +} from '../types'; import { UserTopicVisibilityPolicy } from '../api/modelTypes'; import { EventTypes } from '../api/eventTypes'; import { @@ -40,7 +45,14 @@ export function getTopicVisibilityPolicy( * Whether this topic should appear when already focusing on its stream. * * This is false if the user's visibility policy for the topic is Muted, - * and true if the policy is None. + * and true if the policy is Unmuted or None. + * + * This function is appropriate for muting calculations in UI contexts that + * are already specific to a stream: for example the stream's unread count, + * or the message list in the stream's narrow. + * + * For UI contexts that are not specific to a particular stream, see + * `isTopicVisible`. */ export function isTopicVisibleInStream(streamId: number, topic: string, mute: MuteState): boolean { const policy = getTopicVisibilityPolicy(mute, streamId, topic); @@ -49,6 +61,35 @@ export function isTopicVisibleInStream(streamId: number, topic: string, mute: Mu return true; case UserTopicVisibilityPolicy.Muted: return false; + case UserTopicVisibilityPolicy.Unmuted: + return true; + } +} + +/** + * Whether this topic should appear when not specifically focusing on this stream. + * + * This takes into account the user's visibility policy for the stream + * overall, as well as their policy for this topic. + * + * For UI contexts that are specific to a particular stream, see + * `isTopicVisibleInStream`. + */ +export function isTopicVisible( + streamId: number, + topic: string, + subscription: Subscription, + mute: MuteState, +): boolean { + switch (getTopicVisibilityPolicy(mute, streamId, topic)) { + case UserTopicVisibilityPolicy.None: { + const streamMuted = !subscription.in_home_view; + return !streamMuted; + } + case UserTopicVisibilityPolicy.Muted: + return false; + case UserTopicVisibilityPolicy.Unmuted: + return true; } } diff --git a/src/topics/topicSelectors.js b/src/topics/topicSelectors.js index 1fd6fe0a4f7..7e35c0aa515 100644 --- a/src/topics/topicSelectors.js +++ b/src/topics/topicSelectors.js @@ -6,7 +6,7 @@ import { getTopics } from '../directSelectors'; import { getUnread, getUnreadCountForTopic } from '../unread/unreadModel'; import { NULL_ARRAY } from '../nullObjects'; import { isStreamNarrow, streamIdOfNarrow } from '../utils/narrow'; -import { getMute, isTopicVisibleInStream } from '../mute/muteModel'; +import { getMute, isTopicVisible } from '../mute/muteModel'; import { getSubscriptionsById } from '../subscriptions/subscriptionSelectors'; export const getTopicsForNarrow: Selector<$ReadOnlyArray, Narrow> = createSelector( @@ -36,13 +36,14 @@ export const getTopicsForStream: Selector, number return undefined; } - // If we're looking at a stream the user isn't subscribed to, then - // they won't see unreads from it even if they somehow have - // individual topics set to unmuted. So effectively it's all muted. - const streamMuted = subscription ? !subscription.in_home_view : true; - return topicList.map(({ name, max_id }): TopicExtended => { - const isMuted = streamMuted || !isTopicVisibleInStream(streamId, name, mute); + // prettier-ignore + const isMuted = subscription + ? !isTopicVisible(streamId, name, subscription, mute) + // If we're looking at a stream the user isn't subscribed to, then + // they won't see unreads from it even if they somehow have + // individual topics set to unmuted. So effectively it's all muted. + : true; const unreadCount = getUnreadCountForTopic(unread, streamId, name); return { name, max_id, isMuted, unreadCount }; }); diff --git a/src/unread/__tests__/unreadSelectors-test.js b/src/unread/__tests__/unreadSelectors-test.js index 87e57a6a909..e572703dec8 100644 --- a/src/unread/__tests__/unreadSelectors-test.js +++ b/src/unread/__tests__/unreadSelectors-test.js @@ -21,6 +21,7 @@ import { stream2, } from './unread-testlib'; import { makeMuteState } from '../../mute/__tests__/mute-testlib'; +import { UserTopicVisibilityPolicy } from '../../api/modelTypes'; const subscription0 = eg.makeSubscription({ stream: stream0, color: 'red' }); const subscription2 = eg.makeSubscription({ stream: stream2, color: 'blue' }); @@ -384,6 +385,38 @@ describe('getUnreadStreamsAndTopics', () => { expect(unreadCount).toEqual([]); }); + test('unmuted topics in muted streams are included', () => { + const state = eg.reduxStatePlus({ + subscriptions: [{ ...subscription0, in_home_view: false }], + unread: unreadState, + mute: makeMuteState([[stream0, 'a topic', UserTopicVisibilityPolicy.Unmuted]]), + }); + + const unreadCount = getUnreadStreamsAndTopics(state); + + expect(unreadCount).toEqual([ + { + color: 'red', + data: [ + { + key: 'a topic', + topic: 'a topic', + unread: 3, + lastUnreadMsgId: 3, + isMentioned: true, + }, + ], + isPinned: false, + isPrivate: false, + isWebPublic: false, + key: 'stream:0', + streamId: 0, + streamName: 'stream 0', + unread: 3, + }, + ]); + }); + test('muted topics inside non muted streams are not included', () => { const state = eg.reduxStatePlus({ subscriptions: [subscription0], diff --git a/src/unread/unreadSelectors.js b/src/unread/unreadSelectors.js index eceaac9b523..d283c5f93cd 100644 --- a/src/unread/unreadSelectors.js +++ b/src/unread/unreadSelectors.js @@ -4,7 +4,7 @@ import { createSelector } from 'reselect'; import type { Narrow, Selector } from '../types'; import type { UnreadStreamItem } from './UnreadCards'; import { caseInsensitiveCompareFunc } from '../utils/misc'; -import { getMute, isTopicVisibleInStream } from '../mute/muteModel'; +import { getMute, isTopicVisibleInStream, isTopicVisible } from '../mute/muteModel'; import { getOwnUserId } from '../users/userSelectors'; import { getSubscriptionsById, getStreamsById } from '../subscriptions/subscriptionSelectors'; import { caseNarrow } from '../utils/narrow'; @@ -158,16 +158,12 @@ export const getUnreadStreamsAndTopics: Selector<$ReadOnlyArray