Save dropdown bookmarks and favoriteKey in user settings #7390

jerolimov · 2020-12-02T13:01:20Z

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

Analysis / Root cause:
As a user, I want sync my bookmarks for namespaces, applications and other dropdowns between browsers.

Solution Description:
Update Dropdown component to use useUserSettings hook and save booksmarks and favoriteKey in user settings.

Screen shots / Gifs for design review:
From user perspective nothing has changed. This video shows the migration and update of the configmap:

Unit test coverage report:
Untouched

Test setup:
Open developer perspective and bookmark / unbookmark / favorite namespaces and applications.

Browser conformance:

Chrome
Firefox
Safari
Edge

jerolimov · 2020-12-02T13:57:26Z

/cc @invincibleJai
/assign @christianvogt

sahil143 · 2020-12-02T13:58:48Z

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

  React.useEffect(() => {
-    if (cfData && cfLoaded && settings !== undefined) {
+    if (keyRef.current && cfLoaded && cfData && settings !== undefined) {
      const value = seralizeData(settings);
-      if (value !== cfData.data?.[key]) {
+      if (value !== cfData.data?.[keyRef.current]) {
        updateConfigMap(cfData, keyRef.current, value);
      }
    }


This implementation will change with sync PR.

Reverted this to don't get into a merge conflict. We need to check after both are merged it there are some settings are stored without key.

sahil143 · 2020-12-02T13:59:23Z

frontend/packages/console-shared/src/components/dynamic-form/fields.tsx

@@ -337,7 +337,7 @@ export const DropdownField: React.FC<FieldProps> = ({
      key={idSchema.$id}
      title={t('console-shared~Select {{title}}', { title: title || schema?.title || name })}
      selectedKey={formData}
-      items={items ?? {}}
+      items={(items as object) ?? {}}


Why do you need type cast here?

Because items comes from const { items, title } = getUiOptions(uiSchema); which returns items as string | string[] | array | object .... In this case this must be an object, this is was Dropdown requested before.

Moved this cast to the line where getUiOptions is assigned to items

sahil143 · 2020-12-02T15:45:12Z

frontend/public/components/utils/dropdown.jsx

+  const [favoriteKey, setFavoriteKey] = useUserSettingsCompatibility(
+    storageKey,
+    storageKey,
+    null,
+    // TODO: enable sync when sync PR is merged


@jerolimov how would the returned state work here. If key is undefined and we don't want to initialize it in user settings.

I'm guessing similar to useState?

christianvogt

Unable to clear the favorite item. After page refresh, it comes back. The config map does not update.

christianvogt · 2020-12-02T19:52:24Z

frontend/public/components/utils/dropdown.jsx

+  const [favoriteKey, setFavoriteKey] = useUserSettingsCompatibility(
+    storageKey,
+    storageKey,


The format of the user-settings keys should be namespace.key. eg console.foobar

I'd prefer to see a temporary prop in place for the old key and new one that adheres to the new format.
The current keys I see created are:

dropdown-storage-namespaces dropdown-storage-namespaces-bookmarks

Okay, this (old) Dropdown component has now a userSettingsPrefix and storageKey prop, so that the users can set both attributes for the useUserSettingsCompatibility. I used the prefix name because the component automatically appends .favorite and .bookmarks.

christianvogt · 2020-12-02T19:55:11Z

frontend/public/components/utils/dropdown.jsx

+  const [favoriteKey, setFavoriteKey] = useUserSettingsCompatibility(
+    storageKey,
+    storageKey,
+    null,


Prefer undefined because this is what's being saved in the user settings:

dropdown-storage-namespaces: 'null'

Removed the default so its now undefined.

christianvogt · 2020-12-02T19:55:46Z

frontend/public/components/utils/dropdown.jsx

+  const [bookmarks, setBookmarks] = useUserSettingsCompatibility(
+    bookmarkStorageKey,
+    bookmarkStorageKey,
+    {},


Can this be undefined to start?

Yeah works fine. Removed the default so its now undefined.

jerolimov · 2020-12-03T15:38:51Z

Unable to clear the favorite item. After page refresh, it comes back. The config map does not update.

Fixed this. This sets now null instead of undefined to clear the .favorite entry.

Undefined is currently not supported, let add support for undefined after FF?

@sahil143 @christianvogt Can you take a new look? Thanks

jerolimov · 2020-12-03T15:38:57Z

Need to fix one test, working on this already.

invincibleJai · 2020-12-03T16:38:57Z

Verified the changes , works as expected. Need some work tests looks like

jerolimov · 2020-12-03T17:10:53Z

/retest

jerolimov · 2020-12-03T17:25:39Z

/retest

frontend/packages/console-shared/src/constants/common.ts

jerolimov · 2020-12-03T21:15:26Z

/retest

christianvogt · 2020-12-04T02:53:48Z

/approve

nemesis09 · 2020-12-04T04:10:19Z

/retest

debsmita1 · 2020-12-04T04:54:51Z

tested this locally, works fine
/lgtm

jerolimov · 2020-12-04T07:40:05Z

/retest

jerolimov · 2020-12-04T12:19:52Z

/retest

openshift-bot · 2020-12-04T13:00:28Z

/retest

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

jerolimov · 2020-12-04T14:06:57Z

/retest

jerolimov · 2020-12-04T16:36:31Z

Integration tests found an issue. Working on that.
/hold

jerolimov · 2020-12-04T17:46:51Z

/unhold
The previous version of this PR has a bug that the simple Dropdown in Admin > Workloads > Pods or Developer > Search was empty. This is fixed now. I restored the line ... in dropdown.jsx Dropdown_ constructor.

    this.state.items = props.items;

The dynamic cases of projects and apps works fine because the items was also set when the props are updated.

Here are some working examples:

Simple:

Search:

Project and apps:

jerolimov · 2020-12-04T17:47:03Z

/retest

jerolimov · 2020-12-04T17:50:15Z

/retest

jerolimov · 2020-12-04T17:57:39Z

/retest

andrewballantyne · 2020-12-04T18:08:37Z

frontend/public/components/utils/dropdown.jsx

@@ -562,13 +562,26 @@ export const Dropdown = (props) => {
    [setBookmarks],
  );

+  // FIXME: Remove this after latest namespace wasn't fetched from localStorage anymore.


This is a temporary solution? It's a FIXME but the text does not specify when we would fix it. Could you explain more of what the scenario is?

I guess once my PR #7411 goes in.

andrewballantyne · 2020-12-04T18:23:40Z

/lgtm
/hold

Putting a hold on this since it has a dependency

openshift-ci-robot · 2020-12-04T18:23:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, christianvogt, debsmita1, 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/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-12-04T19:05:09Z

/retest

andrewballantyne · 2020-12-04T19:13:15Z

/hold cancel

Removing hold based on discussion with @jerolimov -- we'll log a ticket to address removing the FIXME

andrewballantyne · 2020-12-04T23:51:45Z

/retest

Operator degraded issue -- don't think it's related, this task passed already.

openshift-ci-robot added the component/core Related to console core functionality label Dec 2, 2020

openshift-ci-robot requested review from jhadvig and zherman0 December 2, 2020 13:01

openshift-ci-robot added the component/shared Related to console-shared label Dec 2, 2020

jerolimov force-pushed the odc-5111-dropdown branch 3 times, most recently from 8583c5b to 21f9a9a Compare December 2, 2020 13:46

openshift-ci-robot assigned christianvogt Dec 2, 2020

openshift-ci-robot requested a review from invincibleJai December 2, 2020 13:57

sahil143 suggested changes Dec 2, 2020

View reviewed changes

christianvogt reviewed Dec 2, 2020

View reviewed changes

jerolimov force-pushed the odc-5111-dropdown branch 2 times, most recently from 953c576 to 1908a22 Compare December 3, 2020 14:57

jerolimov requested review from sahil143 and christianvogt December 3, 2020 15:33

jerolimov force-pushed the odc-5111-dropdown branch from 1908a22 to a2ec078 Compare December 3, 2020 16:45

debsmita1 reviewed Dec 3, 2020

View reviewed changes

frontend/packages/console-shared/src/constants/common.ts Outdated Show resolved Hide resolved

frontend/packages/console-shared/src/constants/common.ts Outdated Show resolved Hide resolved

jerolimov force-pushed the odc-5111-dropdown branch from a2ec078 to d9b7828 Compare December 3, 2020 19:36

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

openshift-ci-robot assigned debsmita1 Dec 4, 2020

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

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 4, 2020

jerolimov added 3 commits December 4, 2020 18:35

Save dropdown bookmarks and favoriteKey in user settings

f08efdb

Add missing PropTypes, fix invalid prop to fix build

0b11f56

Add workaround to support favorite namespace when loading the app

b05cbe0

jerolimov force-pushed the odc-5111-dropdown branch from c795994 to b05cbe0 Compare December 4, 2020 17:35

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

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 4, 2020

andrewballantyne reviewed Dec 4, 2020

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 4, 2020

openshift-ci-robot assigned andrewballantyne Dec 4, 2020

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

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 4, 2020

openshift-merge-robot merged commit caee4d0 into openshift:master Dec 5, 2020

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

Save dropdown bookmarks and favoriteKey in user settings #7390

Save dropdown bookmarks and favoriteKey in user settings #7390

Conversation

jerolimov commented Dec 2, 2020 • edited

jerolimov commented Dec 2, 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

sahil143 Dec 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianvogt left a 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

jerolimov commented Dec 3, 2020

jerolimov commented Dec 3, 2020 • edited

invincibleJai commented Dec 3, 2020

jerolimov commented Dec 3, 2020

jerolimov commented Dec 3, 2020

jerolimov commented Dec 3, 2020

christianvogt commented Dec 4, 2020

nemesis09 commented Dec 4, 2020

debsmita1 commented Dec 4, 2020

jerolimov commented Dec 4, 2020

jerolimov commented Dec 4, 2020

openshift-bot commented Dec 4, 2020

jerolimov commented Dec 4, 2020

jerolimov commented Dec 4, 2020

jerolimov commented Dec 4, 2020

jerolimov commented Dec 4, 2020

jerolimov commented Dec 4, 2020

jerolimov commented Dec 4, 2020

Choose a reason for hiding this comment

sahil143 Dec 4, 2020 • edited

Choose a reason for hiding this comment

andrewballantyne commented Dec 4, 2020

openshift-ci-robot commented Dec 4, 2020

jerolimov commented Dec 4, 2020

andrewballantyne commented Dec 4, 2020

andrewballantyne commented Dec 4, 2020

jerolimov commented Dec 2, 2020 •

edited

sahil143 Dec 2, 2020 •

edited

jerolimov commented Dec 3, 2020 •

edited

sahil143 Dec 4, 2020 •

edited