ABC-214: fix permalinks to direct messages #720
Conversation
This simplifies the changes introduced in ABC-160 to simply check that the channel exists instead of also checking that the channel team id matches the current team. The additional check was redundant, since the set of channels available are already constrained to the current team; it was also wrong, since direct messages and group messages aren't actually assigned team ids, leading to the regression in question. Note that this check (before and even now) only works if you have access to the two teams in question. If you try to open a malformed permalink referencing a team for which you don't have access or that doesn't exist, none of the router code in <NeedsTeam> executes (team is null), and a different path is executed that doesn't try to evaluate the permalink. Fixing that appears to be non-trivial and out of scope for this change.
|
@crspeller, if you have any advice re: the limitations of permalink checking above, let me know. This is my first exposure to react-router, and this seems like one of the challenges of allowing "side effects from views". I fell in love with the https://github.com/redux-saga/redux-saga library to externalize/"purify" side effects for precisely these kinds of challenges. |
| @@ -120,7 +120,7 @@ export async function emitPostFocusEvent(postId) { | |||
| if (data) { | |||
| const channelId = data.posts[data.order[0]].channel_id; | |||
| const channel = ChannelStore.getChannelById(channelId); | |||
| if (!channel || channel.team_id !== TeamStore.getCurrentId()) { | |||
| if (!channel) { | |||
There was a problem hiding this comment.
The check is unfortunately not redundant. This was put here to fix the case where you have a permalink with the wrong team ID at the beginning. So /wrong-team/pl/ID. Without this check, it will break (I forget how). But with it, it will redirect to an error page.
There was a problem hiding this comment.
You're referring to https://mattermost.atlassian.net/browse/ABC-160, right? This was the issue I had in mind while testing, and I've verified that this continues to fix ABC-160. I couldn't find a case where the channel existed but had a different team -- in all cases, the channel wasn't part of the channel store if you permalink to the wrong team.
* Keep autocomplete generated display names for DMs * Fixing tests
* Keep autocomplete generated display names for DMs * Fixing tests
Summary
This simplifies the changes introduced in ABC-160 to simply check that
the channel exists instead of also checking that the channel team id
matches the current team. The additional check was redundant, since the
set of channels available are already constrained to the current team;
it was also wrong, since direct messages and group messages aren't
actually assigned team ids, leading to the regression in question.
Note that this check (before and even now) only works if you have access
to the two teams in question. If you try to open a malformed permalink
referencing a team for which you don't have access or that doesn't
exist, none of the router code in
<NeedsTeam>executes (team is null),and a different path is executed that doesn't try to evaluate the
permalink. Fixing that appears to be non-trivial and out of scope for
this change.
Ticket Link
https://mattermost.atlassian.net/browse/ABC-214
Checklist
make check-styleto check for style errors (required for all pull requests)make testto ensure unit and component tests passed