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.

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

Resource tab optimization #1435

Merged
merged 35 commits into from
Nov 24, 2021
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
360e920
saving temp changes
arivepr Sep 23, 2021
b060909
Merge branch 'master' into ResourceTabOptimization
arivepr Sep 23, 2021
f66ace8
Merge branch 'master' into ResourceTabOptimization
arivepr Oct 3, 2021
0f725de
testing bits
arivepr Oct 4, 2021
41b0aa2
Merge branch 'master' into ResourceTabOptimization
arivepr Oct 5, 2021
c16159d
further testing
arivepr Oct 6, 2021
d6a7d8e
Merge branch 'master' into ResourceTabOptimization
arivepr Oct 6, 2021
862b09e
more tests
arivepr Oct 6, 2021
9f9fa98
Merge branch 'master' into ResourceTabOptimization
arivepr Oct 21, 2021
990535d
adding api call
arivepr Oct 28, 2021
ffc7b1f
Adding new validation for resource tab
arivepr Nov 1, 2021
0c28d0d
Merge branch 'master' into ResourceTabOptimization
arivepr Nov 1, 2021
cb55806
code change
arivepr Nov 2, 2021
7e5812f
Merge branch 'master' into ResourceTabOptimization
arivepr Nov 2, 2021
56c9b9b
adding reducer mutators for ros visibility
arivepr Nov 4, 2021
ee2e804
adding reducer mutators for ros visibility
arivepr Nov 4, 2021
4a47bd5
touch up
arivepr Nov 4, 2021
c1dc3ec
Merge branch 'ResourceTabOptimization' of github.com:RedHatInsights/i…
arivepr Nov 4, 2021
704fdde
General code cleanup and linting
arivepr Nov 4, 2021
17edc4d
Merge branch 'master' into ResourceTabOptimization
arivepr Nov 4, 2021
3575e61
re adding rendering condition to resource tab
arivepr Nov 4, 2021
8269f05
saving changes
arivepr Nov 17, 2021
9c3a67f
Merge branch 'master' into ResourceTabOptimization
arivepr Nov 17, 2021
a1edabf
Adding reducer and component changes
arivepr Nov 17, 2021
e4a4a8a
Testing new logic
arivepr Nov 17, 2021
5588d18
adding conditional statement for tabs filtering
arivepr Nov 17, 2021
8445da6
Fixed logic, working as needed now.
arivepr Nov 17, 2021
4d12d28
Cleaning up code
arivepr Nov 17, 2021
f8d09f8
Adding adjustment for test
arivepr Nov 17, 2021
b33a445
Merge branch 'master' into ResourceTabOptimization
arivepr Nov 19, 2021
f972f94
Merge branch 'master' into ResourceTabOptimization
arivepr Nov 23, 2021
4eb549f
Addressing changes for code optimization
arivepr Nov 23, 2021
b2f87ed
Merge branch 'ResourceTabOptimization' of github.com:RedHatInsights/i…
arivepr Nov 23, 2021
327f6e3
Fixed failing tests
arivepr Nov 23, 2021
80a28e0
Adding wrapping for code
arivepr Nov 23, 2021
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
1 change: 0 additions & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

23 changes: 18 additions & 5 deletions src/components/InventoryDetail/ApplicationDetails.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useEffect } from 'react';
import React, { useEffect, useState } from 'react';
import PropTypes from 'prop-types';
import { useSelector, useDispatch } from 'react-redux';
import { useLocation, useHistory } from 'react-router-dom';
Expand All @@ -14,11 +14,14 @@ const ApplicationDetails = ({ onTabSelect, appList, ...props }) => {
const history = useHistory();
const dispatch = useDispatch();
const searchParams = new URLSearchParams(search);
const items = useSelector(({ entityDetails }) => entityDetails?.activeApps);
const items = useSelector(({ entityDetails }) => entityDetails?.activeApps.filter(({ isVisible }) => isVisible !== false));
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add another guard in here

Suggested change
const items = useSelector(({ entityDetails }) => entityDetails?.activeApps.filter(({ isVisible }) => isVisible !== false));
const items = useSelector(({ entityDetails }) => (entityDetails?.activeApps || []).filter(({ isVisible }) => isVisible !== false));

const activeApp = useSelector(({ entityDetails }) => entityDetails?.activeApp);
const disabledApps = useSelector(({ systemProfileStore }) => systemProfileStore?.disabledApps);
const defaultApp = activeApp?.appName || appList?.find(({ pageId, name }) => items?.[0]?.name === (
pageId || name))?.name || items?.[0]?.name;
const applications = appList || items;
let applications = appList || items;
const [activeTabs, setActiveTabs] = useState(applications);
Comment on lines +23 to +24
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be simplified to directly assign to default state

Suggested change
let applications = appList || items;
const [activeTabs, setActiveTabs] = useState(applications);
const [activeTabs, setActiveTabs] = useState(appList || items);


useEffect(() => {
/**
* Initialize first inventory detail type
Expand All @@ -29,6 +32,16 @@ const ApplicationDetails = ({ onTabSelect, appList, ...props }) => {
}
}, []);

useEffect(() => {
const filteredResult = disabledApps && disabledApps.length && applications.filter(app => app.name !== disabledApps[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be simplified with safe accessor, using includes and fallbacking to original value

Suggested change
const filteredResult = disabledApps && disabledApps.length && applications.filter(app => app.name !== disabledApps[0]);
// filters out disabled apps, if no disabledApps is used it will use the full applications list
const filteredResult = applications.filter(app => !disabledApps?.includes(app.name));
setActiveTabs(filteredResult)

if (filteredResult !== 0 && typeof filteredResult !== undefined) {
setActiveTabs(filteredResult);
}
else {
setActiveTabs(applications);
}
}, [disabledApps]);

return (
<React.Fragment>
{
Expand All @@ -37,7 +50,7 @@ const ApplicationDetails = ({ onTabSelect, appList, ...props }) => {
{...props}
activeKey={ defaultApp }
onSelect={ (event, item) => {
const activeItem = applications.find(oneApp => oneApp.name === item);
const activeItem = activeTabs.find(oneApp => oneApp.name === item);
if (onTabSelect) {
onTabSelect(event, item, activeItem);
} else {
Expand All @@ -50,7 +63,7 @@ const ApplicationDetails = ({ onTabSelect, appList, ...props }) => {
isFilled
className="ins-c-inventory-detail__app-tabs"
>
{ applications.map((item, key) => (
{ activeTabs && activeTabs.length && activeTabs.map((item, key) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be simplified with safe accessor

Suggested change
{ activeTabs && activeTabs.length && activeTabs.map((item, key) => (
{ activeTabs?.map((item, key) => (

<Tab key={ key } eventKey={ item.name } title={ item.title }></Tab>
)) }
</Tabs>
Expand Down
2 changes: 0 additions & 2 deletions src/components/InventoryTable/InventoryTable.js
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,6 @@ const InventoryTable = forwardRef(({ // eslint-disable-line react/display-name
const onRefreshData = (options = {}, disableOnRefresh) => {
const { activeFilters } = store.getState().entities;
const cachedProps = cache.current?.getProps() || {};

// eslint-disable-next-line camelcase
const currPerPage = options?.per_page || options?.perPage || cachedProps.perPage;

const params = {
Expand Down
1 change: 1 addition & 0 deletions src/store/action-types.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,4 @@ export const CLEAR_FILTERS = 'CLEAR_FILTERS';
export const TOGGLE_TAG_MODAL = 'TOGGLE_TAG_MODAL';
export const CONFIG_CHANGED = 'CONFIG_CHANGED';
export const TOGGLE_DRAWER = 'TOGGLE_INVENTORY_DRAWER';
export const SET_ROS_TAB_VISBILITY = 'SET_ROS_TAB_VISBILITY';
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this const

5 changes: 5 additions & 0 deletions src/store/inventory-actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,11 @@ export const deleteEntity = (systems, displayName) => ({
}
});

// export const setRosTabVisibility = (shouldShowRosTab) => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for commented out code

// type: SET_ROS_TAB_VISBILITY,
// payload: shouldShowRosTab
// });

export const configChanged = (config) => ({
type: CONFIG_CHANGED,
payload: config
Expand Down
29 changes: 22 additions & 7 deletions src/store/reducers.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
import { INVENTORY_ACTION_TYPES, ACTION_TYPES, SELECT_ENTITY, SET_INVENTORY_FILTER, SET_PAGINATION } from './action-types';
import {
INVENTORY_ACTION_TYPES,
ACTION_TYPES,
SELECT_ENTITY,
SET_INVENTORY_FILTER,
SET_PAGINATION
} from './action-types';
import systemProfileStore from './systemProfileStore';
import {
ComplianceTab,
Expand Down Expand Up @@ -64,6 +70,7 @@ function entityLoaded(state) {
(!insights.chrome.isProd || (insights.chrome.isProd && insights?.chrome?.isBeta())) && {
title: 'Resource Optimization',
name: 'ros',
isVisible: false,
component: RosTab
}
].filter(Boolean)
Expand Down Expand Up @@ -95,6 +102,17 @@ function entitySelected(state, { payload }) {
};
}

function resourceOptTabVisibility(state, { payload }) {
return {
...state,
activeApps: state.activeApps?.map((entity) => entity.name === 'ros' ? ({
...entity,
isVisible: payload
}) : entity
)
};
}

function entityDeleted(state, { meta }) {
const selected = state.selected || (new Map());
meta.systems.forEach(id => selected.delete(id));
Expand All @@ -105,11 +123,7 @@ function entityDeleted(state, { meta }) {
};
}

function onEntitiesLoaded(state, { payload, meta }) {
if (meta?.lastDateRequest < state?.lastDateRequest) {
return state;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@rvsia this was added to prevent race conditions, right? I don't think we can remove this.


function onEntitiesLoaded(state, { payload }) {
return {
...state,
rows: mergeArraysByKey([state.rows, payload.results.map(result => {
Expand Down Expand Up @@ -160,7 +174,8 @@ export const tableReducer = applyReducerHash(

export const entitesDetailReducer = () => applyReducerHash(
{
[INVENTORY_ACTION_TYPES.LOAD_ENTITY_FULFILLED]: entityLoaded
[INVENTORY_ACTION_TYPES.LOAD_ENTITY_FULFILLED]: entityLoaded,
[INVENTORY_ACTION_TYPES.LOAD_SYSTEM_PROFILE_FULFILLED]: resourceOptTabVisibility
},
defaultState
);
Expand Down
6 changes: 6 additions & 0 deletions src/store/systemProfileStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,14 @@ export function calculateInterfaces(interfaces) {

export function onSystemProfile(state, { payload: { results } }) {
const systemProfile = (results && results[0] && results[0].system_profile) || {};
const cloudProviderObj = (results && results[0] && (typeof results[0].system_profile.cloud_provider !== 'undefined'))
&& results[0].system_profile.cloud_provider;
return {
...state,
disabledApps: [
...(cloudProviderObj === 'aws' || cloudProviderObj === 'test cloud provider') ? [] : ['ros']
// ...(cloudProviderObj === 'aws' || cloudProviderObj === 'azure') ? ['ros'] : []
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we actually want this code to present and the one above not, right?

],
systemProfile: {
loaded: true,
...systemProfile,
Expand Down