[PLT-91] Change URLs of Direct Messages to usernames and more+ #189

cometkim · 2017-10-24T00:37:34Z

Summary

Worked on team routes.

Channel Route: /channels/{channel_name}
GM Route: /messages/{group_id}
DM Route: /messages/@{username}
Fallback Route: /channels/{default_channel_name}

Worked on redirection rules.

/channels/{channel_id} -> Channel Route
/channels/{id1}__{id2} -> DM Route
/messages/{user_id} -> DM Route
/messages/{user_email} -> DM Route
/channels/{group_id} -> GM Route
Any other routes -> Fail Route (Let me know if needs more handling)

And Clean up codes.

Ticket Link

Checklist

Has UI changes
Added or updated unit tests (required for all new features)

jasonblais · 2017-10-24T11:58:43Z

Thanks @cometkim! Looks like there is a build failure, can you help resolve them?

cometkim · 2017-10-24T15:16:57Z

I resolved! @jasonblais

jasonblais · 2017-10-24T18:27:36Z

Have asked @lindalumitchell's help with testing

cometkim · 2017-10-25T18:43:56Z

Please wait for a few more changes.

cometkim · 2017-10-25T19:14:48Z

OK. Looks fine to me now.
I was replaced legacy URLs on the LHS.

Let me know if anyone finds more legacy URLs on the UI.

Worked on team routes. - Channel Route: /channels/{channel_name} - GM Route: /messages/{group_id} - DM Route: /messages/@{username} - Fail Route: /channels/{default_channel_name} Worked on redirection rules. - /channels/{channel_id} -> Channel Route - /channels/{id1}__{id2} -> DM Route - /messages/{user_id} -> DM Route - /messages/{user_email} -> DM Route - /channels/{group_id} -> GM Route - Any other routes -> Fail Route (Let me know if needs more handling)

…nged

cometkim · 2018-01-30T10:12:05Z

@jwilander Can I add the testURL config to jest to pass the test? that will affect to exist test snapshots which has URL.

…updated

cometkim · 2018-01-31T02:26:43Z

I just set the value to http:https://localhost:8065 and updated snapshots.
Now the tests are passed.

Please review and let me know if needs more changes then

jwilander · 2018-01-31T13:54:11Z

components/message_identifier_router.jsx

+import ChannelView from 'components/channel_view';
+import UserStore from 'stores/user_store.jsx';
+import ChannelStore from 'stores/channel_store.jsx';
+import TeamStore from 'stores/team_store.jsx';


This is minor and up to you since the code was already doing this, but it would nice if we could use redux in here instead of these flux stores

Ah, to make router container using redux would be better.

Yeah, actually it was same means above.

@jwilander Can you check the channel_identifier_router.jsx on the latest commit?

Still use the flux stores to effectively select entities so I'm not sure it makes sense for your comment. (ex: getByName)

GlobalActions.emitChannelClickEvent() seems to not work properly after connecting to redux store.
But if you like this style, I'd try to switch to this one.

Nah, that's fine. We can deal with that in another PR in the future

jwilander · 2018-01-31T13:57:02Z

components/message_identifier_router.jsx

+
+const LengthOfId = 26;
+const LengthOfGroupId = 40;
+const LengthOfUserIdPair = 54;


Also minor, but can we use all capitals for constants like so LENGTH_OF_ID? Just to be consistent with the rest of the codebase

jwilander · 2018-01-31T13:58:53Z

components/message_identifier_router.jsx

+ const {path, identifier} = match.params;
+
+ if (path === 'channels') {
+ if (identifier.length === LengthOfId) {


What happens if I have a channel name that happens to be 26 characters long? Is it going to try it as a channel ID and fail?

I've found I cannot enter more than 22 characters for a channel name in the webapp. if it could be in another way and that has not same length as specified, it will going to try it as a channel name, and fail (replaced to fallback URL).

crspeller · 2018-01-31T17:45:15Z

components/message_identifier_router.jsx

+ history.push(team ? `/${team}/channels/${Constants.DEFAULT_CHANNEL}` : '/');
+}
+
+export default class NeedsTeam extends React.PureComponent {


NeedsTeam? You mean MessageIdentifierRouter?

Right? I've wondered about why the previous is

I will change

@crspeller I have changed that to ChannelIdentiferRouter on latest commit

crspeller · 2018-01-31T17:46:28Z

components/message_identifier_router.jsx

+ }
+
+ render() {
+ return <ChannelView/>;


Is there a case where we actually render somthing or do we just redirect? If we just redirect then maybe we should just render nil here.

There is no redirect here, so I think it should keep returning channel view.

Actually, I'm not sure it can be called "router".

It is just a bunch of onEnter callback to select a channel and it does call an action, does not switch a route.

jwilander

LGTM once @crspeller's comments are addressed

jwilander · 2018-02-02T17:22:28Z

Thanks @cometkim ! Great work on this

* Changed getProfilesAndStatusesForPosts to get at mentioned users in posts * Fix loop format * Changed Object.keys to Object.values * Exported getProfilesAndStatusesForPost * Fix merge * Switch emoji endpoints to v4 * Add license upload and remove actions * Add analytics actions and reducers * Add action for creating group channel * Add functions to v4 client * Fix execute command * Update addToTeamFromInvite function * Update analytics reducers * Add client function to get public file link * Add default headers and oauth route to client * Add action to get user by email * Add team import client function * Set last viewed at for channel members in viewChannel * Remove current user from by ids and usernames actions * General clean-up in preparation for merge to master * Add telemetry tracking * Updates to telemetry * Remove accidentl test code * Always include cookies in fetch requests * Updates per feedback * Minor fix

cometkim mentioned this pull request Oct 24, 2017

[Help Wanted] Change URLs of Direct Messages to usernames mattermost/mattermost#7186

Closed

cometkim changed the title ~~WIP: [PLT-91] Change URLs of Direct Messages to usernames and more+~~ [PLT-91] Change URLs of Direct Messages to usernames and more+ Oct 24, 2017

jasonblais added 1: PM Review Requires review by a product manager Setup Old Test Server Triggers the creation of a test server labels Oct 24, 2017

jasonblais self-assigned this Oct 24, 2017

jasonblais added Hacktoberfest Awaiting Submitter Action Blocked on the author labels Oct 24, 2017

cometkim force-pushed the 7186-change-dm-url branch from 1f32e02 to d8bc771 Compare October 24, 2017 14:58

coreyhulen requested review from jwilander and crspeller October 24, 2017 15:56

jasonblais removed Awaiting Submitter Action Blocked on the author Setup Old Test Server Triggers the creation of a test server labels Oct 24, 2017

mattermost deleted a comment from mattermod Oct 24, 2017

jasonblais added the Setup Old Test Server Triggers the creation of a test server label Oct 24, 2017

jasonblais assigned lindalumitchell Oct 24, 2017

cometkim force-pushed the 7186-change-dm-url branch from d8bc771 to 5a9d737 Compare October 25, 2017 17:42

jasonblais removed the Setup Old Test Server Triggers the creation of a test server label Oct 25, 2017

mattermost deleted a comment from mattermod Oct 25, 2017

jasonblais added the Setup Old Test Server Triggers the creation of a test server label Oct 25, 2017

mattermost deleted a comment from mattermod Oct 25, 2017

cometkim added 2 commits January 30, 2018 16:06

Fix the problem on switching to group channel & Remove all useless await

f8edde8

cometkim force-pushed the 7186-change-dm-url branch from 4495e7c to f58ee94 Compare January 30, 2018 07:06

cometkim added 3 commits January 30, 2018 16:44

Change the direct channel url of sidebar

e0318a3

Change the direct channel url of profile popover

9665eee

Change message identifier router to pure component

689d09a

cometkim force-pushed the 7186-change-dm-url branch from f58ee94 to 689d09a Compare January 30, 2018 07:45

Change the url of direct channel immediately when the username is cha…

fb8d360

…nged

Set the testURL to http:https://localhost:8065 to pass the test. snapshots …

c2ef5cf

…updated

lindy65 removed the Awaiting Submitter Action Blocked on the author label Jan 31, 2018

jwilander reviewed Jan 31, 2018

View reviewed changes

jwilander mentioned this pull request Jan 31, 2018

Refactoring Layout components #669

Merged

cometkim added 2 commits February 1, 2018 00:02

Change the constants style to CAPITAL_SNAKE_CASE

e5484cc

Fix a bug that URL is changed with unexpected context

3e1088d

crspeller suggested changes Jan 31, 2018

View reviewed changes

jwilander approved these changes Feb 1, 2018

View reviewed changes

Rename MessageIdentifierRouter -> ChannelIdentifierRouter

cb8b1af

cometkim force-pushed the 7186-change-dm-url branch from 0005e4e to cb8b1af Compare February 2, 2018 01:34

crspeller approved these changes Feb 2, 2018

View reviewed changes

crspeller merged commit b42c94a into mattermost:master Feb 2, 2018

amyblais added Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation labels Feb 2, 2018

lindalumitchell added the Tests/Done Release tests have been written label Feb 9, 2018

hmhealey mentioned this pull request Feb 20, 2018

MM-9654 Updated channel routing to better handle 26 character long character names #840

Merged

cometkim deleted the 7186-change-dm-url branch September 27, 2018 19:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PLT-91] Change URLs of Direct Messages to usernames and more+ #189

[PLT-91] Change URLs of Direct Messages to usernames and more+ #189

cometkim commented Oct 24, 2017 •

edited

Loading

jasonblais commented Oct 24, 2017

cometkim commented Oct 24, 2017

jasonblais commented Oct 24, 2017

cometkim commented Oct 25, 2017

cometkim commented Oct 25, 2017

cometkim commented Jan 30, 2018 •

edited

Loading

cometkim commented Jan 31, 2018

jwilander Jan 31, 2018

cometkim Jan 31, 2018

cometkim Feb 1, 2018 •

edited

Loading

jwilander Feb 1, 2018

jwilander Jan 31, 2018

jwilander Jan 31, 2018

cometkim Jan 31, 2018

crspeller Jan 31, 2018

cometkim Feb 1, 2018

cometkim Feb 2, 2018

crspeller Jan 31, 2018

cometkim Feb 1, 2018

jwilander left a comment

jwilander commented Feb 2, 2018

[PLT-91] Change URLs of Direct Messages to usernames and more+ #189

[PLT-91] Change URLs of Direct Messages to usernames and more+ #189

Conversation

cometkim commented Oct 24, 2017 • edited Loading

Summary

Ticket Link

Checklist

jasonblais commented Oct 24, 2017

cometkim commented Oct 24, 2017

jasonblais commented Oct 24, 2017

cometkim commented Oct 25, 2017

cometkim commented Oct 25, 2017

cometkim commented Jan 30, 2018 • edited Loading

cometkim commented Jan 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cometkim Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwilander left a comment

Choose a reason for hiding this comment

jwilander commented Feb 2, 2018

cometkim commented Oct 24, 2017 •

edited

Loading

cometkim commented Jan 30, 2018 •

edited

Loading

cometkim Feb 1, 2018 •

edited

Loading