feat: managing teams #9285

BartoszJarocki · 2023-12-13T15:57:57Z

Description

Resolves #9208 #9209 #9210 #9211 #9212 #9213 #9214 #9215 #9216 #9217 #9218

(EDITED by Jordan, PR continues below)

Also: feat: As a SuperAdmin, I can promote others to SuperAdmin feat: As a SuperAdmin, I can demote others to other roles chore: yarn changes chore: update .vscode/launch.json from dep. pwa-chome to chrome

github-actions · 2024-03-07T05:17:28Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

jordanh · 2024-03-07T05:18:49Z

.vscode/launch.json

@@ -30,15 +30,15 @@
    {
      "name": "Launch Chrome",
      "request": "launch",
-      "type": "pwa-chrome",


pwa-chrome is no longer supported on my version of VS Code, the type is now simply chrome

Also: feat: As a SuperAdmin, I can promote others to SuperAdmin feat: As a SuperAdmin, I can demote others to other roles chore: yarn changes chore: update .vscode/launch.json from dep. pwa-chome to chrome

github-actions · 2024-03-07T06:13:43Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

jordanh · 2024-03-07T06:30:05Z

jordanh · 2024-03-07T06:36:28Z

packages/client/modules/userDashboard/components/OrgUserRow/OrgMemberRow.tsx

+            Leave Organization
+          </StyledFlatButton>
+        )}
+        {(isViewerOrgAdmin || (isViewerBillingLeader && !isViewerLastBillingLeader)) && (


This (and below) was the most significant change I made to what was started:

I created a wholly new control for Org Admin actions vs. actions that can be taken by the BillingLeader. This helped simplify logic when decided what role can take what action

github-actions · 2024-03-07T06:38:13Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

jordanh · 2024-03-07T16:32:06Z

packages/client/modules/userDashboard/components/OrgTeamMembers/OrgTeamMembers.tsx

+        ))}
+      </div>
+
+      {modalPortal(


@nickoferrall I've noted a bug just now that needs resolving before this can be merged. If the ORG_ADMIN is not a member of the team, the mutation that creates the MassInvitation link will fail. I can see 4 options:

If the ORG_ADMIN clicks the link, the client just-in-time fires a mutation to join this team

We do the same thing, but on the server-side: if an ORG_ADMIN tries to create a massInvite token, they are joined to the team first

Instead of having the ORG_ADMIN generate the mass-invite token, we masquerade as the Team Lead (ick)

We remove the "Add Member" functionality all together

I am split between options #1 and #4 and would love a second opinion. What do you think?

Why not just allow creating mass invitations from non-team members if they are an org admin? It could be irritating to join all those teams and getting the notifications and all.

I think that handling everything on the server would be ideal as it's more secure and less error-prone, but I think we should go with option four.

The PR is already a big 'un. Shipping this without the "Add Member" functionality will still add a lot of value, so I think we should work on "Add Member" in a separate PR.

@nickoferrall I like your logic there. Ok, I will do that.

Why not just allow creating mass invitations from non-team members if they are an org admin? It could be irritating to join all those teams and getting the notifications and all.

The underlying data model has the massInvitation belonging to a TeamMember. I suppose the fifth (and best) option would be to modify the massInvitation table to have invitations also be able to belong to non-TeamMembers (i.e. the ORG_ADMIN) as well

I'd probably be nice to get that table off of rethinkdb before making that modification

nickoferrall · 2024-03-07T18:00:14Z

packages/client/modules/userDashboard/components/OrgTeams/OrgTeams.tsx

+      <div className='divide-y divide-slate-300 overflow-hidden rounded border border-slate-300 bg-white'>
+        <div className='bg-slate-100 px-4 py-2'>
+          <div className='flex w-full justify-between'>
+            <div className='flex items-center font-bold'>{allTeams.length} Teams</div>


+1 if there's only one team, the copy should be "Team" rather than "Teams". We can use the plural helper function for this: https://github.com/ParabolInc/parabol/blob/8c68bf771a350b31c7948e6bd11c5c110e30c977/packages/client/utils/plural.ts

^ this is from a review in progress, not complete

nickoferrall · 2024-03-07T18:50:01Z

packages/client/modules/userDashboard/components/OrgTeamMembers/OrgTeamMembers.tsx

+        ))}
+      </div>
+
+      {modalPortal(


I think that handling everything on the server would be ideal as it's more secure and less error-prone, but I think we should go with option four.

The PR is already a big 'un. Shipping this without the "Add Member" functionality will still add a lot of value, so I think we should work on "Add Member" in a separate PR.

nickoferrall · 2024-03-07T19:05:25Z

packages/client/components/DeleteTeamDialog.tsx

+    history.push(`/me/organizations/${teamOrgId}/teams`)
+  }
+
+  const labelStyles = `text-left text-sm font-semibold mb-3 text-slate-600`


+1 these vars are just used in one place, so I find it kinda confusing that they're defined here rather than inline with the rest of the classNames

I agree! I'll update

nickoferrall · 2024-03-07T19:10:06Z

packages/client/components/DeleteTeamDialog.tsx

+          <Input
+            autoFocus
+            onChange={(e) => {
+              e.preventDefault()


+1 e.preventDefault() can be removed as there's no default to prevent. It'd probably be cleaner to move this to a handleChange function, but not important

nickoferrall · 2024-03-07T19:16:16Z

packages/client/components/DeleteTeamDialogRoot.tsx

+  teamOrgId: string
+}
+
+const DeleteTeamDialogRoot = (props: Props) => {


-1 I know you didn't write this, but I think we should delete this file. The convention is using Root components for lazy loading where we have a separate query.

As there's no query here, we should delete it and just use DeleteTeamDialog in the parent

Oh for sure, nice catch

nickoferrall · 2024-03-07T19:16:49Z

packages/client/components/DeleteTeamDialogRoot.tsx

+    <Suspense fallback={<Loader />}>
+      <DeleteTeamDialog
+        onDeleteTeam={onDeleteTeam}
+        isOpen={true}


+1 rather than isOpen={true}, in the parent component, we can use isDeleteTeamDialogOpened

Thank you, I done did it this way :)

nickoferrall · 2024-03-07T19:21:21Z

packages/client/components/OrgAdminActionMenu.tsx

+  )
+}
+
+export default withMutationProps(OrgAdminActionMenu)


+1 we're normally now using the hook useMutation rather than this HOC

Thank you for helping me climb the learning curve. I refactored this

nickoferrall · 2024-03-07T19:29:23Z

packages/client/modules/userDashboard/components/OrgTeams/OrgTeams.tsx

+        </div>
+      </div>
+
+      <div className='divide-y divide-slate-300 overflow-hidden rounded border border-slate-300 bg-white'>


+1 the tables in Members, Org Settings etc, have some box shadow. This one is a little flat:

Adding a shadow-sm would warm my heart. As would changing rounded to rounded-md.

nickoferrall · 2024-03-07T19:37:00Z

packages/client/modules/userDashboard/components/OrgUserRow/OrgMemberRow.tsx

+    {picture ? (
+      <Avatar hasBadge={false} picture={picture} size={44} />
+    ) : (
+      <img alt='' src={defaultUserAvatar} />


+1 alt tag could be default avatar

nickoferrall · 2024-03-07T19:49:04Z

packages/client/modules/userDashboard/components/OrgTeamMembers/OrgTeamMembersRow.tsx

+      </div>
+      <div>
+        <Button
+          disabled={!showMenuButton}


-1 as a new user, I go to the org team members page, I just see myself in the org members table, and I see the menu button. The menu button is disabled and I don't get feedback as to why it's disabled.

I think we should hide the button instead of disabling it or show a tooltip on hover explaining why it's disabled.

I was the team lead, I made a second user the team lead, and then clicked on my own menu button and saw it was empty:

Great finds! Fixed up

nickoferrall · 2024-03-07T19:55:26Z

packages/client/modules/userDashboard/components/OrgTeamMembers/OrgTeamMembers.tsx

+        </div>
+      </div>
+
+      <div className='divide-y divide-slate-300 overflow-hidden rounded border border-slate-300 bg-white'>


+1 adding rounded-md and shadow-sm would also make this table more consistent with the others and help it pop

Nice suggestion, and done!

down scoping this PR

github-actions · 2024-03-07T22:11:46Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

github-actions · 2024-03-07T23:09:29Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

jordanh · 2024-03-07T23:48:34Z

@nickoferrall thank's for the review! I think I've addressed you're review. If you feel comfortable merging, please do. Else, happy to address anything else or pass to another reviewer.

github-actions bot added the size/m label Dec 13, 2023

This was linked to issues Dec 13, 2023

Managing teams: Delete team #9210

Closed

Managing teams: Add a team member #9211

Closed

Managing teams: Remove a team member #9212

Closed

Managing teams: Manage team members roles #9218

Closed

github-actions bot added size/l and removed size/m labels Dec 13, 2023

jordanh force-pushed the feat/managing-teams branch from d01f583 to ca157f7 Compare February 20, 2024 00:30

nickoferrall added this to the Enabling Sales to Sell milestone Feb 28, 2024

BartoszJarocki and others added 9 commits March 4, 2024 16:51

feat: managing teams

6225717

feat: team members view

d97058a

feat: use team name in team details

97fed26

fix: routing

5919989

fix: some menus

8e6020b

feat: added alert dialog component

7cd6631

Added Delete Team Dialog

7496901

Updated Delete Dialog label

6d2bac8

Fixed Refresh after archiving team

a73feae

jordanh force-pushed the feat/managing-teams branch from ca157f7 to a73feae Compare March 5, 2024 00:52

feat: As a SuperAdmin, I can remove team members

1e45eb4

Also: feat: As a SuperAdmin, I can promote others to SuperAdmin feat: As a SuperAdmin, I can demote others to other roles chore: yarn changes chore: update .vscode/launch.json from dep. pwa-chome to chrome

jordanh force-pushed the feat/managing-teams branch from d3123a3 to 1e45eb4 Compare March 7, 2024 05:17

github-actions bot added size/xl and removed size/l labels Mar 7, 2024

jordanh reviewed Mar 7, 2024

View reviewed changes

jordanh added 2 commits March 6, 2024 22:13

feat: As a SuperAdmin, I can remove team members

8b84345

Also: feat: As a SuperAdmin, I can promote others to SuperAdmin feat: As a SuperAdmin, I can demote others to other roles chore: yarn changes chore: update .vscode/launch.json from dep. pwa-chome to chrome

chore: Refactor OrgMemberRow for readability

5ec917c

jordanh reviewed Mar 7, 2024

View reviewed changes

chore: revert errant tsconfig.json change

7e568eb

jordanh marked this pull request as ready for review March 7, 2024 15:44

jordanh requested a review from nickoferrall March 7, 2024 15:44

github-actions bot requested a review from mattkrick March 7, 2024 15:44

jordanh reviewed Mar 7, 2024

View reviewed changes

nickoferrall reviewed Mar 7, 2024

View reviewed changes

nickoferrall requested changes Mar 7, 2024

View reviewed changes

chore: revert add team members modal

65f1b99

down scoping this PR

chore: code review fixups

c14c0ec

nickoferrall approved these changes Mar 8, 2024

View reviewed changes

nickoferrall merged commit f351cf9 into master Mar 8, 2024
4 checks passed

nickoferrall deleted the feat/managing-teams branch March 8, 2024 13:14

parabol-release-bot bot mentioned this pull request Mar 8, 2024

chore(release): release v7.22.0 #9513

Merged

github-actions bot mentioned this pull request Mar 13, 2024

chore(release): Test v7.22.0 #9532

Closed

24 tasks

feat: managing teams #9285

feat: managing teams #9285

Conversation

BartoszJarocki commented Dec 13, 2023 • edited by jordanh

Description

github-actions bot commented Mar 7, 2024

Choose a reason for hiding this comment

github-actions bot commented Mar 7, 2024

jordanh commented Mar 7, 2024 • edited

Description

Demo

Testing scenarios

Final checklist

Choose a reason for hiding this comment

github-actions bot commented Mar 7, 2024

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

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

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

github-actions bot commented Mar 7, 2024

github-actions bot commented Mar 7, 2024

jordanh commented Mar 7, 2024

BartoszJarocki commented Dec 13, 2023 •

edited by jordanh

jordanh commented Mar 7, 2024 •

edited