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

fix(SystemsPage): RHINENG-5977 - Fix URL params filters handling #2129

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(SystemsPage): RHINENG-5977 - Fix URL params filters handling #2129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion src/Components/SmartComponents/CVEs/CVEs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ 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, useUrlParams } from '../../../Helpers/MiscHelper';
import { constructFilterParameters } from '../../../Helpers/MiscHelper';
import useUrlParams from '../../../Helpers/useUrlParams';

import BusinessRiskModal from '../Modals/BusinessRiskModal';
import StatusModal from '../Modals/CveStatusModal';
import CVEsTable from './CVEsTable';
Expand Down
6 changes: 4 additions & 2 deletions src/Components/SmartComponents/HybridInventory/ImmutableDevicesTab/ImmutableDevices.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import React, { useEffect } from 'react';
import propTypes from 'prop-types';
import { injectIntl } from 'react-intl';
import { useUrlParams } from '../../../../Helpers/MiscHelper';
import useUrlParams from '../../../../Helpers/useUrlParams';
import { useDispatch, useSelector } from 'react-redux';
import useInsightsNavigate from '@redhat-cloud-services/frontend-components-utilities/useInsightsNavigate';
import ReducerRegistry from '../../../../Utilities/ReducerRegistry';
Expand Down Expand Up @@ -47,7 +47,9 @@ const ImmutableDevices = ({ intl, cveName, filterRuleValues, inventoryRef, heade

useEffect(() => apply(urlParameters), []);

useEffect(() => setUrlParams({ ...parameters, ...headerFilters }), [parameters, headerFilters]);
useEffect(() => (
setUrlParams({ ...parameters, ...headerFilters })
), [JSON.stringify(parameters), JSON.stringify(headerFilters)]);

const getEntities = useGetEntities(
{
Expand Down
3 changes: 2 additions & 1 deletion src/Components/SmartComponents/SystemCves/SystemCves.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,10 @@ import { useDispatch, useSelector } from 'react-redux';
import { CVES_ALLOWED_PARAMS, SYSTEM_DETAILS_HEADER } from '../../../Helpers/constants';
import {
constructFilterParameters,
useUrlParams,
updateRef
} from '../../../Helpers/MiscHelper';
import useUrlParams from '../../../Helpers/useUrlParams';

import { createCveListBySystem } from '../../../Helpers/VulnerabilityHelper';
import {
Stack,
Expand Down
3 changes: 2 additions & 1 deletion src/Components/SmartComponents/SystemsExposedTable/SystemsExposedTable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import propTypes from 'prop-types';
import { injectIntl } from 'react-intl';
import messages from '../../../Messages';
import { translateUrlSortParameter, useUrlParams } from '../../../Helpers/MiscHelper';
import { translateUrlSortParameter } from '../../../Helpers/MiscHelper';
import useUrlParams from '../../../Helpers/useUrlParams';
import { useDispatch, useSelector, shallowEqual } from 'react-redux';
import React, { Fragment, useEffect, useState } from 'react';
import CvePairStatusModal from '../Modals/CvePairStatusModal';
Expand Down
8 changes: 3 additions & 5 deletions src/Components/SmartComponents/SystemsPage/SystemsPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ import {
fetchSystems,
fetchSystemsIds
} from '../../../Store/Actions/Actions';
import { translateUrlSortParameter, useUrlParams } from '../../../Helpers/MiscHelper';
import { translateUrlSortParameter } from '../../../Helpers/MiscHelper';
import useUrlParams from '../../../Helpers/useUrlParams';
import { InventoryTable } from '@redhat-cloud-services/frontend-components/Inventory';
import ErrorHandler from '../../PresentationalComponents/ErrorHandler/ErrorHandler';
import { TableVariant } from '@patternfly/react-table';
Expand Down Expand Up @@ -63,7 +64,6 @@ const SystemsPage = () => {

const items = useSelector(({ entities }) => entities?.rows || [], shallowEqual);
const totalItems = useSelector(({ entities }) => entities?.total);
const meta = useSelector(({ entities }) => entities?.meta);
const selectedRows = useSelector(({ entities }) => entities?.selectedRows || []);
const selectedRowsCount = useSelector(({ entities }) => entities?.selectedRowsCount ?? 0);
const isLoaded = useSelector(({ entities }) => entities?.loaded || false);
Expand Down Expand Up @@ -101,8 +101,6 @@ const SystemsPage = () => {

useEffect(() => apply(urlParameters), []);

useEffect(() => setUrlParams({ ...parameters, ...meta }), [setUrlParams, parameters, meta]);

const handleSelect = (payload, selecting) => dispatch(selectRows(payload, selecting));

const onRefreshInventory = () => {
Expand All @@ -114,7 +112,7 @@ const SystemsPage = () => {
};

const doOptOut = useOptOutSystems(onRefreshInventory);
const getEntities = useGetEntities(APIHelper.getSystems, {});
const getEntities = useGetEntities(APIHelper.getSystems, { setUrlParams });

const [columnCounter, setColumnCount] = useState(0);
useEffect(() => setColumnCount(columnCounter + 1), [columns]);
Expand Down
31 changes: 0 additions & 31 deletions src/Helpers/MiscHelper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import findIndex from 'lodash/findIndex';
import propTypes from 'prop-types';
import React from 'react';
import { impactList, PUBLIC_DATE_OPTIONS } from './constants';
import qs from 'query-string';
import { coerce, compare, rcompare } from 'semver';
import cloneDeep from 'lodash/cloneDeep';
import { Icon } from '@patternfly/react-core';
Expand Down Expand Up @@ -54,24 +53,6 @@ export function constructParameters(apiProps, allowedParams, shouldUseHybridSyst
return [];
}

// TODO DRY:similar to constructParameters
export function constructURLParameters(urlParams, allowedParams) {
if (urlParams) {
const params = { ...urlParams };
Object.keys(urlParams).forEach(
key => (
params[key] === undefined
|| params[key] === ''
|| !allowedParams.includes(key)
|| params[key] === false
)
&& delete params[key]

);
return params;
}
}

export function formatDate(date = new Date(), includeTime = false) {
const prepend = (number) => `${`${number}`.length === 1 ? '0' : ''}${number}`;
const toFormat = new Date(date);
Expand Down Expand Up @@ -173,18 +154,6 @@ export const updateStateSet = (stateSet, names, value) => {
return stateSet;
};

export const useUrlParams = (allowedParams) => {
const url = new URL(window.location);
const urlParams = qs.parse(url.search);

const setUrlParams = (parameters) => {
const searchParams = qs.stringify(constructURLParameters(parameters, allowedParams));
window.history.replaceState(null, null, `${url.origin}${url.pathname}?${searchParams}${url.hash}`);
};

return [urlParams, setUrlParams];
};

// TODO: Refactor
export const updateRef = (meta, params, apply) => {
const pages = parseInt(meta.pages);
Expand Down
32 changes: 1 addition & 31 deletions src/Helpers/MiscHelper.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,26 +169,12 @@ describe('MiscHelper', () => {
${{ publish_date: 'last90' }} | ${{ public_from: formatDate(subtractDays(90)), public_to: undefined, publish_date: 'last90' }}
${{ publish_date: 'lastYear' }} | ${{ public_from: formatDate(subtractYears(1)), public_to: undefined, publish_date: 'lastYear' }}
${{ publish_date: 'MoreThanYear' }} | ${{ public_from: undefined, public_to: formatDate(subtractYears(1)), publish_date: 'MoreThanYear' }}
`('constructFilterParameters - publish_filter', ({ publish_filter, expected_data }) => {
const filter = constructFilterParameters(publish_filter);
expect(filter).toEqual(expected_data);
});

it('useUrlParams', () => {
const mockLocation = new URL('https://localhost:1337/insights/vulnerability/cves?page=1&severity=3');
window.location = mockLocation;

const [urlParams, setUrlParams] = useUrlParams(['a', 'b']);
setUrlParams({ filter: 'testFilter' });

expect(urlParams).toEqual({
page: '1',
severity: '3'
});
expect(mockHistory.replaceState).toHaveBeenCalled();
});

it('Should updateRef update with the same page', () => {
const testParameters = { page: 10 };
const testMeta = { page: 10, pages: 10, cvesCount: 10 };
Expand Down Expand Up @@ -217,20 +203,4 @@ describe('MiscHelper', () => {
const result = formatDate();
expect(result).toEqual(expect.anything());
});

it.each`
urlParams | expected_data
${undefined} | ${undefined}
${{}} | ${{}}
${{ a: 'testValue', b: undefined }} | ${{ a: 'testValue' }}
${{ a: false, b: 'testValue' }} | ${{ b: 'testValue' }}
${{ a: 'testValue', c: 'testValue' }} | ${{ a: 'testValue' }}
${{ a: '', b: 'testValue' }} | ${{ b: 'testValue' }}
`('constructURLParameters', ({ urlParams, expected_data }) => {
const result = constructURLParameters(urlParams, ['a', 'b']);
expect(result).toEqual(expected_data);
});

});
34 changes: 34 additions & 0 deletions src/Helpers/useUrlParams.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { useCallback } from 'react';
import { useSearchParams } from 'react-router-dom';

// TODO DRY:similar to constructParameters
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it is time to finally to implement this TODO. Can we try to consolidate this function with constructParameters and get rid of the duplicate code?

export function constructURLParameters(urlParams, allowedParams) {
if (urlParams) {
const params = { ...urlParams };
Object.keys(urlParams).forEach(
key => (
params[key] === undefined
|| params[key] === ''
|| !allowedParams.includes(key)
|| params[key] === false
)
&& delete params[key]

);
return params;
}
}

export const useUrlParams = (allowedParams) => {
const [searchParams, setSearchParams] = useSearchParams();

const setUrlParams = useCallback((parameters) => {
const para = constructURLParameters(parameters, allowedParams);
Copy link
Contributor

Choose a reason for hiding this comment

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

NITPICK: It looks like a typo on the first look.

Suggested change
const para = constructURLParameters(parameters, allowedParams);
const param = constructURLParameters(parameters, allowedParams);


setSearchParams(para);
}, [JSON.stringify(allowedParams)]);

return [searchParams, setUrlParams];
};

export default useUrlParams;
35 changes: 35 additions & 0 deletions src/Helpers/useUrlParams.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { renderHook, act } from '@testing-library/react';
import { createTestWrapper } from '../Utilities/TestWrapper';
import { MemoryRouter } from 'react-router-dom';

import useUrlParams, { constructURLParameters } from './useUrlParams';

describe('MiscHelper', () => {
it('useUrlParams', async () => {
const { result } = renderHook(() => useUrlParams(['a', 'b']), {
wrapper: createTestWrapper(MemoryRouter, { initialEntries: ['/'] })
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the TestWrapper which has already MemortRouter provided?

});

await act(()=> {
result.current[1]({ a: 'testValue', b: 'testValue', c: 'testValue' });
});

expect(result.current[0].get('a')).toEqual('testValue');
expect(result.current[0].get('b')).toEqual('testValue');
expect(result.current[0].get('c')).toEqual(null);
});

it.each`
urlParams | expected_data
${undefined} | ${undefined}
${{}} | ${{}}
${{ a: 'testValue', b: undefined }} | ${{ a: 'testValue' }}
${{ a: false, b: 'testValue' }} | ${{ b: 'testValue' }}
${{ a: 'testValue', c: 'testValue' }} | ${{ a: 'testValue' }}
${{ a: '', b: 'testValue' }} | ${{ b: 'testValue' }}
`('constructURLParameters', ({ urlParams, expected_data }) => { // eslint-disable-line camelcase

const result = constructURLParameters(urlParams, ['a', 'b']);
expect(result).toEqual(expected_data);
});
});
6 changes: 6 additions & 0 deletions src/Utilities/TestWrapper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,10 @@ TestWrapper.propTypes = {
store: propTypes.object
};

export const createTestWrapper = (Wrapper = TestWrapper, props) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we use the shared wrapper, this is not needed.

return function CreatedWrapper({ children }) { // eslint-disable-line
return <Wrapper {...props}>{children}</Wrapper>;
};
};

export default TestWrapper;
Loading