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(RHINENG-9836) duplicate fetch on systems table #2245

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+361 −488
Open

fix(RHINENG-9836) duplicate fetch on systems table #2245

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.

Comment on lines -103 to -120
Copy link
Contributor

Choose a reason for hiding this comment

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

These parameters can be used by other applications that use InventoryTable. Have we checked if this is not a breaking change?

Copy link
Member

@bastilian bastilian Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

As far as I remember when making the original changes, these props were either already not used or the changes removed the behaviour these props would enable and made them obsolete.

The behaviours these props enabled were mostly workarounds and unintuitive. For example the autoRefresh is kinda redundant, because we should always refresh the table when customFilters change, otherwise we have the contents and filters out of sync. This was most likely a "fix" for a bug that caused the table not to get updated automatically.

However, in any case we should test these changes with some/all apps and maybe do a quick GitHub search for these specifically.

Copy link
Contributor

@gkarat gkarat Aug 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

OK, I think I understand. Still, there are a bunch of applications that use these parameters. Which implies a chance that this PR will introduce bugs:

https://github.com/search?q=org%3ARedHatInsights+autoRefresh&type=code
https://github.com/search?q=org%3ARedHatInsights+initialLoading&type=code
https://github.com/search?q=org%3ARedHatInsights+ignoreRefresh&type=code

@johnsonm325, could we make sure this PR is tested with the apps that are listed in these searches?

## showTagModal

*boolean*
Expand Down
4 changes: 1 addition & 3 deletions src/Routes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,7 @@ export const Routes = () => {
},
{
path: '/manage-edge-inventory',
element: (
<RenderWrapper cmp={InventoryTable} isRbacEnabled 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,
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 @@ -137,7 +137,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 @@ -213,7 +212,6 @@ const GroupSystems = ({ groupName, groupId }) => {
ref={inventory}
showCentosVersions
customFilters={{ filters: initialFilters, globalFilter }}
autoRefresh
onRefresh={onRefresh}
ignoreRefresh
/>
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 @@ -171,7 +171,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 @@ -158,7 +158,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 @@ -186,7 +185,6 @@ EntityTable.propTypes = {

EntityTable.defaultProps = {
loaded: false,
showHealth: false,
expandable: false,
hasCheckbox: true,
showActions: false,
Expand Down
68 changes: 25 additions & 43 deletions src/components/InventoryTable/EntityTableToolbar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* eslint-disable camelcase */
import './EntityTableToolbar.scss';
import React, { Fragment, useEffect, useReducer } from 'react';
import React, { Fragment, useCallback, useEffect, useReducer } from 'react';
import { useDispatch, useSelector } from 'react-redux';
import PropTypes from 'prop-types';
import {
Expand Down Expand Up @@ -267,32 +267,17 @@ const EntityTableToolbar = ({
/**
* Function to dispatch load systems and fetch all tags.
*/
const onRefreshDataInner = (options) => {
if (hasAccess) {
onRefreshData(options);
if (showTags && !hasItems) {
dispatch(fetchAllTags(filterTagsBy, {}, getTags));
const onRefreshDataInner = useCallback(
(options) => {
if (hasAccess) {
onRefreshData(options);
if (showTags && !hasItems) {
dispatch(fetchAllTags(filterTagsBy, {}, getTags));
}
}
}
};

/**
* 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 = debounce((config) => updateData(config), 800);
},
[hasAccess]
);

/**
* Component did mount effect to calculate actual filters from redux.
Expand All @@ -314,7 +299,6 @@ const EntityTableToolbar = ({
...(customFilters?.filters || []),
]);

debouncedRefresh();
enabledFilters.name && setTextFilter(textFilter);
enabledFilters.stale && setStaleFilter(staleFilter);
enabledFilters.registeredWith &&
Expand All @@ -334,7 +318,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 = activeFilters?.find(
Expand All @@ -347,8 +331,7 @@ const EntityTableToolbar = ({
activeFilters?.push({ value: TEXT_FILTER, filter: trimmedValue });
}

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

/**
Expand Down Expand Up @@ -384,7 +367,7 @@ const EntityTableToolbar = ({

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

Expand All @@ -393,50 +376,50 @@ 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]);

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

Expand All @@ -448,7 +431,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 @@ -490,7 +473,7 @@ const EntityTableToolbar = ({
setEndDate();
setStartDate(oldestDate);
dispatch(setFilter([]));
updateData({ page: 1, filters: [] });
onRefreshDataInner({ page: 1, filters: [] });
};

/**
Expand Down Expand Up @@ -686,7 +669,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 @@ -702,7 +684,7 @@ EntityTableToolbar.propTypes = {

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