feat: new-app-install-flow follow up #14699
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. |
📦 Next.js Bundle Analysis for @calcom/webThis analysis was generated by the Next.js Bundle Analysis action. 🤖 New Page AddedThe following page was added to the bundle from the code in this PR:
Seventy-five 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. |
There was a problem hiding this comment.
To specify, on my comment. We should remove any instances of userAdminTeams now that we have the new install flow.
This is an expensive query since we're looking for all teams & orgs the user is an admin / owner of. This should live in the new app install flow.
|
Got it!!, i'm looking into it |
| getParentInfo?: boolean; | ||
| includeCredentials?: boolean; | ||
| }): Promise<UserAdminTeams> => { | ||
| const teams = await prisma.team.findMany({ |
There was a problem hiding this comment.
instead of querying all teams and then looking weather this team has current user as admin/owner (which is slow because we have to look at all the teams in db)
| }): Promise<UserAdminTeams> => { | ||
| const teams = await prisma.team.findMany({ | ||
| export const getUserAdminTeams = async (userId: number): Promise<number[]> => { | ||
| const user = await prisma.user.findFirst({ |
There was a problem hiding this comment.
queried the user using userId got all his teams and then filter all the teams that have the current user as admin/owner
(this should be faster because user_id is the primary key for the table so getting all the teams of user should be fast)
| }, | ||
| select: { | ||
| teams: { | ||
| where: { |
There was a problem hiding this comment.
filter all the teams that have the current user as admin/owner
| accepted: true, | ||
| role: { in: [MembershipRole.ADMIN, MembershipRole.OWNER] }, | ||
| }, | ||
| select: { id: true }, |
There was a problem hiding this comment.
only select the required fields
Graphite Automations"Add community label" took an action on this PR • (05/01/24)1 label was added to this PR based on Keith Williams's automation. "Add foundation team as reviewer" took an action on this PR • (05/01/24)1 reviewer was added to this PR based on Keith Williams's automation. "Add consumer team as reviewer" took an action on this PR • (05/01/24)1 reviewer was added to this PR based on Keith Williams's automation. |
| teamId, | ||
| }); | ||
|
|
||
| const res = await fetch( |
There was a problem hiding this comment.
decided to remove this api call and use useAddAppMutation instead
| }, | ||
| mutation.mutate( | ||
| { | ||
| isOmniInstall: true, |
There was a problem hiding this comment.
setting isOmniInstall to true so that we can be redirected back to /apps/installation/[step]?slug
if not set it redirects to apps/installed/[category] and the redirection can't be controlled
| @@ -7,9 +7,6 @@ export function decodeOAuthState(req: NextApiRequest) { | |||
| return undefined; | |||
| } | |||
| const state: IntegrationOAuthCallbackState = JSON.parse(req.query.state); | |||
| if (state.appOnboardingRedirectUrl) { | |||
There was a problem hiding this comment.
no longer needed as we now use returnTo only
| @@ -225,7 +225,6 @@ export const AppPage = ({ | |||
| multiInstall | |||
| concurrentMeetings={concurrentMeetings} | |||
| paid={paid} | |||
| dirName={dirName} | |||
There was a problem hiding this comment.
If we're removing the use of the dirName, let's remove the prop from the component as well.
| return ( | ||
| <Dropdown> |
There was a problem hiding this comment.
I love to see this gone.
There was a problem hiding this comment.
Let's try and completely remove the need for userAdminTeams prop here. Ideally we should be using the new app install flow.
There was a problem hiding this comment.
I would argue the aim of this PR should be to get rid of this function. The new app install flow handles this in better way. I see that this function is only used for apps so the rest of the web app doesn't rely on it.
Anything that relied on this should be handled in the new app install flow.
There was a problem hiding this comment.
Not sure if we can remove it completly
we need this function for -
-
fetching the credentials for an app for the current logged in user and all his teams LINK, for this we need the list of all the teams the user is admin/owner of
-
this credentials are used to check weather this app has been added or not (the green badge that says 2 installed)
-
weather to show the install button or not here for instance i have installed fathom on both my personal acc and the only team i have so we dont show the install button
-
also getUserAdminTeams is also called in
throwIfNotHaveAdminAccessToTeamwhich is a check that prevents non-admin/owner to install an app to a team
so we do need the list of teams the logged in user is admin/owner of
plus now that we are querying it using the primary key, it shouldnt be that bad!!
SomayChauhan
changed the title
fixes CAL-3604