Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

message_actions: Add move message option #5189

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/action-sheets/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import {
import {
navigateToMessageReactionScreen,
navigateToPmConversationDetails,
navigateToMoveMessage,
} from '../nav/navActions';
import { deleteMessagesForTopic } from '../topics/topicActions';
import * as logging from '../utils/logging';
Expand Down Expand Up @@ -91,6 +92,7 @@ type MessageArgs = {
dispatch: Dispatch,
_: GetText,
startEditMessage: (editMessage: EditMessage) => void,
narrow: Narrow,
...
};

Expand Down Expand Up @@ -248,6 +250,14 @@ const resolveTopic = {
action: toggleResolveTopic,
};

const moveMessage = {
title: 'Move message',
errorMessage: 'Failed to move message',
action: async ({ message, narrow }) => {
NavigationService.dispatch(navigateToMoveMessage(message, narrow));
},
};

const unresolveTopic = {
title: 'Unresolve topic',
errorMessage: 'Failed to unresolve topic',
Expand Down Expand Up @@ -574,6 +584,7 @@ export const constructMessageActionButtons = (args: {|
&& (isStreamOrTopicNarrow(narrow) || isPmNarrow(narrow))
) {
buttons.push(editMessage);
buttons.push(moveMessage);
}
if (message.sender_id === ownUser.user_id && messageNotDeleted(message)) {
// TODO(#2793): Don't show if message isn't deletable.
Expand Down
1 change: 1 addition & 0 deletions src/api/messages/updateMessage.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export default async (

propagate_mode?: PropagateMode,
content?: string,
stream_id?: number,

send_notification_to_old_thread?: boolean,
send_notification_to_new_thread?: boolean,
Expand Down
211 changes: 211 additions & 0 deletions src/message/MoveMessage.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,211 @@
/* @flow strict-local */
import React, { useState, useContext } from 'react';
import { Text, View, Platform, Picker, TouchableOpacity, ScrollView } from 'react-native';
import type { Node } from 'react';
import { ThemeContext, BRAND_COLOR } from '../styles';
Comment on lines +4 to +5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: blank line between external and internal imports

import type { RouteProp } from '../react-navigation';
import * as api from '../api';
import Input from '../common/Input';
import { streamNarrow, streamIdOfNarrow } from '../utils/narrow';
import { getStreamForId } from '../subscriptions/subscriptionSelectors';
import type { AppNavigationProp } from '../nav/AppNavigator';
import { getAuth, getStreams, getOwnUser } from '../selectors';
import { useSelector } from '../react-redux';
import { showErrorAlert, showToast } from '../utils/info';
import { Icon } from '../common/Icons';
import type { Narrow, Message, Outbox } from '../types';
import TopicAutocomplete from '../autocomplete/TopicAutocomplete';
import { TranslationContext } from '../boot/TranslationProvider';
import ZulipButton from '../common/ZulipButton';

type Props = $ReadOnly<{|
navigation: AppNavigationProp<'move-message'>,
route: RouteProp<'move-message', {| message: Message | Outbox, messageNarrow: Narrow |}>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the intended meaning of messageNarrow? This could use some jsdoc here to clarify.

I'm not sure what it's supposed to mean here. That in turn is necessary in order to properly review the logic below that consumes it.

|}>;

const inputMarginPadding = {
paddingHorizontal: 8,
paddingVertical: Platform.select({
ios: 8,
android: 2,
}),
};

export default function MoveMessage(props: Props): Node {
const themeContext = useContext(ThemeContext);
const backgroundColor = themeContext.backgroundColor;
const cardColor = themeContext.cardColor;
const iconName = Platform.OS === 'android' ? 'arrow-left' : 'chevron-left';
const auth = useSelector(getAuth);
const allStreams = useSelector(getStreams);
const isAdmin = useSelector(getOwnUser).is_admin;
const messageId = props.route.params.message.id;
const currentStreamId = streamIdOfNarrow(props.route.params.messageNarrow);
const currentStreamName = useSelector(state => getStreamForId(state, currentStreamId)).name;
const [narrow, setNarrow] = useState(streamNarrow(currentStreamId));
const [subject, setSubject] = useState(props.route.params.message.subject);
Comment on lines +43 to +46
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These and some other spots would be made a bit simpler to read by pulling the route params out as locals:

  const { message, messageNarrow } = props.route.params;

That also replaces the need for a messageId local -- can just say message.id.

const [propagateMode, setPropagateMode] = useState('change_one');
const [streamId, setStreamId] = useState(currentStreamId);
const [topicFocus, setTopicFocus] = useState(false);
const _ = useContext(TranslationContext);

Comment on lines +34 to +51
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Organize this by grouping together different kinds of definitions, with blank lines to separate groups.

Follow the patterns seen in other components in the codebase: first props, then context, then selectors, then state.

const styles = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: put styles just before the big JSX expression they're styling, so after all the callback definitions

parent: {
backgroundColor: cardColor,
},
layout: {
margin: 10,
},
title: {
fontSize: 18,
color: backgroundColor === 'white' ? 'black' : 'white',
},
topicInput: {
height: 50,
backgroundColor,
...inputMarginPadding,
},
viewTitle: {
display: 'flex',
flexDirection: 'row',
justifyContent: 'space-between',
alignItems: 'center',
height: 50,
paddingHorizontal: 10,
marginBottom: 20,
},
textColor: {
color: backgroundColor === 'white' ? 'black' : 'white',
},
picker: { backgroundColor, marginBottom: 20 },
submitButton: {
marginTop: 10,
paddingTop: 15,
paddingBottom: 15,
marginLeft: 30,
marginRight: 30,
backgroundColor: BRAND_COLOR,
borderRadius: 10,
borderWidth: 1,
},
};

const handleTopicChange = (topic: string) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: use useCallback on each of these callback functions

setSubject(topic);
};

const handleTopicFocus = () => {
setTopicFocus(true);
};

const setTopicInputValue = (topic: string) => {
handleTopicChange(topic);
setTopicFocus(false);
};

const handleTopicAutocomplete = (topic: string) => {
setTopicInputValue(topic);
};

const updateMessage = () => {
try {
if (isAdmin) {
api.updateMessage(auth, messageId, {
Comment on lines +111 to +113
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These api.foo calls need await.

As discussed here:
#5189 (comment)
search for await api (e.g., use git grep -C2 "await api") to find examples.

Without that, the try/catch basically has no effect. You can demonstrate that by editing the implementation of api.updateMessage to artificially add an error, like throw new Error("oops"); or null.asdf;.

subject,
stream_id: streamId,
propagate_mode: propagateMode,
});
} else {
api.updateMessage(auth, messageId, { subject, propagate_mode: propagateMode });
Comment on lines +112 to +119
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These would be clearer by unifying as one api.updateMessage call, because almost everything is the same in the two cases.

Instead, use a conditional just to control the one thing that changed. One handy trick for this, if you want an extra property in one case and not the other, is to spread a conditional expression:

  {
    subject,
    ...(isAdmin ? { stream_id: streamId } : {}),
    propagate_mode: propagateMode,
  }

}
} catch (error) {
showErrorAlert(_('Failed to move message'), error.message);
props.navigation.goBack();
return;
}
props.navigation.goBack();
showToast(_('Moved message'));
};

const handleNarrow = (pickedStreamId: number) => {
Copy link
Contributor Author

@SilentCruzer SilentCruzer Jan 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handleNarrow

Noticed a bug when I made the PR. When move message function was accessed through the topic narrow, the topic auto complete didn't suggest any topic. So I had to create a new stream narrow.

To create a stream narrow, I needed the name of the stream.

First idea was to use the label I added to the items in the Picker component, but I later realized that I can't access the label after adding it, so I used the below method.

Since I had the id of the stream, mapped through the current stream list, and found the name,

Copy link
Member

@gnprice gnprice Apr 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, looking up the stream name given the stream ID is the right direction.

Don't do it with a linear search through the whole list of streams, though: #3339.

Instead, use getStreamsById, similar to the getStreamsByName I mentioned in another comment.

setStreamId(pickedStreamId);
setNarrow(streamNarrow(pickedStreamId));
};

return (
<ScrollView style={styles.parent}>
<View style={styles.layout}>
<View style={styles.viewTitle}>
<TouchableOpacity onPress={() => props.navigation.goBack()}>
<Icon size={20} color="gray" name={iconName} />
</TouchableOpacity>
<Text style={styles.title}>Move Message</Text>
<View />
</View>
Comment on lines +138 to +144
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't try to make this sort of custom replacement for common widgets that exist across the app. Instead, use the same standard widgets that the rest of the app uses.

You can see the consequences in your screenshots at the top of the thread. The "go back" arrow here doesn't match what appears in the rest of the app:
image
image

Similarly, having the screen's title in the middle of the app bar is inconsistent with the rest of the app, which has it at the left.

To find the right pattern to use, find any other screen in the app that has the piece of UI you're looking for -- so here, an app bar with a go-back button -- and look at the code for that screen.

<Text style={{ fontSize: 14, color: 'gray', marginBottom: 10 }}>Stream:</Text>
{isAdmin ? (
<View style={styles.picker}>
<Picker
selectedValue={currentStreamName}
onValueChange={(itemValue, itemIndex) => handleNarrow(parseInt(itemValue, 10))}
style={styles.textColor}
>
{allStreams.map(item => (
<Picker.Item label={item.name} value={item.stream_id.toString()} />
))}
</Picker>
</View>
) : (
<Text style={[styles.textColor, { marginBottom: 10 }]}>{currentStreamName}</Text>
)}
<Text style={{ fontSize: 14, color: 'gray', marginBottom: 10 }}>Topic:</Text>
<View style={{ marginBottom: 20 }}>
<Input
underlineColorAndroid="transparent"
placeholder="Topic"
autoFocus={false}
defaultValue={subject}
selectTextOnFocus
onChangeText={value => handleTopicChange(value)}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: simplify to just handleTopicChange

onFocus={handleTopicFocus}
blurOnSubmit={false}
returnKeyType="next"
style={styles.topicInput}
/>
<TopicAutocomplete
isFocused={topicFocus}
narrow={narrow}
text={subject}
onAutocomplete={handleTopicAutocomplete}
/>
</View>
<Text style={{ fontSize: 14, color: 'gray', marginBottom: 10 }}>Move options:</Text>
<View style={styles.picker}>
<Picker
selectedValue={propagateMode}
// $FlowFixMe[incompatible-call] : the itemValue will always be one of these values - change_one | change_later | change_all
onValueChange={(itemValue, itemIndex) => setPropagateMode(itemValue)}
Comment on lines +186 to +187
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, this fixme comment is helpful and this code is clearer (cf #5189 (comment) on the previous version.)

Let's do two things to help clean this up further:

  • Let's take some of the information that's in that comment, and move it into code: write a cast, like setPropagateMode((itemValue: 'change_one' | 'change_later' | 'change_all')).
  • Flow will still give an error and require a fixme. But now the code itself already expresses the list of possible values we think it can be. So the fixme comment is now free to explain why -- namely, because those are the values in the items right below this.

style={styles.textColor}
>
<Picker.Item label="Change only this message" value="change_one" />
<Picker.Item label="Change later messages to this topic" value="change_later" />
<Picker.Item
label="Change previous and following messages to this topic"
value="change_all"
/>
</Picker>
</View>
<Text style={{ fontSize: 14, marginBottom: 10, color: 'gray' }}>Content:</Text>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm this doesn't get translated, does it.

Use ZulipTextIntl for this sort of UI text. If you search the codebase for <Text, you'll notice we have very few places using Text directly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will also ensure the text gets an appropriate style. In particular, looking back at the screenshots in dark mode I see that the contrast is pretty low -- the text is harder to read than it should be. ZulipTextIntl will get the text color from the theme, which means it'll get an appropriate color to contrast with the theme's background color.

<Text style={[styles.textColor, { marginBottom: 20 }]}>
{props.route.params.message.content.replace(/<(?:.|\n)*?>/gm, '')}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's factor this expression out as a local variable in the function.

That way it gets a name that helps explain what its intended meaning is. It's kind of opaque as it is.

Hmmm, is the idea of this that we're taking the HTML of the message and attempting to turn it into plain text? I don't think this strategy is going to work for making something readable, in general, for complex messages.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of attempting to turn the HTML into readable text, I think a more reliable strategy will be to use the message's Markdown content.

We can get that from the server using api.getRawMessageContent. See editMessage in src/action-sheets/index.js for an example.

</Text>
<ZulipButton
style={{ flex: 1, margin: 8 }}
secondary={false}
text="Submit"
onPress={updateMessage}
/>
</View>
</ScrollView>
);
}
3 changes: 3 additions & 0 deletions src/nav/AppNavigator.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ import SettingsScreen from '../settings/SettingsScreen';
import UserStatusScreen from '../user-statuses/UserStatusScreen';
import SharingScreen from '../sharing/SharingScreen';
import { useHaveServerDataGate } from '../withHaveServerDataGate';
import moveMessage from '../message/MoveMessage';

export type AppNavigatorParamList = {|
'account-pick': RouteParamsOf<typeof AccountPickScreen>,
Expand All @@ -56,6 +57,7 @@ export type AppNavigatorParamList = {|
'emoji-picker': RouteParamsOf<typeof EmojiPickerScreen>,
'main-tabs': RouteParamsOf<typeof MainTabsScreen>,
'message-reactions': RouteParamsOf<typeof MessageReactionsScreen>,
'move-message': RouteParamsOf<typeof moveMessage>,
'password-auth': RouteParamsOf<typeof PasswordAuthScreen>,
'realm-input': RouteParamsOf<typeof RealmInputScreen>,
'search-messages': RouteParamsOf<typeof SearchMessagesScreen>,
Expand Down Expand Up @@ -124,6 +126,7 @@ export default function AppNavigator(props: Props): Node {
name="message-reactions"
component={useHaveServerDataGate(MessageReactionsScreen)}
/>
<Stack.Screen name="move-message" component={useHaveServerDataGate(moveMessage)} />
<Stack.Screen
name="search-messages"
component={useHaveServerDataGate(SearchMessagesScreen)}
Expand Down
11 changes: 10 additions & 1 deletion src/nav/navActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import {
} from '@react-navigation/native';

import * as NavigationService from './NavigationService';
import type { Message, Narrow, UserId, EmojiType } from '../types';
import type { Message, Narrow, UserId, EmojiType, Outbox } from '../types';
import type { PmKeyRecipients } from '../utils/recipient';
import type { SharedData } from '../sharing/types';
import type { ApiResponseServerSettings } from '../api/settings/getServerSettings';
Expand Down Expand Up @@ -127,6 +127,15 @@ export const navigateToMessageReactionScreen = (
reactionName?: string,
): GenericNavigationAction => StackActions.push('message-reactions', { messageId, reactionName });

export const navigateToMoveMessage = (
message: Message | Outbox,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type needs to agree with the route: RouteProp<…> on the component.

(This is one of the rare places where the type-checker isn't able to check this sort of thing for you, because of limitations in the types in the react-navigation library: #4757 (comment) .)

messageNarrow: Narrow,
): GenericNavigationAction =>
StackActions.push('move-message', {
message,
messageNarrow,
});

export const navigateToLegal = (): GenericNavigationAction => StackActions.push('legal');

export const navigateToUserStatus = (): GenericNavigationAction => StackActions.push('user-status');
Expand Down
4 changes: 4 additions & 0 deletions static/translations/messages_en.json
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
"Search": "Search",
"Log in": "Log in",
"Enter": "Enter",
"Submit": "Submit",
"Switch account": "Switch account",
"Log out": "Log out",
"Log out?": "Log out?",
Expand Down Expand Up @@ -200,12 +201,15 @@
"Cancel": "Cancel",
"Message copied": "Message copied",
"Edit message": "Edit message",
"Move message": "Move message",
"Moved message": "Moved message",
"Network request failed": "Network request failed",
"Failed to add reaction": "Failed to add reaction",
"Failed to reply": "Failed to reply",
"Failed to copy message to clipboard": "Failed to copy message to clipboard",
"Failed to share message": "Failed to share message",
"Failed to edit message": "Failed to edit message",
"Failed to move message": "Failed to move message",
"Failed to delete message": "Failed to delete message",
"Failed to star message": "Failed to star message",
"Failed to unstar message": "Failed to unstar message",
Expand Down