Skip to content
76 changes: 76 additions & 0 deletions ui/apps/pmm-compat/src/lib/utils/variables.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
import { describe, it, expect, beforeEach, jest } from '@jest/globals';
import { getLinkWithVariables, shouldIncludeVars } from './variables';

const prefixes = {
grafana: '/graph',
pmm: '/pmm-ui/next',
};

const dashboards = {
pg: '/d/postgresql-instance-overview/postgresql-instances-overview',
pgSummary: '/d/postgresql-instance-summary/postgresql-instance-summary',
mysql: '/d/mysql-instance-summary/mysql-instance-summary',
node: '/d/node-overview/node-overview',
};

const mockLocation = (pathname: string) => {
Object.defineProperty(window, 'location', {
value: {
pathname,
origin: 'https://percona.com',
},
writable: true,
});
};

describe('getLinkWithVariables', () => {
beforeEach(() => {
mockLocation('/percona.com');
});

it('should return the same url if it is not a dashboard url', () => {
const url = 'https://percona.com';
const result = getLinkWithVariables(url);
expect(result).toBe(url);
});
});

describe('shouldIncludeVars', () => {
it('should handle different prefixes', () => {
const urls = [
dashboards.pg,
`${prefixes.grafana}${dashboards.pg}`,
`${prefixes.pmm}${prefixes.grafana}${dashboards.pg}`,
];

urls.forEach((url) => {
mockLocation(dashboards.pg);
const result = shouldIncludeVars(url);
expect(result).toBe(true);
});
});

it('should return true if the db type matches the current one', () => {
mockLocation(dashboards.pg);
const result = shouldIncludeVars(dashboards.pgSummary);
expect(result).toBe(true);
});

it('should return true if the target db type is node', () => {
mockLocation(dashboards.pg);
const result = shouldIncludeVars(dashboards.node);
expect(result).toBe(true);
});

it('should return false if the target db type is not the same as the current one', () => {
mockLocation(dashboards.pg);
const result = shouldIncludeVars(dashboards.mysql);
expect(result).toBe(false);
});

it('should return false if current db type is node and target db type is not node', () => {
mockLocation(dashboards.node);
const result = shouldIncludeVars(dashboards.pg);
expect(result).toBe(false);
});
});
37 changes: 32 additions & 5 deletions ui/apps/pmm-compat/src/lib/utils/variables.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export const getLinkWithVariables = (url?: string): string => {
url: url,
keepTime: true,
// Check if the DB type matches the current one used
includeVars: checkDbType(url),
includeVars: shouldIncludeVars(url),
asDropdown: false,
icon: '',
tags: [],
Expand All @@ -42,12 +42,39 @@ export const getLinkWithVariables = (url?: string): string => {

const isDashboardUrl = (url?: string) => url?.includes('/d/');

const checkDbType = (url: string): boolean => {
const currentDB = window.location.pathname?.split('/')[3]?.split('-')[0];
const targetDB = url?.split('/')[3]?.split('-')[0];
export const shouldIncludeVars = (url: string): boolean => {
const currentDB = getDbType(window.location.pathname);
const targetDB = getDbType(url);

if (currentDB === undefined || targetDB === undefined) {
return false;
}

// enable variable sharing between same db types and db type -> os/node
return (currentDB !== undefined && currentDB === targetDB) || targetDB === 'node';
return currentDB === targetDB || targetDB === 'node';
};

const getDbType = (url: string): string | undefined => {
const pathname = new URL(url, window.location.origin).pathname;
// normalize to the dashboard uid
const pathParts = pathname
.replace('/pmm-ui', '')
.replace('/next', '')
.replace('/graph', '')
.replace('/d/', '')
.split('/');

if (pathParts.length < 1 || !pathParts[0]) {
return undefined;
}

const dashboardUid = pathParts[0];

if (dashboardUid.includes('-')) {
return dashboardUid.split('-')[0];
}

return dashboardUid;
};

Choose a reason for hiding this comment

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

// Line 7-11
export const shouldIncludeVars = (url: string): boolean => {
const currentDB = getDbType(window.location.pathname);
const targetDB = getDbType(url);
return (currentDB !== undefined && currentDB === targetDB) || targetDB === 'node';
// ^^^^^^^^^^^^^^^^^^^^^^^^ This check doesn't work as intended
};

// Line 13-23
const getDbType = (url: string): string => {
// ^^^^^^ Always returns string, never undefined
// ...
return dashboardUid.includes('-') ? dashboardUid.split('-')[0] : dashboardUid;
};

The thing is: getDbType() always returns a string (never undefined), but on line 10 shouldIncludeVars() checks currentDB !== undefined. This check will always be true, so it doesn't really do anything.

Maybe what you meant was:
// Line 13
const getDbType = (url: string): string | undefined => {
const pathname = new URL(url, window.location.origin).pathname;
const dashboardUid = pathname
.replace('/pmm-ui', '')
.replace('/next', '')
.replace('/graph', '')
.replace('/d/', '')
.split('/')[0];

// Return undefined if we couldn't extract a valid DB type
if (!dashboardUid || dashboardUid === '') {
  return undefined;
}

return dashboardUid.includes('-') ? dashboardUid.split('-')[0] : dashboardUid;

};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

const cleanupVariables = (urlWithLinks: string) => {
Expand Down
18 changes: 6 additions & 12 deletions ui/apps/pmm/src/components/sidebar/nav-item/NavItem.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { useLinkWithVariables } from 'hooks/utils/useLinkWithVariables';
import { FC, useCallback, useEffect, useMemo, useState } from 'react';
import { useNavigate } from 'react-router-dom';
import { NavItemProps } from './NavItem.types';
import KeyboardArrowDownIcon from '@mui/icons-material/KeyboardArrowDown';
import { getLinkProps, hasChildMatch, shouldShowBadge } from './NavItem.utils';
Expand Down Expand Up @@ -33,13 +32,13 @@ const NavItem: FC<NavItemProps> = ({
[activeItem, item]
);
const [open, setIsOpen] = useState(active);
const url = useLinkWithVariables(item.url);
const url = useLinkWithVariables(
item.children?.length ? item.children[0].url : item.url
);
const linkProps = getLinkProps(item, url);
const theme = useTheme();
const styles = getStyles(theme, active, drawerOpen, level);
const children = item.children?.filter((i) => !i.hidden);
const dataTestid = `navitem-${item.id}`;
const navigate = useNavigate();
const showBadge = shouldShowBadge(item, open);

useEffect(() => {
Expand All @@ -59,19 +58,13 @@ const NavItem: FC<NavItemProps> = ({
}, [drawerOpen]);

const handleOpenCollapsible = () => {
const firstChild = (item.children || [])[0];

// prevent opening when sidebar collapsed
if (drawerOpen) {
setIsOpen(true);
}

if (firstChild?.url) {
navigate(firstChild.url);
}
};

if (children?.length) {
if (item.children?.length) {
return (
<>
<NavItemTooltip
Expand All @@ -97,6 +90,7 @@ const NavItem: FC<NavItemProps> = ({
level === 0 && styles.navItemRootCollapsible,
]}
onClick={handleOpenCollapsible}
{...linkProps}
data-testid={dataTestid}
data-navlevel={level}
>
Expand Down Expand Up @@ -144,7 +138,7 @@ const NavItem: FC<NavItemProps> = ({
data-testid={`${dataTestid}-collapse`}
>
<List component="div" disablePadding sx={styles.listCollapsible}>
{children.map((item) => (
{item.children.map((item) => (
<NavItem
key={item.id}
item={item}
Expand Down
26 changes: 3 additions & 23 deletions ui/apps/pmm/src/contexts/navigation/navigation.constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,7 @@ export const NAV_HOME_PAGE: NavItem = {
icon: 'home',
text: 'Home page',
url: `${PMM_NEW_NAV_GRAFANA_PATH}/d/pmm-home`,
children: [
{
id: 'home-page-dashboard',
url: `${PMM_NEW_NAV_GRAFANA_PATH}/d/pmm-home/home-dashboard`,
hidden: true,
},
],
matches: [`${PMM_NEW_NAV_GRAFANA_PATH}/d/pmm-home/home-dashboard`],
};

//
Expand Down Expand Up @@ -561,14 +555,7 @@ export const NAV_INVENTORY: NavItem = {
id: 'add-instance',
url: `${PMM_NEW_NAV_GRAFANA_PATH}/add-instance`,
text: 'Add Service',
children: [
{
id: 'add-instance-form',
url: `${PMM_NEW_NAV_GRAFANA_PATH}/add-instance/:type`,
text: 'Add Service',
hidden: true,
},
],
matches: [`${PMM_NEW_NAV_GRAFANA_PATH}/add-instance/:type`],
},
{
id: 'inventory-services',
Expand Down Expand Up @@ -598,14 +585,7 @@ export const NAV_BACKUPS: NavItem = {
id: 'backup-inventory',
text: 'All backups',
url: `${PMM_NEW_NAV_GRAFANA_PATH}/backup/inventory`,
children: [
{
id: 'backups-new',
text: 'Create backup',
url: `${PMM_NEW_NAV_GRAFANA_PATH}/backup/new`,
hidden: true,
},
],
matches: [`${PMM_NEW_NAV_GRAFANA_PATH}/backup/new`],
},
{
id: 'scheduled-backups',
Expand Down
Loading