-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
fix: join subteam as their respective organization role #12216
Conversation
@SomayChauhan is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link to collect XP and win prizes! |
📦 Next.js Bundle Analysis for @calcom/webThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
@@ -373,7 +377,7 @@ export async function createAndAutoJoinIfInOrg({ | |||
userId: invitee.id, | |||
teamId: team.id, | |||
accepted: true, | |||
role: role, | |||
role: currentRole, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think about just doing this here:
orgMembership.role !== MembershipRole.MEMBER ? orgMembership.role : role
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually can't we just consider that if orgMembership role exists we should just set that ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, because lets say i have a user in my organization as MEMBER
now I want to make that user as a admin/owner of this team
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think about just doing this here:
orgMembership.role !== MembershipRole.MEMBER ? orgMembership.role : role
?
this I can do
This PR is being marked as stale due to inactivity. |
@SomayChauhan Please resolve the conflicts, otherwise it looks good to me. |
userId: userToAutoJoin.id, | ||
teamId: team.id, | ||
accepted: true, | ||
role: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is to handle normal non pending members
Screencast.from.10-12-23.02.09.29.PM.IST.webm
const data = []; | ||
// membership for the team | ||
data.push({ | ||
teamId: input.teamId, | ||
userId: invitee.id, | ||
role: input.role as MembershipRole, | ||
role: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is to join the pending members as their respective org role
Screencast.from.10-12-23.02.10.03.PM.IST.webm
@@ -72,6 +90,21 @@ export const updateUserHandler = async ({ ctx, input }: UpdateUserOptions) => { | |||
}), | |||
]); | |||
|
|||
if (input.role === MembershipRole.ADMIN || input.role === MembershipRole.OWNER) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we make someone an admin in the organization, let's also make them an admin in all the teams they're part of. But, only do this when they go from being a regular member to an admin, We don't want to accidentally strip admin privileges of this user from sub-teams if we have explicitly defined it
Screencast.from.10-12-23.02.14.02.PM.IST.webm
if (input.role === MembershipRole.ADMIN || input.role === MembershipRole.OWNER) { | ||
await prisma.membership.updateMany({ | ||
where: { | ||
userId: input.userId, | ||
teamId: { | ||
in: requestedMember.team.children.map((sub_team) => { | ||
return sub_team.members.find((item) => item.userId === input.userId).teamId; | ||
}), | ||
}, | ||
}, | ||
data: { | ||
role: input.role, | ||
}, | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about abstracting this in a separate function applyRoleToAllTeams
? It would make the code easy to understand and testable if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved it for now and it's ready to merge. You can push it in a separate PR, would be a quick merge.
role: input.role, | ||
})), | ||
data: autoJoinUsers.map((userToAutoJoin) => { | ||
const organizationRole = userToAutoJoin?.teams?.[0]?.role; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const organizationRole = userToAutoJoin?.teams?.[0]?.role; | |
const organizationRole = userToAutoJoin.teams?.[0]?.role; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @SomayChauhan !!
@SomayChauhan There is a type error |
@hariombalhara |
@SomayChauhan another TS error |
Apologies!!, forgot to change that one |
Fixes #11732
Screencast.from.03-11-23.12.29.02.PM.IST.webm