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: Fix always loading RBACProvider on empty appName #1900

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

gkarat
Copy link
Contributor

@gkarat gkarat commented Aug 28, 2023

This fixes the RBACProvider component: now, with the empty appName property, it fetches permissions and update the state properly. This way, it is not stuck in the always loading state, and all the permissions from all scopes are fetched (empty appName == we request all of the user's permissions).

Copy link
Collaborator

@karelhala karelhala left a comment

Choose a reason for hiding this comment

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

I have just one minor concern about firing multiple times the request to fetch permissions.

@@ -38,9 +38,7 @@ export const RBACProvider: React.FunctionComponent<RBACProviderProps> = ({ appNa
};

useEffect(() => {
if (appName) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would trigger the call to fetch permissions twice. It could be better to allow null value to be passed as appName and if such value request empty application.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this makes sense. Added null support

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would flip it arround, if null or string fire a fetchPermissions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@karelhala, implemented

@gkarat gkarat force-pushed the fix-rbac-provider-loading branch from ec4f350 to e8a42f0 Compare August 29, 2023 11:14
This fixes the RBACProvider component: now, with the empty appName
property, it fetches permissions and update the state properly.
@gkarat gkarat force-pushed the fix-rbac-provider-loading branch from e8a42f0 to 80271ef Compare August 29, 2023 11:34
@gkarat gkarat requested a review from karelhala August 29, 2023 13:50
@karelhala karelhala added the release Once merged it will trigger bugfix release label Aug 29, 2023
@karelhala karelhala merged commit dca755e into master Aug 29, 2023
@nacho-bot
Copy link
Collaborator

                      :soon::shipit::octocat:
     :bug:Shipit Squirrel has this release bugfix surrounded, be ready for a new version:beetle:

@nacho-bot
Copy link
Collaborator

     🌱 🌸 🌷 🌻 🌟 New version of package has been released 🌟 🌻 🌷 🌸 🌱

                The release is available on:

         :package:@redhat-cloud-services/frontend-components/v/4.0.2📦

             :boom:This feature is brought to you by probot🚀

@nacho-bot nacho-bot added the released Pr has been released label Aug 29, 2023
@Hyperkid123 Hyperkid123 deleted the fix-rbac-provider-loading branch August 30, 2023 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working release Once merged it will trigger bugfix release released Pr has been released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants