Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[dashboard] Implement inviting team members #4490

Merged
merged 3 commits into from
Jun 15, 2021
Merged

Conversation

jankeromnes
Copy link
Contributor

@jankeromnes jankeromnes commented Jun 11, 2021

Fixes #4421

Depends on #4482 (← please merge that one first, then rebase here).

@jankeromnes jankeromnes changed the title [dashboard] Implement inviting and removing team members [dashboard] Implement inviting team members Jun 11, 2021
@jankeromnes jankeromnes marked this pull request as ready for review June 11, 2021 15:37
@jankeromnes
Copy link
Contributor Author

jankeromnes commented Jun 11, 2021

Okay, this works! 🎉 Many thanks @AlexTugarev and @gtsiolis for helping test this! 🙏

The invite flow works well. 👍

Still to be done (e.g. as follow-ups after my holidays 😇):

  • Some UI bits have no effect (Member search, Member Role toggle, Remove member -- see "TODO"s in the code).
  • When joining fails (e.g. you try to join a second time), the error page is quite basic. It should probably be improved.
  • The URLs /new-team and /join-team maybe aren't ideal. Alternative suggestions welcome. 🙏

Copy link
Contributor

@gtsiolis gtsiolis 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 @jankeromnes! UX LGTM! Thanks for adding this! 🌟

Left a first round of comments that can easily slip into follow-up issues or PRs. 🏓

components/dashboard/src/App.tsx Show resolved Hide resolved
components/dashboard/src/App.tsx Show resolved Hide resolved
const membershipRepo = await this.getMembershipRepo();
const membership = await membershipRepo.findOne({ teamId, userId });
if (!!membership) {
throw new Error('You are already a member of this team');
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: If you are already a team member, ideally we could a) redirect to the members page as usual and b) show a toast notification (see #3530) at the bottom of the page with this message. 🍞

Copy link
Contributor

Choose a reason for hiding this comment

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

fyi: Added follow up issue for this in #5925. 🏓


public async addMemberToTeam(userId: string, teamId: string): Promise<void> {
if (teamId.length !== 36) {
throw new Error('This team ID is incorrect');
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Reminder to facelift these error pages and possibly include an action button. 🎗️

@@ -9,7 +9,11 @@ import moment from "moment";
import { useContext, useEffect, useState } from "react";
import { useLocation } from "react-router";
import Header from "../components/Header";
import DropDown from "../components/DropDown";
import { ItemsList, Item, ItemField, ItemFieldContextMenu } from "../components/ItemsList";
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: Great to see this awesome component again! 🌟 Cc @corneliusludmann

components/dashboard/src/teams/Members.tsx Show resolved Hide resolved
</div>
</div>)}
<button onClick={() => setShowInviteModal(true)} className="ml-2">Invite Members</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Could we get the action button on the empty state of the projects page to also trigger the same modal? Makes sense?

components/dashboard/src/teams/Members.tsx Show resolved Hide resolved
<p className="mt-1 text-gray-500 text-sm">{copied ? 'Copied to clipboard!' : 'Use this URL to join this team as a Member.'}</p>
</div>
<div className="flex justify-end mt-6">
<button className="secondary" onClick={() => setShowInviteModal(false)}>Done</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Not sure if this is needed here but Close could be better and aligned with other similar modals with one secondary action button down here.

Suggested change
<button className="secondary" onClick={() => setShowInviteModal(false)}>Done</button>
<button className="secondary" onClick={() => setShowInviteModal(false)}>Close</button>

@@ -1416,6 +1406,13 @@ export class GitpodServerImpl<Client extends GitpodClient, Server extends Gitpod
return this.teamDB.createTeam(user.id, name);
}

public async joinTeam(teamId: string): Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

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

this is tricky. it basically allow me to join any team, just by knowing the id, thus the id serves as password.
could a OTS help here? there is already a service in place.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think reusing OTS fits the purpose here well. We'll need to associate teams and invalidate the tokens on user action. For the email-based invite we'll also need to associate an email. I think we should introduce a dedicated table for this.

@svenefftinge
Copy link
Member

svenefftinge commented Jun 14, 2021

/werft run

👍 started the job as gitpod-build-jx-invite-team-members.5

const getInviteURL = () => {
const link = new URL(window.location.href);
link.pathname = '/join-team';
link.search = '?teamId=' + team?.id;
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to generate invite IDs on the server, that can be invalidated.

@svenefftinge svenefftinge self-assigned this Jun 14, 2021
@svenefftinge
Copy link
Member

I'll continue.

@svenefftinge svenefftinge force-pushed the jx/invite-team-members branch 4 times, most recently from 57523c1 to 0a8ef97 Compare June 14, 2021 12:06
@svenefftinge
Copy link
Member

svenefftinge commented Jun 14, 2021

/werft run

👍 started the job as gitpod-build-jx-invite-team-members.10

@svenefftinge
Copy link
Member

@AlexTugarev can you have another look?

Copy link
Member

@AlexTugarev AlexTugarev left a comment

Choose a reason for hiding this comment

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

Accepting invitation worked nicely, also rejection.

Left some comments/questions.

@@ -1416,6 +1406,42 @@ export class GitpodServerImpl<Client extends GitpodClient, Server extends Gitpod
return this.teamDB.createTeam(user.id, name);
}

public async joinTeam(inviteId: string): Promise<Team> {
const user = this.checkUser("joinTeam");
const invite = await this.teamDB.findTeamMembershipInviteById(inviteId);
Copy link
Member

Choose a reason for hiding this comment

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

30 sec for db-sync are negligible, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is async and in most cases regional, too.

}

public async resetGenericInvite(teamId: string): Promise<TeamMembershipInvite> {
this.checkUser("getGenericInvite");
Copy link
Member

Choose a reason for hiding this comment

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

typo, should be resetGenericInvite

setMembers(infos);
if (!invite) {
invite = await getGitpodService().server.resetGenericInvite(team.id);
Copy link
Member

Choose a reason for hiding this comment

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

Why calling reset is nothing was found?

Copy link
Member

Choose a reason for hiding this comment

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

If there is no invite URL we want to create one.

};

const resetInviteLink = async () => {
const newInvite = await getGitpodService().server.resetGenericInvite(team!.id);
Copy link
Member

Choose a reason for hiding this comment

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

why resetting by team.id? couldn't there be a race?

@svenefftinge
Copy link
Member

svenefftinge commented Jun 15, 2021

/werft run

👍 started the job as gitpod-build-jx-invite-team-members.14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Dashboard] Team Members
4 participants