From e630dd06eeaa9316134dfecb8fa0c2fdb8550119 Mon Sep 17 00:00:00 2001 From: Akash Date: Sat, 10 Apr 2021 05:09:06 +0530 Subject: [PATCH] ActionSheet [nfc]: Modify code to use stream_id instead of stream name. This commit removes stream name as a parameter to functions related to `TopicActionSheet` and instead introduces streamId in its place. Stream ID is a better parameter since it always remains constant. Related: #3918 --- .../__tests__/messageActionSheet-test.js | 39 +++++--- src/message/messageActionSheet.js | 91 ++++++++----------- src/streams/TopicItem.js | 10 +- src/title/TitleStream.js | 2 +- src/webview/handleOutboundEvents.js | 10 +- 5 files changed, 80 insertions(+), 72 deletions(-) diff --git a/src/message/__tests__/messageActionSheet-test.js b/src/message/__tests__/messageActionSheet-test.js index f5fbaa89996..3ea874492dc 100644 --- a/src/message/__tests__/messageActionSheet-test.js +++ b/src/message/__tests__/messageActionSheet-test.js @@ -1,7 +1,6 @@ // @flow strict-local import deepFreeze from 'deep-freeze'; import { HOME_NARROW } from '../../utils/narrow'; -import { streamNameOfStreamMessage } from '../../utils/recipient'; import * as eg from '../../__tests__/lib/exampleData'; import { constructMessageActionButtons, constructTopicActionButtons } from '../messageActionSheet'; @@ -66,7 +65,7 @@ describe('constructTopicActionButtons', () => { ]); const buttons = constructTopicActionButtons({ backgroundData: { ...baseBackgroundData, subscriptions, unreadStreams }, - stream: 'test stream', + streamId: stream.stream_id, topic: 'test topic', }); expect(buttonTitles(buttons)).toContain('Mark topic as read'); @@ -80,7 +79,7 @@ describe('constructTopicActionButtons', () => { ]); const buttons = constructTopicActionButtons({ backgroundData: { ...baseBackgroundData, subscriptions, unreadStreams }, - stream: 'test stream', + streamId: stream.stream_id, topic: 'read topic', }); expect(buttonTitles(buttons)).not.toContain('Mark topic as read'); @@ -88,57 +87,67 @@ describe('constructTopicActionButtons', () => { test('show Unmute topic option if topic is muted', () => { const mute = deepFreeze([['electron issues', 'issue #556']]); + const stream = eg.makeStream({ name: 'electron issues' }); + const subscriptions = [{ ...eg.subscription, ...stream }]; const buttons = constructTopicActionButtons({ - backgroundData: { ...baseBackgroundData, mute }, - stream: 'electron issues', + backgroundData: { ...baseBackgroundData, subscriptions, mute }, + streamId: stream.stream_id, topic: 'issue #556', }); expect(buttonTitles(buttons)).toContain('Unmute topic'); }); test('show mute topic option if topic is not muted', () => { + const stream = eg.makeStream(); + const subscriptions = [{ ...eg.subscription, ...stream }]; const buttons = constructTopicActionButtons({ - backgroundData: { ...baseBackgroundData, mute: [] }, - stream: streamNameOfStreamMessage(eg.streamMessage()), + backgroundData: { ...baseBackgroundData, subscriptions, mute: [] }, + streamId: stream.stream_id, topic: eg.streamMessage().subject, }); expect(buttonTitles(buttons)).toContain('Mute topic'); }); test('show Unmute stream option if stream is not in home view', () => { - const subscriptions = [{ ...eg.subscription, in_home_view: false }]; + const stream = eg.makeStream(); + const subscriptions = [{ ...eg.subscription, in_home_view: false, ...stream }]; const buttons = constructTopicActionButtons({ backgroundData: { ...baseBackgroundData, subscriptions }, - stream: streamNameOfStreamMessage(eg.streamMessage()), + streamId: stream.stream_id, topic: eg.streamMessage().subject, }); expect(buttonTitles(buttons)).toContain('Unmute stream'); }); test('show mute stream option if stream is in home view', () => { - const subscriptions = [{ ...eg.subscription, in_home_view: true }]; + const stream = eg.makeStream(); + const subscriptions = [{ ...eg.subscription, in_home_view: true, ...stream }]; const buttons = constructTopicActionButtons({ backgroundData: { ...baseBackgroundData, subscriptions }, - stream: streamNameOfStreamMessage(eg.streamMessage()), + streamId: stream.stream_id, topic: eg.streamMessage().subject, }); expect(buttonTitles(buttons)).toContain('Mute stream'); }); test('show delete topic option if current user is an admin', () => { + const stream = eg.makeStream(); + const subscriptions = [{ ...eg.subscription, ...stream }]; const ownUser = { ...eg.selfUser, is_admin: true }; const buttons = constructTopicActionButtons({ - backgroundData: { ...baseBackgroundData, ownUser }, - stream: streamNameOfStreamMessage(eg.streamMessage()), + backgroundData: { ...baseBackgroundData, ownUser, subscriptions }, + streamId: stream.stream_id, topic: eg.streamMessage().subject, }); expect(buttonTitles(buttons)).toContain('Delete topic'); }); test('do not show delete topic option if current user is not an admin', () => { + const stream = eg.makeStream(); + const subscriptions = [{ ...eg.subscription, ...stream }]; const buttons = constructTopicActionButtons({ - backgroundData: baseBackgroundData, - stream: streamNameOfStreamMessage(eg.streamMessage()), + backgroundData: { ...baseBackgroundData, subscriptions }, + streamId: stream.stream_id, topic: eg.streamMessage().subject, }); expect(buttonTitles(buttons)).not.toContain('Delete topic'); diff --git a/src/message/messageActionSheet.js b/src/message/messageActionSheet.js index bb8df3c0edd..293d4b28c26 100644 --- a/src/message/messageActionSheet.js +++ b/src/message/messageActionSheet.js @@ -38,8 +38,8 @@ export type ShowActionSheetWithOptions = ( type TopicArgs = { auth: Auth, - stream: string, topic: string, + streamId: number, subscriptions: Subscription[], dispatch: Dispatch, _: GetText, @@ -119,34 +119,25 @@ const deleteMessage = async ({ auth, message, dispatch }) => { deleteMessage.title = 'Delete message'; deleteMessage.errorMessage = 'Failed to delete message'; -const markTopicAsRead = async ({ auth, stream, topic, subscriptions }) => { - const sub = subscriptions.find(x => x.name === stream); - if (sub) { - await api.markTopicAsRead(auth, sub.stream_id, topic); - } +const markTopicAsRead = async ({ auth, streamId, topic }) => { + await api.markTopicAsRead(auth, streamId, topic); }; markTopicAsRead.title = 'Mark topic as read'; markTopicAsRead.errorMessage = 'Failed to mark topic as read'; -const unmuteTopic = async ({ auth, stream, topic, subscriptions }) => { - const sub = subscriptions.find(x => x.name === stream); - if (sub) { - await api.setTopicMute(auth, sub.stream_id, topic, false); - } +const unmuteTopic = async ({ auth, streamId, topic, subscriptions }) => { + await api.setTopicMute(auth, streamId, topic, false); }; unmuteTopic.title = 'Unmute topic'; unmuteTopic.errorMessage = 'Failed to unmute topic'; -const muteTopic = async ({ auth, stream, topic, subscriptions }) => { - const sub = subscriptions.find(x => x.name === stream); - if (sub) { - await api.setTopicMute(auth, sub.stream_id, topic, true); - } +const muteTopic = async ({ auth, streamId, topic }) => { + await api.setTopicMute(auth, streamId, topic, true); }; muteTopic.title = 'Mute topic'; muteTopic.errorMessage = 'Failed to mute topic'; -const deleteTopic = async ({ auth, stream, topic, subscriptions, dispatch, _ }) => { +const deleteTopic = async ({ auth, streamId, topic, dispatch, _ }) => { const alertTitle = _('Are you sure you want to delete the topic “{topic}”?', { topic }); const AsyncAlert = async (): Promise => new Promise((resolve, reject) => { @@ -173,38 +164,26 @@ const deleteTopic = async ({ auth, stream, topic, subscriptions, dispatch, _ }) ); }); if (await AsyncAlert()) { - const sub = subscriptions.find(x => x.name === stream); - if (sub) { - await dispatch(deleteMessagesForTopic(sub.stream_id, topic)); - } + await dispatch(deleteMessagesForTopic(streamId, topic)); } }; deleteTopic.title = 'Delete topic'; deleteTopic.errorMessage = 'Failed to delete topic'; -const unmuteStream = async ({ auth, stream, subscriptions }) => { - const sub = subscriptions.find(x => x.name === stream); - if (sub) { - await api.setSubscriptionProperty(auth, sub.stream_id, 'is_muted', false); - } +const unmuteStream = async ({ auth, streamId }) => { + await api.setSubscriptionProperty(auth, streamId, 'is_muted', false); }; unmuteStream.title = 'Unmute stream'; unmuteStream.errorMessage = 'Failed to unmute stream'; -const muteStream = async ({ auth, stream, subscriptions }) => { - const sub = subscriptions.find(x => x.name === stream); - if (sub) { - await api.setSubscriptionProperty(auth, sub.stream_id, 'is_muted', true); - } +const muteStream = async ({ auth, streamId }) => { + await api.setSubscriptionProperty(auth, streamId, 'is_muted', true); }; muteStream.title = 'Mute stream'; muteStream.errorMessage = 'Failed to mute stream'; -const showStreamSettings = ({ stream, subscriptions }) => { - const sub = subscriptions.find(x => x.name === stream); - if (sub) { - NavigationService.dispatch(navigateToStream(sub.stream_id)); - } +const showStreamSettings = ({ streamId }) => { + NavigationService.dispatch(navigateToStream(streamId)); }; showStreamSettings.title = 'Stream settings'; showStreamSettings.errorMessage = 'Failed to show stream settings'; @@ -247,8 +226,8 @@ cancel.errorMessage = 'Failed to hide menu'; export const constructTopicActionButtons = ({ backgroundData: { mute, subscriptions, ownUser, unreadStreams }, - stream, topic, + streamId, }: {| backgroundData: $ReadOnly<{ mute: MuteState, @@ -257,29 +236,27 @@ export const constructTopicActionButtons = ({ unreadStreams: UnreadStreamsState, ... }>, - stream: string, + streamId: number, topic: string, |}): Button[] => { + const sub = subscriptions.find(x => x.stream_id === streamId); const buttons = []; if (ownUser.is_admin) { buttons.push(deleteTopic); } - if (isTopicMuted(stream, topic, mute)) { + if (sub && isTopicMuted(sub.name, topic, mute)) { buttons.push(unmuteTopic); } else { buttons.push(muteTopic); } - const sub = subscriptions.find(x => x.name === stream); - if (sub) { - const unreadCount = unreadStreams.get(sub.stream_id)?.get(topic)?.size; - if (unreadCount !== undefined && unreadCount > 0) { - buttons.push(markTopicAsRead); - } - if (!sub.in_home_view) { - buttons.push(unmuteStream); - } else { - buttons.push(muteStream); - } + const unreadCount = unreadStreams.get(streamId)?.get(topic)?.size; + if (unreadCount !== undefined && unreadCount > 0) { + buttons.push(markTopicAsRead); + } + if (sub && !sub.in_home_view) { + buttons.push(unmuteStream); + } else { + buttons.push(muteStream); } buttons.push(showStreamSettings); buttons.push(cancel); @@ -420,7 +397,7 @@ export const showTopicActionSheet = ({ callbacks, backgroundData, topic, - stream, + streamId, }: {| showActionSheetWithOptions: ShowActionSheetWithOptions, callbacks: {| @@ -436,12 +413,18 @@ export const showTopicActionSheet = ({ unreadStreams: UnreadStreamsState, ... }>, - stream: string, + streamId: number, topic: string, |}): void => { + const sub = backgroundData.subscriptions.find(x => x.stream_id === streamId); + if (!sub) { + logging.error('Stream id does not exist.'); + return; + } + const stream = sub.name; const buttonList = constructTopicActionButtons({ backgroundData, - stream, + streamId, topic, }); showActionSheetWithOptions( @@ -453,7 +436,7 @@ export const showTopicActionSheet = ({ makeButtonCallback(buttonList, { ...backgroundData, ...callbacks, - stream, + streamId, topic, }), ); diff --git a/src/streams/TopicItem.js b/src/streams/TopicItem.js index 2766330279f..78d82e6cebf 100644 --- a/src/streams/TopicItem.js +++ b/src/streams/TopicItem.js @@ -11,6 +11,7 @@ import { TranslationContext } from '../boot/TranslationProvider'; import { useDispatch, useSelector } from '../react-redux'; import { getAuth, getMute, getFlags, getSubscriptions, getOwnUser } from '../selectors'; import { getUnreadStreams } from '../unread/unreadModel'; +import * as logging from '../utils/logging'; const componentStyles = createStyleSheet({ selectedRow: { @@ -52,15 +53,22 @@ export default function TopicItem(props: Props) { unreadStreams: getUnreadStreams(state), })); + const subscription = backgroundData.subscriptions.find(x => x.name === stream); + return ( onPress(stream, name)} onLongPress={() => { + if (!subscription) { + logging.error('Stream id does not exist.'); + return; + } + showTopicActionSheet({ showActionSheetWithOptions, callbacks: { dispatch, _ }, backgroundData, - stream, + streamId: subscription.stream_id, topic: name, }); }} diff --git a/src/title/TitleStream.js b/src/title/TitleStream.js index af51b9e84a4..70a45e969a1 100644 --- a/src/title/TitleStream.js +++ b/src/title/TitleStream.js @@ -80,7 +80,7 @@ const TitleStream = (props: Props) => { showActionSheetWithOptions, callbacks: { dispatch, _ }, backgroundData, - stream: stream.name, + streamId: stream.stream_id, topic: topicOfNarrow(narrow), }); } diff --git a/src/webview/handleOutboundEvents.js b/src/webview/handleOutboundEvents.js index afdedb89e9a..bef365a8ea9 100644 --- a/src/webview/handleOutboundEvents.js +++ b/src/webview/handleOutboundEvents.js @@ -214,11 +214,19 @@ const handleLongPress = ( const { dispatch, showActionSheetWithOptions, backgroundData, narrow, startEditMessage } = props; if (target === 'header') { if (message.type === 'stream') { + const stream = streamNameOfStreamMessage(message); + const subscription = backgroundData.subscriptions.find(x => x.name === stream); + + if (!subscription) { + logging.error('Could not find subscription with given stream name.'); + return; + } + showTopicActionSheet({ showActionSheetWithOptions, callbacks: { dispatch, _ }, backgroundData, - stream: streamNameOfStreamMessage(message), + streamId: subscription.stream_id, topic: message.subject, }); } else if (message.type === 'private') {