Skip to content

Commit 8e46bda

Browse files
authored
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: #8469 (comment) 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.
1 parent d976526 commit 8e46bda

File tree

6 files changed

+240
-35
lines changed

6 files changed

+240
-35
lines changed

frontend/src/component/admin/filterAdminRoutes.test.ts

+24-18
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
1-
import { filterAdminRoutes } from './filterAdminRoutes';
1+
import { filterRoutesByPlanData } from './filterRoutesByPlanData';
22

3-
describe('filterAdminRoutes - open souce routes', () => {
3+
describe('filterRoutesByPlanData - open souce routes', () => {
44
test('open source - should show menu item if mode paid plan mode is not defined', () => {
55
expect(
6-
filterAdminRoutes(
6+
filterRoutesByPlanData(
77
{},
88
{
99
pro: false,
@@ -21,12 +21,14 @@ describe('filterAdminRoutes - open souce routes', () => {
2121
billing: false,
2222
};
2323

24-
expect(filterAdminRoutes({ mode: ['pro'] }, state)).toBe(false);
25-
expect(filterAdminRoutes({ mode: ['enterprise'] }, state)).toBe(false);
26-
expect(filterAdminRoutes({ mode: ['pro', 'enterprise'] }, state)).toBe(
24+
expect(filterRoutesByPlanData({ mode: ['pro'] }, state)).toBe(false);
25+
expect(filterRoutesByPlanData({ mode: ['enterprise'] }, state)).toBe(
2726
false,
2827
);
29-
expect(filterAdminRoutes({ billing: true }, state)).toBe(false);
28+
expect(
29+
filterRoutesByPlanData({ mode: ['pro', 'enterprise'] }, state),
30+
).toBe(false);
31+
expect(filterRoutesByPlanData({ billing: true }, state)).toBe(false);
3032
});
3133

3234
test('pro - should show menu item for pro customers', () => {
@@ -36,12 +38,14 @@ describe('filterAdminRoutes - open souce routes', () => {
3638
billing: false,
3739
};
3840

39-
expect(filterAdminRoutes({ mode: ['pro'] }, state)).toBe(true);
40-
expect(filterAdminRoutes({ mode: ['pro', 'enterprise'] }, state)).toBe(
41+
expect(filterRoutesByPlanData({ mode: ['pro'] }, state)).toBe(true);
42+
expect(
43+
filterRoutesByPlanData({ mode: ['pro', 'enterprise'] }, state),
44+
).toBe(true);
45+
// This is to show enterprise badge in pro mode
46+
expect(filterRoutesByPlanData({ mode: ['enterprise'] }, state)).toBe(
4147
true,
4248
);
43-
// This is to show enterprise badge in pro mode
44-
expect(filterAdminRoutes({ mode: ['enterprise'] }, state)).toBe(true);
4549
});
4650

4751
test('enterprise - should show menu item if mode enterprise is defined or mode is undefined', () => {
@@ -51,16 +55,18 @@ describe('filterAdminRoutes - open souce routes', () => {
5155
billing: false,
5256
};
5357

54-
expect(filterAdminRoutes({ mode: ['enterprise'] }, state)).toBe(true);
55-
expect(filterAdminRoutes({ mode: ['pro', 'enterprise'] }, state)).toBe(
58+
expect(filterRoutesByPlanData({ mode: ['enterprise'] }, state)).toBe(
5659
true,
5760
);
58-
expect(filterAdminRoutes({ mode: ['pro'] }, state)).toBe(false);
61+
expect(
62+
filterRoutesByPlanData({ mode: ['pro', 'enterprise'] }, state),
63+
).toBe(true);
64+
expect(filterRoutesByPlanData({ mode: ['pro'] }, state)).toBe(false);
5965
});
6066

6167
test('billing - should show menu item if billing is defined', () => {
6268
expect(
63-
filterAdminRoutes(
69+
filterRoutesByPlanData(
6470
{ mode: ['pro'], billing: true },
6571
{
6672
pro: true,
@@ -70,7 +76,7 @@ describe('filterAdminRoutes - open souce routes', () => {
7076
),
7177
).toBe(true);
7278
expect(
73-
filterAdminRoutes(
79+
filterRoutesByPlanData(
7480
{ mode: ['enterprise'], billing: true },
7581
{
7682
pro: false,
@@ -80,7 +86,7 @@ describe('filterAdminRoutes - open souce routes', () => {
8086
),
8187
).toBe(true);
8288
expect(
83-
filterAdminRoutes(
89+
filterRoutesByPlanData(
8490
{ mode: ['pro', 'enterprise'], billing: true },
8591
{
8692
pro: true,
@@ -90,7 +96,7 @@ describe('filterAdminRoutes - open souce routes', () => {
9096
),
9197
).toBe(true);
9298
expect(
93-
filterAdminRoutes(
99+
filterRoutesByPlanData(
94100
{ mode: ['pro'], billing: true },
95101
{
96102
pro: false,

frontend/src/component/admin/filterAdminRoutes.ts renamed to frontend/src/component/admin/filterRoutesByPlanData.ts

+8-6
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,14 @@
11
import type { INavigationMenuItem } from 'interfaces/route';
22

3-
export const filterAdminRoutes = (
3+
export type PlanData = {
4+
enterprise: boolean;
5+
pro: boolean;
6+
billing: boolean;
7+
};
8+
9+
export const filterRoutesByPlanData = (
410
menu: INavigationMenuItem['menu'],
5-
{
6-
pro,
7-
enterprise,
8-
billing,
9-
}: { pro?: boolean; enterprise?: boolean; billing?: boolean },
11+
{ pro, enterprise, billing }: PlanData,
1012
): boolean => {
1113
const mode = menu?.mode;
1214
if (menu?.billing && !billing) return false;

frontend/src/component/admin/useAdminRoutes.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import useUiConfig from 'hooks/api/getters/useUiConfig/useUiConfig';
22
import { adminRoutes as oldAdminRoutes } from './oldAdminRoutes';
33
import { adminRoutes } from './adminRoutes';
44
import { useInstanceStatus } from 'hooks/api/getters/useInstanceStatus/useInstanceStatus';
5-
import { filterAdminRoutes } from './filterAdminRoutes';
5+
import { filterRoutesByPlanData } from './filterRoutesByPlanData';
66
import { filterByConfig, mapRouteLink } from 'component/common/util';
77
import { useUiFlag } from 'hooks/useUiFlag';
88

@@ -25,7 +25,7 @@ export const useAdminRoutes = () => {
2525
return routes
2626
.filter(filterByConfig(uiConfig))
2727
.filter((route) =>
28-
filterAdminRoutes(route?.menu, {
28+
filterRoutesByPlanData(route?.menu, {
2929
enterprise: isEnterprise(),
3030
pro: isPro(),
3131
billing: isBilling,

frontend/src/component/layout/MainLayout/AdminMenu/AdminNavigationItems.tsx

+2-2
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import useUiConfig from 'hooks/api/getters/useUiConfig/useUiConfig';
1616
import { useInstanceStatus } from 'hooks/api/getters/useInstanceStatus/useInstanceStatus';
1717
import { Link, useLocation } from 'react-router-dom';
1818
import { filterByConfig } from 'component/common/util';
19-
import { filterAdminRoutes } from 'component/admin/filterAdminRoutes';
19+
import { filterRoutesByPlanData } from 'component/admin/filterRoutesByPlanData';
2020
import { adminGroups, adminRoutes } from 'component/admin/adminRoutes';
2121
import { useEffect, useState, type ReactNode } from 'react';
2222
import type { INavigationMenuItem } from 'interfaces/route';
@@ -147,7 +147,7 @@ export const AdminNavigationItems = ({
147147
const routes = adminRoutes
148148
.filter(filterByConfig(uiConfig))
149149
.filter((route) =>
150-
filterAdminRoutes(route?.menu, {
150+
filterRoutesByPlanData(route?.menu, {
151151
enterprise: isEnterprise(),
152152
pro: isPro(),
153153
billing: isBilling,
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
import { renderHook } from '@testing-library/react';
2+
import { useRoutes } from './useRoutes';
3+
import useUiConfig from 'hooks/api/getters/useUiConfig/useUiConfig';
4+
import { useInstanceStatus } from 'hooks/api/getters/useInstanceStatus/useInstanceStatus';
5+
import { type Mock, vi } from 'vitest';
6+
7+
vi.mock('hooks/api/getters/useUiConfig/useUiConfig');
8+
vi.mock('hooks/api/getters/useInstanceStatus/useInstanceStatus');
9+
vi.mock('component/menu/routes', () => ({
10+
getNavRoutes: () => [
11+
{ path: '/features', title: 'Features', menu: { main: true } },
12+
{
13+
path: '/enterprise',
14+
title: 'Enterprise',
15+
menu: { main: true, mode: ['enterprise'] },
16+
},
17+
{ path: '/pro', title: 'Pro', menu: { main: true, mode: ['pro'] } },
18+
{
19+
path: '/billing',
20+
title: 'Billing',
21+
menu: { main: true, billing: true },
22+
},
23+
{
24+
path: '/flagged-enterprise',
25+
title: 'Flagged Enterprise',
26+
menu: { main: true, mode: ['enterprise'] },
27+
flag: 'someFeatureFlag',
28+
},
29+
],
30+
getPrimaryRoutes: () => [
31+
{ path: '/overview', title: 'Overview', menu: { primary: true } },
32+
{
33+
path: '/admin',
34+
title: 'Admin',
35+
menu: { primary: true, mode: ['enterprise'] },
36+
},
37+
],
38+
}));
39+
40+
describe('useRoutes', () => {
41+
beforeEach(() => {
42+
vi.resetAllMocks();
43+
});
44+
45+
test('filters routes based on enterprise access', () => {
46+
(useUiConfig as Mock).mockReturnValue({
47+
uiConfig: { flags: {} },
48+
isEnterprise: () => true,
49+
isPro: () => false,
50+
});
51+
(useInstanceStatus as Mock).mockReturnValue({
52+
isBilling: false,
53+
});
54+
55+
const { result } = renderHook(() => useRoutes());
56+
57+
expect(result.current.routes.mainNavRoutes).toHaveLength(2);
58+
expect(result.current.routes.mainNavRoutes[0].path).toBe('/features');
59+
expect(result.current.routes.mainNavRoutes[1].path).toBe('/enterprise');
60+
});
61+
62+
test('filters routes based on pro access (still shows enterprise routes with badge)', () => {
63+
(useUiConfig as Mock).mockReturnValue({
64+
uiConfig: { flags: {} },
65+
isEnterprise: () => false,
66+
isPro: () => true,
67+
});
68+
(useInstanceStatus as Mock).mockReturnValue({
69+
isBilling: false,
70+
});
71+
72+
const { result } = renderHook(() => useRoutes());
73+
74+
expect(result.current.routes.mainNavRoutes).toHaveLength(3);
75+
expect(result.current.routes.mainNavRoutes[0].path).toBe('/features');
76+
expect(result.current.routes.mainNavRoutes[1].path).toBe('/enterprise');
77+
expect(result.current.routes.mainNavRoutes[2].path).toBe('/pro');
78+
});
79+
80+
test('filters routes based on billing access', () => {
81+
(useUiConfig as Mock).mockReturnValue({
82+
uiConfig: { flags: {} },
83+
isEnterprise: () => false,
84+
isPro: () => false,
85+
});
86+
(useInstanceStatus as Mock).mockReturnValue({
87+
isBilling: true,
88+
});
89+
90+
const { result } = renderHook(() => useRoutes());
91+
92+
expect(result.current.routes.mainNavRoutes).toHaveLength(2);
93+
expect(result.current.routes.mainNavRoutes[0].path).toBe('/features');
94+
expect(result.current.routes.mainNavRoutes[1].path).toBe('/billing');
95+
});
96+
97+
test('filters primary routes based on enterprise access', () => {
98+
(useUiConfig as Mock).mockReturnValue({
99+
uiConfig: { flags: {} },
100+
isEnterprise: () => true,
101+
isPro: () => false,
102+
});
103+
(useInstanceStatus as Mock).mockReturnValue({
104+
isBilling: false,
105+
});
106+
107+
const { result } = renderHook(() => useRoutes());
108+
109+
expect(result.current.routes.primaryRoutes).toHaveLength(2);
110+
expect(result.current.routes.primaryRoutes[0].path).toBe('/overview');
111+
expect(result.current.routes.primaryRoutes[1].path).toBe('/admin');
112+
});
113+
114+
test('filters primary routes without enterprise access', () => {
115+
(useUiConfig as Mock).mockReturnValue({
116+
uiConfig: { flags: {} },
117+
isEnterprise: () => false,
118+
isPro: () => false,
119+
});
120+
(useInstanceStatus as Mock).mockReturnValue({
121+
isBilling: false,
122+
});
123+
124+
const { result } = renderHook(() => useRoutes());
125+
126+
expect(result.current.routes.primaryRoutes).toHaveLength(1);
127+
expect(result.current.routes.primaryRoutes[0].path).toBe('/overview');
128+
});
129+
130+
test('does not show enterprise routes if not enterprise, even if feature flag is enabled', () => {
131+
(useUiConfig as Mock).mockReturnValue({
132+
uiConfig: { flags: { someFeatureFlag: true } },
133+
isEnterprise: () => false,
134+
isPro: () => false,
135+
});
136+
(useInstanceStatus as Mock).mockReturnValue({
137+
isBilling: false,
138+
});
139+
140+
const { result } = renderHook(() => useRoutes());
141+
142+
expect(result.current.routes.mainNavRoutes).toHaveLength(1);
143+
expect(result.current.routes.mainNavRoutes[0].path).toBe('/features');
144+
expect(
145+
result.current.routes.mainNavRoutes.find(
146+
(r) => r.path === '/flagged-enterprise',
147+
),
148+
).toBeUndefined();
149+
});
150+
151+
test('shows enterprise routes with enabled feature flags when enterprise', () => {
152+
(useUiConfig as Mock).mockReturnValue({
153+
uiConfig: { flags: { someFeatureFlag: true } },
154+
isEnterprise: () => true,
155+
isPro: () => false,
156+
});
157+
(useInstanceStatus as Mock).mockReturnValue({
158+
isBilling: false,
159+
});
160+
161+
const { result } = renderHook(() => useRoutes());
162+
163+
expect(result.current.routes.mainNavRoutes).toHaveLength(3);
164+
expect(result.current.routes.mainNavRoutes[0].path).toBe('/features');
165+
expect(result.current.routes.mainNavRoutes[1].path).toBe('/enterprise');
166+
expect(result.current.routes.mainNavRoutes[2].path).toBe(
167+
'/flagged-enterprise',
168+
);
169+
});
170+
});

frontend/src/component/layout/MainLayout/NavigationSidebar/useRoutes.ts

+34-7
Original file line numberDiff line numberDiff line change
@@ -2,21 +2,48 @@ import useUiConfig from 'hooks/api/getters/useUiConfig/useUiConfig';
22
import { getNavRoutes, getPrimaryRoutes } from 'component/menu/routes';
33
import { useAdminRoutes } from 'component/admin/useAdminRoutes';
44
import { filterByConfig, mapRouteLink } from 'component/common/util';
5+
import {
6+
filterRoutesByPlanData,
7+
type PlanData,
8+
} from 'component/admin/filterRoutesByPlanData';
9+
import { useInstanceStatus } from 'hooks/api/getters/useInstanceStatus/useInstanceStatus';
10+
import type { INavigationMenuItem } from 'interfaces/route';
11+
import type { IUiConfig } from 'interfaces/uiConfig';
12+
13+
const filterRoutes = (
14+
routes: INavigationMenuItem[],
15+
uiConfig: IUiConfig,
16+
{ enterprise, pro, billing }: PlanData,
17+
) => {
18+
return routes
19+
.filter(filterByConfig(uiConfig))
20+
.filter((route) =>
21+
filterRoutesByPlanData(route?.menu, {
22+
enterprise,
23+
pro,
24+
billing,
25+
}),
26+
)
27+
.map(mapRouteLink);
28+
};
529

630
export const useRoutes = () => {
7-
const { uiConfig } = useUiConfig();
31+
const { uiConfig, isPro, isEnterprise } = useUiConfig();
32+
const { isBilling } = useInstanceStatus();
833
const routes = getNavRoutes();
934
const adminRoutes = useAdminRoutes();
1035
const primaryRoutes = getPrimaryRoutes();
1136

37+
const planData: PlanData = {
38+
enterprise: isEnterprise(),
39+
pro: isPro(),
40+
billing: isBilling,
41+
};
42+
1243
const filteredMainRoutes = {
13-
mainNavRoutes: routes
14-
.filter(filterByConfig(uiConfig))
15-
.map(mapRouteLink),
44+
mainNavRoutes: filterRoutes(routes, uiConfig, planData),
1645
adminRoutes,
17-
primaryRoutes: primaryRoutes
18-
.filter(filterByConfig(uiConfig))
19-
.map(mapRouteLink),
46+
primaryRoutes: filterRoutes(primaryRoutes, uiConfig, planData),
2047
};
2148

2249
return { routes: filteredMainRoutes };

0 commit comments

Comments
 (0)