msglist: Give error alert when link parses to Narrow we don't have data for

For PM narrows, we've been showing a really busted-looking screen
when we parse the link to a Narrow but we don't have data for the
user IDs; that's #5702.

For stream narrows, we've been taking the user out to a browser,
which is likely to be futile; that's #5698.

Fix both issues by, when we don't have data, preventing any
navigation and showing an error alert instead.

In the streams case (#5698), getNarrowFromLink wasn't distinguishing
the case of an unknown/nonexistent stream from the case of an
unparseable link. To change the behavior for the former but not the
latter case, we relax `parseStreamOperand` so that it still parses a
"new"-format operand even if it points to data we don't know about.
(Like we already do for PM operands.)

Fixes: #5698
Fixes: #5702

Author: chrisbobbe

src/chat/narrowsSelectors.js

@@ -211,6 +211,9 @@ export const getStreamInNarrow: Selector<Subscription | {| ...Stream, in_home_vi
   },
 );
 
+/**
+ * Whether PerAccountState has all data mentioned in `narrow` (user IDs etc.)
+ */
 export const isNarrowValid: Selector<boolean, Narrow> = createSelector(
   (state, narrow) => narrow,
   state => getStreamsById(state),

src/message/messagesActions.js

@@ -19,9 +19,11 @@ import {
   topicOfNarrow,
   streamNarrow,
   caseNarrowDefault,
+  isConversationNarrow,
 } from '../utils/narrow';
 import { hasMessageEverBeenInStream, hasMessageEverHadTopic } from './messageSelectors';
 import { showErrorAlert } from '../utils/info';
+import { isNarrowValid } from '../chat/narrowsSelectors';
 
 /**
  * Navigate to the given narrow.
@@ -232,14 +234,22 @@ export const messageLinkPress =
 
     const narrow = getNarrowFromLink(parsedUrl, auth.realm, streamsById, streamsByName, ownUserId);
 
-    // TODO(#5698): 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) {
       // This call is OK: `narrow` is truthy, so isNarrowLink(…) was true.
       const nearOperand = getNearOperandFromLink(parsedUrl, auth.realm);
+
+      if (!isNarrowValid(state, narrow)) {
+        // E.g., a stream that's hidden from our user (perhaps doesn't exist).
+        const msg =
+          nearOperand !== null
+            ? 'That message could not be found.'
+            : isConversationNarrow(narrow)
+            ? 'That conversation could not be found.'
+            : 'Those messages could not be found.';
+        showErrorAlert(_('Cannot open link'), _(msg));
+        return;
+      }
+
       if (nearOperand === null) {
         dispatch(doNarrow(narrow));
         return;

src/utils/__tests__/internalLinks-test.js

@@ -263,6 +263,10 @@ describe('getNarrowFromLink (part 2)', () => {
       expectStream(`${streamGeneral.stream_id}-general`, [streamGeneral], streamGeneral);
     });
 
+    test('if stream not found, use stream ID anyway', () => {
+      expectStream(`${streamGeneral.stream_id}-general`, [], streamGeneral);
+    });
+
     test('on stream link with wrong name: ID wins', () => {
       expectStream(`${streamGeneral.stream_id}-nonsense`, [streamGeneral], streamGeneral);
       expectStream(`${streamGeneral.stream_id}-`, [streamGeneral], streamGeneral);

src/utils/internalLinks.js

@@ -68,7 +68,14 @@ export const decodeHashComponent = (string: string): string => {
 /**
  * Parse the operand of a `stream` operator, returning a stream ID.
  *
- * Return null if the operand doesn't match any known stream.
+ * The ID might point to a stream that's hidden from our user (perhaps
+ * doesn't exist). If so, most likely the user doesn't have permission to
+ * see the stream's existence -- like with a guest user for any stream
+ * they're not in, or any non-admin with a private stream they're not in.
+ * Could be that whoever wrote the link just made something up.
+ *
+ * Returns null if the operand has an unexpected shape, or has the old shape
+ * (stream name but no ID) and we don't know of a stream by the given name.
  */
 // Why does this parser need stream data? Because the operand formats
 // ("new" and "old") collide, and in choosing which format to apply, we
@@ -91,12 +98,14 @@ const parseStreamOperand = (operand, streamsById, streamsByName): null | number
     return stream.stream_id;
   }
 
-  // Not any stream we know.  (Most likely this means a stream the user
-  // doesn't have permission to see the existence of -- like with a guest
-  // user for any stream they're not in, or any non-admin with a private
-  // stream they're not in.  Could also be an old-format link to a stream
-  // that's since been renamed… or whoever wrote the link could always have
-  // just made something up.)
+  if (newFormatStreamId != null) {
+    // Neither format found a Stream, so it's hidden or doesn't exist. But
+    // at least we have a stream ID; give that to the caller. (See jsdoc.)
+    return newFormatStreamId;
+  }
+
+  // Unexpected shape, or the old shape and we don't know of a stream with
+  // the given name.
   return null;
 };
 

static/translations/messages_en.json

@@ -22,6 +22,9 @@
   "Invalid image URL.": "Invalid image URL.",
   "Cannot open link": "Cannot open link",
   "Invalid URL.": "Invalid URL.",
+  "That message could not be found.": "That message could not be found.",
+  "That conversation could not be found.": "That conversation could not be found.",
+  "Those messages could not be found.": "Those messages could not be found.",
   "Invisible mode": "Invisible mode",
   "{num_of_people, plural,\n  one {This message has been <z-link>read</z-link> by {num_of_people} person:}\n  other {This message has been <z-link>read</z-link> by {num_of_people} people:}}": "{num_of_people, plural,\n  one {This message has been <z-link>read</z-link> by {num_of_people} person:}\n  other {This message has been <z-link>read</z-link> by {num_of_people} people:}}",
   "View read receipts": "View read receipts",