Skip to content

Devin/feature opt in system 1765208720 #1

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.

Sign up for GitHub

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

Open
ayushjrathod-dd wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Devin/feature opt in system 1765208720 #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d65ccd3
feat: implement feature opt-in system
devin-ai-integration[bot] Dec 8, 2025
3c54b1f
refactor: remove enabled column and use row-existence semantics
devin-ai-integration[bot] Dec 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions apps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ const getTabs = (orgBranding: OrganizationBranding | null) => {
{ name: "appearance", href: "/settings/my-account/appearance", trackingMetadata: { section: "my_account", page: "appearance" } },
{ name: "out_of_office", href: "/settings/my-account/out-of-office", trackingMetadata: { section: "my_account", page: "out_of_office" } },
{ name: "push_notifications", href: "/settings/my-account/push-notifications", trackingMetadata: { section: "my_account", page: "push_notifications" } },
{ name: "features", href: "/settings/my-account/features", trackingMetadata: { section: "my_account", page: "features" } },
// TODO
// { name: "referrals", href: "/settings/my-account/referrals" },
],
Expand Down Expand Up @@ -91,6 +92,11 @@ const getTabs = (orgBranding: OrganizationBranding | null) => {
href: "/settings/organizations/general",
trackingMetadata: { section: "organization", page: "general" },
},
{
name: "features",
href: "/settings/organizations/features",
trackingMetadata: { section: "organization", page: "features" },
},
{
name: "guest_notifications",
href: "/settings/organizations/guest-notifications",
Expand Down Expand Up @@ -531,6 +537,14 @@ const TeamListCollapsible = ({ teamFeatures }: { teamFeatures?: Record<number, T
className="px-2! me-5 h-7 w-auto"
disableChevron
/>
<VerticalTabItem
name={t("features")}
href={`/settings/teams/${team.id}/features`}
textClassNames="px-3 text-emphasis font-medium text-sm"
trackingMetadata={{ section: "team", page: "features", teamId: team.id }}
className="px-2! me-5 h-7 w-auto"
disableChevron
/>
</>
)}
</CollapsibleContent>
Expand Down
32 changes: 32 additions & 0 deletions apps/web/app/(use-page-wrapper)/settings/(settings-layout)/my-account/features/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import { _generateMetadata } from "app/_utils";
import { headers, cookies } from "next/headers";
import { redirect } from "next/navigation";

import { getServerSession } from "@calcom/features/auth/lib/getServerSession";

import { buildLegacyRequest } from "@lib/buildLegacyCtx";

import FeaturesView from "~/settings/my-account/features-view";

export const generateMetadata = async () =>
await _generateMetadata(
(t) => t("features"),
(t) => t("features_description"),
undefined,
undefined,
"/settings/my-account/features"
);

const Page = async () => {
const session = await getServerSession({ req: buildLegacyRequest(await headers(), await cookies()) });
const userId = session?.user?.id;
const redirectUrl = "/auth/login?callbackUrl=/settings/my-account/features";

if (!userId) {
return redirect(redirectUrl);
}

return <FeaturesView />;
};

export default Page;
57 changes: 57 additions & 0 deletions apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/features/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
import { _generateMetadata, getTranslate } from "app/_utils";

import { Resource } from "@calcom/features/pbac/domain/types/permission-registry";
import { getResourcePermissions } from "@calcom/features/pbac/lib/resource-permissions";
import SettingsHeader from "@calcom/features/settings/appDir/SettingsHeader";
import { MembershipRole } from "@calcom/prisma/enums";

import { validateUserHasOrg } from "../actions/validateUserHasOrg";

import OrganizationFeaturesView from "~/settings/organizations/organization-features-view";

export const generateMetadata = async () =>
await _generateMetadata(
(t) => t("features"),
(t) => t("organization_features_description"),
undefined,
undefined,
"/settings/organizations/features"
);

const Page = async () => {
const t = await getTranslate();

const session = await validateUserHasOrg();

const { canRead } = await getResourcePermissions({
userId: session.user.id,
teamId: session.user.profile.organizationId,
resource: Resource.Feature,
userRole: session.user.org.role,
fallbackRoles: {
read: {
roles: [MembershipRole.MEMBER, MembershipRole.ADMIN, MembershipRole.OWNER],
},
update: {
roles: [MembershipRole.ADMIN, MembershipRole.OWNER],
},
},
});

if (!canRead) {
return (
<SettingsHeader
title={t("features")}
description={t("organization_features_description")}
borderInShellHeader={true}>
<div className="border-subtle border-x px-4 py-8 sm:px-6">
<p className="text-subtle text-sm">{t("no_permission_to_view")}</p>
</div>
</SettingsHeader>
);
}

return <OrganizationFeaturesView organizationId={session.user.profile.organizationId} />;
};

export default Page;
18 changes: 18 additions & 0 deletions apps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/features/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { _generateMetadata } from "app/_utils";

import TeamFeaturesView from "~/settings/teams/team-features-view";

export const generateMetadata = async ({ params }: { params: Promise<{ id: string }> }) =>
await _generateMetadata(
(t) => t("features"),
(t) => t("team_features_description"),
undefined,
undefined,
`/settings/teams/${(await params).id}/features`
);

const Page = async () => {
return <TeamFeaturesView />;
};
Comment on lines +14 to +16
Copy link

@refacto-visz refacto-visz bot Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing Page Authorization Check

The team features page directly renders without verifying user permissions for the specified team. This inconsistency with the organization features page allows unauthorized access to team feature management UI, potentially exposing sensitive configuration information and enabling unauthorized modification attempts.

const Page = async ({ params }: { params: Promise<{ id: string }> }) => {
  const { id } = await params;
  const teamId = parseInt(id, 10);
  
  if (isNaN(teamId)) {
    throw new Error('Invalid team ID');
  }
  
  return <TeamFeaturesView teamId={teamId} />;
};
Commitable Suggestion
Suggested change
const Page = async () => {
return <TeamFeaturesView />;
};
const Page = async ({ params }: { params: Promise<{ id: string }> }) => {
const { id } = await params;
const teamId = parseInt(id, 10);
if (isNaN(teamId)) {
throw new Error('Invalid team ID');
}
return <TeamFeaturesView teamId={teamId} />;
};
Standards
  • Business-Rule-Validation

Comment on lines +14 to +16
Copy link

@refacto-visz refacto-visz bot Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing Feature Permission Check

The page for managing team features does not verify if the current user has the specific permission (Resource.Feature) to view or manage features for the team. This is inconsistent with the equivalent organization features page, which correctly performs this check. This could allow a team member to view the feature settings even if their role does not grant them this permission, creating a gap in access control.

Standards
  • Business-Rule-Input-Validation
  • Logic-Verification-Data-Integrity

Comment on lines +14 to +16
Copy link

@refacto-visz refacto-visz bot Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing Permission Check

The new Team Features page does not perform a specific permission check for the Feature resource, unlike the corresponding Organization Features page which explicitly checks for canRead permissions. This inconsistency introduces a potential security vulnerability, as it might allow users to access feature management settings for a team without the required authorization. An explicit PBAC check should be added to ensure consistent and secure access control.

Standards
  • Business-Rule-Input-Validation


export default Page;
81 changes: 81 additions & 0 deletions apps/web/modules/settings/my-account/features-view.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
"use client";

import SettingsHeader from "@calcom/features/settings/appDir/SettingsHeader";
import { useLocale } from "@calcom/lib/hooks/useLocale";
import { trpc } from "@calcom/trpc/react";
import { SettingsToggle } from "@calcom/ui/components/form";
import { showToast } from "@calcom/ui/components/toast";
import { SkeletonContainer, SkeletonText } from "@calcom/ui/components/skeleton";

const SkeletonLoader = () => {
return (
<SkeletonContainer>
<div className="border-subtle space-y-6 border-x px-4 py-8 sm:px-6">
<SkeletonText className="h-8 w-full" />
<SkeletonText className="h-8 w-full" />
<SkeletonText className="h-8 w-full" />
</div>
</SkeletonContainer>
);
};

const FeaturesView = () => {
const { t } = useLocale();
const utils = trpc.useUtils();

const { data: features, isLoading } = trpc.viewer.featureManagement.listForUser.useQuery();

const setFeatureEnabledMutation = trpc.viewer.featureManagement.setUserFeatureEnabled.useMutation({
Comment on lines +22 to +28
Copy link

@refacto-visz refacto-visz bot Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated Feature View Components

The feature view components for user, team, and organization are nearly identical, sharing the same skeleton loader, layout structure, and rendering logic. This code duplication increases maintenance effort as UI changes must be replicated across multiple files, violating DRY principles and increasing the risk of inconsistent behavior.

Standards
  • Clean-Code-DRY

onSuccess: () => {
utils.viewer.featureManagement.listForUser.invalidate();
showToast(t("settings_updated_successfully"), "success");
},
onError: () => {
showToast(t("error_updating_settings"), "error");
},
});
Comment on lines +22 to +36
Copy link

@refacto-visz refacto-visz bot Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated Feature Management UI

The FeaturesView component is nearly identical to OrganizationFeaturesView and TeamFeaturesView. All three components share the same structure, skeleton loader, and rendering logic, leading to significant code duplication. This violates the DRY principle and increases maintenance overhead, as future UI changes will need to be replicated in three places. A single generic component should be created that accepts the context (e.g., 'user', 'team', or 'organization') and an optional ID as props to dynamically use the appropriate tRPC procedures.

Standards
  • Clean-Code-DRY
  • Maintainability-Quality-Duplication


if (isLoading) {
return (
<SettingsHeader
title={t("features")}
description={t("features_description")}
borderInShellHeader={true}>
<SkeletonLoader />
</SettingsHeader>
);
}

const userControlledFeatures = features?.filter((f) => f.globallyEnabled) ?? [];

return (
<SettingsHeader title={t("features")} description={t("features_description")} borderInShellHeader={true}>
<div className="border-subtle border-x px-4 py-8 sm:px-6">
{userControlledFeatures.length === 0 ? (
<p className="text-subtle text-sm">{t("no_features_available")}</p>
) : (
<div className="space-y-6">
{userControlledFeatures.map((feature) => (
<SettingsToggle
key={feature.slug}
toggleSwitchAtTheEnd={true}
title={feature.slug}
Copy link

@gemini-code-assist gemini-code-assist bot Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The title prop for SettingsToggle currently uses feature.slug. While slug is a unique identifier, it's often a technical string and might not be user-friendly or translatable for display purposes. For consistency with FeatureOptInBanner, which uses feature.titleI18nKey, consider introducing an i18n key for the feature title in the Feature model or mapping the slug to a more descriptive, translatable title.

Copy link

@refacto-visz refacto-visz bot Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feature Slug Used as UI Label

The feature's internal slug is used directly as the display title in the settings UI. This couples the internal identifier with the user-facing presentation, which is not user-friendly and is brittle against changes to the slug. A human-readable name, preferably managed via i18n keys, should be used instead to improve user experience and decouple the UI from internal implementation details.

Standards
  • Maintainability-Quality-Decoupling

description={feature.description || t("no_description_available")}
Copy link

@gemini-code-assist gemini-code-assist bot Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The description prop for SettingsToggle uses feature.description. If feature.description is intended to be a user-facing string, it should ideally be an i18n key to support localization. Currently, it appears to be a direct string from the database, which might not be translatable. For better internationalization, consider using an i18n key for feature descriptions, similar to feature.descriptionI18nKey used in EligibleOptInFeature for the opt-in banner.

disabled={setFeatureEnabledMutation.isPending}
checked={feature.enabled}
onCheckedChange={(checked) => {
setFeatureEnabledMutation.mutate({
featureSlug: feature.slug,
enabled: checked,
});
}}
/>
))}
</div>
)}
</div>
</SettingsHeader>
);
};
Comment on lines +22 to +79
Copy link

@refacto-visz refacto-visz bot Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated UI View Logic

The three new view components contain nearly identical logic for fetching data, handling loading states, and rendering feature toggles. This duplication violates the DRY principle and increases future maintenance effort, as any change will need to be applied in three places. A single, reusable component should be created that can be parameterized with the appropriate tRPC hooks and entity ID to handle all three use cases.

Standards
  • Clean-Code-DRY

Comment on lines +22 to +79
Copy link

@refacto-visz refacto-visz bot Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated UI View Logic

The three new feature view components (FeaturesView, OrganizationFeaturesView, TeamFeaturesView) share nearly identical structure and logic, including skeleton loaders, headers, and toggle rendering. This duplication violates the DRY principle, increasing maintenance effort. A single, reusable component should be created to encapsulate the common UI and accept entity-specific data-fetching logic (e.g., tRPC hooks) and identifiers as props.

Standards
  • Clean-Code-DRY
  • Maintainability-Quality-Reusability


export default FeaturesView;
Comment on lines +22 to +81
Copy link

@refacto-visz refacto-visz bot Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated Feature View Logic

The components FeaturesView, OrganizationFeaturesView, and TeamFeaturesView contain nearly identical structure and logic for fetching, loading, and displaying feature toggles. This duplication violates the DRY principle, increasing maintenance overhead as any UI or logic change must be replicated in three places. A single, generic component should be created that accepts the specific tRPC hooks and entity IDs as props to promote reusability and simplify future modifications.

Standards
  • Maintainability-Quality-Reusability
  • Clean-Code-DRY

Comment on lines +1 to +81
Copy link

@refacto-visz refacto-visz bot Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate UI Component Logic

The three new feature view components for 'My Account', 'Organization', and 'Team' contain nearly identical code for rendering feature toggles, including skeleton loaders and mutation logic. This duplication violates the DRY principle, increasing future maintenance costs as changes must be manually synchronized across all three files. Consolidating this into a single, generic component that accepts entity-specific data (like IDs and tRPC hooks) would significantly improve code reusability and maintainability.

Standards
  • Clean-Code-Duplication
  • Maintainability-Quality-DRY

91 changes: 91 additions & 0 deletions apps/web/modules/settings/organizations/organization-features-view.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
"use client";

import SettingsHeader from "@calcom/features/settings/appDir/SettingsHeader";
import { useLocale } from "@calcom/lib/hooks/useLocale";
import { trpc } from "@calcom/trpc/react";
import { SettingsToggle } from "@calcom/ui/components/form";
import { showToast } from "@calcom/ui/components/toast";
import { SkeletonContainer, SkeletonText } from "@calcom/ui/components/skeleton";

interface OrganizationFeaturesViewProps {
organizationId: number;
}

const SkeletonLoader = () => {
return (
<SkeletonContainer>
<div className="border-subtle space-y-6 border-x px-4 py-8 sm:px-6">
<SkeletonText className="h-8 w-full" />
<SkeletonText className="h-8 w-full" />
<SkeletonText className="h-8 w-full" />
</div>
</SkeletonContainer>
);
};

const OrganizationFeaturesView = ({ organizationId }: OrganizationFeaturesViewProps) => {
const { t } = useLocale();
const utils = trpc.useUtils();

const { data: features, isLoading } = trpc.viewer.featureManagement.listForOrganization.useQuery({
organizationId,
});

const setFeatureEnabledMutation = trpc.viewer.featureManagement.setOrganizationFeatureEnabled.useMutation({
onSuccess: () => {
utils.viewer.featureManagement.listForOrganization.invalidate({ organizationId });
showToast(t("settings_updated_successfully"), "success");
},
onError: () => {
showToast(t("error_updating_settings"), "error");
},
});

if (isLoading) {
return (
<SettingsHeader
title={t("features")}
description={t("organization_features_description")}
borderInShellHeader={true}>
<SkeletonLoader />
</SettingsHeader>
);
}

const orgControlledFeatures = features?.filter((f) => f.globallyEnabled) ?? [];

return (
<SettingsHeader
title={t("features")}
description={t("organization_features_description")}
borderInShellHeader={true}>
<div className="border-subtle border-x px-4 py-8 sm:px-6">
{orgControlledFeatures.length === 0 ? (
<p className="text-subtle text-sm">{t("no_features_available")}</p>
) : (
<div className="space-y-6">
{orgControlledFeatures.map((feature) => (
<SettingsToggle
key={feature.slug}
toggleSwitchAtTheEnd={true}
title={feature.slug}
Copy link

@gemini-code-assist gemini-code-assist bot Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similar to features-view.tsx, using feature.slug directly as the title for SettingsToggle might not be user-friendly or translatable. It's recommended to use an i18n key for the feature title to ensure consistency and proper localization across the application.

description={feature.description || t("no_description_available")}
Copy link

@gemini-code-assist gemini-code-assist bot Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The description prop for SettingsToggle uses feature.description. For improved localization, consider using an i18n key for feature descriptions. This would align with the approach taken in FeatureOptInBanner and provide a more consistent user experience.

disabled={setFeatureEnabledMutation.isPending}
checked={feature.enabled}
onCheckedChange={(checked) => {
setFeatureEnabledMutation.mutate({
organizationId,
featureSlug: feature.slug,
enabled: checked,
});
}}
/>
))}
</div>
)}
</div>
</SettingsHeader>
);
};

export default OrganizationFeaturesView;
Loading