-
Notifications
You must be signed in to change notification settings - Fork 0
fix: enable DI for FeatureOptInService #24
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
base: coderabbit_combined_20260121_augment_sentry_coderabbit_1_base_fix_enable_di_for_featureoptinservice_pr696
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| import type { IFeatureOptInService } from "@calcom/features/feature-opt-in/services/IFeatureOptInService"; | ||
|
|
||
| import { createContainer } from "../di"; | ||
| import { moduleLoader as featureOptInServiceModuleLoader } from "../modules/FeatureOptInService"; | ||
|
|
||
| export function getFeatureOptInService(): IFeatureOptInService { | ||
| const featureOptInServiceContainer = createContainer(); | ||
| featureOptInServiceModuleLoader.loadModule(featureOptInServiceContainer); | ||
| return featureOptInServiceContainer.get<IFeatureOptInService>(featureOptInServiceModuleLoader.token); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| import { createContainer } from "../di"; | ||
| import { type FeaturesRepository, moduleLoader as featuresRepositoryModuleLoader } from "../modules/FeaturesRepository"; | ||
|
|
||
| export function getFeaturesRepository(): FeaturesRepository { | ||
| const featuresRepositoryContainer = createContainer(); | ||
| featuresRepositoryModuleLoader.loadModule(featuresRepositoryContainer); | ||
| return featuresRepositoryContainer.get<FeaturesRepository>(featuresRepositoryModuleLoader.token); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| import { FEATURE_OPT_IN_DI_TOKENS } from "@calcom/features/feature-opt-in/di/tokens"; | ||
| import { FeatureOptInService } from "@calcom/features/feature-opt-in/services/FeatureOptInService"; | ||
|
|
||
| import { bindModuleToClassOnToken, createModule, type ModuleLoader } from "../di"; | ||
| import { moduleLoader as featuresRepositoryModuleLoader } from "./FeaturesRepository"; | ||
|
|
||
| const thisModule = createModule(); | ||
| const token = FEATURE_OPT_IN_DI_TOKENS.FEATURE_OPT_IN_SERVICE; | ||
| const moduleToken = FEATURE_OPT_IN_DI_TOKENS.FEATURE_OPT_IN_SERVICE_MODULE; | ||
|
|
||
| const loadModule = bindModuleToClassOnToken({ | ||
| module: thisModule, | ||
| moduleToken, | ||
| token, | ||
| classs: FeatureOptInService, | ||
| dep: featuresRepositoryModuleLoader, | ||
| }); | ||
|
|
||
| export const moduleLoader: ModuleLoader = { | ||
| token, | ||
| loadModule, | ||
| }; | ||
|
|
||
| export type { FeatureOptInService }; |
This file was deleted.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,24 @@ | ||||||||||||||||||
| import { FLAGS_DI_TOKENS } from "@calcom/features/flags/di/tokens"; | ||||||||||||||||||
| import { FeaturesRepository } from "@calcom/features/flags/features.repository"; | ||||||||||||||||||
|
|
||||||||||||||||||
| import { bindModuleToClassOnToken, createModule, type ModuleLoader } from "../di"; | ||||||||||||||||||
| import { moduleLoader as prismaModuleLoader } from "./Prisma"; | ||||||||||||||||||
|
|
||||||||||||||||||
| export const featuresRepositoryModule = createModule(); | ||||||||||||||||||
| const token = FLAGS_DI_TOKENS.FEATURES_REPOSITORY; | ||||||||||||||||||
| const moduleToken = FLAGS_DI_TOKENS.FEATURES_REPOSITORY_MODULE; | ||||||||||||||||||
|
|
||||||||||||||||||
| const loadModule = bindModuleToClassOnToken({ | ||||||||||||||||||
| module: featuresRepositoryModule, | ||||||||||||||||||
| moduleToken, | ||||||||||||||||||
| token, | ||||||||||||||||||
| classs: FeaturesRepository, | ||||||||||||||||||
| dep: prismaModuleLoader, | ||||||||||||||||||
| }); | ||||||||||||||||||
|
|
||||||||||||||||||
| export const moduleLoader: ModuleLoader = { | ||||||||||||||||||
| token: moduleToken, | ||||||||||||||||||
| loadModule, | ||||||||||||||||||
| }; | ||||||||||||||||||
|
Comment on lines
+19
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: The Compare with 🐛 Proposed fix export const moduleLoader: ModuleLoader = {
- token: moduleToken,
+ token,
loadModule,
};📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
|
|
||||||||||||||||||
| export type { FeaturesRepository }; | ||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| export const FEATURE_OPT_IN_DI_TOKENS = { | ||
| FEATURE_OPT_IN_SERVICE: Symbol("FeatureOptInService"), | ||
| FEATURE_OPT_IN_SERVICE_MODULE: Symbol("FeatureOptInServiceModule"), | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,12 @@ | ||
| import { afterEach, describe, expect, it } from "vitest"; | ||
|
|
||
| import { getFeatureOptInService } from "@calcom/features/di/containers/FeatureOptInService"; | ||
| import { getFeaturesRepository } from "@calcom/features/di/containers/FeaturesRepository"; | ||
| import type { FeatureId } from "@calcom/features/flags/config"; | ||
| import { FeaturesRepository } from "@calcom/features/flags/features.repository"; | ||
| import type { FeaturesRepository } from "@calcom/features/flags/features.repository"; | ||
| import { prisma } from "@calcom/prisma"; | ||
|
|
||
| import { FeatureOptInService } from "./FeatureOptInService"; | ||
| import type { IFeatureOptInService } from "./IFeatureOptInService"; | ||
|
|
||
| // Helper to generate unique identifiers per test | ||
| const uniqueId = () => `${Date.now()}-${Math.random().toString(36).slice(2, 9)}`; | ||
|
|
@@ -30,7 +32,7 @@ interface TestEntities { | |
| team: { id: number }; | ||
| team2: { id: number }; | ||
| featuresRepository: FeaturesRepository; | ||
| service: FeatureOptInService; | ||
| service: IFeatureOptInService; | ||
| createdFeatures: string[]; | ||
| setupFeature: (enabled?: boolean) => Promise<FeatureId>; | ||
| } | ||
|
|
@@ -77,8 +79,8 @@ async function setup(): Promise<TestEntities> { | |
| }, | ||
| }); | ||
|
|
||
| const featuresRepository = new FeaturesRepository(prisma); | ||
| const service = new FeatureOptInService(featuresRepository); | ||
| const featuresRepository = getFeaturesRepository(); | ||
| const service = getFeatureOptInService(); | ||
|
|
||
|
Comment on lines
+82
to
84
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure service and repository share the same container. Consider resolving both from a single container in 🔧 Example approach (shared container)- const featuresRepository = getFeaturesRepository();
- const service = getFeatureOptInService();
+ // Resolve both from a single DI container so caches are shared
+ const container = createContainer();
+ featureOptInServiceModuleLoader.loadModule(container);
+ featuresRepositoryModuleLoader.loadModule(container);
+ const featuresRepository = container.get<FeaturesRepository>(featuresRepositoryModuleLoader.token);
+ const service = container.get<IFeatureOptInService>(featureOptInServiceModuleLoader.token);🤖 Prompt for AI Agents |
||
| // Helper to create a feature for a test and track it for cleanup | ||
| const setupFeature = async (enabled = true): Promise<FeatureId> => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| import type { FeatureId, FeatureState } from "@calcom/features/flags/config"; | ||
|
|
||
| export type ResolvedFeatureState = { | ||
| featureId: FeatureId; | ||
| globalEnabled: boolean; | ||
| orgState: FeatureState; // Raw state (before auto-opt-in transform) | ||
| teamStates: FeatureState[]; // Raw states | ||
| userState: FeatureState | undefined; // Raw state | ||
| effectiveEnabled: boolean; | ||
| // Auto-opt-in flags for UI to show checkbox state | ||
| orgAutoOptIn: boolean; | ||
| teamAutoOptIns: boolean[]; | ||
| userAutoOptIn: boolean; | ||
| }; | ||
|
|
||
| export interface IFeatureOptInService { | ||
| resolveFeatureStatesAcrossTeams(input: { | ||
| userId: number; | ||
| orgId: number | null; | ||
| teamIds: number[]; | ||
| featureIds: FeatureId[]; | ||
| }): Promise<Record<string, ResolvedFeatureState>>; | ||
| listFeaturesForUser(input: { userId: number; orgId: number | null; teamIds: number[] }): Promise< | ||
| ResolvedFeatureState[] | ||
| >; | ||
| listFeaturesForTeam( | ||
| input: { teamId: number } | ||
| ): Promise<{ featureId: FeatureId; globalEnabled: boolean; teamState: FeatureState }[]>; | ||
| setUserFeatureState( | ||
| input: | ||
| | { userId: number; featureId: FeatureId; state: "enabled" | "disabled"; assignedBy: number } | ||
| | { userId: number; featureId: FeatureId; state: "inherit" } | ||
| ): Promise<void>; | ||
| setTeamFeatureState( | ||
| input: | ||
| | { teamId: number; featureId: FeatureId; state: "enabled" | "disabled"; assignedBy: number } | ||
| | { teamId: number; featureId: FeatureId; state: "inherit" } | ||
| ): Promise<void>; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| export const FLAGS_DI_TOKENS = { | ||
| FEATURES_REPOSITORY: Symbol("FeaturesRepository"), | ||
| FEATURES_REPOSITORY_MODULE: Symbol("FeaturesRepositoryModule"), | ||
| }; |
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.
🧩 Analysis chain
🏁 Script executed:
Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 68
🏁 Script executed:
Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 599
🏁 Script executed:
Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 190
🏁 Script executed:
Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 1010
🏁 Script executed:
Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 306
🏁 Script executed:
Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 239
🏁 Script executed:
Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 3820
🏁 Script executed:
Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 150
🏁 Script executed:
Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 728
🏁 Script executed:
Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 786
🏁 Script executed:
Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 1721
🏁 Script executed:
Repository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 1861
Change
moduleLoader.tokentotokenin the FeaturesRepository module.Line 20 in
packages/features/di/modules/FeaturesRepository.tssetstoken: moduleToken, but should settoken: token. ThebindModuleToClassOnTokenfunction registers the FeaturesRepository class with thetokenparameter, notmoduleToken. UsingmoduleTokenin the container helper will cause a runtime error when attempting to retrieve the repository, since the container will look for the wrong token. All other modules in this directory correctly usetokenin theirmoduleLoaderexports.🤖 Prompt for AI Agents