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
This commit is contained in:
Eunjae Lee
@@ -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";
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
|
||||
@@ -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;
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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<ListFeaturesForUserResult[]> {
|
||||
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<ListFeaturesForTeamResult[]> {
|
||||
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(),
|
||||
|
||||
Reference in New Issue
Block a user