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

refactor: InventoryTable #2129

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

refactor: InventoryTable #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
18 changes: 0 additions & 18 deletions doc/props_table.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,24 +100,6 @@ Example in [Patchman UI](https://github.com/RedHatInsights/patchman-ui/blob/mast

Props passed to paginations components.

## autoRefresh

*boolean*

When `true`, the table is refreshed when `customFilters` are changed.

## initialLoading

*boolean*

When `true`, the table is in loading state on mount until `entities.loaded` is set to `false` (and from that point, `loaded` is the only determinator.). Use when users can go back to already loaded table, this prop ensures that there will be no change from `loaded` > `loading` > `loaded`.

## ignoreRefresh

*boolean = true*

On the initial mount and when items/sortBy are changed, the inventoryTable ignores `onRefresh` prop. By setting the prop to false, you can control this behavior.

## showTagModal

*boolean*
Expand Down
14 changes: 2 additions & 12 deletions src/Routes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,9 @@ import React, {
createContext,
lazy,
useEffect,
useMemo,
useState,
} from 'react';
import { Navigate, useRoutes } from 'react-router-dom';
import { getSearchParams } from './constants';
import RenderWrapper from './Utilities/Wrapper';
import useFeatureFlag from './Utilities/useFeatureFlag';
import { Bullseye, Spinner } from '@patternfly/react-core';
Expand Down Expand Up @@ -46,7 +44,6 @@ export const AccountStatContext = createContext({
});

export const Routes = () => {
const searchParams = useMemo(() => getSearchParams(), []);
Copy link
Member Author

Choose a reason for hiding this comment

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

We shouldn't pass this in here. all components that use params lower in the tree should access theme vie the react router (params) hook

const [hasConventionalSystems, setHasConventionalSystems] = useState(true);
const [hasEdgeDevices, setHasEdgeDevices] = useState(true);
const edgeParityInventoryListEnabled = useFeatureFlag(
Expand All @@ -70,7 +67,7 @@ export const Routes = () => {
let element = useRoutes([
{
path: '/',
element: <RenderWrapper cmp={InventoryTable} {...searchParams} />,
element: <RenderWrapper cmp={InventoryTable} />,
},
{ path: '/:inventoryId', element: <InventoryDetail /> },
{ path: '/:inventoryId/:modalId', element: <InventoryDetail /> },
Expand All @@ -88,14 +85,7 @@ export const Routes = () => {
},
{
path: '/manage-edge-inventory',
element: (
<RenderWrapper
cmp={InventoryTable}
isRbacEnabled
{...searchParams}
isImmutableTabOpen
/>
),
element: <RenderWrapper cmp={InventoryTable} isImmutableTabOpen />,
},
{
path: '*',
Expand Down
10 changes: 1 addition & 9 deletions src/Utilities/Wrapper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,7 @@ import PropTypes from 'prop-types';
import { usePermissionsWithContext } from '@redhat-cloud-services/frontend-components-utilities/RBACHook';
import { GENERAL_HOSTS_READ_PERMISSIONS } from '../constants';

const RenderWrapper = ({
cmp: Component,
isRbacEnabled,
Copy link
Member Author

Choose a reason for hiding this comment

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

This prop is not really needed or used (please double check)

inventoryRef,
store,
...props
}) => {
const RenderWrapper = ({ cmp: Component, inventoryRef, store, ...props }) => {
const { hasAccess } = usePermissionsWithContext(
[GENERAL_HOSTS_READ_PERMISSIONS],
true,
Expand All @@ -22,7 +16,6 @@ const RenderWrapper = ({
{...(inventoryRef && {
ref: inventoryRef,
})}
isRbacEnabled={isRbacEnabled}
hasAccess={hasAccess}
store={store}
/>
Expand All @@ -34,7 +27,6 @@ RenderWrapper.propTypes = {
inventoryRef: PropTypes.any,
store: PropTypes.object,
customRender: PropTypes.bool,
isRbacEnabled: PropTypes.bool,
};

export default RenderWrapper;
2 changes: 0 additions & 2 deletions src/components/GroupSystems/GroupSystems.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ const GroupSystems = ({ groupName, groupId }) => {
<InventoryTable
columns={(columns) => prepareColumns(columns, true)}
hideFilters={{ hostGroupFilter: true }}
initialLoading
getEntities={async (items, config, showTags, defaultGetEntities) =>
await defaultGetEntities(
items,
Expand Down Expand Up @@ -224,7 +223,6 @@ const GroupSystems = ({ groupName, groupId }) => {
ref={inventory}
showCentosVersions
customFilters={{ globalFilter }}
autoRefresh
/>
)}
</div>
Expand Down
2 changes: 0 additions & 2 deletions src/components/ImmutableDevices/ImmutableDevices.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ const ImmutableDevices = ({

return (
<InventoryTable
initialLoading
disableDefaultColumns
onLoad={onLoad}
hideFilters={hideFilters}
Expand All @@ -54,7 +53,6 @@ const ImmutableDevices = ({
hasCheckbox={false}
isFullView
ref={inventoryRef}
autoRefresh
key="inventory"
customFilters={customFilters}
columns={(defaultColumns) => mergeColumns(defaultColumns)}
Expand Down
1 change: 0 additions & 1 deletion src/components/InventoryGroups/Modals/AddSystemsToGroupModal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,6 @@ const AddSystemsToGroupModal = ({
canSelectAll: false,
}}
bulkSelect={bulkSelectConfig}
initialLoading={true}
showTags
showCentosVersions
showNoGroupOption
Expand Down
2 changes: 0 additions & 2 deletions src/components/InventoryTable/EntityTable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,6 @@ EntityTable.propTypes = {
hasCheckbox: PropTypes.bool,
showActions: PropTypes.bool,
hasItems: PropTypes.bool,
showHealth: PropTypes.bool,
sortBy: PropTypes.shape({
key: PropTypes.string,
direction: PropTypes.oneOf(['asc', 'desc']),
Expand Down Expand Up @@ -197,7 +196,6 @@ EntityTable.propTypes = {

EntityTable.defaultProps = {
loaded: false,
showHealth: false,
expandable: false,
hasCheckbox: true,
showActions: false,
Expand Down
53 changes: 15 additions & 38 deletions src/components/InventoryTable/EntityTableToolbar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,35 +237,15 @@ const EntityTableToolbar = ({
*/
const onRefreshDataInner = useCallback(
(options) => {
console.log('OPSSSS', options);
if (hasAccess) {
onRefreshData(options);
if (showTags && !hasItems) {
dispatch(fetchAllTags(filterTagsBy, {}, getTags));
}
}
},
[customFilters?.tags]
);

/**
* Function used to update data, it either calls `onRefresh` from props or dispatches `onRefreshData`.
* `onRefresh` function takes two parameters
* * entire config with new changes.
* * callback to update data.
* @param {*} config new config to fetch data.
*/
const updateData = (config) => {
if (hasAccess) {
onRefreshDataInner(config);
}
};

/**
* Debounced `updateData` function.
*/
const debouncedRefresh = useCallback(
Copy link
Member Author

Choose a reason for hiding this comment

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

This is mainly the culprit as far as I remember, starting from line 247.

These functions and callbacks were called when they shouldn't have been, due to the debouncing here. These changes in the table and the toolbar change that a little and consolidate everything to only call the ONE debounced refresh function in the EntityTable above.

debounce((config) => updateData(config), 800),
[sortBy?.key, sortBy?.direction]
[hasAccess]
);

/**
Expand All @@ -284,7 +264,6 @@ const EntityTableToolbar = ({
hostGroupFilter,
} = reduceFilters([...(filters || []), ...(customFilters?.filters || [])]);

debouncedRefresh();
enabledFilters.name && setTextFilter(textFilter);
enabledFilters.stale && setStaleFilter(staleFilter);
enabledFilters.registeredWith &&
Expand All @@ -303,7 +282,7 @@ const EntityTableToolbar = ({
* @param {*} value new value used for filtering.
* @param {*} debounced if debounce function should be used.
*/
const onSetTextFilter = (value, debounced = true) => {
const onSetTextFilter = (value) => {
const trimmedValue = value?.trim();

const textualFilter = filters?.find(
Expand All @@ -315,8 +294,7 @@ const EntityTableToolbar = ({
filters?.push({ value: TEXT_FILTER, filter: trimmedValue });
}

const refresh = debounced ? debouncedRefresh : updateData;
refresh({ page: 1, perPage, filters });
onRefreshDataInner({ page: 1, perPage, filters });
};

/**
Expand Down Expand Up @@ -352,7 +330,7 @@ const EntityTableToolbar = ({

useEffect(() => {
if (shouldReload && enabledFilters.stale) {
onSetFilter(staleFilter, 'staleFilter', debouncedRefresh);
onSetFilter(staleFilter, 'staleFilter', onRefreshDataInner);
}
}, [staleFilter]);

Expand All @@ -361,44 +339,44 @@ const EntityTableToolbar = ({
onSetFilter(
registeredWithFilter,
'registeredWithFilter',
debouncedRefresh
onRefreshDataInner
);
}
}, [registeredWithFilter]);

useEffect(() => {
if (shouldReload && showTags && enabledFilters.tags) {
onSetFilter(mapGroups(selectedTags), 'tagFilters', debouncedRefresh);
onSetFilter(mapGroups(selectedTags), 'tagFilters', onRefreshDataInner);
}
}, [selectedTags]);

useEffect(() => {
if (shouldReload && enabledFilters.operatingSystem) {
onSetFilter(osFilterValue, 'osFilter', debouncedRefresh);
onSetFilter(osFilterValue, 'osFilter', onRefreshDataInner);
}
}, [osFilterValue]);

useEffect(() => {
if (shouldReload && enabledFilters.rhcdFilter) {
onSetFilter(rhcdFilterValue, 'rhcdFilter', debouncedRefresh);
onSetFilter(rhcdFilterValue, 'rhcdFilter', onRefreshDataInner);
}
}, [rhcdFilterValue]);

useEffect(() => {
if (shouldReload && enabledFilters.lastSeenFilter) {
onSetFilter(lastSeenFilterValue, 'lastSeenFilter', debouncedRefresh);
onSetFilter(lastSeenFilterValue, 'lastSeenFilter', onRefreshDataInner);
}
}, [lastSeenFilterValue]);

useEffect(() => {
if (shouldReload && enabledFilters.updateMethodFilter) {
onSetFilter(updateMethodValue, 'updateMethodFilter', debouncedRefresh);
onSetFilter(updateMethodValue, 'updateMethodFilter', onRefreshDataInner);
}
}, [updateMethodValue]);

useEffect(() => {
if (shouldReload && enabledFilters.hostGroupFilter) {
onSetFilter(hostGroupValue, 'hostGroupFilter', debouncedRefresh);
onSetFilter(hostGroupValue, 'hostGroupFilter', onRefreshDataInner);
}
}, [hostGroupValue]);

Expand All @@ -410,7 +388,7 @@ const EntityTableToolbar = ({
[TAG_CHIP]: (deleted) =>
setSelectedTags(
onDeleteTag(deleted, selectedTags, (selectedTags) =>
onSetFilter(mapGroups(selectedTags), 'tagFilters', updateData)
onSetFilter(mapGroups(selectedTags), 'tagFilters', onRefreshDataInner)
)
),
[STALE_CHIP]: (deleted) =>
Expand Down Expand Up @@ -454,7 +432,7 @@ const EntityTableToolbar = ({
setEndDate();
setStartDate(oldestDate);
dispatch(setFilter([]));
updateData({ page: 1, filters: [] });
onRefreshDataInner({ page: 1, filters: [] });
};

/**
Expand Down Expand Up @@ -637,7 +615,6 @@ EntityTableToolbar.propTypes = {
paginationProps: PropTypes.object,
loaded: PropTypes.bool,
onRefresh: PropTypes.func,
hasCheckbox: PropTypes.bool,
isLoaded: PropTypes.bool,
items: PropTypes.array,
sortBy: PropTypes.object,
Expand All @@ -650,7 +627,7 @@ EntityTableToolbar.propTypes = {

EntityTableToolbar.defaultProps = {
showTags: false,
hasAccess: true,
hasAccess: false,
activeFiltersConfig: {},
hideFilters: {},
showNoGroupOption: false,
Expand Down
Loading