feat: New Organization Schema along with the ability to invite existing users to an organization #13002
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
5 Ignored Deployments
|
|
Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link. |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite. Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
|
|
||
| @@unique([email]) | ||
| @@unique([email, username]) | ||
| @@unique([username, organizationId]) |
There was a problem hiding this comment.
We will drop organizationId in separate deployment. If we remove it now, there would be username conflicts in the table.
📦 Next.js Bundle Analysis for @calcom/webThis analysis was generated by the Next.js Bundle Analysis action. 🤖 Seventy-one Pages Changed SizeThe following pages changed size from the code in this PR compared to its base branch:
DetailsOnly the gzipped size is provided here based on an expert tip. First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If Any third party scripts you have added directly to your app using the The "Budget %" column shows what percentage of your performance budget the First Load total takes up. For example, if your budget was 100kb, and a given page's first load size was 10kb, it would be 10% of your budget. You can also see how much this has increased or decreased compared to the base branch of your PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this. If you see "+/- <0.01%" it means that there was a change in bundle size, but it is a trivial enough amount that it can be ignored. |
hariombalhara
force-pushed
the
organizationId-column
branch
from
395e5e5 to
af0102e
Compare
hariombalhara
force-pushed
the
organizationId-column
branch
from
af0102e to
0f6dd1b
Compare
hariombalhara
force-pushed
the
organizationId-column
branch
from
0f6dd1b to
4bca8f3
Compare
hariombalhara
force-pushed
the
organizationId-column
branch
from
4bca8f3 to
fc3a266
Compare
hariombalhara
force-pushed
the
organizationId-column
branch
from
fc3a266 to
023803d
Compare
There was a problem hiding this comment.
This was such a huge undertaking like damn! I looked through the code and left some comments. I still want to manually test this.
I want to bring up I'm not sure I like the idea of the naming of "profile". While reading through the PR I had to get a grasp of what a profile in this context is. From my understanding, it's the org information and how it relates to the user. I think for maintainability we should name it something more obvious. What about userOrgRepository & userOrgProfile or something along those lines?
| async jwt({ | ||
| // Always available but with a little difference in value | ||
| token, | ||
| // Available only in case of signIn, signUp or useSession().update call. | ||
| trigger, | ||
| // Available when useSession().update is called. The value will be the POST data | ||
| session, | ||
| // Available only in the first call once the user signs in. Not available in subsequent calls | ||
| user, | ||
| // Available only in the first call once the user signs in. Not available in subsequent calls | ||
| account, | ||
| }) { |
| @@ -185,7 +186,7 @@ export async function refreshCredentials( | |||
| * Gets credentials from the user, team, and org if applicable | |||
| * | |||
| */ | |||
| export const getAllCredentials = async ( | |||
There was a problem hiding this comment.
Heads up the return type of this function is needed in NewSeatedBookingObject
There was a problem hiding this comment.
That's interesting. I don't see it imported anywhere. Found a reference but it's not imported there
There was a problem hiding this comment.
Hmm interesting I thought it had to be imported. You're right though and type checks are passing so I think we're good.
| @@ -900,18 +904,22 @@ function SideBar({ bannersHeight, user }: SideBarProps) { | |||
| <div className="flex h-full flex-col justify-between py-3 lg:pt-4"> | |||
| <header className="todesktop:-mt-3 todesktop:flex-col-reverse todesktop:[-webkit-app-region:drag] items-center justify-between md:hidden lg:flex"> | |||
| {orgBranding ? ( | |||
There was a problem hiding this comment.
Have we thought of enabling the profile switcher for people outside of orgs? I have a personal and a work Cal.com account that I switch between.
There was a problem hiding this comment.
Do you intend to use the same email for those or different emails? For different emails see this #13408 (comment).
For the same email, though it's not the focus but it's still doable by having some negative values of organizationId, though not sure if it's the best approach.
There was a problem hiding this comment.
My idea was for different emails. There might be some users that have a personal email and a work email. Their work emails would not be in an org. We could always follow up with another PR.
packages/trpc/server/routers/viewer/availability/team/listTeamAvailability.handler.ts
Show resolved
Hide resolved
packages/trpc/server/routers/viewer/teams/inviteMember/inviteMember.handler.ts
Show resolved
Hide resolved
packages/trpc/server/routers/viewer/teams/removeMember.handler.ts
Outdated
Show resolved
Hide resolved
Actually Profile is going to hold all the profiles including Personal Profile which is currently stored in User table in a way. So, as soon as a user is added to an organization, the User table stops being acting as his profile. After that, to create his personal profile, we would create an entry in Profile table only with organizationId=0 probably. This is how he would be able to switch b/w Personal and Organization Profile(both being stored in Profile table) |
|
Thank you so much for the detailed review @joeauyeung !! Really appreciate going through the huge PR for even minute details 🙏 I have addressed all the feedback. |
There was a problem hiding this comment.
Tested it out manually. I found a lot of small stuff that can honestly be in a follow up PR. We don't need to add more to this one. I did find one blocking thing with the profile switcher but other than that it looks good. Let me know if I missed any test cases.
Test Cases
- Inviting member to org w/o Cal account to auto join org
- Inviting member to an org team w/o Cal account and auto join org
- Inviting existing member to org, throws error
- Editing member role
- Inviting org member to a non org team
- Inviting a member w/o the same org domain & w/o a Cal account to the org & team
- Inviting existing user to org w/o same org domain to the org & team
- Inviting an existing org member to another org
Blocking
- When a user is a member of two orgs, when I go to the profile switcher it stays on the first org instead of switching over.
Minor feedback (Can follow up)
-
On settings shell, org avatar is broken
-
On org member's page, the avatars are falling back to the default
- Interesting though, when inviting an org member to a non org team the avatar renders as expected
- Interesting though, when inviting an org member to a non org team the avatar renders as expected
-
On org invite, create account image is broken
-
When creating a new team under an org and inviting members, the user avatar flashes but then is replaced with the org logo
-
On the switcher the org logo isn't rendering. I also think we should add an option to "Add another account"
-
I think if we're allowing org members to be in non-org / other orgs we should have some separation on the teams page
-
When accepting an org invite from an existing user, the organization doesn't render until the user has logged out. Should we force a logout after accepting?
|
Thanks for the testing @joeauyeung. I have Fixed the profile switcher, it was a regression. https://www.loom.com/share/6bd6d89766764fa38e7e43b26af84bb5 Also, we aren't enable Profile Switcher in this release - Here is the releases plan https://docs.google.com/document/d/1ii3eyn0F0LaUCrX1c58YgdMt2V7xZWI2AGX1yKKM2Ik/edit#heading=h.b2ecpp7kmwhx |
|
"When accepting an org invite from an existing user, the organization doesn't render until the user has logged out. Should we force a logout after accepting?" -After accepting the invite a refresh of the app is needed. I don't think logout is necessary. This is in my todo list and I think a refetch for session should fix the issue. |
There was a problem hiding this comment.
I think we're at a good point for V1. Awesome work @hariombalhara, this was no small feat. I'm excited for V2!
| team: { | ||
| include: { | ||
| eventTypes: true, | ||
| }, | ||
| }, | ||
| hashedLink: true, | ||
| users: true, | ||
| children: { | ||
| include: { | ||
| users: true, | ||
| }, | ||
| }, | ||
| hosts: { | ||
| include: { | ||
| user: true, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
I don't want to be a downer here but this is causing leakage of team members undecoded avatar URIs and passwords. I'm getting payloads of about 30MB locally with production-like data.
There was a problem hiding this comment.
What does this PR do?
Existing cal.com user can be invited to an organization now.
Fixes #13013
Fixes #13002
Fixes #13367
Fixes #13359
Demo - https://www.loom.com/share/a980bb3fc14140b99355aed36b639134
This is a big PR but it wasn't possible to break it down significantly further.
Schema Changes
Seeder
Repositories
Profile,User,Membershipwhich mostly abstract out the reusable queries together.Brainstorming and Release Plan here
https://docs.google.com/document/d/1ii3eyn0F0LaUCrX1c58YgdMt2V7xZWI2AGX1yKKM2Ik/edit
New Organization Accept Invite View inside Teams itself
TODO
Open a separate PR that would go live before this. [Will do this once the PR is fully reviewed and is ready to be merged.Chose against doing it. It is a bit of effort to separate the migration and I see no risk with going with the migration along with the changes as part of this PR.Type of change
How should this be tested?
There is a lot to test
Mandatory Tasks
Checklist