Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(assertion-v2): changed Validation tab to Quality and created new Governance tab #10935

17 changes: 14 additions & 3 deletions datahub-web-react/src/app/ProtectedRoutes.tsx
Original file line number Diff line number Diff line change
@@ -1,15 +1,26 @@
import React from 'react';
import { Switch, Route } from 'react-router-dom';
import React, { useEffect } from 'react';
import { Switch, Route, useLocation, useHistory } from 'react-router-dom';
import { Layout } from 'antd';
import { HomePage } from './home/HomePage';
import { SearchRoutes } from './SearchRoutes';
import EmbedRoutes from './EmbedRoutes';
import { PageRoutes } from '../conf/Global';
import { NEW_ROUTE_MAP, PageRoutes } from '../conf/Global';
import { getRedirectUrl } from '../conf/utils';

/**
* Container for all views behind an authentication wall.
*/
export const ProtectedRoutes = (): JSX.Element => {
const location = useLocation();
const history = useHistory();

useEffect(() => {
if (location.pathname.indexOf('/Validation') !== -1) {
history.replace(getRedirectUrl(NEW_ROUTE_MAP));
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [location]);
Comment on lines +17 to +22
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure proper cleanup in useEffect.

The useEffect hook should ensure proper cleanup to avoid potential memory leaks.

useEffect(() => {
    if (location.pathname.indexOf('/Validation') !== -1) {
        history.replace(getRedirectUrl(NEW_ROUTE_MAP));
    }
    // eslint-disable-next-line react-hooks/exhaustive-deps
    return () => {
        // Cleanup logic if necessary
    };
}, [location]);
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
useEffect(() => {
if (location.pathname.indexOf('/Validation') !== -1) {
history.replace(getRedirectUrl(NEW_ROUTE_MAP));
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [location]);
useEffect(() => {
if (location.pathname.indexOf('/Validation') !== -1) {
history.replace(getRedirectUrl(NEW_ROUTE_MAP));
}
// eslint-disable-next-line react-hooks/exhaustive-deps
return () => {
// Cleanup logic if necessary
};
}, [location]);


return (
<Layout>
<Switch>
Expand Down
17 changes: 13 additions & 4 deletions datahub-web-react/src/app/entity/dataset/DatasetEntity.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import AccessManagement from '../shared/tabs/Dataset/AccessManagement/AccessMana
import { matchedFieldPathsRenderer } from '../../search/matches/matchedFieldPathsRenderer';
import { getLastUpdatedMs } from './shared/utils';
import { IncidentTab } from '../shared/tabs/Incident/IncidentTab';
import { GovernanceTab } from '../shared/tabs/Dataset/Governance/GovernanceTab';

const SUBTYPES = {
VIEW: 'view',
Expand Down Expand Up @@ -166,14 +167,22 @@ export class DatasetEntity implements Entity<Dataset> {
},
},
{
name: 'Validation',
name: 'Quality',
component: ValidationsTab,
display: {
visible: (_, _1) => true,
enabled: (_, dataset: GetDatasetQuery) => {
return (
(dataset?.dataset?.assertions?.total || 0) > 0 || dataset?.dataset?.testResults !== null
);
return (dataset?.dataset?.assertions?.total || 0) > 0;
},
},
},
{
name: 'Governance',
component: GovernanceTab,
display: {
visible: (_, _1) => true,
enabled: (_, dataset: GetDatasetQuery) => {
return dataset?.dataset?.testResults !== null;
},
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export const EntityHealth = ({ health, baseUrl, fontSize, tooltipPlacement }: Pr
return (
<>
{(unhealthy && (
<Link to={`${baseUrl}/Validation`}>
<Link to={`${baseUrl}/Quality`}>
<Container>
<EntityHealthPopover health={health} baseUrl={baseUrl} placement={tooltipPlacement}>
{icon}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
import React, { useEffect } from 'react';
import { Button } from 'antd';
import { useHistory, useLocation } from 'react-router';
import styled from 'styled-components';
import { FileDoneOutlined } from '@ant-design/icons';
import { useEntityData } from '../../../EntityContext';
import { TestResults } from './TestResults';
import TabToolbar from '../../../components/styled/TabToolbar';
import { ANTD_GRAY } from '../../../constants';
import { useGetValidationsTab } from '../Validations/useGetValidationsTab';

const TabTitle = styled.span`
margin-left: 4px;
`;

const TabButton = styled(Button)<{ selected: boolean }>`
background-color: ${(props) => (props.selected && ANTD_GRAY[3]) || 'none'};
margin-left: 4px;
`;

enum TabPaths {
TESTS = 'Tests',
}

const DEFAULT_TAB = TabPaths.TESTS;

/**
* Component used for rendering the Entity Governance Tab.
*/
export const GovernanceTab = () => {
const { entityData } = useEntityData();
const history = useHistory();
const { pathname } = useLocation();

const passingTests = (entityData as any)?.testResults?.passing || [];
const maybeFailingTests = (entityData as any)?.testResults?.failing || [];
const totalTests = maybeFailingTests.length + passingTests.length;

const { selectedTab, basePath } = useGetValidationsTab(pathname, Object.values(TabPaths));

// If no tab was selected, select a default tab.
useEffect(() => {
if (!selectedTab) {
// Route to the default tab.
history.replace(`${basePath}/${DEFAULT_TAB}`);
}
}, [selectedTab, basePath, history]);

/**
* The top-level Toolbar tabs to display.
*/
const tabs = [
{
title: (
<>
<FileDoneOutlined />
<TabTitle>Tests ({totalTests})</TabTitle>
</>
),
path: TabPaths.TESTS,
disabled: totalTests === 0,
content: <TestResults passing={passingTests} failing={maybeFailingTests} />,
},
];

return (
<>
<TabToolbar>
<div>
{tabs.map((tab) => (
<TabButton
type="text"
disabled={tab.disabled}
selected={selectedTab === tab.path}
onClick={() => history.replace(`${basePath}/${tab.path}`)}
>
{tab.title}
</TabButton>
))}
</div>
</TabToolbar>
{tabs.filter((tab) => tab.path === selectedTab).map((tab) => tab.content)}
</>
);
};
Comment on lines +30 to +85
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure the key property in the iterable.

The GovernanceTab component is correctly implemented. However, the TabButton elements in the iterable should have a key property to ensure proper rendering.

-  {tabs.map((tab) => (
+  {tabs.map((tab, index) => (
-      <TabButton
+      <TabButton
+        key={index}
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export const GovernanceTab = () => {
const { entityData } = useEntityData();
const history = useHistory();
const { pathname } = useLocation();
const passingTests = (entityData as any)?.testResults?.passing || [];
const maybeFailingTests = (entityData as any)?.testResults?.failing || [];
const totalTests = maybeFailingTests.length + passingTests.length;
const { selectedTab, basePath } = useGetValidationsTab(pathname, Object.values(TabPaths));
// If no tab was selected, select a default tab.
useEffect(() => {
if (!selectedTab) {
// Route to the default tab.
history.replace(`${basePath}/${DEFAULT_TAB}`);
}
}, [selectedTab, basePath, history]);
/**
* The top-level Toolbar tabs to display.
*/
const tabs = [
{
title: (
<>
<FileDoneOutlined />
<TabTitle>Tests ({totalTests})</TabTitle>
</>
),
path: TabPaths.TESTS,
disabled: totalTests === 0,
content: <TestResults passing={passingTests} failing={maybeFailingTests} />,
},
];
return (
<>
<TabToolbar>
<div>
{tabs.map((tab) => (
<TabButton
type="text"
disabled={tab.disabled}
selected={selectedTab === tab.path}
onClick={() => history.replace(`${basePath}/${tab.path}`)}
>
{tab.title}
</TabButton>
))}
</div>
</TabToolbar>
{tabs.filter((tab) => tab.path === selectedTab).map((tab) => tab.content)}
</>
);
};
export const GovernanceTab = () => {
const { entityData } = useEntityData();
const history = useHistory();
const { pathname } = useLocation();
const passingTests = (entityData as any)?.testResults?.passing || [];
const maybeFailingTests = (entityData as any)?.testResults?.failing || [];
const totalTests = maybeFailingTests.length + passingTests.length;
const { selectedTab, basePath } = useGetValidationsTab(pathname, Object.values(TabPaths));
// If no tab was selected, select a default tab.
useEffect(() => {
if (!selectedTab) {
// Route to the default tab.
history.replace(`${basePath}/${DEFAULT_TAB}`);
}
}, [selectedTab, basePath, history]);
/**
* The top-level Toolbar tabs to display.
*/
const tabs = [
{
title: (
<>
<FileDoneOutlined />
<TabTitle>Tests ({totalTests})</TabTitle>
</>
),
path: TabPaths.TESTS,
disabled: totalTests === 0,
content: <TestResults passing={passingTests} failing={maybeFailingTests} />,
},
];
return (
<>
<TabToolbar>
<div>
{tabs.map((tab, index) => (
<TabButton
type="text"
disabled={tab.disabled}
selected={selectedTab === tab.path}
onClick={() => history.replace(`${basePath}/${tab.path}`)}
key={index}
>
{tab.title}
</TabButton>
))}
</div>
</TabToolbar>
{tabs.filter((tab) => tab.path === selectedTab).map((tab) => tab.content)}
</>
);
};
Tools
Biome

[error] 71-76: Missing key property for this element in iterable.

The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.

(lint/correctness/useJsxKeyInIterable)

Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ export const DatasetAssertionsList = ({
to={`${entityRegistry.getEntityUrl(
EntityType.Dataset,
entityData.urn,
)}/Validation/Data Contract`}
)}/Quality/Data Contract`}
style={{ color: REDESIGN_COLORS.BLUE }}
>
view
Expand All @@ -200,7 +200,7 @@ export const DatasetAssertionsList = ({
to={`${entityRegistry.getEntityUrl(
EntityType.Dataset,
entityData.urn,
)}/Validation/Data Contract`}
)}/Quality/Data Contract`}
>
<DataContractLogo />
</Link>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,8 @@ import React, { useEffect } from 'react';
import { Button } from 'antd';
import { useHistory, useLocation } from 'react-router';
import styled from 'styled-components';
import { AuditOutlined, FileDoneOutlined, FileProtectOutlined } from '@ant-design/icons';
import { AuditOutlined, FileProtectOutlined } from '@ant-design/icons';
import { useEntityData } from '../../../EntityContext';
import { TestResults } from './TestResults';
import { Assertions } from './Assertions';
import TabToolbar from '../../../components/styled/TabToolbar';
import { useGetValidationsTab } from './useGetValidationsTab';
Expand All @@ -22,8 +21,7 @@ const TabButton = styled(Button)<{ selected: boolean }>`
`;

enum TabPaths {
ASSERTIONS = 'Assertions',
TESTS = 'Tests',
ASSERTIONS = 'List',
DATA_CONTRACT = 'Data Contract',
}

Expand All @@ -39,9 +37,6 @@ export const ValidationsTab = () => {
const appConfig = useAppConfig();

const totalAssertions = (entityData as any)?.assertions?.total;
const passingTests = (entityData as any)?.testResults?.passing || [];
const maybeFailingTests = (entityData as any)?.testResults?.failing || [];
const totalTests = maybeFailingTests.length + passingTests.length;

const { selectedTab, basePath } = useGetValidationsTab(pathname, Object.values(TabPaths));

Expand All @@ -68,17 +63,6 @@ export const ValidationsTab = () => {
disabled: totalAssertions === 0,
content: <Assertions />,
},
{
title: (
<>
<FileDoneOutlined />
<TabTitle>Tests ({totalTests})</TabTitle>
</>
),
path: TabPaths.TESTS,
disabled: totalTests === 0,
content: <TestResults passing={passingTests} failing={maybeFailingTests} />,
},
];

if (appConfig.config.featureFlags?.dataContractsEnabled) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,37 +2,37 @@ import { useGetValidationsTab } from '../useGetValidationsTab';

describe('useGetValidationsTab', () => {
it('should correctly extract valid tab', () => {
const pathname = '/dataset/urn:li:abc/Validation/Assertions';
const tabNames = ['Assertions'];
const pathname = '/dataset/urn:li:abc/Quality/List';
const tabNames = ['List'];
const res = useGetValidationsTab(pathname, tabNames);
expect(res.selectedTab).toEqual('Assertions');
expect(res.basePath).toEqual('/dataset/urn:li:abc/Validation');
expect(res.selectedTab).toEqual('List');
expect(res.basePath).toEqual('/dataset/urn:li:abc/Quality');
});
it('should extract undefined for invalid tab', () => {
const pathname = '/dataset/urn:li:abc/Validation/Assertions';
const pathname = '/dataset/urn:li:abc/Quality/Assertions';
const tabNames = ['Tests'];
const res = useGetValidationsTab(pathname, tabNames);
expect(res.selectedTab).toBeUndefined();
expect(res.basePath).toEqual('/dataset/urn:li:abc/Validation');
expect(res.basePath).toEqual('/dataset/urn:li:abc/Quality');
});
it('should extract undefined for missing tab', () => {
const pathname = '/dataset/urn:li:abc/Validation';
const pathname = '/dataset/urn:li:abc/Quality';
const tabNames = ['Tests'];
const res = useGetValidationsTab(pathname, tabNames);
expect(res.selectedTab).toBeUndefined();
expect(res.basePath).toEqual('/dataset/urn:li:abc/Validation');
expect(res.basePath).toEqual('/dataset/urn:li:abc/Quality');
});
it('should handle trailing slashes', () => {
let pathname = '/dataset/urn:li:abc/Validation/Assertions/';
let pathname = '/dataset/urn:li:abc/Quality/Assertions/';
let tabNames = ['Assertions'];
let res = useGetValidationsTab(pathname, tabNames);
expect(res.selectedTab).toEqual('Assertions');
expect(res.basePath).toEqual('/dataset/urn:li:abc/Validation');
expect(res.basePath).toEqual('/dataset/urn:li:abc/Quality');

pathname = '/dataset/urn:li:abc/Validation/';
pathname = '/dataset/urn:li:abc/Quality/';
tabNames = ['Assertions'];
res = useGetValidationsTab(pathname, tabNames);
expect(res.selectedTab).toBeUndefined();
expect(res.basePath).toEqual('/dataset/urn:li:abc/Validation');
expect(res.basePath).toEqual('/dataset/urn:li:abc/Quality');
});
});
2 changes: 1 addition & 1 deletion datahub-web-react/src/app/shared/health/healthUtils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ export const getHealthIcon = (type: HealthStatusType, status: HealthStatus, font
export const getHealthRedirectPath = (type: HealthStatusType) => {
switch (type) {
case HealthStatusType.Assertions: {
return 'Validation/Assertions';
return 'Quality/List';
}
case HealthStatusType.Incidents: {
return 'Incidents';
Expand Down
8 changes: 8 additions & 0 deletions datahub-web-react/src/conf/Global.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,11 @@ export const CLIENT_AUTH_COOKIE = 'actor';
* Name of the unique browser id cookie generated on client side
*/
export const BROWSER_ID_COOKIE = 'bid';

/** New Routes Map for redirection */
export const NEW_ROUTE_MAP = {
'/Validation/Assertions': '/Quality/List',
'/Validation/Tests': '/Governance/Tests',
'/Validation/Data%20Contract': '/Quality/Data%20Contract',
'/Validation': '/Quality',
};
26 changes: 26 additions & 0 deletions datahub-web-react/src/conf/utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/**
*
* as per the new route object
* We are redirecting older routes to new one
* e.g.
* {
'/Validation/Assertions': '/Quality/List',
}
* */

export const getRedirectUrl = (newRoutes: { [key: string]: string }) => {
let newPathname = `${window.location.pathname}${window.location.search}`;
if (!newRoutes) {
return newPathname;
}

// eslint-disable-next-line no-restricted-syntax
for (const path of Object.keys(newRoutes)) {
if (newPathname.indexOf(path) !== -1) {
newPathname = newPathname.replace(path, newRoutes[path]);
break;
}
}

return `${newPathname}${window.location.search}`;
Comment on lines +11 to +25
Copy link
Contributor

Choose a reason for hiding this comment

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

Improve readability and performance.

The function is correct but can be improved for readability and performance. Consider using Object.entries for cleaner iteration and passing pathname and search as parameters for better testability.

export const getRedirectUrl = (newRoutes: { [key: string]: string }, pathname: string, search: string) => {
-    let newPathname = `${window.location.pathname}${window.location.search}`;
+    let newPathname = `${pathname}${search}`;
    if (!newRoutes) {
        return newPathname;
    }

    for (const [path, newPath] of Object.entries(newRoutes)) {
        if (newPathname.includes(path)) {
            newPathname = newPathname.replace(path, newPath);
            break;
        }
    }

-    return `${newPathname}${window.location.search}`;
+    return `${newPathname}${search}`;
};
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export const getRedirectUrl = (newRoutes: { [key: string]: string }) => {
let newPathname = `${window.location.pathname}${window.location.search}`;
if (!newRoutes) {
return newPathname;
}
// eslint-disable-next-line no-restricted-syntax
for (const path of Object.keys(newRoutes)) {
if (newPathname.indexOf(path) !== -1) {
newPathname = newPathname.replace(path, newRoutes[path]);
break;
}
}
return `${newPathname}${window.location.search}`;
export const getRedirectUrl = (newRoutes: { [key: string]: string }, pathname: string, search: string) => {
let newPathname = `${pathname}${search}`;
if (!newRoutes) {
return newPathname;
}
for (const [path, newPath] of Object.entries(newRoutes)) {
if (newPathname.includes(path)) {
newPathname = newPathname.replace(path, newPath);
break;
}
}
return `${newPathname}${search}`;
};

};
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ describe("dataset health test", () => {
cy.login();
cy.goToDataset(urn, datasetName);
// Ensure that the “Health” badge is present and there is an active incident warning
cy.get(`[href="/dataset/${urn}/Validation"]`).should("be.visible");
cy.get(`[href="/dataset/${urn}/Validation"] span`).trigger("mouseover", {
cy.get(`[href="/dataset/${urn}/Quality"]`).should("be.visible");
cy.get(`[href="/dataset/${urn}/Quality"] span`).trigger("mouseover", {
force: true,
});
cy.waitTextVisible("This asset may be unhealthy");
Expand Down
Loading