From 8e46bda8e1d7629ac0adefc68ce3d4679b790f94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nuno=20G=C3=B3is?= Date: Thu, 24 Apr 2025 15:44:06 +0100 Subject: [PATCH] chore: fix admin routes should respect plan data (#9828) https://linear.app/unleash/issue/2-2852/sidebar-bug-with-enterprisepro-only-route-constraints Fixes an issue where admin routes didn't respect plan data if their flag was enabled. First noticed here: https://github.com/Unleash/unleash/pull/8469#discussion_r1804361222 Issue was that only `adminRoutes` respected plan data. `mainNavRoutes` and `primaryRoutes` did not follow the same filtering logic. We can probably clean this up even further in the future, but didn't want to extend the PR too much. Also adds tests to validate the intended behavior. --- .../component/admin/filterAdminRoutes.test.ts | 42 +++-- ...minRoutes.ts => filterRoutesByPlanData.ts} | 14 +- .../src/component/admin/useAdminRoutes.ts | 4 +- .../AdminMenu/AdminNavigationItems.tsx | 4 +- .../NavigationSidebar/useRoutes.test.ts | 170 ++++++++++++++++++ .../MainLayout/NavigationSidebar/useRoutes.ts | 41 ++++- 6 files changed, 240 insertions(+), 35 deletions(-) rename frontend/src/component/admin/{filterAdminRoutes.ts => filterRoutesByPlanData.ts} (75%) create mode 100644 frontend/src/component/layout/MainLayout/NavigationSidebar/useRoutes.test.ts diff --git a/frontend/src/component/admin/filterAdminRoutes.test.ts b/frontend/src/component/admin/filterAdminRoutes.test.ts index d7550a8c8e..2ee5cc4105 100644 --- a/frontend/src/component/admin/filterAdminRoutes.test.ts +++ b/frontend/src/component/admin/filterAdminRoutes.test.ts @@ -1,9 +1,9 @@ -import { filterAdminRoutes } from './filterAdminRoutes'; +import { filterRoutesByPlanData } from './filterRoutesByPlanData'; -describe('filterAdminRoutes - open souce routes', () => { +describe('filterRoutesByPlanData - open souce routes', () => { test('open source - should show menu item if mode paid plan mode is not defined', () => { expect( - filterAdminRoutes( + filterRoutesByPlanData( {}, { pro: false, @@ -21,12 +21,14 @@ describe('filterAdminRoutes - open souce routes', () => { billing: false, }; - expect(filterAdminRoutes({ mode: ['pro'] }, state)).toBe(false); - expect(filterAdminRoutes({ mode: ['enterprise'] }, state)).toBe(false); - expect(filterAdminRoutes({ mode: ['pro', 'enterprise'] }, state)).toBe( + expect(filterRoutesByPlanData({ mode: ['pro'] }, state)).toBe(false); + expect(filterRoutesByPlanData({ mode: ['enterprise'] }, state)).toBe( false, ); - expect(filterAdminRoutes({ billing: true }, state)).toBe(false); + expect( + filterRoutesByPlanData({ mode: ['pro', 'enterprise'] }, state), + ).toBe(false); + expect(filterRoutesByPlanData({ billing: true }, state)).toBe(false); }); test('pro - should show menu item for pro customers', () => { @@ -36,12 +38,14 @@ describe('filterAdminRoutes - open souce routes', () => { billing: false, }; - expect(filterAdminRoutes({ mode: ['pro'] }, state)).toBe(true); - expect(filterAdminRoutes({ mode: ['pro', 'enterprise'] }, state)).toBe( + expect(filterRoutesByPlanData({ mode: ['pro'] }, state)).toBe(true); + expect( + filterRoutesByPlanData({ mode: ['pro', 'enterprise'] }, state), + ).toBe(true); + // This is to show enterprise badge in pro mode + expect(filterRoutesByPlanData({ mode: ['enterprise'] }, state)).toBe( true, ); - // This is to show enterprise badge in pro mode - expect(filterAdminRoutes({ mode: ['enterprise'] }, state)).toBe(true); }); test('enterprise - should show menu item if mode enterprise is defined or mode is undefined', () => { @@ -51,16 +55,18 @@ describe('filterAdminRoutes - open souce routes', () => { billing: false, }; - expect(filterAdminRoutes({ mode: ['enterprise'] }, state)).toBe(true); - expect(filterAdminRoutes({ mode: ['pro', 'enterprise'] }, state)).toBe( + expect(filterRoutesByPlanData({ mode: ['enterprise'] }, state)).toBe( true, ); - expect(filterAdminRoutes({ mode: ['pro'] }, state)).toBe(false); + expect( + filterRoutesByPlanData({ mode: ['pro', 'enterprise'] }, state), + ).toBe(true); + expect(filterRoutesByPlanData({ mode: ['pro'] }, state)).toBe(false); }); test('billing - should show menu item if billing is defined', () => { expect( - filterAdminRoutes( + filterRoutesByPlanData( { mode: ['pro'], billing: true }, { pro: true, @@ -70,7 +76,7 @@ describe('filterAdminRoutes - open souce routes', () => { ), ).toBe(true); expect( - filterAdminRoutes( + filterRoutesByPlanData( { mode: ['enterprise'], billing: true }, { pro: false, @@ -80,7 +86,7 @@ describe('filterAdminRoutes - open souce routes', () => { ), ).toBe(true); expect( - filterAdminRoutes( + filterRoutesByPlanData( { mode: ['pro', 'enterprise'], billing: true }, { pro: true, @@ -90,7 +96,7 @@ describe('filterAdminRoutes - open souce routes', () => { ), ).toBe(true); expect( - filterAdminRoutes( + filterRoutesByPlanData( { mode: ['pro'], billing: true }, { pro: false, diff --git a/frontend/src/component/admin/filterAdminRoutes.ts b/frontend/src/component/admin/filterRoutesByPlanData.ts similarity index 75% rename from frontend/src/component/admin/filterAdminRoutes.ts rename to frontend/src/component/admin/filterRoutesByPlanData.ts index e34f056954..255cec4ac9 100644 --- a/frontend/src/component/admin/filterAdminRoutes.ts +++ b/frontend/src/component/admin/filterRoutesByPlanData.ts @@ -1,12 +1,14 @@ import type { INavigationMenuItem } from 'interfaces/route'; -export const filterAdminRoutes = ( +export type PlanData = { + enterprise: boolean; + pro: boolean; + billing: boolean; +}; + +export const filterRoutesByPlanData = ( menu: INavigationMenuItem['menu'], - { - pro, - enterprise, - billing, - }: { pro?: boolean; enterprise?: boolean; billing?: boolean }, + { pro, enterprise, billing }: PlanData, ): boolean => { const mode = menu?.mode; if (menu?.billing && !billing) return false; diff --git a/frontend/src/component/admin/useAdminRoutes.ts b/frontend/src/component/admin/useAdminRoutes.ts index 4b3ebb6545..002a4482e5 100644 --- a/frontend/src/component/admin/useAdminRoutes.ts +++ b/frontend/src/component/admin/useAdminRoutes.ts @@ -2,7 +2,7 @@ import useUiConfig from 'hooks/api/getters/useUiConfig/useUiConfig'; import { adminRoutes as oldAdminRoutes } from './oldAdminRoutes'; import { adminRoutes } from './adminRoutes'; import { useInstanceStatus } from 'hooks/api/getters/useInstanceStatus/useInstanceStatus'; -import { filterAdminRoutes } from './filterAdminRoutes'; +import { filterRoutesByPlanData } from './filterRoutesByPlanData'; import { filterByConfig, mapRouteLink } from 'component/common/util'; import { useUiFlag } from 'hooks/useUiFlag'; @@ -25,7 +25,7 @@ export const useAdminRoutes = () => { return routes .filter(filterByConfig(uiConfig)) .filter((route) => - filterAdminRoutes(route?.menu, { + filterRoutesByPlanData(route?.menu, { enterprise: isEnterprise(), pro: isPro(), billing: isBilling, diff --git a/frontend/src/component/layout/MainLayout/AdminMenu/AdminNavigationItems.tsx b/frontend/src/component/layout/MainLayout/AdminMenu/AdminNavigationItems.tsx index 20a11b6b76..6cc2b47a2c 100644 --- a/frontend/src/component/layout/MainLayout/AdminMenu/AdminNavigationItems.tsx +++ b/frontend/src/component/layout/MainLayout/AdminMenu/AdminNavigationItems.tsx @@ -16,7 +16,7 @@ import useUiConfig from 'hooks/api/getters/useUiConfig/useUiConfig'; import { useInstanceStatus } from 'hooks/api/getters/useInstanceStatus/useInstanceStatus'; import { Link, useLocation } from 'react-router-dom'; import { filterByConfig } from 'component/common/util'; -import { filterAdminRoutes } from 'component/admin/filterAdminRoutes'; +import { filterRoutesByPlanData } from 'component/admin/filterRoutesByPlanData'; import { adminGroups, adminRoutes } from 'component/admin/adminRoutes'; import { useEffect, useState, type ReactNode } from 'react'; import type { INavigationMenuItem } from 'interfaces/route'; @@ -147,7 +147,7 @@ export const AdminNavigationItems = ({ const routes = adminRoutes .filter(filterByConfig(uiConfig)) .filter((route) => - filterAdminRoutes(route?.menu, { + filterRoutesByPlanData(route?.menu, { enterprise: isEnterprise(), pro: isPro(), billing: isBilling, diff --git a/frontend/src/component/layout/MainLayout/NavigationSidebar/useRoutes.test.ts b/frontend/src/component/layout/MainLayout/NavigationSidebar/useRoutes.test.ts new file mode 100644 index 0000000000..a2a6d63554 --- /dev/null +++ b/frontend/src/component/layout/MainLayout/NavigationSidebar/useRoutes.test.ts @@ -0,0 +1,170 @@ +import { renderHook } from '@testing-library/react'; +import { useRoutes } from './useRoutes'; +import useUiConfig from 'hooks/api/getters/useUiConfig/useUiConfig'; +import { useInstanceStatus } from 'hooks/api/getters/useInstanceStatus/useInstanceStatus'; +import { type Mock, vi } from 'vitest'; + +vi.mock('hooks/api/getters/useUiConfig/useUiConfig'); +vi.mock('hooks/api/getters/useInstanceStatus/useInstanceStatus'); +vi.mock('component/menu/routes', () => ({ + getNavRoutes: () => [ + { path: '/features', title: 'Features', menu: { main: true } }, + { + path: '/enterprise', + title: 'Enterprise', + menu: { main: true, mode: ['enterprise'] }, + }, + { path: '/pro', title: 'Pro', menu: { main: true, mode: ['pro'] } }, + { + path: '/billing', + title: 'Billing', + menu: { main: true, billing: true }, + }, + { + path: '/flagged-enterprise', + title: 'Flagged Enterprise', + menu: { main: true, mode: ['enterprise'] }, + flag: 'someFeatureFlag', + }, + ], + getPrimaryRoutes: () => [ + { path: '/overview', title: 'Overview', menu: { primary: true } }, + { + path: '/admin', + title: 'Admin', + menu: { primary: true, mode: ['enterprise'] }, + }, + ], +})); + +describe('useRoutes', () => { + beforeEach(() => { + vi.resetAllMocks(); + }); + + test('filters routes based on enterprise access', () => { + (useUiConfig as Mock).mockReturnValue({ + uiConfig: { flags: {} }, + isEnterprise: () => true, + isPro: () => false, + }); + (useInstanceStatus as Mock).mockReturnValue({ + isBilling: false, + }); + + const { result } = renderHook(() => useRoutes()); + + expect(result.current.routes.mainNavRoutes).toHaveLength(2); + expect(result.current.routes.mainNavRoutes[0].path).toBe('/features'); + expect(result.current.routes.mainNavRoutes[1].path).toBe('/enterprise'); + }); + + test('filters routes based on pro access (still shows enterprise routes with badge)', () => { + (useUiConfig as Mock).mockReturnValue({ + uiConfig: { flags: {} }, + isEnterprise: () => false, + isPro: () => true, + }); + (useInstanceStatus as Mock).mockReturnValue({ + isBilling: false, + }); + + const { result } = renderHook(() => useRoutes()); + + expect(result.current.routes.mainNavRoutes).toHaveLength(3); + expect(result.current.routes.mainNavRoutes[0].path).toBe('/features'); + expect(result.current.routes.mainNavRoutes[1].path).toBe('/enterprise'); + expect(result.current.routes.mainNavRoutes[2].path).toBe('/pro'); + }); + + test('filters routes based on billing access', () => { + (useUiConfig as Mock).mockReturnValue({ + uiConfig: { flags: {} }, + isEnterprise: () => false, + isPro: () => false, + }); + (useInstanceStatus as Mock).mockReturnValue({ + isBilling: true, + }); + + const { result } = renderHook(() => useRoutes()); + + expect(result.current.routes.mainNavRoutes).toHaveLength(2); + expect(result.current.routes.mainNavRoutes[0].path).toBe('/features'); + expect(result.current.routes.mainNavRoutes[1].path).toBe('/billing'); + }); + + test('filters primary routes based on enterprise access', () => { + (useUiConfig as Mock).mockReturnValue({ + uiConfig: { flags: {} }, + isEnterprise: () => true, + isPro: () => false, + }); + (useInstanceStatus as Mock).mockReturnValue({ + isBilling: false, + }); + + const { result } = renderHook(() => useRoutes()); + + expect(result.current.routes.primaryRoutes).toHaveLength(2); + expect(result.current.routes.primaryRoutes[0].path).toBe('/overview'); + expect(result.current.routes.primaryRoutes[1].path).toBe('/admin'); + }); + + test('filters primary routes without enterprise access', () => { + (useUiConfig as Mock).mockReturnValue({ + uiConfig: { flags: {} }, + isEnterprise: () => false, + isPro: () => false, + }); + (useInstanceStatus as Mock).mockReturnValue({ + isBilling: false, + }); + + const { result } = renderHook(() => useRoutes()); + + expect(result.current.routes.primaryRoutes).toHaveLength(1); + expect(result.current.routes.primaryRoutes[0].path).toBe('/overview'); + }); + + test('does not show enterprise routes if not enterprise, even if feature flag is enabled', () => { + (useUiConfig as Mock).mockReturnValue({ + uiConfig: { flags: { someFeatureFlag: true } }, + isEnterprise: () => false, + isPro: () => false, + }); + (useInstanceStatus as Mock).mockReturnValue({ + isBilling: false, + }); + + const { result } = renderHook(() => useRoutes()); + + expect(result.current.routes.mainNavRoutes).toHaveLength(1); + expect(result.current.routes.mainNavRoutes[0].path).toBe('/features'); + expect( + result.current.routes.mainNavRoutes.find( + (r) => r.path === '/flagged-enterprise', + ), + ).toBeUndefined(); + }); + + test('shows enterprise routes with enabled feature flags when enterprise', () => { + (useUiConfig as Mock).mockReturnValue({ + uiConfig: { flags: { someFeatureFlag: true } }, + isEnterprise: () => true, + isPro: () => false, + }); + (useInstanceStatus as Mock).mockReturnValue({ + isBilling: false, + }); + + const { result } = renderHook(() => useRoutes()); + + expect(result.current.routes.mainNavRoutes).toHaveLength(3); + expect(result.current.routes.mainNavRoutes[0].path).toBe('/features'); + expect(result.current.routes.mainNavRoutes[1].path).toBe('/enterprise'); + expect(result.current.routes.mainNavRoutes[2].path).toBe( + '/flagged-enterprise', + ); + }); +}); diff --git a/frontend/src/component/layout/MainLayout/NavigationSidebar/useRoutes.ts b/frontend/src/component/layout/MainLayout/NavigationSidebar/useRoutes.ts index fbbf379864..27a87eabe9 100644 --- a/frontend/src/component/layout/MainLayout/NavigationSidebar/useRoutes.ts +++ b/frontend/src/component/layout/MainLayout/NavigationSidebar/useRoutes.ts @@ -2,21 +2,48 @@ import useUiConfig from 'hooks/api/getters/useUiConfig/useUiConfig'; import { getNavRoutes, getPrimaryRoutes } from 'component/menu/routes'; import { useAdminRoutes } from 'component/admin/useAdminRoutes'; import { filterByConfig, mapRouteLink } from 'component/common/util'; +import { + filterRoutesByPlanData, + type PlanData, +} from 'component/admin/filterRoutesByPlanData'; +import { useInstanceStatus } from 'hooks/api/getters/useInstanceStatus/useInstanceStatus'; +import type { INavigationMenuItem } from 'interfaces/route'; +import type { IUiConfig } from 'interfaces/uiConfig'; + +const filterRoutes = ( + routes: INavigationMenuItem[], + uiConfig: IUiConfig, + { enterprise, pro, billing }: PlanData, +) => { + return routes + .filter(filterByConfig(uiConfig)) + .filter((route) => + filterRoutesByPlanData(route?.menu, { + enterprise, + pro, + billing, + }), + ) + .map(mapRouteLink); +}; export const useRoutes = () => { - const { uiConfig } = useUiConfig(); + const { uiConfig, isPro, isEnterprise } = useUiConfig(); + const { isBilling } = useInstanceStatus(); const routes = getNavRoutes(); const adminRoutes = useAdminRoutes(); const primaryRoutes = getPrimaryRoutes(); + const planData: PlanData = { + enterprise: isEnterprise(), + pro: isPro(), + billing: isBilling, + }; + const filteredMainRoutes = { - mainNavRoutes: routes - .filter(filterByConfig(uiConfig)) - .map(mapRouteLink), + mainNavRoutes: filterRoutes(routes, uiConfig, planData), adminRoutes, - primaryRoutes: primaryRoutes - .filter(filterByConfig(uiConfig)) - .map(mapRouteLink), + primaryRoutes: filterRoutes(primaryRoutes, uiConfig, planData), }; return { routes: filteredMainRoutes };