From b305b3d475b87d5a42baa44faaa0f14684e51874 Mon Sep 17 00:00:00 2001 From: Akash Date: Sat, 10 Apr 2021 00:42:15 +0530 Subject: [PATCH 01/11] ActionSheet [nfc]: Rename HeaderActionSheet to TopicActionSheet. This is a better name since this type of action sheet is used with Topics. --- .../__tests__/messageActionSheet-test.js | 16 ++++++++-------- src/message/messageActionSheet.js | 17 +++++++---------- src/streams/TopicItem.js | 4 ++-- src/title/TitleStream.js | 4 ++-- src/webview/handleOutboundEvents.js | 4 ++-- 5 files changed, 21 insertions(+), 24 deletions(-) diff --git a/src/message/__tests__/messageActionSheet-test.js b/src/message/__tests__/messageActionSheet-test.js index 87970c4f754..ba4d7776050 100644 --- a/src/message/__tests__/messageActionSheet-test.js +++ b/src/message/__tests__/messageActionSheet-test.js @@ -4,7 +4,7 @@ import { HOME_NARROW } from '../../utils/narrow'; import { streamNameOfStreamMessage } from '../../utils/recipient'; import * as eg from '../../__tests__/lib/exampleData'; -import { constructMessageActionButtons, constructHeaderActionButtons } from '../messageActionSheet'; +import { constructMessageActionButtons, constructTopicActionButtons } from '../messageActionSheet'; const buttonTitles = buttons => buttons.map(button => button.title); @@ -43,10 +43,10 @@ describe('constructActionButtons', () => { }); }); -describe('constructHeaderActionButtons', () => { +describe('constructTopicActionButtons', () => { test('show Unmute topic option if topic is muted', () => { const mute = deepFreeze([['electron issues', 'issue #556']]); - const buttons = constructHeaderActionButtons({ + const buttons = constructTopicActionButtons({ backgroundData: { ...eg.backgroundData, mute }, stream: 'electron issues', topic: 'issue #556', @@ -55,7 +55,7 @@ describe('constructHeaderActionButtons', () => { }); test('show mute topic option if topic is not muted', () => { - const buttons = constructHeaderActionButtons({ + const buttons = constructTopicActionButtons({ backgroundData: { ...eg.backgroundData, mute: [] }, stream: streamNameOfStreamMessage(eg.streamMessage()), topic: eg.streamMessage().subject, @@ -65,7 +65,7 @@ describe('constructHeaderActionButtons', () => { test('show Unmute stream option if stream is not in home view', () => { const subscriptions = [{ ...eg.subscription, in_home_view: false }]; - const buttons = constructHeaderActionButtons({ + const buttons = constructTopicActionButtons({ backgroundData: { ...eg.backgroundData, subscriptions }, stream: streamNameOfStreamMessage(eg.streamMessage()), topic: eg.streamMessage().subject, @@ -75,7 +75,7 @@ describe('constructHeaderActionButtons', () => { test('show mute stream option if stream is in home view', () => { const subscriptions = [{ ...eg.subscription, in_home_view: true }]; - const buttons = constructHeaderActionButtons({ + const buttons = constructTopicActionButtons({ backgroundData: { ...eg.backgroundData, subscriptions }, stream: streamNameOfStreamMessage(eg.streamMessage()), topic: eg.streamMessage().subject, @@ -85,7 +85,7 @@ describe('constructHeaderActionButtons', () => { test('show delete topic option if current user is an admin', () => { const ownUser = { ...eg.selfUser, is_admin: true }; - const buttons = constructHeaderActionButtons({ + const buttons = constructTopicActionButtons({ backgroundData: { ...eg.backgroundData, ownUser }, stream: streamNameOfStreamMessage(eg.streamMessage()), topic: eg.streamMessage().subject, @@ -94,7 +94,7 @@ describe('constructHeaderActionButtons', () => { }); test('do not show delete topic option if current user is not an admin', () => { - const buttons = constructHeaderActionButtons({ + const buttons = constructTopicActionButtons({ backgroundData: eg.backgroundData, stream: streamNameOfStreamMessage(eg.streamMessage()), topic: eg.streamMessage().subject, diff --git a/src/message/messageActionSheet.js b/src/message/messageActionSheet.js index 7c2e508d7bd..e79c1d654e7 100644 --- a/src/message/messageActionSheet.js +++ b/src/message/messageActionSheet.js @@ -35,7 +35,7 @@ export type ShowActionSheetWithOptions = ( (number) => void, ) => void; -type HeaderArgs = { +type TopicArgs = { auth: Auth, stream: string, topic: string, @@ -55,7 +55,7 @@ type MessageArgs = { ... }; -type Button = {| +type Button = {| (Args): void | Promise, /** The label for the button. */ @@ -226,7 +226,7 @@ const cancel = params => {}; cancel.title = 'Cancel'; cancel.errorMessage = 'Failed to hide menu'; -export const constructHeaderActionButtons = ({ +export const constructTopicActionButtons = ({ backgroundData: { mute, subscriptions, ownUser }, stream, topic, @@ -239,7 +239,7 @@ export const constructHeaderActionButtons = ({ }>, stream: string, topic: string, -|}): Button[] => { +|}): Button[] => { const buttons = []; if (ownUser.is_admin) { buttons.push(deleteTopic); @@ -338,10 +338,7 @@ export const constructNonHeaderActionButtons = ({ } }; -function makeButtonCallback( - buttonList: Button[], - args: Args, -) { +function makeButtonCallback(buttonList: Button[], args: Args) { return buttonIndex => { (async () => { const pressedButton: Button = buttonList[buttonIndex]; @@ -392,7 +389,7 @@ export const showMessageActionSheet = ({ ); }; -export const showHeaderActionSheet = ({ +export const showTopicActionSheet = ({ showActionSheetWithOptions, callbacks, backgroundData, @@ -415,7 +412,7 @@ export const showHeaderActionSheet = ({ stream: string, topic: string, |}): void => { - const buttonList = constructHeaderActionButtons({ + const buttonList = constructTopicActionButtons({ backgroundData, stream, topic, diff --git a/src/streams/TopicItem.js b/src/streams/TopicItem.js index 69f2dc83ddf..166f8b977fb 100644 --- a/src/streams/TopicItem.js +++ b/src/streams/TopicItem.js @@ -6,7 +6,7 @@ import { useActionSheet } from '@expo/react-native-action-sheet'; import styles, { BRAND_COLOR, createStyleSheet } from '../styles'; import { RawLabel, Touchable, UnreadCount } from '../common'; -import { showHeaderActionSheet } from '../message/messageActionSheet'; +import { showTopicActionSheet } from '../message/messageActionSheet'; import type { ShowActionSheetWithOptions } from '../message/messageActionSheet'; import { TranslationContext } from '../boot/TranslationProvider'; import { useDispatch, useSelector } from '../react-redux'; @@ -55,7 +55,7 @@ export default function TopicItem(props: Props) { onPress(stream, name)} onLongPress={() => { - showHeaderActionSheet({ + showTopicActionSheet({ showActionSheetWithOptions, callbacks: { dispatch, _ }, backgroundData, diff --git a/src/title/TitleStream.js b/src/title/TitleStream.js index aa9204bd165..f7a96a7dc48 100644 --- a/src/title/TitleStream.js +++ b/src/title/TitleStream.js @@ -28,7 +28,7 @@ import { getOwnUser, getStreamInNarrow, } from '../selectors'; -import { showHeaderActionSheet } from '../message/messageActionSheet'; +import { showTopicActionSheet } from '../message/messageActionSheet'; import type { ShowActionSheetWithOptions } from '../message/messageActionSheet'; type SelectorProps = {| @@ -74,7 +74,7 @@ const TitleStream = (props: Props) => { onLongPress={ isTopicNarrow(narrow) ? () => { - showHeaderActionSheet({ + showTopicActionSheet({ showActionSheetWithOptions, callbacks: { dispatch, _ }, backgroundData, diff --git a/src/webview/handleOutboundEvents.js b/src/webview/handleOutboundEvents.js index 5264ca7ba44..999a14aced6 100644 --- a/src/webview/handleOutboundEvents.js +++ b/src/webview/handleOutboundEvents.js @@ -23,7 +23,7 @@ import { navigateToLightbox, messageLinkPress, } from '../actions'; -import { showHeaderActionSheet, showMessageActionSheet } from '../message/messageActionSheet'; +import { showTopicActionSheet, showMessageActionSheet } from '../message/messageActionSheet'; import { ensureUnreachable } from '../types'; import { base64Utf8Decode } from '../utils/encoding'; @@ -212,7 +212,7 @@ const handleLongPress = ( const { dispatch, showActionSheetWithOptions, backgroundData, narrow, startEditMessage } = props; if (target === 'header') { if (message.type === 'stream') { - showHeaderActionSheet({ + showTopicActionSheet({ showActionSheetWithOptions, callbacks: { dispatch, _ }, backgroundData, From a4dedddbeb0d0e243522beb68d62dcd22aae75be Mon Sep 17 00:00:00 2001 From: Akash Date: Thu, 10 Jun 2021 16:17:54 +0530 Subject: [PATCH 02/11] ActionSheet [nfc]: Rename variable `stream` to `streamName`. `streamName` is a better name as it makes it more explicit that it contains the value of stream's name and not a `Stream` object. This name will also be consistent (from a readability point of view) with variables `streamId` and `streams` that will be introduced in subsequent commits. --- .../__tests__/messageActionSheet-test.js | 12 ++--- src/message/messageActionSheet.js | 44 +++++++++---------- src/streams/TopicItem.js | 8 ++-- src/title/TitleStream.js | 2 +- src/topics/TopicList.js | 2 +- src/unread/UnreadCards.js | 2 +- src/webview/handleOutboundEvents.js | 2 +- 7 files changed, 36 insertions(+), 36 deletions(-) diff --git a/src/message/__tests__/messageActionSheet-test.js b/src/message/__tests__/messageActionSheet-test.js index ba4d7776050..cc6632c0b20 100644 --- a/src/message/__tests__/messageActionSheet-test.js +++ b/src/message/__tests__/messageActionSheet-test.js @@ -48,7 +48,7 @@ describe('constructTopicActionButtons', () => { const mute = deepFreeze([['electron issues', 'issue #556']]); const buttons = constructTopicActionButtons({ backgroundData: { ...eg.backgroundData, mute }, - stream: 'electron issues', + streamName: 'electron issues', topic: 'issue #556', }); expect(buttonTitles(buttons)).toContain('Unmute topic'); @@ -57,7 +57,7 @@ describe('constructTopicActionButtons', () => { test('show mute topic option if topic is not muted', () => { const buttons = constructTopicActionButtons({ backgroundData: { ...eg.backgroundData, mute: [] }, - stream: streamNameOfStreamMessage(eg.streamMessage()), + streamName: streamNameOfStreamMessage(eg.streamMessage()), topic: eg.streamMessage().subject, }); expect(buttonTitles(buttons)).toContain('Mute topic'); @@ -67,7 +67,7 @@ describe('constructTopicActionButtons', () => { const subscriptions = [{ ...eg.subscription, in_home_view: false }]; const buttons = constructTopicActionButtons({ backgroundData: { ...eg.backgroundData, subscriptions }, - stream: streamNameOfStreamMessage(eg.streamMessage()), + streamName: streamNameOfStreamMessage(eg.streamMessage()), topic: eg.streamMessage().subject, }); expect(buttonTitles(buttons)).toContain('Unmute stream'); @@ -77,7 +77,7 @@ describe('constructTopicActionButtons', () => { const subscriptions = [{ ...eg.subscription, in_home_view: true }]; const buttons = constructTopicActionButtons({ backgroundData: { ...eg.backgroundData, subscriptions }, - stream: streamNameOfStreamMessage(eg.streamMessage()), + streamName: streamNameOfStreamMessage(eg.streamMessage()), topic: eg.streamMessage().subject, }); expect(buttonTitles(buttons)).toContain('Mute stream'); @@ -87,7 +87,7 @@ describe('constructTopicActionButtons', () => { const ownUser = { ...eg.selfUser, is_admin: true }; const buttons = constructTopicActionButtons({ backgroundData: { ...eg.backgroundData, ownUser }, - stream: streamNameOfStreamMessage(eg.streamMessage()), + streamName: streamNameOfStreamMessage(eg.streamMessage()), topic: eg.streamMessage().subject, }); expect(buttonTitles(buttons)).toContain('Delete topic'); @@ -96,7 +96,7 @@ describe('constructTopicActionButtons', () => { test('do not show delete topic option if current user is not an admin', () => { const buttons = constructTopicActionButtons({ backgroundData: eg.backgroundData, - stream: streamNameOfStreamMessage(eg.streamMessage()), + streamName: streamNameOfStreamMessage(eg.streamMessage()), topic: eg.streamMessage().subject, }); expect(buttonTitles(buttons)).not.toContain('Delete topic'); diff --git a/src/message/messageActionSheet.js b/src/message/messageActionSheet.js index e79c1d654e7..6c081def6aa 100644 --- a/src/message/messageActionSheet.js +++ b/src/message/messageActionSheet.js @@ -37,7 +37,7 @@ export type ShowActionSheetWithOptions = ( type TopicArgs = { auth: Auth, - stream: string, + streamName: string, topic: string, subscriptions: Subscription[], dispatch: Dispatch, @@ -118,19 +118,19 @@ const deleteMessage = async ({ auth, message, dispatch }) => { deleteMessage.title = 'Delete message'; deleteMessage.errorMessage = 'Failed to delete message'; -const unmuteTopic = async ({ auth, stream, topic }) => { - await api.setTopicMute(auth, stream, topic, false); +const unmuteTopic = async ({ auth, streamName, topic }) => { + await api.setTopicMute(auth, streamName, topic, false); }; unmuteTopic.title = 'Unmute topic'; unmuteTopic.errorMessage = 'Failed to unmute topic'; -const muteTopic = async ({ auth, stream, topic }) => { - await api.setTopicMute(auth, stream, topic, true); +const muteTopic = async ({ auth, streamName, topic }) => { + await api.setTopicMute(auth, streamName, topic, true); }; muteTopic.title = 'Mute topic'; muteTopic.errorMessage = 'Failed to mute topic'; -const deleteTopic = async ({ auth, stream, topic, dispatch, _ }) => { +const deleteTopic = async ({ auth, streamName, topic, dispatch, _ }) => { const alertTitle = _('Are you sure you want to delete the topic “{topic}”?', { topic }); const AsyncAlert = async (): Promise => new Promise((resolve, reject) => { @@ -157,14 +157,14 @@ const deleteTopic = async ({ auth, stream, topic, dispatch, _ }) => { ); }); if (await AsyncAlert()) { - await dispatch(deleteMessagesForTopic(stream, topic)); + await dispatch(deleteMessagesForTopic(streamName, 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); +const unmuteStream = async ({ auth, streamName, subscriptions }) => { + const sub = subscriptions.find(x => x.name === streamName); if (sub) { await api.setSubscriptionProperty(auth, sub.stream_id, 'is_muted', false); } @@ -172,8 +172,8 @@ const unmuteStream = async ({ auth, stream, subscriptions }) => { unmuteStream.title = 'Unmute stream'; unmuteStream.errorMessage = 'Failed to unmute stream'; -const muteStream = async ({ auth, stream, subscriptions }) => { - const sub = subscriptions.find(x => x.name === stream); +const muteStream = async ({ auth, streamName, subscriptions }) => { + const sub = subscriptions.find(x => x.name === streamName); if (sub) { await api.setSubscriptionProperty(auth, sub.stream_id, 'is_muted', true); } @@ -181,8 +181,8 @@ const muteStream = async ({ auth, stream, subscriptions }) => { muteStream.title = 'Mute stream'; muteStream.errorMessage = 'Failed to mute stream'; -const showStreamSettings = ({ stream, subscriptions }) => { - const sub = subscriptions.find(x => x.name === stream); +const showStreamSettings = ({ streamName, subscriptions }) => { + const sub = subscriptions.find(x => x.name === streamName); if (sub) { NavigationService.dispatch(navigateToStream(sub.stream_id)); } @@ -228,7 +228,7 @@ cancel.errorMessage = 'Failed to hide menu'; export const constructTopicActionButtons = ({ backgroundData: { mute, subscriptions, ownUser }, - stream, + streamName, topic, }: {| backgroundData: $ReadOnly<{ @@ -237,19 +237,19 @@ export const constructTopicActionButtons = ({ ownUser: User, ... }>, - stream: string, + streamName: string, topic: string, |}): Button[] => { const buttons = []; if (ownUser.is_admin) { buttons.push(deleteTopic); } - if (isTopicMuted(stream, topic, mute)) { + if (isTopicMuted(streamName, topic, mute)) { buttons.push(unmuteTopic); } else { buttons.push(muteTopic); } - const sub = subscriptions.find(x => x.name === stream); + const sub = subscriptions.find(x => x.name === streamName); if (sub && !sub.in_home_view) { buttons.push(unmuteStream); } else { @@ -394,7 +394,7 @@ export const showTopicActionSheet = ({ callbacks, backgroundData, topic, - stream, + streamName, }: {| showActionSheetWithOptions: ShowActionSheetWithOptions, callbacks: {| @@ -409,24 +409,24 @@ export const showTopicActionSheet = ({ flags: FlagsState, ... }>, - stream: string, + streamName: string, topic: string, |}): void => { const buttonList = constructTopicActionButtons({ backgroundData, - stream, + streamName, topic, }); showActionSheetWithOptions( { - title: `#${stream} > ${topic}`, + title: `#${streamName} > ${topic}`, options: buttonList.map(button => callbacks._(button.title)), cancelButtonIndex: buttonList.length - 1, }, makeButtonCallback(buttonList, { ...backgroundData, ...callbacks, - stream, + streamName, topic, }), ); diff --git a/src/streams/TopicItem.js b/src/streams/TopicItem.js index 166f8b977fb..103f2fe4f80 100644 --- a/src/streams/TopicItem.js +++ b/src/streams/TopicItem.js @@ -28,7 +28,7 @@ const componentStyles = createStyleSheet({ }); type Props = $ReadOnly<{| - stream: string, + streamName: string, name: string, isMuted?: boolean, isSelected?: boolean, @@ -37,7 +37,7 @@ type Props = $ReadOnly<{| |}>; export default function TopicItem(props: Props) { - const { name, stream, isMuted = false, isSelected = false, unreadCount = 0, onPress } = props; + const { name, streamName, isMuted = false, isSelected = false, unreadCount = 0, onPress } = props; const showActionSheetWithOptions: ShowActionSheetWithOptions = useActionSheet() .showActionSheetWithOptions; @@ -53,13 +53,13 @@ export default function TopicItem(props: Props) { return ( onPress(stream, name)} + onPress={() => onPress(streamName, name)} onLongPress={() => { showTopicActionSheet({ showActionSheetWithOptions, callbacks: { dispatch, _ }, backgroundData, - stream, + streamName, topic: name, }); }} diff --git a/src/title/TitleStream.js b/src/title/TitleStream.js index f7a96a7dc48..b691fcee5c3 100644 --- a/src/title/TitleStream.js +++ b/src/title/TitleStream.js @@ -78,7 +78,7 @@ const TitleStream = (props: Props) => { showActionSheetWithOptions, callbacks: { dispatch, _ }, backgroundData, - stream: stream.name, + streamName: stream.name, topic: topicOfNarrow(narrow), }); } diff --git a/src/topics/TopicList.js b/src/topics/TopicList.js index 733610da13d..d7750974a33 100644 --- a/src/topics/TopicList.js +++ b/src/topics/TopicList.js @@ -41,7 +41,7 @@ export default class TopicList extends PureComponent { renderItem={({ item }) => ( Date: Thu, 10 Jun 2021 17:15:52 +0530 Subject: [PATCH 03/11] ActionSheet [nfc]: Pass `StreamsById` to actionsheet. While not directly used within the action sheet, this modification will help us to create TopicActionSheet with `streamId` for any `streamName` at the callsite of `showTopicActionSheet`. --- src/__tests__/lib/exampleData.js | 2 ++ src/message/messageActionSheet.js | 5 ++++- src/streams/TopicItem.js | 10 +++++++++- src/title/TitleStream.js | 3 +++ src/webview/MessageList.js | 4 ++++ 5 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index eb52da4c7c8..d80b98911e5 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -39,6 +39,7 @@ import rootReducer from '../../boot/reducers'; import { authOfAccount } from '../../account/accountMisc'; import { HOME_NARROW } from '../../utils/narrow'; import type { BackgroundData } from '../../webview/MessageList'; +import { getStreamsById } from '../../selectors'; /* ======================================================================== * Utilities @@ -755,6 +756,7 @@ export const backgroundData: BackgroundData = deepFreeze({ mute: [], mutedUsers: Immutable.Map(), ownUser: selfUser, + streams: getStreamsById(baseReduxState), subscriptions: [], theme: 'default', twentyFourHourTime: false, diff --git a/src/message/messageActionSheet.js b/src/message/messageActionSheet.js index 6c081def6aa..4cde9abaa2c 100644 --- a/src/message/messageActionSheet.js +++ b/src/message/messageActionSheet.js @@ -14,6 +14,7 @@ import type { Subscription, User, EditMessage, + Stream, } from '../types'; import { getNarrowForReply, @@ -227,12 +228,13 @@ cancel.title = 'Cancel'; cancel.errorMessage = 'Failed to hide menu'; export const constructTopicActionButtons = ({ - backgroundData: { mute, subscriptions, ownUser }, + backgroundData: { mute, ownUser, streams, subscriptions }, streamName, topic, }: {| backgroundData: $ReadOnly<{ mute: MuteState, + streams: Map, subscriptions: Subscription[], ownUser: User, ... @@ -404,6 +406,7 @@ export const showTopicActionSheet = ({ backgroundData: $ReadOnly<{ auth: Auth, mute: MuteState, + streams: Map, subscriptions: Subscription[], ownUser: User, flags: FlagsState, diff --git a/src/streams/TopicItem.js b/src/streams/TopicItem.js index 103f2fe4f80..458de8eab17 100644 --- a/src/streams/TopicItem.js +++ b/src/streams/TopicItem.js @@ -10,7 +10,14 @@ import { showTopicActionSheet } from '../message/messageActionSheet'; import type { ShowActionSheetWithOptions } from '../message/messageActionSheet'; import { TranslationContext } from '../boot/TranslationProvider'; import { useDispatch, useSelector } from '../react-redux'; -import { getAuth, getMute, getFlags, getSubscriptions, getOwnUser } from '../selectors'; +import { + getAuth, + getMute, + getFlags, + getSubscriptions, + getStreamsById, + getOwnUser, +} from '../selectors'; const componentStyles = createStyleSheet({ selectedRow: { @@ -46,6 +53,7 @@ export default function TopicItem(props: Props) { const backgroundData = useSelector(state => ({ auth: getAuth(state), mute: getMute(state), + streams: getStreamsById(state), subscriptions: getSubscriptions(state), ownUser: getOwnUser(state), flags: getFlags(state), diff --git a/src/title/TitleStream.js b/src/title/TitleStream.js index b691fcee5c3..10192fa6d84 100644 --- a/src/title/TitleStream.js +++ b/src/title/TitleStream.js @@ -25,6 +25,7 @@ import { getMute, getFlags, getSubscriptions, + getStreamsById, getOwnUser, getStreamInNarrow, } from '../selectors'; @@ -36,6 +37,7 @@ type SelectorProps = {| backgroundData: {| auth: Auth, mute: MuteState, + streams: Map, subscriptions: Subscription[], ownUser: User, flags: FlagsState, @@ -113,6 +115,7 @@ export default connect((state, props) => ({ backgroundData: { auth: getAuth(state), mute: getMute(state), + streams: getStreamsById(state), subscriptions: getSubscriptions(state), ownUser: getOwnUser(state), flags: getFlags(state), diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index b0e99e4832c..1f900859bf7 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -21,6 +21,7 @@ import type { ImageEmojiType, HtmlPieceDescriptor, Subscription, + Stream, ThemeName, User, UserOrBot, @@ -41,6 +42,7 @@ import { getOwnUser, getSettings, getSubscriptions, + getStreamsById, getRealm, } from '../selectors'; import { withGetText } from '../boot/TranslationProvider'; @@ -79,6 +81,7 @@ export type BackgroundData = $ReadOnly<{| mute: MuteState, mutedUsers: MutedUsersState, ownUser: User, + streams: Map, subscriptions: Subscription[], theme: ThemeName, twentyFourHourTime: boolean, @@ -307,6 +310,7 @@ const MessageList: ComponentType = connect( mute: getMute(state), mutedUsers: getMutedUsers(state), ownUser: getOwnUser(state), + streams: getStreamsById(state), subscriptions: getSubscriptions(state), theme: getSettings(state).theme, twentyFourHourTime: getRealm(state).twentyFourHourTime, From bf07e663470caf05660797cd541a63273ba24431 Mon Sep 17 00:00:00 2001 From: Akash Date: Tue, 15 Jun 2021 19:53:30 +0530 Subject: [PATCH 04/11] ActionSheet [nfc]: Parameterize some values used in `tests`. This is better since most of these things are / can-be reused in the test suite of concern, and it is easy to change their values should we choose to do so. --- .../__tests__/messageActionSheet-test.js | 30 +++++++++++-------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/src/message/__tests__/messageActionSheet-test.js b/src/message/__tests__/messageActionSheet-test.js index cc6632c0b20..8227d461da1 100644 --- a/src/message/__tests__/messageActionSheet-test.js +++ b/src/message/__tests__/messageActionSheet-test.js @@ -44,12 +44,16 @@ describe('constructActionButtons', () => { }); describe('constructTopicActionButtons', () => { + const streamMessage = eg.streamMessage(); + const streamName = streamNameOfStreamMessage(streamMessage); + const topic = streamMessage.subject; + test('show Unmute topic option if topic is muted', () => { - const mute = deepFreeze([['electron issues', 'issue #556']]); + const mute = deepFreeze([[streamName, topic]]); const buttons = constructTopicActionButtons({ backgroundData: { ...eg.backgroundData, mute }, - streamName: 'electron issues', - topic: 'issue #556', + streamName, + topic, }); expect(buttonTitles(buttons)).toContain('Unmute topic'); }); @@ -57,8 +61,8 @@ describe('constructTopicActionButtons', () => { test('show mute topic option if topic is not muted', () => { const buttons = constructTopicActionButtons({ backgroundData: { ...eg.backgroundData, mute: [] }, - streamName: streamNameOfStreamMessage(eg.streamMessage()), - topic: eg.streamMessage().subject, + streamName, + topic, }); expect(buttonTitles(buttons)).toContain('Mute topic'); }); @@ -67,8 +71,8 @@ describe('constructTopicActionButtons', () => { const subscriptions = [{ ...eg.subscription, in_home_view: false }]; const buttons = constructTopicActionButtons({ backgroundData: { ...eg.backgroundData, subscriptions }, - streamName: streamNameOfStreamMessage(eg.streamMessage()), - topic: eg.streamMessage().subject, + streamName, + topic, }); expect(buttonTitles(buttons)).toContain('Unmute stream'); }); @@ -77,8 +81,8 @@ describe('constructTopicActionButtons', () => { const subscriptions = [{ ...eg.subscription, in_home_view: true }]; const buttons = constructTopicActionButtons({ backgroundData: { ...eg.backgroundData, subscriptions }, - streamName: streamNameOfStreamMessage(eg.streamMessage()), - topic: eg.streamMessage().subject, + streamName, + topic, }); expect(buttonTitles(buttons)).toContain('Mute stream'); }); @@ -87,8 +91,8 @@ describe('constructTopicActionButtons', () => { const ownUser = { ...eg.selfUser, is_admin: true }; const buttons = constructTopicActionButtons({ backgroundData: { ...eg.backgroundData, ownUser }, - streamName: streamNameOfStreamMessage(eg.streamMessage()), - topic: eg.streamMessage().subject, + streamName, + topic, }); expect(buttonTitles(buttons)).toContain('Delete topic'); }); @@ -96,8 +100,8 @@ describe('constructTopicActionButtons', () => { test('do not show delete topic option if current user is not an admin', () => { const buttons = constructTopicActionButtons({ backgroundData: eg.backgroundData, - streamName: streamNameOfStreamMessage(eg.streamMessage()), - topic: eg.streamMessage().subject, + streamName, + topic, }); expect(buttonTitles(buttons)).not.toContain('Delete topic'); }); From cdf194818840a96947515f5ee789f13f7d8bdf0c Mon Sep 17 00:00:00 2001 From: Akash Date: Thu, 1 Jul 2021 21:36:39 +0530 Subject: [PATCH 05/11] selector [nfc]: Introduce `getStreamsByName` and use it. Passed to background data of TopicItem and MessageList, to use in later commit. --- src/__tests__/lib/exampleData.js | 3 ++- src/streams/TopicItem.js | 2 ++ src/subscriptions/subscriptionSelectors.js | 5 +++++ src/webview/MessageList.js | 3 +++ 4 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index d80b98911e5..e17c9edf306 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -39,7 +39,7 @@ import rootReducer from '../../boot/reducers'; import { authOfAccount } from '../../account/accountMisc'; import { HOME_NARROW } from '../../utils/narrow'; import type { BackgroundData } from '../../webview/MessageList'; -import { getStreamsById } from '../../selectors'; +import { getStreamsById, getStreamsByName } from '../../selectors'; /* ======================================================================== * Utilities @@ -757,6 +757,7 @@ export const backgroundData: BackgroundData = deepFreeze({ mutedUsers: Immutable.Map(), ownUser: selfUser, streams: getStreamsById(baseReduxState), + streamsByName: getStreamsByName(baseReduxState), subscriptions: [], theme: 'default', twentyFourHourTime: false, diff --git a/src/streams/TopicItem.js b/src/streams/TopicItem.js index 458de8eab17..11e0634be57 100644 --- a/src/streams/TopicItem.js +++ b/src/streams/TopicItem.js @@ -16,6 +16,7 @@ import { getFlags, getSubscriptions, getStreamsById, + getStreamsByName, getOwnUser, } from '../selectors'; @@ -54,6 +55,7 @@ export default function TopicItem(props: Props) { auth: getAuth(state), mute: getMute(state), streams: getStreamsById(state), + streamsByName: getStreamsByName(state), subscriptions: getSubscriptions(state), ownUser: getOwnUser(state), flags: getFlags(state), diff --git a/src/subscriptions/subscriptionSelectors.js b/src/subscriptions/subscriptionSelectors.js index 78948f0db1c..1c3e8b4815f 100644 --- a/src/subscriptions/subscriptionSelectors.js +++ b/src/subscriptions/subscriptionSelectors.js @@ -21,6 +21,11 @@ export const getStreamsById: Selector> = createSelector( streams => new Map(streams.map(stream => [stream.stream_id, stream])), ); +export const getStreamsByName: Selector> = createSelector( + getStreams, + streams => new Map(streams.map(stream => [stream.name, stream])), +); + export const getSubscriptionsById: Selector> = createSelector( getSubscriptions, subscriptions => diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index 1f900859bf7..2811df55224 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -43,6 +43,7 @@ import { getSettings, getSubscriptions, getStreamsById, + getStreamsByName, getRealm, } from '../selectors'; import { withGetText } from '../boot/TranslationProvider'; @@ -82,6 +83,7 @@ export type BackgroundData = $ReadOnly<{| mutedUsers: MutedUsersState, ownUser: User, streams: Map, + streamsByName: Map, subscriptions: Subscription[], theme: ThemeName, twentyFourHourTime: boolean, @@ -311,6 +313,7 @@ const MessageList: ComponentType = connect( mutedUsers: getMutedUsers(state), ownUser: getOwnUser(state), streams: getStreamsById(state), + streamsByName: getStreamsByName(state), subscriptions: getSubscriptions(state), theme: getSettings(state).theme, twentyFourHourTime: getRealm(state).twentyFourHourTime, From 4e6cd052bcb224d0981cb62d8b71e2d895892de7 Mon Sep 17 00:00:00 2001 From: Akash Date: Thu, 10 Jun 2021 18:50:26 +0530 Subject: [PATCH 06/11] ActionSheet [nfc]: Update signature of actionsheet to use streamId. This is to facilitate complete migration to using stream id for actionsheet functions instead of stream name. --- src/message/__tests__/messageActionSheet-test.js | 7 +++++++ src/message/messageActionSheet.js | 7 +++++++ src/streams/TopicItem.js | 5 +++++ src/title/TitleStream.js | 1 + src/webview/handleOutboundEvents.js | 7 ++++++- 5 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/message/__tests__/messageActionSheet-test.js b/src/message/__tests__/messageActionSheet-test.js index 8227d461da1..967d6c7b6e8 100644 --- a/src/message/__tests__/messageActionSheet-test.js +++ b/src/message/__tests__/messageActionSheet-test.js @@ -47,12 +47,14 @@ describe('constructTopicActionButtons', () => { const streamMessage = eg.streamMessage(); const streamName = streamNameOfStreamMessage(streamMessage); const topic = streamMessage.subject; + const streamId = streamMessage.stream_id; test('show Unmute topic option if topic is muted', () => { const mute = deepFreeze([[streamName, topic]]); const buttons = constructTopicActionButtons({ backgroundData: { ...eg.backgroundData, mute }, streamName, + streamId, topic, }); expect(buttonTitles(buttons)).toContain('Unmute topic'); @@ -62,6 +64,7 @@ describe('constructTopicActionButtons', () => { const buttons = constructTopicActionButtons({ backgroundData: { ...eg.backgroundData, mute: [] }, streamName, + streamId, topic, }); expect(buttonTitles(buttons)).toContain('Mute topic'); @@ -72,6 +75,7 @@ describe('constructTopicActionButtons', () => { const buttons = constructTopicActionButtons({ backgroundData: { ...eg.backgroundData, subscriptions }, streamName, + streamId, topic, }); expect(buttonTitles(buttons)).toContain('Unmute stream'); @@ -82,6 +86,7 @@ describe('constructTopicActionButtons', () => { const buttons = constructTopicActionButtons({ backgroundData: { ...eg.backgroundData, subscriptions }, streamName, + streamId, topic, }); expect(buttonTitles(buttons)).toContain('Mute stream'); @@ -92,6 +97,7 @@ describe('constructTopicActionButtons', () => { const buttons = constructTopicActionButtons({ backgroundData: { ...eg.backgroundData, ownUser }, streamName, + streamId, topic, }); expect(buttonTitles(buttons)).toContain('Delete topic'); @@ -101,6 +107,7 @@ describe('constructTopicActionButtons', () => { const buttons = constructTopicActionButtons({ backgroundData: eg.backgroundData, streamName, + streamId, topic, }); expect(buttonTitles(buttons)).not.toContain('Delete topic'); diff --git a/src/message/messageActionSheet.js b/src/message/messageActionSheet.js index 4cde9abaa2c..0920b455aec 100644 --- a/src/message/messageActionSheet.js +++ b/src/message/messageActionSheet.js @@ -39,6 +39,7 @@ export type ShowActionSheetWithOptions = ( type TopicArgs = { auth: Auth, streamName: string, + streamId: number, topic: string, subscriptions: Subscription[], dispatch: Dispatch, @@ -230,6 +231,7 @@ cancel.errorMessage = 'Failed to hide menu'; export const constructTopicActionButtons = ({ backgroundData: { mute, ownUser, streams, subscriptions }, streamName, + streamId, topic, }: {| backgroundData: $ReadOnly<{ @@ -240,6 +242,7 @@ export const constructTopicActionButtons = ({ ... }>, streamName: string, + streamId: number, topic: string, |}): Button[] => { const buttons = []; @@ -397,6 +400,7 @@ export const showTopicActionSheet = ({ backgroundData, topic, streamName, + streamId, }: {| showActionSheetWithOptions: ShowActionSheetWithOptions, callbacks: {| @@ -413,11 +417,13 @@ export const showTopicActionSheet = ({ ... }>, streamName: string, + streamId: number, topic: string, |}): void => { const buttonList = constructTopicActionButtons({ backgroundData, streamName, + streamId, topic, }); showActionSheetWithOptions( @@ -430,6 +436,7 @@ export const showTopicActionSheet = ({ ...backgroundData, ...callbacks, streamName, + streamId, topic, }), ); diff --git a/src/streams/TopicItem.js b/src/streams/TopicItem.js index 11e0634be57..c7e755405f6 100644 --- a/src/streams/TopicItem.js +++ b/src/streams/TopicItem.js @@ -3,6 +3,7 @@ import React, { useContext } from 'react'; import { View } from 'react-native'; // $FlowFixMe[untyped-import] import { useActionSheet } from '@expo/react-native-action-sheet'; +import invariant from 'invariant'; import styles, { BRAND_COLOR, createStyleSheet } from '../styles'; import { RawLabel, Touchable, UnreadCount } from '../common'; @@ -61,6 +62,9 @@ export default function TopicItem(props: Props) { flags: getFlags(state), })); + const stream = backgroundData.streamsByName.get(streamName); + invariant(stream !== undefined, 'No stream with provided stream name was found.'); + return ( onPress(streamName, name)} @@ -70,6 +74,7 @@ export default function TopicItem(props: Props) { callbacks: { dispatch, _ }, backgroundData, streamName, + streamId: stream.stream_id, topic: name, }); }} diff --git a/src/title/TitleStream.js b/src/title/TitleStream.js index 10192fa6d84..fb49777e2cf 100644 --- a/src/title/TitleStream.js +++ b/src/title/TitleStream.js @@ -81,6 +81,7 @@ const TitleStream = (props: Props) => { callbacks: { dispatch, _ }, backgroundData, streamName: stream.name, + streamId: stream.stream_id, topic: topicOfNarrow(narrow), }); } diff --git a/src/webview/handleOutboundEvents.js b/src/webview/handleOutboundEvents.js index ec8e26afd75..fd93d967878 100644 --- a/src/webview/handleOutboundEvents.js +++ b/src/webview/handleOutboundEvents.js @@ -1,6 +1,7 @@ /* @flow strict-local */ import { Clipboard, Alert } from 'react-native'; +import invariant from 'invariant'; import * as NavigationService from '../nav/NavigationService'; import * as api from '../api'; import config from '../config'; @@ -212,11 +213,15 @@ const handleLongPress = ( const { dispatch, showActionSheetWithOptions, backgroundData, narrow, startEditMessage } = props; if (target === 'header') { if (message.type === 'stream') { + const streamName = streamNameOfStreamMessage(message); + const stream = backgroundData.streamsByName.get(streamName); + invariant(stream !== undefined, 'No stream with provided stream name was found.'); showTopicActionSheet({ showActionSheetWithOptions, callbacks: { dispatch, _ }, backgroundData, - streamName: streamNameOfStreamMessage(message), + streamName, + streamId: stream.stream_id, topic: message.subject, }); } else if (message.type === 'private') { From 7a8cf55c9f10b0a4c78533446ba539e872c5a17e Mon Sep 17 00:00:00 2001 From: Akash Date: Thu, 10 Jun 2021 19:12:29 +0530 Subject: [PATCH 07/11] ActionSheet [nfc]: Use streamId in actionsheet functions wherever possible. This commits uses the streamId parameter (introduced in the previous commit) to replace the usage of `streamName` as stream identifier wherever possible. Wherever possible means where the conversion is straightforward and does not require modification of current api definitions. --- src/message/messageActionSheet.js | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/src/message/messageActionSheet.js b/src/message/messageActionSheet.js index 0920b455aec..88e4c8e3b1a 100644 --- a/src/message/messageActionSheet.js +++ b/src/message/messageActionSheet.js @@ -165,29 +165,20 @@ const deleteTopic = async ({ auth, streamName, topic, dispatch, _ }) => { deleteTopic.title = 'Delete topic'; deleteTopic.errorMessage = 'Failed to delete topic'; -const unmuteStream = async ({ auth, streamName, subscriptions }) => { - const sub = subscriptions.find(x => x.name === streamName); - if (sub) { - await api.setSubscriptionProperty(auth, sub.stream_id, 'is_muted', false); - } +const unmuteStream = async ({ auth, streamId, subscriptions }) => { + await api.setSubscriptionProperty(auth, streamId, 'is_muted', false); }; unmuteStream.title = 'Unmute stream'; unmuteStream.errorMessage = 'Failed to unmute stream'; -const muteStream = async ({ auth, streamName, subscriptions }) => { - const sub = subscriptions.find(x => x.name === streamName); - if (sub) { - await api.setSubscriptionProperty(auth, sub.stream_id, 'is_muted', true); - } +const muteStream = async ({ auth, streamId, subscriptions }) => { + await api.setSubscriptionProperty(auth, streamId, 'is_muted', true); }; muteStream.title = 'Mute stream'; muteStream.errorMessage = 'Failed to mute stream'; -const showStreamSettings = ({ streamName, subscriptions }) => { - const sub = subscriptions.find(x => x.name === streamName); - if (sub) { - NavigationService.dispatch(navigateToStream(sub.stream_id)); - } +const showStreamSettings = ({ streamId, subscriptions }) => { + NavigationService.dispatch(navigateToStream(streamId)); }; showStreamSettings.title = 'Stream settings'; showStreamSettings.errorMessage = 'Failed to show stream settings'; From ad3c65dcba95d1ca933c33f6d9eebaa3b423ae24 Mon Sep 17 00:00:00 2001 From: Akash Date: Thu, 10 Jun 2021 19:40:03 +0530 Subject: [PATCH 08/11] topicActions [nfc]: Use `streamId` in `deleteMessagesForTopic`. This also involves changing the call to `deleteMessagesForTopic` appropriately. The change is done as a step to migrate to using `stream_id` instead of stream name for identifying streams. Related: #3918 --- src/message/messageActionSheet.js | 4 ++-- src/topics/topicActions.js | 19 +++++++++---------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/src/message/messageActionSheet.js b/src/message/messageActionSheet.js index 88e4c8e3b1a..def9e1c558d 100644 --- a/src/message/messageActionSheet.js +++ b/src/message/messageActionSheet.js @@ -132,7 +132,7 @@ const muteTopic = async ({ auth, streamName, topic }) => { muteTopic.title = 'Mute topic'; muteTopic.errorMessage = 'Failed to mute topic'; -const deleteTopic = async ({ auth, streamName, topic, 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) => { @@ -159,7 +159,7 @@ const deleteTopic = async ({ auth, streamName, topic, dispatch, _ }) => { ); }); if (await AsyncAlert()) { - await dispatch(deleteMessagesForTopic(streamName, topic)); + await dispatch(deleteMessagesForTopic(streamId, topic)); } }; deleteTopic.title = 'Delete topic'; diff --git a/src/topics/topicActions.js b/src/topics/topicActions.js index 521410a8f42..78d733346a4 100644 --- a/src/topics/topicActions.js +++ b/src/topics/topicActions.js @@ -1,9 +1,9 @@ /* @flow strict-local */ -import type { GetState, Dispatch, Narrow, Topic, Action, Outbox, Stream } from '../types'; +import type { GetState, Dispatch, Narrow, Topic, Action, Outbox } from '../types'; import * as api from '../api'; import { INIT_TOPICS } from '../actionConstants'; import { isStreamNarrow, streamNameOfNarrow } from '../utils/narrow'; -import { getAuth, getStreams } from '../selectors'; +import { getAuth, getStreams, getStreamsById } from '../selectors'; import { deleteOutboxMessage } from '../actions'; import { getOutbox } from '../directSelectors'; import { streamNameOfStreamMessage } from '../utils/recipient'; @@ -40,25 +40,24 @@ export const fetchTopicsForStream = (narrow: Narrow) => async ( dispatch(fetchTopics(stream.stream_id)); }; -export const deleteMessagesForTopic = (streamName: string, topic: string) => async ( +export const deleteMessagesForTopic = (streamId: number, topic: string) => async ( dispatch: Dispatch, getState: GetState, ) => { const state = getState(); const outbox = getOutbox(state); + const stream = getStreamsById(state).get(streamId); + outbox.forEach((outboxMessage: Outbox) => { if ( outboxMessage.type === 'stream' - && streamNameOfStreamMessage(outboxMessage) === streamName + // TODO: Use StreamId here (see #M3918) when `Outbox` starts + // having that property (see #M3998). + && streamNameOfStreamMessage(outboxMessage) === stream?.name && outboxMessage.subject === topic ) { dispatch(deleteOutboxMessage(outboxMessage.id)); } }); - const currentStream: Stream | void = getStreams(state).find( - (stream: Stream) => stream.name === streamName, - ); - if (currentStream) { - await api.deleteTopic(getAuth(state), currentStream.stream_id, topic); - } + await api.deleteTopic(getAuth(state), streamId, topic); }; From bcaf3bad557547ebc2edf8378db599a47f5d6b37 Mon Sep 17 00:00:00 2001 From: Akash Date: Thu, 10 Jun 2021 19:56:22 +0530 Subject: [PATCH 09/11] ActionSheet [nfc]: Pass unreadState data to action sheet. This is required for mark as read functionality that will be following this commit. --- src/__tests__/lib/exampleData.js | 1 + src/message/messageActionSheet.js | 5 ++++- src/streams/TopicItem.js | 2 ++ src/title/TitleStream.js | 4 ++++ src/webview/MessageList.js | 4 ++++ 5 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index e17c9edf306..3253039e51c 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -759,6 +759,7 @@ export const backgroundData: BackgroundData = deepFreeze({ streams: getStreamsById(baseReduxState), streamsByName: getStreamsByName(baseReduxState), subscriptions: [], + unread: baseReduxState.unread, theme: 'default', twentyFourHourTime: false, }); diff --git a/src/message/messageActionSheet.js b/src/message/messageActionSheet.js index def9e1c558d..dc0f08971e0 100644 --- a/src/message/messageActionSheet.js +++ b/src/message/messageActionSheet.js @@ -16,6 +16,7 @@ import type { EditMessage, Stream, } from '../types'; +import type { UnreadState } from '../unread/unreadModelTypes'; import { getNarrowForReply, isPmNarrow, @@ -220,7 +221,7 @@ cancel.title = 'Cancel'; cancel.errorMessage = 'Failed to hide menu'; export const constructTopicActionButtons = ({ - backgroundData: { mute, ownUser, streams, subscriptions }, + backgroundData: { mute, ownUser, streams, subscriptions, unread }, streamName, streamId, topic, @@ -229,6 +230,7 @@ export const constructTopicActionButtons = ({ mute: MuteState, streams: Map, subscriptions: Subscription[], + unread: UnreadState, ownUser: User, ... }>, @@ -403,6 +405,7 @@ export const showTopicActionSheet = ({ mute: MuteState, streams: Map, subscriptions: Subscription[], + unread: UnreadState, ownUser: User, flags: FlagsState, ... diff --git a/src/streams/TopicItem.js b/src/streams/TopicItem.js index c7e755405f6..97740d43c9c 100644 --- a/src/streams/TopicItem.js +++ b/src/streams/TopicItem.js @@ -20,6 +20,7 @@ import { getStreamsByName, getOwnUser, } from '../selectors'; +import { getUnread } from '../unread/unreadModel'; const componentStyles = createStyleSheet({ selectedRow: { @@ -58,6 +59,7 @@ export default function TopicItem(props: Props) { streams: getStreamsById(state), streamsByName: getStreamsByName(state), subscriptions: getSubscriptions(state), + unread: getUnread(state), ownUser: getOwnUser(state), flags: getFlags(state), })); diff --git a/src/title/TitleStream.js b/src/title/TitleStream.js index fb49777e2cf..46a04633e85 100644 --- a/src/title/TitleStream.js +++ b/src/title/TitleStream.js @@ -31,6 +31,8 @@ import { } from '../selectors'; import { showTopicActionSheet } from '../message/messageActionSheet'; import type { ShowActionSheetWithOptions } from '../message/messageActionSheet'; +import type { UnreadState } from '../unread/unreadModelTypes'; +import { getUnread } from '../unread/unreadModel'; type SelectorProps = {| stream: Subscription | {| ...Stream, in_home_view: boolean |}, @@ -39,6 +41,7 @@ type SelectorProps = {| mute: MuteState, streams: Map, subscriptions: Subscription[], + unread: UnreadState, ownUser: User, flags: FlagsState, |}, @@ -118,6 +121,7 @@ export default connect((state, props) => ({ mute: getMute(state), streams: getStreamsById(state), subscriptions: getSubscriptions(state), + unread: getUnread(state), ownUser: getOwnUser(state), flags: getFlags(state), }, diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index 2811df55224..2ce045e6333 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -58,6 +58,8 @@ import { handleWebViewOutboundEvent } from './handleOutboundEvents'; import { base64Utf8Encode } from '../utils/encoding'; import * as logging from '../utils/logging'; import { tryParseUrl } from '../utils/url'; +import type { UnreadState } from '../unread/unreadModelTypes'; +import { getUnread } from '../unread/unreadModel'; // ESLint doesn't notice how `this.props` escapes, and complains about some // props not being used here. @@ -85,6 +87,7 @@ export type BackgroundData = $ReadOnly<{| streams: Map, streamsByName: Map, subscriptions: Subscription[], + unread: UnreadState, theme: ThemeName, twentyFourHourTime: boolean, |}>; @@ -315,6 +318,7 @@ const MessageList: ComponentType = connect( streams: getStreamsById(state), streamsByName: getStreamsByName(state), subscriptions: getSubscriptions(state), + unread: getUnread(state), theme: getSettings(state).theme, twentyFourHourTime: getRealm(state).twentyFourHourTime, }; From 8f9c707a16fc1597215397141f11106bdcc99cd5 Mon Sep 17 00:00:00 2001 From: Akash Date: Thu, 10 Jun 2021 20:05:46 +0530 Subject: [PATCH 10/11] ActionSheet: Add mark as read option to topic action sheet. --- .../__tests__/messageActionSheet-test.js | 30 +++++++++++++++++++ src/message/messageActionSheet.js | 11 +++++++ static/translations/messages_en.json | 1 + 3 files changed, 42 insertions(+) diff --git a/src/message/__tests__/messageActionSheet-test.js b/src/message/__tests__/messageActionSheet-test.js index 967d6c7b6e8..2aab89ee194 100644 --- a/src/message/__tests__/messageActionSheet-test.js +++ b/src/message/__tests__/messageActionSheet-test.js @@ -5,6 +5,8 @@ import { streamNameOfStreamMessage } from '../../utils/recipient'; import * as eg from '../../__tests__/lib/exampleData'; import { constructMessageActionButtons, constructTopicActionButtons } from '../messageActionSheet'; +import { reducer } from '../../unread/unreadModel'; +import { initialState } from '../../unread/__tests__/unread-testlib'; const buttonTitles = buttons => buttons.map(button => button.title); @@ -49,6 +51,34 @@ describe('constructTopicActionButtons', () => { const topic = streamMessage.subject; const streamId = streamMessage.stream_id; + const baseState = (() => { + const r = (state, action) => reducer(state, action, eg.plusReduxState); + let state = initialState; + state = r(state, eg.mkActionEventNewMessage(streamMessage)); + return state; + })(); + + test('show mark as read if topic is unread', () => { + const unread = baseState; + const buttons = constructTopicActionButtons({ + backgroundData: { ...eg.backgroundData, unread }, + streamName, + streamId, + topic, + }); + expect(buttonTitles(buttons)).toContain('Mark topic as read'); + }); + + test('do not show mark as read if topic is read', () => { + const buttons = constructTopicActionButtons({ + backgroundData: eg.backgroundData, + streamName, + streamId, + topic, + }); + expect(buttonTitles(buttons)).not.toContain('Mark topic as read'); + }); + test('show Unmute topic option if topic is muted', () => { const mute = deepFreeze([[streamName, topic]]); const buttons = constructTopicActionButtons({ diff --git a/src/message/messageActionSheet.js b/src/message/messageActionSheet.js index dc0f08971e0..7056db3f488 100644 --- a/src/message/messageActionSheet.js +++ b/src/message/messageActionSheet.js @@ -30,6 +30,7 @@ import { doNarrow, deleteOutboxMessage, navigateToEmojiPicker, navigateToStream import { navigateToMessageReactionScreen } from '../nav/navActions'; import { deleteMessagesForTopic } from '../topics/topicActions'; import * as logging from '../utils/logging'; +import { getUnreadCountForTopic } from '../unread/unreadModel'; // TODO really this belongs in a libdef. export type ShowActionSheetWithOptions = ( @@ -121,6 +122,12 @@ const deleteMessage = async ({ auth, message, dispatch }) => { deleteMessage.title = 'Delete message'; deleteMessage.errorMessage = 'Failed to delete message'; +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, streamName, topic }) => { await api.setTopicMute(auth, streamName, topic, false); }; @@ -242,6 +249,10 @@ export const constructTopicActionButtons = ({ if (ownUser.is_admin) { buttons.push(deleteTopic); } + const unreadCount = getUnreadCountForTopic(unread, streamId, topic); + if (unreadCount > 0) { + buttons.push(markTopicAsRead); + } if (isTopicMuted(streamName, topic, mute)) { buttons.push(unmuteTopic); } else { diff --git a/static/translations/messages_en.json b/static/translations/messages_en.json index 5766aa2538d..b4867b2eeee 100644 --- a/static/translations/messages_en.json +++ b/static/translations/messages_en.json @@ -247,5 +247,6 @@ "Uploading {fileName} failed.": "Uploading {fileName} failed.", "Attachment": "Attachment", "Attachment {num}": "Attachment {num}", + "Failed to mark topic as read": "Failed to mark topic as read", "Remove account": "Remove account" } From f380b02843515cc1a8618a0c21666f9be68c5d57 Mon Sep 17 00:00:00 2001 From: Akash Date: Thu, 1 Jul 2021 22:25:45 +0530 Subject: [PATCH 11/11] ActionSheet [nfc]: Drop `streamName` from signature. Also involves necessary changes at call sites, and some actionsheet functions. --- .../__tests__/messageActionSheet-test.js | 36 ++++++++----------- src/message/messageActionSheet.js | 31 ++++++++-------- src/streams/TopicItem.js | 1 - src/title/TitleStream.js | 1 - src/webview/handleOutboundEvents.js | 1 - 5 files changed, 31 insertions(+), 39 deletions(-) diff --git a/src/message/__tests__/messageActionSheet-test.js b/src/message/__tests__/messageActionSheet-test.js index 2aab89ee194..8b4c7fa5c26 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'; @@ -46,10 +45,11 @@ describe('constructActionButtons', () => { }); describe('constructTopicActionButtons', () => { - const streamMessage = eg.streamMessage(); - const streamName = streamNameOfStreamMessage(streamMessage); + const stream = eg.makeStream(); + const streamMessage = eg.streamMessage({ stream }); const topic = streamMessage.subject; const streamId = streamMessage.stream_id; + const streams = deepFreeze(new Map([[stream.stream_id, stream]])); const baseState = (() => { const r = (state, action) => reducer(state, action, eg.plusReduxState); @@ -61,8 +61,7 @@ describe('constructTopicActionButtons', () => { test('show mark as read if topic is unread', () => { const unread = baseState; const buttons = constructTopicActionButtons({ - backgroundData: { ...eg.backgroundData, unread }, - streamName, + backgroundData: { ...eg.backgroundData, streams, unread }, streamId, topic, }); @@ -71,8 +70,7 @@ describe('constructTopicActionButtons', () => { test('do not show mark as read if topic is read', () => { const buttons = constructTopicActionButtons({ - backgroundData: eg.backgroundData, - streamName, + backgroundData: { ...eg.backgroundData, streams }, streamId, topic, }); @@ -80,10 +78,9 @@ describe('constructTopicActionButtons', () => { }); test('show Unmute topic option if topic is muted', () => { - const mute = deepFreeze([[streamName, topic]]); + const mute = deepFreeze([[stream.name, topic]]); const buttons = constructTopicActionButtons({ - backgroundData: { ...eg.backgroundData, mute }, - streamName, + backgroundData: { ...eg.backgroundData, streams, mute }, streamId, topic, }); @@ -92,8 +89,7 @@ describe('constructTopicActionButtons', () => { test('show mute topic option if topic is not muted', () => { const buttons = constructTopicActionButtons({ - backgroundData: { ...eg.backgroundData, mute: [] }, - streamName, + backgroundData: { ...eg.backgroundData, streams, mute: [] }, streamId, topic, }); @@ -101,10 +97,9 @@ describe('constructTopicActionButtons', () => { }); test('show Unmute stream option if stream is not in home view', () => { - const subscriptions = [{ ...eg.subscription, in_home_view: false }]; + const subscriptions = [{ ...eg.subscription, in_home_view: false, ...stream }]; const buttons = constructTopicActionButtons({ - backgroundData: { ...eg.backgroundData, subscriptions }, - streamName, + backgroundData: { ...eg.backgroundData, subscriptions, streams }, streamId, topic, }); @@ -112,10 +107,9 @@ describe('constructTopicActionButtons', () => { }); test('show mute stream option if stream is in home view', () => { - const subscriptions = [{ ...eg.subscription, in_home_view: true }]; + const subscriptions = [{ ...eg.subscription, in_home_view: true, ...stream }]; const buttons = constructTopicActionButtons({ - backgroundData: { ...eg.backgroundData, subscriptions }, - streamName, + backgroundData: { ...eg.backgroundData, subscriptions, streams }, streamId, topic, }); @@ -125,8 +119,7 @@ describe('constructTopicActionButtons', () => { test('show delete topic option if current user is an admin', () => { const ownUser = { ...eg.selfUser, is_admin: true }; const buttons = constructTopicActionButtons({ - backgroundData: { ...eg.backgroundData, ownUser }, - streamName, + backgroundData: { ...eg.backgroundData, ownUser, streams }, streamId, topic, }); @@ -135,8 +128,7 @@ describe('constructTopicActionButtons', () => { test('do not show delete topic option if current user is not an admin', () => { const buttons = constructTopicActionButtons({ - backgroundData: eg.backgroundData, - streamName, + backgroundData: { ...eg.backgroundData, streams }, streamId, topic, }); diff --git a/src/message/messageActionSheet.js b/src/message/messageActionSheet.js index 7056db3f488..6374a341f87 100644 --- a/src/message/messageActionSheet.js +++ b/src/message/messageActionSheet.js @@ -1,5 +1,6 @@ /* @flow strict-local */ import { Clipboard, Share, Alert } from 'react-native'; +import invariant from 'invariant'; import * as NavigationService from '../nav/NavigationService'; import type { @@ -40,10 +41,10 @@ export type ShowActionSheetWithOptions = ( type TopicArgs = { auth: Auth, - streamName: string, streamId: number, topic: string, subscriptions: Subscription[], + streams: Map, dispatch: Dispatch, _: GetText, ... @@ -128,14 +129,18 @@ const markTopicAsRead = async ({ auth, streamId, topic }) => { markTopicAsRead.title = 'Mark topic as read'; markTopicAsRead.errorMessage = 'Failed to mark topic as read'; -const unmuteTopic = async ({ auth, streamName, topic }) => { - await api.setTopicMute(auth, streamName, topic, false); +const unmuteTopic = async ({ auth, streamId, topic, streams }) => { + const stream = streams.get(streamId); + invariant(stream !== undefined, 'Stream with provided streamId must exist.'); + await api.setTopicMute(auth, stream.name, topic, false); }; unmuteTopic.title = 'Unmute topic'; unmuteTopic.errorMessage = 'Failed to unmute topic'; -const muteTopic = async ({ auth, streamName, topic }) => { - await api.setTopicMute(auth, streamName, topic, true); +const muteTopic = async ({ auth, streamId, topic, streams }) => { + const stream = streams.get(streamId); + invariant(stream !== undefined, 'Stream with provided streamId must exist.'); + await api.setTopicMute(auth, stream.name, topic, true); }; muteTopic.title = 'Mute topic'; muteTopic.errorMessage = 'Failed to mute topic'; @@ -229,7 +234,6 @@ cancel.errorMessage = 'Failed to hide menu'; export const constructTopicActionButtons = ({ backgroundData: { mute, ownUser, streams, subscriptions, unread }, - streamName, streamId, topic, }: {| @@ -241,7 +245,6 @@ export const constructTopicActionButtons = ({ ownUser: User, ... }>, - streamName: string, streamId: number, topic: string, |}): Button[] => { @@ -253,12 +256,14 @@ export const constructTopicActionButtons = ({ if (unreadCount > 0) { buttons.push(markTopicAsRead); } - if (isTopicMuted(streamName, topic, mute)) { + const stream = streams.get(streamId); + invariant(stream !== undefined, 'Stream with provided streamId not found.'); + if (isTopicMuted(stream.name, topic, mute)) { buttons.push(unmuteTopic); } else { buttons.push(muteTopic); } - const sub = subscriptions.find(x => x.name === streamName); + const sub = subscriptions.find(x => x.stream_id === streamId); if (sub && !sub.in_home_view) { buttons.push(unmuteStream); } else { @@ -403,7 +408,6 @@ export const showTopicActionSheet = ({ callbacks, backgroundData, topic, - streamName, streamId, }: {| showActionSheetWithOptions: ShowActionSheetWithOptions, @@ -421,26 +425,25 @@ export const showTopicActionSheet = ({ flags: FlagsState, ... }>, - streamName: string, streamId: number, topic: string, |}): void => { const buttonList = constructTopicActionButtons({ backgroundData, - streamName, streamId, topic, }); + const stream = backgroundData.streams.get(streamId); + invariant(stream !== undefined, 'Stream with provided streamId not found.'); showActionSheetWithOptions( { - title: `#${streamName} > ${topic}`, + title: `#${stream.name} > ${topic}`, options: buttonList.map(button => callbacks._(button.title)), cancelButtonIndex: buttonList.length - 1, }, makeButtonCallback(buttonList, { ...backgroundData, ...callbacks, - streamName, streamId, topic, }), diff --git a/src/streams/TopicItem.js b/src/streams/TopicItem.js index 97740d43c9c..13ad1cad8f1 100644 --- a/src/streams/TopicItem.js +++ b/src/streams/TopicItem.js @@ -75,7 +75,6 @@ export default function TopicItem(props: Props) { showActionSheetWithOptions, callbacks: { dispatch, _ }, backgroundData, - streamName, streamId: stream.stream_id, topic: name, }); diff --git a/src/title/TitleStream.js b/src/title/TitleStream.js index 46a04633e85..f358657761b 100644 --- a/src/title/TitleStream.js +++ b/src/title/TitleStream.js @@ -83,7 +83,6 @@ const TitleStream = (props: Props) => { showActionSheetWithOptions, callbacks: { dispatch, _ }, backgroundData, - streamName: stream.name, streamId: stream.stream_id, topic: topicOfNarrow(narrow), }); diff --git a/src/webview/handleOutboundEvents.js b/src/webview/handleOutboundEvents.js index fd93d967878..b84ea950d65 100644 --- a/src/webview/handleOutboundEvents.js +++ b/src/webview/handleOutboundEvents.js @@ -220,7 +220,6 @@ const handleLongPress = ( showActionSheetWithOptions, callbacks: { dispatch, _ }, backgroundData, - streamName, streamId: stream.stream_id, topic: message.subject, });