adds fallback to localstorage for usersettings if 403 for configmap #7301

invincibleJai · 2020-11-23T15:49:57Z

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

Analysis / Root cause:
useUserSettings doesn't support fallback to localStorage

Solution Description:
adds support fallback for support for useUserSettings

Implementation can be tested with #7051

Screen shots / Gifs for design review:

NA

Browser conformance:

Chrome
Firefox
Safari
Edge

invincibleJai · 2020-11-24T15:33:46Z

/retest

invincibleJai · 2020-11-24T15:38:05Z

/retest

invincibleJai · 2020-11-24T15:42:02Z

/retest

invincibleJai · 2020-11-24T15:46:30Z

/test backend

invincibleJai · 2020-11-24T15:52:53Z

/ok-to-test

invincibleJai · 2020-11-24T16:26:11Z

/test frontend

christianvogt · 2020-11-24T21:57:35Z

frontend/packages/console-shared/src/utils/user-settings.ts

+  }
+};
+
+export const deseralizeData = <T>(data: T) => {


This should probably be:

Suggested change

export const deseralizeData = <T>(data: T) => {

export const deseralizeData = (data?: string | null) => {

christianvogt · 2020-11-24T21:59:04Z

frontend/packages/console-shared/src/utils/user-settings.ts

+export const createConfigMap = async (configMapData: K8sResourceKind) => {
+  try {
+    const configMapDataResp = await k8sCreate(ConfigMapModel, configMapData);
+    return Promise.resolve(configMapDataResp);
+  } catch (err) {
+    return Promise.reject(err);
+  }
+};


This is an async function. Therefore you only need to return the value or throw an error. There's no need to return a Promise

christianvogt · 2020-11-24T22:02:09Z

frontend/packages/console-shared/src/hooks/useUserSettingsLocalStorage.ts

+          lsConfigMapData?.[keyRef.current] &&
+          seralizeData(lsConfigMapData[keyRef.current]) !== seralizeData(lsData)
+        ) {
+          setLsData(lsConfigMapData[keyRef.current]);


Shouldn't this be deserialized?

in case of localStorage we are seralizing and deseralizing complete data in Configmap key so din't need to do it

const lsConfigMapData = deseralizeData(localStorage.getItem(storageConfigNameRef.current));

christianvogt · 2020-11-24T22:04:56Z

frontend/packages/console-shared/src/hooks/useUserSettingsLocalStorage.ts

+  );
+  const storageConfigNameRef = React.useRef(`${CONFIGMAP_LS_KEY}-${userUid}`);
+  const [lsData, setLsData] = React.useState(() => {
+    const valueInLocalStorage = deseralizeData(localStorage.getItem(storageConfigNameRef.current));


You could use localStorage.hasItem to check whether we fallback to the default or not

hasItem i din't see in API for localStorage so made a check against null

localStorage.getItem(storageConfigNameRef.current) !== null

christianvogt · 2020-11-24T22:10:41Z

frontend/packages/console-shared/src/hooks/useUserSettingsLocalStorage.ts

+      ? valueInLocalStorage[keyRef.current]
+      : defaultValueRef.current;
+  });
+  const lsDataRef = React.useRef<T>(lsData);


This never gets updated but is passed along as the previous value each time.

christianvogt · 2020-11-24T22:15:36Z

frontend/packages/console-shared/src/hooks/useUserSettings.ts

-          setLoaded(true);
-        }
+        })
+          .then(() => setFallbackLocalStorage(false))


fallback already starts as false

christianvogt · 2020-11-24T22:18:50Z

frontend/packages/console-shared/src/hooks/useUserSettings.ts

+            } else {
+              setFallbackLocalStorage(false);
+              setSettings(deseralizeData(defaultValueRef.current));
+              setLoaded(true);
+            }


How will this work at all if we have any error creating the configmap or if the new backend returns an error?
I suppose that it should never error out if the service is working correctly.
Perhaps it shouldn't go to the loaded state if we have an error.
Thoughts?

yeah ideally it should never hit this block , have added it if in considering if it errors our and not with forbidden(403) then pass loaded with default value

christianvogt · 2020-11-24T22:19:27Z

frontend/packages/console-shared/src/hooks/useUserSettings.ts

-          setLoaded(true);
-        }
+        })
+          .then(() => setFallbackLocalStorage(false))


we should be using try / catch if it's an async function

sahil143

Tested this functionality with PR #7051 . Works as expected. Looks good to me.

jerolimov

Tested is with some local changes to 'fake the 403' case. Works fine. 👍

/lgtm

jerolimov · 2020-11-25T13:24:04Z

frontend/packages/console-shared/src/hooks/useUserSettings.ts

+        } catch (err) {
+          if (err?.response?.status === 403) {
+            setFallbackLocalStorage(true);
+          } else {
+            setSettings(defaultValueRef.current);
+            setLoaded(true);
+          }


All other cases of an error are just ignored a.t.m? Can you add here a console.warn / error as well?

we are logging for all in utils like updateConfigmap, createProject, get Project have added there for createConfigMap as well.

…reation

invincibleJai · 2020-11-25T16:04:36Z

/retest

jerolimov · 2020-11-25T18:39:22Z

/lgtm
/retest

jerolimov · 2020-11-25T18:55:52Z

/retest

christianvogt · 2020-11-25T21:50:27Z

/approve
/retest

openshift-ci-robot · 2020-11-25T21:50:48Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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/console-shared/OWNERS~~ [christianvogt]

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

jerolimov · 2020-11-26T00:49:14Z

/retest

openshift-ci-robot requested review from kyoto and zherman0 November 23, 2020 15:50

openshift-ci-robot added the component/shared Related to console-shared label Nov 23, 2020

invincibleJai force-pushed the fallback-localstorage branch 3 times, most recently from d833c83 to cbbd660 Compare November 24, 2020 15:22

openshift-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Nov 24, 2020

invincibleJai closed this Nov 24, 2020

invincibleJai reopened this Nov 24, 2020

invincibleJai force-pushed the fallback-localstorage branch 2 times, most recently from 6faacb3 to 5ca6332 Compare November 24, 2020 16:31

christianvogt reviewed Nov 24, 2020

View reviewed changes

invincibleJai force-pushed the fallback-localstorage branch 3 times, most recently from 1b507df to 35075a2 Compare November 25, 2020 07:40

sahil143 reviewed Nov 25, 2020

View reviewed changes

invincibleJai mentioned this pull request Nov 25, 2020

Convert Pinned Resources local storage to user settings #7291

Merged

4 tasks

jerolimov approved these changes Nov 25, 2020

View reviewed changes

openshift-ci-robot assigned jerolimov Nov 25, 2020

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

jerolimov reviewed Nov 25, 2020

View reviewed changes

adds fallback to localstorage for usersettings if 403 for configmap c…

6eb4b7a

…reation

invincibleJai force-pushed the fallback-localstorage branch from 35075a2 to 6eb4b7a Compare November 25, 2020 14:35

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

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

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

openshift-merge-robot merged commit 867b5d8 into openshift:master Nov 26, 2020

spadgett added this to the v4.7 milestone Nov 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds fallback to localstorage for usersettings if 403 for configmap #7301

adds fallback to localstorage for usersettings if 403 for configmap #7301

invincibleJai commented Nov 23, 2020 •

edited

invincibleJai commented Nov 24, 2020

invincibleJai commented Nov 24, 2020

invincibleJai commented Nov 24, 2020

invincibleJai commented Nov 24, 2020

invincibleJai commented Nov 24, 2020

invincibleJai commented Nov 24, 2020

christianvogt Nov 24, 2020

christianvogt Nov 24, 2020

christianvogt Nov 24, 2020

invincibleJai Nov 25, 2020

christianvogt Nov 24, 2020

invincibleJai Nov 25, 2020

christianvogt Nov 24, 2020

christianvogt Nov 24, 2020

christianvogt Nov 24, 2020

invincibleJai Nov 25, 2020

christianvogt Nov 24, 2020

sahil143 left a comment

jerolimov left a comment

jerolimov Nov 25, 2020

invincibleJai Nov 25, 2020 •

edited

invincibleJai commented Nov 25, 2020

jerolimov commented Nov 25, 2020

jerolimov commented Nov 25, 2020

christianvogt commented Nov 25, 2020

openshift-ci-robot commented Nov 25, 2020

jerolimov commented Nov 26, 2020

	export const deseralizeData = <T>(data: T) => {
	export const deseralizeData = (data?: string \| null) => {

adds fallback to localstorage for usersettings if 403 for configmap #7301

adds fallback to localstorage for usersettings if 403 for configmap #7301

Conversation

invincibleJai commented Nov 23, 2020 • edited

invincibleJai commented Nov 24, 2020

invincibleJai commented Nov 24, 2020

invincibleJai commented Nov 24, 2020

invincibleJai commented Nov 24, 2020

invincibleJai commented Nov 24, 2020

invincibleJai commented Nov 24, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahil143 left a comment

Choose a reason for hiding this comment

jerolimov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

invincibleJai Nov 25, 2020 • edited

Choose a reason for hiding this comment

invincibleJai commented Nov 25, 2020

jerolimov commented Nov 25, 2020

jerolimov commented Nov 25, 2020

christianvogt commented Nov 25, 2020

openshift-ci-robot commented Nov 25, 2020

jerolimov commented Nov 26, 2020

invincibleJai commented Nov 23, 2020 •

edited

invincibleJai Nov 25, 2020 •

edited