stream IDs: sharing, links, tests #5183
Conversation
| // the props. That's because the props may have changed since the | ||
| // actual send request we just made. |
There was a problem hiding this comment.
I guess the proposed change doesn't add a ton of complexity, but also: in the first place, should the UI allow changing the message's destination while a send request is in progress? I think disallowing that is better than allowing it.
I can see wanting to change it after a request has started—but only if the request has failed or we've timed out on it, we've notified the user, and we want to let the user initiate a retry.
There was a problem hiding this comment.
Yeah, disallowing that in the UI would be quite reasonable.
Even then, I think it'd be cleaner for these callbacks to get sendTo from their caller (as they do with this change), rather than from the component's props (as they do before). That way the fact that this code all agrees on what conversation it's referring to is something that's straightforward to see locally -- the information gets read once and then passed around -- rather than have to wonder if there might be a way for it to get mutated between the different times we read it off the props.
src/utils/internalLinks.js
Outdated
| return streamNarrow(parseStreamOperand(paths[1], streamsById)); | ||
| case 'topic': { | ||
| const streamName = parseStreamOperand(paths[1], streamsById, streamsByName); | ||
| return streamName == null ? null : topicNarrow(streamName, parseTopicOperand(paths[3])); |
There was a problem hiding this comment.
internal links: Reject unrecognized streams
Now getNarrowFromLink returns null on a link to a stream we don't
know about, just as it would if the link is malformed. After all,
there's nothing useful we're going to do anyway with such a narrow.
I'd hope it's pretty rare that a stream-narrow link points to a stream that we don't know about. I guess it can happen in at least these cases:
- A user makes their own well-formed stream-narrow link instead of using an auto-generated one. The link points to a stream that clients don't know about (probably because it doesn't exist).
- (After this commit.) A link in the "old", stream-name-only format points to a stream that was renamed after the link was written.
(I think all clients are made aware of all archived streams and private streams, right? If not, that would add more cases.)
I think it'd be reasonable to give the user an error message if the link is well-formed but points to an unknown stream. I don't think it's necessarily worth more than a TODO for now, though.
I guess this change does add another codepath that ends up violating this bit of getFileTemporaryUrl's jsdoc, though:
* @param filePath The partial location of the uploaded file, as provided
* by the Zulip server to us in the message containing the file.
* It is a string of the form `/user_uploads/{realm_id_str}/{filename}`.
Is that OK?
There was a problem hiding this comment.
(I think all clients are made aware of all archived streams and private streams, right? If not, that would add more cases.)
No, I believe the client doesn't get to know about private streams the user isn't in, unless they're an admin. See: https://zulip.com/help/stream-permissions#private-streams
But this basically behaves as an example of your first case, because as far as this user can see it's supposed to be as if that stream never existed.
I think it'd be reasonable to give the user an error message if the link is well-formed but points to an unknown stream. I don't think it's necessarily worth more than a TODO for now, though.
Yeah, that'd be reasonable.
I guess this change does add another codepath that ends up violating this bit of
getFileTemporaryUrl's jsdoc, though:
Hmm, indeed.
That's not any kind of live bug, because that API endpoint has no side effects -- so any request that doesn't produce a useful, intended result will just result in an error. And that's caught by tryGetFileTemporaryUrl, and then its caller falls back to using the original URL directly. But it would be nice to clean up, in jsdoc and/or logic.
After we make the send the request and while we're awaiting its completion, the user could further edit the inputs, in particular the stream and topic. In that case `this.props.sendTo` will change, and when we get to `onShareSuccess` we'd end up using the new `sendTo`, and so navigating to the conversation identified by the new values, which isn't the one we actually sent the message to. Instead, pass to `onShareSuccess` the same `sendTo` value we actually just used for sending. Separately, it'd probably be good to have the UI prevent editing the inputs while the request is pending, because there's no good way for that to behave. Even with that, though, it'd be best to pass the data explicitly like this so that it's clear from local examination that the same `sendTo` is used throughout, rather than have to verify that we're 100% effective at preventing mutation.
We don't yet actually do anything with this stream ID. But with this change we now have one available at the two points in this code where we currently use the name to identify the stream: in sending the message, and in navigating to the conversation. In particular this takes care of one of the few remaining spots that wasn't ready for narrows to start carrying stream IDs.
This lets us have this widget around when the stream comes from an input widget which might pass through states where it doesn't have an actual stream selected -- in particular, ShareToStream, where the stream name comes from a text input. This code was already implicitly accepting narrows that weren't an actual stream, because at present a stream `Narrow` has just a stream name, and ShareToStream would cheerfully pass a narrow with the user's arbitrary text as the stream name. Both of the places where this code uses the narrow -- getTopicsForNarrow and fetchTopicsForStream -- silently ignore such narrows. So this really just gives us a way to make that explicit, by passing `null` for the narrow instead. We'll need that explicitness when we start having stream IDs in our narrows, and expecting them to be the IDs of real actual streams.
This way the narrows we're making are valid; and this code will be ready to straightforwardly start supply stream IDs in constructing these narrows, when we start accepting those. This change is NFC because both of the places we pass these narrows -- fetchTopicsForStream, and TopicAutocomplete -- will already do nothing if the stream name in the narrow is one that doesn't exist in our data.
In particular this means that if the stream name changes, we'll show the right name.
Now getNarrowFromLink returns null on a link to a stream we don't know about, just as it would if the link is malformed. After all, there's nothing useful we're going to do anyway with such a narrow.
This will help us when the Narrow constructors start requiring this.
|
Thanks for the review! Merging, with that TODO comment and other comment / commit-message changes. |
This is the next installment after #5069 on #3918, converting from stream names to stream IDs.
In this PR, we take care of getting stream IDs available at most of the remaining Narrow-constructor call sites that will need them:
Streamobject.