-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
refactor: integrations handler #16946
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
@@ -104,6 +104,16 @@ function getApps(credentials: CredentialDataWithTeamName[], filterOnCredentials? | |||
return apps; | |||
} | |||
|
|||
export function getAppsBySlug( |
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.
If we want to get a specific app instead of all, can be useful for v2 api.
import type { CredentialDataWithTeamName } from "@calcom/app-store/utils"; | ||
import type { PaymentApp } from "@calcom/types/PaymentService"; | ||
|
||
const checkAppSetupStatus = async ( |
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.
Helper function, abstracted the logic from the integrations handler.
|
||
import type { Credentials } from "./getTeamAppCredentials"; | ||
|
||
const constructUserTeams = async (credentials: Credentials, appSlug: string, userTeams: TeamQuery[]) => { |
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.
Helper function, abstracted the logic from the integrations handler.
|
||
import type { EnabledApp } from "./getEnabledAppsFromCredentials"; | ||
|
||
const getAppDependencyData = (enabledApps: EnabledApp[], dependencies?: string[] | undefined) => { |
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.
Helper function, abstracted the logic from the integrations handler.
@@ -42,6 +45,10 @@ const getEnabledAppsFromCredentials = async ( | |||
if (teamIds.length) filterOnIds.credentials.some.OR.push({ teamId: { in: teamIds } }); | |||
} | |||
|
|||
if (!!filterOnAppSlug) { |
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.
Gets a specific app based on slug instead of all.
invalid: boolean | null; | ||
}[]; | ||
|
||
const getTeamAppCredentials = ( |
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.
Helper function, abstracted the logic from the integrations handler.
import getUserTeamsQuery from "@calcom/lib/apps/getUserTeamsQuery"; | ||
import type { TeamQuery } from "@calcom/trpc/server/routers/loggedInViewer/integrations.handler"; | ||
|
||
const getUserAvailableTeams = async (userId: number, teamId?: number | null) => { |
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.
Helper function, abstracted the logic from the integrations handler.
import prisma from "@calcom/prisma"; | ||
import { credentialForCalendarServiceSelect } from "@calcom/prisma/selects/credential"; | ||
|
||
const getUserTeamsQuery = async (userId: number) => { |
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.
Another helper function.
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.
Feels better suited to be in a repository
let credentials = await getUsersCredentials(user); | ||
let userTeams: TeamQuery[] = []; | ||
|
||
if (includeTeamInstalledApps || teamId) { | ||
const teamsQuery = await prisma.team.findMany({ |
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 has been abstracted to getUserTeamsQuery
function.
}); | ||
// If a team is a part of an org then include those apps | ||
// Don't want to iterate over these parent teams | ||
const filteredTeams: TeamQuery[] = []; |
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 has been abstracted to getUserAvailableTeams
function.
|
||
userTeams = [...teamsQuery, ...parentTeams]; | ||
|
||
const teamAppCredentials: CredentialPayload[] = userTeams.flatMap((teamApp) => { |
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 has been abstracted to getTeamAppCredentials
function.
@@ -144,56 +69,21 @@ export const integrationsHandler = async ({ ctx, input }: IntegrationsOptions) = | |||
const invalidCredentialIds = credentials | |||
.filter((c) => c.appId === app.slug && c.invalid) | |||
.map((c) => c.id); | |||
const teams = await Promise.all( |
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 has been abstracted to constructUserTeams
function.
// type infer as CredentialOwner | ||
const credentialOwner: CredentialOwner = { | ||
name: user.name, | ||
avatar: user.avatar, | ||
}; | ||
|
||
// We need to know if app is payment type | ||
// undefined it means that app don't require app/setup/page | ||
let isSetupAlready = undefined; |
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 has been abstracted to checkAppSetupStatus
function.
} | ||
} | ||
|
||
let dependencyData: TDependencyData = []; |
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 has been abstracted to getAppDependencyData
function.
Graphite Automations"Add platform team as reviewer" took an action on this PR • (10/05/24)1 reviewer was added to this PR based on Keith Williams's automation. "Add foundation team as reviewer" took an action on this PR • (10/05/24)1 reviewer was added to this PR based on Keith Williams's automation. |
@@ -39,10 +36,6 @@ type TeamQuery = Prisma.TeamGetPayload<{ | |||
}; | |||
}>; | |||
|
|||
// type TeamQueryWithParent = TeamQuery & { |
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.
@joeauyeung removed this code which was commented but I don't if this is needed still or left there by mistake, should I bring it back?
@@ -205,6 +95,8 @@ export const integrationsHandler = async ({ ctx, input }: IntegrationsOptions) = | |||
isSetupAlready, | |||
...(app.dependencies && { dependencyData }), | |||
}; | |||
|
|||
return appData; | |||
}) | |||
); | |||
|
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.
Was thinking of shifting all the code below into a function of its own, but don't think it'll be really helpful since the logic is fairly simple to understand( just an if conditional ) and there's a lot of variables from input. What do you guys thinkabout this, should we split the code below into its own separate function?
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.
Probably, there's too many variables and if statements involved here not to.
|
||
const getTeamAppCredentials = ( | ||
userTeams: TeamQuery[], | ||
userCredentialIds: Credentials, |
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 this? Maybe merge outside of this function to keep this function dumber?
What does this PR do?
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
This can be tested in the webapp anywhere we have something related to integrations. The best and easiest I found is the app store.