msglist: Handle topic permalinks (/with/<id>) just as we do /near/ links #5868
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Thanks! One comment below.
Let's also have a few unit tests for this.
In particular, looking at getNarrowFromNarrowLink, I think it will reject with links that are to a DM conversation, and it's only by accident that we don't have equally specific logic for links to stream/channel messages. So it'd be good to have a few test cases showing that doesn't get thrown off (and perhaps fix it to treat with the same as near in those DM links, too).
src/utils/internalLinks.js
Outdated
| @@ -217,7 +231,7 @@ export const getNearOperandFromLink = (url: URL, realm: URL): number | null => { | |||
| i % 2 === 0 | |||
| // The operator is 'near' and its meaning is not negated (`str` does | |||
| // not start with "-"). | |||
| && str === 'near', | |||
| && str === (actuallyWith ? 'with' : 'near'), | |||
There was a problem hiding this comment.
How about:
| && str === (actuallyWith ? 'with' : 'near'), | |
| && (str === 'near' || str == 'with'), |
I feel like that'd be somewhat less of a hack — it means we're treating with as a synonym of near, which is exactly what we intend.
There was a problem hiding this comment.
Would this be a good place for this quote—
* […] Quoting Greg from #5866:
* > Currently the app doesn't interpret the "anchor" meaning of `/near/`
* > links at all (that's #3604) — we already effectively give `/near/`
* > links exactly the meaning that the upcoming `/with/` links will have.
* > So to handle the new links, we just need to parse them and then
* > handle them the way we already handle `/near/` links.
? Treating with as a synonym of near is a hack, and this quote helps communicate why that's basically OK for zulip-mobile.
…with/ For this legacy codebase, we plan to treat the new /with/ links the same as /near/ links; see zulip#5866. This change makes getNarrowFromNarrowLink's handling of DM links more parallel with its handling of topic links; now, neither one will reject /with/ links. On Greg's suggestion: zulip#5868 (review)
chrisbobbe
force-pushed
the
pr-with-links
branch
from
9faa84b
to
c752b48
Compare
|
Thanks for the review! Revision pushed. |
| (hashSegments.length === 2 && (hashSegments[0] === 'pm-with' || hashSegments[0] === 'dm')) | ||
| || (hashSegments.length === 4 | ||
| && (hashSegments[0] === 'pm-with' || hashSegments[0] === 'dm') | ||
| && hashSegments[2] === 'near') |
There was a problem hiding this comment.
Hmm, I'd rather not have this regress in being specific about what it accepts, even though the existing code is already vague about this in the case of a stream-and-topic link. My previous comment was a bit unclear but what I meant was just fixing this to accept /with/NNN properly, not making it bug-compatible with the stream-and-topic case's willingness to accept /arbitrary/nonsense 🙂
Should be easy to do, just having it look for either string, the same way as in getNearOperandFromLink below.
…arsing On Greg's suggestion: zulip#5868 (review)
Explanation in a comment, which quotes Greg in zulip#5866. Fixes: zulip#5866
chrisbobbe
force-pushed
the
pr-with-links
branch
from
c752b48
to
9562778
Compare
|
Thanks for the review! Revision pushed. |
|
|
||
| const mockStore = configureStore([thunk]); | ||
| const mockStore = configureStore([thunk.withExtraArgument(combinedThunkExtras)]); |
There was a problem hiding this comment.
Interesting! It looks like we haven't written a new test using this redux-mock-store library since… 2020, in e5268bb. Before that it was 2018 in 75240ef, and before that just two commits in 2017, namely b0c7323 and bc14f0d. (Based on git log -S mockStore as well as some other git log searches.) I must have reviewed the later two of those four commits, but I'd forgotten about this functionality.
Mostly that's meant we just don't have tests for the actions code; git ls-files src/'*'Actions-test.js is very thin.
I guess one thing that's influenced my approach is that the existing tests we've had using this, including the one small existing test in this file, have been pretty unimpressive:
#5052 (comment)
I like these new tests, though. I think it makes a lot of sense to have an end-to-end test that, when you try to open a URL that looks like one of these, we navigate to the right spot. If anything it'd be better if it were more end-to-end — but that sounds like more work, so let's not do that here. 🙂 We do have such tests in zulip-flutter already.
Explanation in a comment, which quotes Greg in #5866.
Fixes: #5866