Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

MM-9479 Add ability to join team and channel through url #1372

Merged
merged 3 commits into from
Jun 28, 2018
Merged

MM-9479 Add ability to join team and channel through url #1372

merged 3 commits into from
Jun 28, 2018

Conversation

sudheerDev
Copy link
Contributor

@sudheerDev sudheerDev commented Jun 22, 2018 •
edited
Loading

Summary

Add ability to join team and channel through url.
This will address the issue of joining the team after account creation or login if there is a redirect.

TODO:
Add access denied screen for private teams and private channels?

Ticket Link

MM-9479

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • Ran make check-style to check for style errors (required for all pull requests)
  • Ran make test to ensure unit and component tests passed
  • Added or updated unit tests (required for all new features)
  • Has UI changes
  • Includes text changes and localization file (.../i18n/en.json) updates

@sudheerDev sudheerDev added the Work in Progress Not yet ready for review label Jun 22, 2018
@amyblais amyblais added this to the v5.1.0 milestone Jun 22, 2018
@esethna esethna requested a review from hmhealey June 22, 2018 17:59
@sudheerDev sudheerDev added 2: Dev Review Requires review by a core commiter and removed Work in Progress Not yet ready for review labels Jun 22, 2018
@sudheerDev
Copy link
Contributor Author

From ticket.

A) Currently, when you land on the login page after clicking the link in step 2 above, the URL is https://pre-release.mattermost.com/login?redirect_to=%2Fcore%2Fchannels%2Fdevelopers.
We should use the same ?redirect_to=%2Fcore%2Fchannels%2Fdevelopers parameter throughout the sign up process to store which team/channel the user is redirected to, and for consistency with the login flow

Check

B) This should work for
links to a team, e.g. https://pre-release.mattermost.com/core/
links to a channel, e.g. https://pre-release.mattermost.com/core/channels/developers
permalinks, e.g. https://pre-release.mattermost.com/core/pl/nabkgtapmfdazyg87o8ak4xgee

Check

C) We should aim to make it work for all auth methods, including email/password, LDAP, SAML, and GitLab/Google/Office365 OAuth. For email/password auth, it should work with email verification on or off by including the redirect_url in the email verification.

Haven't tested with all methods but logic should hold true for all redirect(Will test this to be sure)

D) We should ensure that someone cannot request a verification email for you that redirects you to a malicious page.
This should currently not be possible with how the login redirect is set up right now (we only allow redirects within Mattermost), but we should just make sure of this case during implementation.

I didn't get this. Can someone shed more light on this? @esethna @jasonblais

E) When you first create an account, join a team and see the tutorial, you land on a URL such as https://pre-release.mattermost.com//tutorial, which doesn't contain the channel URL. As a result, after completing the tutorial, the user always gets redirected to the sign up page.
We should make sure that when the user completes the tutorial, they get redirected to the correct channel. This may mean landing the user at https://pre-release.mattermost.com//channels//tutorial instead through the redirect_url parameter.

Check

@jasonblais
Copy link
Contributor

@sudheerDev Regarding item D), this was something that @hmhealey mentioned when we discussed the ticket. @hmhealey Can you help elaborate on that point?

@sudheerDev
Copy link
Contributor Author

sudheerDev commented Jun 22, 2018 •
edited
Loading

There are couple of things.

  1. What should be the state of UI when a user is given a link which is not an open team?.
  2. If the team is open but the link corresponds to a private channel in it then what should the state?.

My take on the requirement was that we should always treat URL as a source of truth i.e don't redirect to a possible thread or team just because user has access

with that in mind should the above cases be handles this way

  1. Tell the user that team does not exist or they don't have permissions to join.
  2. Tell the user that channel does not exist or they don't have permissions to join the channel but at least let them join the team.

as opposed to what master does now

  1. Redirect to a default team
  2. Redirect to default channel in the team.

saturninoabril
saturninoabril reviewed Jun 25, 2018
View reviewed changes
Copy link
Member

@saturninoabril saturninoabril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just minor comments/queries.

components/needs_team/needs_team.jsx Outdated
@@ -50,14 +54,13 @@ export default class NeedsTeam extends React.Component {
team: PropTypes.string.isRequired,
}).isRequired,
}).isRequired,
history: PropTypes.object.isRequired,
};

constructor(params) {
super(params);

this.shortcutKeyDown = (e) => this.onShortcutKeyDown(e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work the same if this is removed and just make onShortcutKeyDown (below) as arrow function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does work. Will fix this.

components/needs_team/needs_team.jsx Outdated
@@ -81,35 +84,104 @@ export default class NeedsTeam extends React.Component {
this.state = {
team,
finishedFetchingChannels: false,
attemptingToJoinTeam: !team,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't see the use of attemptingToJoinTeam.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I don't need this anymore.

components/needs_team/needs_team.jsx
props.history.push('/?redirect_to=' + encodeURIComponent(props.location.pathname));
return null;
// Set up tracking for whether the window is active
window.isActive = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to set this to false on componentWillUnmount?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it is better to set it to false as not to updates on socket emits.

components/needs_team/needs_team.jsx Outdated
}

handleFocus = async () => {
await this.props.actions.markChannelAsRead(this.props.currentChannelId);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not using the return data of markChannelAsRead. Will it work the same without await?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. This does not require await here

components/needs_team/needs_team.jsx Outdated
if (team) {
const {error} = await props.actions.joinTeam(team.invite_id, team.id);
if (error) {
props.history.push('/error_page');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of /error_page, should this be pushed to /error and/or pass params like type, etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting for feedback on this. Yes we should have params and define the states for this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this been addressed yet? I think the way we normally handle this is to kick the user to the / route which then redirects the user like it would when they first log in

tests/components/__snapshots__/needs_team.test.jsx.snap
render={[Function]}
/>
</Switch>
`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

tests/components/needs_team.test.jsx Outdated
expect(wrapper.state().attemptingToJoinTeam).toEqual(true);
expect(wrapper.state().team).toEqual(null);
});
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@sudheerDev
Copy link
Contributor Author

@saturninoabril Fixed with review comments

@amyblais
Copy link
Member

Saturn is off rest of the week, would there be a second engineer who can review this in case Saturn doesn't have time tomorrow?

jwilander
jwilander approved these changes Jun 26, 2018
View reviewed changes
Copy link
Member

@jwilander jwilander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@hmhealey
Copy link
Member

Looks good other than the possible changes needed for the error handling

@amyblais amyblais removed this from the v5.1.0 milestone Jun 27, 2018
@sudheerDev
MM-9479 Add ability to join team and channel through url
3e5f61d 
 * If the url belongs to an unjoined team try to join and
   before redirecting to default team
 * Add tests to check join logic
@sudheerDev
Fix review comments
3d24531
@sudheerDev sudheerDev force-pushed the MM-9479 branch 2 times, most recently from bc3c041 to 83dab2f Compare June 27, 2018 20:12
@sudheerDev
Add error routes for channel and team
ff4229e 
 * Update snapshots with channel and team error pages.
@sudheerDev sudheerDev added the Setup Old Test Server Triggers the creation of a test server label Jun 27, 2018
@mattermod
Copy link
Contributor

Setup Test Server label detected. Spinmint test server created if build succeeds (checks pass and no conflicts with base branch).

@mattermod
Copy link
Contributor

Spinmint test server created at: http:https://i-0b71878690454709c.test.spinmint.com

Test Admin Account: Username: sysadmin | Password: sysadmin

Test User Account: Username: user-1 | Password: user-1

Instance ID: i-0b71878690454709c

esethna
esethna approved these changes Jun 28, 2018
View reviewed changes
Copy link
Contributor

@esethna esethna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work Sudheer!

@esethna esethna added this to the v5.1.0 milestone Jun 28, 2018
@esethna esethna removed the Setup Old Test Server Triggers the creation of a test server label Jun 28, 2018
@mattermod
Copy link
Contributor

Spinmint test server destroyed

@sudheerDev
Copy link
Contributor Author

@jwilander Added one commit for route handling for error scenarios. Can you have one more look at it?

@hmhealey hmhealey removed the 2: Dev Review Requires review by a core commiter label Jun 28, 2018
@hmhealey hmhealey merged commit 2da462f into mattermost:master Jun 28, 2018
@sudheerDev
Copy link
Contributor Author

@jwilander Never mind :)

@sudheerDev sudheerDev deleted the MM-9479 branch June 28, 2018 15:25
@esethna esethna added the 4: Reviews Complete All reviewers have approved the pull request label Jun 28, 2018
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Jun 28, 2018
@lindalumitchell lindalumitchell added the Tests/Done Release tests have been written label Jul 11, 2018
fincha pushed a commit to fincha/mattermost-webapp that referenced this pull request Oct 21, 2018
@sudheerDev
MM-9479 Add ability to join team and channel through url (mattermost#…
d4cdd0e 
…1372)

* MM-9479 Add ability to join team and channel through url

 * If the url belongs to an unjoined team try to join and
   before redirecting to default team
 * Add tests to check join logic

* Fix review comments

* Add error routes for channel and team

 * Update snapshots with channel and team error pages.
hmhealey added a commit that referenced this pull request Mar 17, 2021
@hmhealey
MM-33209 Downgrade serialize-error (#1372)
e0bc76c
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@saturninoabril saturninoabril saturninoabril left review comments

@jwilander jwilander jwilander approved these changes

@hmhealey hmhealey hmhealey approved these changes

@esethna esethna esethna approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation Tests/Done Release tests have been written
Projects
None yet
Milestone
v5.1.0
9 participants
@sudheerDev @jasonblais @amyblais @hmhealey @mattermod @jwilander @saturninoabril @esethna @lindalumitchell