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

Validate that namespace from URL exists before rendering any content #126

Merged

Conversation

mturley
Copy link
Contributor

@mturley mturley commented Jul 27, 2022

This is a followup to #118 (finishes fixing https://issues.redhat.com/browse/MTRHO-123). In that PR we enforced that the namespace should never be undefined, but we didn't handle the case where the user somehow ended up with a defined namespace in the URL that doesn't actually exist.

This PR replaces the useNamespaceContext hook with a useValidatedNamespace hook that actively tries to fetch the namespace specified in the URL, and it returns status and validity in addition to the namespace itself. Incidentally, it now gets the namespace directly from useRouteMatch instead of from context, which makes the entire NamespaceContext unnecessary, so this PR also removes NamespaceContext.

The AppImportsPage and the ImportWizard now show a loading state when the namespace is being validated, and if the namespace is found to be invalid the page automatically redirects to the "all-namespaces" URL (via the new useRedirectOnInvalidNamespaceEffect hook). This redirect is used so that we don't leave the invalid namespace in the URL at all, which would cause the whole console UI to try and use that namespace if you navigate away.

These "all-namespaces" URLs currently render the "No project selected" message and direct the user to go select a namespace. (If they reach this view on the Imports page, clicking "select a project" takes them to the Projects page, and if they reach it on the wizard page, clicking that takes them to the Add page. This UX will be improved when we address #106).

Screen Shot 2022-07-27 at 1 36 03 PM

Note that useValidatedNamespace is called all over the place, so the useHostNamespaceQuery within it will be mounted several times per page. This is okay because react-query will avoid unnecessary refetches and share the cached result. As a bonus though, the namespace will be re-validated whenever the page regains focus (as per react-query default behavior) so if the user goes and deletes their namespace and then returns to our page, they'll get kicked out.

This PR also factors out new components for LoadingEmptyState and NoProjectEmptyState since we're now duplicating this content on both pages.

With this change, it should now be impossible to end up in a state where our pages are rendered with a missing or invalid namespace, either through user interaction or manually typing an invalid URL. You can test it by:

  • Trying to go to /app-imports/ns/fakenamespace or /app-imports/new/ns/fakenamespace and seeing that you get redirected to /.../all-namespaces.
  • Going to the Add page, using the project switcher bar to select "All projects", then going to the Application Imports page and verifying that you still get redirected to /app-imports/all-namespaces.

@mturley mturley requested a review from a team July 27, 2022 17:24
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 27, 2022
@mturley mturley force-pushed the MTRHO-123-check-namespace branch 2 times, most recently from bb7b638 to 7b6460d Compare July 27, 2022 17:34
Copy link
Contributor

@ibolton336 ibolton336 left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 27, 2022
@openshift-ci
Copy link

openshift-ci bot commented Jul 27, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ibolton336, mturley

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:

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

@openshift-merge-robot openshift-merge-robot merged commit 282f61e into migtools:main Jul 27, 2022
@mturley mturley deleted the MTRHO-123-check-namespace branch July 27, 2022 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants