From c87608feb45245185bdf1fccdc50a6c88c126a1f Mon Sep 17 00:00:00 2001 From: LightOfHeaven1994 Date: Thu, 12 Dec 2024 19:13:39 +0100 Subject: [PATCH] fix(RHINENG-14893): Correct empty state SystemDetails - Refactor: Remove GQL implementation - Correct empty state usage on System details page - Handle number of API calls depending on Page --- src/SmartComponents/SystemDetails/Details.js | 72 +--------- .../SystemDetails/EmptyState.js | 33 ++++- .../SystemDetails/EmptyState.test.js | 109 +++++++++++++++ .../SystemDetails/NoReportsState.js | 12 +- .../SystemDetails/NoReportsState.test.js | 4 +- .../SystemDetails/SystemDetails.js | 101 +++----------- .../SystemDetails/SystemDetails.test.js | 54 ++------ .../SystemDetails/SystemPoliciesAndRules.js | 131 +++++------------- .../SystemPoliciesAndRulesRest.js | 59 -------- 9 files changed, 212 insertions(+), 363 deletions(-) create mode 100644 src/SmartComponents/SystemDetails/EmptyState.test.js delete mode 100644 src/SmartComponents/SystemDetails/SystemPoliciesAndRulesRest.js diff --git a/src/SmartComponents/SystemDetails/Details.js b/src/SmartComponents/SystemDetails/Details.js index 3c6d344ad..599960ee2 100644 --- a/src/SmartComponents/SystemDetails/Details.js +++ b/src/SmartComponents/SystemDetails/Details.js @@ -1,18 +1,13 @@ import React from 'react'; import propTypes from 'prop-types'; import { Bullseye } from '@patternfly/react-core'; -import ComplianceEmptyState from 'PresentationalComponents/ComplianceEmptyState'; -import { useQuery } from '@apollo/client'; import { Spinner } from '@redhat-cloud-services/frontend-components/Spinner'; import './compliance.scss'; -import { ErrorCard } from 'PresentationalComponents'; -import useAPIV2FeatureFlag from '../../Utilities/hooks/useAPIV2FeatureFlag'; -import SystemPoliciesAndRules from './SystemPoliciesAndRules'; -import { SYSTEM_QUERY } from './constants'; import useTestResults from './useTestResults'; -import SystemPoliciesAndRulesRest from './SystemPoliciesAndRulesRest'; +import SystemPoliciesAndRules from './SystemPoliciesAndRules'; +import EmptyState from './EmptyState'; -export const DetailsRest = ({ inventoryId, hidePassed, ...props }) => { +export const Details = ({ inventoryId, hidePassed, system, ...props }) => { const { testResults, testResultsLoading } = useTestResults(inventoryId); return ( @@ -23,9 +18,9 @@ export const DetailsRest = ({ inventoryId, hidePassed, ...props }) => { ) : testResults.length === 0 ? ( // we render no policy cards nor rules table if there are no reporting policies - + ) : ( - { ); }; -DetailsRest.propTypes = { - inventoryId: propTypes.string, - hidePassed: propTypes.bool, -}; - -export const DetailsGraphQL = ({ inventoryId, hidePassed, ...props }) => { - const { data, error, loading } = useQuery(SYSTEM_QUERY, { - variables: { systemId: inventoryId }, - fetchPolicy: 'no-cache', - }); - const is404 = error?.networkError?.statusCode === 404; - - if (loading) { - return ; - } - - if (error && !is404) { - // network errors other than 404 are unexpected - return ; - } - - return ( -
- {!data?.system || is404 ? ( - - ) : ( - - )} -
- ); -}; - -DetailsGraphQL.propTypes = { - inventoryId: propTypes.string, - hidePassed: propTypes.bool, -}; - -export const Details = (props) => { - const apiV2Enabled = useAPIV2FeatureFlag(); - - return apiV2Enabled === undefined ? ( - - - - ) : apiV2Enabled ? ( - - ) : ( - - ); -}; - Details.propTypes = { inventoryId: propTypes.string, hidePassed: propTypes.bool, + system: propTypes.object, }; export default Details; diff --git a/src/SmartComponents/SystemDetails/EmptyState.js b/src/SmartComponents/SystemDetails/EmptyState.js index 8611616c5..56d3f0b4c 100644 --- a/src/SmartComponents/SystemDetails/EmptyState.js +++ b/src/SmartComponents/SystemDetails/EmptyState.js @@ -3,20 +3,39 @@ import propTypes from 'prop-types'; import { NotConnected } from '@redhat-cloud-services/frontend-components/NotConnected'; import NoPoliciesState from './NoPoliciesState'; import NoReportsState from './NoReportsState'; +import useSystem from 'Utilities/hooks/api/useSystem'; +import { Bullseye, Spinner } from '@patternfly/react-core'; -const EmptyState = ({ system }) => { - if (!system?.insightsId) { +const EmptyState = ({ inventoryId, system }) => { + // request system data in case Inventory details Compliance opened + const { data: { data } = {} } = useSystem({ + params: [inventoryId], + skip: system, + }); + const policiesCount = system?.policies.length || data?.policies.length; + const insightsId = system?.insights_id || data?.insights_id; + + if (!system && !data) { + return ( + + + + ); + } + + if (!insightsId) { return ; + } + + if (policiesCount === 0) { + return ; } else { - if (!system?.hasPolicy) { - return ; - } else if (system?.hasPolicy && system?.testResultProfiles?.length === 0) { - return ; - } + return ; } }; EmptyState.propTypes = { + inventoryId: propTypes.string, system: propTypes.object, }; diff --git a/src/SmartComponents/SystemDetails/EmptyState.test.js b/src/SmartComponents/SystemDetails/EmptyState.test.js new file mode 100644 index 000000000..09497d022 --- /dev/null +++ b/src/SmartComponents/SystemDetails/EmptyState.test.js @@ -0,0 +1,109 @@ +import { render, screen } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import TestWrapper from '@/Utilities/TestWrapper'; + +import EmptyState from './EmptyState.js'; +import useSystem from 'Utilities/hooks/api/useSystem'; + +jest.mock('Utilities/hooks/api/useSystem', () => jest.fn()); + +describe('EmptyState for systemDetails', () => { + it('expect to render loading state while waiting for data', () => { + useSystem.mockImplementation(() => ({ + data: { data: undefined }, + error: undefined, + loading: undefined, + })); + render( + + + + ); + + expect(screen.getByLabelText('Contents')).toHaveAttribute( + 'aria-valuetext', + 'Loading...' + ); + }); + + it('expect to render NotConnected component', () => { + useSystem.mockImplementation(() => ({ + data: { data: { display_name: 'foo', policies: [], insights_id: '' } }, + error: undefined, + loading: undefined, + })); + render( + + + + ); + + expect( + screen.getByText('This system isn’t connected to Insights yet') + ).toBeInTheDocument(); + }); + + it('expect to render NoPoliciesState component', () => { + useSystem.mockImplementation(() => ({ + data: { data: { display_name: 'foo', policies: [], insights_id: '123' } }, + error: undefined, + loading: undefined, + })); + render( + + + + ); + + expect( + screen.getByText( + 'This system is not part of any SCAP policies defined within Compliance.' + ) + ).toBeInTheDocument(); + }); + + it('expect to render NoReportsState component', () => { + useSystem.mockImplementation(() => ({ + data: { + data: { display_name: 'foo', policies: [{}, {}], insights_id: '123' }, + }, + error: undefined, + loading: undefined, + })); + render( + + + + ); + + expect(screen.getByText('No results reported')).toBeInTheDocument(); + expect( + screen.getByText( + `This system is part of 2 policies, but has not returned any results.` + ) + ).toBeInTheDocument(); + }); + + it('expect to render NoReportsState component', () => { + render( + + + + ); + + expect(useSystem).toHaveBeenCalledWith({ + params: ['123'], + skip: { insights_id: '123', policies: [{}] }, // Ensure correct 'skip' value + }); + + expect(screen.getByText('No results reported')).toBeInTheDocument(); + expect( + screen.getByText( + `This system is part of 1 policy, but has not returned any results.` + ) + ).toBeInTheDocument(); + }); +}); diff --git a/src/SmartComponents/SystemDetails/NoReportsState.js b/src/SmartComponents/SystemDetails/NoReportsState.js index eb74c35d9..b2d5e4b66 100644 --- a/src/SmartComponents/SystemDetails/NoReportsState.js +++ b/src/SmartComponents/SystemDetails/NoReportsState.js @@ -8,8 +8,9 @@ import { EmptyStateIcon, EmptyStateHeader, } from '@patternfly/react-core'; +import { pluralize } from '@patternfly/react-core'; -const NoReportsState = ({ system }) => ( +const NoReportsState = ({ policiesCount }) => ( ( headingLevel="h1" /> - This system is part of {system?.policies?.length} - {system?.policies?.length > 1 ? ' policies' : ' policy'}, but has not - returned any results. + This system is part of {pluralize(policiesCount, 'policy', 'policies')}, + but has not returned any results. Reports are returned when the system checks into Insights. By default, @@ -44,9 +44,7 @@ const NoReportsState = ({ system }) => ( ); NoReportsState.propTypes = { - system: propTypes.shape({ - policies: propTypes.array, - }), + policiesCount: propTypes.number, }; export default NoReportsState; diff --git a/src/SmartComponents/SystemDetails/NoReportsState.test.js b/src/SmartComponents/SystemDetails/NoReportsState.test.js index 553dfc835..f738e63e3 100644 --- a/src/SmartComponents/SystemDetails/NoReportsState.test.js +++ b/src/SmartComponents/SystemDetails/NoReportsState.test.js @@ -5,7 +5,7 @@ import NoReportsState from './NoReportsState'; describe('NoReportsState', () => { it('with a system having policies', () => { - render(); + render(); expect( screen.getByText( @@ -15,7 +15,7 @@ describe('NoReportsState', () => { }); it('with a system having multiple policies', () => { - render(); + render(); expect( screen.getByText( diff --git a/src/SmartComponents/SystemDetails/SystemDetails.js b/src/SmartComponents/SystemDetails/SystemDetails.js index 7f9d9b541..6fecbf991 100644 --- a/src/SmartComponents/SystemDetails/SystemDetails.js +++ b/src/SmartComponents/SystemDetails/SystemDetails.js @@ -1,6 +1,5 @@ import React from 'react'; import propTypes from 'prop-types'; -import { useQuery, gql } from '@apollo/client'; import { useParams } from 'react-router-dom'; import PageHeader from '@redhat-cloud-services/frontend-components/PageHeader'; import Skeleton, { @@ -11,8 +10,6 @@ import { PageSection, Breadcrumb, BreadcrumbItem, - Bullseye, - Spinner, } from '@patternfly/react-core'; import Details from './Details'; import { @@ -23,16 +20,6 @@ import { import { InventoryDetails } from 'SmartComponents'; import { useTitleEntity } from 'Utilities/hooks/useDocumentTitle'; import useSystem from 'Utilities/hooks/api/useSystem'; -import useAPIV2FeatureFlag from 'Utilities/hooks/useAPIV2FeatureFlag'; - -const QUERY = gql` - query SD_System($inventoryId: String!) { - system(id: $inventoryId) { - id - name - } - } -`; const PageBreadcrumb = ({ systemName }) => ( @@ -46,58 +33,7 @@ PageBreadcrumb.propTypes = { systemName: propTypes.string.isRequired, }; -const SystemDetailBase = ({ - data, - error, - loading, - systemName, - inventoryId, -}) => ( - - - - - - - -
- - - - - - - - -); - -SystemDetailBase.propTypes = { - data: propTypes.object, - error: propTypes.object, - loading: propTypes.bool, - systemName: propTypes.string, - inventoryId: propTypes.string, -}; - -const SystemDetailsGraphQL = ({ route }) => { - const { inventoryId } = useParams(); - const { data, error, loading } = useQuery(QUERY, { - variables: { inventoryId }, - }); - const systemName = data?.system?.name; - - useTitleEntity(route, systemName); - - return ( - - ); -}; - -SystemDetailsGraphQL.propTypes = { - route: propTypes.object.isRequired, -}; - -const SystemDetailsRest = ({ route }) => { +const SystemDetails = ({ route }) => { const { inventoryId } = useParams(); let { data: { data } = {}, @@ -109,28 +45,27 @@ const SystemDetailsRest = ({ route }) => { useTitleEntity(route, systemName); return ( - + + + + + + + +
+ + + + + + + + ); }; -SystemDetailsRest.propTypes = { +SystemDetails.propTypes = { route: propTypes.object.isRequired, }; -export const SystemDetails = (props) => { - const restApiEnabled = useAPIV2FeatureFlag(); - const Component = - restApiEnabled === undefined ? ( - - - - ) : restApiEnabled === true ? ( - SystemDetailsRest - ) : ( - SystemDetailsGraphQL - ); - - return ; -}; - export default SystemDetails; diff --git a/src/SmartComponents/SystemDetails/SystemDetails.test.js b/src/SmartComponents/SystemDetails/SystemDetails.test.js index f5325eddd..6778a70f9 100644 --- a/src/SmartComponents/SystemDetails/SystemDetails.test.js +++ b/src/SmartComponents/SystemDetails/SystemDetails.test.js @@ -2,12 +2,8 @@ import { render, screen } from '@testing-library/react'; import '@testing-library/jest-dom'; import TestWrapper from '@/Utilities/TestWrapper'; -import { useQuery } from '@apollo/client'; -import { useLocation } from 'react-router-dom'; import SystemDetails from './SystemDetails.js'; -import useAPIV2FeatureFlag from '../../Utilities/hooks/useAPIV2FeatureFlag.js'; - -jest.mock('@apollo/client'); +import useSystem from 'Utilities/hooks/api/useSystem'; jest.mock('react-router-dom', () => ({ ...jest.requireActual('react-router-dom'), @@ -22,54 +18,26 @@ jest.mock('Utilities/hooks/useDocumentTitle', () => ({ setTitle: () => ({}), })); -jest.mock('../../Utilities/hooks/useAPIV2FeatureFlag.js'); +jest.mock('Utilities/hooks/api/useSystem', () => jest.fn()); describe('SystemDetails', () => { - const defaultLocation = { - query: { - hidePassed: false, - }, - }; - const data = { - system: { - insightsId: 'ID_YEAH', - name: 'test.host.local', - testResultProfiles: [], - }, - }; - const defaultQuery = { - data, - loading: false, - }; - - beforeEach(() => { - useLocation.mockImplementation(jest.fn(() => defaultLocation)); - useQuery.mockImplementation(() => defaultQuery); - useAPIV2FeatureFlag.mockReturnValue(false); - }); - - it('expect to render the inventory details', () => { + it('expect to render Inventory Details Wrapper', () => { + useSystem.mockImplementation(() => ({ + data: { + data: { display_name: 'foo', policies: [{}], insights_id: '123' }, + }, + error: undefined, + loading: undefined, + })); render( - + ); expect( screen.getByLabelText('Inventory Details Wrapper') ).toBeInTheDocument(); - }); - - it('expect to render loading', () => { - useQuery.mockImplementation(() => ({ ...defaultQuery, loading: true })); - render( - - - - ); - expect(screen.getByText('Loading...')).toBeInTheDocument(); }); - - // TODO: add tests for the REST API component }); diff --git a/src/SmartComponents/SystemDetails/SystemPoliciesAndRules.js b/src/SmartComponents/SystemDetails/SystemPoliciesAndRules.js index f7e92942b..e4da7a2d1 100644 --- a/src/SmartComponents/SystemDetails/SystemPoliciesAndRules.js +++ b/src/SmartComponents/SystemDetails/SystemPoliciesAndRules.js @@ -1,114 +1,53 @@ import React, { useState } from 'react'; import propTypes from 'prop-types'; -import natsort from 'natsort'; -import useAPIV2FeatureFlag from '@/Utilities/hooks/useAPIV2FeatureFlag'; -import RulesTable from '@/PresentationalComponents/RulesTable/RulesTable'; -import { SystemPolicyCards as SystemPolicyCardsRest } from '../SystemPolicyCards/SystemPolicyCards'; -import EmptyState from './EmptyState'; -import { - Tabs, - Tab, - TabTitleText, - Bullseye, - Spinner, -} from '@patternfly/react-core'; -import SystemPolicyCards from '../../PresentationalComponents/SystemPolicyCards'; +import SystemPolicyCards from '../SystemPolicyCards/SystemPolicyCards'; +import { Tabs, Tab, TabTitleText } from '@patternfly/react-core'; +import RuleResults from './RuleResults'; -const SystemPoliciesAndRules = ({ data: { system }, loading, hidePassed }) => { - const apiV2Enabled = useAPIV2FeatureFlag(); +const SystemPoliciesAndRules = ({ + systemId, + reportTestResults, + hidePassed, +}) => { + // FYI: test result ID and policy ID are not the same, but test result ID only identifies each tab here. const [selectedPolicy, setSelectedPolicy] = useState( - system.testResultProfiles[0]?.id - ); - const policies = system?.testResultProfiles; - - const sorter = natsort({ desc: false, insensitive: true }); - const sortedTestResultProfiles = system?.testResultProfiles.sort( - (testResultProfile1, testResultProfile2) => - sorter(testResultProfile1?.name, testResultProfile2?.name) + reportTestResults[0].report_id ); return ( <> - {apiV2Enabled === undefined ? ( - - - - ) : apiV2Enabled === true ? ( - - ) : ( - - )} +
- {system?.testResultProfiles?.length ? ( - <> - {system.testResultProfiles.length > 1 && ( - - {sortedTestResultProfiles.map((policy, idx) => { - return ( - {policy.name} } - onClick={() => { - setSelectedPolicy(policy.id); - }} - /> - ); - })} - - )} - profile.supported - ).length > 0, - }} - profileRules={system?.testResultProfiles - .filter((policy) => selectedPolicy === policy.id) - .map((profile) => ({ - system, - profile, - rules: profile.rules, - }))} - loading={loading} - options={{ - sortBy: { - index: 4, - direction: 'asc', - property: 'severity', - }, + + {reportTestResults.map((reportTestResult, idx) => ( + {reportTestResult.title} } + onClick={() => { + setSelectedPolicy(reportTestResult.report_id); }} - /> - - ) : ( - - )} + > + + + ))} + ); }; SystemPoliciesAndRules.propTypes = { - data: propTypes.shape({ - system: propTypes.shape({ - hasPolicy: propTypes.bool, - policies: propTypes.shape({ - id: propTypes.string, - }), - profiles: propTypes.array, - testResultProfiles: propTypes.array, - }), - }), - loading: propTypes.bool, + systemId: propTypes.string.isRequired, + reportTestResults: propTypes.array.isRequired, hidePassed: propTypes.bool, }; diff --git a/src/SmartComponents/SystemDetails/SystemPoliciesAndRulesRest.js b/src/SmartComponents/SystemDetails/SystemPoliciesAndRulesRest.js deleted file mode 100644 index 578ca961b..000000000 --- a/src/SmartComponents/SystemDetails/SystemPoliciesAndRulesRest.js +++ /dev/null @@ -1,59 +0,0 @@ -import React, { useState } from 'react'; -import propTypes from 'prop-types'; -import SystemPolicyCards from '../SystemPolicyCards/SystemPolicyCards'; -import { Tabs, Tab, TabTitleText } from '@patternfly/react-core'; -import RuleResults from './RuleResults'; - -const SystemPoliciesAndRulesRest = ({ - systemId, - reportTestResults, - hidePassed, -}) => { - // FYI: test result ID and policy ID are not the same, but test result ID only identifies each tab here. - const [selectedPolicy, setSelectedPolicy] = useState( - reportTestResults[0].report_id - ); - - console.log('### reportTestResults', reportTestResults); - return ( - <> - -
- - {reportTestResults.map((reportTestResult, idx) => ( - {reportTestResult.title} } - onClick={() => { - setSelectedPolicy(reportTestResult.report_id); - }} - > - - - ))} - - - ); -}; - -SystemPoliciesAndRulesRest.propTypes = { - systemId: propTypes.string.isRequired, - reportTestResults: propTypes.array.isRequired, - hidePassed: propTypes.bool, -}; - -SystemPoliciesAndRulesRest.defaultProps = { - loading: true, -}; - -export default SystemPoliciesAndRulesRest;