From ea15474edf677b409f955840b344e571a0005fd8 Mon Sep 17 00:00:00 2001 From: Eunjae Lee Date: Thu, 29 Jan 2026 10:17:50 +0100 Subject: [PATCH] fix: show bookings-v3 only in the settings but not in the banner (#27330) ## What does this PR do? Adds a `displayLocations` property to the `OptInFeatureConfig` interface to control where opt-in features are displayed (settings page, banner, or both), and applies this filtering at the callers' side. ## How to test locally ``` update "Feature" set enabled = true where slug='bookings-v3'; ``` Run it to globally enable the flag, and then go to the settings page to see the "Features" menu. --- **Changes:** - Added `OptInFeatureDisplayLocation` type with values `"settings"` | `"banner"` - Added optional `displayLocations` property to `OptInFeatureConfig` interface - Added helper functions: - `getFeatureDisplayLocations()` - returns display locations with default of `['settings']` - `shouldDisplayFeatureAt()` - checks if a feature should display at a specific location - `getOptInFeaturesForLocation()` - filters features by location - Updated `getOptInFeaturesForScope()` to accept an optional `displayLocation` parameter for filtering - Simplified `HAS_*_OPT_IN_FEATURES` constants to use `getOptInFeaturesForScope(scope, "settings").length > 0` - Updated `FeatureOptInService.listFeaturesForUser()` to filter by 'settings' location - Updated `FeatureOptInService.listFeaturesForTeam()` to filter by 'settings' location - Updated `useFeatureOptInBanner` hook to check for 'banner' location before showing **Default behavior:** If `displayLocations` is omitted, features default to `['settings']` only. ## Mandatory Tasks (DO NOT REMOVE) - [x] I have self-reviewed the code (A decent size PR without self-review might be rejected). - [x] I have updated the developer docs in /docs if this PR makes changes that would require a [documentation change](https://cal.com/docs). N/A - internal config change only. - [x] I confirm automated tests are in place that prove my fix is effective or that my feature works. ## How should this be tested? 1. Verify the helper functions work as expected: - `getFeatureDisplayLocations({ slug: "test", ... })` should return `["settings"]` (default) - `getFeatureDisplayLocations({ slug: "test", displayLocations: ["banner"] })` should return `["banner"]` - `shouldDisplayFeatureAt(feature, "settings")` should return `true` for features without `displayLocations` - `getOptInFeaturesForScope("user", "banner")` should only return user-scoped features with `"banner"` in their `displayLocations` 2. Verify caller-side filtering: - `listFeaturesForUser()` and `listFeaturesForTeam()` should only return features with 'settings' in displayLocations - Banner hook should only show features with 'banner' in displayLocations - `HAS_*_OPT_IN_FEATURES` constants should only be true if there are features with 'settings' location ## Checklist - [x] My code follows the style guidelines of this project - [x] I have commented my code, particularly in hard-to-understand areas - [x] My changes generate no new warnings --- ### Human Review Checklist - [ ] Verify the default behavior (defaulting to `['settings']`) matches requirements - [ ] Confirm the filtering logic is applied correctly in all callers (service methods, banner hook, constants) - [ ] Verify the banner hook correctly prevents showing features without 'banner' in displayLocations - [ ] Note: The test mock returns all features regardless of `displayLocation` parameter - the filtering logic in `getOptInFeaturesForScope` isn't directly tested. Consider if this is acceptable or if tests should be added. **Link to Devin run:** https://app.devin.ai/sessions/a064ee43a56d458caf2892b55959f1ea **Requested by:** @eunjae-lee --- .../hooks/useFeatureOptInBanner.ts | 4 +- .../playwright/feature-opt-in-banner.e2e.ts | 5 +- packages/features/feature-opt-in/config.ts | 101 ++++++++++++------ .../services/FeatureOptInService.test.ts | 70 ++++++------ .../services/FeatureOptInService.ts | 10 +- 5 files changed, 113 insertions(+), 77 deletions(-) diff --git a/apps/web/modules/feature-opt-in/hooks/useFeatureOptInBanner.ts b/apps/web/modules/feature-opt-in/hooks/useFeatureOptInBanner.ts index 484eef4853..1e9050bed2 100644 --- a/apps/web/modules/feature-opt-in/hooks/useFeatureOptInBanner.ts +++ b/apps/web/modules/feature-opt-in/hooks/useFeatureOptInBanner.ts @@ -1,7 +1,7 @@ "use client"; import type { OptInFeatureConfig } from "@calcom/features/feature-opt-in/config"; -import { getOptInFeatureConfig } from "@calcom/features/feature-opt-in/config"; +import { getOptInFeatureConfig, shouldDisplayFeatureAt } from "@calcom/features/feature-opt-in/config"; import { trpc } from "@calcom/trpc/react"; import { useCallback, useMemo, useState } from "react"; @@ -122,6 +122,8 @@ function useFeatureOptInBanner(featureId: string): UseFeatureOptInBannerResult { if (isDismissed) return false; if (isOptedIn) return false; if (!featureConfig) return false; + // Only show banner if the feature is configured to be displayed as a banner + if (!shouldDisplayFeatureAt(featureConfig, "banner")) return false; if (eligibilityQuery.isLoading) return false; if (!eligibilityQuery.data) return false; return eligibilityQuery.data.status === "can_opt_in"; diff --git a/apps/web/playwright/feature-opt-in-banner.e2e.ts b/apps/web/playwright/feature-opt-in-banner.e2e.ts index 624b405c29..ec0c27be14 100644 --- a/apps/web/playwright/feature-opt-in-banner.e2e.ts +++ b/apps/web/playwright/feature-opt-in-banner.e2e.ts @@ -1,4 +1,4 @@ -import { OPT_IN_FEATURES } from "@calcom/features/feature-opt-in/config"; +import { getOptInFeatureConfig, shouldDisplayFeatureAt } from "@calcom/features/feature-opt-in/config"; import { expect } from "@playwright/test"; import { test } from "./lib/fixtures"; @@ -10,7 +10,8 @@ test.describe("Feature Opt-In Banner", () => { users, prisma, }) => { - if (!OPT_IN_FEATURES.some((item) => item.slug === "bookings-v3")) { + const featureConfig = getOptInFeatureConfig("bookings-v3"); + if (!featureConfig || !shouldDisplayFeatureAt(featureConfig, "banner")) { return; } diff --git a/packages/features/feature-opt-in/config.ts b/packages/features/feature-opt-in/config.ts index 170841cb1f..73429af75f 100644 --- a/packages/features/feature-opt-in/config.ts +++ b/packages/features/feature-opt-in/config.ts @@ -1,6 +1,10 @@ import type { FeatureId } from "@calcom/features/flags/config"; import type { OptInFeaturePolicy, OptInFeatureScope } from "./types"; +export type OptInFeatureDisplayLocation = "settings" | "banner"; + +const DEFAULT_DISPLAY_LOCATIONS: OptInFeatureDisplayLocation[] = ["settings"]; + export interface OptInFeatureConfig { slug: FeatureId; i18n: { @@ -16,32 +20,39 @@ export interface OptInFeatureConfig { policy: OptInFeaturePolicy; /** Scopes where this feature can be configured. Defaults to all scopes if not specified. */ scope?: OptInFeatureScope[]; + /** + * Where this feature should be displayed for opt-in. + * - 'settings': Show in the Settings page + * - 'banner': Show as a banner notification + * + * Defaults to ['settings'] if omitted. + * Use ['settings', 'banner'] for both locations. + * Use [] if you want the feature defined but not displayed anywhere. + */ + displayLocations?: OptInFeatureDisplayLocation[]; } -/** All available scopes for feature opt-in configuration */ -export const ALL_SCOPES: OptInFeatureScope[] = ["org", "team", "user"]; - /** * Features that appear in opt-in settings. * Add new features here to make them available for user/team opt-in. */ export const OPT_IN_FEATURES: OptInFeatureConfig[] = [ - // Example - to be populated with actual features - // { - // slug: "bookings-v3", - // i18n: { - // title: "bookings_v3_title", - // name: "bookings_v3_name", - // description: "bookings_v3_description", - // }, - // bannerImage: { - // src: "/opt_in_banner_bookings_v3.png", - // width: 548, - // height: 348, - // }, - // policy: "permissive", - // scope: ["org", "team", "user"], // Optional: defaults to all scopes if not specified - // }, + { + slug: "bookings-v3", + i18n: { + title: "bookings_v3_title", + name: "bookings_v3_name", + description: "bookings_v3_description", + }, + bannerImage: { + src: "/opt_in_banner_bookings_v3.png", + width: 548, + height: 348, + }, + policy: "permissive", + displayLocations: ["settings"], + scope: ["org", "team", "user"], // Optional: defaults to all scopes if not specified + }, ]; /** @@ -60,31 +71,51 @@ export function isOptInFeature(slug: string): slug is FeatureId { } /** - * Check if there are any opt-in features available. + * Check if a feature should be displayed at a specific location. + * Defaults to 'settings' if displayLocations is not specified. */ -export const HAS_OPT_IN_FEATURES: boolean = OPT_IN_FEATURES.length > 0; +export function shouldDisplayFeatureAt( + feature: OptInFeatureConfig, + location: OptInFeatureDisplayLocation +): boolean { + return (feature.displayLocations ?? DEFAULT_DISPLAY_LOCATIONS).includes(location); +} -/** Whether there are opt-in features available for the user scope */ -export const HAS_USER_OPT_IN_FEATURES: boolean = OPT_IN_FEATURES.some( - (f) => !f.scope || f.scope.includes("user") -); +/** + * Whether there are opt-in features available for the user scope in settings. + * Only counts features that should be displayed in settings (displayLocations includes 'settings' or is omitted). + */ +export const HAS_USER_OPT_IN_FEATURES: boolean = getOptInFeaturesForScope("user", "settings").length > 0; -/** Whether there are opt-in features available for the team scope */ -export const HAS_TEAM_OPT_IN_FEATURES: boolean = OPT_IN_FEATURES.some( - (f) => !f.scope || f.scope.includes("team") -); +/** + * Whether there are opt-in features available for the team scope in settings. + * Only counts features that should be displayed in settings (displayLocations includes 'settings' or is omitted). + */ +export const HAS_TEAM_OPT_IN_FEATURES: boolean = getOptInFeaturesForScope("team", "settings").length > 0; -/** Whether there are opt-in features available for the org scope */ -export const HAS_ORG_OPT_IN_FEATURES: boolean = OPT_IN_FEATURES.some( - (f) => !f.scope || f.scope.includes("org") -); +/** + * Whether there are opt-in features available for the org scope in settings. + * Only counts features that should be displayed in settings (displayLocations includes 'settings' or is omitted). + */ +export const HAS_ORG_OPT_IN_FEATURES: boolean = getOptInFeaturesForScope("org", "settings").length > 0; /** * Get opt-in features that are available for a specific scope. * Features without a scope field are available for all scopes. + * Optionally filter by display location (e.g., 'settings' or 'banner'). */ -export function getOptInFeaturesForScope(scope: OptInFeatureScope): OptInFeatureConfig[] { - return OPT_IN_FEATURES.filter((f) => !f.scope || f.scope.includes(scope)); +export function getOptInFeaturesForScope( + scope: OptInFeatureScope, + displayLocation?: OptInFeatureDisplayLocation +): OptInFeatureConfig[] { + return OPT_IN_FEATURES.filter((f) => { + const scopeMatches = !f.scope || f.scope.includes(scope); + if (!scopeMatches) return false; + if (displayLocation) { + return shouldDisplayFeatureAt(f, displayLocation); + } + return true; + }); } /** diff --git a/packages/features/feature-opt-in/services/FeatureOptInService.test.ts b/packages/features/feature-opt-in/services/FeatureOptInService.test.ts index 4ae35abfb9..cd41c7239e 100644 --- a/packages/features/feature-opt-in/services/FeatureOptInService.test.ts +++ b/packages/features/feature-opt-in/services/FeatureOptInService.test.ts @@ -15,41 +15,41 @@ const mockFindAllByUserId: Mock = vi.fn(); // Mock the OPT_IN_FEATURES config vi.mock("../config", () => { - const mockFeatures = [ - { - slug: "test-feature-1", - i18n: { title: "test_feature_1_title", name: "test_feature_1", description: "test_feature_1_desc" }, - policy: "permissive", - }, - { - slug: "test-feature-2", - i18n: { title: "test_feature_2_title", name: "test_feature_2", description: "test_feature_2_desc" }, - policy: "permissive", - }, - { - slug: "org-only-feature", - i18n: { title: "org_only_title", name: "org_only", description: "org_only_desc" }, - policy: "permissive", - scope: ["org"], - }, - { - slug: "team-only-feature", - i18n: { title: "team_only_title", name: "team_only", description: "team_only_desc" }, - policy: "permissive", - scope: ["team"], - }, - { - slug: "user-only-feature", - i18n: { title: "user_only_title", name: "user_only", description: "user_only_desc" }, - policy: "permissive", - scope: ["user"], - }, - { - slug: "strict-feature", - i18n: { title: "strict_feature_title", name: "strict_feature", description: "strict_feature_desc" }, - policy: "strict", - }, - ]; + const mockFeatures = [ + { + slug: "test-feature-1", + i18n: { title: "test_feature_1_title", name: "test_feature_1", description: "test_feature_1_desc" }, + policy: "permissive", + }, + { + slug: "test-feature-2", + i18n: { title: "test_feature_2_title", name: "test_feature_2", description: "test_feature_2_desc" }, + policy: "permissive", + }, + { + slug: "org-only-feature", + i18n: { title: "org_only_title", name: "org_only", description: "org_only_desc" }, + policy: "permissive", + scope: ["org"], + }, + { + slug: "team-only-feature", + i18n: { title: "team_only_title", name: "team_only", description: "team_only_desc" }, + policy: "permissive", + scope: ["team"], + }, + { + slug: "user-only-feature", + i18n: { title: "user_only_title", name: "user_only", description: "user_only_desc" }, + policy: "permissive", + scope: ["user"], + }, + { + slug: "strict-feature", + i18n: { title: "strict_feature_title", name: "strict_feature", description: "strict_feature_desc" }, + policy: "strict", + }, + ]; return { OPT_IN_FEATURES: mockFeatures, isOptInFeature: (slug: string) => mockFeatures.some((feature) => feature.slug === slug), diff --git a/packages/features/feature-opt-in/services/FeatureOptInService.ts b/packages/features/feature-opt-in/services/FeatureOptInService.ts index 6c91b44119..55844f0927 100644 --- a/packages/features/feature-opt-in/services/FeatureOptInService.ts +++ b/packages/features/feature-opt-in/services/FeatureOptInService.ts @@ -244,11 +244,12 @@ export class FeatureOptInService implements IFeatureOptInService { /** * List all opt-in features with their states for a user across teams. - * Only returns features that are in the allowlist, globally enabled, and scoped to "user". + * Only returns features that are in the allowlist, globally enabled, scoped to "user", + * and configured to be displayed in settings. */ async listFeaturesForUser(input: { userId: number }): Promise { const { userId } = input; - const userScopedFeatures = getOptInFeaturesForScope("user"); + const userScopedFeatures = getOptInFeaturesForScope("user", "settings"); const featureIds = userScopedFeatures.map((config) => config.slug); const resolvedStates = await this.resolveFeatureStates({ @@ -262,7 +263,8 @@ export class FeatureOptInService implements IFeatureOptInService { /** * List all opt-in features with their raw states for a team or organization. * Used for team/org admin settings page to configure feature opt-in. - * Only returns features that are in the allowlist, globally enabled, and scoped to the specified scope. + * Only returns features that are in the allowlist, globally enabled, scoped to the specified scope, + * and configured to be displayed in settings. * If parentOrgId is provided, also returns the organization state for each feature. */ async listFeaturesForTeam(input: { @@ -272,7 +274,7 @@ export class FeatureOptInService implements IFeatureOptInService { }): Promise { const { teamId, parentOrgId, scope = "team" } = input; const teamIdsToQuery = getTeamIdsToQuery(teamId, parentOrgId); - const scopedFeatures = getOptInFeaturesForScope(scope); + const scopedFeatures = getOptInFeaturesForScope(scope, "settings"); const [allFeatures, teamStates] = await Promise.all([ this.deps.featureRepo.findAll(),