convert Topology filters localStorage to userSettings #7326

sahil143 · 2020-11-25T09:36:37Z

Fixes: https://issues.redhat.com/browse/ODC-5038

Removed filter and appliedFilters from redux/local storage and added in userSettings.

Screen shots / Gifs for design review: No UI changes

Unit test coverage report:

Test setup:

Browser conformance:

Chrome
Safari
Edge
Firefox

invincibleJai · 2020-11-25T12:27:52Z

frontend/packages/topology/src/components/page/TopologyView.tsx

@@ -48,7 +43,7 @@ interface StateProps {

 interface DispatchProps {
  onSelectTab?: (name: string) => void;
-  onFiltersChange: (filters: DisplayFilters) => void;
+  // onFiltersChange: (filters: DisplayFilters) => void;


can remove comment

invincibleJai · 2020-11-26T14:06:20Z

frontend/packages/topology/src/filters/FilterProvider.tsx

+
+const TOPOLOGY_DISPLAY_FILTERS_CONFIG_MAP_KEY = `devcosole.topology.filters`;
+
+const getTopologyFilters = (appliedFilters) => {


nit: can we have type for appliedFilters

invincibleJai · 2020-11-26T14:09:12Z

frontend/packages/topology/src/filters/FilterProvider.tsx

+    TOPOLOGY_DISPLAY_FILTERS_CONFIG_MAP_KEY,
+    getDefaultAppliedFilters(),
+  );
+  const [filtersLoaded, setFiltersLoaded] = React.useState<boolean>(false);


Do we need this useState , can't we rely on appliedFiltersLoaded?

Yes, this is needed because filters depend on the data which is coming from user settings(appliedFilters). so once the user settings is loaded we need to update the filters as well and notify the component that all of the data is loaded.

invincibleJai · 2020-11-26T15:29:20Z

/cc @jerolimov

jerolimov · 2020-11-27T05:01:24Z

frontend/packages/topology/src/filters/useAppliedDisplayFilters.ts


-const useAppliedDisplayFilters = (): DisplayFilters => {
-  return useSelector((state: RootState) => getAppliedTopologyFilters(state));
+const useAppliedDisplayFilters = (): { [filiterKey: string]: boolean } => {


Typo

Suggested change

const useAppliedDisplayFilters = (): { [filiterKey: string]: boolean } => {

const useAppliedDisplayFilters = (): { [filterKey: string]: boolean } => {

jerolimov · 2020-11-27T05:02:01Z

frontend/packages/topology/src/filters/FilterProvider.tsx

+import { getAppliedFilters } from '../redux/action';
+import { DisplayFilters } from '../topology-types';
+
+const TOPOLOGY_DISPLAY_FILTERS_CONFIG_MAP_KEY = `devcosole.topology.filters`;


Suggested change

const TOPOLOGY_DISPLAY_FILTERS_CONFIG_MAP_KEY = `devcosole.topology.filters`;

const TOPOLOGY_DISPLAY_FILTERS_USERSETTINGS_KEY = `devconsole.topology.filters`;

Typo in the key. And wdyt about using usersettings key instead of configmap key?

jerolimov · 2020-11-27T05:27:39Z

@sahil143 When having a Deployment and DeploymentConfig in my topology and filter for only Deployments and reload the page, the filter was automatically overridden / dropped.

When adding a console.warn to useUserSetting hook before calling updateConfigMap I notice that the filter is first stored correctly and was then removed when I reloaded the page:

When selecting Deployment it saves:

update devcosole.topology.filters
{
apps~v1~Deployment: true <===
expand-app-groups: true
expand-groups: true
helmGrouping: true
knativeServices: true
operatorGrouping: true
show-groups: true
show-labels: true
show-pod-count: true
}

After reload the page it sends two updates:

update devcosole.topology.filters
{
expand-app-groups: true
expand-groups: true
show-groups: true
show-labels: true
show-pod-count: true
}

update devcosole.topology.filters 
{
expand-app-groups: true
expand-groups: true
helmGrouping: true
knativeServices: true
operatorGrouping: true
show-groups: true
show-labels: true
show-pod-count: true
}

Any ideas? Did you need more info? We can make a call if you cant reproduce this.

andrewballantyne · 2020-11-27T15:57:36Z

frontend/packages/topology/src/filters/FilterProvider.tsx

+  boolean,
+  SetTopologyFilters,
+] => {
+  const [appliedFilters, setAppliedFilters, appliedFiltersLoaded] = useUserSettings(


@invincibleJai Should this not use the fallback wrapper?

Thanks for noticing this, used the wrapper useUserSettingsCompatibility.

andrewballantyne · 2020-11-27T16:00:17Z

frontend/packages/topology/src/filters/FilterProvider.tsx

+] => {
+  const [appliedFilters, setAppliedFilters, appliedFiltersLoaded] = useUserSettings(
+    TOPOLOGY_DISPLAY_FILTERS_CONFIG_MAP_KEY,
+    getDefaultAppliedFilters(),


@sahil143 I thought our goal here was to get rid of the reducer and obfuscate local storage... why are we using this as the defaults?

andrewballantyne · 2020-11-27T16:02:19Z

frontend/packages/topology/src/filters/FilterProvider.tsx

+    [setAppliedFilters],
+  );
+
+  return [filters, appliedFilters, appliedFilters && filtersLoaded, setTopologyFilters];


If appliedFilters are what we have stored in user settings... and filters are just appliedFilters with added defaults... why are we providing both back? Wouldn't we want just filters?

Because appliedFilters is also being used in component.

Hmmm... we probably should look to refactor that -- let's go with this for now since it's the status quo with the implementation. But it seems like an odd situation.

andrewballantyne · 2020-11-27T16:06:50Z

frontend/packages/topology/src/filters/TopologyFilterBar.tsx

@@ -34,36 +31,29 @@ import QuickSearchButton from './quick-search/QuickSearchButton';
 import { getNamespaceDashboardKialiLink } from '../utils/topology-utils';

 import './TopologyFilterBar.scss';
+import { FilterContext } from './FilterProvider';


andrewballantyne · 2020-11-27T16:07:39Z

frontend/packages/topology/src/filters/useDisplayFilters.ts

 import { useDeepCompareMemoize } from '@console/shared/src';
+import { FilterContext } from './FilterProvider';
+import { useContext } from 'react';


sahil143 · 2020-11-30T05:24:00Z

@sahil143 When having a Deployment and DeploymentConfig in my topology and filter for only Deployments and reload the page, the filter was automatically overridden / dropped.

I was able to reproduce this on master as well. I think we can log a bug to handle this.

sahil143 · 2020-11-30T06:08:41Z

@andrewballantyne @jerolimov Addressed all comments. Please review again.

invincibleJai · 2020-11-30T09:12:37Z

@sahil143 When having a Deployment and DeploymentConfig in my topology and filter for only Deployments and reload the page, the filter was automatically overridden / dropped.

I was able to reproduce this on master as well. I think we can log a bug to handle this.

Verified the changes all works as expected apart from what mentioned above (this is observed in master as well so can be taken as separate issue)

jerolimov · 2020-11-30T13:47:48Z

@sahil143 @invincibleJai Yeah works "similar" to master, it was saved successfully and then override when loading the page.

As discussed in slack this is by design because the list of available resource types changes per project.

/lgtm

jerolimov · 2020-11-30T13:53:16Z

frontend/packages/topology/src/filters/FilterProvider.tsx

+import * as React from 'react';
+import { useUserSettingsCompatibility } from '@console/shared';
+import { DEFAULT_TOPOLOGY_FILTERS } from './const';
+import { getAppliedFilters } from '../redux/action';


getAppliedFilters was only used in this Provider. Can we remove it from redux/action.ts since it looks like that we still use redux here?!

andrewballantyne · 2020-11-30T16:08:11Z

frontend/packages/topology/src/filters/FilterProvider.tsx

+    [setAppliedFilters],
+  );
+
+  return [filters, appliedFilters, appliedFilters && filtersLoaded, setTopologyFilters];


Hmmm... we probably should look to refactor that -- let's go with this for now since it's the status quo with the implementation. But it seems like an odd situation.

add typings remove comments address review comments refactor

jerolimov · 2020-11-30T16:29:45Z

/lgtm

invincibleJai · 2020-11-30T17:37:31Z

/test e2e-gcp-console

andrewballantyne · 2020-11-30T19:33:36Z

/retest

openshift-bot · 2020-11-30T20:23:02Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-11-30T20:36:03Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-11-30T21:02:05Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-11-30T21:15:03Z

/retest

Please review the full test history for this PR and help us cut down flakes.

christianvogt · 2020-11-30T22:38:54Z

/approve

openshift-ci-robot · 2020-11-30T22:39:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, christianvogt, jerolimov, sahil143

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/packages/topology/OWNERS~~ [andrewballantyne,christianvogt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2020-11-30T23:25:07Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-11-30T23:38:16Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-12-01T01:09:07Z

/retest

Please review the full test history for this PR and help us cut down flakes.

jerolimov · 2020-12-01T04:02:19Z

/retest

openshift-ci-robot requested review from debsmita1 and karthikjeeyar November 25, 2020 09:36

sahil143 force-pushed the odc-5038 branch 2 times, most recently from 8ed58c5 to 1a5c01d Compare November 25, 2020 09:43

invincibleJai reviewed Nov 25, 2020

View reviewed changes

invincibleJai reviewed Nov 26, 2020

View reviewed changes

sahil143 force-pushed the odc-5038 branch from 1a5c01d to d44f83d Compare November 26, 2020 14:19

openshift-ci-robot requested a review from jerolimov November 26, 2020 15:29

sahil143 force-pushed the odc-5038 branch from d44f83d to 3784172 Compare November 26, 2020 15:31

jerolimov reviewed Nov 27, 2020

View reviewed changes

andrewballantyne reviewed Nov 27, 2020

View reviewed changes

sahil143 force-pushed the odc-5038 branch from 3784172 to 35e3fe7 Compare November 30, 2020 05:53

openshift-ci-robot assigned jerolimov Nov 30, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 30, 2020

jerolimov reviewed Nov 30, 2020

View reviewed changes

andrewballantyne approved these changes Nov 30, 2020

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 30, 2020

convert Topology filters localStorage to userSettings

1e84569

add typings remove comments address review comments refactor

sahil143 force-pushed the odc-5038 branch from 35e3fe7 to 1e84569 Compare November 30, 2020 16:28

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 30, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 30, 2020

openshift-merge-robot merged commit f65ce5e into openshift:master Dec 1, 2020

spadgett added this to the v4.7 milestone Dec 9, 2020


		const TOPOLOGY_DISPLAY_FILTERS_CONFIG_MAP_KEY = `devcosole.topology.filters`;

		const getTopologyFilters = (appliedFilters) => {

	const useAppliedDisplayFilters = (): { [filiterKey: string]: boolean } => {
	const useAppliedDisplayFilters = (): { [filterKey: string]: boolean } => {

	const TOPOLOGY_DISPLAY_FILTERS_CONFIG_MAP_KEY = `devcosole.topology.filters`;
	const TOPOLOGY_DISPLAY_FILTERS_USERSETTINGS_KEY = `devconsole.topology.filters`;

convert Topology filters localStorage to userSettings #7326

convert Topology filters localStorage to userSettings #7326

Conversation

sahil143 commented Nov 25, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

invincibleJai commented Nov 26, 2020

Choose a reason for hiding this comment

jerolimov Nov 27, 2020 • edited

Choose a reason for hiding this comment

jerolimov commented Nov 27, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahil143 commented Nov 30, 2020

sahil143 commented Nov 30, 2020

invincibleJai commented Nov 30, 2020

jerolimov commented Nov 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerolimov commented Nov 30, 2020

invincibleJai commented Nov 30, 2020

andrewballantyne commented Nov 30, 2020

openshift-bot commented Nov 30, 2020

openshift-bot commented Nov 30, 2020

openshift-bot commented Nov 30, 2020

openshift-bot commented Nov 30, 2020

christianvogt commented Nov 30, 2020

openshift-ci-robot commented Nov 30, 2020

openshift-bot commented Nov 30, 2020

openshift-bot commented Nov 30, 2020

openshift-bot commented Dec 1, 2020

jerolimov commented Dec 1, 2020

sahil143 commented Nov 25, 2020 •

edited

jerolimov Nov 27, 2020 •

edited

jerolimov commented Nov 27, 2020 •

edited

jerolimov commented Nov 30, 2020 •

edited