diff --git a/packages/emails/templates/organizer-request-email.ts b/packages/emails/templates/organizer-request-email.ts index b95501262a..424a6e141f 100644 --- a/packages/emails/templates/organizer-request-email.ts +++ b/packages/emails/templates/organizer-request-email.ts @@ -1,4 +1,4 @@ -import { FeaturesRepository } from "@calcom/features/flags/features.repository"; +import { checkIfUserHasFeatureController } from "@calcom/features/flags/operations/check-if-user-has-feature.controller"; import { EMAIL_FROM_NAME } from "@calcom/lib/constants"; import { renderEmail } from "../"; @@ -7,22 +7,15 @@ import OrganizerScheduledEmail from "./organizer-scheduled-email"; /** * TODO: Remove once fully migrated to V2 */ -async function getOrganizerRequestTemplate(args: { teamId?: number; userId?: number }) { - const featuresRepository = new FeaturesRepository(); - const hasNewTemplate = await featuresRepository.checkIfTeamOrUserHasFeature( - args, - "organizer-request-email-v2" - ); +async function getOrganizerRequestTemplate(userId?: number) { + const hasNewTemplate = await checkIfUserHasFeatureController(userId, "organizer-request-email-v2"); return hasNewTemplate ? ("OrganizerRequestEmailV2" as const) : ("OrganizerRequestEmail" as const); } export default class OrganizerRequestEmail extends OrganizerScheduledEmail { protected async getNodeMailerPayload(): Promise> { const toAddresses = [this.teamMember?.email || this.calEvent.organizer.email]; - const template = await getOrganizerRequestTemplate({ - userId: this.calEvent.organizer.id, - teamId: this.calEvent.team?.id, - }); + const template = await getOrganizerRequestTemplate(this.calEvent.organizer.id); return { from: `${EMAIL_FROM_NAME} <${this.getMailerOptions().from}>`, diff --git a/packages/features/flags/features.repository.interface.ts b/packages/features/flags/features.repository.interface.ts index 1447061cc8..f7119884aa 100644 --- a/packages/features/flags/features.repository.interface.ts +++ b/packages/features/flags/features.repository.interface.ts @@ -3,6 +3,4 @@ import type { AppFlags } from "./config"; export interface IFeaturesRepository { checkIfFeatureIsEnabledGlobally(slug: keyof AppFlags): Promise; checkIfUserHasFeature(userId: number, slug: string): Promise; - checkIfTeamHasFeature(teamId: number, slug: string): Promise; - checkIfTeamOrUserHasFeature(args: { teamId?: number; userId?: number }, slug: string): Promise; } diff --git a/packages/features/flags/features.repository.mock.ts b/packages/features/flags/features.repository.mock.ts new file mode 100644 index 0000000000..f6fa7f0440 --- /dev/null +++ b/packages/features/flags/features.repository.mock.ts @@ -0,0 +1,10 @@ +import type { IFeaturesRepository } from "./features.repository.interface"; + +export class MockFeaturesRepository implements IFeaturesRepository { + async checkIfUserHasFeature(userId: number, slug: string) { + return slug === "mock-feature"; + } + async checkIfFeatureIsEnabledGlobally() { + return true; + } +} diff --git a/packages/features/flags/features.repository.ts b/packages/features/flags/features.repository.ts index 5fca481bfa..aa03d2b571 100644 --- a/packages/features/flags/features.repository.ts +++ b/packages/features/flags/features.repository.ts @@ -15,51 +15,50 @@ export class FeaturesRepository implements IFeaturesRepository { throw err; } } - async checkIfTeamHasFeature(teamId: number, slug: string) { - try { - const teamFeature = await db.teamFeatures.findUnique({ - where: { - teamId_featureId: { teamId, featureId: slug }, - }, - }); - return !!teamFeature; - } catch (err) { - captureException(err); - throw err; - } - } - async checkIfUserHasFeature(userId: number, slug: string) { try { - const userFeature = await db.userFeatures.findUnique({ + /** + * findUnique was failing in prismock tests, so I'm using findFirst instead + * FIXME refactor when upgrading prismock + * https://github.com/morintd/prismock/issues/592 + */ + const userHasFeature = await db.userFeatures.findFirst({ where: { - userId_featureId: { userId, featureId: slug }, + userId, + featureId: slug, }, }); - return !!userFeature; + if (userHasFeature) return true; + // If the user doesn't have the feature, check if they belong to a team with the feature. + // This also covers organizations, which are teams. + const userBelongsToTeamWithFeature = await this.checkIfUserBelongsToTeamWithFeature(userId, slug); + if (userBelongsToTeamWithFeature) return true; + return false; } catch (err) { captureException(err); throw err; } } - - async checkIfTeamOrUserHasFeature( - args: { - teamId?: number; - userId?: number; - }, - slug: string - ) { - const { teamId, userId } = args; + private async checkIfUserBelongsToTeamWithFeature(userId: number, slug: string) { try { - if (teamId) { - const teamHasFeature = await this.checkIfTeamHasFeature(teamId, slug); - if (teamHasFeature) return true; - } - if (userId) { - const userHasFeature = await this.checkIfUserHasFeature(userId, slug); - if (userHasFeature) return true; - } + const user = await db.user.findUnique({ + where: { + id: userId, + teams: { + some: { + team: { + features: { + some: { + featureId: slug, + }, + }, + }, + }, + }, + }, + select: { id: true }, + }); + if (user) return true; return false; } catch (err) { captureException(err); diff --git a/packages/features/flags/operations/check-if-user-has-feature.controller.test.ts b/packages/features/flags/operations/check-if-user-has-feature.controller.test.ts new file mode 100644 index 0000000000..cafbb74980 --- /dev/null +++ b/packages/features/flags/operations/check-if-user-has-feature.controller.test.ts @@ -0,0 +1,23 @@ +import prismock from "../../../../tests/libs/__mocks__/prisma"; + +import { expect, it } from "vitest"; + +import { checkIfUserHasFeatureController } from "./check-if-user-has-feature.controller"; + +/** + * Since our current controller doesn't run any authentication checks or input validation, + * this test is identical to the test in the use case. + */ +it("checks if user has access to feature", async () => { + const userId = 1; + await prismock.userFeatures.create({ + data: { + userId, + featureId: "mock-feature", + assignedBy: "1", + updatedAt: new Date(), + }, + }); + await expect(checkIfUserHasFeatureController(userId, "nonexistent-feature")).resolves.toBe(false); + await expect(checkIfUserHasFeatureController(userId, "mock-feature")).resolves.toBe(true); +}); diff --git a/packages/features/flags/operations/check-if-user-has-feature.controller.ts b/packages/features/flags/operations/check-if-user-has-feature.controller.ts new file mode 100644 index 0000000000..cf96b5e99a --- /dev/null +++ b/packages/features/flags/operations/check-if-user-has-feature.controller.ts @@ -0,0 +1,32 @@ +import { startSpan } from "@sentry/nextjs"; + +import { checkIfUserHasFeatureUseCase } from "./check-if-user-has-feature.use-case"; + +/** + * Controllers use Presenters to convert the data to a UI-friendly format just before + * returning it to the "consumer". This helps us ship less JavaScript to the client (logic + * and libraries to convert the data), helps prevent leaking any sensitive properties, like + * emails or hashed passwords, and also helps us slim down the amount of data we're sending + * back to the client. + */ +function presenter(userHasFeature: boolean) { + return startSpan({ name: "checkIfUserHasFeature Presenter", op: "serialize" }, () => { + return userHasFeature; + }); +} + +/** + * Controllers perform authentication checks and input validation before passing the input + * to the specific use cases. Controllers orchestrate Use Cases. They don't implement any + * logic, but define the whole operations using use cases. + */ +export async function checkIfUserHasFeatureController( + userId: number | undefined, + slug: string +): Promise> { + return await startSpan({ name: "checkIfUserHasFeature Controller" }, async () => { + if (!userId) throw new Error("Missing userId in checkIfUserHasFeatureController"); + const userHasFeature = await checkIfUserHasFeatureUseCase(userId, slug); + return presenter(userHasFeature); + }); +} diff --git a/packages/features/flags/operations/check-if-user-has-feature.ts b/packages/features/flags/operations/check-if-user-has-feature.ts new file mode 100644 index 0000000000..018602382e --- /dev/null +++ b/packages/features/flags/operations/check-if-user-has-feature.ts @@ -0,0 +1,2 @@ +/** We export the controller since it's the main entry point for this operation in the app */ +export { checkIfUserHasFeatureController } from "./check-if-user-has-feature.controller"; diff --git a/packages/features/flags/operations/check-if-user-has-feature.use-case.test.ts b/packages/features/flags/operations/check-if-user-has-feature.use-case.test.ts new file mode 100644 index 0000000000..814255abf7 --- /dev/null +++ b/packages/features/flags/operations/check-if-user-has-feature.use-case.test.ts @@ -0,0 +1,21 @@ +import prismock from "../../../../tests/libs/__mocks__/prisma"; + +import { expect, it } from "vitest"; + +import { checkIfUserHasFeatureUseCase } from "./check-if-user-has-feature.use-case"; + +// This is identical to the test in the controller since the controller currently +// doesn't run any authentication checks or input validation. +it("returns if user has access to feature", async () => { + const userId = 1; + await prismock.userFeatures.create({ + data: { + userId, + featureId: "mock-feature", + assignedBy: "1", + updatedAt: new Date(), + }, + }); + await expect(checkIfUserHasFeatureUseCase(userId, "nonexistent-feature")).resolves.toBe(false); + await expect(checkIfUserHasFeatureUseCase(userId, "mock-feature")).resolves.toBe(true); +}); diff --git a/packages/features/flags/operations/check-if-user-has-feature.use-case.ts b/packages/features/flags/operations/check-if-user-has-feature.use-case.ts new file mode 100644 index 0000000000..8a8ab3ed9e --- /dev/null +++ b/packages/features/flags/operations/check-if-user-has-feature.use-case.ts @@ -0,0 +1,18 @@ +import { startSpan } from "@sentry/nextjs"; + +import { FeaturesRepository } from "../features.repository"; + +/** + * Use Cases represent individual operations, like "Create Feature" or "Sign In" or "Toggle Feature". + * Accept pre-validated input (from controllers) and handle authorization checks. + * Use Repositories and Services to access data sources and communicate with external systems. + * Use cases should not use other use cases. That's a code smell. It means the use case + * does multiple things and should be broken down into multiple use cases. + */ +export function checkIfUserHasFeatureUseCase(userId: number, slug: string): Promise { + return startSpan({ name: "checkIfUserHasFeature UseCase", op: "function" }, async () => { + const featuresRepository = new FeaturesRepository(); + + return await featuresRepository.checkIfUserHasFeature(userId, slug); + }); +} diff --git a/packages/prisma/migrations/20241119185538_add_index_for_user_and_team_features/migration.sql b/packages/prisma/migrations/20241119185538_add_index_for_user_and_team_features/migration.sql new file mode 100644 index 0000000000..b9d2189bbe --- /dev/null +++ b/packages/prisma/migrations/20241119185538_add_index_for_user_and_team_features/migration.sql @@ -0,0 +1,5 @@ +-- CreateIndex +CREATE INDEX "TeamFeatures_teamId_featureId_idx" ON "TeamFeatures"("teamId", "featureId"); + +-- CreateIndex +CREATE INDEX "UserFeatures_userId_featureId_idx" ON "UserFeatures"("userId", "featureId"); diff --git a/packages/prisma/schema.prisma b/packages/prisma/schema.prisma index 0d5116740e..ba4685cb6c 100644 --- a/packages/prisma/schema.prisma +++ b/packages/prisma/schema.prisma @@ -1236,6 +1236,7 @@ model UserFeatures { updatedAt DateTime @updatedAt @@id([userId, featureId]) + @@index([userId, featureId]) } model TeamFeatures { @@ -1248,6 +1249,7 @@ model TeamFeatures { updatedAt DateTime @updatedAt @@id([teamId, featureId]) + @@index([teamId, featureId]) } enum FeatureType {