Bug 1996535: Improve detect namespace hook and fix redirect loop and e2e tests #9844

jerolimov · 2021-08-19T22:29:43Z

Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1996535
https://issues.redhat.com/browse/ODC-6267

Analysis / Root cause:
This is an follow up for PR 9386 and this work is based on @nemesis09 work in PR 9846

The new user preferences introduce a change in the hook which detects the current / active namespace which can result in an endless loop after the user creates a new project.
Cluster settings page crashs in some edge cases when a project is created while the cluster settings page is open. (I could reproduce this always with my crc, but not on a cluster. It depends on the installed operators etc.) The new code is introduced in 4.9 so that we don't need a backport for this.

Solution Description:

Cleanup the namespace hook. Ensure that it doesn't result in an rerendering or redirect loop.
- if an URL contains a namespace (or the info all namespaces) known at the beginning
  - saved as current namespace
  - the url-based namespace is returned (with loaded: false)
  - set to the redux state
- if the URL doesn't contain a namespace (or the info all namespaces) the hooks
  - doesn't have a known current namespace and returns loaded: false
  - waits for the preferred and last namespace user settings, and also for flag if project or namespaces are used
  - when all three infos are loaded (remember: the url doesn't contain any namespace info)
    - the preferred or last namespace is set as current namespace
    - the redux state is set and (in setActiveNamespace) the user will be redirected to this namespace
- whenever the url changes, the hook checks if this new url contains a namespace
  - if that url contains a namespace the current namespace (context value) stores this namespace
  - the redux state is set (which might redirect to this namespace again, but this should not happen since the url contains that namespace already)
  - LAST NAMESPACE is not updated automatically at the moment!!
- and finally: setNamespace (available as context value)
  - updates the current namespace (context value)
  - the last namespace!! which is only used when there is no preference and the user returns later to an url without namespace.
  - the redux state is set (which will also automatically redirect the user to a new URL)
Fix the crash in frontend/public/module/k8s/cluster-settings.ts with an additional ?. check.

Screen shots / Gifs for design review:
Recording of multiple namespace tests (4 mb)

namespace-tests.mp4

Unit test coverage report:
Tests changed and passes.

Test setup:
Test everything which is aligned to the following features and mix them:

Switch namespaces
Create a namespace
Set preferred namespace
Return to an URL with and without namespace in it

Browser conformance:

Chrome
Firefox
Safari
Edge

jerolimov · 2021-08-21T21:54:01Z

/uncc abhinandan13jan vojtechszocs

jerolimov · 2021-08-22T10:43:40Z

/cc @nemesis09 @christianvogt

openshift-ci · 2021-08-23T07:22:41Z

@jerolimov: An error was encountered querying GitHub for users with public email (gamore@redhat.com) for bug 1996535 on the Bugzilla server at https://bugzilla.redhat.com. No known errors were detected, please see the full error message for details.

Full error message.


non-200 OK status code: 403 Forbidden body: "{\n  \"documentation_url\": \"https://docs.github.com/en/free-pro-team@latest/rest/overview/resources-in-the-rest-api#secondary-rate-limits\",\n  \"message\": \"You have exceeded a secondary rate limit. Please wait a few minutes before you try again.\"\n}\n"

Please contact an administrator to resolve this issue, then request a bug refresh with /bugzilla refresh.

In response to this:

Bug 1996535: Improve detect namespace hook and fix redirect loop and e2e tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

jerolimov · 2021-08-23T07:23:39Z

/bugzilla refresh

openshift-ci · 2021-08-23T07:24:46Z

@jerolimov: This pull request references Bugzilla bug 1996535, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.0) matches configured target release for branch (4.9.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (gamore@redhat.com), skipping review request.

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2021-08-23T07:27:42Z

@jerolimov: This pull request references Bugzilla bug 1996535, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.0) matches configured target release for branch (4.9.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (gamore@redhat.com), skipping review request.

In response to this:

Bug 1996535: Improve detect namespace hook and fix redirect loop and e2e tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

invincibleJai · 2021-08-23T11:26:26Z

frontend/packages/console-app/src/components/detect-namespace/namespace.ts

@@ -64,28 +47,31 @@ export const useValuesForNamespaceContext = () => {
    }
    // Only run this hook after all resources have loaded.
    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [resourcesLoaded, useProjects]);
+  }, [resourcesLoaded]);


won't we need useProjects as well in the dependency list of effect?

As long as useProjects is pending the resourcesLoaded is false. It switches to true if everything is loaded (useProjects and 2 user settings). So the useEffect must not be called earlier.

The condition in the first line would ignore this extra call, but I think useProjects is not required here.

got it 👍

invincibleJai · 2021-08-23T11:38:11Z

frontend/packages/console-app/src/components/detect-namespace/namespace.ts

-  const urlNamespace = getNamespace(pathname);
-  const [lastNamespace, setLastNamespace, lastNamespaceLoaded] = useLastNamespace();
+  const urlNamespace = useUrlNamespace();
+  const [currentNamespace, setCurrentNamespace] = React.useState<string>(urlNamespace);


@jerolimov sorry getting confused here between what is activeNamespce vs currentNamespace, is it just a rename?

That's right. Changed back to activeNamespace. This makes also more sense with the useActiveNamespace hook to consume this data. Changed the comments also that we set active namespace which means updating this internal state and the redux state.

invincibleJai · 2021-08-23T13:17:42Z

/lgtm

verified the changes works as expected

openshift-ci · 2021-08-23T13:18:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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/OWNERS~~ [invincibleJai,jerolimov]

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

openshift-ci · 2021-08-23T14:52:17Z

@jerolimov: All pull requests linked via external trackers have merged:

openshift/console#9844

Bugzilla bug 1996535 has been moved to the MODIFIED state.

In response to this:

Bug 1996535: Improve detect namespace hook and fix redirect loop and e2e tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 19, 2021

openshift-ci bot requested review from abhinandan13jan and vojtechszocs August 19, 2021 22:30

jerolimov force-pushed the user-preferences-extension-test branch from 6fb3135 to 6f396b1 Compare August 21, 2021 18:23

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 21, 2021

jerolimov force-pushed the user-preferences-extension-test branch 2 times, most recently from 9f5885b to 43fbe96 Compare August 21, 2021 21:15

openshift-ci bot removed request for vojtechszocs and abhinandan13jan August 21, 2021 21:54

jerolimov force-pushed the user-preferences-extension-test branch 2 times, most recently from 1aaf0d6 to cd3ea84 Compare August 22, 2021 10:41

jerolimov changed the title ~~[WIP] User preferences extension test~~ Improve detect namespace hook and fix redirect loop and e2e tests Aug 22, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 22, 2021

openshift-ci bot requested review from christianvogt and nemesis09 August 22, 2021 10:43

jerolimov changed the title ~~Improve detect namespace hook and fix redirect loop and e2e tests~~ Bug 1996535: Improve detect namespace hook and fix redirect loop and e2e tests Aug 23, 2021

openshift-ci bot added the bugzilla/severity-urgent Referenced Bugzilla bug's severity is urgent for the branch this PR is targeting. label Aug 23, 2021

openshift-ci bot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Aug 23, 2021

invincibleJai reviewed Aug 23, 2021

View reviewed changes

jerolimov force-pushed the user-preferences-extension-test branch from cd3ea84 to 31fbda3 Compare August 23, 2021 12:11

Improve detect namespace hook and fix redirect loop and e2e tests

2816eb1

jerolimov force-pushed the user-preferences-extension-test branch from 31fbda3 to 2816eb1 Compare August 23, 2021 12:18

jerolimov requested a review from invincibleJai August 23, 2021 12:31

openshift-ci bot assigned invincibleJai Aug 23, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 23, 2021

jerolimov mentioned this pull request Aug 23, 2021

Fix user preferences extension test cleanup #9846

Closed

openshift-merge-robot merged commit 7e9f3f4 into openshift:master Aug 23, 2021

spadgett added this to the v4.9 milestone Aug 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1996535: Improve detect namespace hook and fix redirect loop and e2e tests #9844

Bug 1996535: Improve detect namespace hook and fix redirect loop and e2e tests #9844

jerolimov commented Aug 19, 2021 •

edited

jerolimov commented Aug 21, 2021

jerolimov commented Aug 22, 2021

openshift-ci bot commented Aug 23, 2021

jerolimov commented Aug 23, 2021

openshift-ci bot commented Aug 23, 2021

openshift-ci bot commented Aug 23, 2021

invincibleJai Aug 23, 2021 •

edited

jerolimov Aug 23, 2021

invincibleJai Aug 23, 2021

invincibleJai Aug 23, 2021 •

edited

jerolimov Aug 23, 2021 •

edited

invincibleJai commented Aug 23, 2021

openshift-ci bot commented Aug 23, 2021

openshift-ci bot commented Aug 23, 2021

Bug 1996535: Improve detect namespace hook and fix redirect loop and e2e tests #9844

Bug 1996535: Improve detect namespace hook and fix redirect loop and e2e tests #9844

Conversation

jerolimov commented Aug 19, 2021 • edited

jerolimov commented Aug 21, 2021

jerolimov commented Aug 22, 2021

openshift-ci bot commented Aug 23, 2021

jerolimov commented Aug 23, 2021

openshift-ci bot commented Aug 23, 2021

openshift-ci bot commented Aug 23, 2021

invincibleJai Aug 23, 2021 • edited

Choose a reason for hiding this comment

jerolimov Aug 23, 2021

Choose a reason for hiding this comment

invincibleJai Aug 23, 2021

Choose a reason for hiding this comment

invincibleJai Aug 23, 2021 • edited

Choose a reason for hiding this comment

jerolimov Aug 23, 2021 • edited

Choose a reason for hiding this comment

invincibleJai commented Aug 23, 2021

openshift-ci bot commented Aug 23, 2021

openshift-ci bot commented Aug 23, 2021

jerolimov commented Aug 19, 2021 •

edited

invincibleJai Aug 23, 2021 •

edited

invincibleJai Aug 23, 2021 •

edited

jerolimov Aug 23, 2021 •

edited