Skip to content

Add stream_id to Outbox values #5000

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Sep 14, 2021
Merged

Add stream_id to Outbox values #5000

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e2ddb9b
compose [nfc]: Rename and document showComposeBoxOnNarrow
gnprice Sep 13, 2021
1299518
narrow: Add isConversationNarrow predicate
gnprice Sep 13, 2021
ce2a0fa
compose [nfc]: Document and assert assumptions about the given narrow
gnprice Sep 13, 2021
6e99c7b
compose [nfc]: Say `destinationNarrow` for narrows we're sending to
gnprice Sep 13, 2021
a040a2b
outbox [nfc]: Drop spurious "whole stream as destination" case
gnprice Sep 10, 2021
87ee111
outbox [nfc]: Rename a helper to outboxPropertiesForNarrow
gnprice Sep 10, 2021
0c251bf
outbox: Add a stream_id property
gnprice Sep 10, 2021
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
4 changes: 2 additions & 2 deletions src/chat/ChatScreen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import InvalidNarrow from './InvalidNarrow';
import { fetchMessagesInNarrow } from '../message/fetchActions';
import ComposeBox from '../compose/ComposeBox';
import UnreadNotice from './UnreadNotice';
import { canSendToNarrow, caseNarrowDefault, keyFromNarrow } from '../utils/narrow';
import { showComposeBoxOnNarrow, caseNarrowDefault, keyFromNarrow } from '../utils/narrow';
Copy link
Contributor

Choose a reason for hiding this comment

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

compose [nfc]: Rename and document showComposeBoxOnNarrow

The name `canSendToNarrow` was misleading: we never actually send
to a whole-stream narrow, but this function returns true for them.
The real point is that we show the compose box when you're looking
at that narrow.

(And then the compose box offers a text input for a topic, and
`ComposeBox#getDestinationNarrow` ensures that we send to the
chosen topic's specific narrow.)

Commit-message nit:

And then the compose box offers a text input for a topic

It doesn't offer it when you're composing a new PM or editing one. Maybe one day it'll grow an input for PM recipients?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, the parenthetical was meant to be about the whole-stream case -- i.e. the case where this isn't a narrow you can actually send to. But that wasn't clear. I'll reword.

import { getLoading, getSession } from '../directSelectors';
import { getFetchingForNarrow } from './fetchingSelectors';
import { getShownMessagesForNarrow, isNarrowValid as getIsNarrowValid } from './narrowsSelectors';
Expand Down Expand Up @@ -127,7 +127,7 @@ export default function ChatScreen(props: Props): Node {

const showMessagePlaceholders = haveNoMessages && isFetching;
const sayNoMessages = haveNoMessages && !isFetching;
const showComposeBox = canSendToNarrow(narrow) && !showMessagePlaceholders;
const showComposeBox = showComposeBoxOnNarrow(narrow) && !showMessagePlaceholders;

const auth = useSelector(getAuth);
const dispatch = useDispatch();
Expand Down
15 changes: 10 additions & 5 deletions src/compose/ComposeBox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import type { LayoutEvent } from 'react-native/Libraries/Types/CoreEventTypes';
import TextInputReset from 'react-native-text-input-reset';
import { type EdgeInsets } from 'react-native-safe-area-context';
import { compose } from 'redux';
import invariant from 'invariant';

import { withSafeAreaInsets } from '../react-native-safe-area-context';
import type { ThemeData } from '../styles';
Expand All @@ -31,6 +32,7 @@ import { FloatingActionButton, Input } from '../common';
import { showToast } from '../utils/info';
import { IconDone, IconSend } from '../common/Icons';
import {
isConversationNarrow,
isStreamNarrow,
isStreamOrTopicNarrow,
isTopicNarrow,
Expand All @@ -43,7 +45,6 @@ import getComposeInputPlaceholder from './getComposeInputPlaceholder';
import NotSubscribed from '../message/NotSubscribed';
import AnnouncementOnly from '../message/AnnouncementOnly';
import MentionWarnings from './MentionWarnings';

import { getAuth, getIsAdmin, getStreamInNarrow, getVideoChatProvider } from '../selectors';
import {
getIsActiveStreamSubscribed,
Expand All @@ -66,9 +67,12 @@ type SelectorProps = {|
|};

type OuterProps = $ReadOnly<{|
/** The narrow shown in the message list. Must be a conversation or stream. */
// In particular `getDestinationNarrow` makes assumptions about the narrow
// (and other code might too.)
narrow: Narrow,

onSend: (string, Narrow) => void,
onSend: (message: string, destinationNarrow: Narrow) => void,

isEditing: boolean,

Expand Down Expand Up @@ -391,23 +395,24 @@ class ComposeBoxInner extends PureComponent<Props, State> {
const topic = this.state.topic.trim();
return topicNarrow(streamName, topic || '(no topic)');
}
invariant(isConversationNarrow(narrow), 'destination narrow must be conversation');
return narrow;
};

handleSend = () => {
const { dispatch } = this.props;
const { message } = this.state;
const narrow = this.getDestinationNarrow();
const destinationNarrow = this.getDestinationNarrow();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be happy see this getDestinationNarrow helper replaced with a this.state.destinationNarrow. Would you be up for doing that, or should we do it later, or do you like it the way it is?

Copy link
Member Author

Choose a reason for hiding this comment

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

The thing about that is that it'd mean we have two different state properties topic and destinationNarrow that need to stay in sync -- there'd be an invariant that there needs to be a certain relationship between them. So any place where we updated one, we'd need to be sure to appropriately update the other at the same time.

That certainly can be done, but it's something that can easily acquire subtle bugs. So I'd keep the function until we have a good reason to avoid it.

In a future where we determine the destination narrow in some other way, the function also keeps it flexible for us to change that: its callsites just keep calling it. It might in the future refer to some other piece of state (like a current message, with the idea in your main comment above), or to a state.destinationNarrow. If we end up doing the latter, it'd be an easy followup to replace the function calls with direct references to the state then.

Copy link
Contributor

Choose a reason for hiding this comment

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

it'd mean we have two different state properties topic and destinationNarrow that need to stay in sync

I agree that this would get tricky. But when the user is composing a PM, in some sense it's a bug that ComposeBox is maintaining a topic state at all. (In the same way that it would be a bug to maintain a PM-recipients state when composing a stream message.) I'd been thinking—see #4773 (comment) —that state.topic could basically be replaced in ComposeBox by a state.destinationNarrow. If we need lower-level management of the topic input's state (e.g., for validation that topics aren't empty), we could split off a child component for that, that shows the topic input exactly when we're composing a stream message. I guess extracting that new child component would make even more sense if we're planning to have a counterpart for handling a PM-recipients input for composing PMs.

Thinking some more about this, it occurs to me that this.state.destinationNarrow vs. this.getDestinationNarrow() will basically collapse once this becomes a Hooks component:

// (if these end up lingering in `ComposeBox` and not split off to new child components)
const [ topic, setTopic ] = useState(/* ... */);
const [ pmRecipients, setPmRecipients ] = useState(/* ... */);

const destinationNarrow = /* ... */;

// Then it's very easy to react to changes in `destinationNarrow` in a
// `useEffect`, e.g., to send typing-stop and typing-start notifications as
// the destination narrow changes

So I think after all we don't have a strong need to make a state.destinationNarrow now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. Another angle is to say that for any future where we change what's in the state, if that makes something like state.destinationNarrow make more sense than getDestinationNarrow, then we can make that change then. It won't be any harder of a refactor than it would be now, and that way we'll only do it if we're confident we'll actually have a need for it.


this.props.onSend(message, narrow);
this.props.onSend(message, destinationNarrow);

this.setMessageInputValue('');

if (this.mentionWarnings.current) {
this.mentionWarnings.current.clearMentionWarnings();
}

dispatch(sendTypingStop(narrow));
dispatch(sendTypingStop(destinationNarrow));
};

inputMarginPadding = {
Expand Down
4 changes: 2 additions & 2 deletions src/message/NoMessages.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
isStreamNarrow,
isTopicNarrow,
isSearchNarrow,
canSendToNarrow,
showComposeBoxOnNarrow,
} from '../utils/narrow';

const styles = createStyleSheet({
Expand Down Expand Up @@ -60,7 +60,7 @@ export default class NoMessages extends PureComponent<Props> {
return (
<View style={styles.container}>
<Label style={styles.text} text={message.text} />
{canSendToNarrow(narrow) ? <Label text="Why not start the conversation?" /> : null}
{showComposeBoxOnNarrow(narrow) ? <Label text="Why not start the conversation?" /> : null}
</View>
);
}
Expand Down
4 changes: 2 additions & 2 deletions src/message/fetchActions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,13 +509,13 @@ export const doInitialFetch = (): ThunkAction<Promise<void>> => async (dispatch,
};

export const uploadFile = (
narrow: Narrow,
destinationNarrow: Narrow,
uri: string,
name: string,
): ThunkAction<Promise<void>> => async (dispatch, getState) => {
const auth = getAuth(getState());
const response = await api.uploadFile(auth, uri, name);
const messageToSend = `[${name}](${response.uri})`;

dispatch(addToOutbox(narrow, messageToSend));
dispatch(addToOutbox(destinationNarrow, messageToSend));
};
51 changes: 31 additions & 20 deletions src/outbox/outboxActions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/* @flow strict-local */
// $FlowFixMe[untyped-import]
import parseMarkdown from 'zulip-markdown-parser';
import invariant from 'invariant';

import * as logging from '../utils/logging';
import type {
Expand All @@ -11,6 +12,7 @@ import type {
Outbox,
PmOutbox,
StreamOutbox,
Stream,
UserOrBot,
UserId,
Action,
Expand All @@ -23,11 +25,11 @@ import {
DELETE_OUTBOX_MESSAGE,
MESSAGE_SEND_COMPLETE,
} from '../actionConstants';
import { getAuth } from '../selectors';
import { getAuth, getStreamsByName } from '../selectors';
import * as api from '../api';
import { getAllUsersById, getOwnUser } from '../users/userSelectors';
import { getUsersAndWildcards } from '../users/userHelpers';
import { caseNarrowPartial } from '../utils/narrow';
import { caseNarrowPartial, isConversationNarrow } from '../utils/narrow';
import { BackoffMachine } from '../utils/async';
import { recipientsOfPrivateMessage, streamNameOfStreamMessage } from '../utils/recipient';

Expand Down Expand Up @@ -126,31 +128,34 @@ const recipientsFromIds = (ids, allUsersById, ownUser) => {
return result;
};

// prettier-ignore
type DataFromNarrow =
| SubsetProperties<PmOutbox, {| type: mixed, display_recipient: mixed, subject: mixed |}>
| SubsetProperties<StreamOutbox, {| type: mixed, display_recipient: mixed, subject: mixed |}>;
| SubsetProperties<StreamOutbox, {| type: mixed, stream_id: mixed, display_recipient: mixed, subject: mixed |}>;

const extractTypeToAndSubjectFromNarrow = (
narrow: Narrow,
const outboxPropertiesForNarrow = (
destinationNarrow: Narrow,
streamsByName: Map<string, Stream>,
allUsersById: Map<UserId, UserOrBot>,
ownUser: UserOrBot,
): DataFromNarrow =>
caseNarrowPartial(narrow, {
caseNarrowPartial(destinationNarrow, {
pm: ids => ({
type: 'private',
display_recipient: recipientsFromIds(ids, allUsersById, ownUser),
subject: '',
}),

// TODO: we shouldn't ever be passing a whole-stream narrow here;
// ensure we don't, then remove this case
stream: name => ({ type: 'stream', display_recipient: name, subject: '(no topic)' }),

topic: (streamName, topic) => ({
type: 'stream',
display_recipient: streamName,
subject: topic,
}),
topic: (streamName, topic) => {
const stream = streamsByName.get(streamName);
invariant(stream, 'narrow must be known stream');
return {
type: 'stream',
stream_id: stream.stream_id,
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for doing this! Sorry I missed it in #4667. 🙂

display_recipient: stream.name,
subject: topic,
};
},
});

const getContentPreview = (content: string, state: GlobalState): string => {
Expand All @@ -168,18 +173,24 @@ const getContentPreview = (content: string, state: GlobalState): string => {
}
};

export const addToOutbox = (narrow: Narrow, content: string): ThunkAction<Promise<void>> => async (
dispatch,
getState,
) => {
export const addToOutbox = (
destinationNarrow: Narrow,
content: string,
): ThunkAction<Promise<void>> => async (dispatch, getState) => {
invariant(isConversationNarrow(destinationNarrow), 'destination narrow must be conversation');
const state = getState();
const ownUser = getOwnUser(state);

const localTime = Math.round(new Date().getTime() / 1000);
dispatch(
messageSendStart({
isSent: false,
...extractTypeToAndSubjectFromNarrow(narrow, getAllUsersById(state), ownUser),
...outboxPropertiesForNarrow(
destinationNarrow,
getStreamsByName(state),
getAllUsersById(state),
ownUser,
),
markdownContent: content,
content: getContentPreview(content, state),
timestamp: localTime,
Expand Down
3 changes: 2 additions & 1 deletion src/types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,8 @@ export type StreamOutbox = $ReadOnly<{|
// in the Outbox values we create; compare a1fad7ca9, for sender_id.
//
// Once it is required, it should move from here to the second type
// argument passed to `SubsetProperties` of `StreamMessage`, below.
// argument passed to `SubsetProperties` of `StreamMessage`, below;
// and we can also drop the hack line about it in `MessageLike`.
stream_id?: number,

...SubsetProperties<
Expand Down
20 changes: 19 additions & 1 deletion src/utils/narrow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,17 @@ export const isSearchNarrow = (narrow?: Narrow): boolean =>
export const isMentionedNarrow = (narrow?: Narrow): boolean =>
!!narrow && caseNarrowDefault(narrow, { mentioned: () => true }, () => false);

/**
* Whether the narrow represents a single whole conversation.
*
* A conversation is the smallest unit that discussions are threaded into:
* either a specific topic in a stream, or a PM thread (either 1:1 or group).
*
* When sending a message, its destination is identified by a conversation.
*/
export const isConversationNarrow = (narrow: Narrow): boolean =>
caseNarrowDefault(narrow, { topic: () => true, pm: () => true }, () => false);

/**
* Convert the narrow into the form used in the Zulip API at get-messages.
*/
Expand Down Expand Up @@ -467,7 +478,14 @@ export const isMessageInNarrow = (
// Adding a case here? Be sure to add to getNarrowsForMessage, too.
});

export const canSendToNarrow = (narrow: Narrow): boolean =>
/**
* Whether we show the compose box on this narrow's message list.
*
* This is really a UI choice that belongs to a specific part of the UI.
* It's here for now because several files refer to it.
*/
// TODO make this appropriately part of the UI code.
export const showComposeBoxOnNarrow = (narrow: Narrow): boolean =>
caseNarrow(narrow, {
pm: () => true,
stream: () => true,
Expand Down