mute: Recognize unmuted topics in muted streams

gnprice · gnprice · commit 7e7f0a80e7d7 · 2023-04-27T21:02:07.000-07:00
Fixes: #5362
diff --git 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
@@ -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
@@ -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
@@ -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<Message | Outbox
             const sub = subscriptions.get(message.stream_id);
             if (!sub) {
               // If there's no matching subscription, then the user must have
-              // unsubscribed from the stream since the message was received.  Leave
-              // those messages out of this view, just like for a muted stream.
+              // unsubscribed from the stream since the message was received.
+              // Leave those messages out of this view, just as we would if
+              // the user had muted the stream (without unmuting topics).
               return false;
             }
-            return (
-              sub.in_home_view
-              && isTopicVisibleInStream(message.stream_id, message.subject, mute)
-            );
+            return isTopicVisible(message.stream_id, message.subject, sub, mute);
           }),
 
         stream: _ =>
diff --git 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
@@ -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,29 +62,81 @@ 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);
+    });
   });
 });
 
 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],
+        ]),
       );
     });
 
     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)],
+        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
@@ -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
@@ -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<string>, Narrow> = createSelector(
@@ -36,13 +36,14 @@ export const getTopicsForStream: Selector<?$ReadOnlyArray<TopicExtended>, 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
diff --git a/src/unread/unreadSelectors.js b/src/unread/unreadSelectors.js

Original file line number	Diff line number	Diff line change
`@@ -653,6 +653,7 @@ export const constructTopicActionButtons = (args: {\|`
`653`	`653`	`buttons.push(unmuteTopic);`
`654`	`654`	`break;`
`655`	`655`	`case UserTopicVisibilityPolicy.None:`
	`656`	`+ case UserTopicVisibilityPolicy.Unmuted:`
`656`	`657`	`buttons.push(muteTopic);`
`657`	`658`	`break;`
`658`	`659`	`}`