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

fix: Roles should be fetched immediately when logging in #5229

Merged
merged 1 commit into from
Oct 11, 2022

Conversation

ClaireNeveu
Copy link
Contributor

@ClaireNeveu ClaireNeveu commented Oct 11, 2022

Description

This fixes an issue noticed by @ioga where it took a bit to populate roles when logging in. This happened because we immediately fetch roles when the page loads then poll for them on an interval; the initial call fails because the user is not yet logged in. There are broader problems with this method of data fetching but I'm not addressing those in this PR.

Specifically for roles I added a fetch when a user logs in.

Test Plan

  1. Run devcluster against determined-ee with rba enabled
  2. Without this change, log in and navigate to http://localhost:3000/settings/user-management
  3. log out then log back in, note the page is blank but populates after a few seconds
  4. Apply this change
  5. Log out then log back in, note the page immediately lists users

Commentary

It seems to me that we might need to all of these fetches to the login action. Were these working correctly before rbac? Did the API maybe not check if you were logged in prior to rbac when performing certain GETs?

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.
  • If modifying /webui/react/src/shared/ verify make -C webui/react test-shared passes.

Ticket

No ticket

@ClaireNeveu ClaireNeveu self-assigned this Oct 11, 2022
@cla-bot cla-bot bot added the cla-signed label Oct 11, 2022
@netlify
Copy link

netlify bot commented Oct 11, 2022

Deploy Preview for storybook-det ready!

Name Link
🔨 Latest commit 95ef6da
🔍 Latest deploy log https://app.netlify.com/sites/storybook-det/deploys/6345a901cda4aa00085ed34d
😎 Deploy Preview https://deploy-preview-5229--storybook-det.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Oct 11, 2022

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit 95ef6da
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/6345a902b3118200080939f7
😎 Deploy Preview https://deploy-preview-5229--determined-ui.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@ClaireNeveu
Copy link
Contributor Author

ClaireNeveu commented Oct 11, 2022

I checked and the other calls we make in Navigation do fail when not logged in regardless of rbac status. It's just that the listing component always refetches the data when it mounts so it's never been an issue. Since roles are never fetched as part of a component they didn't kick in until the polling happened.

Copy link
Contributor

@gt2345 gt2345 left a comment

Choose a reason for hiding this comment

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

I think fetching roles right after login works great

@ClaireNeveu ClaireNeveu merged commit 7096deb into master Oct 11, 2022
@keita-determined keita-determined deleted the claire/fetch-roles-upon-login branch October 11, 2022 20:03
@dannysauer dannysauer added this to the 0.19.6 milestone Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants