stream IDs: sharing, links, tests

src/autocomplete/TopicAutocomplete.js

@@ -19,7 +19,9 @@ const styles = createStyleSheet({
 });
 
 type Props = $ReadOnly<{|
-  narrow: Narrow,
+  /** If null, this component won't do anything. */
+  narrow: Narrow | null,
+
   isFocused: boolean,
   text: string,
   onAutocomplete: (name: string) => void,
@@ -28,7 +30,7 @@ type Props = $ReadOnly<{|
 export default function TopicAutocomplete(props: Props): Node {
   const { narrow, isFocused, text, onAutocomplete } = props;
   const dispatch = useDispatch();
-  const topics = useSelector(state => getTopicsForNarrow(state, narrow));
+  const topics = useSelector(state => (narrow ? getTopicsForNarrow(state, narrow) : []));
 
   useEffect(() => {
     // The following should be sufficient to ensure we're up-to-date
@@ -43,10 +45,12 @@ export default function TopicAutocomplete(props: Props): Node {
     //
     // The latter is already taken care of (see handling of
     // EVENT_NEW_MESSAGE in topicsReducer). So, do the former here.
-    dispatch(fetchTopicsForStream(narrow));
+    if (narrow) {
+      dispatch(fetchTopicsForStream(narrow));
+    }
   }, [dispatch, narrow]);
 
-  if (!isFocused) {
+  if (!isFocused || !narrow) {
     return null;
   }
 

src/drafts/__tests__/draftsSelectors-test.js

@@ -1,12 +1,13 @@
-import deepFreeze from 'deep-freeze';
+/* @flow strict-local */
 
 import { getDraftForNarrow } from '../draftsSelectors';
 import { topicNarrow, keyFromNarrow } from '../../utils/narrow';
+import * as eg from '../../__tests__/lib/exampleData';
 
 describe('getDraftForNarrow', () => {
   test('return content of draft if exists', () => {
-    const narrow = topicNarrow('stream', 'topic');
-    const state = deepFreeze({
+    const narrow = topicNarrow(eg.stream.name, 'topic');
+    const state = eg.reduxState({
       drafts: {
         [keyFromNarrow(narrow)]: 'content',
       },
@@ -18,14 +19,14 @@ describe('getDraftForNarrow', () => {
   });
 
   test('return empty string if not exists', () => {
-    const narrow = topicNarrow('stream', 'topic');
-    const state = deepFreeze({
+    const narrow = topicNarrow(eg.stream.name, 'topic');
+    const state = eg.reduxState({
       drafts: {
         [keyFromNarrow(narrow)]: 'content',
       },
     });
 
-    const draft = getDraftForNarrow(state, topicNarrow('stream', 'topic1'));
+    const draft = getDraftForNarrow(state, topicNarrow(eg.stream.name, 'topic1'));
 
     expect(draft).toEqual('');
   });

src/message/__tests__/fetchActions-test.js

@@ -26,7 +26,7 @@ import * as logging from '../../utils/logging';
 
 const mockStore = configureStore([thunk]);
 
-const narrow = streamNarrow('some stream');
+const narrow = streamNarrow(eg.stream.name);
 const streamNarrowStr = keyFromNarrow(narrow);
 
 global.FormData = class FormData {};
@@ -54,7 +54,7 @@ describe('fetchActions', () => {
         narrows: Immutable.Map({
           [streamNarrowStr]: [],
         }),
-        streams: [eg.makeStream({ name: 'some stream' })],
+        streams: [eg.stream],
       });
 
       const result = isFetchNeededAtAnchor(state, narrow, FIRST_UNREAD_ANCHOR);
@@ -77,7 +77,7 @@ describe('fetchActions', () => {
           [streamNarrowStr]: [1],
         }),
         messages: eg.makeMessagesState([message1, message2]),
-        streams: [eg.makeStream({ name: 'some stream' })],
+        streams: [eg.stream],
       });
 
       const result = isFetchNeededAtAnchor(state, narrow, FIRST_UNREAD_ANCHOR);

src/message/__tests__/messageActions-test.js

@@ -13,7 +13,7 @@ import type { Action } from '../../actionTypes';
 
 const mockStore = configureStore([thunk]);
 
-const streamNarrowObj = streamNarrow('some stream');
+const streamNarrowObj = streamNarrow(eg.stream.name);
 
 describe('messageActions', () => {
   describe('doNarrow', () => {
@@ -25,7 +25,7 @@ describe('messageActions', () => {
         eg.reduxState({
           accounts: [eg.selfAccount],
           session: { ...eg.baseReduxState.session, isHydrated: true },
-          streams: [eg.makeStream({ name: 'some stream' })],
+          streams: [eg.stream],
         }),
       );
 

src/message/messagesActions.js

@@ -6,7 +6,7 @@ import { getMessageIdFromLink, getNarrowFromLink } from '../utils/internalLinks'
 import { openLinkWithUserPreference } from '../utils/openLink';
 import { navigateToChat } from '../nav/navActions';
 import { FIRST_UNREAD_ANCHOR } from '../anchor';
-import { getStreamsById } from '../subscriptions/subscriptionSelectors';
+import { getStreamsById, getStreamsByName } from '../subscriptions/subscriptionSelectors';
 import * as api from '../api';
 import { isUrlOnRealm } from '../utils/url';
 import { getOwnUserId } from '../users/userSelectors';
@@ -30,8 +30,14 @@ export const messageLinkPress = (href: string): ThunkAction<Promise<void>> => as
   const state = getState();
   const auth = getAuth(state);
   const streamsById = getStreamsById(state);
+  const streamsByName = getStreamsByName(state);
   const ownUserId = getOwnUserId(state);
-  const narrow = getNarrowFromLink(href, auth.realm, streamsById, ownUserId);
+  const narrow = getNarrowFromLink(href, auth.realm, streamsById, streamsByName, ownUserId);
+  // TODO: In some cases getNarrowFromLink successfully parses the link, but
+  //   finds it points somewhere we can't see: in particular, to a stream
+  //   that's hidden from our user (perhaps doesn't exist.)  For those,
+  //   perhaps give an error instead of falling back to opening in browser,
+  //   which should be futile.
   if (narrow) {
     const anchor = getMessageIdFromLink(href, auth.realm);
     dispatch(doNarrow(narrow, anchor));

src/sharing/ShareToStream.js

@@ -5,17 +5,16 @@ import type { ComponentType } from 'react';
 import type { ValidationError } from './ShareWrapper';
 import type { SharingNavigationProp } from './SharingScreen';
 import type { RouteProp } from '../react-navigation';
-import type { Dispatch, Subscription, Auth, GetText } from '../types';
+import type { Dispatch, Auth, GetText, Stream } from '../types';
 import type { SharedData } from './types';
 import { TranslationContext } from '../boot/TranslationProvider';
 import { connect } from '../react-redux';
 import { Input } from '../common';
-import { getSubscriptionsById } from '../subscriptions/subscriptionSelectors';
 import StreamAutocomplete from '../autocomplete/StreamAutocomplete';
 import TopicAutocomplete from '../autocomplete/TopicAutocomplete';
 import AnimatedScaleComponent from '../animation/AnimatedScaleComponent';
 import { streamNarrow } from '../utils/narrow';
-import { getAuth, getRealm } from '../selectors';
+import { getAuth, getRealm, getStreamsByName } from '../selectors';
 import { fetchTopicsForStream } from '../topics/topicActions';
 import ShareWrapper from './ShareWrapper';
 
@@ -26,7 +25,7 @@ type OuterProps = $ReadOnly<{|
 |}>;
 
 type SelectorProps = $ReadOnly<{|
-  subscriptions: Map<number, Subscription>,
+  streamsByName: Map<string, Stream>,
   auth: Auth,
   mandatoryTopics: boolean,
 |}>;
@@ -39,7 +38,12 @@ type Props = $ReadOnly<{|
 |}>;
 
 type State = $ReadOnly<{|
-  stream: string,
+  /** The text the user has typed into the "stream name" field. */
+  streamName: string,
+
+  /** An actual stream ID corresponding to streamName, or null if none does. */
+  streamId: number | null,
+
   topic: string,
   isStreamFocused: boolean,
   isTopicFocused: boolean,
@@ -50,7 +54,8 @@ class ShareToStreamInner extends React.Component<Props, State> {
   context: GetText;
 
   state = {
-    stream: '',
+    streamName: '',
+    streamId: null,
     topic: '',
     isStreamFocused: false,
     isTopicFocused: false,
@@ -69,16 +74,27 @@ class ShareToStreamInner extends React.Component<Props, State> {
   };
 
   focusTopic = () => {
-    const { stream } = this.state;
+    const { streamName, streamId } = this.state;
     const { dispatch } = this.props;
-    const narrow = streamNarrow(stream);
 
-    dispatch(fetchTopicsForStream(narrow));
+    if (streamId !== null) {
+      // We have an actual stream selected.  Fetch its recent topic names.
+      // TODO: why do we do this?  `TopicAutocomplete` does the same thing,
+      //   with an effect that reruns when the stream changes.
+      const narrow = streamNarrow(streamName);
+      dispatch(fetchTopicsForStream(narrow));
+    }
+    // If what's entered in the stream-name field isn't an actual stream,
+    // then there's no point fetching topics.
+    // … Maybe we shouldn't be allowing this interaction in that case in
+    // the first place?
+
     this.setState({ isTopicFocused: true });
   };
 
-  handleStreamChange = stream => {
-    this.setState({ stream });
+  handleStreamChange = streamName => {
+    const stream = this.props.streamsByName.get(streamName);
+    this.setState({ streamName, streamId: stream ? stream.stream_id : null });
   };
 
   handleTopicChange = topic => {
@@ -87,8 +103,15 @@ class ShareToStreamInner extends React.Component<Props, State> {
 
   handleStreamAutoComplete = (rawStream: string) => {
     // TODO: What is this for? (write down our assumptions)
-    const stream = rawStream.split('**')[1];
-    this.setState({ stream, isStreamFocused: false });
+    const streamName = rawStream.split('**')[1];
+    const stream = this.props.streamsByName.get(streamName);
+    this.setState({
+      streamName,
+      // TODO the "else" case ought to be impossible: the user chose a
+      //   stream in autocomplete, and we couldn't find it by name.
+      streamId: stream ? stream.stream_id : null,
+      isStreamFocused: false,
+    });
   };
 
   handleTopicAutoComplete = (topic: string) => {
@@ -97,12 +120,16 @@ class ShareToStreamInner extends React.Component<Props, State> {
 
   getValidationErrors: string => $ReadOnlyArray<ValidationError> = message => {
     const { mandatoryTopics } = this.props;
-    const { stream, topic } = this.state;
+    const { streamName, streamId, topic } = this.state;
     const { sharedData } = this.props.route.params;
 
     const result = [];
 
-    if (stream.trim() === '') {
+    if (streamId === null) {
+      result.push('stream-invalid');
+    }
+
+    if (streamName.trim() === '') {
       result.push('stream-empty');
     }
 
@@ -119,9 +146,18 @@ class ShareToStreamInner extends React.Component<Props, State> {
 
   render() {
     const { sharedData } = this.props.route.params;
-    const { stream, topic, isStreamFocused, isTopicFocused } = this.state;
-    const narrow = streamNarrow(stream);
-    const sendTo = { stream, topic, type: 'stream' };
+    const { streamName, streamId, topic, isStreamFocused, isTopicFocused } = this.state;
+    const narrow = streamId !== null ? streamNarrow(streamName) : null;
+    const sendTo = {
+      streamName,
+      /* $FlowFixMe[incompatible-cast]: ShareWrapper will only look at this
+       *   if getValidationErrors returns empty, so only if streamId is
+       *   indeed not null.  Should make that logic less indirected and more
+       *   transparent. */
+      streamId: (streamId: number),
+      topic,
+      type: 'stream',
+    };
 
     return (
       <ShareWrapper
@@ -130,11 +166,11 @@ class ShareToStreamInner extends React.Component<Props, State> {
         sendTo={sendTo}
       >
         <AnimatedScaleComponent visible={isStreamFocused}>
-          <StreamAutocomplete filter={stream} onAutocomplete={this.handleStreamAutoComplete} />
+          <StreamAutocomplete filter={streamName} onAutocomplete={this.handleStreamAutoComplete} />
         </AnimatedScaleComponent>
         <Input
           placeholder="Stream"
-          value={stream}
+          value={streamName}
           onChangeText={this.handleStreamChange}
           onFocus={this.focusStream}
           onChange={this.focusStream}
@@ -155,7 +191,7 @@ class ShareToStreamInner extends React.Component<Props, State> {
           onFocus={this.focusTopic}
           onBlur={this.blurTopic}
           onChangeText={this.handleTopicChange}
-          editable={stream !== ''}
+          editable={streamName !== ''}
           autoCapitalize="none"
         />
       </ShareWrapper>
@@ -164,7 +200,7 @@ class ShareToStreamInner extends React.Component<Props, State> {
 }
 
 const ShareToStream: ComponentType<OuterProps> = connect(state => ({
-  subscriptions: getSubscriptionsById(state),
+  streamsByName: getStreamsByName(state),
   auth: getAuth(state),
   mandatoryTopics: getRealm(state).mandatoryTopics,
 }))(ShareToStreamInner);

src/sharing/ShareWrapper.js

@@ -23,7 +23,8 @@ import { IconAttachment, IconCancel } from '../common/Icons';
 
 type SendTo =
   | {| type: 'pm', selectedRecipients: $ReadOnlyArray<UserId> |}
-  | {| type: 'stream', stream: string, topic: string |};
+  // TODO(#3918): Drop streamName.  Used below for sending, and for narrow.
+  | {| type: 'stream', streamName: string, streamId: number, topic: string |};
 
 const styles = createStyleSheet({
   wrapper: {
@@ -60,6 +61,7 @@ const styles = createStyleSheet({
 export type ValidationError =
   | 'mandatory-topic-empty'
   | 'stream-empty'
+  | 'stream-invalid'
   | 'recipients-empty'
   | 'message-empty';
 
@@ -132,6 +134,8 @@ class ShareWrapperInner extends React.Component<Props, State> {
           switch (error) {
             case 'stream-empty':
               return _('Please specify a stream.');
+            case 'stream-invalid':
+              return _('Please specify a valid stream.');
             case 'mandatory-topic-empty':
               return _('Please specify a topic.');
             case 'recipients-empty':
@@ -169,27 +173,31 @@ class ShareWrapperInner extends React.Component<Props, State> {
             content: messageToSend,
             type: 'stream',
             subject: sendTo.topic || apiConstants.NO_TOPIC_TOPIC,
-            to: sendTo.stream,
+            // TODO(server-2.0): switch to numeric stream ID, not name
+            to: sendTo.streamName,
           };
 
     try {
       await api.sendMessage(auth, messageData);
     } catch (err) {
       showToast(_('Failed to send message'));
       logging.error(err);
-      this.onShareCancelled();
+      this.onShareCancelled(sendTo);
       return;
     }
     showToast(_('Message sent'));
-    this.onShareSuccess();
+    this.onShareSuccess(sendTo);
   };
 
-  onShareCancelled = () => {
+  onShareCancelled = sendTo => {
+    // If in the future this callback uses the `sendTo` data, it should get
+    // it from its parameter (just like `onShareSuccess` does) and not from
+    // the props.  That's because the props may have changed since the
+    // actual send request we just made.
     NavigationService.dispatch(navigateBack());
   };
 
-  onShareSuccess = () => {
-    const { sendTo } = this.props;
+  onShareSuccess = sendTo => {
     switch (sendTo.type) {
       case 'pm': {
         const { selectedRecipients } = sendTo;
@@ -201,8 +209,8 @@ class ShareWrapperInner extends React.Component<Props, State> {
       }
 
       case 'stream': {
-        const { stream } = sendTo;
-        const narrow = streamNarrow(stream);
+        const { streamName } = sendTo;
+        const narrow = streamNarrow(streamName);
         NavigationService.dispatch(replaceWithChat(narrow));
         break;
       }