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.

Sign up for GitHub

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

Jump to bottom

fix(RHINENG-3106): properly apply default filters in CVEs and SystemsCVEs pages #2124

Merged
merged 8 commits into from
Jul 16, 2024
Merged

fix(RHINENG-3106): properly apply default filters in CVEs and SystemsCVEs pages #2124

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9a81065
fix(RHINENG-3106): properly apply default filters in CVEs and Systems…
mkholjuraev Jul 2, 2024
bc327e4
chore: refactor CVEs component modals, cleanup state management
mkholjuraev Jul 2, 2024
7cd2565
chore: fix tests
mkholjuraev Jul 2, 2024
c656c06
chore: fix cy tests
mkholjuraev Jul 3, 2024
71bcd6c
Update src/Components/SmartComponents/CVEs/CVEs.js
mkholjuraev Jul 11, 2024
88144c8
chore: adopt PR suggestions
mkholjuraev Jul 11, 2024
b877d16
fix(RHINENG-3106): show or hide the advisory filter according to api …
mkholjuraev Jul 12, 2024
0d49c07
chore: prever userEvent over fireEvent
mkholjuraev Jul 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cypress.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,6 @@ module.exports = defineConfig({
}
},
env: {
pluginVisualRegressionMaxDiffThreshold: 0.05
pluginVisualRegressionMaxDiffThreshold: 0.07
}
});
7 changes: 6 additions & 1 deletion src/Components/SmartComponents/CVEs/CVEs.cy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,11 @@ import {

const mountComponent = (canEditStatusOrBusinessRisk = true) => {
cy.mountWithContext(
CVEs, {}, { rbac: [[canEditStatusOrBusinessRisk, true, true, true], false] }
CVEs, {
path: '/',
routerProps: { initialEntries: ['/'] }
},
{ rbac: [[canEditStatusOrBusinessRisk, true, true, true], false] }
);
};

Expand Down Expand Up @@ -128,6 +132,7 @@ describe('CVE list table', () => {
});

it('matches screenshot', () => {
cy.get('[data-ouia-component-id="cves-table"]');
cy.get('body').matchImage();
});

Expand Down
95 changes: 35 additions & 60 deletions src/Components/SmartComponents/CVEs/CVEs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
import React, { useMemo, useState, useEffect, useContext } from 'react';
import React, { useMemo, useEffect, useContext } from 'react';
import { useDispatch, useSelector } from 'react-redux';
import { Alert, Stack, StackItem } from '@patternfly/react-core';
import PropTypes from 'prop-types';
import { CVES_ALLOWED_PARAMS, PATCHMAN_ADVISORY_DOCS_PATH, PERMISSIONS, SERVICE_NAME } from '../../../Helpers/constants';
import { createCveListByAccount } from '../../../Helpers/VulnerabilityHelper';
import { constructFilterParameters, updateRef, useUrlParams } from '../../../Helpers/MiscHelper';
import { constructFilterParameters, useUrlParams } from '../../../Helpers/MiscHelper';
import BusinessRiskModal from '../Modals/BusinessRiskModal';
import StatusModal from '../Modals/CveStatusModal';
import CVEsTable from './CVEsTable';
import CVEsTableToolbar from './CVEsTableToolbar';
import DownloadReport from '../../../Helpers/DownloadReport';
import {
changeCveListParameters,
fetchCveListByAccount,
Expand All @@ -19,24 +18,22 @@ import {
changeColumnsCveList
} from '../../../Store/Actions/Actions';
import {
addNotification,
clearNotifications
} from '@redhat-cloud-services/frontend-components-notifications/redux';
import ErrorHandler from '../../PresentationalComponents/ErrorHandler/ErrorHandler';
import { useColumnManagement, useHybridSystemFilterFlag, useRbac } from '../../../Helpers/Hooks';
import { NotAuthorized } from '../../PresentationalComponents/EmptyStates/EmptyStates';
import { ExternalLinkAltIcon } from '@patternfly/react-icons';
import { getCveDefaultFilters } from './CVEsAssets';
import { getCveDefaultFilters, useShowModal } from './CVEsAssets';
import { AccountStatContext } from '../../../Utilities/VulnerabilityRoutes';
import PageLoading from '../../PresentationalComponents/Snippets/PageLoading';
import { useDownloadReport } from './CVEsAssets';
import isEmpty from 'lodash/isEmpty';

export const CVETableContext = React.createContext({});

export const CVEs = ({ rbac }) => {
const dispatch = useDispatch();
const [CveStatusModal, setStatusModal] = useState(() => () => null);
const [CveBusinessRiskModal, setBusinessRiskModal] = useState(() => () => null);
const [isFirstLoad, setFirstLoad] = useState(true);
const { isAdvisoryAvailable, includesCvesWithoutErrata } = useContext(AccountStatContext);

const [[
Expand Down Expand Up @@ -66,31 +63,39 @@ export const CVEs = ({ rbac }) => {
({ CVEsStore }) => CVEsStore.isAllExpanded
);

const [ColumnManagementModal, setColumnManagementModalOpen]
= useColumnManagement(columns, newColumns => dispatch(changeColumnsCveList(newColumns)));
const [ColumnManagementModal, setColumnManagementModalOpen] = useColumnManagement(
columns,
newColumns => dispatch(changeColumnsCveList(newColumns))
);

const cves = useMemo(() =>
createCveListByAccount(cveList, columns, parameters),
[cveList, columns, parameters]
);

const cves = useMemo(() => createCveListByAccount(cveList, columns, parameters), [cveList, columns, parameters]);
const shouldUseHybridSystemFilter = useHybridSystemFilterFlag();
const [urlParameters, setUrlParam] = useUrlParams(['show_irrelevant', ...CVES_ALLOWED_PARAMS], shouldUseHybridSystemFilter);
const [urlParameters, setUrlParam] = useUrlParams(
['show_irrelevant', ...CVES_ALLOWED_PARAMS],
shouldUseHybridSystemFilter
);

const apply = (filterParams = {}) => {
const params = constructFilterParameters(filterParams);
dispatch(changeCveListParameters(params));
};

useEffect(() => {
apply({ ...getCveDefaultFilters(shouldUseHybridSystemFilter), ...urlParameters });
apply(
isEmpty(urlParameters)
? getCveDefaultFilters(shouldUseHybridSystemFilter, includesCvesWithoutErrata)
: urlParameters
);
}, [shouldUseHybridSystemFilter]);

useEffect(() => {
if (isFirstLoad) {
setFirstLoad(false);
}
else {
dispatch(fetchCveListByAccount(parameters, shouldUseHybridSystemFilter));
setUrlParam({ ...parameters });
}
}, [parameters, isFirstLoad, shouldUseHybridSystemFilter]);
dispatch(fetchCveListByAccount(parameters, shouldUseHybridSystemFilter));
setUrlParam({ ...parameters });
}, [parameters, shouldUseHybridSystemFilter]);

useEffect(() => {
return () => {
Expand All @@ -103,46 +108,16 @@ export const CVEs = ({ rbac }) => {
dispatch(selectCve(cveNames || []));
};

const downloadReport = format => {
DownloadReport.exec(
fetchCveListByAccount,
parameters,
format,
'cves',
notification => dispatch(addNotification(notification)),
() => dispatch(clearNotifications()),
shouldUseHybridSystemFilter
);
};

const showBusinessRiskModal = (cvesList, goToFirstPage) => {
const { meta } = cves;
setBusinessRiskModal(() => () =>
<BusinessRiskModal
cves={cvesList}
updateRef={() => {
setFirstLoad(true);
dispatch(clearCVEsStore());
updateRef(goToFirstPage ? { ...meta, page: 1 } : meta, parameters, apply);
}}
/>
);
};
const downloadReport = useDownloadReport(
parameters,
shouldUseHybridSystemFilter
);

const showStatusModal = (cvesList, goToFirstPage) => {
const { meta } = cves;
setStatusModal(() => () =>
<StatusModal
cves={cvesList}
canEditPairStatus={canEditPairStatus}
updateRef={() => {
setFirstLoad(true);
dispatch(clearCVEsStore());
updateRef(goToFirstPage ? { ...meta, page: 1 } : meta, parameters, apply);
}}
/>
);
};
const [CveBusinessRiskModal, showBusinessRiskModal] = useShowModal(BusinessRiskModal);
const [CveStatusModal, showStatusModal] = useShowModal(
StatusModal,
{ canEditPairStatus }
);

const openCves = (cves) => {
dispatch(expandCve(cves));
Expand Down
48 changes: 18 additions & 30 deletions src/Components/SmartComponents/CVEs/CVEs.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { useHybridSystemFilterFlag } from '../../../Helpers/Hooks';
import TestWrapper from '../../../Utilities/TestWrapper';
import { cveState } from '../mockData.fixtures';
import '@testing-library/jest-dom';
import { ComponentWithContext } from '../../../Utilities/TestingUtilities';

jest.mock('../../../Helpers/Hooks', () => ({
...jest.requireActual('../../../Helpers/Hooks'),
Expand All @@ -34,27 +35,16 @@ jest.mock("../../../Helpers/DownloadReport", () => ({
exec: jest.fn()
}));

const customMiddleWare = store => next => action => {
useSelector.mockImplementation(callback => {
return callback({ CVEsStore: cveState });
});
next(action);
}

const mockStore = configureStore([customMiddleWare]);
const mockStore = configureStore([]);
const store = mockStore(initialState);

beforeEach(() => {
store.clearActions();
useSelector.mockImplementation(callback => {
return callback({ CVEsStore: initialState });
return callback({ CVEsStore: cveState });
});
});

afterEach(() => {
useSelector.mockClear();
});

describe('CVEs', () => {
it('Should dispatch CHANGE_CVE_LIST_PARAMETERS and FETCH_CVE_LIST only once on load', () => {
render(
Expand Down Expand Up @@ -83,26 +73,24 @@ describe('CVEs', () => {
});

it('Should generate error', () => {
const customMiddleWareErrors = store => next => action => {
useSelector.mockImplementation(callback => {
return callback({
CVEsStore: {
...initialState,
cveList: { isLoading: false, payload: { errors: true, meta: {}, data: [] } }
}
});
useSelector.mockImplementation(callback => {
return callback({
CVEsStore: {
...initialState,
cveList: { isLoading: false, payload: { errors: true, meta: {}, data: [] } }
}
});
next(action);
}
});

const mockStoreErrors = configureStore([customMiddleWareErrors]);
const storeForErrors = mockStoreErrors(initialState);
const mockStoreErrors = configureStore([]);
const storeForErrors = mockStoreErrors();

render(
<TestWrapper store={ storeForErrors }>
<CVEs />
</TestWrapper>
);

expect(screen.getByRole('heading', { name: 'Something went wrong' })).toBeVisible();
});

Expand Down Expand Up @@ -240,12 +228,12 @@ describe('CVEs', () => {

});

describe('CVEs RTL', () => {
describe('CVEs default filters are enabled, no other filters are added in the URL', () => {
it('Should show extended Systems filter when edge parity feature is disabled', async () => {
render(
<TestWrapper store={ store }>
<ComponentWithContext store={ store } renderOptions={{ initialEntries: ['?affecting=rpmdnf%2Cedge&advisory_available=true']}}>
<CVEs />
</TestWrapper>
</ComponentWithContext>
);
Comment on lines +234 to 237
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like TestWrapper and ComponentWithContext both deal with the same problem: render a component with the necessary providers and contexts. We have to stop duplicating these two components and just stick to only one. I see that TestWrapper doesn't support initialEntries: let's perhaps extend it and keep the usage of it here in this test file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would remove TestWrapper. Because ComponentWithContext follows the conventions that we have in other apps as well. That is why, I replaced TestWrapper with ComponentWithContext. Let's proceed with the review of this PR, I will, first of all, push the component into FEC so that we can share it among apps, and then adopt it in the vuln app in the next PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree that TestWrapper is the better name for the component rather than ComponentWithContext . While pushing to FEC, I will keep this in mind.


fireEvent.click(screen.getByRole('button', {
Expand All @@ -266,9 +254,9 @@ describe('CVEs RTL', () => {

it('Should show extended Systems filter when edge parity feature is enabled', async () => {
useHybridSystemFilterFlag.mockReturnValue(true);
render(<TestWrapper store={ store }>
render(<ComponentWithContext store={ store } renderOptions={{ initialEntries: ['?affecting=rpmdnf%2Cedge&advisory_available=true']}}>
<CVEs />
</TestWrapper>);
</ComponentWithContext>);

fireEvent.click(screen.getByRole('button', {
name: /conditional filter/i
Expand Down
58 changes: 56 additions & 2 deletions src/Components/SmartComponents/CVEs/CVEsAssets.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,17 @@

import React, { useState, useCallback, useContext } from 'react';
import { classNames, expandable, sortable, nowrap, wrappable } from '@patternfly/react-table';
import messages from '../../../Messages';
import { intl } from '../../../Utilities/IntlProvider';
import DownloadReport from '../../../Helpers/DownloadReport';
import {
addNotification,
clearNotifications
} from '@redhat-cloud-services/frontend-components-notifications/redux';
import { useDispatch } from 'react-redux';
import { fetchCveListByAccount, clearCVEsStore } from '../../../Store/Actions/Actions';
import { updateRef } from '../../../Helpers/MiscHelper';
import { CVETableContext } from './CVEs';

export const VULNERABILITIES_HEADER = [
{
Expand Down Expand Up @@ -53,6 +63,50 @@ export const VULNERABILITIES_HEADER = [
}
];

export const getCveDefaultFilters = (shouldUseHybridSystemFilter) => {
return { affecting: shouldUseHybridSystemFilter ? 'rpmdnf,edge' : 'true' };
export const getCveDefaultFilters = (shouldUseHybridSystemFilter, includesCvesWithoutErrata) => {
return {
affecting: shouldUseHybridSystemFilter ? 'rpmdnf,edge' : 'true',
...includesCvesWithoutErrata ? { advisory_available: 'true' } : {}
};
};

export const useDownloadReport = (parameters, shouldUseHybridSystemFilter) => {
const dispatch = useDispatch();

return useCallback((format) => DownloadReport.exec(
fetchCveListByAccount,
parameters,
format,
'cves',
notification => dispatch(addNotification(notification)),
() => dispatch(clearNotifications()),
shouldUseHybridSystemFilter
), [parameters]
);
};

export const useShowModal = (Modal, modalProps = {}) => {
const [ModalComponent, setModalComponent] = useState(() => () => null);
const {
cves: { meta } = {},
params,
methods: { apply } = {}
} = useContext(CVETableContext);

const dispatch = useDispatch();

const showModal = useCallback((cvesList, goToFirstPage) => {
setModalComponent(() => () =>
<Modal
cves={cvesList}
updateRef={() => {
dispatch(clearCVEsStore());
updateRef(goToFirstPage ? { ...meta, page: 1 } : meta, params, apply);
}}
{...modalProps}
/>
);
}, [params, apply, meta]);

return [ModalComponent, showModal];
};
Loading
Loading